FIX: Serialize thread membership for user (#21743)

This commit follows up b6c5a2da08
by serializing the user's thread memberships in these cases:

1. When we do the initial channel fetch with messages, we get
   all threads and all the user's thread memberships for those
   messages.
2. When the thread list is fetched, we get all the user's memberships
   in that list.
3. When the single thread is fetched, either from opening it from
   the list, an OM indicator, or just from doing .find() on the
   manager when a new MessageBus message comes in

This will let us track the lastReadMessageId on the client, and
will also let us fix an issue where the unread indicator in the
channel header was incrementing for every thread that got a
new message, regardless of whether the user was a member.

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

@@ -10,6 +10,7 @@ class Chat::Api::ChannelThreadsController < Chat::ApiController
             threads: result.threads,
             channel: result.channel,
             tracking: result.tracking,
+            memberships: result.memberships,
           ),
           ::Chat::ThreadListSerializer,
           root: false,
@@ -25,7 +26,14 @@ class Chat::Api::ChannelThreadsController < Chat::ApiController
 
   def show
     with_service(::Chat::LookupThread) do
-      on_success { render_serialized(result.thread, ::Chat::ThreadSerializer, root: "thread") }
+      on_success do
+        render_serialized(
+          result.thread,
+          ::Chat::ThreadSerializer,
+          root: "thread",
+          membership: result.membership,
+        )
+      end
       on_failed_policy(:threaded_discussions_enabled) { raise Discourse::NotFound }
       on_failed_policy(:threading_enabled_for_channel) { raise Discourse::NotFound }
       on_model_not_found(:thread) { raise Discourse::NotFound }

plugins/chat/app/models/chat/thread.rb

@@ -36,6 +36,10 @@ module Chat
       Chat::UserChatThreadMembership.find_by(user: user, thread: self)&.destroy
     end
 
+    def membership_for(user)
+      user_chat_thread_memberships.find_by(user: user)
+    end
+
     def replies
       self.chat_messages.where.not(id: self.original_message_id)
     end

plugins/chat/app/models/chat/threads_view.rb

@@ -2,13 +2,14 @@
 
 module Chat
   class ThreadsView
-    attr_reader :user, :channel, :threads, :tracking
+    attr_reader :user, :channel, :threads, :tracking, :memberships
 
-    def initialize(channel:, threads:, user:, tracking:)
+    def initialize(channel:, threads:, user:, tracking:, memberships:)
       @channel = channel
       @threads = threads
       @user = user
       @tracking = tracking
+      @memberships = memberships
     end
   end
 end

plugins/chat/app/models/chat/view.rb

@@ -9,7 +9,8 @@ module Chat
                 :can_load_more_future,
                 :unread_thread_ids,
                 :threads,
-                :tracking
+                :tracking,
+                :thread_memberships
 
     def initialize(
       chat_channel:,
@@ -19,7 +20,8 @@ module Chat
       can_load_more_future: nil,
       unread_thread_ids: nil,
       threads: nil,
-      tracking: nil
+      tracking: nil,
+      thread_memberships: nil
     )
       @chat_channel = chat_channel
       @chat_messages = chat_messages
@@ -29,6 +31,7 @@ module Chat
       @unread_thread_ids = unread_thread_ids
       @threads = threads
       @tracking = tracking
+      @thread_memberships = thread_memberships
     end
 
     def reviewable_ids

plugins/chat/app/serializers/chat/base_thread_membership_serializer.rb

@@ -0,0 +1,7 @@
+# frozen_string_literal: true
+
+module Chat
+  class BaseThreadMembershipSerializer < ApplicationSerializer
+    attributes :notification_level, :thread_id, :last_read_message_id
+  end
+end

plugins/chat/app/serializers/chat/thread_list_serializer.rb

@@ -5,11 +5,14 @@ module Chat
     attributes :meta, :threads, :tracking
 
     def threads
-      ActiveModel::ArraySerializer.new(
-        object.threads,
-        each_serializer: Chat::ThreadSerializer,
-        scope: scope,
-      )
+      object.threads.map do |thread|
+        Chat::ThreadSerializer.new(
+          thread,
+          scope: scope,
+          membership: object.memberships.find { |m| m.thread_id == thread.id },
+          root: nil,
+        )
+      end
     end
 
     def tracking

plugins/chat/app/serializers/chat/thread_serializer.rb

@@ -5,7 +5,7 @@ module Chat
     has_one :original_message_user, serializer: BasicUserWithStatusSerializer, embed: :objects
     has_one :original_message, serializer: Chat::ThreadOriginalMessageSerializer, embed: :objects
 
-    attributes :id, :title, :status, :channel_id, :meta, :reply_count
+    attributes :id, :title, :status, :channel_id, :meta, :reply_count, :current_user_membership
 
     def initialize(object, opts)
       super(object, opts)
