FEATURE: Polymorphic bookmarks pt. 2 (lists, search) (#16335)

This pull request follows on from https://github.com/discourse/discourse/pull/16308. This one does the following:

* Changes `BookmarkQuery` to allow for querying more than just Post and Topic bookmarkables
* Introduces a `Bookmark.register_bookmarkable` method which requires a model, serializer, fields and preload includes for searching. These registered `Bookmarkable` types are then used when validating new bookmarks, and also when determining which serializer to use for the bookmark list. The `Post` and `Topic` bookmarkables are registered by default.
* Adds new specific types for Post and Topic bookmark serializers along with preloading of associations in `UserBookmarkList`
* Changes to the user bookmark list template to allow for more generic bookmarkable types alongside the Post and Topic ones which need to display in a particular way

All of these changes are gated behind the `use_polymorphic_bookmarks` site setting, apart from the .hbs changes where I have updated the original `UserBookmarkSerializer` with some stub methods.

Following this PR will be several plugin PRs (for assign, chat, encrypt) that will register their own bookmarkable types or otherwise alter the bookmark serializers in their own way, also gated behind `use_polymorphic_bookmarks`.

This commit also removes `BookmarkQuery.preloaded_custom_fields` and the functionality surrounding it. It was added in 0cd502a558 but only used by one plugin (discourse-assign) where it has since been removed, and is now used by no plugins. We don't need it anymore.

app/assets/javascripts/discourse/app/models/bookmark.js

@@ -159,17 +159,12 @@ const Bookmark = RestModel.extend({
     });
   },
 
-  @discourseComputed(
-    "post_user_username",
-    "post_user_avatar_template",
-    "post_user_name"
-  )
-  postUser(post_user_username, avatarTemplate, name) {
-    return User.create({
-      username: post_user_username,
-      avatar_template: avatarTemplate,
-      name,
-    });
+  @discourseComputed("bookmarkable_type")
+  bookmarkableTopicAlike(bookmarkable_type) {
+    if (!this.siteSettings.use_polymorphic_bookmarks) {
+      return true;
+    }
+    return ["Topic", "Post"].includes(bookmarkable_type);
   },
 });
 
@@ -177,6 +172,7 @@ Bookmark.reopenClass({
   create(args) {
     args = args || {};
     args.currentUser = args.currentUser || User.current();
+    args.user = User.create(args.user);
     return this._super(args);
   },
 });

app/assets/javascripts/discourse/app/templates/components/bookmark-list.hbs

