From 9b8af0ea9fdd8e82e8c1dc4fedcc754bff8830c9 Mon Sep 17 00:00:00 2001 From: Mark VanLandingham Date: Tue, 12 Nov 2024 14:35:20 -0600 Subject: [PATCH] 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 | 34 +++++++++++++++ app/models/post.rb | 9 ++++ app/models/post_mover.rb | 28 +++---------- app/models/topic.rb | 11 ++++- .../20241108154026_create_moved_posts.rb | 17 ++++++++ spec/fabricators/moved_post_fabricator.rb | 15 +++++++ spec/models/moved_post_spec.rb | 41 +++++++++++++++++++ spec/models/post_mover_spec.rb | 39 ++++++++++++++++++ 8 files changed, 171 insertions(+), 23 deletions(-) create mode 100644 app/models/moved_post.rb create mode 100644 db/migrate/20241108154026_create_moved_posts.rb create mode 100644 spec/fabricators/moved_post_fabricator.rb create mode 100644 spec/models/moved_post_spec.rb diff --git a/app/models/moved_post.rb b/app/models/moved_post.rb new file mode 100644 index 00000000000..60cda5a44b2 --- /dev/null +++ b/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) +# diff --git a/app/models/post.rb b/app/models/post.rb index dfb57dc5d32..066d618fea1 100644 --- a/app/models/post.rb +++ b/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" diff --git a/app/models/post_mover.rb b/app/models/post_mover.rb index 364de8321ca..17740fbc6f1 100644 --- a/app/models/post_mover.rb +++ b/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 diff --git a/app/models/topic.rb b/app/models/topic.rb index f17c7eecf53..2a72df4a541 100644 --- a/app/models/topic.rb +++ b/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 @@ -927,7 +936,7 @@ class Topic < ActiveRecord::Base WHERE topics.archetype = 'private_message' AND X.topic_id = topics.id AND - Y.topic_id = topics.id AND + Y.topic_id = topics.id AND Z.topic_id = topics.id AND ( topics.highest_staff_post_number <> X.highest_post_number OR topics.highest_post_number <> Y.highest_post_number OR diff --git a/db/migrate/20241108154026_create_moved_posts.rb b/db/migrate/20241108154026_create_moved_posts.rb new file mode 100644 index 00000000000..06acfc0d90f --- /dev/null +++ b/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 diff --git a/spec/fabricators/moved_post_fabricator.rb b/spec/fabricators/moved_post_fabricator.rb new file mode 100644 index 00000000000..f210901564a --- /dev/null +++ b/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 diff --git a/spec/models/moved_post_spec.rb b/spec/models/moved_post_spec.rb new file mode 100644 index 00000000000..8a34a3e7f75 --- /dev/null +++ b/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 diff --git a/spec/models/post_mover_spec.rb b/spec/models/post_mover_spec.rb index e2acbf65e56..e46afd4d3dd 100644 --- a/spec/models/post_mover_spec.rb +++ b/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