DEV: Refactor suspend/silence user services

- fetch models inside services
- validate `user_id` in contracts
- use policy objects
- extract more logic to actions
- write specs for services and action
This commit is contained in:
Loïc Guitaut
2024-09-06 12:56:56 +02:00
committed by Loïc Guitaut
parent 04645c458d
commit b806dce13d
16 changed files with 590 additions and 233 deletions

View File

@ -119,37 +119,26 @@ class Admin::UsersController < Admin::StaffController
end
def suspend
SuspendUser.call(user: @user) do
User::Suspend.call do
on_success do
render_json_dump(
suspension: {
suspend_reason: result.reason,
full_suspend_reason: result.user_history&.details,
suspended_till: @user.suspended_till,
suspended_at: @user.suspended_at,
full_suspend_reason: result.full_reason,
suspended_till: result.user.suspended_till,
suspended_at: result.user.suspended_at,
suspended_by: BasicUserSerializer.new(current_user, root: false).as_json,
},
)
end
on_failed_policy(:can_suspend) { raise Discourse::InvalidAccess.new }
on_failed_policy(:not_suspended_already) do
suspend_record = @user.suspend_record
message =
I18n.t(
"user.already_suspended",
staff: suspend_record.acting_user.username,
time_ago:
AgeWords.time_ago_in_words(
suspend_record.created_at,
true,
scope: :"datetime.distance_in_words_verbose",
),
)
render json: failed_json.merge(message: message), status: 409
end
on_failed_contract do |contract|
render json: failed_json.merge(errors: contract.errors.full_messages), status: 400
end
on_model_not_found(:user) { raise Discourse::NotFound }
on_failed_policy(:not_suspended_already) do |policy|
render json: failed_json.merge(message: policy.reason), status: 409
end
on_failed_policy(:can_suspend_all_users) { raise Discourse::InvalidAccess.new }
end
end
@ -325,37 +314,26 @@ class Admin::UsersController < Admin::StaffController
end
def silence
SilenceUser.call(user: @user) do
User::Silence.call do
on_success do
render_json_dump(
silence: {
silenced: true,
silence_reason: result.user_history&.details,
silenced_till: @user.silenced_till,
silenced_at: @user.silenced_at,
silence_reason: result.full_reason,
silenced_till: result.user.silenced_till,
silenced_at: result.user.silenced_at,
silenced_by: BasicUserSerializer.new(current_user, root: false).as_json,
},
)
end
on_failed_policy(:can_silence) { raise Discourse::InvalidAccess.new }
on_failed_policy(:not_silenced_already) do
silenced_record = @user.silenced_record
message =
I18n.t(
"user.already_silenced",
staff: silenced_record.acting_user.username,
time_ago:
AgeWords.time_ago_in_words(
silenced_record.created_at,
true,
scope: :"datetime.distance_in_words_verbose",
),
)
render json: failed_json.merge(message: message), status: 409
end
on_failed_contract do |contract|
render json: failed_json.merge(errors: contract.errors.full_messages), status: 400
end
on_model_not_found(:user) { raise Discourse::NotFound }
on_failed_policy(:not_silenced_already) do |policy|
render json: failed_json.merge(message: policy.reason), status: 409
end
on_failed_policy(:can_silence_all_users) { raise Discourse::InvalidAccess.new }
end
end

View File

@ -0,0 +1,23 @@
# frozen_string_literal: true
class User::NotAlreadySilencedPolicy < PolicyBase
delegate :user, to: :context, private: true
delegate :silenced_record, to: :user, private: true
def call
!user.silenced?
end
def reason
I18n.t(
"user.already_silenced",
staff: silenced_record.acting_user.username,
time_ago:
AgeWords.time_ago_in_words(
silenced_record.created_at,
true,
scope: :"datetime.distance_in_words_verbose",
),
)
end
end

View File

