FEATURE: Allow custom date + time for bookmark reminders (#9185)

A custom date and time can now be selected for a bookmark reminder

The reminder will not happen at the exact time but rather at the next 5 minute interval of the bookmark reminder schedule.

This PR also fixes issues with bulk deleting topic bookmarks.

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

@@ -1,4 +1,5 @@
 import Controller from "@ember/controller";
+import { Promise } from "rsvp";
 import ModalFunctionality from "discourse/mixins/modal-functionality";
 import discourseComputed from "discourse-common/utils/decorators";
 import { popupAjaxError } from "discourse/lib/ajax-error";
@@ -25,6 +26,8 @@ export default Controller.extend(ModalFunctionality, {
   closeWithoutSaving: false,
   isSavingBookmarkManually: false,
   onCloseWithoutSaving: null,
+  customReminderDate: null,
+  customReminderTime: null,
 
   onShow() {
     this.setProperties({
@@ -32,7 +35,9 @@ export default Controller.extend(ModalFunctionality, {
       name: null,
       selectedReminderType: null,
       closeWithoutSaving: false,
-      isSavingBookmarkManually: false
+      isSavingBookmarkManually: false,
+      customReminderDate: null,
+      customReminderTime: null
     });
   },
 
@@ -40,7 +45,7 @@ export default Controller.extend(ModalFunctionality, {
   // clicks the save or cancel button to mimic browser behaviour
   onClose() {
     if (!this.closeWithoutSaving && !this.isSavingBookmarkManually) {
-      this.saveBookmark();
+      this.saveBookmark().catch(e => this.handleSaveError(e));
     }
     if (this.onCloseWithoutSaving && this.closeWithoutSaving) {
       this.onCloseWithoutSaving();
@@ -50,6 +55,11 @@ export default Controller.extend(ModalFunctionality, {
   usingMobileDevice: reads("site.mobileView"),
   showBookmarkReminderControls: true,
 
+  @discourseComputed("selectedReminderType")
+  customDateTimeSelected(selectedReminderType) {
+    return selectedReminderType === REMINDER_TYPES.CUSTOM;
+  },
+
   @discourseComputed()
   reminderTypes: () => {
     return REMINDER_TYPES;
@@ -113,6 +123,11 @@ export default Controller.extend(ModalFunctionality, {
   saveBookmark() {
     const reminderAt = this.reminderAt();
     const reminderAtISO = reminderAt ? reminderAt.toISOString() : null;
+
+    if (!reminderAt) {
+      return Promise.reject(I18n.t("bookmarks.invalid_custom_datetime"));
+    }
+
     const data = {
       reminder_type: this.selectedReminderType,
       reminder_at: reminderAtISO,
@@ -134,8 +149,7 @@ export default Controller.extend(ModalFunctionality, {
 
     switch (this.selectedReminderType) {
       case REMINDER_TYPES.AT_DESKTOP:
-        // TODO: Implement at desktop bookmark reminder functionality
-        return "";
+        return null;
       case REMINDER_TYPES.LATER_TODAY:
         return this.laterToday();
       case REMINDER_TYPES.NEXT_BUSINESS_DAY:
@@ -147,8 +161,18 @@ export default Controller.extend(ModalFunctionality, {
       case REMINDER_TYPES.NEXT_MONTH:
         return this.nextMonth();
       case REMINDER_TYPES.CUSTOM:
-        // TODO: Implement custom bookmark reminder times
-        return "";
+        const customDateTime = moment.tz(
+          this.customReminderDate + " " + this.customReminderTime,
+          this.userTimezone()
+        );
+        if (!customDateTime.isValid()) {
+          this.setProperties({
+            customReminderTime: null,
+            customReminderDate: null
+          });
+          return;
+        }
+        return customDateTime;
     }
   },
 
@@ -200,15 +224,21 @@ export default Controller.extend(ModalFunctionality, {
       : later.add(30, "minutes").startOf("hour");
   },
 
+  handleSaveError(e) {
+    this.isSavingBookmarkManually = false;
+    if (typeof e === "string") {
+      bootbox.alert(e);
+    } else {
+      popupAjaxError(e);
+    }
+  },
+
   actions: {
     saveAndClose() {
       this.isSavingBookmarkManually = true;
       this.saveBookmark()
         .then(() => this.send("closeModal"))
-        .catch(e => {
-          this.isSavingBookmarkManually = false;
-          popupAjaxError(e);
-        });
+        .catch(e => this.handleSaveError(e));
     },
 
     closeWithoutSavingBookmark() {

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

@@ -35,8 +35,20 @@
           {{tap-tile icon="far-sun" text=tomorrowFormatted tileId=reminderTypes.TOMORROW activeTile=grid.activeTile onChange=(action "selectReminderType")}}
           {{tap-tile icon="far-clock" text=nextWeekFormatted tileId=reminderTypes.NEXT_WEEK activeTile=grid.activeTile onChange=(action "selectReminderType")}}
           {{tap-tile icon="far-calendar-plus" text=nextMonthFormatted tileId=reminderTypes.NEXT_MONTH activeTile=grid.activeTile onChange=(action "selectReminderType")}}
-          <!-- {{tap-tile icon="calendar-alt" text=(I18n "bookmarks.reminders.custom") tileId=reminderTypes.CUSTOM activeTile=grid.activeTile onChange=(action "selectReminderType")}} -->
+          {{tap-tile icon="calendar-alt" text=(I18n "bookmarks.reminders.custom") tileId=reminderTypes.CUSTOM activeTile=grid.activeTile onChange=(action "selectReminderType")}}
         {{/tap-tile-grid}}
+        {{#if customDateTimeSelected}}
+          <div class="control-group">
+            {{d-icon "calendar-alt"}}
+            {{date-picker-future
+              value=customReminderDate
+              onSelect=(action (mut customReminderDate))
+            }}
+            {{d-icon "far-clock"}}
+            {{input placeholder="--:--" type="time" value=customReminderTime}}
+          </div>
+        {{/if}}
+
       {{else}}
         <div class="alert alert-info">{{i18n "bookmarks.no_timezone" basePath=basePath }}</div>
       {{/if}}

app/controllers/posts_controller.rb

@@ -512,9 +512,13 @@ class PostsController < ApplicationController
     params.require(:post_id)
 
     existing_bookmark = Bookmark.find_by(post_id: params[:post_id], user_id: current_user.id)
-    existing_bookmark.destroy if existing_bookmark.present?
+    topic_bookmarked = false
+
+    if existing_bookmark.present?
+      topic_bookmarked = Bookmark.exists?(topic_id: existing_bookmark.topic_id, user_id: current_user.id)
+      existing_bookmark.destroy
+    end
 
-    topic_bookmarked = Bookmark.exists?(topic_id: existing_bookmark.topic_id, user_id: current_user.id)
     render json: success_json.merge(topic_bookmarked: topic_bookmarked)
   end
 

config/locales/client.en.yml

@@ -311,6 +311,7 @@ en:
       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>.'
+      invalid_custom_datetime: "The date and time you provided is invalid, please try again."
       reminders:
         at_desktop: "Next time I'm at my desktop"
         later_today: "Later today <br/>{{date}}"

lib/bookmark_manager.rb

@@ -43,7 +43,7 @@ class BookmarkManager
 
     Bookmark.transaction do
       topic_bookmarks.each do |bookmark|
-        raise Discourse::InvalidAccess.new if !Guardian.new(user).can_delete?(bookmark)
+        raise Discourse::InvalidAccess.new if !Guardian.new(@user).can_delete?(bookmark)
         bookmark.destroy
       end
     end

spec/lib/bookmark_manager_spec.rb

@@ -134,8 +134,8 @@ RSpec.describe BookmarkManager do
 
   describe ".destroy_for_topic" do
     let!(:topic) { Fabricate(:topic) }
-    let(:bookmark1) { Fabricate(:bookmark, topic: topic, post: Fabricate(:post, topic: topic), user: user) }
-    let(:bookmark2) { Fabricate(:bookmark, topic: topic, post: Fabricate(:post, topic: topic), user: user) }
+    let!(:bookmark1) { Fabricate(:bookmark, topic: topic, post: Fabricate(:post, topic: topic), user: user) }
+    let!(:bookmark2) { Fabricate(:bookmark, topic: topic, post: Fabricate(:post, topic: topic), user: user) }
 
     it "destroys all bookmarks for the topic for the specified user" do
       subject.destroy_for_topic(topic)

spec/requests/posts_controller_spec.rb

@@ -457,6 +457,31 @@ describe PostsController do
     end
   end
 
+  describe "#destroy_bookmark" do
+    fab!(:post) { Fabricate(:post) }
+    fab!(:bookmark) { Fabricate(:bookmark, user: user, post: post, topic: post.topic) }
+
+    before do
+      sign_in(user)
+    end
+
+    it "deletes the bookmark" do
+      bookmark_id = bookmark.id
+      delete "/posts/#{post.id}/bookmark.json"
+      expect(Bookmark.find_by(id: bookmark_id)).to eq(nil)
+    end
+
+    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)
+      end
+      it "marks topic_bookmaked as true" do
+        delete "/posts/#{post.id}/bookmark.json"
+        expect(JSON.parse(response.body)['topic_bookmarked']).to eq(true)
+      end
+    end
+  end
+
   describe '#bookmark' do
     include_examples 'action requires login', :put, "/posts/2/bookmark.json"
     let!(:post) { post_by_user }