Discource-C
/
discourse
mirror of https://github.com/discourse/discourse.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

DEV: Refactor flag related services a bit 

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

This patch makes the code more compliant with the upcoming service docs
best practices.
This commit is contained in:
Loïc Guitaut 2024-10-17 17:15:35 +02:00 committed by Loïc Guitaut
parent f8360f9665
commit 7f607699b8
10 changed files with 283 additions and 308 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

25
app/services/flags/create_flag.rb
View File

@ -3,6 +3,7 @@
class Flags::CreateFlag
include Service::Base
policy :invalid_access
contract do
attribute :name, :string
attribute :description, :string
@ -16,7 +17,6 @@ class Flags::CreateFlag
validates :description, length: { maximum: Flag::MAX_DESCRIPTION_LENGTH }
validates :applies_to, inclusion: { in: -> { Flag.valid_applies_to_types } }, allow_nil: false
end
policy :invalid_access
policy :unique_name
model :flag, :instantiate_flag
transaction do
@ -26,25 +26,18 @@ class Flags::CreateFlag
private
def unique_name(name:)
!Flag.custom.where(name: name).exists?
end
def instantiate_flag(name:, description:, applies_to:, require_message:, enabled:)
Flag.new(
name: name,
description: description,
applies_to: applies_to,
require_message: require_message,
enabled: enabled,
notify_type: true,
)
end
def invalid_access(guardian:)
guardian.can_create_flag?
end
def unique_name(contract:)
!Flag.custom.where(name: contract.name).exists?
end
def instantiate_flag(contract:)
Flag.new(contract.attributes.merge(notify_type: true))
end
def create(flag:)
flag.save!
end

10
app/services/flags/destroy_flag.rb
View File

@ -7,7 +7,6 @@ class Flags::DestroyFlag
policy :not_system
policy :not_used
policy :invalid_access
transaction do
step :destroy
step :log
@ -16,7 +15,7 @@ class Flags::DestroyFlag
private
def fetch_flag(id:)
Flag.find(id)
Flag.find_by(id: id)
end
def not_system(flag:)
@ -38,12 +37,7 @@ class Flags::DestroyFlag
def log(guardian:, flag:)
StaffActionLogger.new(guardian.user).log_custom(
"delete_flag",
{
name: flag.name,
description: flag.description,
applies_to: flag.applies_to,
enabled: flag.enabled,
},
flag.slice(:name, :description, :applies_to, :enabled),
)
end
end

27
app/services/flags/reorder_flag.rb
View File

@ -1,7 +1,5 @@
# frozen_string_literal: true
VALID_DIRECTIONS = %w[up down]
class Flags::ReorderFlag
include Service::Base
@ -10,10 +8,11 @@ class Flags::ReorderFlag
attribute :direction, :string
validates :flag_id, presence: true
validates :direction, inclusion: { in: VALID_DIRECTIONS }
validates :direction, inclusion: { in: %w[up down] }
end
model :flag
policy :invalid_access
model :all_flags
policy :invalid_move
transaction do
step :move
@ -22,37 +21,37 @@ class Flags::ReorderFlag
private
def fetch_flag(flag_id:)
Flag.find(flag_id)
def fetch_flag(contract:)
Flag.find_by(id: contract.flag_id)
end
def invalid_access(guardian:, flag:)
guardian.can_reorder_flag?(flag)
end
def all_flags
@all_flags ||= Flag.where.not(name_key: "notify_user").order(:position)
def fetch_all_flags
Flag.where.not(name_key: "notify_user").order(:position).to_a
end
def invalid_move(flag:, direction:)
return false if all_flags.first == flag && direction == "up"
return false if all_flags.last == flag && direction == "down"
def invalid_move(flag:, contract:, all_flags:)
return false if all_flags.first == flag && contract.direction == "up"
return false if all_flags.last == flag && contract.direction == "down"
true
end
def move(flag:, direction:)
def move(flag:, contract:, all_flags:)
old_position = flag.position
index = all_flags.index(flag)
target_flag = all_flags[direction == "up" ? index - 1 : index + 1]
target_flag = all_flags[contract.direction == "up" ? index - 1 : index + 1]
flag.update!(position: target_flag.position)
target_flag.update!(position: old_position)
end
def log(guardian:, flag:, direction:)
def log(guardian:, flag:, contract:)
StaffActionLogger.new(guardian.user).log_custom(
"move_flag",
{ flag: flag.name, direction: direction },
{ flag: flag.name, direction: contract.direction },
)
end
end

