From 3e543513558edc7ea3e8f22f8379fded2a97bf64 Mon Sep 17 00:00:00 2001 From: Alan Guo Xiang Tan Date: Wed, 21 Feb 2024 15:27:42 +0800 Subject: [PATCH] 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 | 72 +++++---- .../theme_settings_object_validator_spec.rb | 153 +++++++++++------- 2 files changed, 131 insertions(+), 94 deletions(-) diff --git a/lib/theme_settings_object_validator.rb b/lib/theme_settings_object_validator.rb index d0d9b3d0030..7ad36889e47 100644 --- a/lib/theme_settings_object_validator.rb +++ b/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( - property_name, - I18n.t("themes.settings_errors.objects.not_valid_#{type}_value", property_attributes), - ) + add_error(property_name, "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( - property_name, - I18n.t("themes.settings_errors.objects.string_value_not_valid_min", min:), - ) - + add_error(property_name, :string_value_not_valid_min, min:) return false end if (max = validations&.dig(:max_length)) && value.length > max - add_error( - property_name, - I18n.t("themes.settings_errors.objects.string_value_not_valid_max", max: max), - ) - + add_error(property_name, :string_value_not_valid_max, max:) return false end if validations&.dig(:url) && !value.match?(URI.regexp) - add_error( - property_name, - I18n.t("themes.settings_errors.objects.string_value_not_valid_url"), - ) - + add_error(property_name, :string_value_not_valid_url) return false end when "integer", "float" if (min = validations&.dig(:min)) && value < min - add_error( - property_name, - I18n.t("themes.settings_errors.objects.number_value_not_valid_min", min:), - ) - + add_error(property_name, :number_value_not_valid_min, min:) return false end if (max = validations&.dig(:max)) && value > max - add_error( - property_name, - I18n.t("themes.settings_errors.objects.number_value_not_valid_max", max:), - ) - + add_error(property_name, :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) - @errors[property_name] ||= [] - @errors[property_name] << error + def add_error(property_name, key, i18n_opts = {}) + @errors[property_name] ||= ThemeSettingsObjectErrors.new + @errors[property_name].add_error(key, i18n_opts) end def valid_category_ids diff --git a/spec/lib/theme_settings_object_validator_spec.rb b/spec/lib/theme_settings_object_validator_spec.rb index 2f6ad0be244..f3c75f03b54 100644 --- a/spec/lib/theme_settings_object_validator_spec.rb +++ b/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( - title: ["must be present"], - description: ["must be present"], - links: [ - { - name: ["must be present"], - child_links: [{ title: ["must be present"] }, { title: ["must be present"] }], - }, - { name: ["must be present"] }, - ], + expect(errors[:title].full_messages).to contain_exactly("must be present") + expect(errors[:description].full_messages).to contain_exactly("must be present") + expect(errors[:links][0][:name].full_messages).to contain_exactly("must be present") + + expect(errors[:links][0][:child_links][0][:title].full_messages).to contain_exactly( + "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( - 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]"]) + errors = + described_class.new(schema: schema, object: { enum_property: "random_value" }).validate + + 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( - enum_property: ["must be one of the following: [\"choice 1\", 2, false]"], + errors = described_class.new(schema: schema, object: {}).validate + + 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( - described_class.new(schema: schema, object: { boolean_property: "string" }).validate, - ).to eq(boolean_property: ["must be a boolean"]) + errors = + described_class.new(schema: schema, object: { boolean_property: "string" }).validate + + 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( - described_class.new(schema: schema, object: { float_property: "string" }).validate, - ).to eq(float_property: ["must be a float"]) + errors = described_class.new(schema: schema, object: { float_property: "string" }).validate + + 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( - float_property: ["must be larger than or equal to 5.5"], + errors = described_class.new(schema: schema, object: { float_property: 4.5 }).validate + + expect(errors[:float_property].full_messages).to contain_exactly( + "must be larger than or equal to 5.5", ) - expect( - described_class.new(schema: schema, object: { float_property: 12.5 }).validate, - ).to eq(float_property: ["must be smaller than or equal to 11.5"]) + errors = described_class.new(schema: schema, object: { float_property: 12.5 }).validate + + 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( - described_class.new(schema: schema, object: { integer_property: "string" }).validate, - ).to eq(integer_property: ["must be an integer"]) + errors = + described_class.new(schema: schema, object: { integer_property: "string" }).validate - expect( - described_class.new(schema: schema, object: { integer_property: 1.0 }).validate, - ).to eq(integer_property: ["must be an integer"]) + expect(errors[:integer_property].full_messages).to contain_exactly("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( - integer_property: ["must be larger than or equal to 5"], + errors = described_class.new(schema: schema, object: { integer_property: 4 }).validate + + expect(errors[:integer_property].full_messages).to contain_exactly( + "must be larger than or equal to 5", ) - expect( - described_class.new(schema: schema, object: { integer_property: 11 }).validate, - ).to eq(integer_property: ["must be smaller than or equal to 10"]) + errors = described_class.new(schema: schema, object: { integer_property: 11 }).validate + + 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( - string_property: ["must be a string"], - ) + expect(errors[:string_property].full_messages).to contain_exactly("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( - described_class.new(schema: schema, object: { string_property: "not a url" }).validate, - ).to eq(string_property: ["must be a valid URL"]) + errors = + described_class.new(schema: schema, object: { string_property: "not a url" }).validate + + 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( - described_class.new(schema: schema, object: { string_property: "1234" }).validate, - ).to eq(string_property: ["must be at least 5 characters long"]) + errors = described_class.new(schema: schema, object: { string_property: "1234" }).validate - expect( - described_class.new(schema: schema, object: { string_property: "12345678910" }).validate, - ).to eq(string_property: ["must be at most 10 characters long"]) + expect(errors[:string_property].full_messages).to contain_exactly( + "must be at least 5 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( - described_class.new(schema: schema, object: { category_property: "string" }).validate, - ).to eq(category_property: ["must be a valid category id"]) + errors = + described_class.new(schema: schema, object: { category_property: "string" }).validate + + 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, - ).to eq( - category_property: ["must be a valid category id"], - category_property_2: ["must be a valid category id"], - child_categories: [{ category_property_3: ["must be a valid category id"] }, {}], + ).validate + + expect(errors[:category_property].full_messages).to contain_exactly( + "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