@@ -31,17 +31,29 @@
                   {{#if bookmark.pinned}}
                     {{d-icon "thumbtack" class="bookmark-pinned"}}
                   {{/if}}
-                  {{~topic-status topic=bookmark.topicStatus~}}
-                  {{topic-link bookmark.topicForList}}
+                  {{#if bookmark.bookmarkableTopicAlike}}
+                    {{~topic-status topic=bookmark.topicStatus~}}
+                    {{topic-link bookmark.topicForList}}
+                  {{else}}
+                    <a href={{bookmark.bookmarkable_url}}
+                      role="heading"
+                      aria-level="2"
+                      class="title"
+                      data-topic-id="${topic.id}">
+                      {{bookmark.fancy_title}}
+                    </a>
+                  {{/if}}
                 </div>
               </span>
-              <div class="link-bottom-line">
-                {{category-link bookmark.category}}
-                {{discourse-tags bookmark mode="list" tagsForUser=tagsForUser}}
-              </div>
-              {{#if (and site.mobileView bookmark.excerpt bookmark.post_user_avatar_template)}}
-                <a href={{bookmark.postUser.path}} data-user-card={{bookmark.post_user_username}} class="avatar">
-                  {{avatar bookmark.postUser avatarTemplatePath="avatar_template" usernamePath="username" namePath="name" imageSize="small"}}
+              {{#if bookmark.bookmarkableTopicAlike}}
+                <div class="link-bottom-line">
+                  {{category-link bookmark.category}}
+                  {{discourse-tags bookmark mode="list" tagsForUser=tagsForUser}}
+                </div>
+              {{/if}}
+              {{#if (and site.mobileView bookmark.excerpt bookmark.user.avatar_template)}}
+                <a href={{bookmark.bookmarkableUser.path}} data-user-card={{bookmark.user.username}} class="avatar">
+                  {{avatar bookmark.bookmarkableUser avatarTemplatePath="avatar_template" usernamePath="username" namePath="name" imageSize="small"}}
                 </a>
               {{/if}}
               {{!-- template-lint-disable --}}
@@ -49,9 +61,9 @@
             </td>
             {{#unless site.mobileView}}
               <td class="topic-list-data">
-                {{#if bookmark.post_user_avatar_template}}
-                  <a href={{bookmark.postUser.path}} data-user-card={{bookmark.post_user_username}} class="avatar">
-                    {{avatar bookmark.postUser avatarTemplatePath="avatar_template" usernamePath="username" namePath="name" imageSize="small"}}
+                {{#if bookmark.user.avatar_template}}
+                  <a href={{bookmark.user.path}} data-user-card={{bookmark.user.username}} class="avatar">
+                    {{avatar bookmark.user avatarTemplatePath="avatar_template" usernamePath="username" namePath="name" imageSize="small"}}
                   </a>
                 {{/if}}
               </td>

app/models/bookmark.rb

@@ -8,12 +8,101 @@ class Bookmark < ActiveRecord::Base
     "reminder_type" # TODO 2021-04-01: remove
   ]
 
+  cattr_accessor :registered_bookmarkables
+  self.registered_bookmarkables = []
+
+  def self.registered_bookmarkable_from_type(type)
+    Bookmark.registered_bookmarkables.find { |bm| bm.model.name == type }
+  end
+
+  def self.register_bookmarkable(
+    model:, serializer:, list_query:, search_query:, preload_associations: []
+  )
+    Bookmark.registered_bookmarkables << Bookmarkable.new(
+      model: model,
+      serializer: serializer,
+      list_query: list_query,
+      search_query: search_query,
+      preload_associations: preload_associations
+    )
+  end
+
+  ##
+  # This is called when the app loads, similar to AdminDashboardData.reset_problem_checks,
+  # so the default Post and Topic bookmarkables are registered on
+  # boot.
+  #
+  # This method also can be used in testing to reset bookmarkables between
+  # tests. It will also fire multiple times in development mode because
+  # classes are not cached.
+  def self.reset_bookmarkables
+    self.registered_bookmarkables = []
+    Bookmark.register_bookmarkable(
+      model: Post,
+      serializer: UserPostBookmarkSerializer,
+      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
+
+  def self.valid_bookmarkable_types
+    Bookmark.registered_bookmarkables.map(&:model).map(&:to_s)
+  end
+
   belongs_to :user
   belongs_to :post
   has_one :topic, through: :post
   belongs_to :bookmarkable, polymorphic: true
 
-  delegate :topic_id, to: :post
+  # TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented.
+  def topic_id
+    return if SiteSetting.use_polymorphic_bookmarks
+    post.topic_id
+  end
 
   def self.auto_delete_preferences
     @auto_delete_preferences ||= Enum.new(
@@ -24,6 +113,10 @@ class Bookmark < ActiveRecord::Base
     )
   end
 
+  def self.select_type(bookmarks_relation, type)
+    bookmarks_relation.select { |bm| bm.bookmarkable_type == type }
+  end
+
   # TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented.
   validate :unique_per_post_for_user,
     on: [:create, :update],
@@ -35,6 +128,7 @@ class Bookmark < ActiveRecord::Base
     if: Proc.new { |b| b.will_save_change_to_post_id? || b.will_save_change_to_for_topic? }
 
   validate :polymorphic_columns_present, on: [:create, :update]
+  validate :valid_bookmarkable_type, on: [:create, :update]
 
   validate :unique_per_bookmarkable,
     on: [:create, :update],
@@ -106,6 +200,13 @@ class Bookmark < ActiveRecord::Base
     )
   end
 
+  def valid_bookmarkable_type
+    return if !SiteSetting.use_polymorphic_bookmarks
+    return if Bookmark.valid_bookmarkable_types.include?(self.bookmarkable_type)
+
+    self.errors.add(:base, I18n.t("bookmarks.errors.invalid_bookmarkable", type: self.bookmarkable_type))
+  end
+
   # TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented.
   def is_for_first_post?
     @is_for_first_post ||= new_record? ? Post.exists?(id: post_id, post_number: 1) : post.post_number == 1

app/models/user.rb

@@ -343,6 +343,10 @@ class User < ActiveRecord::Base
     end
   end
 
+  def bookmarks_of_type(type)
+    bookmarks.where(bookmarkable_type: type)
+  end
+
   EMAIL = %r{([^@]+)@([^\.]+)}
   FROM_STAGED = "from_staged"
 

app/serializers/user_bookmark_base_serializer.rb

@@ -0,0 +1,44 @@
+# frozen_string_literal: true
+
+class UserBookmarkBaseSerializer < ApplicationSerializer
+  attributes :id,
+             :created_at,
+             :updated_at,
+             :name,
+             :reminder_at,
+             :pinned,
+             :title,
+             :fancy_title,
+             :excerpt,
+             :bookmarkable_id,
+             :bookmarkable_type,
+             :bookmarkable_url
+
+  def title
+    raise NotImplementedError
+  end
+
+  def fancy_title
+    raise NotImplementedError
+  end
+
+  def cooked
+    raise NotImplementedError
+  end
+
+  def bookmarkable_url
+    raise NotImplementedError
+  end
+
+  def excerpt
+    raise NotImplementedError
+  end
+
+  # Note: This assumes that the bookmarkable has a user attached to it,
+  # we may need to revisit this assumption at some point.
+  has_one :user, serializer: BasicUserSerializer, embed: :objects
+
+  def user
+    bookmarkable_user
+  end
+end

app/serializers/user_bookmark_list_serializer.rb

@@ -1,11 +1,27 @@
 # frozen_string_literal: true
 
 class UserBookmarkListSerializer < ApplicationSerializer
-  attributes :more_bookmarks_url
+  attributes :more_bookmarks_url, :bookmarks
 
-  has_many :bookmarks, serializer: UserBookmarkSerializer, embed: :objects
+  def bookmarks
+    if SiteSetting.use_polymorphic_bookmarks
+      object.bookmarks.map do |bm|
+        serialize_registered_type(bm)
+      end
+    else
+      object.bookmarks.map { |bm| UserBookmarkSerializer.new(bm, scope: scope, root: false) }
+    end
+  end
 
   def include_more_bookmarks_url?
     @include_more_bookmarks_url ||= object.bookmarks.size == object.per_page
   end
+
+  private
+
+  def serialize_registered_type(bookmark)
+    Bookmark.registered_bookmarkable_from_type(
+      bookmark.bookmarkable_type
+    ).serializer.new(bookmark, scope: scope, root: false)
+  end
 end

app/serializers/user_bookmark_serializer.rb

@@ -2,6 +2,7 @@
 
 require_relative 'post_item_excerpt'
 
+# TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented.
 class UserBookmarkSerializer < ApplicationSerializer
   include PostItemExcerpt
   include TopicTagsMixin
@@ -30,7 +31,12 @@ class UserBookmarkSerializer < ApplicationSerializer
              :post_user_username,
              :post_user_avatar_template,
              :post_user_name,
-             :for_topic
+             :for_topic,
+             :bookmarkable_id,
+             :bookmarkable_type,
+             :bookmarkable_user_username,
+             :bookmarkable_user_avatar_template,
+             :bookmarkable_user_name,
 
   def topic_id
     post.topic_id
@@ -148,4 +154,19 @@ class UserBookmarkSerializer < ApplicationSerializer
   def post_user_name
     post_user.name
   end
+
+  # TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented.
+  # Note...these are just stub methods for compatability with the user-bookmark-list.hbs
+  # changes in a transition period for polymorphic bookmarks.
+  def bookmarkable_user_username
+    post_user.username
+  end
+
+  def bookmarkable_user_avatar_template
+    post_user.avatar_template
+  end
+
+  def bookmarkable_user_name
+    post_user.name
+  end
 end

app/serializers/user_post_bookmark_serializer.rb

@@ -0,0 +1,43 @@
+# frozen_string_literal: true
+
+class UserPostBookmarkSerializer < UserPostTopicBookmarkBaseSerializer
+  attr_reader :post_id
+
+  def post_id
+    post.id
+  end
+
+  def linked_post_number
+    post.post_number
+  end
+
+  def deleted
+    topic.deleted_at.present? || post.deleted_at.present?
+  end
+
+  def hidden
+    post.hidden
+  end
+
+  def raw
+    post.raw
+  end
+
+  def cooked
+    post.cooked
+  end
+
+  def bookmarkable_user
+    @bookmarkable_user ||= post.user
+  end
+
+  private
+
+  def topic
+    post.topic
+  end
+
+  def post
+    object.bookmarkable
+  end
+end

app/serializers/user_post_topic_bookmark_base_serializer.rb

@@ -0,0 +1,77 @@
+# frozen_string_literal: true
+
+require_relative 'post_item_excerpt'
+
+class UserPostTopicBookmarkBaseSerializer < UserBookmarkBaseSerializer
+  include TopicTagsMixin
+  include PostItemExcerpt
+
+  attributes :topic_id,
+             :linked_post_number,
+             :deleted,
+             :hidden,
+             :category_id,
+             :closed,
+             :archived,
+             :archetype,
+             :highest_post_number,
+             :last_read_post_number,
+             :bumped_at,
+             :slug
+
+  def topic_id
+    topic.id
+  end
+
+  def title
+    topic.title
+  end
+
+  def fancy_title
+    topic.fancy_title
+  end
+
+  def category_id
+    topic.category_id
+  end
+
+  def archetype
+    topic.archetype
+  end
+
+  def archived
+    topic.archived
+  end
+
+  def closed
+    topic.closed
+  end
+
+  def highest_post_number
+    scope.is_staff? ? topic.highest_staff_post_number : topic.highest_post_number
+  end
+
+  def last_read_post_number
+    topic_user&.last_read_post_number
+  end
+
+  def bumped_at
+    topic.bumped_at
+  end
+
+  def slug
+    topic.slug
+  end
+
+  # Note: This is nil because in the UI there are special topic-status and
+  # topic-link components to display the topic URL, and this is not used.
+  def bookmarkable_url
+    nil
+  end
+
+  private
+
+  def topic_user
+    @topic_user ||= topic.topic_users.find { |tu| tu.user_id == scope.user.id }
+  end
+end

app/serializers/user_topic_bookmark_serializer.rb

@@ -0,0 +1,61 @@
+# frozen_string_literal: true
+
+class UserTopicBookmarkSerializer < UserPostTopicBookmarkBaseSerializer
+  # it does not matter what the linked post number is for topic bookmarks,
+  # on the client we always take the user to the last unread post in the
+  # topic when the bookmark URL is clicked
+  def linked_post_number
+    1
+  end
+
+  def first_post
+    @first_post ||= topic.posts.find { |post| post.post_number == 1 }
+  end
+
+  def deleted
+    topic.deleted_at.present? || first_post.deleted_at.present?
+  end
+
+  def hidden
+    first_post.hidden
+  end
+
+  def raw
+    first_post.raw
+  end
+
+  def cooked
+    @cooked ||= \
+      if last_read_post_number.present?
+        for_topic_cooked_post
+      else
+        first_post.cooked
+      end
+  end
+
+  def for_topic_cooked_post
+    post_number = [last_read_post_number + 1, highest_post_number].min
+    sorted_regular_posts = topic.posts.sort_by(&:post_number).select do |post|
+      post.post_type == Post.types[:regular]
+    end
+    first_unread_post = sorted_regular_posts.find do |post|
+      post.post_number >= post_number
+    end
+
+    # if first_unread_cooked is blank this likely means that the last
+    # read post was either deleted or is a small action post.
+    # in this case we should just get the last regular post and
+    # use that for the cooked value so we have something to show
+    (first_unread_post || sorted_regular_posts.last).cooked
+  end
+
+  def bookmarkable_user
+    @bookmarkable_user ||= first_post.user
+  end
+
+  private
+
+  def topic
+    object.bookmarkable
+  end
+end

app/services/bookmarkable.rb

@@ -0,0 +1,80 @@
+# frozen_string_literal: true
+#
+# 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.
+#
+# Post and Topic bookmarkables are registered by default.
+#
+# Anything other than types registered in this way will throw an error
+# when trying to save the Bookmark record. All things that are bookmarkable
+# must be registered in this way.
+#
+# See Bookmark#reset_bookmarkables for some examples on how registering
+# bookmarkables works.
+class Bookmarkable
+  attr_reader :model, :serializer, :list_query, :search_query, :preload_associations
+  delegate :table_name, to: :@model
+
+  def initialize(model:, serializer:, list_query:, search_query:, preload_associations: [])
+    @model = model
+    @serializer = serializer
+    @list_query = list_query
+    @search_query = search_query
+    @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)
+  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
+  # filter. This is so every bookmarkable is filtered by its own customized
+  # 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
+  # @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|
+      bookmarks_joined.where("#{where_sql} OR bookmarks.name ILIKE :q", q: query)
+    end
+  end
+
+  ##
+  # When displaying the bookmarks in a list for a user there is often additional
+  # information drawn from other tables joined to the bookmarkable that must
+  # be displayed. We preload these additional associations here on top of the
+  # array of bookmarks which has already been filtered, offset by page, ordered,
+  # and limited. The preload_associations array should be in the same format as
+  # used for .includes() e.g.
+  #
+  # [{ topic: [:topic_users, :tags] }, :user]
+  #
+  # @param [Array] bookmarks The array of bookmarks after initial listing and filtering, note this is
+  #                          array _not_ an ActiveRecord::Relation.
+  def perform_preload(bookmarks)
+    ActiveRecord::Associations::Preloader.new.preload(
+      Bookmark.select_type(bookmarks, model.to_s), { bookmarkable: preload_associations }
+    )
+  end
+end

config/locales/server.en.yml

@@ -437,6 +437,7 @@ en:
       time_must_be_provided: "Time must be provided for all reminders"
       for_topic_must_use_first_post: "You can only use the first post to bookmark the topic."
       bookmarkable_id_type_required: "The name and type of the record to bookmark is required."
+      invalid_bookmarkable: "A %{type} cannot be bookmarked."
 
     reminders:
       at_desktop: "Next time I'm at my desktop"

lib/bookmark_query.rb

@@ -5,19 +5,29 @@
 # in the user/activity/bookmarks page.
 
 class BookmarkQuery
-  cattr_accessor :preloaded_custom_fields
-  self.preloaded_custom_fields = Set.new
-
   def self.on_preload(&blk)
     (@preload ||= Set.new) << blk
   end
 
   def self.preload(bookmarks, object)
+    if SiteSetting.use_polymorphic_bookmarks
+      preload_polymorphic_associations(bookmarks)
+    end
     if @preload
       @preload.each { |preload| preload.call(bookmarks, object) }
     end
   end
 
+  # These polymorphic associations are loaded to make the UserBookmarkListSerializer's
+  # life easier, which conditionally chooses the bookmark serializer to use based
+  # on the type, and we want the associations all loaded ahead of time to make
+  # sure we are not doing N+1s.
+  def self.preload_polymorphic_associations(bookmarks)
+    Bookmark.registered_bookmarkables.each do |registered_bookmarkable|
+      registered_bookmarkable.perform_preload(bookmarks)
+    end
+  end
+
   def initialize(user:, guardian: nil, params: {})
     @user = user
     @params = params
@@ -27,49 +37,77 @@ class BookmarkQuery
   end
 
   def list_all
-    results = user_bookmarks.order(
-      '(CASE WHEN bookmarks.pinned THEN 0 ELSE 1 END), bookmarks.reminder_at ASC, bookmarks.updated_at DESC'
-    )
+    return polymorphic_list_all if SiteSetting.use_polymorphic_bookmarks
 
     topics = Topic.listable_topics.secured(@guardian)
     pms = Topic.private_messages_for_user(@user)
-    results = results.merge(topics.or(pms))
+    results = list_all_results(topics, pms)
 
-    results = results.merge(Post.secured(@guardian))
+    results = results.order(
+      "(CASE WHEN bookmarks.pinned THEN 0 ELSE 1 END),
+        bookmarks.reminder_at ASC,
+        bookmarks.updated_at DESC"
+    )
 
     if @params[:q].present?
-      term = @params[:q]
-      bookmark_ts_query = Search.ts_query(term: term)
-      results = results
-        .joins("LEFT JOIN post_search_data ON post_search_data.post_id = bookmarks.post_id")
-        .where(
-          "bookmarks.name ILIKE :q OR #{bookmark_ts_query} @@ post_search_data.search_data",
-          q: "%#{term}%"
-        )
+      results = search(results, @params[:q])
     end
 
     if @page.positive?
       results = results.offset(@page * @params[:per_page])
     end
 
-    results = results.limit(@limit)
-
-    if BookmarkQuery.preloaded_custom_fields.any?
-      Topic.preload_custom_fields(
-        results.map(&:topic), BookmarkQuery.preloaded_custom_fields
-      )
-    end
-
+    results = results.limit(@limit).to_a
     BookmarkQuery.preload(results, self)
-
-    @guardian.filter_allowed_categories(results)
+    results
   end
 
   private
 
-  def user_bookmarks
-    # There is guaranteed to be a TopicUser record if the user has bookmarked
-    # a topic, see BookmarkManager
+  def polymorphic_list_all
+    search_term = @params[:q]
+    ts_query = search_term.present? ? Search.ts_query(term: search_term) : nil
+    search_term_wildcard = search_term.present? ? "%#{search_term}%" : nil
+
+    queries = Bookmark.registered_bookmarkables.map do |bookmarkable|
+      interim_results = bookmarkable.perform_list_query(@user, @guardian)
+
+      if search_term.present?
+        interim_results = bookmarkable.perform_search_query(
+          interim_results, search_term_wildcard, ts_query
+        )
+      end
+
+      # this is purely to make the query easy to read and debug, otherwise it's
+      # all mashed up into a massive ball in MiniProfiler :)
+      "---- #{bookmarkable.model.to_s} bookmarkable ---\n\n #{interim_results.to_sql}"
+    end
+
+    union_sql = queries.join("\n\nUNION\n\n")
+    results = Bookmark.select("bookmarks.*").from("(\n\n#{union_sql}\n\n) as bookmarks")
+    results = results.order(
+      "(CASE WHEN bookmarks.pinned THEN 0 ELSE 1 END),
+        bookmarks.reminder_at ASC,
+        bookmarks.updated_at DESC"
+    )
+
+    if @page.positive?
+      results = results.offset(@page * @params[:per_page])
+    end
+
+    results = results.limit(@limit).to_a
+    BookmarkQuery.preload(results, self)
+    results
+  end
+
+  def list_all_results(topics, pms)
+    results = base_bookmarks.merge(topics.or(pms))
+    results = results.merge(Post.secured(@guardian))
+    results = @guardian.filter_allowed_categories(results)
+    results
+  end
+
+  def base_bookmarks
     Bookmark.where(user: @user)
       .includes(post: :user)
       .includes(post: { topic: :tags })
@@ -77,4 +115,11 @@ class BookmarkQuery
       .references(:post)
       .where(topic_users: { user_id: @user.id })
   end
+
+  def search(results, term)
+    bookmark_ts_query = Search.ts_query(term: term)
+    results
+      .joins("LEFT JOIN post_search_data ON post_search_data.post_id = bookmarks.post_id")
+      .where("bookmarks.name ILIKE :q OR #{bookmark_ts_query} @@ post_search_data.search_data", q: "%#{term}%")
+  end
 end

spec/fabricators/bookmark_fabricator.rb

@@ -2,10 +2,19 @@
 
 Fabricator(:bookmark) do
   user
-  post { Fabricate(:post) }
+  post {
+    if !SiteSetting.use_polymorphic_bookmarks
+      Fabricate(:post)
+    end
+  }
   name "This looked interesting"
   reminder_at { 1.day.from_now.iso8601 }
   reminder_set_at { Time.zone.now }
+  bookmarkable {
+    if SiteSetting.use_polymorphic_bookmarks
+      Fabricate(:post)
+    end
+  }
 
   # TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented.
   before_create do |bookmark|

spec/lib/bookmark_query_spec.rb

@@ -12,6 +12,21 @@ 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) }
@@ -50,28 +65,104 @@ RSpec.describe BookmarkQuery do
       expect(bookmark.topic.topic_users.map(&:user_id)).to contain_exactly(user.id)
     end
 
-    context "when q param is provided" do
+    context "for polymorphic bookmarks" do
       before do
-        @post = Fabricate(:post, raw: "Some post content here", topic: Fabricate(:topic, title: "Bugfix game for devs"))
-        @bookmark3 = Fabricate(:bookmark, user: user, name: "Check up later")
-        @bookmark4 = Fabricate(:bookmark, user: user, post: @post)
-        Fabricate(:topic_user, user: user, topic: @bookmark3.topic)
-        Fabricate(:topic_user, user: user, topic: @bookmark4.topic)
+        SiteSetting.use_polymorphic_bookmarks = true
+        Bookmark.reset_bookmarkables
+        register_user_bookmarkable
+
+        Fabricate(:topic_user, user: user, topic: post_bookmark.bookmarkable.topic)
+        Fabricate(:topic_user, user: user, topic: topic_bookmark.bookmarkable)
+        user_bookmark
       end
 
-      it "can search by bookmark name" do
-        bookmarks = bookmark_query(params: { q: 'check' }).list_all
-        expect(bookmarks.map(&:id)).to eq([@bookmark3.id])
+      let(:post_bookmark) { Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:post)) }
+      let(:topic_bookmark) { Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:topic)) }
+      let(:user_bookmark) { Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:user, username: "bookmarkqueen")) }
+
+      after do
+        Bookmark.reset_bookmarkables
       end
 
-      it "can search by post content" do
-        bookmarks = bookmark_query(params: { q: 'content' }).list_all
-        expect(bookmarks.map(&:id)).to eq([@bookmark4.id])
+      it "returns a mixture of post, topic, and custom bookmarkable type bookmarks" do
+        bookmarks = bookmark_query.list_all
+        expect(bookmarks.map(&:id)).to match_array([post_bookmark.id, topic_bookmark.id, user_bookmark.id])
+      end
+    end
+
+    context "when q param is provided" do
+      let!(:post) { Fabricate(:post, raw: "Some post content here", topic: Fabricate(:topic, title: "Bugfix game for devs")) }
+
+      context "when not using polymorphic bookmarks" do
+        let(:bookmark3) { Fabricate(:bookmark, user: user, name: "Check up later") }
+        let(:bookmark4) { Fabricate(:bookmark, user: user, post: post) }
+
+        before do
+          Fabricate(:topic_user, user: user, topic: bookmark3.topic)
+          Fabricate(:topic_user, user: user, topic: bookmark4.topic)
+        end
+
+        it "can search by bookmark name" do
+          bookmarks = bookmark_query(params: { q: 'check' }).list_all
+          expect(bookmarks.map(&:id)).to eq([bookmark3.id])
+        end
+
+        it "can search by post content" do
+          bookmarks = bookmark_query(params: { q: 'content' }).list_all
+          expect(bookmarks.map(&:id)).to eq([bookmark4.id])
+        end
+
+        it "can search by topic title" do
+          bookmarks = bookmark_query(params: { q: 'bugfix' }).list_all
+          expect(bookmarks.map(&:id)).to eq([bookmark4.id])
+        end
       end
 
-      it "can search by topic title" do
-        bookmarks = bookmark_query(params: { q: 'bugfix' }).list_all
-        expect(bookmarks.map(&:id)).to eq([@bookmark4.id])
+      context "when using polymorphic bookmarks" do
+        before do
+          SiteSetting.use_polymorphic_bookmarks = true
+          Bookmark.reset_bookmarkables
+        end
+
+        after do
+          Bookmark.reset_bookmarkables
+        end
+
+        let(:bookmark3) { Fabricate(:bookmark, user: user, name: "Check up later", bookmarkable: Fabricate(:post)) }
+        let(:bookmark4) { Fabricate(:bookmark, user: user, bookmarkable: post) }
+
+        before do
+          Fabricate(:topic_user, user: user, topic: bookmark3.bookmarkable.topic)
+          Fabricate(:topic_user, user: user, topic: bookmark4.bookmarkable.topic)
+        end
+
+        it "can search by bookmark name" do
+          bookmarks = bookmark_query(params: { q: 'check' }).list_all
+          expect(bookmarks.map(&:id)).to eq([bookmark3.id])
+        end
+
+        it "can search by post content" do
+          bookmarks = bookmark_query(params: { q: 'content' }).list_all
+          expect(bookmarks.map(&:id)).to eq([bookmark4.id])
+        end
+
+        it "can search by topic title" do
+          bookmarks = bookmark_query(params: { q: 'bugfix' }).list_all
+          expect(bookmarks.map(&:id)).to eq([bookmark4.id])
+        end
+
+        context "with custom bookmarkable fitering" do
+          before do
+            register_user_bookmarkable
+          end
+
+          let!(:bookmark5) { Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:user, username: "bookmarkqueen")) }
+
+          it "allows searching bookmarkables by fields in other tables" do
+            bookmarks = bookmark_query(params: { q: 'bookmarkq' }).list_all
+            expect(bookmarks.map(&:id)).to eq([bookmark5.id])
+          end
+        end
       end
     end
 
@@ -159,23 +250,6 @@ RSpec.describe BookmarkQuery do
         expect(bookmark_query.list_all.count).to eq(1)
       end
     end
-
-    context "when there are topic custom fields to preload" do
-      before do
-        TopicCustomField.create(
-          topic_id: bookmark1.topic.id, name: 'test_field', value: 'test'
-        )
-        BookmarkQuery.preloaded_custom_fields << "test_field"
-      end
-      it "preloads them" do
-        Topic.expects(:preload_custom_fields)
-        expect(
-          bookmark_query.list_all.find do |b|
-            b.topic.id = bookmark1.topic.id
-          end.topic.custom_fields['test_field']
-        ).not_to eq(nil)
-      end
-    end
   end
 
   describe "#list_all ordering" do

spec/models/bookmark_spec.rb

@@ -43,6 +43,10 @@ describe Bookmark do
         SiteSetting.use_polymorphic_bookmarks = true
       end
 
+      after do
+        Bookmark.registered_bookmarkables = []
+      end
+
       it "does not allow a user to create a bookmark with only one polymorphic column" do
         user = Fabricate(:user)
         bm = Bookmark.create(bookmarkable_id: post.id, user: user)
@@ -59,6 +63,25 @@ describe Bookmark do
         bm = Bookmark.create(bookmarkable_type: "Post", bookmarkable_id: post.id, user: user)
         expect(bm.errors.full_messages).to include(I18n.t("bookmarks.errors.already_bookmarked", type: "Post"))
       end
+
+      it "does not allow a user to create a bookmarkable for a type that has not been registered" 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(
+          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
   end
 

spec/models/user_bookmark_list_spec.rb

@@ -5,22 +5,55 @@ RSpec.describe UserBookmarkList do
   fab!(:user) { Fabricate(:user) }
   let(:list) { UserBookmarkList.new(user: user, guardian: Guardian.new(user), params: params) }
 
-  before do
-    22.times do
-      bookmark = Fabricate(:bookmark, user: user)
-      Fabricate(:topic_user, topic: bookmark.topic, user: user)
+  context "for non-polymorphic bookmarks" do
+    before do
+      22.times do
+        bookmark = Fabricate(:bookmark, user: user)
+        Fabricate(:topic_user, topic: bookmark.topic, user: user)
+      end
+    end
+
+    it "defaults to 20 per page" do
+      expect(list.per_page).to eq(20)
+    end
+
+    context "when the per_page param is too high" do
+      let(:params) { { per_page: 1000 } }
+
+      it "does not allow more than X bookmarks to be requested per page" do
+        expect(list.load.count).to eq(20)
+      end
     end
   end
 
-  it "defaults to 20 per page" do
-    expect(list.per_page).to eq(20)
-  end
+  context "for polymorphic bookmarks" do
+    before do
+      SiteSetting.use_polymorphic_bookmarks = true
+    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
+    )
 
-  context "when the per_page param is too high" do
-    let(:params) { { per_page: 1000 } }
+      Fabricate(:topic_user, user: user, topic: post_bookmark.bookmarkable.topic)
+      Fabricate(:topic_user, user: user, topic: topic_bookmark.bookmarkable)
+      user_bookmark
+    end
 
-    it "does not allow more than X bookmarks to be requested per page" do
-      expect(list.load.count).to eq(20)
+    let(:post_bookmark) { Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:post)) }
+    let(:topic_bookmark) { Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:topic)) }
+    let(:user_bookmark) { Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:user)) }
+
+    it "returns all types of bookmarks" do
+      list.load
+      expect(list.bookmarks.map(&:id)).to match_array([post_bookmark.id, topic_bookmark.id, user_bookmark.id])
     end
   end
 end

