From e4d51e5b0aa072da1460c33b4ebe657ac7225954 Mon Sep 17 00:00:00 2001
From: Krzysztof Kotlarek <kotlarek.krzysztof@gmail.com>
Date: Fri, 4 Dec 2020 08:43:42 +1100
Subject: [PATCH] FIX: correct link in the notification about moved post
 (#11399)

Notification is created by a job. If the job is evaluated before changes are committed to a database, a notification will have an incorrect URL.

Therefore, the job should be lodged in enqueue_jobs method which is triggered after the transaction:

```ruby
Topic.transaction do
  move_posts_to topic
end
add_allowed_users(participants) if participants.present? && @move_to_pm
enqueue_jobs(topic)
```

I improved a little bit specs to ensure that the destination topic_id is set. However, that tests are passing even without code improvements. I couldn't find an easy way to "delay" database transaction.

Meta: https://meta.discourse.org/t/bug-with-notifications-for-moved-posts/168937
---
 app/models/post_mover.rb       | 21 +++++++--------------
 spec/models/post_mover_spec.rb |  5 ++++-
 2 files changed, 11 insertions(+), 15 deletions(-)

diff --git a/app/models/post_mover.rb b/app/models/post_mover.rb
index 184fd3b9ee..5ac0a416a5 100644
--- a/app/models/post_mover.rb
+++ b/app/models/post_mover.rb
@@ -71,7 +71,7 @@ class PostMover
     create_temp_table
     delete_invalid_post_timings
     move_each_post
-    notify_users_that_posts_have_moved
+    create_moderator_post_in_original_topic
     update_statistics
     update_user_actions
     update_last_post_stats
@@ -438,19 +438,6 @@ class PostMover
     UserAction.synchronize_target_topic_ids(posts.map(&:id))
   end
 
-  def notify_users_that_posts_have_moved
-    enqueue_notification_job
-    create_moderator_post_in_original_topic
-  end
-
-  def enqueue_notification_job
-    Jobs.enqueue(
-      :notify_moved_posts,
-      post_ids: post_ids,
-      moved_by_id: user.id
-    )
-  end
-
   def create_moderator_post_in_original_topic
     move_type_str = PostMover.move_types[@move_type].to_s
     move_type_str.sub!("topic", "message") if @move_to_pm
@@ -538,6 +525,12 @@ class PostMover
   def enqueue_jobs(topic)
     @post_creator.enqueue_jobs if @post_creator
 
+    Jobs.enqueue(
+      :notify_moved_posts,
+      post_ids: post_ids,
+      moved_by_id: user.id
+    )
+
     Jobs.enqueue(
       :delete_inaccessible_notifications,
       topic_id: topic.id
diff --git a/spec/models/post_mover_spec.rb b/spec/models/post_mover_spec.rb
index aebfefe12d..018c850a3e 100644
--- a/spec/models/post_mover_spec.rb
+++ b/spec/models/post_mover_spec.rb
@@ -75,6 +75,7 @@ describe PostMover do
           notification = p2.user.notifications.where(notification_type: Notification.types[:moved_post]).first
 
           expect(notification.topic_id).to eq(p2.topic_id)
+          expect(notification.topic_id).not_to eq(old_topic_id)
           expect(notification.post_number).to eq(1)
 
           # no message for person who made the move
@@ -84,6 +85,7 @@ describe PostMover do
           notification = p6.user.notifications.where(notification_type: Notification.types[:moved_post]).first
 
           expect(notification.topic_id).to eq(p2.topic_id)
+          expect(notification.topic_id).not_to eq(old_topic_id)
 
           # this is the 3rd post we moved
           expect(notification.post_number).to eq(3)
@@ -541,7 +543,7 @@ describe PostMover do
             # Should notify correctly
             notification = p2.user.notifications.where(notification_type: Notification.types[:moved_post]).first
 
-            expect(notification.topic_id).to eq(p2.topic_id)
+            expect(notification.topic_id).to eq(destination_topic.id)
             expect(notification.post_number).to eq(p2.post_number)
 
             # Should update last reads
@@ -1145,6 +1147,7 @@ describe PostMover do
           notification = p2.user.notifications.where(notification_type: Notification.types[:moved_post]).first
 
           expect(notification.topic_id).to eq(p2.topic_id)
+          expect(notification.topic_id).not_to eq(old_message_id)
           expect(notification.post_number).to eq(1)
 
           # no message for person who made the move