DEV: Add save_user_preferences option to BookmarkManager (#16894)

When saving / creating bookmarks, we have code to save
the user's preference of bookmark_auto_delete_preference
to their user_options.

Unfortunately this can cause weirdness when plugins
have code using BookmarkManager to set the auto delete preference for
only a specific bookmark.

This commit introduces a save_user_preferences option (false
by default) so that this user preference is not saved unless
specified by the consumer of BookmarkManager, so plugins will
not have to worry about it.

app/controllers/bookmarks_controller.rb

@@ -6,26 +6,22 @@ class BookmarksController < ApplicationController
   def create
     params.require(:bookmarkable_id)
     params.require(:bookmarkable_type)
+    params.permit(:bookmarkable_id, :bookmarkable_type, :name, :reminder_at, :auto_delete_preference)
 
     RateLimiter.new(
       current_user, "create_bookmark", SiteSetting.max_bookmarks_per_day, 1.day.to_i
     ).performed!
 
     bookmark_manager = BookmarkManager.new(current_user)
-
+    bookmark = bookmark_manager.create_for(
-    create_params = {
+      bookmarkable_id: params[:bookmarkable_id],
+      bookmarkable_type: params[:bookmarkable_type],
       name: params[:name],
       reminder_at: params[:reminder_at],
       options: {
-        auto_delete_preference: params[:auto_delete_preference] || 0
+        auto_delete_preference: params[:auto_delete_preference],
+        save_user_preferences: true
       }
-    }
-
-    bookmark = bookmark_manager.create_for(
-      **create_params.merge(
-        bookmarkable_id: params[:bookmarkable_id],
-        bookmarkable_type: params[:bookmarkable_type]
-      )
     )
 
     if bookmark_manager.errors.empty?
@@ -43,6 +39,7 @@ class BookmarksController < ApplicationController
 
   def update
     params.require(:id)
+    params.permit(:id, :name, :reminder_at, :auto_delete_preference)
 
     bookmark_manager = BookmarkManager.new(current_user)
     bookmark_manager.update(
@@ -50,7 +47,8 @@ class BookmarksController < ApplicationController
       name: params[:name],
       reminder_at: params[:reminder_at],
       options: {
-        auto_delete_preference: params[:auto_delete_preference] || 0
+        auto_delete_preference: params[:auto_delete_preference],
+        save_user_preferences: true
       }
     )
 

lib/bookmark_manager.rb

@@ -40,8 +40,8 @@ class BookmarkManager
   #                                      this is used to determine when to delete a bookmark
   #                                      automatically.
   def create_for(bookmarkable_id:, bookmarkable_type:, name: nil, reminder_at: nil, options: {})
-    bookmarkable = bookmarkable_type.constantize.find_by(id: bookmarkable_id)
     registered_bookmarkable = Bookmark.registered_bookmarkable_from_type(bookmarkable_type)
+    bookmarkable = registered_bookmarkable.model.find_by(id: bookmarkable_id)
     registered_bookmarkable.validate_before_create(@guardian, bookmarkable)
 
     bookmark = Bookmark.create(
@@ -51,13 +51,13 @@ class BookmarkManager
         name: name,
         reminder_at: reminder_at,
         reminder_set_at: Time.zone.now
-      }.merge(options)
+      }.merge(bookmark_model_options_with_defaults(options))
     )
 
     return add_errors_from(bookmark) if bookmark.errors.any?
 
     registered_bookmarkable.after_create(@guardian, bookmark, options)
-    update_user_option(bookmark)
+    update_user_option(bookmark, options)
 
     bookmark
   end
@@ -102,14 +102,14 @@ class BookmarkManager
       {
         name: name,
         reminder_set_at: Time.zone.now,
-      }.merge(options)
+      }.merge(bookmark_model_options_with_defaults(options))
     )
 
     if bookmark.errors.any?
       return add_errors_from(bookmark)
     end
 