spec/rails_helper.rb

@@ -139,6 +139,9 @@ module TestSetup
     # Don't queue badge grant in test mode
     BadgeGranter.disable_queue
 
+    # Make sure the default Post and Topic bookmarkables are registered
+    Bookmark.reset_bookmarkables
+
     OmniAuth.config.test_mode = false
   end
 end

spec/requests/users_controller_spec.rb

@@ -5208,30 +5208,31 @@ describe UsersController do
   end
 
   describe "#bookmarks" do
-    fab!(:bookmark1) { Fabricate(:bookmark, user: user1) }
-    fab!(:bookmark2) { Fabricate(:bookmark, user: user1) }
-    fab!(:bookmark3) { Fabricate(:bookmark) }
+    context "when polymorphic bookmarks are not used" do
+      let(:bookmark1) { Fabricate(:bookmark, user: user1) }
+      let(:bookmark2) { Fabricate(:bookmark, user: user1) }
+      let(:bookmark3) { Fabricate(:bookmark) }
+      before do
+        TopicUser.change(user1.id, bookmark1.topic_id, total_msecs_viewed: 1)
+        TopicUser.change(user1.id, bookmark2.topic_id, total_msecs_viewed: 1)
+        bookmark3
+      end
 
-    before do
-      TopicUser.change(user1.id, bookmark1.topic_id, total_msecs_viewed: 1)
-      TopicUser.change(user1.id, bookmark2.topic_id, total_msecs_viewed: 1)
-    end
+      it "returns a list of serialized bookmarks for the user" do
+        sign_in(user1)
+        get "/u/#{user1.username}/bookmarks.json"
+        expect(response.status).to eq(200)
+        expect(response.parsed_body['user_bookmark_list']['bookmarks'].map { |b| b['id'] }).to match_array([bookmark1.id, bookmark2.id])
+      end
 
