FIX: correctly handle notifications for channels (#27178)

Prior to this fix we had too logic to detect if a user is active or not:

- idle codepath on the frontend
- online user ids on the backend

The frontend solution is not very reliable, and both solution are just trying to be too smart. Making a lot of people questioning why they receive a notification sometimes and sometimes not. This commit removes all this logic and replaces it with a much more simpler logic:

- you can't receive notifications for channel you are actually watching
- we won't play a sound more than once every 3seconds

app/assets/javascripts/discourse/app/instance-initializers/subscribe-user-notifications.js

@@ -68,7 +68,7 @@ export default {
     if (!isTesting()) {
       this.messageBus.subscribe(alertChannel(this.currentUser), this.onAlert);
 
-      initDesktopNotifications(this.messageBus, this.appEvents);
+      initDesktopNotifications(this.messageBus);
 
       if (isPushNotificationsEnabled(this.currentUser)) {
         disableDesktopNotifications();

app/assets/javascripts/discourse/app/lib/desktop-notifications.js

@@ -11,11 +11,8 @@ let primaryTab = false;
 let liveEnabled = false;
 let havePermission = null;
 let mbClientId = "xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx";
-let lastAction = -1;
 
 const focusTrackerKey = "focus-tracker";
-const idleThresholdTime = 1000 * 10; // 10 seconds
-
 const context = "discourse_desktop_notifications_";
 const keyValueStore = new KeyValueStore(context);
 
@@ -28,7 +25,7 @@ export function clearDesktopNotificationHandlers() {
 }
 
 // Called from an initializer
-function init(messageBus, appEvents) {
+function init(messageBus) {
   liveEnabled = false;
   mbClientId = messageBus.clientId;
 
@@ -72,7 +69,7 @@ function init(messageBus, appEvents) {
   liveEnabled = true;
   try {
     // Preliminary checks passed, continue with setup
-    setupNotifications(appEvents);
+    setupNotifications();
   } catch (e) {
     // eslint-disable-next-line no-console
     console.error(e);
@@ -101,7 +98,7 @@ function confirmNotification(siteSettings) {
 }
 
 // This function is only called if permission was granted
-function setupNotifications(appEvents) {
+function setupNotifications() {
   window.addEventListener("storage", function (e) {
     // note: This event only fires when other tabs setItem()
     const key = e.key;
@@ -112,8 +109,6 @@ function setupNotifications(appEvents) {
   });
 
   window.addEventListener("focus", function () {
-    resetIdle();
-
     if (!primaryTab) {
       primaryTab = true;
       keyValueStore.setItem(focusTrackerKey, mbClientId);
@@ -130,35 +125,13 @@ function setupNotifications(appEvents) {
     primaryTab = true;
     keyValueStore.setItem(focusTrackerKey, mbClientId);
   }
-
-  if (document) {
-    document.addEventListener("scroll", resetIdle);
-  }
-
-  appEvents.on("page:changed", resetIdle);
 }
 
-function resetIdle() {
-  lastAction = Date.now() - 10;
-}
-
-function isIdle(idleThreshold = idleThresholdTime) {
-  return lastAction + idleThreshold <= Date.now();
-}
-
-function setLastAction(time) {
-  lastAction = time;
-}
-
-function canUserReceiveNotifications(user, options = { idleThresholdTime }) {
+function canUserReceiveNotifications(user) {
   if (!primaryTab) {
     return false;
   }
 
-  if (!isIdle(options.idleThresholdTime)) {
-    return false;
-  }
-
   if (user.isInDoNotDisturb()) {
     return false;
   }
@@ -264,6 +237,4 @@ export {
   confirmNotification,
   disable,
   canUserReceiveNotifications,
-  resetIdle,
-  setLastAction,
 };

plugins/chat/app/jobs/regular/chat/notify_watching.rb

@@ -47,19 +47,23 @@ module Jobs
         user = membership.user
         return unless user.guardian.can_join_chat_channel?(@chat_channel)
         return if ::Chat::Notifier.user_has_seen_message?(membership, @chat_message.id)
-        return if online_user_ids.include?(user.id)
 
         translation_key =
           (
             if @is_direct_message_channel
-              "discourse_push_notifications.popup.new_direct_chat_message"
+              if @chat_channel.chatable.group
+                "discourse_push_notifications.popup.new_chat_message"
+              else
+                "discourse_push_notifications.popup.new_direct_chat_message"
+              end
             else
               "discourse_push_notifications.popup.new_chat_message"
             end
           )
 
         translation_args = { username: @creator.username }
-        translation_args[:channel] = @chat_channel.title(user) unless @is_direct_message_channel
+        translation_args[:channel] = @chat_channel.title(user) unless @is_direct_message_channel &&
+          !@chat_channel.chatable.group
         translation_args =
           DiscoursePluginRegistry.apply_modifier(
             :chat_notification_translation_args,
@@ -76,6 +80,7 @@ module Jobs
           translated_title: translated_title,
           tag: ::Chat::Notifier.push_notification_tag(:message, @chat_channel.id),
           excerpt: @chat_message.push_notification_excerpt,
+          channel_id: @chat_channel.id,
         }
 
         if membership.desktop_notifications_always? && !membership.muted?
@@ -96,10 +101,6 @@ module Jobs
           ::PostAlerter.push_notification(user, payload)
         end
       end
-
-      def online_user_ids
-        @online_user_ids ||= ::PresenceChannel.new("/chat/online").user_ids
-      end
     end
   end
 end

plugins/chat/assets/javascripts/discourse/components/chat-channel.gjs

@@ -10,7 +10,6 @@ import { service } from "@ember/service";
 import { and, not } from "truth-helpers";
 import concatClass from "discourse/helpers/concat-class";
 import { popupAjaxError } from "discourse/lib/ajax-error";
-import { resetIdle } from "discourse/lib/desktop-notifications";
 import DiscourseURL from "discourse/lib/url";
 import {
   onPresenceChange,
@@ -477,7 +476,6 @@ export default class ChatChannel extends Component {
 
   @action
   onScrollEnd(state) {
-    resetIdle();
     this.needsArrow =
       (this.messagesLoader.fetchedOnce &&
         this.messagesLoader.canLoadMoreFuture) ||
@@ -536,8 +534,6 @@ export default class ChatChannel extends Component {
   async #sendNewMessage(message) {
     this.pane.sending = true;
 
-    resetIdle();
-
     stackingContextFix(this.scroller, async () => {
       await this.args.channel.stageMessage(message);
     });

plugins/chat/assets/javascripts/discourse/components/chat-thread.gjs

@@ -8,7 +8,6 @@ import { cancel, next } from "@ember/runloop";
 import { service } from "@ember/service";
 import concatClass from "discourse/helpers/concat-class";
 import { popupAjaxError } from "discourse/lib/ajax-error";
-import { resetIdle } from "discourse/lib/desktop-notifications";
 import { NotificationLevels } from "discourse/lib/notification-levels";
 import discourseDebounce from "discourse-common/lib/debounce";
 import { bind } from "discourse-common/utils/decorators";
@@ -61,11 +60,6 @@ export default class ChatThread extends Component {
 
   scroller = null;
 
-  @action
-  resetIdle() {
-    resetIdle();
-  }
-
   @cached
   get messagesLoader() {
     return new ChatMessagesLoader(getOwner(this), this.args.thread);
@@ -146,7 +140,6 @@ export default class ChatThread extends Component {
         this.messagesLoader.canLoadMoreFuture) ||
       (state.distanceToBottom.pixels > 250 && !state.atBottom);
     this.isScrolling = false;
-    this.resetIdle();
     this.atBottom = state.atBottom;
     this.args.setFullTitle?.(state.atTop);
 
@@ -366,8 +359,6 @@ export default class ChatThread extends Component {
 
   @action
   async onSendMessage(message) {
-    resetIdle();
-
     await message.cook();
     if (message.editing) {
       await this.#sendEditMessage(message);

plugins/chat/assets/javascripts/discourse/services/chat-channel-notification-sound.js

@@ -1,52 +0,0 @@
-import Service, { service } from "@ember/service";
-import { canUserReceiveNotifications } from "discourse/lib/desktop-notifications";
-
-export default class ChatChannelNotificationSound extends Service {
-  @service chat;
-  @service chatAudioManager;
-  @service currentUser;
-  @service site;
-
-  async play(channel) {
-    if (
-      !canUserReceiveNotifications(this.currentUser, {
-        idleThresholdTime: 0,
-      })
-    ) {
-      return false;
-    }
-
-    if (channel.isCategoryChannel) {
-      return false;
-    }
-
-    if (!this.currentUser.chat_sound) {
-      return false;
-    }
-
-    if (this.site.mobileView) {
-      return false;
-    }
-
-    const membership = channel.currentUserMembership;
-    if (!membership.following) {
-      return false;
-    }
-
-    if (membership.desktopNotificationLevel !== "always") {
-      return false;
-    }
-
-    if (membership.muted) {
-      return false;
-    }
-
-    if (this.chat.activeChannel === channel) {
-      return false;
-    }
-
-    await this.chatAudioManager.play(this.currentUser.chat_sound);
-
-    return true;
-  }
-}

plugins/chat/assets/javascripts/discourse/services/chat-notification-manager.js

@@ -145,6 +145,10 @@ export default class ChatNotificationManager extends Service {
 
   @bind
   onMessage(data) {
+    if (data.channel_id === this.chat.activeChannel?.id) {
+      return;
+    }
+
     return onNotification(
       data,
       this.siteSettings,

plugins/chat/assets/javascripts/discourse/services/chat-subscriptions-manager.js

@@ -7,7 +7,6 @@ import ChatChannelArchive from "../models/chat-channel-archive";
 export default class ChatSubscriptionsManager extends Service {
   @service store;
   @service chatChannelsManager;
-  @service chatChannelNotificationSound;
   @service chatTrackingStateManager;
   @service currentUser;
   @service appEvents;
@@ -206,14 +205,6 @@ export default class ChatSubscriptionsManager extends Service {
             channel.tracking.unreadCount++;
           }
 
-          const secondsPassed = moment().diff(
-            moment(busData.message.created_at),
-            "seconds"
-          );
-          if (secondsPassed < 10) {
-            this.chatChannelNotificationSound.play(channel);
-          }
-
           // Thread should be considered unread if not already.
           if (busData.thread_id && channel.threadingEnabled) {
             channel.threadsManager

plugins/chat/spec/jobs/regular/chat/notify_watching_spec.rb

@@ -170,14 +170,6 @@ RSpec.describe Jobs::Chat::NotifyWatching do
       end
     end
 
-    context "when the target user is online via presence channel" do
-      before { PresenceChannel.any_instance.expects(:user_ids).returns([user2.id]) }
-
-      it "does not send a desktop notification" do
-        expect(notification_messages_for(user2).count).to be_zero
-      end
-    end
-
     context "when the target user is suspended" do
       before { user2.update!(suspended_till: 1.year.from_now) }
 
@@ -310,14 +302,6 @@ RSpec.describe Jobs::Chat::NotifyWatching do
       end
     end
 
-    context "when the target user is online via presence channel" do
-      before { PresenceChannel.any_instance.expects(:user_ids).returns([user2.id]) }
-
-      it "does not send a desktop notification" do
-        expect(notification_messages_for(user2).count).to be_zero
-      end
-    end
-
     context "when the target user is suspended" do
       before { user2.update!(suspended_till: 1.year.from_now) }
 

plugins/chat/test/javascripts/unit/services/chat-channel-notification-sound-test.js

@@ -1,139 +0,0 @@
-import { getOwner } from "@ember/application";
-import { test } from "qunit";
-import {
-  disable,
-  init,
-  resetIdle,
-  setLastAction,
-} from "discourse/lib/desktop-notifications";
-import {
-  acceptance,
-  updateCurrentUser,
-} from "discourse/tests/helpers/qunit-helpers";
-import ChatFabricators from "discourse/plugins/chat/discourse/lib/fabricators";
-
-function buildDirectMessageChannel(owner) {
-  const channel = new ChatFabricators(owner).directMessageChannel();
-  buildMembership(channel);
-  return channel;
-}
-function buildCategoryMessageChannel(owner) {
-  const channel = new ChatFabricators(owner).channel();
-  buildMembership(channel);
-  return channel;
-}
-
-function buildMembership(channel) {
-  channel.currentUserMembership = {
-    following: true,
-    desktop_notification_level: "always",
-    muted: false,
-  };
-  return channel;
-}
-
-acceptance(
-  "Discourse Chat | Unit | Service | chat-channel-notification-sound",
-  function (needs) {
-    needs.hooks.beforeEach(function () {
-      Object.defineProperty(this, "subject", {
-        get: () =>
-          this.container.lookup("service:chat-channel-notification-sound"),
-      });
-
-      Object.defineProperty(this, "site", {
-        get: () => this.container.lookup("service:site"),
-      });
-
-      Object.defineProperty(this, "chat", {
-        get: () => this.container.lookup("service:chat"),
-      });
-
-      updateCurrentUser({ chat_sound: "ding" });
-
-      init(
-        this.container.lookup("service:message-bus"),
-        this.container.lookup("service:app-events")
-      );
-
-      setLastAction(moment().subtract(1, "hour").valueOf());
-    });
-
-    needs.user();
-
-    test("in do not disturb", async function (assert) {
-      updateCurrentUser({ do_not_disturb_until: moment().add(1, "hour") });
-      const channel = buildDirectMessageChannel(getOwner(this));
-
-      assert.deepEqual(await this.subject.play(channel), false);
-    });
-
-    test("no chat sound", async function (assert) {
-      updateCurrentUser({ chat_sound: null });
-      const channel = buildDirectMessageChannel(getOwner(this));
-
-      assert.deepEqual(await this.subject.play(channel), false);
-    });
-
-    test("mobile", async function (assert) {
-      const channel = buildDirectMessageChannel(getOwner(this));
-      this.site.mobileView = true;
-
-      assert.deepEqual(await this.subject.play(channel), false);
-    });
-
-    test("plays sound", async function (assert) {
-      const channel = buildDirectMessageChannel(getOwner(this));
-
-      assert.deepEqual(await this.subject.play(channel), true);
-    });
-
-    test("muted", async function (assert) {
-      const channel = buildDirectMessageChannel(getOwner(this));
-      channel.currentUserMembership.muted = true;
-
-      assert.deepEqual(await this.subject.play(channel), false);
-    });
-
-    test("not following", async function (assert) {
-      const channel = buildDirectMessageChannel(getOwner(this));
-      channel.currentUserMembership.following = false;
-
-      assert.deepEqual(await this.subject.play(channel), false);
-    });
-
-    test("no notification", async function (assert) {
-      const channel = buildDirectMessageChannel(getOwner(this));
-      channel.currentUserMembership.desktopNotificationLevel = "never";
-
-      assert.deepEqual(await this.subject.play(channel), false);
-    });
-
-    test("currently active channel", async function (assert) {
-      const channel = buildDirectMessageChannel(getOwner(this));
-      this.chat.activeChannel = channel;
-
-      assert.deepEqual(await this.subject.play(channel), false);
-    });
-
-    test("category channel", async function (assert) {
-      const channel = buildCategoryMessageChannel(getOwner(this));
-
-      assert.deepEqual(await this.subject.play(channel), false);
-    });
-
-    test("not idle", async function (assert) {
-      const channel = buildDirectMessageChannel(getOwner(this));
-      resetIdle();
-
-      assert.deepEqual(await this.subject.play(channel), true);
-    });
-
-    test("notifications disabled", async function (assert) {
-      const channel = buildDirectMessageChannel(getOwner(this));
-      disable();
-
-      assert.deepEqual(await this.subject.play(channel), false);
-    });
-  }
-);