From 1e85de36e2b80a25e656d3a0be665704086ffbf4 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Tue, 18 Apr 2023 08:28:20 +1000 Subject: [PATCH] FEATURE: Hook up chat bulk delete for threads (#21109) Followup to bd5c5c4b5f7b33a64cc12e2ba13e81767ac00edc, this commit hooks up the bulk delete events for chat messages inside the thread panel, by fanning out the deleted message IDs based on whether they belong to a thread or not. Also adds a system spec to cover this case, as previously the bulk delete event would have been broken with an incorrect `typ` rather than `type` hash key. --- plugins/chat/app/models/chat/thread.rb | 15 +++ plugins/chat/app/services/chat/publisher.rb | 26 ++++- ...chat-channel-pane-subscriptions-manager.js | 9 -- ...annel-thread-pane-subscriptions-manager.js | 5 - .../chat-pane-base-subscriptions-manager.js | 9 +- .../chat/spec/lib/chat/message_mover_spec.rb | 2 +- plugins/chat/spec/models/chat/thread_spec.rb | 103 ++++++++++++++++++ .../chat/spec/system/deleted_message_spec.rb | 45 ++++++++ .../system/page_objects/chat/chat_channel.rb | 13 ++- .../system/page_objects/chat/chat_thread.rb | 19 +++- 10 files changed, 222 insertions(+), 24 deletions(-) diff --git a/plugins/chat/app/models/chat/thread.rb b/plugins/chat/app/models/chat/thread.rb index f78fc05f02e..6b1eafedaea 100644 --- a/plugins/chat/app/models/chat/thread.rb +++ b/plugins/chat/app/models/chat/thread.rb @@ -34,6 +34,21 @@ module Chat original_message.excerpt(max_length: EXCERPT_LENGTH) end + def self.grouped_messages(thread_ids: nil, message_ids: nil, include_original_message: true) + DB.query(<<~SQL, message_ids: message_ids, thread_ids: thread_ids) + SELECT thread_id, + array_agg(chat_messages.id ORDER BY chat_messages.created_at, chat_messages.id) AS thread_message_ids, + chat_threads.original_message_id + FROM chat_messages + INNER JOIN chat_threads ON chat_threads.id = chat_messages.thread_id + WHERE thread_id IS NOT NULL + #{thread_ids ? "AND thread_id IN (:thread_ids)" : ""} + #{message_ids ? "AND chat_messages.id IN (:message_ids)" : ""} + #{include_original_message ? "" : "AND chat_messages.id != chat_threads.original_message_id"} + GROUP BY thread_id, chat_threads.original_message_id; + SQL + end + def self.ensure_consistency! update_counts end diff --git a/plugins/chat/app/services/chat/publisher.rb b/plugins/chat/app/services/chat/publisher.rb index 81e3fd4c736..6b847234dd3 100644 --- a/plugins/chat/app/services/chat/publisher.rb +++ b/plugins/chat/app/services/chat/publisher.rb @@ -167,11 +167,31 @@ module Chat end def self.publish_bulk_delete!(chat_channel, deleted_message_ids) - # TODO (martin) Handle sending this through for all the threads that - # may contain the deleted messages as well. + Chat::Thread + .grouped_messages(message_ids: deleted_message_ids) + .each do |group| + MessageBus.publish( + thread_message_bus_channel(chat_channel.id, group.thread_id), + { + type: "bulk_delete", + deleted_ids: group.thread_message_ids, + deleted_at: Time.zone.now, + }, + permissions(chat_channel), + ) + + # Don't need to publish to the main channel if the messages deleted + # were a part of the thread (except the original message ID, since + # that shows in the main channel). + deleted_message_ids = + deleted_message_ids - (group.thread_message_ids - [group.original_message_id]) + end + + return if deleted_message_ids.empty? + MessageBus.publish( root_message_bus_channel(chat_channel.id), - { typ: "bulk_delete", deleted_ids: deleted_message_ids, deleted_at: Time.zone.now }, + { type: "bulk_delete", deleted_ids: deleted_message_ids, deleted_at: Time.zone.now }, permissions(chat_channel), ) end diff --git a/plugins/chat/assets/javascripts/discourse/services/chat-channel-pane-subscriptions-manager.js b/plugins/chat/assets/javascripts/discourse/services/chat-channel-pane-subscriptions-manager.js index 30d4aec8de3..e8d1080baa5 100644 --- a/plugins/chat/assets/javascripts/discourse/services/chat-channel-pane-subscriptions-manager.js +++ b/plugins/chat/assets/javascripts/discourse/services/chat-channel-pane-subscriptions-manager.js @@ -26,15 +26,6 @@ export default class ChatChannelPaneSubscriptionsManager extends ChatPaneBaseSub return; } - handleBulkDeleteMessage(data) { - data.deleted_ids.forEach((deletedId) => { - this.handleDeleteMessage({ - deleted_id: deletedId, - deleted_at: data.deleted_at, - }); - }); - } - handleThreadCreated(data) { const message = this.messagesManager.findMessage(data.chat_message.id); if (message) { diff --git a/plugins/chat/assets/javascripts/discourse/services/chat-channel-thread-pane-subscriptions-manager.js b/plugins/chat/assets/javascripts/discourse/services/chat-channel-thread-pane-subscriptions-manager.js index a3e897b028f..bee7089edc2 100644 --- a/plugins/chat/assets/javascripts/discourse/services/chat-channel-thread-pane-subscriptions-manager.js +++ b/plugins/chat/assets/javascripts/discourse/services/chat-channel-thread-pane-subscriptions-manager.js @@ -39,11 +39,6 @@ export default class ChatChannelThreadPaneSubscriptionsManager extends ChatPaneB return; } - // TODO (martin) Hook this up correctly in Chat::Publisher for threads. - handleBulkDeleteMessage() { - return; - } - // NOTE: noop for now, later we may want to do scrolling or something like // we do in the channel pane. afterProcessedMessage() { diff --git a/plugins/chat/assets/javascripts/discourse/services/chat-pane-base-subscriptions-manager.js b/plugins/chat/assets/javascripts/discourse/services/chat-pane-base-subscriptions-manager.js index 3a09348d6a0..d02962207fb 100644 --- a/plugins/chat/assets/javascripts/discourse/services/chat-pane-base-subscriptions-manager.js +++ b/plugins/chat/assets/javascripts/discourse/services/chat-pane-base-subscriptions-manager.js @@ -177,8 +177,13 @@ export default class ChatPaneBaseSubscriptionsManager extends Service { } } - handleBulkDeleteMessage() { - throw "not implemented"; + handleBulkDeleteMessage(data) { + data.deleted_ids.forEach((deletedId) => { + this.handleDeleteMessage({ + deleted_id: deletedId, + deleted_at: data.deleted_at, + }); + }); } handleDeleteMessage(data) { diff --git a/plugins/chat/spec/lib/chat/message_mover_spec.rb b/plugins/chat/spec/lib/chat/message_mover_spec.rb index 3c3781be65e..c694b246d9d 100644 --- a/plugins/chat/spec/lib/chat/message_mover_spec.rb +++ b/plugins/chat/spec/lib/chat/message_mover_spec.rb @@ -76,7 +76,7 @@ describe Chat::MessageMover do deleted_messages = Chat::Message.with_deleted.where(id: move_message_ids).order(:id) expect(deleted_messages.count).to eq(3) expect(messages.first.channel).to eq("/chat/#{source_channel.id}") - expect(messages.first.data[:typ]).to eq("bulk_delete") + expect(messages.first.data[:type]).to eq("bulk_delete") expect(messages.first.data[:deleted_ids]).to eq(deleted_messages.map(&:id)) expect(messages.first.data[:deleted_at]).not_to eq(nil) end diff --git a/plugins/chat/spec/models/chat/thread_spec.rb b/plugins/chat/spec/models/chat/thread_spec.rb index dbed433a1c3..f48a4f04f94 100644 --- a/plugins/chat/spec/models/chat/thread_spec.rb +++ b/plugins/chat/spec/models/chat/thread_spec.rb @@ -42,4 +42,107 @@ RSpec.describe Chat::Thread do end end end + + describe ".grouped_messages" do + fab!(:channel) { Fabricate(:category_channel) } + fab!(:thread_1) { Fabricate(:chat_thread, channel: channel) } + fab!(:thread_2) { Fabricate(:chat_thread, channel: channel) } + + fab!(:message_1) { Fabricate(:chat_message, chat_channel: channel, thread: thread_1) } + fab!(:message_2) { Fabricate(:chat_message, chat_channel: channel, thread: thread_1) } + fab!(:message_3) { Fabricate(:chat_message, chat_channel: channel, thread: thread_2) } + + let(:result) { Chat::Thread.grouped_messages(**params) } + + context "when thread_ids provided" do + let(:params) { { thread_ids: [thread_1.id, thread_2.id] } } + + it "groups all the message ids in each thread by thread ID" do + expect(result.find { |res| res.thread_id == thread_1.id }.to_h).to eq( + { + thread_message_ids: [thread_1.original_message_id, message_1.id, message_2.id], + thread_id: thread_1.id, + original_message_id: thread_1.original_message_id, + }, + ) + expect(result.find { |res| res.thread_id == thread_2.id }.to_h).to eq( + { + thread_message_ids: [thread_2.original_message_id, message_3.id], + thread_id: thread_2.id, + original_message_id: thread_2.original_message_id, + }, + ) + end + + context "when include_original_message is false" do + let(:params) { { thread_ids: [thread_1.id, thread_2.id], include_original_message: false } } + + it "does not include the original message in the thread_message_ids" do + expect(result.find { |res| res.thread_id == thread_1.id }.to_h).to eq( + { + thread_message_ids: [message_1.id, message_2.id], + thread_id: thread_1.id, + original_message_id: thread_1.original_message_id, + }, + ) + end + end + end + + context "when message_ids provided" do + let(:params) do + { + message_ids: [ + thread_1.original_message_id, + thread_2.original_message_id, + message_1.id, + message_2.id, + message_3.id, + ], + } + end + + it "groups all the message ids in each thread by thread ID" do + expect(result.find { |res| res.thread_id == thread_1.id }.to_h).to eq( + { + thread_message_ids: [thread_1.original_message_id, message_1.id, message_2.id], + thread_id: thread_1.id, + original_message_id: thread_1.original_message_id, + }, + ) + expect(result.find { |res| res.thread_id == thread_2.id }.to_h).to eq( + { + thread_message_ids: [thread_2.original_message_id, message_3.id], + thread_id: thread_2.id, + original_message_id: thread_2.original_message_id, + }, + ) + end + + context "when include_original_message is false" do + let(:params) do + { + message_ids: [ + thread_1.original_message_id, + thread_2.original_message_id, + message_1.id, + message_2.id, + message_3.id, + ], + include_original_message: false, + } + end + + it "does not include the original message in the thread_message_ids" do + expect(result.find { |res| res.thread_id == thread_1.id }.to_h).to eq( + { + thread_message_ids: [message_1.id, message_2.id], + thread_id: thread_1.id, + original_message_id: thread_1.original_message_id, + }, + ) + 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 aa6d4f9237c..384c64d2c1a 100644 --- a/plugins/chat/spec/system/deleted_message_spec.rb +++ b/plugins/chat/spec/system/deleted_message_spec.rb @@ -24,4 +24,49 @@ RSpec.describe "Deleted message", type: :system, js: true do expect(page).to have_content(I18n.t("js.chat.deleted")) end end + + context "when bulk deleting messages across the channel and a thread" do + let(:side_panel) { PageObjects::Pages::ChatSidePanel.new } + let(:open_thread) { PageObjects::Pages::ChatThread.new } + + 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_1, user: other_user) } + + fab!(:thread) { Fabricate(:chat_thread, channel: channel_1) } + fab!(:message_4) do + Fabricate(:chat_message, chat_channel: channel_1, user: other_user, thread: thread) + end + fab!(:message_5) do + Fabricate(:chat_message, chat_channel: channel_1, user: other_user, thread: thread) + end + + before do + channel_1.update!(threading_enabled: true) + SiteSetting.enable_experimental_chat_threaded_discussions = true + chat_system_user_bootstrap(user: other_user, channel: channel_1) + end + + it "hides the deleted messages" do + chat_page.visit_channel(channel_1) + channel_page.message_thread_indicator(thread.original_message).click + expect(side_panel).to have_open_thread(thread) + + expect(channel_page).to have_message(id: message_1.id) + expect(channel_page).to have_message(id: message_2.id) + expect(open_thread).to have_message(thread.id, id: message_4.id) + expect(open_thread).to have_message(thread.id, id: message_5.id) + + Chat::Publisher.publish_bulk_delete!( + channel_1, + [message_1.id, message_2.id, message_4.id, message_5.id].flatten, + ) + + expect(channel_page).to have_no_message(id: message_1.id) + expect(channel_page).to have_no_message(id: message_2.id) + expect(open_thread).to have_no_message(thread.id, id: message_4.id) + expect(open_thread).to have_no_message(thread.id, id: message_5.id) + end + end end diff --git a/plugins/chat/spec/system/page_objects/chat/chat_channel.rb b/plugins/chat/spec/system/page_objects/chat/chat_channel.rb index 8d0c8db253b..f2e193ade82 100644 --- a/plugins/chat/spec/system/page_objects/chat/chat_channel.rb +++ b/plugins/chat/spec/system/page_objects/chat/chat_channel.rb @@ -157,10 +157,19 @@ module PageObjects end def has_message?(text: nil, id: nil) + check_message_presence(exists: true, text: text, id: id) + end + + def has_no_message?(text: nil, id: nil) + check_message_presence(exists: false, text: text, id: id) + end + + def check_message_presence(exists: true, text: nil, id: nil) + css_method = exists ? :has_css? : :has_no_css? if text - has_css?(".chat-message-text", text: text) + send(css_method, ".chat-message-text", text: text, wait: 5) elsif id - has_css?(".chat-message-container[data-id=\"#{id}\"]", wait: 10) + send(css_method, ".chat-message-container[data-id=\"#{id}\"]", wait: 10) end end diff --git a/plugins/chat/spec/system/page_objects/chat/chat_thread.rb b/plugins/chat/spec/system/page_objects/chat/chat_thread.rb index e9ac6dd96b9..cf857d805fb 100644 --- a/plugins/chat/spec/system/page_objects/chat/chat_thread.rb +++ b/plugins/chat/spec/system/page_objects/chat/chat_thread.rb @@ -49,10 +49,25 @@ module PageObjects end def has_message?(thread_id, text: nil, id: nil) + check_message_presence(thread_id, exists: true, text: text, id: id) + end + + def has_no_message?(thread_id, text: nil, id: nil) + check_message_presence(thread_id, exists: false, text: text, id: id) + end + + def check_message_presence(thread_id, exists: true, text: nil, id: nil) + css_method = exists ? :has_css? : :has_no_css? if text - find(thread_selector_by_id(thread_id)).has_css?(".chat-message-text", text: text, wait: 5) + find(thread_selector_by_id(thread_id)).send( + css_method, + ".chat-message-text", + text: text, + wait: 5, + ) elsif id - find(thread_selector_by_id(thread_id)).has_css?( + find(thread_selector_by_id(thread_id)).send( + css_method, ".chat-message-container[data-id=\"#{id}\"]", wait: 10, )