-    it "returns a list of serialized bookmarks for the user" do
-      sign_in(user1)
-      get "/u/#{user1.username}/bookmarks.json"
-      expect(response.status).to eq(200)
-      expect(response.parsed_body['user_bookmark_list']['bookmarks'].map { |b| b['id'] }).to match_array([bookmark1.id, bookmark2.id])
-    end
+      it "returns an .ics file of bookmark reminders for the user in date order" do
+        bookmark1.update!(name: nil, reminder_at: 1.day.from_now)
+        bookmark2.update!(name: "Some bookmark note", reminder_at: 1.week.from_now)
 
-    it "returns an .ics file of bookmark reminders for the user in date order" do
-      bookmark1.update!(name: nil, reminder_at: 1.day.from_now)
-      bookmark2.update!(name: "Some bookmark note", reminder_at: 1.week.from_now)
-
-      sign_in(user1)
-      get "/u/#{user1.username}/bookmarks.ics"
-      expect(response.status).to eq(200)
-      expect(response.body).to eq(<<~ICS)
+        sign_in(user1)
+        get "/u/#{user1.username}/bookmarks.ics"
+        expect(response.status).to eq(200)
+        expect(response.body).to eq(<<~ICS)
         BEGIN:VCALENDAR
         VERSION:2.0
         PRODID:-//Discourse//#{Discourse.current_hostname}//#{Discourse.full_version}//EN