@ -0,0 +1,23 @@
# frozen_string_literal: true
class User::NotAlreadySuspendedPolicy < PolicyBase
delegate :user, to: :context, private: true
delegate :suspend_record, to: :user, private: true
def call
!user.suspended?
end
def reason
I18n.t(
"user.already_suspended",
staff: suspend_record.acting_user.username,
time_ago:
AgeWords.time_ago_in_words(
suspend_record.created_at,
true,
scope: :"datetime.distance_in_words_verbose",
),
)
end
end

View File

@ -1,30 +0,0 @@
# 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

View File

@ -0,0 +1,51 @@
# frozen_string_literal: true
module Action
module User
class SilenceAll
attr_reader :users, :actor, :contract
delegate :message, :post_id, :silenced_till, :reason, to: :contract, private: true
def initialize(users:, actor:, contract:)
@users, @actor, @contract = users, actor, contract
end
def self.call(...)
new(...).call
end
def call
silenced_users.first.try(:user_history).try(:details)
end
private
def silenced_users
users.map do |user|
UserSilencer
.new(
user,
actor,
message_body: message,
keep_posts: true,
silenced_till:,
reason:,
post_id:,
)
.tap do |silencer|
next unless silencer.silence
Jobs.enqueue(
:critical_user_email,
type: "account_silenced",
user_id: user.id,
user_history_id: silencer.user_history.id,
)
end
rescue => err
Discourse.warn_exception(err, message: "failed to silence user with ID #{user.id}")
end
end
end
end
end

View File

@ -0,0 +1,40 @@
# frozen_string_literal: true
module Action
module User
class SuspendAll
attr_reader :users, :actor, :contract
delegate :message, :post_id, :suspend_until, :reason, to: :contract, private: true
def initialize(users:, actor:, contract:)
@users, @actor, @contract = users, actor, contract
end
def self.call(...)
new(...).call
end
def call
suspended_users.first.try(:user_history).try(:details)
end
private
def suspended_users
users.map do |user|
UserSuspender.new(
user,
suspended_till: suspend_until,
reason: reason,
by_user: actor,
message: message,
post_id: post_id,
).tap(&:suspend)
rescue => err
Discourse.warn_exception(err, message: "failed to suspend user with ID #{user.id}")
end
end
end
end
end

View File

@ -0,0 +1,48 @@
# frozen_string_literal: true
module Action
module User
class TriggerPostAction
attr_reader :guardian, :post, :contract
delegate :post_action, to: :contract, private: true
delegate :user, to: :guardian, private: true
def initialize(guardian:, post:, contract:)
@guardian, @post, @contract = guardian, post, contract
end
def self.call(...)
new(...).call
end
def call
return if post.blank? || post_action.blank?
send(post_action)
rescue NoMethodError
end
private
def delete
return unless guardian.can_delete_post_or_topic?(post)
PostDestroyer.new(user, post).destroy
end
def delete_replies
return unless guardian.can_delete_post_or_topic?(post)
PostDestroyer.delete_with_replies(user, post)
end
def edit
# Take what the moderator edited in as gospel
PostRevisor.new(post).revise!(
user,
{ raw: contract.post_edit },
skip_validations: true,
skip_revision: true,
)
end
end
end
end

View File

@ -1,82 +0,0 @@
# 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

View File

@ -1,72 +0,0 @@
# 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

View File

@ -0,0 +1,57 @@
# frozen_string_literal: true
class User::Silence
include Service::Base
contract
model :user
policy :not_silenced_already, class_name: User::NotAlreadySilencedPolicy
model :users
policy :can_silence_all_users
step :silence
model :post, optional: true
step :perform_post_action
class Contract
attribute :user_id, :integer
attribute :reason, :string
attribute :message, :string
attribute :silenced_till, :datetime
attribute :other_user_ids, :array
attribute :post_id, :integer
attribute :post_action, :string
attribute :post_edit, :string
validates :user_id, presence: true
validates :reason, presence: true, length: { maximum: 300 }
validates :silenced_till, presence: true
validates :other_user_ids, length: { maximum: User::MAX_SIMILAR_USERS }
validates :post_action, inclusion: { in: %w[delete delete_replies edit] }, allow_blank: true
end
private
def fetch_user(contract:)
User.find_by(id: contract.user_id)
end
def fetch_users(user:, contract:)
[user, *User.where(id: contract.other_user_ids.to_a.uniq).to_a]
end
def can_silence_all_users(guardian:, users:)
users.all? { guardian.can_silence_user?(_1) }
end
def silence(guardian:, users:, contract:)
context[:full_reason] = Action::User::SilenceAll.call(users:, actor: guardian.user, contract:)
end
def fetch_post(contract:)
Post.find_by(id: contract.post_id)
end
def perform_post_action(guardian:, post:, contract:)
Action::User::TriggerPostAction.call(guardian:, post:, contract:)
end
end

