diff --git a/app/assets/javascripts/discourse/app/components/bookmark-list.js b/app/assets/javascripts/discourse/app/components/bookmark-list.js index 066a9019cde..9559c87a7e2 100644 --- a/app/assets/javascripts/discourse/app/components/bookmark-list.js +++ b/app/assets/javascripts/discourse/app/components/bookmark-list.js @@ -93,19 +93,23 @@ export default Component.extend(Scrolling, { @action editBookmark(bookmark) { - openBookmarkModal(bookmark, { - onAfterSave: (savedData) => { - this.appEvents.trigger( - "bookmarks:changed", - savedData, - bookmark.attachedTo() - ); - this.reload(); + openBookmarkModal( + bookmark, + { + onAfterSave: (savedData) => { + this.appEvents.trigger( + "bookmarks:changed", + savedData, + bookmark.attachedTo() + ); + this.reload(); + }, + onAfterDelete: () => { + this.reload(); + }, }, - onAfterDelete: () => { - this.reload(); - }, - }); + { use_polymorphic_bookmarks: this.siteSettings.use_polymorphic_bookmarks } + ); }, @action diff --git a/app/assets/javascripts/discourse/app/components/bookmark.js b/app/assets/javascripts/discourse/app/components/bookmark.js index 8aa1dafb80d..354b015b4af 100644 --- a/app/assets/javascripts/discourse/app/components/bookmark.js +++ b/app/assets/javascripts/discourse/app/components/bookmark.js @@ -149,12 +149,19 @@ export default Component.extend({ const data = { reminder_at: reminderAtISO, name: this.model.name, - post_id: this.model.postId, id: this.model.id, auto_delete_preference: this.autoDeletePreference, - for_topic: this.model.forTopic, }; + if (this.siteSettings.use_polymorphic_bookmarks) { + data.bookmarkable_id = this.model.bookmarkableId; + data.bookmarkable_type = this.model.bookmarkableType; + } else { + // TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented. + data.post_id = this.model.postId; + data.for_topic = this.model.forTopic; + } + if (this.editingExistingBookmark) { return ajax(`/bookmarks/${this.model.id}`, { type: "PUT", @@ -173,15 +180,25 @@ export default Component.extend({ if (!this.afterSave) { return; } - this.afterSave({ + + const data = { reminder_at: reminderAtISO, - for_topic: this.model.forTopic, auto_delete_preference: this.autoDeletePreference, - post_id: this.model.postId, id: this.model.id || response.id, name: this.model.name, - topic_id: this.model.topicId, - }); + }; + + if (this.siteSettings.use_polymorphic_bookmarks) { + data.bookmarkable_id = this.model.bookmarkableId; + data.bookmarkable_type = this.model.bookmarkableType; + } else { + // TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented. + data.post_id = this.model.postId; + data.for_topic = this.model.forTopic; + data.topic_id = this.model.topicId; + } + + this.afterSave(data); }, _deleteBookmark() { diff --git a/app/assets/javascripts/discourse/app/controllers/bookmark.js b/app/assets/javascripts/discourse/app/controllers/bookmark.js index 6fce0c15ea5..0d906bbf907 100644 --- a/app/assets/javascripts/discourse/app/controllers/bookmark.js +++ b/app/assets/javascripts/discourse/app/controllers/bookmark.js @@ -1,4 +1,5 @@ import Controller from "@ember/controller"; +import I18n from "I18n"; import ModalFunctionality from "discourse/mixins/modal-functionality"; import { action } from "@ember/object"; import { Promise } from "rsvp"; @@ -10,28 +11,53 @@ export function openBookmarkModal( onCloseWithoutSaving: null, onAfterSave: null, onAfterDelete: null, + }, + options = { + use_polymorphic_bookmarks: false, } ) { return new Promise((resolve) => { const modalTitle = () => { - if (bookmark.for_topic) { - return bookmark.id - ? "post.bookmarks.edit_for_topic" - : "post.bookmarks.create_for_topic"; + if (options.use_polymorphic_bookmarks) { + return I18n.t( + bookmark.id ? "bookmarks.edit_for" : "bookmarks.create_for", + { + type: bookmark.bookmarkable_type, + } + ); + } else if (bookmark.for_topic) { + return I18n.t( + bookmark.id + ? "post.bookmarks.edit_for_topic" + : "post.bookmarks.create_for_topic" + ); + } else { + return I18n.t( + bookmark.id ? "post.bookmarks.edit" : "post.bookmarks.create" + ); } - return bookmark.id ? "post.bookmarks.edit" : "post.bookmarks.create"; }; + + const model = { + id: bookmark.id, + reminderAt: bookmark.reminder_at, + autoDeletePreference: bookmark.auto_delete_preference, + name: bookmark.name, + }; + + if (options.use_polymorphic_bookmarks) { + model.bookmarkableId = bookmark.bookmarkable_id; + model.bookmarkableType = bookmark.bookmarkable_type; + } else { + // TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented. + model.postId = bookmark.post_id; + model.topicId = bookmark.topic_id; + model.forTopic = bookmark.for_topic; + } + let modalController = showModal("bookmark", { - model: { - postId: bookmark.post_id, - topicId: bookmark.topic_id, - id: bookmark.id, - reminderAt: bookmark.reminder_at, - autoDeletePreference: bookmark.auto_delete_preference, - name: bookmark.name, - forTopic: bookmark.for_topic, - }, - title: modalTitle(), + model, + titleTranslated: modalTitle(), modalClass: "bookmark-with-reminder", }); modalController.setProperties({ diff --git a/app/assets/javascripts/discourse/app/controllers/topic.js b/app/assets/javascripts/discourse/app/controllers/topic.js index 1398450d050..9e0b1137745 100644 --- a/app/assets/javascripts/discourse/app/controllers/topic.js +++ b/app/assets/javascripts/discourse/app/controllers/topic.js @@ -754,6 +754,7 @@ export default Controller.extend(bufferedProperty("model"), { } }, + // TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented. toggleBookmark(post) { if (!this.currentUser) { return bootbox.alert(I18n.t("bookmarks.not_bookmarked")); @@ -784,6 +785,39 @@ export default Controller.extend(bufferedProperty("model"), { } }, + toggleBookmarkPolymorphic(post) { + if (!this.currentUser) { + return bootbox.alert(I18n.t("bookmarks.not_bookmarked")); + } else if (post) { + const bookmarkForPost = this.model.bookmarks.find( + (bookmark) => + bookmark.bookmarkable_id === post.id && + bookmark.bookmarkable_type === "Post" + ); + return this._modifyPostBookmark( + bookmarkForPost || + Bookmark.create({ + bookmarkable_id: post.id, + bookmarkable_type: "Post", + auto_delete_preference: this.currentUser + .bookmark_auto_delete_preference, + }), + post + ); + } else { + return this._toggleTopicLevelBookmarkPolymorphic().then( + (changedIds) => { + if (!changedIds) { + return; + } + changedIds.forEach((id) => + this.appEvents.trigger("post-stream:refresh", { id }) + ); + } + ); + } + }, + jumpToIndex(index) { this._jumpToIndex(index); }, @@ -1238,46 +1272,56 @@ export default Controller.extend(bufferedProperty("model"), { }, _modifyTopicBookmark(bookmark) { - return openBookmarkModal(bookmark, { - onAfterSave: (savedData) => { - this._syncBookmarks(savedData); - this.model.set("bookmarking", false); - this.model.set("bookmarked", true); - this.model.incrementProperty("bookmarksWereChanged"); - this.appEvents.trigger( - "bookmarks:changed", - savedData, - bookmark.attachedTo() - ); + return openBookmarkModal( + bookmark, + { + onAfterSave: (savedData) => { + this._syncBookmarks(savedData); + this.model.set("bookmarking", false); + this.model.set("bookmarked", true); + this.model.incrementProperty("bookmarksWereChanged"); + this.appEvents.trigger( + "bookmarks:changed", + savedData, + bookmark.attachedTo() + ); - // TODO (martin) (2022-02-01) Remove these old bookmark events, replaced by bookmarks:changed. - this.appEvents.trigger("topic:bookmark-toggled"); + // TODO (martin) (2022-02-01) Remove these old bookmark events, replaced by bookmarks:changed. + this.appEvents.trigger("topic:bookmark-toggled"); + }, + onAfterDelete: (topicBookmarked, bookmarkId) => { + this.model.removeBookmark(bookmarkId); + }, }, - onAfterDelete: (topicBookmarked, bookmarkId) => { - this.model.removeBookmark(bookmarkId); - }, - }); + { use_polymorphic_bookmarks: this.siteSettings.use_polymorphic_bookmarks } + ); }, _modifyPostBookmark(bookmark, post) { - return openBookmarkModal(bookmark, { - onCloseWithoutSaving: () => { - post.appEvents.trigger("post-stream:refresh", { - id: bookmark.post_id, - }); + return openBookmarkModal( + bookmark, + { + onCloseWithoutSaving: () => { + post.appEvents.trigger("post-stream:refresh", { + id: this.siteSettings.use_polymorphic_bookmarks + ? bookmark.bookmarkable_id + : bookmark.post_id, + }); + }, + onAfterSave: (savedData) => { + this._syncBookmarks(savedData); + this.model.set("bookmarking", false); + post.createBookmark(savedData); + this.model.afterPostBookmarked(post, savedData); + return [post.id]; + }, + onAfterDelete: (topicBookmarked, bookmarkId) => { + this.model.removeBookmark(bookmarkId); + post.deleteBookmark(topicBookmarked); + }, }, - onAfterSave: (savedData) => { - this._syncBookmarks(savedData); - this.model.set("bookmarking", false); - post.createBookmark(savedData); - this.model.afterPostBookmarked(post, savedData); - return [post.id]; - }, - onAfterDelete: (topicBookmarked, bookmarkId) => { - this.model.removeBookmark(bookmarkId); - post.deleteBookmark(topicBookmarked); - }, - }); + { use_polymorphic_bookmarks: this.siteSettings.use_polymorphic_bookmarks } + ); }, _syncBookmarks(data) { @@ -1295,6 +1339,7 @@ export default Controller.extend(bufferedProperty("model"), { } }, + // TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented. async _toggleTopicLevelBookmark() { if (this.model.bookmarking) { return Promise.resolve(); @@ -1329,6 +1374,41 @@ export default Controller.extend(bufferedProperty("model"), { } }, + async _toggleTopicLevelBookmarkPolymorphic() { + if (this.model.bookmarking) { + return Promise.resolve(); + } + + if (this.model.bookmarkCount > 1) { + return this._maybeClearAllBookmarks(); + } + + if (this.model.bookmarkCount === 1) { + const topicBookmark = this.model.bookmarks.findBy( + "bookmarkable_type", + "Topic" + ); + if (topicBookmark) { + return this._modifyTopicBookmark(topicBookmark); + } else { + const bookmark = this.model.bookmarks[0]; + const post = await this.model.postById(bookmark.bookmarkable_id); + return this._modifyPostBookmark(bookmark, post); + } + } + + if (this.model.bookmarkCount === 0) { + return this._modifyTopicBookmark( + Bookmark.create({ + bookmarkable_id: this.model.id, + bookmarkable_type: "Topic", + auto_delete_preference: this.currentUser + .bookmark_auto_delete_preference, + }) + ); + } + }, + _maybeClearAllBookmarks() { return new Promise((resolve) => { bootbox.confirm( diff --git a/app/assets/javascripts/discourse/app/initializers/topic-footer-buttons.js b/app/assets/javascripts/discourse/app/initializers/topic-footer-buttons.js index b8592c13ebb..b1f330c5c73 100644 --- a/app/assets/javascripts/discourse/app/initializers/topic-footer-buttons.js +++ b/app/assets/javascripts/discourse/app/initializers/topic-footer-buttons.js @@ -11,7 +11,8 @@ const DEFER_PRIORITY = 500; export default { name: "topic-footer-buttons", - initialize() { + initialize(container) { + const siteSettings = container.lookup("site-settings:main"); registerTopicFooterButton({ id: "share-and-invite", icon: "d-topic-share", @@ -98,9 +99,14 @@ export default { if (this.topic.bookmarkCount === 0) { return I18n.t("bookmarked.help.bookmark"); } else if (this.topic.bookmarkCount === 1) { - if ( - this.topic.bookmarks.filter((bookmark) => bookmark.for_topic).length - ) { + // TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented. + const anyTopicBookmarks = this.topic.bookmarks.some((bookmark) => { + return siteSettings.use_polymorphic_bookmarks + ? bookmark.for_topic + : bookmark.bookmarkable_type === "Topic"; + }); + + if (anyTopicBookmarks) { return I18n.t("bookmarked.help.edit_bookmark_for_topic"); } else { return I18n.t("bookmarked.help.edit_bookmark"); @@ -113,7 +119,10 @@ export default { return I18n.t("bookmarked.help.unbookmark"); } }, - action: "toggleBookmark", + // TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented. + action: siteSettings.use_polymorphic_bookmarks + ? "toggleBookmarkPolymorphic" + : "toggleBookmark", dropdown() { return this.site.mobileView; }, diff --git a/app/assets/javascripts/discourse/app/models/bookmark.js b/app/assets/javascripts/discourse/app/models/bookmark.js index 8852409a773..f28c92845ee 100644 --- a/app/assets/javascripts/discourse/app/models/bookmark.js +++ b/app/assets/javascripts/discourse/app/models/bookmark.js @@ -38,6 +38,14 @@ const Bookmark = RestModel.extend({ }, attachedTo() { + if (this.siteSettings.use_polymorphic_bookmarks) { + return { + target: this.bookmarkable_type.toLowerCase(), + targetId: this.bookmarkable_id, + }; + } + + // TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented. if (this.for_topic) { return { target: "topic", targetId: this.topic_id }; } diff --git a/app/assets/javascripts/discourse/app/models/topic.js b/app/assets/javascripts/discourse/app/models/topic.js index 6cf798087e2..013467578f3 100644 --- a/app/assets/javascripts/discourse/app/models/topic.js +++ b/app/assets/javascripts/discourse/app/models/topic.js @@ -397,7 +397,15 @@ const Topic = RestModel.extend({ this.set( "bookmarks", this.bookmarks.filter((bookmark) => { - if (bookmark.id === id && bookmark.for_topic) { + // TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented. + if ( + (!this.siteSettings.use_polymorphic_bookmarks && + bookmark.id === id && + bookmark.for_topic) || + (this.siteSettings.use_polymorphic_bookmarks && + bookmark.id === id && + bookmark.bookmarkable_type === "Topic") + ) { // TODO (martin) (2022-02-01) Remove these old bookmark events, replaced by bookmarks:changed. this.appEvents.trigger("topic:bookmark-toggled"); this.appEvents.trigger( @@ -417,7 +425,11 @@ const Topic = RestModel.extend({ clearBookmarks() { this.toggleProperty("bookmarked"); - const postIds = this.bookmarks.mapBy("post_id"); + const postIds = this.siteSettings.use_polymorphic_bookmarks + ? this.bookmarks + .filterBy("bookmarkable_type", "Post") + .mapBy("bookmarkable_id") + : this.bookmarks.mapBy("post_id"); postIds.forEach((postId) => { const loadedPost = this.postStream.findLoadedPost(postId); if (loadedPost) { diff --git a/app/assets/javascripts/discourse/app/templates/topic.hbs b/app/assets/javascripts/discourse/app/templates/topic.hbs index dd268baff07..3b74568e5b8 100644 --- a/app/assets/javascripts/discourse/app/templates/topic.hbs +++ b/app/assets/javascripts/discourse/app/templates/topic.hbs @@ -223,6 +223,7 @@ recoverPost=(action "recoverPost") expandHidden=(action "expandHidden") toggleBookmark=(action "toggleBookmark") + toggleBookmarkPolymorphic=(action "toggleBookmarkPolymorphic") togglePostType=(action "togglePostType") rebakePost=(action "rebakePost") changePostOwner=(action "changePostOwner") @@ -362,6 +363,7 @@ convertToPublicTopic=(action "convertToPublicTopic") convertToPrivateMessage=(action "convertToPrivateMessage") toggleBookmark=(action "toggleBookmark") + toggleBookmarkPolymorphic=(action "toggleBookmarkPolymorphic") showFlagTopic=(route-action "showFlagTopic") toggleArchiveMessage=(action "toggleArchiveMessage") editFirstPost=(action "editFirstPost") diff --git a/app/assets/javascripts/discourse/app/widgets/post-menu.js b/app/assets/javascripts/discourse/app/widgets/post-menu.js index 08a5e212a60..a9187ff9f50 100644 --- a/app/assets/javascripts/discourse/app/widgets/post-menu.js +++ b/app/assets/javascripts/discourse/app/widgets/post-menu.js @@ -302,7 +302,7 @@ registerButton("reply", (attrs, state, siteSettings, postMenuSettings) => { registerButton( "bookmark", - (attrs, _state, _siteSettings, _settings, currentUser) => { + (attrs, _state, siteSettings, _settings, currentUser) => { if (!attrs.canBookmark) { return; } @@ -332,7 +332,9 @@ registerButton( return { id: attrs.bookmarked ? "unbookmark" : "bookmark", - action: "toggleBookmark", + action: siteSettings.use_polymorphic_bookmarks + ? "toggleBookmarkPolymorphic" + : "toggleBookmark", title, titleOptions, className: classNames.join(" "), diff --git a/app/controllers/bookmarks_controller.rb b/app/controllers/bookmarks_controller.rb index a71588f24bf..9aabad449b0 100644 --- a/app/controllers/bookmarks_controller.rb +++ b/app/controllers/bookmarks_controller.rb @@ -4,22 +4,42 @@ class BookmarksController < ApplicationController requires_login def create - params.require(:post_id) + if SiteSetting.use_polymorphic_bookmarks + params.require(:bookmarkable_id) + params.require(:bookmarkable_type) + else + params.require(:post_id) + end RateLimiter.new( current_user, "create_bookmark", SiteSetting.max_bookmarks_per_day, 1.day.to_i ).performed! bookmark_manager = BookmarkManager.new(current_user) - bookmark = bookmark_manager.create( - post_id: params[:post_id], + + create_params = { name: params[:name], reminder_at: params[:reminder_at], - for_topic: params[:for_topic] == "true", options: { auto_delete_preference: params[:auto_delete_preference] || 0 } - ) + } + + if SiteSetting.use_polymorphic_bookmarks + bookmark = bookmark_manager.create_for( + **create_params.merge( + bookmarkable_id: params[:bookmarkable_id], + bookmarkable_type: params[:bookmarkable_type] + ) + ) + else + bookmark = bookmark_manager.create( + **create_params.merge( + post_id: params[:post_id], + for_topic: params[:for_topic] == "true", + ) + ) + end if bookmark_manager.errors.empty? return render json: success_json.merge(id: bookmark.id) @@ -30,8 +50,8 @@ class BookmarksController < ApplicationController def destroy params.require(:id) - result = BookmarkManager.new(current_user).destroy(params[:id]) - render json: success_json.merge(result) + destroyed_bookmark = BookmarkManager.new(current_user).destroy(params[:id]) + render json: success_json.merge(BookmarkManager.bookmark_metadata(destroyed_bookmark, current_user)) end def update diff --git a/app/controllers/posts_controller.rb b/app/controllers/posts_controller.rb index be388f7d7ab..956e1d2e366 100644 --- a/app/controllers/posts_controller.rb +++ b/app/controllers/posts_controller.rb @@ -537,9 +537,9 @@ class PostsController < ApplicationController params.require(:post_id) bookmark_id = Bookmark.where(post_id: params[:post_id], user_id: current_user.id).pluck_first(:id) - result = BookmarkManager.new(current_user).destroy(bookmark_id) + destroyed_bookmark = BookmarkManager.new(current_user).destroy(bookmark_id) - render json: success_json.merge(result) + render json: success_json.merge(BookmarkManager.bookmark_metadata(destroyed_bookmark, current_user)) end def wiki diff --git a/app/jobs/regular/sync_topic_user_bookmarked.rb b/app/jobs/regular/sync_topic_user_bookmarked.rb index bd630253e82..8c0621875f3 100644 --- a/app/jobs/regular/sync_topic_user_bookmarked.rb +++ b/app/jobs/regular/sync_topic_user_bookmarked.rb @@ -1,6 +1,9 @@ # 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] diff --git a/app/models/bookmark.rb b/app/models/bookmark.rb index 5af236ca2f4..6007677cde0 100644 --- a/app/models/bookmark.rb +++ b/app/models/bookmark.rb @@ -24,19 +24,46 @@ class Bookmark < ActiveRecord::Base ) end + # TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented. validate :unique_per_post_for_user, on: [:create, :update], if: Proc.new { |b| b.will_save_change_to_post_id? || b.will_save_change_to_user_id? } + # TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented. validate :for_topic_must_use_first_post, on: [:create, :update], if: Proc.new { |b| b.will_save_change_to_post_id? || b.will_save_change_to_for_topic? } + validate :polymorphic_columns_present, on: [:create, :update] + + validate :unique_per_bookmarkable, + on: [:create, :update], + if: Proc.new { |b| + b.will_save_change_to_bookmarkable_id? || b.will_save_change_to_bookmarkable_type? || b.will_save_change_to_user_id? + } + validate :ensure_sane_reminder_at_time, if: :will_save_change_to_reminder_at? validate :bookmark_limit_not_reached validates :name, length: { maximum: 100 } + def polymorphic_columns_present + return if !SiteSetting.use_polymorphic_bookmarks + return if self.bookmarkable_id.present? && self.bookmarkable_type.present? + + self.errors.add(:base, I18n.t("bookmarks.errors.bookmarkable_id_type_required")) + end + + def unique_per_bookmarkable + return if !SiteSetting.use_polymorphic_bookmarks + return if !Bookmark.exists?(user_id: user_id, bookmarkable_id: bookmarkable_id, bookmarkable_type: bookmarkable_type) + + self.errors.add(:base, I18n.t("bookmarks.errors.already_bookmarked", type: bookmarkable_type)) + end + + # TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented. def unique_per_post_for_user + return if SiteSetting.use_polymorphic_bookmarks + exists = if is_for_first_post? Bookmark.exists?(user_id: user_id, post_id: post_id, for_topic: for_topic) else @@ -48,6 +75,7 @@ class Bookmark < ActiveRecord::Base end end + # TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented. def for_topic_must_use_first_post if !is_for_first_post? && self.for_topic self.errors.add(:base, I18n.t("bookmarks.errors.for_topic_must_use_first_post")) @@ -78,6 +106,7 @@ class Bookmark < ActiveRecord::Base ) end + # TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented. def is_for_first_post? @is_for_first_post ||= new_record? ? Post.exists?(id: post_id, post_number: 1) : post.post_number == 1 end @@ -86,6 +115,8 @@ class Bookmark < ActiveRecord::Base self.auto_delete_preference == Bookmark.auto_delete_preferences[:when_reminder_sent] end + # TODO (martin) [POLYBOOK] This is only relevant for post/topic bookmarkables, need to + # think of a way to do this gracefully. def auto_delete_on_owner_reply? self.auto_delete_preference == Bookmark.auto_delete_preferences[:on_owner_reply] end @@ -118,11 +149,24 @@ class Bookmark < ActiveRecord::Base end scope :for_user_in_topic, ->(user_id, topic_id) { - joins(:post).where(user_id: user_id, posts: { topic_id: topic_id }) + if SiteSetting.use_polymorphic_bookmarks + joins("LEFT JOIN posts ON posts.id = bookmarks.bookmarkable_id AND bookmarks.bookmarkable_type = 'Post'") + .joins("LEFT JOIN topics ON topics.id = bookmarks.bookmarkable_id AND bookmarks.bookmarkable_type = 'Topic'") + .where( + "bookmarks.user_id = :user_id AND (topics.id = :topic_id OR posts.topic_id = :topic_id)", + user_id: user_id, topic_id: topic_id + ) + else + joins(:post).where(user_id: user_id, posts: { topic_id: topic_id }) + end } def self.find_for_topic_by_user(topic_id, user_id) - for_user_in_topic(user_id, topic_id).where(for_topic: true).first + if SiteSetting.use_polymorphic_bookmarks + find_by(user_id: user_id, bookmarkable_id: topic_id, bookmarkable_type: "Topic") + else + for_user_in_topic(user_id, topic_id).where(for_topic: true).first + end end def self.count_per_day(opts = nil) diff --git a/app/models/post_mover.rb b/app/models/post_mover.rb index 945d5473774..1a07f7f77fc 100644 --- a/app/models/post_mover.rb +++ b/app/models/post_mover.rb @@ -178,6 +178,9 @@ class PostMover # we don't want to keep the old topic's OP bookmarked when we are # moving it into a new topic + # + # TODO (martin) [POLYBOOK] This will need to be restructured for polymorphic + # bookmarks when edge cases are handled. Bookmark.where(post_id: post.id).update_all(post_id: new_post.id) new_post diff --git a/app/models/topic.rb b/app/models/topic.rb index bfbc2c1b922..afd37fb7502 100644 --- a/app/models/topic.rb +++ b/app/models/topic.rb @@ -208,12 +208,16 @@ class Topic < ActiveRecord::Base has_many :category_users, through: :category has_many :posts + # TODO (martin): + # # When we are ready we can add as: :bookmarkable here to use the # polymorphic association. # # At that time we may also want to make another association for example # :topic_bookmarks that get all of the bookmarks for that topic's bookmarkable id # and type, because this one gets all of the post bookmarks. + # + # Note: We can use Bookmark#for_user_in_topic for this. has_many :bookmarks, through: :posts has_many :ordered_posts, -> { order(post_number: :asc) }, class_name: "Post" diff --git a/app/serializers/post_serializer.rb b/app/serializers/post_serializer.rb index 2a2eec117b6..e9de4e2d74c 100644 --- a/app/serializers/post_serializer.rb +++ b/app/serializers/post_serializer.rb @@ -367,10 +367,18 @@ class PostSerializer < BasicPostSerializer end def post_bookmark - if @topic_view.present? - @post_bookmark ||= @topic_view.user_post_bookmarks.find { |bookmark| bookmark.post_id == object.id && !bookmark.for_topic } + if SiteSetting.use_polymorphic_bookmarks + if @topic_view.present? + @post_bookmark ||= @topic_view.bookmarks.find { |bookmark| bookmark.bookmarkable == object } + else + @post_bookmark ||= Bookmark.find_by(user: scope.user, bookmarkable: object) + end else - @post_bookmark ||= object.bookmarks.find_by(user: scope.user, for_topic: false) + if @topic_view.present? + @post_bookmark ||= @topic_view.bookmarks.find { |bookmark| bookmark.post_id == object.id && !bookmark.for_topic } + else + @post_bookmark ||= object.bookmarks.find_by(user: scope.user, for_topic: false) + end end end diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index a18aad5903d..7407e3c6408 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -317,6 +317,8 @@ en: bookmarks: created: "You've bookmarked this post. %{name}" + create_for: "Create bookmark for %{type}" + edit_for: "Edit bookmark for %{type}" not_bookmarked: "bookmark this post" remove_reminder_keep_bookmark: "Remove reminder and keep bookmark" created_with_reminder: "You've bookmarked this post with a reminder %{date}. %{name}" diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 24645833a75..34b4497f80b 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -429,11 +429,13 @@ en: bookmarks: errors: already_bookmarked_post: "You cannot bookmark the same post twice." + already_bookmarked: "You cannot bookmark the same %{type} twice." too_many: "Sorry, you cannot add more than %{limit} bookmarks, visit %{user_bookmarks_url} to remove some." cannot_set_past_reminder: "You cannot set a bookmark reminder in the past." cannot_set_reminder_in_distant_future: "You cannot set a bookmark reminder more than 10 years in the future." - time_must_be_provided: "time must be provided for all reminders" + time_must_be_provided: "Time must be provided for all reminders" for_topic_must_use_first_post: "You can only use the first post to bookmark the topic." + bookmarkable_id_type_required: "The name and type of the record to bookmark is required." reminders: at_desktop: "Next time I'm at my desktop" diff --git a/config/site_settings.yml b/config/site_settings.yml index 794c70ba634..94649f14840 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -2385,6 +2385,11 @@ uncategorized: allow_changing_staged_user_tracking: false + use_polymorphic_bookmarks: + client: true + default: false + hidden: true + user_preferences: default_email_digest_frequency: enum: "DigestEmailSiteSetting" diff --git a/lib/bookmark_manager.rb b/lib/bookmark_manager.rb index e4d5db5885d..843c0b0426f 100644 --- a/lib/bookmark_manager.rb +++ b/lib/bookmark_manager.rb @@ -5,6 +5,47 @@ class BookmarkManager def initialize(user) @user = user + @guardian = Guardian.new(user) + end + + def self.bookmark_metadata(bookmark, user) + data = {} + if SiteSetting.use_polymorphic_bookmarks + if bookmark.bookmarkable_type == "Topic" + data[:topic_bookmarked] = Bookmark.for_user_in_topic(user.id, bookmark.bookmarkable.id).exists? + elsif bookmark.bookmarkable_type == "Post" + data[:topic_bookmarked] = Bookmark.for_user_in_topic(user.id, bookmark.bookmarkable.topic.id).exists? + end + else + data[:topic_bookmarked] = Bookmark.for_user_in_topic(user.id, bookmark.topic.id).exists? + end + data + end + + # TODO (martin) [POLYBOOK] This will be used in place of #create once + # polymorphic bookmarks are implemented. + def create_for(bookmarkable_id:, bookmarkable_type:, name: nil, reminder_at: nil, options: {}) + raise NotImplementedError if !SiteSetting.use_polymorphic_bookmarks + + bookmarkable = bookmarkable_type.constantize.find_by(id: bookmarkable_id) + self.send("validate_bookmarkable_#{bookmarkable_type.downcase}", bookmarkable) + + bookmark = Bookmark.create( + { + user_id: @user.id, + bookmarkable: bookmarkable, + name: name, + reminder_at: reminder_at, + reminder_set_at: Time.zone.now + }.merge(options) + ) + + return add_errors_from(bookmark) if bookmark.errors.any? + + self.send("after_create_bookmarkable_#{bookmarkable_type.downcase}", bookmarkable) + update_user_option(bookmark) + + bookmark end ## @@ -41,13 +82,7 @@ class BookmarkManager options: {} ) post = Post.find_by(id: post_id) - - # no bookmarking deleted posts or topics - raise Discourse::InvalidAccess if post.blank? || post.topic.blank? - - if !Guardian.new(@user).can_see_post?(post) || !Guardian.new(@user).can_see_topic?(post.topic) - raise Discourse::InvalidAccess - end + validate_bookmarkable_post(post) bookmark = Bookmark.create( { @@ -75,9 +110,13 @@ class BookmarkManager bookmark.destroy - bookmarks_remaining_in_topic = update_topic_user_bookmarked(bookmark.topic) + if SiteSetting.use_polymorphic_bookmarks + self.send("after_destroy_bookmarkable_#{bookmark.bookmarkable_type.downcase}", bookmark) + else + update_topic_user_bookmarked(bookmark.topic) + end - { topic_bookmarked: bookmarks_remaining_in_topic } + bookmark end def destroy_for_topic(topic, filter = {}, opts = {}) @@ -86,7 +125,7 @@ class BookmarkManager Bookmark.transaction do topic_bookmarks.each do |bookmark| - raise Discourse::InvalidAccess.new if !Guardian.new(@user).can_delete?(bookmark) + raise Discourse::InvalidAccess.new if !@guardian.can_delete?(bookmark) bookmark.destroy end @@ -140,21 +179,45 @@ class BookmarkManager def find_bookmark_and_check_access(bookmark_id) bookmark = Bookmark.find_by(id: bookmark_id) raise Discourse::NotFound if !bookmark - raise Discourse::InvalidAccess.new if !Guardian.new(@user).can_edit?(bookmark) + raise Discourse::InvalidAccess.new if !@guardian.can_edit?(bookmark) bookmark end def update_topic_user_bookmarked(topic, opts = {}) # PostCreator can specify whether auto_track is enabled or not, don't want to # create a TopicUser in that case - bookmarks_remaining_in_topic = Bookmark.for_user_in_topic(@user.id, topic.id).exists? - return bookmarks_remaining_in_topic if opts.key?(:auto_track) && !opts[:auto_track] - - TopicUser.change(@user.id, topic, bookmarked: bookmarks_remaining_in_topic) - bookmarks_remaining_in_topic + return if opts.key?(:auto_track) && !opts[:auto_track] + TopicUser.change(@user.id, topic, bookmarked: Bookmark.for_user_in_topic(@user.id, topic.id).exists?) end def update_user_option(bookmark) @user.user_option.update!(bookmark_auto_delete_preference: bookmark.auto_delete_preference) end + + def after_create_bookmarkable_post(post, opts = {}) + update_topic_user_bookmarked(post.topic, opts) + end + + def after_create_bookmarkable_topic(topic, opts = {}) + update_topic_user_bookmarked(topic, opts) + end + + def after_destroy_bookmarkable_post(bookmark) + update_topic_user_bookmarked(bookmark.bookmarkable.topic) + end + + def after_destroy_bookmarkable_topic(bookmark) + update_topic_user_bookmarked(bookmark.bookmarkable) + end + + def validate_bookmarkable_post(post) + # no bookmarking deleted posts or topics + raise Discourse::InvalidAccess if post.blank? || !@guardian.can_see_post?(post) + validate_bookmarkable_topic(post.topic) + end + + def validate_bookmarkable_topic(topic) + # no bookmarking deleted posts or topics + raise Discourse::InvalidAccess if topic.blank? || !@guardian.can_see_topic?(topic) + end end diff --git a/lib/bookmark_reminder_notification_handler.rb b/lib/bookmark_reminder_notification_handler.rb index c0aa82d79be..4be52433c42 100644 --- a/lib/bookmark_reminder_notification_handler.rb +++ b/lib/bookmark_reminder_notification_handler.rb @@ -7,6 +7,9 @@ class BookmarkReminderNotificationHandler # 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) else diff --git a/lib/topic_view.rb b/lib/topic_view.rb index 5e76ffc17f1..7589bbe19b9 100644 --- a/lib/topic_view.rb +++ b/lib/topic_view.rb @@ -413,9 +413,15 @@ class TopicView end def bookmarks - @bookmarks ||= @topic.bookmarks.where(user: @user).joins(:topic).select( - :id, :post_id, "topics.id AS topic_id", :for_topic, :reminder_at, :name, :auto_delete_preference - ) + if SiteSetting.use_polymorphic_bookmarks + @bookmarks ||= Bookmark.for_user_in_topic(@user, @topic.id).select( + :id, :bookmarkable_id, :bookmarkable_type, :reminder_at, :name, :auto_delete_preference + ) + else + @bookmarks ||= @topic.bookmarks.where(user: @user).joins(:topic).select( + :id, :post_id, "topics.id AS topic_id", :for_topic, :reminder_at, :name, :auto_delete_preference + ) + end end MAX_PARTICIPANTS = 24 @@ -512,10 +518,6 @@ class TopicView @links ||= TopicLink.topic_map(@guardian, @topic.id) end - def user_post_bookmarks - @user_post_bookmarks ||= @topic.bookmarks.where(user: @user) - end - def reviewable_counts @reviewable_counts ||= begin sql = <<~SQL diff --git a/spec/fabricators/bookmark_fabricator.rb b/spec/fabricators/bookmark_fabricator.rb index 01b02d507ee..d86816792a7 100644 --- a/spec/fabricators/bookmark_fabricator.rb +++ b/spec/fabricators/bookmark_fabricator.rb @@ -6,6 +6,15 @@ Fabricator(:bookmark) do name "This looked interesting" reminder_at { 1.day.from_now.iso8601 } reminder_set_at { Time.zone.now } + + # TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented. + before_create do |bookmark| + if bookmark.bookmarkable_id.present? || bookmark.bookmarkable.present? + bookmark.post = nil + bookmark.post_id = nil + bookmark.for_topic = false + end + end end Fabricator(:bookmark_next_business_day_reminder, from: :bookmark) do diff --git a/spec/lib/bookmark_manager_spec.rb b/spec/lib/bookmark_manager_spec.rb index e5980162122..b44925046be 100644 --- a/spec/lib/bookmark_manager_spec.rb +++ b/spec/lib/bookmark_manager_spec.rb @@ -34,17 +34,19 @@ RSpec.describe BookmarkManager do expect(bookmark.for_topic).to eq(false) end + # TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented. + # Topic bookmarks will be distinct, not attached to a post. it "errors when creating a for_topic bookmark for a post that is not the first one" do subject.create(post_id: Fabricate(:post, topic: post.topic).id, name: name, for_topic: true) expect(subject.errors.full_messages).to include(I18n.t("bookmarks.errors.for_topic_must_use_first_post")) end - it "when topic is deleted it raises invalid access from guardian check" do + it "when topic is deleted it raises invalid access" do post.topic.trash! expect { subject.create(post_id: post.id, name: name) }.to raise_error(Discourse::InvalidAccess) end - it "when post is deleted it raises invalid access from guardian check" do + it "when post is deleted it raises invalid access" do post.trash! expect { subject.create(post_id: post.id, name: name) }.to raise_error(Discourse::InvalidAccess) end @@ -147,17 +149,11 @@ RSpec.describe BookmarkManager do describe ".destroy" do let!(:bookmark) { Fabricate(:bookmark, user: user, post: post) } it "deletes the existing bookmark" do - result = subject.destroy(bookmark.id) + subject.destroy(bookmark.id) expect(Bookmark.exists?(id: bookmark.id)).to eq(false) - expect(result[:topic_bookmarked]).to eq(false) - end - - it "returns a value indicating whether there are still other bookmarks in the topic for the user" do - Fabricate(:bookmark, user: user, post: Fabricate(:post, topic: post.topic)) - result = subject.destroy(bookmark.id) - expect(result[:topic_bookmarked]).to eq(true) end + # TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented. context "if the bookmark is the last one bookmarked in the topic" do it "marks the topic user bookmarked column as false" do TopicUser.create(user: user, topic: bookmark.post.topic, bookmarked: true) @@ -167,6 +163,19 @@ RSpec.describe BookmarkManager do end end + context "if the bookmark is the last one bookmarked in the topic (polymorphic)" do + before do + SiteSetting.use_polymorphic_bookmarks = true + end + it "marks the topic user bookmarked column as false" do + poly_bookmark = Fabricate(:bookmark, user: user, bookmarkable: post) + TopicUser.create(user: user, topic: post.topic, bookmarked: true) + subject.destroy(poly_bookmark.id) + tu = TopicUser.find_by(user: user) + expect(tu.bookmarked).to eq(false) + end + end + context "if the bookmark is belonging to some other user" do let!(:bookmark) { Fabricate(:bookmark, user: Fabricate(:admin), post: post) } it "raises an invalid access error" do @@ -262,6 +271,35 @@ RSpec.describe BookmarkManager do end end + describe ".destroy_for_topic (polymorphic)" do + before do + SiteSetting.use_polymorphic_bookmarks = true + end + + let!(:topic) { Fabricate(:topic) } + let!(:bookmark1) { Fabricate(:bookmark, bookmarkable: Fabricate(:post, topic: topic), user: user) } + let!(:bookmark2) { Fabricate(:bookmark, bookmarkable: Fabricate(:post, topic: topic), user: user) } + + it "destroys all bookmarks for the topic for the specified user" do + subject.destroy_for_topic(topic) + expect(Bookmark.for_user_in_topic(user.id, topic.id).length).to eq(0) + end + + it "does not destroy any other user's topic bookmarks" do + user2 = Fabricate(:user) + Fabricate(:bookmark, bookmarkable: Fabricate(:post, topic: topic), user: user2) + subject.destroy_for_topic(topic) + expect(Bookmark.for_user_in_topic(user2.id, topic.id).length).to eq(1) + end + + it "updates the topic user bookmarked column to false" do + TopicUser.create(user: user, topic: topic, bookmarked: true) + subject.destroy_for_topic(topic) + tu = TopicUser.find_by(user: user) + expect(tu.bookmarked).to eq(false) + end + end + describe ".send_reminder_notification" do let(:bookmark) { Fabricate(:bookmark, user: user) } it "sets the reminder_last_sent_at" do @@ -334,4 +372,127 @@ RSpec.describe BookmarkManager do end end end + + describe "#create_for (use_polymorphic_bookmarks)" do + before do + SiteSetting.use_polymorphic_bookmarks = true + end + + it "allows creating a bookmark for the topic and for the first post" do + subject.create_for(bookmarkable_id: post.topic_id, bookmarkable_type: "Topic", name: name) + bookmark = Bookmark.find_by(user: user, bookmarkable: post.topic) + expect(bookmark.present?).to eq(true) + + subject.create_for(bookmarkable_id: post.id, bookmarkable_type: "Post", name: name) + bookmark = Bookmark.find_by(user: user, bookmarkable: post) + expect(bookmark).not_to eq(nil) + end + + it "when topic is deleted it raises invalid access from guardian check" do + post.topic.trash! + expect { + subject.create_for(bookmarkable_id: post.topic_id, bookmarkable_type: "Topic", name: name) + }.to raise_error(Discourse::InvalidAccess) + end + + it "when post is deleted it raises invalid access from guardian check" do + post.trash! + expect { subject.create_for(bookmarkable_id: post.id, bookmarkable_type: "Post", name: name) }.to raise_error(Discourse::InvalidAccess) + end + + it "updates the topic user bookmarked column to true if any post is bookmarked" do + subject.create_for(bookmarkable_id: post.id, bookmarkable_type: "Post", name: name, reminder_at: reminder_at) + tu = TopicUser.find_by(user: user) + expect(tu.bookmarked).to eq(true) + tu.update(bookmarked: false) + new_post = Fabricate(:post, topic: post.topic) + subject.create_for(bookmarkable_id: new_post.id, bookmarkable_type: "Post") + tu.reload + expect(tu.bookmarked).to eq(true) + end + + context "when a reminder time is provided" do + it "saves the values correctly" do + subject.create_for(bookmarkable_id: post.id, bookmarkable_type: "Post", name: name, reminder_at: reminder_at) + bookmark = Bookmark.find_by(user: user, bookmarkable: post) + + expect(bookmark.reminder_at).to eq_time(reminder_at) + expect(bookmark.reminder_set_at).not_to eq(nil) + end + end + + context "when options are provided" do + let(:options) { { auto_delete_preference: Bookmark.auto_delete_preferences[:when_reminder_sent] } } + + it "saves any additional options successfully" do + subject.create_for(bookmarkable_id: post.id, bookmarkable_type: "Post", name: name, reminder_at: reminder_at, options: options) + bookmark = Bookmark.find_by(user: user, bookmarkable: post) + + expect(bookmark.auto_delete_preference).to eq(1) + end + end + + context "when the bookmark already exists for the user & post" do + before do + Bookmark.create(bookmarkable: post, user: user) + end + + it "adds an error to the manager" do + subject.create_for(bookmarkable_id: post.id, bookmarkable_type: "Post") + expect(subject.errors.full_messages).to include(I18n.t("bookmarks.errors.already_bookmarked", type: "Post")) + end + end + + context "when the bookmark name is too long" do + it "adds an error to the manager" do + subject.create_for(bookmarkable_id: post.id, bookmarkable_type: "Post", name: "test" * 100) + expect(subject.errors.full_messages).to include("Name is too long (maximum is 100 characters)") + end + end + + context "when the reminder time is in the past" do + let(:reminder_at) { 10.days.ago } + + it "adds an error to the manager" do + subject.create_for(bookmarkable_id: post.id, bookmarkable_type: "Post", name: name, reminder_at: reminder_at) + expect(subject.errors.full_messages).to include(I18n.t("bookmarks.errors.cannot_set_past_reminder")) + end + end + + context "when the reminder time is far-flung (> 10 years from now)" do + let(:reminder_at) { 11.years.from_now } + + it "adds an error to the manager" do + subject.create_for(bookmarkable_id: post.id, bookmarkable_type: "Post", name: name, reminder_at: reminder_at) + expect(subject.errors.full_messages).to include(I18n.t("bookmarks.errors.cannot_set_reminder_in_distant_future")) + end + end + + context "when the post is inaccessible for the user" do + before do + post.trash! + end + it "raises an invalid access error" do + expect { subject.create_for(bookmarkable_id: post.id, bookmarkable_type: "Post", name: name) }.to raise_error(Discourse::InvalidAccess) + end + end + + context "when the topic is inaccessible for the user" do + before do + post.topic.update(category: Fabricate(:private_category, group: Fabricate(:group))) + end + it "raises an invalid access error" do + expect { subject.create_for(bookmarkable_id: post.id, bookmarkable_type: "Post", name: name) }.to raise_error(Discourse::InvalidAccess) + end + end + + it "saves user's preference" do + subject.create_for(bookmarkable_id: post.id, bookmarkable_type: "Post", options: { auto_delete_preference: Bookmark.auto_delete_preferences[:when_reminder_sent] }) + expect(user.user_option.bookmark_auto_delete_preference).to eq(Bookmark.auto_delete_preferences[:when_reminder_sent]) + + bookmark = Bookmark.find_by(user: user) + subject.update(bookmark_id: bookmark.id, name: "test", reminder_at: 1.day.from_now, options: { auto_delete_preference: Bookmark.auto_delete_preferences[:on_owner_reply] }) + expect(user.user_option.bookmark_auto_delete_preference).to eq(Bookmark.auto_delete_preferences[:on_owner_reply]) + end + end end diff --git a/spec/lib/topic_view_spec.rb b/spec/lib/topic_view_spec.rb index ce197d6265d..2e187bd72b4 100644 --- a/spec/lib/topic_view_spec.rb +++ b/spec/lib/topic_view_spec.rb @@ -394,17 +394,21 @@ RSpec.describe TopicView do end end - context "#user_post_bookmarks" do + context "#bookmarks" do let!(:user) { Fabricate(:user) } let!(:bookmark1) { Fabricate(:bookmark, post: Fabricate(:post, topic: topic), user: user) } let!(:bookmark2) { Fabricate(:bookmark, post: Fabricate(:post, topic: topic), user: user) } let!(:bookmark3) { Fabricate(:bookmark, post: Fabricate(:post, topic: topic)) } it "returns all the bookmarks in the topic for a user" do - expect(TopicView.new(topic.id, user).user_post_bookmarks.pluck(:id)).to match_array( + expect(TopicView.new(topic.id, user).bookmarks.pluck(:id)).to match_array( [bookmark1.id, bookmark2.id] ) end + + it "returns [] for anon users" do + expect(TopicView.new(topic.id, nil).bookmarks.pluck(:id)).to eq([]) + end end context "#bookmarks" do diff --git a/spec/models/bookmark_spec.rb b/spec/models/bookmark_spec.rb index 83550cb6d1d..d42868ca919 100644 --- a/spec/models/bookmark_spec.rb +++ b/spec/models/bookmark_spec.rb @@ -37,6 +37,29 @@ describe Bookmark do expect(bookmark_3.valid?).to eq(false) end + + describe "polymorphic bookmarks" do + before do + SiteSetting.use_polymorphic_bookmarks = true + end + + it "does not allow a user to create a bookmark with only one polymorphic column" do + user = Fabricate(:user) + bm = Bookmark.create(bookmarkable_id: post.id, user: user) + expect(bm.errors.full_messages).to include(I18n.t("bookmarks.errors.bookmarkable_id_type_required")) + bm = Bookmark.create(bookmarkable_type: "Post", user: user) + expect(bm.errors.full_messages).to include(I18n.t("bookmarks.errors.bookmarkable_id_type_required")) + bm = Bookmark.create(bookmarkable_type: "Post", bookmarkable_id: post.id, user: user) + expect(bm.errors.full_messages).to be_empty + end + + it "does not allow a user to create a bookmark for the same record more than once" do + user = Fabricate(:user) + Bookmark.create(bookmarkable_type: "Post", bookmarkable_id: post.id, user: user) + bm = Bookmark.create(bookmarkable_type: "Post", bookmarkable_id: post.id, user: user) + expect(bm.errors.full_messages).to include(I18n.t("bookmarks.errors.already_bookmarked", type: "Post")) + end + end end describe "#find_for_topic_by_user" do diff --git a/spec/requests/bookmarks_controller_spec.rb b/spec/requests/bookmarks_controller_spec.rb index 8767348a8b9..64862fbeb3f 100644 --- a/spec/requests/bookmarks_controller_spec.rb +++ b/spec/requests/bookmarks_controller_spec.rb @@ -3,6 +3,7 @@ describe BookmarksController do let(:current_user) { Fabricate(:user) } let(:bookmark_post) { Fabricate(:post) } + let(:bookmark_topic) { Fabricate(:topic) } let(:bookmark_user) { current_user } before do @@ -28,6 +29,7 @@ describe BookmarksController do expect(response.status).to eq(429) end + # TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented. it "creates a for_topic bookmark" do post "/bookmarks.json", params: { post_id: bookmark_post.id, @@ -40,6 +42,7 @@ describe BookmarksController do expect(bookmark.for_topic).to eq(true) end + # TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented. it "errors when trying to create a for_topic bookmark for post_number > 1" do post "/bookmarks.json", params: { post_id: Fabricate(:post, topic: bookmark_post.topic).id, @@ -92,6 +95,38 @@ describe BookmarksController do ) end end + + context "if the user already has bookmarked the record (polymorphic)" do + before do + SiteSetting.use_polymorphic_bookmarks = true + Fabricate(:bookmark, bookmarkable: bookmark_post, user: bookmark_user) + Fabricate(:bookmark, bookmarkable: bookmark_topic, user: bookmark_user) + end + + it "returns failed JSON with a 400 error" do + post "/bookmarks.json", params: { + bookmarkable_id: bookmark_post.id, + bookmarkable_type: "Post", + reminder_at: (Time.zone.now + 1.day).iso8601 + } + + expect(response.status).to eq(400) + expect(response.parsed_body['errors']).to include( + I18n.t("bookmarks.errors.already_bookmarked", type: "Post") + ) + + post "/bookmarks.json", params: { + bookmarkable_id: bookmark_topic.id, + bookmarkable_type: "Topic", + reminder_at: (Time.zone.now + 1.day).iso8601 + } + + expect(response.status).to eq(400) + expect(response.parsed_body['errors']).to include( + I18n.t("bookmarks.errors.already_bookmarked", type: "Topic") + ) + end + end end describe "#destroy" do @@ -102,6 +137,39 @@ describe BookmarksController do expect(Bookmark.find_by(id: bookmark.id)).to eq(nil) end + it "returns an indication of whether there are still bookmarks in the topic" do + delete "/bookmarks/#{bookmark.id}.json" + expect(Bookmark.find_by(id: bookmark.id)).to eq(nil) + expect(response.parsed_body["topic_bookmarked"]).to eq(false) + bm2 = Fabricate(:bookmark, user: bookmark_user, post: Fabricate(:post, topic: bookmark_post.topic)) + Fabricate(:bookmark, user: bookmark_user, post: Fabricate(:post, topic: bookmark_post.topic)) + delete "/bookmarks/#{bm2.id}.json" + expect(Bookmark.find_by(id: bm2.id)).to eq(nil) + expect(response.parsed_body["topic_bookmarked"]).to eq(true) + end + + context "for polymorphic bookmarks" do + let!(:bookmark) { Fabricate(:bookmark, bookmarkable: bookmark_post, user: bookmark_user) } + + before do + SiteSetting.use_polymorphic_bookmarks = true + end + + it "returns an indication of whether there are still bookmarks in the topic" do + delete "/bookmarks/#{bookmark.id}.json" + expect(Bookmark.find_by(id: bookmark.id)).to eq(nil) + expect(response.parsed_body["topic_bookmarked"]).to eq(false) + bm2 = Fabricate(:bookmark, user: bookmark_user, bookmarkable: Fabricate(:post, topic: bookmark_post.topic)) + bm3 = Fabricate(:bookmark, user: bookmark_user, bookmarkable: bookmark_post.topic) + delete "/bookmarks/#{bm2.id}.json" + expect(Bookmark.find_by(id: bm2.id)).to eq(nil) + expect(response.parsed_body["topic_bookmarked"]).to eq(true) + delete "/bookmarks/#{bm3.id}.json" + expect(Bookmark.find_by(id: bm3.id)).to eq(nil) + expect(response.parsed_body["topic_bookmarked"]).to eq(false) + end + end + context "if the bookmark has already been destroyed" do it "returns failed JSON with a 403 error" do bookmark.destroy!