diff --git a/plugins/chat/config/locales/server.en.yml b/plugins/chat/config/locales/server.en.yml index c9d63812d9c..372eda5e4a3 100644 --- a/plugins/chat/config/locales/server.en.yml +++ b/plugins/chat/config/locales/server.en.yml @@ -73,7 +73,6 @@ en: over_chat_max_direct_message_users: one: "You can only create a direct message with yourself." other: "You can't create a direct message with more than %{count} other users." - root_message_not_found: "The ancestor of the message you are replying cannot be found or has been deleted." reviewables: message_already_handled: "Thanks, but we've already reviewed this message and determined it does not need to be flagged again." actions: diff --git a/plugins/chat/lib/chat_message_creator.rb b/plugins/chat/lib/chat_message_creator.rb index fb7a128c32b..fe7fed04599 100644 --- a/plugins/chat/lib/chat_message_creator.rb +++ b/plugins/chat/lib/chat_message_creator.rb @@ -11,7 +11,6 @@ class Chat::ChatMessageCreator def initialize( chat_channel:, in_reply_to_id: nil, - thread_id: nil, user:, content:, staged_id: nil, @@ -21,15 +20,11 @@ class Chat::ChatMessageCreator @chat_channel = chat_channel @user = user @guardian = Guardian.new(user) - - # NOTE: We confirm this exists and the user can access it in the ChatController, - # but in future the checks should be here @in_reply_to_id = in_reply_to_id @content = content @staged_id = staged_id @incoming_chat_webhook = incoming_chat_webhook @upload_ids = upload_ids || [] - @thread_id = thread_id @error = nil @chat_message = @@ -47,13 +42,9 @@ class Chat::ChatMessageCreator validate_channel_status! uploads = get_uploads validate_message!(has_uploads: uploads.any?) - validate_reply_chain! - validate_existing_thread! - @chat_message.thread_id = @existing_thread&.id @chat_message.cook @chat_message.save! create_chat_webhook_event - create_thread @chat_message.attach_uploads(uploads) ChatDraft.where(user_id: @user.id, chat_channel_id: @chat_channel.id).destroy_all ChatPublisher.publish_new!(@chat_channel, @chat_message, @staged_id) @@ -90,56 +81,6 @@ class Chat::ChatMessageCreator end end - def validate_reply_chain! - return if @in_reply_to_id.blank? - - @root_message_id = DB.query_single(<<~SQL).last - WITH RECURSIVE root_message_finder( id, in_reply_to_id ) - AS ( - -- start with the message id we want to find the parents of - SELECT id, in_reply_to_id - FROM chat_messages - WHERE id = #{@in_reply_to_id} - - UNION ALL - - -- get the chain of direct parents of the message - -- following in_reply_to_id - SELECT cm.id, cm.in_reply_to_id - FROM root_message_finder rm - JOIN chat_messages cm ON rm.in_reply_to_id = cm.id - ) - SELECT id FROM root_message_finder - - -- this makes it so only the root parent ID is returned, we can - -- exclude this to return all parents in the chain - WHERE in_reply_to_id IS NULL; - SQL - - raise StandardError.new(I18n.t("chat.errors.root_message_not_found")) if @root_message_id.blank? - - @root_message = ChatMessage.with_deleted.find_by(id: @root_message_id) - raise StandardError.new(I18n.t("chat.errors.root_message_not_found")) if @root_message&.trashed? - end - - def validate_existing_thread! - return if @thread_id.blank? - @existing_thread = ChatThread.find(@thread_id) - - if @existing_thread.channel_id != @chat_channel.id - raise StandardError.new(I18n.t("chat.errors.thread_invalid_for_channel")) - end - - reply_to_thread_mismatch = - @chat_message.in_reply_to&.thread_id && - @chat_message.in_reply_to.thread_id != @existing_thread.id - root_message_has_no_thread = @root_message && @root_message.thread_id.blank? - root_message_thread_mismatch = @root_message && @root_message.thread_id != @existing_thread.id - if reply_to_thread_mismatch || root_message_has_no_thread || root_message_thread_mismatch - raise StandardError.new(I18n.t("chat.errors.thread_does_not_match_parent")) - end - end - def validate_message!(has_uploads:) @chat_message.validate_message(has_uploads: has_uploads) if @chat_message.errors.present? @@ -160,39 +101,4 @@ class Chat::ChatMessageCreator Upload.where(id: @upload_ids, user_id: @user.id) end - - def create_thread - return if @in_reply_to_id.blank? - return if @chat_message.thread_id.present? - - thread = - @root_message.thread || - ChatThread.create!( - original_message: @chat_message.in_reply_to, - original_message_user: @chat_message.in_reply_to.user, - channel: @chat_message.chat_channel, - ) - - # NOTE: We intentionally do not try to correct thread IDs within the chain - # if they are incorrect, and only set the thread ID of messages where the - # thread ID is NULL. In future we may want some sync/background job to correct - # any inconsistencies. - DB.exec(<<~SQL) - WITH RECURSIVE thread_updater AS ( - SELECT cm.id, cm.in_reply_to_id - FROM chat_messages cm - WHERE cm.in_reply_to_id IS NULL AND cm.id = #{@root_message_id} - - UNION ALL - - SELECT cm.id, cm.in_reply_to_id - FROM chat_messages cm - JOIN thread_updater ON cm.in_reply_to_id = thread_updater.id - ) - UPDATE chat_messages - SET thread_id = #{thread.id} - FROM thread_updater - WHERE thread_id IS NULL AND chat_messages.id = thread_updater.id - SQL - end end diff --git a/plugins/chat/lib/message_mover.rb b/plugins/chat/lib/message_mover.rb index 99330ec2775..11cd84ac33d 100644 --- a/plugins/chat/lib/message_mover.rb +++ b/plugins/chat/lib/message_mover.rb @@ -16,19 +16,6 @@ # all of the references associated to a chat message (e.g. reactions, bookmarks, # notifications, revisions, mentions, uploads) will be updated to the new # message IDs via a moved_chat_messages temporary table. -# -# Reply chains are a little complex. No reply chains are preserved when moving -# messages into a new channel. Remaining messages that referenced moved ones -# have their in_reply_to_id cleared so the data makes sense. -# -# Threads are even more complex. No threads are preserved when moving messages -# into a new channel, they end up as just a flat series of messages that are -# not in a chain. If the original message of a thread and N other messages -# in that thread, then any messages left behind just get placed into a new -# thread. Message moving will be disabled in the thread UI while -# enable_experimental_chat_threaded_discussions is present, its too complicated -# to have end users reason about for now, and we may want a standalone -# "Move Thread" UI later on. class Chat::MessageMover class NoMessagesFound < StandardError end @@ -64,8 +51,6 @@ class Chat::MessageMover bulk_insert_movement_metadata update_references delete_source_messages - update_reply_references - update_thread_references end add_moved_placeholder(destination_channel, moved_messages.first) @@ -75,10 +60,7 @@ class Chat::MessageMover private def find_messages(message_ids, channel) - ChatMessage - .includes(thread: %i[original_message original_message_user]) - .where(id: message_ids, chat_channel_id: channel.id) - .order("created_at ASC, id ASC") + ChatMessage.where(id: message_ids, chat_channel_id: channel.id).order("created_at ASC, id ASC") end def create_temp_table @@ -172,13 +154,7 @@ class Chat::MessageMover end def delete_source_messages - # We do this so @source_messages is not nulled out, which is the - # case when using update_all here. - DB.exec(<<~SQL, source_message_ids: @source_message_ids, deleted_by_id: @acting_user.id) - UPDATE chat_messages - SET deleted_at = NOW(), deleted_by_id = :deleted_by_id - WHERE id IN (:source_message_ids) - SQL + @source_messages.update_all(deleted_at: Time.zone.now, deleted_by_id: @acting_user.id) ChatPublisher.publish_bulk_delete!(@source_channel, @source_message_ids) end @@ -196,41 +172,4 @@ class Chat::MessageMover ), ) end - - def update_reply_references - DB.exec(<<~SQL, deleted_reply_to_ids: @source_message_ids) - UPDATE chat_messages - SET in_reply_to_id = NULL - WHERE in_reply_to_id IN (:deleted_reply_to_ids) - SQL - end - - def update_thread_references - threads_to_update = [] - @source_messages - .select { |message| message.thread_id.present? } - .each do |message_with_thread| - # If one of the messages we are moving is the original message in a thread, - # then all the remaining messages for that thread must be moved to a new one, - # otherwise they will be pointing to a thread in a different channel. - if message_with_thread.thread.original_message_id == message_with_thread.id - threads_to_update << message_with_thread.thread - end - end - - threads_to_update.each do |thread| - # NOTE: We may want to do something with the old empty thread at some - # point, maybe close or delete it. For now just leave it dangling. - next if thread.chat_messages.empty? - - original_message = thread.chat_messages.first - new_thread = - ChatThread.create( - original_message: original_message, - original_message_user: original_message.user, - channel: @source_channel, - ) - thread.chat_messages.update_all(thread_id: new_thread.id) - end - end end diff --git a/plugins/chat/spec/components/chat_message_creator_spec.rb b/plugins/chat/spec/components/chat_message_creator_spec.rb index 4405cdb5016..0f6f56a8b97 100644 --- a/plugins/chat/spec/components/chat_message_creator_spec.rb +++ b/plugins/chat/spec/components/chat_message_creator_spec.rb @@ -357,321 +357,6 @@ describe Chat::ChatMessageCreator do }.to change { ChatMention.count }.by(1) end - describe "replies" do - fab!(:reply_message) do - Fabricate(:chat_message, chat_channel: public_chat_channel, user: user2) - end - fab!(:unrelated_message_1) { Fabricate(:chat_message, chat_channel: public_chat_channel) } - fab!(:unrelated_message_2) { Fabricate(:chat_message, chat_channel: public_chat_channel) } - - it "links the message that the user is replying to" do - message = - Chat::ChatMessageCreator.create( - chat_channel: public_chat_channel, - user: user1, - content: "this is a message", - in_reply_to_id: reply_message.id, - ).chat_message - - expect(message.in_reply_to_id).to eq(reply_message.id) - end - - it "creates a thread and includes the original message and the reply" do - message = nil - expect { - message = - Chat::ChatMessageCreator.create( - chat_channel: public_chat_channel, - user: user1, - content: "this is a message", - in_reply_to_id: reply_message.id, - ).chat_message - }.to change { ChatThread.count }.by(1) - - expect(message.reload.thread).not_to eq(nil) - expect(message.in_reply_to.thread).to eq(message.thread) - expect(message.thread.original_message).to eq(reply_message) - expect(message.thread.original_message_user).to eq(reply_message.user) - end - - context "when the thread_id is provided" do - fab!(:existing_thread) { Fabricate(:chat_thread, channel: public_chat_channel) } - - it "does not create a thread when one is passed in" do - message = nil - expect { - message = - Chat::ChatMessageCreator.create( - chat_channel: public_chat_channel, - user: user1, - content: "this is a message", - thread_id: existing_thread.id, - ).chat_message - }.not_to change { ChatThread.count } - - expect(message.reload.thread).to eq(existing_thread) - end - - it "errors when the thread ID is for a different channel" do - other_channel_thread = Fabricate(:chat_thread, channel: Fabricate(:chat_channel)) - result = - Chat::ChatMessageCreator.create( - chat_channel: public_chat_channel, - user: user1, - content: "this is a message", - thread_id: other_channel_thread.id, - ) - expect(result.error.message).to eq(I18n.t("chat.errors.thread_invalid_for_channel")) - end - - it "errors when the thread does not match the in_reply_to thread" do - reply_message.update!(thread: existing_thread) - result = - Chat::ChatMessageCreator.create( - chat_channel: public_chat_channel, - user: user1, - content: "this is a message", - in_reply_to_id: reply_message.id, - thread_id: Fabricate(:chat_thread, channel: public_chat_channel).id, - ) - expect(result.error.message).to eq(I18n.t("chat.errors.thread_does_not_match_parent")) - end - - it "errors when the root message does not have a thread ID" do - reply_message.update!(thread: nil) - result = - Chat::ChatMessageCreator.create( - chat_channel: public_chat_channel, - user: user1, - content: "this is a message", - in_reply_to_id: reply_message.id, - thread_id: existing_thread.id, - ) - expect(result.error.message).to eq(I18n.t("chat.errors.thread_does_not_match_parent")) - end - end - - context "for missing root messages" do - fab!(:root_message) do - Fabricate( - :chat_message, - chat_channel: public_chat_channel, - user: user2, - created_at: 1.day.ago, - ) - end - - before { reply_message.update!(in_reply_to: root_message) } - - it "raises an error when the root message has been trashed" do - root_message.trash! - result = - Chat::ChatMessageCreator.create( - chat_channel: public_chat_channel, - user: user1, - content: "this is a message", - in_reply_to_id: reply_message.id, - ) - expect(result.error.message).to eq(I18n.t("chat.errors.root_message_not_found")) - end - - it "uses the next message in the chain as the root when the root is deleted" do - root_message.destroy! - Chat::ChatMessageCreator.create( - chat_channel: public_chat_channel, - user: user1, - content: "this is a message", - in_reply_to_id: reply_message.id, - ) - expect(reply_message.reload.thread).not_to eq(nil) - end - end - - context "when there is an existing reply chain" do - fab!(:old_message_1) do - Fabricate( - :chat_message, - chat_channel: public_chat_channel, - user: user1, - created_at: 6.hours.ago, - ) - end - fab!(:old_message_2) do - Fabricate( - :chat_message, - chat_channel: public_chat_channel, - user: user2, - in_reply_to: old_message_1, - created_at: 4.hours.ago, - ) - end - fab!(:old_message_3) do - Fabricate( - :chat_message, - chat_channel: public_chat_channel, - user: user1, - in_reply_to: old_message_2, - created_at: 1.hour.ago, - ) - end - - before do - reply_message.update!( - created_at: old_message_3.created_at + 1.hour, - in_reply_to: old_message_3, - ) - end - - it "creates a thread and updates all the messages in the chain" do - thread_count = ChatThread.count - message = - Chat::ChatMessageCreator.create( - chat_channel: public_chat_channel, - user: user1, - content: "this is a message", - in_reply_to_id: reply_message.id, - ).chat_message - - expect(ChatThread.count).to eq(thread_count + 1) - expect(message.reload.thread).not_to eq(nil) - expect(message.reload.in_reply_to.thread).to eq(message.thread) - expect(old_message_1.reload.thread).to eq(message.thread) - expect(old_message_2.reload.thread).to eq(message.thread) - expect(old_message_3.reload.thread).to eq(message.thread) - expect(message.thread.chat_messages.count).to eq(5) - message = - Chat::ChatMessageCreator.create( - chat_channel: public_chat_channel, - user: user1, - content: "this is a message", - in_reply_to_id: reply_message.id, - ).chat_message - end - - context "when a thread already exists and the thread_id is passed in" do - let!(:last_message) do - Chat::ChatMessageCreator.create( - chat_channel: public_chat_channel, - user: user1, - content: "this is a message", - in_reply_to_id: reply_message.id, - ).chat_message - end - let!(:existing_thread) { last_message.reload.thread } - - it "does not create a new thread" do - thread_count = ChatThread.count - - message = - Chat::ChatMessageCreator.create( - chat_channel: public_chat_channel, - user: user1, - content: "this is a message again", - in_reply_to_id: last_message.id, - thread_id: existing_thread.id, - ).chat_message - - expect(ChatThread.count).to eq(thread_count) - expect(message.reload.thread).to eq(existing_thread) - expect(message.reload.in_reply_to.thread).to eq(existing_thread) - expect(message.thread.chat_messages.count).to eq(6) - end - - it "errors when the thread does not match the root thread" do - old_message_1.update!(thread: Fabricate(:chat_thread, channel: public_chat_channel)) - result = - Chat::ChatMessageCreator.create( - chat_channel: public_chat_channel, - user: user1, - content: "this is a message", - in_reply_to_id: reply_message.id, - thread_id: existing_thread.id, - ) - expect(result.error.message).to eq(I18n.t("chat.errors.thread_does_not_match_parent")) - end - - it "errors when the root message does not have a thread ID" do - old_message_1.update!(thread: nil) - result = - Chat::ChatMessageCreator.create( - chat_channel: public_chat_channel, - user: user1, - content: "this is a message", - in_reply_to_id: reply_message.id, - thread_id: existing_thread.id, - ) - expect(result.error.message).to eq(I18n.t("chat.errors.thread_does_not_match_parent")) - end - end - - context "when there are hundreds of messages in a reply chain already" do - before do - previous_message = nil - 1000.times do |i| - previous_message = - Fabricate( - :chat_message, - chat_channel: public_chat_channel, - user: [user1, user2].sample, - in_reply_to: previous_message, - created_at: i.hours.ago, - ) - end - @last_message_in_chain = previous_message - end - - xit "works" do - thread_count = ChatThread.count - - message = nil - puts Benchmark.measure { - message = - Chat::ChatMessageCreator.create( - chat_channel: public_chat_channel, - user: user1, - content: "this is a message", - in_reply_to_id: @last_message_in_chain.id, - ).chat_message - } - - expect(ChatThread.count).to eq(thread_count + 1) - expect(message.reload.thread).not_to eq(nil) - expect(message.reload.in_reply_to.thread).to eq(message.thread) - expect(message.thread.chat_messages.count).to eq(1001) - end - end - - context "if the root message alread had a thread" do - fab!(:old_thread) { Fabricate(:chat_thread, original_message: old_message_1) } - fab!(:incorrect_thread) { Fabricate(:chat_thread, channel: public_chat_channel) } - - before do - old_message_1.update!(thread: old_thread) - old_message_3.update!(thread: incorrect_thread) - end - - it "does not change any messages in the chain, assumes they have the correct thread ID" do - thread_count = ChatThread.count - message = - Chat::ChatMessageCreator.create( - chat_channel: public_chat_channel, - user: user1, - content: "this is a message", - in_reply_to_id: reply_message.id, - ).chat_message - - expect(ChatThread.count).to eq(thread_count) - expect(message.reload.thread).to eq(old_thread) - expect(message.reload.in_reply_to.thread).to eq(old_thread) - expect(old_message_1.reload.thread).to eq(old_thread) - expect(old_message_2.reload.thread).to eq(old_thread) - expect(old_message_3.reload.thread).to eq(incorrect_thread) - expect(message.thread.chat_messages.count).to eq(4) - end - end - end - end - describe "group mentions" do it "creates chat mentions for group mentions where the group is mentionable" do expect { diff --git a/plugins/chat/spec/fabricators/chat_fabricator.rb b/plugins/chat/spec/fabricators/chat_fabricator.rb index 9c2cf3f7e39..a8d7f3e544e 100644 --- a/plugins/chat/spec/fabricators/chat_fabricator.rb +++ b/plugins/chat/spec/fabricators/chat_fabricator.rb @@ -139,14 +139,3 @@ Fabricator(:chat_draft) do { value: attrs[:value], replyToMsg: attrs[:reply_to_msg], uploads: attrs[:uploads] }.to_json end end - -Fabricator(:chat_thread) do - before_create do |thread, transients| - thread.original_message_user = original_message.user - thread.channel = original_message.chat_channel - end - - transient :channel - - original_message { |attrs| Fabricate(:chat_message, chat_channel: attrs[:channel]) } -end diff --git a/plugins/chat/spec/lib/message_mover_spec.rb b/plugins/chat/spec/lib/message_mover_spec.rb index 34202783b01..6fd4b6079bd 100644 --- a/plugins/chat/spec/lib/message_mover_spec.rb +++ b/plugins/chat/spec/lib/message_mover_spec.rb @@ -36,13 +36,17 @@ describe Chat::MessageMover do fab!(:message6) { Fabricate(:chat_message, chat_channel: destination_channel) } let(:move_message_ids) { [message1.id, message2.id, message3.id] } + subject do + described_class.new( + acting_user: acting_user, + source_channel: source_channel, + message_ids: move_message_ids, + ) + end + describe "#move_to_channel" do - def move!(move_message_ids = [message1.id, message2.id, message3.id]) - described_class.new( - acting_user: acting_user, - source_channel: source_channel, - message_ids: move_message_ids, - ).move_to_channel(destination_channel) + def move! + subject.move_to_channel(destination_channel) end it "raises an error if either the source or destination channels are not public (they cannot be DM channels)" do @@ -122,104 +126,5 @@ describe Chat::MessageMover do expect(revision.reload.chat_message_id).to eq(moved_messages.third.id) expect(webhook_event.reload.chat_message_id).to eq(moved_messages.third.id) end - - it "does not preserve reply chains using in_reply_to_id" do - message3.update!(in_reply_to: message2) - message2.update!(in_reply_to: message1) - move! - moved_messages = - ChatMessage.where(chat_channel: destination_channel).order("created_at ASC, id ASC").last(3) - - expect(moved_messages.pluck(:in_reply_to_id).uniq).to eq([nil]) - end - - it "clears in_reply_to_id for remaining messages when the messages they were replying to are moved" do - message3.update!(in_reply_to: message2) - message2.update!(in_reply_to: message1) - move!([message2.id]) - expect(message3.reload.in_reply_to_id).to eq(nil) - end - - context "when there is a thread" do - fab!(:thread) { Fabricate(:chat_thread, channel: source_channel, original_message: message1) } - - before do - message1.update!(thread: thread) - message2.update!(thread: thread) - message3.update!(thread: thread) - end - - it "does not preserve thread_ids" do - move! - moved_messages = - ChatMessage - .where(chat_channel: destination_channel) - .order("created_at ASC, id ASC") - .last(3) - - expect(moved_messages.pluck(:thread_id).uniq).to eq([nil]) - end - - it "clears in_reply_to_id for remaining messages when the messages they were replying to are moved but leaves the thread_id" do - message3.update!(in_reply_to: message2) - message2.update!(in_reply_to: message1) - move!([message2.id]) - expect(message3.reload.in_reply_to_id).to eq(nil) - expect(message3.reload.thread).to eq(thread) - end - - context "when a thread original message is moved" do - it "creates a new thread for the messages left behind in the old channel" do - message4 = - Fabricate( - :chat_message, - chat_channel: source_channel, - message: "the fourth message", - in_reply_to: message3, - thread: thread, - ) - message5 = - Fabricate( - :chat_message, - chat_channel: source_channel, - message: "the fifth message", - thread: thread, - ) - expect { move! }.to change { ChatThread.count }.by(1) - new_thread = ChatThread.last - expect(message4.reload.thread_id).to eq(new_thread.id) - expect(message5.reload.thread_id).to eq(new_thread.id) - expect(new_thread.channel).to eq(source_channel) - expect(new_thread.original_message).to eq(message4) - end - end - - context "when multiple thread original messages are moved" do - it "works the same as when one is" do - message4 = - Fabricate(:chat_message, chat_channel: source_channel, message: "the fourth message") - message5 = - Fabricate( - :chat_message, - chat_channel: source_channel, - in_reply_to: message5, - message: "the fifth message", - ) - other_thread = - Fabricate(:chat_thread, channel: source_channel, original_message: message4) - message4.update!(thread: other_thread) - message5.update!(thread: other_thread) - expect { move!([message1.id, message4.id]) }.to change { ChatThread.count }.by(2) - - new_threads = ChatThread.order(:created_at).last(2) - expect(message3.reload.thread_id).to eq(new_threads.first.id) - expect(message5.reload.thread_id).to eq(new_threads.second.id) - expect(new_threads.first.channel).to eq(source_channel) - expect(new_threads.second.channel).to eq(source_channel) - expect(new_threads.first.original_message).to eq(message2) - expect(new_threads.second.original_message).to eq(message5) - end - end - end end end