@@ -13,6 +13,7 @@ module Chat
 
       # Avoids an N1 to re-load the thread in the serializer for original_message.
       object.original_message.thread = object
+      @current_user_membership = opts[:membership]
     end
 
     def meta
@@ -23,6 +24,20 @@ module Chat
       object.replies_count_cache || 0
     end
 
+    def include_current_user_membership?
+      @current_user_membership.present?
+    end
+
+    def current_user_membership
+      @current_user_membership.thread = object
+
+      Chat::BaseThreadMembershipSerializer.new(
+        @current_user_membership,
+        scope: scope,
+        root: false,
+      ).as_json
+    end
+
     private
 
     def thread_message_bus_last_id

plugins/chat/app/serializers/chat/view_serializer.rb

@@ -7,11 +7,14 @@ module Chat
     def threads
       return [] if !object.threads
 
-      ActiveModel::ArraySerializer.new(
-        object.threads,
-        each_serializer: Chat::ThreadSerializer,
-        scope: scope,
-      )
+      object.threads.map do |thread|
+        Chat::ThreadSerializer.new(
+          thread,
+          scope: scope,
+          membership: object.thread_memberships.find { |m| m.thread_id == thread.id },
+          root: nil,
+        )
+      end
     end
 
     def tracking

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

@@ -35,6 +35,7 @@ module Chat
     step :fetch_unread_thread_ids
     step :fetch_threads_for_messages
     step :fetch_tracking
+    step :fetch_thread_memberships
     step :build_view
 
     class Contract
@@ -171,6 +172,18 @@ module Chat
       end
     end
 
+    def fetch_thread_memberships(threads:, guardian:, **)
+      if threads.empty?
+        context.thread_memberships = []
+      else
+        context.thread_memberships =
+          ::Chat::UserChatThreadMembership.where(
+            thread_id: threads.map(&:id),
+            user_id: guardian.user.id,
+          )
+      end
+    end
+
     def build_view(
       guardian:,
       channel:,
@@ -180,6 +193,7 @@ module Chat
       unread_thread_ids:,
       can_load_more_past:,
       can_load_more_future:,
+      thread_memberships:,
       **
     )
       context.view =
@@ -192,6 +206,7 @@ module Chat
           unread_thread_ids: unread_thread_ids,
           threads: threads,
           tracking: tracking,
+          thread_memberships: thread_memberships,
         )
     end
   end

plugins/chat/app/services/chat/lookup_channel_threads.rb

@@ -24,6 +24,7 @@ module Chat
     policy :can_view_channel
     model :threads
     step :fetch_tracking
+    step :fetch_memberships
 
     # @!visibility private
     class Contract
@@ -81,5 +82,13 @@ module Chat
           include_threads: true,
         ).thread_tracking
     end
+
+    def fetch_memberships(guardian:, threads:, **)
+      context.memberships =
+        ::Chat::UserChatThreadMembership.where(
+          thread_id: threads.map(&:id),
+          user_id: guardian.user.id,
+        )
+    end
   end
 end

plugins/chat/app/services/chat/lookup_thread.rb

@@ -24,6 +24,7 @@ module Chat
     model :thread, :fetch_thread
     policy :invalid_access
     policy :threading_enabled_for_channel
+    step :fetch_membership
 
     # @!visibility private
     class Contract
@@ -54,5 +55,9 @@ module Chat
     def threading_enabled_for_channel(thread:, **)
       thread.channel.threading_enabled
     end
+
+    def fetch_membership(thread:, guardian:, **)
+      context.membership = thread.membership_for(guardian.user)
+    end
   end
 end

plugins/chat/assets/javascripts/discourse/models/chat-thread.js

@@ -5,6 +5,7 @@ import { tracked } from "@glimmer/tracking";
 import guid from "pretty-text/guid";
 import ChatMessage from "discourse/plugins/chat/discourse/models/chat-message";
 import ChatTrackingState from "discourse/plugins/chat/discourse/models/chat-tracking-state";
