fix: only send state change notifications once (ietf-tools#7953)

* feat: split state_change_event create / save

* refactor: avoid double-save in rfceditor.py

* feat: only send notifications on event creation

* test: update test_notification_signal_receiver()

* chore: update comment

Author: jennifer-richards

ietf/community/models.py

@@ -104,12 +104,15 @@ 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.

ietf/community/tests.py

@@ -17,6 +17,7 @@
 import ietf.community.views
 from ietf.group.models import Group
 from ietf.group.utils import setup_default_community_list_for_group
+from ietf.doc.factories import DocumentFactory
 from ietf.doc.models import State
 from ietf.doc.utils import add_state_change_event
 from ietf.person.models import Person, Email, Alias
@@ -439,39 +440,45 @@ def test_notification_signal_receiver(self, mock_notify_task):
         This implicitly tests that notify_events is hooked up to the post_save signal.
         """
         # Arbitrary model that's not a DocEvent
-        p = PersonFactory()
+        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"):
-            p.save()
+            person.save()
         self.assertFalse(mock_notify_task.delay.called)
 
-        d = DocEventFactory()
-        mock_notify_task.reset_mock()  # clear any calls that resulted from the factories
+        # 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
         # 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)
+        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")
+        
+        mock_notify_task.reset_mock()
+        d = DocEventFactory.build(by=person, doc=doc)
         d.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()
-        self.assertFalse(mock_notify_task.delay.called)
+        self.assertFalse(mock_notify_task.delay.called, "notify_task should not run when skip_community_list_notification is set")
 
-        del(d.skip_community_list_notification)
-        d.doc.type_id="rfc"  # not "draft"
-        d.doc.save()
+        d = 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()
-        self.assertFalse(mock_notify_task.delay.called)
+        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")
     def test_notify_event_to_subscribers(self, mock_send_mail_text):

ietf/doc/utils.py

@@ -398,8 +398,12 @@ def get_unicode_document_content(key, filename, codec='utf-8', errors='ignore'):
 def tags_suffix(tags):
     return ("::" + "::".join(t.name for t in tags)) if tags else ""
 
-def add_state_change_event(doc, by, prev_state, new_state, prev_tags=None, new_tags=None, timestamp=None):
-    """Add doc event to explain that state change just happened."""
+
+def new_state_change_event(doc, by, prev_state, new_state, prev_tags=None, new_tags=None, timestamp=None):
+    """Create unsaved doc event to explain that state change just happened
+    
+    Returns None if no state change occurred.
+    """
     if prev_state and new_state:
         assert prev_state.type_id == new_state.type_id
 
@@ -419,7 +423,22 @@ def add_state_change_event(doc, by, prev_state, new_state, prev_tags=None, new_t
         e.desc += " from %s" % (prev_state.name + tags_suffix(prev_tags))
     if timestamp:
         e.time = timestamp
-    e.save()
+    return e  # not saved!
+
+
+def add_state_change_event(doc, by, prev_state, new_state, prev_tags=None, new_tags=None, timestamp=None):
+    """Add doc event to explain that state change just happened.
+    
+    Returns None if no state change occurred.
+    
+    Note: Creating a state change DocEvent will trigger notifications to be sent to people subscribed
+    to the doc via a CommunityList on its first save(). If you need to adjust the event (say, changing
+    its desc) before that notification is sent, use new_state_change_event() instead and save the
+    event after making your changes. 
+    """
+    e = new_state_change_event(doc, by, prev_state, new_state, prev_tags, new_tags, timestamp)
+    if e is not None:
+        e.save()
     return e
 
 

ietf/sync/rfceditor.py

@@ -21,7 +21,7 @@
 from ietf.doc.models import ( Document, State, StateType, DocEvent, DocRelationshipName,
     DocTagName, RelatedDocument, RelatedDocHistory )
 from ietf.doc.expire import move_draft_files_to_archive
-from ietf.doc.utils import add_state_change_event, prettify_std_name, update_action_holders
+from ietf.doc.utils import add_state_change_event, new_state_change_event, prettify_std_name, update_action_holders
 from ietf.group.models import Group
 from ietf.ipr.models import IprDocRel
 from ietf.name.models import StdLevelName, StreamName
@@ -202,11 +202,14 @@ def update_drafts_from_queue(drafts):
         if prev_state != next_state:
             d.set_state(next_state)
 
-            e = add_state_change_event(d, system, prev_state, next_state)
+            e = new_state_change_event(d, system, prev_state, next_state)  # unsaved
+            if e:
+                if auth48:
+                    e.desc = re.sub(r"(<b>.*</b>)", "<a href=\"%s\">\\1</a>" % auth48, e.desc)
+                e.save()
+                events.append(e)
 
             if auth48:
-                e.desc = re.sub(r"(<b>.*</b>)", "<a href=\"%s\">\\1</a>" % auth48, e.desc)
-                e.save()
                 # Create or update the auth48 URL whether or not this is a state expected to have one.
                 d.documenturl_set.update_or_create(
                     tag_id='auth48',  # look up existing based on this field
@@ -215,8 +218,6 @@ def update_drafts_from_queue(drafts):
             else:
                 # Remove any existing auth48 URL when an update does not have one.
                 d.documenturl_set.filter(tag_id='auth48').delete()
-            if e:
-                events.append(e)
 
             changed.add(name)