Raise CouldNotSetPasswordError more safely

It seems that adding a restriction to allow_value to raise
CouldNotClearAttribute if nil cannot be set on the attribute in question
broke a lot of people's tests. Really the only reason we added this was
for validate_presence_of -- it rescued CouldNotClearAttribute and
re-raised it as CouldNotSetPasswordError, but only for passwords and
only if has_secure_password was being used. So, I've figured out another
way of performing this check inside of validate_presence_of only to
prevent tests that have nothing to do with from breaking unnecessarily.

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

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?

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

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)

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?
             []

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)

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