diff --git a/app/assets/javascripts/discourse/controllers/bookmark.js b/app/assets/javascripts/discourse/controllers/bookmark.js index ec384d54ae..9f630ee913 100644 --- a/app/assets/javascripts/discourse/controllers/bookmark.js +++ b/app/assets/javascripts/discourse/controllers/bookmark.js @@ -41,15 +41,16 @@ const BOOKMARK_BINDINGS = { }, "n m": { handler: "selectReminderType", args: [REMINDER_TYPES.NEXT_MONTH] }, "c r": { handler: "selectReminderType", args: [REMINDER_TYPES.CUSTOM] }, - "n r": { handler: "selectReminderType", args: [REMINDER_TYPES.NONE] } + "n r": { handler: "selectReminderType", args: [REMINDER_TYPES.NONE] }, + "d d": { handler: "delete" } }; export default Controller.extend(ModalFunctionality, { loading: false, errorMessage: null, selectedReminderType: null, - closeWithoutSaving: false, - isSavingBookmarkManually: false, + _closeWithoutSaving: false, + _savingBookmarkManually: false, onCloseWithoutSaving: null, customReminderDate: null, customReminderTime: null, @@ -62,20 +63,20 @@ export default Controller.extend(ModalFunctionality, { this.setProperties({ errorMessage: null, selectedReminderType: REMINDER_TYPES.NONE, - closeWithoutSaving: false, - isSavingBookmarkManually: false, + _closeWithoutSaving: false, + _savingBookmarkManually: false, customReminderDate: null, - customReminderTime: this.defaultCustomReminderTime(), + customReminderTime: this._defaultCustomReminderTime(), lastCustomReminderDate: null, lastCustomReminderTime: null, userTimezone: this.currentUser.resolvedTimezone() }); - this.bindKeyboardShortcuts(); - this.loadLastUsedCustomReminderDatetime(); + this._bindKeyboardShortcuts(); + this._loadLastUsedCustomReminderDatetime(); - if (this.editingExistingBookmark()) { - this.initializeExistingBookmarkData(); + if (this._editingExistingBookmark()) { + this._initializeExistingBookmarkData(); } }, @@ -84,19 +85,19 @@ export default Controller.extend(ModalFunctionality, { * 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)); + this._unbindKeyboardShortcuts(); + this._restoreGlobalShortcuts(); + if (!this._closeWithoutSaving && !this._savingBookmarkManually) { + this._saveBookmark().catch(e => this._handleSaveError(e)); } - if (this.onCloseWithoutSaving && this.closeWithoutSaving) { + if (this.onCloseWithoutSaving && this._closeWithoutSaving) { this.onCloseWithoutSaving(); } }, - initializeExistingBookmarkData() { - if (this.existingBookmarkHasReminder()) { - let parsedReminderAt = this.parseCustomDateTime(this.model.reminderAt); + _initializeExistingBookmarkData() { + if (this._existingBookmarkHasReminder()) { + let parsedReminderAt = this._parseCustomDateTime(this.model.reminderAt); this.setProperties({ customReminderDate: parsedReminderAt.format("YYYY-MM-DD"), customReminderTime: parsedReminderAt.format("HH:mm"), @@ -105,20 +106,20 @@ export default Controller.extend(ModalFunctionality, { } }, - editingExistingBookmark() { + _editingExistingBookmark() { return isPresent(this.model) && isPresent(this.model.id); }, - existingBookmarkHasReminder() { + _existingBookmarkHasReminder() { return isPresent(this.model) && isPresent(this.model.reminderAt); }, - loadLastUsedCustomReminderDatetime() { + _loadLastUsedCustomReminderDatetime() { let lastTime = localStorage.lastCustomBookmarkReminderTime; let lastDate = localStorage.lastCustomBookmarkReminderDate; if (lastTime && lastDate) { - let parsed = this.parseCustomDateTime(lastDate, lastTime); + let parsed = this._parseCustomDateTime(lastDate, lastTime); // can't set reminders in the past if (parsed < this.now()) { @@ -133,7 +134,7 @@ export default Controller.extend(ModalFunctionality, { } }, - bindKeyboardShortcuts() { + _bindKeyboardShortcuts() { KeyboardShortcuts.pause(GLOBAL_SHORTCUTS_TO_PAUSE); Object.keys(BOOKMARK_BINDINGS).forEach(shortcut => { KeyboardShortcuts.addShortcut(shortcut, () => { @@ -146,11 +147,11 @@ export default Controller.extend(ModalFunctionality, { }); }, - unbindKeyboardShortcuts() { + _unbindKeyboardShortcuts() { KeyboardShortcuts.unbind(BOOKMARK_BINDINGS); }, - restoreGlobalShortcuts() { + _restoreGlobalShortcuts() { KeyboardShortcuts.unpause(GLOBAL_SHORTCUTS_TO_PAUSE); }, @@ -159,6 +160,11 @@ export default Controller.extend(ModalFunctionality, { return isPresent(existingReminderAt); }, + @discourseComputed("model.id") + showDelete(id) { + return isPresent(id); + }, + @discourseComputed() showAtDesktop() { return ( @@ -247,8 +253,8 @@ export default Controller.extend(ModalFunctionality, { return !_.isEmpty(userTimezone); }, - saveBookmark() { - const reminderAt = this.reminderAt(); + _saveBookmark() { + const reminderAt = this._reminderAt(); const reminderAtISO = reminderAt ? reminderAt.toISOString() : null; if (this.selectedReminderType === REMINDER_TYPES.CUSTOM) { @@ -277,34 +283,54 @@ export default Controller.extend(ModalFunctionality, { id: this.model.id }; - if (this.editingExistingBookmark()) { + if (this._editingExistingBookmark()) { return ajax("/bookmarks/" + this.model.id, { type: "PUT", data }).then(() => { if (this.afterSave) { - this.afterSave(reminderAtISO, this.selectedReminderType); + this.afterSave({ + reminderAt: reminderAtISO, + reminderType: this.selectedReminderType, + id: this.model.id, + name: this.model.name + }); } }); } else { - return ajax("/bookmarks", { type: "POST", data }).then(() => { + return ajax("/bookmarks", { type: "POST", data }).then(response => { if (this.afterSave) { - this.afterSave(reminderAtISO, this.selectedReminderType); + this.afterSave({ + reminderAt: reminderAtISO, + reminderType: this.selectedReminderType, + id: response.id, + name: this.model.name + }); } }); } }, - parseCustomDateTime(date, time) { + _deleteBookmark() { + return ajax("/bookmarks/" + this.model.id, { + type: "DELETE" + }).then(response => { + if (this.afterDelete) { + this.afterDelete(response.topic_bookmarked); + } + }); + }, + + _parseCustomDateTime(date, time) { let dateTime = isPresent(time) ? date + " " + time : date; return moment.tz(dateTime, this.userTimezone); }, - defaultCustomReminderTime() { + _defaultCustomReminderTime() { return `0${START_OF_DAY_HOUR}:00`; }, - reminderAt() { + _reminderAt() { if (!this.selectedReminderType) { return; } @@ -329,9 +355,9 @@ export default Controller.extend(ModalFunctionality, { case REMINDER_TYPES.CUSTOM: this.set( "customReminderTime", - this.customReminderTime || this.defaultCustomReminderTime() + this.customReminderTime || this._defaultCustomReminderTime() ); - const customDateTime = this.parseCustomDateTime( + const customDateTime = this._parseCustomDateTime( this.customReminderDate, this.customReminderTime ); @@ -385,8 +411,8 @@ export default Controller.extend(ModalFunctionality, { return this.startOfDay(this.now().add(2, "days")); }, - handleSaveError(e) { - this.isSavingBookmarkManually = false; + _handleSaveError(e) { + this._savingBookmarkManually = false; if (typeof e === "string") { bootbox.alert(e); } else { @@ -396,20 +422,35 @@ export default Controller.extend(ModalFunctionality, { actions: { saveAndClose() { - if (this.saving) { + if (this._saving || this._deleting) { return; } - this.saving = true; - this.isSavingBookmarkManually = true; - this.saveBookmark() + this._saving = true; + this._savingBookmarkManually = true; + this._saveBookmark() .then(() => this.send("closeModal")) - .catch(e => this.handleSaveError(e)) - .finally(() => (this.saving = false)); + .catch(e => this._handleSaveError(e)) + .finally(() => (this._saving = false)); + }, + + delete() { + this._deleting = true; + bootbox.confirm(I18n.t("bookmarks.confirm_delete"), result => { + if (result) { + this._closeWithoutSaving = true; + this._deleteBookmark() + .then(() => { + this._deleting = false; + this.send("closeModal"); + }) + .catch(e => this._handleSaveError(e)); + } + }); }, closeWithoutSavingBookmark() { - this.closeWithoutSaving = true; + this._closeWithoutSaving = true; this.send("closeModal"); }, diff --git a/app/assets/javascripts/discourse/controllers/keyboard-shortcuts-help.js b/app/assets/javascripts/discourse/controllers/keyboard-shortcuts-help.js index 85ee690151..a0d42ac061 100644 --- a/app/assets/javascripts/discourse/controllers/keyboard-shortcuts-help.js +++ b/app/assets/javascripts/discourse/controllers/keyboard-shortcuts-help.js @@ -170,6 +170,10 @@ export default Controller.extend(ModalFunctionality, { none: buildShortcut("bookmarks.none", { keys1: ["n", "r"], shortcutsDelimiter: "space" + }), + delete: buildShortcut("bookmarks.delete", { + keys1: ["d", "d"], + shortcutsDelimiter: "space" }) }, actions: { diff --git a/app/assets/javascripts/discourse/controllers/user-activity-bookmarks-with-reminders.js b/app/assets/javascripts/discourse/controllers/user-activity-bookmarks-with-reminders.js index a2176a7a2b..9611aceced 100644 --- a/app/assets/javascripts/discourse/controllers/user-activity-bookmarks-with-reminders.js +++ b/app/assets/javascripts/discourse/controllers/user-activity-bookmarks-with-reminders.js @@ -61,7 +61,11 @@ export default Controller.extend({ actions: { removeBookmark(bookmark) { - return bookmark.destroy().then(() => this.loadItems()); + bootbox.confirm(I18n.t("bookmarks.confirm_delete"), result => { + if (result) { + return bookmark.destroy().then(() => this.loadItems()); + } + }); }, editBookmark(bookmark) { diff --git a/app/assets/javascripts/discourse/models/post.js b/app/assets/javascripts/discourse/models/post.js index f3ae53a606..550a1fbfac 100644 --- a/app/assets/javascripts/discourse/models/post.js +++ b/app/assets/javascripts/discourse/models/post.js @@ -336,48 +336,49 @@ const Post = RestModel.extend({ }, toggleBookmarkWithReminder() { - this.toggleProperty("bookmarked_with_reminder"); - if (this.bookmarked_with_reminder) { - return new Promise(resolve => { - let controller = showModal("bookmark", { - model: { - postId: this.id - }, - title: "post.bookmarks.create", - modalClass: "bookmark-with-reminder" - }); - controller.setProperties({ - onCloseWithoutSaving: () => { - this.toggleProperty("bookmarked_with_reminder"); - resolve({ closedWithoutSaving: true }); - this.appEvents.trigger("post-stream:refresh", { id: this.id }); - }, - afterSave: (reminderAtISO, reminderType) => { - this.setProperties({ - "topic.bookmarked": true, - bookmark_reminder_at: reminderAtISO, - bookmark_reminder_type: reminderType - }); - resolve({ closedWithoutSaving: false }); - this.appEvents.trigger("post-stream:refresh", { id: this.id }); - } - }); + return new Promise(resolve => { + let controller = showModal("bookmark", { + model: { + postId: this.id, + id: this.bookmark_id, + reminderAt: this.bookmark_reminder_at, + name: this.bookmark_name + }, + title: this.bookmark_id + ? "post.bookmarks.edit" + : "post.bookmarks.create", + modalClass: "bookmark-with-reminder" }); - } else { - this.setProperties({ - bookmark_reminder_at: null, - bookmark_reminder_type: null - }); - return Post.destroyBookmark(this.id) - .then(result => { - this.set("topic.bookmarked", result.topic_bookmarked); + controller.setProperties({ + onCloseWithoutSaving: () => { + resolve({ closedWithoutSaving: true }); + this.appEvents.trigger("post-stream:refresh", { id: this.id }); + }, + afterSave: savedData => { + this.setProperties({ + "topic.bookmarked": true, + bookmarked_with_reminder: true, + bookmark_reminder_at: savedData.reminderAt, + bookmark_reminder_type: savedData.reminderType, + bookmark_name: savedData.name, + bookmark_id: savedData.id + }); + resolve({ closedWithoutSaving: false }); + this.appEvents.trigger("post-stream:refresh", { id: this.id }); + }, + afterDelete: topicBookmarked => { + this.set("topic.bookmarked", topicBookmarked); + this.setProperties({ + bookmark_reminder_at: null, + bookmark_reminder_type: null, + bookmark_name: null, + bookmark_id: null, + bookmarked_with_reminder: false + }); this.appEvents.trigger("page:bookmark-post-toggled", this); - }) - .catch(error => { - this.toggleProperty("bookmarked_with_reminder"); - throw new Error(error); - }); - } + } + }); + }); }, updateActionsSummary(json) { diff --git a/app/assets/javascripts/discourse/models/topic.js b/app/assets/javascripts/discourse/models/topic.js index ce951d6edc..6ad8de27ec 100644 --- a/app/assets/javascripts/discourse/models/topic.js +++ b/app/assets/javascripts/discourse/models/topic.js @@ -461,6 +461,12 @@ const Topic = RestModel.extend({ .then(() => { this.toggleProperty("bookmarked"); this.set("bookmark_reminder_at", null); + let clearedBookmarkProps = { + bookmarked_with_reminder: false, + bookmark_id: null, + bookmark_name: null, + bookmark_reminder_at: null + }; if (posts) { const updated = []; posts.forEach(post => { @@ -472,11 +478,11 @@ const Topic = RestModel.extend({ this.siteSettings.enable_bookmarks_with_reminders && post.bookmarked_with_reminder ) { - post.set("bookmarked_with_reminder", false); + post.setProperties(clearedBookmarkProps); updated.push(post.id); } }); - firstPost.set("bookmarked_with_reminder", false); + firstPost.setProperties(clearedBookmarkProps); return updated; } }) diff --git a/app/assets/javascripts/discourse/templates/modal/bookmark.hbs b/app/assets/javascripts/discourse/templates/modal/bookmark.hbs index 8e20711ef3..3a5075ebc9 100644 --- a/app/assets/javascripts/discourse/templates/modal/bookmark.hbs +++ b/app/assets/javascripts/discourse/templates/modal/bookmark.hbs @@ -9,7 +9,7 @@ {{/if}}
- {{input value=model.name name="name" class="bookmark-name" placeholder=(i18n "post.bookmarks.name_placeholder")}} + {{input id="bookmark-name" value=model.name name="name" class="bookmark-name" placeholder=(i18n "post.bookmarks.name_placeholder")}}
{{#if showExistingReminderAt }} @@ -100,6 +100,11 @@
{{d-button label="bookmarks.save" class="btn-primary" action=(action "saveAndClose")}} {{d-modal-cancel close=(action "closeWithoutSavingBookmark")}} + {{#if showDelete}} +
+ {{d-button icon="trash" class="btn-danger" action=(action "delete")}} +
+ {{/if}}
{{/conditional-loading-spinner}} {{/d-modal-body}} diff --git a/app/assets/javascripts/discourse/templates/modal/keyboard-shortcuts-help.hbs b/app/assets/javascripts/discourse/templates/modal/keyboard-shortcuts-help.hbs index f2d6b022e9..5299b52f37 100644 --- a/app/assets/javascripts/discourse/templates/modal/keyboard-shortcuts-help.hbs +++ b/app/assets/javascripts/discourse/templates/modal/keyboard-shortcuts-help.hbs @@ -69,6 +69,7 @@
  • {{html-safe shortcuts.bookmarks.next_business_day}}
  • {{html-safe shortcuts.bookmarks.custom}}
  • {{html-safe shortcuts.bookmarks.none}}
  • +
  • {{html-safe shortcuts.bookmarks.delete}}
  • {{/if}} diff --git a/app/controllers/bookmarks_controller.rb b/app/controllers/bookmarks_controller.rb index 4fd077d45e..f7fed51f59 100644 --- a/app/controllers/bookmarks_controller.rb +++ b/app/controllers/bookmarks_controller.rb @@ -15,7 +15,7 @@ class BookmarksController < ApplicationController ) if bookmark_manager.errors.empty? - return render json: success_json + return render json: success_json.merge(id: bookmark.id) end render json: failed_json.merge(errors: bookmark_manager.errors.full_messages), status: 400 @@ -23,8 +23,8 @@ class BookmarksController < ApplicationController def destroy params.require(:id) - BookmarkManager.new(current_user).destroy(params[:id]) - render json: success_json + result = BookmarkManager.new(current_user).destroy(params[:id]) + render json: success_json.merge(result) end def update diff --git a/app/controllers/posts_controller.rb b/app/controllers/posts_controller.rb index 5d62c6510f..50c4ead17f 100644 --- a/app/controllers/posts_controller.rb +++ b/app/controllers/posts_controller.rb @@ -511,15 +511,10 @@ class PostsController < ApplicationController def destroy_bookmark params.require(:post_id) - existing_bookmark = Bookmark.find_by(post_id: params[:post_id], user_id: current_user.id) - topic_bookmarked = false + bookmark_id = Bookmark.where(post_id: params[:post_id], user_id: current_user.id).pluck_first(:id) + result = BookmarkManager.new(current_user).destroy(bookmark_id) - if existing_bookmark.present? - topic_bookmarked = Bookmark.exists?(topic_id: existing_bookmark.topic_id, user_id: current_user.id) - existing_bookmark.destroy - end - - render json: success_json.merge(topic_bookmarked: topic_bookmarked) + render json: success_json.merge(result) end def wiki diff --git a/app/serializers/post_serializer.rb b/app/serializers/post_serializer.rb index b78c90f4fb..ff880f14c3 100644 --- a/app/serializers/post_serializer.rb +++ b/app/serializers/post_serializer.rb @@ -51,7 +51,9 @@ class PostSerializer < BasicPostSerializer :bookmarked, :bookmarked_with_reminder, :bookmark_reminder_at, + :bookmark_id, :bookmark_reminder_type, + :bookmark_name, :raw, :actions_summary, :moderator?, @@ -334,6 +336,14 @@ class PostSerializer < BasicPostSerializer include_bookmarked_with_reminder? end + def include_bookmark_name? + include_bookmarked_with_reminder? + end + + def include_bookmark_id? + include_bookmarked_with_reminder? + end + def post_bookmark return nil if !SiteSetting.enable_bookmarks_with_reminders? || @topic_view.blank? @post_bookmark ||= @topic_view.user_post_bookmarks.find { |bookmark| bookmark.post_id == object.id } @@ -348,6 +358,14 @@ class PostSerializer < BasicPostSerializer Bookmark.reminder_types[post_bookmark.reminder_type].to_s end + def bookmark_name + post_bookmark&.name + end + + def bookmark_id + post_bookmark&.id + end + def include_display_username? SiteSetting.enable_names? end diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 9bd07e6819..947f19c3ba 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -311,6 +311,8 @@ en: created_with_reminder: "you've bookmarked this post with a reminder %{date}" created_with_at_desktop_reminder: "you've bookmarked this post and will be reminded next time you are at your desktop" remove: "Remove Bookmark" + delete: "Delete Bookmark" + confirm_delete: "Are you sure you want to delete this bookmark? If you set a reminder it will also be deleted." 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.' @@ -3118,6 +3120,7 @@ en: next_business_day: "%{shortcut} Next business day" custom: "%{shortcut} Custom date and time" none: "%{shortcut} No reminder" + delete: "%{shortcut} Delete bookmark" actions: title: "Actions" bookmark_topic: "%{shortcut} Toggle bookmark topic" diff --git a/lib/bookmark_manager.rb b/lib/bookmark_manager.rb index 9da2dc257b..b56a129f10 100644 --- a/lib/bookmark_manager.rb +++ b/lib/bookmark_manager.rb @@ -44,6 +44,8 @@ class BookmarkManager bookmark.destroy clear_at_desktop_cache_if_required + + { topic_bookmarked: Bookmark.exists?(topic_id: bookmark.topic_id, user: @user) } end def destroy_for_topic(topic) diff --git a/spec/lib/bookmark_manager_spec.rb b/spec/lib/bookmark_manager_spec.rb index de25969f2c..979f567433 100644 --- a/spec/lib/bookmark_manager_spec.rb +++ b/spec/lib/bookmark_manager_spec.rb @@ -123,8 +123,15 @@ RSpec.describe BookmarkManager do describe ".destroy" do let!(:bookmark) { Fabricate(:bookmark, user: user, post: post) } it "deletes the existing bookmark" do - subject.destroy(bookmark.id) + result = subject.destroy(bookmark.id) expect(Bookmark.exists?(id: bookmark.id)).to eq(false) + expect(result[:topic_bookmarked]).to eq(false) + end + + it "returns a value indicating whether there are still other bookmarks in the topic for the user" do + Fabricate(:bookmark, user: user, post: Fabricate(:post, topic: post.topic)) + result = subject.destroy(bookmark.id) + expect(result[:topic_bookmarked]).to eq(true) end context "if the bookmark is belonging to some other user" do diff --git a/spec/requests/posts_controller_spec.rb b/spec/requests/posts_controller_spec.rb index 534bcdecd5..09cefae4a0 100644 --- a/spec/requests/posts_controller_spec.rb +++ b/spec/requests/posts_controller_spec.rb @@ -473,7 +473,7 @@ describe PostsController do 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) + Fabricate(:bookmark, user: user, post: Fabricate(:post, topic: post.topic), topic: post.topic) end it "marks topic_bookmaked as true" do delete "/posts/#{post.id}/bookmark.json" diff --git a/test/javascripts/controllers/bookmark-test.js b/test/javascripts/controllers/bookmark-test.js index 7a68739289..67d918703b 100644 --- a/test/javascripts/controllers/bookmark-test.js +++ b/test/javascripts/controllers/bookmark-test.js @@ -201,7 +201,7 @@ QUnit.test( BookmarkController.customReminderDate = "2028-12-12"; BookmarkController.selectedReminderType = BookmarkController.reminderTypes.CUSTOM; - const reminderAt = BookmarkController.reminderAt(); + const reminderAt = BookmarkController._reminderAt(); assert.equal(BookmarkController.customReminderTime, "08:00"); assert.equal( reminderAt.toString(), @@ -223,7 +223,7 @@ QUnit.test( localStorage.lastCustomBookmarkReminderDate = "2019-12-12"; localStorage.lastCustomBookmarkReminderTime = "08:00"; - BookmarkController.loadLastUsedCustomReminderDatetime(); + BookmarkController._loadLastUsedCustomReminderDatetime(); assert.equal(BookmarkController.lastCustomReminderDate, "2019-12-12"); assert.equal(BookmarkController.lastCustomReminderTime, "08:00"); @@ -237,7 +237,7 @@ QUnit.test( localStorage.lastCustomBookmarkReminderDate = "2019-12-11"; localStorage.lastCustomBookmarkReminderTime = "07:00"; - BookmarkController.loadLastUsedCustomReminderDatetime(); + BookmarkController._loadLastUsedCustomReminderDatetime(); assert.equal(BookmarkController.lastCustomReminderDate, null); assert.equal(BookmarkController.lastCustomReminderTime, null);