From 222c8d9b6a03b2e80d26669c90ff2db1a27d6c54 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Mon, 9 May 2022 09:37:23 +1000 Subject: [PATCH] FEATURE: Polymorphic bookmarks pt. 3 (reminders, imports, exports, refactors) (#16591) A bit of a mixed bag, this addresses several edge areas of bookmarks and makes them compatible with polymorphic bookmarks (hidden behind the `use_polymorphic_bookmarks` site setting). The main ones are: * ExportUserArchive compatibility * SyncTopicUserBookmarked job compatibility * Sending different notifications for the bookmark reminders based on the bookmarkable type * Import scripts compatibility * BookmarkReminderNotificationHandler compatibility This PR also refactors the `register_bookmarkable` API so it accepts a class descended from a `BaseBookmarkable` class instead. This was done because we kept having to add more and more lambdas/properties inline and it was very messy, so a factory pattern is cleaner. The classes can be tested independently as well. Some later PRs will address some other areas like the discourse narrative bot, advanced search, reports, and the .ics endpoint for bookmarks. --- .../bookmark-reminder-notification-item.js | 1 + .../app/widgets/default-notification-item.js | 22 ++-- app/controllers/users_controller.rb | 1 + app/jobs/regular/export_user_archive.rb | 80 ++++++++++--- .../regular/sync_topic_user_bookmarked.rb | 36 ++++-- .../bookmark_reminder_notifications.rb | 2 +- app/models/bookmark.rb | 71 ++--------- app/models/post.rb | 1 + app/services/base_bookmarkable.rb | 112 ++++++++++++++++++ app/services/post_bookmarkable.rb | 60 ++++++++++ ...markable.rb => registered_bookmarkable.rb} | 63 +++++----- app/services/topic_bookmarkable.rb | 57 +++++++++ lib/bookmark_manager.rb | 2 +- lib/bookmark_reminder_notification_handler.rb | 64 ++++++---- lib/guardian/bookmark_guardian.rb | 8 +- lib/search.rb | 2 + .../new_user_narrative.rb | 2 + plugins/discourse-narrative-bot/plugin.rb | 1 + script/import_scripts/base.rb | 7 +- script/import_scripts/jive_api.rb | 8 +- .../bookmark_reminder_notifications_spec.rb | 18 +-- spec/jobs/export_user_archive_spec.rb | 60 ++++++++-- spec/jobs/sync_topic_user_bookmarked_spec.rb | 83 ++++++++----- spec/lib/bookmark_query_spec.rb | 19 +-- ...mark_reminder_notification_handler_spec.rb | 53 +++++++-- spec/models/bookmark_spec.rb | 13 +- spec/models/user_bookmark_list_spec.rb | 13 +- spec/requests/users_controller_spec.rb | 13 +- spec/script/import_scripts/base_spec.rb | 14 +++ .../user_bookmark_list_serializer_spec.rb | 15 +-- spec/services/post_bookmarkable_spec.rb | 106 +++++++++++++++++ spec/services/topic_bookmarkable_spec.rb | 102 ++++++++++++++++ spec/support/bookmarkable_helper.rb | 42 +++++++ 33 files changed, 880 insertions(+), 271 deletions(-) create mode 100644 app/services/base_bookmarkable.rb create mode 100644 app/services/post_bookmarkable.rb rename app/services/{bookmarkable.rb => registered_bookmarkable.rb} (51%) create mode 100644 app/services/topic_bookmarkable.rb create mode 100644 spec/services/post_bookmarkable_spec.rb create mode 100644 spec/services/topic_bookmarkable_spec.rb create mode 100644 spec/support/bookmarkable_helper.rb diff --git a/app/assets/javascripts/discourse/app/widgets/bookmark-reminder-notification-item.js b/app/assets/javascripts/discourse/app/widgets/bookmark-reminder-notification-item.js index 5dd5c2c3141..d75cfb16c9b 100644 --- a/app/assets/javascripts/discourse/app/widgets/bookmark-reminder-notification-item.js +++ b/app/assets/javascripts/discourse/app/widgets/bookmark-reminder-notification-item.js @@ -16,6 +16,7 @@ createWidgetFrom( username, }); }, + notificationTitle(notificationName, data) { if (notificationName) { if (data.bookmark_name) { diff --git a/app/assets/javascripts/discourse/app/widgets/default-notification-item.js b/app/assets/javascripts/discourse/app/widgets/default-notification-item.js index 0a7a263f50d..fa571a4e27f 100644 --- a/app/assets/javascripts/discourse/app/widgets/default-notification-item.js +++ b/app/assets/javascripts/discourse/app/widgets/default-notification-item.js @@ -29,9 +29,9 @@ export const DefaultNotificationItem = createWidget( if (attrs.is_warning) { classNames.push("is-warning"); } - const notificationType = attrs.notification_type; - const lookup = this.site.get("notificationLookup"); - const notificationName = lookup[notificationType]; + const notificationName = this.lookupNotificationName( + attrs.notification_type + ); if (notificationName) { classNames.push(notificationName.replace(/_/g, "-")); } @@ -64,6 +64,10 @@ export const DefaultNotificationItem = createWidget( if (data.group_id) { return userPath(data.username + "/messages/group/" + data.group_name); } + + if (data.bookmarkable_url) { + return getURL(data.bookmarkable_url); + } }, description(data) { @@ -90,7 +94,7 @@ export const DefaultNotificationItem = createWidget( return this.attrs.fancy_title; } - const description = data.topic_title; + const description = data.topic_title || data.title; return isEmpty(description) ? "" : escapeExpression(description); }, @@ -126,11 +130,15 @@ export const DefaultNotificationItem = createWidget( } }, - html(attrs) { - const notificationType = attrs.notification_type; + lookupNotificationName(notificationType) { const lookup = this.site.get("notificationLookup"); - const notificationName = lookup[notificationType]; + return lookup[notificationType]; + }, + html(attrs) { + const notificationName = this.lookupNotificationName( + attrs.notification_type + ); let { data } = attrs; let text = emojiUnescape(this.text(notificationName, data)); let icon = this.icon(notificationName, data); diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 2a0c900c8ca..124fbd77c9c 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -1720,6 +1720,7 @@ class UsersController < ApplicationController render_serialized(bookmark_list, UserBookmarkListSerializer) end end + # TODO (martin) Make a separate PR for .ics reminders for polymorphic bookmarks format.ics do @bookmark_reminders = Bookmark.with_reminders .where(user_id: user.id) diff --git a/app/jobs/regular/export_user_archive.rb b/app/jobs/regular/export_user_archive.rb index 489dac28b23..158f2a72f5e 100644 --- a/app/jobs/regular/export_user_archive.rb +++ b/app/jobs/regular/export_user_archive.rb @@ -31,7 +31,9 @@ module Jobs auth_tokens: ['id', 'auth_token_hash', 'prev_auth_token_hash', 'auth_token_seen', 'client_ip', 'user_agent', 'seen_at', 'rotated_at', 'created_at', 'updated_at'], auth_token_logs: ['id', 'action', 'user_auth_token_id', 'client_ip', 'auth_token_hash', 'created_at', 'path', 'user_agent'], badges: ['badge_id', 'badge_name', 'granted_at', 'post_id', 'seq', 'granted_manually', 'notification_id', 'featured_rank'], + # TODO (martin) [POLYBOOK] - Remove the duplication when polymorphic bookmarks are implemented bookmarks: ['post_id', 'topic_id', 'post_number', 'link', 'name', 'created_at', 'updated_at', 'reminder_at', 'reminder_last_sent_at', 'reminder_set_at', 'auto_delete_preference'], + bookmarks_polymorphic: ['bookmarkable_id', 'bookmarkable_type', 'link', 'name', 'created_at', 'updated_at', 'reminder_at', 'reminder_last_sent_at', 'reminder_set_at', 'auto_delete_preference'], category_preferences: ['category_id', 'category_names', 'notification_level', 'dismiss_new_timestamp'], flags: ['id', 'post_id', 'flag_type', 'created_at', 'updated_at', 'deleted_at', 'deleted_by', 'related_post_id', 'targets_topic', 'was_take_action'], likes: ['id', 'post_id', 'topic_id', 'post_number', 'created_at', 'updated_at', 'deleted_at', 'deleted_by'], @@ -48,7 +50,17 @@ module Jobs components = [] COMPONENTS.each do |name| - h = { name: name, method: :"#{name}_export" } + export_method = \ + if name == "bookmarks" + if SiteSetting.use_polymorphic_bookmarks + "bookmarks_polymorphic_export" + else + "bookmarks_export" + end + else + "#{name}_export" + end + h = { name: name, method: :"#{export_method}" } h[:filetype] = :csv filetype_method = :"#{name}_filetype" if respond_to? filetype_method @@ -228,30 +240,56 @@ module Jobs end end + def bookmarks_polymorphic_export + return enum_for(:bookmarks_polymorphic_export) unless block_given? + + @current_user.bookmarks.where.not(bookmarkable_type: nil).order(:id).each do |bookmark| + link = '' + if guardian.can_see_bookmarkable?(bookmark) + if bookmark.bookmarkable.respond_to?(:full_url) + link = bookmark.bookmarkable.full_url + else + link = bookmark.bookmarkable.url + end + end + + yield [ + bookmark.bookmarkable_id, + bookmark.bookmarkable_type, + link, + bookmark.name, + bookmark.created_at, + bookmark.updated_at, + bookmark.reminder_at, + bookmark.reminder_last_sent_at, + bookmark.reminder_set_at, + Bookmark.auto_delete_preferences[bookmark.auto_delete_preference], + ] + end + end + + # TODO (martin) [POLYBOOK] No longer relevant after polymorphic bookmarks implemented def bookmarks_export return enum_for(:bookmarks_export) unless block_given? - Bookmark - .where(user_id: @current_user.id) - .joins(:post) - .order(:id) - .each do |bkmk| + @current_user.bookmarks.joins(:post).order(:id).each do |bookmark| link = '' - if guardian.can_see_post?(bkmk.post) - link = bkmk.post.full_url + if guardian.can_see_bookmarkable?(bookmark) + link = bookmark.post.full_url end + yield [ - bkmk.post_id, - bkmk.topic_id, - bkmk.post&.post_number, + bookmark.post_id, + bookmark.topic_id, + bookmark.post&.post_number, link, - bkmk.name, - bkmk.created_at, - bkmk.updated_at, - bkmk.reminder_at, - bkmk.reminder_last_sent_at, - bkmk.reminder_set_at, - Bookmark.auto_delete_preferences[bkmk.auto_delete_preference], + bookmark.name, + bookmark.created_at, + bookmark.updated_at, + bookmark.reminder_at, + bookmark.reminder_last_sent_at, + bookmark.reminder_set_at, + Bookmark.auto_delete_preferences[bookmark.auto_delete_preference], ] end end @@ -396,6 +434,12 @@ module Jobs end end header_array.push("group_names") + elsif entity == 'bookmarks' + if SiteSetting.use_polymorphic_bookmarks + header_array = HEADER_ATTRS_FOR['bookmarks_polymorphic'] + else + header_array = HEADER_ATTRS_FOR['bookmarks'] + end else header_array = HEADER_ATTRS_FOR[entity] end diff --git a/app/jobs/regular/sync_topic_user_bookmarked.rb b/app/jobs/regular/sync_topic_user_bookmarked.rb index 8c0621875f3..2710861c59d 100644 --- a/app/jobs/regular/sync_topic_user_bookmarked.rb +++ b/app/jobs/regular/sync_topic_user_bookmarked.rb @@ -1,14 +1,35 @@ # frozen_string_literal: true module Jobs - - # TODO (martin) [POLYBOOK] This will need to be restructured for polymorphic - # bookmarks when edge cases are handled. class SyncTopicUserBookmarked < ::Jobs::Base def execute(args = {}) topic_id = args[:topic_id] - DB.exec(<<~SQL, topic_id: topic_id) + if SiteSetting.use_polymorphic_bookmarks + DB.exec(<<~SQL, topic_id: topic_id) + UPDATE topic_users SET bookmarked = true + FROM bookmarks AS b + INNER JOIN posts ON posts.id = b.bookmarkable_id AND b.bookmarkable_type = 'Post' + WHERE NOT topic_users.bookmarked AND + posts.deleted_at IS NULL AND + topic_users.topic_id = posts.topic_id AND + topic_users.user_id = b.user_id #{topic_id.present? ? "AND topic_users.topic_id = :topic_id" : ""} + SQL + + DB.exec(<<~SQL, topic_id: topic_id) + UPDATE topic_users SET bookmarked = false + WHERE topic_users.bookmarked AND + ( + SELECT COUNT(*) + FROM bookmarks + INNER JOIN posts ON posts.id = bookmarks.bookmarkable_id AND bookmarks.bookmarkable_type = 'Post' + WHERE posts.topic_id = topic_users.topic_id + AND bookmarks.user_id = topic_users.user_id + AND posts.deleted_at IS NULL + ) = 0 #{topic_id.present? ? "AND topic_users.topic_id = :topic_id" : ""} + SQL + else + DB.exec(<<~SQL, topic_id: topic_id) UPDATE topic_users SET bookmarked = true FROM bookmarks AS b INNER JOIN posts ON posts.id = b.post_id @@ -16,9 +37,9 @@ module Jobs posts.deleted_at IS NULL AND topic_users.topic_id = posts.topic_id AND topic_users.user_id = b.user_id #{topic_id.present? ? "AND topic_users.topic_id = :topic_id" : ""} - SQL + SQL - DB.exec(<<~SQL, topic_id: topic_id) + DB.exec(<<~SQL, topic_id: topic_id) UPDATE topic_users SET bookmarked = false WHERE topic_users.bookmarked AND ( @@ -29,7 +50,8 @@ module Jobs AND bookmarks.user_id = topic_users.user_id AND posts.deleted_at IS NULL ) = 0 #{topic_id.present? ? "AND topic_users.topic_id = :topic_id" : ""} - SQL + SQL + end end end end diff --git a/app/jobs/scheduled/bookmark_reminder_notifications.rb b/app/jobs/scheduled/bookmark_reminder_notifications.rb index 0795e607bca..712a650fc02 100644 --- a/app/jobs/scheduled/bookmark_reminder_notifications.rb +++ b/app/jobs/scheduled/bookmark_reminder_notifications.rb @@ -20,7 +20,7 @@ module Jobs def execute(args = nil) bookmarks = Bookmark.pending_reminders.includes(:user).order('reminder_at ASC') bookmarks.limit(BookmarkReminderNotifications.max_reminder_notifications_per_run).each do |bookmark| - BookmarkReminderNotificationHandler.send_notification(bookmark) + BookmarkReminderNotificationHandler.new(bookmark).send_notification end end end diff --git a/app/models/bookmark.rb b/app/models/bookmark.rb index 54dc9534512..824b8953ff1 100644 --- a/app/models/bookmark.rb +++ b/app/models/bookmark.rb @@ -8,16 +8,9 @@ class Bookmark < ActiveRecord::Base Bookmark.registered_bookmarkables.find { |bm| bm.model.name == type } end - def self.register_bookmarkable( - 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 - ) + def self.register_bookmarkable(bookmarkable_klass) + return if Bookmark.registered_bookmarkable_from_type(bookmarkable_klass.model.name).present? + Bookmark.registered_bookmarkables << RegisteredBookmarkable.new(bookmarkable_klass) end ## @@ -30,55 +23,9 @@ class Bookmark < ActiveRecord::Base # classes are not cached. def self.reset_bookmarkables self.registered_bookmarkables = [] - Bookmark.register_bookmarkable( - model: Post, - 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 }] - ) + + Bookmark.register_bookmarkable(PostBookmarkable) + Bookmark.register_bookmarkable(TopicBookmarkable) end reset_bookmarkables @@ -133,6 +80,10 @@ class Bookmark < ActiveRecord::Base validate :bookmark_limit_not_reached validates :name, length: { maximum: 100 } + def registered_bookmarkable + Bookmark.registered_bookmarkable_from_type(self.bookmarkable_type) + end + def polymorphic_columns_present return if !SiteSetting.use_polymorphic_bookmarks return if self.bookmarkable_id.present? && self.bookmarkable_type.present? @@ -263,6 +214,8 @@ class Bookmark < ActiveRecord::Base end end + # TODO (martin) [POLYBOOK] Make a separate PR for reports + # functionality as the bookmarkables will have to define this. def self.count_per_day(opts = nil) opts ||= {} result = where('bookmarks.created_at >= ?', opts[:start_date] || (opts[:since_days_ago] || 30).days.ago) diff --git a/app/models/post.rb b/app/models/post.rb index 371ea3172a6..7ffed8307ae 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -47,6 +47,7 @@ class Post < ActiveRecord::Base has_one :post_stat + # TODO (martin) [POLYBOOK] # When we are ready we can add as: :bookmarkable here to use the # polymorphic association. has_many :bookmarks diff --git a/app/services/base_bookmarkable.rb b/app/services/base_bookmarkable.rb new file mode 100644 index 00000000000..a91a5d93805 --- /dev/null +++ b/app/services/base_bookmarkable.rb @@ -0,0 +1,112 @@ +# frozen_string_literal: true + +## +# Anything that we want to be able to bookmark must be registered as a +# bookmarkable type using Bookmark.register_bookmarkable(bookmarkable_klass), +# where the bookmarkable_klass is a class that implements this BaseBookmarkable +# interface. Some examples are TopicBookmarkable and PostBookmarkable. +# +# These methods are then called by the RegisteredBookmarkable class through a public +# interface, used in places where we need to list, send reminders for, +# or otherwise interact with bookmarks in a way that is unique to the +# bookmarkable type. +# +# See RegisteredBookmarkable for additional documentation. +class BaseBookmarkable + attr_reader :model, :serializer, :preload_associations + + # @return [ActiveRecord::Base] The ActiveRecord model class which will be used to denote + # the type of the bookmarkable upon registration along with + # querying. + def self.model + raise NotImplementedError + end + + # @return [ApplicationSerializer] The serializer class inheriting from UserBookmarkBaseSerializer + def self.serializer + raise NotImplementedError + end + + # @return [Array] Used for preloading associations on the bookmarks for listing + # purposes. Should be in the same format used for .includes() e.g. + # + # [{ topic: [:topic_users, :tags] }, :user] + def self.preload_associations + nil + end + + def self.has_preloads? + preload_associations.present? + end + + ## + # This is where the main query to filter the bookmarks by the provided bookmarkable + # type should occur. This should join on additional tables that are required later + # on to preload additional data for serializers, and also is the place where the + # bookmarks should be filtered based on security checks, which is why the Guardian + # instance is provided. + # + # @param [User] user The user to perform the query for, this scopes the bookmarks returned. + # @param [Guardian] guardian An instance of Guardian for the user to be used for security filters. + # @return [Bookmark::ActiveRecord_AssociationRelation] Should be an approprialely scoped list of bookmarks for the user. + def self.list_query(user, guardian) + raise NotImplementedError + end + + ## + # Called from BookmarkQuery when the initial results have been returned by + # perform_list_query. The search_query should join additional tables required + # to filter the bookmarks further, as well as defining a string used for + # where_sql, which can include comparisons with the :q parameter. + # + # @param [Bookmark::ActiveRecord_Relation] bookmarks The bookmark records returned by perform_list_query + # @param [String] query The search query from the user surrounded by the %% wildcards + # @param [String] ts_query The postgres TSQUERY string used for comparisons with full text search columns + # @param [Block] bookmarkable_search This block _must_ be called with the additional WHERE clause SQL relevant + # for the bookmarkable to be searched, as well as the bookmarks relation + # with any additional joins applied. + # @return [Bookmark::ActiveRecord_AssociationRelation] The list of bookmarks from perform_list_query filtered further by + # the query parameter. + def self.search_query(bookmarks, query, ts_query, &bookmarkable_search) + raise NotImplementedError + end + + ## + # When sending bookmark reminders, we want to make sure that whatever we + # are sending the reminder for has not been deleted or is otherwise inaccessible. + # Most of the time we can just check if the bookmarkable record is present + # because it will be trashable, though in some cases there will be additional + # conditions in the form of a lambda that we should use instead. + # + # The logic around whether it is the right time to send a reminder does not belong + # here, that is done in the BookmarkReminderNotifications job. + # + # @param [Bookmark] bookmark The bookmark that we are considering sending a reminder for. + # @return [Boolean] + def self.reminder_conditions(bookmark) + raise NotImplementedError + end + + ## + # Different bookmarkables may have different ways of notifying a user or presenting + # the reminder and what it is for, so it is up to the bookmarkable to register + # its preferred method of sending the reminder. + # + # @param [Bookmark] bookmark The bookmark that we are sending the reminder notification for. + # @return [void] + def self.reminder_handler(bookmark) + raise NotImplementedError + end + + ## + # Access control is dependent on what has been bookmarked, the appropriate guardian + # can_see_X? method should be called from the bookmarkable class to determine + # whether the bookmarkable record (e.g. Post, Topic) is accessible by the guardian user. + # + # @param [Guardian] guardian The guardian class for the user that we are performing the access check for. + # @param [Bookmark] bookmark The bookmark which we are checking access for using the bookmarkable association. + # @return [Boolean] + def self.can_see?(guardian, bookmark) + raise NotImplementedError + end +end diff --git a/app/services/post_bookmarkable.rb b/app/services/post_bookmarkable.rb new file mode 100644 index 00000000000..075f206b0c7 --- /dev/null +++ b/app/services/post_bookmarkable.rb @@ -0,0 +1,60 @@ +# frozen_string_literal: true + +class PostBookmarkable < BaseBookmarkable + def self.model + Post + end + + def self.serializer + UserPostBookmarkSerializer + end + + def self.preload_associations + [{ topic: [:topic_users, :tags] }, :user] + end + + def self.list_query(user, guardian) + topics = Topic.listable_topics.secured(guardian) + pms = Topic.private_messages_for_user(user) + post_bookmarks = user + .bookmarks_of_type("Post") + .joins("INNER JOIN posts ON posts.id = bookmarks.bookmarkable_id AND bookmarks.bookmarkable_type = 'Post'") + .joins("LEFT JOIN topics ON topics.id = posts.topic_id") + .joins("LEFT JOIN topic_users ON topic_users.topic_id = topics.id") + .where("topic_users.user_id = ?", user.id) + guardian.filter_allowed_categories( + post_bookmarks.merge(topics.or(pms)).merge(Post.secured(guardian)) + ) + end + + def self.search_query(bookmarks, query, ts_query, &bookmarkable_search) + bookmarkable_search.call( + bookmarks.joins( + "LEFT JOIN post_search_data ON post_search_data.post_id = bookmarks.bookmarkable_id AND bookmarks.bookmarkable_type = 'Post'" + ), + "#{ts_query} @@ post_search_data.search_data" + ) + end + + def self.reminder_handler(bookmark) + bookmark.user.notifications.create!( + notification_type: Notification.types[:bookmark_reminder], + topic_id: bookmark.bookmarkable.topic_id, + post_number: bookmark.bookmarkable.post_number, + data: { + title: bookmark.bookmarkable.topic.title, + display_username: bookmark.user.username, + bookmark_name: bookmark.name, + bookmarkable_url: bookmark.bookmarkable.url + }.to_json + ) + end + + def self.reminder_conditions(bookmark) + bookmark.bookmarkable.present? && bookmark.bookmarkable.topic.present? + end + + def self.can_see?(guardian, bookmark) + guardian.can_see_post?(bookmark.bookmarkable) + end +end diff --git a/app/services/bookmarkable.rb b/app/services/registered_bookmarkable.rb similarity index 51% rename from app/services/bookmarkable.rb rename to app/services/registered_bookmarkable.rb index b5cd2e8ab60..ba05a84bc2f 100644 --- a/app/services/bookmarkable.rb +++ b/app/services/registered_bookmarkable.rb @@ -3,7 +3,7 @@ # Should only be created via the Bookmark.register_bookmarkable # method; this is used to let the BookmarkQuery class query and # search additional bookmarks for the user bookmark list, and -# also to enumerate on the registered Bookmarkable types. +# also to enumerate on the registered RegisteredBookmarkable types. # # Post and Topic bookmarkables are registered by default. # @@ -13,37 +13,24 @@ # # See Bookmark#reset_bookmarkables for some examples on how registering # bookmarkables works. -class Bookmarkable - attr_reader :model, :serializer, :list_query, :search_query, :preload_associations - delegate :table_name, to: :@model +# +# See BaseBookmarkable for documentation on what return types should be +# and what the arguments to the methods are. +class RegisteredBookmarkable + attr_reader :bookmarkable_klass - 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 + delegate :model, :serializer, to: :@bookmarkable_klass + delegate :table_name, to: :model + + def initialize(bookmarkable_klass) + @bookmarkable_klass = bookmarkable_klass end - ## - # This is where the main query to filter the bookmarks by the provided bookmarkable - # type should occur. This should join on additional tables that are required later - # on to preload additional data for serializers, and also is the place where the - # bookmarks should be filtered based on security checks, which is why the Guardian - # instance is provided. - # - # @param [User] user The user to perform the query for, this scopes the bookmarks returned. - # @param [Guardian] guardian An instance of Guardian for the user to be used for security filters. def perform_list_query(user, guardian) - list_query.call(user, guardian) + bookmarkable_klass.list_query(user, guardian) end ## - # Called from BookmarkQuery when the initial results have been returned by - # perform_list_query. The search_query should join additional tables required - # to filter the bookmarks further, as well as defining a string used for - # where_sql, which can include comparisons with the :q parameter. - # # The block here warrants explanation -- when the search_query is called, we # call the provided block with the bookmark relation with additional joins # as well as the where_sql string, and then also add the additional OR bookmarks.name @@ -51,11 +38,9 @@ class Bookmarkable # columns _as well as_ the bookmark name, because the bookmark name must always # be used in the search. # - # @param [Bookmark::ActiveRecord_Relation] bookmarks The bookmark records returned by perform_list_query - # @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 + # See BaseBookmarkable#search_query for argument docs. def perform_search_query(bookmarks, query, ts_query) - search_query.call(bookmarks, query, ts_query) do |bookmarks_joined, where_sql| + bookmarkable_klass.search_query(bookmarks, query, ts_query) do |bookmarks_joined, where_sql| bookmarks_joined.where("#{where_sql} OR bookmarks.name ILIKE :q", q: query) end end @@ -72,9 +57,27 @@ class Bookmarkable # # @param [Array] bookmarks The array of bookmarks after initial listing and filtering, note this is # array _not_ an ActiveRecord::Relation. + # @return [void] def perform_preload(bookmarks) + return if !bookmarkable_klass.has_preloads? + ActiveRecord::Associations::Preloader - .new(records: Bookmark.select_type(bookmarks, model.to_s), associations: [bookmarkable: preload_associations]) + .new( + records: Bookmark.select_type(bookmarks, bookmarkable_klass.model.to_s), + associations: [bookmarkable: bookmarkable_klass.preload_associations] + ) .call end + + def can_send_reminder?(bookmark) + bookmarkable_klass.reminder_conditions(bookmark) + end + + def send_reminder_notification(bookmark) + bookmarkable_klass.reminder_handler(bookmark) + end + + def can_see?(guardian, bookmark) + bookmarkable_klass.can_see?(guardian, bookmark) + end end diff --git a/app/services/topic_bookmarkable.rb b/app/services/topic_bookmarkable.rb new file mode 100644 index 00000000000..4293903c5e3 --- /dev/null +++ b/app/services/topic_bookmarkable.rb @@ -0,0 +1,57 @@ +# frozen_string_literal: true + +class TopicBookmarkable < BaseBookmarkable + def self.model + Topic + end + + def self.serializer + UserTopicBookmarkSerializer + end + + def self.preload_associations + [:topic_users, :tags, { posts: :user }] + end + + def self.list_query(user, guardian) + topics = Topic.listable_topics.secured(guardian) + pms = Topic.private_messages_for_user(user) + topic_bookmarks = user + .bookmarks_of_type("Topic") + .joins("INNER JOIN topics ON topics.id = bookmarks.bookmarkable_id AND bookmarks.bookmarkable_type = 'Topic'") + .joins("LEFT JOIN topic_users ON topic_users.topic_id = topics.id") + .where("topic_users.user_id = ?", user.id) + guardian.filter_allowed_categories(topic_bookmarks.merge(topics.or(pms))) + end + + def self.search_query(bookmarks, query, ts_query, &bookmarkable_search) + bookmarkable_search.call( + bookmarks + .joins("LEFT JOIN posts ON posts.topic_id = topics.id AND posts.post_number = 1") + .joins("LEFT JOIN post_search_data ON post_search_data.post_id = posts.id"), + "#{ts_query} @@ post_search_data.search_data" + ) + end + + def self.reminder_handler(bookmark) + bookmark.user.notifications.create!( + notification_type: Notification.types[:bookmark_reminder], + topic_id: bookmark.bookmarkable_id, + post_number: 1, + data: { + title: bookmark.bookmarkable.title, + display_username: bookmark.user.username, + bookmark_name: bookmark.name, + bookmarkable_url: bookmark.bookmarkable.first_post.url + }.to_json + ) + end + + def self.reminder_conditions(bookmark) + bookmark.bookmarkable.present? + end + + def self.can_see?(guardian, bookmark) + guardian.can_see_topic?(bookmark.bookmarkable) + end +end diff --git a/lib/bookmark_manager.rb b/lib/bookmark_manager.rb index 843c0b0426f..40be5504259 100644 --- a/lib/bookmark_manager.rb +++ b/lib/bookmark_manager.rb @@ -135,7 +135,7 @@ class BookmarkManager def self.send_reminder_notification(id) bookmark = Bookmark.find_by(id: id) - BookmarkReminderNotificationHandler.send_notification(bookmark) + BookmarkReminderNotificationHandler.new(bookmark).send_notification end def update(bookmark_id:, name:, reminder_at:, options: {}) diff --git a/lib/bookmark_reminder_notification_handler.rb b/lib/bookmark_reminder_notification_handler.rb index 4be52433c42..d4dfd901566 100644 --- a/lib/bookmark_reminder_notification_handler.rb +++ b/lib/bookmark_reminder_notification_handler.rb @@ -1,30 +1,35 @@ # frozen_string_literal: true class BookmarkReminderNotificationHandler - def self.send_notification(bookmark) + attr_reader :bookmark + + def initialize(bookmark) + @bookmark = bookmark + end + + def send_notification return if bookmark.blank? Bookmark.transaction do - # we don't send reminders for deleted posts or topics, - # just as we don't allow creation of bookmarks for deleted - # posts or topics - # - # TODO (martin) [POLYBOOK] This will need to be restructured for polymorphic - # bookmarks when reminders are handled. - if bookmark.post.blank? || bookmark.topic.blank? - clear_reminder(bookmark) + # TODO (martin) [POLYBOOK] Can probably change this to call the + # can_send_reminder? on the registered bookmarkable directly instead + # of having can_send_reminder? + if !can_send_reminder? + clear_reminder else - create_notification(bookmark) + create_notification if bookmark.auto_delete_when_reminder_sent? BookmarkManager.new(bookmark.user).destroy(bookmark.id) end - clear_reminder(bookmark) + clear_reminder end end end - def self.clear_reminder(bookmark) + private + + def clear_reminder Rails.logger.debug( "Clearing bookmark reminder for bookmark_id #{bookmark.id}. reminder at: #{bookmark.reminder_at}" ) @@ -36,17 +41,28 @@ class BookmarkReminderNotificationHandler bookmark.clear_reminder! end - def self.create_notification(bookmark) - user = bookmark.user - user.notifications.create!( - notification_type: Notification.types[:bookmark_reminder], - topic_id: bookmark.topic_id, - post_number: bookmark.post.post_number, - data: { - topic_title: bookmark.topic.title, - display_username: user.username, - bookmark_name: bookmark.name - }.to_json - ) + def can_send_reminder? + if SiteSetting.use_polymorphic_bookmarks + bookmark.registered_bookmarkable.can_send_reminder?(bookmark) + else + bookmark.post.present? && bookmark.topic.present? + end + end + + def create_notification + if SiteSetting.use_polymorphic_bookmarks + bookmark.registered_bookmarkable.send_reminder_notification(bookmark) + else + bookmark.user.notifications.create!( + notification_type: Notification.types[:bookmark_reminder], + topic_id: bookmark.topic_id, + post_number: bookmark.post.post_number, + data: { + topic_title: bookmark.topic.title, + display_username: bookmark.user.username, + bookmark_name: bookmark.name + }.to_json + ) + end end end diff --git a/lib/guardian/bookmark_guardian.rb b/lib/guardian/bookmark_guardian.rb index 7f0b63c6209..fb0bc5370ea 100644 --- a/lib/guardian/bookmark_guardian.rb +++ b/lib/guardian/bookmark_guardian.rb @@ -9,7 +9,11 @@ module BookmarkGuardian @user == bookmark.user end - def can_create_bookmark?(bookmark) - can_see_topic?(bookmark.topic) + def can_see_bookmarkable?(bookmark) + if SiteSetting.use_polymorphic_bookmarks? + return bookmark.registered_bookmarkable.can_see?(self, bookmark) + end + + self.can_see_post?(bookmark.post) end end diff --git a/lib/search.rb b/lib/search.rb index 1d226d1a049..a9974d48564 100644 --- a/lib/search.rb +++ b/lib/search.rb @@ -453,6 +453,8 @@ class Search end end + # TODO (martin) [POLYBOOK] Make a separate PR for advanced searched in:bookmarks + # functionality as the bookmarkables will have to define this. advanced_filter(/^in:(bookmarks)$/i) do |posts, match| if @guardian.user posts.where("posts.id IN (SELECT post_id FROM bookmarks WHERE bookmarks.user_id = #{@guardian.user.id})") diff --git a/plugins/discourse-narrative-bot/lib/discourse_narrative_bot/new_user_narrative.rb b/plugins/discourse-narrative-bot/lib/discourse_narrative_bot/new_user_narrative.rb index 98ef6e4c8eb..3b2d5e48aab 100644 --- a/plugins/discourse-narrative-bot/lib/discourse_narrative_bot/new_user_narrative.rb +++ b/plugins/discourse-narrative-bot/lib/discourse_narrative_bot/new_user_narrative.rb @@ -243,6 +243,7 @@ module DiscourseNarrativeBot post end + # TODO (martin) [POLYBOOK] Fix up narrative bot bookmark interactions in a separate PR. def missing_bookmark return unless valid_topic?(@post.topic_id) return if @post.user_id == self.discobot_user.id @@ -253,6 +254,7 @@ module DiscourseNarrativeBot false end + # TODO (martin) [POLYBOOK] Fix up narrative bot bookmark interactions in a separate PR. def reply_to_bookmark return unless valid_topic?(@post.topic_id) return unless @post.user_id == self.discobot_user.id diff --git a/plugins/discourse-narrative-bot/plugin.rb b/plugins/discourse-narrative-bot/plugin.rb index 2eba865b377..0d1c3a2b211 100644 --- a/plugins/discourse-narrative-bot/plugin.rb +++ b/plugins/discourse-narrative-bot/plugin.rb @@ -281,6 +281,7 @@ after_initialize do end end + # TODO (martin) [POLYBOOK] Fix up narrative bot bookmark interactions in a separate PR. self.add_model_callback(Bookmark, :after_commit, on: :create) do if self.post && self.user.enqueue_narrative_bot_job? Jobs.enqueue(:bot_input, user_id: self.user_id, post_id: self.post_id, input: "bookmark") diff --git a/script/import_scripts/base.rb b/script/import_scripts/base.rb index 6a28124d3fa..67fb5cbc578 100644 --- a/script/import_scripts/base.rb +++ b/script/import_scripts/base.rb @@ -624,7 +624,12 @@ class ImportScripts::Base else begin manager = BookmarkManager.new(user) - bookmark = manager.create(post_id: post.id) + + if SiteSetting.use_polymorphic_bookmarks + bookmark = manager.create_for(bookmarkable_id: post.id, bookmarkable_type: "Post") + else + bookmark = manager.create(post_id: post.id) + end created += 1 if manager.errors.none? skipped += 1 if manager.errors.any? diff --git a/script/import_scripts/jive_api.rb b/script/import_scripts/jive_api.rb index cee4928227d..4c7cbfad190 100644 --- a/script/import_scripts/jive_api.rb +++ b/script/import_scripts/jive_api.rb @@ -285,10 +285,14 @@ class ImportScripts::JiveApi < ImportScripts::Base loop do favorites = get("contents?#{fields}&filter=type(favorite)#{filter}&sort=dateCreatedAsc&count=#{POST_COUNT}&startIndex=#{start_index}") - favorites["list"].each do |favorite| + bookmarks_to_create = favorites["list"].map do |favorite| next unless user_id = user_id_from_imported_user_id(favorite["author"]["id"]) next unless post_id = post_id_from_imported_post_id(favorite["favoriteObject"]["id"]) - PostActionCreator.create(User.find(user_id), Post.find(post_id), :bookmark) + { user_id: user_id, post_id: post_id } + end.flatten + + create_bookmarks(bookmarks_to_create) do |row| + row end break if favorites["list"].size < POST_COUNT || favorites.dig("links", "next").blank? diff --git a/spec/jobs/bookmark_reminder_notifications_spec.rb b/spec/jobs/bookmark_reminder_notifications_spec.rb index 6d64c9f0093..5953b5a34d5 100644 --- a/spec/jobs/bookmark_reminder_notifications_spec.rb +++ b/spec/jobs/bookmark_reminder_notifications_spec.rb @@ -3,10 +3,11 @@ RSpec.describe Jobs::BookmarkReminderNotifications do subject { described_class.new } + fab!(:user) { Fabricate(:user) } let(:five_minutes_ago) { Time.zone.now - 5.minutes } - let(:bookmark1) { Fabricate(:bookmark) } - let(:bookmark2) { Fabricate(:bookmark) } - let(:bookmark3) { Fabricate(:bookmark) } + let(:bookmark1) { Fabricate(:bookmark, user: user) } + let(:bookmark2) { Fabricate(:bookmark, user: user) } + let(:bookmark3) { Fabricate(:bookmark, user: user) } let!(:bookmarks) do [ bookmark1, @@ -34,13 +35,14 @@ RSpec.describe Jobs::BookmarkReminderNotifications do end it "will not send a reminder for a bookmark in the future" do + freeze_time bookmark4 = Fabricate(:bookmark, reminder_at: Time.zone.now + 1.day) - BookmarkReminderNotificationHandler.expects(:send_notification).with(bookmark1) - BookmarkReminderNotificationHandler.expects(:send_notification).with(bookmark2) - BookmarkReminderNotificationHandler.expects(:send_notification).with(bookmark3) - BookmarkReminderNotificationHandler.expects(:send_notification).with(bookmark4).never - subject.execute + expect { subject.execute }.to change { Notification.where(user: user).count }.by(3) + expect(bookmark1.reload.reminder_last_sent_at).to eq_time(Time.zone.now) + expect(bookmark2.reload.reminder_last_sent_at).to eq_time(Time.zone.now) + expect(bookmark3.reload.reminder_last_sent_at).to eq_time(Time.zone.now) expect(bookmark4.reload.reminder_at).not_to eq(nil) + expect(bookmark4.reload.reminder_last_sent_at).to eq(nil) end context "when a user is over the bookmark limit" do diff --git a/spec/jobs/export_user_archive_spec.rb b/spec/jobs/export_user_archive_spec.rb index b9e8514d73c..13c582d348a 100644 --- a/spec/jobs/export_user_archive_spec.rb +++ b/spec/jobs/export_user_archive_spec.rb @@ -276,27 +276,27 @@ describe Jobs::ExportUserArchive do let(:post1) { Fabricate(:post, topic: topic1, post_number: 5) } let(:post2) { Fabricate(:post) } let(:post3) { Fabricate(:post) } - let(:message) { Fabricate(:private_message_topic) } - let(:post4) { Fabricate(:post, topic: message) } + let(:private_message_topic) { Fabricate(:private_message_topic) } + let(:post4) { Fabricate(:post, topic: private_message_topic) } let(:reminder_at) { 1.day.from_now } it 'properly includes bookmark records' do now = freeze_time '2017-03-01 12:00' - bkmk1 = manager.create(post_id: post1.id, name: name) + bookmark1 = manager.create(post_id: post1.id, name: name) update1_at = now + 1.hours - bkmk1.update(name: 'great food recipe', updated_at: update1_at) + bookmark1.update(name: 'great food recipe', updated_at: update1_at) manager.create(post_id: post2.id, name: name, reminder_at: reminder_at, options: { auto_delete_preference: Bookmark.auto_delete_preferences[:when_reminder_sent] }) twelve_hr_ago = freeze_time now - 12.hours pending_reminder = manager.create(post_id: post3.id, name: name, reminder_at: now - 8.hours) freeze_time now - tau_record = message.topic_allowed_users.create!(user_id: user.id) + tau_record = private_message_topic.topic_allowed_users.create!(user_id: user.id) manager.create(post_id: post4.id, name: name) tau_record.destroy! - BookmarkReminderNotificationHandler.send_notification(pending_reminder) + BookmarkReminderNotificationHandler.new(pending_reminder).send_notification data, _csv_out = make_component_csv @@ -316,10 +316,56 @@ describe Jobs::ExportUserArchive do expect(DateTime.parse(data[2]['reminder_last_sent_at'])).to eq(DateTime.parse(now.to_s)) expect(data[2]['reminder_set_at']).to eq('') - expect(data[3]['topic_id']).to eq(message.id.to_s) + expect(data[3]['topic_id']).to eq(private_message_topic.id.to_s) expect(data[3]['link']).to eq('') end + context "for polymorphic bookmarks" do + let(:component) { 'bookmarks_polymorphic' } + before do + SiteSetting.use_polymorphic_bookmarks = true + end + + it "properly includes bookmark records" do + now = freeze_time '2017-03-01 12:00' + + bookmark1 = manager.create_for(bookmarkable_id: post1.id, bookmarkable_type: "Post", name: name) + update1_at = now + 1.hours + bookmark1.update(name: 'great food recipe', updated_at: update1_at) + + manager.create_for(bookmarkable_id: post2.id, bookmarkable_type: "Post", name: name, reminder_at: reminder_at, options: { auto_delete_preference: Bookmark.auto_delete_preferences[:when_reminder_sent] }) + twelve_hr_ago = freeze_time now - 12.hours + pending_reminder = manager.create_for(bookmarkable_id: post3.id, bookmarkable_type: "Post", name: name, reminder_at: now - 8.hours) + freeze_time now + + tau_record = private_message_topic.topic_allowed_users.create!(user_id: user.id) + manager.create_for(bookmarkable_id: post4.id, bookmarkable_type: "Post", name: name) + tau_record.destroy! + + BookmarkReminderNotificationHandler.new(pending_reminder).send_notification + + data, _csv_out = make_component_csv + + expect(data.length).to eq(4) + + expect(data[0]['bookmarkable_id']).to eq(post1.id.to_s) + expect(data[0]['bookmarkable_type']).to eq("Post") + expect(data[0]['link']).to eq(post1.full_url) + expect(DateTime.parse(data[0]['updated_at'])).to eq(DateTime.parse(update1_at.to_s)) + + expect(data[1]['name']).to eq(name) + expect(DateTime.parse(data[1]['reminder_at'])).to eq(DateTime.parse(reminder_at.to_s)) + expect(data[1]['auto_delete_preference']).to eq('when_reminder_sent') + + expect(DateTime.parse(data[2]['created_at'])).to eq(DateTime.parse(twelve_hr_ago.to_s)) + expect(DateTime.parse(data[2]['reminder_last_sent_at'])).to eq(DateTime.parse(now.to_s)) + expect(data[2]['reminder_set_at']).to eq('') + + expect(data[3]['bookmarkable_id']).to eq(post4.id.to_s) + expect(data[3]['bookmarkable_type']).to eq("Post") + expect(data[3]['link']).to eq('') + end + end end context 'category_preferences' do diff --git a/spec/jobs/sync_topic_user_bookmarked_spec.rb b/spec/jobs/sync_topic_user_bookmarked_spec.rb index d4661e8dcc4..3b8b52a1ffd 100644 --- a/spec/jobs/sync_topic_user_bookmarked_spec.rb +++ b/spec/jobs/sync_topic_user_bookmarked_spec.rb @@ -1,18 +1,18 @@ # frozen_string_literal: true RSpec.describe Jobs::SyncTopicUserBookmarked do + fab!(:topic) { Fabricate(:topic) } + fab!(:post1) { Fabricate(:post, topic: topic) } + fab!(:post2) { Fabricate(:post, topic: topic) } + fab!(:post3) { Fabricate(:post, topic: topic) } + + fab!(:tu1) { Fabricate(:topic_user, topic: topic, bookmarked: false) } + fab!(:tu2) { Fabricate(:topic_user, topic: topic, bookmarked: false) } + fab!(:tu3) { Fabricate(:topic_user, topic: topic, bookmarked: true) } + fab!(:tu4) { Fabricate(:topic_user, topic: topic, bookmarked: true) } + fab!(:tu5) { Fabricate(:topic_user, topic: topic, bookmarked: true) } + it "corrects all topic_users.bookmarked records for the topic" do - topic = Fabricate(:topic) - Fabricate(:post, topic: topic) - Fabricate(:post, topic: topic) - Fabricate(:post, topic: topic) - - tu1 = Fabricate(:topic_user, topic: topic, bookmarked: false) - tu2 = Fabricate(:topic_user, topic: topic, bookmarked: false) - tu3 = Fabricate(:topic_user, topic: topic, bookmarked: true) - tu4 = Fabricate(:topic_user, topic: topic, bookmarked: true) - tu5 = Fabricate(:topic_user, bookmarked: false) - Fabricate(:bookmark, user: tu1.user, post: topic.posts.sample) Fabricate(:bookmark, user: tu4.user, post: topic.posts.sample) @@ -26,12 +26,6 @@ RSpec.describe Jobs::SyncTopicUserBookmarked do end it "does not consider topic as bookmarked if the bookmarked post is deleted" do - topic = Fabricate(:topic) - post1 = Fabricate(:post, topic: topic) - - tu1 = Fabricate(:topic_user, topic: topic, bookmarked: false) - tu2 = Fabricate(:topic_user, topic: topic, bookmarked: true) - Fabricate(:bookmark, user: tu1.user, post: post1) Fabricate(:bookmark, user: tu2.user, post: post1) @@ -44,17 +38,6 @@ RSpec.describe Jobs::SyncTopicUserBookmarked do end it "works when no topic id is provided (runs for all topics)" do - topic = Fabricate(:topic) - Fabricate(:post, topic: topic) - Fabricate(:post, topic: topic) - Fabricate(:post, topic: topic) - - tu1 = Fabricate(:topic_user, topic: topic, bookmarked: false) - tu2 = Fabricate(:topic_user, topic: topic, bookmarked: false) - tu3 = Fabricate(:topic_user, topic: topic, bookmarked: true) - tu4 = Fabricate(:topic_user, topic: topic, bookmarked: true) - tu5 = Fabricate(:topic_user, bookmarked: false) - Fabricate(:bookmark, user: tu1.user, post: topic.posts.sample) Fabricate(:bookmark, user: tu4.user, post: topic.posts.sample) @@ -66,4 +49,48 @@ RSpec.describe Jobs::SyncTopicUserBookmarked do expect(tu4.reload.bookmarked).to eq(true) expect(tu5.reload.bookmarked).to eq(false) end + + context "for polymorphic bookmarks" do + before do + SiteSetting.use_polymorphic_bookmarks = true + end + + it "corrects all topic_users.bookmarked records for the topic" do + Fabricate(:bookmark, user: tu1.user, bookmarkable: topic.posts.sample) + Fabricate(:bookmark, user: tu4.user, bookmarkable: topic.posts.sample) + + subject.execute(topic_id: topic.id) + + expect(tu1.reload.bookmarked).to eq(true) + expect(tu2.reload.bookmarked).to eq(false) + expect(tu3.reload.bookmarked).to eq(false) + expect(tu4.reload.bookmarked).to eq(true) + expect(tu5.reload.bookmarked).to eq(false) + end + + it "does not consider topic as bookmarked if the bookmarked post is deleted" do + Fabricate(:bookmark, user: tu1.user, bookmarkable: post1) + Fabricate(:bookmark, user: tu2.user, bookmarkable: post1) + + post1.trash! + + subject.execute(topic_id: topic.id) + + expect(tu1.reload.bookmarked).to eq(false) + expect(tu2.reload.bookmarked).to eq(false) + end + + it "works when no topic id is provided (runs for all topics)" do + Fabricate(:bookmark, user: tu1.user, bookmarkable: topic.posts.sample) + Fabricate(:bookmark, user: tu4.user, bookmarkable: topic.posts.sample) + + subject.execute + + expect(tu1.reload.bookmarked).to eq(true) + expect(tu2.reload.bookmarked).to eq(false) + expect(tu3.reload.bookmarked).to eq(false) + expect(tu4.reload.bookmarked).to eq(true) + expect(tu5.reload.bookmarked).to eq(false) + end + end end diff --git a/spec/lib/bookmark_query_spec.rb b/spec/lib/bookmark_query_spec.rb index a3563dd53f3..636fdf8ed21 100644 --- a/spec/lib/bookmark_query_spec.rb +++ b/spec/lib/bookmark_query_spec.rb @@ -12,21 +12,6 @@ RSpec.describe BookmarkQuery do BookmarkQuery.new(user: user || self.user, params: params || self.params) end - def register_user_bookmarkable - Bookmark.register_bookmarkable( - model: User, - serializer: UserBookmarkSerializer, - list_query: lambda do |user, guardian| - user.bookmarks.joins( - "INNER JOIN users ON users.id = bookmarks.bookmarkable_id AND bookmarks.bookmarkable_type = 'User'" - ).where(bookmarkable_type: "User") - end, - search_query: lambda do |bookmarks, query, ts_query| - bookmarks.where("users.username ILIKE ?", query) - end - ) - end - describe "#list_all" do fab!(:bookmark1) { Fabricate(:bookmark, user: user) } fab!(:bookmark2) { Fabricate(:bookmark, user: user) } @@ -69,7 +54,7 @@ RSpec.describe BookmarkQuery do before do SiteSetting.use_polymorphic_bookmarks = true Bookmark.reset_bookmarkables - register_user_bookmarkable + register_test_bookmarkable Fabricate(:topic_user, user: user, topic: post_bookmark.bookmarkable.topic) Fabricate(:topic_user, user: user, topic: topic_bookmark.bookmarkable) @@ -153,7 +138,7 @@ RSpec.describe BookmarkQuery do context "with custom bookmarkable fitering" do before do - register_user_bookmarkable + register_test_bookmarkable end let!(:bookmark5) { Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:user, username: "bookmarkqueen")) } diff --git a/spec/lib/bookmark_reminder_notification_handler_spec.rb b/spec/lib/bookmark_reminder_notification_handler_spec.rb index aa26c4d6bd3..8665c6d934a 100644 --- a/spec/lib/bookmark_reminder_notification_handler_spec.rb +++ b/spec/lib/bookmark_reminder_notification_handler_spec.rb @@ -10,12 +10,12 @@ RSpec.describe BookmarkReminderNotificationHandler do end describe "#send_notification" do - fab!(:bookmark) do + let!(:bookmark) do Fabricate(:bookmark_next_business_day_reminder, user: user) end it "creates a bookmark reminder notification with the correct details" do - subject.send_notification(bookmark) + subject.new(bookmark).send_notification notif = bookmark.user.notifications.last expect(notif.notification_type).to eq(Notification.types[:bookmark_reminder]) expect(notif.topic_id).to eq(bookmark.topic_id) @@ -33,7 +33,7 @@ RSpec.describe BookmarkReminderNotificationHandler do end it "does not send a notification and updates last notification attempt time" do - expect { subject.send_notification(bookmark) }.not_to change { Notification.count } + expect { subject.new(bookmark).send_notification }.not_to change { Notification.count } expect(bookmark.reload.reminder_last_sent_at).not_to be_blank end end @@ -45,11 +45,46 @@ RSpec.describe BookmarkReminderNotificationHandler do end it "does not send a notification and updates last notification attempt time" do - expect { subject.send_notification(bookmark) }.not_to change { Notification.count } + expect { subject.new(bookmark).send_notification }.not_to change { Notification.count } expect(bookmark.reload.reminder_last_sent_at).not_to be_blank end end + context "using polymorphic bookmarks" do + before do + SiteSetting.use_polymorphic_bookmarks = true + end + + let!(:bookmark) do + Fabricate(:bookmark_next_business_day_reminder, user: user, bookmarkable: Fabricate(:post)) + end + + it "creates a bookmark reminder notification with the correct details" do + subject.new(bookmark).send_notification + notif = bookmark.user.notifications.last + expect(notif.notification_type).to eq(Notification.types[:bookmark_reminder]) + expect(notif.topic_id).to eq(bookmark.bookmarkable.topic_id) + expect(notif.post_number).to eq(bookmark.bookmarkable.post_number) + data = JSON.parse(notif.data) + expect(data["title"]).to eq(bookmark.bookmarkable.topic.title) + expect(data["display_username"]).to eq(bookmark.user.username) + expect(data["bookmark_name"]).to eq(bookmark.name) + expect(data["bookmarkable_url"]).to eq(bookmark.bookmarkable.url) + end + + context "when the bookmarkable is deleted" do + before do + bookmark.bookmarkable.trash! + bookmark.reload + end + + it "does not send a notification and updates last notification attempt time" do + expect { subject.new(bookmark).send_notification }.not_to change { Notification.count } + expect(bookmark.reload.reminder_last_sent_at).not_to be_blank + end + end + end + context "when the auto_delete_preference is when_reminder_sent" do before do TopicUser.create!(topic: bookmark.topic, user: user, bookmarked: true) @@ -57,12 +92,12 @@ RSpec.describe BookmarkReminderNotificationHandler do end it "deletes the bookmark after the reminder gets sent" do - subject.send_notification(bookmark) + subject.new(bookmark).send_notification expect(Bookmark.find_by(id: bookmark.id)).to eq(nil) end it "changes the TopicUser bookmarked column to false" do - subject.send_notification(bookmark) + subject.new(bookmark).send_notification expect(TopicUser.find_by(topic: bookmark.topic, user: user).bookmarked).to eq(false) end @@ -72,7 +107,7 @@ RSpec.describe BookmarkReminderNotificationHandler do end it "does not change the TopicUser bookmarked column to false" do - subject.send_notification(bookmark) + subject.new(bookmark).send_notification expect(TopicUser.find_by(topic: bookmark.topic, user: user).bookmarked).to eq(true) end end @@ -85,7 +120,7 @@ RSpec.describe BookmarkReminderNotificationHandler do end it "resets reminder_at after the reminder gets sent" do - subject.send_notification(bookmark) + subject.new(bookmark).send_notification expect(Bookmark.find_by(id: bookmark.id).reminder_at).to eq(nil) end end @@ -94,7 +129,7 @@ RSpec.describe BookmarkReminderNotificationHandler do it "does not send a notification" do bookmark.post.trash! bookmark.reload - expect { subject.send_notification(bookmark) }.not_to change { Notification.count } + expect { subject.new(bookmark).send_notification }.not_to change { Notification.count } expect(bookmark.reload.reminder_last_sent_at).not_to be_blank end end diff --git a/spec/models/bookmark_spec.rb b/spec/models/bookmark_spec.rb index 0addabef238..3dbcfea26ee 100644 --- a/spec/models/bookmark_spec.rb +++ b/spec/models/bookmark_spec.rb @@ -68,18 +68,7 @@ describe Bookmark do user = Fabricate(:user) bm = Bookmark.create(bookmarkable_type: "User", bookmarkable: Fabricate(:user), user: user) expect(bm.errors.full_messages).to include(I18n.t("bookmarks.errors.invalid_bookmarkable", type: "User")) - Bookmark.register_bookmarkable( - 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 - ) + register_test_bookmarkable expect(bm.valid?).to eq(true) end end diff --git a/spec/models/user_bookmark_list_spec.rb b/spec/models/user_bookmark_list_spec.rb index 98f3ddf3fad..1bd6c04c7bd 100644 --- a/spec/models/user_bookmark_list_spec.rb +++ b/spec/models/user_bookmark_list_spec.rb @@ -29,18 +29,7 @@ RSpec.describe UserBookmarkList do context "for polymorphic bookmarks" do before do SiteSetting.use_polymorphic_bookmarks = true - Bookmark.register_bookmarkable( - 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 - ) + register_test_bookmarkable Fabricate(:topic_user, user: user, topic: post_bookmark.bookmarkable.topic) Fabricate(:topic_user, user: user, topic: topic_bookmark.bookmarkable) diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index 37efe2d9bce..244f5fe5494 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -5352,18 +5352,7 @@ describe UsersController 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 - ) + register_test_bookmarkable 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) diff --git a/spec/script/import_scripts/base_spec.rb b/spec/script/import_scripts/base_spec.rb index 71dbd1fa86d..4b55ecb9d99 100644 --- a/spec/script/import_scripts/base_spec.rb +++ b/spec/script/import_scripts/base_spec.rb @@ -55,6 +55,20 @@ describe ImportScripts::Base do expect(SiteSetting.purge_unactivated_users_grace_period_days).to eq(60) end + context "when polymorphic bookmarks are enabled" do + before do + SiteSetting.use_polymorphic_bookmarks = true + end + + it "creates bookmarks, posts, and users" do + MockSpecImporter.new(import_data).perform + expect(Bookmark.where(bookmarkable_type: "Post").count).to eq(5) + expect(Post.count).to eq(5) + expect(User.where('id > 0').count).to eq(1) + expect(SiteSetting.purge_unactivated_users_grace_period_days).to eq(60) + end + end + it "does not change purge unactivated users setting if disabled" do SiteSetting.purge_unactivated_users_grace_period_days = 0 MockSpecImporter.new(import_data).perform diff --git a/spec/serializers/user_bookmark_list_serializer_spec.rb b/spec/serializers/user_bookmark_list_serializer_spec.rb index 8d32f2c77bc..d7388786b07 100644 --- a/spec/serializers/user_bookmark_list_serializer_spec.rb +++ b/spec/serializers/user_bookmark_list_serializer_spec.rb @@ -1,26 +1,13 @@ # frozen_string_literal: true RSpec.describe UserBookmarkListSerializer do - class UserTestBookmarkSerializer < UserBookmarkBaseSerializer; end fab!(:user) { Fabricate(:user) } context "for polymorphic bookmarks" do before do SiteSetting.use_polymorphic_bookmarks = true - Bookmark.register_bookmarkable( - 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 - ) - + register_test_bookmarkable Fabricate(:topic_user, user: user, topic: post_bookmark.bookmarkable.topic) Fabricate(:topic_user, user: user, topic: topic_bookmark.bookmarkable) user_bookmark diff --git a/spec/services/post_bookmarkable_spec.rb b/spec/services/post_bookmarkable_spec.rb new file mode 100644 index 00000000000..076f3667bb1 --- /dev/null +++ b/spec/services/post_bookmarkable_spec.rb @@ -0,0 +1,106 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe PostBookmarkable do + fab!(:user) { Fabricate(:user) } + fab!(:guardian) { Guardian.new(user) } + fab!(:private_category) { Fabricate(:private_category, group: Fabricate(:group)) } + + before do + SiteSetting.use_polymorphic_bookmarks = true + end + + let!(:post1) { Fabricate(:post) } + let!(:post2) { Fabricate(:post) } + let!(:bookmark1) { Fabricate(:bookmark, user: user, bookmarkable: post1, name: "something i gotta do") } + let!(:bookmark2) { Fabricate(:bookmark, user: user, bookmarkable: post2) } + let!(:bookmark3) { Fabricate(:bookmark) } + let!(:topic_user1) { Fabricate(:topic_user, user: user, topic: post1.topic) } + let!(:topic_user2) { Fabricate(:topic_user, user: user, topic: post2.topic) } + + subject { RegisteredBookmarkable.new(PostBookmarkable) } + + describe "#perform_list_query" do + it "returns all the user's bookmarks" do + expect(subject.perform_list_query(user, guardian).map(&:id)).to match_array([bookmark1.id, bookmark2.id]) + end + + it "does not return bookmarks for posts where the user does not have access to the topic category" do + bookmark1.bookmarkable.topic.update(category: private_category) + expect(subject.perform_list_query(user, guardian).map(&:id)).to match_array([bookmark2.id]) + end + + it "does not return bookmarks for posts where the user does not have access to the private message" do + bookmark1.bookmarkable.update(topic: Fabricate(:private_message_topic)) + expect(subject.perform_list_query(user, guardian).map(&:id)).to match_array([bookmark2.id]) + end + end + + describe "#perform_search_query" do + before do + SearchIndexer.enable + end + + it "returns bookmarks that match by name" do + ts_query = Search.ts_query(term: "gotta", ts_config: "simple") + expect(subject.perform_search_query(subject.perform_list_query(user, guardian), "%gotta%", ts_query).map(&:id)).to match_array([bookmark1.id]) + end + + it "returns bookmarks that match by post search data (topic title or post content)" do + post2.update(raw: "some post content") + post2.topic.update(title: "a great topic title") + + ts_query = Search.ts_query(term: "post content", ts_config: "simple") + expect(subject.perform_search_query(subject.perform_list_query(user, guardian), "%post content%", ts_query).map(&:id)).to match_array([bookmark2.id]) + + ts_query = Search.ts_query(term: "great topic", ts_config: "simple") + expect(subject.perform_search_query(subject.perform_list_query(user, guardian), "%great topic%", ts_query).map(&:id)).to match_array([bookmark2.id]) + + ts_query = Search.ts_query(term: "blah", ts_config: "simple") + expect(subject.perform_search_query(subject.perform_list_query(user, guardian), "%blah%", ts_query).map(&:id)).to eq([]) + end + end + + describe "#can_send_reminder?" do + it "cannot send reminder if the post or topic is deleted" do + expect(subject.can_send_reminder?(bookmark1)).to eq(true) + bookmark1.bookmarkable.trash! + bookmark1.reload + expect(subject.can_send_reminder?(bookmark1)).to eq(false) + Post.with_deleted.find_by(id: bookmark1.bookmarkable_id).recover! + bookmark1.reload + bookmark1.bookmarkable.topic.trash! + bookmark1.reload + expect(subject.can_send_reminder?(bookmark1)).to eq(false) + end + end + + describe "#reminder_handler" do + it "creates a notification for the user with the correct details" do + expect { subject.send_reminder_notification(bookmark1) }.to change { Notification.count }.by(1) + notif = user.notifications.last + expect(notif.notification_type).to eq(Notification.types[:bookmark_reminder]) + expect(notif.topic_id).to eq(bookmark1.bookmarkable.topic_id) + expect(notif.post_number).to eq(bookmark1.bookmarkable.post_number) + expect(notif.data).to eq( + { + title: bookmark1.bookmarkable.topic.title, + display_username: bookmark1.user.username, + bookmark_name: bookmark1.name, + bookmarkable_url: bookmark1.bookmarkable.url + }.to_json + ) + end + end + + describe "#can_see?" do + it "returns false if the post is in a private category or private message the user cannot see" do + expect(subject.can_see?(guardian, bookmark1)).to eq(true) + bookmark1.bookmarkable.topic.update(category: private_category) + expect(subject.can_see?(guardian, bookmark1)).to eq(false) + bookmark1.bookmarkable.update(topic: Fabricate(:private_message_topic)) + expect(subject.can_see?(guardian, bookmark1)).to eq(false) + end + end +end diff --git a/spec/services/topic_bookmarkable_spec.rb b/spec/services/topic_bookmarkable_spec.rb new file mode 100644 index 00000000000..68128a5726b --- /dev/null +++ b/spec/services/topic_bookmarkable_spec.rb @@ -0,0 +1,102 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe TopicBookmarkable do + fab!(:user) { Fabricate(:user) } + fab!(:guardian) { Guardian.new(user) } + fab!(:private_category) { Fabricate(:private_category, group: Fabricate(:group)) } + + before do + SiteSetting.use_polymorphic_bookmarks = true + end + + let!(:topic1) { Fabricate(:topic) } + let!(:topic2) { Fabricate(:topic) } + let!(:post) { Fabricate(:post, topic: topic1) } + let!(:bookmark1) { Fabricate(:bookmark, user: user, bookmarkable: topic1, name: "something i gotta do") } + let!(:bookmark2) { Fabricate(:bookmark, user: user, bookmarkable: topic2) } + let!(:bookmark3) { Fabricate(:bookmark) } + let!(:topic_user1) { Fabricate(:topic_user, user: user, topic: topic1) } + let!(:topic_user2) { Fabricate(:topic_user, user: user, topic: topic2) } + + subject { RegisteredBookmarkable.new(TopicBookmarkable) } + + describe "#perform_list_query" do + it "returns all the user's bookmarks" do + expect(subject.perform_list_query(user, guardian).map(&:id)).to match_array([bookmark1.id, bookmark2.id]) + end + + it "does not return bookmarks for posts where the user does not have access to the topic category" do + bookmark1.bookmarkable.update!(category: private_category) + expect(subject.perform_list_query(user, guardian).map(&:id)).to match_array([bookmark2.id]) + end + + it "does not return bookmarks for posts where the user does not have access to the private message" do + bookmark1.update!(bookmarkable: Fabricate(:private_message_topic)) + expect(subject.perform_list_query(user, guardian).map(&:id)).to match_array([bookmark2.id]) + end + end + + describe "#perform_search_query" do + before do + SearchIndexer.enable + end + + it "returns bookmarks that match by name" do + ts_query = Search.ts_query(term: "gotta", ts_config: "simple") + expect(subject.perform_search_query(subject.perform_list_query(user, guardian), "%gotta%", ts_query).map(&:id)).to match_array([bookmark1.id]) + end + + it "returns bookmarks that match by post search data (topic title or post content)" do + post.update(raw: "some post content") + topic1.update(title: "a great topic title") + + ts_query = Search.ts_query(term: "post content", ts_config: "simple") + expect(subject.perform_search_query(subject.perform_list_query(user, guardian), "%post content%", ts_query).map(&:id)).to match_array([bookmark1.id]) + + ts_query = Search.ts_query(term: "great topic", ts_config: "simple") + expect(subject.perform_search_query(subject.perform_list_query(user, guardian), "%great topic%", ts_query).map(&:id)).to match_array([bookmark1.id]) + + ts_query = Search.ts_query(term: "blah", ts_config: "simple") + expect(subject.perform_search_query(subject.perform_list_query(user, guardian), "%blah%", ts_query).map(&:id)).to eq([]) + end + end + + describe "#can_send_reminder?" do + it "cannot send reminder if the topic is deleted" do + expect(subject.can_send_reminder?(bookmark1)).to eq(true) + bookmark1.bookmarkable.trash! + bookmark1.reload + expect(subject.can_send_reminder?(bookmark1)).to eq(false) + end + end + + describe "#reminder_handler" do + it "creates a notification for the user with the correct details" do + expect { subject.send_reminder_notification(bookmark1) }.to change { Notification.count }.by(1) + notif = user.notifications.last + expect(notif.notification_type).to eq(Notification.types[:bookmark_reminder]) + expect(notif.topic_id).to eq(bookmark1.bookmarkable_id) + expect(notif.post_number).to eq(1) + expect(notif.data).to eq( + { + title: bookmark1.bookmarkable.title, + display_username: bookmark1.user.username, + bookmark_name: bookmark1.name, + bookmarkable_url: bookmark1.bookmarkable.first_post.url + }.to_json + ) + end + end + + describe "#can_see?" do + it "returns false if the post is in a private category or private message the user cannot see" do + expect(subject.can_see?(guardian, bookmark1)).to eq(true) + bookmark1.bookmarkable.update!(category: private_category) + expect(subject.can_see?(guardian, bookmark1)).to eq(false) + bookmark1.update!(bookmarkable: Fabricate(:private_message_topic)) + expect(subject.can_see?(guardian, bookmark1)).to eq(false) + end + end +end diff --git a/spec/support/bookmarkable_helper.rb b/spec/support/bookmarkable_helper.rb new file mode 100644 index 00000000000..9f111b36d56 --- /dev/null +++ b/spec/support/bookmarkable_helper.rb @@ -0,0 +1,42 @@ +# frozen_string_literal: true + +class UserTestBookmarkSerializer < UserBookmarkBaseSerializer; end +class UserTestBookmarkable < BaseBookmarkable + def self.model + User + end + + def self.serializer + UserTestBookmarkSerializer + end + + def self.preload_associations + [:topic_users, :tags, { posts: :user }] + end + + def self.list_query(user, guardian) + user.bookmarks.joins( + "INNER JOIN users ON users.id = bookmarks.bookmarkable_id AND bookmarks.bookmarkable_type = 'User'" + ).where(bookmarkable_type: "User") + end + + def self.search_query(bookmarks, query, ts_query, &bookmarkable_search) + bookmarks.where("users.username ILIKE ?", query) + end + + def self.reminder_handler(bookmark) + # noop + end + + def self.reminder_conditions(bookmark) + bookmark.bookmarkable.present? + end + + def self.can_see?(guardian, bookmark) + true + end +end + +def register_test_bookmarkable + Bookmark.register_bookmarkable(UserTestBookmarkable) +end