@@ -5254,32 +5255,108 @@ describe UsersController do
         URL:#{Discourse.base_url}/t/-/#{bookmark2.topic_id}
         END:VEVENT
         END:VCALENDAR
-      ICS
+        ICS
+      end
+
+      it "does not show another user's bookmarks" do
+        sign_in(user1)
+        get "/u/#{bookmark3.user.username}/bookmarks.json"
+        expect(response.status).to eq(403)
+      end
+
+      it "shows a helpful message if no bookmarks are found" do
+        bookmark1.destroy
+        bookmark2.destroy
+        bookmark3.destroy
+        sign_in(user1)
+        get "/u/#{user1.username}/bookmarks.json"
+        expect(response.status).to eq(200)
+        expect(response.parsed_body['bookmarks']).to eq([])
+      end
+
+      it "shows a helpful message if no bookmarks are found for the search" do
+        sign_in(user1)
+        get "/u/#{user1.username}/bookmarks.json", params: {
+          q: 'badsearch'
+        }
+        expect(response.status).to eq(200)
+        expect(response.parsed_body['bookmarks']).to eq([])
+      end
     end
 
-    it "does not show another user's bookmarks" do
-      sign_in(user1)
-      get "/u/#{bookmark3.user.username}/bookmarks.json"
-      expect(response.status).to eq(403)
-    end
+    context "for polymorphic bookmarks" do
+      class UserTestBookmarkSerializer < UserBookmarkBaseSerializer
+        def title
+          fancy_title
+        end
 
