diff --git a/plugins/chat/app/controllers/chat/api/thread_reads_controller.rb b/plugins/chat/app/controllers/chat/api/thread_reads_controller.rb new file mode 100644 index 00000000000..dfb72d9b70f --- /dev/null +++ b/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 diff --git a/plugins/chat/app/jobs/regular/chat/notify_mentioned.rb b/plugins/chat/app/jobs/regular/chat/notify_mentioned.rb index e88798a74c0..3388d17973a 100644 --- a/plugins/chat/app/jobs/regular/chat/notify_mentioned.rb +++ b/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 = diff --git a/plugins/chat/app/services/chat/action/mark_mentions_read.rb b/plugins/chat/app/services/chat/action/mark_mentions_read.rb index 602b227caf1..8d0207d63bb 100644 --- a/plugins/chat/app/services/chat/action/mark_mentions_read.rb +++ b/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 diff --git a/plugins/chat/app/services/chat/mark_all_user_channels_read.rb b/plugins/chat/app/services/chat/mark_all_user_channels_read.rb index c3a67338d85..9c6376fbc63 100644 --- a/plugins/chat/app/services/chat/mark_all_user_channels_read.rb +++ b/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), ) diff --git a/plugins/chat/app/services/chat/update_user_last_read.rb b/plugins/chat/app/services/chat/update_user_last_read.rb index 5d2b66e60fb..3ba18cb0d4b 100644 --- a/plugins/chat/app/services/chat/update_user_last_read.rb +++ b/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, diff --git a/plugins/chat/app/services/chat/update_user_thread_last_read.rb b/plugins/chat/app/services/chat/update_user_thread_last_read.rb new file mode 100644 index 00000000000..70845090a8d --- /dev/null +++ b/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 diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-thread.js b/plugins/chat/assets/javascripts/discourse/components/chat-thread.js index 90993d746eb..baf84060e22 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-thread.js +++ b/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) { diff --git a/plugins/chat/assets/javascripts/discourse/initializers/chat-user-menu.js b/plugins/chat/assets/javascripts/discourse/initializers/chat-user-menu.js index ca30056fcda..b2c0d4ca8c5 100644 --- a/plugins/chat/assets/javascripts/discourse/initializers/chat-user-menu.js +++ b/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() { diff --git a/plugins/chat/assets/javascripts/discourse/models/chat-channel.js b/plugins/chat/assets/javascripts/discourse/models/chat-channel.js index 76ba2a1209b..8bdc0d26180 100644 --- a/plugins/chat/assets/javascripts/discourse/models/chat-channel.js +++ b/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", }); diff --git a/plugins/chat/assets/javascripts/discourse/services/chat-api.js b/plugins/chat/assets/javascripts/discourse/services/chat-api.js index 3c29d8041f3..d28d92663fc 100644 --- a/plugins/chat/assets/javascripts/discourse/services/chat-api.js +++ b/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"; } diff --git a/plugins/chat/assets/javascripts/discourse/widgets/chat-mention-notification-item.js b/plugins/chat/assets/javascripts/discourse/widgets/chat-mention-notification-item.js index beeeb4c34f4..6b64fe8ed9e 100644 --- a/plugins/chat/assets/javascripts/discourse/widgets/chat-mention-notification-item.js +++ b/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; }, }; diff --git a/plugins/chat/config/routes.rb b/plugins/chat/config/routes.rb index 53d1a723fb6..594f23f2b3c 100644 --- a/plugins/chat/config/routes.rb +++ b/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" diff --git a/plugins/chat/spec/jobs/regular/chat/notify_mentioned_spec.rb b/plugins/chat/spec/jobs/regular/chat/notify_mentioned_spec.rb index a72aa02b4bd..7ac9d674613 100644 --- a/plugins/chat/spec/jobs/regular/chat/notify_mentioned_spec.rb +++ b/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) diff --git a/plugins/chat/spec/requests/chat/api/thread_reads_controller_spec.rb b/plugins/chat/spec/requests/chat/api/thread_reads_controller_spec.rb new file mode 100644 index 00000000000..4f1c0808c33 --- /dev/null +++ b/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 diff --git a/plugins/chat/spec/services/chat/update_user_thread_last_read_spec.rb b/plugins/chat/spec/services/chat/update_user_thread_last_read_spec.rb new file mode 100644 index 00000000000..fcf8ae1863f --- /dev/null +++ b/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 diff --git a/plugins/chat/spec/system/user_menu_notifications/sidebar_spec.rb b/plugins/chat/spec/system/user_menu_notifications/sidebar_spec.rb index a9fe1464dea..3bfa6ad19ac 100644 --- a/plugins/chat/spec/system/user_menu_notifications/sidebar_spec.rb +++ b/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