From 16b9165630565eb02fb477ce5cc22d3af8dc7c08 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Thu, 5 Jan 2023 08:43:58 +1000 Subject: [PATCH] FIX: Bookmark auto delete preference usage and default value (#19707) This commit fixes an issue where the chat message bookmarks did not respect the user's `bookmark_auto_delete_preference` which they select in their user preference page. Also, it changes the default for that value to "keep bookmark and clear reminder" rather than "never", which ends up leaving a lot of expired bookmark reminders around which are a pain to clean up. --- .../discourse/app/components/bookmark.js | 4 +- .../discourse/app/controllers/topic.js | 14 +--- .../discourse/app/models/bookmark.js | 9 ++ lib/bookmark_manager.rb | 64 +++++++++------ .../discourse/components/chat-message.js | 6 +- .../chat/spec/system/bookmark_message_spec.rb | 34 ++++++-- spec/lib/bookmark_manager_spec.rb | 35 +++++++- spec/system/bookmarks_spec.rb | 82 +++++++++++++------ spec/system/page_objects/modals/base.rb | 4 + 9 files changed, 175 insertions(+), 77 deletions(-) diff --git a/app/assets/javascripts/discourse/app/components/bookmark.js b/app/assets/javascripts/discourse/app/components/bookmark.js index 54c8b865937..51cce43e7ce 100644 --- a/app/assets/javascripts/discourse/app/components/bookmark.js +++ b/app/assets/javascripts/discourse/app/components/bookmark.js @@ -60,7 +60,9 @@ export default Component.extend({ userTimezone: this.currentUser.user_option.timezone, showOptions: false, _itsatrap: new ItsATrap(), - autoDeletePreference: this.model.autoDeletePreference || 0, + autoDeletePreference: + this.model.autoDeletePreference || + AUTO_DELETE_PREFERENCES.CLEAR_REMINDER, }); this.registerOnCloseHandler(this._onModalClose); diff --git a/app/assets/javascripts/discourse/app/controllers/topic.js b/app/assets/javascripts/discourse/app/controllers/topic.js index dc6d24f9586..2f1f976f5a5 100644 --- a/app/assets/javascripts/discourse/app/controllers/topic.js +++ b/app/assets/javascripts/discourse/app/controllers/topic.js @@ -836,12 +836,7 @@ export default Controller.extend(bufferedProperty("model"), { ); return this._modifyPostBookmark( bookmarkForPost || - Bookmark.create({ - bookmarkable_id: post.id, - bookmarkable_type: "Post", - auto_delete_preference: - this.currentUser.user_option.bookmark_auto_delete_preference, - }), + Bookmark.createFor(this.currentUser, "Post", post.id), post ); } else { @@ -1349,12 +1344,7 @@ export default Controller.extend(bufferedProperty("model"), { if (this.model.bookmarkCount === 0) { return this._modifyTopicBookmark( - Bookmark.create({ - bookmarkable_id: this.model.id, - bookmarkable_type: "Topic", - auto_delete_preference: - this.currentUser.user_option.bookmark_auto_delete_preference, - }) + Bookmark.createFor(this.currentUser, "Topic", this.model.id) ); } }, diff --git a/app/assets/javascripts/discourse/app/models/bookmark.js b/app/assets/javascripts/discourse/app/models/bookmark.js index 8086c81d2f1..8926d121c0b 100644 --- a/app/assets/javascripts/discourse/app/models/bookmark.js +++ b/app/assets/javascripts/discourse/app/models/bookmark.js @@ -166,6 +166,15 @@ Bookmark.reopenClass({ return this._super(args); }, + createFor(user, bookmarkableType, bookmarkableId) { + return Bookmark.create({ + bookmarkable_type: bookmarkableType, + bookmarkable_id: bookmarkableId, + user_id: user.id, + auto_delete_preference: user.user_option.bookmark_auto_delete_preference, + }); + }, + async applyTransformations(bookmarks) { await applyModelTransformations("bookmark", bookmarks); }, diff --git a/lib/bookmark_manager.rb b/lib/bookmark_manager.rb index a0ecb22d0eb..ce3866f374e 100644 --- a/lib/bookmark_manager.rb +++ b/lib/bookmark_manager.rb @@ -41,18 +41,24 @@ class BookmarkManager # automatically. def create_for(bookmarkable_id:, bookmarkable_type:, name: nil, reminder_at: nil, options: {}) registered_bookmarkable = Bookmark.registered_bookmarkable_from_type(bookmarkable_type) + + if registered_bookmarkable.blank? + return add_error(I18n.t("bookmarks.errors.invalid_bookmarkable", type: bookmarkable_type)) + end + bookmarkable = registered_bookmarkable.model.find_by(id: bookmarkable_id) registered_bookmarkable.validate_before_create(@guardian, bookmarkable) - bookmark = Bookmark.create( - { - user_id: @user.id, - bookmarkable: bookmarkable, - name: name, - reminder_at: reminder_at, - reminder_set_at: Time.zone.now - }.merge(bookmark_model_options_with_defaults(options)) - ) + bookmark = + Bookmark.create( + { + user_id: @user.id, + bookmarkable: bookmarkable, + name: name, + reminder_at: reminder_at, + reminder_set_at: Time.zone.now, + }.merge(bookmark_model_options_with_defaults(options)), + ) return add_errors_from(bookmark) if bookmark.errors.any? @@ -97,16 +103,14 @@ class BookmarkManager bookmark.reminder_last_sent_at = nil end - success = bookmark.update( - { - name: name, - reminder_set_at: Time.zone.now, - }.merge(bookmark_model_options_with_defaults(options)) - ) + success = + bookmark.update( + { name: name, reminder_set_at: Time.zone.now }.merge( + bookmark_model_options_with_defaults(options), + ), + ) - if bookmark.errors.any? - return add_errors_from(bookmark) - end + return add_errors_from(bookmark) if bookmark.errors.any? success end @@ -116,9 +120,7 @@ class BookmarkManager bookmark.pinned = !bookmark.pinned success = bookmark.save - if bookmark.errors.any? - return add_errors_from(bookmark) - end + return add_errors_from(bookmark) if bookmark.errors.any? success end @@ -136,20 +138,30 @@ class BookmarkManager # PostCreator can specify whether auto_track is enabled or not, don't want to # create a TopicUser in that case return if opts.key?(:auto_track) && !opts[:auto_track] - TopicUser.change(@user.id, topic, bookmarked: Bookmark.for_user_in_topic(@user.id, topic.id).exists?) + TopicUser.change( + @user.id, + topic, + bookmarked: Bookmark.for_user_in_topic(@user.id, topic.id).exists?, + ) end def bookmark_model_options_with_defaults(options) - model_options = { - pinned: options[:pinned] - } + model_options = { pinned: options[:pinned] } if options[:auto_delete_preference].blank? - model_options[:auto_delete_preference] = Bookmark.auto_delete_preferences[:never] + model_options[:auto_delete_preference] = if user_auto_delete_preference.present? + user_auto_delete_preference + else + Bookmark.auto_delete_preferences[:clear_reminder] + end else model_options[:auto_delete_preference] = options[:auto_delete_preference] end model_options end + + def user_auto_delete_preference + @guardian.user.user_option&.bookmark_auto_delete_preference + end end diff --git a/plugins/chat/assets/javascripts/discourse/components/chat-message.js b/plugins/chat/assets/javascripts/discourse/components/chat-message.js index 8f82d105015..13e46c27f82 100644 --- a/plugins/chat/assets/javascripts/discourse/components/chat-message.js +++ b/plugins/chat/assets/javascripts/discourse/components/chat-message.js @@ -723,11 +723,7 @@ export default Component.extend({ toggleBookmark() { return openBookmarkModal( this.message.bookmark || - Bookmark.create({ - bookmarkable_type: "ChatMessage", - bookmarkable_id: this.message.id, - user_id: this.currentUser.id, - }), + Bookmark.createFor(this.currentUser, "ChatMessage", this.message.id), { onAfterSave: (savedData) => { const bookmark = Bookmark.create(savedData); diff --git a/plugins/chat/spec/system/bookmark_message_spec.rb b/plugins/chat/spec/system/bookmark_message_spec.rb index e2ebe82eb24..2b843360958 100644 --- a/plugins/chat/spec/system/bookmark_message_spec.rb +++ b/plugins/chat/spec/system/bookmark_message_spec.rb @@ -5,6 +5,8 @@ RSpec.describe "Bookmark message", type: :system, js: true do let(:chat) { PageObjects::Pages::Chat.new } let(:channel) { PageObjects::Pages::ChatChannel.new } + let(:bookmark_modal) { PageObjects::Modals::Bookmark.new } + fab!(:category_channel_1) { Fabricate(:category_channel) } fab!(:message_1) { Fabricate(:chat_message, chat_channel: category_channel_1) } @@ -19,13 +21,32 @@ RSpec.describe "Bookmark message", type: :system, js: true do chat.visit_channel(category_channel_1) channel.bookmark_message(message_1) - expect(page).to have_css("#bookmark-reminder-modal") - - find("#bookmark-name").fill_in(with: "Check this out later") - find("#tap_tile_next_month").click + bookmark_modal.fill_name("Check this out later") + bookmark_modal.select_preset_reminder(:next_month) expect(channel).to have_bookmarked_message(message_1) end + + context "when the user has a bookmark auto_delete_preference" do + before do + current_user.user_option.update!( + bookmark_auto_delete_preference: Bookmark.auto_delete_preferences[:on_owner_reply], + ) + end + + it "is respected when the user creates a new bookmark" do + chat.visit_channel(category_channel_1) + channel.bookmark_message(message_1) + + bookmark_modal.save + expect(channel).to have_bookmarked_message(message_1) + + bookmark = Bookmark.find_by(bookmarkable: message_1, user: current_user) + expect(bookmark.auto_delete_preference).to eq( + Bookmark.auto_delete_preferences[:on_owner_reply], + ) + end + end end context "when mobile", mobile: true do @@ -34,10 +55,9 @@ RSpec.describe "Bookmark message", type: :system, js: true do channel.message_by_id(message_1.id).click(delay: 0.5) find(".bookmark-btn").click - expect(page).to have_css("#bookmark-reminder-modal") - find("#bookmark-name").fill_in(with: "Check this out later") - find("#tap_tile_next_month").click + bookmark_modal.fill_name("Check this out later") + bookmark_modal.select_preset_reminder(:next_month) expect(channel).to have_bookmarked_message(message_1) end diff --git a/spec/lib/bookmark_manager_spec.rb b/spec/lib/bookmark_manager_spec.rb index 7b830952b64..e362a061997 100644 --- a/spec/lib/bookmark_manager_spec.rb +++ b/spec/lib/bookmark_manager_spec.rb @@ -213,7 +213,14 @@ RSpec.describe BookmarkManager do it "when post is deleted it raises invalid access from guardian check" do post.trash! - expect { subject.create_for(bookmarkable_id: post.id, bookmarkable_type: "Post", name: name) }.to raise_error(Discourse::InvalidAccess) + expect do + subject.create_for(bookmarkable_id: post.id, bookmarkable_type: "Post", name: name) + end.to raise_error(Discourse::InvalidAccess) + end + + it "adds a validation error when the bookmarkable_type is not registered" do + subject.create_for(bookmarkable_id: post.id, bookmarkable_type: "BlahFactory", name: name) + expect(subject.errors.full_messages).to include(I18n.t("bookmarks.errors.invalid_bookmarkable", type: "BlahFactory")) end it "updates the topic user bookmarked column to true if any post is bookmarked" do @@ -227,9 +234,31 @@ RSpec.describe BookmarkManager do expect(tu.bookmarked).to eq(true) end - it "sets auto_delete_preference to never by default" do + it "sets auto_delete_preference to clear_reminder by default" do bookmark = subject.create_for(bookmarkable_id: post.id, bookmarkable_type: "Post", name: name, reminder_at: reminder_at) - expect(bookmark.auto_delete_preference).to eq(Bookmark.auto_delete_preferences[:never]) + expect(bookmark.auto_delete_preference).to eq(Bookmark.auto_delete_preferences[:clear_reminder]) + end + + context "when the user has set their bookmark_auto_delete_preference" do + before do + user.user_option.update!(bookmark_auto_delete_preference: Bookmark.auto_delete_preferences[:on_owner_reply]) + end + + it "sets auto_delete_preferences to the user's user_option.bookmark_auto_delete_preference" do + bookmark = subject.create_for(bookmarkable_id: post.id, bookmarkable_type: "Post", name: name, reminder_at: reminder_at) + expect(bookmark.auto_delete_preference).to eq(Bookmark.auto_delete_preferences[:on_owner_reply]) + end + + it "uses the passed in auto_delete_preference option instead of the user's one" do + bookmark = subject.create_for( + bookmarkable_id: post.id, + bookmarkable_type: "Post", + name: name, + reminder_at: reminder_at, + options: { auto_delete_preference: Bookmark.auto_delete_preferences[:when_reminder_sent] } + ) + expect(bookmark.auto_delete_preference).to eq(Bookmark.auto_delete_preferences[:when_reminder_sent]) + end end context "when a reminder time is provided" do diff --git a/spec/system/bookmarks_spec.rb b/spec/system/bookmarks_spec.rb index 82802505a92..bf349ec4419 100644 --- a/spec/system/bookmarks_spec.rb +++ b/spec/system/bookmarks_spec.rb @@ -2,30 +2,34 @@ describe "Bookmarking posts and topics", type: :system, js: true do fab!(:topic) { Fabricate(:topic) } - fab!(:user) { Fabricate(:user, username: "bookmarkguy") } + fab!(:user) { Fabricate(:user) } fab!(:post) { Fabricate(:post, topic: topic, raw: "This is some post to bookmark") } fab!(:post2) { Fabricate(:post, topic: topic, raw: "Some interesting post content") } - it "allows logged in user to create bookmarks with and without reminders" do - sign_in user - visit "/t/#{topic.id}" - topic_page = PageObjects::Pages::Topic.new - expect(topic_page).to have_post_content(post) + let(:topic_page) { PageObjects::Pages::Topic.new } + let(:bookmark_modal) { PageObjects::Modals::Bookmark.new } + + before { sign_in user } + + def visit_topic_and_open_bookmark_modal(post) + topic_page.visit_topic(topic) topic_page.expand_post_actions(post) topic_page.click_post_action_button(post, :bookmark) + end + + it "allows the user to create bookmarks with and without reminders" do + visit_topic_and_open_bookmark_modal(post) - bookmark_modal = PageObjects::Modals::Bookmark.new bookmark_modal.fill_name("something important") bookmark_modal.save expect(topic_page).to have_post_bookmarked(post) bookmark = Bookmark.find_by(bookmarkable: post, user: user) expect(bookmark.name).to eq("something important") + expect(bookmark.reminder_at).to eq(nil) - topic_page.expand_post_actions(post2) - topic_page.click_post_action_button(post2, :bookmark) + visit_topic_and_open_bookmark_modal(post2) - bookmark_modal = PageObjects::Modals::Bookmark.new bookmark_modal.select_preset_reminder(:tomorrow) expect(topic_page).to have_post_bookmarked(post2) bookmark = Bookmark.find_by(bookmarkable: post2, user: user) @@ -34,13 +38,8 @@ describe "Bookmarking posts and topics", type: :system, js: true do end it "does not create a bookmark if the modal is closed with the cancel button" do - sign_in user - visit "/t/#{topic.id}" - topic_page = PageObjects::Pages::Topic.new - topic_page.expand_post_actions(post) - topic_page.click_post_action_button(post, :bookmark) + visit_topic_and_open_bookmark_modal(post) - bookmark_modal = PageObjects::Modals::Bookmark.new bookmark_modal.fill_name("something important") bookmark_modal.cancel @@ -48,20 +47,57 @@ describe "Bookmarking posts and topics", type: :system, js: true do expect(Bookmark.exists?(bookmarkable: post, user: user)).to eq(false) end + it "creates a bookmark if the modal is closed by clicking outside the modal window" do + visit_topic_and_open_bookmark_modal(post) + + bookmark_modal.fill_name("something important") + bookmark_modal.click_outside + + expect(topic_page).to have_post_bookmarked(post) + end + it "allows the topic to be bookmarked" do - sign_in user - visit "/t/#{topic.id}" - topic_page = PageObjects::Pages::Topic.new + topic_page.visit_topic(topic) topic_page.click_topic_footer_button(:bookmark) - bookmark_modal = PageObjects::Modals::Bookmark.new bookmark_modal.fill_name("something important") bookmark_modal.save expect(topic_page).to have_topic_bookmarked - bookmark = try_until_success do - expect(Bookmark.exists?(bookmarkable: topic, user: user)).to eq(true) - end + bookmark = + try_until_success { expect(Bookmark.exists?(bookmarkable: topic, user: user)).to eq(true) } expect(bookmark).not_to eq(nil) end + + context "when the user has a bookmark auto_delete_preference" do + before do + user.user_option.update!( + bookmark_auto_delete_preference: Bookmark.auto_delete_preferences[:on_owner_reply], + ) + end + + it "is respected when the user creates a new bookmark" do + visit_topic_and_open_bookmark_modal(post) + + bookmark_modal.save + expect(topic_page).to have_post_bookmarked(post) + + bookmark = Bookmark.find_by(bookmarkable: post, user: user) + expect(bookmark.auto_delete_preference).to eq( + Bookmark.auto_delete_preferences[:on_owner_reply], + ) + end + + it "allows the user to choose a different auto delete preference for a bookmark" do + visit_topic_and_open_bookmark_modal(post) + + bookmark_modal.save + expect(topic_page).to have_post_bookmarked(post) + + bookmark = Bookmark.find_by(bookmarkable: post, user: user) + expect(bookmark.auto_delete_preference).to eq( + Bookmark.auto_delete_preferences[:on_owner_reply], + ) + end + end end diff --git a/spec/system/page_objects/modals/base.rb b/spec/system/page_objects/modals/base.rb index 1e7f477a859..7bad184882c 100644 --- a/spec/system/page_objects/modals/base.rb +++ b/spec/system/page_objects/modals/base.rb @@ -13,6 +13,10 @@ module PageObjects def cancel find(".d-modal-cancel").click end + + def click_outside + find(".modal-outer-container").click(x: 0, y: 0) + end end end end