From 6ec8728ebf2a41d48220e7e8fef683c58de5d78d Mon Sep 17 00:00:00 2001 From: David Battersby Date: Fri, 2 Aug 2024 17:25:15 +0400 Subject: [PATCH] DEV: refactor live notifications setting in user preferences (#28145) This change is mainly a refactor of the desktop notifications service to improve readability and have standardised values for tracking state for current user in regards to the Notification API and Push API. Also improves readability when handling push notification jobs, especially in scenarios where the push_notification_time_window_mins site setting is set to 0, which will allow sending push notifications instantly. --- .../app/lib/desktop-notifications.js | 7 +- .../app/services/desktop-notifications.js | 96 ++++++++++--------- app/jobs/regular/send_push_notification.rb | 5 +- app/services/post_alerter.rb | 3 +- app/services/push_notification_pusher.rb | 1 - spec/jobs/send_push_notification_spec.rb | 20 ++-- 6 files changed, 69 insertions(+), 63 deletions(-) diff --git a/app/assets/javascripts/discourse/app/lib/desktop-notifications.js b/app/assets/javascripts/discourse/app/lib/desktop-notifications.js index 158c7ea999b..c3b6ffc86f4 100644 --- a/app/assets/javascripts/discourse/app/lib/desktop-notifications.js +++ b/app/assets/javascripts/discourse/app/lib/desktop-notifications.js @@ -61,8 +61,7 @@ function init(messageBus) { } catch (e) { // eslint-disable-next-line no-console console.warn( - "Unexpected error, Notification is defined on window but not a responding correctly " + - e + "Notification is defined on window but is not responding correctly " + e ); } @@ -136,7 +135,7 @@ function canUserReceiveNotifications(user) { return false; } - if (keyValueStore.getItem("notifications-disabled")) { + if (keyValueStore.getItem("notifications-disabled", "disabled")) { return false; } @@ -168,7 +167,7 @@ async function onNotification(data, siteSettings, user, appEvents) { siteSettings.site_logo_small_url || siteSettings.site_logo_url; const notificationTag = - "discourse-notification-" + siteSettings.title + "-" + data.topic_id; + "discourse-notification-" + siteSettings.title + "-" + (data.topic_id || 0); await requestPermission(); diff --git a/app/assets/javascripts/discourse/app/services/desktop-notifications.js b/app/assets/javascripts/discourse/app/services/desktop-notifications.js index 854bc945ea3..8fa5d0958bb 100644 --- a/app/assets/javascripts/discourse/app/services/desktop-notifications.js +++ b/app/assets/javascripts/discourse/app/services/desktop-notifications.js @@ -18,6 +18,7 @@ import { const keyValueStore = new KeyValueStore(context); const DISABLED = "disabled"; const ENABLED = "enabled"; +const SUBSCRIBED = "subscribed"; @disableImplicitInjections export default class DesktopNotificationsService extends Service { @@ -25,17 +26,19 @@ export default class DesktopNotificationsService extends Service { @service site; @service siteSettings; - @tracked notificationsDisabled; - @tracked isEnabledPush; + @tracked isEnabledBrowser = false; + @tracked isEnabledPush = false; constructor() { super(...arguments); - this.notificationsDisabled = - keyValueStore.getItem("notifications-disabled") === DISABLED; + + this.isEnabledBrowser = this.isGrantedPermission + ? keyValueStore.getItem("notifications-disabled") === ENABLED + : false; this.isEnabledPush = this.currentUser ? pushNotificationKeyValueStore.getItem( pushNotificationUserSubscriptionKey(this.currentUser) - ) + ) === SUBSCRIBED : false; } @@ -47,11 +50,6 @@ export default class DesktopNotificationsService extends Service { return this.isNotSupported ? "" : Notification.permission; } - setNotificationsDisabled(value) { - keyValueStore.setItem("notifications-disabled", value); - this.notificationsDisabled = value === DISABLED; - } - get isDeniedPermission() { if (this.isNotSupported) { return false; @@ -68,30 +66,8 @@ export default class DesktopNotificationsService extends Service { return this.notificationsPermission === "granted"; } - get isEnabledDesktop() { - if (this.isGrantedPermission) { - return !this.notificationsDisabled; - } - - return false; - } - - setIsEnabledPush(value) { - const user = this.currentUser; - if (!user) { - return false; - } - pushNotificationKeyValueStore.setItem( - pushNotificationUserSubscriptionKey(user), - value - ); - this.isEnabledPush = pushNotificationKeyValueStore.getItem( - pushNotificationUserSubscriptionKey(user) - ); - } - get isEnabled() { - return this.isEnabledDesktop || this.isEnabledPush; + return this.isEnabledPush || this.isEnabledBrowser; } get isSubscribed() { @@ -99,11 +75,9 @@ export default class DesktopNotificationsService extends Service { return false; } - if (this.isPushNotificationsPreferred) { - return this.isEnabledPush === "subscribed"; - } else { - return !this.notificationsDisabled; - } + return this.isPushNotificationsPreferred + ? this.isEnabledPush + : this.isEnabledBrowser; } get isPushNotificationsPreferred() { @@ -114,14 +88,36 @@ export default class DesktopNotificationsService extends Service { ); } + setIsEnabledBrowser(value) { + const status = value ? ENABLED : DISABLED; + keyValueStore.setItem("notifications-disabled", status); + this.isEnabledBrowser = value; + } + + setIsEnabledPush(value) { + const user = this.currentUser; + const status = value ? SUBSCRIBED : value; + + if (!user) { + return false; + } + + pushNotificationKeyValueStore.setItem( + pushNotificationUserSubscriptionKey(user), + status + ); + + this.isEnabledPush = value; + } + @action async disable() { - if (this.isEnabledDesktop) { - this.setNotificationsDisabled(DISABLED); + if (this.isEnabledBrowser) { + this.setIsEnabledBrowser(false); } if (this.isEnabledPush) { await unsubscribePushNotification(this.currentUser, () => { - this.setIsEnabledPush(""); + this.setIsEnabledPush(false); }); } @@ -129,16 +125,22 @@ export default class DesktopNotificationsService extends Service { } @action - enable() { + async enable() { if (this.isPushNotificationsPreferred) { - return subscribePushNotification(() => { - this.setIsEnabledPush("subscribed"); + await subscribePushNotification(() => { + this.setIsEnabledPush(true); }, this.siteSettings.vapid_public_key_bytes); + + return true; } else { - this.setNotificationsDisabled(ENABLED); - return Notification.requestPermission((permission) => { + await Notification.requestPermission((permission) => { confirmNotification(this.siteSettings); - return permission === "granted"; + if (permission === "granted") { + this.setIsEnabledBrowser(true); + return true; + } else { + return false; + } }); } } diff --git a/app/jobs/regular/send_push_notification.rb b/app/jobs/regular/send_push_notification.rb index d73013fb8b9..143e78662b0 100644 --- a/app/jobs/regular/send_push_notification.rb +++ b/app/jobs/regular/send_push_notification.rb @@ -4,9 +4,8 @@ module Jobs class SendPushNotification < ::Jobs::Base def execute(args) user = User.find_by(id: args[:user_id]) - if !user || user.seen_since?(SiteSetting.push_notification_time_window_mins.minutes.ago) - return - end + push_window = SiteSetting.push_notification_time_window_mins + return if !user || (push_window > 0 && user.seen_since?(push_window.minutes.ago)) PushNotificationPusher.push(user, args[:payload]) end diff --git a/app/services/post_alerter.rb b/app/services/post_alerter.rb index 1cff1d0e826..18b401a0e5a 100644 --- a/app/services/post_alerter.rb +++ b/app/services/post_alerter.rb @@ -84,7 +84,8 @@ class PostAlerter end if user.push_subscriptions.exists? - if user.seen_since?(SiteSetting.push_notification_time_window_mins.minutes.ago) + push_window = SiteSetting.push_notification_time_window_mins + if push_window > 0 && user.seen_since?(push_window.minutes.ago) delay = (SiteSetting.push_notification_time_window_mins - (Time.now - user.last_seen_at) / 60) Jobs.enqueue_in(delay.minutes, :send_push_notification, user_id: user.id, payload: payload) diff --git a/app/services/push_notification_pusher.rb b/app/services/push_notification_pusher.rb index b15e179d24c..5ed7f97350f 100644 --- a/app/services/push_notification_pusher.rb +++ b/app/services/push_notification_pusher.rb @@ -27,7 +27,6 @@ class PushNotificationPusher tag: payload[:tag] || "#{Discourse.current_hostname}-#{payload[:topic_id]}", base_url: Discourse.base_url, url: payload[:post_url], - hide_when_active: true, } subscriptions(user).each { |subscription| send_notification(user, subscription, message) } diff --git a/spec/jobs/send_push_notification_spec.rb b/spec/jobs/send_push_notification_spec.rb index fddee5fac6e..555f724e17a 100644 --- a/spec/jobs/send_push_notification_spec.rb +++ b/spec/jobs/send_push_notification_spec.rb @@ -10,23 +10,29 @@ RSpec.describe Jobs::SendPushNotification do SiteSetting.push_notification_time_window_mins = 10 end - context "with active online user" do - it "does not send push notification" do - user.update!(last_seen_at: 5.minutes.ago) + context "with valid user" do + it "does not send push notification when user is online" do + user.update!(last_seen_at: 2.minutes.ago) PushNotificationPusher.expects(:push).with(user, payload).never Jobs::SendPushNotification.new.execute(user_id: user, payload: payload) end - end - context "with inactive offline user" do - it "sends push notification" do - user.update!(last_seen_at: 40.minutes.ago) + it "sends push notification when user is offline" do + user.update!(last_seen_at: 20.minutes.ago) PushNotificationPusher.expects(:push).with(user, payload) Jobs::SendPushNotification.new.execute(user_id: user, payload: payload) end end + + context "with invalid user" do + it "does not send push notification" do + PushNotificationPusher.expects(:push).with(user, payload).never + + Jobs::SendPushNotification.new.execute(user_id: -999, payload: payload) + end + end end