FIX: Chat deleted last read message and tracking state issues (#21762)

#### FIX: Do not use client lastReadMessageId when fetching channel messages

We had an issue where the following happened:

1. User opened channel and saw the last message, and we set the
   lastReadMessageId on the server and the client
2. User navigated to another channel
3. Another user deleted the message in the original channel
4. The first user navigated back to the original channel before
   the MessageBus event for the deleted message arrived, and got
   a 404 error because we were sending the deleted lastReadMessageId as
   target_message_id to the channel controller.

Instead of this which is a bit flaky and is hard to cover all
the issues for, instead we can pass a fetch_from_last_read boolean
param to the channels controller, and just get the user's
last_read_message_id straight from the database to use for the
target_message_id. This gets rid of any sources of race conditions
or lack of updates from MessageBus.

#### FIX: Include missing memberships for thread tracking publish

When we publish the channel/message tracking state for a
user and that message was a thread reply the publisher
was erroring because we were not telling Chat::TrackingStateReportQuery
to return missing memberships (which have zeroed out unread counts)
as well, which is what we do for the channel tracking state here.
Also just make sure that the TrackingStateReport does not error
when passed an ID it doesn't have data for.

plugins/chat/app/controllers/chat/api/channels_controller.rb

@@ -68,15 +68,24 @@ class Chat::Api::ChannelsController < Chat::ApiController
   end
 
   def show
-    if params[:target_message_id].present? || params[:include_messages].present?
+    if params[:target_message_id].present? || params[:include_messages].present? ||
+         params[:fetch_from_last_read].present?
       with_service(
         Chat::ChannelViewBuilder,
-        **params.permit(:channel_id, :target_message_id, :thread_id, :page_size, :direction).slice(
+        **params.permit(
           :channel_id,
           :target_message_id,
           :thread_id,
           :page_size,
           :direction,
+          :fetch_from_last_read,
+        ).slice(
+          :channel_id,
+          :target_message_id,
+          :thread_id,
+          :page_size,
+          :direction,
+          :fetch_from_last_read,
         ),
       ) do
         on_success { render_serialized(result.view, Chat::ViewSerializer, root: false) }

plugins/chat/app/models/chat/tracking_state_report.rb

@@ -11,8 +11,8 @@ module Chat
       attr_accessor :unread_count, :mention_count
 
       def initialize(info)
-        @unread_count = info[:unread_count]
-        @mention_count = info[:mention_count]
+        @unread_count = info.present? ? info[:unread_count] : 0
+        @mention_count = info.present? ? info[:mention_count] : 0
       end
 
       def to_hash

plugins/chat/app/services/chat/channel_view_builder.rb

@@ -21,6 +21,7 @@ module Chat
     #   @param [Guardian] guardian
     #   @option optional_params [Integer] thread_id
     #   @option optional_params [Integer] target_message_id
+    #   @option optional_params [Boolean] fetch_from_last_read
     #   @option optional_params [Integer] page_size
     #   @option optional_params [String] direction
     #   @return [Service::Base::Context]
@@ -28,6 +29,7 @@ module Chat
     contract
     model :channel
     policy :can_view_channel
+    step :determine_target_message_id
     policy :target_message_exists
     step :determine_threads_enabled
     step :determine_include_thread_messages
@@ -47,6 +49,7 @@ module Chat
       attribute :thread_id, :integer # (optional)
       attribute :direction, :string # (optional)
       attribute :page_size, :integer # (optional)
+      attribute :fetch_from_last_read, :boolean # (optional)
 
       validates :channel_id, presence: true
       validates :direction,
@@ -66,6 +69,12 @@ module Chat
       guardian.can_preview_chat_channel?(channel)
     end
 
+    def determine_target_message_id(contract:, channel:, guardian:, **)
+      if contract.fetch_from_last_read
+        contract.target_message_id = channel.membership_for(guardian.user)&.last_read_message_id
+      end
+    end
+
     def target_message_exists(contract:, guardian:, **)
       return true if contract.target_message_id.blank?
       target_message = Chat::Message.unscoped.find_by(id: contract.target_message_id)

plugins/chat/app/services/chat/publisher.rb

@@ -300,6 +300,7 @@ module Chat
           guardian: user.guardian,
           thread_ids: [message.thread_id],
           include_threads: true,
+          include_missing_memberships: true,
         ).find_thread(message.thread_id)
       end
 

plugins/chat/assets/javascripts/discourse/components/chat-channel.js

@@ -166,10 +166,13 @@ export default class ChatLivePane extends Component {
 
     const findArgs = { pageSize: PAGE_SIZE, includeMessages: true };
     const fetchingFromLastRead = !options.fetchFromLastMessage;
+    let scrollToMessageId = null;
     if (this.requestedTargetMessageId) {
       findArgs.targetMessageId = this.requestedTargetMessageId;
+      scrollToMessageId = this.requestedTargetMessageId;
     } else if (fetchingFromLastRead) {
-      findArgs.targetMessageId =
+      findArgs.fetchFromLastRead = true;
+      scrollToMessageId =
         this.args.channel.currentUserMembership.lastReadMessageId;
     }
 
@@ -199,7 +202,7 @@ export default class ChatLivePane extends Component {
         }
 
         if (this.requestedTargetMessageId) {
-          this.scrollToMessage(findArgs["targetMessageId"], {
+          this.scrollToMessage(scrollToMessageId, {
             highlight: true,
           });
           return;
@@ -208,9 +211,9 @@ export default class ChatLivePane extends Component {
         if (
           fetchingFromLastRead &&
           messages.length &&
-          findArgs["targetMessageId"] !== messages[messages.length - 1].id
+          scrollToMessageId !== messages[messages.length - 1].id
         ) {
-          this.scrollToMessage(findArgs["targetMessageId"]);
+          this.scrollToMessage(scrollToMessageId);
           return;
         }
 

plugins/chat/assets/javascripts/discourse/services/chat-api.js

@@ -27,6 +27,8 @@ export default class ChatApi extends Service {
 
     if (data.targetMessageId) {
       args.target_message_id = data.targetMessageId;
+    } else if (data.fetchFromLastRead) {
+      args.fetch_from_last_read = true;
     } else {
       args.page_size = data.pageSize;
 

plugins/chat/spec/services/chat/channel_view_builder_spec.rb

@@ -20,11 +20,13 @@ RSpec.describe Chat::ChannelViewBuilder do
     let(:page_size) { nil }
     let(:direction) { nil }
     let(:thread_id) { nil }
+    let(:fetch_from_last_read) { nil }
     let(:params) do
       {
         guardian: guardian,
         channel_id: channel_id,
         target_message_id: target_message_id,
+        fetch_from_last_read: fetch_from_last_read,
         page_size: page_size,
         direction: direction,
         thread_id: thread_id,
@@ -194,6 +196,49 @@ RSpec.describe Chat::ChannelViewBuilder do
       it { is_expected.to fail_a_policy(:can_view_channel) }
     end
 
+    context "when fetch_from_last_read is true" do
+      let(:fetch_from_last_read) { true }
+      fab!(:message) { Fabricate(:chat_message, chat_channel: channel) }
+      fab!(:past_message_1) do
+        msg = Fabricate(:chat_message, chat_channel: channel)
+        msg.update!(created_at: message.created_at - 1.day)
+        msg
+      end
+      fab!(:past_message_2) do
+        msg = Fabricate(:chat_message, chat_channel: channel)
+        msg.update!(created_at: message.created_at - 2.days)
+        msg
+      end
+
+      context "if the user is not a member of the channel" do
+        it "does not error and still returns messages" do
+          expect(subject.view.chat_messages).to eq([past_message_2, past_message_1, message])
+        end
+      end
+
+      context "if the user is a member of the channel" do
+        fab!(:membership) do
+          Fabricate(:user_chat_channel_membership, user: current_user, chat_channel: channel)
+        end
+
+        context "if the user's last_read_message_id is not nil" do
+          before { membership.update!(last_read_message_id: past_message_1.id) }
+
+          it "uses the last_read_message_id of the user's membership as the target_message_id" do
+            expect(subject.view.chat_messages).to eq([past_message_2, past_message_1, message])
+          end
+        end
+
+        context "if the user's last_read_message_id is nil" do
+          before { membership.update!(last_read_message_id: nil) }
+
+          it "does not error and still returns messages" do
+            expect(subject.view.chat_messages).to eq([past_message_2, past_message_1, message])
+          end
+        end
+      end
+    end
+
     context "when target_message_id provided" do
       fab!(:message) { Fabricate(:chat_message, chat_channel: channel) }
       fab!(:past_message) do

plugins/chat/spec/services/chat/publisher_spec.rb

@@ -60,6 +60,66 @@ describe Chat::Publisher do
     end
   end
 
+  describe ".publish_user_tracking_state!" do
+    fab!(:channel) { Fabricate(:category_channel) }
+    fab!(:message_1) { Fabricate(:chat_message, chat_channel: channel) }
+    fab!(:user) { Fabricate(:user) }
+
+    let(:data) do
+      MessageBus
+        .track_publish { described_class.publish_user_tracking_state!(user, channel, message_1) }
+        .first
+        .data
+    end
+
+    context "when the user has channel membership" do
+      fab!(:membership) do
+        Fabricate(:user_chat_channel_membership, chat_channel: channel, user: user)
+      end
+
+      it "publishes the tracking state with correct counts" do
+        expect(data["unread_count"]).to eq(1)
+        expect(data["mention_count"]).to eq(0)
+      end
+    end
+
+    context "when the user has no channel membership" do
+      it "publishes the tracking state with zeroed out counts" do
+        expect(data["channel_id"]).to eq(channel.id)
+        expect(data["last_read_message_id"]).to eq(message_1.id)
+        expect(data["thread_id"]).to eq(nil)
+        expect(data["unread_count"]).to eq(0)
+        expect(data["mention_count"]).to eq(0)
+      end
+    end
+
+    context "when the channel has threading enabled and the message is a thread reply" do
+      fab!(:thread) { Fabricate(:chat_thread, channel: channel) }
+      before do
+        message_1.update!(thread: thread)
+        channel.update!(threading_enabled: true)
+      end
+
+      context "when the user has thread membership" do
+        fab!(:membership) { Fabricate(:user_chat_thread_membership, thread: thread, user: user) }
+
+        it "publishes the tracking state with correct counts" do
+          expect(data["thread_id"]).to eq(thread.id)
+          expect(data["unread_thread_ids"]).to eq([thread.id])
+          expect(data["thread_tracking"]).to eq({ "unread_count" => 1, "mention_count" => 0 })
+        end
+      end
+
+      context "when the user has no thread membership" do
+        it "publishes the tracking state with zeroed out counts" do
+          expect(data["thread_id"]).to eq(thread.id)
+          expect(data["unread_thread_ids"]).to eq([])
+          expect(data["thread_tracking"]).to eq({ "unread_count" => 0, "mention_count" => 0 })
+        end
+      end
+    end
+  end
+
   describe ".calculate_publish_targets" do
     context "when enable_experimental_chat_threaded_discussions is false" do
       before { SiteSetting.enable_experimental_chat_threaded_discussions = false }

plugins/chat/spec/system/deleted_message_spec.rb

@@ -47,6 +47,34 @@ RSpec.describe "Deleted message", type: :system, js: true do
       sidebar_component.click_link(channel_1.name)
       expect(channel_page).to have_deleted_message(message, count: 1)
     end
+
+    context "when the current user is not admin" do
+      fab!(:current_user) { Fabricate(:user) }
+
+      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)
+        sidebar_component.click_link(channel_2.name)
+
+        other_user = Fabricate(:admin)
+        chat_system_user_bootstrap(user: other_user, channel: channel_1)
+        using_session(:tab_2) do |session|
+          sign_in(other_user)
+          chat_page.visit_channel(channel_1)
+          channel_page.delete_message(message)
+          session.quit
+        end
+
+        sidebar_component.click_link(channel_1.name)
+        expect(channel_page).to have_no_message(id: message.id)
+      end
+    end
   end
 
   context "when deleting multiple messages" do