FIX: Do not count thread messages for channel unreads (#21126)

We currently don't have a nice UI to show unread messages for the thread,
and it will take some time to create one. For now, this commit makes it so
new messages inside a thread do not count towards a chat channel's unread
counts, and new messages sent in a thread do not update a user's `last_read_message_id`
for a channel.

In addition, this PR refactors the `Chat::ChannelFetcher` to use the `Chat::ChannelUnreadsQuery`
query class for consistency, and made said class able to return zeroed-out records
for channels the user is not a member of.

Finally, a small bug is fixed here where if a user's `last_read_message_id` for
a channel was a thread's OM ID, then the thread OM would not show in the
main channel stream for them until another reply to the channel was posted.

plugins/chat/app/controllers/chat/chat_controller.rb

@@ -111,9 +111,11 @@ module Chat
 
       return render_json_error(chat_message_creator.error) if chat_message_creator.failed?
 
-      @user_chat_channel_membership.update!(
-        last_read_message_id: chat_message_creator.chat_message.id,
-      )
+      if !chat_message_creator.chat_message.thread_id.present?
+        @user_chat_channel_membership.update!(
+          last_read_message_id: chat_message_creator.chat_message.id,
+        )
+      end
 
       if @chat_channel.direct_message_channel?
         # If any of the channel users is ignoring, muting, or preventing DMs from
@@ -141,7 +143,13 @@ module Chat
       Chat::Publisher.publish_user_tracking_state(
         current_user,
         @chat_channel.id,
-        chat_message_creator.chat_message.id,
+        (
+          if chat_message_creator.chat_message.thread_id.present?
+            @user_chat_channel_membership.last_read_message_id
+          else
+            chat_message_creator.chat_message.id
+          end
+        ),
       )
       render json: success_json
     end
@@ -263,11 +271,8 @@ module Chat
 
       can_load_more_past = past_messages.count == PAST_MESSAGE_LIMIT
       can_load_more_future = future_messages.count == FUTURE_MESSAGE_LIMIT
-      messages = [
-        past_messages.reverse,
-        (!include_thread_messages? && @message.in_thread?) ? [] : [@message],
-        future_messages,
-      ].reduce([], :concat)
+      looked_up_message = !include_thread_messages? && @message.thread_reply? ? [] : [@message]
+      messages = [past_messages.reverse, looked_up_message, future_messages].reduce([], :concat)
       chat_view =
         Chat::View.new(
           chat_channel: @chat_channel,

plugins/chat/app/queries/chat/channel_unreads_query.rb

@@ -1,36 +1,58 @@
 # frozen_string_literal: true
 
 module Chat
+  ##
+  # Handles counting unread messages and mentions for a list of channels.
+  # This is used for unread indicators in the chat UI. By default only the
+  # channels that the user is a member of will be counted and returned in
+  # the result.
   class ChannelUnreadsQuery
-    def self.call(channel_ids:, user_id:)
+    ##
+    # @param channel_ids [Array<Integer>] The IDs of the channels to count.
+    # @param user_id [Integer] The ID of the user to count for.
+    # @param include_no_membership_channels [Boolean] Whether to include channels
+    #   that the user is not a member of. These counts will always be 0.
+    def self.call(channel_ids:, user_id:, include_no_membership_channels: false)
       sql = <<~SQL
-      SELECT (
-        SELECT COUNT(*) AS unread_count
-        FROM chat_messages
-        INNER JOIN chat_channels ON chat_channels.id = chat_messages.chat_channel_id
-        INNER JOIN user_chat_channel_memberships ON user_chat_channel_memberships.chat_channel_id = chat_channels.id
-        WHERE chat_channels.id = memberships.chat_channel_id
-        AND chat_messages.user_id != :user_id
-        AND user_chat_channel_memberships.user_id = :user_id
-        AND chat_messages.id > COALESCE(user_chat_channel_memberships.last_read_message_id, 0)
-        AND chat_messages.deleted_at IS NULL
-      ) AS unread_count,
-      (
-        SELECT COUNT(*) AS mention_count
-        FROM notifications
-        INNER JOIN user_chat_channel_memberships ON user_chat_channel_memberships.user_id = :user_id
-        WHERE NOT read
-        AND user_chat_channel_memberships.chat_channel_id = memberships.chat_channel_id
-        AND notifications.user_id = :user_id
-        AND notifications.notification_type = :notification_type
-        AND (data::json->>'chat_message_id')::bigint > COALESCE(user_chat_channel_memberships.last_read_message_id, 0)
-        AND (data::json->>'chat_channel_id')::bigint = memberships.chat_channel_id
-      ) AS mention_count,
-      memberships.chat_channel_id AS channel_id
-      FROM user_chat_channel_memberships AS memberships
-      WHERE memberships.user_id = :user_id AND memberships.chat_channel_id IN (:channel_ids)
-      GROUP BY memberships.chat_channel_id
-    SQL
+        SELECT (
+          SELECT COUNT(*) AS unread_count
+          FROM chat_messages
+          INNER JOIN chat_channels ON chat_channels.id = chat_messages.chat_channel_id
+          INNER JOIN user_chat_channel_memberships ON user_chat_channel_memberships.chat_channel_id = chat_channels.id
+          LEFT JOIN chat_threads ON chat_threads.id = chat_messages.thread_id
+          WHERE chat_channels.id = memberships.chat_channel_id
+          AND chat_messages.user_id != :user_id
+          AND user_chat_channel_memberships.user_id = :user_id
+          AND chat_messages.id > COALESCE(user_chat_channel_memberships.last_read_message_id, 0)
+          AND chat_messages.deleted_at IS NULL
+          AND (chat_messages.thread_id IS NULL OR chat_messages.id = chat_threads.original_message_id)
+        ) AS unread_count,
+        (
+          SELECT COUNT(*) AS mention_count
+          FROM notifications
+          INNER JOIN user_chat_channel_memberships ON user_chat_channel_memberships.user_id = :user_id
+          WHERE NOT read
+          AND user_chat_channel_memberships.chat_channel_id = memberships.chat_channel_id
+          AND notifications.user_id = :user_id
+          AND notifications.notification_type = :notification_type
+          AND (data::json->>'chat_message_id')::bigint > COALESCE(user_chat_channel_memberships.last_read_message_id, 0)
+          AND (data::json->>'chat_channel_id')::bigint = memberships.chat_channel_id
+        ) AS mention_count,
+        memberships.chat_channel_id AS channel_id
+        FROM user_chat_channel_memberships AS memberships
+        WHERE memberships.user_id = :user_id AND memberships.chat_channel_id IN (:channel_ids)
+        GROUP BY memberships.chat_channel_id
+      SQL
+
+      sql += <<~SQL if include_no_membership_channels
+        UNION ALL
+        SELECT 0 AS unread_count, 0 AS mention_count, chat_channels.id AS channel_id
+        FROM chat_channels
+        LEFT JOIN user_chat_channel_memberships ON user_chat_channel_memberships.chat_channel_id = chat_channels.id
+          AND user_chat_channel_memberships.user_id = :user_id
+        WHERE chat_channels.id IN (:channel_ids) AND user_chat_channel_memberships.id IS NULL
+        GROUP BY chat_channels.id
+      SQL
 
       DB.query(
         sql,

plugins/chat/app/services/chat/mark_all_user_channels_read.rb

@@ -30,7 +30,9 @@ module Chat
           (
             SELECT chat_messages.chat_channel_id, MAX(chat_messages.id) AS newest_message_id
             FROM chat_messages
+            LEFT JOIN chat_threads ON chat_threads.id = chat_messages.thread_id
             WHERE chat_messages.deleted_at IS NULL
+            AND (chat_messages.thread_id IS NULL or chat_messages.id = chat_threads.original_message_id)
             GROUP BY chat_messages.chat_channel_id
           ) AS subquery
           WHERE user_chat_channel_memberships.chat_channel_id = subquery.chat_channel_id AND

plugins/chat/assets/javascripts/discourse/services/chat-channel-thread-pane-subscriptions-manager.js

@@ -24,7 +24,8 @@ export default class ChatChannelThreadPaneSubscriptionsManager extends ChatPaneB
     );
     this.messagesManager.addMessages([message]);
 
-    // TODO (martin) All the scrolling and new message indicator shenanigans.
+    // TODO (martin) All the scrolling and new message indicator shenanigans,
+    // as well as handling marking the thread as read.
   }
 
   // NOTE: noop, there is nothing to do when a thread is created

plugins/chat/lib/chat/channel_fetcher.rb

@@ -191,43 +191,25 @@ module Chat
     def self.decorate_memberships_with_tracking_data(guardian, channels, memberships)
       unread_counts_per_channel = unread_counts(channels, guardian.user.id)
 
-      mention_notifications =
-        Notification.unread.where(
-          user_id: guardian.user.id,
-          notification_type: Notification.types[:chat_mention],
-        )
-      mention_notification_data = mention_notifications.map { |m| JSON.parse(m.data) }
-
       channels.each do |channel|
         membership = memberships.find { |m| m.chat_channel_id == channel.id }
 
         if membership
-          membership.unread_mentions =
-            mention_notification_data.count do |data|
-              data["chat_channel_id"] == channel.id &&
-                data["chat_message_id"] > (membership.last_read_message_id || 0)
-            end
+          channel_unread_counts =
+            unread_counts_per_channel.find { |uc| uc.channel_id == channel.id }
 
-          membership.unread_count = unread_counts_per_channel[channel.id] if !membership.muted
+          membership.unread_mentions = channel_unread_counts.mention_count
+          membership.unread_count = channel_unread_counts.unread_count if !membership.muted
         end
       end
     end
 
     def self.unread_counts(channels, user_id)
-      unread_counts = DB.query_array(<<~SQL, channel_ids: channels.map(&:id), user_id: user_id).to_h
-      SELECT cc.id, COUNT(*) as count
-      FROM chat_messages cm
-      JOIN chat_channels cc ON cc.id = cm.chat_channel_id
-      JOIN user_chat_channel_memberships uccm ON uccm.chat_channel_id = cc.id
-      WHERE cc.id IN (:channel_ids)
-        AND cm.user_id != :user_id
-        AND uccm.user_id = :user_id
-        AND cm.id > COALESCE(uccm.last_read_message_id, 0)
-        AND cm.deleted_at IS NULL
-      GROUP BY cc.id
-    SQL
-      unread_counts.default = 0
-      unread_counts
+      Chat::ChannelUnreadsQuery.call(
+        channel_ids: channels.map(&:id),
+        user_id: user_id,
+        include_no_membership_channels: true,
+      )
     end
 
     def self.find_with_access_check(channel_id_or_name, guardian)

plugins/chat/spec/lib/chat/channel_fetcher_spec.rb

@@ -59,14 +59,18 @@ describe Chat::ChannelFetcher do
 
         it "returns the correct count" do
           unread_counts = described_class.unread_counts([category_channel], user1)
-          expect(unread_counts[category_channel.id]).to eq(2)
+          expect(
+            unread_counts.find { |uc| uc.channel_id == category_channel.id }.unread_count,
+          ).to eq(2)
         end
       end
 
       context "with no unread messages" do
         it "returns the correct count" do
           unread_counts = described_class.unread_counts([category_channel], user1)
-          expect(unread_counts[category_channel.id]).to eq(0)
+          expect(
+            unread_counts.find { |uc| uc.channel_id == category_channel.id }.unread_count,
+          ).to eq(0)
         end
       end
 
@@ -79,7 +83,9 @@ describe Chat::ChannelFetcher do
 
         it "returns the correct count" do
           unread_counts = described_class.unread_counts([category_channel], user1)
-          expect(unread_counts[category_channel.id]).to eq(0)
+          expect(
+            unread_counts.find { |uc| uc.channel_id == category_channel.id }.unread_count,
+          ).to eq(0)
         end
       end
     end
@@ -92,7 +98,9 @@ describe Chat::ChannelFetcher do
 
         it "returns the correct count" do
           unread_counts = described_class.unread_counts([category_channel], user1)
-          expect(unread_counts[category_channel.id]).to eq(0)
+          expect(
+            unread_counts.find { |uc| uc.channel_id == category_channel.id }.unread_count,
+          ).to eq(0)
         end
       end
     end

plugins/chat/spec/queries/chat/channel_unreads_query_spec.rb

@@ -21,14 +21,35 @@ describe Chat::ChannelUnreadsQuery do
       ).to eq({ mention_count: 0, unread_count: 1, channel_id: channel_1.id })
     end
 
