FIX: Ensure topic user bookmarked synced on bookmark auto-delete (#10323)

For the following conditions, the TopicUser.bookmarked column was not updated correctly:

* When a bookmark was auto-deleted because the reminder was sent
* When a bookmark was auto-deleted because the owner of the bookmark replied to the topic

This adds another migration to fix the out-of-sync column and also some refactors to BookmarkManager to allow for more of these delete cases. BookmarkManager is used instead of directly destroying the bookmark in PostCreator and BookmarkReminderNotificationHandler.

app/models/bookmark.rb

@@ -77,6 +77,15 @@ class Bookmark < ActiveRecord::Base
     self.auto_delete_preference == Bookmark.auto_delete_preferences[:on_owner_reply]
   end
 
+  def clear_reminder!
+    update(
+      reminder_at: nil,
+      reminder_type: nil,
+      reminder_last_sent_at: Time.zone.now,
+      reminder_set_at: nil
+    )
+  end
+
   scope :pending_reminders, ->(before_time = Time.now.utc) do
     where("reminder_at IS NOT NULL AND reminder_at <= :before_time", before_time: before_time)
   end

db/migrate/20200728022830_sync_topic_user_bookmarked_column_with_bookmarks.rb

@@ -0,0 +1,30 @@
+# frozen_string_literal: true
+
+class SyncTopicUserBookmarkedColumnWithBookmarks < ActiveRecord::Migration[6.0]
+  def up
+    should_be_bookmarked_sql = <<~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 AND
+          bookmarks.topic_id = topic_users.topic_id
+        WHERE NOT topic_users.bookmarked
+      ) AND NOT bookmarked
+    SQL
+    DB.exec(should_be_bookmarked_sql)
+
+    # post_action_type_id 1 is bookmark
+    should_not_be_bookmarked_sql = <<~SQL
+    UPDATE topic_users SET bookmarked = FALSE WHERE ID IN (
+      SELECT DISTINCT topic_users.id FROM topic_users
+      LEFT JOIN bookmarks ON bookmarks.topic_id = topic_users.topic_id AND bookmarks.user_id = topic_users.user_id
+      LEFT JOIN post_actions ON post_actions.user_id = topic_users.user_id AND post_actions.post_action_type_id = 1 AND post_actions.post_id IN (SELECT id FROM posts WHERE posts.topic_id = topic_users.topic_id) 
+      WHERE topic_users.bookmarked = true AND (bookmarks.id IS NULL AND post_actions.id IS NULL))
+    SQL
+    DB.exec(should_not_be_bookmarked_sql)
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end

lib/bookmark_manager.rb

@@ -29,7 +29,7 @@ class BookmarkManager
       return add_errors_from(bookmark)
     end
 
-    update_topic_user_bookmarked(topic: post.topic, bookmarked: true)
+    update_topic_user_bookmarked(post.topic)
 
     bookmark
   end
@@ -42,16 +42,14 @@ class BookmarkManager
 
     bookmark.destroy
 
-    bookmarks_remaining_in_topic = Bookmark.exists?(topic_id: bookmark.topic_id, user: @user)
+    bookmarks_remaining_in_topic = update_topic_user_bookmarked(bookmark.topic)
-    if !bookmarks_remaining_in_topic
-      update_topic_user_bookmarked(topic: bookmark.topic, bookmarked: false)
-    end
 
     { topic_bookmarked: bookmarks_remaining_in_topic }
   end
 
-  def destroy_for_topic(topic)
+  def destroy_for_topic(topic, filter = {}, opts = {})
     topic_bookmarks = Bookmark.where(user_id: @user.id, topic_id: topic.id)
+    topic_bookmarks = topic_bookmarks.where(filter)
 
     Bookmark.transaction do
       topic_bookmarks.each do |bookmark|
@@ -59,7 +57,7 @@ class BookmarkManager
         bookmark.destroy
       end
 
-      update_topic_user_bookmarked(topic: topic, bookmarked: false)
+      update_topic_user_bookmarked(topic, opts)
     end
   end
 
@@ -94,8 +92,14 @@ class BookmarkManager
 
   private
 
-  def update_topic_user_bookmarked(topic:, bookmarked:)
+  def update_topic_user_bookmarked(topic, opts = {})
-    TopicUser.change(@user.id, topic, bookmarked: bookmarked)
+    # PostCreator can specify whether auto_track is enabled or not, don't want to
+    # create a TopicUser in that case
+    bookmarks_remaining_in_topic = Bookmark.exists?(topic_id: topic.id, user: @user)
+    return bookmarks_remaining_in_topic if opts.key?(:auto_track) && !opts[:auto_track]
+
+    TopicUser.change(@user.id, topic, bookmarked: bookmarks_remaining_in_topic)
+    bookmarks_remaining_in_topic
   end
 
   def parse_reminder_type(reminder_type)