10
app/services/flags/toggle_flag.rb
View File

@ -3,13 +3,13 @@
class Flags::ToggleFlag
include Service::Base
policy :invalid_access
contract do
attribute :flag_id, :integer
validates :flag_id, presence: true
end
model :flag
policy :invalid_access
transaction do
step :toggle
step :log
@ -17,14 +17,14 @@ class Flags::ToggleFlag
private
def fetch_flag(flag_id:)
Flag.find(flag_id)
end
def invalid_access(guardian:)
guardian.can_toggle_flag?
end
def fetch_flag(contract:)
Flag.find_by(id: contract.flag_id)
end
def toggle(flag:)
flag.update!(enabled: !flag.enabled)
end

32
app/services/flags/update_flag.rb
View File

@ -4,12 +4,14 @@ class Flags::UpdateFlag
include Service::Base
contract do
attribute :id, :integer
attribute :name, :string
attribute :description, :string
attribute :require_message, :boolean
attribute :enabled, :boolean
attribute :applies_to
validates :id, presence: true
validates :name, presence: true
validates :description, presence: true
validates :name, length: { maximum: Flag::MAX_NAME_LENGTH }
@ -28,12 +30,8 @@ class Flags::UpdateFlag
private
def unique_name(id:, name:)
!Flag.custom.where(name: name).where.not(id: id).exists?
end
def fetch_flag(id:)
Flag.find(id)
def fetch_flag(contract:)
Flag.find_by(id: contract.id)
end
def not_system(flag:)
@ -48,26 +46,18 @@ class Flags::UpdateFlag
guardian.can_edit_flag?(flag)
end
def update(flag:, name:, description:, applies_to:, require_message:, enabled:)
flag.update!(
name: name,
description: description,
applies_to: applies_to,
require_message: require_message,
enabled: enabled,
)
def unique_name(contract:)
!Flag.custom.where(name: contract.name).where.not(id: contract.id).exists?
end
def update(flag:, contract:)
flag.update!(contract.attributes)
end
def log(guardian:, flag:)
StaffActionLogger.new(guardian.user).log_custom(
"update_flag",
{
name: flag.name,
description: flag.description,
applies_to: flag.applies_to,
require_message: flag.require_message,
enabled: flag.enabled,
},
flag.slice(:name, :description, :applies_to, :require_message, :enabled),
)
end
end

147
spec/services/flags/create_flag_spec.rb
View File

