FIX: Remove user_option saving for bookmark auto delete pref (#19476)

We were changing the user's user_option.bookmark_auto_delete_preference
to whatever they changed it to in the bookmark modal to use as default
for future bookmarks. However this was leading to a lot of confusion
since if you wanted to set it for one bookmark you had to remember to
change it back on the next one.

This commit removes that automatic functionality, and instead moves
the bookmark auto delete preference to User Preferences > Interface
in an explicit dropdown.

app/assets/javascripts/discourse/app/components/bookmark.js

@@ -153,11 +153,6 @@ export default Component.extend({
       }
     }
 
-    this.currentUser.set(
-      "user_option.bookmark_auto_delete_preference",
-      this.autoDeletePreference
-    );
-
     const data = {
       reminder_at: reminderAtISO,
       name: this.model.name,

app/assets/javascripts/discourse/app/controllers/preferences/interface.js

@@ -1,4 +1,5 @@
 import Controller, { inject as controller } from "@ember/controller";
+import { AUTO_DELETE_PREFERENCES } from "discourse/models/bookmark";
 import Session from "discourse/models/session";
 import { setDefaultHomepage } from "discourse/lib/utilities";
 import {
@@ -61,6 +62,7 @@ export default Controller.extend({
       "seen_popups",
       "color_scheme_id",
       "dark_scheme_id",
+      "bookmark_auto_delete_preference",
     ];
 
     if (makeThemeDefault) {
@@ -105,6 +107,16 @@ export default Controller.extend({
     });
   },
 
+  @discourseComputed
+  bookmarkAfterNotificationModes() {
+    return Object.keys(AUTO_DELETE_PREFERENCES).map((key) => {
+      return {
+        value: AUTO_DELETE_PREFERENCES[key],
+        name: I18n.t(`bookmarks.auto_delete_preference.${key.toLowerCase()}`),
+      };
+    });
+  },
+
   @discourseComputed
   userSelectableThemes() {
     return listThemes(this.site);

app/assets/javascripts/discourse/app/templates/preferences/interface.hbs

@@ -116,6 +116,10 @@
     <label for="user-title-count-mode">{{i18n "user.title_count_mode.title"}}</label>
     <ComboBox @valueProperty="value" @content={{this.titleCountModes}} @value={{this.model.user_option.title_count_mode}} @id="user-title-count-mode" @onChange={{action (mut this.model.user_option.title_count_mode)}} />
   </div>
+  <div class="controls controls-dropdown pref-bookmark-after-notification">
+    <label for="bookmark-after-notification-mode">{{i18n "user.bookmark_after_notification.title"}}</label>
+    <ComboBox @valueProperty="value" @content={{this.bookmarkAfterNotificationModes}} @value={{this.model.user_option.bookmark_auto_delete_preference}} @id="boookmark-after-notification-mode" @onChange={{action (mut this.model.user_option.bookmark_auto_delete_preference)}} />
+  </div>
   <PreferenceCheckbox @labelKey="user.skip_new_user_tips.description" @checked={{this.model.user_option.skip_new_user_tips}} @class="pref-new-user-tips" />
   {{#if this.site.user_tips}}
     <DButton @class="pref-reset-seen-user-tips" @action={{action "resetSeenUserTips"}}>{{i18n "user.reset_seen_user_tips"}}</DButton>

app/assets/javascripts/discourse/tests/acceptance/bookmarks-test.js

@@ -10,7 +10,6 @@ import selectKit from "discourse/tests/helpers/select-kit-helper";
 import { test } from "qunit";
 import topicFixtures from "discourse/tests/fixtures/topic";
 import { cloneJSON } from "discourse-common/lib/object";
-import User from "discourse/models/user";
 
 async function openBookmarkModal(postNumber = 1) {
   if (exists(`#post_${postNumber} button.show-more-actions`)) {
@@ -169,12 +168,6 @@ acceptance("Bookmarking", function (needs) {
     await selectKit(".bookmark-option-selector").expand();
     await selectKit(".bookmark-option-selector").selectRowByValue(1);
     await click("#save-bookmark");
-
-    assert.equal(
-      User.currentProp("user_option.bookmark_auto_delete_preference"),
-      "1"
-    );
-
     await openEditBookmarkModal();
 
     assert.ok(

app/controllers/bookmarks_controller.rb

@@ -19,8 +19,7 @@ class BookmarksController < ApplicationController
       name: params[:name],
       reminder_at: params[:reminder_at],
       options: {
-        auto_delete_preference: params[:auto_delete_preference],
-        save_user_preferences: true
+        auto_delete_preference: params[:auto_delete_preference]
       }
     )
 
@@ -47,8 +46,7 @@ class BookmarksController < ApplicationController
       name: params[:name],
       reminder_at: params[:reminder_at],
       options: {
-        auto_delete_preference: params[:auto_delete_preference],
-        save_user_preferences: true
+        auto_delete_preference: params[:auto_delete_preference]
       }
     )
 

app/serializers/user_option_serializer.rb

@@ -31,6 +31,7 @@ class UserOptionSerializer < ApplicationSerializer
              :text_size,
              :text_size_seq,
              :title_count_mode,
+             :bookmark_auto_delete_preference,
              :timezone,
              :skip_new_user_tips,
              :default_calendar,

app/services/user_updater.rb

@@ -49,7 +49,8 @@ class UserUpdater
     :skip_new_user_tips,
     :seen_popups,
     :default_calendar,
-    :sidebar_list_destination
+    :sidebar_list_destination,
+    :bookmark_auto_delete_preference
   ]
 
   NOTIFICATION_SCHEDULE_ATTRS = -> {

config/locales/client.en.yml

@@ -1618,6 +1618,8 @@ en:
         title: "Background page title displays count of:"
         notifications: "New notifications"
         contextual: "New page content"
+      bookmark_after_notification:
+        title: "After a bookmark reminder notification is sent:"
 
       like_notification_frequency:
         title: "Notify when liked"

lib/bookmark_manager.rb

@@ -57,7 +57,6 @@ class BookmarkManager
     return add_errors_from(bookmark) if bookmark.errors.any?
 
     registered_bookmarkable.after_create(@guardian, bookmark, options)
-    update_user_option(bookmark, options)
 
     bookmark
   end
@@ -109,8 +108,6 @@ class BookmarkManager
       return add_errors_from(bookmark)
     end
 
-    update_user_option(bookmark, options)
-
     success
   end
 
@@ -142,15 +139,6 @@ class BookmarkManager
     TopicUser.change(@user.id, topic, bookmarked: Bookmark.for_user_in_topic(@user.id, topic.id).exists?)
   end
 
-  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]

spec/lib/bookmark_manager_spec.rb

@@ -306,67 +306,5 @@ RSpec.describe BookmarkManager do
         expect { subject.create_for(bookmarkable_id: post.id, bookmarkable_type: "Post", name: name) }.to raise_error(Discourse::InvalidAccess)
       end
     end
-
-    it "does not save user preference by default" 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] }
-      )
-      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 }
-      )
-      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)
-      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 }
-      )
-      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
 end

spec/requests/api/schemas/json/user_get_response.json

@@ -672,6 +672,9 @@
             "external_links_in_new_tab": {
               "type": "boolean"
             },
+            "bookmark_auto_delete_preference": {
+              "type": "integer"
+            },
             "color_scheme_id": {
               "type": [
                 "string",

spec/system/user_preferences_interface_spec.rb

@@ -0,0 +1,36 @@
+# frozen_string_literal: true
+
+describe "User preferences for Interface", type: :system, js: true do
+  fab!(:user) { Fabricate(:user) }
+  let(:user_preferences_page) { PageObjects::Pages::UserPreferences.new }
+
+  before { sign_in(user) }
+
+  describe "Bookmarks" do
+    it "changes the bookmark after notification preference" do
+      user_preferences_page.visit(user)
+      click_link "Interface"
+
+      # preselects the default user_option.bookmark_auto_delete_preference value of 3 (clear_reminder)
+      expect(page).to have_css(
+        "#boookmark-after-notification-mode .select-kit-header[data-value='#{Bookmark.auto_delete_preferences[:clear_reminder]}']",
+      )
+      page.find("#boookmark-after-notification-mode").click
+      page.find(
+        ".select-kit-row[data-value=\"#{Bookmark.auto_delete_preferences[:when_reminder_sent]}\"]",
+      ).click
+
+      click_button "Save Changes"
+
+      # the preference page reloads after saving, so we need to poll the db
+      try_until_success do
+        expect(
+          UserOption.exists?(
+            user_id: user.id,
+            bookmark_auto_delete_preference: Bookmark.auto_delete_preferences[:when_reminder_sent],
+          ),
+        ).to be_truthy
+      end
+    end
+  end
+end