feat: auto-pause campaign if initial bounce rate exceeds threshold (#… - #644
feat: auto-pause campaign if initial bounce rate exceeds threshold (#…#644Bheemeswari497 wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughCampaign bounce processing now performs an atomic health check after recording bounces. Campaigns with at least 50 sent emails and bounce rates above 10% are paused and generate a notification. Completion logic skips paused campaigns, with tests covering thresholds and precedence. ChangesCampaign auto-pause
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant BounceProcessor
participant CampaignHealthCheck
participant Campaign
participant NotificationService
BounceProcessor->>CampaignHealthCheck: evaluate campaign after recording bounce
CampaignHealthCheck->>Campaign: lock row and inspect sent and bounced counts
CampaignHealthCheck->>Campaign: set status to PAUSED when bounce rate exceeds 10%
CampaignHealthCheck->>NotificationService: schedule campaign_paused notification on commit
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Hi @Kuldeeep18, I have completed the implementation for Issue #480. The feature has been implemented along with backend tests covering the required scenarios. I would appreciate it if you could review the PR when you have time. Thank you! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
backend/campaigns/tests.py (1)
2059-2079: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis test doesn't actually exercise pause-over-completion precedence.
The lead is created with
status='ACTIVE', so in_maybe_mark_campaign_completedthehas_unfinishedcheck isTrueand the function returns before reaching the.exclude(status='PAUSED').update(...)logic. The campaign staysPAUSEDtrivially, not because of the precedence guard. To truly validate that pause wins over completion, the enrolled lead should be in a terminal state (e.g.BOUNCED) so completion would otherwise fire.💚 Make the lead terminal so completion is a real contender
lead = Lead.objects.create(organization=self.organization, email='b4@acme.test') clead = CampaignLead.objects.create( organization=self.organization, campaign=self.campaign, lead=lead, current_step=self.step, - status='ACTIVE' + status='BOUNCED' )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/campaigns/tests.py` around lines 2059 - 2079, Update test_pause_priority_over_completion so the CampaignLead created for the precedence scenario uses a terminal status such as BOUNCED instead of ACTIVE. Keep the existing health-check and _maybe_mark_campaign_completed calls, ensuring completion would otherwise be eligible while the assertion verifies the campaign remains PAUSED.backend/campaigns/tasks.py (1)
155-162: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMove the notification out of the locked transaction.
send_notificationperforms external Firestore I/O while theselect_for_update()row lock is still held insidetransaction.atomic(). A slow or hanging network call keeps the campaign row locked, blocking concurrent bounce processing for the same campaign. Prefer persisting the status change, then emitting the notification after the transaction commits.♻️ Send notification after commit
with transaction.atomic(): try: campaign = Campaign.objects.select_for_update().get(id=campaign_id) except Campaign.DoesNotExist: return if campaign.status == 'PAUSED': return + paused = False if campaign.sent_count >= 50: bounce_rate = campaign.bounced_count / campaign.sent_count if bounce_rate > 0.10: campaign.status = 'PAUSED' campaign.save(update_fields=['status']) logger.info(f"Campaign {campaign.id} auto-paused due to high bounce rate.") - send_notification( - campaign.organization_id, - 'campaign_paused', - { - 'message': 'Campaign Auto-Paused due to high bounce rates', - 'campaign_id': str(campaign.id) - } - ) + paused = True + org_id, cid = campaign.organization_id, str(campaign.id) + + if paused: + send_notification( + org_id, + 'campaign_paused', + { + 'message': 'Campaign Auto-Paused due to high bounce rates', + 'campaign_id': cid, + }, + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/campaigns/tasks.py` around lines 155 - 162, Move the send_notification call out of the transaction.atomic/select_for_update block in the campaign auto-pause flow, while preserving the status update within the transaction. Register or perform the campaign_paused notification only after the transaction successfully commits, using the existing campaign organization_id and id values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@backend/campaigns/tasks.py`:
- Around line 155-162: Move the send_notification call out of the
transaction.atomic/select_for_update block in the campaign auto-pause flow,
while preserving the status update within the transaction. Register or perform
the campaign_paused notification only after the transaction successfully
commits, using the existing campaign organization_id and id values.
In `@backend/campaigns/tests.py`:
- Around line 2059-2079: Update test_pause_priority_over_completion so the
CampaignLead created for the precedence scenario uses a terminal status such as
BOUNCED instead of ACTIVE. Keep the existing health-check and
_maybe_mark_campaign_completed calls, ensuring completion would otherwise be
eligible while the assertion verifies the campaign remains PAUSED.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: acfda693-f11f-4530-b7e8-0d641b1708e2
📒 Files selected for processing (2)
backend/campaigns/tasks.pybackend/campaigns/tests.py
|
Hi @Kuldeeep18, I have addressed all the CodeRabbit review comments and updated the PR accordingly. Changes made:
All checks are now passing successfully. Kindly review the PR when you have time. Thank you! |
1 participant
Related Issue
Closes #480
Summary
Implemented automatic campaign pausing when the initial bounce rate exceeds 10% after at least 50 emails have been sent.
Changes
Type of Change
Testing
Summary by CodeRabbit