DEV: Refactor some core services

Extracted from https://github.com/discourse/discourse/pull/29129.

This patch makes the code more compliant with the upcoming service docs
best practices.

app/controllers/admin/config/about_controller.rb

@@ -36,8 +36,8 @@ class Admin::Config::AboutController < Admin::AdminController
     end
 
     settings_map.each do |name, value|
-      UpdateSiteSetting.call(
+      SiteSetting::Update.call(
-        guardian: guardian,
+        guardian:,
         setting_name: name,
         new_value: value,
         allow_changing_hidden: %i[

app/controllers/admin/site_settings_controller.rb

@@ -39,10 +39,10 @@ class Admin::SiteSettingsController < Admin::AdminController
 
     previous_value = value_or_default(SiteSetting.get(id)) if update_existing_users
 
-    UpdateSiteSetting.call(service_params.merge(setting_name: id, new_value: value)) do
+    SiteSetting::Update.call(service_params.merge(setting_name: id, new_value: value)) do
       on_success do
         if update_existing_users
-          SiteSettingUpdateExistingUsers.call(id, result.new_value, previous_value)
+          SiteSettingUpdateExistingUsers.call(id, result.contract.new_value, previous_value)
         end
         render body: nil
       end

app/controllers/admin/users_controller.rb

@@ -124,7 +124,7 @@ class Admin::UsersController < Admin::StaffController
       on_success do
         render_json_dump(
           suspension: {
-            suspend_reason: result.reason,
+            suspend_reason: result.contract.reason,
             full_suspend_reason: result.full_reason,
             suspended_till: result.user.suspended_till,
             suspended_at: result.user.suspended_at,

app/services/admin_notices/dismiss.rb

@@ -3,10 +3,8 @@
 class AdminNotices::Dismiss
   include Service::Base
 
-  model :admin_notice, optional: true
-
   policy :invalid_access
-
+  model :admin_notice, optional: true
   transaction do
     step :destroy
     step :reset_problem_check
@@ -14,14 +12,14 @@ class AdminNotices::Dismiss
 
   private
 
-  def fetch_admin_notice(id:)
-    AdminNotice.find_by(id: id)
-  end
-
   def invalid_access(guardian:)
     guardian.is_admin?
   end
 
+  def fetch_admin_notice(id:)
+    AdminNotice.find_by(id: id)
+  end
+
   def destroy(admin_notice:)
     return if admin_notice.blank?
 

app/services/site_setting/update.rb

@@ -0,0 +1,59 @@
+# frozen_string_literal: true
+
+class SiteSetting::Update
+  include Service::Base
+
+  policy :current_user_is_admin
+  contract do
+    attribute :setting_name
+    attribute :new_value
+    attribute :allow_changing_hidden, :boolean, default: false
+
+    before_validation do
+      self.setting_name = setting_name&.to_sym
+      self.new_value = new_value.to_s.strip
+    end
+
+    validates :setting_name, presence: true
+
+    after_validation do
+      next if setting_name.blank?
+      self.new_value =
+        case SiteSetting.type_supervisor.get_type(setting_name)
+        when :integer
+          new_value.tr("^-0-9", "").to_i
+        when :file_size_restriction
+          new_value.tr("^0-9", "").to_i
+        when :uploaded_image_list
+          new_value.blank? ? "" : Upload.get_from_urls(new_value.split("|")).to_a
+        when :upload
+          Upload.get_from_url(new_value) || ""
+        else
+          new_value
+        end
+    end
+  end
+  policy :setting_is_visible
+  policy :setting_is_configurable
+  step :save
+
+  private
+
+  def current_user_is_admin(guardian:)
+    guardian.is_admin?
+  end
+
+  def setting_is_visible(contract:)
+    contract.allow_changing_hidden || !SiteSetting.hidden_settings.include?(contract.setting_name)
+  end
+
+  def setting_is_configurable(contract:)
+    return true if !SiteSetting.plugins[contract.setting_name]
+
+    Discourse.plugins_by_name[SiteSetting.plugins[contract.setting_name]].configurable?
+  end
+
+  def save(contract:, guardian:)
+    SiteSetting.set_and_log(contract.setting_name, contract.new_value, guardian.user)
+  end
+end

app/services/update_site_setting.rb

@@ -1,57 +0,0 @@
-# frozen_string_literal: true
-
-class UpdateSiteSetting
-  include Service::Base
-
-  policy :current_user_is_admin
-  contract do
-    attribute :setting_name
-    attribute :new_value
-    attribute :allow_changing_hidden, :boolean, default: false
-
-    before_validation { self.setting_name = setting_name&.to_sym }
-
-    validates :setting_name, presence: true
-  end
-  policy :setting_is_visible
-  policy :setting_is_configurable
-  step :cleanup_value
-  step :save
-
-  private
-
-  def current_user_is_admin(guardian:)
-    guardian.is_admin?
-  end
-
-  def setting_is_visible(contract:)
-    contract.allow_changing_hidden || !SiteSetting.hidden_settings.include?(contract.setting_name)
-  end
-
-  def setting_is_configurable(contract:)
-    return true if !SiteSetting.plugins[contract.setting_name]
-
-    Discourse.plugins_by_name[SiteSetting.plugins[contract.setting_name]].configurable?
-  end
-
-  def cleanup_value(contract:)
-    new_value = contract.new_value
-    new_value = new_value.strip if new_value.is_a?(String)
-
-    case SiteSetting.type_supervisor.get_type(contract.setting_name)
-    when :integer
-      new_value = new_value.tr("^-0-9", "").to_i if new_value.is_a?(String)
-    when :file_size_restriction
-      new_value = new_value.tr("^0-9", "").to_i if new_value.is_a?(String)
-    when :uploaded_image_list
-      new_value = new_value.blank? ? "" : Upload.get_from_urls(new_value.split("|")).to_a
-    when :upload
-      new_value = Upload.get_from_url(new_value) || ""
-    end
-    context[:new_value] = new_value
-  end
-
-  def save(contract:, new_value:, guardian:)
-    SiteSetting.set_and_log(contract.setting_name, new_value, guardian.user)
-  end
-end

spec/services/admin_notices/dismiss_spec.rb

@@ -1,10 +1,14 @@
 # frozen_string_literal: true
 
 RSpec.describe(AdminNotices::Dismiss) do
-  subject(:result) { described_class.call(id: admin_notice.id, guardian: current_user.guardian) }
+  subject(:result) { described_class.call(**params, **dependencies) }
 
-  let!(:admin_notice) { Fabricate(:admin_notice, identifier: "problem.test") }
+  fab!(:current_user) { Fabricate(:admin) }
-  let!(:problem_check) { Fabricate(:problem_check_tracker, identifier: "problem.test", blips: 3) }
+  fab!(:admin_notice) { Fabricate(:admin_notice, identifier: "problem.test") }
+  fab!(:problem_check) { Fabricate(:problem_check_tracker, identifier: "problem.test", blips: 3) }
+
+  let(:params) { { id: admin_notice.id } }
+  let(:dependencies) { { guardian: current_user.guardian } }
 
   context "when user is not allowed to perform the action" do
     fab!(:current_user) { Fabricate(:user) }
@@ -13,22 +17,14 @@ RSpec.describe(AdminNotices::Dismiss) do
   end
 
   context "when the admin notice has already been dismissed" do
-    fab!(:current_user) { Fabricate(:admin) }
-
     before { admin_notice.destroy! }
 
     it { is_expected.to run_successfully }
   end
 
-  context "when user is allowed to perform the action" do
+  context "when everything's ok" do
-    fab!(:current_user) { Fabricate(:admin) }
-
     it { is_expected.to run_successfully }
 
-    it "sets the service result as successful" do
-      expect(result).to be_a_success
-    end
-
     it "destroys the admin notice" do
       expect { result }.to change { AdminNotice.count }.from(1).to(0)
     end

spec/services/site_setting/update_spec.rb

@@ -0,0 +1,79 @@
+# frozen_string_literal: true
+
+RSpec.describe SiteSetting::Update do
+  describe described_class::Contract, type: :model do
+    it { is_expected.to validate_presence_of :setting_name }
+  end
+
+  describe ".call" do
+    subject(:result) { described_class.call(**params, **options, **dependencies) }
+
+    fab!(:admin)
+    let(:params) { { setting_name:, new_value: } }
+    let(:options) { { allow_changing_hidden: } }
+    let(:dependencies) { { guardian: } }
+    let(:setting_name) { :title }
+    let(:new_value) { "blah whatever" }
+    let(:guardian) { admin.guardian }
+    let(:allow_changing_hidden) { false }
+
+    context "when setting_name is blank" do
+      let(:setting_name) { nil }
+
+      it { is_expected.to fail_a_contract }
+    end
+
+    context "when a non-admin user tries to change a setting" do
+      let(:guardian) { Guardian.new }
+
+      it { is_expected.to fail_a_policy(:current_user_is_admin) }
+    end
+
+    context "when the user changes a hidden setting" do
+      let(:setting_name) { :max_category_nesting }
+      let(:new_value) { 3 }
+
+      context "when allow_changing_hidden is false" do
+        it { is_expected.to fail_a_policy(:setting_is_visible) }
+      end
+
+      context "when allow_changing_hidden is true" do
+        let(:allow_changing_hidden) { true }
+
+        it { is_expected.to run_successfully }
+
+        it "updates the specified setting" do
+          expect { result }.to change { SiteSetting.max_category_nesting }.to(3)
+        end
+      end
+    end
+
+    context "when the user changes a visible setting" do
+      let(:new_value) { "hello this is title" }
+
+      it { is_expected.to run_successfully }
+
+      it "updates the specified setting" do
+        expect { result }.to change { SiteSetting.title }.to(new_value)
+      end
+
+      it "creates an entry in the staff action logs" do
+        expect { result }.to change {
+          UserHistory.where(
+            action: UserHistory.actions[:change_site_setting],
+            subject: "title",
+          ).count
+        }.by(1)
+      end
+
+      context "when value needs cleanup" do
+        let(:setting_name) { :max_image_size_kb }
+        let(:new_value) { "8zf843" }
+
+        it "cleans up the new setting value before using it" do
+          expect { result }.to change { SiteSetting.max_image_size_kb }.to(8843)
+        end
+      end
+    end
+  end
+end

spec/services/update_site_setting_spec.rb

@@ -1,74 +0,0 @@
-# frozen_string_literal: true
-
-RSpec.describe UpdateSiteSetting do
-  subject(:result) { described_class.call(params) }
-
-  describe described_class::Contract, type: :model do
-    subject(:contract) { described_class.new }
-
-    it { is_expected.to validate_presence_of :setting_name }
-  end
-
-  fab!(:admin)
-  let(:params) { { setting_name:, new_value:, guardian:, allow_changing_hidden: } }
-  let(:setting_name) { :title }
-  let(:new_value) { "blah whatever" }
-  let(:guardian) { admin.guardian }
-  let(:allow_changing_hidden) { false }
-
-  context "when setting_name is blank" do
-    let(:setting_name) { nil }
-
-    it { is_expected.to fail_a_contract }
-  end
-
-  context "when a non-admin user tries to change a setting" do
-    let(:guardian) { Guardian.new }
-
-    it { is_expected.to fail_a_policy(:current_user_is_admin) }
-  end
-
-  context "when the user changes a hidden setting" do
-    let(:setting_name) { :max_category_nesting }
-    let(:new_value) { 3 }
-
-    context "when allow_changing_hidden is false" do
-      it { is_expected.to fail_a_policy(:setting_is_visible) }
-    end
-
-    context "when allow_changing_hidden is true" do
-      let(:allow_changing_hidden) { true }
-
-      it { is_expected.to run_successfully }
-
-      it "updates the specified setting" do
-        expect { result }.to change { SiteSetting.max_category_nesting }.to(3)
-      end
-    end
-  end
-
-  context "when the user changes a visible setting" do
-    let(:new_value) { "hello this is title" }
-
-    it { is_expected.to run_successfully }
-
-    it "updates the specified setting" do
-      expect { result }.to change { SiteSetting.title }.to(new_value)
-    end
-
-    it "creates an entry in the staff action logs" do
-      expect { result }.to change {
-        UserHistory.where(action: UserHistory.actions[:change_site_setting], subject: "title").count
-      }.by(1)
-    end
-
-    context "when value needs cleanup" do
-      let(:setting_name) { :max_image_size_kb }
-      let(:new_value) { "8zf843" }
-
-      it "cleans up the new setting value before using it" do
-        expect { result }.to change { SiteSetting.max_image_size_kb }.to(8843)
-      end
-    end
-  end
-end

spec/services/user/silence_spec.rb

@@ -21,13 +21,14 @@ RSpec.describe User::Silence do
   end
 
   describe ".call" do
-    subject(:result) { described_class.call(params) }
+    subject(:result) { described_class.call(**params, **dependencies) }
 
     fab!(:admin)
     fab!(:user)
     fab!(:other_user) { Fabricate(:user) }
 
-    let(:params) { { guardian:, user_id:, reason:, silenced_till:, other_user_ids:, message: } }
+    let(:params) { { user_id:, reason:, silenced_till:, other_user_ids:, message: } }
+    let(:dependencies) { { guardian: } }
     let(:guardian) { admin.guardian }
     let(:user_id) { user.id }
     let(:other_user_ids) { other_user.id }
@@ -41,28 +42,25 @@ RSpec.describe User::Silence do
       it { is_expected.to fail_a_contract }
     end
 
-    context "when data is valid" do
     context "when provided user does not exist" do
       let(:user_id) { 0 }
 
       it { is_expected.to fail_to_find_a_model(:user) }
     end
 
-      context "when provided user exists" do
     context "when user is already silenced" do
       before { UserSilencer.silence(user, admin) }
 
       it { is_expected.to fail_a_policy(:not_silenced_already) }
     end
 
-        context "when user is not already silenced" do
     context "when all users cannot be silenced" do
       let(:other_user_ids) { [other_user.id, Fabricate(:admin).id].join(",") }
 
       it { is_expected.to fail_a_policy(:can_silence_all_users) }
     end
 
-          context "when all users can be silenced" do
+    context "when everything's ok" do
       before { allow(User::Action::TriggerPostAction).to receive(:call) }
 
       it "silences all provided users" do
@@ -88,7 +86,4 @@ RSpec.describe User::Silence do
       end
     end
   end
-      end
-    end
-  end
 end

spec/services/user/suspend_spec.rb

@@ -21,13 +21,14 @@ RSpec.describe User::Suspend do
   end
 
   describe ".call" do
-    subject(:result) { described_class.call(params) }
+    subject(:result) { described_class.call(**params, **dependencies) }
 
     fab!(:admin)
     fab!(:user)
     fab!(:other_user) { Fabricate(:user) }
 
-    let(:params) { { guardian:, user_id:, reason:, suspend_until:, other_user_ids:, message: } }
+    let(:params) { { user_id:, reason:, suspend_until:, other_user_ids:, message: } }
+    let(:dependencies) { { guardian: } }
     let(:guardian) { admin.guardian }
     let(:user_id) { user.id }
     let(:other_user_ids) { other_user.id }
@@ -41,14 +42,12 @@ RSpec.describe User::Suspend do
       it { is_expected.to fail_a_contract }
     end
 
-    context "when data is valid" do
     context "when provided user does not exist" do
       let(:user_id) { 0 }
 
       it { is_expected.to fail_to_find_a_model(:user) }
     end
 
-      context "when provided user exists" do
     context "when user is already suspended" do
       before do
         UserSuspender.new(user, by_user: admin, suspended_till: suspend_until, reason:).suspend
@@ -57,14 +56,13 @@ RSpec.describe User::Suspend do
       it { is_expected.to fail_a_policy(:not_suspended_already) }
     end
 
-        context "when user is not already suspended" do
     context "when all users cannot be suspended" do
       let(:other_user_ids) { [other_user.id, Fabricate(:admin).id].join(",") }
 
       it { is_expected.to fail_a_policy(:can_suspend_all_users) }
     end
 
-          context "when all users can be suspended" do
+    context "when everything's ok" do
       before { allow(User::Action::TriggerPostAction).to receive(:call) }
 
       it "suspends all provided users" do
@@ -86,7 +84,4 @@ RSpec.describe User::Suspend do
       end
     end
   end
-      end
-    end
-  end
 end