lib/bookmark_reminder_notification_handler.rb

@@ -11,7 +11,7 @@ class BookmarkReminderNotificationHandler
       create_notification(bookmark)
 
       if bookmark.delete_when_reminder_sent?
-        return bookmark.destroy
+        BookmarkManager.new(bookmark.user).destroy(bookmark.id)
       end
 
       clear_reminder(bookmark)
@@ -23,12 +23,7 @@ class BookmarkReminderNotificationHandler
       "Clearing bookmark reminder for bookmark_id #{bookmark.id}. reminder info: #{bookmark.reminder_at} | #{Bookmark.reminder_types[bookmark.reminder_type]}"
     )
 
-    bookmark.update(
+    bookmark.clear_reminder!
-      reminder_at: nil,
-      reminder_type: nil,
-      reminder_last_sent_at: Time.zone.now,
-      reminder_set_at: nil
-    )
   end
 
   def self.create_notification(bookmark)

lib/post_creator.rb

@@ -405,10 +405,11 @@ class PostCreator
 
   def delete_owned_bookmarks
     return if !@post.topic_id
-    @user.bookmarks.where(
+    BookmarkManager.new(@user).destroy_for_topic(
-      topic_id: @post.topic_id,
+      Topic.with_deleted.find(@post.topic_id),
-      auto_delete_preference: Bookmark.auto_delete_preferences[:on_owner_reply]
+      { auto_delete_preference: Bookmark.auto_delete_preferences[:on_owner_reply] },
-    ).destroy_all
+      @opts
+    )
   end
 
   def handle_spam

spec/components/post_creator_spec.rb

@@ -653,12 +653,22 @@ describe PostCreator do
       before do
         Fabricate(:bookmark, topic: topic, user: user, auto_delete_preference: Bookmark.auto_delete_preferences[:on_owner_reply])
         Fabricate(:bookmark, topic: topic, user: user, auto_delete_preference: Bookmark.auto_delete_preferences[:on_owner_reply])
+        TopicUser.create!(topic: topic, user: user, bookmarked: true)
+      end
+
+      it "deletes the bookmarks, but not the ones without an auto_delete_preference" do
         Fabricate(:bookmark, topic: topic, user: user)
         Fabricate(:bookmark, user: user)
-      end
-      it "deletes the bookmarks" do
         creator.create
         expect(Bookmark.where(user: user).count).to eq(2)
+        expect(TopicUser.find_by(topic: topic, user: user).bookmarked).to eq(true)
+      end
+
+      context "when there are no bookmarks left in the topic" do
+        it "sets TopicUser.bookmarked to false" do
+          creator.create
+          expect(TopicUser.find_by(topic: topic, user: user).bookmarked).to eq(false)
+        end
       end
     end
 

spec/lib/bookmark_reminder_notification_handler_spec.rb

@@ -35,11 +35,31 @@ RSpec.describe BookmarkReminderNotificationHandler do
     end
 
     context "when the auto_delete_preference is when_reminder_sent" do
-      it "deletes the bookmark after the reminder gets sent" do
+      before do
+        TopicUser.create!(topic: bookmark.topic, user: user, bookmarked: true)
         bookmark.update(auto_delete_preference: Bookmark.auto_delete_preferences[:when_reminder_sent])
+      end
+
+      it "deletes the bookmark after the reminder gets sent" do
         subject.send_notification(bookmark)
         expect(Bookmark.find_by(id: bookmark.id)).to eq(nil)
       end
+
+      it "changes the TopicUser bookmarked column to false" do
+        subject.send_notification(bookmark)
+        expect(TopicUser.find_by(topic: bookmark.topic, user: user).bookmarked).to eq(false)
+      end
+
+      context "if there are still other bookmarks in the topic" do
+        before do
+          Fabricate(:bookmark, topic: bookmark.topic, post: Fabricate(:post, topic: bookmark.topic), user: user)
+        end
+
+        it "does not change the TopicUser bookmarked column to false" do
+          subject.send_notification(bookmark)
+          expect(TopicUser.find_by(topic: bookmark.topic, user: user).bookmarked).to eq(true)
+        end
+      end
     end
 
     context "when the post has been deleted" do