DEV: Change shape of errors in ThemeSettingsObjectValidator (#25784)

Why this change?

The current shape of errors returns the error messages after it has been
translated but there are cases where we want to customize the error
messages and the current way return only translated error messages is
making customization of error messages difficult. If we
wish to have the error messages in complete sentences like
"`some_property` property must be present in #link 1", this is not
possible at the moment with the current shape of the errors we return.

What does this change do?

This change introduces the `ThemeSettingsObjectValidator::ThemeSettingsObjectErrors`
and `ThemeSettingsObjectValidator::ThemeSettingsObjectError` classes to
hold the relevant error key and i18n translation options.

lib/theme_settings_object_validator.rb

@@ -1,6 +1,31 @@
 # frozen_string_literal: true
 
 class ThemeSettingsObjectValidator
+  class ThemeSettingsObjectErrors
+    def initialize
+      @errors = []
+    end
+
+    def add_error(key, i18n_opts = {})
+      @errors << ThemeSettingsObjectError.new(key, i18n_opts)
+    end
+
+    def full_messages
+      @errors.map(&:error_message)
+    end
+  end
+
+  class ThemeSettingsObjectError
+    def initialize(key, i18n_opts = {})
+      @key = key
+      @i18n_opts = i18n_opts
+    end
+
+    def error_message
+      I18n.t("themes.settings_errors.objects.#{@key}", @i18n_opts)
+    end
+  end
+
   def initialize(schema:, object:, valid_category_ids: nil)
     @object = object
     @schema_name = schema[:name]
@@ -60,17 +85,14 @@ class ThemeSettingsObjectValidator
       when "enum"
         property_attributes[:choices].include?(value)
       else
-        add_error(property_name, I18n.t("themes.settings_errors.objects.invalid_type", type:))
+        add_error(property_name, :invalid_type, type:)
         return false
       end
 
     if is_value_valid
       true
     else
-      add_error(
+      add_error(property_name, "not_valid_#{type}_value", property_attributes)
-        property_name,
-        I18n.t("themes.settings_errors.objects.not_valid_#{type}_value", property_attributes),
-      )
       false
     end
   end
@@ -83,52 +105,32 @@ class ThemeSettingsObjectValidator
     case type
     when "category"
       if !valid_category_ids.include?(value)
-        add_error(property_name, I18n.t("themes.settings_errors.objects.not_valid_category_value"))
+        add_error(property_name, :not_valid_category_value)
         return false
       end
     when "string"
       if (min = validations&.dig(:min_length)) && value.length < min
-        add_error(
+        add_error(property_name, :string_value_not_valid_min, min:)
-          property_name,
-          I18n.t("themes.settings_errors.objects.string_value_not_valid_min", min:),
-        )
-
         return false
       end
 
       if (max = validations&.dig(:max_length)) && value.length > max
-        add_error(
+        add_error(property_name, :string_value_not_valid_max, max:)
-          property_name,
-          I18n.t("themes.settings_errors.objects.string_value_not_valid_max", max: max),
-        )
-
         return false
       end
 
       if validations&.dig(:url) && !value.match?(URI.regexp)
-        add_error(
+        add_error(property_name, :string_value_not_valid_url)
-          property_name,
-          I18n.t("themes.settings_errors.objects.string_value_not_valid_url"),
-        )
-
         return false
       end
     when "integer", "float"
       if (min = validations&.dig(:min)) && value < min
-        add_error(
+        add_error(property_name, :number_value_not_valid_min, min:)
-          property_name,
-          I18n.t("themes.settings_errors.objects.number_value_not_valid_min", min:),
-        )
-
         return false
       end
 
       if (max = validations&.dig(:max)) && value > max
-        add_error(
+        add_error(property_name, :number_value_not_valid_max, max:)
-          property_name,
-          I18n.t("themes.settings_errors.objects.number_value_not_valid_max", max:),
-        )
-
         return false
       end
     end
@@ -138,16 +140,16 @@ class ThemeSettingsObjectValidator
 
   def is_property_present?(property_name)
     if @object[property_name].nil?
-      add_error(property_name, I18n.t("themes.settings_errors.objects.required"))
+      add_error(property_name, :required)
       false
     else
       true
     end
   end
 
-  def add_error(property_name, error)
+  def add_error(property_name, key, i18n_opts = {})
-    @errors[property_name] ||= []
+    @errors[property_name] ||= ThemeSettingsObjectErrors.new
-    @errors[property_name] << error
+    @errors[property_name].add_error(key, i18n_opts)
   end
 
   def valid_category_ids

spec/lib/theme_settings_object_validator_spec.rb

@@ -46,7 +46,8 @@ RSpec.describe ThemeSettingsObjectValidator do
 
       errors = described_class.new(schema:, object: {}).validate
 
-      expect(errors).to eq(title: ["must be present"], description: ["must be present"])
+      expect(errors[:description].full_messages).to contain_exactly("must be present")
+      expect(errors[:title].full_messages).to contain_exactly("must be present")
 
       errors =
         described_class.new(
@@ -56,17 +57,19 @@ RSpec.describe ThemeSettingsObjectValidator do
           },
         ).validate
 
