FEATURE: Add thread support to the chat message mover (#26147)

When selecting messages to move to a new channel, if any of the selected messages is the original message of a thread, the entire thread, including all its replies, will be moved to the destination channel

plugins/chat/lib/chat/message_mover.rb

@@ -21,13 +21,11 @@
 # 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
+# The service supports moving threads. If any of the selected messages is the
-# into a new channel, they end up as just a flat series of messages that are
+# original message of a thread, the entire thread with all its replies will be
-# not in a chain. If the original message of a thread and N other messages
+# moved to the destination channel. Moving individual messages out of a thread
-# in that thread, then any messages left behind just get placed into a new
+# is still disabled.
-# thread. Message moving will be disabled in the thread UI, its too complicated
+
-# to have end users reason about for now, and we may want a standalone
-# "Move Thread" UI later on.
 module Chat
   class MessageMover
     class NoMessagesFound < StandardError
@@ -42,6 +40,7 @@ module Chat
       @source_message_ids = message_ids
       @source_messages = find_messages(@source_message_ids, source_channel)
       @ordered_source_message_ids = @source_messages.map(&:id)
+      @source_thread_ids = @source_messages.pluck(:thread_id).uniq.compact
     end
 
     def move_to_channel(destination_channel)
@@ -56,18 +55,21 @@ module Chat
       moved_messages = nil
 
       Chat::Message.transaction do
-        create_temp_table
+        create_temp_table_for_messages
+        create_temp_table_for_threads
+        moved_thread_ids = create_destination_threads_in_channel(destination_channel)
         moved_messages =
           find_messages(
-            create_destination_messages_in_channel(destination_channel),
+            create_destination_messages_in_channel(destination_channel, moved_thread_ids),
             destination_channel,
           )
-        bulk_insert_movement_metadata
+        bulk_insert_movement_metadata_for_messages
-        update_references
+        update_message_references
         delete_source_messages
         update_reply_references
         update_tracking_state
-        update_thread_references
+        update_thread_references(moved_thread_ids)
+        delete_source_threads
       end
 
       add_moved_placeholder(destination_channel, moved_messages.first)
@@ -79,11 +81,15 @@ module Chat
     def find_messages(message_ids, channel)
       Chat::Message
         .includes(thread: %i[original_message original_message_user])
-        .where(id: message_ids, chat_channel_id: channel.id)
+        .where(chat_channel_id: channel.id)
+        .where(
+          "id IN (:message_ids) OR thread_id IN (SELECT thread_id FROM chat_messages WHERE id IN (:message_ids))",
+          message_ids: message_ids,
+        )
         .order("created_at ASC, id ASC")
     end
 
-    def create_temp_table
+    def create_temp_table_for_messages
       DB.exec("DROP TABLE IF EXISTS moved_chat_messages") if Rails.env.test?
 
       DB.exec <<~SQL
@@ -96,41 +102,76 @@ module Chat
     SQL
     end
 
-    def bulk_insert_movement_metadata
+    def create_temp_table_for_threads
+      DB.exec("DROP TABLE IF EXISTS moved_chat_threads") if Rails.env.test?
+
+      DB.exec <<~SQL
+      CREATE TEMPORARY TABLE moved_chat_threads (
+        old_thread_id INTEGER,
+        new_thread_id INTEGER
+      ) ON COMMIT DROP;
+
+      CREATE INDEX moved_chat_threads_old_thread_id ON moved_chat_threads(old_thread_id);
+    SQL
+    end
+
+    def bulk_insert_movement_metadata_for_messages
       values_sql = @movement_metadata.map { |mm| "(#{mm[:old_id]}, #{mm[:new_id]})" }.join(",\n")
       DB.exec(
         "INSERT INTO moved_chat_messages(old_chat_message_id, new_chat_message_id) VALUES #{values_sql}",
       )
     end
 
+    def create_destination_threads_in_channel(destination_channel)
+      moved_thread_ids =
+        @source_thread_ids.each_with_object({}) do |old_thread_id, hash|
+          old_thread = Chat::Thread.find(old_thread_id)
+          new_thread =
+            Chat::Thread.create!(
+              channel_id: destination_channel.id,
+              original_message_user_id: old_thread.original_message_user_id,
+              original_message_id: old_thread.original_message_id, # Placeholder, will be updated later
+              replies_count: old_thread.replies_count,
+              status: old_thread.status,
+              title: old_thread.title,
+            )
+          hash[old_thread_id] = new_thread.id
+        end
+
+      moved_thread_ids
+    end
+
     ##
     # We purposefully omit in_reply_to_id when creating the messages in the
     # new channel, because it could be pointing to a message that has not
     # been moved.
-    def create_destination_messages_in_channel(destination_channel)
+    def create_destination_messages_in_channel(destination_channel, moved_thread_ids)
-      query_args = {
+      insert_messages = <<-SQL
-        message_ids: @ordered_source_message_ids,
-        destination_channel_id: destination_channel.id,
-      }
-
-      moved_message_ids = DB.query_single(<<~SQL, query_args)
         INSERT INTO chat_messages (
-        chat_channel_id, user_id, last_editor_id, message, cooked, cooked_version, created_at, updated_at
+          chat_channel_id, user_id, last_editor_id, message, cooked, cooked_version, thread_id, created_at, updated_at
         )
-      SELECT :destination_channel_id,
+        SELECT :destination_channel_id, user_id, last_editor_id, message, cooked, cooked_version, :new_thread_id, CLOCK_TIMESTAMP(), CLOCK_TIMESTAMP()
-             user_id,
-             last_editor_id,
-             message,
-             cooked,
-             cooked_version,
-             CLOCK_TIMESTAMP(),
-             CLOCK_TIMESTAMP()
         FROM chat_messages
-      WHERE id IN (:message_ids)
+        WHERE id = :source_message_id
-      ORDER BY created_at ASC, id ASC
         RETURNING id
       SQL
 
+      moved_message_ids =
+        @source_messages.map do |source_message|
+          new_thread_id = moved_thread_ids[source_message.thread_id]
+
+          new_message_id =
+            DB.query_single(
+              insert_messages,
+              {
+                destination_channel_id: destination_channel.id,
+                new_thread_id: new_thread_id,
+                source_message_id: source_message.id,
+              },
+            ).first
+
+          new_message_id
+        end
       @movement_metadata =
         moved_message_ids.map.with_index do |chat_message_id, idx|
           { old_id: @ordered_source_message_ids[idx], new_id: chat_message_id }
@@ -138,7 +179,7 @@ module Chat
       moved_message_ids
     end
 
-    def update_references
+    def update_message_references
       DB.exec(<<~SQL)
       UPDATE chat_message_reactions cmr
       SET chat_message_id = mm.new_chat_message_id
@@ -178,11 +219,17 @@ module Chat
     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)
+      DB.exec(
+        <<~SQL,
       UPDATE chat_messages
       SET deleted_at = NOW(), deleted_by_id = :deleted_by_id
       WHERE id IN (:source_message_ids)
+      OR thread_id IN (:source_thread_ids)
     SQL
+        source_message_ids: @source_message_ids,
+        deleted_by_id: @acting_user.id,
+        source_thread_ids: @source_thread_ids,
+      )
       Chat::Publisher.publish_bulk_delete!(@source_channel, @source_message_ids)
     end
 
@@ -210,43 +257,37 @@ module Chat
     SQL
     end
 
+    def update_thread_references(moved_thread_ids)
+      Chat::Thread.transaction do
+        moved_thread_ids.each do |old_thread_id, new_thread_id|
+          thread = Chat::Thread.find(new_thread_id)
+
+          new_original_message_id, new_last_message_id =
+            DB.query_single(<<-SQL, new_thread_id: new_thread_id)
+          SELECT MIN(id), MAX(id)
+          FROM chat_messages
+          WHERE thread_id = :new_thread_id
+        SQL
+
+          thread.update!(
+            original_message_id: new_original_message_id,
+            last_message_id: new_last_message_id,
+          )
+
+          thread.set_replies_count_cache(thread.replies_count)
+        end
+      end
+    end
+
+    def delete_source_threads
+      @source_thread_ids.each do |thread_id|
+        thread = Chat::Thread.find_by(id: thread_id)
+        thread.destroy if thread.present?
+      end
+    end
+
     def update_tracking_state
       ::Chat::Action::ResetUserLastReadChannelMessage.call(@source_message_ids, @source_channel.id)
     end
-
-    def update_thread_references
-      threads_to_update = []
-      @source_messages
-        .select { |message| message.in_thread? }
-        .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 different with the old empty thread at some
-        # point when we add an explicit thread move UI, for now we can just delete it,
-        # since it will not contain any important data.
-        if thread.chat_messages.empty?
-          thread.destroy!
-          next
-        end
-
-        Chat::Thread.transaction do
-          original_message = thread.chat_messages.first
-          new_thread =
-            Chat::Thread.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
   end
 end

plugins/chat/spec/lib/chat/message_mover_spec.rb

@@ -160,15 +160,18 @@ describe Chat::MessageMover do
         message3.update!(thread: thread)
       end
 
-      it "does not preserve thread_ids" do
+      it "creates a new thread_id for the moved messages" do
         move!
         moved_messages =
           Chat::Message
             .where(chat_channel: destination_channel)
             .order("created_at ASC, id ASC")
             .last(3)
+        moved_thread_ids = moved_messages.pluck(:thread_id).uniq
 
-        expect(moved_messages.pluck(:thread_id).uniq).to eq([nil])
+        expect(moved_thread_ids.size).to eq(1)
+        expect(moved_thread_ids.first).not_to be_nil
+        expect(moved_thread_ids.first).not_to eq(thread.id)
       end
 
       it "deletes the empty thread" do
@@ -176,14 +179,6 @@ describe Chat::MessageMover do
         expect(Chat::Thread.exists?(id: thread.id)).to eq(false)
       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
-
       it "updates the tracking to the last non-deleted channel message for users whose last_read_message_id was the moved message" do
         membership_1 =
           Fabricate(
@@ -210,7 +205,7 @@ describe Chat::MessageMover do
       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
+        it "moves the entire thread to the new channel" do
           message4 =
             Fabricate(
               :chat_message,
@@ -226,39 +221,53 @@ describe Chat::MessageMover do
               message: "the fifth message",
               thread: thread,
             )
-          expect { move! }.to change { Chat::Thread.count }.by(1)
+
-          new_thread = Chat::Thread.last
+          expect { move! }.to change {
-          expect(message4.reload.thread_id).to eq(new_thread.id)
+            Chat::Message.where(chat_channel: destination_channel).count
-          expect(message5.reload.thread_id).to eq(new_thread.id)
+          }.by(5)
-          expect(new_thread.channel).to eq(source_channel)
+
-          expect(new_thread.original_message).to eq(message4)
+          moved_messages =
+            Chat::Message
+              .where(chat_channel: destination_channel)
+              .order("created_at ASC, id ASC")
+              .last(5)
+
+          expect(moved_messages.map(&:thread_id).uniq.size).to eq(1)
+          expect(moved_messages.map(&:chat_channel_id).uniq).to eq([destination_channel.id])
         end
       end
 
       context "when multiple thread original messages are moved" do
-        it "works the same as when one is" do
+        it "moves the entire threads to the new channel" 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,
+              in_reply_to: message4,
               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 { Chat::Thread.count }.by(2)
 
-          new_threads = Chat::Thread.order(:created_at).last(2)
+          expect { move!([message1.id, message4.id]) }.to change {
-          expect(message3.reload.thread_id).to eq(new_threads.first.id)
+            Chat::Message.where(chat_channel: destination_channel).count
-          expect(message5.reload.thread_id).to eq(new_threads.second.id)
+          }.by(5)
-          expect(new_threads.first.channel).to eq(source_channel)
+
-          expect(new_threads.second.channel).to eq(source_channel)
+          moved_messages =
-          expect(new_threads.first.original_message).to eq(message2)
+            Chat::Message
-          expect(new_threads.second.original_message).to eq(message5)
+              .where(chat_channel: destination_channel)
+              .order("created_at ASC, id ASC")
+              .last(5)
+
+          moved_thread_ids = moved_messages.map(&:thread_id).uniq
+          expect(moved_thread_ids.size).to eq(2)
+          moved_thread_ids.each do |thread_id|
+            expect(Chat::Thread.find(thread_id).channel_id).to eq(destination_channel.id)
+          end
         end
       end
     end

plugins/chat/spec/system/move_message_to_channel_spec.rb

@@ -3,6 +3,7 @@
 RSpec.describe "Move message to channel", type: :system do
   let(:chat_page) { PageObjects::Pages::Chat.new }
   let(:channel_page) { PageObjects::Pages::ChatChannel.new }
+  let(:thread_page) { PageObjects::Pages::ChatThread.new }
 
   before { chat_system_bootstrap }
 
@@ -64,11 +65,18 @@ RSpec.describe "Move message to channel", type: :system do
     end
 
     context "when category channel" do
-      fab!(:channel_1) { Fabricate(:chat_channel) }
+      fab!(:channel_1) { Fabricate(:chat_channel, threading_enabled: true) }
-      fab!(:channel_2) { Fabricate(:chat_channel) }
+      fab!(:channel_2) { Fabricate(:chat_channel, threading_enabled: true) }
       fab!(:message_1) do
         Fabricate(:chat_message, chat_channel: channel_1, user: current_admin_user)
       end
+      fab!(:thread) do
+        chat_thread_chain_bootstrap(
+          channel: channel_1,
+          users: [Fabricate(:user), Fabricate(:user)],
+          messages_count: 3,
+        )
+      end
 
       before do
         channel_1.add(current_admin_user)
@@ -90,6 +98,63 @@ RSpec.describe "Move message to channel", type: :system do
 
         expect(channel_page.messages).to have_deleted_message(message_1)
       end
+
+      it "moves the thread" do
+        chat_page.visit_channel(channel_1)
+        channel_page.messages.select(thread.original_message)
+        channel_page.selection_management.move
+        find(".chat-modal-move-message-to-channel__channel-chooser").click
+        find("[data-value='#{channel_2.id}']").click
+        click_button(I18n.t("js.chat.move_to_channel.confirm_move"))
+
+        expect(page).to have_current_path(chat.channel_path(channel_2.slug, channel_2.id))
+        expect(channel_page.messages).to have_message(text: thread.original_message.message)
+
+        chat_page.visit_thread(channel_2.threads.first)
+        chat_page.find(".chat-message .chat-message-thread-indicator").click
+        thread.replies.each { |reply| expect(thread_page.messages).to have_message(id: reply.id) }
+
+        chat_page.visit_channel(channel_1)
+
+        expect(channel_page.messages).to have_no_message(text: thread.original_message.message)
+      end
+
+      context "when message has an upload and no text" do
+        fab!(:image) do
+          Fabricate(
+            :upload,
+            original_filename: "image.png",
+            width: 400,
+            height: 300,
+            extension: "png",
+          )
+        end
+        fab!(:message_with_upload) do
+          Fabricate(
+            :chat_message,
+            chat_channel: channel_1,
+            user: current_admin_user,
+            message: "",
+            uploads: [image],
+          )
+        end
+
+        it "moves the message without errors" do
+          chat_page.visit_channel(channel_1)
+          channel_page.messages.select(message_with_upload)
+          channel_page.selection_management.move
+          find(".chat-modal-move-message-to-channel__channel-chooser").click
+          find("[data-value='#{channel_2.id}']").click
+          click_button(I18n.t("js.chat.move_to_channel.confirm_move"))
+
+          expect(page).to have_current_path(chat.channel_path(channel_2.slug, channel_2.id))
+          expect(channel_page.messages).to have_css(".chat-uploads .chat-img-upload")
+
+          chat_page.visit_channel(channel_1)
+
+          expect(channel_page.messages).to have_deleted_message(message_with_upload)
+        end
+      end
     end
   end
 end