FIX: Thread mention read state and notification links (#21385)

* FIX: Link to thread for mentions inside thread

When mentioning a user in a thread, when we send the
notification and display it in the UI we want the URL
of the notification to point to the thread URL to open
the panel, rather than the main channel which is confusing.

For now, we don't have a way to highlight the linked-to message
in the thread, we can revisit this later.

* FIX: Mark mention notifications read when thread opens

Since we have no scrolling/message visibility/thread membership
for now, when a user opens the thread panel we just want to mark
all mention notifications relating to messages in the thread
for the user as read.

plugins/chat/app/controllers/chat/api/thread_reads_controller.rb

@@ -0,0 +1,11 @@
+# frozen_string_literal: true
+
+class Chat::Api::ThreadReadsController < Chat::ApiController
+  def update
+    params.require(%i[channel_id thread_id])
+
+    with_service(Chat::UpdateUserThreadLastRead) do
+      on_model_not_found(:thread) { raise Discourse::NotFound }
+    end
+  end
+end

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

@@ -40,6 +40,8 @@ module Jobs
           is_direct_message_channel: @chat_channel.direct_message_channel?,
         }
 
+        data[:chat_thread_id] = @chat_message.thread_id if @chat_message.in_thread?
+
         if !@is_direct_message_channel
           data[:chat_channel_title] = @chat_channel.title(membership.user)
           data[:chat_channel_slug] = @chat_channel.slug
@@ -61,12 +63,19 @@ module Jobs
       end
 
       def build_payload_for(membership, identifier_type:)
+        post_url =
+          if @chat_message.in_thread?
+            @chat_message.thread.relative_url
+          else
+            "#{@chat_channel.relative_url}/#{@chat_message.id}"
+          end
+
         payload = {
           notification_type: ::Notification.types[:chat_mention],
           username: @creator.username,
           tag: ::Chat::Notifier.push_notification_tag(:mention, @chat_channel.id),
           excerpt: @chat_message.push_notification_excerpt,
-          post_url: "#{@chat_channel.relative_url}/#{@chat_message.id}",
+          post_url: post_url,
         }
 
         translation_prefix =

plugins/chat/app/services/chat/action/mark_mentions_read.rb

@@ -10,7 +10,9 @@ module Chat
       #   marked as read.
       # @param [Integer] message_id Optional, used to limit the max message ID to mark
       #   mentions read for in the channel.
-      def self.call(user, channel_ids:, message_id: nil)
+      # @param [Integer] thread_id Optional, if provided then all notifications related
+      #   to messages in the thread will be marked as read.
+      def self.call(user, channel_ids:, message_id: nil, thread_id: nil)
         ::Notification
           .where(notification_type: Notification.types[:chat_mention])
           .where(user: user)
@@ -19,8 +21,14 @@ module Chat
           .joins("INNER JOIN chat_messages ON chat_mentions.chat_message_id = chat_messages.id")
           .where("chat_messages.chat_channel_id IN (?)", channel_ids)
           .then do |notifications|
-            break notifications if message_id.blank?
-            notifications.where("chat_messages.id <= ?", message_id)
+            break notifications if message_id.blank? && thread_id.blank?
+            break notifications.where("chat_messages.id <= ?", message_id) if message_id.present?
+            if thread_id.present?
+              notifications.where(
+                "chat_messages.id IN (SELECT id FROM chat_messages WHERE thread_id = ?)",
+                thread_id,
+              )
+            end
           end
           .update_all(read: true)
       end

plugins/chat/app/services/chat/mark_all_user_channels_read.rb

@@ -49,7 +49,7 @@ module Chat
     def mark_associated_mentions_as_read(guardian:, updated_memberships:, **)
       return if updated_memberships.empty?
 