@ -1,105 +1,78 @@
# frozen_string_literal: true
RSpec.describe(Flags::CreateFlag) do
subject(:result) do
described_class.call(
guardian: current_user.guardian,
name: name,
description: description,
applies_to: applies_to,
require_message: require_message,
enabled: enabled,
)
describe described_class::Contract, type: :model do
it { is_expected.to validate_presence_of(:name) }
it { is_expected.to validate_presence_of(:description) }
it { is_expected.to validate_length_of(:name).is_at_most(Flag::MAX_NAME_LENGTH) }
it { is_expected.to validate_length_of(:description).is_at_most(Flag::MAX_DESCRIPTION_LENGTH) }
it { is_expected.to validate_inclusion_of(:applies_to).in_array(Flag.valid_applies_to_types) }
end
let(:name) { "custom flag name" }
let(:description) { "custom flag description" }
let(:applies_to) { ["Topic"] }
let(:enabled) { true }
let(:require_message) { true }
describe ".call" do
subject(:result) { described_class.call(**params, **dependencies) }
context "when user is not allowed to perform the action" do
fab!(:current_user) { Fabricate(:user) }
it { is_expected.to fail_a_policy(:invalid_access) }
end
context "when title is not unique" do
fab!(:current_user) { Fabricate(:admin) }
fab!(:flag) { Fabricate(:flag, name: "custom flag name") }
it { is_expected.to fail_a_policy(:unique_name) }
let(:params) { { name:, description:, applies_to:, require_message:, enabled: } }
let(:dependencies) { { guardian: current_user.guardian } }
let(:name) { "custom flag name" }
let(:description) { "custom flag description" }
let(:applies_to) { ["Topic"] }
let(:enabled) { true }
let(:require_message) { true }
after { Flag.destroy_by(name: "custom flag name") }
end
context "when user is not allowed to perform the action" do
fab!(:current_user) { Fabricate(:user) }
context "when applies to is invalid" do
fab!(:current_user) { Fabricate(:admin) }
let(:applies_to) { ["User"] }
it { is_expected.to fail_a_contract }
end
context "when title is empty" do
fab!(:current_user) { Fabricate(:admin) }
let(:name) { nil }
it { is_expected.to fail_a_contract }
end
context "when title is too long" do
fab!(:current_user) { Fabricate(:admin) }
let(:name) { "a" * 201 }
it { is_expected.to fail_a_contract }
end
context "when description is empty" do
fab!(:current_user) { Fabricate(:admin) }
let(:description) { nil }
it { is_expected.to fail_a_contract }
end
context "when description is too long" do
fab!(:current_user) { Fabricate(:admin) }
let(:description) { "a" * 1001 }
it { is_expected.to fail_a_contract }
end
context "when user is allowed to perform the action" do
fab!(:current_user) { Fabricate(:admin) }
let(:applies_to) { ["Topic::Custom"] }
before do
DiscoursePluginRegistry.register_flag_applies_to_type(
"Topic::Custom",
OpenStruct.new(enabled?: true),
)
it { is_expected.to fail_a_policy(:invalid_access) }
end
after { Flag.destroy_by(name: "custom flag name") }
context "when contract is invalid" do
let(:name) { nil }
it { is_expected.to run_successfully }
it "creates the flag" do
result
flag = Flag.last
expect(flag.name).to eq("custom flag name")
expect(flag.description).to eq("custom flag description")
expect(flag.applies_to).to eq(["Topic::Custom"])
expect(flag.require_message).to be true
expect(flag.enabled).to be true
it { is_expected.to fail_a_contract }
end
it "logs the action" do
expect { result }.to change { UserHistory.count }.by(1)
expect(UserHistory.last).to have_attributes(
custom_type: "create_flag",
details:
"name: custom flag name\ndescription: custom flag description\napplies_to: [\"Topic::Custom\"]\nrequire_message: true\nenabled: true",
)
context "when name is not unique" do
let!(:flag) { Fabricate(:flag, name:) }
it { is_expected.to fail_a_policy(:unique_name) }
end
context "when everything's ok" do
let(:applies_to) { ["Topic::Custom"] }
let(:flag) { Flag.last }
before do
DiscoursePluginRegistry.register_flag_applies_to_type(
"Topic::Custom",
OpenStruct.new(enabled?: true),
)
end
it { is_expected.to run_successfully }
it "creates the flag" do
expect { result }.to change { Flag.count }.by(1)
expect(flag).to have_attributes(
name: "custom flag name",
description: "custom flag description",
applies_to: ["Topic::Custom"],
require_message: true,
enabled: true,
notify_type: true,
)
end
it "logs the action" do
expect { result }.to change { UserHistory.count }.by(1)
expect(UserHistory.last).to have_attributes(
custom_type: "create_flag",
details:
"name: custom flag name\ndescription: custom flag description\napplies_to: [\"Topic::Custom\"]\nrequire_message: true\nenabled: true",
)
end
end
end
end

36
spec/services/flags/destroy_flag_spec.rb
View File