+import UserChatThreadMembership from "discourse/plugins/chat/discourse/models/user-chat-thread-membership";
 
 export const THREAD_STATUSES = {
   open: "open",
@@ -28,6 +29,7 @@ export default class ChatThread {
   @tracked threadMessageBusLastId;
   @tracked replyCount;
   @tracked tracking;
+  @tracked currentUserMembership = null;
 
   messagesManager = new ChatMessagesManager(getOwner(this));
 
@@ -41,6 +43,12 @@ export default class ChatThread {
     this.replyCount = args.reply_count;
     this.originalMessage = ChatMessage.create(channel, args.original_message);
 
+    if (args.current_user_membership) {
+      this.currentUserMembership = UserChatThreadMembership.create(
+        args.current_user_membership
+      );
+    }
+
     this.tracking = new ChatTrackingState(getOwner(this));
   }
 

plugins/chat/assets/javascripts/discourse/models/user-chat-thread-membership.js

@@ -0,0 +1,15 @@
+import { tracked } from "@glimmer/tracking";
+
+export default class UserChatThreadMembership {
+  static create(args = {}) {
+    return new UserChatThreadMembership(args);
+  }
+
+  @tracked lastReadMessageId = null;
+  @tracked notificationLevel = null;
+
+  constructor(args = {}) {
+    this.lastReadMessageId = args.last_read_message_id;
+    this.notificationLevel = args.notification_level;
+  }
+}

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

@@ -34,7 +34,10 @@ export default class ChatChannelThreadPaneSubscriptionsManager extends ChatPaneB
     return;
   }
 
-  _afterDeleteMessage() {
-    // TODO (martin) Handle this once we have lastReadMessageId for thread memberships.
+  _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-subscriptions-manager.js

@@ -199,7 +199,6 @@ export default class ChatSubscriptionsManager extends Service {
         if (this.currentUser.ignored_users.includes(busData.username)) {
           channel.currentUserMembership.lastReadMessageId = busData.message_id;
         } else {
-          // Message from other user. Increment unread for channel tracking state.
           if (
             busData.message_id >
             (channel.currentUserMembership.lastReadMessageId || 0)
@@ -209,7 +208,13 @@ export default class ChatSubscriptionsManager extends Service {
 
           // Thread should be considered unread if not already.
           if (busData.thread_id) {
-            channel.unreadThreadIds.add(busData.thread_id);
+            channel.threadsManager
+              .find(busData.channel_id, busData.thread_id)
+              .then((thread) => {
+                if (thread.currentUserMembership) {
+                  channel.unreadThreadIds.add(busData.thread_id);
+                }
+              });
           }
         }
       }
@@ -225,21 +230,27 @@ export default class ChatSubscriptionsManager extends Service {
         .then((thread) => {
           if (busData.user_id === this.currentUser.id) {
             // Thread should no longer be considered unread.
-            channel.unreadThreadIds.remove(busData.thread_id);
-            // TODO (martin) Update lastReadMessageId for thread membership on client.
+            if (thread.currentUserMembership) {
+              channel.unreadThreadIds.delete(busData.thread_id);
+              thread.currentUserMembership.lastReadMessageId =
+                busData.message_id;
+            }
           } else {
+            // Ignored user sent message, update tracking state to no unread
             if (this.currentUser.ignored_users.includes(busData.username)) {
-              // TODO (martin) Update lastReadMessageId for thread membership on client.
+              if (thread.currentUserMembership) {
+                thread.currentUserMembership.lastReadMessageId =
+                  busData.message_id;
+              }
             } else {
+              // Message from other user. Increment unread for thread tracking state.
               if (
-                this.chat.activeChannel?.activeThread?.id === busData.thread_id
+                thread.currentUserMembership &&
+                busData.message_id >
+                  (thread.currentUserMembership.lastReadMessageId || 0)
               ) {
-                // TODO (martin) HACK: We don't yet have the lastReadMessageId on the client,
-                // so if the user is looking at the thread don't do anything to mark it unread.
-              } else {
-                // Message from other user. Thread should be considered unread if not already.
                 channel.unreadThreadIds.add(busData.thread_id);
-                thread.tracking.unreadCount += 1;
+                thread.tracking.unreadCount++;
               }
             }
           }

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

@@ -123,6 +123,19 @@ RSpec.describe Chat::ChannelViewBuilder do
         expect(subject.view.threads).to eq([message_1.thread])
       end
 
+      it "fetches thread memberships for the current user for fetched threads" do
+        message_1 =
+          Fabricate(
+            :chat_message,
+            chat_channel: channel,
+            thread: Fabricate(:chat_thread, channel: channel),
+          )
+        message_1.thread.add(current_user)
+        expect(subject.view.thread_memberships).to eq(
+          [message_1.thread.membership_for(current_user)],
+        )
+      end
+
       it "calls the tracking state report query for thread overview and tracking" do
         thread = Fabricate(:chat_thread, channel: channel)
         message_1 = Fabricate(:chat_message, chat_channel: channel, thread: thread)

plugins/chat/spec/system/thread_tracking/full_page_spec.rb

@@ -54,5 +54,14 @@ describe "Thread tracking state | full page", type: :system, js: true do
       expect(channel_page).to have_unread_thread_indicator(count: 1)
       expect(thread_list_page).to have_unread_item(thread.id)
     end
+
+    it "does not change the unread indicator for the header icon when the user is not a member of the thread" do
+      thread.remove(current_user)
+      chat_page.visit_channel(channel)
+      channel_page.open_thread_list
+      Fabricate(:chat_message, chat_channel: channel, thread: thread)
+      expect(channel_page).to have_no_unread_thread_indicator
+      expect(thread_list_page).to have_no_unread_item(thread.id)
+    end
   end
 end