+    context "for unread messages in a thread" do
+      fab!(:thread_om) { Fabricate(:chat_message, chat_channel: channel_1) }
+      fab!(:thread) { Fabricate(:chat_thread, channel: channel_1, original_message: thread_om) }
+
+      it "does include the original message in the unread count" do
+        expect(
+          described_class.call(channel_ids: [channel_1.id], user_id: current_user.id).first.to_h,
+        ).to eq({ mention_count: 0, unread_count: 2, channel_id: channel_1.id })
+      end
+
+      it "does not include other thread messages in the unread count" do
+        Fabricate(:chat_message, chat_channel: channel_1, thread: thread)
+        Fabricate(:chat_message, chat_channel: channel_1, thread: thread)
+        expect(
+          described_class.call(channel_ids: [channel_1.id], user_id: current_user.id).first.to_h,
+        ).to eq({ mention_count: 0, unread_count: 2, channel_id: channel_1.id })
+      end
+    end
+
     context "for multiple channels" do
       fab!(:channel_2) { Fabricate(:category_channel) }
 
-      it "returns accurate counts" do
+      before do
         channel_2.add(current_user)
         Fabricate(:chat_message, chat_channel: channel_2)
         Fabricate(:chat_message, chat_channel: channel_2)
+      end
 
+      it "returns accurate counts" do
         expect(
           described_class.call(
             channel_ids: [channel_1.id, channel_2.id],
@@ -41,6 +62,41 @@ describe Chat::ChannelUnreadsQuery do
           ],
         )
       end
