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/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 " +
+      "Notification is defined on window but is not responding correctly " + e
-        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();
 

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 isEnabledBrowser = false;
-  @tracked isEnabledPush;
+  @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.isPushNotificationsPreferred
-      return this.isEnabledPush === "subscribed";
+      ? this.isEnabledPush
-    } else {
+      : this.isEnabledBrowser;
-      return !this.notificationsDisabled;
-    }
   }
 
   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) {
+    if (this.isEnabledBrowser) {
-      this.setNotificationsDisabled(DISABLED);
+      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(() => {
+      await subscribePushNotification(() => {
-        this.setIsEnabledPush("subscribed");
+        this.setIsEnabledPush(true);
       }, this.siteSettings.vapid_public_key_bytes);
+
+      return true;
     } else {
-      this.setNotificationsDisabled(ENABLED);
+      await Notification.requestPermission((permission) => {
-      return Notification.requestPermission((permission) => {
         confirmNotification(this.siteSettings);
-        return permission === "granted";
+        if (permission === "granted") {
+          this.setIsEnabledBrowser(true);
+          return true;
+        } else {
+          return false;
+        }
       });
     }
   }

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)
+      push_window = SiteSetting.push_notification_time_window_mins
-        return
+      return if !user || (push_window > 0 && user.seen_since?(push_window.minutes.ago))
-      end
 
       PushNotificationPusher.push(user, args[:payload])
     end

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)

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) }

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
+  context "with valid user" do
-    it "does not send push notification" do
+    it "does not send push notification when user is online" do
-      user.update!(last_seen_at: 5.minutes.ago)
+      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 when user is offline" do
-    it "sends push notification" do
+      user.update!(last_seen_at: 20.minutes.ago)
-      user.update!(last_seen_at: 40.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