From 351e3ccd98985f6b6a76aabddd8209ba33dc1306 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Thu, 20 Apr 2023 14:38:00 +1000 Subject: [PATCH] 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 | 30 +-- .../chat/spec/services/chat/publisher_spec.rb | 192 ++++++++++++++++-- 2 files changed, 192 insertions(+), 30 deletions(-) diff --git a/plugins/chat/app/services/chat/publisher.rb b/plugins/chat/app/services/chat/publisher.rb index dd4013a1e81..a14bc288dcd 100644 --- a/plugins/chat/app/services/chat/publisher.rb +++ b/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), { diff --git a/plugins/chat/spec/services/chat/publisher_spec.rb b/plugins/chat/spec/services/chat/publisher_spec.rb index d5cc2ec781f..78e3e3d15f3 100644 --- a/plugins/chat/spec/services/chat/publisher_spec.rb +++ b/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