FEATURE: Allow editing bookmark reminders (#9437)

Users can now edit the bookmark name and reminder time from their list of bookmarks.

We use "Custom" for the date and time in the modal because if the user set a reminder for "tomorrow" then edit the reminder "tomorrow", the definition of what "tomorrow" is has changed.

app/assets/javascripts/discourse/components/bookmark-actions-dropdown.js

@@ -20,6 +20,12 @@ export default DropdownSelectBoxComponent.extend({
         description: I18n.t(
           "post.bookmarks.actions.delete_bookmark.description"
         )
+      },
+      {
+        id: "edit",
+        icon: "pencil",
+        name: I18n.t("post.bookmarks.actions.edit_bookmark.name"),
+        description: I18n.t("post.bookmarks.actions.edit_bookmark.description")
       }
     ];
   }),
@@ -28,6 +34,8 @@ export default DropdownSelectBoxComponent.extend({
   onChange(selectedAction) {
     if (selectedAction === "remove") {
       this.removeBookmark(this.bookmark);
+    } else if (selectedAction === "edit") {
+      this.editBookmark(this.bookmark);
     }
   }
 });

app/assets/javascripts/discourse/controllers/bookmark.js

@@ -1,4 +1,5 @@
 import { and } from "@ember/object/computed";
+import { isPresent } from "@ember/utils";
 import { next } from "@ember/runloop";
 import Controller from "@ember/controller";
 import { Promise } from "rsvp";
@@ -7,7 +8,7 @@ import discourseComputed from "discourse-common/utils/decorators";
 import { popupAjaxError } from "discourse/lib/ajax-error";
 import { ajax } from "discourse/lib/ajax";
 import KeyboardShortcuts from "discourse/lib/keyboard-shortcuts";
-import { REMINDER_TYPES } from "discourse/lib/bookmark";
+import { formattedReminderTime, REMINDER_TYPES } from "discourse/lib/bookmark";
 
 // global shortcuts that interfere with these modal shortcuts, they are rebound when the
 // modal is closed
