From b573964bfa8a8f61f8a99462743993a7094da4f6 Mon Sep 17 00:00:00 2001 From: akshatdalton Date: Tue, 27 Apr 2021 23:04:06 +0000 Subject: [PATCH] notifications: Add support for `None` option in `Notification sound`. This commit adds support for a `None` option in the dropdown menu of `Notification sound`. When this option is selected, no audible notification is sent to the user. `None` will appear as the first option in the dropdown menu, since this is not categorized as a playable audio. This new option is added so that folks can disable audio notifications without losing their other notification configuration (like for PMs, mentions). Necessary test case is added for this new option. Fixes #16090. --- frontend_tests/node_tests/notifications.js | 25 ++++++++++++++++++- static/js/notifications.js | 18 +++++++++---- static/js/settings_notifications.js | 4 ++- .../settings/notification_settings.hbs | 1 + zerver/views/user_settings.py | 1 + 5 files changed, 42 insertions(+), 7 deletions(-) diff --git a/frontend_tests/node_tests/notifications.js b/frontend_tests/node_tests/notifications.js index 9ce83e0e4a..cadec04891 100644 --- a/frontend_tests/node_tests/notifications.js +++ b/frontend_tests/node_tests/notifications.js @@ -59,6 +59,7 @@ function test(label, f) { page_params.enable_desktop_notifications = true; page_params.enable_sounds = true; page_params.wildcard_mentions_notify = true; + page_params.notification_sound = "ding"; f(override); }); } @@ -239,13 +240,35 @@ test("message_is_notifiable", () => { assert.equal(notifications.should_send_audible_notification(message), true); assert.equal(notifications.message_is_notifiable(message), false); + // Case 9: If `None` is selected as the notification sound, send no + // audible notification, no matter what other user configurations are. + message = { + id: 50, + content: "message number 7", + sent_by_me: false, + notification_sent: false, + mentioned: true, + mentioned_me_directly: true, + type: "stream", + stream: "general", + stream_id: general.stream_id, + topic: "whatever", + }; + page_params.notification_sound = "none"; + assert.equal(notifications.should_send_desktop_notification(message), true); + assert.equal(notifications.should_send_audible_notification(message), false); + assert.equal(notifications.message_is_notifiable(message), true); + + // Reset state + page_params.notification_sound = "ding"; + // If none of the above cases apply // (ie: topic is not muted, message does not mention user, // no notification sent before, message not sent by user), // return true to pass it to notifications settings, which will return false. message = { id: 60, - content: "message number 7", + content: "message number 8", sent_by_me: false, notification_sent: false, mentioned: false, diff --git a/static/js/notifications.js b/static/js/notifications.js index 664472f487..f3685a4d1f 100644 --- a/static/js/notifications.js +++ b/static/js/notifications.js @@ -91,14 +91,16 @@ export function initialize() { } function update_notification_sound_source() { - const audio_file_without_extension = - "/static/audio/notification_sounds/" + page_params.notification_sound; + const notification_sound = page_params.notification_sound; + const audio_file_without_extension = "/static/audio/notification_sounds/" + notification_sound; $("#notification-sound-source-ogg").attr("src", `${audio_file_without_extension}.ogg`); $("#notification-sound-source-mp3").attr("src", `${audio_file_without_extension}.mp3`); - // Load it so that it is ready to be played; without this the old sound - // is played. - $("#notification-sound-audio")[0].load(); + if (notification_sound !== "none") { + // Load it so that it is ready to be played; without this the old sound + // is played. + $("#notification-sound-audio")[0].load(); + } } export function permission_state() { @@ -415,6 +417,12 @@ export function should_send_desktop_notification(message) { } export function should_send_audible_notification(message) { + // If `None` is selected as the notification sound, never send + // audible notifications regardless of other configuration. + if (page_params.notification_sound === "none") { + return false; + } + // For streams, ding if sounds are enabled for all messages on // this stream. if ( diff --git a/static/js/settings_notifications.js b/static/js/settings_notifications.js index 91972f40a5..be66afa338 100644 --- a/static/js/settings_notifications.js +++ b/static/js/settings_notifications.js @@ -92,7 +92,9 @@ export function set_up() { }); $("#play_notification_sound").on("click", () => { - $("#notification-sound-audio")[0].play(); + if (page_params.notification_sound !== "none") { + $("#notification-sound-audio")[0].play(); + } }); const notification_sound_dropdown = $("#notification_sound"); diff --git a/static/templates/settings/notification_settings.hbs b/static/templates/settings/notification_settings.hbs index 6d8b87dd16..c06f7549d6 100644 --- a/static/templates/settings/notification_settings.hbs +++ b/static/templates/settings/notification_settings.hbs @@ -69,6 +69,7 @@ {{#unless enable_sound_select}} disabled {{/unless}}> + {{#each page_params.available_notification_sounds}} {{/each}} diff --git a/zerver/views/user_settings.py b/zerver/views/user_settings.py index 551a6b8450..c52b610acb 100644 --- a/zerver/views/user_settings.py +++ b/zerver/views/user_settings.py @@ -272,6 +272,7 @@ def json_change_notify_settings( if ( notification_sound is not None and notification_sound not in get_available_notification_sounds() + and notification_sound != "none" ): raise JsonableError(_("Invalid notification sound '{}'").format(notification_sound))