From 28a1463baf7956090aba11cd3d48250d220d134e Mon Sep 17 00:00:00 2001 From: Gary Pendergast Date: Wed, 11 Dec 2024 17:03:47 +1100 Subject: [PATCH] FIX: When moving first posts between topics, ensure only relevant timings are moved (#30217) --- app/models/post_mover.rb | 13 +++--- spec/models/post_mover_spec.rb | 80 ++++++++++++++++++++++++++++++++-- 2 files changed, 82 insertions(+), 11 deletions(-) diff --git a/app/models/post_mover.rb b/app/models/post_mover.rb index bb846b79036..555b460a4ae 100644 --- a/app/models/post_mover.rb +++ b/app/models/post_mover.rb @@ -461,7 +461,7 @@ class PostMover # copy post_timings for shifted posts to a temp table using the new_post_number # they'll be copied back after delete_invalid_post_timings makes room for them - DB.exec <<~SQL + DB.exec(<<~SQL, post_ids: @post_ids_after_move) CREATE TEMPORARY TABLE temp_post_timings ON COMMIT DROP AS ( SELECT pt.topic_id, mp.new_post_number as post_number, pt.user_id, pt.msecs @@ -477,17 +477,18 @@ class PostMover def copy_shifted_post_timings_from_temp DB.exec <<~SQL INSERT INTO post_timings (topic_id, user_id, post_number, msecs) - SELECT topic_id, user_id, post_number, msecs FROM temp_post_timings + SELECT DISTINCT topic_id, user_id, post_number, msecs FROM temp_post_timings SQL end def copy_first_post_timings - DB.exec <<~SQL + DB.exec(<<~SQL, post_ids: @post_ids_after_move) INSERT INTO post_timings (topic_id, user_id, post_number, msecs) SELECT mp.new_topic_id, pt.user_id, mp.new_post_number, pt.msecs FROM post_timings pt - JOIN moved_posts mp ON (pt.topic_id = mp.old_topic_id AND pt.post_number = mp.old_post_number) + JOIN moved_posts mp ON (pt.topic_id = mp.old_topic_id AND pt.post_number = mp.old_post_number) WHERE mp.old_post_id <> mp.new_post_id + AND mp.old_post_id IN (:post_ids) ON CONFLICT (topic_id, post_number, user_id) DO UPDATE SET msecs = GREATEST(post_timings.msecs, excluded.msecs) SQL @@ -504,9 +505,7 @@ class PostMover end def move_post_timings - params = { post_ids: @post_ids_after_move } - - DB.exec(<<~SQL, params) + DB.exec(<<~SQL, post_ids: @post_ids_after_move) UPDATE post_timings pt SET topic_id = mp.new_topic_id, post_number = mp.new_post_number diff --git a/spec/models/post_mover_spec.rb b/spec/models/post_mover_spec.rb index 3bbf915067e..9002006eb2d 100644 --- a/spec/models/post_mover_spec.rb +++ b/spec/models/post_mover_spec.rb @@ -2008,18 +2008,18 @@ RSpec.describe PostMover do it "allows moving posts from multiple topics into one existing topic" do dest_topic = Fabricate(:topic, user: user, created_at: 5.hours.ago) Fabricate(:post, topic: dest_topic, created_at: 5.hours.ago) - create_post_timing(dest_topic.first_post, user, 500) + create_post_timing(dest_topic.first_post, user, 100) source_1_topic = Fabricate(:topic, user: user, created_at: 4.hours.ago) Fabricate(:post, topic: source_1_topic, user: user, created_at: 4.hours.ago) - create_post_timing(source_1_topic.first_post, user, 500) + create_post_timing(source_1_topic.first_post, user, 200) source_1_post = Fabricate(:post, topic: source_1_topic, user: user, created_at: 3.hours.ago) - create_post_timing(source_1_topic.posts.second, user, 500) + create_post_timing(source_1_topic.posts.second, user, 300) source_2_topic = Fabricate(:topic, user: user, created_at: 2.hours.ago) Fabricate(:post, topic: source_2_topic, user: user, created_at: 2.hours.ago) - create_post_timing(source_2_topic.first_post, user, 500) + create_post_timing(source_2_topic.first_post, user, 400) source_2_post = Fabricate(:post, topic: source_2_topic, user: user, created_at: 1.hours.ago) create_post_timing(source_2_topic.posts.second, user, 500) @@ -2044,6 +2044,78 @@ RSpec.describe PostMover do ) expect(moved_to_too).to be_present + + expect( + PostTiming.where(topic_id: dest_topic.id).pluck(:post_number, :msecs), + ).to contain_exactly([1, 100], [2, 300], [3, 500]) + end + + it "handles moving two older first posts into a newer destination" do + source_1_topic = Fabricate(:topic, user: user, created_at: 5.hours.ago) + Fabricate(:post, topic: source_1_topic, user: user, created_at: 5.hours.ago) + create_post_timing(source_1_topic.first_post, user, 500) + + source_2_topic = Fabricate(:topic, user: user, created_at: 3.hours.ago) + Fabricate(:post, topic: source_2_topic, user: user, created_at: 3.hours.ago) + create_post_timing(source_2_topic.first_post, user, 500) + + dest_topic = Fabricate(:topic, user: user, created_at: 2.hours.ago) + Fabricate(:post, topic: dest_topic, created_at: 2.hours.ago) + create_post_timing(dest_topic.first_post, user, 500) + + moved_to = + source_2_topic.move_posts( + user, + [source_2_topic.first_post.id], + destination_topic_id: dest_topic.id, + chronological_order: true, + ) + + expect(moved_to).to be_present + + moved_to_too = + source_1_topic.move_posts( + user, + [source_1_topic.first_post.id], + destination_topic_id: dest_topic.id, + chronological_order: true, + ) + + expect(moved_to_too).to be_present + end + + it "handles moving an older second post, then the first post, into a newer destination" do + source_1_topic = Fabricate(:topic, user: user, created_at: 5.hours.ago) + Fabricate(:post, topic: source_1_topic, user: user, created_at: 5.hours.ago) + create_post_timing(source_1_topic.first_post, user, 400) + + source_1_post = + Fabricate(:post, topic: source_1_topic, user: user, created_at: 4.hours.ago) + create_post_timing(source_1_post, user, 500) + + dest_topic = dest_topic = Fabricate(:topic, user: user, created_at: 3.hours.ago) + Fabricate(:post, topic: dest_topic, created_at: 3.hours.ago) + create_post_timing(dest_topic.first_post, user, 600) + + moved_to = + source_1_topic.move_posts( + user, + [source_1_post.id], + destination_topic_id: dest_topic.id, + chronological_order: true, + ) + + expect(moved_to).to be_present + + moved_to_too = + source_1_topic.move_posts( + user, + [source_1_topic.first_post.id], + destination_topic_id: dest_topic.id, + chronological_order: true, + ) + + expect(moved_to_too).to be_present end context "with read state and other stats per user" do