@ -1,11 +1,32 @@
# frozen_string_literal: true
RSpec.describe(Flags::DestroyFlag) do
subject(:result) { described_class.call(id: flag.id, guardian: current_user.guardian) }
subject(:result) { described_class.call(**params, **dependencies) }
fab!(:flag)
fab!(:current_user) { Fabricate(:admin) }
after { flag.destroy }
let(:params) { { id: flag_id } }
let(:dependencies) { { guardian: current_user.guardian } }
let(:flag_id) { flag.id }
context "when model is not found" do
let(:flag_id) { 0 }
it { is_expected.to fail_to_find_a_model(:flag) }
end
context "when the flag is a system one" do
let(:flag) { Flag.first }
it { is_expected.to fail_a_policy(:not_system) }
end
context "when the flag has been used" do
let!(:post_action) { Fabricate(:post_action, post_action_type_id: flag.id) }
it { is_expected.to fail_a_policy(:not_used) }
end
context "when user is not allowed to perform the action" do
fab!(:current_user) { Fabricate(:user) }
@ -13,18 +34,11 @@ RSpec.describe(Flags::DestroyFlag) do
it { is_expected.to fail_a_policy(:invalid_access) }
end
context "when user is allowed to perform the action" do
fab!(:current_user) { Fabricate(:admin) }
context "when everything's ok" do
it { is_expected.to run_successfully }
it "sets the service result as successful" do
expect(result).to be_a_success
end
it "destroys the flag" do
result
expect { flag.reload }.to raise_error(ActiveRecord::RecordNotFound)
expect { result }.to change { Flag.where(id: flag).count }.by(-1)
end
it "logs the action" do

88
spec/services/flags/reorder_flag_spec.rb
View File

@ -1,58 +1,62 @@
# frozen_string_literal: true
RSpec.describe(Flags::ReorderFlag) do
subject(:result) do
described_class.call(flag_id: flag.id, guardian: current_user.guardian, direction: direction)
describe described_class::Contract, type: :model do
it { is_expected.to validate_presence_of(:flag_id) }
it { is_expected.to validate_inclusion_of(:direction).in_array(%w[up down]) }
end
let(:flag) { Flag.order(:position).last }
let(:direction) { "up" }
describe ".call" do
subject(:result) { described_class.call(**params, **dependencies) }
context "when user is not allowed to perform the action" do
fab!(:current_user) { Fabricate(:user) }
it { is_expected.to fail_a_policy(:invalid_access) }
end
context "when direction is invalid" do
fab!(:current_user) { Fabricate(:admin) }
let(:direction) { "side" }
it { is_expected.to fail_a_contract }
end
context "when move is invalid" do
fab!(:current_user) { Fabricate(:admin) }
let(:direction) { "down" }
it { is_expected.to fail_a_policy(:invalid_move) }
end
context "when user is allowed to perform the action" do
fab!(:current_user) { Fabricate(:admin) }
after do
described_class.call(flag_id: flag.id, guardian: current_user.guardian, direction: "down")
let(:params) { { flag_id: flag_id, direction: } }
let(:dependencies) { { guardian: current_user.guardian } }
let(:flag_id) { flag.id }
let(:flag) { Flag.order(:position).last }
let(:direction) { "up" }
context "when contract is invalid" do
let(:direction) { "left" }
it { is_expected.to fail_a_contract }
end
it { is_expected.to run_successfully }
context "when model is not found" do
let(:flag_id) { 0 }
it "moves the flag" do
expect(Flag.order(:position).map(&:name)).to eq(
%w[notify_user off_topic inappropriate spam illegal notify_moderators],
)
result
expect(Flag.order(:position).map(&:name)).to eq(
%w[notify_user off_topic inappropriate spam notify_moderators illegal],
)
it { is_expected.to fail_to_find_a_model(:flag) }
end
it "logs the action" do
expect { result }.to change { UserHistory.count }.by(1)
expect(UserHistory.last).to have_attributes(
custom_type: "move_flag",
details: "flag: #{result[:flag].name}\ndirection: up",
)
context "when user is not allowed to perform the action" do
fab!(:current_user) { Fabricate(:user) }
it { is_expected.to fail_a_policy(:invalid_access) }
end
context "when move is invalid" do
let(:direction) { "down" }
it { is_expected.to fail_a_policy(:invalid_move) }
end
context "when everything's ok" do
it { is_expected.to run_successfully }
it "moves the flag" do
expect { result }.to change { Flag.order(:position).map(&:name) }.from(
%w[notify_user off_topic inappropriate spam illegal notify_moderators],
).to(%w[notify_user off_topic inappropriate spam notify_moderators illegal])
end
it "logs the action" do
expect { result }.to change { UserHistory.count }.by(1)
expect(UserHistory.last).to have_attributes(
custom_type: "move_flag",
details: "flag: #{result[:flag].name}\ndirection: up",
)
end
end
end
end

