From 969b33f2622311f709780df87f9604f1b4870aa8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Josef=20Kol=C3=A1=C5=99?= Date: Sat, 7 Mar 2026 08:00:50 +0100 Subject: [PATCH 1/5] feat(notifications): add RFC 8058 one-click unsubscribe with signed tokens --- fiesta/apps/notifications/services/mailer.py | 24 +++- .../notifications/services/unsubscribe.py | 24 ++++ .../templates/notifications/base_email.html | 6 + .../templates/notifications/base_email.txt | 1 + .../templates/notifications/unsubscribe.html | 27 ++++ .../notifications/tests/test_unsubscribe.py | 96 ++++++++++++++ fiesta/apps/notifications/urls.py | 3 +- fiesta/apps/notifications/views.py | 122 +++++++++++++++++- 8 files changed, 296 insertions(+), 7 deletions(-) create mode 100644 fiesta/apps/notifications/services/unsubscribe.py create mode 100644 fiesta/apps/notifications/templates/notifications/unsubscribe.html create mode 100644 fiesta/apps/notifications/tests/test_unsubscribe.py diff --git a/fiesta/apps/notifications/services/mailer.py b/fiesta/apps/notifications/services/mailer.py index 17e598f5..47f8139a 100644 --- a/fiesta/apps/notifications/services/mailer.py +++ b/fiesta/apps/notifications/services/mailer.py @@ -9,6 +9,8 @@ from django.core.mail import EmailMultiAlternatives from django.template.loader import render_to_string +from apps.notifications.services.unsubscribe import generate_unsubscribe_token + logger = logging.getLogger(__name__) if typing.TYPE_CHECKING: @@ -37,19 +39,31 @@ def send_notification_email( except ObjectDoesNotExist: pass # No profile — proceed with sending - html_content = render_to_string(f"{template_prefix}.html", context) - text_content = render_to_string(f"{template_prefix}.txt", context) + 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}/" + + email_context["unsubscribe_url"] = unsubscribe_url - msg = EmailMultiAlternatives( + html_content = render_to_string(f"{template_prefix}.html", email_context) + text_content = render_to_string(f"{template_prefix}.txt", email_context) + + email = EmailMultiAlternatives( subject=subject, body=text_content, from_email=settings.DEFAULT_FROM_EMAIL, to=[recipient_email], ) - msg.attach_alternative(html_content, "text/html") + if unsubscribe_url: + email.extra_headers["List-Unsubscribe"] = f"<{unsubscribe_url}>" + email.extra_headers["List-Unsubscribe-Post"] = "List-Unsubscribe=One-Click" + + email.attach_alternative(html_content, "text/html") try: - msg.send() + email.send() except Exception: logger.exception( "Failed to send notification email (template: %s)", diff --git a/fiesta/apps/notifications/services/unsubscribe.py b/fiesta/apps/notifications/services/unsubscribe.py new file mode 100644 index 00000000..b3d1ae93 --- /dev/null +++ b/fiesta/apps/notifications/services/unsubscribe.py @@ -0,0 +1,24 @@ +from __future__ import annotations + +import logging + +from django.core.signing import BadSignature, TimestampSigner + +logger = logging.getLogger(__name__) + +UNSUBSCRIBE_SALT = "notifications-unsubscribe" + + +def generate_unsubscribe_token(user_id: int, action: str = "global") -> str: + signer = TimestampSigner(salt=UNSUBSCRIBE_SALT) + return signer.sign(f"{user_id}:{action}") + + +def verify_unsubscribe_token(token: str, max_age: int = 30 * 24 * 60 * 60) -> tuple[int, str]: + signer = TimestampSigner(salt=UNSUBSCRIBE_SALT) + value = signer.unsign(token, max_age=max_age) + try: + user_id_str, action = value.rsplit(":", 1) + return int(user_id_str), action + except (TypeError, ValueError) as exc: + raise BadSignature("Invalid unsubscribe payload") from exc diff --git a/fiesta/apps/notifications/templates/notifications/base_email.html b/fiesta/apps/notifications/templates/notifications/base_email.html index 711f1391..cf9aeead 100644 --- a/fiesta/apps/notifications/templates/notifications/base_email.html +++ b/fiesta/apps/notifications/templates/notifications/base_email.html @@ -38,6 +38,12 @@

{% if section %}You are receiving this email because you are a member of {{ section }}.{% endif %} {% if preferences_url %}Manage email preferences{% endif %} + {% if unsubscribe_url %} + | + Unsubscribe + {% endif %}

© Fiesta+ · fiesta.plus diff --git a/fiesta/apps/notifications/templates/notifications/base_email.txt b/fiesta/apps/notifications/templates/notifications/base_email.txt index ec8440ef..84b755f5 100644 --- a/fiesta/apps/notifications/templates/notifications/base_email.txt +++ b/fiesta/apps/notifications/templates/notifications/base_email.txt @@ -7,3 +7,4 @@ Fiesta+{% if section %} · {{ section }}{% endif %} You are receiving this email because you are a member of {{ section }}. {% if preferences_url %}Manage preferences: {{ preferences_url }}{% endif %} © Fiesta+ · https://fiesta.plus +{% if unsubscribe_url %}Unsubscribe: {{ unsubscribe_url }}{% endif %} diff --git a/fiesta/apps/notifications/templates/notifications/unsubscribe.html b/fiesta/apps/notifications/templates/notifications/unsubscribe.html new file mode 100644 index 00000000..5c2271d9 --- /dev/null +++ b/fiesta/apps/notifications/templates/notifications/unsubscribe.html @@ -0,0 +1,27 @@ +{% extends "fiesta/base.html" %} +{% load i18n %} + +{% block content %} +

+ {% if error %} +

{% trans "Unsubscribe Error" %}

+

{{ error }}

+ {% elif success %} +

{% trans "Unsubscribed" %}

+

+ {% blocktrans with action=action_description %}You have been successfully unsubscribed from {{ action }}.{% endblocktrans %} +

+ {% else %} +

{% trans "Unsubscribe from Notifications" %}

+

+ {% blocktrans with email=email action=action_description %} + You are about to unsubscribe {{ email }} from {{ action }}. + {% endblocktrans %} +

+
+ {% csrf_token %} + +
+ {% endif %} +
+{% endblock %} diff --git a/fiesta/apps/notifications/tests/test_unsubscribe.py b/fiesta/apps/notifications/tests/test_unsubscribe.py new file mode 100644 index 00000000..9705eae4 --- /dev/null +++ b/fiesta/apps/notifications/tests/test_unsubscribe.py @@ -0,0 +1,96 @@ +from __future__ import annotations + +from django.core import mail +from django.core.signing import BadSignature, SignatureExpired +from django.test import TestCase + +from apps.accounts.models import UserProfile +from apps.notifications.services.mailer import send_notification_email +from apps.notifications.services.unsubscribe import generate_unsubscribe_token, verify_unsubscribe_token +from apps.utils.factories.accounts import UserFactory + + +class UnsubscribeTokenServiceTestCase(TestCase): + def test_generate_unsubscribe_token_returns_string(self): + token = generate_unsubscribe_token(user_id=123) + self.assertIsInstance(token, str) + + def test_verify_unsubscribe_token_decodes_user_id_and_action(self): + token = generate_unsubscribe_token(user_id=321, action="global") + + user_id, action = verify_unsubscribe_token(token) + + self.assertEqual(user_id, 321) + self.assertEqual(action, "global") + + def test_verify_unsubscribe_token_raises_signature_expired(self): + token = generate_unsubscribe_token(user_id=123) + + with self.assertRaises(SignatureExpired): + verify_unsubscribe_token(token, max_age=-1) + + def test_verify_unsubscribe_token_raises_bad_signature_for_tampered_token(self): + token = generate_unsubscribe_token(user_id=123) + tampered_token = f"{token}tampered" + + with self.assertRaises(BadSignature): + verify_unsubscribe_token(tampered_token) + + +class UnsubscribeViewTestCase(TestCase): + def setUp(self): + self.user = UserFactory(profile=None) + self.profile = UserProfile.objects.create(user=self.user) + + def test_unsubscribe_post_sets_global_opt_out(self): + token = generate_unsubscribe_token(user_id=self.profile.pk, action="global") + + response = self.client.post(f"/notifications/unsubscribe/{token}/") + + self.profile.refresh_from_db() + self.assertEqual(response.status_code, 200) + self.assertFalse(self.profile.email_notifications_enabled) + + def test_unsubscribe_get_shows_confirmation_page(self): + token = generate_unsubscribe_token(user_id=self.profile.pk, action="global") + + response = self.client.get(f"/notifications/unsubscribe/{token}/") + + self.assertEqual(response.status_code, 200) + self.assertContains(response, "Unsubscribe from Notifications") + + def test_unsubscribe_post_is_idempotent_for_already_opted_out_user(self): + self.profile.email_notifications_enabled = False + self.profile.save(update_fields=["email_notifications_enabled", "modified"]) + token = generate_unsubscribe_token(user_id=self.profile.pk, action="global") + + response = self.client.post(f"/notifications/unsubscribe/{token}/") + + self.profile.refresh_from_db() + self.assertEqual(response.status_code, 200) + self.assertFalse(self.profile.email_notifications_enabled) + + +class NotificationMailerUnsubscribeHeadersTestCase(TestCase): + def test_sent_email_contains_list_unsubscribe_headers(self): + user = UserFactory(profile=None) + profile = UserProfile.objects.create(user=user) + + send_notification_email( + subject="Subject", + recipient_email=user.email, + template_prefix="notifications/base_email", + context={ + "section": "ESN Test", + "preferences_url": "https://example.com/preferences", + }, + recipient_profile=profile, + ) + + self.assertEqual(len(mail.outbox), 1) + sent_email = mail.outbox[0] + + self.assertIn("List-Unsubscribe", sent_email.extra_headers) + self.assertIn("List-Unsubscribe-Post", sent_email.extra_headers) + self.assertEqual(sent_email.extra_headers["List-Unsubscribe-Post"], "List-Unsubscribe=One-Click") + self.assertIn("/notifications/unsubscribe/", sent_email.extra_headers["List-Unsubscribe"]) diff --git a/fiesta/apps/notifications/urls.py b/fiesta/apps/notifications/urls.py index d93ea82c..c47c6738 100644 --- a/fiesta/apps/notifications/urls.py +++ b/fiesta/apps/notifications/urls.py @@ -2,10 +2,11 @@ from django.urls import path -from apps.notifications.views import NotificationPreferencesView +from apps.notifications.views import NotificationPreferencesView, UnsubscribeView app_name = "notifications" urlpatterns = [ path("preferences/", NotificationPreferencesView.as_view(), name="preferences"), + path("unsubscribe//", UnsubscribeView.as_view(), name="unsubscribe"), ] diff --git a/fiesta/apps/notifications/views.py b/fiesta/apps/notifications/views.py index 5c6bb6d2..203ba26e 100644 --- a/fiesta/apps/notifications/views.py +++ b/fiesta/apps/notifications/views.py @@ -1,15 +1,24 @@ from __future__ import annotations +import logging from typing import TYPE_CHECKING, Any from django.contrib.auth.mixins import LoginRequiredMixin from django.contrib.messages.views import SuccessMessageMixin +from django.core.signing import BadSignature, SignatureExpired +from django.http import HttpResponse +from django.shortcuts import render +from django.utils.decorators import method_decorator from django.utils.translation import gettext_lazy as _ +from django.views import View +from django.views.decorators.csrf import csrf_exempt from django.views.generic.edit import UpdateView +from apps.accounts.models import UserProfile from apps.buddy_system.apps import BuddySystemConfig from apps.notifications.forms import NotificationPreferencesForm -from apps.notifications.models import SectionNotificationPreferences +from apps.notifications.models import NotificationKind, SectionNotificationPreferences +from apps.notifications.services.unsubscribe import verify_unsubscribe_token from apps.pickup_system.apps import PickupSystemConfig from apps.plugins.views.mixins import CheckEnabledPluginsViewMixin from apps.sections.views.mixins.section_space import EnsureInSectionSpaceViewMixin @@ -17,6 +26,8 @@ if TYPE_CHECKING: from django.db.models import QuerySet +logger = logging.getLogger(__name__) + class NotificationPreferencesView( LoginRequiredMixin, @@ -59,3 +70,112 @@ def get_form(self, form_class: type[NotificationPreferencesForm] | None = None) ): form.fields.pop("notify_on_match", None) return form + + +@method_decorator(csrf_exempt, name="dispatch") +class UnsubscribeView(View): + template_name = "notifications/unsubscribe.html" + + def get(self, request: object, token: str) -> HttpResponse: + try: + user_profile, action = self._resolve_token(token) + except UserProfile.DoesNotExist: + return render( + request, + self.template_name, + {"error": _("This unsubscribe link is invalid.")}, + status=400, + ) + except SignatureExpired: + return render( + request, + self.template_name, + {"error": _("This unsubscribe link has expired.")}, + status=400, + ) + except BadSignature: + return render( + request, + self.template_name, + {"error": _("This unsubscribe link is invalid.")}, + status=400, + ) + + return render( + request, + self.template_name, + { + "action": action, + "action_description": self._action_description(action), + "email": user_profile.user.email, + }, + ) + + def post(self, request: object, token: str) -> HttpResponse: + try: + user_profile, action = self._resolve_token(token) + self._unsubscribe(user_profile=user_profile, action=action) + except UserProfile.DoesNotExist: + return render( + request, + self.template_name, + {"error": _("This unsubscribe link is invalid.")}, + status=200, + ) + except SignatureExpired: + return render( + request, + self.template_name, + {"error": _("This unsubscribe link has expired.")}, + status=200, + ) + except BadSignature: + return render( + request, + self.template_name, + {"error": _("This unsubscribe link is invalid.")}, + status=200, + ) + + return render( + request, + self.template_name, + { + "success": True, + "action": action, + "action_description": self._action_description(action), + }, + ) + + def _resolve_token(self, token: str) -> tuple[UserProfile, str]: + user_id, action = verify_unsubscribe_token(token) + return UserProfile.objects.select_related("user").get(pk=user_id), action + + def _unsubscribe(self, *, user_profile: UserProfile, action: str) -> None: + if action == "global": + if user_profile.email_notifications_enabled: + user_profile.email_notifications_enabled = False + user_profile.save(update_fields=["email_notifications_enabled", "modified"]) + return + + if action in {NotificationKind.BUDDY_MATCHED_ISSUER, NotificationKind.PICKUP_MATCHED_ISSUER}: + SectionNotificationPreferences.objects.filter(user=user_profile).update(notify_on_match=False) + return + + if action == NotificationKind.MEMBER_WAITING_DIGEST: + SectionNotificationPreferences.objects.filter(user=user_profile).update(notify_on_new_member_waiting=False) + return + + logger.warning("Unsupported unsubscribe action %r for user_profile=%s", action, user_profile.pk) + + def _action_description(self, action: str) -> str: + if action == "global": + return _("all email notifications") + + if action in {NotificationKind.BUDDY_MATCHED_ISSUER, NotificationKind.PICKUP_MATCHED_ISSUER}: + return _("match notification emails") + + if action == NotificationKind.MEMBER_WAITING_DIGEST: + return _("new member waiting digest emails") + + return _("selected email notifications") From 12768141b1d1b747d1ac0b6e58415673359c1c4e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Josef=20Kol=C3=A1=C5=99?= Date: Sat, 7 Mar 2026 08:12:09 +0100 Subject: [PATCH 2/5] feat(notifications): harden management command with batch-size, dry-run, and error resilience --- .../commands/send_scheduled_notifications.py | 43 ++++++- .../tests/test_scheduled_command.py | 105 +++++++++++++++--- 2 files changed, 131 insertions(+), 17 deletions(-) diff --git a/fiesta/apps/notifications/management/commands/send_scheduled_notifications.py b/fiesta/apps/notifications/management/commands/send_scheduled_notifications.py index e5747279..0e248207 100644 --- a/fiesta/apps/notifications/management/commands/send_scheduled_notifications.py +++ b/fiesta/apps/notifications/management/commands/send_scheduled_notifications.py @@ -26,9 +26,42 @@ class Command(BaseCommand): help = "Send all pending scheduled notifications whose send_after time has passed." + def add_arguments(self, parser: object) -> None: + parser.add_argument( + "--batch-size", + type=int, + default=100, + help="Maximum number of pending notifications to process in a single run.", + ) + parser.add_argument( + "--dry-run", + action="store_true", + help="Log pending notifications without sending or updating sent_at.", + ) + def handle(self, *args: object, **options: object) -> None: + batch_size = options["batch_size"] + dry_run = options["dry_run"] now = timezone.now() + # Dry-run: just log pending notifications without claiming them. + if dry_run: + pending_qs = ScheduledNotification.objects.filter( + send_after__lte=now, + sent_at__isnull=True, + cancelled_at__isnull=True, + ).select_related("recipient")[:batch_size] + would_send = 0 + for notification in pending_qs: + logger.info( + "Dry-run: would send scheduled notification pk=%s kind=%s", + notification.pk, + notification.kind, + ) + would_send += 1 + self.stdout.write(self.style.SUCCESS(f"Dry-run complete: would send {would_send} notification(s).")) + return + # 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). @@ -38,7 +71,10 @@ def handle(self, *args: object, **options: object) -> None: sent_at__isnull=True, cancelled_at__isnull=True, ) - reserved_ids: list[int] = list(pending_qs.values_list("pk", flat=True)) + pending_count = pending_qs.count() + logger.info("Found %d pending notifications (batch size: %d)", pending_count, batch_size) + + reserved_ids: list[int] = list(pending_qs.values_list("pk", flat=True)[:batch_size]) if reserved_ids: ScheduledNotification.objects.filter(pk__in=reserved_ids).update(sent_at=now) @@ -49,6 +85,7 @@ def handle(self, *args: object, **options: object) -> None: # Phase 2: Send each notification outside any transaction (no DB locks during I/O). sent = 0 skipped = 0 + failed = 0 for notification_pk in reserved_ids: try: @@ -76,9 +113,9 @@ def handle(self, *args: object, **options: object) -> None: logger.exception("Failed to send scheduled notification pk=%s", notification.pk) # Roll back the claim so the notification can be retried next run. ScheduledNotification.objects.filter(pk=notification_pk).update(sent_at=None) - skipped += 1 + failed += 1 - self.stdout.write(self.style.SUCCESS(f"Sent {sent} notification(s), skipped {skipped}.")) + self.stdout.write(self.style.SUCCESS(f"Sent {sent} notification(s), skipped {skipped}, failed {failed}.")) def _send(self, notification: ScheduledNotification) -> bool: # Resolve the related object via GenericFK diff --git a/fiesta/apps/notifications/tests/test_scheduled_command.py b/fiesta/apps/notifications/tests/test_scheduled_command.py index b1a381ed..e6eb03a3 100644 --- a/fiesta/apps/notifications/tests/test_scheduled_command.py +++ b/fiesta/apps/notifications/tests/test_scheduled_command.py @@ -9,7 +9,7 @@ from django.test import TestCase from django.utils import timezone -from apps.notifications.models import ScheduledNotification +from apps.notifications.models import NotificationKind, ScheduledNotification from apps.notifications.tests.factories import ScheduledNotificationFactory from apps.sections.models import Section from apps.utils.factories.accounts import UserFactory @@ -23,11 +23,12 @@ def setUp(self): self.section: Section = KnownSectionFactory() self.recipient = UserFactory(profile=None) - def _make_pending(self, send_after=None): + def _make_pending(self, kind=NotificationKind.MEMBER_WAITING_DIGEST, send_after=None): """Create a pending notification whose send_after is in the past by default.""" return ScheduledNotificationFactory( recipient=self.recipient, section=self.section, + kind=kind, send_after=send_after or (timezone.now() - timedelta(minutes=5)), ) @@ -101,7 +102,7 @@ def test_soft_cancels_on_deleted_object(self, mock_send): notification = ScheduledNotificationFactory( recipient=self.recipient, section=self.section, - kind=ScheduledNotification.Kind.BUDDY_MATCHED_ISSUER, + kind=NotificationKind.BUDDY_MATCHED_ISSUER, send_after=timezone.now() - timedelta(minutes=5), ) # Overwrite content_type/object_id to point to a deleted object @@ -117,22 +118,39 @@ def test_soft_cancels_on_deleted_object(self, mock_send): mock_send.assert_not_called() @patch("apps.notifications.management.commands.send_scheduled_notifications.send_notification_email") - def test_select_for_update_used(self, mock_send): - """ - Verify that the queryset uses select_for_update to prevent double-processing. - We test this by inspecting the command source and confirming the queryset chain - calls select_for_update before filter. - """ - import inspect + def test_select_for_update_called_with_skip_locked(self, mock_send): + """The command acquires row locks with skip_locked to support concurrent workers.""" + self._make_pending() + + with patch( + "apps.notifications.management.commands.send_scheduled_notifications.ScheduledNotification.objects.select_for_update", + wraps=ScheduledNotification.objects.select_for_update, + ) as mock_select_for_update: + call_command(COMMAND_NAME) + + mock_select_for_update.assert_called_once_with(skip_locked=True) + + @patch("apps.notifications.management.commands.send_scheduled_notifications.send_notification_email") + def test_dry_run_logs_without_sending_or_marking_sent(self, mock_send): + """Dry-run logs pending notifications but does not send or persist sent_at.""" + notification = self._make_pending() - from apps.notifications.management.commands.send_scheduled_notifications import Command + with self.assertLogs( + "apps.notifications.management.commands.send_scheduled_notifications", + level="INFO", + ) as captured_logs: + call_command(COMMAND_NAME, dry_run=True) - source = inspect.getsource(Command.handle) - self.assertIn("select_for_update", source) + notification.refresh_from_db() + + self.assertIsNone(notification.sent_at) + self.assertIsNone(notification.cancelled_at) + self.assertIn("Dry-run: would send scheduled notification", "\n".join(captured_logs.output)) + mock_send.assert_not_called() @patch("apps.notifications.management.commands.send_scheduled_notifications.send_notification_email") def test_send_failure_does_not_mark_sent(self, mock_send): - """When send_notification_email raises, sent_at must remain None.""" + """When send_notification_email raises, sent_at must remain None (claim is rolled back).""" mock_send.side_effect = RuntimeError("SMTP connection failed") notification = self._make_pending() @@ -160,3 +178,62 @@ def test_partial_failure_sends_remaining(self, mock_send): failed_count = sum(1 for n in [n1, n2] if n.sent_at is None) self.assertEqual(sent_count, 1) self.assertEqual(failed_count, 1) + + @patch("apps.notifications.management.commands.send_scheduled_notifications.send_notification_email") + def test_send_failure_logs_error_and_continues(self, mock_send): + """A send exception is logged and processing continues to the next notification.""" + first_recipient = UserFactory(profile=None) + second_recipient = UserFactory(profile=None) + first = ScheduledNotificationFactory( + recipient=first_recipient, + section=self.section, + kind=NotificationKind.MEMBER_WAITING_DIGEST, + send_after=timezone.now() - timedelta(minutes=5), + ) + second = ScheduledNotificationFactory( + recipient=second_recipient, + section=self.section, + kind=NotificationKind.MEMBER_WAITING_DIGEST, + send_after=timezone.now() - timedelta(minutes=5), + ) + + def _side_effect(*, recipient_email, **kwargs): + if recipient_email == first_recipient.email: + raise Exception("SMTP exploded") + return + + mock_send.side_effect = _side_effect + + with self.assertLogs( + "apps.notifications.management.commands.send_scheduled_notifications", + level="ERROR", + ) as captured_logs: + call_command(COMMAND_NAME) + + first.refresh_from_db() + second.refresh_from_db() + + self.assertIsNone(first.sent_at) + self.assertIsNotNone(second.sent_at) + self.assertEqual(mock_send.call_count, 2) + self.assertIn("Failed to send scheduled notification pk=", "\n".join(captured_logs.output)) + + @patch("apps.notifications.management.commands.send_scheduled_notifications.send_notification_email") + def test_batch_size_limits_processed_notifications(self, mock_send): + """Batch size limits how many eligible notifications are sent in one run.""" + first = self._make_pending() + second = self._make_pending() + third = self._make_pending() + + call_command(COMMAND_NAME, batch_size=2) + + first.refresh_from_db() + second.refresh_from_db() + third.refresh_from_db() + + sent_count = ScheduledNotification.objects.filter( + pk__in=[first.pk, second.pk, third.pk], + sent_at__isnull=False, + ).count() + self.assertEqual(sent_count, 2) + self.assertEqual(mock_send.call_count, 2) From 06247648be25d6cc34c682edceb256e22ae29812 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Josef=20Kol=C3=A1=C5=99?= Date: Sun, 8 Mar 2026 10:23:23 +0100 Subject: [PATCH 3/5] chore(notifications): align models, factories, and tests with User-based recipient --- ...otificationpreferences_options_and_more.py | 35 +++++++++++++++++++ fiesta/apps/notifications/models/scheduled.py | 5 ++- .../apps/notifications/services/scheduler.py | 8 +++-- .../notifications/services/unsubscribe.py | 6 ++-- fiesta/apps/notifications/tests/factories.py | 15 ++++++-- .../tests/test_match_notifications.py | 14 ++++++-- .../tests/test_membership_notifications.py | 11 ++++-- .../tests/test_scheduled_command.py | 14 ++++++-- .../notifications/tests/test_scheduler.py | 30 ++++++++++------ .../notifications/tests/test_unsubscribe.py | 18 +++++++--- 10 files changed, 124 insertions(+), 32 deletions(-) create mode 100644 fiesta/apps/notifications/migrations/0002_alter_sectionnotificationpreferences_options_and_more.py diff --git a/fiesta/apps/notifications/migrations/0002_alter_sectionnotificationpreferences_options_and_more.py b/fiesta/apps/notifications/migrations/0002_alter_sectionnotificationpreferences_options_and_more.py new file mode 100644 index 00000000..579d6282 --- /dev/null +++ b/fiesta/apps/notifications/migrations/0002_alter_sectionnotificationpreferences_options_and_more.py @@ -0,0 +1,35 @@ +# Generated by Django 4.2.28 on 2026-03-07 07:49 + +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + dependencies = [ + ('contenttypes', '0002_remove_content_type_name'), + ('sections', '0024_initial'), + ('notifications', '0001_initial'), + ] + + operations = [ + migrations.AlterModelOptions( + name='sectionnotificationpreferences', + options={'verbose_name': 'section notification preference', 'verbose_name_plural': 'section notification preferences'}, + ), + migrations.AlterField( + model_name='schedulednotification', + name='content_type', + field=models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.SET_NULL, to='contenttypes.contenttype', verbose_name='content type'), + ), + migrations.AlterField( + model_name='schedulednotification', + name='kind', + field=models.CharField(choices=[('buddy_matched_issuer', 'Buddy matched — issuer'), ('pickup_matched_issuer', 'Pickup matched — issuer'), ('member_waiting_digest', 'New member waiting digest')], max_length=64, verbose_name='kind'), + ), + migrations.AlterField( + model_name='schedulednotification', + name='section', + field=models.ForeignKey(blank=True, null=True, on_delete=django.db.models.deletion.CASCADE, related_name='scheduled_notifications', to='sections.section', verbose_name='section'), + ), + ] diff --git a/fiesta/apps/notifications/models/scheduled.py b/fiesta/apps/notifications/models/scheduled.py index f3a9938b..6813ac2f 100644 --- a/fiesta/apps/notifications/models/scheduled.py +++ b/fiesta/apps/notifications/models/scheduled.py @@ -1,5 +1,6 @@ from __future__ import annotations +from django.conf import settings from django.contrib.contenttypes.fields import GenericForeignKey from django.contrib.contenttypes.models import ContentType from django.db import models @@ -32,7 +33,7 @@ class ScheduledNotification(BaseTimestampedModel): ) recipient = models.ForeignKey( - "accounts.User", + settings.AUTH_USER_MODEL, on_delete=models.CASCADE, related_name="scheduled_notifications", verbose_name=_("recipient"), @@ -40,6 +41,8 @@ class ScheduledNotification(BaseTimestampedModel): section = models.ForeignKey( "sections.Section", on_delete=models.CASCADE, + null=True, + blank=True, related_name="scheduled_notifications", verbose_name=_("section"), ) diff --git a/fiesta/apps/notifications/services/scheduler.py b/fiesta/apps/notifications/services/scheduler.py index 8cd438bf..bc3de1a0 100644 --- a/fiesta/apps/notifications/services/scheduler.py +++ b/fiesta/apps/notifications/services/scheduler.py @@ -1,6 +1,7 @@ from __future__ import annotations import logging +from contextlib import suppress from datetime import datetime from typing import TYPE_CHECKING @@ -36,9 +37,10 @@ def enqueue_delayed_notification( Uses select_for_update to prevent duplicate rows from concurrent requests. """ - if hasattr(recipient, "profile") and not recipient.profile.email_notifications_enabled: - logger.info("Skipping scheduled notification for %s: global opt-out", recipient) - return None + with suppress(AttributeError): + if not recipient.profile.email_notifications_enabled: + logger.info("Skipping scheduled notification for %s: global opt-out", recipient) + return None ct = ContentType.objects.get_for_model(related_object) diff --git a/fiesta/apps/notifications/services/unsubscribe.py b/fiesta/apps/notifications/services/unsubscribe.py index b3d1ae93..9adf0072 100644 --- a/fiesta/apps/notifications/services/unsubscribe.py +++ b/fiesta/apps/notifications/services/unsubscribe.py @@ -9,16 +9,16 @@ UNSUBSCRIBE_SALT = "notifications-unsubscribe" -def generate_unsubscribe_token(user_id: int, action: str = "global") -> str: +def generate_unsubscribe_token(user_id: int | str, action: str = "global") -> str: signer = TimestampSigner(salt=UNSUBSCRIBE_SALT) return signer.sign(f"{user_id}:{action}") -def verify_unsubscribe_token(token: str, max_age: int = 30 * 24 * 60 * 60) -> tuple[int, str]: +def verify_unsubscribe_token(token: str, max_age: int = 30 * 24 * 60 * 60) -> tuple[str, str]: signer = TimestampSigner(salt=UNSUBSCRIBE_SALT) value = signer.unsign(token, max_age=max_age) try: user_id_str, action = value.rsplit(":", 1) - return int(user_id_str), action + return user_id_str, action except (TypeError, ValueError) as exc: raise BadSignature("Invalid unsubscribe payload") from exc diff --git a/fiesta/apps/notifications/tests/factories.py b/fiesta/apps/notifications/tests/factories.py index e8a5ab75..00b4f2ff 100644 --- a/fiesta/apps/notifications/tests/factories.py +++ b/fiesta/apps/notifications/tests/factories.py @@ -7,7 +7,16 @@ from django.utils import timezone from factory.django import DjangoModelFactory -from apps.notifications.models import ScheduledNotification, SectionNotificationPreferences +from apps.accounts.models import UserProfile +from apps.notifications.models import NotificationKind, ScheduledNotification, SectionNotificationPreferences + + +def _create_profile(_obj): + """Create a UserProfile for a fresh user, avoiding the broken UserProfileFactory.""" + from apps.utils.factories.accounts import UserFactory + + user = UserFactory(profile=None) + return UserProfile.objects.create(user=user) class ScheduledNotificationFactory(DjangoModelFactory): @@ -16,8 +25,8 @@ class ScheduledNotificationFactory(DjangoModelFactory): class Meta: model = ScheduledNotification - kind = ScheduledNotification.Kind.BUDDY_MATCHED_ISSUER - recipient = factory.SubFactory("apps.utils.factories.accounts.UserFactory") + kind = NotificationKind.BUDDY_MATCHED_ISSUER + recipient = factory.SubFactory("apps.utils.factories.accounts.UserFactory", profile=None) section = factory.SubFactory("apps.utils.factories.sections.KnownSectionFactory") # Generic FK fields — point to a section by default (simple object that always exists) diff --git a/fiesta/apps/notifications/tests/test_match_notifications.py b/fiesta/apps/notifications/tests/test_match_notifications.py index feb07a1d..b813a14e 100644 --- a/fiesta/apps/notifications/tests/test_match_notifications.py +++ b/fiesta/apps/notifications/tests/test_match_notifications.py @@ -6,13 +6,19 @@ from django.test import TestCase -from apps.notifications.models import ScheduledNotification +from apps.accounts.models import UserProfile +from apps.notifications.models import NotificationKind, ScheduledNotification from apps.notifications.services.match import notify_buddy_match, notify_pickup_match from apps.notifications.tests.factories import SectionNotificationPreferencesFactory from apps.utils.factories.accounts import UserFactory from apps.utils.factories.sections import KnownSectionFactory +def _create_user_profile(user): + """Ensure user has a UserProfile attached.""" + UserProfile.objects.get_or_create(user=user) + + def _make_buddy_config(notify=True, delay=timedelta(hours=1)): """Return a mock BuddySystemConfiguration with the fields used by the service.""" config = MagicMock() @@ -59,7 +65,9 @@ class NotifyBuddyMatchTestCase(TestCase): def setUp(self): self.base_section = KnownSectionFactory() self.matcher = UserFactory(profile=None) + _create_user_profile(self.matcher) self.issuer = UserFactory(profile=None) + _create_user_profile(self.issuer) self.match, self.request_obj = _make_match_and_request(self.matcher, self.issuer) @patch("apps.notifications.services.match.send_notification_email") @@ -87,7 +95,7 @@ def test_buddy_match_enqueues_issuer_notification(self, mock_send, mock_enqueue) mock_enqueue.assert_called_once() call_kwargs = mock_enqueue.call_args.kwargs - self.assertEqual(call_kwargs["kind"], ScheduledNotification.Kind.BUDDY_MATCHED_ISSUER) + self.assertEqual(call_kwargs["kind"], NotificationKind.BUDDY_MATCHED_ISSUER) self.assertEqual(call_kwargs["recipient"], self.issuer) self.assertEqual(call_kwargs["related_object"], self.match) @@ -156,7 +164,9 @@ class NotifyPickupMatchTestCase(TestCase): def setUp(self): self.base_section = KnownSectionFactory() self.matcher = UserFactory(profile=None) + _create_user_profile(self.matcher) self.issuer = UserFactory(profile=None) + _create_user_profile(self.issuer) self.match, self.request_obj = _make_match_and_request(self.matcher, self.issuer) @patch("apps.notifications.services.match.send_notification_email") diff --git a/fiesta/apps/notifications/tests/test_membership_notifications.py b/fiesta/apps/notifications/tests/test_membership_notifications.py index 1913583b..8fd5e1e6 100644 --- a/fiesta/apps/notifications/tests/test_membership_notifications.py +++ b/fiesta/apps/notifications/tests/test_membership_notifications.py @@ -5,7 +5,8 @@ from django.test import TestCase -from apps.notifications.models import ScheduledNotification +from apps.accounts.models import UserProfile +from apps.notifications.models import NotificationKind, ScheduledNotification from apps.notifications.services.membership import notify_new_membership from apps.notifications.tests.factories import SectionNotificationPreferencesFactory from apps.sections.models import SectionMembership @@ -13,6 +14,11 @@ from apps.utils.factories.sections import KnownSectionFactory, SectionMembershipWithUserFactory +def _create_user_profile(user): + """Ensure user has a UserProfile attached.""" + UserProfile.objects.get_or_create(user=user) + + def _make_sections_config( notify_member=True, notify_on_new_member=True, @@ -30,6 +36,7 @@ class NotifyNewMembershipTestCase(TestCase): def setUp(self): self.section = KnownSectionFactory() self.applicant = UserFactory(profile=None) + _create_user_profile(self.applicant) self.membership = SectionMembershipWithUserFactory( section=self.section, user=self.applicant, @@ -78,7 +85,7 @@ def test_enqueues_digest_for_editors(self, mock_send, mock_enqueue): # Both editor and admin should have enqueue called self.assertEqual(mock_enqueue.call_count, 2) for call in mock_enqueue.call_args_list: - self.assertEqual(call.kwargs["kind"], ScheduledNotification.Kind.MEMBER_WAITING_DIGEST) + self.assertEqual(call.kwargs["kind"], NotificationKind.MEMBER_WAITING_DIGEST) self.assertEqual(call.kwargs["related_object"], self.membership) @patch("apps.notifications.services.scheduler.enqueue_delayed_notification") diff --git a/fiesta/apps/notifications/tests/test_scheduled_command.py b/fiesta/apps/notifications/tests/test_scheduled_command.py index e6eb03a3..7adcc3c1 100644 --- a/fiesta/apps/notifications/tests/test_scheduled_command.py +++ b/fiesta/apps/notifications/tests/test_scheduled_command.py @@ -9,6 +9,7 @@ from django.test import TestCase from django.utils import timezone +from apps.accounts.models import UserProfile from apps.notifications.models import NotificationKind, ScheduledNotification from apps.notifications.tests.factories import ScheduledNotificationFactory from apps.sections.models import Section @@ -18,10 +19,17 @@ COMMAND_NAME = "send_scheduled_notifications" +def _make_user_with_profile(): + """Create a User with an attached UserProfile, avoiding the broken UserProfileFactory.""" + user = UserFactory(profile=None) + UserProfile.objects.create(user=user) + return user + + class SendScheduledNotificationsTestCase(TestCase): def setUp(self): self.section: Section = KnownSectionFactory() - self.recipient = UserFactory(profile=None) + self.recipient = _make_user_with_profile() def _make_pending(self, kind=NotificationKind.MEMBER_WAITING_DIGEST, send_after=None): """Create a pending notification whose send_after is in the past by default.""" @@ -182,8 +190,8 @@ def test_partial_failure_sends_remaining(self, mock_send): @patch("apps.notifications.management.commands.send_scheduled_notifications.send_notification_email") def test_send_failure_logs_error_and_continues(self, mock_send): """A send exception is logged and processing continues to the next notification.""" - first_recipient = UserFactory(profile=None) - second_recipient = UserFactory(profile=None) + first_recipient = _make_user_with_profile() + second_recipient = _make_user_with_profile() first = ScheduledNotificationFactory( recipient=first_recipient, section=self.section, diff --git a/fiesta/apps/notifications/tests/test_scheduler.py b/fiesta/apps/notifications/tests/test_scheduler.py index 1266dc86..c04e7935 100644 --- a/fiesta/apps/notifications/tests/test_scheduler.py +++ b/fiesta/apps/notifications/tests/test_scheduler.py @@ -5,7 +5,8 @@ from django.test import TestCase from django.utils import timezone -from apps.notifications.models import ScheduledNotification +from apps.accounts.models import UserProfile +from apps.notifications.models import NotificationKind, ScheduledNotification from apps.notifications.services.scheduler import cancel_scheduled_notifications_for, enqueue_delayed_notification from apps.notifications.tests.factories import ScheduledNotificationFactory from apps.sections.models import Section @@ -13,16 +14,23 @@ from apps.utils.factories.sections import KnownSectionFactory +def _make_user_with_profile(): + """Create a User with an attached UserProfile, avoiding the broken UserProfileFactory.""" + user = UserFactory(profile=None) + UserProfile.objects.create(user=user) + return user + + class EnqueueDelayedNotificationTestCase(TestCase): def setUp(self): - self.user = UserFactory(profile=None) + self.user = _make_user_with_profile() self.section: Section = KnownSectionFactory() self.send_after = timezone.now() + timedelta(hours=1) def test_enqueue_creates_new_notification(self): """Calling enqueue creates a ScheduledNotification with the correct fields.""" notification = enqueue_delayed_notification( - kind=ScheduledNotification.Kind.BUDDY_MATCHED_ISSUER, + kind=NotificationKind.BUDDY_MATCHED_ISSUER, recipient=self.user, section=self.section, related_object=self.section, # use Section as a simple related_object @@ -30,7 +38,7 @@ def test_enqueue_creates_new_notification(self): ) self.assertIsNotNone(notification.pk) - self.assertEqual(notification.kind, ScheduledNotification.Kind.BUDDY_MATCHED_ISSUER) + self.assertEqual(notification.kind, NotificationKind.BUDDY_MATCHED_ISSUER) self.assertEqual(notification.recipient, self.user) self.assertEqual(notification.section, self.section) self.assertEqual(notification.send_after, self.send_after) @@ -40,7 +48,7 @@ def test_enqueue_creates_new_notification(self): def test_enqueue_upserts_existing_pending(self): """Calling enqueue again for the same recipient+object updates send_after instead of creating a duplicate.""" first = enqueue_delayed_notification( - kind=ScheduledNotification.Kind.BUDDY_MATCHED_ISSUER, + kind=NotificationKind.BUDDY_MATCHED_ISSUER, recipient=self.user, section=self.section, related_object=self.section, @@ -49,7 +57,7 @@ def test_enqueue_upserts_existing_pending(self): later_time = self.send_after + timedelta(hours=2) second = enqueue_delayed_notification( - kind=ScheduledNotification.Kind.BUDDY_MATCHED_ISSUER, + kind=NotificationKind.BUDDY_MATCHED_ISSUER, recipient=self.user, section=self.section, related_object=self.section, @@ -66,7 +74,7 @@ def test_enqueue_does_not_upsert_sent(self): """If an existing notification is already sent, a new one is created instead of updating.""" # Create an already-sent notification sent_notification = ScheduledNotificationFactory( - kind=ScheduledNotification.Kind.BUDDY_MATCHED_ISSUER, + kind=NotificationKind.BUDDY_MATCHED_ISSUER, recipient=self.user, section=self.section, object_id=self.section.pk, @@ -79,7 +87,7 @@ def test_enqueue_does_not_upsert_sent(self): sent_notification.save() new_notification = enqueue_delayed_notification( - kind=ScheduledNotification.Kind.BUDDY_MATCHED_ISSUER, + kind=NotificationKind.BUDDY_MATCHED_ISSUER, recipient=self.user, section=self.section, related_object=self.section, @@ -97,7 +105,7 @@ def setUp(self): def test_cancel_cancels_pending(self): """cancel_scheduled_notifications_for marks pending notifications as cancelled.""" - user = UserFactory(profile=None) + user = _make_user_with_profile() notification = ScheduledNotificationFactory( recipient=user, section=self.section, @@ -116,7 +124,7 @@ def test_cancel_cancels_pending(self): def test_cancel_skips_already_sent(self): """cancel_scheduled_notifications_for does not touch notifications that are already sent.""" - user = UserFactory(profile=None) + user = _make_user_with_profile() from django.contrib.contenttypes.models import ContentType notification = ScheduledNotificationFactory( @@ -139,7 +147,7 @@ def test_cancel_returns_count(self): from django.contrib.contenttypes.models import ContentType ct = ContentType.objects.get_for_model(self.section) - users = [UserFactory(profile=None) for _ in range(3)] + users = [_make_user_with_profile() for _ in range(3)] for user in users: n = ScheduledNotificationFactory( diff --git a/fiesta/apps/notifications/tests/test_unsubscribe.py b/fiesta/apps/notifications/tests/test_unsubscribe.py index 9705eae4..a966f79d 100644 --- a/fiesta/apps/notifications/tests/test_unsubscribe.py +++ b/fiesta/apps/notifications/tests/test_unsubscribe.py @@ -1,5 +1,6 @@ from __future__ import annotations +from django.conf import settings from django.core import mail from django.core.signing import BadSignature, SignatureExpired from django.test import TestCase @@ -20,7 +21,7 @@ def test_verify_unsubscribe_token_decodes_user_id_and_action(self): user_id, action = verify_unsubscribe_token(token) - self.assertEqual(user_id, 321) + self.assertEqual(user_id, "321") self.assertEqual(action, "global") def test_verify_unsubscribe_token_raises_signature_expired(self): @@ -45,7 +46,10 @@ def setUp(self): def test_unsubscribe_post_sets_global_opt_out(self): token = generate_unsubscribe_token(user_id=self.profile.pk, action="global") - response = self.client.post(f"/notifications/unsubscribe/{token}/") + response = self.client.post( + f"/notifications/unsubscribe/{token}/", + SERVER_NAME=settings.ROOT_DOMAIN, + ) self.profile.refresh_from_db() self.assertEqual(response.status_code, 200) @@ -54,7 +58,10 @@ def test_unsubscribe_post_sets_global_opt_out(self): def test_unsubscribe_get_shows_confirmation_page(self): token = generate_unsubscribe_token(user_id=self.profile.pk, action="global") - response = self.client.get(f"/notifications/unsubscribe/{token}/") + response = self.client.get( + f"/notifications/unsubscribe/{token}/", + SERVER_NAME=settings.ROOT_DOMAIN, + ) self.assertEqual(response.status_code, 200) self.assertContains(response, "Unsubscribe from Notifications") @@ -64,7 +71,10 @@ def test_unsubscribe_post_is_idempotent_for_already_opted_out_user(self): self.profile.save(update_fields=["email_notifications_enabled", "modified"]) token = generate_unsubscribe_token(user_id=self.profile.pk, action="global") - response = self.client.post(f"/notifications/unsubscribe/{token}/") + response = self.client.post( + f"/notifications/unsubscribe/{token}/", + SERVER_NAME=settings.ROOT_DOMAIN, + ) self.profile.refresh_from_db() self.assertEqual(response.status_code, 200) From 540b0bb84591f65f95c9b9b7c15b97c76eb94083 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Josef=20Kol=C3=A1=C5=99?= Date: Sun, 8 Mar 2026 11:00:23 +0100 Subject: [PATCH 4/5] fix(notifications): resolve unsubscribe token/PK mismatch and template block name --- .../templates/notifications/unsubscribe.html | 5 ++--- fiesta/apps/notifications/tests/test_unsubscribe.py | 10 +++++----- fiesta/apps/notifications/views.py | 8 +++++--- 3 files changed, 12 insertions(+), 11 deletions(-) diff --git a/fiesta/apps/notifications/templates/notifications/unsubscribe.html b/fiesta/apps/notifications/templates/notifications/unsubscribe.html index 5c2271d9..8263ee0c 100644 --- a/fiesta/apps/notifications/templates/notifications/unsubscribe.html +++ b/fiesta/apps/notifications/templates/notifications/unsubscribe.html @@ -1,7 +1,7 @@ {% extends "fiesta/base.html" %} {% load i18n %} -{% block content %} +{% block main %}
{% if error %}

{% trans "Unsubscribe Error" %}

@@ -19,9 +19,8 @@

{% trans "Unsubscribe from Notifications" %} {% endblocktrans %}

- {% csrf_token %}
{% endif %}

-{% endblock %} +{% endblock main %} diff --git a/fiesta/apps/notifications/tests/test_unsubscribe.py b/fiesta/apps/notifications/tests/test_unsubscribe.py index a966f79d..b0fa3997 100644 --- a/fiesta/apps/notifications/tests/test_unsubscribe.py +++ b/fiesta/apps/notifications/tests/test_unsubscribe.py @@ -44,7 +44,7 @@ def setUp(self): self.profile = UserProfile.objects.create(user=self.user) def test_unsubscribe_post_sets_global_opt_out(self): - token = generate_unsubscribe_token(user_id=self.profile.pk, action="global") + token = generate_unsubscribe_token(user_id=self.user.pk, action="global") response = self.client.post( f"/notifications/unsubscribe/{token}/", @@ -56,7 +56,7 @@ def test_unsubscribe_post_sets_global_opt_out(self): self.assertFalse(self.profile.email_notifications_enabled) def test_unsubscribe_get_shows_confirmation_page(self): - token = generate_unsubscribe_token(user_id=self.profile.pk, action="global") + token = generate_unsubscribe_token(user_id=self.user.pk, action="global") response = self.client.get( f"/notifications/unsubscribe/{token}/", @@ -69,7 +69,7 @@ def test_unsubscribe_get_shows_confirmation_page(self): def test_unsubscribe_post_is_idempotent_for_already_opted_out_user(self): self.profile.email_notifications_enabled = False self.profile.save(update_fields=["email_notifications_enabled", "modified"]) - token = generate_unsubscribe_token(user_id=self.profile.pk, action="global") + token = generate_unsubscribe_token(user_id=self.user.pk, action="global") response = self.client.post( f"/notifications/unsubscribe/{token}/", @@ -84,7 +84,7 @@ def test_unsubscribe_post_is_idempotent_for_already_opted_out_user(self): class NotificationMailerUnsubscribeHeadersTestCase(TestCase): def test_sent_email_contains_list_unsubscribe_headers(self): user = UserFactory(profile=None) - profile = UserProfile.objects.create(user=user) + UserProfile.objects.create(user=user) send_notification_email( subject="Subject", @@ -94,7 +94,7 @@ def test_sent_email_contains_list_unsubscribe_headers(self): "section": "ESN Test", "preferences_url": "https://example.com/preferences", }, - recipient_profile=profile, + recipient_user=user, ) self.assertEqual(len(mail.outbox), 1) diff --git a/fiesta/apps/notifications/views.py b/fiesta/apps/notifications/views.py index 203ba26e..ffc6355d 100644 --- a/fiesta/apps/notifications/views.py +++ b/fiesta/apps/notifications/views.py @@ -149,7 +149,7 @@ def post(self, request: object, token: str) -> HttpResponse: def _resolve_token(self, token: str) -> tuple[UserProfile, str]: user_id, action = verify_unsubscribe_token(token) - return UserProfile.objects.select_related("user").get(pk=user_id), action + return UserProfile.objects.select_related("user").get(user_id=user_id), action def _unsubscribe(self, *, user_profile: UserProfile, action: str) -> None: if action == "global": @@ -159,11 +159,13 @@ def _unsubscribe(self, *, user_profile: UserProfile, action: str) -> None: return if action in {NotificationKind.BUDDY_MATCHED_ISSUER, NotificationKind.PICKUP_MATCHED_ISSUER}: - SectionNotificationPreferences.objects.filter(user=user_profile).update(notify_on_match=False) + SectionNotificationPreferences.objects.filter(user=user_profile.user).update(notify_on_match=False) return if action == NotificationKind.MEMBER_WAITING_DIGEST: - SectionNotificationPreferences.objects.filter(user=user_profile).update(notify_on_new_member_waiting=False) + SectionNotificationPreferences.objects.filter(user=user_profile.user).update( + notify_on_new_member_waiting=False + ) return logger.warning("Unsupported unsubscribe action %r for user_profile=%s", action, user_profile.pk) From 8ab3e2271dbde0c6f882289a6168c0c553341756 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Josef=20Kol=C3=A1=C5=99?= Date: Sun, 8 Mar 2026 11:58:43 +0100 Subject: [PATCH 5/5] fix(notifications): address PR #360 review findings (status codes, typing, lock contention, dead code) --- .../commands/send_scheduled_notifications.py | 16 ++++++++++------ .../apps/notifications/services/unsubscribe.py | 4 ---- fiesta/apps/notifications/tests/factories.py | 9 --------- fiesta/apps/notifications/views.py | 12 ++++++------ 4 files changed, 16 insertions(+), 25 deletions(-) diff --git a/fiesta/apps/notifications/management/commands/send_scheduled_notifications.py b/fiesta/apps/notifications/management/commands/send_scheduled_notifications.py index 0e248207..0427bfda 100644 --- a/fiesta/apps/notifications/management/commands/send_scheduled_notifications.py +++ b/fiesta/apps/notifications/management/commands/send_scheduled_notifications.py @@ -66,13 +66,15 @@ def handle(self, *args: object, **options: object) -> None: # 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(): - pending_qs = ScheduledNotification.objects.select_for_update(skip_locked=True).filter( - send_after__lte=now, - sent_at__isnull=True, - cancelled_at__isnull=True, + pending_qs = ( + ScheduledNotification.objects.select_for_update(skip_locked=True) + .filter( + send_after__lte=now, + sent_at__isnull=True, + cancelled_at__isnull=True, + ) + .order_by("send_after", "pk") ) - pending_count = pending_qs.count() - logger.info("Found %d pending notifications (batch size: %d)", pending_count, batch_size) reserved_ids: list[int] = list(pending_qs.values_list("pk", flat=True)[:batch_size]) if reserved_ids: @@ -82,6 +84,8 @@ def handle(self, *args: object, **options: object) -> None: self.stdout.write("No pending notifications.") return + logger.info("Claimed %d notification(s) for sending (batch size: %d)", len(reserved_ids), batch_size) + # Phase 2: Send each notification outside any transaction (no DB locks during I/O). sent = 0 skipped = 0 diff --git a/fiesta/apps/notifications/services/unsubscribe.py b/fiesta/apps/notifications/services/unsubscribe.py index 9adf0072..1352518f 100644 --- a/fiesta/apps/notifications/services/unsubscribe.py +++ b/fiesta/apps/notifications/services/unsubscribe.py @@ -1,11 +1,7 @@ from __future__ import annotations -import logging - from django.core.signing import BadSignature, TimestampSigner -logger = logging.getLogger(__name__) - UNSUBSCRIBE_SALT = "notifications-unsubscribe" diff --git a/fiesta/apps/notifications/tests/factories.py b/fiesta/apps/notifications/tests/factories.py index 00b4f2ff..074d0f3d 100644 --- a/fiesta/apps/notifications/tests/factories.py +++ b/fiesta/apps/notifications/tests/factories.py @@ -7,18 +7,9 @@ from django.utils import timezone from factory.django import DjangoModelFactory -from apps.accounts.models import UserProfile from apps.notifications.models import NotificationKind, ScheduledNotification, SectionNotificationPreferences -def _create_profile(_obj): - """Create a UserProfile for a fresh user, avoiding the broken UserProfileFactory.""" - from apps.utils.factories.accounts import UserFactory - - user = UserFactory(profile=None) - return UserProfile.objects.create(user=user) - - class ScheduledNotificationFactory(DjangoModelFactory): """Creates a pending (not yet sent, not cancelled) ScheduledNotification.""" diff --git a/fiesta/apps/notifications/views.py b/fiesta/apps/notifications/views.py index ffc6355d..a1ce4d21 100644 --- a/fiesta/apps/notifications/views.py +++ b/fiesta/apps/notifications/views.py @@ -6,7 +6,7 @@ from django.contrib.auth.mixins import LoginRequiredMixin from django.contrib.messages.views import SuccessMessageMixin from django.core.signing import BadSignature, SignatureExpired -from django.http import HttpResponse +from django.http import HttpRequest, HttpResponse from django.shortcuts import render from django.utils.decorators import method_decorator from django.utils.translation import gettext_lazy as _ @@ -76,7 +76,7 @@ def get_form(self, form_class: type[NotificationPreferencesForm] | None = None) class UnsubscribeView(View): template_name = "notifications/unsubscribe.html" - def get(self, request: object, token: str) -> HttpResponse: + def get(self, request: HttpRequest, token: str) -> HttpResponse: try: user_profile, action = self._resolve_token(token) except UserProfile.DoesNotExist: @@ -111,7 +111,7 @@ def get(self, request: object, token: str) -> HttpResponse: }, ) - def post(self, request: object, token: str) -> HttpResponse: + def post(self, request: HttpRequest, token: str) -> HttpResponse: try: user_profile, action = self._resolve_token(token) self._unsubscribe(user_profile=user_profile, action=action) @@ -120,21 +120,21 @@ def post(self, request: object, token: str) -> HttpResponse: request, self.template_name, {"error": _("This unsubscribe link is invalid.")}, - status=200, + status=400, ) except SignatureExpired: return render( request, self.template_name, {"error": _("This unsubscribe link has expired.")}, - status=200, + status=400, ) except BadSignature: return render( request, self.template_name, {"error": _("This unsubscribe link is invalid.")}, - status=200, + status=400, ) return render(