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.

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 toggleBookmarkOnServer = () => {
-        if (bookmark) {
-          return this._togglePostBookmark(firstPost).then((opts) => {
+      const bookmarkPost = async (post) => {
+        const opts = await this._togglePostBookmark(post);
         this.model.set("bookmarking", false);
-            if (opts && opts.closedWithoutSaving) {
+        if (opts.closedWithoutSaving) {
           return;
         }
-            return this.model.afterTopicBookmarked(firstPost);
-          });
+        this.model.afterPostBookmarked(post);
+        return [post.id];
+      };
+
+      const toggleBookmarkOnServer = () => {
+        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);
-                  }
+              alreadyBookmarkedPosts.forEach((post) => {
+                post.clearBookmark();
               });
-                firstPost.setProperties(clearedBookmarkProps);
-                return updated;
-              }
+              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"),

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() {

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) {

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() {

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"
+    );
+  });
 });

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"