altf4arnold/datatracker
1
0
Fork 0
Code Issues Pull requests Actions 4 Packages Projects Releases Wiki Activity

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

Browse source 
* 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
This commit is contained in:
Jennifer Richards 2024-09-18 16:02:00 -03:00 committed by GitHub
parent 97b719505e
commit c6389ba65f
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
4 changed files with 50 additions and 20 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

5
ietf/community/models.py
View file

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

27
ietf/community/tests.py
View file

@ -17,6 +17,7 @@ from ietf.community.tasks import notify_event_to_subscribers_task
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 @@ class CommunityListTests(TestCase):
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):

25
ietf/doc/utils.py
View file

@ -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

13
ietf/sync/rfceditor.py
View file

@ -21,7 +21,7 @@ import debug # pyflakes:ignore
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)