From c8ccd4da315d66746e35c75d494e0f5fb9cdaad6 Mon Sep 17 00:00:00 2001 From: Ted Johansson Date: Wed, 26 Mar 2025 15:56:18 +0800 Subject: [PATCH] DEV: Move setting deprecation check to SiteSetting::Update service (#31993) This PR moves the logic that checks if a site setting we're trying to update has been deprecated from one of the controllers into a policy of the SiteSetting::Update service. It also gives us the opportunity to shift the error message into a locale file. --- .../admin/config/about_controller.rb | 3 ++ .../admin/site_settings_controller.rb | 20 +++-------- .../policy/settings_are_not_deprecated.rb | 33 +++++++++++++++++++ app/services/site_setting/update.rb | 14 ++++++-- config/locales/server.en.yml | 1 + .../admin/site_settings_controller_spec.rb | 13 +++++++- spec/services/site_setting/update_spec.rb | 27 +++++++++++++++ 7 files changed, 91 insertions(+), 20 deletions(-) create mode 100644 app/services/site_setting/policy/settings_are_not_deprecated.rb diff --git a/app/controllers/admin/config/about_controller.rb b/app/controllers/admin/config/about_controller.rb index a881edc35c4..ccb6ff2f99f 100644 --- a/app/controllers/admin/config/about_controller.rb +++ b/app/controllers/admin/config/about_controller.rb @@ -51,6 +51,9 @@ class Admin::Config::AboutController < Admin::AdminController }, ) do on_success { render json: success_json } + on_failed_policy(:settings_are_not_deprecated) do |policy| + raise Discourse::InvalidParameters, policy.reason + end on_failed_policy(:settings_are_visible) do |policy| raise Discourse::InvalidParameters, policy.reason end diff --git a/app/controllers/admin/site_settings_controller.rb b/app/controllers/admin/site_settings_controller.rb index 2136f51b1a1..1b109db4f20 100644 --- a/app/controllers/admin/site_settings_controller.rb +++ b/app/controllers/admin/site_settings_controller.rb @@ -21,25 +21,10 @@ class Admin::SiteSettingsController < Admin::AdminController params.require(:id) id = params[:id] update_existing_users = params[:update_existing_user].present? - value = params[id] - - new_setting_name = - SiteSettings::DeprecatedSettings::SETTINGS.find do |old_name, new_name, override, _| - if old_name == id - if !override - raise Discourse::InvalidParameters, - "You cannot change this site setting because it is deprecated, use #{new_name} instead." - end - - break new_name - end - end - - id = new_setting_name if new_setting_name previous_value = value_or_default(SiteSetting.get(id)) if update_existing_users - SiteSetting::Update.call(params: { settings: { id => value } }, guardian:) do + SiteSetting::Update.call(params: { settings: { id => params[id] } }, guardian:) do on_success do |params:| if update_existing_users params.settings.to_a.each do |setting_name, setting_value| @@ -48,6 +33,9 @@ class Admin::SiteSettingsController < Admin::AdminController end render body: nil end + on_failed_policy(:settings_are_not_deprecated) do |policy| + raise Discourse::InvalidParameters, policy.reason + end on_failed_policy(:settings_are_visible) do |policy| raise Discourse::InvalidParameters, policy.reason end diff --git a/app/services/site_setting/policy/settings_are_not_deprecated.rb b/app/services/site_setting/policy/settings_are_not_deprecated.rb new file mode 100644 index 00000000000..1ca43e47b10 --- /dev/null +++ b/app/services/site_setting/policy/settings_are_not_deprecated.rb @@ -0,0 +1,33 @@ +# frozen_string_literal: true + +class SiteSetting::Policy::SettingsAreNotDeprecated < Service::PolicyBase + delegate :options, :params, to: :context + + def call + @hard_deprecations = + params.settings.keys.filter_map do |id| + SiteSettings::DeprecatedSettings::SETTINGS.find do |old_name, new_name, override, _| + if old_name.to_sym == id + if override + options.overridden_setting_names[old_name.to_sym] = new_name + break + else + break old_name, new_name + end + end + end + end + + @hard_deprecations.empty? + end + + def reason + old_names, new_names = @hard_deprecations.transpose + + I18n.t( + "errors.site_settings.site_settings_are_deprecated", + old_names: old_names.join(", "), + new_names: new_names.join(", "), + ) + end +end diff --git a/app/services/site_setting/update.rb b/app/services/site_setting/update.rb index 910a686aa0c..97c02aa93bd 100644 --- a/app/services/site_setting/update.rb +++ b/app/services/site_setting/update.rb @@ -3,7 +3,10 @@ class SiteSetting::Update include Service::Base - options { attribute :allow_changing_hidden, :array, default: [] } + options do + attribute :allow_changing_hidden, :array, default: [] + attribute :overridden_setting_names, default: {} + end policy :current_user_is_admin @@ -40,6 +43,7 @@ class SiteSetting::Update end end + policy :settings_are_not_deprecated, class_name: SiteSetting::Policy::SettingsAreNotDeprecated policy :settings_are_unshadowed_globally, class_name: SiteSetting::Policy::SettingsAreUnshadowedGlobally policy :settings_are_visible, class_name: SiteSetting::Policy::SettingsAreVisible @@ -53,9 +57,13 @@ class SiteSetting::Update guardian.is_admin? end - def save(params:, guardian:) + def save(params:, options:, guardian:) params.settings.each do |setting_name, value| - SiteSetting.set_and_log(setting_name, value, guardian.user) + SiteSetting.set_and_log( + options.overridden_setting_names[setting_name] || setting_name, + value, + guardian.user, + ) end end end diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 65fd7726b9d..d2da8edb57d 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -341,6 +341,7 @@ en: site_settings: invalid_site_setting: "No setting named '%{name}' exists" invalid_category_id: "You specified a category that does not exist" + site_settings_are_deprecated: "The following settings are deprecated: %{old_names}. Use %{new_names} instead" site_settings_are_hidden: "You are not allowed to change hidden settings: %{setting_names}" site_settings_are_shadowed_globally: "You cannot change those settings because they are globally configured: %{setting_names}" site_settings_are_unconfigurable: "You are not allowed to change unconfigurable settings: %{setting_names}" diff --git a/spec/requests/admin/site_settings_controller_spec.rb b/spec/requests/admin/site_settings_controller_spec.rb index db24f2fd189..043bdddbfb0 100644 --- a/spec/requests/admin/site_settings_controller_spec.rb +++ b/spec/requests/admin/site_settings_controller_spec.rb @@ -235,7 +235,18 @@ RSpec.describe Admin::SiteSettingsController do expect(SiteSetting.title).to eq("hello") end - it "works for deprecated settings" do + it "throws an error for hard deprecated settings" do + stub_deprecated_settings!(override: false) do + put "/admin/site_settings/old_one.json", params: { old_one: true } + + expect(response.status).to eq(422) + expect(response.parsed_body["errors"]).to contain_exactly( + "The following settings are deprecated: old_one. Use new_one instead", + ) + end + end + + it "works for soft deprecated settings" do stub_deprecated_settings!(override: true) do put "/admin/site_settings/old_one.json", params: { old_one: true } diff --git a/spec/services/site_setting/update_spec.rb b/spec/services/site_setting/update_spec.rb index 90c84e1ad3a..01b7097944f 100644 --- a/spec/services/site_setting/update_spec.rb +++ b/spec/services/site_setting/update_spec.rb @@ -30,6 +30,33 @@ RSpec.describe SiteSetting::Update do it { is_expected.to fail_a_policy(:current_user_is_admin) } end + context "when trying to change a deprecated setting" do + let(:hard_deprecated_setting) { ["suggested_topics", "new_suggested_topics", false, "3.3"] } + + let(:soft_deprecated_setting) do + ["suggested_topics", "suggested_topics_unread_max_days_old", true, "3.3"] + end + + let(:setting_name) { :suggested_topics } + let(:new_value) { 3 } + + context "when trying to change a hard deprecated setting" do + it "does not pass" do + stub_const(SiteSettings::DeprecatedSettings, "SETTINGS", [hard_deprecated_setting]) do + is_expected.to fail_a_policy(:settings_are_not_deprecated) + end + end + end + + context "when trying to change a soft deprecated (renamed) setting" do + it "updates the new setting" do + stub_const(SiteSettings::DeprecatedSettings, "SETTINGS", [soft_deprecated_setting]) do + expect { result }.to change { SiteSetting.suggested_topics_unread_max_days_old }.to(3) + end + end + end + end + context "when the user changes a hidden setting" do let(:setting_name) { :max_category_nesting } let(:new_value) { 3 }