feat: Add dynamic merge tag validation on campaign launch#274
Open
surbhii-thisside wants to merge 3 commits into
Open
feat: Add dynamic merge tag validation on campaign launch#274surbhii-thisside wants to merge 3 commits into
surbhii-thisside wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Adds server-side validation during campaign launch to detect unresolved merge tags (e.g., {{firstName}}, {{company}}) and block launching when enrolled leads are missing required data, returning structured error details to the client.
Changes:
- Introduces merge-tag extraction and per-campaign validation helpers in
backend/campaigns/views.py. - Integrates merge-tag validation into the
CampaignViewSet.launch()action and returns a 400 response with missing-field details when validation fails.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+32
to
+35
| for step in campaign.steps.all(): | ||
| if hasattr(step, 'email_template') and step.email_template: | ||
| all_tags.update(extract_merge_tags(step.email_template.subject)) | ||
| all_tags.update(extract_merge_tags(step.email_template.body)) |
Comment on lines
+41
to
+43
| enrolled_leads = campaign.enrolled_leads.all() | ||
| if not enrolled_leads: | ||
| return True, None, [] |
Comment on lines
+49
to
+52
| for lead in enrolled_leads: | ||
| field_value = getattr(lead, tag, None) | ||
| if not field_value or str(field_value).strip() == '': | ||
| leads_missing_field.append(lead.email) |
Comment on lines
+17
to
+21
| if not text: | ||
| return set() | ||
| pattern = r'\{\{(\w+)\}\}' | ||
| matches = re.findall(pattern, text) | ||
| return set(m.lower() for m in matches) |
Comment on lines
+114
to
+125
| # Validate merge tags before launch | ||
| is_valid, error_message, missing_fields = validate_merge_tags_in_campaign(campaign) | ||
| if not is_valid: | ||
| return Response( | ||
| { | ||
| "error": error_message, | ||
| "campaign_id": str(campaign.id), | ||
| "missing_fields": missing_fields, | ||
| "message": "Please ensure all leads have required fields before launching.", | ||
| }, | ||
| status=status.HTTP_400_BAD_REQUEST, | ||
| ) |
Comment on lines
+114
to
+116
| # Validate merge tags before launch | ||
| is_valid, error_message, missing_fields = validate_merge_tags_in_campaign(campaign) | ||
| if not is_valid: |
Owner
🚀 PR Guidelines — Read Before Raising a PRHey contributors 👋 I’m LeadOrbit's Bot, and I’ll review every PR before it gets merged. ✅ Your PR will only be merged if:
❌ PRs that may be rejected:
Before submitting:
And if you find the project useful, consider ⭐ starring the repository — it helps the project grow and motivates further development. Quality contributions > PR count. |
|
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 (1)
📝 WalkthroughWalkthroughBackend merge-tag validation helpers ( ChangesMerge-Tag Validation on Campaign Launch
Sequence Diagram(s)sequenceDiagram
participant User
participant launchCampaign
participant Backend as CampaignViewSet.launch
participant validate_merge_tags_in_campaign
participant process_active_leads
User->>launchCampaign: click Launch (forceLaunch=false)
launchCampaign->>Backend: POST /launch {force_launch: false}
Backend->>validate_merge_tags_in_campaign: validate(campaign)
validate_merge_tags_in_campaign-->>Backend: {valid: false, missing_fields: [...]}
Backend-->>launchCampaign: 400 {requires_confirmation, missing_fields}
launchCampaign->>showMergeTagWarningModal: show modal with missing-fields summary
showMergeTagWarningModal-->>User: display "Review Leads" / "Launch Anyway"
alt User clicks Launch Anyway
User->>launchCampaign: launchCampaign(true)
launchCampaign->>Backend: POST /launch {force_launch: true}
Backend->>validate_merge_tags_in_campaign: validate(campaign)
validate_merge_tags_in_campaign-->>Backend: {valid: false, missing_fields: [...]}
Backend->>process_active_leads: delay(campaign_id)
Backend-->>launchCampaign: 200 OK
else User clicks Review Leads
showMergeTagWarningModal-->>User: dismiss modal
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
backend/campaigns/tests.py (2)
1358-1359: ⚡ Quick winAssert campaign state transitions on successful launch paths.
All three success tests only assert
200, so a regression where launch returns OK but does not activate the campaign would go undetected. Addcampaign.status == 'ACTIVE'assertions (and optionally assert confirmation keys are absent on success responses).Suggested assertion additions
self.assertEqual(response.status_code, status.HTTP_200_OK) + campaign.refresh_from_db() + self.assertEqual(campaign.status, 'ACTIVE') + self.assertNotIn('requires_confirmation', response.data)Also applies to: 1393-1394, 1425-1425
🤖 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 1358 - 1359, The three campaign launch success test methods only verify that the response status is HTTP_200_OK but do not assert that the campaign's state has actually transitioned to ACTIVE status. Add assertions after the launch requests to verify that campaign.status equals 'ACTIVE' to ensure state transitions are working correctly. Additionally, consider adding assertions to verify that confirmation keys are absent on success responses. Apply this fix to all three affected test methods at the indicated locations.
1322-1325: ⚡ Quick winStrengthen the blocked-launch contract assertions.
This test currently verifies only key presence, so it would still pass if
missing_fieldsis empty or unrelated. Please assert the actual payload semantics (expected merge tags and at least one affected lead) and that campaign status staysDRAFTafter rejection.Suggested assertion additions
self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) self.assertIn('missing_fields', response.data) self.assertTrue(response.data.get('requires_confirmation')) + self.assertGreater(len(response.data['missing_fields']), 0) + missing_tags = {item['field'] for item in response.data['missing_fields']} + self.assertTrue({'firstName', 'company'}.issubset(missing_tags)) + campaign.refresh_from_db() + self.assertEqual(campaign.status, 'DRAFT')🤖 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 1322 - 1325, The current assertions only verify that the 'missing_fields' key exists in the response but do not validate its actual content or other critical state changes. Strengthen the assertions by checking that the missing_fields value contains the expected merge tags, verify that at least one affected lead is present in the response data, and add an assertion to confirm that the campaign status remains in the DRAFT state after the failed launch rejection. These additions will ensure the blocked-launch contract properly validates both the error payload semantics and the campaign state integrity.backend/campaigns/views.py (1)
54-56: 💤 Low valueRedundant query from
.exists()check.The
.exists()call on line 55 executes a separate database query, followed by another query when iterating overenrolled_cleadson line 63. You can eliminate the extra round-trip by just iterating and checking if the list is empty.♻️ Suggested simplification
# campaign.enrolled_leads returns CampaignLead rows, not Lead rows enrolled_cleads = campaign.enrolled_leads.select_related('lead').all() - if not enrolled_cleads.exists(): + enrolled_cleads = list(enrolled_cleads) + if not enrolled_cleads: return True, None, []🤖 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/views.py` around lines 54 - 56, The enrolled_cleads queryset is being checked with .exists() which executes a separate database query, and then iterated over later which executes another query. To eliminate this redundancy, convert enrolled_cleads to a list by calling list() on the queryset immediately after the select_related, then check if the resulting list is empty using a simple truthiness check (if not enrolled_cleads:) instead of using the .exists() method. This way only a single database query is executed to fetch all the data.
🤖 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.
Inline comments:
In `@backend/campaigns/views.py`:
- Around line 135-162: The code block starting with the force_launch variable
assignment has inconsistent indentation and extra blank lines that violate PEP 8
standards. Fix the indentation of the comment on the line preceding the
force_launch assignment to use 8 spaces instead of 6 spaces to match the
standard indentation used elsewhere in this method. Additionally, remove one of
the three consecutive blank lines that appear after the logger.warning call
(around the end of the code block) to comply with PEP 8, which allows at most
two consecutive blank lines.
In `@frontend/campaign-builder.html`:
- Around line 1769-1781: The message parameter is being interpolated directly
into the innerHTML template string in the modal creation code, which creates a
security vulnerability even if current data sources are safe. Instead of
including ${message} in the innerHTML template, set the innerHTML with the
static markup first, then retrieve the paragraph element and use textContent to
set the message content separately. This ensures user-facing messages are safely
inserted as text rather than potentially executable HTML, preventing XSS
vulnerabilities if data sources change in the future.
---
Nitpick comments:
In `@backend/campaigns/tests.py`:
- Around line 1358-1359: The three campaign launch success test methods only
verify that the response status is HTTP_200_OK but do not assert that the
campaign's state has actually transitioned to ACTIVE status. Add assertions
after the launch requests to verify that campaign.status equals 'ACTIVE' to
ensure state transitions are working correctly. Additionally, consider adding
assertions to verify that confirmation keys are absent on success responses.
Apply this fix to all three affected test methods at the indicated locations.
- Around line 1322-1325: The current assertions only verify that the
'missing_fields' key exists in the response but do not validate its actual
content or other critical state changes. Strengthen the assertions by checking
that the missing_fields value contains the expected merge tags, verify that at
least one affected lead is present in the response data, and add an assertion to
confirm that the campaign status remains in the DRAFT state after the failed
launch rejection. These additions will ensure the blocked-launch contract
properly validates both the error payload semantics and the campaign state
integrity.
In `@backend/campaigns/views.py`:
- Around line 54-56: The enrolled_cleads queryset is being checked with
.exists() which executes a separate database query, and then iterated over later
which executes another query. To eliminate this redundancy, convert
enrolled_cleads to a list by calling list() on the queryset immediately after
the select_related, then check if the resulting list is empty using a simple
truthiness check (if not enrolled_cleads:) instead of using the .exists()
method. This way only a single database query is executed to fetch all the data.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 04f76704-9786-4157-9dbe-e38a9ae13fa3
📒 Files selected for processing (3)
backend/campaigns/tests.pybackend/campaigns/views.pyfrontend/campaign-builder.html
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Implements dynamic merge tag validation on campaign launch to prevent sending campaigns with missing lead data.
What Changed
extract_merge_tags()function to identify merge tags like {{firstName}}, {{company}} in campaign contentvalidate_merge_tags_in_campaign()function to check if enrolled leads have data for required fieldslaunch()method to prevent launch if critical fields are missingHow to Test
Testing
Issue
Closes #187 (LO-041)
Summary by CodeRabbit