FEATURE: Polymorphic bookmarks pt. 3 (reminders, imports, exports, refactors) (#16591)

A bit of a mixed bag, this addresses several edge areas of bookmarks and makes them compatible with polymorphic bookmarks (hidden behind the `use_polymorphic_bookmarks` site setting). The main ones are:

* ExportUserArchive compatibility
* SyncTopicUserBookmarked job compatibility
* Sending different notifications for the bookmark reminders based on the bookmarkable type
* Import scripts compatibility
* BookmarkReminderNotificationHandler compatibility

This PR also refactors the `register_bookmarkable` API so it accepts a class descended from a `BaseBookmarkable` class instead. This was done because we kept having to add more and more lambdas/properties inline and it was very messy, so a factory pattern is cleaner. The classes can be tested independently as well.

Some later PRs will address some other areas like the discourse narrative bot, advanced search, reports, and the .ics endpoint for bookmarks.

app/assets/javascripts/discourse/app/widgets/bookmark-reminder-notification-item.js

@@ -16,6 +16,7 @@ createWidgetFrom(
         username,
       });
     },
+
     notificationTitle(notificationName, data) {
       if (notificationName) {
         if (data.bookmark_name) {

app/assets/javascripts/discourse/app/widgets/default-notification-item.js

@@ -29,9 +29,9 @@ export const DefaultNotificationItem = createWidget(
       if (attrs.is_warning) {
         classNames.push("is-warning");
       }
-      const notificationType = attrs.notification_type;
+      const notificationName = this.lookupNotificationName(
-      const lookup = this.site.get("notificationLookup");
+        attrs.notification_type
-      const notificationName = lookup[notificationType];
+      );
       if (notificationName) {
         classNames.push(notificationName.replace(/_/g, "-"));
       }
@@ -64,6 +64,10 @@ export const DefaultNotificationItem = createWidget(
       if (data.group_id) {
         return userPath(data.username + "/messages/group/" + data.group_name);
       }
+
+      if (data.bookmarkable_url) {
+        return getURL(data.bookmarkable_url);
+      }
     },
 
     description(data) {
@@ -90,7 +94,7 @@ export const DefaultNotificationItem = createWidget(
         return this.attrs.fancy_title;
       }
 
-      const description = data.topic_title;
+      const description = data.topic_title || data.title;
 
       return isEmpty(description) ? "" : escapeExpression(description);
     },
@@ -126,11 +130,15 @@ export const DefaultNotificationItem = createWidget(
       }
     },
 
-    html(attrs) {
+    lookupNotificationName(notificationType) {
-      const notificationType = attrs.notification_type;
       const lookup = this.site.get("notificationLookup");
-      const notificationName = lookup[notificationType];
+      return lookup[notificationType];
+    },
 
+    html(attrs) {
+      const notificationName = this.lookupNotificationName(
+        attrs.notification_type
+      );
       let { data } = attrs;
       let text = emojiUnescape(this.text(notificationName, data));
       let icon = this.icon(notificationName, data);

app/controllers/users_controller.rb

@@ -1720,6 +1720,7 @@ class UsersController < ApplicationController
           render_serialized(bookmark_list, UserBookmarkListSerializer)
         end
       end
+      # TODO (martin) Make a separate PR for .ics reminders for polymorphic bookmarks
       format.ics do
         @bookmark_reminders = Bookmark.with_reminders
           .where(user_id: user.id)

app/jobs/regular/export_user_archive.rb

@@ -31,7 +31,9 @@ module Jobs
       auth_tokens: ['id', 'auth_token_hash', 'prev_auth_token_hash', 'auth_token_seen', 'client_ip', 'user_agent', 'seen_at', 'rotated_at', 'created_at', 'updated_at'],
       auth_token_logs: ['id', 'action', 'user_auth_token_id', 'client_ip', 'auth_token_hash', 'created_at', 'path', 'user_agent'],
       badges: ['badge_id', 'badge_name', 'granted_at', 'post_id', 'seq', 'granted_manually', 'notification_id', 'featured_rank'],
+      # TODO (martin) [POLYBOOK] - Remove the duplication when polymorphic bookmarks are implemented
       bookmarks: ['post_id', 'topic_id', 'post_number', 'link', 'name', 'created_at', 'updated_at', 'reminder_at', 'reminder_last_sent_at', 'reminder_set_at', 'auto_delete_preference'],
+      bookmarks_polymorphic: ['bookmarkable_id', 'bookmarkable_type', 'link', 'name', 'created_at', 'updated_at', 'reminder_at', 'reminder_last_sent_at', 'reminder_set_at', 'auto_delete_preference'],
       category_preferences: ['category_id', 'category_names', 'notification_level', 'dismiss_new_timestamp'],
       flags: ['id', 'post_id', 'flag_type', 'created_at', 'updated_at', 'deleted_at', 'deleted_by', 'related_post_id', 'targets_topic', 'was_take_action'],
       likes: ['id', 'post_id', 'topic_id', 'post_number', 'created_at', 'updated_at', 'deleted_at', 'deleted_by'],
@@ -48,7 +50,17 @@ module Jobs
       components = []
 
       COMPONENTS.each do |name|
-        h = { name: name, method: :"#{name}_export" }
+        export_method = \
+          if name == "bookmarks"
+            if SiteSetting.use_polymorphic_bookmarks
+              "bookmarks_polymorphic_export"
+            else
+              "bookmarks_export"
+            end
+          else
+            "#{name}_export"
+          end
+        h = { name: name, method: :"#{export_method}" }
         h[:filetype] = :csv
         filetype_method = :"#{name}_filetype"
         if respond_to? filetype_method
@@ -228,30 +240,56 @@ module Jobs
       end
     end
 
+    def bookmarks_polymorphic_export
+      return enum_for(:bookmarks_polymorphic_export) unless block_given?
+
+      @current_user.bookmarks.where.not(bookmarkable_type: nil).order(:id).each do |bookmark|
+        link = ''
+        if guardian.can_see_bookmarkable?(bookmark)
+          if bookmark.bookmarkable.respond_to?(:full_url)
+            link = bookmark.bookmarkable.full_url
+          else
+            link = bookmark.bookmarkable.url
+          end
+        end
+
+        yield [
+          bookmark.bookmarkable_id,
+          bookmark.bookmarkable_type,
+          link,
+          bookmark.name,
+          bookmark.created_at,
+          bookmark.updated_at,
+          bookmark.reminder_at,
+          bookmark.reminder_last_sent_at,
+          bookmark.reminder_set_at,
+          Bookmark.auto_delete_preferences[bookmark.auto_delete_preference],
+        ]
+      end
+    end
+
+    # TODO (martin) [POLYBOOK] No longer relevant after polymorphic bookmarks implemented
     def bookmarks_export
       return enum_for(:bookmarks_export) unless block_given?
 
-      Bookmark
+      @current_user.bookmarks.joins(:post).order(:id).each do |bookmark|
-        .where(user_id: @current_user.id)
-        .joins(:post)
-        .order(:id)
-        .each do |bkmk|
         link = ''
-        if guardian.can_see_post?(bkmk.post)
+        if guardian.can_see_bookmarkable?(bookmark)
-          link = bkmk.post.full_url
+          link = bookmark.post.full_url
         end
+
         yield [
-          bkmk.post_id,
+          bookmark.post_id,
-          bkmk.topic_id,
+          bookmark.topic_id,
-          bkmk.post&.post_number,
+          bookmark.post&.post_number,
           link,
-          bkmk.name,
+          bookmark.name,
-          bkmk.created_at,
+          bookmark.created_at,
-          bkmk.updated_at,
+          bookmark.updated_at,
-          bkmk.reminder_at,
+          bookmark.reminder_at,
-          bkmk.reminder_last_sent_at,
+          bookmark.reminder_last_sent_at,
-          bkmk.reminder_set_at,
+          bookmark.reminder_set_at,
-          Bookmark.auto_delete_preferences[bkmk.auto_delete_preference],
+          Bookmark.auto_delete_preferences[bookmark.auto_delete_preference],
         ]
       end
     end
@@ -396,6 +434,12 @@ module Jobs
           end
         end
         header_array.push("group_names")
+      elsif entity == 'bookmarks'
+        if SiteSetting.use_polymorphic_bookmarks
+          header_array = HEADER_ATTRS_FOR['bookmarks_polymorphic']
+        else
+          header_array = HEADER_ATTRS_FOR['bookmarks']
+        end
       else
         header_array = HEADER_ATTRS_FOR[entity]
       end

app/jobs/regular/sync_topic_user_bookmarked.rb

@@ -1,14 +1,35 @@
 # frozen_string_literal: true
 
 module Jobs
-
-  # TODO (martin) [POLYBOOK] This will need to be restructured for polymorphic
-  # bookmarks when edge cases are handled.
   class SyncTopicUserBookmarked < ::Jobs::Base
     def execute(args = {})
       topic_id = args[:topic_id]
 
-      DB.exec(<<~SQL, topic_id: topic_id)
+      if SiteSetting.use_polymorphic_bookmarks
+        DB.exec(<<~SQL, topic_id: topic_id)
+        UPDATE topic_users SET bookmarked = true
+        FROM bookmarks AS b
+        INNER JOIN posts ON posts.id = b.bookmarkable_id AND b.bookmarkable_type = 'Post'
+        WHERE NOT topic_users.bookmarked AND
+          posts.deleted_at IS NULL AND
+          topic_users.topic_id = posts.topic_id AND
+          topic_users.user_id = b.user_id #{topic_id.present? ? "AND topic_users.topic_id = :topic_id" : ""}
+        SQL
+
+        DB.exec(<<~SQL, topic_id: topic_id)
+        UPDATE topic_users SET bookmarked = false
+        WHERE topic_users.bookmarked AND
+          (
+            SELECT COUNT(*)
+            FROM bookmarks
+            INNER JOIN posts ON posts.id = bookmarks.bookmarkable_id AND bookmarks.bookmarkable_type = 'Post'
+            WHERE posts.topic_id = topic_users.topic_id
+            AND bookmarks.user_id = topic_users.user_id
+            AND posts.deleted_at IS NULL
+        ) = 0 #{topic_id.present? ? "AND topic_users.topic_id = :topic_id" : ""}
+        SQL
+      else
+        DB.exec(<<~SQL, topic_id: topic_id)
         UPDATE topic_users SET bookmarked = true
         FROM bookmarks AS b
         INNER JOIN posts ON posts.id = b.post_id
@@ -16,9 +37,9 @@ module Jobs
           posts.deleted_at IS NULL AND
           topic_users.topic_id = posts.topic_id AND
           topic_users.user_id = b.user_id #{topic_id.present? ? "AND topic_users.topic_id = :topic_id" : ""}
-      SQL
+        SQL
 
-      DB.exec(<<~SQL, topic_id: topic_id)
+        DB.exec(<<~SQL, topic_id: topic_id)
         UPDATE topic_users SET bookmarked = false
         WHERE topic_users.bookmarked AND
           (
@@ -29,7 +50,8 @@ module Jobs
             AND bookmarks.user_id = topic_users.user_id
             AND posts.deleted_at IS NULL
         ) = 0 #{topic_id.present? ? "AND topic_users.topic_id = :topic_id" : ""}
-      SQL
+        SQL
+      end
     end
   end
 end

app/jobs/scheduled/bookmark_reminder_notifications.rb

@@ -20,7 +20,7 @@ module Jobs
     def execute(args = nil)
       bookmarks = Bookmark.pending_reminders.includes(:user).order('reminder_at ASC')
       bookmarks.limit(BookmarkReminderNotifications.max_reminder_notifications_per_run).each do |bookmark|
-        BookmarkReminderNotificationHandler.send_notification(bookmark)
+        BookmarkReminderNotificationHandler.new(bookmark).send_notification
       end
     end
   end

app/models/bookmark.rb

@@ -8,16 +8,9 @@ class Bookmark < ActiveRecord::Base
     Bookmark.registered_bookmarkables.find { |bm| bm.model.name == type }
   end
 
-  def self.register_bookmarkable(
+  def self.register_bookmarkable(bookmarkable_klass)
-    model:, serializer:, list_query:, search_query:, preload_associations: []
+    return if Bookmark.registered_bookmarkable_from_type(bookmarkable_klass.model.name).present?
-  )
+    Bookmark.registered_bookmarkables << RegisteredBookmarkable.new(bookmarkable_klass)
-    Bookmark.registered_bookmarkables << Bookmarkable.new(
-      model: model,
-      serializer: serializer,
-      list_query: list_query,
-      search_query: search_query,
-      preload_associations: preload_associations
-    )
   end
 
   ##
@@ -30,55 +23,9 @@ class Bookmark < ActiveRecord::Base
   # classes are not cached.
   def self.reset_bookmarkables
     self.registered_bookmarkables = []
-    Bookmark.register_bookmarkable(
+
-      model: Post,
+    Bookmark.register_bookmarkable(PostBookmarkable)
-      serializer: UserPostBookmarkSerializer,
+    Bookmark.register_bookmarkable(TopicBookmarkable)
-      list_query: lambda do |user, guardian|
-        topics = Topic.listable_topics.secured(guardian)
-        pms = Topic.private_messages_for_user(user)
-        post_bookmarks = user
-          .bookmarks_of_type("Post")
-          .joins("INNER JOIN posts ON posts.id = bookmarks.bookmarkable_id AND bookmarks.bookmarkable_type = 'Post'")
-          .joins("LEFT JOIN topics ON topics.id = posts.topic_id")
-          .joins("LEFT JOIN topic_users ON topic_users.topic_id = topics.id")
-          .where("topic_users.user_id = ?", user.id)
-        guardian.filter_allowed_categories(
-          post_bookmarks.merge(topics.or(pms)).merge(Post.secured(guardian))
-        )
-      end,
-      search_query: lambda do |bookmarks, query, ts_query, &bookmarkable_search|
-        bookmarkable_search.call(
-          bookmarks.joins(
-            "LEFT JOIN post_search_data ON post_search_data.post_id = bookmarks.bookmarkable_id AND bookmarks.bookmarkable_type = 'Post'"
-          ),
-          "#{ts_query} @@ post_search_data.search_data"
-        )
-      end,
-      preload_associations: [{ topic: [:topic_users, :tags] }, :user]
-    )
-    Bookmark.register_bookmarkable(
-      model: Topic,
-      serializer: UserTopicBookmarkSerializer,
-      list_query: lambda do |user, guardian|
-        topics = Topic.listable_topics.secured(guardian)
-        pms = Topic.private_messages_for_user(user)
-        topic_bookmarks = user
-          .bookmarks_of_type("Topic")
-          .joins("INNER JOIN topics ON topics.id = bookmarks.bookmarkable_id AND bookmarks.bookmarkable_type = 'Topic'")
-          .joins("LEFT JOIN topic_users ON topic_users.topic_id = topics.id")
-          .where("topic_users.user_id = ?", user.id)
-        guardian.filter_allowed_categories(topic_bookmarks.merge(topics.or(pms)))
-      end,
-      search_query: lambda do |bookmarks, query, ts_query, &bookmarkable_search|
-        bookmarkable_search.call(
-          bookmarks
-          .joins("LEFT JOIN posts ON posts.topic_id = topics.id AND posts.post_number = 1")
-          .joins("LEFT JOIN post_search_data ON post_search_data.post_id = posts.id"),
-        "#{ts_query} @@ post_search_data.search_data"
-        )
-      end,
-      preload_associations: [:topic_users, :tags, { posts: :user }]
-    )
   end
   reset_bookmarkables
 
@@ -133,6 +80,10 @@ class Bookmark < ActiveRecord::Base
   validate :bookmark_limit_not_reached
   validates :name, length: { maximum: 100 }
 
+  def registered_bookmarkable
+    Bookmark.registered_bookmarkable_from_type(self.bookmarkable_type)
+  end
+
   def polymorphic_columns_present
     return if !SiteSetting.use_polymorphic_bookmarks
     return if self.bookmarkable_id.present? && self.bookmarkable_type.present?
@@ -263,6 +214,8 @@ class Bookmark < ActiveRecord::Base
     end
   end
 
+  # TODO (martin) [POLYBOOK] Make a separate PR for reports
+  # functionality as the bookmarkables will have to define this.
   def self.count_per_day(opts = nil)
     opts ||= {}
     result = where('bookmarks.created_at >= ?', opts[:start_date] || (opts[:since_days_ago] || 30).days.ago)

app/models/post.rb

@@ -47,6 +47,7 @@ class Post < ActiveRecord::Base
 
   has_one :post_stat
 
+  # TODO (martin) [POLYBOOK]
   # When we are ready we can add as: :bookmarkable here to use the
   # polymorphic association.
   has_many :bookmarks

app/services/base_bookmarkable.rb

@@ -0,0 +1,112 @@
+# frozen_string_literal: true
+
+##
+# Anything that we want to be able to bookmark must be registered as a
+# bookmarkable type using Bookmark.register_bookmarkable(bookmarkable_klass),
+# where the bookmarkable_klass is a class that implements this BaseBookmarkable
+# interface. Some examples are TopicBookmarkable and PostBookmarkable.
+#
+# These methods are then called by the RegisteredBookmarkable class through a public
+# interface, used in places where we need to list, send reminders for,
+# or otherwise interact with bookmarks in a way that is unique to the
+# bookmarkable type.
+#
+# See RegisteredBookmarkable for additional documentation.
+class BaseBookmarkable
+  attr_reader :model, :serializer, :preload_associations
+
+  # @return [ActiveRecord::Base] The ActiveRecord model class which will be used to denote
+  #                              the type of the bookmarkable upon registration along with
+  #                              querying.
+  def self.model
+    raise NotImplementedError
+  end
+
+  # @return [ApplicationSerializer] The serializer class inheriting from UserBookmarkBaseSerializer
+  def self.serializer
+    raise NotImplementedError
+  end
+
+  # @return [Array] Used for preloading associations on the bookmarks for listing
+  #                 purposes. Should be in the same format used for .includes() e.g.
+  #
+  #                 [{ topic: [:topic_users, :tags] }, :user]
+  def self.preload_associations
+    nil
+  end
+
+  def self.has_preloads?
+    preload_associations.present?
+  end
+
+  ##
+  # This is where the main query to filter the bookmarks by the provided bookmarkable
+  # type should occur. This should join on additional tables that are required later
+  # on to preload additional data for serializers, and also is the place where the
+  # bookmarks should be filtered based on security checks, which is why the Guardian
+  # instance is provided.
+  #
+  # @param [User] user The user to perform the query for, this scopes the bookmarks returned.
+  # @param [Guardian] guardian An instance of Guardian for the user to be used for security filters.
+  # @return [Bookmark::ActiveRecord_AssociationRelation] Should be an approprialely scoped list of bookmarks for the user.
+  def self.list_query(user, guardian)
+    raise NotImplementedError
+  end
+
+  ##
+  # Called from BookmarkQuery when the initial results have been returned by
+  # perform_list_query. The search_query should join additional tables required
+  # to filter the bookmarks further, as well as defining a string used for
+  # where_sql, which can include comparisons with the :q parameter.
+  #
+  # @param [Bookmark::ActiveRecord_Relation] bookmarks The bookmark records returned by perform_list_query
+  # @param [String] query The search query from the user surrounded by the %% wildcards
+  # @param [String] ts_query The postgres TSQUERY string used for comparisons with full text search columns
+  # @param [Block] bookmarkable_search This block _must_ be called with the additional WHERE clause SQL relevant
+  #                                    for the bookmarkable to be searched, as well as the bookmarks relation
+  #                                    with any additional joins applied.
+  # @return [Bookmark::ActiveRecord_AssociationRelation] The list of bookmarks from perform_list_query filtered further by
+  #                                                      the query parameter.
+  def self.search_query(bookmarks, query, ts_query, &bookmarkable_search)
+    raise NotImplementedError
+  end
+
+  ##
+  # When sending bookmark reminders, we want to make sure that whatever we
+  # are sending the reminder for has not been deleted or is otherwise inaccessible.
+  # Most of the time we can just check if the bookmarkable record is present
+  # because it will be trashable, though in some cases there will be additional
+  # conditions in the form of a lambda that we should use instead.
+  #
+  # The logic around whether it is the right time to send a reminder does not belong
+  # here, that is done in the BookmarkReminderNotifications job.
+  #
+  # @param [Bookmark] bookmark The bookmark that we are considering sending a reminder for.
+  # @return [Boolean]
+  def self.reminder_conditions(bookmark)
+    raise NotImplementedError
+  end
+
+  ##
+  # Different bookmarkables may have different ways of notifying a user or presenting
+  # the reminder and what it is for, so it is up to the bookmarkable to register
+  # its preferred method of sending the reminder.
+  #
+  # @param [Bookmark] bookmark The bookmark that we are sending the reminder notification for.
+  # @return [void]
+  def self.reminder_handler(bookmark)
+    raise NotImplementedError
+  end
+
+  ##
+  # Access control is dependent on what has been bookmarked, the appropriate guardian
+  # can_see_X? method should be called from the bookmarkable class to determine
+  # whether the bookmarkable record (e.g. Post, Topic) is accessible by the guardian user.
+  #
+  # @param [Guardian] guardian The guardian class for the user that we are performing the access check for.
+  # @param [Bookmark] bookmark The bookmark which we are checking access for using the bookmarkable association.
+  # @return [Boolean]
+  def self.can_see?(guardian, bookmark)
+    raise NotImplementedError
+  end
+end

app/services/post_bookmarkable.rb

@@ -0,0 +1,60 @@
+# frozen_string_literal: true
+
+class PostBookmarkable < BaseBookmarkable
+  def self.model
+    Post
+  end
+
+  def self.serializer
+    UserPostBookmarkSerializer
+  end
+
+  def self.preload_associations
+    [{ topic: [:topic_users, :tags] }, :user]
+  end
+
+  def self.list_query(user, guardian)
+    topics = Topic.listable_topics.secured(guardian)
+    pms = Topic.private_messages_for_user(user)
+    post_bookmarks = user
+      .bookmarks_of_type("Post")
+      .joins("INNER JOIN posts ON posts.id = bookmarks.bookmarkable_id AND bookmarks.bookmarkable_type = 'Post'")
+      .joins("LEFT JOIN topics ON topics.id = posts.topic_id")
+      .joins("LEFT JOIN topic_users ON topic_users.topic_id = topics.id")
+      .where("topic_users.user_id = ?", user.id)
+    guardian.filter_allowed_categories(
+      post_bookmarks.merge(topics.or(pms)).merge(Post.secured(guardian))
+    )
+  end
+
+  def self.search_query(bookmarks, query, ts_query, &bookmarkable_search)
+    bookmarkable_search.call(
+      bookmarks.joins(
+        "LEFT JOIN post_search_data ON post_search_data.post_id = bookmarks.bookmarkable_id AND bookmarks.bookmarkable_type = 'Post'"
+      ),
+      "#{ts_query} @@ post_search_data.search_data"
+    )
+  end
+
+  def self.reminder_handler(bookmark)
+    bookmark.user.notifications.create!(
+      notification_type: Notification.types[:bookmark_reminder],
+      topic_id: bookmark.bookmarkable.topic_id,
+      post_number: bookmark.bookmarkable.post_number,
+      data: {
+        title: bookmark.bookmarkable.topic.title,
+        display_username: bookmark.user.username,
+        bookmark_name: bookmark.name,
+        bookmarkable_url: bookmark.bookmarkable.url
+      }.to_json
+    )
+  end
+
+  def self.reminder_conditions(bookmark)
+    bookmark.bookmarkable.present? && bookmark.bookmarkable.topic.present?
+  end
+
+  def self.can_see?(guardian, bookmark)
+    guardian.can_see_post?(bookmark.bookmarkable)
+  end
+end

app/services/registered_bookmarkable.rb

@@ -3,7 +3,7 @@
 # Should only be created via the Bookmark.register_bookmarkable
 # method; this is used to let the BookmarkQuery class query and
 # search additional bookmarks for the user bookmark list, and
-# also to enumerate on the registered Bookmarkable types.
+# also to enumerate on the registered RegisteredBookmarkable types.
 #
 # Post and Topic bookmarkables are registered by default.
 #
@@ -13,37 +13,24 @@
 #
 # See Bookmark#reset_bookmarkables for some examples on how registering
 # bookmarkables works.
-class Bookmarkable
+#
-  attr_reader :model, :serializer, :list_query, :search_query, :preload_associations
+# See BaseBookmarkable for documentation on what return types should be
-  delegate :table_name, to: :@model
+# and what the arguments to the methods are.
+class RegisteredBookmarkable
+  attr_reader :bookmarkable_klass
 
-  def initialize(model:, serializer:, list_query:, search_query:, preload_associations: [])
+  delegate :model, :serializer, to: :@bookmarkable_klass
-    @model = model
+  delegate :table_name, to: :model
-    @serializer = serializer
+
-    @list_query = list_query
+  def initialize(bookmarkable_klass)
-    @search_query = search_query
+    @bookmarkable_klass = bookmarkable_klass
-    @preload_associations = preload_associations
   end
 
-  ##
-  # This is where the main query to filter the bookmarks by the provided bookmarkable
-  # type should occur. This should join on additional tables that are required later
-  # on to preload additional data for serializers, and also is the place where the
-  # bookmarks should be filtered based on security checks, which is why the Guardian
-  # instance is provided.
-  #
-  # @param [User] user The user to perform the query for, this scopes the bookmarks returned.
-  # @param [Guardian] guardian An instance of Guardian for the user to be used for security filters.
   def perform_list_query(user, guardian)
-    list_query.call(user, guardian)
+    bookmarkable_klass.list_query(user, guardian)
   end
 
   ##
-  # Called from BookmarkQuery when the initial results have been returned by
-  # perform_list_query. The search_query should join additional tables required
-  # to filter the bookmarks further, as well as defining a string used for
-  # where_sql, which can include comparisons with the :q parameter.
-  #
   # The block here warrants explanation -- when the search_query is called, we
   # call the provided block with the bookmark relation with additional joins
   # as well as the where_sql string, and then also add the additional OR bookmarks.name
@@ -51,11 +38,9 @@ class Bookmarkable
   # columns _as well as_ the bookmark name, because the bookmark name must always
   # be used in the search.
   #
-  # @param [Bookmark::ActiveRecord_Relation] bookmarks The bookmark records returned by perform_list_query
+  # See BaseBookmarkable#search_query for argument docs.
-  # @param [String] query The search query from the user surrounded by the %% wildcards
-  # @param [String] ts_query The postgres TSQUERY string used for comparisons with full text search columns
   def perform_search_query(bookmarks, query, ts_query)
-    search_query.call(bookmarks, query, ts_query) do |bookmarks_joined, where_sql|
+    bookmarkable_klass.search_query(bookmarks, query, ts_query) do |bookmarks_joined, where_sql|
       bookmarks_joined.where("#{where_sql} OR bookmarks.name ILIKE :q", q: query)
     end
   end
@@ -72,9 +57,27 @@ class Bookmarkable
   #
   # @param [Array] bookmarks The array of bookmarks after initial listing and filtering, note this is
   #                          array _not_ an ActiveRecord::Relation.
+  # @return [void]
   def perform_preload(bookmarks)
+    return if !bookmarkable_klass.has_preloads?
+
     ActiveRecord::Associations::Preloader
-      .new(records: Bookmark.select_type(bookmarks, model.to_s), associations: [bookmarkable: preload_associations])
+      .new(
+        records: Bookmark.select_type(bookmarks, bookmarkable_klass.model.to_s),
+        associations: [bookmarkable: bookmarkable_klass.preload_associations]
+      )
       .call
   end
+
+  def can_send_reminder?(bookmark)
+    bookmarkable_klass.reminder_conditions(bookmark)
+  end
+
+  def send_reminder_notification(bookmark)
+    bookmarkable_klass.reminder_handler(bookmark)
+  end
+
+  def can_see?(guardian, bookmark)
+    bookmarkable_klass.can_see?(guardian, bookmark)
+  end
 end

app/services/topic_bookmarkable.rb

@@ -0,0 +1,57 @@
+# frozen_string_literal: true
+
+class TopicBookmarkable < BaseBookmarkable
+  def self.model
+    Topic
+  end
+
+  def self.serializer
+    UserTopicBookmarkSerializer
+  end
+
+  def self.preload_associations
+    [:topic_users, :tags, { posts: :user }]
+  end
+
+  def self.list_query(user, guardian)
+    topics = Topic.listable_topics.secured(guardian)
+    pms = Topic.private_messages_for_user(user)
+    topic_bookmarks = user
+      .bookmarks_of_type("Topic")
+      .joins("INNER JOIN topics ON topics.id = bookmarks.bookmarkable_id AND bookmarks.bookmarkable_type = 'Topic'")
+      .joins("LEFT JOIN topic_users ON topic_users.topic_id = topics.id")
+      .where("topic_users.user_id = ?", user.id)
+    guardian.filter_allowed_categories(topic_bookmarks.merge(topics.or(pms)))
+  end
+
+  def self.search_query(bookmarks, query, ts_query, &bookmarkable_search)
+    bookmarkable_search.call(
+      bookmarks
+      .joins("LEFT JOIN posts ON posts.topic_id = topics.id AND posts.post_number = 1")
+      .joins("LEFT JOIN post_search_data ON post_search_data.post_id = posts.id"),
+    "#{ts_query} @@ post_search_data.search_data"
+    )
+  end
+
+  def self.reminder_handler(bookmark)
+    bookmark.user.notifications.create!(
+      notification_type: Notification.types[:bookmark_reminder],
+      topic_id: bookmark.bookmarkable_id,
+      post_number: 1,
+      data: {
+        title: bookmark.bookmarkable.title,
+        display_username: bookmark.user.username,
+        bookmark_name: bookmark.name,
+        bookmarkable_url: bookmark.bookmarkable.first_post.url
+      }.to_json
+    )
+  end
+
+  def self.reminder_conditions(bookmark)
+    bookmark.bookmarkable.present?
+  end
+
+  def self.can_see?(guardian, bookmark)
+    guardian.can_see_topic?(bookmark.bookmarkable)
+  end
+end

lib/bookmark_manager.rb

@@ -135,7 +135,7 @@ class BookmarkManager
 
   def self.send_reminder_notification(id)
     bookmark = Bookmark.find_by(id: id)
-    BookmarkReminderNotificationHandler.send_notification(bookmark)
+    BookmarkReminderNotificationHandler.new(bookmark).send_notification
   end
 
   def update(bookmark_id:, name:, reminder_at:, options: {})

lib/bookmark_reminder_notification_handler.rb

@@ -1,30 +1,35 @@
 # frozen_string_literal: true
 
 class BookmarkReminderNotificationHandler
-  def self.send_notification(bookmark)
+  attr_reader :bookmark
+
+  def initialize(bookmark)
+    @bookmark = bookmark
+  end
+
+  def send_notification
     return if bookmark.blank?
     Bookmark.transaction do
-      # we don't send reminders for deleted posts or topics,
+      # TODO (martin) [POLYBOOK] Can probably change this to call the
-      # just as we don't allow creation of bookmarks for deleted
+      # can_send_reminder? on the registered bookmarkable directly instead
-      # posts or topics
+      # of having can_send_reminder?
-      #
+      if !can_send_reminder?
-      # TODO (martin) [POLYBOOK] This will need to be restructured for polymorphic
+        clear_reminder
-      # bookmarks when reminders are handled.
-      if bookmark.post.blank? || bookmark.topic.blank?
-        clear_reminder(bookmark)
       else
-        create_notification(bookmark)
+        create_notification
 
         if bookmark.auto_delete_when_reminder_sent?
           BookmarkManager.new(bookmark.user).destroy(bookmark.id)
         end
 
-        clear_reminder(bookmark)
+        clear_reminder
       end
     end
   end
 
-  def self.clear_reminder(bookmark)
+  private
+
+  def clear_reminder
     Rails.logger.debug(
       "Clearing bookmark reminder for bookmark_id #{bookmark.id}. reminder at: #{bookmark.reminder_at}"
     )
@@ -36,17 +41,28 @@ class BookmarkReminderNotificationHandler
     bookmark.clear_reminder!
   end
 
-  def self.create_notification(bookmark)
+  def can_send_reminder?
-    user = bookmark.user
+    if SiteSetting.use_polymorphic_bookmarks
-    user.notifications.create!(
+      bookmark.registered_bookmarkable.can_send_reminder?(bookmark)
-      notification_type: Notification.types[:bookmark_reminder],
+    else
-      topic_id: bookmark.topic_id,
+      bookmark.post.present? && bookmark.topic.present?
-      post_number: bookmark.post.post_number,
+    end
-      data: {
+  end
-        topic_title: bookmark.topic.title,
+
-        display_username: user.username,
+  def create_notification
-        bookmark_name: bookmark.name
+    if SiteSetting.use_polymorphic_bookmarks
-      }.to_json
+      bookmark.registered_bookmarkable.send_reminder_notification(bookmark)
-    )
+    else
+      bookmark.user.notifications.create!(
+        notification_type: Notification.types[:bookmark_reminder],
+        topic_id: bookmark.topic_id,
+        post_number: bookmark.post.post_number,
+        data: {
+          topic_title: bookmark.topic.title,
+          display_username: bookmark.user.username,
+          bookmark_name: bookmark.name
+        }.to_json
+      )
+    end
   end
 end

lib/guardian/bookmark_guardian.rb

@@ -9,7 +9,11 @@ module BookmarkGuardian
     @user == bookmark.user
   end
 
-  def can_create_bookmark?(bookmark)
+  def can_see_bookmarkable?(bookmark)
-    can_see_topic?(bookmark.topic)
+    if SiteSetting.use_polymorphic_bookmarks?
+      return bookmark.registered_bookmarkable.can_see?(self, bookmark)
+    end
+
+    self.can_see_post?(bookmark.post)
   end
 end

lib/search.rb

@@ -453,6 +453,8 @@ class Search
     end
   end
 
+  # TODO (martin) [POLYBOOK] Make a separate PR for advanced searched in:bookmarks
+  # functionality as the bookmarkables will have to define this.
   advanced_filter(/^in:(bookmarks)$/i) do |posts, match|
     if @guardian.user
       posts.where("posts.id IN (SELECT post_id FROM bookmarks WHERE bookmarks.user_id = #{@guardian.user.id})")

plugins/discourse-narrative-bot/lib/discourse_narrative_bot/new_user_narrative.rb

@@ -243,6 +243,7 @@ module DiscourseNarrativeBot
       post
     end
 
+    # TODO (martin) [POLYBOOK] Fix up narrative bot bookmark interactions in a separate PR.
     def missing_bookmark
       return unless valid_topic?(@post.topic_id)
       return if @post.user_id == self.discobot_user.id
@@ -253,6 +254,7 @@ module DiscourseNarrativeBot
       false
     end
 
+    # TODO (martin) [POLYBOOK] Fix up narrative bot bookmark interactions in a separate PR.
     def reply_to_bookmark
       return unless valid_topic?(@post.topic_id)
       return unless @post.user_id == self.discobot_user.id

plugins/discourse-narrative-bot/plugin.rb

@@ -281,6 +281,7 @@ after_initialize do
     end
   end
 
+  # TODO (martin) [POLYBOOK] Fix up narrative bot bookmark interactions in a separate PR.
   self.add_model_callback(Bookmark, :after_commit, on: :create) do
     if self.post && self.user.enqueue_narrative_bot_job?
       Jobs.enqueue(:bot_input, user_id: self.user_id, post_id: self.post_id, input: "bookmark")

script/import_scripts/base.rb

@@ -624,7 +624,12 @@ class ImportScripts::Base
         else
           begin
             manager = BookmarkManager.new(user)
-            bookmark = manager.create(post_id: post.id)
+
+            if SiteSetting.use_polymorphic_bookmarks
+              bookmark = manager.create_for(bookmarkable_id: post.id, bookmarkable_type: "Post")
+            else
+              bookmark = manager.create(post_id: post.id)
+            end
 
             created += 1 if manager.errors.none?
             skipped += 1 if manager.errors.any?

script/import_scripts/jive_api.rb

@@ -285,10 +285,14 @@ class ImportScripts::JiveApi < ImportScripts::Base
 
     loop do
       favorites = get("contents?#{fields}&filter=type(favorite)#{filter}&sort=dateCreatedAsc&count=#{POST_COUNT}&startIndex=#{start_index}")
-      favorites["list"].each do |favorite|
+      bookmarks_to_create = favorites["list"].map do |favorite|
         next unless user_id = user_id_from_imported_user_id(favorite["author"]["id"])
         next unless post_id = post_id_from_imported_post_id(favorite["favoriteObject"]["id"])
-        PostActionCreator.create(User.find(user_id), Post.find(post_id), :bookmark)
+        { user_id: user_id, post_id: post_id }
+      end.flatten
+
+      create_bookmarks(bookmarks_to_create) do |row|
+        row
       end
 
       break if favorites["list"].size < POST_COUNT || favorites.dig("links", "next").blank?

spec/jobs/bookmark_reminder_notifications_spec.rb

@@ -3,10 +3,11 @@
 RSpec.describe Jobs::BookmarkReminderNotifications do
   subject { described_class.new }
 
+  fab!(:user) { Fabricate(:user) }
   let(:five_minutes_ago) { Time.zone.now - 5.minutes }
-  let(:bookmark1) { Fabricate(:bookmark) }
+  let(:bookmark1) { Fabricate(:bookmark, user: user) }
-  let(:bookmark2) { Fabricate(:bookmark) }
+  let(:bookmark2) { Fabricate(:bookmark, user: user) }
-  let(:bookmark3) { Fabricate(:bookmark) }
+  let(:bookmark3) { Fabricate(:bookmark, user: user) }
   let!(:bookmarks) do
     [
       bookmark1,
@@ -34,13 +35,14 @@ RSpec.describe Jobs::BookmarkReminderNotifications do
   end
 
   it "will not send a reminder for a bookmark in the future" do
+    freeze_time
     bookmark4 = Fabricate(:bookmark, reminder_at: Time.zone.now + 1.day)
-    BookmarkReminderNotificationHandler.expects(:send_notification).with(bookmark1)
+    expect { subject.execute }.to change { Notification.where(user: user).count }.by(3)
-    BookmarkReminderNotificationHandler.expects(:send_notification).with(bookmark2)
+    expect(bookmark1.reload.reminder_last_sent_at).to eq_time(Time.zone.now)
-    BookmarkReminderNotificationHandler.expects(:send_notification).with(bookmark3)
+    expect(bookmark2.reload.reminder_last_sent_at).to eq_time(Time.zone.now)
-    BookmarkReminderNotificationHandler.expects(:send_notification).with(bookmark4).never
+    expect(bookmark3.reload.reminder_last_sent_at).to eq_time(Time.zone.now)
-    subject.execute
     expect(bookmark4.reload.reminder_at).not_to eq(nil)
+    expect(bookmark4.reload.reminder_last_sent_at).to eq(nil)
   end
 
   context "when a user is over the bookmark limit" do

spec/jobs/export_user_archive_spec.rb

@@ -276,27 +276,27 @@ describe Jobs::ExportUserArchive do
     let(:post1) { Fabricate(:post, topic: topic1, post_number: 5) }
     let(:post2) { Fabricate(:post) }
     let(:post3) { Fabricate(:post) }
-    let(:message) { Fabricate(:private_message_topic) }
+    let(:private_message_topic) { Fabricate(:private_message_topic) }
-    let(:post4) { Fabricate(:post, topic: message) }
+    let(:post4) { Fabricate(:post, topic: private_message_topic) }
     let(:reminder_at) { 1.day.from_now }
 
     it 'properly includes bookmark records' do
       now = freeze_time '2017-03-01 12:00'
 
-      bkmk1 = manager.create(post_id: post1.id, name: name)
+      bookmark1 = manager.create(post_id: post1.id, name: name)
       update1_at = now + 1.hours
-      bkmk1.update(name: 'great food recipe', updated_at: update1_at)
+      bookmark1.update(name: 'great food recipe', updated_at: update1_at)
 
       manager.create(post_id: post2.id, name: name, reminder_at: reminder_at, options: { auto_delete_preference: Bookmark.auto_delete_preferences[:when_reminder_sent] })
       twelve_hr_ago = freeze_time now - 12.hours
       pending_reminder = manager.create(post_id: post3.id, name: name, reminder_at: now - 8.hours)
       freeze_time now
 
-      tau_record = message.topic_allowed_users.create!(user_id: user.id)
+      tau_record = private_message_topic.topic_allowed_users.create!(user_id: user.id)
       manager.create(post_id: post4.id, name: name)
       tau_record.destroy!
 
-      BookmarkReminderNotificationHandler.send_notification(pending_reminder)
+      BookmarkReminderNotificationHandler.new(pending_reminder).send_notification
 
       data, _csv_out = make_component_csv
 
@@ -316,10 +316,56 @@ describe Jobs::ExportUserArchive do
       expect(DateTime.parse(data[2]['reminder_last_sent_at'])).to eq(DateTime.parse(now.to_s))
       expect(data[2]['reminder_set_at']).to eq('')
 
-      expect(data[3]['topic_id']).to eq(message.id.to_s)
+      expect(data[3]['topic_id']).to eq(private_message_topic.id.to_s)
       expect(data[3]['link']).to eq('')
     end
 
+    context "for polymorphic bookmarks" do
+      let(:component) { 'bookmarks_polymorphic' }
+      before do
+        SiteSetting.use_polymorphic_bookmarks = true
+      end
+
+      it "properly includes bookmark records" do
+        now = freeze_time '2017-03-01 12:00'
+
+        bookmark1 = manager.create_for(bookmarkable_id: post1.id, bookmarkable_type: "Post", name: name)
+        update1_at = now + 1.hours
+        bookmark1.update(name: 'great food recipe', updated_at: update1_at)
+
+        manager.create_for(bookmarkable_id: post2.id, bookmarkable_type: "Post", name: name, reminder_at: reminder_at, options: { auto_delete_preference: Bookmark.auto_delete_preferences[:when_reminder_sent] })
+        twelve_hr_ago = freeze_time now - 12.hours
+        pending_reminder = manager.create_for(bookmarkable_id: post3.id, bookmarkable_type: "Post", name: name, reminder_at: now - 8.hours)
+        freeze_time now
+
+        tau_record = private_message_topic.topic_allowed_users.create!(user_id: user.id)
+        manager.create_for(bookmarkable_id: post4.id, bookmarkable_type: "Post", name: name)
+        tau_record.destroy!
+
+        BookmarkReminderNotificationHandler.new(pending_reminder).send_notification
+
+        data, _csv_out = make_component_csv
+
+        expect(data.length).to eq(4)
+
+        expect(data[0]['bookmarkable_id']).to eq(post1.id.to_s)
+        expect(data[0]['bookmarkable_type']).to eq("Post")
+        expect(data[0]['link']).to eq(post1.full_url)
+        expect(DateTime.parse(data[0]['updated_at'])).to eq(DateTime.parse(update1_at.to_s))
+
+        expect(data[1]['name']).to eq(name)
+        expect(DateTime.parse(data[1]['reminder_at'])).to eq(DateTime.parse(reminder_at.to_s))
+        expect(data[1]['auto_delete_preference']).to eq('when_reminder_sent')
+
+        expect(DateTime.parse(data[2]['created_at'])).to eq(DateTime.parse(twelve_hr_ago.to_s))
+        expect(DateTime.parse(data[2]['reminder_last_sent_at'])).to eq(DateTime.parse(now.to_s))
+        expect(data[2]['reminder_set_at']).to eq('')
+
+        expect(data[3]['bookmarkable_id']).to eq(post4.id.to_s)
+        expect(data[3]['bookmarkable_type']).to eq("Post")
+        expect(data[3]['link']).to eq('')
+      end
+    end
   end
 
   context 'category_preferences' do

spec/jobs/sync_topic_user_bookmarked_spec.rb

@@ -1,18 +1,18 @@
 # frozen_string_literal: true
 
 RSpec.describe Jobs::SyncTopicUserBookmarked do
+  fab!(:topic) { Fabricate(:topic) }
+  fab!(:post1) { Fabricate(:post, topic: topic) }
+  fab!(:post2) { Fabricate(:post, topic: topic) }
+  fab!(:post3) { Fabricate(:post, topic: topic) }
+
+  fab!(:tu1) { Fabricate(:topic_user, topic: topic, bookmarked: false) }
+  fab!(:tu2) { Fabricate(:topic_user, topic: topic, bookmarked: false) }
+  fab!(:tu3) { Fabricate(:topic_user, topic: topic, bookmarked: true) }
+  fab!(:tu4) { Fabricate(:topic_user, topic: topic, bookmarked: true) }
+  fab!(:tu5) { Fabricate(:topic_user, topic: topic, bookmarked: true) }
+
   it "corrects all topic_users.bookmarked records for the topic" do
-    topic = Fabricate(:topic)
-    Fabricate(:post, topic: topic)
-    Fabricate(:post, topic: topic)
-    Fabricate(:post, topic: topic)
-
-    tu1 = Fabricate(:topic_user, topic: topic, bookmarked: false)
-    tu2 = Fabricate(:topic_user, topic: topic, bookmarked: false)
-    tu3 = Fabricate(:topic_user, topic: topic, bookmarked: true)
-    tu4 = Fabricate(:topic_user, topic: topic, bookmarked: true)
-    tu5 = Fabricate(:topic_user, bookmarked: false)
-
     Fabricate(:bookmark, user: tu1.user, post: topic.posts.sample)
     Fabricate(:bookmark, user: tu4.user, post: topic.posts.sample)
 
@@ -26,12 +26,6 @@ RSpec.describe Jobs::SyncTopicUserBookmarked do
   end
 
   it "does not consider topic as bookmarked if the bookmarked post is deleted" do
-    topic = Fabricate(:topic)
-    post1 = Fabricate(:post, topic: topic)
-
-    tu1 = Fabricate(:topic_user, topic: topic, bookmarked: false)
-    tu2 = Fabricate(:topic_user, topic: topic, bookmarked: true)
-
     Fabricate(:bookmark, user: tu1.user, post: post1)
     Fabricate(:bookmark, user: tu2.user, post: post1)
 
@@ -44,17 +38,6 @@ RSpec.describe Jobs::SyncTopicUserBookmarked do
   end
 
   it "works when no topic id is provided (runs for all topics)" do
-    topic = Fabricate(:topic)
-    Fabricate(:post, topic: topic)
-    Fabricate(:post, topic: topic)
-    Fabricate(:post, topic: topic)
-
-    tu1 = Fabricate(:topic_user, topic: topic, bookmarked: false)
-    tu2 = Fabricate(:topic_user, topic: topic, bookmarked: false)
-    tu3 = Fabricate(:topic_user, topic: topic, bookmarked: true)
-    tu4 = Fabricate(:topic_user, topic: topic, bookmarked: true)
-    tu5 = Fabricate(:topic_user, bookmarked: false)
-
     Fabricate(:bookmark, user: tu1.user, post: topic.posts.sample)
     Fabricate(:bookmark, user: tu4.user, post: topic.posts.sample)
 
@@ -66,4 +49,48 @@ RSpec.describe Jobs::SyncTopicUserBookmarked do
     expect(tu4.reload.bookmarked).to eq(true)
     expect(tu5.reload.bookmarked).to eq(false)
   end
+
+  context "for polymorphic bookmarks" do
+    before do
+      SiteSetting.use_polymorphic_bookmarks = true
+    end
+
+    it "corrects all topic_users.bookmarked records for the topic" do
+      Fabricate(:bookmark, user: tu1.user, bookmarkable: topic.posts.sample)
+      Fabricate(:bookmark, user: tu4.user, bookmarkable: topic.posts.sample)
+
+      subject.execute(topic_id: topic.id)
+
+      expect(tu1.reload.bookmarked).to eq(true)
+      expect(tu2.reload.bookmarked).to eq(false)
+      expect(tu3.reload.bookmarked).to eq(false)
+      expect(tu4.reload.bookmarked).to eq(true)
+      expect(tu5.reload.bookmarked).to eq(false)
+    end
+
+    it "does not consider topic as bookmarked if the bookmarked post is deleted" do
+      Fabricate(:bookmark, user: tu1.user, bookmarkable: post1)
+      Fabricate(:bookmark, user: tu2.user, bookmarkable: post1)
+
+      post1.trash!
+
+      subject.execute(topic_id: topic.id)
+
+      expect(tu1.reload.bookmarked).to eq(false)
+      expect(tu2.reload.bookmarked).to eq(false)
+    end
+
+    it "works when no topic id is provided (runs for all topics)" do
+      Fabricate(:bookmark, user: tu1.user, bookmarkable: topic.posts.sample)
+      Fabricate(:bookmark, user: tu4.user, bookmarkable: topic.posts.sample)
+
+      subject.execute
+
+      expect(tu1.reload.bookmarked).to eq(true)
+      expect(tu2.reload.bookmarked).to eq(false)
+      expect(tu3.reload.bookmarked).to eq(false)
+      expect(tu4.reload.bookmarked).to eq(true)
+      expect(tu5.reload.bookmarked).to eq(false)
+    end
+  end
 end

spec/lib/bookmark_query_spec.rb

@@ -12,21 +12,6 @@ RSpec.describe BookmarkQuery do
     BookmarkQuery.new(user: user || self.user, params: params || self.params)
   end
 
-  def register_user_bookmarkable
-    Bookmark.register_bookmarkable(
-      model: User,
-      serializer: UserBookmarkSerializer,
-      list_query: lambda do |user, guardian|
-        user.bookmarks.joins(
-          "INNER JOIN users ON users.id = bookmarks.bookmarkable_id AND bookmarks.bookmarkable_type = 'User'"
-        ).where(bookmarkable_type: "User")
-      end,
-      search_query: lambda do |bookmarks, query, ts_query|
-        bookmarks.where("users.username ILIKE ?", query)
-      end
-    )
-  end
-
   describe "#list_all" do
     fab!(:bookmark1) { Fabricate(:bookmark, user: user) }
     fab!(:bookmark2) { Fabricate(:bookmark, user: user) }
@@ -69,7 +54,7 @@ RSpec.describe BookmarkQuery do
       before do
         SiteSetting.use_polymorphic_bookmarks = true
         Bookmark.reset_bookmarkables
-        register_user_bookmarkable
+        register_test_bookmarkable
 
         Fabricate(:topic_user, user: user, topic: post_bookmark.bookmarkable.topic)
         Fabricate(:topic_user, user: user, topic: topic_bookmark.bookmarkable)
@@ -153,7 +138,7 @@ RSpec.describe BookmarkQuery do
 
         context "with custom bookmarkable fitering" do
           before do
-            register_user_bookmarkable
+            register_test_bookmarkable
           end
 
           let!(:bookmark5) { Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:user, username: "bookmarkqueen")) }

spec/lib/bookmark_reminder_notification_handler_spec.rb

@@ -10,12 +10,12 @@ RSpec.describe BookmarkReminderNotificationHandler do
   end
 
   describe "#send_notification" do
-    fab!(:bookmark) do
+    let!(:bookmark) do
       Fabricate(:bookmark_next_business_day_reminder, user: user)
     end
 
     it "creates a bookmark reminder notification with the correct details" do
-      subject.send_notification(bookmark)
+      subject.new(bookmark).send_notification
       notif = bookmark.user.notifications.last
       expect(notif.notification_type).to eq(Notification.types[:bookmark_reminder])
       expect(notif.topic_id).to eq(bookmark.topic_id)
@@ -33,7 +33,7 @@ RSpec.describe BookmarkReminderNotificationHandler do
       end
 
       it "does not send a notification and updates last notification attempt time" do
-        expect { subject.send_notification(bookmark) }.not_to change { Notification.count }
+        expect { subject.new(bookmark).send_notification }.not_to change { Notification.count }
         expect(bookmark.reload.reminder_last_sent_at).not_to be_blank
       end
     end
@@ -45,11 +45,46 @@ RSpec.describe BookmarkReminderNotificationHandler do
       end
 
       it "does not send a notification and updates last notification attempt time" do
-        expect { subject.send_notification(bookmark) }.not_to change { Notification.count }
+        expect { subject.new(bookmark).send_notification }.not_to change { Notification.count }
         expect(bookmark.reload.reminder_last_sent_at).not_to be_blank
       end
     end
 
+    context "using polymorphic bookmarks" do
+      before do
+        SiteSetting.use_polymorphic_bookmarks = true
+      end
+
+      let!(:bookmark) do
+        Fabricate(:bookmark_next_business_day_reminder, user: user, bookmarkable: Fabricate(:post))
+      end
+
+      it "creates a bookmark reminder notification with the correct details" do
+        subject.new(bookmark).send_notification
+        notif = bookmark.user.notifications.last
+        expect(notif.notification_type).to eq(Notification.types[:bookmark_reminder])
+        expect(notif.topic_id).to eq(bookmark.bookmarkable.topic_id)
+        expect(notif.post_number).to eq(bookmark.bookmarkable.post_number)
+        data = JSON.parse(notif.data)
+        expect(data["title"]).to eq(bookmark.bookmarkable.topic.title)
+        expect(data["display_username"]).to eq(bookmark.user.username)
+        expect(data["bookmark_name"]).to eq(bookmark.name)
+        expect(data["bookmarkable_url"]).to eq(bookmark.bookmarkable.url)
+      end
+
+      context "when the bookmarkable is deleted" do
+        before do
+          bookmark.bookmarkable.trash!
+          bookmark.reload
+        end
+
+        it "does not send a notification and updates last notification attempt time" do
+          expect { subject.new(bookmark).send_notification }.not_to change { Notification.count }
+          expect(bookmark.reload.reminder_last_sent_at).not_to be_blank
+        end
+      end
+    end
+
     context "when the auto_delete_preference is when_reminder_sent" do
       before do
         TopicUser.create!(topic: bookmark.topic, user: user, bookmarked: true)
@@ -57,12 +92,12 @@ RSpec.describe BookmarkReminderNotificationHandler do
       end
 
       it "deletes the bookmark after the reminder gets sent" do
-        subject.send_notification(bookmark)
+        subject.new(bookmark).send_notification
         expect(Bookmark.find_by(id: bookmark.id)).to eq(nil)
       end
 
       it "changes the TopicUser bookmarked column to false" do
-        subject.send_notification(bookmark)
+        subject.new(bookmark).send_notification
         expect(TopicUser.find_by(topic: bookmark.topic, user: user).bookmarked).to eq(false)
       end
 
@@ -72,7 +107,7 @@ RSpec.describe BookmarkReminderNotificationHandler do
         end
 
         it "does not change the TopicUser bookmarked column to false" do
-          subject.send_notification(bookmark)
+          subject.new(bookmark).send_notification
           expect(TopicUser.find_by(topic: bookmark.topic, user: user).bookmarked).to eq(true)
         end
       end
@@ -85,7 +120,7 @@ RSpec.describe BookmarkReminderNotificationHandler do
       end
 
       it "resets reminder_at after the reminder gets sent" do
-        subject.send_notification(bookmark)
+        subject.new(bookmark).send_notification
         expect(Bookmark.find_by(id: bookmark.id).reminder_at).to eq(nil)
       end
     end
@@ -94,7 +129,7 @@ RSpec.describe BookmarkReminderNotificationHandler do
       it "does not send a notification" do
         bookmark.post.trash!
         bookmark.reload
-        expect { subject.send_notification(bookmark) }.not_to change { Notification.count }
+        expect { subject.new(bookmark).send_notification }.not_to change { Notification.count }
         expect(bookmark.reload.reminder_last_sent_at).not_to be_blank
       end
     end

spec/models/bookmark_spec.rb

@@ -68,18 +68,7 @@ describe Bookmark do
         user = Fabricate(:user)
         bm = Bookmark.create(bookmarkable_type: "User", bookmarkable: Fabricate(:user), user: user)
         expect(bm.errors.full_messages).to include(I18n.t("bookmarks.errors.invalid_bookmarkable", type: "User"))
-        Bookmark.register_bookmarkable(
+        register_test_bookmarkable
-          model: User,
-          serializer: UserBookmarkSerializer,
-          list_query: lambda do |bookmark_user, guardian|
-            bookmark_user.bookmarks.joins(
-              "INNER JOIN users ON users.id = bookmarks.bookmarkable_id AND bookmarks.bookmarkable_type = 'User'"
-            ).where(bookmarkable_type: "User")
-          end,
-          search_query: lambda do |bookmarks, query, ts_query|
-            bookmarks.where("users.username ILIKE ?", query)
-          end
-        )
         expect(bm.valid?).to eq(true)
       end
     end

spec/models/user_bookmark_list_spec.rb

@@ -29,18 +29,7 @@ RSpec.describe UserBookmarkList do
   context "for polymorphic bookmarks" do
     before do
       SiteSetting.use_polymorphic_bookmarks = true
-    Bookmark.register_bookmarkable(
+      register_test_bookmarkable
-      model: User,
-      serializer: UserBookmarkSerializer,
-      list_query: lambda do |user, guardian|
-        user.bookmarks.joins(
-          "INNER JOIN users ON users.id = bookmarks.bookmarkable_id AND bookmarks.bookmarkable_type = 'User'"
-        ).where(bookmarkable_type: "User")
-      end,
-      search_query: lambda do |bookmarks, query, ts_query|
-        bookmarks.where("users.username ILIKE ?", query)
-      end
-    )
 
       Fabricate(:topic_user, user: user, topic: post_bookmark.bookmarkable.topic)
       Fabricate(:topic_user, user: user, topic: topic_bookmark.bookmarkable)

spec/requests/users_controller_spec.rb

@@ -5352,18 +5352,7 @@ describe UsersController do
 
       before do
         SiteSetting.use_polymorphic_bookmarks = true
-        Bookmark.register_bookmarkable(
+        register_test_bookmarkable
-          model: User,
-          serializer: UserTestBookmarkSerializer,
-          list_query: lambda do |user, guardian|
-            user.bookmarks.joins(
-              "INNER JOIN users ON users.id = bookmarks.bookmarkable_id AND bookmarks.bookmarkable_type = 'User'"
-            ).where(bookmarkable_type: "User")
-          end,
-          search_query: lambda do |bookmarks, query, ts_query|
-            bookmarks.where("users.username ILIKE ?", query)
-          end
-        )
         TopicUser.change(user1.id, bookmark1.bookmarkable.topic_id, total_msecs_viewed: 1)
         TopicUser.change(user1.id, bookmark2.bookmarkable_id, total_msecs_viewed: 1)
         Fabricate(:post, topic: bookmark2.bookmarkable)

spec/script/import_scripts/base_spec.rb

@@ -55,6 +55,20 @@ describe ImportScripts::Base do
     expect(SiteSetting.purge_unactivated_users_grace_period_days).to eq(60)
   end
 
+  context "when polymorphic bookmarks are enabled" do
+    before do
+      SiteSetting.use_polymorphic_bookmarks = true
+    end
+
+    it "creates bookmarks, posts, and users" do
+      MockSpecImporter.new(import_data).perform
+      expect(Bookmark.where(bookmarkable_type: "Post").count).to eq(5)
+      expect(Post.count).to eq(5)
+      expect(User.where('id > 0').count).to eq(1)
+      expect(SiteSetting.purge_unactivated_users_grace_period_days).to eq(60)
+    end
+  end
+
   it "does not change purge unactivated users setting if disabled" do
     SiteSetting.purge_unactivated_users_grace_period_days = 0
     MockSpecImporter.new(import_data).perform

spec/serializers/user_bookmark_list_serializer_spec.rb

@@ -1,26 +1,13 @@
 # frozen_string_literal: true
 
 RSpec.describe UserBookmarkListSerializer do
-  class UserTestBookmarkSerializer < UserBookmarkBaseSerializer; end
 
   fab!(:user) { Fabricate(:user) }
 
   context "for polymorphic bookmarks" do
     before do
       SiteSetting.use_polymorphic_bookmarks = true
-      Bookmark.register_bookmarkable(
+      register_test_bookmarkable
-        model: User,
-        serializer: UserTestBookmarkSerializer,
-        list_query: lambda do |user, guardian|
-          user.bookmarks.joins(
-            "INNER JOIN users ON users.id = bookmarks.bookmarkable_id AND bookmarks.bookmarkable_type = 'User'"
-          ).where(bookmarkable_type: "User")
-        end,
-        search_query: lambda do |bookmarks, query, ts_query|
-          bookmarks.where("users.username ILIKE ?", query)
-        end
-      )
-
       Fabricate(:topic_user, user: user, topic: post_bookmark.bookmarkable.topic)
       Fabricate(:topic_user, user: user, topic: topic_bookmark.bookmarkable)
       user_bookmark

spec/services/post_bookmarkable_spec.rb

@@ -0,0 +1,106 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+describe PostBookmarkable do
+  fab!(:user) { Fabricate(:user) }
+  fab!(:guardian) { Guardian.new(user) }
+  fab!(:private_category) { Fabricate(:private_category, group: Fabricate(:group)) }
+
+  before do
+    SiteSetting.use_polymorphic_bookmarks = true
+  end
+
+  let!(:post1) { Fabricate(:post) }
+  let!(:post2) { Fabricate(:post) }
+  let!(:bookmark1) { Fabricate(:bookmark, user: user, bookmarkable: post1, name: "something i gotta do") }
+  let!(:bookmark2) { Fabricate(:bookmark, user: user, bookmarkable: post2) }
+  let!(:bookmark3) { Fabricate(:bookmark) }
+  let!(:topic_user1) { Fabricate(:topic_user, user: user, topic: post1.topic) }
+  let!(:topic_user2) { Fabricate(:topic_user, user: user, topic: post2.topic) }
+
+  subject { RegisteredBookmarkable.new(PostBookmarkable) }
+
+  describe "#perform_list_query" do
+    it "returns all the user's bookmarks" do
+      expect(subject.perform_list_query(user, guardian).map(&:id)).to match_array([bookmark1.id, bookmark2.id])
+    end
+
+    it "does not return bookmarks for posts where the user does not have access to the topic category" do
+      bookmark1.bookmarkable.topic.update(category: private_category)
+      expect(subject.perform_list_query(user, guardian).map(&:id)).to match_array([bookmark2.id])
+    end
+
+    it "does not return bookmarks for posts where the user does not have access to the private message" do
+      bookmark1.bookmarkable.update(topic: Fabricate(:private_message_topic))
+      expect(subject.perform_list_query(user, guardian).map(&:id)).to match_array([bookmark2.id])
+    end
+  end
+
+  describe "#perform_search_query" do
+    before do
+      SearchIndexer.enable
+    end
+
+    it "returns bookmarks that match by name" do
+      ts_query = Search.ts_query(term: "gotta", ts_config: "simple")
+      expect(subject.perform_search_query(subject.perform_list_query(user, guardian), "%gotta%", ts_query).map(&:id)).to match_array([bookmark1.id])
+    end
+
+    it "returns bookmarks that match by post search data (topic title or post content)" do
+      post2.update(raw: "some post content")
+      post2.topic.update(title: "a great topic title")
+
+      ts_query = Search.ts_query(term: "post content", ts_config: "simple")
+      expect(subject.perform_search_query(subject.perform_list_query(user, guardian), "%post content%", ts_query).map(&:id)).to match_array([bookmark2.id])
+
+      ts_query = Search.ts_query(term: "great topic", ts_config: "simple")
+      expect(subject.perform_search_query(subject.perform_list_query(user, guardian), "%great topic%", ts_query).map(&:id)).to match_array([bookmark2.id])
+
+      ts_query = Search.ts_query(term: "blah", ts_config: "simple")
+      expect(subject.perform_search_query(subject.perform_list_query(user, guardian), "%blah%", ts_query).map(&:id)).to eq([])
+    end
+  end
+
+  describe "#can_send_reminder?" do
+    it "cannot send reminder if the post or topic is deleted" do
+      expect(subject.can_send_reminder?(bookmark1)).to eq(true)
+      bookmark1.bookmarkable.trash!
+      bookmark1.reload
+      expect(subject.can_send_reminder?(bookmark1)).to eq(false)
+      Post.with_deleted.find_by(id: bookmark1.bookmarkable_id).recover!
+      bookmark1.reload
+      bookmark1.bookmarkable.topic.trash!
+      bookmark1.reload
+      expect(subject.can_send_reminder?(bookmark1)).to eq(false)
+    end
+  end
+
+  describe "#reminder_handler" do
+    it "creates a notification for the user with the correct details" do
+      expect { subject.send_reminder_notification(bookmark1) }.to change { Notification.count }.by(1)
+      notif = user.notifications.last
+      expect(notif.notification_type).to eq(Notification.types[:bookmark_reminder])
+      expect(notif.topic_id).to eq(bookmark1.bookmarkable.topic_id)
+      expect(notif.post_number).to eq(bookmark1.bookmarkable.post_number)
+      expect(notif.data).to eq(
+        {
+          title: bookmark1.bookmarkable.topic.title,
+          display_username: bookmark1.user.username,
+          bookmark_name: bookmark1.name,
+          bookmarkable_url: bookmark1.bookmarkable.url
+        }.to_json
+      )
+    end
+  end
+
+  describe "#can_see?" do
+    it "returns false if the post is in a private category or private message the user cannot see" do
+      expect(subject.can_see?(guardian, bookmark1)).to eq(true)
+      bookmark1.bookmarkable.topic.update(category: private_category)
+      expect(subject.can_see?(guardian, bookmark1)).to eq(false)
+      bookmark1.bookmarkable.update(topic: Fabricate(:private_message_topic))
+      expect(subject.can_see?(guardian, bookmark1)).to eq(false)
+    end
+  end
+end

spec/services/topic_bookmarkable_spec.rb

@@ -0,0 +1,102 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+describe TopicBookmarkable do
+  fab!(:user) { Fabricate(:user) }
+  fab!(:guardian) { Guardian.new(user) }
+  fab!(:private_category) { Fabricate(:private_category, group: Fabricate(:group)) }
+
+  before do
+    SiteSetting.use_polymorphic_bookmarks = true
+  end
+
+  let!(:topic1) { Fabricate(:topic) }
+  let!(:topic2) { Fabricate(:topic) }
+  let!(:post) { Fabricate(:post, topic: topic1) }
+  let!(:bookmark1) { Fabricate(:bookmark, user: user, bookmarkable: topic1, name: "something i gotta do") }
+  let!(:bookmark2) { Fabricate(:bookmark, user: user, bookmarkable: topic2) }
+  let!(:bookmark3) { Fabricate(:bookmark) }
+  let!(:topic_user1) { Fabricate(:topic_user, user: user, topic: topic1) }
+  let!(:topic_user2) { Fabricate(:topic_user, user: user, topic: topic2) }
+
+  subject { RegisteredBookmarkable.new(TopicBookmarkable) }
+
+  describe "#perform_list_query" do
+    it "returns all the user's bookmarks" do
+      expect(subject.perform_list_query(user, guardian).map(&:id)).to match_array([bookmark1.id, bookmark2.id])
+    end
+
+    it "does not return bookmarks for posts where the user does not have access to the topic category" do
+      bookmark1.bookmarkable.update!(category: private_category)
+      expect(subject.perform_list_query(user, guardian).map(&:id)).to match_array([bookmark2.id])
+    end
+
+    it "does not return bookmarks for posts where the user does not have access to the private message" do
+      bookmark1.update!(bookmarkable: Fabricate(:private_message_topic))
+      expect(subject.perform_list_query(user, guardian).map(&:id)).to match_array([bookmark2.id])
+    end
+  end
+
+  describe "#perform_search_query" do
+    before do
+      SearchIndexer.enable
+    end
+
+    it "returns bookmarks that match by name" do
+      ts_query = Search.ts_query(term: "gotta", ts_config: "simple")
+      expect(subject.perform_search_query(subject.perform_list_query(user, guardian), "%gotta%", ts_query).map(&:id)).to match_array([bookmark1.id])
+    end
+
+    it "returns bookmarks that match by post search data (topic title or post content)" do
+      post.update(raw: "some post content")
+      topic1.update(title: "a great topic title")
+
+      ts_query = Search.ts_query(term: "post content", ts_config: "simple")
+      expect(subject.perform_search_query(subject.perform_list_query(user, guardian), "%post content%", ts_query).map(&:id)).to match_array([bookmark1.id])
+
+      ts_query = Search.ts_query(term: "great topic", ts_config: "simple")
+      expect(subject.perform_search_query(subject.perform_list_query(user, guardian), "%great topic%", ts_query).map(&:id)).to match_array([bookmark1.id])
+
+      ts_query = Search.ts_query(term: "blah", ts_config: "simple")
+      expect(subject.perform_search_query(subject.perform_list_query(user, guardian), "%blah%", ts_query).map(&:id)).to eq([])
+    end
+  end
+
+  describe "#can_send_reminder?" do
+    it "cannot send reminder if the topic is deleted" do
+      expect(subject.can_send_reminder?(bookmark1)).to eq(true)
+      bookmark1.bookmarkable.trash!
+      bookmark1.reload
+      expect(subject.can_send_reminder?(bookmark1)).to eq(false)
+    end
+  end
+
+  describe "#reminder_handler" do
+    it "creates a notification for the user with the correct details" do
+      expect { subject.send_reminder_notification(bookmark1) }.to change { Notification.count }.by(1)
+      notif = user.notifications.last
+      expect(notif.notification_type).to eq(Notification.types[:bookmark_reminder])
+      expect(notif.topic_id).to eq(bookmark1.bookmarkable_id)
+      expect(notif.post_number).to eq(1)
+      expect(notif.data).to eq(
+        {
+          title: bookmark1.bookmarkable.title,
+          display_username: bookmark1.user.username,
+          bookmark_name: bookmark1.name,
+          bookmarkable_url: bookmark1.bookmarkable.first_post.url
+        }.to_json
+      )
+    end
+  end
+
+  describe "#can_see?" do
+    it "returns false if the post is in a private category or private message the user cannot see" do
+      expect(subject.can_see?(guardian, bookmark1)).to eq(true)
+      bookmark1.bookmarkable.update!(category: private_category)
+      expect(subject.can_see?(guardian, bookmark1)).to eq(false)
+      bookmark1.update!(bookmarkable: Fabricate(:private_message_topic))
+      expect(subject.can_see?(guardian, bookmark1)).to eq(false)
+    end
+  end
+end

spec/support/bookmarkable_helper.rb

@@ -0,0 +1,42 @@
+# frozen_string_literal: true
+
+class UserTestBookmarkSerializer < UserBookmarkBaseSerializer; end
+class UserTestBookmarkable < BaseBookmarkable
+  def self.model
+    User
+  end
+
+  def self.serializer
+    UserTestBookmarkSerializer
+  end
+
+  def self.preload_associations
+    [:topic_users, :tags, { posts: :user }]
+  end
+
+  def self.list_query(user, guardian)
+    user.bookmarks.joins(
+      "INNER JOIN users ON users.id = bookmarks.bookmarkable_id AND bookmarks.bookmarkable_type = 'User'"
+    ).where(bookmarkable_type: "User")
+  end
+
+  def self.search_query(bookmarks, query, ts_query, &bookmarkable_search)
+    bookmarks.where("users.username ILIKE ?", query)
+  end
+
+  def self.reminder_handler(bookmark)
+    # noop
+  end
+
+  def self.reminder_conditions(bookmark)
+    bookmark.bookmarkable.present?
+  end
+
+  def self.can_see?(guardian, bookmark)
+    true
+  end
+end
+
+def register_test_bookmarkable
+  Bookmark.register_bookmarkable(UserTestBookmarkable)
+end