diff --git a/plugins/chat/app/controllers/chat/api/tracking_controller.rb b/plugins/chat/app/controllers/chat/api/tracking_controller.rb new file mode 100644 index 0000000000..1ae06665fe --- /dev/null +++ b/plugins/chat/app/controllers/chat/api/tracking_controller.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +class Chat::Api::TrackingController < Chat::ApiController + def read + params.permit(:channel_id, :message_id) + + channel_id = params[:channel_id] + message_id = params[:message_id] + + if channel_id.present? && message_id.present? + mark_single_message_read(channel_id, message_id) + else + mark_all_messages_read + end + end + + private + + def mark_single_message_read(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 mark_all_messages_read + 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 fcb918f1e4..8a797bd53a 100644 --- a/plugins/chat/app/controllers/chat/chat_controller.rb +++ b/plugins/chat/app/controllers/chat/chat_controller.rb @@ -160,47 +160,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 c8e820cc2c..e9c3191cf8 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/mark_all_user_channels_read.rb b/plugins/chat/app/services/chat/mark_all_user_channels_read.rb new file mode 100644 index 0000000000..3327821ef7 --- /dev/null +++ b/plugins/chat/app/services/chat/mark_all_user_channels_read.rb @@ -0,0 +1,59 @@ +# 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 [Chat::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? + + Notification + .where(notification_type: ::Notification.types[:chat_mention]) + .where(user: guardian.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 (?)", updated_memberships.map(&:channel_id)) + .update_all(read: true) + 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 0eb01159b2..3be52cdd98 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] + # @return [Chat::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,52 @@ 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 invalid_access(guardian:, membership:, **) - guardian.can_join_chat_channel?(membership.chat_channel) + def fetch_active_membership(guardian:, channel:, **) + ::Chat::ChannelMembershipManager.new(channel).find_for_user(guardian.user, following: true) end - def ensure_message_id_recency(message_id:, membership:, **) - !membership.last_read_message_id || message_id >= membership.last_read_message_id + def invalid_access(guardian:, active_membership:, **) + guardian.can_join_chat_channel?(active_membership.chat_channel) end - def ensure_message_exists(channel_id:, message_id:, **) - Chat::Message.with_deleted.exists?(chat_channel_id: channel_id, id: message_id) + def ensure_message_exists(channel:, contract:, **) + ::Chat::Message.with_deleted.exists?(chat_channel_id: channel.id, id: contract.message_id) end - def update_last_read_message_id(message_id:, membership:, **) - membership.update!(last_read_message_id: message_id) + 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 mark_associated_mentions_as_read(membership:, message_id:, **) - Notification + 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:, **) + ::Notification .where(notification_type: Notification.types[:chat_mention]) - .where(user: membership.user) + .where(user: active_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) + .where("chat_messages.id <= ?", contract.message_id) + .where("chat_messages.chat_channel_id = ?", active_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 ae70c3e454..f9d564fcf9 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 e1eb2b5840..1dec46d201 100644 --- a/plugins/chat/assets/javascripts/discourse/models/chat-channel.js +++ b/plugins/chat/assets/javascripts/discourse/models/chat-channel.js @@ -192,8 +192,11 @@ 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/tracking/read/me`, { method: "PUT", + data: { message_id: messageId, channel_id: this.id }, }).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 44ad75dc5c..5227a0c2e4 100644 --- a/plugins/chat/assets/javascripts/discourse/services/chat-api.js +++ b/plugins/chat/assets/javascripts/discourse/services/chat-api.js @@ -282,6 +282,22 @@ export default class ChatApi extends Service { ); } + /** + * Marks messages for a user's chat channel memberships as read. If no + * channel ID and no message ID are provided, then all of the user's + * followed chat channel memberships will be marked as read. + * + * @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} + */ + updateCurrentUserTracking({ channelId, messageId } = {}) { + return this.#putRequest(`/tracking/read/me`, { + channel_id: channelId, + message_id: 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 0905f46802..08da50452f 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.updateCurrentUserTracking().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; + } + }); + }); + } + get unreadCount() { let count = 0; this.publicMessageChannels.forEach((channel) => { diff --git a/plugins/chat/config/locales/client.en.yml b/plugins/chat/config/locales/client.en.yml index 5925fe1e6f..b9b5f112aa 100644 --- a/plugins/chat/config/locales/client.en.yml +++ b/plugins/chat/config/locales/client.en.yml @@ -598,6 +598,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 0c66fac78e..a51a5da3dc 100644 --- a/plugins/chat/config/routes.rb +++ b/plugins/chat/config/routes.rb @@ -17,6 +17,7 @@ Chat::Engine.routes.draw do post "/channels/:channel_id/memberships/me" => "channels_current_user_membership#create" put "/channels/:channel_id/notifications-settings/me" => "channels_current_user_notifications_settings#update" + put "/tracking/read/me" => "tracking#read" # Category chatables controller hints. Only used by staff members, we don't want to leak category permissions. get "/category-chatables/:id/permissions" => "category_chatables#permissions", diff --git a/plugins/chat/lib/service_runner.rb b/plugins/chat/lib/service_runner.rb index b82ff5c9dc..62db6ed2fd 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/plugin.rb b/plugins/chat/plugin.rb index 1f2f5b81d9..b83ad11bb7 100644 --- a/plugins/chat/plugin.rb +++ b/plugins/chat/plugin.rb @@ -381,25 +381,6 @@ after_initialize do end end - Discourse::Application.routes.append do - mount ::Chat::Engine, at: "/chat" - - get "/admin/plugins/chat" => "chat/admin/incoming_webhooks#index", - :constraints => StaffConstraint.new - post "/admin/plugins/chat/hooks" => "chat/admin/incoming_webhooks#create", - :constraints => StaffConstraint.new - put "/admin/plugins/chat/hooks/:incoming_chat_webhook_id" => - "chat/admin/incoming_webhooks#update", - :constraints => StaffConstraint.new - delete "/admin/plugins/chat/hooks/:incoming_chat_webhook_id" => - "chat/admin/incoming_webhooks#destroy", - :constraints => StaffConstraint.new - get "u/:username/preferences/chat" => "users#preferences", - :constraints => { - username: RouteFormat.username, - } - end - if defined?(DiscourseAutomation) add_automation_scriptable("send_chat_message") do field :chat_channel_id, component: :text, required: true diff --git a/plugins/chat/spec/requests/chat/api/tracking_controller_spec.rb b/plugins/chat/spec/requests/chat/api/tracking_controller_spec.rb new file mode 100644 index 0000000000..be40bdc5c5 --- /dev/null +++ b/plugins/chat/spec/requests/chat/api/tracking_controller_spec.rb @@ -0,0 +1,236 @@ +# frozen_string_literal: true + +RSpec.describe Chat::Api::TrackingController 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/tracking/read/me.json", + params: { + channel_id: chat_channel.id, + message_id: message_1.id, + } + + 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/tracking/read/me.json", + params: { + channel_id: chat_channel.id, + message_id: message_1.id, + } + + 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/tracking/read/me.json", + params: { + channel_id: chat_channel.id, + message_id: -999, + } + + 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/tracking/read/me.json", + params: { + channel_id: chat_channel.id, + message_id: message_1.id, + } + + 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/tracking/read/me", + params: { + channel_id: chat_channel.id, + message_id: message_1.id, + } + + expect(response.status).to eq(200) + end + end + + it "updates timing records" do + expect { + put "/chat/api/tracking/read/me.json", + params: { + channel_id: chat_channel.id, + message_id: message_1.id, + } + }.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/tracking/read/me.json", + params: { + channel_id: chat_channel.id, + message_id: message_2.id, + } + 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/tracking/read/me.json", + params: { + channel_id: chat_channel.id, + message_id: message_2.id, + } + 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/tracking/read/me.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/tracking/read/me.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/tracking/read/me.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 2ba8451579..34eb2d7c75 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/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 0000000000..31adc20f29 --- /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 87051de62a..8cd9e44b71 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 @@ -18,17 +18,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 +30,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 +49,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 0000000000..760525d11c --- /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