View File

@ -0,0 +1,57 @@
# frozen_string_literal: true
class User::Suspend
include Service::Base
contract
model :user
policy :not_suspended_already, class_name: User::NotAlreadySuspendedPolicy
model :users
policy :can_suspend_all_users
step :suspend
model :post, optional: true
step :perform_post_action
class Contract
attribute :user_id, :integer
attribute :reason, :string
attribute :message, :string
attribute :suspend_until, :datetime
attribute :other_user_ids, :array
attribute :post_id, :integer
attribute :post_action, :string
attribute :post_edit, :string
validates :user_id, presence: true
validates :reason, presence: true, length: { maximum: 300 }
validates :suspend_until, presence: true
validates :other_user_ids, length: { maximum: User::MAX_SIMILAR_USERS }
validates :post_action, inclusion: { in: %w[delete delete_replies edit] }, allow_blank: true
end
private
def fetch_user(contract:)
User.find_by(id: contract.user_id)
end
def fetch_users(user:, contract:)
[user, *User.where(id: contract.other_user_ids.to_a.uniq).to_a]
end
def can_suspend_all_users(guardian:, users:)
users.all? { guardian.can_suspend?(_1) }
end
def suspend(guardian:, users:, contract:)
context[:full_reason] = Action::User::SuspendAll.call(users:, actor: guardian.user, contract:)
end
def fetch_post(contract:)
Post.find_by(id: contract.post_id)
end
def perform_post_action(guardian:, post:, contract:)
Action::User::TriggerPostAction.call(guardian:, post:, contract:)
end
end

View File

@ -11,18 +11,10 @@ module ActiveSupportTypeExtensions
when String
value.split(",")
when ::Array
value.map { |item| convert_to_integer(item) }
value.map { |item| Integer(item, exception: false) || item }
else
::Array.wrap(value)
end
end
private
def convert_to_integer(item)
Integer(item)
rescue ArgumentError
item
end
end
end

View File

@ -0,0 +1,86 @@
# frozen_string_literal: true
RSpec.describe Action::User::TriggerPostAction do
describe ".call" do
subject(:action) { described_class.call(guardian:, post:, contract:) }
fab!(:post)
fab!(:admin)
let(:guardian) { admin.guardian }
let(:contract) { User::Suspend::Contract.new(post_action:, post_edit:) }
let(:post_action) { nil }
let(:post_edit) { nil }
context "when post is blank" do
let(:post) { nil }
it "does nothing" do
expect { action }.not_to change { Post.count }
end
end
context "when post_action is blank" do
it "does nothing" do
expect { action }.not_to change { Post.count }
end
end
context "when post and post_action are defined" do
context "when post_action is 'delete'" do
let(:post_action) { "delete" }
context "when user cannot delete a post" do
let(:guardian) { Guardian.new }
it "does nothing" do
expect { action }.not_to change { Post.count }
end
end
context "when user can delete a post" do
it "deletes the provided post" do
expect { action }.to change { Post.where(id: post.id).count }.by(-1)
end
end
end
context "when post_action is 'delete_replies'" do
let(:post_action) { "delete_replies" }
context "when user cannot delete a post" do
let(:guardian) { Guardian.new }
it "does nothing" do
expect { action }.not_to change { Post.count }
end
end
context "when user can delete a post" do
fab!(:reply) do
Fabricate(:reply, topic: post.topic, reply_to_post_number: post.post_number)
end
before { post.replies << reply }
it "deletes the provided post" do
expect { action }.to change { Post.where(id: post.id).count }.by(-1)
end
it "deletes the post's replies" do
expect { action }.to change { Post.where(id: reply.id).count }.by(-1)
end
end
end
context "when post_action is 'edit'" do
let(:post_action) { "edit" }
let(:post_edit) { "blabla" }
it "edits the post with what the moderator wrote" do
expect { action }.to change { post.reload.raw }.to eq("blabla")
end
end
end
end
end

