From ad9b112ccaa08db2284a5f2b9965c891d51b4033 Mon Sep 17 00:00:00 2001 From: Elliot Winkler Date: Tue, 11 Sep 2018 23:05:06 -0600 Subject: [PATCH] Fix inclusion to correctly disallow outside values The inclusion matcher, when qualified with `in_array`, was using AllowValueMatcher to check that values outside the array were disallowed by the model (and then inverting its result). However, it should have been using DisallowValueMatcher all this time. This commit fixes that. Without this fix, the following error is raised when using the inclusion matcher against a model which does not have the proper inclusion validation on it: undefined method `attribute_setter' for nil:NilClass This happens because the inclusion matcher is a complex matcher, i.e., it runs a series of submatchers internally and the result of those submatchers contributes to whether or not the matcher matches. If one of those submatchers fails, the inclusion matcher immediately fails and spits out the failure message associated with that submatcher. However, there is a fundamental difference between AllowValueMatcher and DisallowValueMatcher as it relates to how they function: * AllowValueMatcher sets an attribute to a value on a record and expects the record not to fail validation. * DisallowValueMatcher sets an attribute to a value on a record, but expects the record *to* fail validation. The issue in this case is that, because AllowValueMatcher was used instead of DisallowValueMatcher, the inclusion matcher thought that the AllowValueMatcher failed, when in fact it passed (this result was just inverted). So it tried to generate a failure message for a matcher that didn't fail in the first place. By using DisallowValueMatcher, we set the proper expectations. --- NEWS.md | 6 ++++++ .../validate_inclusion_of_matcher.rb | 4 ++-- .../validate_inclusion_of_matcher_spec.rb | 18 +++++++++++++++--- spec/unit_spec_helper.rb | 3 +++ 4 files changed, 26 insertions(+), 5 deletions(-) diff --git a/NEWS.md b/NEWS.md index d38ef1b9a..0ee30ab99 100644 --- a/NEWS.md +++ b/NEWS.md @@ -70,6 +70,11 @@ is now: * *PR: [#1073]* * *Original issue: [#949]* +* Fix `validate_inclusion_of` so that if it fails, it will no longer blow up + with the error "undefined method \`attribute_setter' for nil:NilClass". + + * *Original issue: [#904]* + ### Features * Add `required` and `optional` qualifiers to `belong_to` and `have_one` @@ -143,6 +148,7 @@ is now: [#961]: https://github.com/thoughtbot/shoulda-matchers/issues/961 [795ca68]: https://github.com/thoughtbot/shoulda-matchers/commit/795ca688bff08590dbd2ab6f2b51ea415e0c7473 [#1089]: https://github.com/thoughtbot/shoulda-matchers/pulls/1089 +[#904]: https://github.com/thoughtbot/shoulda-matchers/issues/904 ### Improvements diff --git a/lib/shoulda/matchers/active_model/validate_inclusion_of_matcher.rb b/lib/shoulda/matchers/active_model/validate_inclusion_of_matcher.rb index f152cb540..6997f950d 100644 --- a/lib/shoulda/matchers/active_model/validate_inclusion_of_matcher.rb +++ b/lib/shoulda/matchers/active_model/validate_inclusion_of_matcher.rb @@ -465,8 +465,8 @@ def disallows_value_outside_of_array? end end - !values_outside_of_array.any? do |value| - allows_value_of(value, @low_message) + values_outside_of_array.all? do |value| + disallows_value_of(value, @low_message) end end 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 a5efb15e5..b47f8ec53 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 @@ -448,9 +448,21 @@ def configure_validation_matcher(matcher) define_method(:valid_values) { args.fetch(:possible_values) } - it 'does not match a record with no validations' do - builder = build_object - expect_not_to_match_on_values(builder, possible_values) + context 'when the record has no validations' do + it 'passes when used in the negative' do + builder = build_object + expect_not_to_match_on_values(builder, possible_values) + end + + it 'fails when used in the positive with an appropriate failure message' do + builder = build_object + + assertion = lambda do + expect_to_match_on_values(builder, possible_values) + end + + expect(&assertion).to fail + end end it 'matches given the same array of valid values' do diff --git a/spec/unit_spec_helper.rb b/spec/unit_spec_helper.rb index 26c832ad3..7bc23e6ca 100644 --- a/spec/unit_spec_helper.rb +++ b/spec/unit_spec_helper.rb @@ -1,6 +1,7 @@ require_relative 'support/unit/load_environment' require 'rspec/rails' +require 'rspec/matchers/fail_matchers' require 'shoulda-matchers' require 'spec_helper' @@ -10,6 +11,8 @@ end RSpec.configure do |config| + config.include RSpec::Matchers::FailMatchers + UnitTests::ActionPackVersions.configure_example_group(config) UnitTests::ActiveModelHelpers.configure_example_group(config) UnitTests::ActiveModelVersions.configure_example_group(config)