From 61c5037eabfb79a338959fdc2293beda065302c3 Mon Sep 17 00:00:00 2001 From: Jennifer Richards Date: Wed, 10 Jul 2024 21:27:58 -0300 Subject: [PATCH 01/12] feat: notify_event_to_subscribers_task --- ietf/community/models.py | 6 ++++-- ietf/community/tasks.py | 11 +++++++++++ 2 files changed, 15 insertions(+), 2 deletions(-) create mode 100644 ietf/community/tasks.py diff --git a/ietf/community/models.py b/ietf/community/models.py index 2e40031768..04705f7bd6 100644 --- a/ietf/community/models.py +++ b/ietf/community/models.py @@ -11,6 +11,9 @@ 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) group = ForeignKey(Group, blank=True, null=True) @@ -106,8 +109,7 @@ def notify_events(sender, instance, **kwargs): if getattr(instance, "skip_community_list_notification", False): return - from ietf.community.utils import notify_event_to_subscribers - notify_event_to_subscribers(instance) + notify_event_to_subscribers_task.delay(event_id=instance.pk) signals.post_save.connect(notify_events) diff --git a/ietf/community/tasks.py b/ietf/community/tasks.py new file mode 100644 index 0000000000..7688eadd90 --- /dev/null +++ b/ietf/community/tasks.py @@ -0,0 +1,11 @@ +# Copyright The IETF Trust 2024, All Rights Reserved +from celery import shared_task + +from ietf.doc.models import DocEvent +from .utils import notify_event_to_subscribers + + +@shared_task +def notify_event_to_subscribers_task(event_id): + event = DocEvent.objects.get(pk=event_id) + notify_event_to_subscribers(event) From 851293821d451aa176a245c3117d6cdca966c238 Mon Sep 17 00:00:00 2001 From: Jennifer Richards Date: Thu, 11 Jul 2024 18:00:38 -0300 Subject: [PATCH 02/12] fix: avoid circular import, handle error --- ietf/community/tasks.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/ietf/community/tasks.py b/ietf/community/tasks.py index 7688eadd90..1f79e170dc 100644 --- a/ietf/community/tasks.py +++ b/ietf/community/tasks.py @@ -2,10 +2,13 @@ from celery import shared_task from ietf.doc.models import DocEvent -from .utils import notify_event_to_subscribers +from ietf.utils.log import log @shared_task def notify_event_to_subscribers_task(event_id): - event = DocEvent.objects.get(pk=event_id) + from .utils import notify_event_to_subscribers + event = DocEvent.objects.filter(pk=event_id).first() + if event is None: + log(f"Unable to send subscriber notifications because DocEvent {event_id} was not found") notify_event_to_subscribers(event) From 1e95b9431adfec39470529c6fce2ba1d926e45df Mon Sep 17 00:00:00 2001 From: Jennifer Richards Date: Thu, 11 Jul 2024 18:15:25 -0300 Subject: [PATCH 03/12] fix: don't queue task in test mode --- ietf/community/models.py | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/ietf/community/models.py b/ietf/community/models.py index 04705f7bd6..6a695bc128 100644 --- a/ietf/community/models.py +++ b/ietf/community/models.py @@ -2,6 +2,7 @@ # -*- coding: utf-8 -*- +from django.conf import settings from django.db import models from django.db.models import signals from django.urls import reverse as urlreverse @@ -109,7 +110,14 @@ def notify_events(sender, instance, **kwargs): if getattr(instance, "skip_community_list_notification", False): return - notify_event_to_subscribers_task.delay(event_id=instance.pk) + # 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": + from .utils import notify_event_to_subscribers + notify_event_to_subscribers(instance) + else: + notify_event_to_subscribers_task.delay(event_id=instance.pk) signals.post_save.connect(notify_events) From b03bdba7dff0fc666ba35cecf6cd376858e523e6 Mon Sep 17 00:00:00 2001 From: Jennifer Richards Date: Thu, 11 Jul 2024 19:43:06 -0300 Subject: [PATCH 04/12] fix: don't even send mail in test mode --- ietf/community/models.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/ietf/community/models.py b/ietf/community/models.py index 6a695bc128..b1295461d6 100644 --- a/ietf/community/models.py +++ b/ietf/community/models.py @@ -113,10 +113,7 @@ def notify_events(sender, instance, **kwargs): # 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": - from .utils import notify_event_to_subscribers - notify_event_to_subscribers(instance) - else: + if settings.SERVER_MODE != "test": notify_event_to_subscribers_task.delay(event_id=instance.pk) From 31a518bb648bdf11f421f86fc124136baea89745 Mon Sep 17 00:00:00 2001 From: Jennifer Richards Date: Thu, 11 Jul 2024 19:43:49 -0300 Subject: [PATCH 05/12] test: separately test signal --- ietf/community/tests.py | 46 +++++++++++++++++++++++++++++++++++++++-- 1 file changed, 44 insertions(+), 2 deletions(-) diff --git a/ietf/community/tests.py b/ietf/community/tests.py index 387877887f..e0f543f4b7 100644 --- a/ietf/community/tests.py +++ b/ietf/community/tests.py @@ -2,8 +2,10 @@ # -*- coding: utf-8 -*- +import mock from pyquery import PyQuery +from django.test.utils import override_settings from django.urls import reverse as urlreverse import debug # pyflakes:ignore @@ -18,8 +20,7 @@ from ietf.doc.utils import add_state_change_event from ietf.person.models import Person, Email, Alias from ietf.utils.test_utils import TestCase, login_testing_unauthorized -from ietf.utils.mail import outbox -from ietf.doc.factories import WgDraftFactory +from ietf.doc.factories import DocEventFactory, WgDraftFactory from ietf.group.factories import GroupFactory, RoleFactory from ietf.person.factories import PersonFactory, EmailFactory, AliasFactory @@ -423,6 +424,47 @@ def test_subscription_for_group(self): r = self.client.get(url) self.assertEqual(r.status_code, 200) + @mock.patch("ietf.community.models.notify_event_to_subscribers_task") + def test_notification_signal(self, mock_notify): + """Saving a DocEvent should notify subscribers + + This implicitly tests that notify_events is hooked up to the post_save signal. + """ + # Arbitrary model that's not a DocEvent + p = PersonFactory() + mock_notify.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() + self.assertFalse(mock_notify.delay.called) + + d = DocEventFactory() + mock_notify.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.delay.call_count, 1) + self.assertEqual(mock_notify.delay.call_args, mock.call(event_id = d.pk)) + + mock_notify.reset_mock() + 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.delay.called) + + del(d.skip_community_list_notification) + d.doc.update(type_id="rfc") # not "draft" + # 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.delay.called) + + def test_notification(self): person = PersonFactory(user__username='plain') draft = WgDraftFactory() From 70dbcd9554b9234c9e593a48b67de15adc665b64 Mon Sep 17 00:00:00 2001 From: Jennifer Richards Date: Thu, 11 Jul 2024 21:32:00 -0300 Subject: [PATCH 06/12] fix: if/else error --- ietf/community/tasks.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ietf/community/tasks.py b/ietf/community/tasks.py index 1f79e170dc..763a596495 100644 --- a/ietf/community/tasks.py +++ b/ietf/community/tasks.py @@ -11,4 +11,5 @@ def notify_event_to_subscribers_task(event_id): event = DocEvent.objects.filter(pk=event_id).first() if event is None: log(f"Unable to send subscriber notifications because DocEvent {event_id} was not found") - notify_event_to_subscribers(event) + else: + notify_event_to_subscribers(event) From ff99ef6c1a9230e487dcfa210fc8b2f8c57742db Mon Sep 17 00:00:00 2001 From: Jennifer Richards Date: Thu, 11 Jul 2024 21:32:50 -0300 Subject: [PATCH 07/12] test: better naming --- ietf/community/tests.py | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/ietf/community/tests.py b/ietf/community/tests.py index e0f543f4b7..6f1b8b48d4 100644 --- a/ietf/community/tests.py +++ b/ietf/community/tests.py @@ -13,6 +13,7 @@ from ietf.community.models import CommunityList, SearchRule, EmailSubscription 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 +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 @@ -425,36 +426,36 @@ def test_subscription_for_group(self): self.assertEqual(r.status_code, 200) @mock.patch("ietf.community.models.notify_event_to_subscribers_task") - def test_notification_signal(self, mock_notify): + def test_notification_signal(self, mock_notify_task): """Saving a DocEvent should notify subscribers This implicitly tests that notify_events is hooked up to the post_save signal. """ # Arbitrary model that's not a DocEvent p = PersonFactory() - mock_notify.reset_mock() # clear any calls that resulted from the factories + 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() - self.assertFalse(mock_notify.delay.called) + self.assertFalse(mock_notify_task.delay.called) d = DocEventFactory() - mock_notify.reset_mock() # clear any calls that resulted from the factories + 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.delay.call_count, 1) - self.assertEqual(mock_notify.delay.call_args, mock.call(event_id = d.pk)) + self.assertEqual(mock_notify_task.delay.call_count, 1) + self.assertEqual(mock_notify_task.delay.call_args, mock.call(event_id = d.pk)) - mock_notify.reset_mock() + mock_notify_task.reset_mock() 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.delay.called) + self.assertFalse(mock_notify_task.delay.called) del(d.skip_community_list_notification) d.doc.update(type_id="rfc") # not "draft" @@ -462,9 +463,9 @@ def test_notification_signal(self, mock_notify): # under test does not make this call when in "test" mode with override_settings(SERVER_MODE="not-test"): d.save() - self.assertFalse(mock_notify.delay.called) + self.assertFalse(mock_notify_task.delay.called) - + # todo refactor to directly test notify_event_to_subscribers def test_notification(self): person = PersonFactory(user__username='plain') draft = WgDraftFactory() From 48459a812b771f63d591557c62ecdbafa631f009 Mon Sep 17 00:00:00 2001 From: Jennifer Richards Date: Thu, 11 Jul 2024 21:33:17 -0300 Subject: [PATCH 08/12] test: test the new task --- ietf/community/tests.py | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/ietf/community/tests.py b/ietf/community/tests.py index 6f1b8b48d4..939f79d566 100644 --- a/ietf/community/tests.py +++ b/ietf/community/tests.py @@ -487,5 +487,16 @@ def test_notification(self): add_state_change_event(draft, system, active_state, rfc_state) self.assertEqual(len(outbox), mailbox_before + 1) self.assertTrue(draft.name in outbox[-1]["Subject"]) - - + + @mock.patch("ietf.community.utils.notify_event_to_subscribers") + def test_notify_event_to_subscribers_task(self, mock_notify): + d = DocEventFactory() + notify_event_to_subscribers_task(event_id=d.pk) + self.assertEqual(mock_notify.call_count, 1) + self.assertEqual(mock_notify.call_args, mock.call(d)) + mock_notify.reset_mock() + + d.delete() + notify_event_to_subscribers_task(event_id=d.pk) + self.assertFalse(mock_notify.called) + From 3adbc34facdfbd650c9c9d02ffe97593513cda23 Mon Sep 17 00:00:00 2001 From: Jennifer Richards Date: Thu, 11 Jul 2024 21:34:51 -0300 Subject: [PATCH 09/12] test: better test name --- ietf/community/tests.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ietf/community/tests.py b/ietf/community/tests.py index 939f79d566..78115ef874 100644 --- a/ietf/community/tests.py +++ b/ietf/community/tests.py @@ -426,7 +426,7 @@ def test_subscription_for_group(self): self.assertEqual(r.status_code, 200) @mock.patch("ietf.community.models.notify_event_to_subscribers_task") - def test_notification_signal(self, mock_notify_task): + def test_notification_signal_receiver(self, mock_notify_task): """Saving a DocEvent should notify subscribers This implicitly tests that notify_events is hooked up to the post_save signal. From e926965503cf4bae86097fec491f09c963d8faba Mon Sep 17 00:00:00 2001 From: Jennifer Richards Date: Thu, 11 Jul 2024 21:48:42 -0300 Subject: [PATCH 10/12] test: refactor notify email test --- ietf/community/tests.py | 43 ++++++++++++++++++++++++++++++----------- 1 file changed, 32 insertions(+), 11 deletions(-) diff --git a/ietf/community/tests.py b/ietf/community/tests.py index 78115ef874..c0efd1691b 100644 --- a/ietf/community/tests.py +++ b/ietf/community/tests.py @@ -12,7 +12,7 @@ from ietf.community.models import CommunityList, SearchRule, EmailSubscription 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 +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 import ietf.community.views from ietf.group.models import Group @@ -465,8 +465,8 @@ def test_notification_signal_receiver(self, mock_notify_task): d.save() self.assertFalse(mock_notify_task.delay.called) - # todo refactor to directly test notify_event_to_subscribers - def test_notification(self): + @mock.patch("ietf.community.utils.send_mail") + def test_notify_event_to_subscribers(self, mock_send_mail): person = PersonFactory(user__username='plain') draft = WgDraftFactory() @@ -474,19 +474,40 @@ def test_notification(self): if not draft in clist.added_docs.all(): clist.added_docs.add(draft) - EmailSubscription.objects.create(community_list=clist, email=Email.objects.filter(person__user__username="plain").first(), notify_on="significant") + sub_to_significant = EmailSubscription.objects.create( + community_list=clist, + email=Email.objects.filter(person__user__username="plain").first(), + notify_on="significant", + ) + sub_to_all = EmailSubscription.objects.create( + community_list=clist, + email=Email.objects.filter(person__user__username="plain").first(), + notify_on="all", + ) - mailbox_before = len(outbox) active_state = State.objects.get(type="draft", slug="active") system = Person.objects.get(name="(System)") - add_state_change_event(draft, system, None, active_state) - self.assertEqual(len(outbox), mailbox_before) + event = add_state_change_event(draft, system, None, active_state) + notify_event_to_subscribers(event) + self.assertEqual(mock_send_mail.call_count, 1) + address = mock_send_mail.call_args[0][1] + subject = mock_send_mail.call_args[0][3] + self.assertEqual(address, sub_to_all.email.address) + self.assertIn(draft.name, subject) - mailbox_before = len(outbox) rfc_state = State.objects.get(type="draft", slug="rfc") - add_state_change_event(draft, system, active_state, rfc_state) - self.assertEqual(len(outbox), mailbox_before + 1) - self.assertTrue(draft.name in outbox[-1]["Subject"]) + event = add_state_change_event(draft, system, active_state, rfc_state) + mock_send_mail.reset_mock() + notify_event_to_subscribers(event) + self.assertEqual(mock_send_mail.call_count, 2) + addresses = [call_args[0][1] for call_args in mock_send_mail.call_args_list] + subjects = {call_args[0][3] for call_args in mock_send_mail.call_args_list} + self.assertCountEqual( + addresses, + [sub_to_significant.email.address, sub_to_all.email.address], + ) + self.assertEqual(len(subjects), 1) + self.assertIn(draft.name, subjects.pop()) @mock.patch("ietf.community.utils.notify_event_to_subscribers") def test_notify_event_to_subscribers_task(self, mock_notify): From ff2a882c0f957d5f05641367fabdd54e7af71306 Mon Sep 17 00:00:00 2001 From: Jennifer Richards Date: Fri, 12 Jul 2024 10:10:49 -0300 Subject: [PATCH 11/12] fix: save, not update --- ietf/community/tests.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ietf/community/tests.py b/ietf/community/tests.py index c0efd1691b..1bebb9f5b4 100644 --- a/ietf/community/tests.py +++ b/ietf/community/tests.py @@ -458,7 +458,8 @@ def test_notification_signal_receiver(self, mock_notify_task): self.assertFalse(mock_notify_task.delay.called) del(d.skip_community_list_notification) - d.doc.update(type_id="rfc") # not "draft" + d.doc.type_id="rfc" # not "draft" + d.doc.save() # 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"): From be3a2b5ce2a48feaa59a77fed04a0e37c27c520f Mon Sep 17 00:00:00 2001 From: Jennifer Richards Date: Fri, 12 Jul 2024 10:52:41 -0300 Subject: [PATCH 12/12] test: restore template coverage --- ietf/community/tests.py | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/ietf/community/tests.py b/ietf/community/tests.py index 1bebb9f5b4..d76347b70a 100644 --- a/ietf/community/tests.py +++ b/ietf/community/tests.py @@ -466,8 +466,8 @@ def test_notification_signal_receiver(self, mock_notify_task): d.save() self.assertFalse(mock_notify_task.delay.called) - @mock.patch("ietf.community.utils.send_mail") - def test_notify_event_to_subscribers(self, mock_send_mail): + @mock.patch("ietf.utils.mail.send_mail_text") + def test_notify_event_to_subscribers(self, mock_send_mail_text): person = PersonFactory(user__username='plain') draft = WgDraftFactory() @@ -490,25 +490,30 @@ def test_notify_event_to_subscribers(self, mock_send_mail): system = Person.objects.get(name="(System)") event = add_state_change_event(draft, system, None, active_state) notify_event_to_subscribers(event) - self.assertEqual(mock_send_mail.call_count, 1) - address = mock_send_mail.call_args[0][1] - subject = mock_send_mail.call_args[0][3] + self.assertEqual(mock_send_mail_text.call_count, 1) + address = mock_send_mail_text.call_args[0][1] + subject = mock_send_mail_text.call_args[0][3] + content = mock_send_mail_text.call_args[0][4] self.assertEqual(address, sub_to_all.email.address) self.assertIn(draft.name, subject) + self.assertIn(clist.long_name(), content) rfc_state = State.objects.get(type="draft", slug="rfc") event = add_state_change_event(draft, system, active_state, rfc_state) - mock_send_mail.reset_mock() + mock_send_mail_text.reset_mock() notify_event_to_subscribers(event) - self.assertEqual(mock_send_mail.call_count, 2) - addresses = [call_args[0][1] for call_args in mock_send_mail.call_args_list] - subjects = {call_args[0][3] for call_args in mock_send_mail.call_args_list} + self.assertEqual(mock_send_mail_text.call_count, 2) + addresses = [call_args[0][1] for call_args in mock_send_mail_text.call_args_list] + subjects = {call_args[0][3] for call_args in mock_send_mail_text.call_args_list} + contents = {call_args[0][4] for call_args in mock_send_mail_text.call_args_list} self.assertCountEqual( addresses, [sub_to_significant.email.address, sub_to_all.email.address], ) self.assertEqual(len(subjects), 1) self.assertIn(draft.name, subjects.pop()) + self.assertEqual(len(contents), 1) + self.assertIn(clist.long_name(), contents.pop()) @mock.patch("ietf.community.utils.notify_event_to_subscribers") def test_notify_event_to_subscribers_task(self, mock_notify):