From 0c42a1e5f3ea55090936551329cac4af111ca7f8 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Tue, 21 Sep 2021 08:45:47 +1000 Subject: [PATCH] FEATURE: Topic-level bookmarks (#14353) Allows creating a bookmark with the `for_topic` flag introduced in https://github.com/discourse/discourse/commit/d1d2298a4cee0b021db66ff116a665ee8ae27111 set to true. This happens when clicking on the Bookmark button in the topic footer when no other posts are bookmarked. In a later PR, when clicking on these topic-level bookmarks the user will be taken to the last unread post in the topic, not the OP. Only the OP can have a topic level bookmark, and users can also make a post-level bookmark on the OP of the topic. I had to do some pretty heavy refactors because most of the bookmark code in the JS topics controller was centred around instances of Post JS models, but the topic level bookmark is not centred around a post. Some refactors were just for readability as well. Also removes some missed reminderType code from the purge in https://github.com/discourse/discourse/commit/41e19adb0d7685303009650efec3ebec0319c577 --- .../discourse/app/components/bookmark.js | 23 +- .../discourse/app/controllers/topic.js | 215 +++++++++++------- .../app/initializers/topic-footer-buttons.js | 29 +-- .../javascripts/discourse/app/models/post.js | 6 +- .../javascripts/discourse/app/models/topic.js | 19 +- .../tests/acceptance/bookmarks-test.js | 98 ++++++-- .../tests/acceptance/topic-timeline-test.js | 5 +- .../discourse/tests/fixtures/poll.js | 10 +- .../fixtures/private-messages-fixtures.js | 1 + .../discourse/tests/fixtures/topic.js | 26 +++ app/controllers/bookmarks_controller.rb | 1 + app/models/bookmark.rb | 17 +- app/serializers/post_serializer.rb | 4 +- app/serializers/topic_view_serializer.rb | 6 +- app/serializers/user_bookmark_serializer.rb | 3 +- .../web_hook_topic_view_serializer.rb | 2 +- config/locales/client.en.yml | 3 + config/locales/server.en.yml | 1 + lib/bookmark_manager.rb | 38 +++- lib/topic_view.rb | 14 +- .../initializers/new-user-narrative.js.es6 | 6 +- .../poll-in-reply-history-test.js.es6 | 5 + .../acceptance/poll-quote-test.js.es6 | 5 + .../acceptance/poll-results-test.js.es6 | 7 + spec/components/topic_view_spec.rb | 12 +- spec/lib/bookmark_manager_spec.rb | 23 +- spec/requests/bookmarks_controller_spec.rb | 25 ++ spec/serializers/post_serializer_spec.rb | 14 +- 28 files changed, 446 insertions(+), 172 deletions(-) diff --git a/app/assets/javascripts/discourse/app/components/bookmark.js b/app/assets/javascripts/discourse/app/components/bookmark.js index 25157f2eaca..b6354419d66 100644 --- a/app/assets/javascripts/discourse/app/components/bookmark.js +++ b/app/assets/javascripts/discourse/app/components/bookmark.js @@ -167,25 +167,13 @@ export default Component.extend({ localStorage.bookmarkDeleteOption = this.autoDeletePreference; - let reminderType; - if (this.selectedReminderType === TIME_SHORTCUT_TYPES.NONE) { - reminderType = null; - } else if ( - this.selectedReminderType === TIME_SHORTCUT_TYPES.LAST_CUSTOM || - this.selectedReminderType === TIME_SHORTCUT_TYPES.POST_LOCAL_DATE - ) { - reminderType = TIME_SHORTCUT_TYPES.CUSTOM; - } else { - reminderType = this.selectedReminderType; - } - const data = { - reminder_type: reminderType, reminder_at: reminderAtISO, name: this.model.name, post_id: this.model.postId, id: this.model.id, auto_delete_preference: this.autoDeletePreference, + for_topic: this.model.forTopic, }; if (this.editingExistingBookmark) { @@ -207,9 +195,10 @@ export default Component.extend({ return; } this.afterSave({ - reminderAt: reminderAtISO, - reminderType: this.selectedReminderType, - autoDeletePreference: this.autoDeletePreference, + reminder_at: reminderAtISO, + for_topic: this.model.forTopic, + auto_delete_preference: this.autoDeletePreference, + post_id: this.model.postId, id: this.model.id || response.id, name: this.model.name, }); @@ -220,7 +209,7 @@ export default Component.extend({ type: "DELETE", }).then((response) => { if (this.afterDelete) { - this.afterDelete(response.topic_bookmarked); + this.afterDelete(response.topic_bookmarked, this.model.id); } }); }, diff --git a/app/assets/javascripts/discourse/app/controllers/topic.js b/app/assets/javascripts/discourse/app/controllers/topic.js index d41257b8afd..42db37f14f6 100644 --- a/app/assets/javascripts/discourse/app/controllers/topic.js +++ b/app/assets/javascripts/discourse/app/controllers/topic.js @@ -215,15 +215,23 @@ export default Controller.extend(bufferedProperty("model"), { if (posts) { posts .filter( - (p) => - p.bookmarked && - p.bookmark_auto_delete_preference === + (post) => + post.bookmarked && + post.bookmark_auto_delete_preference === AUTO_DELETE_PREFERENCES.ON_OWNER_REPLY ) - .forEach((p) => { - p.clearBookmark(); + .forEach((post) => { + post.clearBookmark(); + this.model.removeBookmark(post.bookmark_id); }); } + const forTopicBookmark = this.model.bookmarks.findBy("for_topic", true); + if ( + forTopicBookmark?.auto_delete_preference === + AUTO_DELETE_PREFERENCES.ON_OWNER_REPLY + ) { + this.model.removeBookmark(forTopicBookmark.id); + } }, _forceRefreshPostStream() { @@ -723,9 +731,15 @@ export default Controller.extend(bufferedProperty("model"), { if (!this.currentUser) { return bootbox.alert(I18n.t("bookmarks.not_bookmarked")); } else if (post) { - return this._togglePostBookmark(post); + const bookmarkForPost = this.model.bookmarks.find( + (bookmark) => bookmark.post_id === post.id && !bookmark.for_topic + ); + return this._modifyPostBookmark( + bookmarkForPost || { post_id: post.id, for_topic: false }, + post + ); } else { - return this._toggleTopicBookmark(this.model).then((changedIds) => { + return this._toggleTopicLevelBookmark().then((changedIds) => { if (!changedIds) { return; } @@ -1189,110 +1203,151 @@ export default Controller.extend(bufferedProperty("model"), { } }, - _togglePostBookmark(post) { + _modifyTopicBookmark(bookmark) { + const title = bookmark.id + ? "post.bookmarks.edit_for_topic" + : "post.bookmarks.create_for_topic"; + return this._openBookmarkModal(bookmark, title, { + onAfterSave: () => { + this.model.set("bookmarked", true); + this.model.incrementProperty("bookmarksWereChanged"); + }, + }); + }, + + _modifyPostBookmark(bookmark, post) { + const title = bookmark.id ? "post.bookmarks.edit" : "post.bookmarks.create"; + return this._openBookmarkModal(bookmark, title, { + onCloseWithoutSaving: () => { + post.appEvents.trigger("post-stream:refresh", { + id: bookmark.post_id, + }); + }, + onAfterSave: (savedData) => { + post.createBookmark(savedData); + this.model.afterPostBookmarked(post, savedData); + return [post.id]; + }, + onAfterDelete: (topicBookmarked) => { + post.deleteBookmark(topicBookmarked); + }, + }); + }, + + _openBookmarkModal( + bookmark, + title, + callbacks = { + onCloseWithoutSaving: null, + onAfterSave: null, + onAfterDelete: null, + } + ) { return new Promise((resolve) => { let modalController = showModal("bookmark", { model: { - postId: post.id, - id: post.bookmark_id, - reminderAt: post.bookmark_reminder_at, - autoDeletePreference: post.bookmark_auto_delete_preference, - name: post.bookmark_name, + postId: bookmark.post_id, + id: bookmark.id, + reminderAt: bookmark.reminder_at, + autoDeletePreference: bookmark.auto_delete_preference, + name: bookmark.name, + forTopic: bookmark.for_topic, }, - title: post.bookmark_id - ? "post.bookmarks.edit" - : "post.bookmarks.create", + title, modalClass: "bookmark-with-reminder", }); modalController.setProperties({ onCloseWithoutSaving: () => { - resolve({ closedWithoutSaving: true }); - post.appEvents.trigger("post-stream:refresh", { id: post.id }); + if (callbacks.onCloseWithoutSaving) { + callbacks.onCloseWithoutSaving(); + } + resolve(); }, afterSave: (savedData) => { - this._addOrUpdateBookmarkedPost(post.id, savedData.reminderAt); - post.createBookmark(savedData); - resolve({ closedWithoutSaving: false }); + this._syncBookmarks(savedData); + this.model.set("bookmarking", false); + let resolveData; + if (callbacks.onAfterSave) { + resolveData = callbacks.onAfterSave(savedData); + } + resolve(resolveData); }, - afterDelete: (topicBookmarked) => { - this.model.set( - "bookmarked_posts", - this.model.bookmarked_posts.filter((x) => x.post_id !== post.id) - ); - post.deleteBookmark(topicBookmarked); + afterDelete: (topicBookmarked, bookmarkId) => { + this.model.removeBookmark(bookmarkId); + if (callbacks.onAfterDelete) { + callbacks.onAfterDelete(topicBookmarked); + } + resolve(); }, }); }); }, - _addOrUpdateBookmarkedPost(postId, reminderAt) { - if (!this.model.bookmarked_posts) { - this.model.set("bookmarked_posts", []); + _syncBookmarks(data) { + if (!this.model.bookmarks) { + this.model.set("bookmarks", []); } - let bookmarkedPost = this.model.bookmarked_posts.findBy("post_id", postId); - if (!bookmarkedPost) { - bookmarkedPost = { post_id: postId }; - this.model.bookmarked_posts.pushObject(bookmarkedPost); + const bookmark = this.model.bookmarks.findBy("id", data.id); + if (!bookmark) { + this.model.bookmarks.pushObject(data); + } else { + bookmark.reminder_at = data.reminder_at; + bookmark.name = data.name; + bookmark.auto_delete_preference = data.auto_delete_preference; } - - bookmarkedPost.reminder_at = reminderAt; }, - _toggleTopicBookmark() { + async _toggleTopicLevelBookmark() { if (this.model.bookmarking) { return Promise.resolve(); } - this.model.set("bookmarking", true); - const bookmarkedPostsCount = this.model.bookmarked_posts - ? this.model.bookmarked_posts.length - : 0; - const bookmarkPost = async (post) => { - const opts = await this._togglePostBookmark(post); - this.model.set("bookmarking", false); - if (opts.closedWithoutSaving) { - return; - } - this.model.afterPostBookmarked(post); - return [post.id]; - }; + if (this.model.bookmarkCount > 1) { + return this._maybeClearAllBookmarks(); + } - const toggleBookmarkOnServer = async () => { - if (bookmarkedPostsCount === 0) { - const firstPost = await this.model.firstPost(); - return bookmarkPost(firstPost); - } else if (bookmarkedPostsCount === 1) { - const postId = this.model.bookmarked_posts[0].post_id; - const post = await this.model.postById(postId); - return bookmarkPost(post); + if (this.model.bookmarkCount === 1) { + const forTopicBookmark = this.model.bookmarks.findBy("for_topic", true); + if (forTopicBookmark) { + return this._modifyTopicBookmark(forTopicBookmark); } else { - return this.model - .deleteBookmarks() - .then(() => this.model.clearBookmarks()) - .catch(popupAjaxError) - .finally(() => this.model.set("bookmarking", false)); + 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({ + post_id: firstPost.id, + for_topic: true, + }); + } + }, + + _maybeClearAllBookmarks() { return new Promise((resolve) => { - if (bookmarkedPostsCount > 1) { - bootbox.confirm( - I18n.t("bookmarks.confirm_clear"), - I18n.t("no_value"), - I18n.t("yes_value"), - (confirmed) => { - if (confirmed) { - toggleBookmarkOnServer().then(resolve); - } else { - this.model.set("bookmarking", false); - resolve(); - } + bootbox.confirm( + I18n.t("bookmarks.confirm_clear"), + I18n.t("no_value"), + I18n.t("yes_value"), + (confirmed) => { + if (confirmed) { + return this.model + .deleteBookmarks() + .then(() => resolve(this.model.clearBookmarks())) + .catch(popupAjaxError) + .finally(() => { + this.model.set("bookmarking", false); + }); + } else { + this.model.set("bookmarking", false); + resolve(); } - ); - } else { - toggleBookmarkOnServer().then(resolve); - } + } + ); }); }, 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 344b4b0c811..33c22e412b5 100644 --- a/app/assets/javascripts/discourse/app/initializers/topic-footer-buttons.js +++ b/app/assets/javascripts/discourse/app/initializers/topic-footer-buttons.js @@ -67,8 +67,7 @@ export default { dependentKeys: ["topic.bookmarked", "topic.bookmarksWereChanged"], id: "bookmark", icon() { - const bookmarkedPosts = this.topic.bookmarked_posts; - if (bookmarkedPosts && bookmarkedPosts.find((x) => x.reminder_at)) { + if (this.topic.bookmarks.some((bookmark) => bookmark.reminder_at)) { return "discourse-bookmark-clock"; } return "bookmark"; @@ -81,14 +80,9 @@ export default { }, label() { if (!this.topic.isPrivateMessage || this.site.mobileView) { - const bookmarkedPosts = this.topic.bookmarked_posts; - const bookmarkedPostsCount = bookmarkedPosts - ? bookmarkedPosts.length - : 0; - - if (bookmarkedPostsCount === 0) { + if (this.topic.bookmarkCount === 0) { return "bookmarked.title"; - } else if (bookmarkedPostsCount === 1) { + } else if (this.topic.bookmarkCount === 1) { return "bookmarked.edit_bookmark"; } else { return "bookmarked.clear_bookmarks"; @@ -96,12 +90,19 @@ export default { } }, translatedTitle() { - const bookmarkedPosts = this.topic.bookmarked_posts; - if (!bookmarkedPosts || bookmarkedPosts.length === 0) { + if (this.topic.bookmarkCount === 0) { return I18n.t("bookmarked.help.bookmark"); - } else if (bookmarkedPosts.length === 1) { - return I18n.t("bookmarked.help.edit_bookmark"); - } else if (bookmarkedPosts.find((x) => x.reminder_at)) { + } else if (this.topic.bookmarkCount === 1) { + if ( + this.topic.bookmarks.filter((bookmark) => bookmark.for_topic).length + ) { + return I18n.t("bookmarked.help.edit_bookmark_for_topic"); + } else { + return I18n.t("bookmarked.help.edit_bookmark"); + } + } else if ( + this.topic.bookmarks.some((bookmark) => bookmark.reminder_at) + ) { return I18n.t("bookmarked.help.unbookmark_with_reminder"); } else { return I18n.t("bookmarked.help.unbookmark"); diff --git a/app/assets/javascripts/discourse/app/models/post.js b/app/assets/javascripts/discourse/app/models/post.js index 6b352f1c87b..451a06d8ceb 100644 --- a/app/assets/javascripts/discourse/app/models/post.js +++ b/app/assets/javascripts/discourse/app/models/post.js @@ -308,9 +308,8 @@ const Post = RestModel.extend({ this.setProperties({ "topic.bookmarked": true, bookmarked: true, - bookmark_reminder_at: data.reminderAt, - bookmark_reminder_type: data.reminderType, - bookmark_auto_delete_preference: data.autoDeletePreference, + bookmark_reminder_at: data.reminder_at, + bookmark_auto_delete_preference: data.auto_delete_preference, bookmark_name: data.name, bookmark_id: data.id, }); @@ -329,7 +328,6 @@ const Post = RestModel.extend({ clearBookmark() { this.setProperties({ bookmark_reminder_at: null, - bookmark_reminder_type: null, bookmark_name: null, bookmark_id: null, bookmarked: false, diff --git a/app/assets/javascripts/discourse/app/models/topic.js b/app/assets/javascripts/discourse/app/models/topic.js index d15d70e903d..66c7f685240 100644 --- a/app/assets/javascripts/discourse/app/models/topic.js +++ b/app/assets/javascripts/discourse/app/models/topic.js @@ -376,17 +376,31 @@ const Topic = RestModel.extend({ return ajax(`/t/${this.id}/remove_bookmarks`, { type: "PUT" }); }, + bookmarkCount: alias("bookmarks.length"), + + removeBookmark(id) { + if (!this.bookmarks) { + this.set("bookmarks", []); + } + this.set( + "bookmarks", + this.bookmarks.filter((bookmark) => bookmark.id !== id) + ); + this.set("bookmarked", this.bookmarks.length); + this.incrementProperty("bookmarksWereChanged"); + }, + clearBookmarks() { this.toggleProperty("bookmarked"); - const postIds = this.bookmarked_posts.mapBy("post_id"); + const postIds = this.bookmarks.mapBy("post_id"); postIds.forEach((postId) => { const loadedPost = this.postStream.findLoadedPost(postId); if (loadedPost) { loadedPost.clearBookmark(); } }); - this.set("bookmarked_posts", []); + this.set("bookmarks", []); return postIds; }, @@ -609,6 +623,7 @@ Topic.reopenClass({ munge(json) { // ensure we are not overriding category computed property delete json.category; + json.bookmarks = json.bookmarks || []; return json; }, diff --git a/app/assets/javascripts/discourse/tests/acceptance/bookmarks-test.js b/app/assets/javascripts/discourse/tests/acceptance/bookmarks-test.js index 60978895f8f..c3895abe34f 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/bookmarks-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/bookmarks-test.js @@ -57,11 +57,6 @@ async function testTopicLevelBookmarkButtonIcon(assert, postNumber) { acceptance("Bookmarking", function (needs) { needs.user(); - let steps = []; - - needs.hooks.beforeEach(function () { - steps = []; - }); const topicResponse = topicFixtures["/t/280/1.json"]; topicResponse.post_stream.posts[0].cooked += ` @@ -85,10 +80,13 @@ acceptance("Bookmarking", function (needs) { needs.pretender((server, helper) => { function handleRequest(request) { const data = helper.parsePostData(request.requestBody); - steps.push(data.reminder_type || "none"); if (data.post_id === "398") { - return helper.response({ id: 1, success: "OK" }); + 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") { return helper.response({ id: 2, success: "OK" }); } else { @@ -98,6 +96,7 @@ acceptance("Bookmarking", function (needs) { server.post("/bookmarks", handleRequest); server.put("/bookmarks/1", handleRequest); server.put("/bookmarks/2", handleRequest); + server.put("/bookmarks/3", handleRequest); server.delete("/bookmarks/1", () => helper.response({ success: "OK", topic_bookmarked: false }) ); @@ -131,13 +130,6 @@ acceptance("Bookmarking", function (needs) { assert.ok(exists(".tap-tile-date-input"), "it shows the custom date input"); assert.ok(exists(".tap-tile-time-input"), "it shows the custom time input"); await click("#save-bookmark"); - - assert.deepEqual(steps, [ - "tomorrow", - "start_of_next_business_week", - "next_month", - "custom", - ]); }); test("Saving a bookmark with a reminder", async function (assert) { @@ -156,7 +148,6 @@ acceptance("Bookmarking", function (needs) { ), "it shows the bookmark clock icon because of the reminder" ); - assert.deepEqual(steps, ["tomorrow"]); }); test("Opening the options panel and remembering the option", async function (assert) { @@ -177,7 +168,6 @@ acceptance("Bookmarking", function (needs) { "it should reopen the options panel" ); assert.equal(selectKit(".bookmark-option-selector").header().value(), 1); - assert.deepEqual(steps, ["none"]); }); test("Saving a bookmark with no reminder or name", async function (assert) { @@ -195,7 +185,6 @@ acceptance("Bookmarking", function (needs) { ), "it shows the regular bookmark active icon" ); - assert.deepEqual(steps, ["none"]); }); test("Deleting a bookmark with a reminder", async function (assert) { @@ -203,8 +192,6 @@ acceptance("Bookmarking", function (needs) { await openBookmarkModal(); await click("#tap_tile_tomorrow"); - assert.deepEqual(steps, ["tomorrow"]); - await openEditBookmarkModal(); assert.ok( @@ -264,7 +251,6 @@ acceptance("Bookmarking", function (needs) { "08:00", "it should prefill the bookmark time" ); - assert.deepEqual(steps, ["tomorrow"]); }); test("Using a post date for the reminder date", async function (assert) { @@ -370,6 +356,78 @@ 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.equal( + 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( + exists(".topic-post:first-child button.bookmark.bookmarked"), + "the first post is not marked as being bookmarked" + ); + + assert.equal( + query("#topic-footer-button-bookmark").innerText, + I18n.t("bookmarked.edit_bookmark"), + "A topic level bookmark button has a label 'Edit Bookmark'" + ); + + await click("#topic-footer-button-bookmark"); + + assert.equal( + 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"); + + await click("#topic-footer-button-bookmark"); + + assert.equal( + query("input#bookmark-name").value, + "Test name", + "The topic level bookmark editing preserves the values entered" + ); + + await click(".d-modal-cancel"); + + await openBookmarkModal(1); + await click("#save-bookmark"); + + assert.ok( + exists(".topic-post:first-child button.bookmark.bookmarked"), + "the first post is bookmarked independently of the topic level bookmark" + ); + + // deleting all bookmarks in the topic + assert.equal( + query("#topic-footer-button-bookmark").innerText, + I18n.t("bookmarked.clear_bookmarks"), + "the footer button says Clear Bookmarks because there is more than one" + ); + await click("#topic-footer-button-bookmark"); + await click("a.btn-primary"); + + assert.ok( + !exists(".topic-post:first-child button.bookmark.bookmarked"), + "the first post bookmark is deleted" + ); + assert.equal( + query("#topic-footer-button-bookmark").innerText, + I18n.t("bookmarked.title"), + "the topic level bookmark is deleted" + ); + }); + test("The topic level bookmark button opens the edit modal if only one post in the post stream is bookmarked", async function (assert) { await visit("/t/internationalization-localization/280"); await openBookmarkModal(2); diff --git a/app/assets/javascripts/discourse/tests/acceptance/topic-timeline-test.js b/app/assets/javascripts/discourse/tests/acceptance/topic-timeline-test.js index 9f43748553d..51b377d19dd 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/topic-timeline-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/topic-timeline-test.js @@ -45,6 +45,7 @@ acceptance("Topic Timeline", function (needs) { read: true, user_title: null, bookmarked: false, + bookmarks: [], actions_summary: [ { id: 3, @@ -120,6 +121,7 @@ acceptance("Topic Timeline", function (needs) { read: true, user_title: null, bookmarked: false, + bookmarks: [], actions_summary: [ { id: 3, @@ -236,6 +238,7 @@ acceptance("Topic Timeline", function (needs) { current_post_number: 1, highest_post_number: 2, last_read_post_number: 0, + bookmarks: [], last_read_post_id: null, deleted_by: { id: 7, @@ -266,7 +269,7 @@ acceptance("Topic Timeline", function (needs) { ], chunk_size: 20, bookmarked: false, - bookmarked_posts: null, + bookmarks: [], topic_timer: null, message_bus_last_id: 5, participant_count: 1, diff --git a/app/assets/javascripts/discourse/tests/fixtures/poll.js b/app/assets/javascripts/discourse/tests/fixtures/poll.js index e1c33f7a078..084a18686df 100644 --- a/app/assets/javascripts/discourse/tests/fixtures/poll.js +++ b/app/assets/javascripts/discourse/tests/fixtures/poll.js @@ -234,7 +234,8 @@ export default { { id: 8, count: 0, hidden: false, can_act: true } ], chunk_size: 20, - bookmarked: false + bookmarked: false, + bookmarks: [], }, "/t/14.json": { post_stream: { @@ -390,7 +391,8 @@ export default { { id: 8, count: 0, hidden: false, can_act: true } ], chunk_size: 20, - bookmarked: false + bookmarked: false, + bookmarks: [], }, "/t/15.json": { post_stream: { @@ -543,7 +545,8 @@ export default { { id: 8, count: 0, hidden: false, can_act: true } ], chunk_size: 20, - bookmarked: false + bookmarked: false, + bookmarks: [], }, "/t/topic_with_pie_chart_poll.json": { post_stream: { @@ -691,6 +694,7 @@ export default { ], chunk_size: 20, bookmarked: false, + bookmarks: [], topic_timer: null, message_bus_last_id: 1, participant_count: 1, diff --git a/app/assets/javascripts/discourse/tests/fixtures/private-messages-fixtures.js b/app/assets/javascripts/discourse/tests/fixtures/private-messages-fixtures.js index cbacf8dd9e0..a781a546fdd 100644 --- a/app/assets/javascripts/discourse/tests/fixtures/private-messages-fixtures.js +++ b/app/assets/javascripts/discourse/tests/fixtures/private-messages-fixtures.js @@ -46,6 +46,7 @@ export default { archived: false, notification_level: 3, bookmarked: false, + bookmarks: [], liked: false, views: 5, like_count: 0, diff --git a/app/assets/javascripts/discourse/tests/fixtures/topic.js b/app/assets/javascripts/discourse/tests/fixtures/topic.js index b2cde97a0d5..d6f982021b4 100644 --- a/app/assets/javascripts/discourse/tests/fixtures/topic.js +++ b/app/assets/javascripts/discourse/tests/fixtures/topic.js @@ -136,6 +136,7 @@ export default { ], chunk_size: 20, bookmarked: false, + bookmarks: [], message_archived: false, topic_timer: null, message_bus_last_id: 5, @@ -3005,6 +3006,7 @@ export default { ], chunk_size: 20, bookmarked: false, + bookmarks: [], suggested_topics: [ { id: 27331, @@ -3029,6 +3031,7 @@ export default { archived: false, notification_level: 2, bookmarked: false, + bookmarks: [], liked: false, archetype: "regular", like_count: 11, @@ -3072,6 +3075,7 @@ export default { archived: false, notification_level: 2, bookmarked: false, + bookmarks: [], liked: false, archetype: "regular", like_count: 3, @@ -3152,6 +3156,7 @@ export default { archived: false, notification_level: 2, bookmarked: false, + bookmarks: [], liked: false, archetype: "regular", like_count: 4, @@ -3195,6 +3200,8 @@ export default { archived: false, notification_level: 2, bookmarked: false, + bookmarks: [], + bookmarks: [], liked: false, archetype: "regular", like_count: 62, @@ -3273,6 +3280,7 @@ export default { archived: false, notification_level: 1, bookmarked: false, + bookmarks: [], liked: false, archetype: "regular", like_count: 9, @@ -3862,6 +3870,7 @@ export default { ], chunk_size: 20, bookmarked: null, + bookmarks: [], tags: null, }, "/t/9/1.json": { @@ -4093,6 +4102,7 @@ export default { archived: false, notification_level: 2, bookmarked: false, + bookmarks: [], liked: false, archetype: "regular", like_count: 0, @@ -4122,6 +4132,7 @@ export default { archived: false, notification_level: 3, bookmarked: false, + bookmarks: [], liked: false, archetype: "regular", like_count: 0, @@ -4152,6 +4163,7 @@ export default { ], chunk_size: 20, bookmarked: false, + bookmarks: [], destination_category_id: 3, }, "/t/12/1.json": { @@ -4179,6 +4191,7 @@ export default { archived: false, notification_level: 2, bookmarked: false, + bookmarks: [], liked: false, archetype: "regular", like_count: 0, @@ -4420,6 +4433,7 @@ export default { archived: false, notification_level: 3, bookmarked: false, + bookmarks: [], liked: false, archetype: "private_message", like_count: 0, @@ -4463,6 +4477,7 @@ export default { ], chunk_size: 20, bookmarked: false, + bookmarks: [], message_archived: false, featured_link: null, }, @@ -4696,6 +4711,7 @@ export default { archived: false, notification_level: 2, bookmarked: false, + bookmarks: [], liked: false, archetype: "regular", like_count: 0, @@ -4725,6 +4741,7 @@ export default { archived: false, notification_level: 3, bookmarked: false, + bookmarks: [], liked: false, archetype: "regular", like_count: 0, @@ -4755,6 +4772,7 @@ export default { ], chunk_size: 20, bookmarked: false, + bookmarks: [], }, "/t/301/1.json": { post_stream: { @@ -4986,6 +5004,7 @@ export default { archived: false, notification_level: 2, bookmarked: false, + bookmarks: [], liked: false, archetype: "regular", like_count: 0, @@ -5015,6 +5034,7 @@ export default { archived: false, notification_level: 3, bookmarked: false, + bookmarks: [], liked: false, archetype: "regular", like_count: 0, @@ -5045,6 +5065,7 @@ export default { ], chunk_size: 20, bookmarked: false, + bookmarks: [], }, "/t/34/1.json": { post_stream: { @@ -5296,6 +5317,7 @@ export default { ], chunk_size: 20, bookmarked: false, + bookmarks: [], message_archived: false, topic_timer: null, message_bus_last_id: 7, @@ -5341,6 +5363,7 @@ export default { user_title: "a title", title_is_group: false, bookmarked: false, + bookmarks: [], actions_summary: [ { id: 3, @@ -5409,6 +5432,7 @@ export default { read: true, user_title: null, bookmarked: false, + bookmarks: [], actions_summary: [ { id: 2, @@ -5484,6 +5508,7 @@ export default { user_title: "a title", title_is_group: false, bookmarked: false, + bookmarks: [], actions_summary: [ { id: 3, @@ -5579,6 +5604,7 @@ export default { ], chunk_size: 20, bookmarked: false, + bookmarks: [], topic_timer: null, message_bus_last_id: 4, participant_count: 2, diff --git a/app/controllers/bookmarks_controller.rb b/app/controllers/bookmarks_controller.rb index f8b68bed285..a71588f24bf 100644 --- a/app/controllers/bookmarks_controller.rb +++ b/app/controllers/bookmarks_controller.rb @@ -15,6 +15,7 @@ class BookmarksController < ApplicationController post_id: params[:post_id], name: params[:name], reminder_at: params[:reminder_at], + for_topic: params[:for_topic] == "true", options: { auto_delete_preference: params[:auto_delete_preference] || 0 } diff --git a/app/models/bookmark.rb b/app/models/bookmark.rb index 14f9eba0cda..adf728dab6e 100644 --- a/app/models/bookmark.rb +++ b/app/models/bookmark.rb @@ -38,13 +38,16 @@ class Bookmark < ActiveRecord::Base on: [:create, :update], if: Proc.new { |b| b.will_save_change_to_post_id? || b.will_save_change_to_user_id? } + 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 :ensure_sane_reminder_at_time validate :bookmark_limit_not_reached validates :name, length: { maximum: 100 } def unique_per_post_for_user - is_first_post = Post.where(id: post_id).pluck_first(:post_number) == 1 - exists = if is_first_post + 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) @@ -55,6 +58,12 @@ class Bookmark < ActiveRecord::Base end end + 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 @@ -79,6 +88,10 @@ class Bookmark < ActiveRecord::Base ) end + 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 no_reminder? self.reminder_at.blank? end diff --git a/app/serializers/post_serializer.rb b/app/serializers/post_serializer.rb index 8de921eefff..ac91d65d2d8 100644 --- a/app/serializers/post_serializer.rb +++ b/app/serializers/post_serializer.rb @@ -359,9 +359,9 @@ class PostSerializer < BasicPostSerializer def post_bookmark if @topic_view.present? - @post_bookmark ||= @topic_view.user_post_bookmarks.find { |bookmark| bookmark.post_id == object.id } + @post_bookmark ||= @topic_view.user_post_bookmarks.find { |bookmark| bookmark.post_id == object.id && !bookmark.for_topic } else - @post_bookmark ||= object.bookmarks.find_by(user: scope.user) + @post_bookmark ||= object.bookmarks.find_by(user: scope.user, for_topic: false) end end diff --git a/app/serializers/topic_view_serializer.rb b/app/serializers/topic_view_serializer.rb index 6af55aefe78..b300b368026 100644 --- a/app/serializers/topic_view_serializer.rb +++ b/app/serializers/topic_view_serializer.rb @@ -62,7 +62,7 @@ class TopicViewSerializer < ApplicationSerializer :is_warning, :chunk_size, :bookmarked, - :bookmarked_posts, + :bookmarks, :message_archived, :topic_timer, :unicode_title, @@ -193,8 +193,8 @@ class TopicViewSerializer < ApplicationSerializer object.has_bookmarks? end - def bookmarked_posts - object.bookmarked_posts + def bookmarks + object.bookmarks end def topic_timer diff --git a/app/serializers/user_bookmark_serializer.rb b/app/serializers/user_bookmark_serializer.rb index e724931c5dd..67d10950509 100644 --- a/app/serializers/user_bookmark_serializer.rb +++ b/app/serializers/user_bookmark_serializer.rb @@ -28,7 +28,8 @@ class UserBookmarkSerializer < ApplicationSerializer :slug, :post_user_username, :post_user_avatar_template, - :post_user_name + :post_user_name, + :for_topic def topic_id post.topic_id diff --git a/app/serializers/web_hook_topic_view_serializer.rb b/app/serializers/web_hook_topic_view_serializer.rb index bff763f9b89..2098c7a67be 100644 --- a/app/serializers/web_hook_topic_view_serializer.rb +++ b/app/serializers/web_hook_topic_view_serializer.rb @@ -22,7 +22,7 @@ class WebHookTopicViewSerializer < TopicViewSerializer image_url slow_mode_seconds slow_mode_enabled_until - bookmarked_posts + bookmarks }.each do |attr| define_method("include_#{attr}?") do false diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index b806ce95191..8976053a692 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -303,6 +303,7 @@ en: help: bookmark: "Click to bookmark the first post on this topic" edit_bookmark: "Click to edit the bookmark on this topic" + edit_bookmark_for_topic: "Click to edit the bookmark for this topic" unbookmark: "Click to remove all bookmarks in this topic" unbookmark_with_reminder: "Click to remove all bookmarks and reminders in this topic." @@ -3187,7 +3188,9 @@ en: bookmarks: create: "Create bookmark" + create_for_topic: "Create bookmark for topic" edit: "Edit bookmark" + edit_for_topic: "Edit bookmark for topic" created: "Created" updated: "Updated" name: "Name" diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index f4c7cb12e4b..a66525434f2 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -431,6 +431,7 @@ en: cannot_set_past_reminder: "You cannot set a bookmark reminder in the past." cannot_set_reminder_in_distant_future: "You cannot set a bookmark reminder more than 10 years in the future." time_must_be_provided: "time must be provided for all reminders" + for_topic_must_use_first_post: "You can only use the first post to bookmark the topic." reminders: at_desktop: "Next time I'm at my desktop" diff --git a/lib/bookmark_manager.rb b/lib/bookmark_manager.rb index 1a3558adad1..48be152e513 100644 --- a/lib/bookmark_manager.rb +++ b/lib/bookmark_manager.rb @@ -7,8 +7,41 @@ class BookmarkManager @user = user 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. # TODO (martin) (2021-12-01) Remove reminder_type keyword argument once plugins are not using it. - def create(post_id:, name: nil, reminder_at: nil, reminder_type: nil, options: {}) + def create( + post_id:, + name: nil, + reminder_type: nil, + reminder_at: nil, + for_topic: false, + options: {} + ) post = Post.find_by(id: post_id) # no bookmarking deleted posts or topics @@ -24,7 +57,8 @@ class BookmarkManager post: post, name: name, reminder_at: reminder_at, - reminder_set_at: Time.zone.now + reminder_set_at: Time.zone.now, + for_topic: for_topic }.merge(options) ) diff --git a/lib/topic_view.rb b/lib/topic_view.rb index b91ea3af1f0..c396604d879 100644 --- a/lib/topic_view.rb +++ b/lib/topic_view.rb @@ -393,17 +393,13 @@ class TopicView def has_bookmarks? return false if @user.blank? return false if @topic.trashed? - @topic.bookmarks.exists?(user_id: @user.id) + bookmarks.any? end - def bookmarked_posts - return nil unless has_bookmarks? - @topic.bookmarks.where(user: @user).pluck(:post_id, :reminder_at).map do |post_id, reminder_at| - { - post_id: post_id, - reminder_at: reminder_at - } - end + def bookmarks + @bookmarks ||= @topic.bookmarks.where(user: @user).joins(:topic).select( + :id, :post_id, :for_topic, :reminder_at, :name, :auto_delete_preference + ) end MAX_PARTICIPANTS = 24 diff --git a/plugins/discourse-narrative-bot/assets/javascripts/initializers/new-user-narrative.js.es6 b/plugins/discourse-narrative-bot/assets/javascripts/initializers/new-user-narrative.js.es6 index 89d8c460a1b..5e1830e6f6b 100644 --- a/plugins/discourse-narrative-bot/assets/javascripts/initializers/new-user-narrative.js.es6 +++ b/plugins/discourse-narrative-bot/assets/javascripts/initializers/new-user-narrative.js.es6 @@ -19,12 +19,12 @@ function initialize(api) { api.modifyClass("controller:topic", { pluginId: PLUGIN_ID, - _togglePostBookmark(post) { + _modifyBookmark(bookmark, post) { // if we are talking to discobot then any bookmarks should just // be created without reminder options, to streamline the new user // narrative. const discobotUserId = -2; - if (post.user_id === discobotUserId && !post.bookmarked) { + if (post && post.user_id === discobotUserId && !post.bookmarked) { return ajax("/bookmarks", { type: "POST", data: { post_id: post.id }, @@ -37,7 +37,7 @@ function initialize(api) { post.appEvents.trigger("post-stream:refresh", { id: this.id }); }); } - return this._super(post); + return this._super(bookmark, post); }, }); diff --git a/plugins/poll/test/javascripts/acceptance/poll-in-reply-history-test.js.es6 b/plugins/poll/test/javascripts/acceptance/poll-in-reply-history-test.js.es6 index 412fdbbe314..7399ee3aac6 100644 --- a/plugins/poll/test/javascripts/acceptance/poll-in-reply-history-test.js.es6 +++ b/plugins/poll/test/javascripts/acceptance/poll-in-reply-history-test.js.es6 @@ -54,6 +54,7 @@ acceptance("Poll in a post reply history", function (needs) { "/letter_avatar_proxy/v4/letter/a/bbce88/{size}.png", }, bookmarked: false, + bookmarks: [], actions_summary: [ { id: 3, @@ -158,6 +159,7 @@ acceptance("Poll in a post reply history", function (needs) { archived: false, notification_level: 2, bookmarked: false, + bookmarks: [], liked: false, like_count: 0, views: 3, @@ -212,6 +214,7 @@ acceptance("Poll in a post reply history", function (needs) { archived: false, notification_level: 2, bookmarked: false, + bookmarks: [], liked: false, like_count: 0, views: 4, @@ -302,6 +305,7 @@ acceptance("Poll in a post reply history", function (needs) { ], chunk_size: 20, bookmarked: false, + bookmarks: [], topic_timer: null, message_bus_last_id: 4, participant_count: 1, @@ -395,6 +399,7 @@ acceptance("Poll in a post reply history", function (needs) { can_wiki: false, user_title: null, bookmarked: false, + bookmarks: [], actions_summary: [], moderator: false, admin: true, diff --git a/plugins/poll/test/javascripts/acceptance/poll-quote-test.js.es6 b/plugins/poll/test/javascripts/acceptance/poll-quote-test.js.es6 index 8d7e075a531..d15664bd55f 100644 --- a/plugins/poll/test/javascripts/acceptance/poll-quote-test.js.es6 +++ b/plugins/poll/test/javascripts/acceptance/poll-quote-test.js.es6 @@ -46,6 +46,7 @@ acceptance("Poll quote", function (needs) { user_title: "Tester", title_is_group: false, bookmarked: false, + bookmarks: [], raw: "[poll name=poll1 type=regular results=always chartType=bar]\n* Alpha\n* Beta\n[/poll]\n\n[poll name=poll2 type=regular results=always chartType=bar]\n* First\n* Second\n[/poll]", actions_summary: [ @@ -175,6 +176,7 @@ acceptance("Poll quote", function (needs) { user_title: "Tester", title_is_group: false, bookmarked: false, + bookmarks: [], actions_summary: [ { id: 3, @@ -238,6 +240,7 @@ acceptance("Poll quote", function (needs) { archived: false, notification_level: 1, bookmarked: false, + bookmarks: [], liked: false, tags: [], like_count: 0, @@ -282,6 +285,7 @@ acceptance("Poll quote", function (needs) { archived: false, notification_level: 3, bookmarked: false, + bookmarks: [], liked: false, tags: [], like_count: 0, @@ -362,6 +366,7 @@ acceptance("Poll quote", function (needs) { ], chunk_size: 20, bookmarked: false, + bookmarks: [], topic_timer: null, message_bus_last_id: 2, participant_count: 1, diff --git a/plugins/poll/test/javascripts/acceptance/poll-results-test.js.es6 b/plugins/poll/test/javascripts/acceptance/poll-results-test.js.es6 index 6adb069b3d7..cf2e9f70685 100644 --- a/plugins/poll/test/javascripts/acceptance/poll-results-test.js.es6 +++ b/plugins/poll/test/javascripts/acceptance/poll-results-test.js.es6 @@ -48,6 +48,7 @@ acceptance("Poll results", function (needs) { can_wiki: true, title_is_group: false, bookmarked: false, + bookmarks: [], raw: "[poll type=regular results=always public=true chartType=bar]\n* Option #1\n* Option #2\n[/poll]", actions_summary: [ @@ -154,6 +155,7 @@ acceptance("Poll results", function (needs) { read: true, title_is_group: false, bookmarked: false, + bookmarks: [], actions_summary: [ { id: 3, can_act: true }, { id: 4, can_act: true }, @@ -247,6 +249,7 @@ acceptance("Poll results", function (needs) { archived: false, notification_level: 2, bookmarked: false, + bookmarks: [], liked: false, tags: [], like_count: 0, @@ -302,6 +305,7 @@ acceptance("Poll results", function (needs) { archived: false, notification_level: 2, bookmarked: false, + bookmarks: [], liked: false, tags: [], like_count: 0, @@ -349,6 +353,7 @@ acceptance("Poll results", function (needs) { archived: false, notification_level: 3, bookmarked: false, + bookmarks: [], liked: false, tags: ["abc", "e", "b"], like_count: 0, @@ -394,6 +399,7 @@ acceptance("Poll results", function (needs) { archived: false, notification_level: 3, bookmarked: false, + bookmarks: [], liked: false, tags: [], like_count: 0, @@ -461,6 +467,7 @@ acceptance("Poll results", function (needs) { ], chunk_size: 20, bookmarked: false, + bookmarks: [], topic_timer: null, message_bus_last_id: 5, participant_count: 1, diff --git a/spec/components/topic_view_spec.rb b/spec/components/topic_view_spec.rb index 671c5f9f454..16790f947b6 100644 --- a/spec/components/topic_view_spec.rb +++ b/spec/components/topic_view_spec.rb @@ -408,7 +408,7 @@ describe TopicView do end end - context "#bookmarked_posts" 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) } @@ -416,7 +416,7 @@ describe TopicView do it "gets the first post bookmark reminder at for the user" do topic_view = TopicView.new(topic.id, user) - first, second = topic_view.bookmarked_posts + first, second = topic_view.bookmarks expect(first[:post_id]).to eq(bookmark1.post_id) expect(first[:reminder_at]).to eq_time(bookmark1.reminder_at) expect(second[:post_id]).to eq(bookmark2.post_id) @@ -424,12 +424,12 @@ describe TopicView do end context "when the topic is deleted" do - it "returns nil" do + it "returns []" do topic_view = TopicView.new(topic, user) PostDestroyer.new(Fabricate(:admin), topic.first_post).destroy topic.reload - expect(topic_view.bookmarked_posts).to eq(nil) + expect(topic_view.bookmarks).to eq([]) end end @@ -439,8 +439,8 @@ describe TopicView do PostDestroyer.new(Fabricate(:admin), topic.posts.second).destroy topic.reload - expect(topic_view.bookmarked_posts.length).to eq(1) - first = topic_view.bookmarked_posts.first + expect(topic_view.bookmarks.length).to eq(1) + first = topic_view.bookmarks.first expect(first[:post_id]).to eq(bookmark1.post_id) expect(first[:reminder_at]).to eq_time(bookmark1.reminder_at) end diff --git a/spec/lib/bookmark_manager_spec.rb b/spec/lib/bookmark_manager_spec.rb index 95600c2be97..d2d910cb676 100644 --- a/spec/lib/bookmark_manager_spec.rb +++ b/spec/lib/bookmark_manager_spec.rb @@ -17,7 +17,28 @@ RSpec.describe BookmarkManager do bookmark = Bookmark.find_by(user: user) expect(bookmark.post_id).to eq(post.id) - expect(bookmark.topic.id).to eq(post.topic_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 + + it "errors when creating a for_topic bookmark for a post that is not the first one" do + subject.create(post_id: Fabricate(:post, topic: post.topic).id, name: name, for_topic: true) + expect(subject.errors.full_messages).to include(I18n.t("bookmarks.errors.for_topic_must_use_first_post")) end it "when topic is deleted it raises invalid access from guardian check" do diff --git a/spec/requests/bookmarks_controller_spec.rb b/spec/requests/bookmarks_controller_spec.rb index 05e8f2429b7..8a5d299e02a 100644 --- a/spec/requests/bookmarks_controller_spec.rb +++ b/spec/requests/bookmarks_controller_spec.rb @@ -30,6 +30,31 @@ describe BookmarksController do expect(response.status).to eq(429) end + 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 + + 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 diff --git a/spec/serializers/post_serializer_spec.rb b/spec/serializers/post_serializer_spec.rb index 8c020985801..53243fd6004 100644 --- a/spec/serializers/post_serializer_spec.rb +++ b/spec/serializers/post_serializer_spec.rb @@ -45,7 +45,8 @@ describe PostSerializer do it "should not allow user to flag post and notify non human user" do post.update!(user: Discourse.system_user) - serializer = PostSerializer.new(post, + serializer = PostSerializer.new( + post, scope: Guardian.new(actor), root: false ) @@ -247,6 +248,17 @@ 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