@@ -47,7 +48,6 @@ const BOOKMARK_BINDINGS = {
 export default Controller.extend(ModalFunctionality, {
   loading: false,
   errorMessage: null,
-  name: null,
   selectedReminderType: null,
   closeWithoutSaving: false,
   isSavingBookmarkManually: false,
@@ -62,7 +62,6 @@ export default Controller.extend(ModalFunctionality, {
   onShow() {
     this.setProperties({
       errorMessage: null,
-      name: null,
       selectedReminderType: REMINDER_TYPES.NONE,
       closeWithoutSaving: false,
       isSavingBookmarkManually: false,
@@ -76,9 +75,47 @@ export default Controller.extend(ModalFunctionality, {
     this.bindKeyboardShortcuts();
     this.loadLastUsedCustomReminderDatetime();
 
+    if (this.editingExistingBookmark()) {
+      this.initializeExistingBookmarkData();
+    } else {
       // make sure the input is cleared, otherwise the keyboard shortcut to toggle
       // bookmark for post ends up in the input
-    next(() => this.set("name", null));
+      next(() => this.set("model.name", null));
+    }
+  },
+
+  /**
+   * We always want to save the bookmark unless the user specifically
+   * 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));
+    }
+    if (this.onCloseWithoutSaving && this.closeWithoutSaving) {
+      this.onCloseWithoutSaving();
+    }
+  },
+
+  initializeExistingBookmarkData() {
+    if (this.existingBookmarkHasReminder()) {
+      let parsedReminderAt = this.parseCustomDateTime(this.model.reminderAt);
+      this.setProperties({
+        customReminderDate: parsedReminderAt.format("YYYY-MM-DD"),
+        customReminderTime: parsedReminderAt.format("HH:mm"),
+        selectedReminderType: REMINDER_TYPES.CUSTOM
+      });
+    }
+  },
+
+  editingExistingBookmark() {
+    return isPresent(this.model) && isPresent(this.model.id);
+  },
+
+  existingBookmarkHasReminder() {
+    return isPresent(this.model) && isPresent(this.model.reminderAt);
   },
 
   loadLastUsedCustomReminderDatetime() {
@@ -88,6 +125,7 @@ export default Controller.extend(ModalFunctionality, {
     if (lastTime && lastDate) {
       let parsed = this.parseCustomDateTime(lastDate, lastTime);
 
+      // can't set reminders in the past
       if (parsed < this.now()) {
         return;
       }
@@ -121,21 +159,11 @@ export default Controller.extend(ModalFunctionality, {
     KeyboardShortcuts.unpause(GLOBAL_SHORTCUTS_TO_PAUSE);
   },
 
-  // we always want to save the bookmark unless the user specifically
+  @discourseComputed("model.reminderAt")
-  // clicks the save or cancel button to mimic browser behaviour
+  showExistingReminderAt(existingReminderAt) {
-  onClose() {
+    return isPresent(existingReminderAt);
-    this.unbindKeyboardShortcuts();
-    this.restoreGlobalShortcuts();
-    if (!this.closeWithoutSaving && !this.isSavingBookmarkManually) {
-      this.saveBookmark().catch(e => this.handleSaveError(e));
-    }
-    if (this.onCloseWithoutSaving && this.closeWithoutSaving) {
-      this.onCloseWithoutSaving();
-    }
   },
 
-  showBookmarkReminderControls: true,
-
   @discourseComputed()
   showAtDesktop() {
     return (
@@ -177,6 +205,11 @@ export default Controller.extend(ModalFunctionality, {
     );
   },
 
+  @discourseComputed("model.reminderAt")
+  existingReminderAtFormatted(existingReminderAt) {
+    return formattedReminderTime(existingReminderAt, this.userTimezone);
+  },
+
   @discourseComputed()
   startNextBusinessWeekFormatted() {
     return this.nextWeek()
@@ -244,19 +277,32 @@ export default Controller.extend(ModalFunctionality, {
     const data = {
       reminder_type: reminderType,
       reminder_at: reminderAtISO,
-      name: this.name,
+      name: this.model.name,
-      post_id: this.model.postId
+      post_id: this.model.postId,
+      id: this.model.id
     };
 
+    if (this.editingExistingBookmark()) {
+      return ajax("/bookmarks/" + this.model.id, {
+        type: "PUT",
+        data
+      }).then(() => {
+        if (this.afterSave) {
+          this.afterSave(reminderAtISO, this.selectedReminderType);
+        }
+      });
+    } else {
       return ajax("/bookmarks", { type: "POST", data }).then(() => {
         if (this.afterSave) {
           this.afterSave(reminderAtISO, this.selectedReminderType);
         }
       });
+    }
   },
 
   parseCustomDateTime(date, time) {
-    return moment.tz(date + " " + time, this.userTimezone);
+    let dateTime = isPresent(time) ? date + " " + time : date;
+    return moment.tz(dateTime, this.userTimezone);
   },
 
   defaultCustomReminderTime() {

app/assets/javascripts/discourse/controllers/user-activity-bookmarks-with-reminders.js

@@ -1,4 +1,5 @@
 import Controller from "@ember/controller";
+import showModal from "discourse/lib/show-modal";
 import { Promise } from "rsvp";
 import { inject } from "@ember/controller";
 import discourseComputed from "discourse-common/utils/decorators";
@@ -35,9 +36,9 @@ export default Controller.extend({
       );
   },
 
-  @discourseComputed("loaded", "content.length")
+  @discourseComputed("loaded", "content.length", "noResultsHelp")
-  noContent(loaded, contentLength) {
+  noContent(loaded, contentLength, noResultsHelp) {
-    return loaded && contentLength === 0;
+    return loaded && contentLength === 0 && noResultsHelp !== null;
   },
 
   processLoadResponse(response) {
@@ -63,6 +64,22 @@ export default Controller.extend({
       return bookmark.destroy().then(() => this.loadItems());
     },
 
+    editBookmark(bookmark) {
+      let controller = showModal("bookmark", {
+        model: {
+          postId: bookmark.post_id,
+          id: bookmark.id,
+          reminderAt: bookmark.reminder_at,
+          name: bookmark.name
+        },
+        title: "post.bookmarks.edit",
+        modalClass: "bookmark-with-reminder"
+      });
+      controller.setProperties({
+        afterSave: () => this.loadItems()
+      });
+    },
+
     loadMore() {
       if (this.loadingMore) {
         return Promise.resolve();

app/assets/javascripts/discourse/models/bookmark.js

@@ -10,6 +10,7 @@ import { ajax } from "discourse/lib/ajax";
 import { Promise } from "rsvp";
 import RestModel from "discourse/models/rest";
 import discourseComputed from "discourse-common/utils/decorators";
+import { formattedReminderTime } from "discourse/lib/bookmark";
 
 const Bookmark = RestModel.extend({
   newBookmark: none("id"),
@@ -110,9 +111,10 @@ const Bookmark = RestModel.extend({
 
   @discourseComputed("reminder_at", "currentUser")
   formattedReminder(bookmarkReminderAt, currentUser) {
-    return moment
+    return formattedReminderTime(
-      .tz(bookmarkReminderAt, currentUser.resolvedTimezone())
+      bookmarkReminderAt,
-      .format(I18n.t("dates.long_with_year"));
+      currentUser.resolvedTimezone()
+    ).capitalize();
   },
 
   loadItems() {

app/assets/javascripts/discourse/templates/modal/bookmark.hbs

@@ -9,10 +9,16 @@
     {{/if}}
 
     <div class="control-group">
-      {{input value=name name="name" class="bookmark-name" placeholder=(i18n "post.bookmarks.name_placeholder")}}
+      {{input value=model.name name="name" class="bookmark-name" placeholder=(i18n "post.bookmarks.name_placeholder")}}
     </div>
 
-    {{#if showBookmarkReminderControls}}
+    {{#if showExistingReminderAt }}
+      <div class="alert alert-info existing-reminder-at-alert">
+        {{d-icon "far-clock"}}
+        <span>{{i18n "bookmarks.reminders.existing_reminder"}} {{existingReminderAtFormatted}}</span>
+      </div>
+    {{/if}}
+
     <div class="control-group">
       <label class="control-label" for="set_reminder">
         {{i18n "post.bookmarks.set_reminder"}}
@@ -90,7 +96,6 @@
         <div class="alert alert-info">{{html-safe (i18n "bookmarks.no_timezone" basePath=basePath)}}</div>
       {{/if}}
     </div>
-    {{/if}}
 
     <div class="control-group">
       {{d-button label="bookmarks.save" class="btn-primary" action=(action "saveAndClose")}}

app/assets/javascripts/discourse/templates/user/bookmarks.hbs

@@ -42,7 +42,11 @@
               <td>{{format-date bookmark.created_at format="tiny"}}</td>
               {{raw "list/activity-column" topic=bookmark class="num" tagName="td"}}
               <td>
-                {{bookmark-actions-dropdown bookmark=bookmark removeBookmark=(action "removeBookmark")}}
+                {{bookmark-actions-dropdown
+                  bookmark=bookmark
+                  removeBookmark=(action "removeBookmark")
+                  editBookmark=(action "editBookmark")
+                }}
               </td>
             </tr>
           {{/each}}

app/assets/stylesheets/common/components/bookmark-modal.scss

@@ -19,6 +19,16 @@
     }
   }
 
+  .existing-reminder-at-alert {
+    display: flex;
+    flex-direction: row;
+    align-items: center;
+
+    .d-icon {
+      margin-right: 1em;
+    }
+  }
+
   .custom-date-time-wrap {
     padding: 1em 1em 0.5em;
     border: 1px solid $primary-low;

app/controllers/bookmarks_controller.rb

@@ -26,4 +26,22 @@ class BookmarksController < ApplicationController
     BookmarkManager.new(current_user).destroy(params[:id])
     render json: success_json
   end
+
+  def update
+    params.require(:id)
+
+    bookmark_manager = BookmarkManager.new(current_user)
+    bookmark_manager.update(
+      bookmark_id: params[:id],
+      name: params[:name],
+      reminder_type: params[:reminder_type],
+      reminder_at: params[:reminder_at]
+    )
+
+    if bookmark_manager.errors.empty?
+      return render json: success_json
+    end
+
+    render json: failed_json.merge(errors: bookmark_manager.errors.full_messages), status: 400
+  end
 end

config/locales/client.en.yml

@@ -331,6 +331,7 @@ en:
         today_with_time: "today at %{time}"
         tomorrow_with_time: "tomorrow at %{time}"
         at_time: "at %{date_time}"
+        existing_reminder: "You have a reminder set for this bookmark which will be sent"
 
     drafts:
       resume: "Resume"
@@ -2716,6 +2717,7 @@ en:
 
       bookmarks:
         create: "Create bookmark"
+        edit: "Edit bookmark"
         created: "Created"
         name: "Name"
         name_placeholder: "What is this bookmark for?"
@@ -2724,6 +2726,9 @@ en:
           delete_bookmark:
             name: "Delete bookmark"
             description: "Removes the bookmark from your profile and stops all reminders for the bookmark"
+          edit_bookmark:
+            name: "Edit bookmark"
+            description: "Edit the bookmark name or change the reminder date and time"
 
     category:
       can: "can… "

config/routes.rb

@@ -606,7 +606,7 @@ Discourse::Application.routes.draw do
     end
   end
 
-  resources :bookmarks, only: %i[create destroy]
+  resources :bookmarks, only: %i[create destroy update]
 
   resources :notifications, except: :show do
     collection do

lib/bookmark_manager.rb

@@ -66,6 +66,24 @@ class BookmarkManager
     BookmarkReminderNotificationHandler.send_notification(bookmark)
   end
 
+  def update(bookmark_id:, name:, reminder_type:, reminder_at:)
+    bookmark = Bookmark.find_by(id: bookmark_id)
+
+    raise Discourse::NotFound if bookmark.blank?
+    raise Discourse::InvalidAccess.new if !Guardian.new(@user).can_edit?(bookmark)
+
+    if bookmark.errors.any?
+      return add_errors_from(bookmark)
+    end
+
+    bookmark.update(
+      name: name,
+      reminder_at: reminder_at,
+      reminder_type: reminder_type,
+      reminder_set_at: Time.zone.now
+    )
+  end
+
   private
 
   def clear_at_desktop_cache_if_required

lib/guardian/bookmark_guardian.rb

@@ -5,6 +5,10 @@ module BookmarkGuardian
     @user == bookmark.user
   end
 
+  def can_edit_bookmark?(bookmark)
+    @user == bookmark.user
+  end
+
   def can_create_bookmark?(bookmark)
     can_see_topic?(bookmark.topic)
   end

spec/lib/bookmark_manager_spec.rb

@@ -160,6 +160,43 @@ RSpec.describe BookmarkManager do
     end
   end
 
+  describe ".update" do
+    let!(:bookmark) { Fabricate(:bookmark_next_business_day_reminder, user: user, post: post, name: "Old name") }
+    let(:new_name) { "Some new name" }
+    let(:new_reminder_at) { 10.days.from_now }
+    let(:new_reminder_type) { Bookmark.reminder_types[:custom] }
+
+    def update_bookmark
+      subject.update(
+        bookmark_id: bookmark.id, name: new_name, reminder_type: new_reminder_type, reminder_at: new_reminder_at
+      )
+    end
+
+    it "saves the time and new reminder type sucessfully" do
+      update_bookmark
+       bookmark.reload
+       expect(bookmark.name).to eq(new_name)
+       expect(bookmark.reminder_at).to eq_time(new_reminder_at)
+       expect(bookmark.reminder_type).to eq(new_reminder_type)
+    end
+
+    context "if the bookmark is belonging to some other user" do
+      let!(:bookmark) { Fabricate(:bookmark, user: Fabricate(:admin), post: post) }
+      it "raises an invalid access error" do
+        expect { update_bookmark }.to raise_error(Discourse::InvalidAccess)
+      end
+    end
+
+    context "if the bookmark no longer exists" do
+      before do
+        bookmark.destroy!
+      end
+      it "raises an invalid access error" do
+        expect { update_bookmark }.to raise_error(Discourse::NotFound)
+      end
+    end
+  end
+
   describe ".destroy_for_topic" do
     let!(:topic) { Fabricate(:topic) }
     let!(:bookmark1) { Fabricate(:bookmark, topic: topic, post: Fabricate(:post, topic: topic), user: user) }

test/javascripts/controllers/bookmark-test.js

@@ -1,6 +1,7 @@
 import { logIn } from "helpers/qunit-helpers";
 import User from "discourse/models/user";
 import KeyboardShortcutInitializer from "discourse/initializers/keyboard-shortcuts";
+import { REMINDER_TYPES } from "discourse/lib/bookmark";
 let BookmarkController;
 
 moduleFor("controller:bookmark", {
@@ -263,3 +264,20 @@ QUnit.test("user timezone updates when the modal is shown", function(assert) {
   );
   stub.restore();
 });
+
+QUnit.test(
+  "opening the modal with an existing bookmark with reminder at prefills the custom reminder type",
+  function(assert) {
+    let name = "test";
+    let reminderAt = "2020-05-15T09:45:00";
+    BookmarkController.model = { id: 1, name: name, reminderAt: reminderAt };
+    BookmarkController.onShow();
+    assert.equal(
+      BookmarkController.selectedReminderType,
+      REMINDER_TYPES.CUSTOM
+    );
+    assert.equal(BookmarkController.customReminderDate, "2020-05-15");
+    assert.equal(BookmarkController.customReminderTime, "09:45");
+    assert.equal(BookmarkController.model.name, name);
+  }
+);