From c908eeacc96e947025176c1744ee4071ee252b91 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Tue, 23 May 2023 18:32:19 +0200 Subject: [PATCH] FIX: Update client lastReadMessageId on trashed message (#21697) Followup ae3231e1406d4ccf1d048ef8a8d8f744f840896b, when a message is trashed we already update the lastReadMessageId of all users in the channel to the latest non-deleted message on the server side. However we didn't propagate this to the client, so in some cases when we did the following: 1. Delete the last message in the channel 2. Switch to another channel 3. Switch back to the original We would get a 404 error from the target message ID being looked up still being the old lastReadMessageId (now deleted) for the user's channel membership. All we need to do is send the last not-deleted message ID for the channel (or thread) to all the member users. --- plugins/chat/app/models/chat/channel.rb | 10 ++++++ plugins/chat/app/models/chat/thread.rb | 11 +++++++ plugins/chat/app/services/chat/publisher.rb | 14 +++++++- ...chat-channel-pane-subscriptions-manager.js | 7 ++++ ...annel-thread-pane-subscriptions-manager.js | 4 +++ .../chat-pane-base-subscriptions-manager.js | 6 ++++ .../chat/spec/services/chat/publisher_spec.rb | 33 +++++++++++++++++++ .../spec/services/chat/trash_message_spec.rb | 1 + .../chat/spec/system/deleted_message_spec.rb | 17 ++++++++++ 9 files changed, 102 insertions(+), 1 deletion(-) diff --git a/plugins/chat/app/models/chat/channel.rb b/plugins/chat/app/models/chat/channel.rb index f002185a8a6..9749be0e56c 100644 --- a/plugins/chat/app/models/chat/channel.rb +++ b/plugins/chat/app/models/chat/channel.rb @@ -127,6 +127,16 @@ module Chat SQL end + def latest_not_deleted_message_id + DB.query_single(<<~SQL, channel_id: self.id).first + SELECT id FROM chat_messages + WHERE chat_channel_id = :channel_id + AND deleted_at IS NULL + ORDER BY created_at DESC, id DESC + LIMIT 1 + SQL + end + private def change_status(acting_user, target_status) diff --git a/plugins/chat/app/models/chat/thread.rb b/plugins/chat/app/models/chat/thread.rb index 3c245be26ce..6730dd82f30 100644 --- a/plugins/chat/app/models/chat/thread.rb +++ b/plugins/chat/app/models/chat/thread.rb @@ -52,6 +52,17 @@ module Chat original_message.rich_excerpt(max_length: EXCERPT_LENGTH) end + def latest_not_deleted_message_id + DB.query_single(<<~SQL, channel_id: self.channel_id, thread_id: self.id).first + SELECT id FROM chat_messages + WHERE chat_channel_id = :channel_id + AND thread_id = :thread_id + AND deleted_at IS NULL + ORDER BY created_at DESC, id DESC + LIMIT 1 + SQL + 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, diff --git a/plugins/chat/app/services/chat/publisher.rb b/plugins/chat/app/services/chat/publisher.rb index a1348177316..3458b034a1b 100644 --- a/plugins/chat/app/services/chat/publisher.rb +++ b/plugins/chat/app/services/chat/publisher.rb @@ -137,10 +137,22 @@ module Chat def self.publish_delete!(chat_channel, chat_message) message_bus_targets = calculate_publish_targets(chat_channel, chat_message) + latest_not_deleted_message_id = + if chat_message.thread_reply? && chat_channel.threading_enabled && + SiteSetting.enable_experimental_chat_threaded_discussions + chat_message.thread.latest_not_deleted_message_id + else + chat_channel.latest_not_deleted_message_id + end publish_to_targets!( message_bus_targets, chat_channel, - { type: "delete", deleted_id: chat_message.id, deleted_at: chat_message.deleted_at }, + { + type: "delete", + deleted_id: chat_message.id, + deleted_at: chat_message.deleted_at, + latest_not_deleted_message_id: latest_not_deleted_message_id, + }, ) 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 14d5ebc8262..cf8e85f336f 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 @@ -29,4 +29,11 @@ export default class ChatChannelPaneSubscriptionsManager extends ChatPaneBaseSub message.threadTitle = data.title; } } + + _afterDeleteMessage(targetMsg, data) { + if (this.model.currentUserMembership.lastReadMessageId === targetMsg.id) { + this.model.currentUserMembership.lastReadMessageId = + data.latest_not_deleted_message_id; + } + } } 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 c0ffc6aa17d..0a3114951e2 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 @@ -33,4 +33,8 @@ export default class ChatChannelThreadPaneSubscriptionsManager extends ChatPaneB handleThreadOriginalMessageUpdate() { return; } + + _afterDeleteMessage() { + // TODO (martin) Handle this once we have lastReadMessageId for thread memberships. + } } 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 180946c26d6..b87953d9c77 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 @@ -182,6 +182,8 @@ export default class ChatPaneBaseSubscriptionsManager extends Service { } else { this.messagesManager.removeMessage(targetMsg); } + + this._afterDeleteMessage(targetMsg, data); } handleRestoreMessage(data) { @@ -249,4 +251,8 @@ export default class ChatPaneBaseSubscriptionsManager extends Service { handleThreadOriginalMessageUpdate() { throw "not implemented"; } + + _afterDeleteMessage() { + throw "not implemented"; + } } diff --git a/plugins/chat/spec/services/chat/publisher_spec.rb b/plugins/chat/spec/services/chat/publisher_spec.rb index 6a70614974c..95b43660b41 100644 --- a/plugins/chat/spec/services/chat/publisher_spec.rb +++ b/plugins/chat/spec/services/chat/publisher_spec.rb @@ -6,6 +6,39 @@ describe Chat::Publisher do fab!(:channel) { Fabricate(:category_channel) } fab!(:message) { Fabricate(:chat_message, chat_channel: channel) } + describe ".publish_delete!" do + fab!(:message_2) { Fabricate(:chat_message, chat_channel: channel) } + before { message.trash! } + + it "publishes the correct data" do + data = MessageBus.track_publish { described_class.publish_delete!(channel, message) }[0].data + + expect(data["deleted_at"]).to eq(message.deleted_at.iso8601(3)) + expect(data["deleted_id"]).to eq(message.id) + expect(data["latest_not_deleted_message_id"]).to eq(message_2.id) + expect(data["type"]).to eq("delete") + end + + context "when the message is in a thread and the channel has threading_enabled" do + before do + SiteSetting.enable_experimental_chat_threaded_discussions = true + thread = Fabricate(:chat_thread, channel: channel) + message.update!(thread: thread) + channel.update!(threading_enabled: true) + end + + it "publishes the correct latest not deleted message id" do + data = + MessageBus.track_publish { described_class.publish_delete!(channel, message) }[0].data + + expect(data["deleted_at"]).to eq(message.deleted_at.iso8601(3)) + expect(data["deleted_id"]).to eq(message.id) + expect(data["latest_not_deleted_message_id"]).to eq(message.thread.original_message_id) + expect(data["type"]).to eq("delete") + end + end + end + describe ".publish_refresh!" do it "publishes the message" do data = MessageBus.track_publish { described_class.publish_refresh!(channel, message) }[0].data diff --git a/plugins/chat/spec/services/chat/trash_message_spec.rb b/plugins/chat/spec/services/chat/trash_message_spec.rb index 30eedb5760d..a88388aca54 100644 --- a/plugins/chat/spec/services/chat/trash_message_spec.rb +++ b/plugins/chat/spec/services/chat/trash_message_spec.rb @@ -66,6 +66,7 @@ RSpec.describe Chat::TrashMessage do "type" => "delete", "deleted_id" => message.id, "deleted_at" => message.reload.deleted_at.iso8601(3), + "latest_not_deleted_message_id" => nil, }, ) end diff --git a/plugins/chat/spec/system/deleted_message_spec.rb b/plugins/chat/spec/system/deleted_message_spec.rb index 7c10a55e08b..c32bff814ab 100644 --- a/plugins/chat/spec/system/deleted_message_spec.rb +++ b/plugins/chat/spec/system/deleted_message_spec.rb @@ -3,6 +3,7 @@ RSpec.describe "Deleted message", type: :system, js: true do let(:chat_page) { PageObjects::Pages::Chat.new } let(:channel_page) { PageObjects::Pages::ChatChannel.new } + let(:sidebar_component) { PageObjects::Components::Sidebar.new } fab!(:current_user) { Fabricate(:admin) } fab!(:channel_1) { Fabricate(:category_channel) } @@ -28,6 +29,22 @@ RSpec.describe "Deleted message", type: :system, js: true do count: 1, ) end + + it "does not error when coming back to the channel from another channel" do + message = Fabricate(:chat_message, chat_channel: channel_1) + channel_2 = Fabricate(:category_channel, name: "other channel") + channel_2.add(current_user) + channel_1 + .user_chat_channel_memberships + .find_by(user: current_user) + .update!(last_read_message_id: message.id) + chat_page.visit_channel(channel_1) + channel_page.delete_message(message) + expect(channel_page).to have_deleted_message(message, count: 1) + sidebar_component.click_link(channel_2.name) + sidebar_component.click_link(channel_1.name) + expect(channel_page).to have_deleted_message(message, count: 1) + end end context "when deleting multiple messages" do