From 9e9abe0a821de7fc2fb10570c146d56244de6486 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Lo=C3=AFc=20Guitaut?= <loic@discourse.org>
Date: Wed, 11 Dec 2024 17:58:17 +0100
Subject: [PATCH] DEV: Unify params access in services
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Currently, there are two ways (kind of) for accessing `params` inside a
service:
- when there is no contract or it hasn’t been reached yet, `params` is
  just the hash that was provided to the service. To access a key, you
  have to use the bracket notation `params[:my_key]`.
- when there is a contract and it has been executed successfully,
  `params` now references the contract and the attributes are accessible
  using methods (`params.my_key`).

This patch unifies how `params` exposes its attributes. Now, even if
there is no contract at all in a service, `params` will expose its
attributes through methods, that way things are more consistent.

This patch also makes sure there is always a `params` object available
even when no `params` key is provided to the service (this allows a
contract to fail because its attributes are blank instead of having the
service raising an error because it doesn’t find `params` in its context).
---
 lib/service/base.rb                           | 22 +++++++++++-
 .../chat/app/services/chat/create_message.rb  |  2 +-
 .../chat/app/services/chat/update_channel.rb  |  2 +-
 spec/lib/service_spec.rb                      | 34 +++++++++++++++++++
 4 files changed, 57 insertions(+), 3 deletions(-)

diff --git a/lib/service/base.rb b/lib/service/base.rb
index 13a59d68e08..bde43a0f9aa 100644
--- a/lib/service/base.rb
+++ b/lib/service/base.rb
@@ -441,8 +441,12 @@ module Service
     def initialize(initial_context = {})
       @context =
         Context.build(
-          initial_context.merge(__steps__: self.class.steps, __service_class__: self.class),
+          initial_context
+            .compact
+            .reverse_merge(params: {})
+            .merge(__steps__: self.class.steps, __service_class__: self.class),
         )
+      initialize_params
     end
 
     # @!visibility private
@@ -463,5 +467,21 @@ module Service
       context["result.step.#{step_name}"].fail(error: message)
       context.fail!
     end
+
+    private
+
+    def initialize_params
+      klass =
+        Data.define(*context[:params].keys) do
+          alias to_hash to_h
+
+          delegate :slice, :merge, to: :to_h
+
+          def method_missing(*)
+            nil
+          end
+        end
+      context[:params] = klass.new(*context[:params].values)
+    end
   end
 end
diff --git a/plugins/chat/app/services/chat/create_message.rb b/plugins/chat/app/services/chat/create_message.rb
index 2d4c2aad2a4..449e814d1a2 100644
--- a/plugins/chat/app/services/chat/create_message.rb
+++ b/plugins/chat/app/services/chat/create_message.rb
@@ -92,7 +92,7 @@ module Chat
     private
 
     def accept_blocks(guardian:, params:)
-      params[:blocks] ? guardian.user.bot? : true
+      params.blocks ? guardian.user.bot? : true
     end
 
     def no_silenced_user(guardian:)
diff --git a/plugins/chat/app/services/chat/update_channel.rb b/plugins/chat/app/services/chat/update_channel.rb
index e50d04652fd..fbcadd5bbb3 100644
--- a/plugins/chat/app/services/chat/update_channel.rb
+++ b/plugins/chat/app/services/chat/update_channel.rb
@@ -61,7 +61,7 @@ module Chat
     private
 
     def fetch_channel(params:)
-      Chat::Channel.find_by(id: params[:channel_id])
+      Chat::Channel.find_by(id: params.channel_id)
     end
 
     def check_channel_permission(guardian:, channel:)
diff --git a/spec/lib/service_spec.rb b/spec/lib/service_spec.rb
index 94746999223..18575c8876a 100644
--- a/spec/lib/service_spec.rb
+++ b/spec/lib/service_spec.rb
@@ -58,4 +58,38 @@ RSpec.describe Service do
       end
     end
   end
+
+  describe "Parameters handling" do
+    subject(:result) { service_class.call(**args) }
+
+    context "when calling the service without any params" do
+      let(:args) { {} }
+
+      it "instantiate a default params object" do
+        expect(result[:params]).not_to be_nil
+      end
+    end
+
+    context "when calling the service with params" do
+      let(:args) { { params: { param1: "one" } } }
+
+      context "when there is no `params` step defined" do
+        it "allows accessing `params` through methods" do
+          expect(result[:params].param1).to eq("one")
+        end
+
+        it "returns nothing for a non-existent key" do
+          expect(result[:params].non_existent_key).to be_nil
+        end
+      end
+
+      context "when there is a `params` step defined" do
+        before { service_class.class_eval { params { attribute :param1 } } }
+
+        it "returns the contract as the params object" do
+          expect(result[:params]).to be_a(Service::ContractBase)
+        end
+      end
+    end
+  end
 end