Fix nested attributes error messages which is broken in 2.3.4. It sti…

…ll copies the message from child to parent, but does the lookup in the child, not in the parent, avoiding error messages duplication (as happened in 2.3.3). [#3147 state:resolved]

Signed-off-by: Joshua Peek <josh@joshpeek.com>

activerecord/lib/active_record/autosave_association.rb

@@ -256,9 +256,8 @@ def association_valid?(reflection, association)
       unless valid = association.valid?
         if reflection.options[:autosave]
           association.errors.each_error do |attribute, error|
-            error = error.dup
-            error.attribute = "#{reflection.name}_#{attribute}"
-            errors.add(error) unless errors.on(error.attribute)
+            attribute = "#{reflection.name}.#{attribute}"
+            errors.add(attribute, error.dup) unless errors.on(attribute)
           end
         else
           errors.add(reflection.name)
@@ -362,4 +361,4 @@ def save_belongs_to_association(reflection)
       end
     end
   end
-end
+end

activerecord/lib/active_record/validations.rb

@@ -22,16 +22,21 @@ def initialize(base, attribute, type = nil, options = {})
       self.base      = base
       self.attribute = attribute
       self.type      = type || :invalid
-      self.options   = options
       self.message   = options.delete(:message) || self.type
+      self.options   = {
+        :scope => [:activerecord, :errors],
+        :model => @base.class.human_name,
+        :attribute => @base.class.human_attribute_name(attribute.to_s),
+        :value => value
+      }.merge!(options)
     end
 
     def message
-      generate_message(@message, default_options)
+      generate_message(@message, options.dup)
     end
 
     def full_message
-      attribute.to_s == 'base' ? message : generate_full_message(message, default_options)
+      attribute.to_s == 'base' ? message : generate_full_message(message, options.dup)
     end
 
     alias :to_s :message
@@ -113,15 +118,6 @@ def generate_full_message(message, options = {})
         options.merge!(:default => keys, :message => self.message)
         I18n.translate(keys.shift, options)
       end
-
-      # Return user options with default options.
-      #
-      def default_options
-        options.reverse_merge :scope => [:activerecord, :errors],
-                              :model => @base.class.human_name,
-                              :attribute => @base.class.human_attribute_name(attribute.to_s),
-                              :value => value
-      end
   end
 
   # Active Record validation is reported to and from this object, which is used by Base#save to
@@ -154,16 +150,10 @@ def add_to_base(msg)
     # error can be added to the same +attribute+ in which case an array will be returned on a call to <tt>on(attribute)</tt>.
     # If no +messsage+ is supplied, :invalid is assumed.
     # If +message+ is a Symbol, it will be translated, using the appropriate scope (see translate_error).
-    # def add(attribute, message = nil, options = {})
-    #   message ||= :invalid
-    #   message = generate_message(attribute, message, options)) if message.is_a?(Symbol)
-    #   @errors[attribute.to_s] ||= []
-    #   @errors[attribute.to_s] << message
-    # end
-
-    def add(error_or_attr, message = nil, options = {})
-      error, attribute = error_or_attr.is_a?(Error) ? [error_or_attr, error_or_attr.attribute] : [nil, error_or_attr]
+    #
+    def add(attribute, message = nil, options = {})
       options[:message] = options.delete(:default) if options.has_key?(:default)
+      error, message = message, nil if message.is_a?(Error)
 
       @errors[attribute.to_s] ||= []
       @errors[attribute.to_s] << (error || Error.new(@base, attribute, message, options))

activerecord/test/cases/autosave_association_test.rb

@@ -750,15 +750,15 @@ def test_should_automatically_save_bang_the_associated_model
   def test_should_automatically_validate_the_associated_model
     @pirate.ship.name = ''
     assert !@pirate.valid?
-    assert !@pirate.errors.on(:ship_name).blank?
+    assert_equal "can't be blank", @pirate.errors.on(:"ship.name")
   end
 
   def test_should_merge_errors_on_the_associated_models_onto_the_parent_even_if_it_is_not_valid
     @pirate.ship.name   = nil
     @pirate.catchphrase = nil
     assert !@pirate.valid?
-    assert !@pirate.errors.on(:ship_name).blank?
-    assert !@pirate.errors.on(:catchphrase).blank?
+    assert @pirate.errors.full_messages.include?("Name can't be blank")
+    assert @pirate.errors.full_messages.include?("Catchphrase can't be blank")
   end
 
   def test_should_still_allow_to_bypass_validations_on_the_associated_model
@@ -840,15 +840,15 @@ def test_should_automatically_save_bang_the_associated_model
   def test_should_automatically_validate_the_associated_model
     @ship.pirate.catchphrase = ''
     assert !@ship.valid?
-    assert !@ship.errors.on(:pirate_catchphrase).blank?
+    assert_equal "can't be blank", @ship.errors.on(:"pirate.catchphrase")
   end
 
   def test_should_merge_errors_on_the_associated_model_onto_the_parent_even_if_it_is_not_valid
     @ship.name = nil
     @ship.pirate.catchphrase = nil
     assert !@ship.valid?
-    assert !@ship.errors.on(:name).blank?
-    assert !@ship.errors.on(:pirate_catchphrase).blank?
+    assert @ship.errors.full_messages.include?("Name can't be blank")
+    assert @ship.errors.full_messages.include?("Catchphrase can't be blank")
   end
 
   def test_should_still_allow_to_bypass_validations_on_the_associated_model
@@ -910,15 +910,15 @@ def test_should_automatically_validate_the_associated_models
     @pirate.send(@association_name).each { |child| child.name = '' }
 
     assert !@pirate.valid?
-    assert_equal "can't be blank", @pirate.errors.on("#{@association_name}_name")
+    assert @pirate.errors.full_messages.include?("Name can't be blank")
     assert @pirate.errors.on(@association_name).blank?
   end
 
   def test_should_not_use_default_invalid_error_on_associated_models
     @pirate.send(@association_name).build(:name => '')
 
     assert !@pirate.valid?
-    assert_equal "can't be blank", @pirate.errors.on("#{@association_name}_name")
+    assert_equal "can't be blank", @pirate.errors.on("#{@association_name}.name")
     assert @pirate.errors.on(@association_name).blank?
   end
 
@@ -927,7 +927,7 @@ def test_should_merge_errors_on_the_associated_models_onto_the_parent_even_if_it
     @pirate.catchphrase = nil
 
     assert !@pirate.valid?
-    assert_equal "can't be blank", @pirate.errors.on("#{@association_name}_name")
+    assert_equal "can't be blank", @pirate.errors.on("#{@association_name}.name")
     assert !@pirate.errors.on(:catchphrase).blank?
   end