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

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: Improved Bookmarks with Reminders - #203 by tshenry - announcements - Discourse Meta

diff --git a/app/models/post_mover.rb b/app/models/post_mover.rb
index 5ac0a41..6c49481 100644
--- a/app/models/post_mover.rb
+++ b/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
diff --git a/db/post_migrate/20210328233843_fix_bookmarks_with_incorrect_topic_id.rb b/db/post_migrate/20210328233843_fix_bookmarks_with_incorrect_topic_id.rb
new file mode 100644
index 0000000..0c735dc
--- /dev/null
+++ b/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
diff --git a/spec/models/post_mover_spec.rb b/spec/models/post_mover_spec.rb
index 018c850..0cf705f 100644
--- a/spec/models/post_mover_spec.rb
+++ b/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) }
 

GitHub sha: 2d686191

This commit appears in #12542 which was approved by lis2. It was merged by martin.