FEATURE: Optionally delete bookmark when reminder sent (#9637)

We now show an options gear icon next to the bookmark name. When expanded we show the "delete bookmark when reminder sent" option. The value of this checkbox is saved in local storage for the user. If this is ticked, when a reminder is sent for the bookmark the bookmark itself is deleted. This is so people can use the reminder functionality by itself. Also remove the blue alert reminder section from the "Edit Bookmark" modal as it just added clutter, because the user can already see they had a reminder set: Adds a default false boolean column `delete_when_reminder_sent` to bookmarks.
2024-11-22 10:33:51 +08:00 · 2020-05-07 13:37:39 +10:00 · 2020-05-07 13:37:39 +10:00 · 6fb0f36ce1
commit 6fb0f36ce1
parent 423802fbce
14 changed files with 196 additions and 31 deletions
--- a/app/assets/javascripts/discourse/app/controllers/bookmark.js
+++ b/app/assets/javascripts/discourse/app/controllers/bookmark.js
@ -1,4 +1,5 @@
 import { and } from "@ember/object/computed";
+import { next } from "@ember/runloop";
 import { action } from "@ember/object";
 import { isPresent } from "@ember/utils";
 import Controller from "@ember/controller";
@ -59,6 +60,8 @@ export default Controller.extend(ModalFunctionality, {
  lastCustomReminderTime: null,
  mouseTrap: null,
  userTimezone: null,
+  showOptions: false,
+  options: null,

  onShow() {
    this.setProperties({
@ -70,9 +73,13 @@ export default Controller.extend(ModalFunctionality, {
      customReminderTime: this._defaultCustomReminderTime(),
      lastCustomReminderDate: null,
      lastCustomReminderTime: null,
-      userTimezone: this.currentUser.resolvedTimezone()
+      userTimezone: this.currentUser.resolvedTimezone(),
+      showOptions: false,
+      options: {},
+      model: this.model || {}
    });

+    this._loadBookmarkOptions();
    this._bindKeyboardShortcuts();
    this._loadLastUsedCustomReminderDatetime();

@ -124,6 +131,21 @@ export default Controller.extend(ModalFunctionality, {
    return isPresent(this.model) && isPresent(this.model.reminderAt);
  },

+  _loadBookmarkOptions() {
+    this.set(
+      "options.deleteWhenReminderSent",
+      this.model.deleteWhenReminderSent ||
+        localStorage.bookmarkOptionsDeleteWhenReminderSent === "true"
+    );
+
+    // we want to make sure the options panel opens so the user
+    // knows they have set these options previously. run next otherwise
+    // the modal is not visible when it tries to slide down the options
+    if (this.options.deleteWhenReminderSent) {
+      next(() => this.toggleOptionsPanel());
+    }
+  },
+
  _loadLastUsedCustomReminderDatetime() {
    let lastTime = localStorage.lastCustomBookmarkReminderTime;
    let lastDate = localStorage.lastCustomBookmarkReminderDate;
@ -268,6 +290,8 @@ export default Controller.extend(ModalFunctionality, {
      localStorage.lastCustomBookmarkReminderDate = this.customReminderDate;
    }

+    localStorage.bookmarkOptionsDeleteWhenReminderSent = this.options.deleteWhenReminderSent;
+
    let reminderType;
    if (this.selectedReminderType === REMINDER_TYPES.NONE) {
      reminderType = null;
@ -282,7 +306,8 @@ export default Controller.extend(ModalFunctionality, {
      reminder_at: reminderAtISO,
      name: this.model.name,
      post_id: this.model.postId,
-      id: this.model.id
+      id: this.model.id,
+      delete_when_reminder_sent: this.options.deleteWhenReminderSent
    };

    if (this._editingExistingBookmark()) {
@ -294,6 +319,7 @@ export default Controller.extend(ModalFunctionality, {
          this.afterSave({
            reminderAt: reminderAtISO,
            reminderType: this.selectedReminderType,
+            deleteWhenReminderSent: this.options.deleteWhenReminderSent,
            id: this.model.id,
            name: this.model.name
          });
@ -305,6 +331,7 @@ export default Controller.extend(ModalFunctionality, {
          this.afterSave({
            reminderAt: reminderAtISO,
            reminderType: this.selectedReminderType,
+            deleteWhenReminderSent: this.options.deleteWhenReminderSent,
            id: response.id,
            name: this.model.name
          });
@ -420,6 +447,16 @@ export default Controller.extend(ModalFunctionality, {
    }
  },

+  @action
+  toggleOptionsPanel() {
+    if (this.showOptions) {
+      $(".bookmark-options-panel").slideUp("fast");
+    } else {
+      $(".bookmark-options-panel").slideDown("fast");
+    }
+    this.toggleProperty("showOptions");
+  },
+
  @action
  saveAndClose() {
    if (this._saving || this._deleting) {
--- a/app/assets/javascripts/discourse/app/models/post.js
+++ b/app/assets/javascripts/discourse/app/models/post.js
@ -316,6 +316,7 @@ const Post = RestModel.extend({
          postId: this.id,
          id: this.bookmark_id,
          reminderAt: this.bookmark_reminder_at,
+          deleteWhenReminderSent: this.bookmark_delete_when_reminder_sent,
          name: this.bookmark_name
        },
        title: this.bookmark_id
@ -334,6 +335,8 @@ const Post = RestModel.extend({
            bookmarked: true,
            bookmark_reminder_at: savedData.reminderAt,
            bookmark_reminder_type: savedData.reminderType,
+            bookmark_delete_when_reminder_sent:
+              savedData.deleteWhenReminderSent,
            bookmark_name: savedData.name,
            bookmark_id: savedData.id
          });
--- a/app/assets/javascripts/discourse/app/templates/modal/bookmark.hbs
+++ b/app/assets/javascripts/discourse/app/templates/modal/bookmark.hbs
@ -8,8 +8,16 @@
      </div>
    {{/if}}

-    <div class="control-group">
+    <div class="control-group bookmark-name-wrap">
      {{input id="bookmark-name" value=model.name name="name" class="bookmark-name" placeholder=(i18n "post.bookmarks.name_placeholder")}}
+      {{d-button icon="gear" action=(action "toggleOptionsPanel") class="bookmark-options-button"}}
+    </div>
+
+    <div class="bookmark-options-panel">
+      <label for="delete_when_reminder_sent">
+        {{i18n "bookmarks.delete_when_reminder_sent"}}
+        {{input id="delete_when_reminder_sent" type="checkbox" checked=options.deleteWhenReminderSent}}
+      </label>
    </div>

    {{#if showExistingReminderAt }}
--- a/app/assets/stylesheets/common/components/bookmark-modal.scss
+++ b/app/assets/stylesheets/common/components/bookmark-modal.scss
@ -62,4 +62,24 @@
      flex: 1 1 auto;
    }
  }
+
+  .bookmark-name-wrap {
+    display: inline-flex;
+    width: 100%;
+    align-items: end;
+  }
+
+  .bookmark-options-button {
+    margin-left: 0.5em;
+    background: transparent;
+  }
+
+  .bookmark-options-panel {
+    margin-bottom: 18px;
+    display: none;
+
+    input[type="checkbox"] {
+      margin-right: 0.85em;
+    }
+  }
 }
--- a/app/controllers/bookmarks_controller.rb
+++ b/app/controllers/bookmarks_controller.rb
@ -11,7 +11,10 @@ class BookmarksController < ApplicationController
      post_id: params[:post_id],
      name: params[:name],
      reminder_type: params[:reminder_type],
-      reminder_at: params[:reminder_at]
+      reminder_at: params[:reminder_at],
+      options: {
+        delete_when_reminder_sent: params[:delete_when_reminder_sent]
+      }
    )

    if bookmark_manager.errors.empty?
@ -35,7 +38,10 @@ class BookmarksController < ApplicationController
      bookmark_id: params[:id],
      name: params[:name],
      reminder_type: params[:reminder_type],
-      reminder_at: params[:reminder_at]
+      reminder_at: params[:reminder_at],
+      options: {
+        delete_when_reminder_sent: params[:delete_when_reminder_sent]
+      }
    )

    if bookmark_manager.errors.empty?
--- a/app/models/bookmark.rb
+++ b/app/models/bookmark.rb
@ -79,17 +79,18 @@ end
 #
 # Table name: bookmarks
 #
-#  id                    :bigint           not null, primary key
-#  user_id               :bigint           not null
-#  topic_id              :bigint           not null
-#  post_id               :bigint           not null
-#  name                  :string
-#  reminder_type         :integer
-#  reminder_at           :datetime
-#  created_at            :datetime         not null
-#  updated_at            :datetime         not null
-#  reminder_last_sent_at :datetime
-#  reminder_set_at       :datetime
+#  id                        :bigint           not null, primary key
+#  user_id                   :bigint           not null
+#  topic_id                  :bigint           not null
+#  post_id                   :bigint           not null
+#  name                      :string
+#  reminder_type             :integer
+#  reminder_at               :datetime
+#  created_at                :datetime         not null
+#  updated_at                :datetime         not null
+#  reminder_last_sent_at     :datetime
+#  reminder_set_at           :datetime
+#  delete_when_reminder_sent :boolean          default(FALSE)
 #
 # Indexes
 #
--- a/app/serializers/post_serializer.rb
+++ b/app/serializers/post_serializer.rb
@ -53,6 +53,7 @@ class PostSerializer < BasicPostSerializer
             :bookmark_id,
             :bookmark_reminder_type,
             :bookmark_name,
+             :bookmark_delete_when_reminder_sent,
             :raw,
             :actions_summary,
             :moderator?,
@ -331,6 +332,10 @@ class PostSerializer < BasicPostSerializer
    include_bookmarked?
  end

+  def include_bookmark_delete_when_reminder_sent?
+    include_bookmarked?
+  end
+
  def include_bookmark_id?
    include_bookmarked?
  end
@ -353,6 +358,10 @@ class PostSerializer < BasicPostSerializer
    post_bookmark&.name
  end

+  def bookmark_delete_when_reminder_sent
+    post_bookmark&.delete_when_reminder_sent
+  end
+
  def bookmark_id
    post_bookmark&.id
  end
--- a/config/locales/client.en.yml
+++ b/config/locales/client.en.yml
@ -319,6 +319,7 @@ en:
      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>.'
      invalid_custom_datetime: "The date and time you provided is invalid, please try again."
      list_permission_denied: "You do not have permission to view this user's bookmarks."
+      delete_when_reminder_sent: "Delete this bookmark when the reminder notification is sent."
      reminders:
        at_desktop: "Next time I'm at my desktop"
        later_today: "Later today"
--- a/db/migrate/20200505060712_add_delete_when_reminder_sent_boolean_to_bookmarks.rb
+++ b/db/migrate/20200505060712_add_delete_when_reminder_sent_boolean_to_bookmarks.rb
@ -0,0 +1,7 @@
+# frozen_string_literal: true
+
+class AddDeleteWhenReminderSentBooleanToBookmarks < ActiveRecord::Migration[6.0]
+  def change
+    add_column :bookmarks, :delete_when_reminder_sent, :boolean, nullabe: false, default: false
+  end
+end
--- a/lib/bookmark_manager.rb
+++ b/lib/bookmark_manager.rb
@ -7,20 +7,22 @@ class BookmarkManager
    @user = user
  end

-  def create(post_id:, name: nil, reminder_type: nil, reminder_at: nil)
+  def create(post_id:, name: nil, reminder_type: nil, reminder_at: nil, options: {})
    post = Post.unscoped.includes(:topic).find(post_id)
    reminder_type = parse_reminder_type(reminder_type)

    raise Discourse::InvalidAccess.new if !Guardian.new(@user).can_see_post?(post)

    bookmark = Bookmark.create(
-      user_id: @user.id,
-      topic: post.topic,
-      post: post,
-      name: name,
-      reminder_type: reminder_type,
-      reminder_at: reminder_at,
-      reminder_set_at: Time.zone.now
+      {
+        user_id: @user.id,
+        topic: post.topic,
+        post: post,
+        name: name,
+        reminder_type: reminder_type,
+        reminder_at: reminder_at,
+        reminder_set_at: Time.zone.now
+      }.merge(options)
    )

    if bookmark.errors.any?
@ -66,7 +68,7 @@ class BookmarkManager
    BookmarkReminderNotificationHandler.send_notification(bookmark)
  end

-  def update(bookmark_id:, name:, reminder_type:, reminder_at:)
+  def update(bookmark_id:, name:, reminder_type:, reminder_at:, options: {})
    bookmark = Bookmark.find_by(id: bookmark_id)

    raise Discourse::NotFound if bookmark.blank?
@ -75,10 +77,12 @@ class BookmarkManager
    reminder_type = parse_reminder_type(reminder_type)

    success = bookmark.update(
-      name: name,
-      reminder_at: reminder_at,
-      reminder_type: reminder_type,
-      reminder_set_at: Time.zone.now
+      {
+        name: name,
+        reminder_at: reminder_at,
+        reminder_type: reminder_type,
+        reminder_set_at: Time.zone.now
+      }.merge(options)
    )

    if bookmark.errors.any?
--- a/lib/bookmark_reminder_notification_handler.rb
+++ b/lib/bookmark_reminder_notification_handler.rb
@ -9,6 +9,11 @@ class BookmarkReminderNotificationHandler
      end

      create_notification(bookmark)
+
+      if bookmark.delete_when_reminder_sent?
+        return bookmark.destroy
+      end
+
      clear_reminder(bookmark)
    end
  end
--- a/spec/lib/bookmark_manager_spec.rb
+++ b/spec/lib/bookmark_manager_spec.rb
@ -42,6 +42,17 @@ RSpec.describe BookmarkManager do
      end
    end

+    context "when options are provided" do
+      let(:options) { { delete_when_reminder_sent: true } }
+
+      it "saves any additional options successfully" do
+        subject.create(post_id: post.id, name: name, options: options)
+        bookmark = Bookmark.find_by(user: user)
+
+        expect(bookmark.delete_when_reminder_sent).to eq(true)
+      end
+    end
+
    context "when the bookmark already exists for the user & post" do
      before do
        Bookmark.create(post: post, user: user, topic: post.topic)
@ -142,14 +153,19 @@ RSpec.describe BookmarkManager do
    let(:new_name) { "Some new name" }
    let(:new_reminder_at) { 10.days.from_now }
    let(:new_reminder_type) { Bookmark.reminder_types[:custom] }
+    let(:options) { {} }

    def update_bookmark
      subject.update(
-        bookmark_id: bookmark.id, name: new_name, reminder_type: new_reminder_type, reminder_at: new_reminder_at
+        bookmark_id: bookmark.id,
+        name: new_name,
+        reminder_type: new_reminder_type,
+        reminder_at: new_reminder_at,
+        options: options
      )
    end

-    it "saves the time and new reminder type sucessfully" do
+    it "saves the time and new reminder type and new name sucessfully" do
      update_bookmark
      bookmark.reload
      expect(bookmark.name).to eq(new_name)
@ -157,6 +173,16 @@ RSpec.describe BookmarkManager do
      expect(bookmark.reminder_type).to eq(new_reminder_type)
    end

+    context "when options are provided" do
+      let(:options) { { delete_when_reminder_sent: true } }
+
+      it "saves any additional options successfully" do
+        update_bookmark
+        bookmark.reload
+        expect(bookmark.delete_when_reminder_sent).to eq(true)
+      end
+    end
+
    context "if the new reminder type is a string" do
      let(:new_reminder_type) { "custom" }
      it "is parsed" do
--- a/spec/lib/bookmark_reminder_notification_handler_spec.rb
+++ b/spec/lib/bookmark_reminder_notification_handler_spec.rb
@ -28,6 +28,20 @@ RSpec.describe BookmarkReminderNotificationHandler do
      expect(data["bookmark_name"]).to eq(bookmark.name)
    end

+    it "clears the reminder" do
+      subject.send_notification(bookmark)
+      bookmark.reload
+      expect(bookmark.reload.no_reminder?).to eq(true)
+    end
+
+    context "when the delete_when_reminder_sent boolean is true " do
+      it "deletes the bookmark after the reminder gets sent" do
+        bookmark.update(delete_when_reminder_sent: true)
+        subject.send_notification(bookmark)
+        expect(Bookmark.find_by(id: bookmark.id)).to eq(nil)
+      end
+    end
+
    context "when the post has been deleted" do
      it "clears the reminder and does not send a notification" do
        bookmark.post.trash!
--- a/test/javascripts/acceptance/bookmarks-test.js
+++ b/test/javascripts/acceptance/bookmarks-test.js
@ -106,6 +106,30 @@ test("Saving a bookmark with a reminder", async assert => {
  assert.verifySteps(["tomorrow"]);
 });

+test("Opening the options panel and remembering the option", async assert => {
+  mockSuccessfulBookmarkPost(assert);
+  await visit("/t/internationalization-localization/280");
+  await openBookmarkModal();
+  await click(".bookmark-options-button");
+  assert.ok(
+    exists(".bookmark-options-panel"),
+    "it should open the options panel"
+  );
+  await click("#delete_when_reminder_sent");
+  await click("#save-bookmark");
+  await openEditBookmarkModal();
+
+  assert.ok(
+    exists(".bookmark-options-panel"),
+    "it should reopen the options panel"
+  );
+  assert.ok(
+    exists(".bookmark-options-panel #delete_when_reminder_sent:checked"),
+    "it should pre-check delete when reminder sent option"
+  );
+  assert.verifySteps(["none"]);
+});
+
 test("Saving a bookmark with no reminder or name", async assert => {
  mockSuccessfulBookmarkPost(assert);
  await visit("/t/internationalization-localization/280");