57
spec/services/flags/toggle_flag_spec.rb
View File

@ -1,33 +1,52 @@
# frozen_string_literal: true
RSpec.describe(Flags::ToggleFlag) do
subject(:result) { described_class.call(flag_id: flag.id, guardian: current_user.guardian) }
let(:flag) { Flag.system.last }
context "when user is not allowed to perform the action" do
fab!(:current_user) { Fabricate(:user) }
it { is_expected.to fail_a_policy(:invalid_access) }
describe described_class::Contract, type: :model do
it { is_expected.to validate_presence_of(:flag_id) }
end
context "when user is allowed to perform the action" do
describe ".call" do
subject(:result) { described_class.call(**params, **dependencies) }
fab!(:flag)
fab!(:current_user) { Fabricate(:admin) }
after { flag.reload.update!(enabled: true) }
let(:flag_id) { flag.id }
let(:params) { { flag_id: flag_id } }
let(:dependencies) { { guardian: current_user.guardian } }
it { is_expected.to run_successfully }
context "when user is not allowed to perform the action" do
fab!(:current_user) { Fabricate(:user) }
it "toggles the flag" do
expect(result[:flag].enabled).to be false
it { is_expected.to fail_a_policy(:invalid_access) }
end
it "logs the action" do
expect { result }.to change { UserHistory.count }.by(1)
expect(UserHistory.last).to have_attributes(
custom_type: "toggle_flag",
details: "flag: #{result[:flag].name}\nenabled: #{result[:flag].enabled}",
)
context "when contract is invalid" do
let(:flag_id) { nil }
it { is_expected.to fail_a_contract }
end
context "when model is not found" do
let(:flag_id) { 0 }
it { is_expected.to fail_to_find_a_model(:flag) }
end
context "when everything's ok" do
it { is_expected.to run_successfully }
it "toggles the flag" do
expect(result[:flag].enabled).to be false
end
it "logs the action" do
expect { result }.to change { UserHistory.count }.by(1)
expect(UserHistory.last).to have_attributes(
custom_type: "toggle_flag",
details: "flag: #{result[:flag].name}\nenabled: #{result[:flag].enabled}",
)
end
end
end
end

159
spec/services/flags/update_flag_spec.rb
View File

