From eef17318c31897d4e6805c27d41e172b80b57e8c Mon Sep 17 00:00:00 2001 From: Gerhard Schlager Date: Mon, 23 May 2022 15:20:51 +0200 Subject: [PATCH] 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. --- .../admin/site_settings_controller.rb | 16 +++-- app/models/user_option.rb | 2 + .../admin/site_settings_controller_spec.rb | 66 ++++++++++++++++++- 3 files changed, 77 insertions(+), 7 deletions(-) diff --git a/app/controllers/admin/site_settings_controller.rb b/app/controllers/admin/site_settings_controller.rb index 06cea0a2167..197269f0d09 100644 --- a/app/controllers/admin/site_settings_controller.rb +++ b/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 diff --git a/app/models/user_option.rb b/app/models/user_option.rb index b35cf37453a..0db31719a95 100644 --- a/app/models/user_option.rb +++ b/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! diff --git a/spec/requests/admin/site_settings_controller_spec.rb b/spec/requests/admin/site_settings_controller_spec.rb index 4fcb0a059f8..cc9859a54eb 100644 --- a/spec/requests/admin/site_settings_controller_spec.rb +++ b/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