From d5f005fd614447d3905372cccd08dc9cd1efee9f Mon Sep 17 00:00:00 2001 From: Nat1405 Date: Mon, 25 Nov 2019 17:37:12 -0800 Subject: [PATCH] wildcard_mentions_notify: Add per-stream override of global setting. Adds required API and front-end changes to modify and read the wildcard_mentions_notify field in the Subscription model. It includes front-end code to add the setting to the user's "manage streams" page. This setting will be greyed out when a stream is muted. The PR also includes back-end code to add the setting the initial state of a subscription. New automated tests were added for the API, events system and front-end. In manual testing, we checked that modifying the setting in the front end persisted the change in the Subscription model. We noticed the notifications were not behaving exactly as expected in manual testing; see https://github.com/zulip/zulip/issues/13073#issuecomment-560263081 . Tweaked by tabbott to fix real-time synchronization issues. Fixes: #13429. --- frontend_tests/node_tests/stream_events.js | 6 ++++ static/js/stream_data.js | 4 +++ static/js/stream_edit.js | 9 +++++- static/js/stream_events.js | 3 ++ .../zerver/help/mention-a-user-or-group.md | 4 ++- .../help/pm-mention-alert-notifications.md | 3 ++ zerver/lib/actions.py | 4 ++- zerver/tests/test_event_queue.py | 24 ++++++++++++++ zerver/tests/test_events.py | 1 + zerver/tests/test_subs.py | 31 +++++++++++++++++++ zerver/views/streams.py | 3 +- 11 files changed, 88 insertions(+), 4 deletions(-) diff --git a/frontend_tests/node_tests/stream_events.js b/frontend_tests/node_tests/stream_events.js index f70fcc2d8c..55ac952080 100644 --- a/frontend_tests/node_tests/stream_events.js +++ b/frontend_tests/node_tests/stream_events.js @@ -94,6 +94,12 @@ run_test('update_property', () => { checkbox = $("#email_notifications_1"); assert.equal(checkbox.prop('checked'), true); + // Tests wildcard_mentions_notify notifications + stream_events.update_property(1, 'wildcard_mentions_notify', true); + assert.equal(frontend.wildcard_mentions_notify, true); + checkbox = $("#wildcard_mentions_notify_1"); + assert.equal(checkbox.prop('checked'), true); + // Test name change with_overrides(function (override) { global.with_stub(function (stub) { diff --git a/static/js/stream_data.js b/static/js/stream_data.js index 930262c61a..d3ae0d414b 100644 --- a/static/js/stream_data.js +++ b/static/js/stream_data.js @@ -293,6 +293,9 @@ exports.receives_notifications = function (stream_name, notification_name) { if (sub[notification_name] !== null) { return sub[notification_name]; } + if (notification_name === 'wildcard_mentions_notify') { + return page_params[notification_name]; + } return page_params["enable_stream_" + notification_name]; }; @@ -301,6 +304,7 @@ const stream_notification_settings = [ "audible_notifications", "push_notifications", "email_notifications", + "wildcard_mentions_notify", ]; exports.update_calculated_fields = function (sub) { diff --git a/static/js/stream_edit.js b/static/js/stream_edit.js index 85b5dccdb4..ee86057338 100644 --- a/static/js/stream_edit.js +++ b/static/js/stream_edit.js @@ -227,6 +227,8 @@ function show_subscription_settings(sub_row) { exports.is_notification_setting = function (setting_label) { if (setting_label.indexOf("_notifications") > -1) { return true; + } else if (setting_label.indexOf("_notify") > -1) { + return true; } return false; }; @@ -238,6 +240,7 @@ const settings_labels = { push_notifications: i18n.t("Mobile notifications"), email_notifications: i18n.t("Email notifications"), pin_to_top: i18n.t("Pin stream to top of left sidebar"), + wildcard_mentions_notify: i18n.t("Notifications for @all/@everyone mentions"), }; const check_realm_setting = { @@ -322,7 +325,11 @@ function stream_setting_clicked(e) { return false; } if (exports.is_notification_setting(setting) && sub[setting] === null) { - sub[setting] = page_params["enable_stream_" + setting]; + if (setting === 'wildcard_mentions_notify') { + sub[setting] = page_params[setting]; + } else { + sub[setting] = page_params["enable_stream_" + setting]; + } } exports.set_stream_property(sub, setting, !sub[setting]); diff --git a/static/js/stream_events.js b/static/js/stream_events.js index b7fb436d51..a470a8979d 100644 --- a/static/js/stream_events.js +++ b/static/js/stream_events.js @@ -50,6 +50,9 @@ exports.update_property = function (stream_id, property, value, other_values) { history_public_to_subscribers: other_values.history_public_to_subscribers, }); break; + case 'wildcard_mentions_notify': + update_stream_setting(sub, value, property); + break; case 'is_announcement_only': subs.update_stream_announcement_only(sub, value); break; diff --git a/templates/zerver/help/mention-a-user-or-group.md b/templates/zerver/help/mention-a-user-or-group.md index 3ab4bf16b9..17af8a31f8 100644 --- a/templates/zerver/help/mention-a-user-or-group.md +++ b/templates/zerver/help/mention-a-user-or-group.md @@ -51,4 +51,6 @@ notification. Silent mentions start with `@_` instead of `@`. 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, and users can disable receiving email/push notifications for -these wildcard mentions. +these wildcard mentions, either +[globally](/help/pm-mention-alert-notifications) or for [individual +streams](/help/stream-notifications). diff --git a/templates/zerver/help/pm-mention-alert-notifications.md b/templates/zerver/help/pm-mention-alert-notifications.md index 4dd9fc299f..3905d438df 100644 --- a/templates/zerver/help/pm-mention-alert-notifications.md +++ b/templates/zerver/help/pm-mention-alert-notifications.md @@ -34,6 +34,9 @@ can toggle whether you receive notifications for wildcard mentions: {end_tabs} +Additionally, you can override this configuration for individual +streams in your [Stream settings](/help/stream-notifications). + ## Related articles * [Add an alert word](/help/add-an-alert-word) diff --git a/zerver/lib/actions.py b/zerver/lib/actions.py index ceccf33633..51571efbd4 100644 --- a/zerver/lib/actions.py +++ b/zerver/lib/actions.py @@ -2781,6 +2781,7 @@ def notify_subscriptions_added(user_profile: UserProfile, audible_notifications=subscription.audible_notifications, push_notifications=subscription.push_notifications, email_notifications=subscription.email_notifications, + wildcard_mentions_notify=subscription.wildcard_mentions_notify, description=stream.description, rendered_description=stream.rendered_description, pin_to_top=subscription.pin_to_top, @@ -4734,7 +4735,7 @@ def gather_subscriptions_helper(user_profile: UserProfile, sub_dicts = get_stream_subscriptions_for_user(user_profile).values( "recipient_id", "is_muted", "color", "desktop_notifications", "audible_notifications", "push_notifications", "email_notifications", - "active", "pin_to_top" + "wildcard_mentions_notify", "active", "pin_to_top" ).order_by("recipient_id") sub_dicts = list(sub_dicts) @@ -4820,6 +4821,7 @@ def gather_subscriptions_helper(user_profile: UserProfile, 'audible_notifications': sub["audible_notifications"], 'push_notifications': sub["push_notifications"], 'email_notifications': sub["email_notifications"], + 'wildcard_mentions_notify': sub["wildcard_mentions_notify"], 'pin_to_top': sub["pin_to_top"], 'stream_id': stream["id"], 'first_message_id': stream["first_message_id"], diff --git a/zerver/tests/test_event_queue.py b/zerver/tests/test_event_queue.py index 5e5bc2fdf5..96a8ac7a6f 100644 --- a/zerver/tests/test_event_queue.py +++ b/zerver/tests/test_event_queue.py @@ -365,6 +365,30 @@ class MissedMessageNotificationsTest(ZulipTestCase): user_profile.wildcard_mentions_notify = True user_profile.save() + # If wildcard_mentions_notify=True for a stream and False for a user, we treat the user + # as mentioned for that stream. + user_profile.wildcard_mentions_notify = False + sub.wildcard_mentions_notify = True + user_profile.save() + sub.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, True, False, + False, "Denmark", False, True, + {'email_notified': True, 'push_notified': True})) + destroy_event_queue(client_descriptor.event_queue.id) + user_profile.wildcard_mentions_notify = True + sub.wildcard_mentions_notify = None + user_profile.save() + sub.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() diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index f8ee7a4b09..3dd4ba59e3 100644 --- a/zerver/tests/test_events.py +++ b/zerver/tests/test_events.py @@ -2495,6 +2495,7 @@ class EventsRegisterTest(ZulipTestCase): ('pin_to_top', check_bool), ('stream_weekly_traffic', check_none_or(check_int)), ('is_old_stream', check_bool), + ('wildcard_mentions_notify', check_none_or(check_bool)), ] if include_subscribers: subscription_fields.append(('subscribers', check_list(check_int))) diff --git a/zerver/tests/test_subs.py b/zerver/tests/test_subs.py index 362c728fd3..5832353a9f 100644 --- a/zerver/tests/test_subs.py +++ b/zerver/tests/test_subs.py @@ -1611,6 +1611,28 @@ class SubscriptionPropertiesTest(ZulipTestCase): self.assert_json_error( result, "value key is missing from subscription_data[0]") + def test_set_stream_wildcard_mentions_notify(self) -> None: + """ + A POST request to /api/v1/users/me/subscriptions/properties with wildcard_mentions_notify + sets the property. + """ + test_user = self.example_user('hamlet') + test_email = test_user.email + self.login(test_email) + + subs = gather_subscriptions(test_user)[0] + sub = subs[0] + result = self.api_post(test_email, "/api/v1/users/me/subscriptions/properties", + {"subscription_data": ujson.dumps([{"property": "wildcard_mentions_notify", + "stream_id": sub["stream_id"], + "value": True}])}) + + self.assert_json_success(result) + + updated_sub = get_subscription(sub['name'], test_user) + self.assertIsNotNone(updated_sub) + self.assertEqual(updated_sub.wildcard_mentions_notify, True) + def test_set_pin_to_top(self) -> None: """ A POST request to /api/v1/users/me/subscriptions/properties with stream_id and @@ -1752,6 +1774,15 @@ class SubscriptionPropertiesTest(ZulipTestCase): self.assert_json_error(result, '%s is not a boolean' % (property_name,)) + property_name = "wildcard_mentions_notify" + result = self.api_post(test_email, "/api/v1/users/me/subscriptions/properties", + {"subscription_data": ujson.dumps([{"property": property_name, + "value": "bad", + "stream_id": subs[0]["stream_id"]}])}) + + self.assert_json_error(result, + "%s is not a boolean" % (property_name,)) + property_name = "color" result = self.api_post(test_email, "/api/v1/users/me/subscriptions/properties", {"subscription_data": ujson.dumps([{"property": property_name, diff --git a/zerver/views/streams.py b/zerver/views/streams.py index 4e513a7cee..baee5562ee 100644 --- a/zerver/views/streams.py +++ b/zerver/views/streams.py @@ -580,7 +580,8 @@ def update_subscription_properties_backend( "audible_notifications": check_bool, "push_notifications": check_bool, "email_notifications": check_bool, - "pin_to_top": check_bool} + "pin_to_top": check_bool, + "wildcard_mentions_notify": check_bool} response_data = [] for change in subscription_data: