DEV: Create permanent version of moved_posts table from PostMover class (#29664)

This is a very simple change, which creates a permanent table in the DB, rather than generating a temporary table when moving posts. This change is about capturing data and any usage will appear in a follow-up.

I did include a new column created_new_topic in the new table, so that it can be easily audited without having to compare destination topic created_at with moved_post records.

app/models/moved_post.rb

@@ -0,0 +1,34 @@
+# frozen_string_literal: true
+
+class MovedPost < ActiveRecord::Base
+  belongs_to :old_topic, class_name: "Topic", foreign_key: :old_topic_id
+  belongs_to :old_post, class_name: "Post", foreign_key: :old_post_id
+
+  belongs_to :new_topic, class_name: "Topic", foreign_key: :new_topic_id
+  belongs_to :new_post, class_name: "Post", foreign_key: :new_post_id
+end
+
+# == Schema Information
+#
+# Table name: moved_posts
+#
+#  id                :bigint           not null, primary key
+#  old_topic_id      :bigint           not null
+#  old_post_id       :bigint           not null
+#  old_post_number   :bigint           not null
+#  new_topic_id      :bigint           not null
+#  new_topic_title   :string           not null
+#  new_post_id       :bigint           not null
+#  new_post_number   :bigint           not null
+#  created_new_topic :boolean          default(FALSE), not null
+#  created_at        :datetime         not null
+#  updated_at        :datetime         not null
+#
+# Indexes
+#
+#  index_moved_posts_on_new_post_id      (new_post_id)
+#  index_moved_posts_on_new_topic_id     (new_topic_id)
+#  index_moved_posts_on_old_post_id      (old_post_id)
+#  index_moved_posts_on_old_post_number  (old_post_number)
+#  index_moved_posts_on_old_topic_id     (old_topic_id)
+#

app/models/post.rb

@@ -56,6 +56,15 @@ class Post < ActiveRecord::Base
   has_many :post_revisions
   has_many :revisions, -> { order(:number) }, foreign_key: :post_id, class_name: "PostRevision"
 
+  has_many :moved_posts_as_old_post,
+           class_name: "MovedPost",
+           foreign_key: :old_post_id,
+           dependent: :destroy
+  has_many :moved_posts_as_new_post,
+           class_name: "MovedPost",
+           foreign_key: :new_post_id,
+           dependent: :destroy
+
   has_many :user_actions, foreign_key: :target_post_id
 
   belongs_to :image_upload, class_name: "Upload"

app/models/post_mover.rb

@@ -16,6 +16,7 @@ class PostMover
 
   def to_topic(id, participants: nil, chronological_order: false)
     @move_type = PostMover.move_types[:existing_topic]
+    @creating_new_topic = false
     @chronological_order = chronological_order
 
     topic = Topic.find_by_id(id)
@@ -32,6 +33,7 @@ class PostMover
 
   def to_new_topic(title, category_id = nil, tags = nil)
     @move_type = PostMover.move_types[:new_topic]
+    @creating_new_topic = true
 
     post = Post.find_by(id: post_ids.first)
     raise Discourse::InvalidParameters unless post
@@ -88,7 +90,6 @@ class PostMover
     @first_post_number_moved =
       posts.first.is_first_post? ? posts[1]&.post_number : posts.first.post_number
 
-    create_temp_table
     move_each_post
     handle_moved_references
 
@@ -105,25 +106,6 @@ class PostMover
     destination_topic
   end
 
-  def create_temp_table
-    DB.exec("DROP TABLE IF EXISTS moved_posts") if Rails.env.test?
-
-    DB.exec <<~SQL
-      CREATE TEMPORARY TABLE moved_posts (
-        old_topic_id INTEGER,
-        old_post_id INTEGER,
-        old_post_number INTEGER,
-        new_topic_id INTEGER,
-        new_topic_title VARCHAR,
-        new_post_id INTEGER,
-        new_post_number INTEGER
-      ) ON COMMIT DROP;
-
-      CREATE INDEX moved_posts_old_post_number ON moved_posts(old_post_number);
-      CREATE INDEX moved_posts_old_post_id ON moved_posts(old_post_id);
-    SQL
-  end
-
   def handle_moved_references
     move_incoming_emails
     move_notifications
@@ -340,10 +322,12 @@ class PostMover
 
   def store_movement(metadata, new_post)
     metadata[:new_post_id] = new_post.id
+    metadata[:now] = Time.zone.now
+    metadata[:created_new_topic] = @creating_new_topic
 
     DB.exec(<<~SQL, metadata)
-      INSERT INTO moved_posts(old_topic_id, old_post_id, old_post_number, new_topic_id, new_topic_title, new_post_id, new_post_number)
-      VALUES (:old_topic_id, :old_post_id, :old_post_number, :new_topic_id, :new_topic_title, :new_post_id, :new_post_number)
+      INSERT INTO moved_posts(old_topic_id, old_post_id, old_post_number, new_topic_id, new_topic_title, new_post_id, new_post_number, created_new_topic, created_at, updated_at)
+      VALUES (:old_topic_id, :old_post_id, :old_post_number, :new_topic_id, :new_topic_title, :new_post_id, :new_post_number, :created_new_topic, :now, :now)
     SQL
   end
 

app/models/topic.rb

@@ -276,6 +276,15 @@ class Topic < ActiveRecord::Base
   has_many :tags, through: :topic_tags, dependent: :destroy # dependent destroy applies to the topic_tags records
   has_many :tag_users, through: :tags
 
+  has_many :moved_posts_as_old_topic,
+           class_name: "MovedPost",
+           foreign_key: :old_topic_id,
+           dependent: :destroy
+  has_many :moved_posts_as_new_topic,
+           class_name: "MovedPost",
+           foreign_key: :new_topic_id,
+           dependent: :destroy
+
   has_one :top_topic
   has_one :shared_draft, dependent: :destroy
   has_one :published_page

db/migrate/20241108154026_create_moved_posts.rb

@@ -0,0 +1,17 @@
+# frozen_string_literal: true
+
+class CreateMovedPosts < ActiveRecord::Migration[7.1]
+  def change
+    create_table :moved_posts do |t|
+      t.bigint :old_topic_id, null: false, index: true
+      t.bigint :old_post_id, null: false, index: true
+      t.bigint :old_post_number, null: false, index: true
+      t.bigint :new_topic_id, null: false, index: true
+      t.string :new_topic_title, null: false
+      t.bigint :new_post_id, null: false, index: true
+      t.bigint :new_post_number, null: false
+      t.boolean :created_new_topic, null: false, default: false
+      t.timestamps
+    end
+  end
+end

spec/fabricators/moved_post_fabricator.rb

@@ -0,0 +1,15 @@
+# frozen_string_literal: true
+
+Fabricator(:moved_post) do
+  created_new_topic false
+  new_topic { Fabricate(:topic) }
+  new_post { Fabricate(:post) }
+  old_topic { Fabricate(:topic) }
+  old_post { Fabricate(:post) }
+
+  after_build do |moved_post, transients|
+    moved_post.new_topic_title = moved_post.new_topic.title
+    moved_post.new_post_number = moved_post.new_post.post_number
+    moved_post.old_post_number = moved_post.old_post.post_number
+  end
+end

spec/models/moved_post_spec.rb

@@ -0,0 +1,41 @@
+# frozen_string_literal: true
+
+RSpec.describe MovedPost do
+  fab!(:new_topic) { Fabricate(:topic) }
+  fab!(:new_post) { Fabricate(:post, topic: new_topic) }
+
+  fab!(:old_topic) { Fabricate(:topic) }
+  fab!(:old_post) { Fabricate(:post, topic: old_topic) }
+
+  fab!(:moved_post) do
+    Fabricate(
+      :moved_post,
+      new_topic: new_topic,
+      new_post: new_post,
+      old_topic: old_topic,
+      old_post: old_post,
+    )
+  end
+
+  describe "Topic & Post associations" do
+    it "deletes the MovePost record when new_topic is deleted" do
+      new_topic.destroy
+      expect { moved_post.reload }.to raise_exception(ActiveRecord::RecordNotFound)
+    end
+
+    it "deletes the MovePost record when old_topic is deleted" do
+      old_topic.destroy
+      expect { moved_post.reload }.to raise_exception(ActiveRecord::RecordNotFound)
+    end
+
+    it "deletes the MovePost record when new_post is deleted" do
+      new_post.destroy
+      expect { moved_post.reload }.to raise_exception(ActiveRecord::RecordNotFound)
+    end
+
+    it "deletes the MovePost record when old_post is deleted" do
+      old_post.destroy
+      expect { moved_post.reload }.to raise_exception(ActiveRecord::RecordNotFound)
+    end
+  end
+end

spec/models/post_mover_spec.rb

@@ -308,6 +308,26 @@ RSpec.describe PostMover do
               ),
             ).to eq(true)
             expect(TopicUser.exists?(user_id: user, topic_id: new_topic.id)).to eq(true)
+
+            # moved_post records are created correctly
+            expect(
+              MovedPost.exists?(
+                new_topic: new_topic,
+                new_post_id: p2.id,
+                old_topic: topic,
+                old_post_id: p2.id,
+                created_new_topic: true,
+              ),
+            ).to eq(true)
+            expect(
+              MovedPost.exists?(
+                new_topic: new_topic,
+                new_post_id: p4.id,
+                old_topic: topic,
+                old_post_id: p4.id,
+                created_new_topic: true,
+              ),
+            ).to eq(true)
           end
 
           it "moving all posts will close the topic" do
@@ -670,6 +690,25 @@ RSpec.describe PostMover do
             expect(
               TopicUser.find_by(user_id: user.id, topic_id: topic.id).last_read_post_number,
             ).to eq(p3.post_number)
+
+            expect(
+              MovedPost.exists?(
+                new_topic: destination_topic,
+                new_post_id: p2.id,
+                old_topic: topic,
+                old_post_id: p2.id,
+                created_new_topic: false,
+              ),
+            ).to eq(true)
+            expect(
+              MovedPost.exists?(
+                new_topic: destination_topic,
+                new_post_id: p4.id,
+                old_topic: topic,
+                old_post_id: p4.id,
+                created_new_topic: false,
+              ),
+            ).to eq(true)
           end
 
           it "moving all posts will close the topic" do