feat(notifications): Implement one-click unsubscribe and enhance management command#360
feat(notifications): Implement one-click unsubscribe and enhance management command#360thejoeejoee wants to merge 5 commits intofeat-notificationsfrom
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
There was a problem hiding this comment.
Pull request overview
Implements one-click unsubscribe support for notification emails (List-Unsubscribe headers + unsubscribe endpoint) and enhances the scheduled-notifications management command with batching, dry-run, and improved concurrency behavior.
Changes:
- Add signed unsubscribe token service + public unsubscribe GET/POST view and URL route.
- Add List-Unsubscribe/List-Unsubscribe-Post headers and unsubscribe link to notification email templates.
- Enhance
send_scheduled_notificationscommand (batch size, dry-run, skip_locked locking) and update scheduled-notification model/tests accordingly.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| fiesta/apps/notifications/views.py | Adds UnsubscribeView for token-based unsubscribe flows. |
| fiesta/apps/notifications/urls.py | Routes unsubscribe URLs to the new view. |
| fiesta/apps/notifications/tests/test_unsubscribe.py | Adds tests for token service, unsubscribe view, and email headers. |
| fiesta/apps/notifications/tests/test_scheduler.py | Updates scheduler tests to use NotificationKind and ensure profiles exist. |
| fiesta/apps/notifications/tests/test_scheduled_command.py | Extends command tests for skip_locked, dry-run, batch sizing, and error handling. |
| fiesta/apps/notifications/tests/test_membership_notifications.py | Updates membership notification tests for NotificationKind and profile creation. |
| fiesta/apps/notifications/tests/test_match_notifications.py | Updates match notification tests for NotificationKind and profile creation. |
| fiesta/apps/notifications/tests/factories.py | Updates factories to use NotificationKind and adjust user/profile creation. |
| fiesta/apps/notifications/templates/notifications/unsubscribe.html | Adds unsubscribe confirmation/success/error page. |
| fiesta/apps/notifications/templates/notifications/base_email.txt | Adds unsubscribe link to plaintext email footer. |
| fiesta/apps/notifications/templates/notifications/base_email.html | Adds unsubscribe link to HTML email footer. |
| fiesta/apps/notifications/services/unsubscribe.py | Adds token generation/verification service using TimestampSigner. |
| fiesta/apps/notifications/services/scheduler.py | Tweaks global opt-out check to suppress missing-profile attribute errors. |
| fiesta/apps/notifications/services/mailer.py | Injects unsubscribe_url into email context and sets List-Unsubscribe headers. |
| fiesta/apps/notifications/models/scheduled.py | Uses swappable AUTH_USER_MODEL FK and makes section nullable. |
| fiesta/apps/notifications/migrations/0002_alter_sectionnotificationpreferences_options_and_more.py | Migration to reflect scheduled-notification model field changes. |
| fiesta/apps/notifications/management/commands/send_scheduled_notifications.py | Adds batch size + dry-run; uses skip_locked row-lock claiming and improved accounting/logging. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
fiesta/apps/notifications/management/commands/send_scheduled_notifications.py
Outdated
Show resolved
Hide resolved
fiesta/apps/notifications/management/commands/send_scheduled_notifications.py
Outdated
Show resolved
Hide resolved
…ping, lock contention, dead code)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| email_context = dict(context) | ||
| unsubscribe_url = "" | ||
| if recipient_user is not None: | ||
| token = generate_unsubscribe_token(recipient_user.pk) | ||
| unsubscribe_url = f"https://{settings.ROOT_DOMAIN}/notifications/unsubscribe/{token}/" | ||
|
|
There was a problem hiding this comment.
send_notification_email always generates an unsubscribe_url when recipient_user is provided, even if that user has no UserProfile (the function explicitly allows sending when the profile is missing). In that case the unsubscribe link will reliably 400 because UnsubscribeView looks up UserProfile and treats DoesNotExist as an invalid link. Consider only adding unsubscribe_url/List-Unsubscribe headers when a profile exists, or update the unsubscribe view to handle users without a profile (e.g., create one or persist the opt-out on the User model).
| logger.warning("Unsupported unsubscribe action %r for user_profile=%s", action, user_profile.pk) | ||
|
|
There was a problem hiding this comment.
For unsupported/unknown action values, _unsubscribe only logs a warning and returns without changing any preferences, but the POST handler will still render the success page. That can falsely tell users they unsubscribed when nothing happened. Consider treating unknown actions as invalid (render an error / 400) or mapping them to a safe default behavior.
| "action": action, | ||
| "action_description": self._action_description(action), | ||
| "email": user_profile.user.email, | ||
| }, | ||
| ) |
There was a problem hiding this comment.
The GET confirmation page renders the user’s full email address based solely on possession of the token. If an unsubscribe link is forwarded or leaked, this unnecessarily discloses PII. Consider omitting the email entirely or masking it (e.g., a***@domain.tld) in the template context.
| # Phase 1: Atomically claim pending notifications by stamping sent_at. | ||
| # select_for_update(skip_locked=True) + immediate UPDATE prevents concurrent | ||
| # workers from picking the same rows (locks held only for the UPDATE, not I/O). | ||
| with transaction.atomic(): |
There was a problem hiding this comment.
The new “claim” mechanism sets sent_at=now before any email I/O occurs. If the process crashes or is killed after claiming but before sending/rollback, those notifications will look sent and will never be retried, causing silent message loss. Consider introducing a separate claim field (e.g., claimed_at/locked_at), or only setting sent_at after a successful send while using another durable marker to prevent concurrent workers.
Follow-up to #357 — rebased onto
feat-notifications.Resolves #359.
Changes
RFC 8058 one-click unsubscribe — signed token flow (generate → email header → POST handler → preference update), unsubscribe view with GET confirmation + POST action, template with
{% block main %}.Management command hardening —
--batch-size/--dry-runflags,select_for_update(skip_locked=True)for concurrent safety, deterministicorder_by('send_after', 'pk'), per-notification error handling withsent_atrollback on failure.Bug fixes — token signs
User.pk(notUserProfile.pk), preference filter uses.userFK correctly, POST errors return 400 (not 200),requesttyped asHttpRequest.Cleanup — models/factories/tests aligned to
User-based recipient, removed dead_create_profilehelper + unusedloggerimport.