diff --git a/service/notification/migrations/0002_add_expired_delivery_status.py b/service/notification/migrations/0002_add_expired_delivery_status.py new file mode 100644 index 000000000..fc836dca0 --- /dev/null +++ b/service/notification/migrations/0002_add_expired_delivery_status.py @@ -0,0 +1,22 @@ +# Generated by Django 6.1 on 2026-08-29 13:05 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("notification", "0001_initial"), + ] + + operations = [ + migrations.AlterField( + model_name="notificationdelivery", + name="status", + field=models.CharField( + choices=[("pending", "Pending"), ("sent", "Sent"), ("failed", "Failed"), ("expired", "Expired")], + default="pending", + max_length=20, + ), + ), + ] diff --git a/service/notification/models.py b/service/notification/models.py index 42f3bb0d9..c279f13e8 100644 --- a/service/notification/models.py +++ b/service/notification/models.py @@ -1,5 +1,6 @@ from django.contrib.auth import get_user_model from django.db import models +from django.utils import timezone from common.models import BaseModel @@ -62,6 +63,14 @@ def broadcast(self, notifications, spec): deliver.defer(delivery_ids=[d.id for d in deliveries]) return deliveries + def expire_stale(self, deliveries, max_age): + cutoff = timezone.now() - max_age + stale = [d for d in deliveries if d.created_at < cutoff] + if not stale: + return deliveries + self.filter(id__in=[d.id for d in stale]).update(status=self.model.Status.EXPIRED) + return [d for d in deliveries if d.created_at >= cutoff] + def _email_enabled_ids(self, recipient_ids): return set( User.objects.filter( @@ -79,6 +88,7 @@ class Status(models.TextChoices): PENDING = "pending" SENT = "sent" FAILED = "failed" + EXPIRED = "expired" objects = NotificationDeliveryManager() diff --git a/service/notification/tasks.py b/service/notification/tasks.py index e5e39201e..cce02e092 100644 --- a/service/notification/tasks.py +++ b/service/notification/tasks.py @@ -11,17 +11,23 @@ logger = logging.getLogger(__name__) PRUNE_AFTER_DAYS = 30 +DELIVER_MAX_AGE_HOURS = 1 @app.task(name="notification.deliver", retry=3) def deliver(delivery_ids): deliveries = list( - NotificationDelivery.objects.filter(id__in=delivery_ids).select_related("notification") + NotificationDelivery.objects.filter( + id__in=delivery_ids, status=NotificationDelivery.Status.PENDING + ).select_related("notification") ) if not deliveries: return - _deliver_channel(deliveries, NotificationDelivery.Channel.PUSH, _send_push) - _deliver_channel(deliveries, NotificationDelivery.Channel.EMAIL, _send_email) + fresh = NotificationDelivery.objects.expire_stale(deliveries, timedelta(hours=DELIVER_MAX_AGE_HOURS)) + if not fresh: + return + _deliver_channel(fresh, NotificationDelivery.Channel.PUSH, _send_push) + _deliver_channel(fresh, NotificationDelivery.Channel.EMAIL, _send_email) def _deliver_channel(deliveries, channel, send): diff --git a/service/notification/tests.py b/service/notification/tests.py index 0fd0fbe04..3dcfd6c8e 100644 --- a/service/notification/tests.py +++ b/service/notification/tests.py @@ -16,7 +16,7 @@ from notification.models import Notification, NotificationDelivery from notification.registry import REGISTRY as NOTIFICATION_REGISTRY from notification.registry import ChannelMessageSpec -from notification.tasks import PRUNE_AFTER_DAYS, deliver, prune +from notification.tasks import DELIVER_MAX_AGE_HOURS, PRUNE_AFTER_DAYS, deliver, prune from notification.utils import PUSH_TTL, build_push_message from phase.models import Phase from user_profile.models import UserProfile @@ -55,6 +55,21 @@ def _rendered_push(**overrides): return [content] +def _push_delivery(user, status=NotificationDelivery.Status.PENDING): + notification = Notification.objects.create(recipient=user, event_type="game_start") + return NotificationDelivery.objects.create( + notification=notification, + channel=NotificationDelivery.Channel.PUSH, + heading="Game", + body="Started", + status=status, + ) + + +def _backdate(delivery, age): + NotificationDelivery.objects.filter(id=delivery.id).update(created_at=timezone.now() - age) + + class _StubSpec: def __init__(self, rendered): self.channels = [content["channel"] for content in rendered] @@ -868,6 +883,56 @@ def test_missing_ids_is_noop(self, mock_send_notification_to_users, in_memory_pr mock_send_notification_to_users.assert_not_called() + @pytest.mark.django_db + def test_already_sent_delivery_is_not_resent(self, user_factory, mock_send_notification_to_users): + delivery = _push_delivery(user_factory(), status=NotificationDelivery.Status.SENT) + + deliver(delivery_ids=[delivery.id]) + + mock_send_notification_to_users.assert_not_called() + + @pytest.mark.django_db + def test_pending_delivery_in_a_partly_sent_batch_is_still_sent( + self, user_factory, mock_send_notification_to_users + ): + one, two = user_factory(), user_factory() + sent = _push_delivery(one, status=NotificationDelivery.Status.SENT) + pending = _push_delivery(two) + + deliver(delivery_ids=[sent.id, pending.id]) + + assert mock_send_notification_to_users.call_args.kwargs["user_ids"] == [two.id] + + @pytest.mark.django_db + def test_delivery_older_than_max_age_is_expired_not_sent( + self, user_factory, mock_send_notification_to_users + ): + delivery = _push_delivery(user_factory()) + _backdate(delivery, timedelta(hours=DELIVER_MAX_AGE_HOURS, minutes=1)) + + deliver(delivery_ids=[delivery.id]) + + mock_send_notification_to_users.assert_not_called() + delivery.refresh_from_db() + assert delivery.status == NotificationDelivery.Status.EXPIRED + + @pytest.mark.django_db + def test_fresh_delivery_in_a_partly_stale_batch_is_still_sent( + self, user_factory, mock_send_notification_to_users + ): + one, two = user_factory(), user_factory() + stale = _push_delivery(one) + _backdate(stale, timedelta(hours=DELIVER_MAX_AGE_HOURS, minutes=1)) + fresh = _push_delivery(two) + + deliver(delivery_ids=[stale.id, fresh.id]) + + assert mock_send_notification_to_users.call_args.kwargs["user_ids"] == [two.id] + stale.refresh_from_db() + fresh.refresh_from_db() + assert stale.status == NotificationDelivery.Status.EXPIRED + assert fresh.status == NotificationDelivery.Status.SENT + class TestNotificationPrune: @pytest.mark.django_db