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