From 3e4621c2cb578e65965eb392b88faae29c7d471a Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Fri, 22 Apr 2022 08:23:42 +1000 Subject: [PATCH] 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 https://github.com/discourse/discourse/commit/0cd502a55838d5d27f96f13c0794f3669ac41fcc 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. --- .../discourse/app/models/bookmark.js | 18 +- .../templates/components/bookmark-list.hbs | 36 ++-- app/models/bookmark.rb | 103 ++++++++++- app/models/user.rb | 4 + .../user_bookmark_base_serializer.rb | 44 +++++ .../user_bookmark_list_serializer.rb | 20 ++- app/serializers/user_bookmark_serializer.rb | 23 ++- .../user_post_bookmark_serializer.rb | 43 +++++ ...ser_post_topic_bookmark_base_serializer.rb | 77 +++++++++ .../user_topic_bookmark_serializer.rb | 61 +++++++ app/services/bookmarkable.rb | 80 +++++++++ config/locales/server.en.yml | 1 + lib/bookmark_query.rb | 103 +++++++---- spec/fabricators/bookmark_fabricator.rb | 11 +- spec/lib/bookmark_query_spec.rb | 138 +++++++++++---- spec/models/bookmark_spec.rb | 23 +++ spec/models/user_bookmark_list_spec.rb | 55 ++++-- spec/rails_helper.rb | 3 + spec/requests/users_controller_spec.rb | 163 +++++++++++++----- .../user_bookmark_list_serializer_spec.rb | 44 +++++ .../user_post_bookmark_serializer_spec.rb | 27 +++ .../user_topic_bookmark_serializer_spec.rb | 42 +++++ 22 files changed, 976 insertions(+), 143 deletions(-) create mode 100644 app/serializers/user_bookmark_base_serializer.rb create mode 100644 app/serializers/user_post_bookmark_serializer.rb create mode 100644 app/serializers/user_post_topic_bookmark_base_serializer.rb create mode 100644 app/serializers/user_topic_bookmark_serializer.rb create mode 100644 app/services/bookmarkable.rb create mode 100644 spec/serializers/user_bookmark_list_serializer_spec.rb create mode 100644 spec/serializers/user_post_bookmark_serializer_spec.rb create mode 100644 spec/serializers/user_topic_bookmark_serializer_spec.rb diff --git a/app/assets/javascripts/discourse/app/models/bookmark.js b/app/assets/javascripts/discourse/app/models/bookmark.js index f28c92845ee..5d874499a81 100644 --- a/app/assets/javascripts/discourse/app/models/bookmark.js +++ b/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); }, }); diff --git a/app/assets/javascripts/discourse/app/templates/components/bookmark-list.hbs b/app/assets/javascripts/discourse/app/templates/components/bookmark-list.hbs index 819b95296ae..c7954015474 100644 --- a/app/assets/javascripts/discourse/app/templates/components/bookmark-list.hbs +++ b/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}} + + {{bookmark.fancy_title}} + + {{/if}} -
- {{category-link bookmark.category}} - {{discourse-tags bookmark mode="list" tagsForUser=tagsForUser}} -
- {{#if (and site.mobileView bookmark.excerpt bookmark.post_user_avatar_template)}} - - {{avatar bookmark.postUser avatarTemplatePath="avatar_template" usernamePath="username" namePath="name" imageSize="small"}} + {{#if bookmark.bookmarkableTopicAlike}} +
+ {{category-link bookmark.category}} + {{discourse-tags bookmark mode="list" tagsForUser=tagsForUser}} +
+ {{/if}} + {{#if (and site.mobileView bookmark.excerpt bookmark.user.avatar_template)}} + + {{avatar bookmark.bookmarkableUser avatarTemplatePath="avatar_template" usernamePath="username" namePath="name" imageSize="small"}} {{/if}} {{!-- template-lint-disable --}} @@ -49,9 +61,9 @@ {{#unless site.mobileView}} - {{#if bookmark.post_user_avatar_template}} - - {{avatar bookmark.postUser avatarTemplatePath="avatar_template" usernamePath="username" namePath="name" imageSize="small"}} + {{#if bookmark.user.avatar_template}} + + {{avatar bookmark.user avatarTemplatePath="avatar_template" usernamePath="username" namePath="name" imageSize="small"}} {{/if}} diff --git a/app/models/bookmark.rb b/app/models/bookmark.rb index 6007677cde0..aa48363ac6a 100644 --- a/app/models/bookmark.rb +++ b/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 diff --git a/app/models/user.rb b/app/models/user.rb index 2e5bab0b851..3a5f3837529 100644 --- a/app/models/user.rb +++ b/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" diff --git a/app/serializers/user_bookmark_base_serializer.rb b/app/serializers/user_bookmark_base_serializer.rb new file mode 100644 index 00000000000..94ac277c79d --- /dev/null +++ b/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 diff --git a/app/serializers/user_bookmark_list_serializer.rb b/app/serializers/user_bookmark_list_serializer.rb index a7a4ed0fdae..25c840f9c64 100644 --- a/app/serializers/user_bookmark_list_serializer.rb +++ b/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 diff --git a/app/serializers/user_bookmark_serializer.rb b/app/serializers/user_bookmark_serializer.rb index 1edb26adb69..0f8386703ba 100644 --- a/app/serializers/user_bookmark_serializer.rb +++ b/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 diff --git a/app/serializers/user_post_bookmark_serializer.rb b/app/serializers/user_post_bookmark_serializer.rb new file mode 100644 index 00000000000..d1cafcf6ff5 --- /dev/null +++ b/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 diff --git a/app/serializers/user_post_topic_bookmark_base_serializer.rb b/app/serializers/user_post_topic_bookmark_base_serializer.rb new file mode 100644 index 00000000000..b33898d2381 --- /dev/null +++ b/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 diff --git a/app/serializers/user_topic_bookmark_serializer.rb b/app/serializers/user_topic_bookmark_serializer.rb new file mode 100644 index 00000000000..c6a02b8772a --- /dev/null +++ b/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 diff --git a/app/services/bookmarkable.rb b/app/services/bookmarkable.rb new file mode 100644 index 00000000000..525f07ee65b --- /dev/null +++ b/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 diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 7125477c1ab..dbab889524c 100644 --- a/config/locales/server.en.yml +++ b/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" diff --git a/lib/bookmark_query.rb b/lib/bookmark_query.rb index 34ba8318b6a..fea26a77e85 100644 --- a/lib/bookmark_query.rb +++ b/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 diff --git a/spec/fabricators/bookmark_fabricator.rb b/spec/fabricators/bookmark_fabricator.rb index d86816792a7..6cbabbfa00f 100644 --- a/spec/fabricators/bookmark_fabricator.rb +++ b/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| diff --git a/spec/lib/bookmark_query_spec.rb b/spec/lib/bookmark_query_spec.rb index 8bfe3cf97c2..a3563dd53f3 100644 --- a/spec/lib/bookmark_query_spec.rb +++ b/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 diff --git a/spec/models/bookmark_spec.rb b/spec/models/bookmark_spec.rb index d42868ca919..0addabef238 100644 --- a/spec/models/bookmark_spec.rb +++ b/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 diff --git a/spec/models/user_bookmark_list_spec.rb b/spec/models/user_bookmark_list_spec.rb index 43726477364..98f3ddf3fad 100644 --- a/spec/models/user_bookmark_list_spec.rb +++ b/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 diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index b63eda110ae..1b633bd3117 100644 --- a/spec/rails_helper.rb +++ b/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 diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index 163aaf84aa8..6ad8ce3b3b6 100644 --- a/spec/requests/users_controller_spec.rb +++ b/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 + "

Something cooked

" + 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 diff --git a/spec/serializers/user_bookmark_list_serializer_spec.rb b/spec/serializers/user_bookmark_list_serializer_spec.rb new file mode 100644 index 00000000000..8d32f2c77bc --- /dev/null +++ b/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 diff --git a/spec/serializers/user_post_bookmark_serializer_spec.rb b/spec/serializers/user_post_bookmark_serializer_spec.rb new file mode 100644 index 00000000000..2e678c4e969 --- /dev/null +++ b/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 diff --git a/spec/serializers/user_topic_bookmark_serializer_spec.rb b/spec/serializers/user_topic_bookmark_serializer_spec.rb new file mode 100644 index 00000000000..34f332e6edb --- /dev/null +++ b/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