From 849631188fe343b77868fa55152968c8de937b1b Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Thu, 12 Mar 2020 10:52:15 +1000 Subject: [PATCH] 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. --- .../discourse/controllers/bookmark.js.es6 | 50 +++++++++++++++---- .../discourse/templates/modal/bookmark.hbs | 14 +++++- app/controllers/posts_controller.rb | 8 ++- config/locales/client.en.yml | 1 + lib/bookmark_manager.rb | 2 +- spec/lib/bookmark_manager_spec.rb | 4 +- spec/requests/posts_controller_spec.rb | 25 ++++++++++ 7 files changed, 88 insertions(+), 16 deletions(-) diff --git a/app/assets/javascripts/discourse/controllers/bookmark.js.es6 b/app/assets/javascripts/discourse/controllers/bookmark.js.es6 index 27e3e8418bd..109678a7b2c 100644 --- a/app/assets/javascripts/discourse/controllers/bookmark.js.es6 +++ b/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() { diff --git a/app/assets/javascripts/discourse/templates/modal/bookmark.hbs b/app/assets/javascripts/discourse/templates/modal/bookmark.hbs index e32f2467ebf..4fb9e462f9f 100644 --- a/app/assets/javascripts/discourse/templates/modal/bookmark.hbs +++ b/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-grid}} + {{#if customDateTimeSelected}} +
+ {{d-icon "calendar-alt"}} + {{date-picker-future + value=customReminderDate + onSelect=(action (mut customReminderDate)) + }} + {{d-icon "far-clock"}} + {{input placeholder="--:--" type="time" value=customReminderTime}} +
+ {{/if}} + {{else}}
{{i18n "bookmarks.no_timezone" basePath=basePath }}
{{/if}} diff --git a/app/controllers/posts_controller.rb b/app/controllers/posts_controller.rb index 5da0ded4228..5d62c6510f1 100644 --- a/app/controllers/posts_controller.rb +++ b/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 diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index c699eef32f5..d8ff29639d7 100644 --- a/config/locales/client.en.yml +++ b/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 in your profile.' + 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
{{date}}" diff --git a/lib/bookmark_manager.rb b/lib/bookmark_manager.rb index 0dcfa2b79f0..f4831e13503 100644 --- a/lib/bookmark_manager.rb +++ b/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 diff --git a/spec/lib/bookmark_manager_spec.rb b/spec/lib/bookmark_manager_spec.rb index 92b7c2df213..dbfecd565dd 100644 --- a/spec/lib/bookmark_manager_spec.rb +++ b/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) diff --git a/spec/requests/posts_controller_spec.rb b/spec/requests/posts_controller_spec.rb index dd3afa7cf0b..b90b7cd3e3b 100644 --- a/spec/requests/posts_controller_spec.rb +++ b/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 }