FEATURE: Use new bookmark menu in topic footer buttons (#26670)

Followup to 67a8080e33

This commit makes it so the topic footer button for bookmarks
uses the new BookmarkMenu component, and makes some tweaks to
that component to allow for a label and CSS class options.

Also introduces a TopicBookmarkManager to manage the saving/editing/
deleting of the topic level bookmarks and the reactivity that happens
in the topic UI afterward.

Next commit should rip out old bookmark associated code in the
topic controller as it will no longer be needed.

---------

Co-authored-by: Joffrey JAFFEUX <j.jaffeux@gmail.com>

app/assets/javascripts/discourse/app/components/bookmark-menu.gjs

@@ -6,7 +6,6 @@ import didInsert from "@ember/render-modifiers/modifiers/did-insert";
 import { inject as service } from "@ember/service";
 import DButton from "discourse/components/d-button";
 import BookmarkModal from "discourse/components/modal/bookmark";
-import concatClass from "discourse/helpers/concat-class";
 import { popupAjaxError } from "discourse/lib/ajax-error";
 import {
   TIME_SHORTCUT_TYPES,
@@ -59,18 +58,61 @@ export default class BookmarkMenu extends Component {
       return I18n.t("bookmarks.not_bookmarked");
     } else {
       if (this.existingBookmark.reminderAt) {
-        return I18n.t("bookmarks.created_with_reminder", {
+        return I18n.t("bookmarks.created_with_reminder_generic", {
           date: this.existingBookmark.formattedReminder(this.timezone),
           name: this.existingBookmark.name || "",
         });
       } else {
-        return I18n.t("bookmarks.created", {
+        return I18n.t("bookmarks.created_generic", {
           name: this.existingBookmark.name || "",
         });
       }
     }
   }
 
+  get buttonClasses() {
+    let cssClasses = ["bookmark widget-button bookmark-menu__trigger"];
+
+    if (!this.args.showLabel) {
+      cssClasses.push("btn-icon no-text");
+    } else {
+      cssClasses.push("btn-icon-text");
+    }
+
+    if (this.args.buttonClasses) {
+      cssClasses.push(this.args.buttonClasses);
+    }
+
+    if (this.existingBookmark) {
+      cssClasses.push("bookmarked");
+      if (this.existingBookmark.reminderAt) {
+        cssClasses.push("with-reminder");
+      }
+    }
+
+    return cssClasses.join(" ");
+  }
+
+  get buttonIcon() {
+    if (this.existingBookmark?.reminderAt) {
+      return "discourse-bookmark-clock";
+    } else {
+      return "bookmark";
+    }
+  }
+
+  get buttonLabel() {
+    if (!this.args.showLabel) {
+      return;
+    }
+
+    if (this.existingBookmark) {
+      return I18n.t("bookmarked.edit_bookmark");
+    } else {
+      return I18n.t("bookmarked.title");
+    }
+  }
+
   @action
   reminderShortcutTimeTitle(option) {
     if (!option.time) {
@@ -196,25 +238,16 @@ export default class BookmarkMenu extends Component {
       {{didInsert this.setReminderShortcuts}}
       @identifier="bookmark-menu"
       @triggers={{array "click"}}
-      class={{concatClass
+      class={{this.buttonClasses}}
-        "bookmark widget-button btn-flat no-text btn-icon bookmark-menu__trigger"
-        (if this.existingBookmark "bookmarked")
-        (if this.existingBookmark.reminderAt "with-reminder")
-      }}
       @title={{this.buttonTitle}}
+      @label={{this.buttonLabel}}
+      @icon={{this.buttonIcon}}
       @onClose={{this.onCloseMenu}}
       @onShow={{this.onShowMenu}}
       @onRegisterApi={{this.onRegisterApi}}
       @modalForMobile={{true}}
       @arrow={{false}}
     >
-      <:trigger>
-        {{#if this.existingBookmark.reminderAt}}
-          {{icon "discourse-bookmark-clock"}}
-        {{else}}
-          {{icon "bookmark"}}
-        {{/if}}
-      </:trigger>
       <:content>
         <div class="bookmark-menu__body">
 

app/assets/javascripts/discourse/app/components/topic-footer-buttons.hbs

@@ -27,20 +27,27 @@
 
   {{#each this.inlineActionables as |actionable|}}
     {{#if (eq actionable.type "inline-button")}}
-      <DButton
+      {{#if (eq actionable.id "bookmark")}}
-        @action={{actionable.action}}
+        <BookmarkMenu
-        @icon={{actionable.icon}}
+          @showLabel={{true}}
-        @translatedLabel={{actionable.label}}
+          @bookmarkManager={{this.topicBookmarkManager}}
-        @translatedTitle={{actionable.title}}
+        />
-        @translatedAriaLabel={{actionable.ariaLabel}}
+      {{else}}
-        @disabled={{actionable.disabled}}
+        <DButton
-        id={{concat "topic-footer-button-" actionable.id}}
+          @action={{actionable.action}}
-        class={{concat-class
+          @icon={{actionable.icon}}
-          "btn-default"
+          @translatedLabel={{actionable.label}}
-          "topic-footer-button"
+          @translatedTitle={{actionable.title}}
-          actionable.classNames
+          @translatedAriaLabel={{actionable.ariaLabel}}
-        }}
+          @disabled={{actionable.disabled}}
-      />
+          id={{concat "topic-footer-button-" actionable.id}}
+          class={{concat-class
+            "btn-default"
+            "topic-footer-button"
+            actionable.classNames
+          }}
+        />
+      {{/if}}
     {{else}}
       <DropdownSelectBox
         @id={{concat "topic-footer-dropdown-" actionable.id}}

app/assets/javascripts/discourse/app/components/topic-footer-buttons.js

@@ -1,9 +1,11 @@
+import { getOwner } from "@ember/application";
 import Component from "@ember/component";
 import { computed } from "@ember/object";
 import { alias, or } from "@ember/object/computed";
 import { NotificationLevels } from "discourse/lib/notification-levels";
 import { getTopicFooterButtons } from "discourse/lib/register-topic-footer-button";
 import { getTopicFooterDropdowns } from "discourse/lib/register-topic-footer-dropdown";
+import TopicBookmarkManager from "discourse/lib/topic-bookmark-manager";
 import discourseComputed from "discourse-common/utils/decorators";
 
 export default Component.extend({
@@ -34,6 +36,10 @@ export default Component.extend({
     }
   ),
 
+  topicBookmarkManager: computed("topic", function () {
+    return new TopicBookmarkManager(getOwner(this), this.topic);
+  }),
+
   // topic.assigned_to_user is for backward plugin support
   @discourseComputed("inlineButtons.[]", "topic.assigned_to_user")
   dropdownButtons(inlineButtons) {

app/assets/javascripts/discourse/app/lib/topic-bookmark-manager.js

@@ -0,0 +1,88 @@
+import { tracked } from "@glimmer/tracking";
+import { setOwner } from "@ember/application";
+import { inject as controller } from "@ember/controller";
+import { inject as service } from "@ember/service";
+import { BookmarkFormData } from "discourse/lib/bookmark-form-data";
+import Bookmark from "discourse/models/bookmark";
+
+export default class TopicBookmarkManager {
+  @service currentUser;
+  @service bookmarkApi;
+  @controller("topic") topicController;
+  @tracked trackedBookmark;
+  @tracked bookmarkModel;
+
+  constructor(owner, topic) {
+    setOwner(this, owner);
+
+    this.model = topic;
+    this.type = "Topic";
+
+    this.bookmarkModel =
+      this.topicController.model?.bookmarks.find(
+        (bookmark) =>
+          bookmark.bookmarkable_id === this.model.id &&
+          bookmark.bookmarkable_type === this.type
+      ) || this.bookmarkApi.buildNewBookmark(this.type, this.model.id);
+    this.trackedBookmark = new BookmarkFormData(this.bookmarkModel);
+  }
+
+  create() {
+    return this.bookmarkApi
+      .create(this.trackedBookmark)
+      .then((updatedBookmark) => {
+        this.trackedBookmark = updatedBookmark;
+      });
+  }
+
+  delete() {
+    return this.bookmarkApi.delete(this.trackedBookmark.id);
+  }
+
+  save() {
+    return this.bookmarkApi.update(this.trackedBookmark);
+  }
+
+  // noop for topics
+  afterModalClose() {
+    return;
+  }
+
+  afterSave(bookmarkFormData) {
+    this.trackedBookmark = bookmarkFormData;
+    this._syncBookmarks(bookmarkFormData.saveData);
+    this.topicController.model.set("bookmarking", false);
+    this.topicController.model.set("bookmarked", true);
+    this.topicController.model.incrementProperty("bookmarksWereChanged");
+    this.topicController.model.appEvents.trigger(
+      "bookmarks:changed",
+      bookmarkFormData.saveData,
+      this.bookmarkModel.attachedTo()
+    );
+    return [this.model.id];
+  }
+
+  afterDelete(deleteResponse, bookmarkId) {
+    this.topicController.model.removeBookmark(bookmarkId);
+    this.bookmarkModel = this.bookmarkApi.buildNewBookmark(
+      this.type,
+      this.model.id
+    );
+    this.trackedBookmark = new BookmarkFormData(this.bookmarkModel);
+  }
+
+  _syncBookmarks(data) {
+    if (!this.topicController.bookmarks) {
+      this.topicController.set("bookmarks", []);
+    }
+
+    const bookmark = this.topicController.bookmarks.findBy("id", data.id);
+    if (!bookmark) {
+      this.topicController.bookmarks.pushObject(Bookmark.create(data));
+    } else {
+      bookmark.reminder_at = data.reminder_at;
+      bookmark.name = data.name;
+      bookmark.auto_delete_preference = data.auto_delete_preference;
+    }
+  }
+}

app/assets/javascripts/discourse/app/widgets/bookmark-menu.js

@@ -4,5 +4,5 @@ import { registerWidgetShim } from "discourse/widgets/render-glimmer";
 registerWidgetShim(
   "bookmark-menu-shim",
   "div.bookmark-menu-shim",
-  hbs`<BookmarkMenu @bookmarkManager={{@data.bookmarkManager}} />`
+  hbs`<BookmarkMenu @bookmarkManager={{@data.bookmarkManager}} @buttonClasses="btn-flat" />`
 );

spec/system/bookmarks_spec.rb

@@ -45,7 +45,7 @@ describe "Bookmarking posts and topics", type: :system do
     bookmark_menu.click_menu_option("tomorrow")
 
     expect(topic_page).to have_post_bookmarked(post, with_reminder: true)
-    expect(page).to have_no_css(".bookmark-menu-content")
+    expect(page).to have_no_css(".bookmark-menu-content.-expanded")
     expect(Bookmark.find_by(bookmarkable: post, user: current_user).reminder_at).not_to be_blank
   end
 
@@ -97,34 +97,24 @@ describe "Bookmarking posts and topics", type: :system do
   describe "topic level bookmarks" do
     it "allows the topic to be bookmarked" do
       topic_page.visit_topic(topic)
-      topic_page.click_topic_footer_button(:bookmark)
+      topic_page.click_topic_bookmark_button
-
+      expect(topic_page).to have_topic_bookmarked(topic)
-      bookmark_modal.fill_name("something important")
-      bookmark_modal.save
-
-      expect(topic_page).to have_topic_bookmarked
       expect(Bookmark.exists?(bookmarkable: topic, user: current_user)).to be_truthy
     end
 
-    it "opens the edit bookmark modal from the topic bookmark button if one post is bookmarked" do
+    it "opens the edit bookmark modal from the topic bookmark button and saves edits" do
-      bookmark = Fabricate(:bookmark, bookmarkable: post_2, user: current_user)
+      bookmark = Fabricate(:bookmark, bookmarkable: topic, user: current_user)
       topic_page.visit_topic(topic)
-      topic_page.click_topic_footer_button(:bookmark)
+      topic_page.click_topic_bookmark_button
+      bookmark_menu.click_menu_option("edit")
       expect(bookmark_modal).to be_open
       expect(bookmark_modal).to be_editing_id(bookmark.id)
-    end
+      bookmark_modal.fill_name("something important")
+      bookmark_modal.click_primary_button
 
-    it "clears all topic bookmarks from the topic bookmark button if more than one post is bookmarked" do
+      try_until_success(frequency: 0.5) do
-      Fabricate(:bookmark, bookmarkable: post, user: current_user)
+        expect(bookmark.reload.name).to eq("something important")
-      Fabricate(:bookmark, bookmarkable: post_2, user: current_user)
+      end
-      topic_page.visit_topic(topic)
-      topic_page.click_topic_footer_button(:bookmark)
-      dialog = PageObjects::Components::Dialog.new
-      expect(dialog).to have_content(I18n.t("js.bookmarks.confirm_clear"))
-      dialog.click_yes
-      expect(dialog).to be_closed
-      expect(topic_page).to have_no_bookmarks
-      expect(Bookmark.where(user: current_user).count).to eq(0)
     end
   end
 

spec/system/page_objects/pages/topic.rb

@@ -63,9 +63,7 @@ module PageObjects
       end
 
       def has_post_more_actions?(post)
-        within post_by_number(post) do
+        within_post(post) { has_css?(".show-more-actions") }
-          has_css?(".show-more-actions")
-        end
       end
 
       def has_post_bookmarked?(post, with_reminder: false)
@@ -83,20 +81,20 @@ module PageObjects
       def click_post_action_button(post, button)
         case button
         when :bookmark
-          post_by_number(post).find(".bookmark").click
+          within_post(post) { find(".bookmark").click }
         when :reply
-          post_by_number(post).find(".post-controls .reply").click
+          within_post(post) { find(".post-controls .reply").click }
         when :flag
-          post_by_number(post).find(".post-controls .create-flag").click
+          within_post(post) { find(".post-controls .create-flag").click }
         when :copy_link
-          post_by_number(post).find(".post-controls .post-action-menu__copy-link").click
+          within_post(post) { find(".post-controls .post-action-menu__copy-link").click }
         when :edit
-          post_by_number(post).find(".post-controls .edit").click
+          within_post(post) { find(".post-controls .edit").click }
         end
       end
 
       def expand_post_admin_actions(post)
-        post_by_number(post).find(".show-post-admin-menu").click
+        within_post(post) { find(".show-post-admin-menu").click }
       end
 
       def click_post_admin_action_button(post, button)
@@ -111,24 +109,22 @@ module PageObjects
         find(element_klass).click
       end
 
-      def click_topic_footer_button(button)
+      def click_topic_bookmark_button
-        find_topic_footer_button(button).click
+        within_topic_footer_buttons { find(".bookmark-menu-trigger").click }
       end
 
-      def has_topic_bookmarked?
+      def has_topic_bookmarked?(topic)
-        has_css?("#{topic_footer_button_id("bookmark")}.bookmarked", text: "Edit Bookmark")
+        within_topic_footer_buttons do
+          has_css?(".bookmark-menu-trigger.bookmarked", text: "Edit Bookmark")
+        end
       end
 
-      def has_no_bookmarks?
+      def has_no_bookmarks?(topic)
-        has_no_css?("#{topic_footer_button_id("bookmark")}.bookmarked")
+        within_topic_footer_buttons { has_no_css?(".bookmark-menu-trigger.bookmarked") }
-      end
-
-      def find_topic_footer_button(button)
-        find(topic_footer_button_id(button))
       end
 
       def click_reply_button
-        find(".topic-footer-main-buttons > .create").click
+        within_topic_footer_buttons { find(".create").click }
         has_expanded_composer?
       end
 
@@ -186,9 +182,7 @@ module PageObjects
       end
 
       def click_mention(post, mention)
-        within post_by_number(post) do
+        within_post(post) { find("a.mention-group", text: mention).click }
-          find("a.mention-group", text: mention).click
-        end
       end
 
       def click_footer_reply
@@ -197,7 +191,7 @@ module PageObjects
       end
 
       def click_like_reaction_for(post)
-        post_by_number(post).find(".post-controls .actions .like").click
+        within_post(post) { find(".post-controls .actions .like").click }
       end
 
       def has_topic_map?
@@ -217,7 +211,7 @@ module PageObjects
       end
 
       def click_admin_menu_button
-        find("#topic-footer-buttons .topic-admin-menu-button").click
+        within_topic_footer_buttons { find(".topic-admin-menu-button").click }
       end
 
       def watch_topic
@@ -236,12 +230,16 @@ module PageObjects
 
       private
 
-      def topic_footer_button_id(button)
+      def within_post(post)
-        "#topic-footer-button-#{button}"
+        within(post_by_number(post)) { yield }
+      end
+
+      def within_topic_footer_buttons
+        within("#topic-footer-buttons") { yield }
       end
 
       def is_post_bookmarked(post, bookmarked:, with_reminder: false)
-        within post_by_number(post) do
+        within_post(post) do
           css_class = ".bookmark.bookmarked#{with_reminder ? ".with-reminder" : ""}"
           page.public_send(bookmarked ? :has_css? : :has_no_css?, css_class)
         end