FEATURE: Edit bookmark reminders from post and explicit delete button (#9455)

There is now an explicit "Delete Bookmark" button in the edit modal. A confirmation is shown before deleting. Along with this, when the bookmarked post icon is clicked the modal is now shown instead of just deleting the bookmark. Also, the "Delete Bookmark" button from the user bookmark list now confirms the action. Add a `d d` shortcut in the modal to delete the bookmark.
2024-11-22 10:33:51 +08:00 · 2020-04-20 13:30:04 +10:00 · 2020-04-20 13:30:04 +10:00 · 344ef5226c
commit 344ef5226c
parent c6b411f6c1
15 changed files with 192 additions and 105 deletions
--- a/app/assets/javascripts/discourse/controllers/bookmark.js
+++ b/app/assets/javascripts/discourse/controllers/bookmark.js
@ -41,15 +41,16 @@ const BOOKMARK_BINDINGS = {
  },
  "n m": { handler: "selectReminderType", args: [REMINDER_TYPES.NEXT_MONTH] },
  "c r": { handler: "selectReminderType", args: [REMINDER_TYPES.CUSTOM] },
-  "n r": { handler: "selectReminderType", args: [REMINDER_TYPES.NONE] }
+  "n r": { handler: "selectReminderType", args: [REMINDER_TYPES.NONE] },
+  "d d": { handler: "delete" }
 };

 export default Controller.extend(ModalFunctionality, {
  loading: false,
  errorMessage: null,
  selectedReminderType: null,
-  closeWithoutSaving: false,
-  isSavingBookmarkManually: false,
+  _closeWithoutSaving: false,
+  _savingBookmarkManually: false,
  onCloseWithoutSaving: null,
  customReminderDate: null,
  customReminderTime: null,
@ -62,20 +63,20 @@ export default Controller.extend(ModalFunctionality, {
    this.setProperties({
      errorMessage: null,
      selectedReminderType: REMINDER_TYPES.NONE,
-      closeWithoutSaving: false,
-      isSavingBookmarkManually: false,
+      _closeWithoutSaving: false,
+      _savingBookmarkManually: false,
      customReminderDate: null,
-      customReminderTime: this.defaultCustomReminderTime(),
+      customReminderTime: this._defaultCustomReminderTime(),
      lastCustomReminderDate: null,
      lastCustomReminderTime: null,
      userTimezone: this.currentUser.resolvedTimezone()
    });

-    this.bindKeyboardShortcuts();
-    this.loadLastUsedCustomReminderDatetime();
+    this._bindKeyboardShortcuts();
+    this._loadLastUsedCustomReminderDatetime();

-    if (this.editingExistingBookmark()) {
-      this.initializeExistingBookmarkData();
+    if (this._editingExistingBookmark()) {
+      this._initializeExistingBookmarkData();
    }
  },

@ -84,19 +85,19 @@ export default Controller.extend(ModalFunctionality, {
   * clicks the save or cancel button to mimic browser behaviour.
   */
  onClose() {
-    this.unbindKeyboardShortcuts();
-    this.restoreGlobalShortcuts();
-    if (!this.closeWithoutSaving && !this.isSavingBookmarkManually) {
-      this.saveBookmark().catch(e => this.handleSaveError(e));
+    this._unbindKeyboardShortcuts();
+    this._restoreGlobalShortcuts();
+    if (!this._closeWithoutSaving && !this._savingBookmarkManually) {
+      this._saveBookmark().catch(e => this._handleSaveError(e));
    }
-    if (this.onCloseWithoutSaving && this.closeWithoutSaving) {
+    if (this.onCloseWithoutSaving && this._closeWithoutSaving) {
      this.onCloseWithoutSaving();
    }
  },

-  initializeExistingBookmarkData() {
-    if (this.existingBookmarkHasReminder()) {
-      let parsedReminderAt = this.parseCustomDateTime(this.model.reminderAt);
+  _initializeExistingBookmarkData() {
+    if (this._existingBookmarkHasReminder()) {
+      let parsedReminderAt = this._parseCustomDateTime(this.model.reminderAt);
      this.setProperties({
        customReminderDate: parsedReminderAt.format("YYYY-MM-DD"),
        customReminderTime: parsedReminderAt.format("HH:mm"),
@ -105,20 +106,20 @@ export default Controller.extend(ModalFunctionality, {
    }
  },

-  editingExistingBookmark() {
+  _editingExistingBookmark() {
    return isPresent(this.model) && isPresent(this.model.id);
  },

-  existingBookmarkHasReminder() {
+  _existingBookmarkHasReminder() {
    return isPresent(this.model) && isPresent(this.model.reminderAt);
  },

-  loadLastUsedCustomReminderDatetime() {
+  _loadLastUsedCustomReminderDatetime() {
    let lastTime = localStorage.lastCustomBookmarkReminderTime;
    let lastDate = localStorage.lastCustomBookmarkReminderDate;

    if (lastTime && lastDate) {
-      let parsed = this.parseCustomDateTime(lastDate, lastTime);
+      let parsed = this._parseCustomDateTime(lastDate, lastTime);

      // can't set reminders in the past
      if (parsed < this.now()) {
@ -133,7 +134,7 @@ export default Controller.extend(ModalFunctionality, {
    }
  },

-  bindKeyboardShortcuts() {
+  _bindKeyboardShortcuts() {
    KeyboardShortcuts.pause(GLOBAL_SHORTCUTS_TO_PAUSE);
    Object.keys(BOOKMARK_BINDINGS).forEach(shortcut => {
      KeyboardShortcuts.addShortcut(shortcut, () => {
@ -146,11 +147,11 @@ export default Controller.extend(ModalFunctionality, {
    });
  },

-  unbindKeyboardShortcuts() {
+  _unbindKeyboardShortcuts() {
    KeyboardShortcuts.unbind(BOOKMARK_BINDINGS);
  },

-  restoreGlobalShortcuts() {
+  _restoreGlobalShortcuts() {
    KeyboardShortcuts.unpause(GLOBAL_SHORTCUTS_TO_PAUSE);
  },

@ -159,6 +160,11 @@ export default Controller.extend(ModalFunctionality, {
    return isPresent(existingReminderAt);
  },

+  @discourseComputed("model.id")
+  showDelete(id) {
+    return isPresent(id);
+  },
+
  @discourseComputed()
  showAtDesktop() {
    return (
@ -247,8 +253,8 @@ export default Controller.extend(ModalFunctionality, {
    return !_.isEmpty(userTimezone);
  },

-  saveBookmark() {
-    const reminderAt = this.reminderAt();
+  _saveBookmark() {
+    const reminderAt = this._reminderAt();
    const reminderAtISO = reminderAt ? reminderAt.toISOString() : null;

    if (this.selectedReminderType === REMINDER_TYPES.CUSTOM) {
@ -277,34 +283,54 @@ export default Controller.extend(ModalFunctionality, {
      id: this.model.id
    };

-    if (this.editingExistingBookmark()) {
+    if (this._editingExistingBookmark()) {
      return ajax("/bookmarks/" + this.model.id, {
        type: "PUT",
        data
      }).then(() => {
        if (this.afterSave) {
-          this.afterSave(reminderAtISO, this.selectedReminderType);
+          this.afterSave({
+            reminderAt: reminderAtISO,
+            reminderType: this.selectedReminderType,
+            id: this.model.id,
+            name: this.model.name
+          });
        }
      });
    } else {
-      return ajax("/bookmarks", { type: "POST", data }).then(() => {
+      return ajax("/bookmarks", { type: "POST", data }).then(response => {
        if (this.afterSave) {
-          this.afterSave(reminderAtISO, this.selectedReminderType);
+          this.afterSave({
+            reminderAt: reminderAtISO,
+            reminderType: this.selectedReminderType,
+            id: response.id,
+            name: this.model.name
+          });
        }
      });
    }
  },

-  parseCustomDateTime(date, time) {
+  _deleteBookmark() {
+    return ajax("/bookmarks/" + this.model.id, {
+      type: "DELETE"
+    }).then(response => {
+      if (this.afterDelete) {
+        this.afterDelete(response.topic_bookmarked);
+      }
+    });
+  },
+
+  _parseCustomDateTime(date, time) {
    let dateTime = isPresent(time) ? date + " " + time : date;
    return moment.tz(dateTime, this.userTimezone);
  },

-  defaultCustomReminderTime() {
+  _defaultCustomReminderTime() {
    return `0${START_OF_DAY_HOUR}:00`;
  },

-  reminderAt() {
+  _reminderAt() {
    if (!this.selectedReminderType) {
      return;
    }
@ -329,9 +355,9 @@ export default Controller.extend(ModalFunctionality, {
      case REMINDER_TYPES.CUSTOM:
        this.set(
          "customReminderTime",
-          this.customReminderTime || this.defaultCustomReminderTime()
+          this.customReminderTime || this._defaultCustomReminderTime()
        );
-        const customDateTime = this.parseCustomDateTime(
+        const customDateTime = this._parseCustomDateTime(
          this.customReminderDate,
          this.customReminderTime
        );
@ -385,8 +411,8 @@ export default Controller.extend(ModalFunctionality, {
    return this.startOfDay(this.now().add(2, "days"));
  },

-  handleSaveError(e) {
-    this.isSavingBookmarkManually = false;
+  _handleSaveError(e) {
+    this._savingBookmarkManually = false;
    if (typeof e === "string") {
      bootbox.alert(e);
    } else {
@ -396,20 +422,35 @@ export default Controller.extend(ModalFunctionality, {

  actions: {
    saveAndClose() {
-      if (this.saving) {
+      if (this._saving || this._deleting) {
        return;
      }

-      this.saving = true;
-      this.isSavingBookmarkManually = true;
-      this.saveBookmark()
+      this._saving = true;
+      this._savingBookmarkManually = true;
+      this._saveBookmark()
        .then(() => this.send("closeModal"))
-        .catch(e => this.handleSaveError(e))
-        .finally(() => (this.saving = false));
+        .catch(e => this._handleSaveError(e))
+        .finally(() => (this._saving = false));
+    },
+
+    delete() {
+      this._deleting = true;
+      bootbox.confirm(I18n.t("bookmarks.confirm_delete"), result => {
+        if (result) {
+          this._closeWithoutSaving = true;
+          this._deleteBookmark()
+            .then(() => {
+              this._deleting = false;
+              this.send("closeModal");
+            })
+            .catch(e => this._handleSaveError(e));
+        }
+      });
    },

    closeWithoutSavingBookmark() {
-      this.closeWithoutSaving = true;
+      this._closeWithoutSaving = true;
      this.send("closeModal");
    },

--- a/app/assets/javascripts/discourse/controllers/keyboard-shortcuts-help.js
+++ b/app/assets/javascripts/discourse/controllers/keyboard-shortcuts-help.js
@ -170,6 +170,10 @@ export default Controller.extend(ModalFunctionality, {
        none: buildShortcut("bookmarks.none", {
          keys1: ["n", "r"],
          shortcutsDelimiter: "space"
+        }),
+        delete: buildShortcut("bookmarks.delete", {
+          keys1: ["d", "d"],
+          shortcutsDelimiter: "space"
        })
      },
      actions: {
--- a/app/assets/javascripts/discourse/controllers/user-activity-bookmarks-with-reminders.js
+++ b/app/assets/javascripts/discourse/controllers/user-activity-bookmarks-with-reminders.js
@ -61,7 +61,11 @@ export default Controller.extend({

  actions: {
    removeBookmark(bookmark) {
-      return bookmark.destroy().then(() => this.loadItems());
+      bootbox.confirm(I18n.t("bookmarks.confirm_delete"), result => {
+        if (result) {
+          return bookmark.destroy().then(() => this.loadItems());
+        }
+      });
    },

    editBookmark(bookmark) {
--- a/app/assets/javascripts/discourse/models/post.js
+++ b/app/assets/javascripts/discourse/models/post.js
@ -336,48 +336,49 @@ const Post = RestModel.extend({
  },

  toggleBookmarkWithReminder() {
-    this.toggleProperty("bookmarked_with_reminder");
-    if (this.bookmarked_with_reminder) {
-      return new Promise(resolve => {
-        let controller = showModal("bookmark", {
-          model: {
-            postId: this.id
-          },
-          title: "post.bookmarks.create",
-          modalClass: "bookmark-with-reminder"
-        });
-        controller.setProperties({
-          onCloseWithoutSaving: () => {
-            this.toggleProperty("bookmarked_with_reminder");
-            resolve({ closedWithoutSaving: true });
-            this.appEvents.trigger("post-stream:refresh", { id: this.id });
-          },
-          afterSave: (reminderAtISO, reminderType) => {
-            this.setProperties({
-              "topic.bookmarked": true,
-              bookmark_reminder_at: reminderAtISO,
-              bookmark_reminder_type: reminderType
-            });
-            resolve({ closedWithoutSaving: false });
-            this.appEvents.trigger("post-stream:refresh", { id: this.id });
-          }
-        });
+    return new Promise(resolve => {
+      let controller = showModal("bookmark", {
+        model: {
+          postId: this.id,
+          id: this.bookmark_id,
+          reminderAt: this.bookmark_reminder_at,
+          name: this.bookmark_name
+        },
+        title: this.bookmark_id
+          ? "post.bookmarks.edit"
+          : "post.bookmarks.create",
+        modalClass: "bookmark-with-reminder"
      });
-    } else {
-      this.setProperties({
-        bookmark_reminder_at: null,
-        bookmark_reminder_type: null
-      });
-      return Post.destroyBookmark(this.id)
-        .then(result => {
-          this.set("topic.bookmarked", result.topic_bookmarked);
+      controller.setProperties({
+        onCloseWithoutSaving: () => {
+          resolve({ closedWithoutSaving: true });
+          this.appEvents.trigger("post-stream:refresh", { id: this.id });
+        },
+        afterSave: savedData => {
+          this.setProperties({
+            "topic.bookmarked": true,
+            bookmarked_with_reminder: true,
+            bookmark_reminder_at: savedData.reminderAt,
+            bookmark_reminder_type: savedData.reminderType,
+            bookmark_name: savedData.name,
+            bookmark_id: savedData.id
+          });
+          resolve({ closedWithoutSaving: false });
+          this.appEvents.trigger("post-stream:refresh", { id: this.id });
+        },
+        afterDelete: topicBookmarked => {
+          this.set("topic.bookmarked", topicBookmarked);
+          this.setProperties({
+            bookmark_reminder_at: null,
+            bookmark_reminder_type: null,
+            bookmark_name: null,
+            bookmark_id: null,
+            bookmarked_with_reminder: false
+          });
          this.appEvents.trigger("page:bookmark-post-toggled", this);
-        })
-        .catch(error => {
-          this.toggleProperty("bookmarked_with_reminder");
-          throw new Error(error);
-        });
-    }
+        }
+      });
+    });
  },

  updateActionsSummary(json) {
--- a/app/assets/javascripts/discourse/models/topic.js
+++ b/app/assets/javascripts/discourse/models/topic.js
@ -461,6 +461,12 @@ const Topic = RestModel.extend({
            .then(() => {
              this.toggleProperty("bookmarked");
              this.set("bookmark_reminder_at", null);
+              let clearedBookmarkProps = {
+                bookmarked_with_reminder: false,
+                bookmark_id: null,
+                bookmark_name: null,
+                bookmark_reminder_at: null
+              };
              if (posts) {
                const updated = [];
                posts.forEach(post => {
@ -472,11 +478,11 @@ const Topic = RestModel.extend({
                    this.siteSettings.enable_bookmarks_with_reminders &&
                    post.bookmarked_with_reminder
                  ) {
-                    post.set("bookmarked_with_reminder", false);
+                    post.setProperties(clearedBookmarkProps);
                    updated.push(post.id);
                  }
                });
-                firstPost.set("bookmarked_with_reminder", false);
+                firstPost.setProperties(clearedBookmarkProps);
                return updated;
              }
            })
--- a/app/assets/javascripts/discourse/templates/modal/bookmark.hbs
+++ b/app/assets/javascripts/discourse/templates/modal/bookmark.hbs
@ -9,7 +9,7 @@
    {{/if}}

    <div class="control-group">
-      {{input value=model.name name="name" class="bookmark-name" placeholder=(i18n "post.bookmarks.name_placeholder")}}
+      {{input id="bookmark-name" value=model.name name="name" class="bookmark-name" placeholder=(i18n "post.bookmarks.name_placeholder")}}
    </div>

    {{#if showExistingReminderAt }}
@ -100,6 +100,11 @@
    <div class="control-group">
      {{d-button label="bookmarks.save" class="btn-primary" action=(action "saveAndClose")}}
      {{d-modal-cancel close=(action "closeWithoutSavingBookmark")}}
+      {{#if showDelete}}
+        <div class="pull-right">
+          {{d-button icon="trash" class="btn-danger" action=(action "delete")}}
+        </div>
+      {{/if}}
    </div>
  {{/conditional-loading-spinner}}
 {{/d-modal-body}}
--- a/app/assets/javascripts/discourse/templates/modal/keyboard-shortcuts-help.hbs
+++ b/app/assets/javascripts/discourse/templates/modal/keyboard-shortcuts-help.hbs
@ -69,6 +69,7 @@
          <li>{{html-safe shortcuts.bookmarks.next_business_day}}</li>
          <li>{{html-safe shortcuts.bookmarks.custom}}</li>
          <li>{{html-safe shortcuts.bookmarks.none}}</li>
+          <li>{{html-safe shortcuts.bookmarks.delete}}</li>
        </ul>
      </section>
    {{/if}}
--- a/app/controllers/bookmarks_controller.rb
+++ b/app/controllers/bookmarks_controller.rb
@ -15,7 +15,7 @@ class BookmarksController < ApplicationController
    )

    if bookmark_manager.errors.empty?
-      return render json: success_json
+      return render json: success_json.merge(id: bookmark.id)
    end

    render json: failed_json.merge(errors: bookmark_manager.errors.full_messages), status: 400
@ -23,8 +23,8 @@ class BookmarksController < ApplicationController

  def destroy
    params.require(:id)
-    BookmarkManager.new(current_user).destroy(params[:id])
-    render json: success_json
+    result = BookmarkManager.new(current_user).destroy(params[:id])
+    render json: success_json.merge(result)
  end

  def update
--- a/app/controllers/posts_controller.rb
+++ b/app/controllers/posts_controller.rb
@ -511,15 +511,10 @@ class PostsController < ApplicationController
  def destroy_bookmark
    params.require(:post_id)

-    existing_bookmark = Bookmark.find_by(post_id: params[:post_id], user_id: current_user.id)
-    topic_bookmarked = false
+    bookmark_id = Bookmark.where(post_id: params[:post_id], user_id: current_user.id).pluck_first(:id)
+    result = BookmarkManager.new(current_user).destroy(bookmark_id)

-    if existing_bookmark.present?
-      topic_bookmarked = Bookmark.exists?(topic_id: existing_bookmark.topic_id, user_id: current_user.id)
-      existing_bookmark.destroy
-    end
-
-    render json: success_json.merge(topic_bookmarked: topic_bookmarked)
+    render json: success_json.merge(result)
  end

  def wiki
--- a/app/serializers/post_serializer.rb
+++ b/app/serializers/post_serializer.rb
@ -51,7 +51,9 @@ class PostSerializer < BasicPostSerializer
             :bookmarked,
             :bookmarked_with_reminder,
             :bookmark_reminder_at,
+             :bookmark_id,
             :bookmark_reminder_type,
+             :bookmark_name,
             :raw,
             :actions_summary,
             :moderator?,
@ -334,6 +336,14 @@ class PostSerializer < BasicPostSerializer
    include_bookmarked_with_reminder?
  end

+  def include_bookmark_name?
+    include_bookmarked_with_reminder?
+  end
+
+  def include_bookmark_id?
+    include_bookmarked_with_reminder?
+  end
+
  def post_bookmark
    return nil if !SiteSetting.enable_bookmarks_with_reminders? || @topic_view.blank?
    @post_bookmark ||= @topic_view.user_post_bookmarks.find { |bookmark| bookmark.post_id == object.id }
@ -348,6 +358,14 @@ class PostSerializer < BasicPostSerializer
    Bookmark.reminder_types[post_bookmark.reminder_type].to_s
  end

+  def bookmark_name
+    post_bookmark&.name
+  end
+
+  def bookmark_id
+    post_bookmark&.id
+  end
+
  def include_display_username?
    SiteSetting.enable_names?
  end
--- a/config/locales/client.en.yml
+++ b/config/locales/client.en.yml
@ -311,6 +311,8 @@ en:
      created_with_reminder: "you've bookmarked this post with a reminder %{date}"
      created_with_at_desktop_reminder: "you've bookmarked this post and will be reminded next time you are at your desktop"
      remove: "Remove Bookmark"
+      delete: "Delete Bookmark"
+      confirm_delete: "Are you sure you want to delete this bookmark? If you set a reminder it will also be deleted."
      confirm_clear: "Are you sure you want to clear all your bookmarks from this topic?"
      save: "Save"
      no_timezone: 'You have not set a timezone yet. You will not be able to set reminders. Set one up <a href="%{basePath}/my/preferences/profile">in your profile</a>.'
@ -3118,6 +3120,7 @@ en:
        next_business_day: "%{shortcut} Next business day"
        custom: "%{shortcut} Custom date and time"
        none: "%{shortcut} No reminder"
+        delete: "%{shortcut} Delete bookmark"
      actions:
        title: "Actions"
        bookmark_topic: "%{shortcut} Toggle bookmark topic"
--- a/lib/bookmark_manager.rb
+++ b/lib/bookmark_manager.rb
@ -44,6 +44,8 @@ class BookmarkManager

    bookmark.destroy
    clear_at_desktop_cache_if_required
+
+    { topic_bookmarked: Bookmark.exists?(topic_id: bookmark.topic_id, user: @user) }
  end

  def destroy_for_topic(topic)
--- a/spec/lib/bookmark_manager_spec.rb
+++ b/spec/lib/bookmark_manager_spec.rb
@ -123,8 +123,15 @@ RSpec.describe BookmarkManager do
  describe ".destroy" do
    let!(:bookmark) { Fabricate(:bookmark, user: user, post: post) }
    it "deletes the existing bookmark" do
-      subject.destroy(bookmark.id)
+      result = subject.destroy(bookmark.id)
      expect(Bookmark.exists?(id: bookmark.id)).to eq(false)
+      expect(result[:topic_bookmarked]).to eq(false)
+    end
+
+    it "returns a value indicating whether there are still other bookmarks in the topic for the user" do
+      Fabricate(:bookmark, user: user, post: Fabricate(:post, topic: post.topic))
+      result = subject.destroy(bookmark.id)
+      expect(result[:topic_bookmarked]).to eq(true)
    end

    context "if the bookmark is belonging to some other user" do
--- a/spec/requests/posts_controller_spec.rb
+++ b/spec/requests/posts_controller_spec.rb
@ -473,7 +473,7 @@ describe PostsController do

    context "when the user still has bookmarks in the topic" do
      before do
-        Fabricate(:bookmark, user: user, post: Fabricate(:post, topic: post.topic), topic: topic)
+        Fabricate(:bookmark, user: user, post: Fabricate(:post, topic: post.topic), topic: post.topic)
      end
      it "marks topic_bookmaked as true" do
        delete "/posts/#{post.id}/bookmark.json"
--- a/test/javascripts/controllers/bookmark-test.js
+++ b/test/javascripts/controllers/bookmark-test.js
@ -201,7 +201,7 @@ QUnit.test(
    BookmarkController.customReminderDate = "2028-12-12";
    BookmarkController.selectedReminderType =
      BookmarkController.reminderTypes.CUSTOM;
-    const reminderAt = BookmarkController.reminderAt();
+    const reminderAt = BookmarkController._reminderAt();
    assert.equal(BookmarkController.customReminderTime, "08:00");
    assert.equal(
      reminderAt.toString(),
@ -223,7 +223,7 @@ QUnit.test(
    localStorage.lastCustomBookmarkReminderDate = "2019-12-12";
    localStorage.lastCustomBookmarkReminderTime = "08:00";

-    BookmarkController.loadLastUsedCustomReminderDatetime();
+    BookmarkController._loadLastUsedCustomReminderDatetime();

    assert.equal(BookmarkController.lastCustomReminderDate, "2019-12-12");
    assert.equal(BookmarkController.lastCustomReminderTime, "08:00");
@ -237,7 +237,7 @@ QUnit.test(
    localStorage.lastCustomBookmarkReminderDate = "2019-12-11";
    localStorage.lastCustomBookmarkReminderTime = "07:00";

-    BookmarkController.loadLastUsedCustomReminderDatetime();
+    BookmarkController._loadLastUsedCustomReminderDatetime();

    assert.equal(BookmarkController.lastCustomReminderDate, null);
    assert.equal(BookmarkController.lastCustomReminderTime, null);