FIX: Update client lastReadMessageId on trashed message (#21697)

Followup ae3231e140, 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

@@ -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)

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,

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
 

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;
+    }
+  }
 }

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.
+  }
 }

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";
+  }
 }

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

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

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