FIX: Use registered bookmarkables for BookmarkManager (#16695)

These validate/after_create/after_destroy methods were added
back in b8828d4a2d 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

@@ -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

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

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

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

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

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"
+      bookmark.registered_bookmarkable.bookmark_metadata(bookmark, user)
-        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
     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