FIX: Old notifications didn't link to correct post after moving post

FIX: Old notifications didn’t link to correct post after moving post

diff --git a/app/models/post_mover.rb b/app/models/post_mover.rb
index df801a0..bac6992 100644
--- a/app/models/post_mover.rb
+++ b/app/models/post_mover.rb
@@ -108,6 +108,8 @@ class PostMover
   end
 
   def create_first_post(post)
+    old_post_attributes = post_attributes(post)
+
     new_post = PostCreator.create(
       post.user,
       raw: post.raw,
@@ -123,6 +125,7 @@ class PostMover
 
     move_incoming_emails(post, new_post)
     move_email_logs(post, new_post)
+    move_notifications(old_post_attributes, new_post)
 
     PostAction.copy(post, new_post)
     new_post.update_column(:reply_count, @reply_count[1] || 0)
@@ -149,11 +152,12 @@ class PostMover
       update[:reply_to_user_id] = nil
     end
 
+    old_post_attributes = post_attributes(post)
     post.attributes = update
     post.save(validate: false)
 
     move_incoming_emails(post, post)
-    move_email_logs(post, post)
+    move_notifications(old_post_attributes, post)
 
     DiscourseEvent.trigger(:post_moved, post, original_topic.id)
 
@@ -175,6 +179,31 @@ class PostMover
       .update_all(post_id: new_post.id)
   end
 
+  def move_notifications(old_post_attributes, new_post)
+    params = {
+      old_topic_id: old_post_attributes[:topic_id],
+      old_post_number: old_post_attributes[:post_number],
+      new_topic_id: new_post.topic_id,
+      new_post_number: new_post.post_number,
+      new_topic_title: new_post.topic.title
+    }
+
+    DB.exec(<<~SQL, params)
+      UPDATE notifications
+      SET topic_id  = :new_topic_id,
+        post_number = :new_post_number,
+        data        = (data :: JSONB ||
+          jsonb_strip_nulls(
+              jsonb_build_object(
+                  'topic_title', CASE WHEN data :: JSONB ->> 'topic_title' IS NULL
+                                        THEN NULL
+                                      ELSE :new_topic_title END
+                )
+            )) :: JSON
+      WHERE topic_id = :old_topic_id AND post_number = :old_post_number
+    SQL
+  end
+
   def update_statistics
     destination_topic.update_statistics
     original_topic.update_statistics
@@ -276,4 +305,11 @@ class PostMover
     end
     destination_topic.save!
   end
+
+  def post_attributes(post)
+    {
+      topic_id: post.topic_id,
+      post_number: post.post_number
+    }
+  end
 end
diff --git a/spec/fabricators/notification_fabricator.rb b/spec/fabricators/notification_fabricator.rb
index da533b9..2d22ed3 100644
--- a/spec/fabricators/notification_fabricator.rb
+++ b/spec/fabricators/notification_fabricator.rb
@@ -5,6 +5,7 @@ Fabricator(:notification) do
   notification_type Notification.types[:mentioned]
   user
   topic { |attrs| attrs[:post]&.topic || Fabricate(:topic, user: attrs[:user]) }
+  post_number { |attrs| attrs[:post]&.post_number }
   data '{"poison":"ivy","killer":"croc"}'
 end
 
@@ -57,3 +58,17 @@ Fabricator(:posted_notification, from: :notification) do
     }.to_json
   end
 end
+
+Fabricator(:mentioned_notification, from: :notification) do
+  notification_type Notification.types[:mentioned]
+  data do |attrs|
+    {
+      topic_title: attrs[:topic].title,
+      original_post_id: attrs[:post].id,
+      original_post_type: attrs[:post].post_type,
+      original_username: attrs[:post].user.username,
+      revision_number: nil,
+      display_username: attrs[:post].user.username
+    }.to_json
+  end
+end
diff --git a/spec/models/post_mover_spec.rb b/spec/models/post_mover_spec.rb
index d1afb37..833705e 100644
--- a/spec/models/post_mover_spec.rb
+++ b/spec/models/post_mover_spec.rb
@@ -292,6 +292,22 @@ describe PostMover do
               notifications_reason_id: TopicUser.notification_reasons[:created_topic]
             )).to eq(true)
           end
+
+          it "updates existing notifications" do
+            n3 = Fabricate(:mentioned_notification, post: p3, user: another_user)
+            n4 = Fabricate(:mentioned_notification, post: p4, user: another_user)
+
+            new_topic = topic.move_posts(user, [p3.id], title: "new testing topic name")
+
+            n3.reload
+            expect(n3.topic_id).to eq(new_topic.id)
+            expect(n3.post_number).to eq(1)
+            expect(n3.data_hash[:topic_title]).to eq(new_topic.title)
+
+            n4.reload
+            expect(n4.topic_id).to eq(topic.id)
+            expect(n4.post_number).to eq(4)
+          end
         end
 
         context "to an existing topic" do
@@ -369,6 +385,22 @@ describe PostMover do
             moderator_post = topic.posts.find_by(post_number: 2)
             expect(moderator_post.raw).to include("4 posts were merged")
           end
+
+          it "updates existing notifications" do
+            n3 = Fabricate(:mentioned_notification, post: p3, user: another_user)
+            n4 = Fabricate(:mentioned_notification, post: p4, user: another_user)
+
+            moved_to = topic.move_posts(user, [p3.id], destination_topic_id: destination_topic.id)
+
+            n3.reload
+            expect(n3.topic_id).to eq(moved_to.id)
+            expect(n3.post_number).to eq(2)
+            expect(n3.data_hash[:topic_title]).to eq(moved_to.title)
+
+            n4.reload
+            expect(n4.topic_id).to eq(topic.id)
+            expect(n4.post_number).to eq(4)
+          end
         end
 
         context "to a message" do

GitHub sha: 1235105c

1 Like