From fcc2e7ebbf3fa5f17c24c70149a949850f29a5f3 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Mon, 23 May 2022 10:07:15 +1000 Subject: [PATCH] FEATURE: Promote polymorphic bookmarks to default and migrate (#16729) This commit migrates all bookmarks to be polymorphic (using the bookmarkable_id and bookmarkable_type) columns. It also deletes all the old code guarded behind the use_polymorphic_bookmarks setting and changes that setting to true for all sites and by default for the sake of plugins. No data is deleted in the migrations, the old post_id and for_topic columns for bookmarks will be dropped later on. --- .../discourse/app/components/bookmark-list.js | 28 +- .../discourse/app/components/bookmark.js | 27 +- .../discourse/app/controllers/bookmark.js | 34 +-- .../discourse/app/controllers/topic.js | 160 +++-------- .../app/initializers/topic-footer-buttons.js | 17 +- .../discourse/app/models/bookmark.js | 19 +- .../javascripts/discourse/app/models/topic.js | 18 +- .../discourse/app/templates/topic.hbs | 2 - .../discourse/app/widgets/post-menu.js | 4 +- .../app/widgets/quick-access-bookmarks.js | 14 +- .../tests/acceptance/bookmarks-test.js | 31 +-- .../discourse/tests/fixtures/user-fixtures.js | 4 +- app/controllers/bookmarks_controller.rb | 27 +- app/controllers/posts_controller.rb | 6 +- app/controllers/topics_controller.rb | 3 +- app/controllers/users_controller.rb | 21 +- app/jobs/regular/export_user_archive.rb | 51 +--- .../regular/sync_topic_user_bookmarked.rb | 33 +-- app/models/bookmark.rb | 99 ++----- app/models/post.rb | 5 +- app/models/post_mover.rb | 5 +- app/serializers/post_serializer.rb | 14 +- .../user_bookmark_list_serializer.rb | 8 +- app/serializers/user_bookmark_serializer.rb | 3 + app/views/users/bookmarks.ics.erb | 19 -- config/site_settings.yml | 2 +- ..._polymorphic_bookmarks_and_make_default.rb | 28 ++ ...12011531_backfill_polymorphic_bookmarks.rb | 22 ++ lib/bookmark_manager.rb | 108 +++----- lib/bookmark_query.rb | 55 +--- lib/bookmark_reminder_notification_handler.rb | 32 +-- lib/guardian/bookmark_guardian.rb | 6 +- lib/search.rb | 16 +- lib/topic_view.rb | 12 +- plugins/discourse-narrative-bot/plugin.rb | 4 +- .../new_user_narrative_spec.rb | 7 - script/import_scripts/base.rb | 7 +- spec/fabricators/bookmark_fabricator.rb | 20 +- .../bookmark_reminder_notifications_spec.rb | 6 +- spec/jobs/export_user_archive_spec.rb | 65 +---- spec/jobs/sync_topic_user_bookmarked_spec.rb | 56 +--- spec/lib/bookmark_manager_spec.rb | 200 +------------- spec/lib/bookmark_query_spec.rb | 215 ++++++--------- ...mark_reminder_notification_handler_spec.rb | 79 +----- spec/lib/post_creator_spec.rb | 6 +- spec/lib/search_spec.rb | 14 +- spec/lib/topic_view_spec.rb | 19 +- spec/models/bookmark_spec.rb | 252 +++++------------- spec/models/post_mover_spec.rb | 36 +-- spec/models/user_bookmark_list_spec.rb | 56 ++-- spec/requests/admin/users_controller_spec.rb | 3 +- spec/requests/bookmarks_controller_spec.rb | 91 ++----- spec/requests/posts_controller_spec.rb | 4 +- spec/requests/topics_controller_spec.rb | 33 +-- spec/requests/users_controller_spec.rb | 181 +++++-------- spec/script/import_scripts/base_spec.rb | 16 +- spec/serializers/post_serializer_spec.rb | 13 +- .../user_bookmark_list_serializer_spec.rb | 1 - .../user_bookmark_serializer_spec.rb | 82 ------ .../user_post_bookmark_serializer_spec.rb | 4 - .../user_topic_bookmark_serializer_spec.rb | 4 - spec/services/post_bookmarkable_spec.rb | 4 - spec/services/topic_bookmarkable_spec.rb | 4 - 63 files changed, 582 insertions(+), 1833 deletions(-) create mode 100644 db/migrate/20220512011522_backfill_polymorphic_bookmarks_and_make_default.rb create mode 100644 db/post_migrate/20220512011531_backfill_polymorphic_bookmarks.rb delete mode 100644 spec/serializers/user_bookmark_serializer_spec.rb diff --git a/app/assets/javascripts/discourse/app/components/bookmark-list.js b/app/assets/javascripts/discourse/app/components/bookmark-list.js index f96c4aa94a5..03f2b863629 100644 --- a/app/assets/javascripts/discourse/app/components/bookmark-list.js +++ b/app/assets/javascripts/discourse/app/components/bookmark-list.js @@ -86,23 +86,19 @@ export default Component.extend(Scrolling, { @action editBookmark(bookmark) { - openBookmarkModal( - bookmark, - { - onAfterSave: (savedData) => { - this.appEvents.trigger( - "bookmarks:changed", - savedData, - bookmark.attachedTo() - ); - this.reload(); - }, - onAfterDelete: () => { - this.reload(); - }, + openBookmarkModal(bookmark, { + onAfterSave: (savedData) => { + this.appEvents.trigger( + "bookmarks:changed", + savedData, + bookmark.attachedTo() + ); + this.reload(); }, - { use_polymorphic_bookmarks: this.siteSettings.use_polymorphic_bookmarks } - ); + onAfterDelete: () => { + this.reload(); + }, + }); }, @action diff --git a/app/assets/javascripts/discourse/app/components/bookmark.js b/app/assets/javascripts/discourse/app/components/bookmark.js index 9d7a8eb01c6..5a7b20b8845 100644 --- a/app/assets/javascripts/discourse/app/components/bookmark.js +++ b/app/assets/javascripts/discourse/app/components/bookmark.js @@ -113,8 +113,12 @@ export default Component.extend({ }, _loadPostLocalDates() { + if (this.model.bookmarkableType !== "Post") { + return; + } + let postEl = document.querySelector( - `[data-post-id="${this.model.postId}"]` + `[data-post-id="${this.model.bookmarkableId}"]` ); let localDateEl; if (postEl) { @@ -156,14 +160,8 @@ export default Component.extend({ auto_delete_preference: this.autoDeletePreference, }; - 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.bookmarkable_id = this.model.bookmarkableId; + data.bookmarkable_type = this.model.bookmarkableType; if (this.editingExistingBookmark) { return ajax(`/bookmarks/${this.model.id}`, { @@ -191,15 +189,8 @@ export default Component.extend({ name: this.model.name, }; - 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; - } + data.bookmarkable_id = this.model.bookmarkableId; + data.bookmarkable_type = this.model.bookmarkableType; this.afterSave(data); }, diff --git a/app/assets/javascripts/discourse/app/controllers/bookmark.js b/app/assets/javascripts/discourse/app/controllers/bookmark.js index fc1737a8df8..c05fcda47ad 100644 --- a/app/assets/javascripts/discourse/app/controllers/bookmark.js +++ b/app/assets/javascripts/discourse/app/controllers/bookmark.js @@ -11,28 +11,9 @@ export function openBookmarkModal( onCloseWithoutSaving: null, onAfterSave: null, onAfterDelete: null, - }, - options = { - use_polymorphic_bookmarks: false, } ) { return new Promise((resolve) => { - const modalTitle = () => { - if (options.use_polymorphic_bookmarks) { - return I18n.t(bookmark.id ? "bookmarks.edit" : "bookmarks.create"); - } 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" - ); - } - }; - const model = { id: bookmark.id, reminderAt: bookmark.reminder_at, @@ -40,19 +21,14 @@ export function openBookmarkModal( 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; - } + model.bookmarkableId = bookmark.bookmarkable_id; + model.bookmarkableType = bookmark.bookmarkable_type; let modalController = showModal("bookmark", { model, - titleTranslated: modalTitle(), + titleTranslated: I18n.t( + bookmark.id ? "bookmarks.edit" : "bookmarks.create" + ), 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 ab42d954fa3..f50bc723c02 100644 --- a/app/assets/javascripts/discourse/app/controllers/topic.js +++ b/app/assets/javascripts/discourse/app/controllers/topic.js @@ -767,38 +767,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")); - } else if (post) { - const bookmarkForPost = this.model.bookmarks.find( - (bookmark) => bookmark.post_id === post.id && !bookmark.for_topic - ); - return this._modifyPostBookmark( - bookmarkForPost || - Bookmark.create({ - post_id: post.id, - topic_id: post.topic_id, - for_topic: false, - auto_delete_preference: this.currentUser - .bookmark_auto_delete_preference, - }), - post - ); - } else { - return this._toggleTopicLevelBookmark().then((changedIds) => { - if (!changedIds) { - return; - } - changedIds.forEach((id) => - this.appEvents.trigger("post-stream:refresh", { id }) - ); - }); - } - }, - - toggleBookmarkPolymorphic(post) { if (!this.currentUser) { return bootbox.alert(I18n.t("bookmarks.not_bookmarked")); } else if (post) { @@ -818,16 +787,14 @@ export default Controller.extend(bufferedProperty("model"), { post ); } else { - return this._toggleTopicLevelBookmarkPolymorphic().then( - (changedIds) => { - if (!changedIds) { - return; - } - changedIds.forEach((id) => - this.appEvents.trigger("post-stream:refresh", { id }) - ); + return this._toggleTopicLevelBookmark().then((changedIds) => { + if (!changedIds) { + return; } - ); + changedIds.forEach((id) => + this.appEvents.trigger("post-stream:refresh", { id }) + ); + }); } }, @@ -1285,56 +1252,46 @@ 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"); - }, - onAfterDelete: (topicBookmarked, bookmarkId) => { - this.model.removeBookmark(bookmarkId); - }, + // TODO (martin) (2022-02-01) Remove these old bookmark events, replaced by bookmarks:changed. + this.appEvents.trigger("topic:bookmark-toggled"); }, - { use_polymorphic_bookmarks: this.siteSettings.use_polymorphic_bookmarks } - ); + onAfterDelete: (topicBookmarked, bookmarkId) => { + this.model.removeBookmark(bookmarkId); + }, + }); }, _modifyPostBookmark(bookmark, post) { - 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); - }, + return openBookmarkModal(bookmark, { + onCloseWithoutSaving: () => { + post.appEvents.trigger("post-stream:refresh", { + id: bookmark.bookmarkable_id, + }); }, - { use_polymorphic_bookmarks: this.siteSettings.use_polymorphic_bookmarks } - ); + 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); + }, + }); }, _syncBookmarks(data) { @@ -1352,7 +1309,6 @@ 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(); @@ -1362,40 +1318,6 @@ export default Controller.extend(bufferedProperty("model"), { return this._maybeClearAllBookmarks(); } - if (this.model.bookmarkCount === 1) { - const forTopicBookmark = this.model.bookmarks.findBy("for_topic", true); - if (forTopicBookmark) { - return this._modifyTopicBookmark(forTopicBookmark); - } else { - const bookmark = this.model.bookmarks[0]; - const post = await this.model.postById(bookmark.post_id); - return this._modifyPostBookmark(bookmark, post); - } - } - - if (this.model.bookmarkCount === 0) { - const firstPost = await this.model.firstPost(); - return this._modifyTopicBookmark( - Bookmark.create({ - post_id: firstPost.id, - topic_id: this.model.id, - for_topic: true, - auto_delete_preference: this.currentUser - .bookmark_auto_delete_preference, - }) - ); - } - }, - - 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", 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 b1f330c5c73..83e5a63b40a 100644 --- a/app/assets/javascripts/discourse/app/initializers/topic-footer-buttons.js +++ b/app/assets/javascripts/discourse/app/initializers/topic-footer-buttons.js @@ -11,8 +11,7 @@ const DEFER_PRIORITY = 500; export default { name: "topic-footer-buttons", - initialize(container) { - const siteSettings = container.lookup("site-settings:main"); + initialize() { registerTopicFooterButton({ id: "share-and-invite", icon: "d-topic-share", @@ -99,12 +98,9 @@ export default { if (this.topic.bookmarkCount === 0) { return I18n.t("bookmarked.help.bookmark"); } else if (this.topic.bookmarkCount === 1) { - // 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"; - }); + const anyTopicBookmarks = this.topic.bookmarks.some( + (bookmark) => bookmark.bookmarkable_type === "Topic" + ); if (anyTopicBookmarks) { return I18n.t("bookmarked.help.edit_bookmark_for_topic"); @@ -119,10 +115,7 @@ export default { return I18n.t("bookmarked.help.unbookmark"); } }, - // TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented. - action: siteSettings.use_polymorphic_bookmarks - ? "toggleBookmarkPolymorphic" - : "toggleBookmark", + action: "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 c38a5b69cc6..f2f1b2f6d6e 100644 --- a/app/assets/javascripts/discourse/app/models/bookmark.js +++ b/app/assets/javascripts/discourse/app/models/bookmark.js @@ -39,18 +39,10 @@ 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 }; - } - return { target: "post", targetId: this.post_id }; + return { + target: this.bookmarkable_type.toLowerCase(), + targetId: this.bookmarkable_id, + }; }, togglePin() { @@ -161,9 +153,6 @@ const Bookmark = RestModel.extend({ @discourseComputed("bookmarkable_type") bookmarkableTopicAlike(bookmarkable_type) { - if (!this.siteSettings.use_polymorphic_bookmarks) { - return true; - } return ["Topic", "Post"].includes(bookmarkable_type); }, }); diff --git a/app/assets/javascripts/discourse/app/models/topic.js b/app/assets/javascripts/discourse/app/models/topic.js index e6b960f4eac..9b4e5c8efcc 100644 --- a/app/assets/javascripts/discourse/app/models/topic.js +++ b/app/assets/javascripts/discourse/app/models/topic.js @@ -397,15 +397,7 @@ const Topic = RestModel.extend({ this.set( "bookmarks", this.bookmarks.filter((bookmark) => { - // 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") - ) { + if (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( @@ -425,11 +417,9 @@ const Topic = RestModel.extend({ clearBookmarks() { this.toggleProperty("bookmarked"); - const postIds = this.siteSettings.use_polymorphic_bookmarks - ? this.bookmarks - .filterBy("bookmarkable_type", "Post") - .mapBy("bookmarkable_id") - : this.bookmarks.mapBy("post_id"); + const postIds = this.bookmarks + .filterBy("bookmarkable_type", "Post") + .mapBy("bookmarkable_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 3b74568e5b8..dd268baff07 100644 --- a/app/assets/javascripts/discourse/app/templates/topic.hbs +++ b/app/assets/javascripts/discourse/app/templates/topic.hbs @@ -223,7 +223,6 @@ recoverPost=(action "recoverPost") expandHidden=(action "expandHidden") toggleBookmark=(action "toggleBookmark") - toggleBookmarkPolymorphic=(action "toggleBookmarkPolymorphic") togglePostType=(action "togglePostType") rebakePost=(action "rebakePost") changePostOwner=(action "changePostOwner") @@ -363,7 +362,6 @@ 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 1f542697ecb..9023b0ed767 100644 --- a/app/assets/javascripts/discourse/app/widgets/post-menu.js +++ b/app/assets/javascripts/discourse/app/widgets/post-menu.js @@ -355,9 +355,7 @@ registerButton( return { id: attrs.bookmarked ? "unbookmark" : "bookmark", - action: siteSettings.use_polymorphic_bookmarks - ? "toggleBookmarkPolymorphic" - : "toggleBookmark", + action: "toggleBookmark", title, titleOptions, className: classNames.join(" "), diff --git a/app/assets/javascripts/discourse/app/widgets/quick-access-bookmarks.js b/app/assets/javascripts/discourse/app/widgets/quick-access-bookmarks.js index 16888bee7e7..4be4dfbfca6 100644 --- a/app/assets/javascripts/discourse/app/widgets/quick-access-bookmarks.js +++ b/app/assets/javascripts/discourse/app/widgets/quick-access-bookmarks.js @@ -47,15 +47,25 @@ createWidgetFrom(QuickAccessPanel, "quick-access-bookmarks", { // for topic level bookmarks we want to jump to the last unread post // instead of the OP let postNumber; - if (bookmark.for_topic) { + if (bookmark.bookmarkable_type === "Topic") { postNumber = bookmark.last_read_post_number + 1; } else { postNumber = bookmark.linked_post_number; } + let href; + if ( + bookmark.bookmarkable_type === "Topic" || + bookmark.bookmarkable_type === "Post" + ) { + href = postUrl(bookmark.slug, bookmark.topic_id, postNumber); + } else { + href = bookmark.bookmarkable_type; + } + return this.attach("quick-access-item", { icon: this.icon(bookmark), - href: postUrl(bookmark.slug, bookmark.topic_id, postNumber), + href, title: bookmark.name, content: bookmark.title, username: bookmark.post_user_username, diff --git a/app/assets/javascripts/discourse/tests/acceptance/bookmarks-test.js b/app/assets/javascripts/discourse/tests/acceptance/bookmarks-test.js index 0b09dc508cb..0ee4fda4dfc 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/bookmarks-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/bookmarks-test.js @@ -83,16 +83,15 @@ acceptance("Bookmarking", function (needs) { function handleRequest(request) { const data = helper.parsePostData(request.requestBody); - if (data.post_id === "398") { - if (data.for_topic === "true") { - return helper.response({ id: 3, success: "OK" }); - } else { - return helper.response({ id: 1, success: "OK" }); - } - } else if (data.post_id === "419") { + if (data.bookmarkable_id === "398" && data.bookmarkable_type === "Post") { + return helper.response({ id: 1, success: "OK" }); + } else if (data.bookmarkable_type === "Topic") { + return helper.response({ id: 3, success: "OK" }); + } else if ( + data.bookmarkable_id === "419" && + data.bookmarkable_type === "Post" + ) { return helper.response({ id: 2, success: "OK" }); - } else { - throw new Error("Pretender: unknown post_id"); } } server.post("/bookmarks", handleRequest); @@ -368,13 +367,6 @@ acceptance("Bookmarking", function (needs) { test("Creating and editing a topic level bookmark", async function (assert) { await visit("/t/internationalization-localization/280"); await click("#topic-footer-button-bookmark"); - - assert.strictEqual( - query("#discourse-modal-title").innerText, - I18n.t("post.bookmarks.create_for_topic"), - "The create modal says creating a topic bookmark" - ); - await click("#save-bookmark"); assert.notOk( @@ -389,13 +381,6 @@ acceptance("Bookmarking", function (needs) { ); await click("#topic-footer-button-bookmark"); - - assert.strictEqual( - query("#discourse-modal-title").innerText, - I18n.t("post.bookmarks.edit_for_topic"), - "The edit modal says editing a topic bookmark" - ); - await fillIn("input#bookmark-name", "Test name"); await click("#tap_tile_tomorrow"); diff --git a/app/assets/javascripts/discourse/tests/fixtures/user-fixtures.js b/app/assets/javascripts/discourse/tests/fixtures/user-fixtures.js index 749f16fa0ab..cafbe1a5672 100644 --- a/app/assets/javascripts/discourse/tests/fixtures/user-fixtures.js +++ b/app/assets/javascripts/discourse/tests/fixtures/user-fixtures.js @@ -427,7 +427,9 @@ export default { created_at: "2020-04-07T05:30:40.446Z", topic_id: 119, linked_post_number: 1, - post_id: 281, + bookmarkable_id: 281, + bookmarkable_type: "Post", + bookmarkable_url: "http://localhost:4200/t/yelling-topic-title/119", name: "test", reminder_at: null, title: "Yelling topic title :/", diff --git a/app/controllers/bookmarks_controller.rb b/app/controllers/bookmarks_controller.rb index 9aabad449b0..09810cae00c 100644 --- a/app/controllers/bookmarks_controller.rb +++ b/app/controllers/bookmarks_controller.rb @@ -4,12 +4,8 @@ class BookmarksController < ApplicationController requires_login def create - if SiteSetting.use_polymorphic_bookmarks - params.require(:bookmarkable_id) - params.require(:bookmarkable_type) - else - params.require(:post_id) - end + params.require(:bookmarkable_id) + params.require(:bookmarkable_type) RateLimiter.new( current_user, "create_bookmark", SiteSetting.max_bookmarks_per_day, 1.day.to_i @@ -25,21 +21,12 @@ class BookmarksController < ApplicationController } } - if SiteSetting.use_polymorphic_bookmarks - bookmark = bookmark_manager.create_for( - **create_params.merge( - bookmarkable_id: params[:bookmarkable_id], - bookmarkable_type: params[:bookmarkable_type] - ) + 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) diff --git a/app/controllers/posts_controller.rb b/app/controllers/posts_controller.rb index f1292d4fe08..b75866ba71e 100644 --- a/app/controllers/posts_controller.rb +++ b/app/controllers/posts_controller.rb @@ -539,7 +539,11 @@ class PostsController < ApplicationController def destroy_bookmark params.require(:post_id) - bookmark_id = Bookmark.where(post_id: params[:post_id], user_id: current_user.id).pluck_first(:id) + bookmark_id = Bookmark.where( + bookmarkable_id: params[:post_id], + bookmarkable_type: "Post", + user_id: current_user.id + ).pluck_first(:id) destroyed_bookmark = BookmarkManager.new(current_user).destroy(bookmark_id) render json: success_json.merge(BookmarkManager.bookmark_metadata(destroyed_bookmark, current_user)) diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index 5e4c9639560..d30a1981e02 100644 --- a/app/controllers/topics_controller.rb +++ b/app/controllers/topics_controller.rb @@ -608,10 +608,9 @@ class TopicsController < ApplicationController def bookmark topic = Topic.find(params[:topic_id].to_i) - first_post = topic.ordered_posts.first bookmark_manager = BookmarkManager.new(current_user) - bookmark_manager.create(post_id: first_post.id) + bookmark_manager.create_for(bookmarkable_id: topic.id, bookmarkable_type: "Topic") if bookmark_manager.errors.any? return render_json_error(bookmark_manager, status: 400) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 67f78b1bbba..6976cdb2d12 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -1725,20 +1725,13 @@ class UsersController < ApplicationController end end format.ics do - if SiteSetting.use_polymorphic_bookmarks - @bookmark_reminders = Bookmark.with_reminders - .where(user_id: user.id) - .order(:reminder_at) - .map do |bookmark| - bookmark.registered_bookmarkable.serializer.new( - bookmark, scope: user_guardian, root: false - ) - end - else - @bookmark_reminders = Bookmark.with_reminders - .where(user_id: user.id) - .includes(:topic) - .order(:reminder_at) + @bookmark_reminders = Bookmark.with_reminders + .where(user_id: user.id) + .order(:reminder_at) + .map do |bookmark| + bookmark.registered_bookmarkable.serializer.new( + bookmark, scope: user_guardian, root: false + ) end end end diff --git a/app/jobs/regular/export_user_archive.rb b/app/jobs/regular/export_user_archive.rb index c07590c8af4..3e76285f9d7 100644 --- a/app/jobs/regular/export_user_archive.rb +++ b/app/jobs/regular/export_user_archive.rb @@ -31,9 +31,7 @@ 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'], + bookmarks: ['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'], @@ -50,16 +48,7 @@ module Jobs components = [] COMPONENTS.each do |name| - export_method = \ - if name == "bookmarks" - if SiteSetting.use_polymorphic_bookmarks - "bookmarks_polymorphic_export" - else - "bookmarks_export" - end - else - "#{name}_export" - end + export_method = "#{name}_export" h = { name: name, method: :"#{export_method}" } h[:filetype] = :csv filetype_method = :"#{name}_filetype" @@ -240,8 +229,8 @@ module Jobs end end - def bookmarks_polymorphic_export - return enum_for(:bookmarks_polymorphic_export) unless block_given? + def bookmarks_export + return enum_for(:bookmarks_export) unless block_given? @current_user.bookmarks.where.not(bookmarkable_type: nil).order(:id).each do |bookmark| link = '' @@ -268,32 +257,6 @@ module Jobs end end - # TODO (martin) [POLYBOOK] No longer relevant after polymorphic bookmarks implemented - def bookmarks_export - return enum_for(:bookmarks_export) unless block_given? - - @current_user.bookmarks.joins(:post).order(:id).each do |bookmark| - link = '' - if guardian.can_see_bookmarkable?(bookmark) - link = bookmark.post.full_url - end - - yield [ - bookmark.post_id, - bookmark.topic_id, - bookmark.post&.post_number, - 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 - def category_preferences_export return enum_for(:category_preferences_export) unless block_given? @@ -434,12 +397,6 @@ 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 2710861c59d..0633e472ff8 100644 --- a/app/jobs/regular/sync_topic_user_bookmarked.rb +++ b/app/jobs/regular/sync_topic_user_bookmarked.rb @@ -5,8 +5,7 @@ module Jobs def execute(args = {}) topic_id = args[:topic_id] - if SiteSetting.use_polymorphic_bookmarks - DB.exec(<<~SQL, topic_id: topic_id) + 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' @@ -14,9 +13,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 ( @@ -27,31 +26,7 @@ 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 - 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 - 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.post_id - 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 - end + SQL end end end diff --git a/app/models/bookmark.rb b/app/models/bookmark.rb index 9d8e86b838b..a74abbbac5e 100644 --- a/app/models/bookmark.rb +++ b/app/models/bookmark.rb @@ -34,16 +34,8 @@ class Bookmark < ActiveRecord::Base end belongs_to :user - belongs_to :post - has_one :topic, through: :post belongs_to :bookmarkable, polymorphic: true - # TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented. - def topic_id - return if SiteSetting.use_polymorphic_bookmarks - post.topic_id - end - def self.auto_delete_preferences @auto_delete_preferences ||= Enum.new( never: 0, @@ -57,16 +49,6 @@ class Bookmark < ActiveRecord::Base bookmarks_relation.select { |bm| bm.bookmarkable_type == type } end - # TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented. - validate :unique_per_post_for_user, - on: [:create, :update], - 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 :valid_bookmarkable_type, on: [:create, :update] @@ -85,41 +67,17 @@ class Bookmark < ActiveRecord::Base end 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 - Bookmark.exists?(user_id: user_id, post_id: post_id) - end - - if exists - self.errors.add(:base, I18n.t("bookmarks.errors.already_bookmarked_post")) - 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")) - end - end - def ensure_sane_reminder_at_time return if reminder_at.blank? if reminder_at < Time.zone.now @@ -145,27 +103,15 @@ class Bookmark < ActiveRecord::Base end def valid_bookmarkable_type - return if !SiteSetting.use_polymorphic_bookmarks return if Bookmark.valid_bookmarkable_types.include?(self.bookmarkable_type) self.errors.add(:base, I18n.t("bookmarks.errors.invalid_bookmarkable", type: self.bookmarkable_type)) end - # TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented. - def is_for_first_post? - @is_for_first_post ||= new_record? ? Post.exists?(id: post_id, post_number: 1) : post.post_number == 1 - end - def auto_delete_when_reminder_sent? 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 - def auto_clear_reminder_when_reminder_sent? self.auto_delete_preference == Bookmark.auto_delete_preferences[:clear_reminder] end @@ -194,26 +140,16 @@ class Bookmark < ActiveRecord::Base end scope :for_user_in_topic, ->(user_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 + 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') OR + (topics.id = posts.topic_id)") + .where( + "bookmarks.user_id = :user_id AND (topics.id = :topic_id OR posts.topic_id = :topic_id) + AND posts.deleted_at IS NULL AND topics.deleted_at IS NULL", + user_id: user_id, topic_id: topic_id + ) } - def self.find_for_topic_by_user(topic_id, user_id) - 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) opts ||= {} result = where('bookmarks.created_at >= ?', opts[:start_date] || (opts[:since_days_ago] || 30).days.ago) @@ -223,15 +159,11 @@ class Bookmark < ActiveRecord::Base end if opts[:category_id] - if SiteSetting.use_polymorphic_bookmarks - result = result - .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') OR (topics.id = posts.topic_id)") - .where("topics.deleted_at IS NULL AND posts.deleted_at IS NULL") - .merge(Topic.in_category_and_subcategories(opts[:category_id])) - else - result = result.joins(:topic).merge(Topic.in_category_and_subcategories(opts[:category_id])) - end + result = result + .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') OR (topics.id = posts.topic_id)") + .where("topics.deleted_at IS NULL AND posts.deleted_at IS NULL") + .merge(Topic.in_category_and_subcategories(opts[:category_id])) end result.group('date(bookmarks.created_at)') @@ -248,7 +180,10 @@ class Bookmark < ActiveRecord::Base topics_deleted = DB.query(<<~SQL, grace_time: grace_time) DELETE FROM bookmarks b USING topics t, posts p - WHERE (t.id = p.topic_id AND b.post_id = p.id) + WHERE (t.id = p.topic_id AND ( + (b.bookmarkable_id = p.id AND b.bookmarkable_type = 'Post') OR + (b.bookmarkable_id = p.id AND b.bookmarkable_type = 'Topic') + )) AND (t.deleted_at < :grace_time OR p.deleted_at < :grace_time) RETURNING t.id AS topic_id SQL diff --git a/app/models/post.rb b/app/models/post.rb index d956255c9fd..4f81ce7939e 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -47,10 +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 + has_many :bookmarks, as: :bookmarkable has_one :incoming_email diff --git a/app/models/post_mover.rb b/app/models/post_mover.rb index 1a07f7f77fc..f8e77c7cd6d 100644 --- a/app/models/post_mover.rb +++ b/app/models/post_mover.rb @@ -178,10 +178,7 @@ 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) + Bookmark.where(bookmarkable: post).update_all(bookmarkable_id: new_post.id) new_post end diff --git a/app/serializers/post_serializer.rb b/app/serializers/post_serializer.rb index bfb37faf26a..472e3e87bb0 100644 --- a/app/serializers/post_serializer.rb +++ b/app/serializers/post_serializer.rb @@ -369,18 +369,10 @@ class PostSerializer < BasicPostSerializer end def post_bookmark - 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 + if @topic_view.present? + @post_bookmark ||= @topic_view.bookmarks.find { |bookmark| bookmark.bookmarkable == object } else - 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 + @post_bookmark ||= Bookmark.find_by(user: scope.user, bookmarkable: object) end end diff --git a/app/serializers/user_bookmark_list_serializer.rb b/app/serializers/user_bookmark_list_serializer.rb index b8e472d15b7..60859f557bc 100644 --- a/app/serializers/user_bookmark_list_serializer.rb +++ b/app/serializers/user_bookmark_list_serializer.rb @@ -4,12 +4,8 @@ class UserBookmarkListSerializer < ApplicationSerializer attributes :more_bookmarks_url, :bookmarks def bookmarks - if SiteSetting.use_polymorphic_bookmarks - object.bookmarks.map do |bm| - bm.registered_bookmarkable.serializer.new(bm, scope: scope, root: false) - end - else - object.bookmarks.map { |bm| UserBookmarkSerializer.new(bm, scope: scope, root: false) } + object.bookmarks.map do |bm| + bm.registered_bookmarkable.serializer.new(bm, scope: scope, root: false) end end diff --git a/app/serializers/user_bookmark_serializer.rb b/app/serializers/user_bookmark_serializer.rb index 0f8386703ba..0992830d9b2 100644 --- a/app/serializers/user_bookmark_serializer.rb +++ b/app/serializers/user_bookmark_serializer.rb @@ -3,6 +3,9 @@ require_relative 'post_item_excerpt' # TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented. +# +# This must be deleted after the plugins relying on it are updated once +# polymorphic bookmarks become the norm. class UserBookmarkSerializer < ApplicationSerializer include PostItemExcerpt include TopicTagsMixin diff --git a/app/views/users/bookmarks.ics.erb b/app/views/users/bookmarks.ics.erb index 8b0b6be6c96..2c80cc951b8 100644 --- a/app/views/users/bookmarks.ics.erb +++ b/app/views/users/bookmarks.ics.erb @@ -1,4 +1,3 @@ -<% if SiteSetting.use_polymorphic_bookmarks %> BEGIN:VCALENDAR VERSION:2.0 PRODID:-//Discourse//<%= Discourse.current_hostname %>//<%= Discourse.full_version %>//EN @@ -14,21 +13,3 @@ URL:<%= bookmark.bookmarkable_url %> END:VEVENT <% end %> END:VCALENDAR - -<% else %> -BEGIN:VCALENDAR -VERSION:2.0 -PRODID:-//Discourse//<%= Discourse.current_hostname %>//<%= Discourse.full_version %>//EN -<% @bookmark_reminders.each do |bookmark| %> -BEGIN:VEVENT -UID:bookmark_reminder_#<%= bookmark.id %>@<%= Discourse.current_hostname %> -DTSTAMP:<%= bookmark.updated_at.strftime(I18n.t("datetime_formats.formats.calendar_ics")) %> -DTSTART:<%= bookmark.reminder_at_ics %> -DTEND:<%= bookmark.reminder_at_ics(offset: 1.hour) %> -SUMMARY:<%= bookmark.name.presence || bookmark.topic.title %> -DESCRIPTION:<%= Discourse.base_url %>/t/-/<%= bookmark.topic_id %> -URL:<%= Discourse.base_url %>/t/-/<%= bookmark.topic_id %> -END:VEVENT -<% end %> -END:VCALENDAR -<% end %> diff --git a/config/site_settings.yml b/config/site_settings.yml index 6d287f87e6a..4e254263add 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -2403,7 +2403,7 @@ uncategorized: use_polymorphic_bookmarks: client: true - default: false + default: true hidden: true suggest_weekends_in_date_pickers: diff --git a/db/migrate/20220512011522_backfill_polymorphic_bookmarks_and_make_default.rb b/db/migrate/20220512011522_backfill_polymorphic_bookmarks_and_make_default.rb new file mode 100644 index 00000000000..f35aaaefe17 --- /dev/null +++ b/db/migrate/20220512011522_backfill_polymorphic_bookmarks_and_make_default.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +class BackfillPolymorphicBookmarksAndMakeDefault < ActiveRecord::Migration[7.0] + def up + DB.exec(<<~SQL) + UPDATE site_settings + SET value = 't' + WHERE name = 'use_polymorphic_bookmarks' + SQL + + DB.exec(<<~SQL) + UPDATE bookmarks + SET bookmarkable_id = post_id, bookmarkable_type = 'Post' + WHERE NOT bookmarks.for_topic AND bookmarkable_id IS NULL + SQL + + DB.exec(<<~SQL) + UPDATE bookmarks + SET bookmarkable_id = posts.topic_id, bookmarkable_type = 'Topic' + FROM posts + WHERE bookmarks.for_topic AND posts.id = bookmarks.post_id AND bookmarkable_id IS NULL + SQL + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/db/post_migrate/20220512011531_backfill_polymorphic_bookmarks.rb b/db/post_migrate/20220512011531_backfill_polymorphic_bookmarks.rb new file mode 100644 index 00000000000..d92e7aa05c7 --- /dev/null +++ b/db/post_migrate/20220512011531_backfill_polymorphic_bookmarks.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +class BackfillPolymorphicBookmarks < ActiveRecord::Migration[7.0] + def up + DB.exec(<<~SQL) + UPDATE bookmarks + SET bookmarkable_id = post_id, bookmarkable_type = 'Post' + WHERE NOT bookmarks.for_topic AND bookmarkable_id IS NULL + SQL + + DB.exec(<<~SQL) + UPDATE bookmarks + SET bookmarkable_id = posts.topic_id, bookmarkable_type = 'Topic' + FROM posts + WHERE bookmarks.for_topic AND posts.id = bookmarks.post_id AND bookmarkable_id IS NULL + SQL + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/lib/bookmark_manager.rb b/lib/bookmark_manager.rb index 11a6186e086..4117d314d7d 100644 --- a/lib/bookmark_manager.rb +++ b/lib/bookmark_manager.rb @@ -9,18 +9,37 @@ class BookmarkManager end def self.bookmark_metadata(bookmark, user) - if SiteSetting.use_polymorphic_bookmarks - bookmark.registered_bookmarkable.bookmark_metadata(bookmark, user) - else - { topic_bookmarked: Bookmark.for_user_in_topic(user.id, bookmark.topic.id).exists? } - end + bookmark.registered_bookmarkable.bookmark_metadata(bookmark, user) end - # TODO (martin) [POLYBOOK] This will be used in place of #create once - # polymorphic bookmarks are implemented. + ## + # Creates a bookmark for a registered bookmarkable (see Bookmark.register_bookmarkable + # and RegisteredBookmarkable for details on this). + # + # Only allows creation of bookmarks for records the user + # can access via Guardian. + # + # Any ActiveModel validation errors raised by the Bookmark model are + # hoisted to the instance of this class for further reporting. + # + # Before creation validations, after create callbacks, and after delete + # callbacks are all RegisteredBookmarkable specific and should be defined + # there. + # + # @param [Integer] bookmarkable_id The ID of the ActiveRecord model to attach the bookmark to. + # @param [String] bookmarkable_type The class name of the ActiveRecord model to attach the bookmark to. + # @param [String] name A short note for the bookmark, shown on the user bookmark list + # and on hover of reminder notifications. + # @param reminder_at The datetime when a bookmark reminder should be sent after. + # Note this is not the exact time a reminder will be sent, as + # we send reminders on a rolling schedule. + # See Jobs::BookmarkReminderNotifications + # @params options Additional options when creating a bookmark + # - auto_delete_preference: + # See Bookmark.auto_delete_preferences, + # this is used to determine when to delete a bookmark + # automatically. 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) registered_bookmarkable = Bookmark.registered_bookmarkable_from_type(bookmarkable_type) registered_bookmarkable.validate_before_create(@guardian, bookmarkable) @@ -43,78 +62,12 @@ class BookmarkManager bookmark end - ## - # Creates a bookmark for a post where both the post and the topic are - # not deleted. Only allows creation of bookmarks for posts the user - # can access via Guardian. - # - # Any ActiveModel validation errors raised by the Bookmark model are - # hoisted to the instance of this class for further reporting. - # - # Also handles setting the associated TopicUser.bookmarked value for - # the post's topic for the user that is creating the bookmark. - # - # @param post_id A post ID for a post that is not deleted. - # @param name A short note for the bookmark, shown on the user bookmark list - # and on hover of reminder notifications. - # @param reminder_at The datetime when a bookmark reminder should be sent after. - # Note this is not the exact time a reminder will be sent, as - # we send reminders on a rolling schedule. - # See Jobs::BookmarkReminderNotifications - # @param for_topic Whether we are creating a topic-level bookmark which - # has different behaviour in the UI. Only bookmarks for - # posts with post_number 1 can be marked as for_topic. - # @params options Additional options when creating a bookmark - # - auto_delete_preference: - # See Bookmark.auto_delete_preferences, - # this is used to determine when to delete a bookmark - # automatically. - def create( - post_id:, - name: nil, - reminder_at: nil, - for_topic: false, - options: {} - ) - post = Post.find_by(id: post_id) - if post.blank? || - post.topic.blank? || - !@guardian.can_see_topic?(post.topic) || - !@guardian.can_see_post?(post) - raise Discourse::InvalidAccess - end - - bookmark = Bookmark.create( - { - user_id: @user.id, - post: post, - name: name, - reminder_at: reminder_at, - reminder_set_at: Time.zone.now, - for_topic: for_topic - }.merge(options) - ) - - if bookmark.errors.any? - return add_errors_from(bookmark) - end - - update_topic_user_bookmarked(post.topic) - update_user_option(bookmark) - - bookmark - end - def destroy(bookmark_id) bookmark = find_bookmark_and_check_access(bookmark_id) bookmark.destroy - if SiteSetting.use_polymorphic_bookmarks - bookmark.registered_bookmarkable.after_destroy(@guardian, bookmark) - else - update_topic_user_bookmarked(bookmark.topic) - end + bookmark.registered_bookmarkable.after_destroy(@guardian, bookmark) bookmark end @@ -134,8 +87,7 @@ class BookmarkManager end def self.send_reminder_notification(id) - bookmark = Bookmark.find_by(id: id) - BookmarkReminderNotificationHandler.new(bookmark).send_notification + BookmarkReminderNotificationHandler.new(Bookmark.find_by(id: id)).send_notification end def update(bookmark_id:, name:, reminder_at:, options: {}) diff --git a/lib/bookmark_query.rb b/lib/bookmark_query.rb index 16f5536d1ee..9cbc7b6abe5 100644 --- a/lib/bookmark_query.rb +++ b/lib/bookmark_query.rb @@ -10,9 +10,7 @@ class BookmarkQuery end def self.preload(bookmarks, object) - if SiteSetting.use_polymorphic_bookmarks - preload_polymorphic_associations(bookmarks) - end + preload_polymorphic_associations(bookmarks) if @preload @preload.each { |preload| preload.call(bookmarks, object) } end @@ -37,34 +35,6 @@ class BookmarkQuery end def list_all - return polymorphic_list_all if SiteSetting.use_polymorphic_bookmarks - - topics = Topic.listable_topics.secured(@guardian) - pms = Topic.private_messages_for_user(@user) - results = list_all_results(topics, pms) - - results = results.order( - "(CASE WHEN bookmarks.pinned THEN 0 ELSE 1 END), - bookmarks.reminder_at ASC, - bookmarks.updated_at DESC" - ) - - if @params[:q].present? - results = search(results, @params[:q]) - end - - if @page.positive? - results = results.offset(@page * @params[:per_page]) - end - - results = results.limit(@limit).to_a - BookmarkQuery.preload(results, self) - results - end - - private - - def polymorphic_list_all search_term = @params[:q] ts_query = search_term.present? ? Search.ts_query(term: search_term) : nil search_term_wildcard = search_term.present? ? "%#{search_term}%" : nil @@ -109,27 +79,4 @@ class BookmarkQuery BookmarkQuery.preload(results, self) results end - - def list_all_results(topics, pms) - results = base_bookmarks.merge(topics.or(pms)) - results = results.merge(Post.secured(@guardian)) - results = @guardian.filter_allowed_categories(results) - results - end - - def base_bookmarks - Bookmark.where(user: @user) - .includes(post: :user) - .includes(post: { topic: :tags }) - .includes(topic: :topic_users) - .references(:post) - .where(topic_users: { user_id: @user.id }) - end - - def search(results, term) - bookmark_ts_query = Search.ts_query(term: term) - results - .joins("LEFT JOIN post_search_data ON post_search_data.post_id = bookmarks.post_id") - .where("bookmarks.name ILIKE :q OR #{bookmark_ts_query} @@ post_search_data.search_data", q: "%#{term}%") - end end diff --git a/lib/bookmark_reminder_notification_handler.rb b/lib/bookmark_reminder_notification_handler.rb index d4dfd901566..13f5b2966e1 100644 --- a/lib/bookmark_reminder_notification_handler.rb +++ b/lib/bookmark_reminder_notification_handler.rb @@ -10,13 +10,10 @@ class BookmarkReminderNotificationHandler def send_notification return if bookmark.blank? Bookmark.transaction do - # 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? + if !bookmark.registered_bookmarkable.can_send_reminder?(bookmark) clear_reminder else - create_notification + bookmark.registered_bookmarkable.send_reminder_notification(bookmark) if bookmark.auto_delete_when_reminder_sent? BookmarkManager.new(bookmark.user).destroy(bookmark.id) @@ -40,29 +37,4 @@ class BookmarkReminderNotificationHandler bookmark.clear_reminder! end - - 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 fb0bc5370ea..a3703ed9ebd 100644 --- a/lib/guardian/bookmark_guardian.rb +++ b/lib/guardian/bookmark_guardian.rb @@ -10,10 +10,6 @@ module BookmarkGuardian end 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) + bookmark.registered_bookmarkable.can_see?(self, bookmark) end end diff --git a/lib/search.rb b/lib/search.rb index 8a92641d399..f2cc711880a 100644 --- a/lib/search.rb +++ b/lib/search.rb @@ -459,16 +459,12 @@ class Search # search based on a RegisteredBookmarkable's #search_query method. advanced_filter(/^in:(bookmarks)$/i) do |posts, match| if @guardian.user - if SiteSetting.use_polymorphic_bookmarks - posts.where(<<~SQL) - posts.id IN ( - SELECT bookmarkable_id FROM bookmarks - WHERE bookmarks.user_id = #{@guardian.user.id} AND bookmarks.bookmarkable_type = 'Post' - ) - SQL - else - posts.where("posts.id IN (SELECT post_id FROM bookmarks WHERE bookmarks.user_id = #{@guardian.user.id})") - end + posts.where(<<~SQL) + posts.id IN ( + SELECT bookmarkable_id FROM bookmarks + WHERE bookmarks.user_id = #{@guardian.user.id} AND bookmarks.bookmarkable_type = 'Post' + ) + SQL end end diff --git a/lib/topic_view.rb b/lib/topic_view.rb index 7589bbe19b9..c3363715d60 100644 --- a/lib/topic_view.rb +++ b/lib/topic_view.rb @@ -413,15 +413,9 @@ class TopicView end def bookmarks - 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 + @bookmarks ||= Bookmark.for_user_in_topic(@user, @topic.id).select( + :id, :bookmarkable_id, :bookmarkable_type, :reminder_at, :name, :auto_delete_preference + ) end MAX_PARTICIPANTS = 24 diff --git a/plugins/discourse-narrative-bot/plugin.rb b/plugins/discourse-narrative-bot/plugin.rb index 989f6ef19eb..c0fa830128a 100644 --- a/plugins/discourse-narrative-bot/plugin.rb +++ b/plugins/discourse-narrative-bot/plugin.rb @@ -281,10 +281,8 @@ after_initialize do self.add_model_callback(Bookmark, :after_commit, on: :create) do if self.user.enqueue_narrative_bot_job? - if SiteSetting.use_polymorphic_bookmarks && self.bookmarkable_type == "Post" + if self.bookmarkable_type == "Post" Jobs.enqueue(:bot_input, user_id: self.user_id, post_id: self.bookmarkable_id, input: "bookmark") - elsif !SiteSetting.use_polymorphic_bookmarks && self.post.present? - Jobs.enqueue(:bot_input, user_id: self.user_id, post_id: self.post_id, input: "bookmark") end end end diff --git a/plugins/discourse-narrative-bot/spec/discourse_narrative_bot/new_user_narrative_spec.rb b/plugins/discourse-narrative-bot/spec/discourse_narrative_bot/new_user_narrative_spec.rb index ccebc8f6fbc..f2aea1e2e10 100644 --- a/plugins/discourse-narrative-bot/spec/discourse_narrative_bot/new_user_narrative_spec.rb +++ b/plugins/discourse-narrative-bot/spec/discourse_narrative_bot/new_user_narrative_spec.rb @@ -249,13 +249,6 @@ describe DiscourseNarrativeBot::NewUserNarrative do end it 'adds an after commit model callback to bookmark' do - Jobs.run_later! - bookmark = Fabricate(:bookmark, post: Fabricate(:post)) - expect_job_enqueued(job: :bot_input, args: { user_id: bookmark.user_id, post_id: bookmark.post_id, input: "bookmark" }) - end - - it 'adds an after commit model callback to bookmark for polymorphic bookmarks (but only for post polymorphic bookmarks)' do - SiteSetting.use_polymorphic_bookmarks = true Jobs.run_later! bookmark = Fabricate(:bookmark, bookmarkable: Fabricate(:post)) expect_job_enqueued(job: :bot_input, args: { user_id: bookmark.user_id, post_id: bookmark.bookmarkable_id, input: "bookmark" }) diff --git a/script/import_scripts/base.rb b/script/import_scripts/base.rb index 67fb5cbc578..1345f206d78 100644 --- a/script/import_scripts/base.rb +++ b/script/import_scripts/base.rb @@ -624,12 +624,7 @@ class ImportScripts::Base else begin manager = BookmarkManager.new(user) - - 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 + bookmark = manager.create_for(bookmarkable_id: post.id, bookmarkable_type: "Post") created += 1 if manager.errors.none? skipped += 1 if manager.errors.any? diff --git a/spec/fabricators/bookmark_fabricator.rb b/spec/fabricators/bookmark_fabricator.rb index 6cbabbfa00f..30dafdf0f11 100644 --- a/spec/fabricators/bookmark_fabricator.rb +++ b/spec/fabricators/bookmark_fabricator.rb @@ -2,28 +2,10 @@ Fabricator(:bookmark) do user - post { - if !SiteSetting.use_polymorphic_bookmarks - Fabricate(:post) - end - } name "This looked interesting" reminder_at { 1.day.from_now.iso8601 } reminder_set_at { Time.zone.now } - bookmarkable { - if SiteSetting.use_polymorphic_bookmarks - Fabricate(:post) - end - } - - # TODO (martin) [POLYBOOK] Not relevant once polymorphic bookmarks are implemented. - before_create do |bookmark| - if bookmark.bookmarkable_id.present? || bookmark.bookmarkable.present? - bookmark.post = nil - bookmark.post_id = nil - bookmark.for_topic = false - end - end + bookmarkable { Fabricate(:post) } end Fabricator(:bookmark_next_business_day_reminder, from: :bookmark) do diff --git a/spec/jobs/bookmark_reminder_notifications_spec.rb b/spec/jobs/bookmark_reminder_notifications_spec.rb index 5953b5a34d5..6a68eb439e7 100644 --- a/spec/jobs/bookmark_reminder_notifications_spec.rb +++ b/spec/jobs/bookmark_reminder_notifications_spec.rb @@ -67,9 +67,9 @@ RSpec.describe Jobs::BookmarkReminderNotifications do end it 'will not send notification when topic is not available' do - bookmark1.topic.destroy - bookmark2.topic.destroy - bookmark3.topic.destroy + bookmark1.bookmarkable.topic.destroy + bookmark2.bookmarkable.topic.destroy + bookmark3.bookmarkable.topic.destroy expect { subject.execute }.not_to change { Notification.where(notification_type: Notification.types[:bookmark_reminder]).count } end end diff --git a/spec/jobs/export_user_archive_spec.rb b/spec/jobs/export_user_archive_spec.rb index 13c582d348a..27b490904d8 100644 --- a/spec/jobs/export_user_archive_spec.rb +++ b/spec/jobs/export_user_archive_spec.rb @@ -280,20 +280,20 @@ describe Jobs::ExportUserArchive do let(:post4) { Fabricate(:post, topic: private_message_topic) } let(:reminder_at) { 1.day.from_now } - it 'properly includes bookmark records' do + it "properly includes bookmark records" do now = freeze_time '2017-03-01 12:00' - bookmark1 = manager.create(post_id: post1.id, name: name) + 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(post_id: post2.id, name: name, reminder_at: reminder_at, options: { auto_delete_preference: Bookmark.auto_delete_preferences[:when_reminder_sent] }) + 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(post_id: post3.id, name: name, reminder_at: now - 8.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(post_id: post4.id, name: name) + manager.create_for(bookmarkable_id: post4.id, bookmarkable_type: "Post", name: name) tau_record.destroy! BookmarkReminderNotificationHandler.new(pending_reminder).send_notification @@ -302,9 +302,8 @@ describe Jobs::ExportUserArchive do expect(data.length).to eq(4) - expect(data[0]['post_id']).to eq(post1.id.to_s) - expect(data[0]['topic_id']).to eq(topic1.id.to_s) - expect(data[0]['post_number']).to eq('5') + 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)) @@ -316,56 +315,10 @@ 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(private_message_topic.id.to_s) + 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 - - 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 3b8b52a1ffd..3938085fe38 100644 --- a/spec/jobs/sync_topic_user_bookmarked_spec.rb +++ b/spec/jobs/sync_topic_user_bookmarked_spec.rb @@ -13,8 +13,8 @@ RSpec.describe Jobs::SyncTopicUserBookmarked do fab!(:tu5) { Fabricate(:topic_user, topic: topic, bookmarked: true) } it "corrects all topic_users.bookmarked records for the topic" do - Fabricate(:bookmark, user: tu1.user, post: topic.posts.sample) - Fabricate(:bookmark, user: tu4.user, post: topic.posts.sample) + Fabricate(:bookmark, user: tu1.user, bookmarkable: topic.posts.sample) + Fabricate(:bookmark, user: tu4.user, bookmarkable: topic.posts.sample) subject.execute(topic_id: topic.id) @@ -26,8 +26,8 @@ RSpec.describe Jobs::SyncTopicUserBookmarked do end it "does not consider topic as bookmarked if the bookmarked post is deleted" do - Fabricate(:bookmark, user: tu1.user, post: post1) - Fabricate(:bookmark, user: tu2.user, post: post1) + Fabricate(:bookmark, user: tu1.user, bookmarkable: post1) + Fabricate(:bookmark, user: tu2.user, bookmarkable: post1) post1.trash! @@ -38,8 +38,8 @@ RSpec.describe Jobs::SyncTopicUserBookmarked do end it "works when no topic id is provided (runs for all topics)" do - Fabricate(:bookmark, user: tu1.user, post: topic.posts.sample) - Fabricate(:bookmark, user: tu4.user, post: topic.posts.sample) + Fabricate(:bookmark, user: tu1.user, bookmarkable: topic.posts.sample) + Fabricate(:bookmark, user: tu4.user, bookmarkable: topic.posts.sample) subject.execute @@ -49,48 +49,4 @@ 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_manager_spec.rb b/spec/lib/bookmark_manager_spec.rb index b44925046be..3b8e9cf75d1 100644 --- a/spec/lib/bookmark_manager_spec.rb +++ b/spec/lib/bookmark_manager_spec.rb @@ -9,175 +9,24 @@ RSpec.describe BookmarkManager do subject { described_class.new(user) } - describe ".create" do - it "creates the bookmark for the user" do - subject.create(post_id: post.id, name: name) - bookmark = Bookmark.find_by(user: user) - - expect(bookmark.post_id).to eq(post.id) - expect(bookmark.topic_id).to eq(post.topic_id) - end - - it "allows creating a bookmark for the topic and for the first post" do - subject.create(post_id: post.id, name: name, for_topic: true) - bookmark = Bookmark.find_by(user: user, post_id: post.id, for_topic: true) - - expect(bookmark.post_id).to eq(post.id) - expect(bookmark.topic_id).to eq(post.topic_id) - expect(bookmark.for_topic).to eq(true) - - subject.create(post_id: post.id, name: name) - bookmark = Bookmark.find_by(user: user, post_id: post.id, for_topic: false) - - expect(bookmark.post_id).to eq(post.id) - expect(bookmark.topic_id).to eq(post.topic_id) - 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" 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" do - post.trash! - expect { subject.create(post_id: post.id, 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(post_id: post.id, name: name, reminder_at: reminder_at) - tu = TopicUser.find_by(user: user) - expect(tu.bookmarked).to eq(true) - tu.update(bookmarked: false) - subject.create(post_id: Fabricate(:post, topic: post.topic).id) - 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(post_id: post.id, name: name, reminder_at: reminder_at) - bookmark = Bookmark.find_by(user: user) - - 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(post_id: post.id, name: name, options: options) - bookmark = Bookmark.find_by(user: user) - - 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(post: post, user: user) - end - - it "adds an error to the manager" do - subject.create(post_id: post.id) - expect(subject.errors.full_messages).to include(I18n.t("bookmarks.errors.already_bookmarked_post")) - end - end - - context "when the bookmark name is too long" do - it "adds an error to the manager" do - subject.create(post_id: post.id, 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(post_id: post.id, 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(post_id: post.id, 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(post_id: post.id, 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(post_id: post.id, name: name) }.to raise_error(Discourse::InvalidAccess) - end - end - - it "saves user's preference" do - subject.create(post_id: post.id, 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, 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 - describe ".destroy" do - let!(:bookmark) { Fabricate(:bookmark, user: user, post: post) } + let!(:bookmark) { Fabricate(:bookmark, user: user, bookmarkable: post) } it "deletes the existing bookmark" do subject.destroy(bookmark.id) expect(Bookmark.exists?(id: bookmark.id)).to eq(false) 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) + TopicUser.create(user: user, topic: post.topic, bookmarked: true) subject.destroy(bookmark.id) tu = TopicUser.find_by(user: user) expect(tu.bookmarked).to eq(false) 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) } + let!(:bookmark) { Fabricate(:bookmark, user: Fabricate(:admin), bookmarkable: post) } it "raises an invalid access error" do expect { subject.destroy(bookmark.id) }.to raise_error(Discourse::InvalidAccess) end @@ -191,7 +40,7 @@ RSpec.describe BookmarkManager do end describe ".update" do - let!(:bookmark) { Fabricate(:bookmark_next_business_day_reminder, user: user, post: post, name: "Old name") } + let!(:bookmark) { Fabricate(:bookmark_next_business_day_reminder, user: user, bookmarkable: post, name: "Old name") } let(:new_name) { "Some new name" } let(:new_reminder_at) { 10.days.from_now } let(:options) { {} } @@ -230,7 +79,7 @@ RSpec.describe BookmarkManager do end context "if the bookmark is belonging to some other user" do - let!(:bookmark) { Fabricate(:bookmark, user: Fabricate(:admin), post: post) } + let!(:bookmark) { Fabricate(:bookmark, user: Fabricate(:admin), bookmarkable: post) } it "raises an invalid access error" do expect { update_bookmark }.to raise_error(Discourse::InvalidAccess) end @@ -247,35 +96,6 @@ RSpec.describe BookmarkManager do end describe ".destroy_for_topic" do - let!(:topic) { Fabricate(:topic) } - let!(:bookmark1) { Fabricate(:bookmark, post: Fabricate(:post, topic: topic), user: user) } - let!(:bookmark2) { Fabricate(:bookmark, post: 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, post: 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 ".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) } @@ -312,7 +132,7 @@ RSpec.describe BookmarkManager do it "creates a notification for the reminder" do described_class.send_reminder_notification(bookmark.id) notif = notifications_for_user.last - expect(notif.post_number).to eq(bookmark.post.post_number) + expect(notif.post_number).to eq(bookmark.bookmarkable.post_number) end context "when the bookmark does no longer exist" do @@ -327,7 +147,7 @@ RSpec.describe BookmarkManager do context "if the post has been deleted" do before do - bookmark.post.trash! + bookmark.bookmarkable.trash! end it "does not error and does not create a notification" do described_class.send_reminder_notification(bookmark.id) @@ -373,11 +193,7 @@ RSpec.describe BookmarkManager do end end - describe "#create_for (use_polymorphic_bookmarks)" do - before do - SiteSetting.use_polymorphic_bookmarks = true - end - + describe "#create_for" do 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) diff --git a/spec/lib/bookmark_query_spec.rb b/spec/lib/bookmark_query_spec.rb index 58cdc1d7933..2855013d290 100644 --- a/spec/lib/bookmark_query_spec.rb +++ b/spec/lib/bookmark_query_spec.rb @@ -13,22 +13,30 @@ RSpec.describe BookmarkQuery do end describe "#list_all" do - fab!(:bookmark1) { Fabricate(:bookmark, user: user) } - fab!(:bookmark2) { Fabricate(:bookmark, user: user) } - let!(:topic_user1) { Fabricate(:topic_user, topic: bookmark1.topic, user: user) } - let!(:topic_user2) { Fabricate(:topic_user, topic: bookmark2.topic, user: user) } + before do + Bookmark.reset_bookmarkables + register_test_bookmarkable + + Fabricate(:topic_user, user: user, topic: post_bookmark.bookmarkable.topic) + Fabricate(:topic_user, user: user, topic: topic_bookmark.bookmarkable) + user_bookmark + end + + let(:post_bookmark) { Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:post)) } + let(:topic_bookmark) { Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:topic)) } + let(:user_bookmark) { Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:user, username: "bookmarkqueen")) } + + after do + Bookmark.reset_bookmarkables + end it "returns all the bookmarks for a user" do - expect(bookmark_query.list_all.count).to eq(2) + expect(bookmark_query.list_all.count).to eq(3) end - it "does not return deleted posts" do - bookmark1.post.trash! - expect(bookmark_query.list_all.count).to eq(1) - end - - it "does not return deleted topics" do - bookmark1.topic.trash! + it "does not return deleted bookmarkables" do + post_bookmark.bookmarkable.trash! + topic_bookmark.bookmarkable.trash! expect(bookmark_query.list_all.count).to eq(1) end @@ -41,149 +49,92 @@ RSpec.describe BookmarkQuery do expect(preloaded_bookmarks.any?).to eq(true) end - it "does not query topic_users for the bookmark topic that are not the current user" do - topic_user3 = Fabricate(:topic_user, topic: bookmark1.topic) - bookmark = bookmark_query.list_all.find do |b| - b.topic_id == bookmark1.topic_id - end - - expect(bookmark.topic.topic_users.map(&:user_id)).to contain_exactly(user.id) + it "returns a mixture of post, topic, and custom bookmarkable type bookmarks" do + bookmarks = bookmark_query.list_all + expect(bookmarks.map(&:id)).to match_array([post_bookmark.id, topic_bookmark.id, user_bookmark.id]) end - context "for polymorphic bookmarks" do - before do - SiteSetting.use_polymorphic_bookmarks = true - Bookmark.reset_bookmarkables - register_test_bookmarkable + it "handles the user not having permission for all of the bookmarks of a certain bookmarkable" do + UserTestBookmarkable.expects(:list_query).returns(nil) + bookmarks = bookmark_query.list_all + expect(bookmarks.map(&:id)).to match_array([post_bookmark.id, topic_bookmark.id]) + end - Fabricate(:topic_user, user: user, topic: post_bookmark.bookmarkable.topic) - Fabricate(:topic_user, user: user, topic: topic_bookmark.bookmarkable) - user_bookmark - end - - let(:post_bookmark) { Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:post)) } - let(:topic_bookmark) { Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:topic)) } - let(:user_bookmark) { Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:user, username: "bookmarkqueen")) } - - after do - Bookmark.reset_bookmarkables - end - - it "returns a mixture of post, topic, and custom bookmarkable type bookmarks" do - bookmarks = bookmark_query.list_all - expect(bookmarks.map(&:id)).to match_array([post_bookmark.id, topic_bookmark.id, user_bookmark.id]) - end - - it "handles the user not having permission for all of the bookmarks of a certain bookmarkable" do - UserTestBookmarkable.expects(:list_query).returns(nil) - bookmarks = bookmark_query.list_all - expect(bookmarks.map(&:id)).to match_array([post_bookmark.id, topic_bookmark.id]) - end - - it "handles the user not having permission to see any of their bookmarks" do - topic_bookmark.bookmarkable.update(category: Fabricate(:private_category, group: Fabricate(:group))) - post_bookmark.bookmarkable.topic.update(category: topic_bookmark.bookmarkable.category) - UserTestBookmarkable.expects(:list_query).returns(nil) - bookmarks = bookmark_query.list_all - expect(bookmarks.map(&:id)).to eq([]) - end + it "handles the user not having permission to see any of their bookmarks" do + topic_bookmark.bookmarkable.update(category: Fabricate(:private_category, group: Fabricate(:group))) + post_bookmark.bookmarkable.topic.update(category: topic_bookmark.bookmarkable.category) + UserTestBookmarkable.expects(:list_query).returns(nil) + bookmarks = bookmark_query.list_all + expect(bookmarks.map(&:id)).to eq([]) end context "when q param is provided" do let!(:post) { Fabricate(:post, raw: "Some post content here", topic: Fabricate(:topic, title: "Bugfix game for devs")) } - context "when not using polymorphic bookmarks" do - let(:bookmark3) { Fabricate(:bookmark, user: user, name: "Check up later") } - let(:bookmark4) { Fabricate(:bookmark, user: user, post: post) } - - before do - Fabricate(:topic_user, user: user, topic: bookmark3.topic) - Fabricate(:topic_user, user: user, topic: bookmark4.topic) - end - - it "can search by bookmark name" do - bookmarks = bookmark_query(params: { q: 'check' }).list_all - expect(bookmarks.map(&:id)).to eq([bookmark3.id]) - end - - it "can search by post content" do - bookmarks = bookmark_query(params: { q: 'content' }).list_all - expect(bookmarks.map(&:id)).to eq([bookmark4.id]) - end - - it "can search by topic title" do - bookmarks = bookmark_query(params: { q: 'bugfix' }).list_all - expect(bookmarks.map(&:id)).to eq([bookmark4.id]) - end + before do + Bookmark.reset_bookmarkables end - context "when using polymorphic bookmarks" do + after do + Bookmark.reset_bookmarkables + end + + let(:bookmark3) { Fabricate(:bookmark, user: user, name: "Check up later", bookmarkable: Fabricate(:post)) } + let(:bookmark4) { Fabricate(:bookmark, user: user, bookmarkable: post) } + + before do + Fabricate(:topic_user, user: user, topic: bookmark3.bookmarkable.topic) + Fabricate(:topic_user, user: user, topic: bookmark4.bookmarkable.topic) + end + + it "can search by bookmark name" do + bookmarks = bookmark_query(params: { q: 'check' }).list_all + expect(bookmarks.map(&:id)).to eq([bookmark3.id]) + end + + it "can search by post content" do + bookmarks = bookmark_query(params: { q: 'content' }).list_all + expect(bookmarks.map(&:id)).to eq([bookmark4.id]) + end + + it "can search by topic title" do + bookmarks = bookmark_query(params: { q: 'bugfix' }).list_all + expect(bookmarks.map(&:id)).to eq([bookmark4.id]) + end + + context "with custom bookmarkable fitering" do before do - SiteSetting.use_polymorphic_bookmarks = true - Bookmark.reset_bookmarkables + register_test_bookmarkable end - after do - Bookmark.reset_bookmarkables - end + let!(:bookmark5) { Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:user, username: "bookmarkking")) } - let(:bookmark3) { Fabricate(:bookmark, user: user, name: "Check up later", bookmarkable: Fabricate(:post)) } - let(:bookmark4) { Fabricate(:bookmark, user: user, bookmarkable: post) } - - before do - Fabricate(:topic_user, user: user, topic: bookmark3.bookmarkable.topic) - Fabricate(:topic_user, user: user, topic: bookmark4.bookmarkable.topic) - end - - it "can search by bookmark name" do - bookmarks = bookmark_query(params: { q: 'check' }).list_all - expect(bookmarks.map(&:id)).to eq([bookmark3.id]) - end - - it "can search by post content" do - bookmarks = bookmark_query(params: { q: 'content' }).list_all - expect(bookmarks.map(&:id)).to eq([bookmark4.id]) - end - - it "can search by topic title" do - bookmarks = bookmark_query(params: { q: 'bugfix' }).list_all - expect(bookmarks.map(&:id)).to eq([bookmark4.id]) - end - - context "with custom bookmarkable fitering" do - before do - register_test_bookmarkable - end - - let!(:bookmark5) { Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:user, username: "bookmarkqueen")) } - - it "allows searching bookmarkables by fields in other tables" do - bookmarks = bookmark_query(params: { q: 'bookmarkq' }).list_all - expect(bookmarks.map(&:id)).to eq([bookmark5.id]) - end + it "allows searching bookmarkables by fields in other tables" do + bookmarks = bookmark_query(params: { q: 'bookmarkk' }).list_all + expect(bookmarks.map(&:id)).to eq([bookmark5.id]) end end end context "for a whispered post" do before do - bookmark1.post.update(post_type: Post.types[:whisper]) + post_bookmark.bookmarkable.update(post_type: Post.types[:whisper]) end context "when the user is moderator" do it "does return the whispered post" do user.update!(moderator: true) - expect(bookmark_query.list_all.count).to eq(2) + expect(bookmark_query.list_all.count).to eq(3) end end context "when the user is admin" do it "does return the whispered post" do user.update!(admin: true) - expect(bookmark_query.list_all.count).to eq(2) + expect(bookmark_query.list_all.count).to eq(3) end end context "when the user is not staff" do it "does not return the whispered post" do - expect(bookmark_query.list_all.count).to eq(1) + expect(bookmark_query.list_all.count).to eq(2) end end end @@ -191,7 +142,7 @@ RSpec.describe BookmarkQuery do context "for a private message topic bookmark" do let(:pm_topic) { Fabricate(:private_message_topic) } before do - bookmark1.update(post: Fabricate(:post, topic: pm_topic)) + post_bookmark.update(bookmarkable: Fabricate(:post, topic: pm_topic)) TopicUser.change(user.id, pm_topic.id, total_msecs_viewed: 1) end @@ -200,7 +151,7 @@ RSpec.describe BookmarkQuery do TopicAllowedUser.create(topic: pm_topic, user: user) end it "shows the user the bookmark in the PM" do - expect(bookmark_query.list_all.map(&:id).count).to eq(2) + expect(bookmark_query.list_all.map(&:id).count).to eq(3) end end @@ -211,19 +162,19 @@ RSpec.describe BookmarkQuery do TopicAllowedGroup.create(topic: pm_topic, group: group) end it "shows the user the bookmark in the PM" do - expect(bookmark_query.list_all.map(&:id).count).to eq(2) + expect(bookmark_query.list_all.map(&:id).count).to eq(3) end end context "when the user is not a topic_allowed_user" do it "does not show the user a bookmarked post in a PM where they are not an allowed user" do - expect(bookmark_query.list_all.map(&:id).count).to eq(1) + expect(bookmark_query.list_all.map(&:id).count).to eq(2) end end context "when the user is not in a topic_allowed_group" do it "does not show the user a bookmarked post in a PM where they are not in an allowed group" do - expect(bookmark_query.list_all.map(&:id).count).to eq(1) + expect(bookmark_query.list_all.map(&:id).count).to eq(2) end end end @@ -231,15 +182,15 @@ RSpec.describe BookmarkQuery do context "when the topic category is private" do let(:group) { Fabricate(:group) } before do - bookmark1.topic.update(category: Fabricate(:private_category, group: group)) - bookmark1.reload + post_bookmark.bookmarkable.topic.update(category: Fabricate(:private_category, group: group)) + post_bookmark.reload end it "does not show the user a post/topic in a private category they cannot see" do - expect(bookmark_query.list_all.map(&:id)).not_to include(bookmark1.id) + expect(bookmark_query.list_all.map(&:id)).not_to include(post_bookmark.id) end it "does show the user a post/topic in a private category they can see" do GroupUser.create(user: user, group: group) - expect(bookmark_query.list_all.map(&:id)).to include(bookmark1.id) + expect(bookmark_query.list_all.map(&:id)).to include(post_bookmark.id) end end @@ -260,7 +211,7 @@ RSpec.describe BookmarkQuery do before do [bookmark1, bookmark2, bookmark3, bookmark4, bookmark5].each do |bm| - Fabricate(:topic_user, topic: bm.topic, user: user) + Fabricate(:topic_user, topic: bm.bookmarkable.topic, user: user) bm.reload end end diff --git a/spec/lib/bookmark_reminder_notification_handler_spec.rb b/spec/lib/bookmark_reminder_notification_handler_spec.rb index 8665c6d934a..cf660394cee 100644 --- a/spec/lib/bookmark_reminder_notification_handler_spec.rb +++ b/spec/lib/bookmark_reminder_notification_handler_spec.rb @@ -11,24 +11,25 @@ RSpec.describe BookmarkReminderNotificationHandler do describe "#send_notification" do let!(:bookmark) do - Fabricate(:bookmark_next_business_day_reminder, user: user) + 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.topic_id) - expect(notif.post_number).to eq(bookmark.post.post_number) + 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["topic_title"]).to eq(bookmark.topic.title) + 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 topic is deleted" do + context "when the bookmarkable is deleted" do before do - bookmark.topic.trash! + bookmark.bookmarkable.trash! bookmark.reload end @@ -38,56 +39,9 @@ RSpec.describe BookmarkReminderNotificationHandler do end end - context "when the post is deleted" do - before do - bookmark.post.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 - - 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) + TopicUser.create!(topic: bookmark.bookmarkable.topic, user: user, bookmarked: true) bookmark.update(auto_delete_preference: Bookmark.auto_delete_preferences[:when_reminder_sent]) end @@ -98,24 +52,24 @@ RSpec.describe BookmarkReminderNotificationHandler do it "changes the TopicUser bookmarked column to false" do subject.new(bookmark).send_notification - expect(TopicUser.find_by(topic: bookmark.topic, user: user).bookmarked).to eq(false) + expect(TopicUser.find_by(topic: bookmark.bookmarkable.topic, user: user).bookmarked).to eq(false) end context "if there are still other bookmarks in the topic" do before do - Fabricate(:bookmark, post: Fabricate(:post, topic: bookmark.topic), user: user) + Fabricate(:bookmark, bookmarkable: Fabricate(:post, topic: bookmark.bookmarkable.topic), user: user) end it "does not change the TopicUser bookmarked column to false" do subject.new(bookmark).send_notification - expect(TopicUser.find_by(topic: bookmark.topic, user: user).bookmarked).to eq(true) + expect(TopicUser.find_by(topic: bookmark.bookmarkable.topic, user: user).bookmarked).to eq(true) end end end context "when the auto_delete_preference is clear_reminder" do before do - TopicUser.create!(topic: bookmark.topic, user: user, bookmarked: true) + TopicUser.create!(topic: bookmark.bookmarkable.topic, user: user, bookmarked: true) bookmark.update(auto_delete_preference: Bookmark.auto_delete_preferences[:clear_reminder]) end @@ -124,14 +78,5 @@ RSpec.describe BookmarkReminderNotificationHandler do expect(Bookmark.find_by(id: bookmark.id).reminder_at).to eq(nil) end end - - context "when the post has been deleted" do - it "does not send a notification" do - bookmark.post.trash! - bookmark.reload - 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 end diff --git a/spec/lib/post_creator_spec.rb b/spec/lib/post_creator_spec.rb index 400076faba1..de5533e5317 100644 --- a/spec/lib/post_creator_spec.rb +++ b/spec/lib/post_creator_spec.rb @@ -807,13 +807,13 @@ describe PostCreator do context "when the user has bookmarks with auto_delete_preference on_owner_reply" do before do - Fabricate(:bookmark, user: user, post: Fabricate(:post, topic: topic), auto_delete_preference: Bookmark.auto_delete_preferences[:on_owner_reply]) - Fabricate(:bookmark, user: user, post: Fabricate(:post, topic: topic), auto_delete_preference: Bookmark.auto_delete_preferences[:on_owner_reply]) + Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:post, topic: topic), auto_delete_preference: Bookmark.auto_delete_preferences[:on_owner_reply]) + Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:post, topic: topic), auto_delete_preference: Bookmark.auto_delete_preferences[:on_owner_reply]) TopicUser.create!(topic: topic, user: user, bookmarked: true) end it "deletes the bookmarks, but not the ones without an auto_delete_preference" do - Fabricate(:bookmark, post: Fabricate(:post, topic: topic), user: user) + Fabricate(:bookmark, bookmarkable: Fabricate(:post, topic: topic), user: user) Fabricate(:bookmark, user: user) creator.create expect(Bookmark.where(user: user).count).to eq(2) diff --git a/spec/lib/search_spec.rb b/spec/lib/search_spec.rb index ddf15b55a02..ab700ea5771 100644 --- a/spec/lib/search_spec.rb +++ b/spec/lib/search_spec.rb @@ -1482,21 +1482,9 @@ describe Search do it "can filter by posts in the user's bookmarks" do expect(search_with_bookmarks.posts.map(&:id)).to eq([]) - Fabricate(:bookmark, user: user, post: bookmark_post1) + bm = Fabricate(:bookmark, user: user, bookmarkable: bookmark_post1) expect(search_with_bookmarks.posts.map(&:id)).to match_array([bookmark_post1.id]) end - - context "using polymorphic bookmarks" do - before do - SiteSetting.use_polymorphic_bookmarks = true - end - - it "can filter by posts in the user's bookmarks" do - expect(search_with_bookmarks.posts.map(&:id)).to eq([]) - bm = Fabricate(:bookmark, user: user, bookmarkable: bookmark_post1) - expect(search_with_bookmarks.posts.map(&:id)).to match_array([bookmark_post1.id]) - end - end end it 'supports pinned' do diff --git a/spec/lib/topic_view_spec.rb b/spec/lib/topic_view_spec.rb index 2e187bd72b4..b8151a4015d 100644 --- a/spec/lib/topic_view_spec.rb +++ b/spec/lib/topic_view_spec.rb @@ -396,9 +396,9 @@ RSpec.describe TopicView 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)) } + let!(:bookmark1) { Fabricate(:bookmark, bookmarkable: Fabricate(:post, topic: topic), user: user) } + let!(:bookmark2) { Fabricate(:bookmark, bookmarkable: Fabricate(:post, topic: topic), user: user) } + let!(:bookmark3) { Fabricate(:bookmark, bookmarkable: Fabricate(:post, topic: topic)) } it "returns all the bookmarks in the topic for a user" do expect(TopicView.new(topic.id, user).bookmarks.pluck(:id)).to match_array( @@ -413,25 +413,26 @@ RSpec.describe TopicView do context "#bookmarks" do let!(:user) { Fabricate(:user) } - let!(:bookmark1) { Fabricate(:bookmark_next_business_day_reminder, post: topic.first_post, user: user) } - let!(:bookmark2) { Fabricate(:bookmark_next_business_day_reminder, post: topic.posts[1], user: user) } + let!(:bookmark1) { Fabricate(:bookmark_next_business_day_reminder, bookmarkable: topic.first_post, user: user) } + let!(:bookmark2) { Fabricate(:bookmark_next_business_day_reminder, bookmarkable: topic.posts[1], user: user) } it "gets the first post bookmark reminder at for the user" do topic_view = TopicView.new(topic.id, user) first, second = topic_view.bookmarks.sort_by(&:id) - expect(first[:post_id]).to eq(bookmark1.post_id) + expect(first[:bookmarkable_id]).to eq(bookmark1.bookmarkable_id) expect(first[:reminder_at]).to eq_time(bookmark1.reminder_at) - expect(second[:post_id]).to eq(bookmark2.post_id) + expect(second[:bookmarkable_id]).to eq(bookmark2.bookmarkable_id) expect(second[:reminder_at]).to eq_time(bookmark2.reminder_at) end context "when the topic is deleted" do it "returns []" do topic_view = TopicView.new(topic, user) + expect(topic_view.bookmarks).to match_array([bookmark1, bookmark2]) PostDestroyer.new(Fabricate(:admin), topic.first_post).destroy topic.reload - + topic_view.instance_variable_set(:@bookmarks, nil) expect(topic_view.bookmarks).to eq([]) end end @@ -444,7 +445,7 @@ RSpec.describe TopicView do expect(topic_view.bookmarks.length).to eq(1) first = topic_view.bookmarks.first - expect(first[:post_id]).to eq(bookmark1.post_id) + expect(first[:bookmarkable_id]).to eq(bookmark1.bookmarkable_id) expect(first[:reminder_at]).to eq_time(bookmark1.reminder_at) end end diff --git a/spec/models/bookmark_spec.rb b/spec/models/bookmark_spec.rb index a5eb5296993..162d2ddacfa 100644 --- a/spec/models/bookmark_spec.rb +++ b/spec/models/bookmark_spec.rb @@ -4,95 +4,36 @@ describe Bookmark do fab!(:post) { Fabricate(:post) } context "validations" do - it "does not allow user to bookmark a post twice, enforces unique bookmark per post, user, and for_topic" do - bookmark = Fabricate(:bookmark, post: post) - user = bookmark.user - - bookmark_2 = Fabricate.build( - :bookmark, - post: post, - user: user - ) - - expect(bookmark_2.valid?).to eq(false) - end - - it "allows a user to bookmark a post twice if it is the first post and for_topic is different" do - post.update!(post_number: 1) - bookmark = Fabricate(:bookmark, post: post, for_topic: false) - user = bookmark.user - - bookmark_2 = Fabricate( - :bookmark, - post: post, - user: user, - for_topic: true - ) - - expect(bookmark_2.valid?).to eq(true) - - bookmark_3 = Fabricate.build( - :bookmark, - post: post, - user: user, - for_topic: true - ) - - expect(bookmark_3.valid?).to eq(false) - end - - describe "polymorphic bookmarks" do - before do - SiteSetting.use_polymorphic_bookmarks = true - end - - after do - Bookmark.registered_bookmarkables = [] - end - - it "does not allow a user to create a bookmark with only one polymorphic column" do - user = Fabricate(:user) - bm = Bookmark.create(bookmarkable_id: post.id, user: user) - 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 - - it "does not allow a user to create a bookmarkable for a type that has not been registered" do - user = Fabricate(:user) - bm = Bookmark.create(bookmarkable_type: "User", bookmarkable: Fabricate(:user), user: user) - expect(bm.errors.full_messages).to include(I18n.t("bookmarks.errors.invalid_bookmarkable", type: "User")) - register_test_bookmarkable - expect(bm.valid?).to eq(true) - end - end - end - - describe "#find_for_topic_by_user" do - it "gets the for_topic bookmark for a user for a specific topic" do + it "does not allow a user to create a bookmark with only one polymorphic column" do user = Fabricate(:user) - post.update!(post_number: 1) - bookmark = Fabricate(:bookmark, user: user) - bookmark_2 = Fabricate(:bookmark, user: user, post: post, for_topic: true) - expect(Bookmark.find_for_topic_by_user(post.topic_id, user.id)).to eq(bookmark_2) - bookmark_2.update!(for_topic: false) - expect(Bookmark.find_for_topic_by_user(post.topic_id, user.id)).to eq(nil) + 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 + + it "does not allow a user to create a bookmarkable for a type that has not been registered" do + user = Fabricate(:user) + bm = Bookmark.create(bookmarkable_type: "User", bookmarkable: Fabricate(:user), user: user) + expect(bm.errors.full_messages).to include(I18n.t("bookmarks.errors.invalid_bookmarkable", type: "User")) + register_test_bookmarkable + expect(bm.valid?).to eq(true) end end describe "#cleanup!" do it "deletes bookmarks attached to a deleted post which has been deleted for > 3 days" do - bookmark = Fabricate(:bookmark, post: post) - bookmark2 = Fabricate(:bookmark, post: Fabricate(:post, topic: post.topic)) + bookmark = Fabricate(:bookmark, bookmarkable: post) + bookmark2 = Fabricate(:bookmark, bookmarkable: Fabricate(:post, topic: post.topic)) post.trash! post.update(deleted_at: 4.days.ago) Bookmark.cleanup! @@ -102,10 +43,10 @@ describe Bookmark do it "runs a SyncTopicUserBookmarked job for all deleted bookmark unique topics to make sure topic_user.bookmarked is in sync" do post2 = Fabricate(:post) - bookmark = Fabricate(:bookmark, post: post) - bookmark2 = Fabricate(:bookmark, post: Fabricate(:post, topic: post.topic)) - bookmark3 = Fabricate(:bookmark, post: post2) - bookmark4 = Fabricate(:bookmark, post: post2) + bookmark = Fabricate(:bookmark, bookmarkable: post) + bookmark2 = Fabricate(:bookmark, bookmarkable: Fabricate(:post, topic: post.topic)) + bookmark3 = Fabricate(:bookmark, bookmarkable: post2) + bookmark4 = Fabricate(:bookmark, bookmarkable: post2) post.trash! post.update(deleted_at: 4.days.ago) post2.trash! @@ -120,8 +61,8 @@ describe Bookmark do end it "deletes bookmarks attached to a deleted topic which has been deleted for > 3 days" do - bookmark = Fabricate(:bookmark, post: post) - bookmark2 = Fabricate(:bookmark, post: Fabricate(:post, topic: post.topic)) + bookmark = Fabricate(:bookmark, bookmarkable: post) + bookmark2 = Fabricate(:bookmark, bookmarkable: Fabricate(:post, topic: post.topic)) bookmark3 = Fabricate(:bookmark) post.topic.trash! post.topic.update(deleted_at: 4.days.ago) @@ -132,7 +73,7 @@ describe Bookmark do end it "does not delete bookmarks attached to posts that are not deleted or that have not met the 3 day grace period" do - bookmark = Fabricate(:bookmark, post: post) + bookmark = Fabricate(:bookmark, bookmarkable: post) bookmark2 = Fabricate(:bookmark) Bookmark.cleanup! expect(Bookmark.find(bookmark.id)).to eq(bookmark) @@ -146,106 +87,51 @@ describe Bookmark do let(:category) { Fabricate(:category) } let(:topic_in_category) { Fabricate(:topic, category: category) } - context "for non-polymorphic bookmarks" do - let!(:bookmark1) { Fabricate(:bookmark, created_at: 1.day.ago) } - let!(:bookmark2) { Fabricate(:bookmark, created_at: 2.days.ago) } - let!(:bookmark3) { Fabricate(:bookmark, created_at: 3.days.ago) } - let!(:bookmark4) { Fabricate(:bookmark, post: Fabricate(:post, topic: topic_in_category), created_at: 3.days.ago) } - let!(:bookmark5) { Fabricate(:bookmark, created_at: 40.days.ago) } + let!(:bookmark1) { Fabricate(:bookmark, created_at: 1.day.ago) } + let!(:bookmark2) { Fabricate(:bookmark, created_at: 2.days.ago) } + let!(:bookmark3) { Fabricate(:bookmark, created_at: 3.days.ago) } + let!(:bookmark4) { Fabricate(:bookmark, bookmarkable: Fabricate(:post, topic: topic_in_category), created_at: 3.days.ago) } + let!(:bookmark5) { Fabricate(:bookmark, created_at: 40.days.ago) } - it "gets the count of bookmarks grouped by date within the last 30 days by default" do - expect(Bookmark.count_per_day).to eq({ - 1.day.ago.to_date => 1, - 2.days.ago.to_date => 1, - 3.days.ago.to_date => 2 - }) - end - - it "respects the start_date option" do - expect(Bookmark.count_per_day(start_date: 1.day.ago - 1.hour)).to eq({ - 1.day.ago.to_date => 1, - }) - end - - it "respects the since_days_ago option" do - expect(Bookmark.count_per_day(since_days_ago: 2)).to eq({ - 1.day.ago.to_date => 1, - }) - end - - it "respects the end_date option" do - expect(Bookmark.count_per_day(end_date: 2.days.ago)).to eq({ - 2.days.ago.to_date => 1, - 3.days.ago.to_date => 2, - }) - end - - it "respects the category_id option" do - expect(Bookmark.count_per_day(category_id: category.id)).to eq({ - 3.days.ago.to_date => 1, - }) - end - - it "does not include deleted posts or topics" do - bookmark4.post.trash! - expect(Bookmark.count_per_day(category_id: category.id)).to eq({}) - bookmark4.post.recover! - bookmark4.topic.trash! - expect(Bookmark.count_per_day(category_id: category.id)).to eq({}) - end + it "gets the count of bookmarks grouped by date within the last 30 days by default" do + expect(Bookmark.count_per_day).to eq({ + 1.day.ago.to_date => 1, + 2.days.ago.to_date => 1, + 3.days.ago.to_date => 2 + }) end - context "for polymorphic bookmarks" do - before do - SiteSetting.use_polymorphic_bookmarks = true - end + it "respects the start_date option" do + expect(Bookmark.count_per_day(start_date: 1.day.ago - 1.hour)).to eq({ + 1.day.ago.to_date => 1, + }) + end - let!(:bookmark1) { Fabricate(:bookmark, created_at: 1.day.ago) } - let!(:bookmark2) { Fabricate(:bookmark, created_at: 2.days.ago) } - let!(:bookmark3) { Fabricate(:bookmark, created_at: 3.days.ago) } - let!(:bookmark4) { Fabricate(:bookmark, bookmarkable: Fabricate(:post, topic: topic_in_category), created_at: 3.days.ago) } - let!(:bookmark5) { Fabricate(:bookmark, created_at: 40.days.ago) } + it "respects the since_days_ago option" do + expect(Bookmark.count_per_day(since_days_ago: 2)).to eq({ + 1.day.ago.to_date => 1, + }) + end - it "gets the count of bookmarks grouped by date within the last 30 days by default" do - expect(Bookmark.count_per_day).to eq({ - 1.day.ago.to_date => 1, - 2.days.ago.to_date => 1, - 3.days.ago.to_date => 2 - }) - end + it "respects the end_date option" do + expect(Bookmark.count_per_day(end_date: 2.days.ago)).to eq({ + 2.days.ago.to_date => 1, + 3.days.ago.to_date => 2, + }) + end - it "respects the start_date option" do - expect(Bookmark.count_per_day(start_date: 1.day.ago - 1.hour)).to eq({ - 1.day.ago.to_date => 1, - }) - end + it "respects the category_id option" do + expect(Bookmark.count_per_day(category_id: category.id)).to eq({ + 3.days.ago.to_date => 1, + }) + end - it "respects the since_days_ago option" do - expect(Bookmark.count_per_day(since_days_ago: 2)).to eq({ - 1.day.ago.to_date => 1, - }) - end - - it "respects the end_date option" do - expect(Bookmark.count_per_day(end_date: 2.days.ago)).to eq({ - 2.days.ago.to_date => 1, - 3.days.ago.to_date => 2, - }) - end - - it "respects the category_id option" do - expect(Bookmark.count_per_day(category_id: category.id)).to eq({ - 3.days.ago.to_date => 1, - }) - end - - it "does not include deleted posts or topics" do - bookmark4.bookmarkable.trash! - expect(Bookmark.count_per_day(category_id: category.id)).to eq({}) - bookmark4.bookmarkable.recover! - bookmark4.bookmarkable.topic.trash! - expect(Bookmark.count_per_day(category_id: category.id)).to eq({}) - end + it "does not include deleted posts or topics" do + bookmark4.bookmarkable.trash! + expect(Bookmark.count_per_day(category_id: category.id)).to eq({}) + bookmark4.bookmarkable.recover! + bookmark4.bookmarkable.topic.trash! + expect(Bookmark.count_per_day(category_id: category.id)).to eq({}) end end diff --git a/spec/models/post_mover_spec.rb b/spec/models/post_mover_spec.rb index 2e5378e7bf6..bec5790145f 100644 --- a/spec/models/post_mover_spec.rb +++ b/spec/models/post_mover_spec.rb @@ -392,30 +392,16 @@ describe PostMover do .to contain_exactly([1, 500], [2, 750]) end - it "updates bookmark topic_ids to the new topic id and does not affect bookmarks for posts not moving" do - some_user = Fabricate(:user) - new_post = Fabricate(:post, topic: p1.topic) - bookmark1 = Fabricate(:bookmark, post: p1, user: some_user) - bookmark2 = Fabricate(:bookmark, post: p4) - bookmark3 = Fabricate(:bookmark, post: p4) - bookmark4 = Fabricate(:bookmark, post: new_post) - new_topic = topic.move_posts(user, [p1.id, p4.id], title: "new testing topic name") - expect(bookmark1.reload.topic_id).to eq(new_topic.id) - expect(bookmark2.reload.topic_id).to eq(new_topic.id) - expect(bookmark3.reload.topic_id).to eq(new_topic.id) - expect(bookmark4.reload.topic_id).to eq(new_post.topic_id) - end - it "makes sure the topic_user.bookmarked value is reflected for users in the source and destination topic" do Jobs.run_immediately! user1 = Fabricate(:user) user2 = Fabricate(:user) - bookmark1 = Fabricate(:bookmark, post: p1, user: user1) - bookmark2 = Fabricate(:bookmark, post: p4, user: user1) + bookmark1 = Fabricate(:bookmark, bookmarkable: p1, user: user1) + bookmark2 = Fabricate(:bookmark, bookmarkable: p4, user: user1) - bookmark3 = Fabricate(:bookmark, post: p3, user: user2) - bookmark4 = Fabricate(:bookmark, post: p4, user: user2) + bookmark3 = Fabricate(:bookmark, bookmarkable: p3, user: user2) + bookmark4 = Fabricate(:bookmark, bookmarkable: p4, user: user2) tu1 = Fabricate( :topic_user, @@ -724,20 +710,6 @@ describe PostMover do expect(Notification.exists?(admin_notification.id)).to eq(true) end - it "updates bookmark topic_ids to the new topic id and does not affect bookmarks for posts not moving" do - some_user = Fabricate(:user) - new_post = Fabricate(:post, topic: p3.topic) - bookmark1 = Fabricate(:bookmark, post: p3, user: some_user) - bookmark2 = Fabricate(:bookmark, post: p3) - bookmark3 = Fabricate(:bookmark, post: p3) - bookmark4 = Fabricate(:bookmark, post: new_post) - topic.move_posts(user, [p3.id], destination_topic_id: destination_topic.id) - expect(bookmark1.reload.topic_id).to eq(destination_topic.id) - expect(bookmark2.reload.topic_id).to eq(destination_topic.id) - expect(bookmark3.reload.topic_id).to eq(destination_topic.id) - expect(bookmark4.reload.topic_id).to eq(new_post.topic_id) - end - context "post timings" do fab!(:some_user) { Fabricate(:user) } diff --git a/spec/models/user_bookmark_list_spec.rb b/spec/models/user_bookmark_list_spec.rb index 1bd6c04c7bd..b03a7141d74 100644 --- a/spec/models/user_bookmark_list_spec.rb +++ b/spec/models/user_bookmark_list_spec.rb @@ -5,44 +5,36 @@ RSpec.describe UserBookmarkList do fab!(:user) { Fabricate(:user) } let(:list) { UserBookmarkList.new(user: user, guardian: Guardian.new(user), params: params) } - context "for non-polymorphic bookmarks" do - before do - 22.times do - bookmark = Fabricate(:bookmark, user: user) - Fabricate(:topic_user, topic: bookmark.topic, user: user) - end - end + before do + register_test_bookmarkable - it "defaults to 20 per page" do - expect(list.per_page).to eq(20) - end - - context "when the per_page param is too high" do - let(:params) { { per_page: 1000 } } - - it "does not allow more than X bookmarks to be requested per page" do - expect(list.load.count).to eq(20) - end - end + Fabricate(:topic_user, user: user, topic: post_bookmark.bookmarkable.topic) + Fabricate(:topic_user, user: user, topic: topic_bookmark.bookmarkable) + user_bookmark end - context "for polymorphic bookmarks" do - before do - SiteSetting.use_polymorphic_bookmarks = true - register_test_bookmarkable + let(:post_bookmark) { Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:post)) } + let(:topic_bookmark) { Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:topic)) } + let(:user_bookmark) { Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:user)) } - Fabricate(:topic_user, user: user, topic: post_bookmark.bookmarkable.topic) - Fabricate(:topic_user, user: user, topic: topic_bookmark.bookmarkable) - user_bookmark - end + it "returns all types of bookmarks" do + list.load + expect(list.bookmarks.map(&:id)).to match_array([post_bookmark.id, topic_bookmark.id, user_bookmark.id]) + end - let(:post_bookmark) { Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:post)) } - let(:topic_bookmark) { Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:topic)) } - let(:user_bookmark) { Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:user)) } + it "defaults to 20 per page" do + expect(list.per_page).to eq(20) + end - it "returns all types of bookmarks" do - list.load - expect(list.bookmarks.map(&:id)).to match_array([post_bookmark.id, topic_bookmark.id, user_bookmark.id]) + context "when the per_page param is too high" do + let(:params) { { per_page: 1000 } } + + it "does not allow more than X bookmarks to be requested per page" do + 22.times do + bookmark = Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:post)) + Fabricate(:topic_user, topic: bookmark.bookmarkable.topic, user: user) + end + expect(list.load.count).to eq(20) end end end diff --git a/spec/requests/admin/users_controller_spec.rb b/spec/requests/admin/users_controller_spec.rb index 2ad2eef4eed..d5f467d6915 100644 --- a/spec/requests/admin/users_controller_spec.rb +++ b/spec/requests/admin/users_controller_spec.rb @@ -308,7 +308,8 @@ RSpec.describe Admin::UsersController do it "also prevents use of any api keys" do api_key = Fabricate(:api_key, user: user) post "/bookmarks.json", params: { - post_id: Fabricate(:post).id + bookmarkable_id: Fabricate(:post).id, + bookmarkable_type: "Post" }, headers: { HTTP_API_KEY: api_key.key } expect(response.status).to eq(200) diff --git a/spec/requests/bookmarks_controller_spec.rb b/spec/requests/bookmarks_controller_spec.rb index 64862fbeb3f..0d2ed96885d 100644 --- a/spec/requests/bookmarks_controller_spec.rb +++ b/spec/requests/bookmarks_controller_spec.rb @@ -17,45 +17,20 @@ describe BookmarksController do RateLimiter.clear_all! post "/bookmarks.json", params: { - post_id: bookmark_post.id, + bookmarkable_id: bookmark_post.id, + bookmarkable_type: "Post", reminder_at: (Time.zone.now + 1.day).iso8601 } expect(response.status).to eq(200) post "/bookmarks.json", params: { - post_id: Fabricate(:post).id + bookmarkable_id: bookmark_post.id, + bookmarkable_type: "Post", } 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, - reminder_type: "tomorrow", - reminder_at: (Time.zone.now + 1.day).iso8601, - for_topic: true - } - expect(response.status).to eq(200) - bookmark = Bookmark.find(response.parsed_body["id"]) - 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, - reminder_type: "tomorrow", - reminder_at: (Time.zone.now + 1.day).iso8601, - for_topic: true - } - expect(response.status).to eq(400) - expect(response.parsed_body['errors']).to include( - I18n.t("bookmarks.errors.for_topic_must_use_first_post") - ) - end - context "if the user reached the max bookmark limit" do before do SiteSetting.max_bookmarks_per_user = 1 @@ -63,11 +38,13 @@ describe BookmarksController do it "returns failed JSON with a 400 error" do post "/bookmarks.json", params: { - post_id: bookmark_post.id, + bookmarkable_id: bookmark_post.id, + bookmarkable_type: "Post", reminder_at: (Time.zone.now + 1.day).iso8601 } post "/bookmarks.json", params: { - post_id: Fabricate(:post).id + bookmarkable_id: bookmark_post.id, + bookmarkable_type: "Post", } expect(response.status).to eq(400) @@ -78,27 +55,8 @@ describe BookmarksController do end end - context "if the user already has bookmarked the post" do + context "if the user already has bookmarked the record" do before do - Fabricate(:bookmark, post: bookmark_post, user: bookmark_user) - end - - it "returns failed JSON with a 400 error" do - post "/bookmarks.json", params: { - post_id: bookmark_post.id, - 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_post") - ) - 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 @@ -130,7 +88,7 @@ describe BookmarksController do end describe "#destroy" do - let!(:bookmark) { Fabricate(:bookmark, post: bookmark_post, user: bookmark_user) } + let!(:bookmark) { Fabricate(:bookmark, bookmarkable: bookmark_post, user: bookmark_user) } it "destroys the bookmark" do delete "/bookmarks/#{bookmark.id}.json" @@ -141,33 +99,14 @@ describe BookmarksController 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)) + 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) - 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 + 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 context "if the bookmark has already been destroyed" do diff --git a/spec/requests/posts_controller_spec.rb b/spec/requests/posts_controller_spec.rb index c5b50cbe0f8..c7c1eb0a3f1 100644 --- a/spec/requests/posts_controller_spec.rb +++ b/spec/requests/posts_controller_spec.rb @@ -649,7 +649,7 @@ describe PostsController do describe "#destroy_bookmark" do fab!(:post) { Fabricate(:post) } - fab!(:bookmark) { Fabricate(:bookmark, user: user, post: post) } + fab!(:bookmark) { Fabricate(:bookmark, user: user, bookmarkable: post) } before do sign_in(user) @@ -663,7 +663,7 @@ describe PostsController do context "when the user still has bookmarks in the topic" do before do - Fabricate(:bookmark, user: user, post: Fabricate(:post, topic: post.topic)) + Fabricate(:bookmark, user: user, bookmarkable: Fabricate(:post, topic: post.topic)) end it "marks topic_bookmarked as true" do delete "/posts/#{post.id}/bookmark.json" diff --git a/spec/requests/topics_controller_spec.rb b/spec/requests/topics_controller_spec.rb index 3f14d1c8c96..8b13a2607fe 100644 --- a/spec/requests/topics_controller_spec.rb +++ b/spec/requests/topics_controller_spec.rb @@ -3273,8 +3273,8 @@ RSpec.describe TopicsController do post = create_post post2 = create_post(topic_id: post.topic_id) - Fabricate(:bookmark, user: user, post: post) - Fabricate(:bookmark, user: user, post: post2) + Fabricate(:bookmark, user: user, bookmarkable: post) + Fabricate(:bookmark, user: user, bookmarkable: post2) put "/t/#{post.topic_id}/remove_bookmarks.json" expect(Bookmark.where(user: user).count).to eq(0) @@ -3292,7 +3292,7 @@ RSpec.describe TopicsController do it "deletes all the bookmarks for the user in the topic" do sign_in(user) post = create_post - Fabricate(:bookmark, post: post, user: user) + Fabricate(:bookmark, bookmarkable: post, user: user) put "/t/#{post.topic_id}/remove_bookmarks.json" expect(Bookmark.for_user_in_topic(user.id, post.topic_id).count).to eq(0) end @@ -3304,42 +3304,21 @@ RSpec.describe TopicsController do sign_in(user) end - it "should create a new bookmark on the first post of the topic" do + it "should create a new bookmark for the topic" do post = create_post post2 = create_post(topic_id: post.topic_id) put "/t/#{post.topic_id}/bookmark.json" - expect(Bookmark.find_by(user_id: user.id).post_id).to eq(post.id) + expect(Bookmark.find_by(user_id: user.id).bookmarkable_id).to eq(post.topic_id) end it "errors if the topic is already bookmarked for the user" do post = create_post - Bookmark.create(post: post, user: user) + Bookmark.create(bookmarkable: post.topic, user: user) put "/t/#{post.topic_id}/bookmark.json" expect(response.status).to eq(400) end - - context "bookmarks with reminders" do - it "should create a new bookmark on the first post of the topic" do - post = create_post - post2 = create_post(topic_id: post.topic_id) - put "/t/#{post.topic_id}/bookmark.json" - expect(response.status).to eq(200) - - bookmarks_for_topic = Bookmark.for_user_in_topic(user.id, post.topic_id) - expect(bookmarks_for_topic.count).to eq(1) - expect(bookmarks_for_topic.first.post_id).to eq(post.id) - end - - it "errors if the topic is already bookmarked for the user" do - post = create_post - Bookmark.create(post: post, user: user) - - put "/t/#{post.topic_id}/bookmark.json" - expect(response.status).to eq(400) - end - end end describe '#reset_new' do diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index 9e90df0a79a..fa8396c389a 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -5240,125 +5240,54 @@ describe UsersController do end describe "#bookmarks" do - context "when polymorphic bookmarks are not used" do - let(:bookmark1) { Fabricate(:bookmark, user: user1) } - let(:bookmark2) { Fabricate(:bookmark, user: user1) } - let(:bookmark3) { Fabricate(:bookmark) } - before do - TopicUser.change(user1.id, bookmark1.topic_id, total_msecs_viewed: 1) - TopicUser.change(user1.id, bookmark2.topic_id, total_msecs_viewed: 1) - bookmark3 - end - - it "returns a list of serialized bookmarks for the user" do - sign_in(user1) - get "/u/#{user1.username}/bookmarks.json" - expect(response.status).to eq(200) - expect(response.parsed_body['user_bookmark_list']['bookmarks'].map { |b| b['id'] }).to match_array( - [bookmark1.id, bookmark2.id] - ) - end - - it "returns an .ics file of bookmark reminders for the user in date order" do - bookmark1.update!(name: nil, reminder_at: 1.day.from_now) - bookmark2.update!(name: "Some bookmark note", reminder_at: 1.week.from_now) - - sign_in(user1) - get "/u/#{user1.username}/bookmarks.ics" - expect(response.status).to eq(200) - expect(response.body).to eq(<<~ICS) - BEGIN:VCALENDAR - VERSION:2.0 - PRODID:-//Discourse//#{Discourse.current_hostname}//#{Discourse.full_version}//EN - BEGIN:VEVENT - UID:bookmark_reminder_##{bookmark1.id}@#{Discourse.current_hostname} - DTSTAMP:#{bookmark1.updated_at.strftime(I18n.t("datetime_formats.formats.calendar_ics"))} - DTSTART:#{bookmark1.reminder_at_ics} - DTEND:#{bookmark1.reminder_at_ics(offset: 1.hour)} - SUMMARY:#{bookmark1.topic.title} - DESCRIPTION:#{Discourse.base_url}/t/-/#{bookmark1.topic_id} - URL:#{Discourse.base_url}/t/-/#{bookmark1.topic_id} - END:VEVENT - BEGIN:VEVENT - UID:bookmark_reminder_##{bookmark2.id}@#{Discourse.current_hostname} - DTSTAMP:#{bookmark2.updated_at.strftime(I18n.t("datetime_formats.formats.calendar_ics"))} - DTSTART:#{bookmark2.reminder_at_ics} - DTEND:#{bookmark2.reminder_at_ics(offset: 1.hour)} - SUMMARY:Some bookmark note - DESCRIPTION:#{Discourse.base_url}/t/-/#{bookmark2.topic_id} - URL:#{Discourse.base_url}/t/-/#{bookmark2.topic_id} - END:VEVENT - END:VCALENDAR - ICS - end - - it "does not show another user's bookmarks" do - sign_in(user1) - get "/u/#{bookmark3.user.username}/bookmarks.json" - expect(response.status).to eq(403) - end - - it "shows a helpful message if no bookmarks are found" do - bookmark1.destroy - bookmark2.destroy - bookmark3.destroy - sign_in(user1) - get "/u/#{user1.username}/bookmarks.json" - expect(response.status).to eq(200) - expect(response.parsed_body['bookmarks']).to eq([]) - end - - it "shows a helpful message if no bookmarks are found for the search" do - sign_in(user1) - get "/u/#{user1.username}/bookmarks.json", params: { - q: 'badsearch' - } - expect(response.status).to eq(200) - expect(response.parsed_body['bookmarks']).to eq([]) - end + before do + 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) + bookmark3 && bookmark4 end - context "for polymorphic bookmarks" do - before do - SiteSetting.use_polymorphic_bookmarks = true - 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) - bookmark3 && bookmark4 - end + after do + Bookmark.registered_bookmarkables = [] + end - after do - Bookmark.registered_bookmarkables = [] - end + let(:bookmark1) { Fabricate(:bookmark, user: user1, bookmarkable: Fabricate(:post)) } + let(:bookmark2) { Fabricate(:bookmark, user: user1, bookmarkable: Fabricate(:topic)) } + let(:bookmark3) { Fabricate(:bookmark, user: user1, bookmarkable: Fabricate(:user)) } + let(:bookmark4) { Fabricate(:bookmark) } - let(:bookmark1) { Fabricate(:bookmark, user: user1, bookmarkable: Fabricate(:post)) } - let(:bookmark2) { Fabricate(:bookmark, user: user1, bookmarkable: Fabricate(:topic)) } - let(:bookmark3) { Fabricate(:bookmark, user: user1, bookmarkable: Fabricate(:user)) } - let(:bookmark4) { Fabricate(:bookmark) } + it "returns a list of serialized bookmarks for the user" do + sign_in(user1) + get "/u/#{user1.username}/bookmarks.json" + expect(response.status).to eq(200) + expect(response.parsed_body['user_bookmark_list']['bookmarks'].map { |b| b['id'] }).to match_array( + [bookmark1.id, bookmark2.id, bookmark3.id] + ) + end - it "returns a list of serialized bookmarks for the user including custom registered bookmarkables" do - sign_in(user1) - bookmark3.bookmarkable.user_profile.update!(bio_raw: "

Something cooked

") - bookmark3.bookmarkable.user_profile.rebake! - get "/u/#{user1.username}/bookmarks.json" - expect(response.status).to eq(200) - response_bookmarks = response.parsed_body['user_bookmark_list']['bookmarks'] - expect(response_bookmarks.map { |b| b['id'] }).to match_array( - [bookmark1.id, bookmark2.id, bookmark3.id] - ) - expect(response_bookmarks.find { |b| b['id'] == bookmark3.id }['excerpt']).to eq('Something cooked') - end + it "returns a list of serialized bookmarks for the user including custom registered bookmarkables" do + sign_in(user1) + bookmark3.bookmarkable.user_profile.update!(bio_raw: "

Something cooked

") + bookmark3.bookmarkable.user_profile.rebake! + get "/u/#{user1.username}/bookmarks.json" + expect(response.status).to eq(200) + response_bookmarks = response.parsed_body['user_bookmark_list']['bookmarks'] + expect(response_bookmarks.map { |b| b['id'] }).to match_array( + [bookmark1.id, bookmark2.id, bookmark3.id] + ) + expect(response_bookmarks.find { |b| b['id'] == bookmark3.id }['excerpt']).to eq('Something cooked') + end - it "returns an .ics file of bookmark reminders for the user in date order" do - bookmark1.update!(name: nil, reminder_at: 1.day.from_now) - bookmark2.update!(name: "Some bookmark note", reminder_at: 1.week.from_now) - bookmark3.update!(name: nil, reminder_at: 2.weeks.from_now) + it "returns an .ics file of bookmark reminders for the user in date order" do + bookmark1.update!(name: nil, reminder_at: 1.day.from_now) + bookmark2.update!(name: "Some bookmark note", reminder_at: 1.week.from_now) + bookmark3.update!(name: nil, reminder_at: 2.weeks.from_now) - sign_in(user1) - get "/u/#{user1.username}/bookmarks.ics" - expect(response.status).to eq(200) - expect(response.body.chomp).to eq(<<~ICS) + sign_in(user1) + get "/u/#{user1.username}/bookmarks.ics" + expect(response.status).to eq(200) + expect(response.body).to eq(<<~ICS) BEGIN:VCALENDAR VERSION:2.0 PRODID:-//Discourse//#{Discourse.current_hostname}//#{Discourse.full_version}//EN @@ -5390,8 +5319,32 @@ describe UsersController do URL:#{Discourse.base_url}/u/#{bookmark3.bookmarkable.username} END:VEVENT END:VCALENDAR - ICS - end + ICS + end + + it "does not show another user's bookmarks" do + sign_in(Fabricate(:user)) + get "/u/#{bookmark3.user.username}/bookmarks.json" + expect(response.status).to eq(403) + end + + it "shows a helpful message if no bookmarks are found" do + bookmark1.destroy + bookmark2.destroy + bookmark3.destroy + sign_in(user1) + get "/u/#{user1.username}/bookmarks.json" + expect(response.status).to eq(200) + expect(response.parsed_body['bookmarks']).to eq([]) + end + + it "shows a helpful message if no bookmarks are found for the search" do + sign_in(user1) + get "/u/#{user1.username}/bookmarks.json", params: { + q: 'badsearch' + } + expect(response.status).to eq(200) + expect(response.parsed_body['bookmarks']).to eq([]) end end diff --git a/spec/script/import_scripts/base_spec.rb b/spec/script/import_scripts/base_spec.rb index 4b55ecb9d99..030e2cd3359 100644 --- a/spec/script/import_scripts/base_spec.rb +++ b/spec/script/import_scripts/base_spec.rb @@ -49,26 +49,12 @@ describe ImportScripts::Base do it "creates bookmarks, posts, and users" do MockSpecImporter.new(import_data).perform - expect(Bookmark.count).to eq(5) + 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 - 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/post_serializer_spec.rb b/spec/serializers/post_serializer_spec.rb index 57db8590f32..ac049c13d70 100644 --- a/spec/serializers/post_serializer_spec.rb +++ b/spec/serializers/post_serializer_spec.rb @@ -254,7 +254,7 @@ describe PostSerializer do end context "when a Bookmark record exists for the user on the post" do - let!(:bookmark) { Fabricate(:bookmark_next_business_day_reminder, user: current_user, post: post) } + let!(:bookmark) { Fabricate(:bookmark_next_business_day_reminder, user: current_user, bookmarkable: post) } context "bookmarks with reminders" do it "returns true" do @@ -266,17 +266,6 @@ describe PostSerializer do end end end - - context "when the post bookmark is for_topic" do - let!(:bookmark) { Fabricate(:bookmark_next_business_day_reminder, user: current_user, post: post, for_topic: true) } - - context "bookmarks with reminders" do - it "returns false, because we do not want to mark the post as bookmarked, because the bookmark is for the topic" do - expect(serialized.as_json[:bookmarked]).to eq(false) - expect(serialized.as_json[:bookmark_reminder_at]).to eq(nil) - end - end - end end context "posts when group moderation is enabled" do diff --git a/spec/serializers/user_bookmark_list_serializer_spec.rb b/spec/serializers/user_bookmark_list_serializer_spec.rb index d7388786b07..df14e2f6c30 100644 --- a/spec/serializers/user_bookmark_list_serializer_spec.rb +++ b/spec/serializers/user_bookmark_list_serializer_spec.rb @@ -6,7 +6,6 @@ RSpec.describe UserBookmarkListSerializer do context "for polymorphic bookmarks" do before do - SiteSetting.use_polymorphic_bookmarks = true 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/serializers/user_bookmark_serializer_spec.rb b/spec/serializers/user_bookmark_serializer_spec.rb deleted file mode 100644 index b88903d44a5..00000000000 --- a/spec/serializers/user_bookmark_serializer_spec.rb +++ /dev/null @@ -1,82 +0,0 @@ -# frozen_string_literal: true - -RSpec.describe UserBookmarkSerializer do - let(:user) { Fabricate(:user) } - let(:post) { Fabricate(:post, user: user) } - let!(:bookmark) { Fabricate(:bookmark, name: 'Test', user: user, post: post) } - - it "serializes all properties correctly" do - s = UserBookmarkSerializer.new(bookmark, scope: Guardian.new(user)) - - expect(s.id).to eq(bookmark.id) - expect(s.created_at).to eq_time(bookmark.created_at) - expect(s.topic_id).to eq(bookmark.topic_id) - expect(s.linked_post_number).to eq(bookmark.post.post_number) - expect(s.post_id).to eq(bookmark.post_id) - expect(s.name).to eq(bookmark.name) - expect(s.reminder_at).to eq_time(bookmark.reminder_at) - expect(s.title).to eq(bookmark.topic.title) - expect(s.deleted).to eq(false) - expect(s.hidden).to eq(false) - expect(s.closed).to eq(false) - expect(s.archived).to eq(false) - expect(s.category_id).to eq(bookmark.topic.category_id) - expect(s.archetype).to eq(bookmark.topic.archetype) - expect(s.highest_post_number).to eq(1) - expect(s.bumped_at).to eq_time(bookmark.topic.bumped_at) - expect(s.slug).to eq(bookmark.topic.slug) - expect(s.post_user_username).to eq(bookmark.post.user.username) - expect(s.post_user_name).to eq(bookmark.post.user.name) - expect(s.post_user_avatar_template).not_to eq(nil) - expect(s.excerpt).to eq(PrettyText.excerpt(post.cooked, 300, keep_emoji_images: true)) - end - - it "uses the correct highest_post_number column based on whether the user is staff" do - Fabricate(:post, topic: bookmark.topic) - Fabricate(:post, topic: bookmark.topic) - Fabricate(:whisper, topic: bookmark.topic) - bookmark.reload - serializer = UserBookmarkSerializer.new(bookmark, scope: Guardian.new(user)) - - expect(serializer.highest_post_number).to eq(3) - - user.update!(admin: true) - - expect(serializer.highest_post_number).to eq(4) - end - - context "for_topic bookmarks" do - before do - bookmark.update!(for_topic: true) - end - - it "uses the last_read_post_number + 1 for the for_topic bookmarks excerpt" do - next_unread_post = Fabricate(:post_with_long_raw_content, topic: bookmark.topic) - Fabricate(:post_with_external_links, topic: bookmark.topic) - bookmark.reload - TopicUser.change(user.id, bookmark.topic.id, { last_read_post_number: post.post_number }) - serializer = UserBookmarkSerializer.new(bookmark, scope: Guardian.new(user)) - expect(serializer.excerpt).to eq(PrettyText.excerpt(next_unread_post.cooked, 300, keep_emoji_images: true)) - end - - it "does not use a small post for the last unread cooked post" do - small_action_post = Fabricate(:small_action, topic: bookmark.topic) - next_unread_post = Fabricate(:post_with_long_raw_content, topic: bookmark.topic) - Fabricate(:post_with_external_links, topic: bookmark.topic) - bookmark.reload - TopicUser.change(user.id, bookmark.topic.id, { last_read_post_number: post.post_number }) - serializer = UserBookmarkSerializer.new(bookmark, scope: Guardian.new(user)) - expect(serializer.excerpt).to eq(PrettyText.excerpt(next_unread_post.cooked, 300, keep_emoji_images: true)) - end - - it "handles the last read post in the topic being a small post by getting the last read regular post" do - last_regular_post = Fabricate(:post_with_long_raw_content, topic: bookmark.topic) - small_action_post = Fabricate(:small_action, topic: bookmark.topic) - bookmark.reload - TopicUser.change(user.id, bookmark.topic.id, { last_read_post_number: small_action_post.post_number }) - serializer = UserBookmarkSerializer.new(bookmark, scope: Guardian.new(user)) - expect(serializer.cooked).to eq(last_regular_post.cooked) - expect(serializer.excerpt).to eq(PrettyText.excerpt(last_regular_post.cooked, 300, keep_emoji_images: true)) - end - end -end diff --git a/spec/serializers/user_post_bookmark_serializer_spec.rb b/spec/serializers/user_post_bookmark_serializer_spec.rb index 2e678c4e969..f0d55f868a0 100644 --- a/spec/serializers/user_post_bookmark_serializer_spec.rb +++ b/spec/serializers/user_post_bookmark_serializer_spec.rb @@ -1,10 +1,6 @@ # frozen_string_literal: true RSpec.describe UserPostBookmarkSerializer do - before do - SiteSetting.use_polymorphic_bookmarks = true - end - let(:user) { Fabricate(:user) } let(:post) { Fabricate(:post, user: user, topic: topic) } let(:topic) { Fabricate(:topic) } diff --git a/spec/serializers/user_topic_bookmark_serializer_spec.rb b/spec/serializers/user_topic_bookmark_serializer_spec.rb index 34f332e6edb..9f4534c5bee 100644 --- a/spec/serializers/user_topic_bookmark_serializer_spec.rb +++ b/spec/serializers/user_topic_bookmark_serializer_spec.rb @@ -1,10 +1,6 @@ # frozen_string_literal: true RSpec.describe UserTopicBookmarkSerializer do - before do - SiteSetting.use_polymorphic_bookmarks = true - end - fab!(:user) { Fabricate(:user) } let!(:topic) { Fabricate(:topic, user: user) } let!(:post) { Fabricate(:post, topic: topic) } diff --git a/spec/services/post_bookmarkable_spec.rb b/spec/services/post_bookmarkable_spec.rb index 076f3667bb1..9f0d0397515 100644 --- a/spec/services/post_bookmarkable_spec.rb +++ b/spec/services/post_bookmarkable_spec.rb @@ -7,10 +7,6 @@ describe PostBookmarkable do 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") } diff --git a/spec/services/topic_bookmarkable_spec.rb b/spec/services/topic_bookmarkable_spec.rb index 68128a5726b..46368592f8c 100644 --- a/spec/services/topic_bookmarkable_spec.rb +++ b/spec/services/topic_bookmarkable_spec.rb @@ -7,10 +7,6 @@ describe TopicBookmarkable do 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) }