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 }
```

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
+        on_failed_contract do |contract|
-          render(
+          render(json: failed_json.merge(errors: contract.errors.full_messages), status: 400)
-            json:
-              failed_json.merge(errors: result[:"result.contract.default"].errors.full_messages),
-            status: 400,
-          )
         end
       end
     end

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

plugins/chat/app/services/service/base.rb

@@ -84,8 +84,8 @@ module Service
         )
       end
 
-      def policy(name = :default)
+      def policy(name = :default, class_name: nil)
-        steps << PolicyStep.new(name)
+        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)

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

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
+# * +on_model_not_found(name)+: will execute the provided block if the model
-#   fails and its model is not present
+#   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_success: {
-    on_failure: -> { result.failure? },
+      condition: -> { result.success? },
-    on_failed_step: ->(name) { failure_for?("result.step.#{name}") },
+      key: [],
-    on_failed_policy: ->(name = "default") { failure_for?("result.policy.#{name}") },
+    },
-    on_failed_contract: ->(name = "default") { failure_for?("result.contract.#{name}") },
+    on_failure: {
-    on_model_not_found: ->(name = "model") do
+      condition: -> { result.failure? },
-      failure_for?("result.model.#{name}") && result[name].blank?
+      key: [],
-    end,
+    },
-    on_model_errors: ->(name = "model") do
+    on_failed_step: {
-      failure_for?("result.model.#{name}") && result["result.model.#{name}"].invalid
+      condition: ->(name) { failure_for?("result.step.#{name}") },
-    end,
+      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]) },
+      -> { instance_exec(*args, &action[:condition]) },
-      -> { object.instance_eval(&block) },
+      -> do
+        object.instance_exec(
+          result[[*action[:key], args.first || action[:default_name]].join(".")],
+          &block
+        )
+      end,
     ]
   end
 

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") }
 

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
+        context "when not using the block argument" do
-          expect(runner).to eq :policy_failure
+          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
+        context "when not using the block argument" do
-          expect(runner).to eq :contract_failure
+          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
+          context "when not using the block argument" do
-            expect(runner).to eq :no_model
+            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
+        context "when not using the block argument" do
-          expect(runner).to eq :model_errors
+          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