diff --git a/NEWS.md b/NEWS.md index 37ca3bc4..c9591792 100644 --- a/NEWS.md +++ b/NEWS.md @@ -6,6 +6,11 @@ * Fix so that ActiveRecord matchers aren't included when ActiveRecord isn't defined (i.e. if you are using ActiveModel only). +* Revert the behavior of `allow_value` changed in 2.6.0 (it will no longer raise + CouldNotClearAttribute). This was originally done as a part of a fix for + `validate_presence_of` when used in conjunction with `has_secure_password`. + That fix has been updated so that it does not affect `allow_value`. + # 2.6.0 * The boolean argument to `have_db_index`'s `unique` option is now optional, for diff --git a/lib/shoulda/matchers/active_model/allow_value_matcher.rb b/lib/shoulda/matchers/active_model/allow_value_matcher.rb index 8f19c6e0..89a32ca7 100644 --- a/lib/shoulda/matchers/active_model/allow_value_matcher.rb +++ b/lib/shoulda/matchers/active_model/allow_value_matcher.rb @@ -37,6 +37,7 @@ module Shoulda # :nodoc: self.values_to_match = values self.message_finder_factory = ValidationMessageFinder self.options = {} + self.after_setting_value_callback = -> {} end def for(attribute) @@ -63,12 +64,16 @@ module Shoulda # :nodoc: self end + def _after_setting_value(&callback) # :nodoc: + self.after_setting_value_callback = callback + end + def matches?(instance) self.instance = instance values_to_match.none? do |value| self.value = value - set_and_double_check_attribute!(attribute_to_set, value) + set_value(value) errors_match? end end @@ -91,24 +96,11 @@ module Shoulda # :nodoc: attr_accessor :values_to_match, :message_finder_factory, :instance, :attribute_to_set, :attribute_to_check_message_against, - :context, :value, :matched_error + :context, :value, :matched_error, :after_setting_value_callback - def set_and_double_check_attribute!(attribute_name, value) - instance.__send__("#{attribute_name}=", value) - - if value.nil? - ensure_attribute_was_cleared!(attribute_name) - end - end - - def ensure_attribute_was_cleared!(attribute_name) - if instance.respond_to?(attribute_name) - actual_value = instance.__send__(attribute_name) - - if !actual_value.nil? - raise Shoulda::Matchers::ActiveModel::CouldNotClearAttribute.create(actual_value) - end - end + def set_value(value) + instance.__send__("#{attribute_to_set}=", value) + after_setting_value_callback.call end def errors_match? diff --git a/lib/shoulda/matchers/active_model/disallow_value_matcher.rb b/lib/shoulda/matchers/active_model/disallow_value_matcher.rb index 71480672..b8bcc407 100644 --- a/lib/shoulda/matchers/active_model/disallow_value_matcher.rb +++ b/lib/shoulda/matchers/active_model/disallow_value_matcher.rb @@ -1,7 +1,13 @@ +require 'forwardable' + module Shoulda # :nodoc: module Matchers module ActiveModel # :nodoc: class DisallowValueMatcher # :nodoc: + extend Forwardable + + def_delegators :allow_matcher, :_after_setting_value + def initialize(value) @allow_matcher = AllowValueMatcher.new(value) end @@ -39,6 +45,10 @@ module Shoulda # :nodoc: @allow_matcher.strict self end + + private + + attr_reader :allow_matcher end end end diff --git a/lib/shoulda/matchers/active_model/errors.rb b/lib/shoulda/matchers/active_model/errors.rb index 5ca447f0..ba6f0f8a 100644 --- a/lib/shoulda/matchers/active_model/errors.rb +++ b/lib/shoulda/matchers/active_model/errors.rb @@ -5,18 +5,6 @@ module Shoulda # :nodoc: class NonNullableBooleanError < Shoulda::Matchers::Error; end - class CouldNotClearAttribute < Shoulda::Matchers::Error - def self.create(actual_value) - super(actual_value: actual_value) - end - - attr_accessor :actual_value - - def message - "Expected value to be nil, but was #{actual_value.inspect}." - end - end - class CouldNotSetPasswordError < Shoulda::Matchers::Error def self.create(model) super(model: model) diff --git a/lib/shoulda/matchers/active_model/validate_presence_of_matcher.rb b/lib/shoulda/matchers/active_model/validate_presence_of_matcher.rb index 61ba5241..200e1392 100644 --- a/lib/shoulda/matchers/active_model/validate_presence_of_matcher.rb +++ b/lib/shoulda/matchers/active_model/validate_presence_of_matcher.rb @@ -28,12 +28,11 @@ module Shoulda # :nodoc: def matches?(subject) super(subject) @expected_message ||= :blank - disallows_value_of(blank_value, @expected_message) - rescue Shoulda::Matchers::ActiveModel::CouldNotClearAttribute => error - if @attribute == :password - raise Shoulda::Matchers::ActiveModel::CouldNotSetPasswordError.create(subject.class) + + if secure_password_being_validated? + disallows_and_double_checks_value_of!(blank_value, @expected_message) else - raise error + disallows_value_of(blank_value, @expected_message) end end @@ -43,6 +42,26 @@ module Shoulda # :nodoc: private + def secure_password_being_validated? + defined?(::ActiveModel::SecurePassword) && + @subject.class.ancestors.include?(::ActiveModel::SecurePassword::InstanceMethodsOnActivation) && + @attribute == :password + end + + def disallows_and_double_checks_value_of!(value, message) + error_class = Shoulda::Matchers::ActiveModel::CouldNotSetPasswordError + + disallows_value_of(value, message) do |matcher| + matcher._after_setting_value do + actual_value = @subject.__send__(@attribute) + + if !actual_value.nil? + raise error_class.create(@subject.class) + end + end + end + end + def blank_value if collection? [] diff --git a/spec/shoulda/matchers/active_model/allow_value_matcher_spec.rb b/spec/shoulda/matchers/active_model/allow_value_matcher_spec.rb index ba4381d1..e3f6254a 100644 --- a/spec/shoulda/matchers/active_model/allow_value_matcher_spec.rb +++ b/spec/shoulda/matchers/active_model/allow_value_matcher_spec.rb @@ -24,6 +24,20 @@ describe Shoulda::Matchers::ActiveModel::AllowValueMatcher do end end + describe '#_after_setting_value' do + it 'sets a block which is yielded after each value is set on the attribute' do + attribute = :attr + record = define_model(:example, attribute => :string).new + matcher = described_class.new('a', 'b', 'c').for(attribute) + call_count = 0 + + matcher._after_setting_value { call_count += 1 } + matcher.matches?(record) + + expect(call_count).to eq 3 + end + end + context 'an attribute with a validation' do it 'allows a good value' do expect(validating_format(with: /abc/)).to allow_value('abcde').for(:attr) diff --git a/spec/shoulda/matchers/active_model/validate_presence_of_matcher_spec.rb b/spec/shoulda/matchers/active_model/validate_presence_of_matcher_spec.rb index ae586287..16cdcf3d 100644 --- a/spec/shoulda/matchers/active_model/validate_presence_of_matcher_spec.rb +++ b/spec/shoulda/matchers/active_model/validate_presence_of_matcher_spec.rb @@ -146,9 +146,9 @@ describe Shoulda::Matchers::ActiveModel::ValidatePresenceOfMatcher do if rails_4_x? context 'against a pre-set password in a model that has_secure_password' do - it 'raises an error to instruct the user' do + it 'raises a CouldNotSetPasswordError exception' do user_class = define_model :user, password_digest: :string do - has_secure_password + has_secure_password validations: false validates_presence_of :password end @@ -156,9 +156,28 @@ describe Shoulda::Matchers::ActiveModel::ValidatePresenceOfMatcher do user.password = 'something' error_class = Shoulda::Matchers::ActiveModel::CouldNotSetPasswordError - expect { + expect do expect(user).to validate_presence_of(:password) - }.to raise_error(error_class) + end.to raise_error(error_class) + end + end + end + + context 'when the attribute being tested intercepts the blank value we set on it (issue #479)' do + context 'for a non-collection attribute' do + it 'does not raise an error' do + record = define_model :example, attr: :string do + validates :attr, presence: true + + def attr=(value) + value = '' if value.nil? + super(value) + end + end.new + + expect do + expect(record).to validate_presence_of(:attr) + end.not_to raise_error end end end