-    it "shows a helpful message if no bookmarks are found" do
-      bookmark1.destroy
-      bookmark2.destroy
-      bookmark3.destroy
-      sign_in(user1)
-      get "/u/#{user1.username}/bookmarks.json"
-      expect(response.status).to eq(200)
-      expect(response.parsed_body['bookmarks']).to eq([])
-    end
+        def fancy_title
+          @fancy_title ||= user.username
+        end
 
-    it "shows a helpful message if no bookmarks are found for the search" do
-      sign_in(user1)
-      get "/u/#{user1.username}/bookmarks.json", params: {
-        q: 'badsearch'
-      }
-      expect(response.status).to eq(200)
-      expect(response.parsed_body['bookmarks']).to eq([])
+        def cooked
+          "<p>Something cooked</p>"
+        end
+
+        def bookmarkable_user
+          @bookmarkable_user ||= user
+        end
+
+        def bookmarkable_url
+          "#{Discourse.base_url}/u/#{user.username}"
+        end
+
+        def excerpt
+          return nil unless cooked
+          @excerpt ||= PrettyText.excerpt(cooked, 300, keep_emoji_images: true)
+        end
+
+        private
+
+        def user
+          object.bookmarkable
+        end
+      end
+
+      before do
+        SiteSetting.use_polymorphic_bookmarks = true
+        Bookmark.register_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)
+        bookmark3 && bookmark4
+      end
+
+      after do
+        Bookmark.registered_bookmarkables = []
+      end
+
+      let(:bookmark1) { Fabricate(:bookmark, user: user1, bookmarkable: Fabricate(:post)) }
+      let(:bookmark2) { Fabricate(:bookmark, user: user1, bookmarkable: Fabricate(:topic)) }
+      let(:bookmark3) { Fabricate(:bookmark, user: user1, bookmarkable: Fabricate(:user)) }
+      let(:bookmark4) { Fabricate(:bookmark) }
+
+      it "returns a list of serialized bookmarks for the user including custom registered bookmarkables" do
+        sign_in(user1)
+        get "/u/#{user1.username}/bookmarks.json"
+        expect(response.status).to eq(200)
+        response_bookmarks = response.parsed_body['user_bookmark_list']['bookmarks']
+        expect(response_bookmarks.map { |b| b['id'] }).to match_array(
+          [bookmark1.id, bookmark2.id, bookmark3.id]
+        )
+        expect(response_bookmarks.find { |b| b['id'] == bookmark3.id }['excerpt']).to eq('Something cooked')
+      end
     end
   end
 

