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