@ -1,99 +1,88 @@
# frozen_string_literal: true
RSpec.describe(Flags::UpdateFlag) do
subject(:result) do
described_class.call(
id: flag.id,
guardian: current_user.guardian,
name: name,
description: description,
applies_to: applies_to,
require_message: require_message,
enabled: enabled,
)
describe described_class::Contract, type: :model do
it { is_expected.to validate_presence_of(:id) }
it { is_expected.to validate_presence_of(:name) }
it { is_expected.to validate_presence_of(:description) }
it { is_expected.to validate_length_of(:name).is_at_most(Flag::MAX_NAME_LENGTH) }
it { is_expected.to validate_length_of(:description).is_at_most(Flag::MAX_DESCRIPTION_LENGTH) }
it { is_expected.to validate_inclusion_of(:applies_to).in_array(Flag.valid_applies_to_types) }
end
fab!(:flag)
describe ".call" do
subject(:result) { described_class.call(**params, **dependencies) }
after { flag.destroy }
let(:name) { "edited custom flag name" }
let(:description) { "edited custom flag description" }
let(:applies_to) { ["Topic"] }
let(:require_message) { true }
let(:enabled) { false }
context "when user is not allowed to perform the action" do
fab!(:current_user) { Fabricate(:user) }
it { is_expected.to fail_a_policy(:invalid_access) }
end
context "when applies to is invalid" do
fab!(:current_user) { Fabricate(:admin) }
let(:applies_to) { ["User"] }
it { is_expected.to fail_a_contract }
end
context "when title is empty" do
fab!(:current_user) { Fabricate(:admin) }
let(:name) { nil }
it { is_expected.to fail_a_contract }
end
context "when title is not unique" do
fab!(:current_user) { Fabricate(:admin) }
fab!(:flag_2) { Fabricate(:flag, name: "edited custom flag name") }
it { is_expected.to fail_a_policy(:unique_name) }
after { Flag.destroy_by(name: "edited custom flag name") }
end
context "when title is too long" do
fab!(:current_user) { Fabricate(:admin) }
let(:name) { "a" * 201 }
it { is_expected.to fail_a_contract }
end
context "when description is empty" do
fab!(:current_user) { Fabricate(:admin) }
let(:description) { nil }
it { is_expected.to fail_a_contract }
end
context "when description is too long" do
fab!(:current_user) { Fabricate(:admin) }
let(:description) { "a" * 1001 }
it { is_expected.to fail_a_contract }
end
context "when user is allowed to perform the action" do
fab!(:flag)
fab!(:current_user) { Fabricate(:admin) }
it { is_expected.to run_successfully }
let(:params) { { id: flag_id, name:, description:, applies_to:, require_message:, enabled: } }
let(:dependencies) { { guardian: current_user.guardian } }
let(:flag_id) { flag.id }
let(:name) { "edited custom flag name" }
let(:description) { "edited custom flag description" }
let(:applies_to) { ["Topic"] }
let(:require_message) { true }
let(:enabled) { false }
it "updates the flag" do
result
expect(flag.reload.name).to eq("edited custom flag name")
expect(flag.description).to eq("edited custom flag description")
expect(flag.applies_to).to eq(["Topic"])
expect(flag.require_message).to be true
expect(flag.enabled).to be false
context "when contract is invalid" do
let(:name) { nil }
it { is_expected.to fail_a_contract }
end
it "logs the action" do
expect { result }.to change { UserHistory.count }.by(1)
expect(UserHistory.last).to have_attributes(
custom_type: "update_flag",
details:
"name: edited custom flag name\ndescription: edited custom flag description\napplies_to: [\"Topic\"]\nrequire_message: true\nenabled: false",
)
context "when model is not found" do
let(:flag_id) { 0 }
it { is_expected.to fail_to_find_a_model(:flag) }
end
context "when the flag is a system one" do
let(:flag) { Flag.first }
it { is_expected.to fail_a_policy(:not_system) }
end
context "when the flag has been used" do
let!(:post_action) { Fabricate(:post_action, post_action_type_id: flag.id) }
it { is_expected.to fail_a_policy(:not_used) }
end
context "when user is not allowed to perform the action" do
fab!(:current_user) { Fabricate(:user) }
it { is_expected.to fail_a_policy(:invalid_access) }
end
context "when title is not unique" do
let!(:flag_2) { Fabricate(:flag, name:) }
it { is_expected.to fail_a_policy(:unique_name) }
end
context "when everything's ok" do
it { is_expected.to run_successfully }
it "updates the flag" do
result
expect(flag.reload).to have_attributes(
name: "edited custom flag name",
description: "edited custom flag description",
applies_to: ["Topic"],
require_message: true,
enabled: false,
)
end
it "logs the action" do
expect { result }.to change { UserHistory.count }.by(1)
expect(UserHistory.last).to have_attributes(
custom_type: "update_flag",
details:
"name: edited custom flag name\ndescription: edited custom flag description\napplies_to: [\"Topic\"]\nrequire_message: true\nenabled: false",
)
end
end
end
end