FIX: Applying default user options didn't work for boolean flags (#16890)

It also ensures that only human users are updated and replaces usage of `send` with `public_send`. Also, it adds more specs for existing code.

app/controllers/admin/site_settings_controller.rb

@@ -34,12 +34,12 @@ class Admin::SiteSettingsController < Admin::AdminController
     end
 
     update_existing_users = params[:update_existing_user].present?
-    previous_value = SiteSetting.public_send(id) || "" if update_existing_users
+    previous_value = value_or_default(SiteSetting.public_send(id)) if update_existing_users
 
     SiteSetting.set_and_log(id, value, current_user)
 
     if update_existing_users
-      new_value = value || ""
+      new_value = value_or_default(value)
 
       if (user_option = user_options[id.to_sym]).present?
         if user_option == "text_size_key"
@@ -53,7 +53,7 @@ class Admin::SiteSettingsController < Admin::AdminController
         attrs = { user_option => new_value }
         attrs[:email_digests] = (new_value.to_i != 0) if id == "default_email_digest_frequency"
 
-        UserOption.where(user_option => previous_value).update_all(attrs)
+        UserOption.human_users.where(user_option => previous_value).update_all(attrs)
       elsif id.start_with?("default_categories_")
         previous_category_ids = previous_value.split("|")
         new_category_ids = new_value.split("|")
@@ -106,10 +106,10 @@ class Admin::SiteSettingsController < Admin::AdminController
     params.require(:site_setting_id)
     id = params[:site_setting_id]
     raise Discourse::NotFound unless id.start_with?("default_")
-    new_value = params[id] || ""
+    new_value = value_or_default(params[id])
 
     raise_access_hidden_setting(id)
-    previous_value = SiteSetting.send(id) || ""
+    previous_value = value_or_default(SiteSetting.public_send(id))
     json = {}
 
     if (user_option = user_options[id.to_sym]).present?
@@ -119,7 +119,7 @@ class Admin::SiteSettingsController < Admin::AdminController
         previous_value = UserOption.title_count_modes[previous_value.to_sym]
       end
 
-      json[:user_count] = UserOption.where(user_option => previous_value).count
+      json[:user_count] = UserOption.human_users.where(user_option => previous_value).count
     elsif id.start_with?("default_categories_")
       previous_category_ids = previous_value.split("|")
       new_category_ids = new_value.split("|")
@@ -220,4 +220,8 @@ class Admin::SiteSettingsController < Admin::AdminController
       NotificationLevels.all[:regular]
     end
   end
+
+  def value_or_default(value)
+    value.nil? ? "" : value
+  end
 end

app/models/user_option.rb

@@ -11,6 +11,8 @@ class UserOption < ActiveRecord::Base
 
   after_save :update_tracked_topics
 
+  scope :human_users, -> { where('user_id > 0') }
+
   enum default_calendar: { none_selected: 0, ics: 1, google: 2 }, _scopes: false
 
   def self.ensure_consistency!

spec/requests/admin/site_settings_controller_spec.rb

@@ -102,7 +102,7 @@ describe Admin::SiteSettingsController do
               default_email_digest_frequency: 0,
               update_existing_user: true
             }
-          }.to change { UserOption.where(email_digests: false).count }.by(User.count)
+          }.to change { UserOption.where(email_digests: false).count }.by(User.human_users.count)
         end
       end
 
@@ -254,6 +254,70 @@ describe Admin::SiteSettingsController do
 
           SiteSetting.setting(:default_tags_watching, "")
         end
+
+        context "user options" do
+          def expect_user_count(site_setting_name:, user_setting_name:, current_site_setting_value:, new_site_setting_value:,
+                                current_user_setting_value: nil, new_user_setting_value: nil)
+
+            current_user_setting_value ||= current_site_setting_value
+            new_user_setting_value ||= new_site_setting_value
+
+            SiteSetting.public_send("#{site_setting_name}=", current_site_setting_value)
+            UserOption.human_users.update_all(user_setting_name => current_user_setting_value)
+            user_count = User.human_users.count
+
+            # Correctly counts users when all of them have default value
+            put "/admin/site_settings/#{site_setting_name}/user_count.json", params: {
+              site_setting_name => new_site_setting_value
+            }
+            expect(response.parsed_body["user_count"]).to eq(user_count)
+
+            # Correctly counts users when one of them already has new value
+            user.user_option.update!(user_setting_name => new_user_setting_value)
+            put "/admin/site_settings/#{site_setting_name}/user_count.json", params: {
+              site_setting_name => new_site_setting_value
+            }
+            expect(response.parsed_body["user_count"]).to eq(user_count - 1)
+
+            # Correctly counts users when site setting value has been changed
+            SiteSetting.public_send("#{site_setting_name}=", new_site_setting_value)
+            put "/admin/site_settings/#{site_setting_name}/user_count.json", params: {
+              site_setting_name => current_site_setting_value
+            }
+            expect(response.parsed_body["user_count"]).to eq(1)
+          end
+
+          it "should return correct user count for boolean setting" do
+            expect_user_count(
+              site_setting_name: "default_other_external_links_in_new_tab",
+              user_setting_name: "external_links_in_new_tab",
+              current_site_setting_value: false,
+              new_site_setting_value: true
+            )
+          end
+
+          it "should return correct user count for 'text_size_key'" do
+            expect_user_count(
+              site_setting_name: "default_text_size",
+              user_setting_name: "text_size_key",
+              current_site_setting_value: "normal",
+              new_site_setting_value: "larger",
+              current_user_setting_value: UserOption.text_sizes[:normal],
+              new_user_setting_value: UserOption.text_sizes[:larger]
+            )
+          end
+
+          it "should return correct user count for 'title_count_mode_key'" do
+            expect_user_count(
+              site_setting_name: "default_title_count_mode",
+              user_setting_name: "title_count_mode_key",
+              current_site_setting_value: "notifications",
+              new_site_setting_value: "contextual",
+              current_user_setting_value: UserOption.title_count_modes[:notifications],
+              new_user_setting_value: UserOption.title_count_modes[:contextual]
+            )
+          end
+        end
       end
 
       describe 'upload site settings' do