FIX: Mark threads read when threading enabled for a channel (#22458)

Since we create threads in the background regardless of whether
threading is enabled for a channel, we get the unexpected behaviour
of everyone having a lot of unread threads when threading is enabled
for the channel.

To counteract this, when the admin enables threads for a channel
we can just run a high priority background job to mark all threads
as read in the channel for all users, so they are essentially
starting from a clean slate.

plugins/chat/app/jobs/regular/chat/mark_all_channel_threads_read.rb

@@ -0,0 +1,16 @@
+# frozen_string_literal: true
+
+module Jobs
+  module Chat
+    class MarkAllChannelThreadsRead < Jobs::Base
+      sidekiq_options queue: "critical"
+
+      def execute(args = {})
+        return if !SiteSetting.enable_experimental_chat_threaded_discussions
+        channel = ::Chat::Channel.find_by(id: args[:channel_id])
+        return if channel.blank?
+        channel.mark_all_threads_as_read
+      end
+    end
+  end
+end

plugins/chat/app/models/chat/channel.rb

@@ -164,6 +164,31 @@ module Chat
       SQL
     end
 
+    def mark_all_threads_as_read(user: nil)
+      if !(self.threading_enabled || SiteSetting.enable_experimental_chat_threaded_discussions)
+        return
+      end
+
+      DB.exec(<<~SQL, channel_id: self.id)
+        UPDATE user_chat_thread_memberships
+        SET last_read_message_id = subquery.last_message_id
+        FROM (
+          SELECT chat_threads.id AS thread_id, MAX(chat_messages.id) AS last_message_id
+          FROM chat_threads
+          INNER JOIN chat_messages ON chat_messages.thread_id = chat_threads.id
+          WHERE chat_threads.channel_id = :channel_id
+          AND chat_messages.deleted_at IS NULL
+          GROUP BY chat_threads.id
+        ) subquery
+        WHERE user_chat_thread_memberships.thread_id = subquery.thread_id
+        #{user ? "AND user_chat_thread_memberships.user_id = #{user.id}" : ""}
+        AND (
+          user_chat_thread_memberships.last_read_message_id < subquery.last_message_id OR
+          user_chat_thread_memberships.last_read_message_id IS NULL
+        )
+      SQL
+    end
+
     private
 
     def change_status(acting_user, target_status)

plugins/chat/app/services/chat/update_channel.rb

@@ -13,7 +13,7 @@ module Chat
   #   name: "SuperChannel",
   #   description: "This is the best channel",
   #   slug: "super-channel",
-  #   threading_enaled: true,
+  #   threading_enabled: true,
   #  )
   #
   class UpdateChannel
@@ -36,6 +36,7 @@ module Chat
     policy :check_channel_permission
     contract default_values_from: :channel
     step :update_channel
+    step :mark_all_threads_as_read_if_needed
     step :publish_channel_update
     step :auto_join_users_if_needed
 
@@ -70,7 +71,14 @@ module Chat
     end
 
     def update_channel(channel:, contract:, **)
-      channel.update!(contract.attributes)
+      channel.assign_attributes(contract.attributes)
+      context.threading_enabled_changed = channel.threading_enabled_changed?
+      channel.save!
+    end
+
+    def mark_all_threads_as_read_if_needed(channel:, **)
+      return if !(context.threading_enabled_changed && channel.threading_enabled)
+      Jobs.enqueue(Jobs::Chat::MarkAllChannelThreadsRead, channel_id: channel.id)
     end
 
     def publish_channel_update(channel:, guardian:, **)

plugins/chat/spec/jobs/regular/mark_all_channel_threads_read_spec.rb

@@ -0,0 +1,46 @@
+# frozen_string_literal: true
+
+RSpec.describe Jobs::Chat::MarkAllChannelThreadsRead do
+  fab!(:channel) { Fabricate(:chat_channel, threading_enabled: true) }
+
+  context "when enable_experimental_chat_threaded_discussions is false" do
+    before { SiteSetting.enable_experimental_chat_threaded_discussions = false }
+
+    it "does nothing" do
+      Chat::Channel.any_instance.expects(:mark_all_threads_as_read).never
+      described_class.new.execute(channel_id: channel.id)
+    end
+  end
+
+  context "when enable_experimental_chat_threaded_discussions is true" do
+    fab!(:thread_1) { Fabricate(:chat_thread, channel: channel) }
+    fab!(:thread_2) { Fabricate(:chat_thread, channel: channel) }
+    fab!(:user_1) { Fabricate(:user) }
+    fab!(:user_2) { Fabricate(:user) }
+    fab!(:thread_1_message_1) { Fabricate(:chat_message, thread: thread_1) }
+    fab!(:thread_1_message_2) { Fabricate(:chat_message, thread: thread_1) }
+    fab!(:thread_1_message_3) { Fabricate(:chat_message, thread: thread_1) }
+    fab!(:thread_2_message_1) { Fabricate(:chat_message, thread: thread_2) }
+    fab!(:thread_2_message_2) { Fabricate(:chat_message, thread: thread_2) }
+
+    before do
+      SiteSetting.enable_experimental_chat_threaded_discussions = true
+      channel.add(user_1)
+      channel.add(user_2)
+      thread_1.add(user_1)
+      thread_2.add(user_2)
+    end
+
+    def unread_count(user)
+      Chat::ThreadUnreadsQuery.call(channel_ids: [channel.id], user_id: user.id).first.unread_count
+    end
+
+    it "marks all threads as read across all users in the channel" do
+      expect(unread_count(user_1)).to eq(3)
+      expect(unread_count(user_2)).to eq(2)
+      described_class.new.execute(channel_id: channel.id)
+      expect(unread_count(user_1)).to eq(0)
+      expect(unread_count(user_2)).to eq(0)
+    end
+  end
+end

plugins/chat/spec/services/chat/update_channel_spec.rb

@@ -113,22 +113,38 @@ RSpec.describe Chat::UpdateChannel do
 
       describe "threading_enabled" do
         context "when true" do
+          before { params[:threading_enabled] = true }
+
           it "changes the value to true" do
-            expect {
+            expect { result }.to change { channel.reload.threading_enabled }.from(false).to(true)
-              params[:threading_enabled] = true
+          end
-              result
+
-            }.to change { channel.reload.threading_enabled }.from(false).to(true)
+          it "enqueues a job to mark all threads in the channel as read" do
+            expect_enqueued_with(
+              job: Jobs::Chat::MarkAllChannelThreadsRead,
+              args: {
+                channel_id: channel.id,
+              },
+            ) { result }
           end
         end
 
         context "when false" do
-          it "changes the value to true" do
+          before { params[:threading_enabled] = false }
+
+          it "changes the value to false" do
             channel.update!(threading_enabled: true)
 
-            expect {
+            expect { result }.to change { channel.reload.threading_enabled }.from(true).to(false)
-              params[:threading_enabled] = false
+          end
-              result
+
-            }.to change { channel.reload.threading_enabled }.from(true).to(false)
+          it "does not enqueue a job to mark all threads in the channel as read" do
+            expect_not_enqueued_with(
+              job: Jobs::Chat::MarkAllChannelThreadsRead,
+              args: {
+                channel_id: channel.id,
+              },
+            ) { result }
           end
         end
       end