spec/serializers/user_bookmark_list_serializer_spec.rb

@@ -0,0 +1,44 @@
+# 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(
+        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
+    end
+
+    let(:post_bookmark) { Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:post)) }
+    let(:topic_bookmark) { Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:topic)) }
+    let(:user_bookmark) { Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:user)) }
+
+    def run_serializer
+      bookmark_list = UserBookmarkList.new(user: user, guardian: Guardian.new(user), params: {})
+      bookmark_list.load
+      UserBookmarkListSerializer.new(bookmark_list)
+    end
+
+    it "chooses the correct class of serializer for all the bookmarkable types" do
+      serializer = run_serializer
+      expect(serializer.bookmarks.map(&:class).map(&:to_s)).to match_array(["UserTestBookmarkSerializer", "UserTopicBookmarkSerializer", "UserPostBookmarkSerializer"])
+    end
+  end
+end

spec/serializers/user_post_bookmark_serializer_spec.rb

@@ -0,0 +1,27 @@
+# frozen_string_literal: true
+
+RSpec.describe UserPostBookmarkSerializer do
+  before do
+    SiteSetting.use_polymorphic_bookmarks = true
+  end
+
+  let(:user) { Fabricate(:user) }
+  let(:post) { Fabricate(:post, user: user, topic: topic) }
+  let(:topic) { Fabricate(:topic) }
+  let!(:bookmark) { Fabricate(:bookmark, name: 'Test', user: user, bookmarkable: post) }
+
+  it "uses the correct highest_post_number column based on whether the user is staff" do
+    Fabricate(:post, topic: topic)
+    Fabricate(:post, topic: topic)
+    Fabricate(:whisper, topic: topic)
+    topic.reload
+    bookmark.reload
+    serializer = UserPostBookmarkSerializer.new(bookmark, scope: Guardian.new(user))
+
+    expect(serializer.highest_post_number).to eq(3)
+
+    user.update!(admin: true)
+
+    expect(serializer.highest_post_number).to eq(4)
+  end
+end