-      expect(errors).to eq(
+      expect(errors[:title].full_messages).to contain_exactly("must be present")
-        title: ["must be present"],
+      expect(errors[:description].full_messages).to contain_exactly("must be present")
-        description: ["must be present"],
+      expect(errors[:links][0][:name].full_messages).to contain_exactly("must be present")
-        links: [
+
-          {
+      expect(errors[:links][0][:child_links][0][:title].full_messages).to contain_exactly(
-            name: ["must be present"],
+        "must be present",
-            child_links: [{ title: ["must be present"] }, { title: ["must be present"] }],
-          },
-          { name: ["must be present"] },
-        ],
       )
+
+      expect(errors[:links][0][:child_links][1][:title].full_messages).to contain_exactly(
+        "must be present",
+      )
+
+      expect(errors[:links][1][:name].full_messages).to contain_exactly("must be present")
     end
 
     context "for enum properties" do
@@ -89,14 +92,19 @@ RSpec.describe ThemeSettingsObjectValidator do
       end
 
       it "should return the right hash of error messages when value of property is not in the enum" do
-        expect(
+        errors =
-          described_class.new(schema: schema, object: { enum_property: "random_value" }).validate,
+          described_class.new(schema: schema, object: { enum_property: "random_value" }).validate
-        ).to eq(enum_property: ["must be one of the following: [\"choice 1\", 2, false]"])
+
+        expect(errors[:enum_property].full_messages).to contain_exactly(
+          "must be one of the following: [\"choice 1\", 2, false]",
+        )
       end
 
       it "should return the right hash of error messages when enum property is not present" do
-        expect(described_class.new(schema: schema, object: {}).validate).to eq(
+        errors = described_class.new(schema: schema, object: {}).validate
-          enum_property: ["must be one of the following: [\"choice 1\", 2, false]"],
+
+        expect(errors[:enum_property].full_messages).to contain_exactly(
+          "must be one of the following: [\"choice 1\", 2, false]",
         )
       end
     end
@@ -115,9 +123,10 @@ RSpec.describe ThemeSettingsObjectValidator do
       end
 
       it "should return the right hash of error messages when value of property is not of type boolean" do
-        expect(
+        errors =
-          described_class.new(schema: schema, object: { boolean_property: "string" }).validate,
+          described_class.new(schema: schema, object: { boolean_property: "string" }).validate
-        ).to eq(boolean_property: ["must be a boolean"])
+
+        expect(errors[:boolean_property].full_messages).to contain_exactly("must be a boolean")
       end
     end
 
@@ -135,9 +144,9 @@ RSpec.describe ThemeSettingsObjectValidator do
       end
 
       it "should return the right hash of error messages when value of property is not of type float" do
-        expect(
+        errors = described_class.new(schema: schema, object: { float_property: "string" }).validate
-          described_class.new(schema: schema, object: { float_property: "string" }).validate,
+
-        ).to eq(float_property: ["must be a float"])
+        expect(errors[:float_property].full_messages).to contain_exactly("must be a float")
       end
 
       it "should return the right hash of error messages when integer property does not satisfy min or max validations" do
@@ -154,13 +163,17 @@ RSpec.describe ThemeSettingsObjectValidator do
           },
         }
 
-        expect(described_class.new(schema: schema, object: { float_property: 4.5 }).validate).to eq(
+        errors = described_class.new(schema: schema, object: { float_property: 4.5 }).validate
-          float_property: ["must be larger than or equal to 5.5"],
+
+        expect(errors[:float_property].full_messages).to contain_exactly(
+          "must be larger than or equal to 5.5",
         )
 
-        expect(
+        errors = described_class.new(schema: schema, object: { float_property: 12.5 }).validate
-          described_class.new(schema: schema, object: { float_property: 12.5 }).validate,
+
-        ).to eq(float_property: ["must be smaller than or equal to 11.5"])
+        expect(errors[:float_property].full_messages).to contain_exactly(
+          "must be smaller than or equal to 11.5",
+        )
       end
     end
 
@@ -174,13 +187,14 @@ RSpec.describe ThemeSettingsObjectValidator do
       end
 
       it "should return the right hash of error messages when value of property is not of type integer" do
-        expect(
+        errors =
-          described_class.new(schema: schema, object: { integer_property: "string" }).validate,
+          described_class.new(schema: schema, object: { integer_property: "string" }).validate
-        ).to eq(integer_property: ["must be an integer"])
 
-        expect(
+        expect(errors[:integer_property].full_messages).to contain_exactly("must be an integer")
-          described_class.new(schema: schema, object: { integer_property: 1.0 }).validate,
+
-        ).to eq(integer_property: ["must be an integer"])
+        errors = described_class.new(schema: schema, object: { integer_property: 1.0 }).validate
+
+        expect(errors[:integer_property].full_messages).to contain_exactly("must be an integer")
       end
 
       it "should not return any error messages when the value of the integer property satisfies min and max validations" do
@@ -216,13 +230,17 @@ RSpec.describe ThemeSettingsObjectValidator do
           },
         }
 
