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