From b58d168f059bf305dbd8b4dbc72d458e76ed2d99 Mon Sep 17 00:00:00 2001
From: Martin Brennan <martin@discourse.org>
Date: Tue, 30 Aug 2022 09:21:41 +1000
Subject: [PATCH] FIX: Do not save default auto_delete_preference for bookmark
 (#18119)

We don't want to save the auto_delete_preference for bookmarks to the
user options if it was passed through as nil from the frontend,
this leads to confusion for the end user since they did not explicitly set it.

It's fine to create the bookmark with the default of "never" if no
auto_delete_preference is provided since it applies only to the
single bookmark, not future bookmarks.
---
 lib/bookmark_manager.rb           | 12 ++++++++++--
 spec/lib/bookmark_manager_spec.rb | 24 ++++++++++++++++++++++++
 2 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/lib/bookmark_manager.rb b/lib/bookmark_manager.rb
index d7d7d2aaa97..982b18a737f 100644
--- a/lib/bookmark_manager.rb
+++ b/lib/bookmark_manager.rb
@@ -144,16 +144,24 @@ class BookmarkManager
 
   def update_user_option(bookmark, options)
     return if !options[:save_user_preferences]
+    return if options[:auto_delete_preference].blank?
+
     @user.user_option.update!(
       bookmark_auto_delete_preference: bookmark.auto_delete_preference
     )
   end
 
   def bookmark_model_options_with_defaults(options)
+    model_options = {
+      pinned: options[:pinned]
+    }
+
     if options[:auto_delete_preference].blank?
-      options[:auto_delete_preference] = Bookmark.auto_delete_preferences[:never]
+      model_options[:auto_delete_preference] = Bookmark.auto_delete_preferences[:never]
+    else
+      model_options[:auto_delete_preference] = options[:auto_delete_preference]
     end
 
-    options.slice(:auto_delete_preference, :pinned)
+    model_options
   end
 end
diff --git a/spec/lib/bookmark_manager_spec.rb b/spec/lib/bookmark_manager_spec.rb
index 4ea077e7739..328f7d1a348 100644
--- a/spec/lib/bookmark_manager_spec.rb
+++ b/spec/lib/bookmark_manager_spec.rb
@@ -333,6 +333,7 @@ RSpec.describe BookmarkManager do
         bookmarkable_type: "Post",
         options: { auto_delete_preference: Bookmark.auto_delete_preferences[:when_reminder_sent], save_user_preferences: true }
       )
+      user.user_option.reload
       expect(user.user_option.bookmark_auto_delete_preference).to eq(Bookmark.auto_delete_preferences[:when_reminder_sent])
 
       bookmark = Bookmark.find_by(user: user)
@@ -342,6 +343,29 @@ RSpec.describe BookmarkManager do
         reminder_at: 1.day.from_now,
         options: { auto_delete_preference: Bookmark.auto_delete_preferences[:on_owner_reply], save_user_preferences: true }
       )
+      user.user_option.reload
+      expect(user.user_option.bookmark_auto_delete_preference).to eq(Bookmark.auto_delete_preferences[:on_owner_reply])
+    end
+
+    it "does not save user preferences when save_user_preferences is false" 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: false }
+      )
+      user.user_option.reload
+      expect(user.user_option.bookmark_auto_delete_preference).to eq(Bookmark.auto_delete_preferences[:on_owner_reply])
+    end
+
+    it "does not save user preferences when save_user_preferences is true and auto_delete_preference is nil" 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: nil, save_user_preferences: true }
+      )
+      user.user_option.reload
       expect(user.user_option.bookmark_auto_delete_preference).to eq(Bookmark.auto_delete_preferences[:on_owner_reply])
     end
   end