From 42c93b042502b9fa04f749739dff64e17b8220ca Mon Sep 17 00:00:00 2001 From: Matt Segal Date: Fri, 11 Aug 2023 21:57:58 +1000 Subject: [PATCH] feat(backend): fix notifiaction topic leakage (#104) --- app/core/factories.py | 34 +++++++- app/notify/admin.py | 2 +- app/notify/signals.py | 9 +- app/notify/tests/test_notifications.py | 115 +++++++++++++++++++++++++ app/pyproject.toml | 1 + 5 files changed, 158 insertions(+), 3 deletions(-) create mode 100644 app/notify/tests/test_notifications.py diff --git a/app/core/factories.py b/app/core/factories.py index 2a831c7b..a2a2cbf5 100644 --- a/app/core/factories.py +++ b/app/core/factories.py @@ -10,6 +10,14 @@ from accounts.models import User from emails.models import Email, EmailState, EmailAttachment from core.models import Client, FileUpload, Issue, Person, Tenancy +from core.models.issue import CaseStage +from notify.models import ( + Notification, + NOTIFY_TOPIC_CHOICES, + NotifyEvent, + NotifyChannel, + NotifyTarget, +) TINY_PNG = b"\x89PNG\r\n\x1a\n\x00\x00\x00\rIHDR\x00\x00\x00\x01\x00\x00\x00\x01\x08\x06\x00\x00\x00\x1f\x15\xc4\x89\x00\x00\x00\tpHYs\x00\x00\x0e\xc4\x00\x00\x0e\xc4\x01\x95+\x0e\x1b\x00\x00\x00\x19tEXtSoftware\x00gnome-screenshot\xef\x03\xbf>\x00\x00\x00\rIDAT\x08\x99c```\xf8\x0f\x00\x01\x04\x01\x00}\xb2\xc8\xdf\x00\x00\x00\x00IEND\xaeB`\x82" @@ -35,7 +43,6 @@ def username(self): class TimestampedModelFactory(factory.django.DjangoModelFactory): - modified_at = factory.Faker( "date_time_between", tzinfo=timezone.utc, start_date="-1m", end_date="now" ) @@ -158,3 +165,28 @@ def file(self, create, extracted, **kwargs): def get_dummy_file(name): f = io.BytesIO(TINY_PNG) return InMemoryUploadedFile(f, None, name, "image/png", len(TINY_PNG), None) + + +@factory.django.mute_signals(post_save) +class NotificationFactory(factory.django.DjangoModelFactory): + class Meta: + model = Notification + + name = factory.Faker("color_name") + topic = factory.Faker( + "random_element", elements=[c[0] for c in NOTIFY_TOPIC_CHOICES] + ) + event = factory.Faker( + "random_element", elements=[c[0] for c in NotifyEvent.choices] + ) + channel = factory.Faker( + "random_element", elements=[c[0] for c in NotifyChannel.choices] + ) + target = factory.Faker( + "random_element", elements=[c[0] for c in NotifyTarget.choices] + ) + event_stage = factory.Faker( + "random_element", elements=[c[0] for c in CaseStage.CHOICES] + ) + raw_text = factory.Faker("sentence") + message_text = factory.Faker("sentence") diff --git a/app/notify/admin.py b/app/notify/admin.py index ebe27f39..70a45908 100644 --- a/app/notify/admin.py +++ b/app/notify/admin.py @@ -4,7 +4,7 @@ @admin.register(Notification) -class EmailTemplateAdmin(admin.ModelAdmin): +class NotificationAdmin(admin.ModelAdmin): list_display = ( "id", "name", diff --git a/app/notify/signals.py b/app/notify/signals.py index aa024616..98981f6d 100644 --- a/app/notify/signals.py +++ b/app/notify/signals.py @@ -41,6 +41,9 @@ def on_issue_stage_change(issue_pk, old_stage: str, new_stage: str): event=NotifyEvent.STAGE_CHANGE, event_stage=new_stage ) for notification in notifications: + if notification.topic not in (issue.topic, "GENERAL"): + continue + if not notification.channel == NotifyChannel.SLACK: logger.error("Notification[%s] has unsupported channel", notification) continue @@ -75,5 +78,9 @@ def on_issue_stage_change(issue_pk, old_stage: str, new_stage: str): ) continue - message_text = f"*Notification for case <{issue.url}|{issue.fileref}>*\n{notification.message_text}" + message_text = get_notification_message_text(issue, notification) send_slack_direct_message(message_text, slack_user["id"]) + + +def get_notification_message_text(issue: Issue, notification: Notification): + return f"*Notification for case <{issue.url}|{issue.fileref}>*\n{notification.message_text}" diff --git a/app/notify/tests/test_notifications.py b/app/notify/tests/test_notifications.py new file mode 100644 index 00000000..1f2d5d0f --- /dev/null +++ b/app/notify/tests/test_notifications.py @@ -0,0 +1,115 @@ +from unittest.mock import patch, call + +import pytest + +from core.factories import IssueFactory, NotificationFactory, UserFactory +from notify.signals import get_notification_message_text + +TEST_SLACK_USER_ID = "some-slack-id" + + +def mock_get_slack_user_by_email(email: str): + return {"id": f"slack-{email}"} + + +@pytest.mark.django_db +@patch("notify.signals.get_slack_user_by_email", mock_get_slack_user_by_email) +@patch("notify.signals.send_slack_direct_message") +def test_send_notifcation__with_same_issue_topic(mock_send_message): + paralegal = UserFactory() + issue = IssueFactory(topic="REPAIRS", stage="UNSTARTED", paralegal=paralegal) + notification = NotificationFactory( + topic="REPAIRS", + channel="SLACK", + event="STAGE_CHANGE", + event_stage="CLIENT_AGREEMENT", + target="PARALEGAL", + ) + + assert mock_send_message.call_count == 0 + issue.stage = "CLIENT_AGREEMENT" + issue.save() + assert mock_send_message.call_count == 1 + mock_send_message.assert_has_calls( + [ + call( + get_notification_message_text(issue, notification), + f"slack-{paralegal.email}", + ) + ] + ) + + +@pytest.mark.django_db +@patch("notify.signals.get_slack_user_by_email", mock_get_slack_user_by_email) +@patch("notify.signals.send_slack_direct_message") +def test_send_notifcation__with_different_issue_topic(mock_send_message): + issue = IssueFactory(topic="BONDS", stage="UNSTARTED", paralegal=UserFactory()) + NotificationFactory( + topic="REPAIRS", + channel="SLACK", + event="STAGE_CHANGE", + event_stage="CLIENT_AGREEMENT", + target="PARALEGAL", + ) + + assert mock_send_message.call_count == 0 + issue.stage = "CLIENT_AGREEMENT" + issue.save() + assert mock_send_message.call_count == 0 + + +@pytest.mark.django_db +@patch("notify.signals.get_slack_user_by_email", mock_get_slack_user_by_email) +@patch("notify.signals.send_slack_direct_message") +def test_send_notifcation__with_general_issue_topic(mock_send_message): + paralegal = UserFactory() + issue = IssueFactory(topic="BONDS", stage="UNSTARTED", paralegal=paralegal) + notification = NotificationFactory( + topic="GENERAL", + channel="SLACK", + event="STAGE_CHANGE", + event_stage="CLIENT_AGREEMENT", + target="PARALEGAL", + ) + + assert mock_send_message.call_count == 0 + issue.stage = "CLIENT_AGREEMENT" + issue.save() + assert mock_send_message.call_count == 1 + mock_send_message.assert_has_calls( + [ + call( + get_notification_message_text(issue, notification), + f"slack-{paralegal.email}", + ) + ] + ) + + +@pytest.mark.django_db +@patch("notify.signals.get_slack_user_by_email", mock_get_slack_user_by_email) +@patch("notify.signals.send_slack_direct_message") +def test_send_notifcation__with_lawyer_target(mock_send_message): + lawyer = UserFactory() + issue = IssueFactory(topic="BONDS", stage="UNSTARTED", lawyer=lawyer) + notification = NotificationFactory( + topic="GENERAL", + channel="SLACK", + event="STAGE_CHANGE", + event_stage="CLIENT_AGREEMENT", + target="LAWYER", + ) + + assert mock_send_message.call_count == 0 + issue.stage = "CLIENT_AGREEMENT" + issue.save() + assert mock_send_message.call_count == 1 + mock_send_message.assert_has_calls( + [ + call( + get_notification_message_text(issue, notification), + f"slack-{lawyer.email}", + ) + ] + ) diff --git a/app/pyproject.toml b/app/pyproject.toml index 91f981d2..a1225c76 100644 --- a/app/pyproject.toml +++ b/app/pyproject.toml @@ -4,3 +4,4 @@ filterwarnings = [ "ignore::django.utils.deprecation.RemovedInDjango50Warning", "ignore::wagtail.utils.deprecation.RemovedInWagtail217Warning", ] +addopts = "--reuse-db --ds=clerk.settings.test"