spec/serializers/user_topic_bookmark_serializer_spec.rb

@@ -0,0 +1,42 @@
+# frozen_string_literal: true
+
+RSpec.describe UserTopicBookmarkSerializer do
+  before do
+    SiteSetting.use_polymorphic_bookmarks = true
+  end
+
+  fab!(:user) { Fabricate(:user) }
+  let!(:topic) { Fabricate(:topic, user: user) }
+  let!(:post) { Fabricate(:post, topic: topic) }
+  let!(:bookmark) { Fabricate(:bookmark, name: 'Test', user: user, bookmarkable: topic) }
+
+  it "uses the last_read_post_number + 1 for the bookmarks excerpt" do
+    next_unread_post = Fabricate(:post_with_long_raw_content, topic: bookmark.bookmarkable)
+    Fabricate(:post_with_external_links, topic: bookmark.bookmarkable)
+    bookmark.reload
+    TopicUser.change(user.id, bookmark.bookmarkable.id, { last_read_post_number: post.post_number })
+    serializer = UserTopicBookmarkSerializer.new(bookmark, scope: Guardian.new(user))
+    expect(serializer.excerpt).to eq(PrettyText.excerpt(next_unread_post.cooked, 300, keep_emoji_images: true))
+  end
+
+  it "does not use a small post for the last unread cooked post" do
+    small_action_post = Fabricate(:small_action, topic: bookmark.bookmarkable)
+    next_unread_post = Fabricate(:post_with_long_raw_content, topic: bookmark.bookmarkable)
+    Fabricate(:post_with_external_links, topic: bookmark.bookmarkable)
+    bookmark.reload
+    TopicUser.change(user.id, bookmark.bookmarkable.id, { last_read_post_number: post.post_number })
+    serializer = UserTopicBookmarkSerializer.new(bookmark, scope: Guardian.new(user))
+    expect(serializer.excerpt).to eq(PrettyText.excerpt(next_unread_post.cooked, 300, keep_emoji_images: true))
+  end
+
+  it "handles the last read post in the topic being a small post by getting the last read regular post" do
+    last_regular_post = Fabricate(:post_with_long_raw_content, topic: bookmark.bookmarkable)
+    small_action_post = Fabricate(:small_action, topic: bookmark.bookmarkable)
+    bookmark.reload
+    topic.reload
+    TopicUser.change(user.id, bookmark.bookmarkable.id, { last_read_post_number: small_action_post.post_number })
+    serializer = UserTopicBookmarkSerializer.new(bookmark, scope: Guardian.new(user))
+    expect(serializer.cooked).to eq(last_regular_post.cooked)
+    expect(serializer.excerpt).to eq(PrettyText.excerpt(last_regular_post.cooked, 300, keep_emoji_images: true))
+  end
+end