FIX: Make sure topic_user.bookmarked is synced in more places (#13383)

When we call Bookmark.cleanup! we want to make sure that
topic_user.bookmarked is updated for topics linked to the
bookmarks that were deleted. Also when PostDestroyer calls
destroy and recover. We have a job for this already --
SyncTopicUserBookmarked -- so we just utilize that.

app/jobs/regular/sync_topic_user_bookmarked.rb

@@ -8,7 +8,9 @@ module Jobs
       DB.exec(<<~SQL, topic_id: topic_id)
         UPDATE topic_users SET bookmarked = true
         FROM bookmarks AS b
+        INNER JOIN posts ON posts.id = b.post_id
         WHERE NOT topic_users.bookmarked AND
+          posts.deleted_at IS NULL 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
@@ -17,9 +19,12 @@ module Jobs
         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
+            SELECT COUNT(*)
+            FROM bookmarks
+            INNER JOIN posts ON posts.id = bookmarks.post_id
+            WHERE bookmarks.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" : ""}
       SQL
     end

app/models/bookmark.rb

@@ -125,12 +125,18 @@ class Bookmark < ActiveRecord::Base
   # is deleted so that there is a grace period to un-delete.
   def self.cleanup!
     grace_time = 3.days.ago
-    DB.exec(<<~SQL, grace_time: grace_time)
+    topics_deleted = DB.query(<<~SQL, grace_time: grace_time)
       DELETE FROM bookmarks b
       USING topics t, posts p
       WHERE (b.topic_id = t.id AND b.post_id = p.id)
         AND (t.deleted_at < :grace_time OR p.deleted_at < :grace_time)
+       RETURNING b.topic_id
     SQL
+
+    topics_deleted_ids = topics_deleted.map(&:topic_id).uniq
+    topics_deleted_ids.each do |topic_id|
+      Jobs.enqueue(:sync_topic_user_bookmarked, topic_id: topic_id)
+    end
   end
 end
 

lib/post_destroyer.rb

@@ -76,6 +76,7 @@ class PostDestroyer
 
     DiscourseEvent.trigger(:post_destroyed, @post, @opts, @user)
     WebHook.enqueue_post_hooks(:post_destroyed, @post, payload)
+    Jobs.enqueue(:sync_topic_user_bookmarked, topic_id: topic.id) if topic
 
     if is_first_post
       UserProfile.remove_featured_topic_from_all_profiles(@topic)
@@ -95,8 +96,11 @@ class PostDestroyer
     topic.update_column(:user_id, Discourse::SYSTEM_USER_ID) if !topic.user_id
     topic.recover!(@user) if @post.is_first_post?
     topic.update_statistics
+
     UserActionManager.post_created(@post)
     DiscourseEvent.trigger(:post_recovered, @post, @opts, @user)
+    Jobs.enqueue(:sync_topic_user_bookmarked, topic_id: topic.id) if topic
+
     if @post.is_first_post?
       UserActionManager.topic_created(topic)
       DiscourseEvent.trigger(:topic_recovered, topic, @user)

spec/components/post_destroyer_spec.rb

@@ -241,6 +241,13 @@ describe PostDestroyer do
           expect(UserAction.where(target_topic_id: post.topic_id, action_type: UserAction::NEW_TOPIC).count).to eq(1)
           expect(UserAction.where(target_topic_id: post.topic_id, action_type: UserAction::REPLY).count).to eq(1)
         end
+
+        it "runs the SyncTopicUserBookmarked for the topic that the post is in so topic_users.bookmarked is correct" do
+          PostDestroyer.new(@user, @reply).destroy
+          expect_enqueued_with(job: :sync_topic_user_bookmarked, args: { topic_id: @reply.topic_id  }) do
+            PostDestroyer.new(@user, @reply.reload).recover
+          end
+        end
       end
 
       context "recovered by admin" do
@@ -465,6 +472,12 @@ describe PostDestroyer do
       expect(post.raw).to eq(I18n.t('js.post.deleted_by_author_simple'))
     end
 
+    it "runs the SyncTopicUserBookmarked for the topic that the post is in so topic_users.bookmarked is correct" do
+      post2 = create_post
+      PostDestroyer.new(post2.user, post2).destroy
+      expect_job_enqueued(job: :sync_topic_user_bookmarked, args: { topic_id: post2.topic_id })
+    end
+
     context "as a moderator" do
       it "deletes the post" do
         author = post.user

spec/jobs/sync_topic_user_bookmarked_spec.rb

@@ -27,6 +27,24 @@ RSpec.describe Jobs::SyncTopicUserBookmarked do
     expect(tu5.reload.bookmarked).to eq(false)
   end
 
+  it "does not consider topic as bookmarked if the bookmarked post is deleted" do
+    topic = Fabricate(:topic)
+    post1 = Fabricate(:post, topic: topic)
+
+    tu1 = Fabricate(:topic_user, topic: topic, bookmarked: false)
+    tu2 = Fabricate(:topic_user, topic: topic, bookmarked: true)
+
+    Fabricate(:bookmark, user: tu1.user, topic: topic, post: post1)
+    Fabricate(:bookmark, user: tu2.user, topic: topic, post: post1)
+
+    post1.trash!
+
+    subject.execute(topic_id: topic.id)
+
+    expect(tu1.reload.bookmarked).to eq(false)
+    expect(tu2.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)

spec/models/bookmark_spec.rb

@@ -15,6 +15,26 @@ describe Bookmark do
       expect(Bookmark.find_by(id: bookmark2.id)).to eq(bookmark2)
     end
 
+    it "runs a SyncTopicUserBookmarked job for all deleted bookmark unique topics to make sure topic_user.bookmarked is in sync" do
+      post = Fabricate(:post)
+      post2 = Fabricate(:post)
+      bookmark = Fabricate(:bookmark, post: post, topic: post.topic)
+      bookmark2 = Fabricate(:bookmark, post: Fabricate(:post, topic: post.topic))
+      bookmark3 = Fabricate(:bookmark, post: post2, topic: post2.topic)
+      bookmark4 = Fabricate(:bookmark, post: post2, topic: post2.topic)
+      post.trash!
+      post.update(deleted_at: 4.days.ago)
+      post2.trash!
+      post2.update(deleted_at: 4.days.ago)
+      Bookmark.cleanup!
+      expect(Bookmark.find_by(id: bookmark.id)).to eq(nil)
+      expect(Bookmark.find_by(id: bookmark2.id)).to eq(bookmark2)
+      expect(Bookmark.find_by(id: bookmark3.id)).to eq(nil)
+      expect(Bookmark.find_by(id: bookmark4.id)).to eq(nil)
+      expect_job_enqueued(job: :sync_topic_user_bookmarked, args: { topic_id: post.topic_id })
+      expect_job_enqueued(job: :sync_topic_user_bookmarked, args: { topic_id: post2.topic_id })
+    end
+
     it "deletes bookmarks attached to a deleted topic which has been deleted for > 3 days" do
       post = Fabricate(:post)
       bookmark = Fabricate(:bookmark, post: post, topic: post.topic)