feat: add unsubscribe feedback survey (#560)#631
Conversation
|
Warning Review limit reached
Next review available in: 33 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds an unsubscribe feedback model and migration, updates the unsubscribe page to collect and save selected reasons, refreshes related tests, and introduces a click-tracking endpoint that validates signed links and redirects. ChangesUnsubscribe Feedback Survey
Estimated code review effort: 3 (Moderate) | ~20 minutes Click Tracking Endpoint
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Prospect
participant UnsubscribeView
participant UnsubscribeFeedback
Prospect->>UnsubscribeView: GET unsubscribe page
UnsubscribeView-->>Prospect: confirmation form with reasons
Prospect->>UnsubscribeView: POST selected reasons
UnsubscribeView->>UnsubscribeFeedback: save feedback record
UnsubscribeView-->>Prospect: unsubscribe response
sequenceDiagram
participant Recipient
participant ClickTrackingView
participant CampaignLead
participant RedirectTarget
Recipient->>ClickTrackingView: GET tracking link
ClickTrackingView->>ClickTrackingView: verify token and destination
ClickTrackingView->>CampaignLead: update last_clicked_at
ClickTrackingView->>CampaignLead: run CONDITION_CLICK step if needed
ClickTrackingView-->>RedirectTarget: redirect
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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 (1)
backend/campaigns/views.py (1)
793-830: 🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy liftSign the redirect target before using it
destis only a query parameter, so any valid tracking link can be repointed to an arbitrary URL and turned into an open redirect.- Unexpected errors in the analytics /
_execute_condition_click_steppath can still abort the request before the redirect; keep that path from failing the click-through.🤖 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 793 - 830, The ClickTrackingView.get flow uses the raw dest query parameter as the redirect target, which allows repointing to arbitrary URLs and creates an open redirect. Update ClickTrackingView.get to verify or sign the destination before redirecting, and only redirect after validating that the decoded_dest matches the expected signed target. Also make the analytics and _execute_condition_click_step path fully non-fatal by catching unexpected exceptions around the CampaignLead update so click-through still proceeds to HttpResponseRedirect even if tracking logic fails.
🧹 Nitpick comments (2)
backend/campaigns/models.py (1)
152-161: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider adding a uniqueness constraint on
leadto prevent duplicate feedback records.The view creates a new
UnsubscribeFeedbackon every POST without checking for an existing record. Since unsubscribed leads shouldn't receive further emails, one feedback per lead is the expected cardinality — but the model'sForeignKeyallows multiples. The test attests.py:1745uses.get(lead=lead), which would raiseMultipleObjectsReturnedif duplicates exist (e.g., from a double-click or replayed POST).♻️ Proposed fix: add unique constraint
class UnsubscribeFeedback(TenantModel): lead = models.ForeignKey( Lead, on_delete=models.CASCADE, related_name="unsubscribe_feedbacks", ) reasons = models.JSONField(default=list, blank=True) + class Meta: + ordering = ['-created_at'] + def __str__(self): return f"Feedback for {self.lead.email}"Then update the view to use
update_or_createinstead ofcreate(see the view review comment for the corresponding fix).🤖 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/models.py` around lines 152 - 161, Add a uniqueness constraint to UnsubscribeFeedback so each Lead can have only one feedback record; the current ForeignKey on lead allows duplicates and breaks callers like the test using .get(lead=lead). Update the UnsubscribeFeedback model to enforce one-to-one cardinality for lead, or add an equivalent unique constraint, and then make sure the unsubscribe view that creates feedback uses update_or_create instead of always creating a new row.backend/campaigns/tests.py (1)
1730-1751: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test case for POST with no reasons selected.
The
if reasons:guard in the view means noUnsubscribeFeedbackrecord is created when no checkboxes are selected, but this path is not tested. A missing test leaves the "no feedback created" behavior unverified.🧪 Proposed additional test
def test_unsubscribe_post_without_reasons_does_not_create_feedback(self): lead = Lead.objects.create( organization=self.organization, email='no-reasons@acme.test', ) token = generate_unsubscribe_token(lead.id) response = self.client.post( f'/api/v1/unsubscribe/{lead.id}/{token}/', ) self.assertEqual(response.status_code, status.HTTP_200_OK) self.assertIn('You have been unsubscribed', response.content.decode('utf-8')) lead.refresh_from_db() self.assertTrue(lead.global_unsubscribe) self.assertFalse(UnsubscribeFeedback.objects.filter(lead=lead).exists())🤖 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 1730 - 1751, Add a test in the unsubscribe test class to cover POST requests made without any reasons selected, using the same unsubscribe endpoint and token flow as the existing unsubscribe test. Verify the response still returns success and the lead becomes globally unsubscribed, then assert that UnsubscribeFeedback.objects.filter(lead=lead).exists() is false so the no-feedback path in the view’s if reasons guard is exercised. Keep the new test alongside the existing unsubscribe POST test that currently checks UnsubscribeFeedback.objects.get(lead=lead) when reasons are provided.
🤖 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 775-783: The unsubscribe feedback creation in the view is not
idempotent and accepts unvalidated reason values. Update the `reasons` handling
in the unsubscribe flow to validate incoming values against the allowed form
choices before saving, then replace the plain
`UnsubscribeFeedback.objects.create(...)` call with an idempotent
`update_or_create`-style write keyed by `lead` (and `organization` if needed) so
repeated POSTs reuse the same `UnsubscribeFeedback` record instead of creating
duplicates.
---
Outside diff comments:
In `@backend/campaigns/views.py`:
- Around line 793-830: The ClickTrackingView.get flow uses the raw dest query
parameter as the redirect target, which allows repointing to arbitrary URLs and
creates an open redirect. Update ClickTrackingView.get to verify or sign the
destination before redirecting, and only redirect after validating that the
decoded_dest matches the expected signed target. Also make the analytics and
_execute_condition_click_step path fully non-fatal by catching unexpected
exceptions around the CampaignLead update so click-through still proceeds to
HttpResponseRedirect even if tracking logic fails.
---
Nitpick comments:
In `@backend/campaigns/models.py`:
- Around line 152-161: Add a uniqueness constraint to UnsubscribeFeedback so
each Lead can have only one feedback record; the current ForeignKey on lead
allows duplicates and breaks callers like the test using .get(lead=lead). Update
the UnsubscribeFeedback model to enforce one-to-one cardinality for lead, or add
an equivalent unique constraint, and then make sure the unsubscribe view that
creates feedback uses update_or_create instead of always creating a new row.
In `@backend/campaigns/tests.py`:
- Around line 1730-1751: Add a test in the unsubscribe test class to cover POST
requests made without any reasons selected, using the same unsubscribe endpoint
and token flow as the existing unsubscribe test. Verify the response still
returns success and the lead becomes globally unsubscribed, then assert that
UnsubscribeFeedback.objects.filter(lead=lead).exists() is false so the
no-feedback path in the view’s if reasons guard is exercised. Keep the new test
alongside the existing unsubscribe POST test that currently checks
UnsubscribeFeedback.objects.get(lead=lead) when reasons are provided.
🪄 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: 114ba323-124a-491f-954c-a35ff3a964b7
📒 Files selected for processing (4)
backend/campaigns/migrations/0011_unsubscribefeedback.pybackend/campaigns/models.pybackend/campaigns/tests.pybackend/campaigns/views.py
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 (1)
backend/campaigns/views.py (1)
808-821: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winOpen redirect:
destis still attacker-controlled
dest_urlcomes straight from the query string, is onlyunquoted, and then gets passed toHttpResponseRedirect. A validtcan be reused with&dest=https://evil.com, so this endpoint can redirect to arbitrary sites. Binddestinto the signed payload or restrict redirects to an allowlist of internal targets.🤖 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 808 - 821, The tracking redirect in the get handler still trusts the attacker-controlled dest query parameter, so fix it by removing raw dest_url usage and binding the destination into the signed payload verified by Signer.unsign in this view. Update the token handling around signed_token/campaign_lead_id/step_id so the redirect target is validated from the signed data or restricted to an internal allowlist before HttpResponseRedirect is called.
🤖 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 775-791: The unsubscribe feedback handling block is accidentally
dedented out of `unsubscribe_view`, so it runs at module import time instead of
during the request. Re-indent the `ALLOWED_REASONS` filtering and
`UnsubscribeFeedback.objects.update_or_create` logic back inside
`unsubscribe_view` so `request` and `lead` are in scope and the module can
import cleanly.
---
Outside diff comments:
In `@backend/campaigns/views.py`:
- Around line 808-821: The tracking redirect in the get handler still trusts the
attacker-controlled dest query parameter, so fix it by removing raw dest_url
usage and binding the destination into the signed payload verified by
Signer.unsign in this view. Update the token handling around
signed_token/campaign_lead_id/step_id so the redirect target is validated from
the signed data or restricted to an internal allowlist before
HttpResponseRedirect is called.
🪄 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: 38e8e45f-a4bb-4b9b-9c5e-727dfdaf5b2b
📒 Files selected for processing (1)
backend/campaigns/views.py
| ALLOWED_REASONS = { | ||
| "Too frequent emails", | ||
| "No longer relevant", | ||
| "Never signed up", | ||
| } | ||
|
|
||
| reasons = request.POST.getlist("reasons") | ||
| valid_reasons = [reason for reason in reasons if reason in ALLOWED_REASONS] | ||
|
|
||
| if valid_reasons: | ||
| UnsubscribeFeedback.objects.update_or_create( | ||
| lead=lead, | ||
| defaults={ | ||
| "organization": lead.organization, | ||
| "reasons": valid_reasons, | ||
| }, | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Critical: block is dedented out of unsubscribe_view, breaking module import.
Lines 781–791 sit at module scope (column 0), so request, lead, and ALLOWED_REASONS resolve at import time and raise NameError — the entire views.py module fails to load. This matches the Ruff F821 errors. The feedback-saving logic also never runs inside the request path. Re-indent the whole block into the function body.
🐛 Proposed fix: re-indent into the function
- ALLOWED_REASONS = {
- "Too frequent emails",
- "No longer relevant",
- "Never signed up",
-}
-
-reasons = request.POST.getlist("reasons")
-valid_reasons = [reason for reason in reasons if reason in ALLOWED_REASONS]
-
-if valid_reasons:
- UnsubscribeFeedback.objects.update_or_create(
- lead=lead,
- defaults={
- "organization": lead.organization,
- "reasons": valid_reasons,
- },
- )
+ ALLOWED_REASONS = {
+ "Too frequent emails",
+ "No longer relevant",
+ "Never signed up",
+ }
+
+ reasons = request.POST.getlist("reasons")
+ valid_reasons = [reason for reason in reasons if reason in ALLOWED_REASONS]
+
+ if valid_reasons:
+ UnsubscribeFeedback.objects.update_or_create(
+ lead=lead,
+ defaults={
+ "organization": lead.organization,
+ "reasons": valid_reasons,
+ },
+ )📝 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.
| ALLOWED_REASONS = { | |
| "Too frequent emails", | |
| "No longer relevant", | |
| "Never signed up", | |
| } | |
| reasons = request.POST.getlist("reasons") | |
| valid_reasons = [reason for reason in reasons if reason in ALLOWED_REASONS] | |
| if valid_reasons: | |
| UnsubscribeFeedback.objects.update_or_create( | |
| lead=lead, | |
| defaults={ | |
| "organization": lead.organization, | |
| "reasons": valid_reasons, | |
| }, | |
| ) | |
| ALLOWED_REASONS = { | |
| "Too frequent emails", | |
| "No longer relevant", | |
| "Never signed up", | |
| } | |
| reasons = request.POST.getlist("reasons") | |
| valid_reasons = [reason for reason in reasons if reason in ALLOWED_REASONS] | |
| if valid_reasons: | |
| UnsubscribeFeedback.objects.update_or_create( | |
| lead=lead, | |
| defaults={ | |
| "organization": lead.organization, | |
| "reasons": valid_reasons, | |
| }, | |
| ) |
🧰 Tools
🪛 Ruff (0.15.20)
[error] 781-781: Undefined name request
(F821)
[error] 782-782: Undefined name ALLOWED_REASONS
(F821)
[error] 786-786: Undefined name lead
(F821)
[error] 788-788: Undefined name lead
(F821)
🤖 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 775 - 791, The unsubscribe feedback
handling block is accidentally dedented out of `unsubscribe_view`, so it runs at
module import time instead of during the request. Re-indent the
`ALLOWED_REASONS` filtering and `UnsubscribeFeedback.objects.update_or_create`
logic back inside `unsubscribe_view` so `request` and `lead` are in scope and
the module can import cleanly.
Source: Linters/SAST tools
1 participant
Pull Request
🔗 Related Issue
Closes #560
📝 Summary of Changes
This PR improves the email unsubscribe experience by adding an optional feedback survey to the confirmation page. Users can select one or more reasons for unsubscribing, and their responses are stored in the database to provide insights that can help improve future email campaigns.
Changes made:
Added a new
UnsubscribeFeedbackmodel to store unsubscribe feedback.Updated the unsubscribe confirmation page to display optional checkbox-based feedback.
Saved the selected feedback reasons when users confirm their unsubscribe request.
Added tests to verify that:
🏷️ Type of Change
🧪 Testing
Verified the implementation by running the relevant unsubscribe tests.
Steps to test:
Open the unsubscribe link for a valid lead.
Verify that the feedback survey with checkbox options is displayed.
Select one or more reasons and confirm the unsubscribe.
Verify that the lead is unsubscribed and the selected feedback is stored in the database.
Run:
python manage.py test campaigns.tests.CampaignWorkflowTests.test_unsubscribe_get_shows_confirmation_without_updating_leadpython manage.py test campaigns.tests.CampaignWorkflowTests.test_unsubscribe_post_marks_lead_unsubscribed📸 Screenshots (if applicable)
N/A (Backend functionality with a simple HTML form update.)
✅ Checklist
Summary by CodeRabbit
New Features
Bug Fixes