From 1fe4f795afc5928c4a809c83823fc93425f88ec8 Mon Sep 17 00:00:00 2001 From: Tim Abbott Date: Tue, 3 Sep 2019 14:27:45 -0700 Subject: [PATCH] settings: Add notification settings checkboxes for wildcard mentions. This change makes it possible for users to control the notification settings for wildcard mentions as a separate control from PMs and direct @-mentions. --- frontend_tests/node_tests/templates.js | 2 + static/js/settings.js | 1 + static/js/settings_notifications.js | 1 + .../zerver/help/mention-a-user-or-group.md | 4 +- .../help/pm-mention-alert-notifications.md | 19 +++- zerver/lib/actions.py | 52 ++++++++- ...53_userprofile_wildcard_mentions_notify.py | 25 +++++ zerver/models.py | 3 + zerver/openapi/zulip.yaml | 6 + zerver/tests/test_event_queue.py | 103 +++++++++++++----- zerver/tests/test_events.py | 1 + zerver/tests/test_home.py | 1 + .../tests/test_message_edit_notifications.py | 80 +++++++++++++- zerver/tests/test_users.py | 56 +++++++++- zerver/tornado/event_queue.py | 43 +++++--- zerver/views/user_settings.py | 1 + 16 files changed, 345 insertions(+), 53 deletions(-) create mode 100644 zerver/migrations/0253_userprofile_wildcard_mentions_notify.py diff --git a/frontend_tests/node_tests/templates.js b/frontend_tests/node_tests/templates.js index 6d72895dbe..a033a045e0 100644 --- a/frontend_tests/node_tests/templates.js +++ b/frontend_tests/node_tests/templates.js @@ -1096,6 +1096,7 @@ run_test('settings_tab', () => { realm_digest_emails_enabled: true, realm_name_in_notifications: true, realm_push_notifications_enabled: true, + wildcard_mentions_notify: true, }; const page_params = $.extend(page_param_checkbox_options, { full_name: "Alyssa P. Hacker", password_auth_enabled: true, @@ -1112,6 +1113,7 @@ run_test('settings_tab', () => { "enable_online_push_notifications", "enable_digest_emails", "realm_name_in_notifications", + "wildcard_mentions_notify", ]; // Render with all booleans set to true. diff --git a/static/js/settings.js b/static/js/settings.js index 81a7eca896..46da223f87 100644 --- a/static/js/settings.js +++ b/static/js/settings.js @@ -67,6 +67,7 @@ function setup_settings_label() { enable_stream_audible_notifications: i18n.t("Audible desktop notifications"), enable_stream_push_notifications: i18n.t("Mobile notifications"), enable_stream_email_notifications: i18n.t("Email notifications"), + wildcard_mentions_notify: i18n.t("Notifications for @all/@everyone mentions"), // pm_mention_notification_settings enable_desktop_notifications: i18n.t("Visual desktop notifications"), diff --git a/static/js/settings_notifications.js b/static/js/settings_notifications.js index 61c55bfadd..ba6823f725 100644 --- a/static/js/settings_notifications.js +++ b/static/js/settings_notifications.js @@ -3,6 +3,7 @@ const stream_notification_settings = [ "enable_stream_audible_notifications", "enable_stream_push_notifications", "enable_stream_email_notifications", + "wildcard_mentions_notify" ]; const pm_mention_notification_settings = [ diff --git a/templates/zerver/help/mention-a-user-or-group.md b/templates/zerver/help/mention-a-user-or-group.md index 0e85677c53..3ab4bf16b9 100644 --- a/templates/zerver/help/mention-a-user-or-group.md +++ b/templates/zerver/help/mention-a-user-or-group.md @@ -49,4 +49,6 @@ notification. Silent mentions start with `@_` instead of `@`. ## Mention everyone on a stream You can mention everyone on a stream with the `@**all**` mention. Use -sparingly! Note that this will not notify anyone who has muted the stream. +sparingly! Note that this will not notify anyone who has muted the +stream, and users can disable receiving email/push notifications for +these wildcard mentions. diff --git a/templates/zerver/help/pm-mention-alert-notifications.md b/templates/zerver/help/pm-mention-alert-notifications.md index 190d770821..4dd9fc299f 100644 --- a/templates/zerver/help/pm-mention-alert-notifications.md +++ b/templates/zerver/help/pm-mention-alert-notifications.md @@ -12,15 +12,28 @@ You can configure desktop, mobile, and email notifications for {end_tabs} -These settings will affect notifications for private messages, group private -messages, mentions, and alert words. The one exception is that we never -send email notifications for `@all` or `@everyone` mentions. +These settings will affect notifications for private messages, group +private messages, mentions, and alert words. You can also hide the content of private messages (and group private messages) from desktop notifications. Under **Other notification settings**, uncheck **Include content of private messages in desktop notifications**. +## Wildcard mentions + +By default, wildcard mentions (`@**all**`, `@**everyone**`) trigger +email/push notifications as though they were personal @-mentions. You +can toggle whether you receive notifications for wildcard mentions: + +{start_tabs} + +{settings_tab|notifications} + +1. Under **Stream messages**, toggle **Notifications for @all/@everyone mentions**. + +{end_tabs} + ## Related articles * [Add an alert word](/help/add-an-alert-word) diff --git a/zerver/lib/actions.py b/zerver/lib/actions.py index 02569f62cb..72687ab88d 100644 --- a/zerver/lib/actions.py +++ b/zerver/lib/actions.py @@ -1002,8 +1002,9 @@ def get_typing_user_profiles(recipient: Recipient, sender_id: int) -> List[UserP RecipientInfoResult = TypedDict('RecipientInfoResult', { 'active_user_ids': Set[int], 'push_notify_user_ids': Set[int], - 'stream_push_user_ids': Set[int], 'stream_email_user_ids': Set[int], + 'stream_push_user_ids': Set[int], + 'wildcard_mention_user_ids': Set[int], 'um_eligible_user_ids': Set[int], 'long_term_idle_user_ids': Set[int], 'default_bot_user_ids': Set[int], @@ -1013,9 +1014,11 @@ RecipientInfoResult = TypedDict('RecipientInfoResult', { def get_recipient_info(recipient: Recipient, sender_id: int, stream_topic: Optional[StreamTopicTarget], - possibly_mentioned_user_ids: Optional[Set[int]]=None) -> RecipientInfoResult: + possibly_mentioned_user_ids: Optional[Set[int]]=None, + possible_wildcard_mention: bool=True) -> RecipientInfoResult: stream_push_user_ids = set() # type: Set[int] stream_email_user_ids = set() # type: Set[int] + wildcard_mention_user_ids = set() # type: Set[int] if recipient.type == Recipient.PERSONAL: # The sender and recipient may be the same id, so @@ -1033,12 +1036,16 @@ def get_recipient_info(recipient: Recipient, subscription_rows = stream_topic.get_active_subscriptions().annotate( user_profile_email_notifications=F('user_profile__enable_stream_email_notifications'), user_profile_push_notifications=F('user_profile__enable_stream_push_notifications'), + user_profile_wildcard_mentions_notify=F( + 'user_profile__wildcard_mentions_notify'), ).values( 'user_profile_id', 'push_notifications', 'email_notifications', + 'wildcard_mentions_notify', 'user_profile_email_notifications', 'user_profile_push_notifications', + 'user_profile_wildcard_mentions_notify', 'is_muted', ).order_by('user_profile_id') @@ -1073,6 +1080,23 @@ def get_recipient_info(recipient: Recipient, if should_send('email_notifications', row) } + if possible_wildcard_mention: + # If there's a possible wildcard mention, we need to + # determine which users would receive a wildcard mention + # notification for this message should the message indeed + # contain a wildcard mention. + # + # We don't have separate values for push/email + # notifications here; at this stage, we're just + # determining whether this wildcard mention should be + # treated as a mention (and follow the user's mention + # notification preferences) or a normal message. + wildcard_mention_user_ids = { + row['user_profile_id'] + for row in subscription_rows + if should_send("wildcard_mentions_notify", row) + } + elif recipient.type == Recipient.HUDDLE: message_to_user_ids = get_huddle_user_ids(recipient) @@ -1171,6 +1195,7 @@ def get_recipient_info(recipient: Recipient, push_notify_user_ids=push_notify_user_ids, stream_push_user_ids=stream_push_user_ids, stream_email_user_ids=stream_email_user_ids, + wildcard_mention_user_ids=wildcard_mention_user_ids, um_eligible_user_ids=um_eligible_user_ids, long_term_idle_user_ids=long_term_idle_user_ids, default_bot_user_ids=default_bot_user_ids, @@ -1313,6 +1338,13 @@ def do_send_messages(messages_maybe_none: Sequence[Optional[MutableMapping[str, sender_id=message['message'].sender_id, stream_topic=stream_topic, possibly_mentioned_user_ids=mention_data.get_user_ids(), + # TODO: We should improve the `mention_data` logic to + # populate the possible_wildcard_mention field based on + # whether wildcard mention syntax actually appears in the + # message, to avoid wasting resources computing + # wildcard_mention_user_ids for messages that could + # not possibly contain a wildcard mention. + possible_wildcard_mention=True, ) message['active_user_ids'] = info['active_user_ids'] @@ -1344,6 +1376,15 @@ def do_send_messages(messages_maybe_none: Sequence[Optional[MutableMapping[str, members = message['mention_data'].get_group_members(group_id) message['message'].mentions_user_ids.update(members) + # Only send data to Tornado about wildcard mentions if message + # rendering determined the message had an actual wildcard + # mention in it (and not e.g. wildcard mention syntax inside a + # code block). + if message['message'].mentions_wildcard: + message['wildcard_mention_user_ids'] = info['wildcard_mention_user_ids'] + else: + message['wildcard_mention_user_ids'] = [] + ''' Once we have the actual list of mentioned ids from message rendering, we can patch in "default bots" (aka normal bots) @@ -1445,6 +1486,7 @@ def do_send_messages(messages_maybe_none: Sequence[Optional[MutableMapping[str, always_push_notify=(user_id in message['push_notify_user_ids']), stream_push_notify=(user_id in message['stream_push_user_ids']), stream_email_notify=(user_id in message['stream_email_user_ids']), + wildcard_mention_notify=(user_id in message['wildcard_mention_user_ids']), ) for user_id in user_ids ] @@ -4473,11 +4515,11 @@ def do_update_message(user_profile: UserProfile, message: Message, topic_name: O else: stream_topic = None - # TODO: We may want a slightly leaner of this function for updates. info = get_recipient_info( recipient=message.recipient, sender_id=message.sender_id, stream_topic=stream_topic, + possible_wildcard_mention=True, ) event['push_notify_user_ids'] = list(info['push_notify_user_ids']) @@ -4486,6 +4528,10 @@ def do_update_message(user_profile: UserProfile, message: Message, topic_name: O event['prior_mention_user_ids'] = list(prior_mention_user_ids) event['mention_user_ids'] = list(mention_user_ids) event['presence_idle_user_ids'] = filter_presence_idle_user_ids(info['active_user_ids']) + if message.mentions_wildcard: + event['wildcard_mention_user_ids'] = list(info['wildcard_mention_user_ids']) + else: + event['wildcard_mention_user_ids'] = [] if topic_name is not None: orig_topic_name = message.topic_name() diff --git a/zerver/migrations/0253_userprofile_wildcard_mentions_notify.py b/zerver/migrations/0253_userprofile_wildcard_mentions_notify.py new file mode 100644 index 0000000000..3f85f37c6e --- /dev/null +++ b/zerver/migrations/0253_userprofile_wildcard_mentions_notify.py @@ -0,0 +1,25 @@ +# -*- coding: utf-8 -*- +# Generated by Django 1.11.25 on 2019-11-06 23:43 +from __future__ import unicode_literals + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('zerver', '0252_realm_user_group_edit_policy'), + ] + + operations = [ + migrations.AddField( + model_name='userprofile', + name='wildcard_mentions_notify', + field=models.BooleanField(default=True), + ), + migrations.AddField( + model_name='subscription', + name='wildcard_mentions_notify', + field=models.NullBooleanField(default=None), + ), + ] diff --git a/zerver/models.py b/zerver/models.py index 928986ccc9..38f189015b 100644 --- a/zerver/models.py +++ b/zerver/models.py @@ -860,6 +860,7 @@ class UserProfile(AbstractBaseUser, PermissionsMixin): enable_stream_push_notifications = models.BooleanField(default=False) # type: bool enable_stream_audible_notifications = models.BooleanField(default=False) # type: bool notification_sound = models.CharField(max_length=20, default='zulip') # type: str + wildcard_mentions_notify = models.BooleanField(default=True) # type: bool # PM + @-mention notifications. enable_desktop_notifications = models.BooleanField(default=True) # type: bool @@ -1004,6 +1005,7 @@ class UserProfile(AbstractBaseUser, PermissionsMixin): enable_stream_email_notifications=bool, enable_stream_push_notifications=bool, enable_stream_audible_notifications=bool, + wildcard_mentions_notify=bool, message_content_in_email_notifications=bool, notification_sound=str, pm_content_in_desktop_notifications=bool, @@ -2079,6 +2081,7 @@ class Subscription(models.Model): audible_notifications = models.NullBooleanField(default=None) # type: Optional[bool] push_notifications = models.NullBooleanField(default=None) # type: Optional[bool] email_notifications = models.NullBooleanField(default=None) # type: Optional[bool] + wildcard_mentions_notify = models.NullBooleanField(default=None) # type: Optional[bool] class Meta: unique_together = ("user_profile", "recipient") diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index 5719b0ca4b..f23283108f 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -2355,6 +2355,12 @@ paths: schema: type: boolean example: true + - name: wildcard_mentions_notify + in: query + description: Whether wildcard mentions (E.g. @**all**) should send notifications like a personal mention. + schema: + type: boolean + example: true - name: desktop_icon_count_display in: query description: > diff --git a/zerver/tests/test_event_queue.py b/zerver/tests/test_event_queue.py index 4974e394e0..5e5bc2fdf5 100644 --- a/zerver/tests/test_event_queue.py +++ b/zerver/tests/test_event_queue.py @@ -47,7 +47,7 @@ class MissedMessageNotificationsTest(ZulipTestCase): # Boring message doesn't send a notice email_notice, mobile_notice = self.check_will_notify( user_profile.id, message_id, private_message=False, - mentioned=False, wildcard_mentioned=False, + mentioned=False, wildcard_mention_notify=False, stream_push_notify=False, stream_email_notify=False, stream_name=None, always_push_notify=False, idle=True, already_notified={}) self.assertTrue(email_notice is None) @@ -56,7 +56,7 @@ class MissedMessageNotificationsTest(ZulipTestCase): # Private message sends a notice email_notice, mobile_notice = self.check_will_notify( user_profile.id, message_id, private_message=True, - mentioned=False, wildcard_mentioned=False, + mentioned=False, wildcard_mention_notify=False, stream_push_notify=False, stream_email_notify=True, stream_name=None, always_push_notify=False, idle=True, already_notified={}) self.assertTrue(email_notice is not None) @@ -66,7 +66,7 @@ class MissedMessageNotificationsTest(ZulipTestCase): # already sent notices before. email_notice, mobile_notice = self.check_will_notify( user_profile.id, message_id, private_message=True, - mentioned=False, wildcard_mentioned=False, + mentioned=False, wildcard_mention_notify=False, stream_push_notify=False, stream_email_notify=False, stream_name=None, always_push_notify=False, idle=True, already_notified={ 'push_notified': True, @@ -77,7 +77,7 @@ class MissedMessageNotificationsTest(ZulipTestCase): email_notice, mobile_notice = self.check_will_notify( user_profile.id, message_id, private_message=True, - mentioned=False, wildcard_mentioned=False, + mentioned=False, wildcard_mention_notify=False, stream_push_notify=False, stream_email_notify=False, stream_name=None, always_push_notify=False, idle=True, already_notified={ 'push_notified': False, @@ -89,16 +89,18 @@ class MissedMessageNotificationsTest(ZulipTestCase): # Mention sends a notice email_notice, mobile_notice = self.check_will_notify( user_profile.id, message_id, private_message=False, - mentioned=True, wildcard_mentioned=False, + mentioned=True, wildcard_mention_notify=False, stream_push_notify=False, stream_email_notify=False, stream_name=None, always_push_notify=False, idle=True, already_notified={}) self.assertTrue(email_notice is not None) self.assertTrue(mobile_notice is not None) - # Wildcard mention sends a notice + # Wildcard mention triggers both email and push notices (Like a + # direct mention, whether the notice is actually delivered is + # determined later, in the email/push notification code) email_notice, mobile_notice = self.check_will_notify( user_profile.id, message_id, private_message=False, - mentioned=False, wildcard_mentioned=True, + mentioned=False, wildcard_mention_notify=True, stream_push_notify=False, stream_email_notify=False, stream_name=None, always_push_notify=False, idle=True, already_notified={}) self.assertTrue(email_notice is not None) @@ -107,7 +109,7 @@ class MissedMessageNotificationsTest(ZulipTestCase): # stream_push_notify pushes but doesn't email email_notice, mobile_notice = self.check_will_notify( user_profile.id, message_id, private_message=False, - mentioned=False, wildcard_mentioned=False, + mentioned=False, wildcard_mention_notify=False, stream_push_notify=True, stream_email_notify=False, stream_name="Denmark", always_push_notify=False, idle=True, already_notified={}) self.assertTrue(email_notice is None) @@ -116,7 +118,7 @@ class MissedMessageNotificationsTest(ZulipTestCase): # stream_email_notify emails but doesn't push email_notice, mobile_notice = self.check_will_notify( user_profile.id, message_id, private_message=False, - mentioned=False, wildcard_mentioned=False, + mentioned=False, wildcard_mention_notify=False, stream_push_notify=False, stream_email_notify=True, stream_name="Denmark", always_push_notify=False, idle=True, already_notified={}) self.assertTrue(email_notice is not None) @@ -125,7 +127,7 @@ class MissedMessageNotificationsTest(ZulipTestCase): # Private message doesn't send a notice if not idle email_notice, mobile_notice = self.check_will_notify( user_profile.id, message_id, private_message=True, - mentioned=False, wildcard_mentioned=False, + mentioned=False, wildcard_mention_notify=False, stream_push_notify=False, stream_email_notify=True, stream_name=None, always_push_notify=False, idle=False, already_notified={}) self.assertTrue(email_notice is None) @@ -134,7 +136,7 @@ class MissedMessageNotificationsTest(ZulipTestCase): # Mention doesn't send a notice if not idle email_notice, mobile_notice = self.check_will_notify( user_profile.id, message_id, private_message=False, - mentioned=True, wildcard_mentioned=False, + mentioned=True, wildcard_mention_notify=False, stream_push_notify=False, stream_email_notify=False, stream_name=None, always_push_notify=False, idle=False, already_notified={}) self.assertTrue(email_notice is None) @@ -143,7 +145,7 @@ class MissedMessageNotificationsTest(ZulipTestCase): # Wildcard mention doesn't send a notice if not idle email_notice, mobile_notice = self.check_will_notify( user_profile.id, message_id, private_message=False, - mentioned=False, wildcard_mentioned=True, + mentioned=False, wildcard_mention_notify=True, stream_push_notify=False, stream_email_notify=False, stream_name=None, always_push_notify=False, idle=False, already_notified={}) self.assertTrue(email_notice is None) @@ -152,7 +154,7 @@ class MissedMessageNotificationsTest(ZulipTestCase): # Private message sends push but not email if not idle but always_push_notify email_notice, mobile_notice = self.check_will_notify( user_profile.id, message_id, private_message=True, - mentioned=False, wildcard_mentioned=False, + mentioned=False, wildcard_mention_notify=False, stream_push_notify=False, stream_email_notify=True, stream_name=None, always_push_notify=True, idle=False, already_notified={}) self.assertTrue(email_notice is None) @@ -161,7 +163,7 @@ class MissedMessageNotificationsTest(ZulipTestCase): # Stream message sends push but not email if not idle but always_push_notify email_notice, mobile_notice = self.check_will_notify( user_profile.id, message_id, private_message=False, - mentioned=False, wildcard_mentioned=False, + mentioned=False, wildcard_mention_notify=False, stream_push_notify=True, stream_email_notify=True, stream_name="Denmark", always_push_notify=True, idle=False, already_notified={}) self.assertTrue(email_notice is None) @@ -216,6 +218,11 @@ class MissedMessageNotificationsTest(ZulipTestCase): Combined with the previous test, this ensures that the missedmessage_hook is correct""" user_profile = self.example_user('hamlet') email = user_profile.email + # Fetch the Denmark stream for testing + stream = get_stream("Denmark", user_profile.realm) + sub = Subscription.objects.get(user_profile=user_profile, recipient__type=Recipient.STREAM, + recipient__type_id=stream.id) + self.login(email) def change_subscription_properties(user_profile: UserProfile, stream: Stream, sub: Subscription, @@ -261,7 +268,7 @@ class MissedMessageNotificationsTest(ZulipTestCase): {'email_notified': False, 'push_notified': False})) destroy_event_queue(client_descriptor.event_queue.id) - # Test the hook with a private message; will have already notified on send + # Test the hook with a private message; this should trigger notifications client_descriptor = allocate_event_queue() self.assertTrue(client_descriptor.event_queue.empty()) msg_id = self.send_personal_message(self.example_email("iago"), email) @@ -275,7 +282,7 @@ class MissedMessageNotificationsTest(ZulipTestCase): {'email_notified': True, 'push_notified': True})) destroy_event_queue(client_descriptor.event_queue.id) - # Test the hook with a mention; will have already notified on send. + # Test the hook with a mention; this should trigger notifications client_descriptor = allocate_event_queue() self.assertTrue(client_descriptor.event_queue.empty()) msg_id = self.send_stream_message(self.example_email("iago"), "Denmark", @@ -288,13 +295,9 @@ class MissedMessageNotificationsTest(ZulipTestCase): self.assertEqual(args_list, (user_profile.id, msg_id, False, True, False, False, False, "Denmark", False, True, {'email_notified': True, 'push_notified': True})) - - stream = get_stream("Denmark", user_profile.realm) - sub = Subscription.objects.get(user_profile=user_profile, recipient__type=Recipient.STREAM, - recipient__type_id=stream.id) destroy_event_queue(client_descriptor.event_queue.id) - # Test the hook with a wildcard mention; will have already notified on send. + # Test the hook with a wildcard mention; this should trigger notifications client_descriptor = allocate_event_queue() self.assertTrue(client_descriptor.event_queue.empty()) msg_id = self.send_stream_message(self.example_email("iago"), "Denmark", @@ -309,13 +312,61 @@ class MissedMessageNotificationsTest(ZulipTestCase): {'email_notified': True, 'push_notified': True})) destroy_event_queue(client_descriptor.event_queue.id) - # Fetch the Denmark stream for testing - stream = get_stream("Denmark", user_profile.realm) - sub = Subscription.objects.get(user_profile=user_profile, recipient__type=Recipient.STREAM, - recipient__type_id=stream.id) - change_subscription_properties(user_profile, stream, sub, {'push_notifications': True}) + # Test the hook with a wildcard mention sent by the user + # themself using a human client; should not notify. + client_descriptor = allocate_event_queue() + self.assertTrue(client_descriptor.event_queue.empty()) + msg_id = self.send_stream_message(self.example_email("hamlet"), "Denmark", + content="@**all** what's up?", + sending_client_name="website") + 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_list = mock_enqueue.call_args_list[0][0] + + self.assertEqual(args_list, (user_profile.id, msg_id, False, False, False, False, + False, "Denmark", False, True, + {'email_notified': False, 'push_notified': False})) + destroy_event_queue(client_descriptor.event_queue.id) + + # Wildcard mentions in muted streams don't notify. + change_subscription_properties(user_profile, stream, sub, {'is_muted': True}) + client_descriptor = allocate_event_queue() + self.assertTrue(client_descriptor.event_queue.empty()) + msg_id = self.send_stream_message(self.example_email("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_list = mock_enqueue.call_args_list[0][0] + + self.assertEqual(args_list, (user_profile.id, msg_id, False, False, False, False, + False, "Denmark", False, True, + {'email_notified': False, 'push_notified': False})) + destroy_event_queue(client_descriptor.event_queue.id) + change_subscription_properties(user_profile, stream, sub, {'is_muted': False}) + + # With wildcard_mentions_notify=False, we treat the user as not mentioned. + user_profile.wildcard_mentions_notify = False + user_profile.save() + client_descriptor = allocate_event_queue() + self.assertTrue(client_descriptor.event_queue.empty()) + msg_id = self.send_stream_message(self.example_email("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_list = mock_enqueue.call_args_list[0][0] + + self.assertEqual(args_list, (user_profile.id, msg_id, False, False, False, False, + False, "Denmark", False, True, + {'email_notified': False, 'push_notified': False})) + destroy_event_queue(client_descriptor.event_queue.id) + user_profile.wildcard_mentions_notify = True + user_profile.save() # Test the hook with a stream message with stream_push_notify + change_subscription_properties(user_profile, stream, sub, {'push_notifications': True}) client_descriptor = allocate_event_queue() self.assertTrue(client_descriptor.event_queue.empty()) msg_id = self.send_stream_message(self.example_email("iago"), "Denmark", diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index 47c0125244..b5d98b75aa 100644 --- a/zerver/tests/test_events.py +++ b/zerver/tests/test_events.py @@ -706,6 +706,7 @@ class EventsRegisterTest(ZulipTestCase): ('message_ids', check_list(check_int)), ('prior_mention_user_ids', check_list(check_int)), ('mention_user_ids', check_list(check_int)), + ('wildcard_mention_user_ids', check_list(check_int)), ('presence_idle_user_ids', check_list(check_int)), ('stream_push_user_ids', check_list(check_int)), ('stream_email_user_ids', check_list(check_int)), diff --git a/zerver/tests/test_home.py b/zerver/tests/test_home.py index 85e5f19ce7..367c19e45a 100644 --- a/zerver/tests/test_home.py +++ b/zerver/tests/test_home.py @@ -218,6 +218,7 @@ class HomeTest(ZulipTestCase): "user_id", "user_status", "warn_no_email", + "wildcard_mentions_notify", "zulip_version", ] diff --git a/zerver/tests/test_message_edit_notifications.py b/zerver/tests/test_message_edit_notifications.py index 0101cbf10e..ac5016e292 100644 --- a/zerver/tests/test_message_edit_notifications.py +++ b/zerver/tests/test_message_edit_notifications.py @@ -161,7 +161,7 @@ class EditMessageSideEffectsTest(ZulipTestCase): message_id=message_id, private_message=False, mentioned=True, - wildcard_mentioned=False, + wildcard_mention_notify=False, stream_push_notify=False, stream_email_notify=False, stream_name='Scotland', @@ -314,7 +314,7 @@ class EditMessageSideEffectsTest(ZulipTestCase): message_id=message_id, private_message=False, mentioned=True, - wildcard_mentioned=False, + wildcard_mention_notify=False, stream_push_notify=False, stream_email_notify=False, stream_name='Scotland', @@ -353,7 +353,7 @@ class EditMessageSideEffectsTest(ZulipTestCase): message_id=message_id, private_message=False, mentioned=False, - wildcard_mentioned=False, + wildcard_mention_notify=False, stream_push_notify=False, stream_email_notify=False, stream_name='Scotland', @@ -392,7 +392,7 @@ class EditMessageSideEffectsTest(ZulipTestCase): message_id=message_id, private_message=False, mentioned=True, - wildcard_mentioned=False, + wildcard_mention_notify=False, stream_push_notify=False, stream_email_notify=False, stream_name='Scotland', @@ -406,6 +406,76 @@ class EditMessageSideEffectsTest(ZulipTestCase): # actual content of these messages.) self.assertEqual(len(info['queue_messages']), 2) + def test_updates_with_wildcard_mention(self) -> None: + cordelia = self.example_user('cordelia') + + message_id = self._login_and_send_original_stream_message( + content='no mention' + ) + + # We will simulate that the user still has a an active client, + # but they don't have UserPresence rows, so we will still + # send offline notifications. + with self._cordelia_connected_to_zulip(): + info = self._get_queued_data_for_message_update( + message_id=message_id, + content='now we mention @**all**', + ) + + expected_enqueue_kwargs = dict( + user_profile_id=cordelia.id, + message_id=message_id, + private_message=False, + mentioned=False, + wildcard_mention_notify=True, + stream_push_notify=False, + stream_email_notify=False, + stream_name='Scotland', + always_push_notify=False, + idle=True, + already_notified={}, + ) + self.assertEqual(info['enqueue_kwargs'], expected_enqueue_kwargs) + + # She will get messages enqueued. + self.assertEqual(len(info['queue_messages']), 2) + + def test_updates_with_upgrade_wildcard_mention(self) -> None: + message_id = self._login_and_send_original_stream_message( + content='Mention @**all**' + ) + + # If there was a previous wildcard mention delivered to the + # user (because wildcard_mention_notify=True), we don't notify + with self._cordelia_connected_to_zulip(): + self._get_queued_data_for_message_update( + message_id=message_id, + content='now we mention @**Cordelia Lear**', + expect_short_circuit=True, + ) + + def test_updates_with_upgrade_wildcard_mention_disabled(self) -> None: + # If the user has disabled notifications for wildcard + # mentions, they won't have been notified at first, which + # means they should be notified when the message is edited to + # contain a wildcard mention. + # + # This is a bug that we're not equipped to fix right now. + cordelia = self.example_user('cordelia') + cordelia.wildcard_mentions_notify = False + cordelia.save() + + message_id = self._login_and_send_original_stream_message( + content='Mention @**all**' + ) + + with self._cordelia_connected_to_zulip(): + self._get_queued_data_for_message_update( + message_id=message_id, + content='now we mention @**Cordelia Lear**', + expect_short_circuit=True, + ) + def test_updates_with_stream_mention_of_fully_present_user(self) -> None: cordelia = self.example_user('cordelia') @@ -428,7 +498,7 @@ class EditMessageSideEffectsTest(ZulipTestCase): message_id=message_id, private_message=False, mentioned=True, - wildcard_mentioned=False, + wildcard_mention_notify=False, stream_push_notify=False, stream_email_notify=False, stream_name='Scotland', diff --git a/zerver/tests/test_users.py b/zerver/tests/test_users.py index a9f62df122..26a9a55150 100644 --- a/zerver/tests/test_users.py +++ b/zerver/tests/test_users.py @@ -1015,6 +1015,7 @@ class RecipientInfoTest(ZulipTestCase): recipient=recipient, sender_id=hamlet.id, stream_topic=stream_topic, + possible_wildcard_mention=False, ) all_user_ids = {hamlet.id, cordelia.id, othello.id} @@ -1024,6 +1025,7 @@ class RecipientInfoTest(ZulipTestCase): push_notify_user_ids=set(), stream_push_user_ids=set(), stream_email_user_ids=set(), + wildcard_mention_user_ids=set(), um_eligible_user_ids=all_user_ids, long_term_idle_user_ids=set(), default_bot_user_ids=set(), @@ -1032,14 +1034,26 @@ class RecipientInfoTest(ZulipTestCase): self.assertEqual(info, expected_info) + cordelia.wildcard_mentions_notify = False + cordelia.save() hamlet.enable_stream_push_notifications = True hamlet.save() info = get_recipient_info( recipient=recipient, sender_id=hamlet.id, stream_topic=stream_topic, + possible_wildcard_mention=False, ) self.assertEqual(info['stream_push_user_ids'], {hamlet.id}) + self.assertEqual(info['wildcard_mention_user_ids'], set()) + + info = get_recipient_info( + recipient=recipient, + sender_id=hamlet.id, + stream_topic=stream_topic, + possible_wildcard_mention=True, + ) + self.assertEqual(info['wildcard_mention_user_ids'], {hamlet.id, othello.id}) sub = get_subscription(stream_name, hamlet) sub.push_notifications = False @@ -1075,9 +1089,49 @@ class RecipientInfoTest(ZulipTestCase): recipient=recipient, sender_id=hamlet.id, stream_topic=stream_topic, + possible_wildcard_mention=False, ) - self.assertEqual(info['stream_push_user_ids'], set()) + self.assertEqual(info['wildcard_mention_user_ids'], set()) + + info = get_recipient_info( + recipient=recipient, + sender_id=hamlet.id, + stream_topic=stream_topic, + possible_wildcard_mention=True, + ) + self.assertEqual(info['stream_push_user_ids'], set()) + # Since Hamlet has muted the stream and Cordelia has disabled + # wildcard notifications, it should just be Othello here. + self.assertEqual(info['wildcard_mention_user_ids'], {othello.id}) + + sub = get_subscription(stream_name, othello) + sub.wildcard_mentions_notify = False + sub.save() + + info = get_recipient_info( + recipient=recipient, + sender_id=hamlet.id, + stream_topic=stream_topic, + possible_wildcard_mention=True, + ) + self.assertEqual(info['stream_push_user_ids'], set()) + # Verify that stream-level wildcard_mentions_notify=False works correctly. + self.assertEqual(info['wildcard_mention_user_ids'], set()) + + # Verify that True works as expected as well + sub = get_subscription(stream_name, othello) + sub.wildcard_mentions_notify = True + sub.save() + + info = get_recipient_info( + recipient=recipient, + sender_id=hamlet.id, + stream_topic=stream_topic, + possible_wildcard_mention=True, + ) + self.assertEqual(info['stream_push_user_ids'], set()) + self.assertEqual(info['wildcard_mention_user_ids'], {othello.id}) # Add a service bot. service_bot = do_create_user( diff --git a/zerver/tornado/event_queue.py b/zerver/tornado/event_queue.py index 5fd5d22230..41cea8e3ea 100644 --- a/zerver/tornado/event_queue.py +++ b/zerver/tornado/event_queue.py @@ -667,11 +667,12 @@ def missedmessage_hook(user_profile_id: int, client: ClientDescriptor, last_for_ flags = event.get('flags') mentioned = 'mentioned' in flags and 'read' not in flags - wildcard_mentioned = 'wildcard_mentioned' in flags and 'read' not in flags private_message = event['message']['type'] == 'private' # stream_push_notify is set in process_message_event. stream_push_notify = event.get('stream_push_notify', False) stream_email_notify = event.get('stream_email_notify', False) + wildcard_mention_notify = (event.get('wildcard_mention_notify', False) and + 'read' not in flags and 'wildcard_mentioned' in flags) stream_name = None if not private_message: @@ -689,8 +690,8 @@ def missedmessage_hook(user_profile_id: int, client: ClientDescriptor, last_for_ email_notified = event.get("email_notified", False), ) maybe_enqueue_notifications(user_profile_id, message_id, private_message, mentioned, - wildcard_mentioned, - stream_push_notify, stream_email_notify, stream_name, + wildcard_mention_notify, stream_push_notify, + stream_email_notify, stream_name, always_push_notify, idle, already_notified) def receiver_is_off_zulip(user_profile_id: int) -> bool: @@ -702,7 +703,8 @@ def receiver_is_off_zulip(user_profile_id: int) -> bool: return off_zulip def maybe_enqueue_notifications(user_profile_id: int, message_id: int, private_message: bool, - mentioned: bool, wildcard_mentioned: bool, + mentioned: bool, + wildcard_mention_notify: bool, stream_push_notify: bool, stream_email_notify: bool, stream_name: Optional[str], always_push_notify: bool, idle: bool, @@ -713,13 +715,13 @@ def maybe_enqueue_notifications(user_profile_id: int, message_id: int, private_m notified = dict() # type: Dict[str, bool] if (idle or always_push_notify) and (private_message or mentioned or - wildcard_mentioned or stream_push_notify): + wildcard_mention_notify or stream_push_notify): notice = build_offline_notification(user_profile_id, message_id) if private_message: notice['trigger'] = 'private_message' elif mentioned: notice['trigger'] = 'mentioned' - elif wildcard_mentioned: + elif wildcard_mention_notify: notice['trigger'] = 'wildcard_mentioned' elif stream_push_notify: notice['trigger'] = 'stream_push_notify' @@ -734,13 +736,13 @@ def maybe_enqueue_notifications(user_profile_id: int, message_id: int, private_m # mention. Eventually, we'll add settings to allow email # notifications to match the model of push notifications # above. - if idle and (private_message or mentioned or wildcard_mentioned or stream_email_notify): + if idle and (private_message or mentioned or wildcard_mention_notify or stream_email_notify): notice = build_offline_notification(user_profile_id, message_id) if private_message: notice['trigger'] = 'private_message' elif mentioned: notice['trigger'] = 'mentioned' - elif wildcard_mentioned: + elif wildcard_mention_notify: notice['trigger'] = 'wildcard_mentioned' elif stream_email_notify: notice['trigger'] = 'stream_email_notify' @@ -833,23 +835,26 @@ def process_message_event(event_template: Mapping[str, Any], users: Iterable[Map # or they were @-notified potentially notify more immediately private_message = message_type == "private" and user_profile_id != sender_id mentioned = 'mentioned' in flags and 'read' not in flags - wildcard_mentioned = 'wildcard_mentioned' in flags and 'read' not in flags stream_push_notify = user_data.get('stream_push_notify', False) stream_email_notify = user_data.get('stream_email_notify', False) + wildcard_mention_notify = (user_data.get('wildcard_mention_notify', False) and + 'wildcard_mentioned' in flags and 'read' not in flags) # We first check if a message is potentially mentionable, # since receiver_is_off_zulip is somewhat expensive. - if (private_message or mentioned or wildcard_mentioned + if (private_message or mentioned or wildcard_mention_notify or stream_push_notify or stream_email_notify): idle = receiver_is_off_zulip(user_profile_id) or (user_profile_id in presence_idle_user_ids) always_push_notify = user_data.get('always_push_notify', False) stream_name = event_template.get('stream_name') result = maybe_enqueue_notifications(user_profile_id, message_id, private_message, - mentioned, wildcard_mentioned, + mentioned, + wildcard_mention_notify, stream_push_notify, stream_email_notify, stream_name, always_push_notify, idle, {}) result['stream_push_notify'] = stream_push_notify result['stream_email_notify'] = stream_email_notify + result['wildcard_mention_notify'] = wildcard_mention_notify extra_user_data[user_profile_id] = result for client_data in send_to_clients.values(): @@ -919,6 +924,7 @@ def process_message_update_event(event_template: Mapping[str, Any], presence_idle_user_ids = set(event_template.get('presence_idle_user_ids', [])) stream_push_user_ids = set(event_template.get('stream_push_user_ids', [])) stream_email_user_ids = set(event_template.get('stream_email_user_ids', [])) + wildcard_mention_user_ids = set(event_template.get('wildcard_mention_user_ids', [])) push_notify_user_ids = set(event_template.get('push_notify_user_ids', [])) stream_name = event_template.get('stream_name') @@ -931,6 +937,8 @@ def process_message_update_event(event_template: Mapping[str, Any], if key != "id": user_event[key] = user_data[key] wildcard_mentioned = 'wildcard_mentioned' in user_event['flags'] + wildcard_mention_notify = wildcard_mentioned and ( + user_profile_id in wildcard_mention_user_ids) maybe_enqueue_notifications_for_message_update( user_profile_id=user_profile_id, @@ -938,7 +946,7 @@ def process_message_update_event(event_template: Mapping[str, Any], stream_name=stream_name, prior_mention_user_ids=prior_mention_user_ids, mention_user_ids=mention_user_ids, - wildcard_mentioned = wildcard_mentioned, + wildcard_mention_notify = wildcard_mention_notify, presence_idle_user_ids=presence_idle_user_ids, stream_push_user_ids=stream_push_user_ids, stream_email_user_ids=stream_email_user_ids, @@ -956,7 +964,7 @@ def maybe_enqueue_notifications_for_message_update(user_profile_id: UserProfile, stream_name: str, prior_mention_user_ids: Set[int], mention_user_ids: Set[int], - wildcard_mentioned: bool, + wildcard_mention_notify: bool, presence_idle_user_ids: Set[int], stream_push_user_ids: Set[int], stream_email_user_ids: Set[int], @@ -975,6 +983,13 @@ def maybe_enqueue_notifications_for_message_update(user_profile_id: UserProfile, # # Note that prior_mention_user_ids contains users who received # a wildcard mention as well as normal mentions. + # + # TODO: Ideally, that would mean that we exclude here cases + # where user_profile.wildcard_mentions_notify=False and have + # those still send a notification. However, we don't have the + # data to determine whether or not that was the case at the + # time the original message was sent, so we can't do that + # without extending the UserMessage data model. return stream_push_notify = (user_profile_id in stream_push_user_ids) @@ -1001,7 +1016,7 @@ def maybe_enqueue_notifications_for_message_update(user_profile_id: UserProfile, message_id=message_id, private_message=private_message, mentioned=mentioned, - wildcard_mentioned=wildcard_mentioned, + wildcard_mention_notify=wildcard_mention_notify, stream_push_notify=stream_push_notify, stream_email_notify=stream_email_notify, stream_name=stream_name, diff --git a/zerver/views/user_settings.py b/zerver/views/user_settings.py index 84ff6c2452..ac836332f1 100644 --- a/zerver/views/user_settings.py +++ b/zerver/views/user_settings.py @@ -162,6 +162,7 @@ def json_change_notify_settings( enable_stream_email_notifications: Optional[bool]=REQ(validator=check_bool, default=None), enable_stream_push_notifications: Optional[bool]=REQ(validator=check_bool, default=None), enable_stream_audible_notifications: Optional[bool]=REQ(validator=check_bool, default=None), + wildcard_mentions_notify: Optional[bool]=REQ(validator=check_bool, default=None), notification_sound: Optional[str]=REQ(validator=check_string, default=None), enable_desktop_notifications: Optional[bool]=REQ(validator=check_bool, default=None), enable_sounds: Optional[bool]=REQ(validator=check_bool, default=None),