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