test: add integration tests for campaign launch flow#304
Open
bh462007 wants to merge 2 commits into
Open
Conversation
📝 WalkthroughWalkthroughAdds a ChangesCampaign Launch Integration Tests
Leads Page JS/HTML Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR updates the Leads frontend to include import history/error visibility and adds backend API tests covering campaign launch validation and task dispatch behavior.
Changes:
- Adds an “Import History” table and an “Import Errors” modal to the leads page UI.
- Resolves merge-conflict artifacts and refactors parts of the leads page JS (filters/search/import history rendering).
- Adds a new
CampaignLaunchTestssuite to validate/campaigns/{id}/launch/behavior across common failure/success paths.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| frontend/leads.html | Adds import history + error modal UI and related JS helpers while cleaning up prior merge conflict changes. |
| backend/campaigns/tests.py | Adds comprehensive API tests around campaign launch preconditions and Celery dispatch behavior. |
Comments suppressed due to low confidence (1)
frontend/leads.html:1
- This uses
innerHTMLwith server-provided values (e.g.,job.filename,job.status) without escaping, which can enable XSS if any field contains HTML. Prefer inserting viatextContent/DOM APIs, or wrap interpolated server-provided strings withescapeHtml()before concatenating intoinnerHTML.
<!DOCTYPE html>
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
510
to
512
| function openImportErrors(jobId) { | ||
| const job = allImportJobs.find(item => item.id === jobId); | ||
| if (!job) return; |
| def test_launch_without_connected_account_and_no_leads_returns_400(self): | ||
| """Campaign with no connected account and no leads should return 400.""" | ||
| campaign = self._make_campaign() | ||
| response = self.client.post(f'/api/v1/campaigns/{campaign.id}/launch/', {}, format='json') |
| response = self.client.post(f'/api/v1/campaigns/{campaign.id}/launch/', {}, format='json') | ||
|
|
||
| self.assertEqual(response.status_code, status.HTTP_200_OK) | ||
| mocked_once.assert_called() |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
frontend/leads.html (2)
501-507:⚠️ Potential issue | 🟠 Major | ⚡ Quick winXSS vulnerability:
entry.emailandentry.errorare rendered without escaping.These values originate from CSV import validation and can contain attacker-controlled content. A malicious CSV with an email like
<img src=x onerror=alert(1)>would execute JavaScript when viewing errors.🔒 Proposed fix
tbody.innerHTML = errors.map(entry => ` <tr> <td class="fw-semibold">${entry.row ?? '-'}</td> - <td>${entry.email || '<span class="text-muted">-</span>'}</td> - <td>${entry.error || '<span class="text-muted">-</span>'}</td> + <td>${entry.email ? escapeHtml(entry.email) : '<span class="text-muted">-</span>'}</td> + <td>${entry.error ? escapeHtml(entry.error) : '<span class="text-muted">-</span>'}</td> </tr> `).join('');🤖 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 `@frontend/leads.html` around lines 501 - 507, The entry.email and entry.error properties are being directly inserted into the HTML template without HTML escaping, creating an XSS vulnerability where malicious content from CSV imports can execute JavaScript. Create or use a helper function to HTML-escape these values (replacing special characters like <, >, &, ", ') before they are interpolated into the template string. Apply this escaping to both entry.email and entry.error in the template literal where tbody.innerHTML is being set.
574-587:⚠️ Potential issue | 🟠 Major | ⚡ Quick winXSS vulnerability:
job.filenameis rendered without escaping.Filenames are user-controlled. An attacker could upload a file named
<script>alert(1)</script>.csvto execute arbitrary JavaScript.🔒 Proposed fix
tbody.innerHTML = jobs.map(job => ` <tr> - <td class="fw-semibold">${job.filename}</td> + <td class="fw-semibold">${escapeHtml(job.filename)}</td> <td>${job.total_rows ?? 0}</td>🤖 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 `@frontend/leads.html` around lines 574 - 587, The `job.filename` value is being rendered directly into the HTML template without escaping, creating an XSS vulnerability where filenames containing malicious code (like `<script>alert(1)</script>.csv`) could execute arbitrary JavaScript. Escape the HTML entities in the `job.filename` variable before inserting it into the template string by using a helper function that replaces special characters such as `<`, `>`, `&`, `"`, and `'` with their corresponding HTML entities. Apply this escaping specifically to the `${job.filename}` interpolation in the template literal used in the tbody.innerHTML assignment.
🤖 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/tests.py`:
- Around line 1434-1440: The test
`test_launch_without_connected_account_and_no_leads_returns_400` currently tests
two invalid conditions simultaneously, so it passes via the zero-leads
validation check and never actually verifies the missing-connected-account
requirement. To isolate the missing-connected-account failure path, enroll at
least one lead in the campaign (using an appropriate method to add a lead to the
campaign object before making the API call) so the request reaches the
connected-account validation logic rather than failing early on the leads check.
In `@frontend/leads.html`:
- Around line 510-517: The `openImportErrors` function has a type mismatch where
`jobId` parameter is a string (from `btn.dataset.jobId`) but `item.id` from the
API is a number, causing the strict equality check in the find method to always
fail silently. Convert `jobId` to a number using `parseInt` or the unary plus
operator before comparing it with `item.id` in the find condition to ensure the
types match and the job lookup succeeds.
---
Outside diff comments:
In `@frontend/leads.html`:
- Around line 501-507: The entry.email and entry.error properties are being
directly inserted into the HTML template without HTML escaping, creating an XSS
vulnerability where malicious content from CSV imports can execute JavaScript.
Create or use a helper function to HTML-escape these values (replacing special
characters like <, >, &, ", ') before they are interpolated into the template
string. Apply this escaping to both entry.email and entry.error in the template
literal where tbody.innerHTML is being set.
- Around line 574-587: The `job.filename` value is being rendered directly into
the HTML template without escaping, creating an XSS vulnerability where
filenames containing malicious code (like `<script>alert(1)</script>.csv`) could
execute arbitrary JavaScript. Escape the HTML entities in the `job.filename`
variable before inserting it into the template string by using a helper function
that replaces special characters such as `<`, `>`, `&`, `"`, and `'` with their
corresponding HTML entities. Apply this escaping specifically to the
`${job.filename}` interpolation in the template literal used in the
tbody.innerHTML assignment.
🪄 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: 56ee7144-e664-4dca-87bd-26ba9c204050
📒 Files selected for processing (2)
backend/campaigns/tests.pyfrontend/leads.html
Comment on lines
+1434
to
+1440
| def test_launch_without_connected_account_and_no_leads_returns_400(self): | ||
| """Campaign with no connected account and no leads should return 400.""" | ||
| campaign = self._make_campaign() | ||
| response = self.client.post(f'/api/v1/campaigns/{campaign.id}/launch/', {}, format='json') | ||
| self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) | ||
| self.assertIn('error', response.data) | ||
|
|
There was a problem hiding this comment.
Isolate the missing-connected-account failure path
Line 1434 currently combines two invalid conditions (no connected account + no enrolled leads), so this test passes via the zero-leads branch and does not actually verify the “missing connected account” requirement. Please enroll a lead here (or rename the test to reflect zero-leads validation) so coverage matches the intended scenario.
Suggested test adjustment
- def test_launch_without_connected_account_and_no_leads_returns_400(self):
- """Campaign with no connected account and no leads should return 400."""
+ def test_launch_without_connected_account_returns_400(self):
+ """Campaign with enrolled leads but no sender account should return 400."""
campaign = self._make_campaign()
+ self._enroll_lead(campaign)
response = self.client.post(f'/api/v1/campaigns/{campaign.id}/launch/', {}, format='json')
self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
self.assertIn('error', response.data)
+ 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 1434 - 1440, The test
`test_launch_without_connected_account_and_no_leads_returns_400` currently tests
two invalid conditions simultaneously, so it passes via the zero-leads
validation check and never actually verifies the missing-connected-account
requirement. To isolate the missing-connected-account failure path, enroll at
least one lead in the campaign (using an appropriate method to add a lead to the
campaign object before making the API call) so the request reaches the
connected-account validation logic rather than failing early on the leads check.
Comment on lines
510
to
517
| function openImportErrors(jobId) { | ||
| const job = allImportJobs.find(item => item.id === jobId); | ||
| if (!job) return; | ||
|
|
||
| document.getElementById('import-errors-meta').textContent = | ||
| `${job.filename} · ${job.failed_count} error${job.failed_count === 1 ? '' : 's'}`; | ||
| renderImportErrorRows(job); | ||
| bootstrap.Modal.getOrCreateInstance(document.getElementById('importErrorsModal')).show(); | ||
| } |
There was a problem hiding this comment.
Type mismatch: jobId is a string but item.id is likely a number.
btn.dataset.jobId always returns a string, but job.id from the API is typically a number. The strict equality check item.id === jobId will always fail, causing the "View" button to silently do nothing.
🐛 Proposed fix
function openImportErrors(jobId) {
- const job = allImportJobs.find(item => item.id === jobId);
+ const job = allImportJobs.find(item => String(item.id) === jobId);
if (!job) return;Alternatively, parse to number if IDs are always numeric:
function openImportErrors(jobId) {
- const job = allImportJobs.find(item => item.id === jobId);
+ const job = allImportJobs.find(item => item.id === Number(jobId));
if (!job) return;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function openImportErrors(jobId) { | |
| const job = allImportJobs.find(item => item.id === jobId); | |
| if (!job) return; | |
| document.getElementById('import-errors-meta').textContent = | |
| `${job.filename} · ${job.failed_count} error${job.failed_count === 1 ? '' : 's'}`; | |
| renderImportErrorRows(job); | |
| bootstrap.Modal.getOrCreateInstance(document.getElementById('importErrorsModal')).show(); | |
| } | |
| function openImportErrors(jobId) { | |
| const job = allImportJobs.find(item => String(item.id) === jobId); | |
| if (!job) return; | |
| document.getElementById('import-errors-meta').textContent = | |
| `${job.filename} · ${job.failed_count} error${job.failed_count === 1 ? '' : 's'}`; | |
| renderImportErrorRows(job); | |
| bootstrap.Modal.getOrCreateInstance(document.getElementById('importErrorsModal')).show(); | |
| } |
Suggested change
| function openImportErrors(jobId) { | |
| const job = allImportJobs.find(item => item.id === jobId); | |
| if (!job) return; | |
| document.getElementById('import-errors-meta').textContent = | |
| `${job.filename} · ${job.failed_count} error${job.failed_count === 1 ? '' : 's'}`; | |
| renderImportErrorRows(job); | |
| bootstrap.Modal.getOrCreateInstance(document.getElementById('importErrorsModal')).show(); | |
| } | |
| function openImportErrors(jobId) { | |
| const job = allImportJobs.find(item => item.id === Number(jobId)); | |
| if (!job) return; | |
| document.getElementById('import-errors-meta').textContent = | |
| `${job.filename} · ${job.failed_count} error${job.failed_count === 1 ? '' : 's'}`; | |
| renderImportErrorRows(job); | |
| bootstrap.Modal.getOrCreateInstance(document.getElementById('importErrorsModal')).show(); | |
| } |
🤖 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 `@frontend/leads.html` around lines 510 - 517, The `openImportErrors` function
has a type mismatch where `jobId` parameter is a string (from
`btn.dataset.jobId`) but `item.id` from the API is a number, causing the strict
equality check in the find method to always fail silently. Convert `jobId` to a
number using `parseInt` or the unary plus operator before comparing it with
`item.id` in the find condition to ensure the types match and the job lookup
succeeds.
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.
Summary
This PR adds comprehensive integration test coverage for the campaign launch endpoint (
POST /api/v1/campaigns/{id}/launch/).A new
CampaignLaunchTeststest class has been added tobackend/campaigns/tests.pyto validate the endpoint's behavior across both success and failure paths, including campaign validation rules and Celery execution modes.No production code was modified as part of this change.
Closes #291
What Was Added
Created a dedicated
CampaignLaunchTestssuite covering the following scenarios:Validation & Error Cases
For all invalid launch attempts, the API returns the expected error response and the campaign remains in the
DRAFTstate.Successful Launch Cases
ACTIVEcampaign to verify existing behaviorCelery Execution Modes
CELERY_TASK_ALWAYS_EAGER=Trueto verify leads are processed synchronously and no async task is dispatchedprocess_active_leads.delay()is triggered correctlyFiles Modified
backend/campaigns/tests.pyCampaignLaunchTestsintegration test suiteTesting
All campaign tests pass successfully after adding the new coverage:
Found 46 test(s). ---------------------------------------------------------------------- Ran 46 tests in 14.275s OKRun locally with:
Type of Change
Checklist
campaignstest suite passes successfullySummary by CodeRabbit
New Features
Improvements
Tests