From f2db1f2f56b3327e945d343fa19663e59a1ac2ba Mon Sep 17 00:00:00 2001 From: Matheus Sales Date: Fri, 8 Dec 2023 17:00:52 -0300 Subject: [PATCH] feat: When an unrelated error is seen with negated allow_value, give a hint (#1570) * When an unrelated error is seen with negated allow_value, give a hint In cases when an unrelated error is seen that makes `allow_value` fail, the validation message was using the erroneous text "but it was valid instead". This changes the validation message to indicate which attribute(s) actually failed validation. * fix: Minor adjustment on wording * fix: Adjust validate inclusion spec This spec was failing because the polymorphic association was not optional, that was causing it to have an error on the association and not in the attribute `severity_type` that was being validated the inclusion. Setting the association as optional fixed the issue. --------- Co-authored-by: Matt Mitchell --- .../active_model/allow_value_matcher.rb | 9 ++ .../matchers/active_model/validator.rb | 4 + .../helpers/allow_value_matcher_helpers.rb | 4 + .../record_with_unrelated_error_builder.rb | 90 +++++++++++++++++++ .../active_model/allow_value_matcher_spec.rb | 22 +++++ .../validate_inclusion_of_matcher_spec.rb | 4 +- 6 files changed, 131 insertions(+), 2 deletions(-) create mode 100644 spec/support/unit/record_with_unrelated_error_builder.rb diff --git a/lib/shoulda/matchers/active_model/allow_value_matcher.rb b/lib/shoulda/matchers/active_model/allow_value_matcher.rb index 023e0833b..ff33f20cf 100644 --- a/lib/shoulda/matchers/active_model/allow_value_matcher.rb +++ b/lib/shoulda/matchers/active_model/allow_value_matcher.rb @@ -402,6 +402,10 @@ def does_not_match?(instance) @result.nil? end + def has_any_errors? + validator.record.errors.any? + end + def failure_message attribute_setter = result.attribute_setter @@ -480,6 +484,11 @@ def failure_message_when_negated # rubocop:disable Metrics/MethodLength message << " it produced these validation errors instead:\n\n" message << validator.all_formatted_validation_error_messages end + elsif validator.has_any_errors? + message << ", placing a validation error on :#{attribute_setter.attribute_name}" + message << '. The Example was invalid,' + message << " but it had errors involving other attributes:\n\n" + message << validator.all_formatted_validation_error_messages else message << ', but it was valid instead.' end diff --git a/lib/shoulda/matchers/active_model/validator.rb b/lib/shoulda/matchers/active_model/validator.rb index a319444d4..7e66a8d8a 100644 --- a/lib/shoulda/matchers/active_model/validator.rb +++ b/lib/shoulda/matchers/active_model/validator.rb @@ -25,6 +25,10 @@ def has_messages? messages.any? end + def has_any_errors? + record.errors.any? + end + def captured_validation_exception? @captured_validation_exception end diff --git a/spec/support/unit/helpers/allow_value_matcher_helpers.rb b/spec/support/unit/helpers/allow_value_matcher_helpers.rb index c4143989e..272fe1c4e 100644 --- a/spec/support/unit/helpers/allow_value_matcher_helpers.rb +++ b/spec/support/unit/helpers/allow_value_matcher_helpers.rb @@ -7,6 +7,10 @@ def builder_for_record_with_different_error_attribute(options = {}) RecordWithDifferentErrorAttributeBuilder.new(options) end + def builder_for_record_with_unrelated_error(options = {}) + RecordWithUnrelatedErrorBuilder.new(options) + end + def builder_for_record_with_different_error_attribute_using_i18n(options = {}) builder = builder_for_record_with_different_error_attribute(options) RecordBuilderWithI18nValidationMessage.new(builder) diff --git a/spec/support/unit/record_with_unrelated_error_builder.rb b/spec/support/unit/record_with_unrelated_error_builder.rb new file mode 100644 index 000000000..9c5663c03 --- /dev/null +++ b/spec/support/unit/record_with_unrelated_error_builder.rb @@ -0,0 +1,90 @@ +require_relative 'helpers/model_builder' + +module UnitTests + class RecordWithUnrelatedErrorBuilder + include ModelBuilder + + def initialize(options) + @options = options.reverse_merge(default_options) + end + + def attribute_that_receives_error + options[:attribute_that_receives_error] + end + + def attribute_to_validate + options[:attribute_to_validate] + end + + def message + options[:message] + end + + def message=(message) + options[:message] = message + end + + def model + @_model ||= create_model + end + + def model_name + 'Example' + end + + def record + model.new + end + + def valid_value + 'some value' + end + + protected + + attr_reader :options + + private + + def context + { + validation_method_name: validation_method_name, + valid_value: valid_value, + attribute_to_validate: attribute_to_validate, + attribute_that_receives_error: attribute_that_receives_error, + message: message, + } + end + + def create_model + _context = context + + define_model model_name, model_columns do + validate _context[:validation_method_name] + + define_method(_context[:validation_method_name]) do + errors.add(_context[:attribute_that_receives_error], _context[:message]) + end + end + end + + def validation_method_name + :custom_validation + end + + def model_columns + { + attribute_to_validate => :string, + attribute_that_receives_error => :string, + } + end + + def default_options + { + attribute_that_receives_error: :attribute_that_receives_error, + attribute_to_validate: :attribute_to_validate, + message: 'some message', + } + end + end +end diff --git a/spec/unit/shoulda/matchers/active_model/allow_value_matcher_spec.rb b/spec/unit/shoulda/matchers/active_model/allow_value_matcher_spec.rb index 881758a1f..929a9ce2f 100644 --- a/spec/unit/shoulda/matchers/active_model/allow_value_matcher_spec.rb +++ b/spec/unit/shoulda/matchers/active_model/allow_value_matcher_spec.rb @@ -353,6 +353,28 @@ context 'when the attribute being validated is different than the attribute that receives the validation error' do include UnitTests::AllowValueMatcherHelpers + context 'when no validation message was provided directly' do + context 'when asserting the negative' do + it 'rejects with an appropriate failure message' do + builder = builder_for_record_with_unrelated_error + assertion = lambda do + expect(builder.record). + not_to allow_value(builder.valid_value). + for(builder.attribute_to_validate) + end + + message = <<-MESSAGE +After setting :attribute_to_validate to ‹"some value"›, the matcher +expected the Example to be invalid, placing a validation error on +:attribute_to_validate. The Example was invalid, but it had errors +involving other attributes: + +* attribute_that_receives_error: ["some message"] + MESSAGE + expect(&assertion).to fail_with_message(message) + end + end + end context 'when the validation error message was provided directly' do context 'given a valid value' do diff --git a/spec/unit/shoulda/matchers/active_model/validate_inclusion_of_matcher_spec.rb b/spec/unit/shoulda/matchers/active_model/validate_inclusion_of_matcher_spec.rb index 3bffc3303..e237a412b 100644 --- a/spec/unit/shoulda/matchers/active_model/validate_inclusion_of_matcher_spec.rb +++ b/spec/unit/shoulda/matchers/active_model/validate_inclusion_of_matcher_spec.rb @@ -890,7 +890,7 @@ def validation_matcher_scenario_args context 'against a polymorphic association' do it 'matches when the subject configures validate_inclusion_of' do define_model(:issue, severity_id: :integer, severity_type: :string) do - belongs_to :severity, polymorphic: true + belongs_to :severity, polymorphic: true, optional: true validates_inclusion_of :severity_type, in: %w(Low Medium High) end define_model(:high) @@ -902,7 +902,7 @@ def validation_matcher_scenario_args it 'does not match when subject does not set validate_inclusion_of' do define_model(:issue, severity_id: :integer, severity_type: :string) do - belongs_to :severity, polymorphic: true + belongs_to :severity, polymorphic: true, optional: true end define_model(:high) define_model(:medium)