+
+      context "for channels where the user has no membership" do
+        before do
+          current_user
+            .user_chat_channel_memberships
+            .where(chat_channel_id: channel_2.id)
+            .destroy_all
+        end
+
+        it "does not return counts for the channels" do
+          expect(
+            described_class.call(
+              channel_ids: [channel_1.id, channel_2.id],
+              user_id: current_user.id,
+            ).map(&:to_h),
+          ).to match_array([{ mention_count: 0, unread_count: 1, channel_id: channel_1.id }])
+        end
+
+        context "when include_no_membership_channels is true" do
+          it "does return zeroed counts for the channels" do
+            expect(
+              described_class.call(
+                channel_ids: [channel_1.id, channel_2.id],
+                user_id: current_user.id,
+                include_no_membership_channels: true,
+              ).map(&:to_h),
+            ).to match_array(
+              [
+                { mention_count: 0, unread_count: 1, channel_id: channel_1.id },
+                { mention_count: 0, unread_count: 0, channel_id: channel_2.id },
+              ],
+            )
+          end
+        end
+      end
     end
   end
 

plugins/chat/spec/requests/chat_controller_spec.rb

@@ -347,19 +347,72 @@ RSpec.describe Chat::ChatController do
         )
       end
 
-      it "sends a message for regular user when staff-only is disabled and they are following channel" do
-        sign_in(user)
-        Chat::UserChatChannelMembership.create(
-          user: user,
-          chat_channel: chat_channel,
-          following: true,
-        )
+      context "when the regular user is following the channel" do
+        fab!(:message_1) { Fabricate(:chat_message, chat_channel: chat_channel) }
+        fab!(:membership) do
+          Chat::UserChatChannelMembership.create(
+            user: user,
+            chat_channel: chat_channel,
+            following: true,
+            last_read_message_id: message_1.id,
+          )
+        end
 
