From 1e6e0a7aff34c25bb1fc2aab2df683a23e772fa3 Mon Sep 17 00:00:00 2001 From: Gaspard Bucher Date: Tue, 10 Feb 2009 17:43:51 +0100 Subject: [PATCH] Autosave should check the validity of the associations even if the master record is not valid so that the end user has all errors (consistent with has_many et al. validations). --- .../lib/active_record/autosave_association.rb | 21 ++++++++----------- .../test/cases/autosave_association_test.rb | 14 +++++++++++++ 2 files changed, 23 insertions(+), 12 deletions(-) diff --git a/activerecord/lib/active_record/autosave_association.rb b/activerecord/lib/active_record/autosave_association.rb index 680b415..75aea87 100644 --- a/activerecord/lib/active_record/autosave_association.rb +++ b/activerecord/lib/active_record/autosave_association.rb @@ -175,21 +175,18 @@ module ActiveRecord # Returns whether or not the parent, self, and any loaded autosave associations are valid. def valid_with_autosave_associations? - if valid_without_autosave_associations? - self.class.reflect_on_all_autosave_associations.all? do |reflection| - if (association = association_instance_get(reflection.name)) && association.loaded? - if association.is_a?(Array) - association.proxy_target.all? { |child| autosave_association_valid?(reflection, child) } - else - autosave_association_valid?(reflection, association) - end + record_valid = valid_without_autosave_associations? + self.class.reflect_on_all_autosave_associations.all? do |reflection| + if (association = association_instance_get(reflection.name)) && association.loaded? + if association.is_a?(Array) + association.proxy_target.all? { |child| autosave_association_valid?(reflection, child) } else - true # association not loaded yet, so it should be valid + autosave_association_valid?(reflection, association) end + else + true # association not loaded yet, so it should be valid end - else - false # self was not valid - end + end && record_valid end # Returns whether or not the association is valid and applies any errors to the parent, self, if it wasn't. diff --git a/activerecord/test/cases/autosave_association_test.rb b/activerecord/test/cases/autosave_association_test.rb index 381249c..3d64727 100644 --- a/activerecord/test/cases/autosave_association_test.rb +++ b/activerecord/test/cases/autosave_association_test.rb @@ -228,6 +228,20 @@ class TestAutosaveAssociationOnAHasOneAssociation < ActiveRecord::TestCase def test_should_not_load_the_associated_model assert_queries(1) { @pirate.catchphrase = 'Arr'; @pirate.save! } end + + def test_should_merge_errors_in_the_associated_model + @pirate.ship.name = nil + assert !@pirate.save + assert_equal "can't be blank", @pirate.errors['ship_name'] + end + + def test_should_merge_errors_in_the_associated_model_even_if_master_is_not_valid + @pirate.ship.name = nil + @pirate.catchphrase = nil + assert !@pirate.save + assert_equal "can't be blank", @pirate.errors['ship_name'] + assert_equal "can't be blank", @pirate.errors['catchphrase'] + end end class TestAutosaveAssociationOnABelongsToAssociation < ActiveRecord::TestCase -- 1.6.0 From 1d0b766835b948ce0fd1aaa27f19e08ae77aae30 Mon Sep 17 00:00:00 2001 From: Gaspard Bucher Date: Wed, 11 Feb 2009 12:39:32 +0100 Subject: [PATCH] Better validations for autosave (replaces association validations instead of patching 'save'). --- activerecord/lib/active_record/associations.rb | 15 ++-- .../lib/active_record/autosave_association.rb | 78 ++++++++++++++----- .../lib/active_record/nested_attributes.rb | 2 +- .../test/cases/autosave_association_test.rb | 44 +++++++---- 4 files changed, 94 insertions(+), 45 deletions(-) diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index 05ce8ff..919a3b7 100755 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -790,7 +790,7 @@ module ActiveRecord configure_dependency_for_has_many(reflection) - add_multiple_associated_validation_callbacks(reflection.name) unless options[:validate] == false + add_multiple_associated_validation_callbacks(reflection) unless options[:validate] == false add_multiple_associated_save_callbacks(reflection.name) add_association_callbacks(reflection.name, reflection.options) @@ -913,7 +913,7 @@ module ActiveRecord end after_save method_name - add_single_associated_validation_callbacks(reflection.name) if options[:validate] == true + add_single_associated_validation_callbacks(reflection) if options[:validate] == true || options[:autosave] == true association_accessor_methods(reflection, HasOneAssociation) association_constructor_method(:build, reflection, HasOneAssociation) association_constructor_method(:create, reflection, HasOneAssociation) @@ -1074,7 +1074,7 @@ module ActiveRecord ) end - add_single_associated_validation_callbacks(reflection.name) if options[:validate] == true + add_single_associated_validation_callbacks(reflection) if options[:validate] == true || options[:autosave] == true configure_dependency_for_belongs_to(reflection) end @@ -1242,7 +1242,7 @@ module ActiveRecord def has_and_belongs_to_many(association_id, options = {}, &extension) reflection = create_has_and_belongs_to_many_reflection(association_id, options, &extension) - add_multiple_associated_validation_callbacks(reflection.name) unless options[:validate] == false + add_multiple_associated_validation_callbacks(reflection) unless options[:validate] == false add_multiple_associated_save_callbacks(reflection.name) collection_accessor_methods(reflection, HasAndBelongsToManyAssociation) @@ -1365,7 +1365,8 @@ module ActiveRecord end end - def add_single_associated_validation_callbacks(association_name) + def add_single_associated_validation_callbacks(reflection) + association_name = reflection.name method_name = "validate_associated_records_for_#{association_name}".to_sym define_method(method_name) do if association = association_instance_get(association_name) @@ -1376,11 +1377,11 @@ module ActiveRecord validate method_name end - def add_multiple_associated_validation_callbacks(association_name) + def add_multiple_associated_validation_callbacks(reflection) + association_name = reflection.name method_name = "validate_associated_records_for_#{association_name}".to_sym define_method(method_name) do association = association_instance_get(association_name) - if association if new_record? association diff --git a/activerecord/lib/active_record/autosave_association.rb b/activerecord/lib/active_record/autosave_association.rb index 75aea87..4dacd2d 100644 --- a/activerecord/lib/active_record/autosave_association.rb +++ b/activerecord/lib/active_record/autosave_association.rb @@ -130,14 +130,66 @@ module ActiveRecord alias_method_chain :reload, :autosave_associations alias_method_chain :save, :autosave_associations alias_method_chain :save!, :autosave_associations - alias_method_chain :valid?, :autosave_associations %w{ has_one belongs_to has_many has_and_belongs_to_many }.each do |type| base.send("valid_keys_for_#{type}_association") << :autosave end + base.extend(ClassMethods) end end + + module ClassMethods + def add_single_associated_validation_callbacks(reflection) + association_name = reflection.name + method_name = "validate_associated_records_for_#{association_name}".to_sym + define_method(method_name) do + if (association = association_instance_get(association_name)) && !association.target.nil? + returning(association.valid?) do |valid| + if reflection.options[:autosave] + association.errors.each do |attribute, message| + errors.add "#{reflection.name}_#{attribute}", message + end + else + errors.add association_name + end unless valid + end + end + end + validate method_name + end + + def add_multiple_associated_validation_callbacks(reflection) + association_name = reflection.name + method_name = "validate_associated_records_for_#{association_name}".to_sym + define_method(method_name) do + autosave = reflection.options[:autosave] + association = association_instance_get(association_name) + if association + if new_record? + association + elsif association.loaded? + autosave ? association : association.select { |record| record.new_record? } + else + autosave ? (association.target || []) : association.target.select { |record| record.new_record? } + end.each do |record| + returning(record.valid?) do |valid| + if autosave + record.errors.each do |attribute, message| + error_name = "#{reflection.name}_#{attribute}" + errors.add error_name, message unless errors.on(error_name) + end + else + errors.add association_name + end unless valid + end + end + end + end + + validate method_name + end + end # Saves the parent, self, and any loaded autosave associations. # In addition, it destroys all children that were marked for destruction # with mark_for_destruction. @@ -151,10 +203,10 @@ module ActiveRecord if (association = association_instance_get(reflection.name)) && association.loaded? if association.is_a?(Array) association.proxy_target.each do |child| - child.marked_for_destruction? ? child.destroy : child.save(perform_validation) + child.marked_for_destruction? ? child.destroy : child.save(false) end else - association.marked_for_destruction? ? association.destroy : association.save(perform_validation) + association.marked_for_destruction? ? association.destroy : association.save(false) end end end @@ -166,29 +218,13 @@ module ActiveRecord # will raise a RecordInvalid exception instead of returning false if the # record is not valid. def save_with_autosave_associations! - if valid_with_autosave_associations? + if valid? save_with_autosave_associations(false) || raise(RecordNotSaved) else raise RecordInvalid.new(self) end end - - # Returns whether or not the parent, self, and any loaded autosave associations are valid. - def valid_with_autosave_associations? - record_valid = valid_without_autosave_associations? - self.class.reflect_on_all_autosave_associations.all? do |reflection| - if (association = association_instance_get(reflection.name)) && association.loaded? - if association.is_a?(Array) - association.proxy_target.all? { |child| autosave_association_valid?(reflection, child) } - else - autosave_association_valid?(reflection, association) - end - else - true # association not loaded yet, so it should be valid - end - end && record_valid - end - + # Returns whether or not the association is valid and applies any errors to the parent, self, if it wasn't. def autosave_association_valid?(reflection, association) returning(association.valid?) do |valid| diff --git a/activerecord/lib/active_record/nested_attributes.rb b/activerecord/lib/active_record/nested_attributes.rb index 5778223..55ec0dc 100644 --- a/activerecord/lib/active_record/nested_attributes.rb +++ b/activerecord/lib/active_record/nested_attributes.rb @@ -175,7 +175,7 @@ module ActiveRecord :collection end - reflection.options[:autosave] = true + send(reflection.macro, reflection.name, reflection.options.merge(:autosave => true)) self.reject_new_nested_attributes_procs[association_name.to_sym] = options[:reject_if] # def pirate_attributes=(attributes) diff --git a/activerecord/test/cases/autosave_association_test.rb b/activerecord/test/cases/autosave_association_test.rb index 3d64727..c2054fe 100644 --- a/activerecord/test/cases/autosave_association_test.rb +++ b/activerecord/test/cases/autosave_association_test.rb @@ -180,7 +180,15 @@ class TestAutosaveAssociationOnAHasOneAssociation < ActiveRecord::TestCase assert !@pirate.valid? assert !@pirate.errors.on(:ship_name).blank? end - + + def test_should_merge_errors_in_the_associated_model_even_if_master_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? + end + def test_should_still_allow_to_bypass_validations_on_the_associated_model @pirate.catchphrase = '' @pirate.ship.name = '' @@ -228,20 +236,6 @@ class TestAutosaveAssociationOnAHasOneAssociation < ActiveRecord::TestCase def test_should_not_load_the_associated_model assert_queries(1) { @pirate.catchphrase = 'Arr'; @pirate.save! } end - - def test_should_merge_errors_in_the_associated_model - @pirate.ship.name = nil - assert !@pirate.save - assert_equal "can't be blank", @pirate.errors['ship_name'] - end - - def test_should_merge_errors_in_the_associated_model_even_if_master_is_not_valid - @pirate.ship.name = nil - @pirate.catchphrase = nil - assert !@pirate.save - assert_equal "can't be blank", @pirate.errors['ship_name'] - assert_equal "can't be blank", @pirate.errors['catchphrase'] - end end class TestAutosaveAssociationOnABelongsToAssociation < ActiveRecord::TestCase @@ -339,7 +333,25 @@ module AutosaveAssociationOnACollectionAssociationTests assert_equal "can't be blank", @pirate.errors.on("#{@association_name}_name") assert @pirate.errors.on(@association_name).blank? end - + + def test_should_not_use_default_invalid_error_on_associated_models + # default association validations only run on new associated records + @pirate.send(@association_name).build(:name => '') + + assert !@pirate.valid? + assert_equal "can't be blank", @pirate.errors.on("#{@association_name}_name") + assert @pirate.errors.on(@association_name).blank? + end + + def test_should_merge_errors_in_the_associated_model_even_if_master_is_not_valid + @pirate.send(@association_name).each { |child| child.name = '' } + @pirate.catchphrase = nil + + assert !@pirate.valid? + assert_equal "can't be blank", @pirate.errors.on("#{@association_name}_name") + assert !@pirate.errors.on(:catchphrase).blank? + end + def test_should_still_allow_to_bypass_validations_on_the_associated_models @pirate.catchphrase = '' @pirate.send(@association_name).each { |child| child.name = '' } -- 1.6.0