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 <[email protected]>

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

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

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)

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

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

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)