FIX: Use bookmarkable pattern for bookmark cleanup (#17202)

We have a `cleanup!` class method on bookmarks that deletes
bookmarks X days after their related record (post/topic) are
deleted. This commit changes this method to use the
registered_bookmarkables for this instead, and each bookmarkable
type can delete related bookmarks in their own way.

app/models/bookmark.rb

@@ -177,26 +177,11 @@ class Bookmark < ActiveRecord::Base
   end
 
   ##
-  # Deletes bookmarks that are attached to posts/topics that were deleted
+  # Deletes bookmarks that are attached to the bookmarkable records that were deleted
-  # more than X days ago. We don't delete bookmarks instantly when a post/topic
+  # more than X days ago. We don't delete bookmarks instantly when trashable bookmarkables
-  # is deleted so that there is a grace period to un-delete.
+  # are deleted so that there is a grace period to un-delete.
   def self.cleanup!
-    grace_time = 3.days.ago
+    Bookmark.registered_bookmarkables.each(&:cleanup_deleted)
-    topics_deleted = DB.query(<<~SQL, grace_time: grace_time)
-      DELETE FROM bookmarks b
-      USING topics t, posts p
-      WHERE (t.id = p.topic_id AND (
-          (b.bookmarkable_id = p.id AND b.bookmarkable_type = 'Post') OR
-          (b.bookmarkable_id = p.id AND b.bookmarkable_type = 'Topic')
-        ))
-        AND (t.deleted_at < :grace_time OR p.deleted_at < :grace_time)
-       RETURNING t.id AS 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
 

app/services/base_bookmarkable.rb

@@ -155,4 +155,17 @@ class BaseBookmarkable
   def self.after_destroy(guardian, bookmark, opts)
     # noop
   end
+
+  ##
+  # Some bookmarkable records are Trashable, and as such we don't delete the
+  # bookmark with dependent_destroy. This should be used to delete those records
+  # after a grace period, defined by the bookmarkable. For example, post bookmarks
+  # may be deleted 3 days after the post or topic is deleted.
+  #
+  # In the case of bookmarkable records that are not trashable, and where
+  # dependent_destroy is not used, this shouldjust delete the bookmarks pointing
+  # to the record which no longer exists in the database.
+  def self.cleanup_deleted
+    # noop
+  end
 end

app/services/post_bookmarkable.rb

@@ -80,4 +80,19 @@ class PostBookmarkable < BaseBookmarkable
   def self.after_destroy(guardian, bookmark, opts)
     sync_topic_user_bookmarked(guardian.user, bookmark.bookmarkable.topic, opts)
   end
+
+  def self.cleanup_deleted
+    related_topics = DB.query(<<~SQL, grace_time: 3.days.ago)
+      DELETE FROM bookmarks b
+      USING topics t, posts p
+      WHERE t.id = p.topic_id AND b.bookmarkable_id = p.id AND b.bookmarkable_type = 'Post'
+      AND (t.deleted_at < :grace_time OR p.deleted_at < :grace_time)
+      RETURNING t.id AS topic_id
+    SQL
+
+    related_topics_ids = related_topics.map(&:topic_id).uniq
+    related_topics_ids.each do |topic_id|
+      Jobs.enqueue(:sync_topic_user_bookmarked, topic_id: topic_id)
+    end
+  end
 end

app/services/registered_bookmarkable.rb

@@ -96,4 +96,8 @@ class RegisteredBookmarkable
   def after_destroy(guardian, bookmark, opts = {})
     bookmarkable_klass.after_destroy(guardian, bookmark, opts)
   end
+
+  def cleanup_deleted
+    bookmarkable_klass.cleanup_deleted
+  end
 end

app/services/topic_bookmarkable.rb

@@ -72,4 +72,19 @@ class TopicBookmarkable < BaseBookmarkable
   def self.after_destroy(guardian, bookmark, opts)
     sync_topic_user_bookmarked(guardian.user, bookmark.bookmarkable, opts)
   end
+
+  def self.cleanup_deleted
+    related_topics = DB.query(<<~SQL, grace_time: 3.days.ago)
+      DELETE FROM bookmarks b
+      USING topics t
+      WHERE b.bookmarkable_id = t.id AND b.bookmarkable_type = 'Topic'
+      AND (t.deleted_at < :grace_time)
+      RETURNING t.id AS topic_id
+    SQL
+
+    related_topics_ids = related_topics.map(&:topic_id).uniq
+    related_topics_ids.each do |topic_id|
+      Jobs.enqueue(:sync_topic_user_bookmarked, topic_id: topic_id)
+    end
+  end
 end

spec/models/bookmark_spec.rb

@@ -60,7 +60,7 @@ describe Bookmark do
       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
+    it "deletes bookmarks attached via a post to a deleted topic which has been deleted for > 3 days" do
       bookmark = Fabricate(:bookmark, bookmarkable: post)
       bookmark2 = Fabricate(:bookmark, bookmarkable: Fabricate(:post, topic: post.topic))
       bookmark3 = Fabricate(:bookmark)
@@ -70,6 +70,21 @@ describe Bookmark do
       expect(Bookmark.find_by(id: bookmark.id)).to eq(nil)
       expect(Bookmark.find_by(id: bookmark2.id)).to eq(nil)
       expect(Bookmark.find_by(id: bookmark3.id)).to eq(bookmark3)
+      expect_job_enqueued(job: :sync_topic_user_bookmarked, args: { topic_id: post.topic_id })
+    end
+
+    it "deletes bookmarks attached via the topic to a deleted topic which has been deleted for > 3 days" do
+      topic = Fabricate(:topic)
+      bookmark = Fabricate(:bookmark, bookmarkable: topic)
+      bookmark2 = Fabricate(:bookmark, bookmarkable: Fabricate(:post, topic: topic))
+      bookmark3 = Fabricate(:bookmark)
+      topic.trash!
+      topic.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(nil)
+      expect(Bookmark.find_by(id: bookmark3.id)).to eq(bookmark3)
+      expect_job_enqueued(job: :sync_topic_user_bookmarked, args: { topic_id: topic.id })
     end
 
     it "does not delete bookmarks attached to posts that are not deleted or that have not met the 3 day grace period" do