From 9889547475e6f06e0dc58340984eefda4c0da0c1 Mon Sep 17 00:00:00 2001 From: Jan Cernik <66427541+jancernik@users.noreply.github.com> Date: Wed, 22 May 2024 08:57:00 -0300 Subject: [PATCH] FEATURE: Allow to bulk delete chat messages (#26586) --- .../chat/api/channel_messages_controller.rb | 12 + .../chat/app/services/chat/trash_message.rb | 2 +- .../chat/app/services/chat/trash_messages.rb | 97 +++++++ .../discourse/components/chat-channel.gjs | 1 + .../discourse/components/chat-thread.gjs | 5 +- .../chat/modal/delete-messages-confirm.gjs | 46 ++++ .../components/chat/selection-manager.gjs | 62 ++++- .../discourse/services/chat-api.js | 12 + plugins/chat/config/locales/client.en.yml | 5 + plugins/chat/config/routes.rb | 1 + .../spec/services/chat/trash_messages_spec.rb | 236 ++++++++++++++++++ .../chat/spec/system/deleted_message_spec.rb | 12 + .../chat/components/selection_management.rb | 6 + 13 files changed, 494 insertions(+), 3 deletions(-) create mode 100644 plugins/chat/app/services/chat/trash_messages.rb create mode 100644 plugins/chat/assets/javascripts/discourse/components/chat/modal/delete-messages-confirm.gjs create mode 100644 plugins/chat/spec/services/chat/trash_messages_spec.rb diff --git a/plugins/chat/app/controllers/chat/api/channel_messages_controller.rb b/plugins/chat/app/controllers/chat/api/channel_messages_controller.rb index 6b02983982b..42ca5c8f727 100644 --- a/plugins/chat/app/controllers/chat/api/channel_messages_controller.rb +++ b/plugins/chat/app/controllers/chat/api/channel_messages_controller.rb @@ -26,6 +26,18 @@ class Chat::Api::ChannelMessagesController < Chat::ApiController end end + def bulk_destroy + with_service(Chat::TrashMessages) do + on_success { render(json: success_json) } + on_failure { render(json: failed_json, status: 422) } + on_model_not_found(:messages) { raise Discourse::NotFound } + on_failed_policy(:invalid_access) { raise Discourse::InvalidAccess } + on_failed_contract do |contract| + render(json: failed_json.merge(errors: contract.errors.full_messages), status: 400) + end + end + end + def restore with_service(Chat::RestoreMessage) do on_success { render(json: success_json) } diff --git a/plugins/chat/app/services/chat/trash_message.rb b/plugins/chat/app/services/chat/trash_message.rb index b0d0c317f89..6bb8100bc1e 100644 --- a/plugins/chat/app/services/chat/trash_message.rb +++ b/plugins/chat/app/services/chat/trash_message.rb @@ -80,7 +80,7 @@ module Chat message.chat_channel.update_last_message_id! end - def publish_events(guardian:, message:) + def publish_events(contract:, guardian:, message:) DiscourseEvent.trigger(:chat_message_trashed, message, message.chat_channel, guardian.user) Chat::Publisher.publish_delete!(message.chat_channel, message) diff --git a/plugins/chat/app/services/chat/trash_messages.rb b/plugins/chat/app/services/chat/trash_messages.rb new file mode 100644 index 00000000000..1593752e793 --- /dev/null +++ b/plugins/chat/app/services/chat/trash_messages.rb @@ -0,0 +1,97 @@ +# frozen_string_literal: true + +module Chat + # Service responsible for trashing multiple chat messages + # for a channel and ensuring that their client and read state + # is updated. + # + # @example + # Chat::TrashMessages.call(message_ids: [2, 3], channel_id: 1, guardian: guardian) + # + class TrashMessages + include Service::Base + + # @!method call(message_ids:, channel_id:, guardian:) + # @param [Array] message_ids + # @param [Integer] channel_id + # @param [Guardian] guardian + # @return [Service::Base::Context] + + contract + model :messages + policy :can_delete_all_chat_messages + transaction do + step :trash_messages + step :destroy_notifications + step :update_last_message_ids + step :update_tracking_states + step :update_thread_reply_cache + end + step :publish_events + + # @!visibility private + class Contract + attribute :channel_id, :integer + attribute :message_ids, :array + validates :channel_id, presence: true + validates :message_ids, length: { minimum: 1, maximum: 50 } + end + + private + + def fetch_messages(contract:) + Chat::Message.includes(chat_channel: :chatable).where( + id: contract.message_ids, + chat_channel_id: contract.channel_id, + ) + end + + def can_delete_all_chat_messages(guardian:, messages:) + messages.all? { |message| guardian.can_delete_chat?(message, message.chat_channel.chatable) } + end + + def trash_messages(guardian:, messages:) + messages.each { |message| message.trash!(guardian.user) } + end + + def destroy_notifications(messages:) + Notification.where( + id: + Chat::Mention + .where(chat_message_id: messages.map(&:id)) + .joins(:notifications) + .select("notifications.id"), + ).destroy_all + end + + def update_last_message_ids(messages:) + messages.each do |message| + message.thread&.update_last_message_id! + message.chat_channel.update_last_message_id! + end + end + + def update_tracking_states(messages:) + messages.each do |message| + ::Chat::Action::ResetUserLastReadChannelMessage.call( + [message.id], + [message.chat_channel_id], + ) + if message.thread_id.present? + ::Chat::Action::ResetUserLastReadThreadMessage.call([message.id], [message.thread_id]) + end + end + end + + def update_thread_reply_cache(messages:) + messages.each { |message| message.thread&.decrement_replies_count_cache } + end + + def publish_events(contract:, guardian:, messages:) + messages.each do |message| + DiscourseEvent.trigger(:chat_message_trashed, message, message.chat_channel, guardian.user) + end + Chat::Publisher.publish_bulk_delete!(messages.first.chat_channel, contract.message_ids) + end + end +end diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-channel.gjs b/plugins/chat/assets/javascripts/discourse/components/chat-channel.gjs index 4c504f60ae1..2639a6fed5d 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-channel.gjs +++ b/plugins/chat/assets/javascripts/discourse/components/chat-channel.gjs @@ -738,6 +738,7 @@ export default class ChatChannel extends Component { @channel.canModerate }} @pane={{this.pane}} + @messagesManager={{this.messagesManager}} /> {{else}} {{#if (and (not @channel.isFollowing) @channel.isCategoryChannel)}} diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-thread.gjs b/plugins/chat/assets/javascripts/discourse/components/chat-thread.gjs index d7d17565dad..c891de45ab2 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-thread.gjs +++ b/plugins/chat/assets/javascripts/discourse/components/chat-thread.gjs @@ -573,7 +573,10 @@ export default class ChatThread extends Component { /> {{#if this.chatThreadPane.selectingMessages}} - + {{else}} + + <:body> + {{i18n + "chat.delete_messages.confirm" + count=@model.selectedMessageIds.length + }} + + <:footer> + + + + + +} diff --git a/plugins/chat/assets/javascripts/discourse/components/chat/selection-manager.gjs b/plugins/chat/assets/javascripts/discourse/components/chat/selection-manager.gjs index d39e20d1cf6..ca3b24f6ea6 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat/selection-manager.gjs +++ b/plugins/chat/assets/javascripts/discourse/components/chat/selection-manager.gjs @@ -2,21 +2,25 @@ import Component from "@glimmer/component"; import { getOwner } from "@ember/application"; import { action } from "@ember/object"; import { service } from "@ember/service"; -import { not } from "truth-helpers"; +import { not, or } from "truth-helpers"; import DButton from "discourse/components/d-button"; import { popupAjaxError } from "discourse/lib/ajax-error"; import { clipboardCopyAsync } from "discourse/lib/utilities"; import { isTesting } from "discourse-common/config/environment"; import { bind } from "discourse-common/utils/decorators"; import I18n from "discourse-i18n"; +import DeleteMessagesConfirm from "discourse/plugins/chat/discourse/components/chat/modal/delete-messages-confirm"; import ChatModalMoveMessageToChannel from "discourse/plugins/chat/discourse/components/chat/modal/move-message-to-channel"; +const DELETE_COUNT_LIMIT = 50; + export default class ChatSelectionManager extends Component { @service("composer") topicComposer; @service router; @service modal; @service site; @service toasts; + @service currentUser; @service("chat-api") api; get enableMove() { @@ -27,6 +31,40 @@ export default class ChatSelectionManager extends Component { return this.args.pane.selectedMessageIds.length > 0; } + get deleteCountLimitReached() { + return this.args.pane.selectedMessageIds.length > DELETE_COUNT_LIMIT; + } + + get canDeleteMessages() { + return this.args.pane.selectedMessageIds.every((id) => { + return this.canDeleteMessage(id); + }); + } + + canDeleteMessage(id) { + const message = this.args.messagesManager?.findMessage(id); + + if (message) { + const canDelete = + this.currentUser.id === message.user.id + ? message.channel?.canDeleteSelf + : message.channel?.canDeleteOthers; + + return ( + canDelete && + !message.deletedAt && + message.channel?.canModifyMessages?.(this.currentUser) + ); + } + } + + get deleteButtonTitle() { + return I18n.t("chat.selection.delete", { + selectionCount: this.args.pane.selectedMessageIds.length, + totalCount: DELETE_COUNT_LIMIT, + }); + } + @bind async generateQuote() { const { markdown } = await this.api.generateQuote( @@ -47,6 +85,16 @@ export default class ChatSelectionManager extends Component { }); } + @action + openDeleteMessagesModal() { + this.modal.show(DeleteMessagesConfirm, { + model: { + sourceChannel: this.args.pane.channel, + selectedMessageIds: this.args.pane.selectedMessageIds, + }, + }); + } + @action async quoteMessages() { let quoteMarkdown; @@ -140,6 +188,18 @@ export default class ChatSelectionManager extends Component { /> {{/if}} + + } messageIds - IDs of the messages to delete. + * @returns {Promise} + */ + trashMessages(channelId, messageIds) { + return this.#deleteRequest(`/channels/${channelId}/messages`, { + message_ids: messageIds, + }); + } + /** * Creates a channel archive. * @param {number} channelId - The ID of the channel. diff --git a/plugins/chat/config/locales/client.en.yml b/plugins/chat/config/locales/client.en.yml index 684f51705c6..498fdff7143 100644 --- a/plugins/chat/config/locales/client.en.yml +++ b/plugins/chat/config/locales/client.en.yml @@ -58,6 +58,10 @@ en: one: "You are moving %{count} message. Select a destination channel. A placeholder message will be created in the %{channelTitle} channel to indicate that this message has been moved. Note that reply chains will not be preserved in the new channel, and messages in the old channel will no longer show as replying to any moved messages." other: "You are moving %{count} messages. Select a destination channel. A placeholder message will be created in the %{channelTitle} channel to indicate that these messages have been moved. Note that reply chains will not be preserved in the new channel, and messages in the old channel will no longer show as replying to any moved messages." confirm_move: "Move Messages" + delete_messages: + confirm: + one: "Are you sure you want to delete this message?" + other: "Are you sure you want to delete these %{count} messages?" channel_settings: title: "Channel settings" edit: "Edit" @@ -552,6 +556,7 @@ en: cancel: "Cancel" quote_selection: "Quote in Topic" copy: "Copy" + delete: "Delete (%{selectionCount}/%{totalCount})" move_selection_to_channel: "Move to Channel" error: "There was an error moving the chat messages" title: "Move Chat to Topic" diff --git a/plugins/chat/config/routes.rb b/plugins/chat/config/routes.rb index 85ef3957390..7aa754f76c5 100644 --- a/plugins/chat/config/routes.rb +++ b/plugins/chat/config/routes.rb @@ -58,6 +58,7 @@ Chat::Engine.routes.draw do put "/channels/:channel_id/messages/:message_id/restore" => "channel_messages#restore" delete "/channels/:channel_id/messages/:message_id" => "channel_messages#destroy" + delete "/channels/:channel_id/messages" => "channel_messages#bulk_destroy" get "/channels/:channel_id/summarize" => "summaries#get_summary" end diff --git a/plugins/chat/spec/services/chat/trash_messages_spec.rb b/plugins/chat/spec/services/chat/trash_messages_spec.rb new file mode 100644 index 00000000000..034fa33a866 --- /dev/null +++ b/plugins/chat/spec/services/chat/trash_messages_spec.rb @@ -0,0 +1,236 @@ +# frozen_string_literal: true + +RSpec.describe Chat::TrashMessages do + fab!(:current_user) { Fabricate(:user) } + let!(:guardian) { Guardian.new(current_user) } + fab!(:chat_channel) { Fabricate(:chat_channel) } + fab!(:message1) { Fabricate(:chat_message, user: current_user, chat_channel: chat_channel) } + fab!(:message2) { Fabricate(:chat_message, user: current_user, chat_channel: chat_channel) } + + describe ".call" do + subject(:result) { described_class.call(params) } + + context "when params are not valid" do + let(:params) { { guardian: guardian } } + + it { is_expected.to fail_a_contract } + end + + context "when params are valid" do + let(:params) do + { guardian: guardian, message_ids: [message1.id, message2.id], channel_id: chat_channel.id } + end + + context "when the user does not have permission to delete" do + before { message1.update!(user: Fabricate(:admin)) } + + it { is_expected.to fail_a_policy(:can_delete_all_chat_messages) } + end + + context "when the channel does not match the message" do + let(:params) do + { + guardian: guardian, + message_ids: [message1.id, message2.id], + channel_id: Fabricate(:chat_channel).id, + } + end + + it { is_expected.to fail_to_find_a_model(:messages) } + end + + context "when the user has permission to delete" do + it "sets the service result as successful" do + expect(result).to be_a_success + end + + it "trashes the messages" do + result + [message1, message2].each do |message| + expect(Chat::Message.find_by(id: message.id)).to be_nil + + deleted_message = Chat::Message.unscoped.find_by(id: message.id) + expect(deleted_message.deleted_by_id).to eq(current_user.id) + expect(deleted_message.deleted_at).to be_within(1.minute).of(Time.zone.now) + end + end + + it "destroys notifications for mentions" do + mention1 = + Fabricate( + :user_chat_mention, + chat_message: message1, + notifications: [Fabricate(:notification)], + ) + mention2 = + Fabricate( + :user_chat_mention, + chat_message: message2, + notifications: [Fabricate(:notification)], + ) + + result + + [mention1, mention2].each do |mention| + mention = Chat::Mention.find_by(id: mention.id) + expect(mention).to be_present + expect(mention.notifications).to be_empty + end + end + + it "publishes associated Discourse and MessageBus events for multiple messages" do + freeze_time + messages = nil + + events = + DiscourseEvent + .track_events { messages = MessageBus.track_publish { result } } + .select { |e| e[:event_name] == :chat_message_trashed } + + [message1, message2].each do |message| + event = events.find { |e| e[:params].first.id == message.id } + expect(event).to be_present + expect(event[:params]).to eq([message, message.chat_channel, current_user]) + end + + message_data = messages.find { |m| m.channel == "/chat/#{chat_channel.id}" }.data + expect(message_data).to eq( + { + "type" => "bulk_delete", + "deleted_ids" => [message1.id, message2.id], + "deleted_at" => message1.reload.deleted_at.iso8601(3), + }, + ) + end + + it "updates the tracking to the last non-deleted channel message for users whose last_read_message_id was the trashed message" do + other_message = Fabricate(:chat_message, chat_channel: chat_channel) + membership_1 = + Fabricate( + :user_chat_channel_membership, + chat_channel: chat_channel, + last_read_message: message1, + ) + membership_2 = + Fabricate( + :user_chat_channel_membership, + chat_channel: chat_channel, + last_read_message: message2, + ) + membership_3 = + Fabricate( + :user_chat_channel_membership, + chat_channel: chat_channel, + last_read_message: other_message, + ) + result + expect(membership_1.reload.last_read_message_id).to eq(other_message.id) + expect(membership_2.reload.last_read_message_id).to eq(other_message.id) + expect(membership_3.reload.last_read_message_id).to eq(other_message.id) + end + + it "updates the tracking to nil when there are no other messages left in the channnel" do + membership_1 = + Fabricate( + :user_chat_channel_membership, + chat_channel: chat_channel, + last_read_message: message1, + ) + membership_2 = + Fabricate( + :user_chat_channel_membership, + chat_channel: chat_channel, + last_read_message: message2, + ) + result + expect(membership_1.reload.last_read_message_id).to be_nil + expect(membership_2.reload.last_read_message_id).to be_nil + end + + it "updates the channel last_message_id to the previous message in the channel" do + message3 = Fabricate(:chat_message, chat_channel: chat_channel, user: current_user) + params[:message_ids] = [message2.id, message3.id] + chat_channel.update!(last_message: message3) + result + expect(chat_channel.reload.last_message).to eq(message1) + end + + context "when the message has a thread" do + fab!(:thread) { Fabricate(:chat_thread, channel: chat_channel) } + + before do + message1.update!(thread: thread) + message2.update!(thread: thread, created_at: message1.created_at - 1.hour) + thread.update!(last_message: message1) + thread.original_message.update!(created_at: message1.created_at - 2.hours) + end + + it "decrements the thread reply count" do + thread.set_replies_count_cache(5) + result + expect(thread.replies_count_cache).to eq(3) + end + + it "updates the tracking to the last non-deleted thread message for users whose last_read_message_id was the trashed message" do + other_message = Fabricate(:chat_message, chat_channel: chat_channel, thread: thread) + membership_1 = + Fabricate(:user_chat_thread_membership, thread: thread, last_read_message: message1) + membership_2 = + Fabricate(:user_chat_thread_membership, thread: thread, last_read_message: message2) + membership_3 = + Fabricate( + :user_chat_thread_membership, + thread: thread, + last_read_message: other_message, + ) + result + expect(membership_1.reload.last_read_message_id).to eq(other_message.id) + expect(membership_2.reload.last_read_message_id).to eq(other_message.id) + expect(membership_3.reload.last_read_message_id).to eq(other_message.id) + end + + it "updates the tracking to nil when there are no other messages left in the thread" do + membership_1 = + Fabricate(:user_chat_thread_membership, thread: thread, last_read_message: message1) + membership_2 = + Fabricate(:user_chat_thread_membership, thread: thread, last_read_message: message2) + result + expect(membership_1.reload.last_read_message_id).to be_nil + expect(membership_2.reload.last_read_message_id).to be_nil + end + + it "updates the thread last_message_id to the previous message in the thread" do + next_message = + Fabricate( + :chat_message, + thread: thread, + user: current_user, + chat_channel: chat_channel, + ) + params[:message_ids] = [message2.id, next_message.id] + thread.update!(last_message: next_message) + result + expect(thread.reload.last_message).to eq(message1) + end + + context "when there are no other messages left in the thread except the original message" do + it "updates the thread last_message_id to the original message" do + expect(thread.last_message).to eq(message1) + result + expect(thread.reload.last_message).to eq(thread.original_message) + end + end + end + + context "when all messages are already deleted" do + before do + message1.trash! + message2.trash! + end + + it { is_expected.to fail_to_find_a_model(:messages) } + end + end + end + end +end diff --git a/plugins/chat/spec/system/deleted_message_spec.rb b/plugins/chat/spec/system/deleted_message_spec.rb index ffce295b9f7..4268adbc93b 100644 --- a/plugins/chat/spec/system/deleted_message_spec.rb +++ b/plugins/chat/spec/system/deleted_message_spec.rb @@ -80,6 +80,18 @@ RSpec.describe "Deleted message", type: :system do fab!(:message_5) { Fabricate(:chat_message, chat_channel: channel_1) } fab!(:message_6) { Fabricate(:chat_message, chat_channel: channel_1) } + it "allows user to bulk delete" do + chat_page.visit_channel(channel_1) + + channel_page.messages.select(message_2) + channel_page.messages.select(message_4) + channel_page.messages.select(message_6) + channel_page.selection_management.delete + click_button(I18n.t("js.delete")) + + expect(channel_page.messages).to have_deleted_messages(message_2, message_4, message_6) + end + it "groups them" do chat_page.visit_channel(channel_1) diff --git a/plugins/chat/spec/system/page_objects/chat/components/selection_management.rb b/plugins/chat/spec/system/page_objects/chat/components/selection_management.rb index 3d89b6e6696..0b9dfcd5aec 100644 --- a/plugins/chat/spec/system/page_objects/chat/components/selection_management.rb +++ b/plugins/chat/spec/system/page_objects/chat/components/selection_management.rb @@ -48,6 +48,10 @@ module PageObjects click_button("move") end + def delete + click_button("delete") + end + private def selector_for(action) @@ -60,6 +64,8 @@ module PageObjects "chat-cancel-selection-btn" when "move" "chat-move-to-channel-btn" + when "delete" + "chat-delete-btn" end end