From a176b57be08621941d5bf26b9f09124c44145482 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Thu, 23 Jun 2022 14:09:39 +1000 Subject: [PATCH] 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 | 23 ++++------------------- app/services/base_bookmarkable.rb | 13 +++++++++++++ app/services/post_bookmarkable.rb | 15 +++++++++++++++ app/services/registered_bookmarkable.rb | 4 ++++ app/services/topic_bookmarkable.rb | 15 +++++++++++++++ spec/models/bookmark_spec.rb | 17 ++++++++++++++++- 6 files changed, 67 insertions(+), 20 deletions(-) diff --git a/app/models/bookmark.rb b/app/models/bookmark.rb index 37491ebf600..2bd607d1e66 100644 --- a/app/models/bookmark.rb +++ b/app/models/bookmark.rb @@ -177,26 +177,11 @@ class Bookmark < ActiveRecord::Base end ## - # Deletes bookmarks that are attached to posts/topics that were deleted - # more than X days ago. We don't delete bookmarks instantly when a post/topic - # is deleted so that there is a grace period to un-delete. + # Deletes bookmarks that are attached to the bookmarkable records that were deleted + # more than X days ago. We don't delete bookmarks instantly when trashable bookmarkables + # are deleted so that there is a grace period to un-delete. def self.cleanup! - grace_time = 3.days.ago - 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 + Bookmark.registered_bookmarkables.each(&:cleanup_deleted) end end diff --git a/app/services/base_bookmarkable.rb b/app/services/base_bookmarkable.rb index 788e8619197..111e84bc7da 100644 --- a/app/services/base_bookmarkable.rb +++ b/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 diff --git a/app/services/post_bookmarkable.rb b/app/services/post_bookmarkable.rb index 06ac39a133f..d72f98cce65 100644 --- a/app/services/post_bookmarkable.rb +++ b/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 diff --git a/app/services/registered_bookmarkable.rb b/app/services/registered_bookmarkable.rb index 1429131f4e8..ce3cb1a4901 100644 --- a/app/services/registered_bookmarkable.rb +++ b/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 diff --git a/app/services/topic_bookmarkable.rb b/app/services/topic_bookmarkable.rb index 5adfc1cfa00..a08cce71d22 100644 --- a/app/services/topic_bookmarkable.rb +++ b/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 diff --git a/spec/models/bookmark_spec.rb b/spec/models/bookmark_spec.rb index 162d2ddacfa..267a94873f1 100644 --- a/spec/models/bookmark_spec.rb +++ b/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