refactor: separate concerns / rename notify_events (#8328)

* refactor: separate signal receiver from work

* test: split test to match code structure

* test: fix test

* refactor: reorg signals in community app

Author: jennifer-richards

ietf/community/apps.py

@@ -0,0 +1,12 @@
+# Copyright The IETF Trust 2024, All Rights Reserved
+
+from django.apps import AppConfig
+
+
+class CommunityConfig(AppConfig):
+    name = "ietf.community"
+
+    def ready(self):
+        """Initialize the app after the registry is populated"""
+        # implicitly connects @receiver-decorated signals
+        from . import signals  # pyflakes: ignore

ietf/community/models.py

@@ -1,19 +1,14 @@
 # Copyright The IETF Trust 2012-2020, All Rights Reserved
 # -*- coding: utf-8 -*-
 
-
-from django.conf import settings
-from django.db import models, transaction
-from django.db.models import signals
+from django.db import models
 from django.urls import reverse as urlreverse
 
-from ietf.doc.models import Document, DocEvent, State
+from ietf.doc.models import Document, State
 from ietf.group.models import Group
 from ietf.person.models import Person, Email
 from ietf.utils.models import ForeignKey
 
-from .tasks import notify_event_to_subscribers_task
-
 
 class CommunityList(models.Model):
     person = ForeignKey(Person, blank=True, null=True)
@@ -98,29 +93,3 @@ class EmailSubscription(models.Model):
 
     def __str__(self):
         return "%s to %s (%s changes)" % (self.email, self.community_list, self.notify_on)
-
-
-def notify_events(sender, instance, **kwargs):
-    if not isinstance(instance, DocEvent):
-        return
-
-    if not kwargs.get("created", False):
-        return  # only notify on creation
-
-    if instance.doc.type_id != 'draft':
-        return
-
-    if getattr(instance, "skip_community_list_notification", False):
-        return
-    
-    # kludge alert: queuing a celery task in response to a signal can cause unexpected attempts to
-    # start a Celery task during tests. To prevent this, don't queue a celery task if we're running
-    # tests.
-    if settings.SERVER_MODE != "test":
-        # Wrap in on_commit in case a transaction is open
-        transaction.on_commit(
-            lambda: notify_event_to_subscribers_task.delay(event_id=instance.pk)
-        )
-
-
-signals.post_save.connect(notify_events)

ietf/community/signals.py

@@ -0,0 +1,44 @@
+# Copyright The IETF Trust 2024, All Rights Reserved
+
+from django.conf import settings
+from django.db import transaction
+from django.db.models.signals import post_save
+from django.dispatch import receiver
+
+from ietf.doc.models import DocEvent
+from .tasks import notify_event_to_subscribers_task
+
+
+def notify_of_event(event: DocEvent):
+    """Send subscriber notification emails for a 'draft'-related DocEvent
+
+    If the event is attached to a draft of type 'doc', queues a task to send notification emails to
+    community list subscribers. No emails will be sent when SERVER_MODE is 'test'.
+    """
+    if event.doc.type_id != "draft":
+        return
+
+    if getattr(event, "skip_community_list_notification", False):
+        return
+
+    # kludge alert: queuing a celery task in response to a signal can cause unexpected attempts to
+    # start a Celery task during tests. To prevent this, don't queue a celery task if we're running
+    # tests.
+    if settings.SERVER_MODE != "test":
+        # Wrap in on_commit in case a transaction is open
+        transaction.on_commit(
+            lambda: notify_event_to_subscribers_task.delay(event_id=event.pk)
+        )
+
+
+# dispatch_uid ensures only a single signal receiver binding is made
+@receiver(post_save, dispatch_uid="notify_of_events_receiver_uid")
+def notify_of_events_receiver(sender, instance, **kwargs):
+    """Call notify_of_event after saving a new DocEvent"""
+    if not isinstance(instance, DocEvent):
+        return
+
+    if not kwargs.get("created", False):
+        return  # only notify on creation
+
+    notify_of_event(instance)

ietf/community/tests.py

@@ -1,7 +1,6 @@
 # Copyright The IETF Trust 2016-2023, All Rights Reserved
 # -*- coding: utf-8 -*-
 
-
 import mock
 from pyquery import PyQuery
 
@@ -11,6 +10,7 @@
 import debug                            # pyflakes:ignore
 
 from ietf.community.models import CommunityList, SearchRule, EmailSubscription
+from ietf.community.signals import notify_of_event
 from ietf.community.utils import docs_matching_community_list_rule, community_list_rules_matching_doc
 from ietf.community.utils import reset_name_contains_index_for_rule, notify_event_to_subscribers
 from ietf.community.tasks import notify_event_to_subscribers_task
@@ -431,53 +431,58 @@ def test_subscription_for_group(self):
         r = self.client.get(url)
         self.assertEqual(r.status_code, 200)
 
-    # Mock out the on_commit call so we can tell whether the task was actually queued
-    @mock.patch("ietf.submit.views.transaction.on_commit", side_effect=lambda x: x())
-    @mock.patch("ietf.community.models.notify_event_to_subscribers_task")
-    def test_notification_signal_receiver(self, mock_notify_task, mock_on_commit):
-        """Saving a DocEvent should notify subscribers
+    @mock.patch("ietf.community.signals.notify_of_event")
+    def test_notification_signal_receiver(self, mock_notify_of_event):
+        """Saving a newly created DocEvent should notify subscribers
         
-        This implicitly tests that notify_events is hooked up to the post_save signal.
+        This implicitly tests that notify_of_event_receiver is hooked up to the post_save signal.
         """
         # Arbitrary model that's not a DocEvent
-        person = PersonFactory()
-        mock_notify_task.reset_mock()  # clear any calls that resulted from the factories
-        # be careful overriding SERVER_MODE - we do it here because the method
-        # under test does not make this call when in "test" mode
-        with override_settings(SERVER_MODE="not-test"):
-            person.save()
-        self.assertFalse(mock_notify_task.delay.called)
-        
+        person = PersonFactory.build()  # builds but does not save...
+        mock_notify_of_event.reset_mock()  # clear any calls that resulted from the factories
+        person.save()
+        self.assertFalse(mock_notify_of_event.called)
+    
         # build a DocEvent that is not yet persisted
         doc = DocumentFactory()
-        d = DocEventFactory.build(by=person, doc=doc)
-        # mock_notify_task.reset_mock()  # clear any calls that resulted from the factories
+        event = DocEventFactory.build(by=person, doc=doc)  # builds but does not save...
+        mock_notify_of_event.reset_mock()  # clear any calls that resulted from the factories
+        event.save()
+        self.assertEqual(mock_notify_of_event.call_count, 1, "notify_task should be run on creation of DocEvent")
+        self.assertEqual(mock_notify_of_event.call_args, mock.call(event))
+
+        # save the existing DocEvent and see that no notification is sent    
+        mock_notify_of_event.reset_mock()
+        event.save()
+        self.assertFalse(mock_notify_of_event.called, "notify_task should not be run save of on existing DocEvent")
+
+    # Mock out the on_commit call so we can tell whether the task was actually queued
+    @mock.patch("ietf.submit.views.transaction.on_commit", side_effect=lambda x: x())
+    @mock.patch("ietf.community.signals.notify_event_to_subscribers_task")
+    def test_notify_of_event(self, mock_notify_task, mock_on_commit):
+        """The community notification task should be called as intended"""
+        person = PersonFactory()  # builds but does not save...
+        doc = DocumentFactory()
+        event = DocEventFactory(by=person, doc=doc)
         # be careful overriding SERVER_MODE - we do it here because the method
         # under test does not make this call when in "test" mode
         with override_settings(SERVER_MODE="not-test"):
-            d.save()
-        self.assertEqual(mock_notify_task.delay.call_count, 1, "notify_task should be run on creation of DocEvent")
-        self.assertEqual(mock_notify_task.delay.call_args, mock.call(event_id = d.pk))
-        
-        mock_notify_task.reset_mock()
-        with override_settings(SERVER_MODE="not-test"):
-            d.save()
-        self.assertFalse(mock_notify_task.delay.called, "notify_task should not be run save of on existing DocEvent")
-        
+            notify_of_event(event)
+        self.assertTrue(mock_notify_task.delay.called, "notify_task should run for a DocEvent on a draft")
         mock_notify_task.reset_mock()
-        d = DocEventFactory.build(by=person, doc=doc)
-        d.skip_community_list_notification = True
+
+        event.skip_community_list_notification = True
         # be careful overriding SERVER_MODE - we do it here because the method
         # under test does not make this call when in "test" mode
         with override_settings(SERVER_MODE="not-test"):
-            d.save()
+            notify_of_event(event)
         self.assertFalse(mock_notify_task.delay.called, "notify_task should not run when skip_community_list_notification is set")
 
-        d = DocEventFactory.build(by=person, doc=DocumentFactory(type_id="rfc"))
+        event = DocEventFactory.build(by=person, doc=DocumentFactory(type_id="rfc"))
         # be careful overriding SERVER_MODE - we do it here because the method
         # under test does not make this call when in "test" mode
         with override_settings(SERVER_MODE="not-test"):
-            d.save()
+            notify_of_event(event)
         self.assertFalse(mock_notify_task.delay.called, "notify_task should not run on a document with type 'rfc'")
 
     @mock.patch("ietf.utils.mail.send_mail_text")

ietf/utils/management/commands/loadrelated.py

@@ -23,7 +23,7 @@
 
 import debug                            # pyflakes:ignore
 
-from ietf.community.models import notify_events
+from ietf.community.signals import notify_of_events_receiver
 
 class Command(loaddata.Command):
     help = ("""
@@ -62,7 +62,7 @@ def handle(self, *args, **options):
         #
         self.serialization_formats = serializers.get_public_serializer_formats()
         #
-        post_save.disconnect(notify_events)
+        post_save.disconnect(notify_of_events_receiver())
         #
         connection = connections[self.using]
         self.fixture_count = 0