From ac70a2d2e1b292cea558c7de31423aac988907f8 Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Bodas Date: Tue, 10 Aug 2021 19:11:41 +0530 Subject: [PATCH] notifications: Fix unnecessary wildcard mention notifications. This fixes a bug where email notifications were sent for wildcard mentions even if the `enable_offline_email_notifications` setting was turned off. This was because the `notification_data` class incorrectly considered `wildcard_mentions_notify` as an indeoendent setting, instead of a wrapper around `enable_offline_email_notifications` and `enable_offline_push_notifications`. Also add a test for this case. --- zerver/lib/notification_data.py | 26 ++++++++---- zerver/lib/test_classes.py | 3 +- zerver/tests/test_event_queue.py | 41 +++++++++++++++++-- .../tests/test_message_edit_notifications.py | 3 +- zerver/tests/test_notification_data.py | 16 +++++--- zerver/tornado/event_queue.py | 3 +- 6 files changed, 72 insertions(+), 20 deletions(-) diff --git a/zerver/lib/notification_data.py b/zerver/lib/notification_data.py index a90cd7d45d..92d35d88c7 100644 --- a/zerver/lib/notification_data.py +++ b/zerver/lib/notification_data.py @@ -13,9 +13,10 @@ class UserMessageNotificationsData: pm_push_notify: bool mention_email_notify: bool mention_push_notify: bool + wildcard_mention_email_notify: bool + wildcard_mention_push_notify: bool stream_push_notify: bool stream_email_notify: bool - wildcard_mention_notify: bool sender_is_muted: bool def __post_init__(self) -> None: @@ -41,29 +42,40 @@ class UserMessageNotificationsData: wildcard_mention_user_ids: Set[int], muted_sender_user_ids: Set[int], ) -> "UserMessageNotificationsData": - wildcard_mention_notify = ( - user_id in wildcard_mention_user_ids and "wildcard_mentioned" in flags - ) + # `wildcard_mention_user_ids` are those user IDs for whom wildcard mentions should + # obey notification settings of personal mentions. Hence, it isn't an independent + # notification setting and acts as a wrapper. pm_email_notify = user_id not in pm_mention_email_disabled_user_ids and private_message mention_email_notify = ( user_id not in pm_mention_email_disabled_user_ids and "mentioned" in flags ) + wildcard_mention_email_notify = ( + user_id in wildcard_mention_user_ids + and user_id not in pm_mention_email_disabled_user_ids + and "wildcard_mentioned" in flags + ) pm_push_notify = user_id not in pm_mention_push_disabled_user_ids and private_message mention_push_notify = ( user_id not in pm_mention_push_disabled_user_ids and "mentioned" in flags ) + wildcard_mention_push_notify = ( + user_id in wildcard_mention_user_ids + and user_id not in pm_mention_push_disabled_user_ids + and "wildcard_mentioned" in flags + ) return cls( user_id=user_id, pm_email_notify=pm_email_notify, mention_email_notify=mention_email_notify, + wildcard_mention_email_notify=wildcard_mention_email_notify, pm_push_notify=pm_push_notify, mention_push_notify=mention_push_notify, + wildcard_mention_push_notify=wildcard_mention_push_notify, online_push_enabled=(user_id in online_push_user_ids), stream_push_notify=(user_id in stream_push_user_ids), stream_email_notify=(user_id in stream_email_user_ids), - wildcard_mention_notify=wildcard_mention_notify, sender_is_muted=(user_id in muted_sender_user_ids), ) @@ -95,7 +107,7 @@ class UserMessageNotificationsData: return NotificationTriggers.PRIVATE_MESSAGE elif self.mention_push_notify: return NotificationTriggers.MENTION - elif self.wildcard_mention_notify: + elif self.wildcard_mention_push_notify: return NotificationTriggers.WILDCARD_MENTION elif self.stream_push_notify: return NotificationTriggers.STREAM_PUSH @@ -122,7 +134,7 @@ class UserMessageNotificationsData: return NotificationTriggers.PRIVATE_MESSAGE elif self.mention_email_notify: return NotificationTriggers.MENTION - elif self.wildcard_mention_notify: + elif self.wildcard_mention_email_notify: return NotificationTriggers.WILDCARD_MENTION elif self.stream_email_notify: return NotificationTriggers.STREAM_EMAIL diff --git a/zerver/lib/test_classes.py b/zerver/lib/test_classes.py index 79a65b4479..364e5f0a28 100644 --- a/zerver/lib/test_classes.py +++ b/zerver/lib/test_classes.py @@ -1344,9 +1344,10 @@ Output: pm_push_notify=kwargs.get("pm_push_notify", False), mention_email_notify=kwargs.get("mention_email_notify", False), mention_push_notify=kwargs.get("mention_push_notify", False), + wildcard_mention_email_notify=kwargs.get("wildcard_mention_email_notify", False), + wildcard_mention_push_notify=kwargs.get("wildcard_mention_push_notify", False), stream_email_notify=kwargs.get("stream_email_notify", False), stream_push_notify=kwargs.get("stream_push_notify", False), - wildcard_mention_notify=kwargs.get("wildcard_mention_notify", False), sender_is_muted=kwargs.get("sender_is_muted", False), ) diff --git a/zerver/tests/test_event_queue.py b/zerver/tests/test_event_queue.py index 3bf7f25653..1ef581c341 100644 --- a/zerver/tests/test_event_queue.py +++ b/zerver/tests/test_event_queue.py @@ -310,7 +310,8 @@ class MissedMessageNotificationsTest(ZulipTestCase): assert_maybe_enqueue_notifications_call_args( args_dict=args_dict, message_id=msg_id, - wildcard_mention_notify=True, + wildcard_mention_email_notify=True, + wildcard_mention_push_notify=True, already_notified={"email_notified": True, "push_notified": True}, ) destroy_event_queue(client_descriptor.event_queue.id) @@ -327,7 +328,8 @@ class MissedMessageNotificationsTest(ZulipTestCase): assert_maybe_enqueue_notifications_call_args( args_dict=args_dict, - wildcard_mention_notify=False, + wildcard_mention_email_notify=False, + wildcard_mention_push_notify=False, message_id=msg_id, already_notified={"email_notified": False, "push_notified": False}, ) @@ -348,7 +350,8 @@ class MissedMessageNotificationsTest(ZulipTestCase): assert_maybe_enqueue_notifications_call_args( args_dict=args_dict, message_id=msg_id, - wildcard_mention_notify=False, + wildcard_mention_email_notify=False, + wildcard_mention_push_notify=False, already_notified={"email_notified": False, "push_notified": False}, ) destroy_event_queue(client_descriptor.event_queue.id) @@ -372,7 +375,8 @@ class MissedMessageNotificationsTest(ZulipTestCase): assert_maybe_enqueue_notifications_call_args( args_dict=args_dict, message_id=msg_id, - wildcard_mention_notify=True, + wildcard_mention_email_notify=True, + wildcard_mention_push_notify=True, already_notified={"email_notified": True, "push_notified": True}, ) destroy_event_queue(client_descriptor.event_queue.id) @@ -381,6 +385,35 @@ class MissedMessageNotificationsTest(ZulipTestCase): user_profile.save() sub.save() + # If notifications for personal mentions themselves have been turned off, + # even turning on `wildcard_mentions_notify` should not send notifications + user_profile.enable_offline_email_notifications = False + user_profile.wildcard_mentions_notify = True + user_profile.save() + client_descriptor = allocate_event_queue() + self.assertTrue(client_descriptor.event_queue.empty()) + msg_id = self.send_stream_message(iago, "Denmark", content="@**all** what's up?") + with mock.patch("zerver.tornado.event_queue.maybe_enqueue_notifications") as mock_enqueue: + missedmessage_hook(user_profile.id, client_descriptor, True) + mock_enqueue.assert_called_once() + args_dict = mock_enqueue.call_args_list[0][1] + + # We've turned off email notifications for personal mentions, but push notifications + # for personal mentions are still on. + # Because `wildcard_mentions_notify` is True, a message with `@all` should follow the + # personal mention settings + assert_maybe_enqueue_notifications_call_args( + args_dict=args_dict, + message_id=msg_id, + wildcard_mention_email_notify=False, + wildcard_mention_push_notify=True, + already_notified={"email_notified": False, "push_notified": True}, + ) + destroy_event_queue(client_descriptor.event_queue.id) + user_profile.enable_offline_email_notifications = True + user_profile.wildcard_mentions_notify = True + user_profile.save() + # Test with a user group mention hamlet_and_cordelia = create_user_group( "hamlet_and_cordelia", [user_profile, cordelia], cordelia.realm diff --git a/zerver/tests/test_message_edit_notifications.py b/zerver/tests/test_message_edit_notifications.py index 40f36500e0..0c30e0d893 100644 --- a/zerver/tests/test_message_edit_notifications.py +++ b/zerver/tests/test_message_edit_notifications.py @@ -420,7 +420,8 @@ class EditMessageSideEffectsTest(ZulipTestCase): user_id=cordelia.id, acting_user_id=hamlet.id, message_id=message_id, - wildcard_mention_notify=True, + wildcard_mention_email_notify=True, + wildcard_mention_push_notify=True, already_notified={}, ) self.assertEqual(info["enqueue_kwargs"], expected_enqueue_kwargs) diff --git a/zerver/tests/test_notification_data.py b/zerver/tests/test_notification_data.py index 876ed4a8ae..51ce3c1009 100644 --- a/zerver/tests/test_notification_data.py +++ b/zerver/tests/test_notification_data.py @@ -37,7 +37,7 @@ class TestNotificationData(ZulipTestCase): # Wildcard mention user_data = self.create_user_notifications_data_object( - user_id=user_id, wildcard_mention_notify=True + user_id=user_id, wildcard_mention_push_notify=True ) self.assertEqual( user_data.get_push_notification_trigger(acting_user_id=acting_user_id, idle=True), @@ -79,11 +79,12 @@ class TestNotificationData(ZulipTestCase): user_data = self.create_user_notifications_data_object( user_id=user_id, sender_is_muted=True, - wildcard_mention_notify=True, pm_push_notify=True, pm_email_notify=True, mention_push_notify=True, mention_email_notify=True, + wildcard_mention_push_notify=True, + wildcard_mention_email_notify=True, ) self.assertEqual( user_data.get_push_notification_trigger(acting_user_id=acting_user_id, idle=True), @@ -94,11 +95,12 @@ class TestNotificationData(ZulipTestCase): # Message sender is the user the object corresponds to. user_data = self.create_user_notifications_data_object( user_id=acting_user_id, - wildcard_mention_notify=True, pm_push_notify=True, pm_email_notify=True, mention_push_notify=True, mention_email_notify=True, + wildcard_mention_push_notify=True, + wildcard_mention_email_notify=True, ) self.assertEqual( user_data.get_push_notification_trigger(acting_user_id=acting_user_id, idle=True), @@ -140,7 +142,7 @@ class TestNotificationData(ZulipTestCase): # Wildcard mention user_data = self.create_user_notifications_data_object( - user_id=user_id, wildcard_mention_notify=True + user_id=user_id, wildcard_mention_email_notify=True ) self.assertEqual( user_data.get_email_notification_trigger(acting_user_id=acting_user_id, idle=True), @@ -173,11 +175,12 @@ class TestNotificationData(ZulipTestCase): user_data = self.create_user_notifications_data_object( user_id=user_id, sender_is_muted=True, - wildcard_mention_notify=True, pm_push_notify=True, pm_email_notify=True, mention_push_notify=True, mention_email_notify=True, + wildcard_mention_push_notify=True, + wildcard_mention_email_notify=True, ) self.assertEqual( user_data.get_email_notification_trigger(acting_user_id=acting_user_id, idle=True), @@ -188,11 +191,12 @@ class TestNotificationData(ZulipTestCase): # Message sender is the user the object corresponds to. user_data = self.create_user_notifications_data_object( user_id=acting_user_id, - wildcard_mention_notify=True, pm_push_notify=True, pm_email_notify=True, mention_push_notify=True, mention_email_notify=True, + wildcard_mention_push_notify=True, + wildcard_mention_email_notify=True, ) self.assertEqual( user_data.get_email_notification_trigger(acting_user_id=acting_user_id, idle=True), diff --git a/zerver/tornado/event_queue.py b/zerver/tornado/event_queue.py index 02c5cb75a8..dade5d769e 100644 --- a/zerver/tornado/event_queue.py +++ b/zerver/tornado/event_queue.py @@ -762,9 +762,10 @@ def missedmessage_hook( pm_email_notify=internal_data.get("pm_email_notify", False), mention_push_notify=internal_data.get("mention_push_notify", False), mention_email_notify=internal_data.get("mention_email_notify", False), + wildcard_mention_push_notify=internal_data.get("wildcard_mention_push_notify", False), + wildcard_mention_email_notify=internal_data.get("wildcard_mention_email_notify", False), stream_push_notify=internal_data.get("stream_push_notify", False), stream_email_notify=internal_data.get("stream_email_notify", False), - wildcard_mention_notify=internal_data.get("wildcard_mention_notify", False), # Since one is by definition idle, we don't need to check online_push_enabled online_push_enabled=False, )