-        expect { post "/chat/#{chat_channel.id}.json", params: { message: message } }.to change {
-          Chat::Message.count
-        }.by(1)
-        expect(response.status).to eq(200)
-        expect(Chat::Message.last.message).to eq(message)
+        it "sends a message for regular user when staff-only is disabled and they are following channel" do
+          sign_in(user)
+
+          expect { post "/chat/#{chat_channel.id}.json", params: { message: message } }.to change {
+            Chat::Message.count
+          }.by(1)
+          expect(response.status).to eq(200)
+          expect(Chat::Message.last.message).to eq(message)
+        end
+
+        it "updates the last_read_message_id for the user who sent the message" do
+          sign_in(user)
+          post "/chat/#{chat_channel.id}.json", params: { message: message }
+          expect(response.status).to eq(200)
+          expect(membership.reload.last_read_message_id).to eq(Chat::Message.last.id)
+        end
+
+        it "publishes user tracking state using the new chat message as the last_read_message_id" do
+          sign_in(user)
+          messages =
+            MessageBus.track_publish(
+              Chat::Publisher.user_tracking_state_message_bus_channel(user.id),
+            ) { post "/chat/#{chat_channel.id}.json", params: { message: message } }
+          expect(response.status).to eq(200)
+          expect(messages.first.data["last_read_message_id"]).to eq(Chat::Message.last.id)
+        end
+
+        context "when sending a message in a thread" do
+          fab!(:thread) do
+            Fabricate(:chat_thread, channel: chat_channel, original_message: message_1)
+          end
+
+          it "does not update the last_read_message_id for the user who sent the message" do
+            sign_in(user)
+            post "/chat/#{chat_channel.id}.json", params: { message: message, thread_id: thread.id }
+            expect(response.status).to eq(200)
+            expect(membership.reload.last_read_message_id).to eq(message_1.id)
+          end
+
+          it "publishes user tracking state using the old membership last_read_message_id" do
+            sign_in(user)
+            messages =
+              MessageBus.track_publish(
+                Chat::Publisher.user_tracking_state_message_bus_channel(user.id),
+              ) do
+                post "/chat/#{chat_channel.id}.json",
+                     params: {
+                       message: message,
+                       thread_id: thread.id,
+                     }
+              end
+            expect(response.status).to eq(200)
+            expect(messages.first.data["last_read_message_id"]).to eq(message_1.id)
+          end
+        end
       end
     end
 