-    update_user_option(bookmark)
+    update_user_option(bookmark, options)
 
     success
   end
@@ -142,7 +142,18 @@ class BookmarkManager
     TopicUser.change(@user.id, topic, bookmarked: Bookmark.for_user_in_topic(@user.id, topic.id).exists?)
   end
 
-  def update_user_option(bookmark)
+  def update_user_option(bookmark, options)
-    @user.user_option.update!(bookmark_auto_delete_preference: bookmark.auto_delete_preference)
+    return if !options[:save_user_preferences]
+    @user.user_option.update!(
+      bookmark_auto_delete_preference: bookmark.auto_delete_preference
+    )
+  end
+
+  def bookmark_model_options_with_defaults(options)
+    if options[:auto_delete_preference].blank?
+      options[:auto_delete_preference] = Bookmark.auto_delete_preferences[:never]
+    end
+
+    options.slice(:auto_delete_preference, :pinned)
   end
 end

spec/lib/bookmark_manager_spec.rb

@@ -227,6 +227,11 @@ RSpec.describe BookmarkManager do
       expect(tu.bookmarked).to eq(true)
     end
 
+    it "sets auto_delete_preference to never 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])
+    end
+
     context "when a reminder time is provided" do
       it "saves the values correctly" do
         subject.create_for(bookmarkable_id: post.id, bookmarkable_type: "Post", name: name, reminder_at: reminder_at)
@@ -302,12 +307,41 @@ RSpec.describe BookmarkManager do
       end
     end
 
-    it "saves user's preference" do
+    it "does not save user preference by default" do
-      subject.create_for(bookmarkable_id: post.id, bookmarkable_type: "Post", options: { auto_delete_preference: Bookmark.auto_delete_preferences[:when_reminder_sent] })
+      user.user_option.update(bookmark_auto_delete_preference: Bookmark.auto_delete_preferences[:on_owner_reply])
+      subject.create_for(
+        bookmarkable_id: post.id,
+        bookmarkable_type: "Post",
+        options: { auto_delete_preference: Bookmark.auto_delete_preferences[:when_reminder_sent] }
+      )
+      expect(user.user_option.bookmark_auto_delete_preference).to eq(Bookmark.auto_delete_preferences[:on_owner_reply])
+
+      bookmark = Bookmark.find_by(user: user)
+      subject.update(
+        bookmark_id: bookmark.id,
+        name: "test",
+        reminder_at: 1.day.from_now,
+        options: { auto_delete_preference: Bookmark.auto_delete_preferences[:when_reminder_sent] }
+      )
+      expect(user.user_option.bookmark_auto_delete_preference).to eq(Bookmark.auto_delete_preferences[:on_owner_reply])
+    end
+
+    it "saves user's preference when save_user_preferences option is specified" do
+      user.user_option.update(bookmark_auto_delete_preference: Bookmark.auto_delete_preferences[:on_owner_reply])
+      subject.create_for(
+        bookmarkable_id: post.id,
+        bookmarkable_type: "Post",
+        options: { auto_delete_preference: Bookmark.auto_delete_preferences[:when_reminder_sent], save_user_preferences: true }
+      )
       expect(user.user_option.bookmark_auto_delete_preference).to eq(Bookmark.auto_delete_preferences[:when_reminder_sent])
 
       bookmark = Bookmark.find_by(user: user)
-      subject.update(bookmark_id: bookmark.id, name: "test", reminder_at: 1.day.from_now, options: { auto_delete_preference: Bookmark.auto_delete_preferences[:on_owner_reply] })
+      subject.update(
+        bookmark_id: bookmark.id,
+        name: "test",
+        reminder_at: 1.day.from_now,
+        options: { auto_delete_preference: Bookmark.auto_delete_preferences[:on_owner_reply], save_user_preferences: true }
+      )
       expect(user.user_option.bookmark_auto_delete_preference).to eq(Bookmark.auto_delete_preferences[:on_owner_reply])
     end
   end