FIX: Bookmark topics were not being updated when the post moved (#12542)

Because bookmarks have both topic and post ID, when the post was moved into another topic the bookmark was still attached to the post but did not show in the UI. This PR makes it so the all topic IDs for bookmarks attached to a post are updated when a post is moved.

Also included is a migration to fix affected records (e.g. on Meta there are 20 affected records).

See: https://meta.discourse.org/t/improved-bookmarks-with-reminders/144542/203

app/models/post_mover.rb

@@ -76,6 +76,7 @@ class PostMover
     update_user_actions
     update_last_post_stats
     update_upload_security_status
+    update_bookmarks
 
     if moving_all_posts
       @original_topic.update_status('closed', true, @user)
@@ -491,6 +492,10 @@ class PostMover
     end
   end
 
+  def update_bookmarks
+    Bookmark.where(post_id: post_ids).update_all(topic_id: @destination_topic.id)
+  end
+
   def watch_new_topic
     if @destination_topic.archetype == Archetype.private_message
       if @original_topic.archetype == Archetype.private_message

db/post_migrate/20210328233843_fix_bookmarks_with_incorrect_topic_id.rb

@@ -0,0 +1,22 @@
+# frozen_string_literal: true
+
+class FixBookmarksWithIncorrectTopicId < ActiveRecord::Migration[6.0]
+  def up
+    result = DB.exec(<<~SQL)
+      UPDATE bookmarks bm
+      SET topic_id = subquery.correct_topic_id, updated_at = NOW()
+      FROM (
+        SELECT bookmarks.id AS bookmark_id, bookmarks.post_id, bookmarks.topic_id,
+               posts.topic_id AS correct_topic_id
+        FROM bookmarks
+        INNER JOIN posts ON posts.id = bookmarks.post_id
+        WHERE posts.topic_id != bookmarks.topic_id
+      ) AS subquery
+      WHERE bm.id = subquery.bookmark_id;
+    SQL
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end

spec/models/post_mover_spec.rb

@@ -385,6 +385,20 @@ describe PostMover do
               .to contain_exactly([1, 500], [2, 750])
           end
 
+          it "updates bookmark topic_ids to the new topic id and does not affect bookmarks for posts not moving" do
+            some_user = Fabricate(:user)
+            new_post = Fabricate(:post, topic: p1.topic)
+            bookmark1 = Fabricate(:bookmark, topic: p1.topic, post: p1, user: some_user)
+            bookmark2 = Fabricate(:bookmark, topic: p4.topic, post: p4)
+            bookmark3 = Fabricate(:bookmark, topic: p4.topic, post: p4)
+            bookmark4 = Fabricate(:bookmark, topic: new_post.topic, post: new_post)
+            new_topic = topic.move_posts(user, [p1.id, p4.id], title: "new testing topic name")
+            expect(bookmark1.reload.topic_id).to eq(new_topic.id)
+            expect(bookmark2.reload.topic_id).to eq(new_topic.id)
+            expect(bookmark3.reload.topic_id).to eq(new_topic.id)
+            expect(bookmark4.reload.topic_id).to eq(new_post.topic_id)
+          end
+
           context "read state and other stats per user" do
             def create_topic_user(user, opts = {})
               notification_level = opts.delete(:notification_level) || :regular
@@ -600,6 +614,20 @@ describe PostMover do
             expect(Notification.exists?(admin_notification.id)).to eq(true)
           end
 
+          it "updates bookmark topic_ids to the new topic id and does not affect bookmarks for posts not moving" do
+            some_user = Fabricate(:user)
+            new_post = Fabricate(:post, topic: p3.topic)
+            bookmark1 = Fabricate(:bookmark, topic: p3.topic, post: p3, user: some_user)
+            bookmark2 = Fabricate(:bookmark, topic: p3.topic, post: p3)
+            bookmark3 = Fabricate(:bookmark, topic: p3.topic, post: p3)
+            bookmark4 = Fabricate(:bookmark, topic: new_post.topic, post: new_post)
+            topic.move_posts(user, [p3.id], destination_topic_id: destination_topic.id)
+            expect(bookmark1.reload.topic_id).to eq(destination_topic.id)
+            expect(bookmark2.reload.topic_id).to eq(destination_topic.id)
+            expect(bookmark3.reload.topic_id).to eq(destination_topic.id)
+            expect(bookmark4.reload.topic_id).to eq(new_post.topic_id)
+          end
+
           context "post timings" do
             fab!(:some_user) { Fabricate(:user) }