plugins/chat/spec/services/chat/mark_all_user_channels_read_spec.rb

@@ -154,6 +154,23 @@ RSpec.describe Chat::MarkAllUserChannelsRead do
           },
         )
       end
+
+      context "for threads" do
+        fab!(:thread) { Fabricate(:chat_thread, channel: channel_1, original_message: message_2) }
+
+        it "does not use thread replies for last_read_message_id" do
+          Fabricate(:chat_message, chat_channel: channel_1, user: other_user, thread: thread)
+          result
+          expect(membership_1.reload.last_read_message_id).to eq(message_2.id)
+        end
+
+        it "does use thread original messages for last_read_message_id" do
+          new_om = Fabricate(:chat_message, chat_channel: channel_1, user: other_user)
+          thread.update!(original_message: new_om, original_message_user: other_user)
+          result
+          expect(membership_1.reload.last_read_message_id).to eq(new_om.id)
+        end
+      end
     end
   end
 end

plugins/chat/spec/system/channel_thread_message_echoing_spec.rb

@@ -85,5 +85,17 @@ describe "Channel thread message echoing", type: :system, js: true do
         channel_page.message_by_id_selector(thread.replies.last.id),
       )
     end
+
+    it "does show the thread original_message if it is the last message in the channel" do
+      new_thread = Fabricate(:chat_thread, channel: channel)
+      current_user
+        .user_chat_channel_memberships
+        .find_by(chat_channel: channel)
+        .update!(last_read_message_id: new_thread.original_message_id)
+      chat_page.visit_channel(channel)
+      expect(channel_page).to have_css(
+        channel_page.message_by_id_selector(new_thread.original_message_id),
+      )
+    end
   end
 end

plugins/chat/spec/system/single_thread_spec.rb

@@ -143,6 +143,22 @@ describe "Single thread in side panel", type: :system, js: true do
           expect(open_thread).to have_message(thread.id, text: "this is a test message")
         end
       end
+
+      it "does not mark the channel unread if another user sends a message in the thread" do
+        other_user = Fabricate(:user)
+        chat_system_user_bootstrap(user: other_user, channel: channel)
+        Chat::MessageCreator.create(
+          chat_channel: channel,
+          user: other_user,
+          content: "Hello world!",
+          thread_id: thread.id,
+        )
+        sign_in(current_user)
+        chat_page.visit_channel(channel)
+        expect(page).not_to have_css(
+          ".sidebar-section-link.channel-#{channel.id} .sidebar-section-link-suffix.unread",
+        )
+      end
     end
 
     context "when using mobile" do