FIX: Topic user bookmarked column is out of sync after post moves (#12612)

When posts are moved from one topic to another, the `topic_user.bookmarked` column for all users in the new and the old topic needs to be resynced, for example because a user bookmarks post 12 in topic 1, then it is moved to topic 2, the topic_user record for topic 1 should no longer be bookmarked. A background job has been added to sync the column for a specified topic, or for no topic at all, which does it for all topics like the migration.

Also includes a migration that we have run in the past to fix bad data.

----

This has been addressed in other places in the past:

https://github.com/discourse/discourse/pull/10211
https://github.com/discourse/discourse/pull/10188

app/jobs/regular/sync_topic_user_bookmarked.rb

@@ -0,0 +1,27 @@
+# frozen_string_literal: true
+
+module Jobs
+  class SyncTopicUserBookmarked < ::Jobs::Base
+    def execute(args = {})
+      topic_id = args[:topic_id]
+
+      DB.exec(<<~SQL, topic_id: topic_id)
+        UPDATE topic_users SET bookmarked = true
+        FROM bookmarks AS b
+        WHERE NOT topic_users.bookmarked AND
+          topic_users.topic_id = b.topic_id AND
+          topic_users.user_id = b.user_id #{topic_id.present? ? "AND topic_users.topic_id = :topic_id" : ""}
+      SQL
+
+      DB.exec(<<~SQL, topic_id: topic_id)
+        UPDATE topic_users SET bookmarked = false
+        WHERE topic_users.bookmarked AND
+          (
+            SELECT COUNT(*) FROM bookmarks
+            WHERE topic_id = topic_users.topic_id
+            AND user_id = topic_users.user_id
+        ) = 0 #{topic_id.present? ? "AND topic_users.topic_id = :topic_id" : ""}
+      SQL
+    end
+  end
+end

app/models/post_mover.rb

@@ -494,6 +494,11 @@ class PostMover
 
   def update_bookmarks
     Bookmark.where(post_id: post_ids).update_all(topic_id: @destination_topic.id)
+
+    DB.after_commit do
+      Jobs.enqueue(:sync_topic_user_bookmarked, topic_id: @original_topic.id)
+      Jobs.enqueue(:sync_topic_user_bookmarked, topic_id: @destination_topic.id)
+    end
   end
 
   def watch_new_topic

db/migrate/20210406060434_fix_topic_user_bookmarked_sync_issues_again.rb

@@ -0,0 +1,26 @@
+# frozen_string_literal: true
+
+class FixTopicUserBookmarkedSyncIssuesAgain < ActiveRecord::Migration[6.0]
+  disable_ddl_transaction!
+
+  def up
+    DB.exec(
+      <<~SQL
+        UPDATE topic_users SET bookmarked = true
+        FROM bookmarks AS b
+        WHERE NOT topic_users.bookmarked AND topic_users.topic_id = b.topic_id AND topic_users.user_id = b.user_id
+      SQL
+    )
+
+    DB.exec(
+      <<~SQL
+        UPDATE topic_users SET bookmarked = false
+        WHERE topic_users.bookmarked AND (SELECT COUNT(*) FROM bookmarks WHERE topic_id = topic_users.topic_id AND user_id = topic_users.user_id) = 0
+      SQL
+    )
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end

spec/jobs/sync_topic_user_bookmarked_spec.rb

@@ -0,0 +1,53 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+RSpec.describe Jobs::SyncTopicUserBookmarked do
+  it "corrects all topic_users.bookmarked records for the topic" do
+    topic = Fabricate(:topic)
+    Fabricate(:post, topic: topic)
+    Fabricate(:post, topic: topic)
+    Fabricate(:post, topic: topic)
+
+    tu1 = Fabricate(:topic_user, topic: topic, bookmarked: false)
+    tu2 = Fabricate(:topic_user, topic: topic, bookmarked: false)
+    tu3 = Fabricate(:topic_user, topic: topic, bookmarked: true)
+    tu4 = Fabricate(:topic_user, topic: topic, bookmarked: true)
+    tu5 = Fabricate(:topic_user, bookmarked: false)
+
+    Fabricate(:bookmark, user: tu1.user, topic: topic, post: topic.posts.sample)
+    Fabricate(:bookmark, user: tu4.user, topic: topic, post: topic.posts.sample)
+
+    subject.execute(topic_id: topic.id)
+
+    expect(tu1.reload.bookmarked).to eq(true)
+    expect(tu2.reload.bookmarked).to eq(false)
+    expect(tu3.reload.bookmarked).to eq(false)
+    expect(tu4.reload.bookmarked).to eq(true)
+    expect(tu5.reload.bookmarked).to eq(false)
+  end
+
+  it "works when no topic id is provided (runs for all topics)" do
+    topic = Fabricate(:topic)
+    Fabricate(:post, topic: topic)
+    Fabricate(:post, topic: topic)
+    Fabricate(:post, topic: topic)
+
+    tu1 = Fabricate(:topic_user, topic: topic, bookmarked: false)
+    tu2 = Fabricate(:topic_user, topic: topic, bookmarked: false)
+    tu3 = Fabricate(:topic_user, topic: topic, bookmarked: true)
+    tu4 = Fabricate(:topic_user, topic: topic, bookmarked: true)
+    tu5 = Fabricate(:topic_user, bookmarked: false)
+
+    Fabricate(:bookmark, user: tu1.user, topic: topic, post: topic.posts.sample)
+    Fabricate(:bookmark, user: tu4.user, topic: topic, post: topic.posts.sample)
+
+    subject.execute
+
+    expect(tu1.reload.bookmarked).to eq(true)
+    expect(tu2.reload.bookmarked).to eq(false)
+    expect(tu3.reload.bookmarked).to eq(false)
+    expect(tu4.reload.bookmarked).to eq(true)
+    expect(tu5.reload.bookmarked).to eq(false)
+  end
+end

spec/models/post_mover_spec.rb

@@ -399,6 +399,48 @@ describe PostMover do
             expect(bookmark4.reload.topic_id).to eq(new_post.topic_id)
           end
 
+          it "makes sure the topic_user.bookmarked value is reflected for users in the source and destination topic" do
+            Jobs.run_immediately!
+            user1 = Fabricate(:user)
+            user2 = Fabricate(:user)
+
+            bookmark1 = Fabricate(:bookmark, topic: p1.topic, post: p1, user: user1)
+            bookmark2 = Fabricate(:bookmark, topic: p4.topic, post: p4, user: user1)
+
+            bookmark3 = Fabricate(:bookmark, topic: p3.topic, post: p3, user: user2)
+            bookmark4 = Fabricate(:bookmark, topic: p4.topic, post: p4, user: user2)
+
+            tu1 = Fabricate(
+              :topic_user,
+              user: user1,
+              topic: p1.topic,
+              bookmarked: true,
+              notification_level: TopicUser.notification_levels[:watching],
+              last_read_post_number: 4,
+              highest_seen_post_number: 4,
+              last_emailed_post_number: 3
+            )
+            tu2 = Fabricate(
+              :topic_user,
+              user: user2,
+              topic: p1.topic,
+              bookmarked: true,
+              notification_level: TopicUser.notification_levels[:watching],
+              last_read_post_number: 4,
+              highest_seen_post_number: 4,
+              last_emailed_post_number: 3
+            )
+
+            new_topic = topic.move_posts(user, [p1.id, p4.id], title: "new testing topic name")
+            new_topic_user1 = TopicUser.find_by(topic: new_topic, user: user1)
+            new_topic_user2 = TopicUser.find_by(topic: new_topic, user: user2)
+
+            expect(tu1.reload.bookmarked).to eq(false)
+            expect(tu2.reload.bookmarked).to eq(true)
+            expect(new_topic_user1.bookmarked).to eq(true)
+            expect(new_topic_user2.bookmarked).to eq(true)
+          end
+
           context "read state and other stats per user" do
             def create_topic_user(user, opts = {})
               notification_level = opts.delete(:notification_level) || :regular