diff --git a/plugins/chat/app/controllers/chat/api/reads_controller.rb b/plugins/chat/app/controllers/chat/api/reads_controller.rb new file mode 100644 index 00000000000..ec80f2b7f67 --- /dev/null +++ b/plugins/chat/app/controllers/chat/api/reads_controller.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +class Chat::Api::ReadsController < Chat::ApiController + def update + params.require(%i[channel_id message_id]) + + with_service(Chat::UpdateUserLastRead) do + on_failed_policy(:ensure_message_id_recency) do + raise Discourse::InvalidParameters.new(:message_id) + end + on_failed_policy(:ensure_message_exists) { raise Discourse::NotFound } + on_model_not_found(:active_membership) { raise Discourse::NotFound } + on_model_not_found(:channel) { raise Discourse::NotFound } + end + end + + def update_all + with_service(Chat::MarkAllUserChannelsRead) do + on_success do + render(json: success_json.merge(updated_memberships: result.updated_memberships)) + end + end + end +end diff --git a/plugins/chat/app/controllers/chat/chat_controller.rb b/plugins/chat/app/controllers/chat/chat_controller.rb index 0be60a23526..4e3f63de529 100644 --- a/plugins/chat/app/controllers/chat/chat_controller.rb +++ b/plugins/chat/app/controllers/chat/chat_controller.rb @@ -161,47 +161,6 @@ module Chat render json: success_json end - def update_user_last_read - membership = - Chat::ChannelMembershipManager.new(@chat_channel).find_for_user( - current_user, - following: true, - ) - raise Discourse::NotFound if membership.nil? - - if membership.last_read_message_id && - params[:message_id].to_i < membership.last_read_message_id - raise Discourse::InvalidParameters.new(:message_id) - end - - unless Chat::Message.with_deleted.exists?( - chat_channel_id: @chat_channel.id, - id: params[:message_id], - ) - raise Discourse::NotFound - end - - membership.update!(last_read_message_id: params[:message_id]) - - Notification - .where(notification_type: Notification.types[:chat_mention]) - .where(user: current_user) - .where(read: false) - .joins("INNER JOIN chat_mentions ON chat_mentions.notification_id = notifications.id") - .joins("INNER JOIN chat_messages ON chat_mentions.chat_message_id = chat_messages.id") - .where("chat_messages.id <= ?", params[:message_id].to_i) - .where("chat_messages.chat_channel_id = ?", @chat_channel.id) - .update_all(read: true) - - Chat::Publisher.publish_user_tracking_state( - current_user, - @chat_channel.id, - params[:message_id], - ) - - render json: success_json - end - def messages page_size = params[:page_size]&.to_i || 1000 direction = params[:direction].to_s diff --git a/plugins/chat/app/helpers/chat/with_service_helper.rb b/plugins/chat/app/helpers/chat/with_service_helper.rb index c8e820cc2c3..7faf0aeff07 100644 --- a/plugins/chat/app/helpers/chat/with_service_helper.rb +++ b/plugins/chat/app/helpers/chat/with_service_helper.rb @@ -5,6 +5,9 @@ module Chat @_result end + # @param service [Class] A class including {Chat::Service::Base} + # @param dependencies [kwargs] Any additional params to load into the service context, + # in addition to controller @params. def with_service(service, default_actions: true, **dependencies, &block) object = self merged_block = diff --git a/plugins/chat/app/services/chat/action/mark_mentions_read.rb b/plugins/chat/app/services/chat/action/mark_mentions_read.rb new file mode 100644 index 00000000000..602b227caf1 --- /dev/null +++ b/plugins/chat/app/services/chat/action/mark_mentions_read.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +module Chat + module Action + # When updating the read state of chat channel memberships, we also need + # to be sure to mark any mention-based notifications read at the same time. + class MarkMentionsRead + # @param [User] user The user that we are marking notifications read for. + # @param [Array] channel_ids The chat channels that are having their notifications + # 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) + ::Notification + .where(notification_type: Notification.types[:chat_mention]) + .where(user: user) + .where(read: false) + .joins("INNER JOIN chat_mentions ON chat_mentions.notification_id = notifications.id") + .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) + end + .update_all(read: true) + end + end + end +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 new file mode 100644 index 00000000000..e9291148d8a --- /dev/null +++ b/plugins/chat/app/services/chat/mark_all_user_channels_read.rb @@ -0,0 +1,55 @@ +# frozen_string_literal: true + +module Chat + # Service responsible for marking all the channels that a user is a + # member of _and following_ as read, including mentions. + # + # @example + # Chat::MarkAllUserChannelsRead.call(guardian: guardian) + # + class MarkAllUserChannelsRead + include ::Service::Base + + # @!method call(guardian:) + # @param [Guardian] guardian + # @return [Service::Base::Context] + + transaction do + step :update_last_read_message_ids + step :mark_associated_mentions_as_read + end + + private + + def update_last_read_message_ids(guardian:, **) + updated_memberships = DB.query(<<~SQL, user_id: guardian.user.id) + UPDATE user_chat_channel_memberships + SET last_read_message_id = subquery.newest_message_id + FROM + ( + SELECT chat_messages.chat_channel_id, MAX(chat_messages.id) AS newest_message_id + FROM chat_messages + WHERE chat_messages.deleted_at IS NULL + GROUP BY chat_messages.chat_channel_id + ) AS subquery + WHERE user_chat_channel_memberships.chat_channel_id = subquery.chat_channel_id AND + subquery.newest_message_id > COALESCE(user_chat_channel_memberships.last_read_message_id, 0) AND + user_chat_channel_memberships.user_id = :user_id AND + user_chat_channel_memberships.following + RETURNING user_chat_channel_memberships.id AS membership_id, + user_chat_channel_memberships.chat_channel_id AS channel_id, + user_chat_channel_memberships.last_read_message_id; + SQL + context[:updated_memberships] = updated_memberships + end + + def mark_associated_mentions_as_read(guardian:, updated_memberships:, **) + return if updated_memberships.empty? + + Chat::Action::MarkMentionsRead.call( + guardian.user, + channel_ids: updated_memberships.map(&:channel_id), + ) + end + end +end 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 0eb01159b25..5d2b66e60fb 100644 --- a/plugins/chat/app/services/chat/update_user_last_read.rb +++ b/plugins/chat/app/services/chat/update_user_last_read.rb @@ -4,23 +4,23 @@ module Chat # Service responsible for updating the last read message id of a membership. # # @example - # Chat::UpdateUserLastRead.call(user_id: 1, channel_id: 2, message_id: 3, guardian: guardian) + # Chat::UpdateUserLastRead.call(channel_id: 2, message_id: 3, guardian: guardian) # class UpdateUserLastRead - include Service::Base + include ::Service::Base - # @!method call(user_id:, channel_id:, message_id:, guardian:) - # @param [Integer] user_id + # @!method call(channel_id:, message_id:, guardian:) # @param [Integer] channel_id # @param [Integer] message_id # @param [Guardian] guardian # @return [Service::Base::Context] contract - model :membership, :fetch_active_membership + model :channel + model :active_membership policy :invalid_access - policy :ensure_message_id_recency policy :ensure_message_exists + policy :ensure_message_id_recency step :update_last_read_message_id step :mark_associated_mentions_as_read step :publish_new_last_read_to_clients @@ -28,52 +28,48 @@ module Chat # @!visibility private class Contract attribute :message_id, :integer - attribute :user_id, :integer attribute :channel_id, :integer - validates :message_id, :user_id, :channel_id, presence: true + validates :message_id, :channel_id, presence: true end private - def fetch_active_membership(user_id:, channel_id:, **) - Chat::UserChatChannelMembership.includes(:user, :chat_channel).find_by( - user_id: user_id, - chat_channel_id: channel_id, - following: true, + def fetch_channel(contract:, **) + ::Chat::Channel.find_by(id: contract.channel_id) + end + + def fetch_active_membership(guardian:, channel:, **) + ::Chat::ChannelMembershipManager.new(channel).find_for_user(guardian.user, following: true) + end + + def invalid_access(guardian:, active_membership:, **) + guardian.can_join_chat_channel?(active_membership.chat_channel) + end + + def ensure_message_exists(channel:, contract:, **) + ::Chat::Message.with_deleted.exists?(chat_channel_id: channel.id, id: contract.message_id) + end + + def ensure_message_id_recency(contract:, active_membership:, **) + !active_membership.last_read_message_id || + contract.message_id >= active_membership.last_read_message_id + end + + def update_last_read_message_id(contract:, active_membership:, **) + active_membership.update!(last_read_message_id: contract.message_id) + end + + def mark_associated_mentions_as_read(active_membership:, contract:, **) + Chat::Action::MarkMentionsRead.call( + active_membership.user, + channel_ids: [active_membership.chat_channel.id], + message_id: contract.message_id, ) end - def invalid_access(guardian:, membership:, **) - guardian.can_join_chat_channel?(membership.chat_channel) - end - - def ensure_message_id_recency(message_id:, membership:, **) - !membership.last_read_message_id || message_id >= membership.last_read_message_id - end - - def ensure_message_exists(channel_id:, message_id:, **) - Chat::Message.with_deleted.exists?(chat_channel_id: channel_id, id: message_id) - end - - def update_last_read_message_id(message_id:, membership:, **) - membership.update!(last_read_message_id: message_id) - end - - def mark_associated_mentions_as_read(membership:, message_id:, **) - Notification - .where(notification_type: Notification.types[:chat_mention]) - .where(user: membership.user) - .where(read: false) - .joins("INNER JOIN chat_mentions ON chat_mentions.notification_id = notifications.id") - .joins("INNER JOIN chat_messages ON chat_mentions.chat_message_id = chat_messages.id") - .where("chat_messages.id <= ?", message_id) - .where("chat_messages.chat_channel_id = ?", membership.chat_channel.id) - .update_all(read: true) - end - - def publish_new_last_read_to_clients(guardian:, channel_id:, message_id:, **) - Chat::Publisher.publish_user_tracking_state(guardian.user, channel_id, message_id) + def publish_new_last_read_to_clients(guardian:, channel:, contract:, **) + ::Chat::Publisher.publish_user_tracking_state(guardian.user, channel.id, contract.message_id) end end end diff --git a/plugins/chat/assets/javascripts/discourse/initializers/chat-keyboard-shortcuts.js b/plugins/chat/assets/javascripts/discourse/initializers/chat-keyboard-shortcuts.js index ae70c3e4543..f9d564fcf97 100644 --- a/plugins/chat/assets/javascripts/discourse/initializers/chat-keyboard-shortcuts.js +++ b/plugins/chat/assets/javascripts/discourse/initializers/chat-keyboard-shortcuts.js @@ -17,6 +17,9 @@ export default { const router = container.lookup("service:router"); const appEvents = container.lookup("service:app-events"); const chatStateManager = container.lookup("service:chat-state-manager"); + const chatChannelsManager = container.lookup( + "service:chat-channels-manager" + ); const openChannelSelector = (e) => { e.preventDefault(); e.stopPropagation(); @@ -92,6 +95,15 @@ export default { appEvents.trigger("chat:toggle-close", event); }; + const markAllChannelsRead = (event) => { + event.preventDefault(); + event.stopPropagation(); + + if (chatStateManager.isActive) { + chatChannelsManager.markAllChannelsRead(); + } + }; + withPluginApi("0.12.1", (api) => { api.addKeyboardShortcut(`${KEY_MODIFIER}+k`, openChannelSelector, { global: true, @@ -201,6 +213,21 @@ export default { }, }, }); + api.addKeyboardShortcut( + `shift+esc`, + (event) => markAllChannelsRead(event), + { + global: true, + help: { + category: "chat", + name: "chat.keyboard_shortcuts.mark_all_channels_read", + definition: { + keys1: ["shift", "esc"], + keysDelimiter: "plus", + }, + }, + } + ); }); }, }; diff --git a/plugins/chat/assets/javascripts/discourse/models/chat-channel.js b/plugins/chat/assets/javascripts/discourse/models/chat-channel.js index ec45c76b929..5c629cf8d67 100644 --- a/plugins/chat/assets/javascripts/discourse/models/chat-channel.js +++ b/plugins/chat/assets/javascripts/discourse/models/chat-channel.js @@ -168,7 +168,9 @@ export default class ChatChannel extends RestModel { return; } - return ajax(`/chat/${this.id}/read/${messageId}.json`, { + // TODO (martin) Change this to use chatApi service once we change this + // class not to use RestModel + return ajax(`/chat/api/channels/${this.id}/read/${messageId}`, { method: "PUT", }).then(() => { this.currentUserMembership.last_read_message_id = messageId; diff --git a/plugins/chat/assets/javascripts/discourse/services/chat-api.js b/plugins/chat/assets/javascripts/discourse/services/chat-api.js index 2f7f41f4a22..a04049895d9 100644 --- a/plugins/chat/assets/javascripts/discourse/services/chat-api.js +++ b/plugins/chat/assets/javascripts/discourse/services/chat-api.js @@ -286,6 +286,28 @@ export default class ChatApi extends Service { ); } + /** + * Marks messages for all of a user's chat channel memberships as read. + * + * @returns {Promise} + */ + markAllChannelsAsRead() { + return this.#putRequest(`/channels/read`); + } + + /** + * Marks messages for a single user chat channel membership as read. If no + * message ID is provided, then the latest message for the channel is fetched + * on the server and used for the last read message. + * + * @param {number} channelId - The ID of the channel for the message being marked as read. + * @param {number} [messageId] - The ID of the message being marked as read. + * @returns {Promise} + */ + markChannelAsRead(channelId, messageId = null) { + return this.#putRequest(`/channels/${channelId}/read/${messageId}`); + } + get #basePath() { return "/chat/api"; } diff --git a/plugins/chat/assets/javascripts/discourse/services/chat-channels-manager.js b/plugins/chat/assets/javascripts/discourse/services/chat-channels-manager.js index 4a9e26dd698..61d7c0033c5 100644 --- a/plugins/chat/assets/javascripts/discourse/services/chat-channels-manager.js +++ b/plugins/chat/assets/javascripts/discourse/services/chat-channels-manager.js @@ -1,4 +1,5 @@ import Service, { inject as service } from "@ember/service"; +import { debounce } from "discourse-common/utils/decorators"; import Promise from "rsvp"; import ChatChannel from "discourse/plugins/chat/discourse/models/chat-channel"; import { tracked } from "@glimmer/tracking"; @@ -82,6 +83,19 @@ export default class ChatChannelsManager extends Service { }); } + @debounce(300) + async markAllChannelsRead() { + return this.chatApi.markAllChannelsAsRead().then((response) => { + response.updated_memberships.forEach((membership) => { + let channel = this.channels.findBy("id", membership.channel_id); + if (channel) { + channel.currentUserMembership.unread_count = 0; + channel.currentUserMembership.unread_mentions = 0; + } + }); + }); + } + remove(model) { this.chatSubscriptionsManager.stopChannelSubscription(model); delete this._cached[model.id]; diff --git a/plugins/chat/config/locales/client.en.yml b/plugins/chat/config/locales/client.en.yml index 3e62bd650c5..657353d8405 100644 --- a/plugins/chat/config/locales/client.en.yml +++ b/plugins/chat/config/locales/client.en.yml @@ -600,6 +600,7 @@ en: composer_code: "%{shortcut} Code (composer only)" drawer_open: "%{shortcut} Open chat drawer" drawer_close: "%{shortcut} Close chat drawer" + mark_all_channels_read: "%{shortcut} Mark all channels read" topic_statuses: chat: help: "Chat is enabled for this topic" diff --git a/plugins/chat/config/routes.rb b/plugins/chat/config/routes.rb index 0c66fac78eb..fbf08fd86cb 100644 --- a/plugins/chat/config/routes.rb +++ b/plugins/chat/config/routes.rb @@ -6,6 +6,8 @@ Chat::Engine.routes.draw do get "/channels" => "channels#index" get "/channels/me" => "current_user_channels#index" post "/channels" => "channels#create" + put "/channels/read/" => "reads#update_all" + put "/channels/:channel_id/read/:message_id" => "reads#update" delete "/channels/:channel_id" => "channels#destroy" put "/channels/:channel_id" => "channels#update" get "/channels/:channel_id" => "channels#show" diff --git a/plugins/chat/lib/service_runner.rb b/plugins/chat/lib/service_runner.rb index b82ff5c9dcf..62db6ed2fdb 100644 --- a/plugins/chat/lib/service_runner.rb +++ b/plugins/chat/lib/service_runner.rb @@ -19,6 +19,8 @@ # * +on_model_not_found(name)+: will execute the provided block if the service # fails and its model is not present # +# Default actions for each of these are defined in [Chat::ApiController#default_actions_for_service] +# # @example In a controller # def create # with_service MyService do diff --git a/plugins/chat/spec/requests/chat/api/reads_controller_spec.rb b/plugins/chat/spec/requests/chat/api/reads_controller_spec.rb new file mode 100644 index 00000000000..11a6a37f5cc --- /dev/null +++ b/plugins/chat/spec/requests/chat/api/reads_controller_spec.rb @@ -0,0 +1,199 @@ +# frozen_string_literal: true + +RSpec.describe Chat::Api::ReadsController 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 "#read" do + describe "marking a single message read" do + fab!(:chat_channel) { Fabricate(:chat_channel) } + fab!(:other_user) { Fabricate(:user) } + fab!(:message_1) { Fabricate(:chat_message, chat_channel: chat_channel, user: other_user) } + fab!(:message_2) { Fabricate(:chat_message, chat_channel: chat_channel, user: other_user) } + + it "returns a 404 when the user is not a channel member" do + put "/chat/api/channels/#{chat_channel.id}/read/#{message_1.id}.json" + expect(response.status).to eq(404) + end + + it "returns a 404 when the user is not following the channel" do + Fabricate( + :user_chat_channel_membership, + chat_channel: chat_channel, + user: current_user, + following: false, + ) + + put "/chat/api/channels/#{chat_channel.id}/read/#{message_1.id}.json" + expect(response.status).to eq(404) + end + + describe "when the user is a channel member" do + fab!(:membership) do + Fabricate(:user_chat_channel_membership, chat_channel: chat_channel, user: current_user) + end + + context "when message_id param doesn't link to a message of the channel" do + it "raises a not found" do + put "/chat/api/channels/#{chat_channel.id}/read/-999.json" + expect(response.status).to eq(404) + end + end + + context "when message_id param is inferior to existing last read" do + before { membership.update!(last_read_message_id: message_2.id) } + + it "raises an invalid request" do + put "/chat/api/channels/#{chat_channel.id}/read/#{message_1.id}.json" + expect(response.status).to eq(400) + expect(response.parsed_body["errors"][0]).to match(/message_id/) + end + end + + context "when message_id refers to deleted message" do + before { message_1.trash!(Discourse.system_user) } + + it "works" do + put "/chat/api/channels/#{chat_channel.id}/read/#{message_1.id}" + expect(response.status).to eq(200) + end + end + + it "updates timing records" do + expect { + put "/chat/api/channels/#{chat_channel.id}/read/#{message_1.id}.json" + }.not_to change { Chat::UserChatChannelMembership.count } + + membership.reload + expect(membership.chat_channel_id).to eq(chat_channel.id) + expect(membership.last_read_message_id).to eq(message_1.id) + expect(membership.user_id).to eq(current_user.id) + end + + it "marks all mention notifications as read for the channel" do + notification = create_notification_and_mention_for(current_user, other_user, message_1) + + put "/chat/api/channels/#{chat_channel.id}/read/#{message_2.id}.json" + expect(response.status).to eq(200) + expect(notification.reload.read).to eq(true) + end + + it "doesn't mark notifications of messages that weren't read yet" do + message_3 = Fabricate(:chat_message, chat_channel: chat_channel, user: other_user) + notification = create_notification_and_mention_for(current_user, other_user, message_3) + + put "/chat/api/channels/#{chat_channel.id}/read/#{message_2.id}.json" + expect(response.status).to eq(200) + expect(notification.reload.read).to eq(false) + end + end + end + + describe "marking all messages read" do + fab!(:chat_channel_1) { Fabricate(:chat_channel) } + fab!(:chat_channel_2) { Fabricate(:chat_channel) } + fab!(:chat_channel_3) { Fabricate(:chat_channel) } + + fab!(:other_user) { Fabricate(:user) } + + fab!(:message_1) { Fabricate(:chat_message, chat_channel: chat_channel_1, user: other_user) } + fab!(:message_2) { Fabricate(:chat_message, chat_channel: chat_channel_1, user: other_user) } + fab!(:message_3) { Fabricate(:chat_message, chat_channel: chat_channel_2, user: other_user) } + fab!(:message_4) { Fabricate(:chat_message, chat_channel: chat_channel_2, user: other_user) } + fab!(:message_5) { Fabricate(:chat_message, chat_channel: chat_channel_3, user: other_user) } + fab!(:message_6) { Fabricate(:chat_message, chat_channel: chat_channel_3, user: other_user) } + + fab!(:membership_1) do + Fabricate( + :user_chat_channel_membership, + chat_channel: chat_channel_1, + user: current_user, + following: true, + ) + end + fab!(:membership_2) do + Fabricate( + :user_chat_channel_membership, + chat_channel: chat_channel_2, + user: current_user, + following: true, + ) + end + fab!(:membership_3) do + Fabricate( + :user_chat_channel_membership, + chat_channel: chat_channel_3, + user: current_user, + following: true, + ) + end + + it "marks all messages as read across the user's channel memberships with the correct last_read_message_id" do + put "/chat/api/channels/read.json" + + expect(membership_1.reload.last_read_message_id).to eq(message_2.id) + expect(membership_2.reload.last_read_message_id).to eq(message_4.id) + expect(membership_3.reload.last_read_message_id).to eq(message_6.id) + end + + it "doesn't mark messages for channels the user is not following as read" do + membership_1.update!(following: false) + + put "/chat/api/channels/read.json" + + expect(membership_1.reload.last_read_message_id).to eq(nil) + expect(membership_2.reload.last_read_message_id).to eq(message_4.id) + expect(membership_3.reload.last_read_message_id).to eq(message_6.id) + end + + it "returns the updated memberships, channels, and last message id" do + put "/chat/api/channels/read.json" + expect(response.parsed_body["updated_memberships"]).to match_array( + [ + { + "channel_id" => chat_channel_1.id, + "last_read_message_id" => message_2.id, + "membership_id" => membership_1.id, + }, + { + "channel_id" => chat_channel_2.id, + "last_read_message_id" => message_4.id, + "membership_id" => membership_2.id, + }, + { + "channel_id" => chat_channel_3.id, + "last_read_message_id" => message_6.id, + "membership_id" => membership_3.id, + }, + ], + ) + 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/requests/chat_controller_spec.rb b/plugins/chat/spec/requests/chat_controller_spec.rb index 2ba84515790..34eb2d7c75f 100644 --- a/plugins/chat/spec/requests/chat_controller_spec.rb +++ b/plugins/chat/spec/requests/chat_controller_spec.rb @@ -725,117 +725,6 @@ RSpec.describe Chat::ChatController do end end - describe "#update_user_last_read" do - before { sign_in(user) } - - fab!(:message_1) { Fabricate(:chat_message, chat_channel: chat_channel, user: other_user) } - fab!(:message_2) { Fabricate(:chat_message, chat_channel: chat_channel, user: other_user) } - - it "returns a 404 when the user is not a channel member" do - put "/chat/#{chat_channel.id}/read/#{message_1.id}.json" - - expect(response.status).to eq(404) - end - - it "returns a 404 when the user is not following the channel" do - Fabricate( - :user_chat_channel_membership, - chat_channel: chat_channel, - user: user, - following: false, - ) - - put "/chat/#{chat_channel.id}/read/#{message_1.id}.json" - - expect(response.status).to eq(404) - end - - describe "when the user is a channel member" do - fab!(:membership) do - Fabricate(:user_chat_channel_membership, chat_channel: chat_channel, user: user) - end - - context "when message_id param doesn't link to a message of the channel" do - it "raises a not found" do - put "/chat/#{chat_channel.id}/read/-999.json" - - expect(response.status).to eq(404) - end - end - - context "when message_id param is inferior to existing last read" do - before { membership.update!(last_read_message_id: message_2.id) } - - it "raises an invalid request" do - put "/chat/#{chat_channel.id}/read/#{message_1.id}.json" - - expect(response.status).to eq(400) - expect(response.parsed_body["errors"][0]).to match(/message_id/) - end - end - - context "when message_id refers to deleted message" do - before { message_1.trash!(Discourse.system_user) } - - it "works" do - put "/chat/#{chat_channel.id}/read/#{message_1.id}.json" - - expect(response.status).to eq(200) - end - end - - it "updates timing records" do - expect { put "/chat/#{chat_channel.id}/read/#{message_1.id}.json" }.not_to change { - Chat::UserChatChannelMembership.count - } - - membership.reload - expect(membership.chat_channel_id).to eq(chat_channel.id) - expect(membership.last_read_message_id).to eq(message_1.id) - expect(membership.user_id).to eq(user.id) - 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 - - it "marks all mention notifications as read for the channel" do - notification = create_notification_and_mention_for(user, other_user, message_1) - - put "/chat/#{chat_channel.id}/read/#{message_2.id}.json" - expect(response.status).to eq(200) - expect(notification.reload.read).to eq(true) - end - - it "doesn't mark notifications of messages that weren't read yet" do - message_3 = Fabricate(:chat_message, chat_channel: chat_channel, user: other_user) - notification = create_notification_and_mention_for(user, other_user, message_3) - - put "/chat/#{chat_channel.id}/read/#{message_2.id}.json" - - expect(response.status).to eq(200) - expect(notification.reload.read).to eq(false) - end - end - end - describe "react" do fab!(:chat_channel) { Fabricate(:category_channel) } fab!(:chat_message) { Fabricate(:chat_message, chat_channel: chat_channel, user: user) } diff --git a/plugins/chat/spec/services/auto_remove/handle_destroyed_group_spec.rb b/plugins/chat/spec/services/auto_remove/handle_destroyed_group_spec.rb index 1999747a867..6e920fab63c 100644 --- a/plugins/chat/spec/services/auto_remove/handle_destroyed_group_spec.rb +++ b/plugins/chat/spec/services/auto_remove/handle_destroyed_group_spec.rb @@ -35,8 +35,6 @@ RSpec.describe Chat::AutoRemove::HandleDestroyedGroup do describe "step remove_users_outside_allowed_groups" do context "when chat_allowed_groups is empty" do - let(:action) { UserHistory.where(custom_type: "chat_auto_remove_membership").last } - before do SiteSetting.chat_allowed_groups = "" channel_1.add(user_1) @@ -104,11 +102,22 @@ RSpec.describe Chat::AutoRemove::HandleDestroyedGroup do it "logs a staff action" do result - expect(action).to have_attributes( - details: "users_removed: 2\nchannel_id: #{channel_2.id}\nevent: destroyed_group", - acting_user_id: Discourse.system_user.id, - custom_type: "chat_auto_remove_membership", - ) + actions = UserHistory.where(custom_type: "chat_auto_remove_membership") + expect(actions.count).to eq(2) + expect( + actions.exists?( + details: "users_removed: 2\nchannel_id: #{channel_2.id}\nevent: destroyed_group", + acting_user_id: Discourse.system_user.id, + custom_type: "chat_auto_remove_membership", + ), + ).to eq(true) + expect( + actions.exists?( + details: "users_removed: 2\nchannel_id: #{channel_1.id}\nevent: destroyed_group", + acting_user_id: Discourse.system_user.id, + custom_type: "chat_auto_remove_membership", + ), + ).to eq(true) end end diff --git a/plugins/chat/spec/services/chat/mark_all_user_channels_read_spec.rb b/plugins/chat/spec/services/chat/mark_all_user_channels_read_spec.rb new file mode 100644 index 00000000000..31adc20f294 --- /dev/null +++ b/plugins/chat/spec/services/chat/mark_all_user_channels_read_spec.rb @@ -0,0 +1,133 @@ +# frozen_string_literal: true + +RSpec.describe Chat::MarkAllUserChannelsRead do + describe ".call" do + subject(:result) { described_class.call(params) } + + let(:params) { { guardian: guardian } } + let(:guardian) { Guardian.new(current_user) } + + fab!(:current_user) { Fabricate(:user) } + + fab!(:channel_1) { Fabricate(:chat_channel) } + fab!(:channel_2) { Fabricate(:chat_channel) } + fab!(:channel_3) { Fabricate(:chat_channel) } + + fab!(:other_user) { Fabricate(:user) } + + fab!(:message_1) { Fabricate(:chat_message, chat_channel: channel_1, user: other_user) } + fab!(:message_2) { Fabricate(:chat_message, chat_channel: channel_1, user: other_user) } + fab!(:message_3) { Fabricate(:chat_message, chat_channel: channel_2, user: other_user) } + fab!(:message_4) { Fabricate(:chat_message, chat_channel: channel_2, user: other_user) } + fab!(:message_5) { Fabricate(:chat_message, chat_channel: channel_3, user: other_user) } + fab!(:message_6) { Fabricate(:chat_message, chat_channel: channel_3, user: other_user) } + + fab!(:membership_1) do + Fabricate( + :user_chat_channel_membership, + chat_channel: channel_1, + user: current_user, + following: true, + ) + end + fab!(:membership_2) do + Fabricate( + :user_chat_channel_membership, + chat_channel: channel_2, + user: current_user, + following: true, + ) + end + fab!(:membership_3) do + Fabricate( + :user_chat_channel_membership, + chat_channel: channel_3, + user: current_user, + following: true, + ) + end + + context "when the user has no memberships" do + let(:guardian) { Guardian.new(Fabricate(:user)) } + + it "sets the service result as successful" do + expect(result).to be_a_success + end + + it "returns the updated_memberships in context" do + expect(result.updated_memberships).to eq([]) + end + 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 + Chat::Mention.create!( + notification: notification_1, + user: current_user, + chat_message: message_1, + ) + Chat::Mention.create!( + notification: notification_2, + user: current_user, + chat_message: message_3, + ) + end + + it "sets the service result as successful" do + expect(result).to be_a_success + end + + it "updates the last_read_message_ids" do + result + expect(membership_1.reload.last_read_message_id).to eq(message_2.id) + expect(membership_2.reload.last_read_message_id).to eq(message_4.id) + expect(membership_3.reload.last_read_message_id).to eq(message_6.id) + end + + it "does not update memberships where the user is not following" do + membership_1.update!(following: false) + result + expect(membership_1.reload.last_read_message_id).to eq(nil) + end + + it "does not use deleted messages for the last_read_message_id" do + message_2.trash! + result + expect(membership_1.reload.last_read_message_id).to eq(message_1.id) + end + + it "returns the updated_memberships in context" do + expect(result.updated_memberships.map(&:channel_id)).to match_array( + [channel_1.id, channel_2.id, channel_3.id], + ) + end + + it "marks existing notifications for all affected channels as read" do + expect { result }.to change { + Notification.where( + notification_type: Notification.types[:chat_mention], + user: current_user, + read: false, + ).count + }.by(-2) + end + end + end +end diff --git a/plugins/chat/spec/services/chat/update_user_last_read_spec.rb b/plugins/chat/spec/services/chat/update_user_last_read_spec.rb index 87051de62ac..06b2c218af1 100644 --- a/plugins/chat/spec/services/chat/update_user_last_read_spec.rb +++ b/plugins/chat/spec/services/chat/update_user_last_read_spec.rb @@ -2,7 +2,6 @@ RSpec.describe Chat::UpdateUserLastRead do describe Chat::UpdateUserLastRead::Contract, type: :model do - it { is_expected.to validate_presence_of :user_id } it { is_expected.to validate_presence_of :channel_id } it { is_expected.to validate_presence_of :message_id } end @@ -18,17 +17,10 @@ RSpec.describe Chat::UpdateUserLastRead do fab!(:message_1) { Fabricate(:chat_message, chat_channel: membership.chat_channel) } let(:guardian) { Guardian.new(current_user) } - let(:params) do - { - guardian: guardian, - user_id: current_user.id, - channel_id: channel.id, - message_id: message_1.id, - } - end + let(:params) { { guardian: guardian, channel_id: channel.id, message_id: message_1.id } } context "when params are not valid" do - before { params.delete(:user_id) } + before { params.delete(:message_id) } it { is_expected.to fail_a_contract } end @@ -37,7 +29,7 @@ RSpec.describe Chat::UpdateUserLastRead do context "when user has no membership" do before { membership.destroy! } - it { is_expected.to fail_to_find_a_model(:membership) } + it { is_expected.to fail_to_find_a_model(:active_membership) } end context "when user can’t access the channel" do @@ -56,8 +48,10 @@ RSpec.describe Chat::UpdateUserLastRead do context "when message_id is older than membership's last_read_message_id" do before do - params[:message_id] = -2 - membership.update!(last_read_message_id: -1) + message_old = Fabricate(:chat_message, chat_channel: channel) + message_new = Fabricate(:chat_message, chat_channel: channel) + params[:message_id] = message_old.id + membership.update!(last_read_message_id: message_new.id) end it { is_expected.to fail_a_policy(:ensure_message_id_recency) } diff --git a/plugins/chat/spec/system/shortcuts/mark_all_read_spec.rb b/plugins/chat/spec/system/shortcuts/mark_all_read_spec.rb new file mode 100644 index 00000000000..760525d11c0 --- /dev/null +++ b/plugins/chat/spec/system/shortcuts/mark_all_read_spec.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +RSpec.describe "Shortcuts | mark all read", type: :system, js: true do + fab!(:user_1) { Fabricate(:admin) } + fab!(:channel_1) { Fabricate(:chat_channel) } + fab!(:channel_2) { Fabricate(:chat_channel) } + fab!(:channel_3) { Fabricate(:chat_channel) } + + let(:chat_page) { PageObjects::Pages::Chat.new } + let(:drawer) { PageObjects::Pages::ChatDrawer.new } + + before do + SiteSetting.navigation_menu = "sidebar" + chat_system_bootstrap(user_1, [channel_1, channel_2, channel_3]) + sign_in(user_1) + Fabricate(:chat_message, chat_channel: channel_1) + Fabricate(:chat_message, chat_channel: channel_1) + Fabricate(:chat_message, chat_channel: channel_2) + Fabricate(:chat_message, chat_channel: channel_2) + end + + context "when chat is open" do + before { visit(channel_3.url) } + + context "when pressing shift+esc" do + it "marks all channels read" do + expect(page).to have_css( + ".sidebar-section-link.channel-#{channel_1.id} .sidebar-section-link-suffix.unread", + ) + expect(page).to have_css( + ".sidebar-section-link.channel-#{channel_2.id} .sidebar-section-link-suffix.unread", + ) + find("body").send_keys(%i[shift escape]) + expect(page).not_to have_css( + ".sidebar-section-link.channel-#{channel_1.id} .sidebar-section-link-suffix.unread", + ) + expect(page).not_to have_css( + ".sidebar-section-link.channel-#{channel_2.id} .sidebar-section-link-suffix.unread", + ) + end + end + end +end