FIX: Chat publisher publishing to thread when threading disabled (#21179)

Followup to bd5c5c4b5f7b33a64cc12e2ba13e81767ac00edc, a
bug was introduced there for any channel that did not have
threading enabled or sites with the experimental threading
disabled. When the user replied to another chat message,
since this is always a thread in the background, we weren't
sending any MessageBus messages to the main channel, since
the message was a thread reply.

However in the UI these messages still show in the main stream
of the channel if threading is turned off, so the UI was not
reacting to these things happening in the backend. The worst
issue was that new clients would not see new replies sent in
reply to other messages in the channel.

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

@@ -15,18 +15,22 @@ module Chat
     end
 
     def self.calculate_publish_targets(channel, message)
-      targets =
-        if message.thread_om?
-          [
-            root_message_bus_channel(channel.id),
-            thread_message_bus_channel(channel.id, message.thread_id),
-          ]
-        elsif message.thread_reply?
-          [thread_message_bus_channel(channel.id, message.thread_id)]
-        else
-          [root_message_bus_channel(channel.id)]
-        end
-      targets
+      return [root_message_bus_channel(channel.id)] if !allow_publish_to_thread?(channel)
+
+      if message.thread_om?
+        [
+          root_message_bus_channel(channel.id),
+          thread_message_bus_channel(channel.id, message.thread_id),
+        ]
+      elsif message.thread_reply?
+        [thread_message_bus_channel(channel.id, message.thread_id)]
+      else
+        [root_message_bus_channel(channel.id)]
+      end
+    end
+
+    def self.allow_publish_to_thread?(channel)
+      SiteSetting.enable_experimental_chat_threaded_discussions && channel.threading_enabled
     end
 
     def self.publish_new!(chat_channel, chat_message, staged_id)
@@ -40,7 +44,7 @@ module Chat
       # NOTE: This means that the read count is only updated in the client
       # for new messages in the main channel stream, maybe in future we want to
       # do this for thread messages as well?
-      if !chat_message.thread_reply?
+      if !chat_message.thread_reply? || !allow_publish_to_thread?(chat_channel)
         MessageBus.publish(
           self.new_messages_message_bus_channel(chat_channel.id),
           {

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

@@ -16,19 +16,149 @@ describe Chat::Publisher do
   end
 
   describe ".calculate_publish_targets" do
-    context "when the chat message is the original message of a thread" do
-      fab!(:thread) { Fabricate(:chat_thread, original_message: message, channel: channel) }
+    context "when enable_experimental_chat_threaded_discussions is false" do
+      before { SiteSetting.enable_experimental_chat_threaded_discussions = false }
 
-      it "generates the correct targets" do
-        targets = described_class.calculate_publish_targets(channel, message)
-        expect(targets).to contain_exactly(
-          "/chat/#{channel.id}",
-          "/chat/#{channel.id}/thread/#{thread.id}",
+      context "when the message is the original message of a thread" do
+        fab!(:thread) { Fabricate(:chat_thread, original_message: message, channel: channel) }
+
+        it "generates the correct targets" do
+          targets = described_class.calculate_publish_targets(channel, message)
+          expect(targets).to contain_exactly("/chat/#{channel.id}")
+        end
+      end
+
+      context "when the message is a thread reply" do
+        fab!(:thread) do
+          Fabricate(
+            :chat_thread,
+            original_message: Fabricate(:chat_message, chat_channel: channel),
+            channel: channel,
+          )
+        end
+
+        before { message.update!(thread: thread) }
+
+        it "generates the correct targets" do
+          targets = described_class.calculate_publish_targets(channel, message)
+          expect(targets).to contain_exactly("/chat/#{channel.id}")
+        end
+      end
+
+      context "when the message is not part of a thread" do
+        it "generates the correct targets" do
+          targets = described_class.calculate_publish_targets(channel, message)
+          expect(targets).to contain_exactly("/chat/#{channel.id}")
+        end
+      end
+    end
+
+    context "when threading_enabled is false for the channel" do
+      before do
+        SiteSetting.enable_experimental_chat_threaded_discussions = true
+        channel.update!(threading_enabled: false)
+      end
+
+      context "when the message is the original message of a thread" do
+        fab!(:thread) { Fabricate(:chat_thread, original_message: message, channel: channel) }
+
+        it "generates the correct targets" do
+          targets = described_class.calculate_publish_targets(channel, message)
+          expect(targets).to contain_exactly("/chat/#{channel.id}")
+        end
+      end
+
+      context "when the message is a thread reply" do
+        fab!(:thread) do
+          Fabricate(
+            :chat_thread,
+            original_message: Fabricate(:chat_message, chat_channel: channel),
+            channel: channel,
+          )
+        end
+
+        before { message.update!(thread: thread) }
+
+        it "generates the correct targets" do
+          targets = described_class.calculate_publish_targets(channel, message)
+          expect(targets).to contain_exactly("/chat/#{channel.id}")
+        end
+      end
+
+      context "when the message is not part of a thread" do
+        it "generates the correct targets" do
+          targets = described_class.calculate_publish_targets(channel, message)
+          expect(targets).to contain_exactly("/chat/#{channel.id}")
+        end
+      end
+    end
+
+    context "when enable_experimental_chat_threaded_discussions is true and threading_enabled is true for the channel" do
+      before do
+        channel.update!(threading_enabled: true)
+        SiteSetting.enable_experimental_chat_threaded_discussions = true
+      end
+
+      context "when the message is the original message of a thread" do
+        fab!(:thread) { Fabricate(:chat_thread, original_message: message, channel: channel) }
+
+        it "generates the correct targets" do
+          targets = described_class.calculate_publish_targets(channel, message)
+          expect(targets).to contain_exactly(
+            "/chat/#{channel.id}",
+            "/chat/#{channel.id}/thread/#{thread.id}",
+          )
+        end
+      end
+
+      context "when the message is a thread reply" do
+        fab!(:thread) do
+          Fabricate(
+            :chat_thread,
+            original_message: Fabricate(:chat_message, chat_channel: channel),
+            channel: channel,
+          )
+        end
+
+        before { message.update!(thread: thread) }
+
+        it "generates the correct targets" do
+          targets = described_class.calculate_publish_targets(channel, message)
+          expect(targets).to contain_exactly("/chat/#{channel.id}/thread/#{thread.id}")
+        end
+      end
+
+      context "when the message is not part of a thread" do
+        it "generates the correct targets" do
+          targets = described_class.calculate_publish_targets(channel, message)
+          expect(targets).to contain_exactly("/chat/#{channel.id}")
+        end
+      end
+    end
+  end
+
+  describe ".publish_new!" do
+    let(:staged_id) { 999 }
+
+    context "when the message is not a thread reply" do
+      it "publishes to the new_messages_message_bus_channel" do
+        messages =
+          MessageBus.track_publish(described_class.new_messages_message_bus_channel(channel.id)) do
+            described_class.publish_new!(channel, message, staged_id)
+          end
+        expect(messages.first.data).to eq(
+          {
+            channel_id: channel.id,
+            message_id: message.id,
+            user_id: message.user_id,
+            username: message.user.username,
+            thread_id: nil,
+          },
         )
       end
     end
 
-    context "when the chat message is a thread reply" do
+    context "when the message is a thread reply" do
       fab!(:thread) do
         Fabricate(
           :chat_thread,
@@ -39,16 +169,44 @@ describe Chat::Publisher do
 
       before { message.update!(thread: thread) }
 
-      it "generates the correct targets" do
-        targets = described_class.calculate_publish_targets(channel, message)
-        expect(targets).to contain_exactly("/chat/#{channel.id}/thread/#{thread.id}")
-      end
-    end
+      context "if enable_experimental_chat_threaded_discussions is false" do
+        before { SiteSetting.enable_experimental_chat_threaded_discussions = false }
 
-    context "when the chat message is not part of a thread" do
-      it "generates the correct targets" do
-        targets = described_class.calculate_publish_targets(channel, message)
-        expect(targets).to contain_exactly("/chat/#{channel.id}")
+        it "publishes to the new_messages_message_bus_channel" do
+          messages =
+            MessageBus.track_publish(
+              described_class.new_messages_message_bus_channel(channel.id),
+            ) { described_class.publish_new!(channel, message, staged_id) }
+          expect(messages).not_to be_empty
+        end
+      end
+
+      context "if enable_experimental_chat_threaded_discussions is true" do
+        before { SiteSetting.enable_experimental_chat_threaded_discussions = true }
+
+        context "if threading_enabled is false for the channel" do
+          before { channel.update!(threading_enabled: false) }
+
+          it "publishes to the new_messages_message_bus_channel" do
+            messages =
+              MessageBus.track_publish(
+                described_class.new_messages_message_bus_channel(channel.id),
+              ) { described_class.publish_new!(channel, message, staged_id) }
+            expect(messages).not_to be_empty
+          end
+        end
+
+        context "if threading_enabled is true for the channel" do
+          before { channel.update!(threading_enabled: true) }
+
+          it "does not publish to the new_messages_message_bus_channel" do
+            messages =
+              MessageBus.track_publish(
+                described_class.new_messages_message_bus_channel(channel.id),
+              ) { described_class.publish_new!(channel, message, staged_id) }
+            expect(messages).to be_empty
+          end
+        end
       end
     end
   end