feat: celery task + admin to resend Messages (#8661)

* feat: Message re-send task

* feat: admin action to queue redelivery

* feat: MessageAdmin list_filters

* feat: show sent status

* feat: better date filtering

* chore: remove send-by-date task

Adds complexity and risk - the improved Messages admin lets us do most of what it did without the opportunity for accidentally resending huge ranges

* chore: fill in empty docstring

* style: black

* fix: unused import

* feat: better logging

* chore: mypy lint

* test: test retry_send_messages_by_pk_task

* test: test retry_send_messages

ietf/message/admin.py

@@ -1,32 +1,99 @@
-from django.contrib import admin
+# Copyright The IETF Trust 2012-2025, All Rights Reserved
+from django.contrib import admin, messages
+from django.db.models import QuerySet
+from rangefilter.filters import DateRangeQuickSelectListFilterBuilder
 
 from ietf.message.models import Message, MessageAttachment, SendQueue, AnnouncementFrom
+from ietf.message.tasks import retry_send_messages_by_pk_task
+
+
+class MessageSentStatusListFilter(admin.SimpleListFilter):
+    """Filter Messages by whether or not they were sent"""
+
+    title = "status"
+    parameter_name = "status"
+
+    def lookups(self, request, model_admin):
+        return [
+            ("sent", "Sent"),
+            ("unsent", "Not sent"),
+        ]
+
+    def queryset(self, request, queryset):
+        if self.value() == "unsent":
+            return queryset.filter(sent__isnull=True)
+        elif self.value() == "sent":
+            return queryset.filter(sent__isnull=False)
+
 
 class MessageAdmin(admin.ModelAdmin):
-    list_display = ["subject", "by", "time", "groups"]
+    list_display = ["sent_status", "subject", "by", "time", "groups"]
     search_fields = ["subject", "body"]
     raw_id_fields = ["by", "related_groups", "related_docs"]
+    list_filter = [
+        MessageSentStatusListFilter,
+        ("time", DateRangeQuickSelectListFilterBuilder()),
+    ]
     ordering = ["-time"]
+    actions = ["retry_send"]
 
     def groups(self, instance):
         return ", ".join(g.acronym for g in instance.related_groups.all())
+
+    @admin.display(description="Sent", boolean=True)
+    def sent_status(self, instance):
+        return instance.sent is not None
+
+    @admin.action(description="Send selected messages if unsent")
+    def retry_send(self, request, queryset: QuerySet[Message]):
+        try:
+            retry_send_messages_by_pk_task.delay(
+                message_pks=list(queryset.values_list("pk", flat=True)),
+                resend=False,
+            )
+        except Exception as err:
+            self.message_user(
+                request,
+                f"Error: {repr(err)}",
+                messages.ERROR,
+            )
+        else:
+            self.message_user(request, "Messages queued for delivery", messages.SUCCESS)
+
+
 admin.site.register(Message, MessageAdmin)
 
+
 class MessageAttachmentAdmin(admin.ModelAdmin):
-    list_display = ['id', 'message', 'filename', 'removed',]
-    raw_id_fields = ['message']
+    list_display = [
+        "id",
+        "message",
+        "filename",
+        "removed",
+    ]
+    raw_id_fields = ["message"]
+
+
 admin.site.register(MessageAttachment, MessageAttachmentAdmin)
 
+
 class SendQueueAdmin(admin.ModelAdmin):
     list_display = ["time", "by", "message", "send_at", "sent_at"]
     list_filter = ["time", "send_at", "sent_at"]
     search_fields = ["message__body"]
     raw_id_fields = ["by", "message"]
     ordering = ["-time"]
+
+
 admin.site.register(SendQueue, SendQueueAdmin)
 
+
 class AnnouncementFromAdmin(admin.ModelAdmin):
-    list_display = ['name', 'group', 'address', ]
+    list_display = [
+        "name",
+        "group",
+        "address",
+    ]
+
+
 admin.site.register(AnnouncementFrom, AnnouncementFromAdmin)
-
-

ietf/message/tasks.py

@@ -5,8 +5,8 @@
 from celery import shared_task
 from smtplib import SMTPException
 
-from ietf.message.utils import send_scheduled_message_from_send_queue
-from ietf.message.models import SendQueue
+from ietf.message.utils import send_scheduled_message_from_send_queue, retry_send_messages
+from ietf.message.models import SendQueue, Message
 from ietf.utils import log
 from ietf.utils.mail import log_smtp_exception, send_error_email
 
@@ -25,3 +25,23 @@ def send_scheduled_mail_task():
         except SMTPException as e:
             log_smtp_exception(e)
             send_error_email(e)
+
+
+@shared_task
+def retry_send_messages_by_pk_task(message_pks: list, resend=False):
+    """Task to retry sending Messages by PK
+    
+    Sends Messages whose PK is included in the list.
+    Only previously unsent messages are sent unless `resend` is true.
+    """
+    log.log(
+        "retry_send_messages_by_pk_task: "
+        "retrying send of Message PKs [{}] (resend={})".format(
+            ", ".join(str(pk) for pk in message_pks),
+            resend,
+        )
+    )
+    retry_send_messages(
+        messages=Message.objects.filter(pk__in=message_pks),
+        resend=resend,
+    )

ietf/message/tests.py

@@ -11,10 +11,10 @@ from django.utils import timezone
 import debug                            # pyflakes:ignore
 
 from ietf.group.factories import GroupFactory
-from ietf.message.factories import SendQueueFactory
+from ietf.message.factories import MessageFactory, SendQueueFactory
 from ietf.message.models import Message, SendQueue
-from ietf.message.tasks import send_scheduled_mail_task
-from ietf.message.utils import send_scheduled_message_from_send_queue
+from ietf.message.tasks import send_scheduled_mail_task, retry_send_messages_by_pk_task
+from ietf.message.utils import send_scheduled_message_from_send_queue, retry_send_messages
 from ietf.person.models import Person
 from ietf.utils.mail import outbox, send_mail_text, send_mail_message, get_payload_text
 from ietf.utils.test_utils import TestCase
@@ -133,6 +133,44 @@ class SendScheduledAnnouncementsTests(TestCase):
         self.assertTrue(SendQueue.objects.get(id=q.id).sent_at)
 
 
+class UtilsTests(TestCase):
+    @mock.patch("ietf.message.utils.send_mail_message")
+    def test_retry_send_messages(self, mock_send_mail_message):
+        sent_message = MessageFactory(sent=timezone.now())
+        unsent_messages = MessageFactory.create_batch(2, sent=None)
+        
+        # Send the sent message and one of the unsent messages
+        retry_send_messages(
+            Message.objects.filter(pk__in=[
+                sent_message.pk,
+                unsent_messages[0].pk,
+            ]),
+            resend=False,
+        )
+        self.assertEqual(mock_send_mail_message.call_count, 1)
+        self.assertEqual(
+            mock_send_mail_message.call_args.args[1],
+            unsent_messages[0],
+        )
+        
+        mock_send_mail_message.reset_mock()
+        # Once again, send the sent message and one of the unsent messages 
+        # (we can use the same one because our mock prevented it from having
+        # its status updated to sent)
+        retry_send_messages(
+            Message.objects.filter(pk__in=[
+                sent_message.pk,
+                unsent_messages[0].pk,
+            ]),
+            resend=True,
+        )
+        self.assertEqual(mock_send_mail_message.call_count, 2)
+        self.assertCountEqual(
+            [call_args.args[1] for call_args in mock_send_mail_message.call_args_list],
+            [sent_message, unsent_messages[0]],
+        )
+
+
 class TaskTests(TestCase):
     @mock.patch("ietf.message.tasks.log_smtp_exception")
     @mock.patch("ietf.message.tasks.send_scheduled_message_from_send_queue")
@@ -150,3 +188,18 @@ class TaskTests(TestCase):
         self.assertEqual(mock_send_message.call_count, 1)
         self.assertEqual(mock_send_message.call_args[0], (not_yet_sent,))
         self.assertTrue(mock_log_smtp_exception.called)
+
+    @mock.patch("ietf.message.tasks.retry_send_messages")
+    def test_retry_send_messages_by_pk_task(self, mock_retry_send):
+        msgs = MessageFactory.create_batch(3)
+        MessageFactory()  # an extra message that won't be resent
+
+        retry_send_messages_by_pk_task([msg.pk for msg in msgs], resend=False)
+        called_with_messages = mock_retry_send.call_args.kwargs["messages"]
+        self.assertCountEqual(msgs, called_with_messages)
+        self.assertFalse(mock_retry_send.call_args.kwargs["resend"])
+
+        retry_send_messages_by_pk_task([msg.pk for msg in msgs], resend=True)
+        called_with_messages = mock_retry_send.call_args.kwargs["messages"]
+        self.assertCountEqual(msgs, called_with_messages)
+        self.assertTrue(mock_retry_send.call_args.kwargs["resend"])

ietf/message/utils.py

@@ -1,13 +1,17 @@
 # Copyright The IETF Trust 2012-2020, All Rights Reserved
 # -*- coding: utf-8 -*-
 
+import email
+import email.utils
+import re
+import smtplib
 
-import re, email
-
+from django.db.models import QuerySet
 from django.utils import timezone
 from django.utils.encoding import force_str
 
-from ietf.utils.mail import send_mail_text, send_mail_mime
+from ietf.utils import log
+from ietf.utils.mail import send_mail_text, send_mail_mime, send_mail_message
 from ietf.message.models import Message
 
 first_dot_on_line_re = re.compile(r'^\.', re.MULTILINE)
@@ -58,3 +62,29 @@ def send_scheduled_message_from_send_queue(queue_item):
 
     queue_item.message.sent = queue_item.sent_at
     queue_item.message.save()
+
+
+def retry_send_messages(messages: QuerySet[Message], resend=False):
+    """Attempt delivery of Messages"""
+    if not resend:
+        # only include sent messages on explicit request
+        for already_sent in messages.filter(sent__isnull=False):
+            assert already_sent.sent is not None  # appease mypy type checking
+            log.log(
+                f"retry_send_messages: skipping {already_sent.pk} "
+                f"(already sent {already_sent.sent.isoformat(timespec='milliseconds')})"
+            )
+        messages = messages.filter(sent__isnull=True)
+    for msg in messages:
+        to = ",".join(a[1] for a in email.utils.getaddresses([msg.to]))
+        try:
+            send_mail_message(None, msg)
+            log.log(
+                f'retry_send_messages: '
+                f'sent {msg.pk} {msg.frm} -> {to} "{msg.subject.strip()}"'
+            )
+        except smtplib.SMTPException as e:
+            log.log(
+                f'retry_send_messages: '
+                f'Failure {e}:  {msg.pk}  {msg.frm} -> {to}  "{msg.subject.strip()}"'
+            )

ietf/settings.py

@@ -465,6 +465,7 @@ INSTALLED_APPS = [
     'drf_spectacular',
     'drf_standardized_errors',
     'rest_framework',
+    'rangefilter',
     'simple_history',
     'tastypie',
     'widget_tweaks',

ietf/templates/admin/base.html

@@ -20,6 +20,7 @@
                 --header-color: var(--bs-secondary);
                 --breadcrumbs-fg: var(--bs-secondary);
                 --breadcrumbs-link-fg: var(--link-fg);
+                .calendar caption { background-color: var(--secondary);}
             }
             span.text-danger { color: var(--bs-danger); }
         </style>

requirements.txt

@@ -13,6 +13,7 @@ celery>=5.2.6
 coverage>=4.5.4,<5.0    # Coverage 5.x moves from a json database to SQLite.  Moving to 5.x will require substantial rewrites in ietf.utils.test_runner and ietf.release.views
 defusedxml>=0.7.1    # for TastyPie when using xml; not a declared dependency
 Django>4.2,<5
+django-admin-rangefilter>=0.13.2
 django-analytical>=3.1.0
 django-bootstrap5>=21.3
 django-celery-beat>=2.3.0