-      Chat::Action::MarkMentionsRead.call(
+      ::Chat::Action::MarkMentionsRead.call(
         guardian.user,
         channel_ids: updated_memberships.map(&:channel_id),
       )

plugins/chat/app/services/chat/update_user_last_read.rb

@@ -61,7 +61,7 @@ module Chat
     end
 
     def mark_associated_mentions_as_read(active_membership:, contract:, **)
-      Chat::Action::MarkMentionsRead.call(
+      ::Chat::Action::MarkMentionsRead.call(
         active_membership.user,
         channel_ids: [active_membership.chat_channel.id],
         message_id: contract.message_id,

plugins/chat/app/services/chat/update_user_thread_last_read.rb

@@ -0,0 +1,61 @@
+# frozen_string_literal: true
+
+module Chat
+  # Service responsible for marking messages in a thread
+  # as read. For now this just marks any mentions in the thread
+  # as read, but as we add user tracking state to threads it
+  # will work in a similar way to Chat::UpdateUserLastRead
+  #
+  # @example
+  #  Chat::UpdateUserThreadLastRead.call(channel_id: 2, thread_id: 3, guardian: guardian)
+  #
+  class UpdateUserThreadLastRead
+    include ::Service::Base
+
+    # @!method call(channel_id:, thread_id:, guardian:)
+    #   @param [Integer] channel_id
+    #   @param [Integer] thread_id
+    #   @param [Guardian] guardian
+    #   @return [Service::Base::Context]
+
+    contract
+    model :thread
+    policy :invalid_access
+    step :mark_associated_mentions_as_read
+    step :publish_new_last_read_to_clients
+
+    # @!visibility private
+    class Contract
+      attribute :thread_id, :integer
+      attribute :channel_id, :integer
+
+      validates :thread_id, :channel_id, presence: true
+    end
+
+    private
+
+    def fetch_thread(contract:, **)
+      ::Chat::Thread.find_by(id: contract.thread_id, channel_id: contract.channel_id)
+    end
+
+    def invalid_access(guardian:, thread:, **)
+      guardian.can_join_chat_channel?(thread.channel)
+    end
+
+    def mark_associated_mentions_as_read(thread:, guardian:, **)
+      ::Chat::Action::MarkMentionsRead.call(
+        guardian.user,
+        channel_ids: [thread.channel_id],
+        thread_id: thread.id,
+      )
+    end
+
+    def publish_new_last_read_to_clients(guardian:, thread:, **)
+      ::Chat::Publisher.publish_user_tracking_state(
+        guardian.user,
+        thread.channel_id,
+        thread.replies.last.id,
+      )
+    end
+  end
+end

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

@@ -1,4 +1,5 @@
 import Component from "@glimmer/component";
+import { Promise } from "rsvp";
 import { tracked } from "@glimmer/tracking";
 import { action } from "@ember/object";
 import ChatMessage from "discourse/plugins/chat/discourse/models/chat-message";
@@ -93,7 +94,7 @@ export default class ChatThreadPanel extends Component {
   @debounce(100)
   fetchMessages() {
     if (this._selfDeleted) {
-      return;
+      return Promise.resolve();
     }
 
     this.loadingMorePast = true;
@@ -138,6 +139,8 @@ export default class ChatThreadPanel extends Component {
         // } else if (messages.length) {
         //   this.scrollToMessage(messages.lastObject.id);
         // }
+        //
+        this.markThreadAsRead();
       })
       .catch(this.#handleErrors)
       .finally(() => {
@@ -179,6 +182,13 @@ export default class ChatThreadPanel extends Component {
     return [messages, results.meta];
   }
 
+  // NOTE: At some point we want to do this based on visible messages
+  // and scrolling; for now it's enough to do it when the thread panel
+  // opens/messages are loaded since we have no pagination for threads.
+  markThreadAsRead() {
+    return this.chatApi.markThreadAsRead(this.channel.id, this.thread.id);
+  }
+
   @action
   onSendMessage(message) {
     if (message.editing) {

plugins/chat/assets/javascripts/discourse/initializers/chat-user-menu.js

@@ -59,9 +59,16 @@ export default {
                   title: this.notification.data.chat_channel_title,
                   slug: this.notification.data.chat_channel_slug,
                 });
-                return `/chat/c/${slug || "-"}/${
+
+                let notificationRoute = `/chat/c/${slug || "-"}/${
                   this.notification.data.chat_channel_id
-                }/${this.notification.data.chat_message_id}`;
+                }`;
+                if (this.notification.data.chat_thread_id) {
+                  notificationRoute += `/t/${this.notification.data.chat_thread_id}`;
+                } else {
+                  notificationRoute += `/${this.notification.data.chat_message_id}`;
+                }
+                return notificationRoute;
               }
 
               get linkTitle() {

plugins/chat/assets/javascripts/discourse/models/chat-channel.js

@@ -202,8 +202,8 @@ export default class ChatChannel extends RestModel {
       return;
     }
 
-    // TODO (martin) Change this to use chatApi service once we change this
-    // class not to use RestModel
+    // TODO (martin) Change this to use chatApi service markChannelAsRead once we change this
+    // class not to use RestModel.
     return ajax(`/chat/api/channels/${this.id}/read/${messageId}`, {
       method: "PUT",
     });

plugins/chat/assets/javascripts/discourse/services/chat-api.js

@@ -407,6 +407,20 @@ export default class ChatApi extends Service {
     return this.#putRequest(`/channels/${channelId}/read/${messageId}`);
   }
 
+  /**
+   * Marks all messages and mentions in a thread as read. This is quite
+   * far-reaching for now, and is not granular since there is no membership/
+   * read state per-user for threads. In future this will be expanded to
+   * also pass message ID in the same way as markChannelAsRead
+   *
+   * @param {number} channelId - The ID of the channel for the thread being marked as read.
+   * @param {number} threadId - The ID of the thread being marked as read.
+   * @returns {Promise}
+   */
+  markThreadAsRead(channelId, threadId) {
+    return this.#putRequest(`/channels/${channelId}/threads/${threadId}/read`);
+  }
+
   get #basePath() {
     return "/chat/api";
   }

plugins/chat/assets/javascripts/discourse/widgets/chat-mention-notification-item.js

@@ -48,9 +48,15 @@ const chatNotificationItem = {
       title: data.chat_channel_title,
       slug: data.chat_channel_slug,
     });
-    return `/chat/c/${slug || "-"}/${data.chat_channel_id}/${
-      data.chat_message_id
-    }`;
+
+    let notificationRoute = `/chat/c/${slug || "-"}/${data.chat_channel_id}`;
+    if (data.chat_thread_id) {
+      notificationRoute += `/t/${data.chat_thread_id}`;
+    } else {
+      notificationRoute += `/${data.chat_message_id}`;
+    }
+
+    return notificationRoute;
   },
 };
 

plugins/chat/config/routes.rb

@@ -29,6 +29,7 @@ Chat::Engine.routes.draw do
     get "/mentions/groups" => "hints#check_group_mentions", :format => :json
 
     get "/channels/:channel_id/threads/:thread_id" => "channel_threads#show"
+    put "/channels/:channel_id/threads/:thread_id/read" => "thread_reads#update"
 
     put "/channels/:channel_id/messages/:message_id/restore" => "channel_messages#restore"
     delete "/channels/:channel_id/messages/:message_id" => "channel_messages#destroy"

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

@@ -21,9 +21,20 @@ describe Jobs::Chat::NotifyMentioned do
     end
   end
 
-  def create_chat_message(channel: public_channel, author: user_1, mentioned_user: user_2)
+  def create_chat_message(
+    channel: public_channel,
+    author: user_1,
+    mentioned_user: user_2,
+    thread: nil
+  )
     message =
-      Fabricate(:chat_message, chat_channel: channel, user: author, created_at: 10.minutes.ago)
+      Fabricate(
+        :chat_message,
+        chat_channel: channel,
+        user: author,
+        created_at: 10.minutes.ago,
+        thread: thread,
+      )
     Fabricate(:chat_mention, chat_message: message, user: mentioned_user)
     message
   end
@@ -408,6 +419,29 @@ describe Jobs::Chat::NotifyMentioned do
         expect(desktop_notification.data[:translated_title]).to eq(payload_translated_title)
       end
 
+      context "when the mention is within a thread" do
+        before do
+          SiteSetting.enable_experimental_chat_threaded_discussions = true
+          public_channel.update!(threading_enabled: true)
+        end
+
+        fab!(:thread) { Fabricate(:chat_thread, channel: public_channel) }
+
+        it "uses the thread URL for the post_url in the desktop notification" do
+          message = create_chat_message(thread: thread)
+          desktop_notification =
+            track_desktop_notification(message: message, to_notify_ids_map: to_notify_ids_map)
+          expect(desktop_notification.data[:post_url]).to eq(thread.relative_url)
+        end
+
+        it "includes the thread ID in the core notification data" do
+          message = create_chat_message(thread: thread)
+          created_notification =
+            track_core_notification(message: message, to_notify_ids_map: to_notify_ids_map)
+          expect(created_notification.data_hash[:chat_thread_id]).to eq(thread.id)
+        end
+      end
+
       context "with private channels" do
         it "users a different translated title" do
           message = create_chat_message(channel: @personal_chat_channel)

plugins/chat/spec/requests/chat/api/thread_reads_controller_spec.rb

@@ -0,0 +1,72 @@
+# frozen_string_literal: true
+
+RSpec.describe Chat::Api::ThreadReadsController do
+  fab!(:current_user) { Fabricate(:user) }
+
+  before do
+    SiteSetting.chat_enabled = true
+    SiteSetting.chat_allowed_groups = Group::AUTO_GROUPS[:everyone]
+    sign_in(current_user)
+  end
+
+  describe "#update" do
+    describe "marking the thread messages as read" do
+      fab!(:channel) { Fabricate(:chat_channel) }
+      fab!(:other_user) { Fabricate(:user) }
+      fab!(:thread) { Fabricate(:chat_thread, channel: channel) }
+      fab!(:message_1) do
+        Fabricate(:chat_message, chat_channel: channel, user: other_user, thread: thread)
+      end
+      fab!(:message_2) do
+        Fabricate(:chat_message, chat_channel: channel, user: other_user, thread: thread)
+      end
+
+      context "when the user cannot access the channel" do
+        fab!(:channel) { Fabricate(:private_category_channel) }
+        fab!(:thread) { Fabricate(:chat_thread, channel: channel) }
+        it "raises invalid access" do
+          put "/chat/api/channels/#{channel.id}/threads/#{thread.id}/read.json"
+          expect(response.status).to eq(403)
+        end
+      end
+
+      context "when the channel_id and thread_id params do not match" do
+        it "raises a not found" do
+          put "/chat/api/channels/#{Fabricate(:chat_channel).id}/threads/#{thread.id}/read.json"
+          expect(response.status).to eq(404)
+        end
+      end
+
+      it "marks all mention notifications as read for the thread" do
+        notification_1 = create_notification_and_mention_for(current_user, other_user, message_1)
+        notification_2 = create_notification_and_mention_for(current_user, other_user, message_2)
+
+        put "/chat/api/channels/#{channel.id}/threads/#{thread.id}/read.json"
+        expect(response.status).to eq(200)
+        expect(notification_1.reload.read).to eq(true)
+        expect(notification_2.reload.read).to eq(true)
+      end
+    end
+  end
+
+  def create_notification_and_mention_for(user, sender, msg)
+    Notification
+      .create!(
+        notification_type: Notification.types[:chat_mention],
+        user: user,
+        high_priority: true,
+        read: false,
+        data: {
+          message: "chat.mention_notification",
+          chat_message_id: msg.id,
+          chat_channel_id: msg.chat_channel_id,
+          chat_channel_title: msg.chat_channel.title(user),
+          chat_channel_slug: msg.chat_channel.slug,
+          mentioned_by_username: sender.username,
+        }.to_json,
+      )
+      .tap do |notification|
+        Chat::Mention.create!(user: user, chat_message: msg, notification: notification)
+      end
+  end
+end

plugins/chat/spec/services/chat/update_user_thread_last_read_spec.rb

@@ -0,0 +1,91 @@
+# frozen_string_literal: true
+
+RSpec.describe Chat::UpdateUserThreadLastRead do
+  describe Chat::UpdateUserThreadLastRead::Contract, type: :model do
+    it { is_expected.to validate_presence_of :channel_id }
+    it { is_expected.to validate_presence_of :thread_id }
+  end
+
+  describe ".call" do
+    subject(:result) { described_class.call(params) }
+
+    fab!(:current_user) { Fabricate(:user) }
+    fab!(:channel) { Fabricate(:chat_channel) }
+    fab!(:thread) { Fabricate(:chat_thread, channel: channel) }
+
+    let(:guardian) { Guardian.new(current_user) }
+    let(:params) { { guardian: guardian, channel_id: channel.id, thread_id: thread.id } }
+
+    context "when params are not valid" do
+      before { params.delete(:thread_id) }
+
+      it { is_expected.to fail_a_contract }
+    end
+
+    context "when params are valid" do
+      context "when user can’t access the channel" do
+        fab!(:channel) { Fabricate(:private_category_channel) }
+        fab!(:thread) { Fabricate(:chat_thread, channel: channel) }
+
+        it { is_expected.to fail_a_policy(:invalid_access) }
+      end
+
+      context "when thread cannot be found" do
+        before { params[:channel_id] = Fabricate(:chat_channel).id }
+
+        it { is_expected.to fail_to_find_a_model(:thread) }
+      end
+
+      context "when everything is fine" do
+        fab!(:notification_1) do
+          Fabricate(
+            :notification,
+            notification_type: Notification.types[:chat_mention],
+            user: current_user,
+          )
+        end
+        fab!(:notification_2) do
+          Fabricate(
+            :notification,
+            notification_type: Notification.types[:chat_mention],
+            user: current_user,
+          )
+        end
+
+        let(:messages) { MessageBus.track_publish { result } }
+
+        before do
+          Jobs.run_immediately!
+          Chat::Mention.create!(
+            notification: notification_1,
+            user: current_user,
+            chat_message: Fabricate(:chat_message, chat_channel: channel, thread: thread),
+          )
+          Chat::Mention.create!(
+            notification: notification_2,
+            user: current_user,
+            chat_message: Fabricate(:chat_message, chat_channel: channel, thread: thread),
+          )
+        end
+
+        it "sets the service result as successful" do
+          expect(result).to be_a_success
+        end
+
+        it "marks existing notifications related to all messages in the thread as read" do
+          expect { result }.to change {
+            Notification.where(
+              notification_type: Notification.types[:chat_mention],
+              user: current_user,
+              read: false,
+            ).count
+          }.by(-2)
+        end
+
+        it "publishes new last read to clients" do
+          expect(messages.map(&:channel)).to include("/chat/user-tracking-state/#{current_user.id}")
+        end
+      end
+    end
+  end
+end

plugins/chat/spec/system/user_menu_notifications/sidebar_spec.rb

@@ -129,6 +129,31 @@ RSpec.describe "User menu notifications | sidebar", type: :system, js: true do
             href: "/chat/c/#{channel_1.slug}/#{channel_1.id}/#{message.id}",
           )
         end
+
+        it "shows a mention notification when the message is in a thread" do
+          Jobs.run_immediately!
+
+          message =
+            Chat::MessageCreator.create(
+              chat_channel: channel_1,
+              user: other_user,
+              content: "this is fine @#{current_user.username}",
+              thread_id: Fabricate(:chat_thread, channel: channel_1).id,
+            ).chat_message
+
+          visit("/")
+
+          find(".header-dropdown-toggle.current-user").click
+          within("#user-menu-button-chat-notifications") do |panel|
+            expect(panel).to have_content(1)
+            panel.click
+          end
+
+          expect(find("#quick-access-chat-notifications")).to have_link(
+            I18n.t("js.notifications.popup.chat_mention.direct", channel: channel_1.name),
+            href: "/chat/c/#{channel_1.slug}/#{channel_1.id}/t/#{message.thread_id}",
+          )
+        end
       end
 
       context "when @all" do