From 0733dda1cba3de52d1fd92e8ac9619978d193ba4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Guitaut?= Date: Wed, 24 May 2023 16:32:49 +0200 Subject: [PATCH] DEV: Add policy objects to services This patch introduces policy objects to chat services. It allows putting more complex logic in a dedicated class, which will make services thinner. It also allows providing a reason why the policy failed. Some change has been made to the service runner too to use more easily these new policy objects: when matching a failing policy (or any failing step actually), the result object is now provided to the block. This way, instead of having to access the reason why the policy failed by doing `result["result.policy.policy_name"].reason` inside the block, this one can be simply written like this: ```ruby on_failed_policy(:policy_name) { |policy| policy.reason } ``` --- .../app/controllers/chat/api_controller.rb | 8 +- plugins/chat/app/policies/policy_base.rb | 19 +++++ plugins/chat/app/services/service/base.rb | 38 ++++++++-- plugins/chat/lib/chat/steps_inspector.rb | 3 + plugins/chat/lib/service_runner.rb | 73 +++++++++++++----- .../spec/lib/chat/steps_inspector_spec.rb | 24 ++++++ plugins/chat/spec/lib/service_runner_spec.rb | 76 ++++++++++++++++--- 7 files changed, 199 insertions(+), 42 deletions(-) create mode 100644 plugins/chat/app/policies/policy_base.rb diff --git a/plugins/chat/app/controllers/chat/api_controller.rb b/plugins/chat/app/controllers/chat/api_controller.rb index b95446c6cd8..57ec914e2cd 100644 --- a/plugins/chat/app/controllers/chat/api_controller.rb +++ b/plugins/chat/app/controllers/chat/api_controller.rb @@ -19,12 +19,8 @@ module Chat on_success { render(json: success_json) } on_failure { render(json: failed_json, status: 422) } on_failed_policy(:invalid_access) { raise Discourse::InvalidAccess } - on_failed_contract do - render( - json: - failed_json.merge(errors: result[:"result.contract.default"].errors.full_messages), - status: 400, - ) + on_failed_contract do |contract| + render(json: failed_json.merge(errors: contract.errors.full_messages), status: 400) end end end diff --git a/plugins/chat/app/policies/policy_base.rb b/plugins/chat/app/policies/policy_base.rb new file mode 100644 index 00000000000..90f79aa106a --- /dev/null +++ b/plugins/chat/app/policies/policy_base.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +class PolicyBase + attr_reader :context + + delegate :guardian, to: :context + + def initialize(context) + @context = context + end + + def call + raise "Not implemented" + end + + def reason + raise "Not implemented" + end +end diff --git a/plugins/chat/app/services/service/base.rb b/plugins/chat/app/services/service/base.rb index 983f2130cc3..48f8c8e822b 100644 --- a/plugins/chat/app/services/service/base.rb +++ b/plugins/chat/app/services/service/base.rb @@ -84,8 +84,8 @@ module Service ) end - def policy(name = :default) - steps << PolicyStep.new(name) + def policy(name = :default, class_name: nil) + steps << PolicyStep.new(name, class_name: class_name) end def step(name) @@ -108,10 +108,11 @@ module Service end def call(instance, context) - method = instance.method(method_name) + object = class_name&.new(context) + method = object&.method(:call) || instance.method(method_name) args = {} args = context.to_h if method.arity.nonzero? - context[result_key] = Context.build + context[result_key] = Context.build(object: object) instance.instance_exec(**args, &method) end @@ -145,7 +146,7 @@ module Service class PolicyStep < Step def call(instance, context) if !super - context[result_key].fail + context[result_key].fail(reason: context[result_key].object&.reason) context.fail! end end @@ -248,12 +249,20 @@ module Service # end # @!scope class - # @!method policy(name = :default) + # @!method policy(name = :default, class_name: nil) # @param name [Symbol] name for this policy + # @param class_name [Class] a policy object (should inherit from +PolicyBase+) # Performs checks related to the state of the system. If the # step doesn’t return a truthy value, then the policy will fail. # - # @example + # When using a policy object, there is no need to define a method on the + # service for the policy step. The policy object `#call` method will be + # called and if the result isn’t truthy, a `#reason` method is expected to + # be implemented to explain the failure. + # + # Policy objects are usually useful for more complex logic. + # + # @example Without a policy object # policy :no_direct_message_channel # # private @@ -261,6 +270,21 @@ module Service # def no_direct_message_channel(channel:, **) # !channel.direct_message_channel? # end + # + # @example With a policy object + # # in the service object + # policy :no_direct_message_channel, class_name: NoDirectMessageChannelPolicy + # + # # in the policy object File + # class NoDirectMessageChannelPolicy < PolicyBase + # def call + # !context.channel.direct_message_channel? + # end + # + # def reason + # "Direct message channels aren’t supported" + # end + # end # @!scope class # @!method contract(name = :default, class_name: self::Contract, default_values_from: nil) diff --git a/plugins/chat/lib/chat/steps_inspector.rb b/plugins/chat/lib/chat/steps_inspector.rb index f8cddfe595a..2cc40a0c1f6 100644 --- a/plugins/chat/lib/chat/steps_inspector.rb +++ b/plugins/chat/lib/chat/steps_inspector.rb @@ -80,6 +80,9 @@ module Chat # @!visibility private class Policy < Step + def error + step_result.reason + end end # @!visibility private diff --git a/plugins/chat/lib/service_runner.rb b/plugins/chat/lib/service_runner.rb index e362c743bcd..78ecff46998 100644 --- a/plugins/chat/lib/service_runner.rb +++ b/plugins/chat/lib/service_runner.rb @@ -12,12 +12,20 @@ # # * +on_success+: will execute the provided block if the service succeeds # * +on_failure+: will execute the provided block if the service fails +# * +on_failed_step(name)+: will execute the provided block if the step named +# `name` fails # * +on_failed_policy(name)+: will execute the provided block if the policy # named `name` fails # * +on_failed_contract(name)+: will execute the provided block if the contract # named `name` fails -# * +on_model_not_found(name)+: will execute the provided block if the service -# fails and its model is not present +# * +on_model_not_found(name)+: will execute the provided block if the model +# named `name` is not present +# * +on_model_errors(name)+: will execute the provided block if the model named +# `name` contains validation errors +# +# All the specialized steps receive the failing step result object as an +# argument to their block. `on_model_errors` receives the actual model so it’s +# easier to inspect it. # # Default actions for each of these are defined in [Chat::ApiController#default_actions_for_service] # @@ -28,7 +36,7 @@ # flash[:notice] = "Success!" # redirect_to a_path # end -# on_failed_policy(:a_named_policy) { redirect_to root_path } +# on_failed_policy(:a_named_policy) { |policy| redirect_to root_path, alert: policy.reason } # on_failure { render :new } # end # end @@ -49,21 +57,42 @@ # class ServiceRunner - # @!visibility private - NULL_RESULT = OpenStruct.new(failure?: false) # @!visibility private AVAILABLE_ACTIONS = { - on_success: -> { result.success? }, - on_failure: -> { result.failure? }, - on_failed_step: ->(name) { failure_for?("result.step.#{name}") }, - on_failed_policy: ->(name = "default") { failure_for?("result.policy.#{name}") }, - on_failed_contract: ->(name = "default") { failure_for?("result.contract.#{name}") }, - on_model_not_found: ->(name = "model") do - failure_for?("result.model.#{name}") && result[name].blank? - end, - on_model_errors: ->(name = "model") do - failure_for?("result.model.#{name}") && result["result.model.#{name}"].invalid - end, + on_success: { + condition: -> { result.success? }, + key: [], + }, + on_failure: { + condition: -> { result.failure? }, + key: [], + }, + on_failed_step: { + condition: ->(name) { failure_for?("result.step.#{name}") }, + key: %w[result step], + }, + on_failed_policy: { + condition: ->(name = "default") { failure_for?("result.policy.#{name}") }, + key: %w[result policy], + default_name: "default", + }, + on_failed_contract: { + condition: ->(name = "default") { failure_for?("result.contract.#{name}") }, + key: %w[result contract], + default_name: "default", + }, + on_model_not_found: { + condition: ->(name = "model") { failure_for?("result.model.#{name}") && result[name].blank? }, + key: %w[result model], + default_name: "model", + }, + on_model_errors: { + condition: ->(name = "model") do + failure_for?("result.model.#{name}") && result["result.model.#{name}"].invalid + end, + key: [], + default_name: "model", + }, }.with_indifferent_access.freeze # @!visibility private @@ -104,13 +133,19 @@ class ServiceRunner attr_reader :actions def failure_for?(key) - (object.result[key] || NULL_RESULT).failure? + object.result[key]&.failure? end def add_action(name, *args, &block) + action = AVAILABLE_ACTIONS[name] actions[[name, *args].join("_").to_sym] = [ - -> { instance_exec(*args, &AVAILABLE_ACTIONS[name]) }, - -> { object.instance_eval(&block) }, + -> { instance_exec(*args, &action[:condition]) }, + -> do + object.instance_exec( + result[[*action[:key], args.first || action[:default_name]].join(".")], + &block + ) + end, ] end diff --git a/plugins/chat/spec/lib/chat/steps_inspector_spec.rb b/plugins/chat/spec/lib/chat/steps_inspector_spec.rb index 6ebba92001b..51b956d9cb6 100644 --- a/plugins/chat/spec/lib/chat/steps_inspector_spec.rb +++ b/plugins/chat/spec/lib/chat/steps_inspector_spec.rb @@ -221,6 +221,30 @@ RSpec.describe Chat::StepsInspector do end end + context "when the policy step is failing" do + before do + class DummyService + def policy + false + end + end + end + + context "when there is no reason provided" do + it "returns nothing" do + expect(error).to be_blank + end + end + + context "when a reason is provided" do + before { result["result.policy.policy"].reason = "failed" } + + it "returns the reason" do + expect(error).to eq "failed" + end + end + end + context "when a common step is failing" do before { result["result.step.final_step"].fail(error: "my error") } diff --git a/plugins/chat/spec/lib/service_runner_spec.rb b/plugins/chat/spec/lib/service_runner_spec.rb index bd9a8973f03..bbeb4df6b94 100644 --- a/plugins/chat/spec/lib/service_runner_spec.rb +++ b/plugins/chat/spec/lib/service_runner_spec.rb @@ -210,8 +210,22 @@ RSpec.describe ServiceRunner do context "when the service policy fails" do let(:service) { FailedPolicyService } - it "runs the provided block" do - expect(runner).to eq :policy_failure + context "when not using the block argument" do + it "runs the provided block" do + expect(runner).to eq :policy_failure + end + end + + context "when using the block argument" do + let(:actions) { <<-BLOCK } + proc do + on_failed_policy(:test) { |policy| policy == result["result.policy.test"] } + end + BLOCK + + it "runs the provided block" do + expect(runner).to be true + end end end @@ -234,8 +248,22 @@ RSpec.describe ServiceRunner do context "when the service contract fails" do let(:service) { FailedContractService } - it "runs the provided block" do - expect(runner).to eq :contract_failure + context "when not using the block argument" do + it "runs the provided block" do + expect(runner).to eq :contract_failure + end + end + + context "when using the block argument" do + let(:actions) { <<-BLOCK } + proc do + on_failed_contract { |contract| contract == result["result.contract.default"] } + end + BLOCK + + it "runs the provided block" do + expect(runner).to be true + end end end @@ -250,7 +278,7 @@ RSpec.describe ServiceRunner do context "when using the on_model_not_found action" do let(:actions) { <<-BLOCK } - ->(*) do + proc do on_model_not_found(:fake_model) { :no_model } end BLOCK @@ -259,8 +287,22 @@ RSpec.describe ServiceRunner do context "when the service fails without a model" do let(:service) { FailureWithModelService } - it "runs the provided block" do - expect(runner).to eq :no_model + context "when not using the block argument" do + it "runs the provided block" do + expect(runner).to eq :no_model + end + end + + context "when using the block argument" do + let(:actions) { <<-BLOCK } + proc do + on_model_not_found(:fake_model) { |model| model == result["result.model.fake_model"] } + end + BLOCK + + it "runs the provided block" do + expect(runner).to be true + end end end @@ -294,7 +336,7 @@ RSpec.describe ServiceRunner do context "when using the on_model_errors action" do let(:actions) { <<-BLOCK } - ->(*) do + proc do on_model_errors(:fake_model) { :model_errors } end BLOCK @@ -302,8 +344,22 @@ RSpec.describe ServiceRunner do context "when the service fails with a model containing errors" do let(:service) { FailureWithModelErrorsService } - it "runs the provided block" do - expect(runner).to eq :model_errors + context "when not using the block argument" do + it "runs the provided block" do + expect(runner).to eq :model_errors + end + end + + context "when using the block argument" do + let(:actions) { <<-BLOCK } + proc do + on_model_errors(:fake_model) { |model| model == OpenStruct.new(invalid?: true) } + end + BLOCK + + it "runs the provided block" do + expect(runner).to be true + end end end