View File

@ -0,0 +1,94 @@
# frozen_string_literal: true
require "rails_helper"
RSpec.describe User::Silence do
describe described_class::Contract, type: :model do
it { is_expected.to validate_presence_of(:user_id) }
it { is_expected.to validate_presence_of(:reason) }
it { is_expected.to validate_presence_of(:silenced_till) }
it { is_expected.to validate_length_of(:reason).is_at_most(300) }
it do
is_expected.to validate_length_of(:other_user_ids).as_array.is_at_most(
User::MAX_SIMILAR_USERS,
)
end
it do
is_expected.to validate_inclusion_of(:post_action).in_array(
%w[delete delete_replies edit],
).allow_blank
end
end
describe ".call" do
subject(:result) { described_class.call(params) }
fab!(:admin)
fab!(:user)
fab!(:other_user) { Fabricate(:user) }
let(:params) { { guardian:, user_id:, reason:, silenced_till:, other_user_ids:, message: } }
let(:guardian) { admin.guardian }
let(:user_id) { user.id }
let(:other_user_ids) { other_user.id }
let(:reason) { "spam" }
let(:message) { "it was spam" }
let(:silenced_till) { 3.hours.from_now.to_s }
context "when invalid data is provided" do
let(:user_id) { nil }
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
before { allow(Action::User::TriggerPostAction).to receive(:call) }
it "silences all provided users" do
result
expect([user, other_user].map(&:reload)).to all be_silenced
end
it "enqueues jobs to send an email" do
expect { result }.to change { Jobs::CriticalUserEmail.jobs.size }.by(2)
end
it "exposes the full reason in the result object" do
expect(result[:full_reason]).to eq("spam\n\nit was spam")
end
it "triggers a post action" do
result
expect(Action::User::TriggerPostAction).to have_received(:call).with(
guardian:,
post: nil,
contract: result[:contract],
)
end
end
end
end
end
end
end

View File

@ -0,0 +1,92 @@
# frozen_string_literal: true
require "rails_helper"
RSpec.describe User::Suspend do
describe described_class::Contract, type: :model do
it { is_expected.to validate_presence_of(:user_id) }
it { is_expected.to validate_presence_of(:reason) }
it { is_expected.to validate_presence_of(:suspend_until) }
it { is_expected.to validate_length_of(:reason).is_at_most(300) }
it do
is_expected.to validate_length_of(:other_user_ids).as_array.is_at_most(
User::MAX_SIMILAR_USERS,
)
end
it do
is_expected.to validate_inclusion_of(:post_action).in_array(
%w[delete delete_replies edit],
).allow_blank
end
end
describe ".call" do
subject(:result) { described_class.call(params) }
fab!(:admin)
fab!(:user)
fab!(:other_user) { Fabricate(:user) }
let(:params) { { guardian:, user_id:, reason:, suspend_until:, other_user_ids:, message: } }
let(:guardian) { admin.guardian }
let(:user_id) { user.id }
let(:other_user_ids) { other_user.id }
let(:reason) { "spam" }
let(:message) { "it was spam" }
let(:suspend_until) { 3.hours.from_now.to_s }
context "when invalid data is provided" do
let(:user_id) { nil }
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
end
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
before { allow(Action::User::TriggerPostAction).to receive(:call) }
it "suspends all provided users" do
result
expect([user, other_user].map(&:reload)).to all be_suspended
end
it "triggers a post action" do
result
expect(Action::User::TriggerPostAction).to have_received(:call).with(
guardian:,
post: nil,
contract: result[:contract],
)
end
it "exposes the full reason in the result object" do
expect(result[:full_reason]).to eq("spam\n\nit was spam")
end
end
end
end
end
end
end