diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 59bb045e0fa..b5b74a7868b 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -137,18 +137,31 @@ en: string_value_not_valid_min: "Value must be at least %{min} characters long." string_value_not_valid_max: "Value must be at most %{max} characters long." objects: + humanize_required: "The property at JSON Pointer '%{property_json_pointer}' must be present." required: "must be present" + humanize_invalid_type: "The property at JSON Pointer '%{property_json_pointer}' must be of type %{type}." invalid_type: "%{type} is not a valid type" + humanize_not_valid_string_value: "The property at JSON Pointer '%{property_json_pointer}' must be a string." not_valid_string_value: "must be a string" + humanize_not_valid_integer_value: "The property at JSON Pointer '%{property_json_pointer}' must be an integer." not_valid_integer_value: "must be an integer" + humanize_not_valid_float_value: "The property at JSON Pointer '%{property_json_pointer}' must be a float." not_valid_float_value: "must be a float" + humanize_not_valid_boolean_value: "The property at JSON Pointer '%{property_json_pointer}' must be a boolean." not_valid_boolean_value: "must be a boolean" + humanize_not_valid_enum_value: "The property at JSON Pointer '%{property_json_pointer}' must be one of the following %{choices}." not_valid_enum_value: "must be one of the following: %{choices}" + humanize_not_valid_category_value: "The property at JSON Pointer '%{property_json_pointer}' must be a valid category id." not_valid_category_value: "must be a valid category id" + humanize_string_value_not_valid_min: "The property at JSON Pointer '%{property_json_pointer}' must be at least %{min} characters long." string_value_not_valid_min: "must be at least %{min} characters long" + humanize_string_value_not_valid_max: "The property at JSON Pointer '%{property_json_pointer}' must be at most %{max} characters long." string_value_not_valid_max: "must be at most %{max} characters long" + humanize_number_value_not_valid_min: "The property at JSON Pointer '%{property_json_pointer}' must be larger than or equal to %{min}." number_value_not_valid_min: "must be larger than or equal to %{min}" + humanize_number_value_not_valid_max: "The property at JSON Pointer '%{property_json_pointer}' must be smaller than or equal to %{max}." number_value_not_valid_max: "must be smaller than or equal to %{max}" + humanize_string_value_not_valid_url: "The property at JSON Pointer '%{property_json_pointer}' must be a valid URL." string_value_not_valid_url: "must be a valid URL" locale_errors: top_level_locale: "The top level key in a locale file must match the locale name" diff --git a/lib/theme_settings_object_validator.rb b/lib/theme_settings_object_validator.rb index 7ad36889e47..e641ba7f3b7 100644 --- a/lib/theme_settings_object_validator.rb +++ b/lib/theme_settings_object_validator.rb @@ -1,37 +1,72 @@ # frozen_string_literal: true class ThemeSettingsObjectValidator + class << self + def validate_objects(schema:, objects:) + error_messages = [] + + objects.each_with_index do |object, index| + humanize_error_messages( + self.new(schema: schema, object: object).validate, + index:, + error_messages:, + ) + end + + error_messages + end + + private + + def humanize_error_messages(errors, index:, error_messages:) + errors.each do |property_json_pointer, error_details| + error_messages.push(*error_details.humanize_messages("/#{index}#{property_json_pointer}")) + end + end + end + class ThemeSettingsObjectErrors def initialize @errors = [] end - def add_error(key, i18n_opts = {}) - @errors << ThemeSettingsObjectError.new(key, i18n_opts) + def add_error(error, i18n_opts = {}) + @errors << ThemeSettingsObjectError.new(error, i18n_opts) + end + + def humanize_messages(property_json_pointer) + @errors.map { |error| error.humanize_messages(property_json_pointer) } end def full_messages @errors.map(&:error_message) end end - class ThemeSettingsObjectError - def initialize(key, i18n_opts = {}) - @key = key + def initialize(error, i18n_opts = {}) + @error = error @i18n_opts = i18n_opts end + def humanize_messages(property_json_pointer) + I18n.t( + "themes.settings_errors.objects.humanize_#{@error}", + @i18n_opts.merge(property_json_pointer:), + ) + end + def error_message - I18n.t("themes.settings_errors.objects.#{@key}", @i18n_opts) + I18n.t("themes.settings_errors.objects.#{@error}", @i18n_opts) end end - def initialize(schema:, object:, valid_category_ids: nil) + def initialize(schema:, object:, valid_category_ids: nil, json_pointer_prefix: "", errors: {}) @object = object @schema_name = schema[:name] @properties = schema[:properties] - @errors = {} + @errors = errors @valid_category_ids = valid_category_ids + @json_pointer_prefix = json_pointer_prefix end def validate @@ -39,15 +74,17 @@ class ThemeSettingsObjectValidator @properties.each do |property_name, property_attributes| if property_attributes[:type] == "objects" - @object[property_name]&.each do |child_object| - @errors[property_name] ||= [] - - @errors[property_name].push( - self - .class - .new(schema: property_attributes[:schema], object: child_object, valid_category_ids:) - .validate, - ) + @object[property_name]&.each_with_index do |child_object, index| + self + .class + .new( + schema: property_attributes[:schema], + object: child_object, + valid_category_ids:, + json_pointer_prefix: "#{@json_pointer_prefix}#{property_name}/#{index}/", + errors: @errors, + ) + .validate end end end @@ -148,8 +185,13 @@ class ThemeSettingsObjectValidator end def add_error(property_name, key, i18n_opts = {}) - @errors[property_name] ||= ThemeSettingsObjectErrors.new - @errors[property_name].add_error(key, i18n_opts) + pointer = json_pointer(property_name) + @errors[pointer] ||= ThemeSettingsObjectErrors.new + @errors[pointer].add_error(key, i18n_opts) + end + + def json_pointer(property_name) + "/#{@json_pointer_prefix}#{property_name}" end def valid_category_ids diff --git a/lib/theme_settings_validator.rb b/lib/theme_settings_validator.rb index 19ad9d7f758..03385884376 100644 --- a/lib/theme_settings_validator.rb +++ b/lib/theme_settings_validator.rb @@ -51,6 +51,10 @@ class ThemeSettingsValidator errors:, translation_prefix: "string", ) + when types[:objects] + errors.push( + ThemeSettingsObjectValidator.validate_objects(schema: opts[:schema], objects: value), + ) end errors diff --git a/spec/fixtures/theme_settings/invalid_settings.yaml b/spec/fixtures/theme_settings/invalid_settings.yaml index 184f7e59691..68adfd1d8f3 100644 --- a/spec/fixtures/theme_settings/invalid_settings.yaml +++ b/spec/fixtures/theme_settings/invalid_settings.yaml @@ -17,3 +17,32 @@ default_out_of_range: string_default_out_of_range: default: "abcdefg" min: 20 + +invalid_default_objects_setting: + type: objects + default: + - min_5_chars_string: "12345" + children: + - required_integer: 1 + - required_string: "some string" + min_5_chars_string: "123" + children: + - non_existent: "string" + schema: + name: parent + properties: + required_string: + type: string + required: true + min_5_chars_string: + type: string + validations: + min_length: 5 + children: + type: objects + schema: + name: child + properties: + required_integer: + type: integer + required: true diff --git a/spec/lib/theme_settings_object_validator_spec.rb b/spec/lib/theme_settings_object_validator_spec.rb index f3c75f03b54..c0c4ae79606 100644 --- a/spec/lib/theme_settings_object_validator_spec.rb +++ b/spec/lib/theme_settings_object_validator_spec.rb @@ -1,6 +1,76 @@ # frozen_string_literal: true RSpec.describe ThemeSettingsObjectValidator do + describe ".validate_objects" do + it "should return the right array of humanized error messages for objects that are invalid" do + schema = { + name: "section", + properties: { + title: { + type: "string", + required: true, + validations: { + min_length: 5, + max_length: 10, + }, + }, + category_property: { + type: "category", + required: true, + }, + links: { + type: "objects", + schema: { + name: "link", + properties: { + position: { + type: "integer", + required: true, + }, + float: { + type: "float", + required: true, + validations: { + min: 5.5, + max: 11.5, + }, + }, + }, + }, + }, + }, + } + + category = Fabricate(:category) + + error_messages = + described_class.validate_objects( + schema: schema, + objects: [ + { + title: "1234", + category_property: category.id, + links: [{ position: 1, float: 4.5 }, { position: "string", float: 12 }], + }, + { title: "12345678910", category_property: 99_999_999, links: [{ float: 5 }] }, + ], + ) + + expect(error_messages).to eq( + [ + "The property at JSON Pointer '/0/title' must be at least 5 characters long.", + "The property at JSON Pointer '/0/links/0/float' must be larger than or equal to 5.5.", + "The property at JSON Pointer '/0/links/1/position' must be an integer.", + "The property at JSON Pointer '/0/links/1/float' must be smaller than or equal to 11.5.", + "The property at JSON Pointer '/1/title' must be at most 10 characters long.", + "The property at JSON Pointer '/1/category_property' must be a valid category id.", + "The property at JSON Pointer '/1/links/0/position' must be present.", + "The property at JSON Pointer '/1/links/0/float' must be larger than or equal to 5.5.", + ], + ) + end + end + describe "#validate" do it "should return the right hash of error messages when properties are required but missing" do schema = { @@ -46,8 +116,9 @@ RSpec.describe ThemeSettingsObjectValidator do errors = described_class.new(schema:, object: {}).validate - expect(errors[:description].full_messages).to contain_exactly("must be present") - expect(errors[:title].full_messages).to contain_exactly("must be present") + expect(errors.keys).to contain_exactly("/description", "/title") + 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( @@ -57,19 +128,30 @@ RSpec.describe ThemeSettingsObjectValidator do }, ).validate - 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.keys).to eq( + %w[ + /title + /description + /links/0/name + /links/0/child_links/0/title + /links/0/child_links/1/title + /links/1/name + ], + ) - expect(errors[:links][0][:child_links][0][:title].full_messages).to contain_exactly( + 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( + 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") + expect(errors["/links/1/name"].full_messages).to contain_exactly("must be present") end context "for enum properties" do @@ -95,7 +177,9 @@ RSpec.describe ThemeSettingsObjectValidator do errors = described_class.new(schema: schema, object: { enum_property: "random_value" }).validate - expect(errors[:enum_property].full_messages).to contain_exactly( + expect(errors.keys).to eq(["/enum_property"]) + + expect(errors["/enum_property"].full_messages).to contain_exactly( "must be one of the following: [\"choice 1\", 2, false]", ) end @@ -103,7 +187,9 @@ RSpec.describe ThemeSettingsObjectValidator do it "should return the right hash of error messages when enum property is not present" do errors = described_class.new(schema: schema, object: {}).validate - expect(errors[:enum_property].full_messages).to contain_exactly( + expect(errors.keys).to eq(["/enum_property"]) + + expect(errors["/enum_property"].full_messages).to contain_exactly( "must be one of the following: [\"choice 1\", 2, false]", ) end @@ -126,7 +212,8 @@ RSpec.describe ThemeSettingsObjectValidator do errors = described_class.new(schema: schema, object: { boolean_property: "string" }).validate - expect(errors[:boolean_property].full_messages).to contain_exactly("must be a boolean") + expect(errors.keys).to eq(["/boolean_property"]) + expect(errors["/boolean_property"].full_messages).to contain_exactly("must be a boolean") end end @@ -146,7 +233,8 @@ RSpec.describe ThemeSettingsObjectValidator do it "should return the right hash of error messages when value of property is not of type float" do errors = described_class.new(schema: schema, object: { float_property: "string" }).validate - expect(errors[:float_property].full_messages).to contain_exactly("must be a float") + expect(errors.keys).to eq(["/float_property"]) + 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 @@ -165,13 +253,17 @@ RSpec.describe ThemeSettingsObjectValidator do errors = described_class.new(schema: schema, object: { float_property: 4.5 }).validate - expect(errors[:float_property].full_messages).to contain_exactly( + expect(errors.keys).to eq(["/float_property"]) + + expect(errors["/float_property"].full_messages).to contain_exactly( "must be larger than or equal to 5.5", ) errors = described_class.new(schema: schema, object: { float_property: 12.5 }).validate - expect(errors[:float_property].full_messages).to contain_exactly( + expect(errors.keys).to eq(["/float_property"]) + + expect(errors["/float_property"].full_messages).to contain_exactly( "must be smaller than or equal to 11.5", ) end @@ -190,11 +282,13 @@ RSpec.describe ThemeSettingsObjectValidator do errors = described_class.new(schema: schema, object: { integer_property: "string" }).validate - expect(errors[:integer_property].full_messages).to contain_exactly("must be an integer") + expect(errors.keys).to eq(["/integer_property"]) + 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") + expect(errors.keys).to eq(["/integer_property"]) + 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 @@ -232,13 +326,17 @@ RSpec.describe ThemeSettingsObjectValidator do errors = described_class.new(schema: schema, object: { integer_property: 4 }).validate - expect(errors[:integer_property].full_messages).to contain_exactly( + expect(errors.keys).to eq(["/integer_property"]) + + expect(errors["/integer_property"].full_messages).to contain_exactly( "must be larger than or equal to 5", ) errors = described_class.new(schema: schema, object: { integer_property: 11 }).validate - expect(errors[:integer_property].full_messages).to contain_exactly( + expect(errors.keys).to eq(["/integer_property"]) + + expect(errors["/integer_property"].full_messages).to contain_exactly( "must be smaller than or equal to 10", ) end @@ -257,7 +355,8 @@ RSpec.describe ThemeSettingsObjectValidator do schema = { name: "section", properties: { string_property: { type: "string" } } } errors = described_class.new(schema: schema, object: { string_property: 1 }).validate - expect(errors[:string_property].full_messages).to contain_exactly("must be a string") + expect(errors.keys).to eq(["/string_property"]) + 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 @@ -276,7 +375,8 @@ RSpec.describe ThemeSettingsObjectValidator do 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") + expect(errors.keys).to eq(["/string_property"]) + 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 @@ -314,14 +414,18 @@ RSpec.describe ThemeSettingsObjectValidator do errors = described_class.new(schema: schema, object: { string_property: "1234" }).validate - expect(errors[:string_property].full_messages).to contain_exactly( + expect(errors.keys).to eq(["/string_property"]) + + 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( + expect(errors.keys).to eq(["/string_property"]) + + expect(errors["/string_property"].full_messages).to contain_exactly( "must be at most 10 characters long", ) end @@ -344,7 +448,9 @@ RSpec.describe ThemeSettingsObjectValidator do errors = described_class.new(schema: schema, object: { category_property: "string" }).validate - expect(errors[:category_property].full_messages).to contain_exactly( + expect(errors.keys).to eq(["/category_property"]) + + expect(errors["/category_property"].full_messages).to contain_exactly( "must be a valid category id", ) end @@ -390,19 +496,21 @@ RSpec.describe ThemeSettingsObjectValidator do }, ).validate - expect(errors[:category_property].full_messages).to contain_exactly( + expect(errors.keys).to eq( + %w[/category_property /category_property_2 /child_categories/0/category_property_3], + ) + + 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( + 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, + 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 diff --git a/spec/models/theme_field_spec.rb b/spec/models/theme_field_spec.rb index e9ec45c544d..d996ea7b1fc 100644 --- a/spec/models/theme_field_spec.rb +++ b/spec/models/theme_field_spec.rb @@ -412,6 +412,14 @@ HTML ) end + it "generates the right errors when setting of type objects have default values which does not matches the schema" do + field = create_yaml_field(get_fixture("invalid")) + + expect(field.error).to include( + "Setting `invalid_default_objects_setting` default value isn't valid. The property at JSON Pointer '/0/required_string' must be present. The property at JSON Pointer '/1/min_5_chars_string' must be at least 5 characters long. The property at JSON Pointer '/1/children/0/required_integer' must be present.", + ) + end + it "works correctly when valid yaml is provided" do field = create_yaml_field(get_fixture("valid")) expect(field.error).to be_nil