From 6fb0f36ce141e5638899b19860d6d5c38ceb49dd Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Thu, 7 May 2020 13:37:39 +1000 Subject: [PATCH] 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. --- .../discourse/app/controllers/bookmark.js | 41 ++++++++++++++++++- .../javascripts/discourse/app/models/post.js | 3 ++ .../app/templates/modal/bookmark.hbs | 10 ++++- .../common/components/bookmark-modal.scss | 20 +++++++++ app/controllers/bookmarks_controller.rb | 10 ++++- app/models/bookmark.rb | 23 ++++++----- app/serializers/post_serializer.rb | 9 ++++ config/locales/client.en.yml | 1 + ...when_reminder_sent_boolean_to_bookmarks.rb | 7 ++++ lib/bookmark_manager.rb | 30 ++++++++------ lib/bookmark_reminder_notification_handler.rb | 5 +++ spec/lib/bookmark_manager_spec.rb | 30 +++++++++++++- ...mark_reminder_notification_handler_spec.rb | 14 +++++++ test/javascripts/acceptance/bookmarks-test.js | 24 +++++++++++ 14 files changed, 196 insertions(+), 31 deletions(-) create mode 100644 db/migrate/20200505060712_add_delete_when_reminder_sent_boolean_to_bookmarks.rb diff --git a/app/assets/javascripts/discourse/app/controllers/bookmark.js b/app/assets/javascripts/discourse/app/controllers/bookmark.js index 992c3228a0..7d86ed2b74 100644 --- 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) { diff --git a/app/assets/javascripts/discourse/app/models/post.js b/app/assets/javascripts/discourse/app/models/post.js index e7cf6028e5..e0565b420c 100644 --- 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 }); diff --git a/app/assets/javascripts/discourse/app/templates/modal/bookmark.hbs b/app/assets/javascripts/discourse/app/templates/modal/bookmark.hbs index 2cac81e9b6..b69713e757 100644 --- a/app/assets/javascripts/discourse/app/templates/modal/bookmark.hbs +++ b/app/assets/javascripts/discourse/app/templates/modal/bookmark.hbs @@ -8,8 +8,16 @@ {{/if}} -
+
{{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"}} +
+ +
+
{{#if showExistingReminderAt }} diff --git a/app/assets/stylesheets/common/components/bookmark-modal.scss b/app/assets/stylesheets/common/components/bookmark-modal.scss index 45ad12fc18..b870ec00d1 100644 --- 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; + } + } } diff --git a/app/controllers/bookmarks_controller.rb b/app/controllers/bookmarks_controller.rb index f7fed51f59..001e75c119 100644 --- 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? diff --git a/app/models/bookmark.rb b/app/models/bookmark.rb index 3cfae4fe17..3125f8f9fe 100644 --- 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 # diff --git a/app/serializers/post_serializer.rb b/app/serializers/post_serializer.rb index ef39e37caa..35a50a5f96 100644 --- 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 diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index b4b6ad31f2..e4a65a3b1e 100644 --- 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 in your profile.' 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" diff --git 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 new file mode 100644 index 0000000000..7e4ca807fa --- /dev/null +++ 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 diff --git a/lib/bookmark_manager.rb b/lib/bookmark_manager.rb index 23328021f6..90088181f6 100644 --- 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? diff --git a/lib/bookmark_reminder_notification_handler.rb b/lib/bookmark_reminder_notification_handler.rb index 223c55b2ac..b1f32cee61 100644 --- 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 diff --git a/spec/lib/bookmark_manager_spec.rb b/spec/lib/bookmark_manager_spec.rb index d821e99446..24bdd6bd15 100644 --- 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 diff --git a/spec/lib/bookmark_reminder_notification_handler_spec.rb b/spec/lib/bookmark_reminder_notification_handler_spec.rb index d86ce73a5e..6af860f2f0 100644 --- 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! diff --git a/test/javascripts/acceptance/bookmarks-test.js b/test/javascripts/acceptance/bookmarks-test.js index df8fb56e93..762cd1ee07 100644 --- 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");