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.

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

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

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

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

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}"

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 }
 

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 }