Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -26,29 +26,70 @@
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).
with transaction.atomic():
Comment on lines 65 to 68
Copy link

Copilot AI Mar 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
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")
)
reserved_ids: list[int] = list(pending_qs.values_list("pk", flat=True))

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)

if not reserved_ids:
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
failed = 0

for notification_pk in reserved_ids:
try:
Expand Down Expand Up @@ -76,9 +117,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
Expand Down
Original file line number Diff line number Diff line change
@@ -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'),
),
]
5 changes: 4 additions & 1 deletion fiesta/apps/notifications/models/scheduled.py
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -32,14 +33,16 @@ class ScheduledNotification(BaseTimestampedModel):
)

recipient = models.ForeignKey(
"accounts.User",
settings.AUTH_USER_MODEL,
on_delete=models.CASCADE,
related_name="scheduled_notifications",
verbose_name=_("recipient"),
)
section = models.ForeignKey(
"sections.Section",
on_delete=models.CASCADE,
null=True,
blank=True,
related_name="scheduled_notifications",
verbose_name=_("section"),
)
Expand Down
24 changes: 19 additions & 5 deletions fiesta/apps/notifications/services/mailer.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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}/"

Comment on lines +42 to +47
Copy link

Copilot AI Mar 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copilot uses AI. Check for mistakes.
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)",
Expand Down
8 changes: 5 additions & 3 deletions fiesta/apps/notifications/services/scheduler.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
from __future__ import annotations

import logging
from contextlib import suppress
from datetime import datetime
from typing import TYPE_CHECKING

Expand Down Expand Up @@ -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)

Expand Down
20 changes: 20 additions & 0 deletions fiesta/apps/notifications/services/unsubscribe.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
from __future__ import annotations

from django.core.signing import BadSignature, TimestampSigner

UNSUBSCRIBE_SALT = "notifications-unsubscribe"


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[str, str]:
signer = TimestampSigner(salt=UNSUBSCRIBE_SALT)
value = signer.unsign(token, max_age=max_age)
try:
user_id_str, action = value.rsplit(":", 1)
return user_id_str, action
except (TypeError, ValueError) as exc:
raise BadSignature("Invalid unsubscribe payload") from exc
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,12 @@ <h1>
<p>
{% if section %}You are receiving this email because you are a member of {{ section }}.{% endif %}
{% if preferences_url %}<a href="{{ preferences_url }}">Manage email preferences</a>{% endif %}
{% if unsubscribe_url %}
|
<a href="{{ unsubscribe_url }}"
style="color: #6b7280;
text-decoration: underline">Unsubscribe</a>
{% endif %}
</p>
<p>
© Fiesta+ · <a href="https://fiesta.plus">fiesta.plus</a>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 %}
26 changes: 26 additions & 0 deletions fiesta/apps/notifications/templates/notifications/unsubscribe.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
{% extends "fiesta/base.html" %}
{% load i18n %}

{% block main %}
<div class="container mx-auto max-w-lg py-8">
{% if error %}
<h1 class="text-2xl font-bold mb-4">{% trans "Unsubscribe Error" %}</h1>
<p>{{ error }}</p>
{% elif success %}
<h1 class="text-2xl font-bold mb-4">{% trans "Unsubscribed" %}</h1>
<p>
{% blocktrans with action=action_description %}You have been successfully unsubscribed from {{ action }}.{% endblocktrans %}
</p>
{% else %}
<h1 class="text-2xl font-bold mb-4">{% trans "Unsubscribe from Notifications" %}</h1>
<p>
{% blocktrans with email=email action=action_description %}
You are about to unsubscribe {{ email }} from {{ action }}.
{% endblocktrans %}
</p>
<form method="post">
<button type="submit" class="btn btn-primary mt-4">{% trans "Unsubscribe" %}</button>
</form>
{% endif %}
</div>
{% endblock main %}
6 changes: 3 additions & 3 deletions fiesta/apps/notifications/tests/factories.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
from django.utils import timezone
from factory.django import DjangoModelFactory

from apps.notifications.models import ScheduledNotification, SectionNotificationPreferences
from apps.notifications.models import NotificationKind, ScheduledNotification, SectionNotificationPreferences


class ScheduledNotificationFactory(DjangoModelFactory):
Expand All @@ -16,8 +16,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)
Expand Down
14 changes: 12 additions & 2 deletions fiesta/apps/notifications/tests/test_match_notifications.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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")
Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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")
Expand Down
11 changes: 9 additions & 2 deletions fiesta/apps/notifications/tests/test_membership_notifications.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,20 @@

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
from apps.utils.factories.accounts import UserFactory
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,
Expand All @@ -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,
Expand Down Expand Up @@ -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")
Expand Down
Loading
Loading