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.

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);

app/assets/javascripts/discourse/app/controllers/topic.js

@@ -836,12 +836,7 @@ export default Controller.extend(bufferedProperty("model"), {
         );
         return this._modifyPostBookmark(
           bookmarkForPost ||
-            Bookmark.create({
+            Bookmark.createFor(this.currentUser, "Post", post.id),
-              bookmarkable_id: post.id,
-              bookmarkable_type: "Post",
-              auto_delete_preference:
-                this.currentUser.user_option.bookmark_auto_delete_preference,
-            }),
           post
         );
       } else {
@@ -1349,12 +1344,7 @@ export default Controller.extend(bufferedProperty("model"), {
 
     if (this.model.bookmarkCount === 0) {
       return this._modifyTopicBookmark(
-        Bookmark.create({
+        Bookmark.createFor(this.currentUser, "Topic", this.model.id)
-          bookmarkable_id: this.model.id,
-          bookmarkable_type: "Topic",
-          auto_delete_preference:
-            this.currentUser.user_option.bookmark_auto_delete_preference,
-        })
       );
     }
   },

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);
   },

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(
+    bookmark =
-      {
+      Bookmark.create(
-        user_id: @user.id,
+        {
-        bookmarkable: bookmarkable,
+          user_id: @user.id,
-        name: name,
+          bookmarkable: bookmarkable,
-        reminder_at: reminder_at,
+          name: name,
-        reminder_set_at: Time.zone.now
+          reminder_at: reminder_at,
-      }.merge(bookmark_model_options_with_defaults(options))
+          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(
+    success =
-      {
+      bookmark.update(
-        name: name,
+        { name: name, reminder_set_at: Time.zone.now }.merge(
-        reminder_set_at: Time.zone.now,
+          bookmark_model_options_with_defaults(options),
-      }.merge(bookmark_model_options_with_defaults(options))
+        ),
-    )
+      )
 
-    if bookmark.errors.any?
+    return add_errors_from(bookmark) if bookmark.errors.any?
-      return add_errors_from(bookmark)
-    end
 
     success
   end
@@ -116,9 +120,7 @@ class BookmarkManager
     bookmark.pinned = !bookmark.pinned
     success = bookmark.save
 
-    if bookmark.errors.any?
+    return add_errors_from(bookmark) if bookmark.errors.any?
-      return add_errors_from(bookmark)
-    end
 
     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 = {
+    model_options = { pinned: options[:pinned] }
-      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

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({
+        Bookmark.createFor(this.currentUser, "ChatMessage", this.message.id),
-          bookmarkable_type: "ChatMessage",
-          bookmarkable_id: this.message.id,
-          user_id: this.currentUser.id,
-        }),
       {
         onAfterSave: (savedData) => {
           const bookmark = Bookmark.create(savedData);

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")
+      bookmark_modal.fill_name("Check this out later")
-
+      bookmark_modal.select_preset_reminder(:next_month)
-      find("#bookmark-name").fill_in(with: "Check this out later")
-      find("#tap_tile_next_month").click
 
       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")
+      bookmark_modal.fill_name("Check this out later")
-      find("#tap_tile_next_month").click
+      bookmark_modal.select_preset_reminder(:next_month)
 
       expect(channel).to have_bookmarked_message(message_1)
     end

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

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
+  let(:topic_page) { PageObjects::Pages::Topic.new }
-    sign_in user
+  let(:bookmark_modal) { PageObjects::Modals::Bookmark.new }
-    visit "/t/#{topic.id}"
+
-    topic_page = PageObjects::Pages::Topic.new
+  before { sign_in user }
-    expect(topic_page).to have_post_content(post)
+
+  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)
+    visit_topic_and_open_bookmark_modal(post2)
-    topic_page.click_post_action_button(post2, :bookmark)
 
-    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_topic_and_open_bookmark_modal(post)
-    visit "/t/#{topic.id}"
-    topic_page = PageObjects::Pages::Topic.new
-    topic_page.expand_post_actions(post)
-    topic_page.click_post_action_button(post, :bookmark)
 
-    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
+    topic_page.visit_topic(topic)
-    visit "/t/#{topic.id}"
-    topic_page = PageObjects::Pages::Topic.new
     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
+    bookmark =
-      expect(Bookmark.exists?(bookmarkable: topic, user: user)).to eq(true)
+      try_until_success { expect(Bookmark.exists?(bookmarkable: topic, user: user)).to eq(true) }
-    end
     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

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