From 907adce1cbab83909555c14064546d0f7ff0cb23 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Wed, 11 May 2022 09:51:03 +1000 Subject: [PATCH] FIX: Use registered bookmarkables for BookmarkManager (#16695) These validate/after_create/after_destroy methods were added back in b8828d4a2d27176cc7e882aa87fd084945f1501a before the RegisteredBookmarkable API and pattern was nailed down. This commit updates BookmarkManager to call out to the relevant bookmarkable for these and bookmark_metadata for consistency. --- app/helpers/topic_post_bookmarkable_helper.rb | 16 ++++++ app/services/base_bookmarkable.rb | 46 +++++++++++++++++ app/services/post_bookmarkable.rb | 23 +++++++++ app/services/registered_bookmarkable.rb | 16 ++++++ app/services/topic_bookmarkable.rb | 18 +++++++ lib/bookmark_manager.rb | 51 +++++-------------- 6 files changed, 131 insertions(+), 39 deletions(-) create mode 100644 app/helpers/topic_post_bookmarkable_helper.rb diff --git a/app/helpers/topic_post_bookmarkable_helper.rb b/app/helpers/topic_post_bookmarkable_helper.rb new file mode 100644 index 00000000000..43f5699b876 --- /dev/null +++ b/app/helpers/topic_post_bookmarkable_helper.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +module TopicPostBookmarkableHelper + extend ActiveSupport::Concern + + module ClassMethods + def sync_topic_user_bookmarked(user, topic, opts) + return if opts.key?(:auto_track) && !opts[:auto_track] + TopicUser.change( + user.id, + topic.id, + bookmarked: Bookmark.for_user_in_topic(user.id, topic).exists? + ) + end + end +end diff --git a/app/services/base_bookmarkable.rb b/app/services/base_bookmarkable.rb index a91a5d93805..788e8619197 100644 --- a/app/services/base_bookmarkable.rb +++ b/app/services/base_bookmarkable.rb @@ -109,4 +109,50 @@ class BaseBookmarkable def self.can_see?(guardian, bookmark) raise NotImplementedError end + + ## + # Some additional information about the bookmark or the surrounding relations + # may be required when the bookmark is created or destroyed. For example, when + # destroying a bookmark within a topic we need to know whether there are other + # bookmarks still remaining in the topic. + # + # @param [Bookmark] bookmark The bookmark that we are retrieving additional metadata for. + # @param [User] user The current user which is accessing the bookmark metadata. + # @return [Hash] (optional) + def self.bookmark_metadata(bookmark, user) + {} + end + + ## + # Optional bookmarkable specific validations may need to be run before a bookmark is created + # via the BookmarkManager. From here an error should be raised if there is an issue + # with the bookmarkable. + # + # @param [Guardian] guardian The guardian for the user which is creating the bookmark. + # @param [Model] bookmarkable The ActiveRecord model which is acting as the bookmarkable for the new bookmark. + def self.validate_before_create(guardian, bookmarkable) + # noop + end + + ## + # Optional additional actions may need to occur after a bookmark is created + # via the BookmarkManager. + # + # @param [Guardian] guardian The guardian for the user which is creating the bookmark. + # @param [Model] bookmark The bookmark which was created. + # @param [Hash] opts Additional options that may be passed down via BookmarkManager. + def self.after_create(guardian, bookmark, opts) + # noop + end + + ## + # Optional additional actions may need to occur after a bookmark is destroyed + # via the BookmarkManager. + # + # @param [Guardian] guardian The guardian for the user which is destroying the bookmark. + # @param [Model] bookmark The bookmark which was destroyed. + # @param [Hash] opts Additional options that may be passed down via BookmarkManager. + def self.after_destroy(guardian, bookmark, opts) + # noop + end end diff --git a/app/services/post_bookmarkable.rb b/app/services/post_bookmarkable.rb index 075f206b0c7..06ac39a133f 100644 --- a/app/services/post_bookmarkable.rb +++ b/app/services/post_bookmarkable.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true class PostBookmarkable < BaseBookmarkable + include TopicPostBookmarkableHelper + def self.model Post end @@ -57,4 +59,25 @@ class PostBookmarkable < BaseBookmarkable def self.can_see?(guardian, bookmark) guardian.can_see_post?(bookmark.bookmarkable) end + + def self.bookmark_metadata(bookmark, user) + { topic_bookmarked: Bookmark.for_user_in_topic(user.id, bookmark.bookmarkable.topic_id).exists? } + end + + def self.validate_before_create(guardian, bookmarkable) + if bookmarkable.blank? || + bookmarkable.topic.blank? || + !guardian.can_see_topic?(bookmarkable.topic) || + !guardian.can_see_post?(bookmarkable) + raise Discourse::InvalidAccess + end + end + + def self.after_create(guardian, bookmark, opts) + sync_topic_user_bookmarked(guardian.user, bookmark.bookmarkable.topic, opts) + end + + def self.after_destroy(guardian, bookmark, opts) + sync_topic_user_bookmarked(guardian.user, bookmark.bookmarkable.topic, opts) + end end diff --git a/app/services/registered_bookmarkable.rb b/app/services/registered_bookmarkable.rb index ba05a84bc2f..1429131f4e8 100644 --- a/app/services/registered_bookmarkable.rb +++ b/app/services/registered_bookmarkable.rb @@ -80,4 +80,20 @@ class RegisteredBookmarkable def can_see?(guardian, bookmark) bookmarkable_klass.can_see?(guardian, bookmark) end + + def bookmark_metadata(bookmark, user) + bookmarkable_klass.bookmark_metadata(bookmark, user) + end + + def validate_before_create(guardian, bookmarkable) + bookmarkable_klass.validate_before_create(guardian, bookmarkable) + end + + def after_create(guardian, bookmark, opts = {}) + bookmarkable_klass.after_create(guardian, bookmark, opts) + end + + def after_destroy(guardian, bookmark, opts = {}) + bookmarkable_klass.after_destroy(guardian, bookmark, opts) + end end diff --git a/app/services/topic_bookmarkable.rb b/app/services/topic_bookmarkable.rb index 4293903c5e3..5adfc1cfa00 100644 --- a/app/services/topic_bookmarkable.rb +++ b/app/services/topic_bookmarkable.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true class TopicBookmarkable < BaseBookmarkable + include TopicPostBookmarkableHelper + def self.model Topic end @@ -54,4 +56,20 @@ class TopicBookmarkable < BaseBookmarkable def self.can_see?(guardian, bookmark) guardian.can_see_topic?(bookmark.bookmarkable) end + + def self.bookmark_metadata(bookmark, user) + { topic_bookmarked: Bookmark.for_user_in_topic(user.id, bookmark.bookmarkable.id).exists? } + end + + def self.validate_before_create(guardian, bookmarkable) + raise Discourse::InvalidAccess if bookmarkable.blank? || !guardian.can_see_topic?(bookmarkable) + end + + def self.after_create(guardian, bookmark, opts) + sync_topic_user_bookmarked(guardian.user, bookmark.bookmarkable, opts) + end + + def self.after_destroy(guardian, bookmark, opts) + sync_topic_user_bookmarked(guardian.user, bookmark.bookmarkable, opts) + end end diff --git a/lib/bookmark_manager.rb b/lib/bookmark_manager.rb index 40be5504259..11a6186e086 100644 --- a/lib/bookmark_manager.rb +++ b/lib/bookmark_manager.rb @@ -9,17 +9,11 @@ class BookmarkManager end def self.bookmark_metadata(bookmark, user) - data = {} if SiteSetting.use_polymorphic_bookmarks - if bookmark.bookmarkable_type == "Topic" - data[:topic_bookmarked] = Bookmark.for_user_in_topic(user.id, bookmark.bookmarkable.id).exists? - elsif bookmark.bookmarkable_type == "Post" - data[:topic_bookmarked] = Bookmark.for_user_in_topic(user.id, bookmark.bookmarkable.topic.id).exists? - end + bookmark.registered_bookmarkable.bookmark_metadata(bookmark, user) else - data[:topic_bookmarked] = Bookmark.for_user_in_topic(user.id, bookmark.topic.id).exists? + { topic_bookmarked: Bookmark.for_user_in_topic(user.id, bookmark.topic.id).exists? } end - data end # TODO (martin) [POLYBOOK] This will be used in place of #create once @@ -28,7 +22,8 @@ class BookmarkManager raise NotImplementedError if !SiteSetting.use_polymorphic_bookmarks bookmarkable = bookmarkable_type.constantize.find_by(id: bookmarkable_id) - self.send("validate_bookmarkable_#{bookmarkable_type.downcase}", bookmarkable) + registered_bookmarkable = Bookmark.registered_bookmarkable_from_type(bookmarkable_type) + registered_bookmarkable.validate_before_create(@guardian, bookmarkable) bookmark = Bookmark.create( { @@ -42,7 +37,7 @@ class BookmarkManager return add_errors_from(bookmark) if bookmark.errors.any? - self.send("after_create_bookmarkable_#{bookmarkable_type.downcase}", bookmarkable) + registered_bookmarkable.after_create(@guardian, bookmark, options) update_user_option(bookmark) bookmark @@ -82,7 +77,12 @@ class BookmarkManager options: {} ) post = Post.find_by(id: post_id) - validate_bookmarkable_post(post) + if post.blank? || + post.topic.blank? || + !@guardian.can_see_topic?(post.topic) || + !@guardian.can_see_post?(post) + raise Discourse::InvalidAccess + end bookmark = Bookmark.create( { @@ -111,7 +111,7 @@ class BookmarkManager bookmark.destroy if SiteSetting.use_polymorphic_bookmarks - self.send("after_destroy_bookmarkable_#{bookmark.bookmarkable_type.downcase}", bookmark) + bookmark.registered_bookmarkable.after_destroy(@guardian, bookmark) else update_topic_user_bookmarked(bookmark.topic) end @@ -193,31 +193,4 @@ class BookmarkManager def update_user_option(bookmark) @user.user_option.update!(bookmark_auto_delete_preference: bookmark.auto_delete_preference) end - - def after_create_bookmarkable_post(post, opts = {}) - update_topic_user_bookmarked(post.topic, opts) - end - - def after_create_bookmarkable_topic(topic, opts = {}) - update_topic_user_bookmarked(topic, opts) - end - - def after_destroy_bookmarkable_post(bookmark) - update_topic_user_bookmarked(bookmark.bookmarkable.topic) - end - - def after_destroy_bookmarkable_topic(bookmark) - update_topic_user_bookmarked(bookmark.bookmarkable) - end - - def validate_bookmarkable_post(post) - # no bookmarking deleted posts or topics - raise Discourse::InvalidAccess if post.blank? || !@guardian.can_see_post?(post) - validate_bookmarkable_topic(post.topic) - end - - def validate_bookmarkable_topic(topic) - # no bookmarking deleted posts or topics - raise Discourse::InvalidAccess if topic.blank? || !@guardian.can_see_topic?(topic) - end end