test: add integration tests for webhook event processing#305
Open
bh462007 wants to merge 3 commits into
Open
Conversation
📝 WalkthroughWalkthroughTwo test suites are added to ChangesCampaign Launch & Webhook Tests
Leads Page UI Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
🚥 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 resolves a merge conflict in the leads page while expanding the leads UI with CSV import history/error viewing, and adds backend API test coverage for campaign launching and email webhook event handling.
Changes:
- Update
frontend/leads.htmlto include an import history table, an import errors modal, and related client-side rendering/handlers. - Add new DRF
APITestCasesuites covering campaign launch validation/dispatch behavior and webhook event processing. - Clean up merge-conflict artifacts and reformat some HTML/JS blocks.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| frontend/leads.html | Adds import history + import error modal and supporting JS, alongside filter/search UI adjustments. |
| backend/campaigns/tests.py | Introduces tests for /campaigns/{id}/launch/ scenarios and /webhooks/email/ event handling/routing. |
Comments suppressed due to low confidence (1)
frontend/leads.html:1
renderImportHistorybuilds HTML withinnerHTMLand interpolatesjob.filename(and likely other fields such as status elsewhere) without escaping. If any of these fields can contain user-controlled content, this enables XSS. UseescapeHtml(job.filename)(and escape any other interpolated text fields) or build DOM nodes viatextContent.
<!DOCTYPE html>
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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(); | ||
| } |
Comment on lines
+1516
to
+1517
| with patch('campaigns.tasks.process_active_leads.delay'): | ||
| response = self.client.post(f'/api/v1/campaigns/{campaign.id}/launch/', {}, format='json') |
Comment on lines
+1895
to
+1899
| def test_missing_event_type_does_not_crash_returns_200(self): | ||
| response = self._post_webhook({ | ||
| 'email': self.lead.email, | ||
| }) | ||
| self.assertEqual(response.status_code, status.HTTP_200_OK) |
Comment on lines
+1698
to
+1711
| def test_reply_event_without_reply_routes_to_no_branch(self): | ||
| campaign = Campaign.objects.create( | ||
| organization=self.organization, | ||
| name='Reply No Branch Campaign', | ||
| status='ACTIVE', | ||
| connected_account=self.account, | ||
| settings={ | ||
| 'steps': [ | ||
| {'type': 'CONDITION_REPLY', 'condition_time': '1 day'}, | ||
| {'type': 'EMAIL', 'subject': 'No path', 'body': 'no', | ||
| 'condition_branch': 'no', 'condition_parent_index': 0}, | ||
| ] | ||
| }, | ||
| ) |
Comment on lines
+1741
to
+1749
| # No reply detected — webhook fires a non-reply event, status stays ACTIVE | ||
| response = self._post_webhook({ | ||
| 'event': 'open', | ||
| 'email': lead.email, | ||
| 'message_id': 'msg-no-branch', | ||
| }) | ||
| self.assertEqual(response.status_code, status.HTTP_200_OK) | ||
| campaign_lead.refresh_from_db() | ||
| self.assertNotEqual(campaign_lead.status, 'REPLIED') |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
frontend/leads.html (3)
501-507:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winEscape interpolated import fields before writing
innerHTMLto prevent XSS.At Line 504-505 and Line 576, unescaped values (
entry.email,entry.error,job.filename) are inserted into HTML. These can originate from uploaded CSV content/metadata and should be escaped before rendering.Suggested fix
function renderImportErrorRows(job) { @@ 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(''); } @@ tbody.innerHTML = jobs.map(job => ` <tr> - <td class="fw-semibold">${job.filename}</td> + <td class="fw-semibold">${escapeHtml(job.filename)}</td>Also applies to: 574-577
🤖 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 unescaped user-provided values (entry.email, entry.error, and job.filename) are being directly inserted into innerHTML, creating an XSS vulnerability since these values originate from user-uploaded CSV content. Create or use an HTML escaping utility function that converts special characters (like <, >, &, quotes) to their HTML entity equivalents, then apply this escaping function to entry.email and entry.error in the template literal at the tbody.innerHTML assignment, and similarly escape job.filename at the other location mentioned (around line 576). This ensures malicious content cannot be executed as scripts.
689-694:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
!important(or class toggling) when hiding active-filter chips row.At Line 693,
row.style.display = 'none'can be overridden by Bootstrap’sd-flexclass (display:flex !important), so the row may stay visible after being shown once.Suggested fix
if (chips.length) { row.innerHTML = chips.join(''); row.style.removeProperty('display'); } else { - row.style.display = 'none'; + row.style.setProperty('display', 'none', 'important'); }🤖 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 689 - 694, The inline style assignment `row.style.display = 'none'` in the else branch (when chips.length is falsy) can be overridden by Bootstrap's d-flex class that uses display:flex !important. To fix this, either add !important flag to the display none style assignment to match Bootstrap's specificity, or alternatively replace the inline style manipulation with a class toggling approach where you add/remove a CSS class that handles the display property with appropriate specificity instead of directly manipulating the style property.
815-826:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid re-enabling upload button after a successful 202 to prevent duplicate imports.
At Line 817, reload is deferred; but Line 824-826 re-enables the button immediately, allowing repeated POSTs in that window and potentially creating duplicate import jobs.
Suggested fix
try { const res = await fetchWithAuth('/leads/import_csv/', { method: 'POST', body: formData, }); + let queuedForReload = false; if (res.status === 202) { + queuedForReload = true; statusEl.innerHTML = '<span class="text-success"><i class="bi bi-check-circle me-1" aria-hidden="true"></i>File uploaded. Processing now...</span>'; setTimeout(() => window.location.reload(), 1800); } else { statusEl.innerHTML = '<span class="text-danger"><i class="bi bi-x-circle me-1" aria-hidden="true"></i>Upload failed.</span>'; } } catch (err) { statusEl.innerHTML = `<span class="text-danger">Error: ${err.message}</span>`; } finally { - btn.disabled = false; - btn.innerHTML = '<i class="bi bi-cloud-upload me-1" aria-hidden="true"></i> Upload and Process'; + if (!queuedForReload) { + btn.disabled = false; + btn.innerHTML = '<i class="bi bi-cloud-upload me-1" aria-hidden="true"></i> Upload and Process'; + } }🤖 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 815 - 826, The button is being re-enabled in the finally block (lines 824-826) immediately after a 202 success response, even though the page reload is deferred to 1800ms later. This creates a window where users can click the button again and trigger duplicate imports. Modify the finally block to only re-enable the button when the request did not succeed with a 202 status. You can do this by adding a conditional check inside the finally block to skip the button re-enabling when res.status === 202, ensuring the button remains disabled until the page reloads.
🤖 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 1698-1750: The test
test_reply_event_without_reply_routes_to_no_branch is sending an 'open' event
instead of a 'reply' event, and only asserting that status is not equal to
'REPLIED', which does not validate the actual no-branch routing behavior. Change
the webhook event from 'open' to 'reply' in the self._post_webhook call, and add
assertions to verify that the campaign_lead transitions to the correct step or
reaches the expected terminal state when following the no-branch path of the
CONDITION_REPLY step (check current_step assignment or status for the no-branch
outcome).
---
Outside diff comments:
In `@frontend/leads.html`:
- Around line 501-507: The unescaped user-provided values (entry.email,
entry.error, and job.filename) are being directly inserted into innerHTML,
creating an XSS vulnerability since these values originate from user-uploaded
CSV content. Create or use an HTML escaping utility function that converts
special characters (like <, >, &, quotes) to their HTML entity equivalents, then
apply this escaping function to entry.email and entry.error in the template
literal at the tbody.innerHTML assignment, and similarly escape job.filename at
the other location mentioned (around line 576). This ensures malicious content
cannot be executed as scripts.
- Around line 689-694: The inline style assignment `row.style.display = 'none'`
in the else branch (when chips.length is falsy) can be overridden by Bootstrap's
d-flex class that uses display:flex !important. To fix this, either add
!important flag to the display none style assignment to match Bootstrap's
specificity, or alternatively replace the inline style manipulation with a class
toggling approach where you add/remove a CSS class that handles the display
property with appropriate specificity instead of directly manipulating the style
property.
- Around line 815-826: The button is being re-enabled in the finally block
(lines 824-826) immediately after a 202 success response, even though the page
reload is deferred to 1800ms later. This creates a window where users can click
the button again and trigger duplicate imports. Modify the finally block to only
re-enable the button when the request did not succeed with a 202 status. You can
do this by adding a conditional check inside the finally block to skip the
button re-enabling when res.status === 202, ensuring the button remains disabled
until the page reloads.
🪄 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: 4d1179a7-54ed-4191-9b7f-a1d04b82c1c5
📒 Files selected for processing (2)
backend/campaigns/tests.pyfrontend/leads.html
Comment on lines
+1698
to
+1750
| def test_reply_event_without_reply_routes_to_no_branch(self): | ||
| campaign = Campaign.objects.create( | ||
| organization=self.organization, | ||
| name='Reply No Branch Campaign', | ||
| status='ACTIVE', | ||
| connected_account=self.account, | ||
| settings={ | ||
| 'steps': [ | ||
| {'type': 'CONDITION_REPLY', 'condition_time': '1 day'}, | ||
| {'type': 'EMAIL', 'subject': 'No path', 'body': 'no', | ||
| 'condition_branch': 'no', 'condition_parent_index': 0}, | ||
| ] | ||
| }, | ||
| ) | ||
| condition_step = SequenceStep.objects.create( | ||
| organization=self.organization, | ||
| campaign=campaign, | ||
| step_order=1, | ||
| channel_type='CONDITION_REPLY', | ||
| delay_minutes=0, | ||
| ) | ||
| SequenceStep.objects.create( | ||
| organization=self.organization, | ||
| campaign=campaign, | ||
| step_order=2, | ||
| channel_type='EMAIL', | ||
| delay_minutes=0, | ||
| template_subject='No path', | ||
| template_body='no', | ||
| ) | ||
| lead = Lead.objects.create( | ||
| organization=self.organization, | ||
| email='reply-no@webhookcorp.test', | ||
| ) | ||
| campaign_lead = CampaignLead.objects.create( | ||
| organization=self.organization, | ||
| campaign=campaign, | ||
| lead=lead, | ||
| status='ACTIVE', | ||
| current_step=condition_step, | ||
| last_sent_message_id='msg-no-branch', | ||
| ) | ||
|
|
||
| # No reply detected — webhook fires a non-reply event, status stays ACTIVE | ||
| response = self._post_webhook({ | ||
| 'event': 'open', | ||
| 'email': lead.email, | ||
| 'message_id': 'msg-no-branch', | ||
| }) | ||
| self.assertEqual(response.status_code, status.HTTP_200_OK) | ||
| campaign_lead.refresh_from_db() | ||
| self.assertNotEqual(campaign_lead.status, 'REPLIED') | ||
|
|
There was a problem hiding this comment.
CONDITION_REPLY no-branch scenario is not actually being tested
This test sends an open webhook and only asserts status != REPLIED, so it does not verify the required “reply with no-branch routes correctly” behavior. Please make this a true reply-path assertion (e.g., event='reply' with no detected reply state for that condition flow) and assert concrete routing outcome (current_step/terminal state) for the no-branch.
🤖 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 1698 - 1750, The test
test_reply_event_without_reply_routes_to_no_branch is sending an 'open' event
instead of a 'reply' event, and only asserting that status is not equal to
'REPLIED', which does not validate the actual no-branch routing behavior. Change
the webhook event from 'open' to 'reply' in the self._post_webhook call, and add
assertions to verify that the campaign_lead transitions to the correct step or
reaches the expected terminal state when following the no-branch path of the
CONDITION_REPLY step (check current_step assignment or status for the no-branch
outcome).
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 a dedicated
WebhookEventTestsintegration test suite for the email webhook endpoint (POST /api/v1/webhooks/email/).The new tests validate webhook processing across all supported event types, condition-based workflow routing, and various edge cases to ensure webhook events are handled reliably without breaking existing campaign flows.
No production code was modified as part of this change.
Closes #292
What Was Added
A new
WebhookEventTestsclass was added tobackend/campaigns/tests.pywith coverage for the following scenarios:Event Processing
BOUNCEDREPLIEDand stop the active sequence when no condition branch is involvedlast_opened_atlast_clicked_atConditional Workflow Routing
CONDITION_REPLYyes-branch when a matching condition step existsCONDITION_OPENroutingCONDITION_CLICKroutingEdge Cases
message_idvalues do not cause errorsFiles Modified
backend/campaigns/tests.pyWebhookEventTestsintegration test suiteTesting
All campaign tests pass successfully with the new coverage:
Found 58 test(s). ---------------------------------------------------------------------- Ran 58 tests in 18.210s OKRun locally with:
Type of Change
Checklist
campaignstest suite passes successfullySummary by CodeRabbit
New Features
Improvements
Tests