-        expect(described_class.new(schema: schema, object: { integer_property: 4 }).validate).to eq(
+        errors = described_class.new(schema: schema, object: { integer_property: 4 }).validate
-          integer_property: ["must be larger than or equal to 5"],
+
+        expect(errors[:integer_property].full_messages).to contain_exactly(
+          "must be larger than or equal to 5",
         )
 
-        expect(
+        errors = described_class.new(schema: schema, object: { integer_property: 11 }).validate
-          described_class.new(schema: schema, object: { integer_property: 11 }).validate,
+
-        ).to eq(integer_property: ["must be smaller than or equal to 10"])
+        expect(errors[:integer_property].full_messages).to contain_exactly(
+          "must be smaller than or equal to 10",
+        )
       end
     end
 
@@ -237,10 +255,9 @@ RSpec.describe ThemeSettingsObjectValidator do
 
       it "should return the right hash of error messages when value of property is not of type string" do
         schema = { name: "section", properties: { string_property: { type: "string" } } }
+        errors = described_class.new(schema: schema, object: { string_property: 1 }).validate
 
-        expect(described_class.new(schema: schema, object: { string_property: 1 }).validate).to eq(
+        expect(errors[:string_property].full_messages).to contain_exactly("must be a string")
-          string_property: ["must be a string"],
-        )
       end
 
       it "should return the right hash of error messages when string property does not statisfy url validation" do
@@ -256,9 +273,10 @@ RSpec.describe ThemeSettingsObjectValidator do
           },
         }
 
-        expect(
+        errors =
-          described_class.new(schema: schema, object: { string_property: "not a url" }).validate,
+          described_class.new(schema: schema, object: { string_property: "not a url" }).validate
-        ).to eq(string_property: ["must be a valid URL"])
+
+        expect(errors[:string_property].full_messages).to contain_exactly("must be a valid URL")
       end
 
       it "should not return any error messages when the value of the string property satisfies min_length and max_length validations" do
@@ -294,13 +312,18 @@ RSpec.describe ThemeSettingsObjectValidator do
           },
         }
 
-        expect(
+        errors = described_class.new(schema: schema, object: { string_property: "1234" }).validate
-          described_class.new(schema: schema, object: { string_property: "1234" }).validate,
-        ).to eq(string_property: ["must be at least 5 characters long"])
 
-        expect(
+        expect(errors[:string_property].full_messages).to contain_exactly(
-          described_class.new(schema: schema, object: { string_property: "12345678910" }).validate,
+          "must be at least 5 characters long",
-        ).to eq(string_property: ["must be at most 10 characters long"])
+        )
+
+        errors =
+          described_class.new(schema: schema, object: { string_property: "12345678910" }).validate
+
+        expect(errors[:string_property].full_messages).to contain_exactly(
+          "must be at most 10 characters long",
+        )
       end
     end
 
@@ -318,9 +341,12 @@ RSpec.describe ThemeSettingsObjectValidator do
       it "should return the right hash of error messages when value of property is not an integer" do
         schema = { name: "section", properties: { category_property: { type: "category" } } }
 
-        expect(
+        errors =
-          described_class.new(schema: schema, object: { category_property: "string" }).validate,
+          described_class.new(schema: schema, object: { category_property: "string" }).validate
-        ).to eq(category_property: ["must be a valid category id"])
+
+        expect(errors[:category_property].full_messages).to contain_exactly(
+          "must be a valid category id",
+        )
       end
 
       it "should return the right hash of error messages when value of property is not a valid id of a category record" do
@@ -351,7 +377,7 @@ RSpec.describe ThemeSettingsObjectValidator do
 
         queries =
           track_sql_queries do
-            expect(
+            errors =
               described_class.new(
                 schema: schema,
                 object: {
@@ -362,12 +388,21 @@ RSpec.describe ThemeSettingsObjectValidator do
                     { category_property_3: category.id },
                   ],
                 },
-              ).validate,
+              ).validate
-            ).to eq(
+
-              category_property: ["must be a valid category id"],
+            expect(errors[:category_property].full_messages).to contain_exactly(
-              category_property_2: ["must be a valid category id"],
+              "must be a valid category id",
-              child_categories: [{ category_property_3: ["must be a valid category id"] }, {}],
             )
+
+            expect(errors[:category_property_2].full_messages).to contain_exactly(
+              "must be a valid category id",
+            )
+
+            expect(
+              errors[:child_categories][0][:category_property_3].full_messages,
+            ).to contain_exactly("must be a valid category id")
+
+            expect(errors[:child_categories][1]).to eq({})
           end
 
         # only 1 SQL query should be executed to check if category ids are valid