From 3b87271647aebb7e2ade427a9fe5cda5d74f35c6 Mon Sep 17 00:00:00 2001 From: Andrei Prigorshnev Date: Thu, 17 Jun 2021 19:24:06 +0400 Subject: [PATCH] FEATURE: Open the edit bookmark modal when clicking on the topic level bookmark button (#13407) If you click on a bookmark in the post stream you get an Edit Bookmark modal. This does not happen if you click the topic bookmark button. We want to open the Edit modal too if there is only one bookmark on a topic (it doesn't matter on the first post or not). The other behaviour if there are > 1 bookmarks in the topic is to prompt the user to confirm delete of all the bookmarks in the topic. This behaviour will stay as-is. I have done some refactoring in this PR, and still, there is a place for improvement. For example, we don't call post.deleteBookmark() method when deleting several bookmarks. I just don't want to refactor too much in one PR. --- .../discourse/app/controllers/topic.js | 56 +++++++------------ .../app/initializers/topic-footer-buttons.js | 13 ++++- .../javascripts/discourse/app/models/post.js | 2 + .../javascripts/discourse/app/models/topic.js | 14 +++-- .../tests/acceptance/bookmarks-test.js | 39 +++++++++++++ config/locales/client.en.yml | 1 + 6 files changed, 81 insertions(+), 44 deletions(-) diff --git a/app/assets/javascripts/discourse/app/controllers/topic.js b/app/assets/javascripts/discourse/app/controllers/topic.js index f9b2b935606..f8427f5347b 100644 --- a/app/assets/javascripts/discourse/app/controllers/topic.js +++ b/app/assets/javascripts/discourse/app/controllers/topic.js @@ -1219,57 +1219,43 @@ export default Controller.extend(bufferedProperty("model"), { return Promise.resolve(); } this.model.set("bookmarking", true); - const bookmark = !this.model.bookmarked; - let posts = this.model.postStream.posts; + const alreadyBookmarkedPosts = this.model.bookmarkedPosts; return this.model.firstPost().then((firstPost) => { + 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]; + }; + const toggleBookmarkOnServer = () => { - if (bookmark) { - return this._togglePostBookmark(firstPost).then((opts) => { - this.model.set("bookmarking", false); - if (opts && opts.closedWithoutSaving) { - return; - } - return this.model.afterTopicBookmarked(firstPost); - }); + if (alreadyBookmarkedPosts.length === 0) { + return bookmarkPost(firstPost); + } else if (alreadyBookmarkedPosts.length === 1) { + const post = alreadyBookmarkedPosts[0]; + return bookmarkPost(post); } else { return this.model .deleteBookmark() .then(() => { this.model.toggleProperty("bookmarked"); this.model.set("bookmark_reminder_at", null); - let clearedBookmarkProps = { - bookmarked: false, - bookmark_id: null, - bookmark_name: null, - bookmark_reminder_at: null, - }; - if (posts) { - const updated = []; - posts.forEach((post) => { - if (post.bookmarked) { - post.setProperties(clearedBookmarkProps); - updated.push(post.id); - } - }); - firstPost.setProperties(clearedBookmarkProps); - return updated; - } + alreadyBookmarkedPosts.forEach((post) => { + post.clearBookmark(); + }); + return alreadyBookmarkedPosts.mapBy("id"); }) .catch(popupAjaxError) .finally(() => this.model.set("bookmarking", false)); } }; - const unbookmarkedPosts = []; - if (!bookmark && posts) { - posts.forEach( - (post) => post.bookmarked && unbookmarkedPosts.push(post) - ); - } - return new Promise((resolve) => { - if (unbookmarkedPosts.length > 1) { + if (alreadyBookmarkedPosts.length > 1) { bootbox.confirm( I18n.t("bookmarks.confirm_clear"), I18n.t("no_value"), 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 6cbc34c5462..d2c365b18ad 100644 --- a/app/assets/javascripts/discourse/app/initializers/topic-footer-buttons.js +++ b/app/assets/javascripts/discourse/app/initializers/topic-footer-buttons.js @@ -66,7 +66,7 @@ export default { }); registerTopicFooterButton({ - dependentKeys: ["topic.bookmarked"], + dependentKeys: ["topic.bookmarked", "topic.bookmarkedPosts"], id: "bookmark", icon() { if (this.get("topic.bookmark_reminder_at")) { @@ -81,8 +81,15 @@ export default { }, label() { if (!this.get("topic.isPrivateMessage") || this.site.mobileView) { - const bookmarked = this.get("topic.bookmarked"); - return bookmarked ? "bookmarked.clear_bookmarks" : "bookmarked.title"; + const bookmarkedPostsCount = this.get("topic.bookmarkedPosts").length; + + if (bookmarkedPostsCount === 0) { + return "bookmarked.title"; + } else if (bookmarkedPostsCount === 1) { + return "bookmarked.edit_bookmark"; + } else { + return "bookmarked.clear_bookmarks"; + } } }, translatedTitle() { diff --git a/app/assets/javascripts/discourse/app/models/post.js b/app/assets/javascripts/discourse/app/models/post.js index 97642f069fa..8a936b55967 100644 --- a/app/assets/javascripts/discourse/app/models/post.js +++ b/app/assets/javascripts/discourse/app/models/post.js @@ -314,6 +314,7 @@ const Post = RestModel.extend({ bookmark_name: data.name, bookmark_id: data.id, }); + this.topic.incrementProperty("bookmarksWereChanged"); this.appEvents.trigger("page:bookmark-post-toggled", this); this.appEvents.trigger("post-stream:refresh", { id: this.id }); }, @@ -333,6 +334,7 @@ const Post = RestModel.extend({ bookmarked: false, bookmark_auto_delete_preference: null, }); + this.topic.incrementProperty("bookmarksWereChanged"); }, updateActionsSummary(json) { diff --git a/app/assets/javascripts/discourse/app/models/topic.js b/app/assets/javascripts/discourse/app/models/topic.js index deef4b89f49..d52f9e16eaa 100644 --- a/app/assets/javascripts/discourse/app/models/topic.js +++ b/app/assets/javascripts/discourse/app/models/topic.js @@ -322,6 +322,11 @@ const Topic = RestModel.extend({ return Site.currentProp("archetypes").findBy("id", archetype); }, + @discourseComputed("bookmarksWereChanged") + bookmarkedPosts() { + return this.postStream.posts.filterBy("bookmarked", true); + }, + isPrivateMessage: equal("archetype", "private_message"), isBanner: equal("archetype", "banner"), @@ -356,12 +361,9 @@ const Topic = RestModel.extend({ }).then(() => this.set("archetype", "regular")); }, - afterTopicBookmarked(firstPost) { - if (firstPost) { - firstPost.set("bookmarked", true); - this.set("bookmark_reminder_at", firstPost.bookmark_reminder_at); - return [firstPost.id]; - } + afterPostBookmarked(post) { + post.set("bookmarked", true); + this.set("bookmark_reminder_at", post.bookmark_reminder_at); }, firstPost() { diff --git a/app/assets/javascripts/discourse/tests/acceptance/bookmarks-test.js b/app/assets/javascripts/discourse/tests/acceptance/bookmarks-test.js index f2e0dfce500..e0dc6dea964 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/bookmarks-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/bookmarks-test.js @@ -2,6 +2,7 @@ import { acceptance, exists, loggedInUser, + query, queryAll, } from "discourse/tests/helpers/qunit-helpers"; import { click, fillIn, visit } from "@ember/test-helpers"; @@ -315,4 +316,42 @@ acceptance("Bookmarking", function (needs) { "the second bookmark is deleted" ); }); + + test("The topic level bookmark button opens the edit modal if only the first post on the topic is bookmarked", async function (assert) { + await visit("/t/internationalization-localization/280"); + await openBookmarkModal(1); + await click("#save-bookmark"); + + 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.ok( + exists("div.modal.bookmark-with-reminder"), + "The edit modal is opened" + ); + }); + + 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); + await click("#save-bookmark"); + + 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.ok( + exists("div.modal.bookmark-with-reminder"), + "The edit modal is opened" + ); + }); }); diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 3aa726602f0..dbfc5cc67a7 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -297,6 +297,7 @@ en: bookmarked: title: "Bookmark" + edit_bookmark: "Edit Bookmark" clear_bookmarks: "Clear Bookmarks" help: bookmark: "Click to bookmark the first post on this topic"