FIX: When moving first posts between topics, ensure only relevant timings are moved (#30217)

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)
       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, post_ids: @post_ids_after_move)
-
-    DB.exec(<<~SQL, params)
       UPDATE post_timings pt
       SET topic_id    = mp.new_topic_id,
           post_number = mp.new_post_number

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