Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat: Multi-Technology Campaign Events #1040

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

abdullai-t
Copy link
Contributor

@abdullai-t abdullai-t commented Jun 3, 2024

Summary / Highlights

This pull request introduces a feature that allows campaign events to be associated with multiple technologies.

Details (Give details about what this PR accomplishes, include any screenshots etc)

Testing Steps (Provide details on how your changes can be tested)

Requirements (place an x in each [ ])

  • I've read and understood the Contributing Guidelines and have done my best effort to follow them.
  • I've tested my changes manually.
  • I've added unit tests to my PR.
Transparency (Project board)
  • I've given my PR a meaningful title.
  • I linked this PR to the relevant issue.
  • I moved the linked issue from the "Sprint backlog" to "In progress" when I started this.
  • I moved the linked issue to "QA Verification" now that it is ready to merge.

This commit simplifies and optimizes the querying logic in campaign.py. The main changes involve minimizing the number of separate queries within loops and instead using single queries with the "__in" lookup. This is more performant and reduces database load. The code is also slightly more readable with this structure.
Updated the .gitignore file to ignore JetBrains IDE files .idea/. In the store/campaign.py, refactored add_campaign_technology_event method to handle multiple campaign technologies instead of a single one. Modified error messages and variable names to reflect changes.
This commit involves refactoring of the implementation of add_campaign_technology_event method in the campaign.py file. The multiple campaign ids handling has been removed and replaced with handling for a single campaign id. This promises for a more direct, and less error-prone system.
The commit introduces iterative processing for assigning multiple technologies to a campaign. Instead of dealing with a single technology at a time, the updated method accepts and processes a list of technologies, allowing numerous assignments in one operation, and subsequently returns a list of successful assignments. This offers a more efficient way to handle campaign technologies.
Updated the validation rules in the "campaign" API handler to require "campaign_technology_ids" and "event_ids". Removed a redundant comment in the "campaign" data store file. The changes aim to improve data integrity and code readability.
@abdullai-t abdullai-t changed the title Feat: Feat: Multi-Technology Campaign Events Jun 3, 2024
The payload in the test "test_add_campaign_technology_event" in "test_campaigns.py" has been updated. The "campaign_technology_id" attribute has been renamed to "campaign_technology_ids" and now accepts a list of ids instead of a single id.
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 78.26087% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 53.02%. Comparing base (d1ad2aa) to head (979e3de).
Report is 3 commits behind head on main.

Files Patch % Lines
src/api/store/campaign.py 75.00% 5 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1040   +/-   ##
=======================================
  Coverage   53.02%   53.02%           
=======================================
  Files         419      419           
  Lines       31087    31089    +2     
=======================================
+ Hits        16483    16485    +2     
  Misses      14604    14604           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@abdullai-t abdullai-t marked this pull request as draft June 6, 2024 12:06
@archx3
Copy link
Contributor

archx3 commented Jun 19, 2024

@abdullai-t any update on this draft PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Multiple Technologies to Events
3 participants