From 23c486799fbcca348ab0496f40850c0c03e850af Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Lo=C3=AFc=20Guitaut?= <loic@discourse.org>
Date: Thu, 17 Oct 2024 15:26:43 +0200
Subject: [PATCH] DEV: Improve `array` type in service contracts
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This patch improves the custom `array` type available in contracts.
It’s now able to split strings on `|` on top of `,`, and to be more
consistent, it also tries to cast the resulting items to integers.
---
 lib/active_support_type_extensions/array.rb   |  2 +-
 .../handle_chat_allowed_groups_change.rb      | 16 +++++--------
 .../array_spec.rb                             | 24 +++++++++++++++++++
 3 files changed, 31 insertions(+), 11 deletions(-)

diff --git a/lib/active_support_type_extensions/array.rb b/lib/active_support_type_extensions/array.rb
index 9ea0dffe8a5..843624c824e 100644
--- a/lib/active_support_type_extensions/array.rb
+++ b/lib/active_support_type_extensions/array.rb
@@ -9,7 +9,7 @@ module ActiveSupportTypeExtensions
     def cast_value(value)
       case value
       when String
-        value.split(",")
+        cast_value(value.split(/,(?!.*\|)|\|(?!.*,)/))
       when ::Array
         value.map { |item| Integer(item, exception: false) || item }
       else
diff --git a/plugins/chat/app/services/chat/auto_remove/handle_chat_allowed_groups_change.rb b/plugins/chat/app/services/chat/auto_remove/handle_chat_allowed_groups_change.rb
index 86a87ae985f..fd212f63c61 100644
--- a/plugins/chat/app/services/chat/auto_remove/handle_chat_allowed_groups_change.rb
+++ b/plugins/chat/app/services/chat/auto_remove/handle_chat_allowed_groups_change.rb
@@ -19,7 +19,7 @@ module Chat
       include Service::Base
 
       policy :chat_enabled
-      step :cast_new_allowed_groups_to_array
+      contract { attribute :new_allowed_groups, :array }
       policy :not_everyone_allowed
       model :users
       model :memberships_to_remove
@@ -32,15 +32,11 @@ module Chat
         SiteSetting.chat_enabled
       end
 
-      def cast_new_allowed_groups_to_array(new_allowed_groups:)
-        context[:new_allowed_groups] = new_allowed_groups.to_s.split("|").map(&:to_i)
+      def not_everyone_allowed(contract:)
+        contract.new_allowed_groups.exclude?(Group::AUTO_GROUPS[:everyone])
       end
 
-      def not_everyone_allowed(new_allowed_groups:)
-        !new_allowed_groups.include?(Group::AUTO_GROUPS[:everyone])
-      end
-
-      def fetch_users(new_allowed_groups:)
+      def fetch_users(contract:)
         User
           .real
           .activated
@@ -50,8 +46,8 @@ module Chat
           .joins(:user_chat_channel_memberships)
           .distinct
           .then do |users|
-            break users if new_allowed_groups.blank?
-            users.where(<<~SQL, new_allowed_groups)
+            break users if contract.new_allowed_groups.blank?
+            users.where(<<~SQL, contract.new_allowed_groups)
                 users.id NOT IN (
                   SELECT DISTINCT group_users.user_id
                   FROM group_users
diff --git a/spec/lib/active_support_type_extensions/array_spec.rb b/spec/lib/active_support_type_extensions/array_spec.rb
index a6aab57b07c..1cb522bdbc0 100644
--- a/spec/lib/active_support_type_extensions/array_spec.rb
+++ b/spec/lib/active_support_type_extensions/array_spec.rb
@@ -14,6 +14,30 @@ RSpec.describe ActiveSupportTypeExtensions::Array do
       end
     end
 
+    context "when 'value' is a string of numbers" do
+      let(:value) { "1,2,3" }
+
+      it "splits it with strings casted as integers" do
+        expect(casted_value).to eq([1, 2, 3])
+      end
+    end
+
+    context "when 'value' is a string of numbers separated by '|'" do
+      let(:value) { "1|2|3" }
+
+      it "splits it with strings casted as integers" do
+        expect(casted_value).to eq([1, 2, 3])
+      end
+    end
+
+    context "when 'value' has mixed separators" do
+      let(:value) { "1,2,3|4" }
+
+      it "splits only on one of the separators" do
+        expect(casted_value).to eq(["1,2,3", 4])
+      end
+    end
+
     context "when 'value' is an array" do
       let(:value) { %w[existing array] }