FIX: Deleting/recovering a post in topics caused bookmark side effects (#24226)

This commit fixes an issue where when some actions were done
(deleting/recovering post, moving posts) we updated the
topic_users.bookmarked column to the wrong value. This was happening
because the SyncTopicUserBookmarked job was not taking into account
Topic level bookmarks, so if there was a Topic bookmark and no
Post bookmarks for a user in the topic, they would have
topic_users.bookmarked set to false, which meant the bookmark would
no longer show in the /bookmarks list.

To reproduce before the fix:

* Bookmark a topic and don’t bookmark any posts within
* Delete or recover any post in the topic

c.f. https://meta.discourse.org/t/disappearing-bookmarks-and-expected-behavior-of-bookmarks/264670/36

app/jobs/regular/sync_topic_user_bookmarked.rb

@@ -3,29 +3,37 @@
 module Jobs
   class SyncTopicUserBookmarked < ::Jobs::Base
     def execute(args = {})
-      topic_id = args[:topic_id]
+      raise Discourse::InvalidParameters.new(:topic_id) unless args[:topic_id].present?
 
-      DB.exec(<<~SQL, topic_id: topic_id)
-        UPDATE topic_users SET bookmarked = true
-        FROM bookmarks AS b
-        INNER JOIN posts ON posts.id = b.bookmarkable_id AND b.bookmarkable_type = 'Post'
-        WHERE NOT topic_users.bookmarked AND
-          posts.deleted_at IS NULL AND
-          topic_users.topic_id = posts.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: args[:topic_id])
+        SELECT bookmarks.user_id, COUNT(*)
+        INTO TEMP TABLE tmp_sync_topic_user_bookmarks
+        FROM bookmarks
+        LEFT JOIN posts ON posts.id = bookmarks.bookmarkable_id AND bookmarks.bookmarkable_type = 'Post'
+        LEFT JOIN topics ON (topics.id = bookmarks.bookmarkable_id AND bookmarks.bookmarkable_type = 'Topic') OR
+         (topics.id = posts.topic_id)
+        WHERE (topics.id = :topic_id OR posts.topic_id = :topic_id)
+        AND posts.deleted_at IS NULL AND topics.deleted_at IS NULL
+        GROUP BY bookmarks.user_id;
 
-      DB.exec(<<~SQL, topic_id: topic_id)
-        UPDATE topic_users SET bookmarked = false
-        WHERE topic_users.bookmarked AND
-          (
-            SELECT COUNT(*)
-            FROM bookmarks
-            INNER JOIN posts ON posts.id = bookmarks.bookmarkable_id AND bookmarks.bookmarkable_type = 'Post'
-            WHERE posts.topic_id = topic_users.topic_id
-            AND bookmarks.user_id = topic_users.user_id
-            AND posts.deleted_at IS NULL
-        ) = 0 #{topic_id.present? ? "AND topic_users.topic_id = :topic_id" : ""}
+        UPDATE topic_users
+        SET bookmarked = true
+        FROM tmp_sync_topic_user_bookmarks
+        WHERE topic_users.user_id = tmp_sync_topic_user_bookmarks.user_id AND
+          topic_users.topic_id = :topic_id AND
+          tmp_sync_topic_user_bookmarks.count > 0;
+
+        UPDATE topic_users
+        SET bookmarked = false
+        FROM tmp_sync_topic_user_bookmarks
+        WHERE topic_users.topic_id = :topic_id AND
+          topic_users.bookmarked = true AND
+          topic_users.user_id NOT IN (
+            SELECT tmp_sync_topic_user_bookmarks.user_id
+            FROM tmp_sync_topic_user_bookmarks
+          );
+
+        DROP TABLE tmp_sync_topic_user_bookmarks;
       SQL
     end
   end

db/migrate/20231103060018_fix_invalid_topic_user_bookmarked_data.rb

@@ -0,0 +1,23 @@
+# frozen_string_literal: true
+
+class FixInvalidTopicUserBookmarkedData < ActiveRecord::Migration[7.0]
+  def up
+    DB.exec(<<~SQL)
+      UPDATE topic_users
+      SET bookmarked = true
+      WHERE id IN (
+        SELECT
+          topic_users.id
+        FROM topic_users
+        INNER JOIN bookmarks ON bookmarks.user_id = topic_users.user_id
+        WHERE bookmarkable_type = 'Topic'
+          AND bookmarks.bookmarkable_id = topic_users.topic_id
+          AND topic_users.bookmarked = false
+      ) AND topic_users.bookmarked = false
+    SQL
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end

spec/jobs/sync_topic_user_bookmarked_spec.rb

@@ -39,16 +39,10 @@ RSpec.describe Jobs::SyncTopicUserBookmarked do
     expect(tu2.reload.bookmarked).to eq(false)
   end
 
-  it "works when no topic id is provided (runs for all topics)" do
-    Fabricate(:bookmark, user: tu1.user, bookmarkable: topic.posts.sample)
-    Fabricate(:bookmark, user: tu4.user, bookmarkable: topic.posts.sample)
-
-    job.execute
-
+  it "still considers the topic bookmarked if it has a Topic bookmarkable" do
+    expect(tu1.reload.bookmarked).to eq(false)
+    Fabricate(:bookmark, user: tu1.user, bookmarkable: topic)
+    job.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
 end