DEV: Use Service::Base for suspend and silence actions (#28459)

This commit moves the business logic in the `Admin::UsersController#suspend` and `Admin::UsersController#silence` actions to dedicated service classes. There's no functional changes in this commit.

Internal topic: t/130014.

app/controllers/admin/users_controller.rb

@@ -1,8 +1,6 @@
 # frozen_string_literal: true
 
 class Admin::UsersController < Admin::StaffController
-  MAX_SIMILAR_USERS = 10
-
   before_action :fetch_user,
                 only: %i[
                   suspend
@@ -62,7 +60,7 @@ class Admin::UsersController < Admin::StaffController
       {
         users:
           ActiveModel::ArraySerializer.new(
-            @user.similar_users.limit(MAX_SIMILAR_USERS),
+            @user.similar_users.limit(User::MAX_SIMILAR_USERS),
             each_serializer: SimilarAdminUserSerializer,
             scope: guardian,
             root: false,
@@ -121,14 +119,22 @@ class Admin::UsersController < Admin::StaffController
   end
 
   def suspend
-    guardian.ensure_can_suspend!(@user)
+    with_service(SuspendUser, user: @user) do
-    reason = params[:reason]
+      on_success do
-
+        render_json_dump(
-    if reason && (!reason.is_a?(String) || reason.size > 300)
+          suspension: {
-      raise Discourse::InvalidParameters.new(:reason)
+            suspend_reason: result.reason,
+            full_suspend_reason: result.user_history&.details,
+            suspended_till: @user.suspended_till,
+            suspended_at: @user.suspended_at,
+            suspended_by: BasicUserSerializer.new(current_user, root: false).as_json,
+          },
+        )
       end
 
-    if @user.suspended?
+      on_failed_policy(:can_suspend) { raise Discourse::InvalidAccess.new }
+
+      on_failed_policy(:not_suspended_already) do
         suspend_record = @user.suspend_record
         message =
           I18n.t(
@@ -141,50 +147,13 @@ class Admin::UsersController < Admin::StaffController
                 scope: :"datetime.distance_in_words_verbose",
               ),
           )
-      return render json: failed_json.merge(message: message), status: 409
+        render json: failed_json.merge(message: message), status: 409
       end
 
-    params.require(%i[suspend_until reason])
+      on_failed_contract do |contract|
-
+        render json: failed_json.merge(errors: contract.errors.full_messages), status: 400
-    all_users = [@user]
-    if Array === params[:other_user_ids]
-      if params[:other_user_ids].size > MAX_SIMILAR_USERS
-        raise Discourse::InvalidParameters.new(:other_user_ids)
       end
-
-      all_users.concat(User.where(id: params[:other_user_ids]).to_a)
-      all_users.uniq!
     end
-
-    user_history = nil
-
-    all_users.each { |user| raise Discourse::InvalidAccess.new if !guardian.can_suspend?(user) }
-
-    all_users.each do |user|
-      suspender =
-        UserSuspender.new(
-          user,
-          suspended_till: params[:suspend_until],
-          reason: params[:reason],
-          by_user: current_user,
-          message: params[:message],
-          post_id: params[:post_id],
-        )
-      suspender.suspend
-      user_history = suspender.user_history
-    end
-
-    perform_post_action
-
-    render_json_dump(
-      suspension: {
-        suspend_reason: params[:reason],
-        full_suspend_reason: user_history&.details,
-        suspended_till: @user.suspended_till,
-        suspended_at: @user.suspended_at,
-        suspended_by: BasicUserSerializer.new(current_user, root: false).as_json,
-      },
-    )
   end
 
   def unsuspend
@@ -359,13 +328,22 @@ class Admin::UsersController < Admin::StaffController
   end
 
   def silence
-    reason = params[:reason]
+    with_service(SilenceUser, user: @user) do
-
+      on_success do
-    if reason && (!reason.is_a?(String) || reason.size > 300)
+        render_json_dump(
-      raise Discourse::InvalidParameters.new(:reason)
+          silence: {
+            silenced: true,
+            silence_reason: result.user_history&.details,
+            silenced_till: @user.silenced_till,
+            silenced_at: @user.silenced_at,
+            silenced_by: BasicUserSerializer.new(current_user, root: false).as_json,
+          },
+        )
       end
 
-    if @user.silenced?
+      on_failed_policy(:can_silence) { raise Discourse::InvalidAccess.new }
+
+      on_failed_policy(:not_silenced_already) do
         silenced_record = @user.silenced_record
         message =
           I18n.t(
@@ -378,59 +356,13 @@ class Admin::UsersController < Admin::StaffController
                 scope: :"datetime.distance_in_words_verbose",
               ),
           )
-      return render json: failed_json.merge(message: message), status: 409
+        render json: failed_json.merge(message: message), status: 409
       end
 
-    all_users = [@user]
+      on_failed_contract do |contract|
-    if Array === params[:other_user_ids]
+        render json: failed_json.merge(errors: contract.errors.full_messages), status: 400
-      if params[:other_user_ids].size > MAX_SIMILAR_USERS
-        raise Discourse::InvalidParameters.new(:other_user_ids)
-      end
-
-      all_users.concat(User.where(id: params[:other_user_ids]).to_a)
-      all_users.uniq!
-    end
-
-    user_history = nil
-
-    all_users.each do |user|
-      raise Discourse::InvalidAccess.new if !guardian.can_silence_user?(user)
-    end
-
-    all_users.each do |user|
-      silencer =
-        UserSilencer.new(
-          user,
-          current_user,
-          silenced_till: params[:silenced_till],
-          reason: params[:reason],
-          message_body: params[:message],
-          keep_posts: true,
-          post_id: params[:post_id],
-        )
-
-      if silencer.silence
-        user_history = silencer.user_history
-        Jobs.enqueue(
-          :critical_user_email,
-          type: "account_silenced",
-          user_id: user.id,
-          user_history_id: user_history.id,
-        )
       end
     end
-
-    perform_post_action
-
-    render_json_dump(
-      silence: {
-        silenced: true,
-        silence_reason: user_history.try(:details),
-        silenced_till: @user.silenced_till,
-        silenced_at: @user.silenced_at,
-        silenced_by: BasicUserSerializer.new(current_user, root: false).as_json,
-      },
-    )
   end
 
   def unsilence
@@ -612,31 +544,6 @@ class Admin::UsersController < Admin::StaffController
 
   private
 
-  def perform_post_action
-    return if params[:post_id].blank? || params[:post_action].blank?
-
-    if post = Post.where(id: params[:post_id]).first
-      case params[:post_action]
-      when "delete"
-        PostDestroyer.new(current_user, post).destroy if guardian.can_delete_post_or_topic?(post)
-      when "delete_replies"
-        if guardian.can_delete_post_or_topic?(post)
-          PostDestroyer.delete_with_replies(current_user, post)
-        end
-      when "edit"
-        revisor = PostRevisor.new(post)
-
-        # Take what the moderator edited in as gospel
-        revisor.revise!(
-          current_user,
-          { raw: params[:post_edit] },
-          skip_validations: true,
-          skip_revision: true,
-        )
-      end
-    end
-  end
-
   def fetch_user
     @user = User.find_by(id: params[:user_id])
     raise Discourse::NotFound unless @user

app/models/user.rb

@@ -14,6 +14,8 @@ class User < ActiveRecord::Base
   TARGET_PASSWORD_ALGORITHM =
     "$pbkdf2-#{Rails.configuration.pbkdf2_algorithm}$i=#{Rails.configuration.pbkdf2_iterations},l=32$"
 
+  MAX_SIMILAR_USERS = 10
+
   deprecate_column :flag_level, drop_from: "3.2"
 
   # not deleted on user delete

app/services/action/suspend_silence_post_action.rb

@@ -0,0 +1,30 @@
+# frozen_string_literal: true
+
+module Action
+  class SuspendSilencePostAction
+    def self.call(guardian:, context:)
+      return if context.post_id.blank? || context.post_action.blank?
+
+      if post = Post.where(id: context.post_id).first
+        case context.post_action
+        when "delete"
+          PostDestroyer.new(guardian.user, post).destroy if guardian.can_delete_post_or_topic?(post)
+        when "delete_replies"
+          if guardian.can_delete_post_or_topic?(post)
+            PostDestroyer.delete_with_replies(guardian.user, post)
+          end
+        when "edit"
+          revisor = PostRevisor.new(post)
+
+          # Take what the moderator edited in as gospel
+          revisor.revise!(
+            guardian.user,
+            { raw: context.post_edit },
+            skip_validations: true,
+            skip_revision: true,
+          )
+        end
+      end
+    end
+  end
+end

app/services/silence_user.rb

@@ -0,0 +1,82 @@
+# frozen_string_literal: true
+
+class SilenceUser
+  include Service::Base
+
+  contract
+
+  step :set_users
+
+  policy :can_silence
+  policy :not_silenced_already
+
+  step :silence
+  step :perform_post_action
+
+  class Contract
+    attribute :reason, :string
+    attribute :message, :string
+    attribute :silenced_till, :string
+    attribute :other_user_ids, :array
+    attribute :post_id, :string
+    attribute :post_action, :string
+    attribute :post_edit, :string
+
+    validates :reason, presence: true, length: { maximum: 300 }
+    validates :silenced_till, presence: true
+    validates :other_user_ids, length: { maximum: User::MAX_SIMILAR_USERS }
+  end
+
+  private
+
+  def set_users(user:)
+    list = [user]
+
+    if context.other_user_ids.present?
+      list.concat(User.where(id: context.other_user_ids).to_a)
+      list.uniq!
+    end
+
+    context.users = list
+  end
+
+  def can_silence(guardian:, users:)
+    users.all? { |user| guardian.can_silence_user?(user) }
+  end
+
+  def not_silenced_already(user:)
+    !user.silenced?
+  end
+
+  def silence(guardian:, users:, silenced_till:, reason:)
+    users.each do |user|
+      silencer =
+        UserSilencer.new(
+          user,
+          guardian.user,
+          silenced_till: silenced_till,
+          reason: reason,
+          message_body: context.message,
+          keep_posts: true,
+          post_id: context.post_id,
+        )
+
+      if silencer.silence
+        user_history = silencer.user_history
+        Jobs.enqueue(
+          :critical_user_email,
+          type: "account_silenced",
+          user_id: user.id,
+          user_history_id: user_history.id,
+        )
+        context.user_history = user_history
+      end
+    rescue => err
+      Discourse.warn_exception(err, message: "failed to silence user with ID #{user.id}")
+    end
+  end
+
+  def perform_post_action(guardian:)
+    Action::SuspendSilencePostAction.call(guardian:, context: context)
+  end
+end

app/services/suspend_user.rb

@@ -0,0 +1,72 @@
+# frozen_string_literal: true
+
+class SuspendUser
+  include Service::Base
+
+  contract
+
+  step :set_users
+
+  policy :can_suspend
+  policy :not_suspended_already
+
+  step :suspend
+  step :perform_post_action
+
+  class Contract
+    attribute :reason, :string
+    attribute :message, :string
+    attribute :suspend_until, :string
+    attribute :other_user_ids, :array
+    attribute :post_id, :string
+    attribute :post_action, :string
+    attribute :post_edit, :string
+
+    validates :reason, presence: true, length: { maximum: 300 }
+    validates :suspend_until, presence: true
+    validates :other_user_ids, length: { maximum: User::MAX_SIMILAR_USERS }
+  end
+
+  private
+
+  def set_users(user:)
+    list = [user]
+
+    if context.other_user_ids.present?
+      list.concat(User.where(id: context.other_user_ids).to_a)
+      list.uniq!
+    end
+
+    context.users = list
+  end
+
+  def can_suspend(guardian:, users:)
+    users.all? { |user| guardian.can_suspend?(user) }
+  end
+
+  def not_suspended_already(user:)
+    !user.suspended?
+  end
+
+  def suspend(guardian:, users:, suspend_until:, reason:)
+    users.each do |user|
+      suspender =
+        UserSuspender.new(
+          user,
+          suspended_till: suspend_until,
+          reason: reason,
+          by_user: guardian.user,
+          message: context.message,
+          post_id: context.post_id,
+        )
+      suspender.suspend
+      context.user_history = suspender.user_history
+    rescue => err
+      Discourse.warn_exception(err, message: "failed to suspend user with ID #{user.id}")
+    end
+  end
+
+  def perform_post_action(guardian:)
+    Action::SuspendSilencePostAction.call(guardian:, context: context)
+  end
+end

spec/requests/admin/users_controller_spec.rb

@@ -402,6 +402,27 @@ RSpec.describe Admin::UsersController do
         expect(user).not_to be_suspended
       end
 
+      it "fails the request if other_user_ids is too big" do
+        another_user = Fabricate(:user)
+        other_user_ids = [another_user.id]
+        other_user_ids.push(*(1..304).to_a)
+
+        put "/admin/users/#{user.id}/suspend.json",
+            params: {
+              reason: "because I said so",
+              suspend_until: 5.hours.from_now,
+              other_user_ids:,
+            }
+
+        expect(response.status).to eq(400)
+
+        user.reload
+        expect(user).not_to be_suspended
+
+        another_user.reload
+        expect(another_user).not_to be_suspended
+      end
+
       context "with an associated post" do
         it "can have an associated post" do
           put "/admin/users/#{user.id}/suspend.json", params: suspend_params
@@ -1561,7 +1582,11 @@ RSpec.describe Admin::UsersController do
       end
 
       it "doesn't allow silencing another admin" do
-        put "/admin/users/#{another_admin.id}/silence.json"
+        put "/admin/users/#{another_admin.id}/silence.json",
+            params: {
+              reason: "because reasons",
+              silenced_till: 6.hours.from_now,
+            }
         expect(response.status).to eq(403)
         expect(another_admin.reload).to_not be_silenced
       end
@@ -1570,6 +1595,8 @@ RSpec.describe Admin::UsersController do
         put "/admin/users/#{reg_user.id}/silence.json",
             params: {
               other_user_ids: [another_admin.id],
+              reason: "because reasons",
+              silenced_till: 6.hours.from_now,
             }
         expect(response.status).to eq(403)
         expect(another_admin.reload).to_not be_silenced
@@ -1577,7 +1604,11 @@ RSpec.describe Admin::UsersController do
       end
 
       it "punishes the user for spamming" do
-        put "/admin/users/#{reg_user.id}/silence.json"
+        put "/admin/users/#{reg_user.id}/silence.json",
+            params: {
+              reason: "because reasons",
+              silenced_till: 7.hours.from_now,
+            }
         expect(response.status).to eq(200)
         reg_user.reload
         expect(reg_user).to be_silenced
@@ -1589,6 +1620,8 @@ RSpec.describe Admin::UsersController do
 
         put "/admin/users/#{reg_user.id}/silence.json",
             params: {
+              reason: "because reasons",
+              silenced_till: 7.hours.from_now,
               post_id: silence_post.id,
               post_action: "edit",
               post_edit: "this is the new contents for the post",
@@ -1612,7 +1645,11 @@ RSpec.describe Admin::UsersController do
 
       it "will set a length of time if provided" do
         future_date = 1.month.from_now.to_date
-        put "/admin/users/#{reg_user.id}/silence.json", params: { silenced_till: future_date }
+        put "/admin/users/#{reg_user.id}/silence.json",
+            params: {
+              reason: "because reasons",
+              silenced_till: future_date,
+            }
 
         expect(response.status).to eq(200)
         reg_user.reload
@@ -1624,6 +1661,8 @@ RSpec.describe Admin::UsersController do
         expect do
           put "/admin/users/#{reg_user.id}/silence.json",
               params: {
+                reason: "none of your biz",
+                silenced_till: 666.hours.from_now,
                 message: "Email this to the user",
               }
         end.to change { Jobs::CriticalUserEmail.jobs.size }.by(1)
@@ -1662,7 +1701,12 @@ RSpec.describe Admin::UsersController do
       end
 
       it "can silence multiple users" do
-        put "/admin/users/#{reg_user.id}/silence.json", params: { other_user_ids: [other_user.id] }
+        put "/admin/users/#{reg_user.id}/silence.json",
+            params: {
+              reason: "because I want to",
+              silenced_till: 14.hours.from_now,
+              other_user_ids: [other_user.id],
+            }
         expect(response.status).to eq(200)
         expect(reg_user.reload).to be_silenced
         expect(other_user.reload).to be_silenced
@@ -1679,13 +1723,38 @@ RSpec.describe Admin::UsersController do
         user.reload
         expect(user).not_to be_suspended
       end
+
+      it "fails the request if other_user_ids is too big" do
+        another_user = Fabricate(:user)
+        other_user_ids = [another_user.id]
+        other_user_ids.push(*(1..304).to_a)
+
+        put "/admin/users/#{user.id}/silence.json",
+            params: {
+              reason: "because I said so",
+              silenced_till: 5.hours.from_now,
+              other_user_ids:,
+            }
+
+        expect(response.status).to eq(400)
+
+        user.reload
+        expect(user).not_to be_silenced
+
+        another_user.reload
+        expect(another_user).not_to be_silenced
+      end
     end
 
     context "when logged in as a moderator" do
       before { sign_in(moderator) }
 
       it "silences user" do
-        put "/admin/users/#{reg_user.id}/silence.json"
+        put "/admin/users/#{reg_user.id}/silence.json",
+            params: {
+              reason: "cuz I wanna",
+              silenced_till: 66.hours.from_now,
+            }
 
         expect(response.status).to eq(200)
         reg_user.reload
@@ -1694,7 +1763,11 @@ RSpec.describe Admin::UsersController do
       end
 
       it "doesn't allow silencing another admin" do
-        put "/admin/users/#{another_admin.id}/silence.json"
+        put "/admin/users/#{another_admin.id}/silence.json",
+            params: {
+              reason: "because reasons",
+              silenced_till: 3.hours.from_now,
+            }
         expect(response.status).to eq(403)
         expect(another_admin.reload).to_not be_silenced
       end
@@ -1703,7 +1776,10 @@ RSpec.describe Admin::UsersController do
         put "/admin/users/#{reg_user.id}/silence.json",
             params: {
               other_user_ids: [another_admin.id],
+              reason: "because reasons",
+              silenced_till: 3.hours.from_now,
             }
+
         expect(response.status).to eq(403)
         expect(another_admin.reload).to_not be_silenced
         expect(reg_user.reload).to_not be_silenced

spec/requests/api/schemas/json/user_silence_request.json

@@ -16,5 +16,9 @@
       "type": "string",
       "example": "delete"
     }
-  }
+  },
+  "required": [
+    "silenced_till",
+    "reason"
+  ]
 }

spec/requests/api/users_spec.rb

@@ -454,7 +454,7 @@ RSpec.describe "users" do
       produces "application/json"
       response "200", "response" do
         let(:id) { Fabricate(:user).id }
-        let(:params) {}
+        let(:params) { { "reason" => "up to me", "silenced_till" => "2301-08-15" } }
 
         expected_response_schema = load_spec_schema("user_silence_response")
         schema(expected_response_schema)