diff options
| -rw-r--r-- | actionpack/lib/action_view/helpers/form_helper.rb | 26 | ||||
| -rw-r--r-- | actionpack/test/template/form_helper_test.rb | 36 | ||||
| -rwxr-xr-x | activerecord/lib/active_record/associations.rb | 7 | ||||
| -rw-r--r-- | activerecord/lib/active_record/autosave_association.rb | 52 | ||||
| -rw-r--r-- | activerecord/lib/active_record/nested_attributes.rb | 60 | ||||
| -rw-r--r-- | activerecord/test/cases/associations/has_one_associations_test.rb | 9 | ||||
| -rw-r--r-- | activerecord/test/cases/autosave_association_test.rb | 221 | ||||
| -rw-r--r-- | activerecord/test/cases/nested_attributes_test.rb | 80 | ||||
| -rw-r--r-- | activerecord/test/cases/reflection_test.rb | 6 | ||||
| -rw-r--r-- | activerecord/test/fixtures/accounts.yml | 1 | ||||
| -rw-r--r-- | activerecord/test/models/company.rb | 6 | ||||
| -rw-r--r-- | activerecord/test/models/pirate.rb | 9 | ||||
| -rw-r--r-- | activerecord/test/models/ship.rb | 2 | ||||
| -rw-r--r-- | activerecord/test/models/treasure.rb | 2 | ||||
| -rw-r--r-- | activerecord/test/schema/schema.rb | 1 |
15 files changed, 424 insertions, 94 deletions
diff --git a/actionpack/lib/action_view/helpers/form_helper.rb b/actionpack/lib/action_view/helpers/form_helper.rb index 81029102b1..32b9c4a7dd 100644 --- a/actionpack/lib/action_view/helpers/form_helper.rb +++ b/actionpack/lib/action_view/helpers/form_helper.rb @@ -449,6 +449,15 @@ module ActionView # <% end %> # <% end %> # + # Or a collection to be used: + # + # <% form_for @person, :url => { :action => "update" } do |person_form| %> + # ... + # <% person_form.fields_for :projects, @active_projects do |project_fields| %> + # Name: <%= project_fields.text_field :name %> + # <% end %> + # <% end %> + # # When projects is already an association on Person you can use # +accepts_nested_attributes_for+ to define the writer method for you: # @@ -1037,18 +1046,21 @@ module ActionView def fields_for_with_nested_attributes(association_name, args, block) name = "#{object_name}[#{association_name}_attributes]" - association = @object.send(association_name) - explicit_object = args.first.to_model if args.first.respond_to?(:to_model) + association = args.first.to_model if args.first.respond_to?(:to_model) + + if association.respond_to?(:new_record?) + association = [association] if @object.send(association_name).is_a?(Array) + elsif !association.is_a?(Array) + association = @object.send(association_name) + end if association.is_a?(Array) - children = explicit_object ? [explicit_object] : association explicit_child_index = args.last[:child_index] if args.last.is_a?(Hash) - - children.map do |child| + association.map do |child| fields_for_nested_model("#{name}[#{explicit_child_index || nested_child_index(name)}]", child, args, block) end.join - else - fields_for_nested_model(name, explicit_object || association, args, block) + elsif association + fields_for_nested_model(name, association, args, block) end end diff --git a/actionpack/test/template/form_helper_test.rb b/actionpack/test/template/form_helper_test.rb index 8fd018f86d..be15b06372 100644 --- a/actionpack/test/template/form_helper_test.rb +++ b/actionpack/test/template/form_helper_test.rb @@ -784,6 +784,42 @@ class FormHelperTest < ActionView::TestCase assert_dom_equal expected, output_buffer end + def test_nested_fields_for_with_an_empty_supplied_attributes_collection + form_for(:post, @post) do |f| + concat f.text_field(:title) + f.fields_for(:comments, []) do |cf| + concat cf.text_field(:name) + end + end + + expected = '<form action="http://www.example.com" method="post">' + + '<input name="post[title]" size="30" type="text" id="post_title" value="Hello World" />' + + '</form>' + + assert_dom_equal expected, output_buffer + end + + def test_nested_fields_for_with_existing_records_on_a_supplied_nested_attributes_collection + @post.comments = Array.new(2) { |id| Comment.new(id + 1) } + + form_for(:post, @post) do |f| + concat f.text_field(:title) + f.fields_for(:comments, @post.comments) do |cf| + concat cf.text_field(:name) + end + end + + expected = '<form action="http://www.example.com" method="post">' + + '<input name="post[title]" size="30" type="text" id="post_title" value="Hello World" />' + + '<input id="post_comments_attributes_0_id" name="post[comments_attributes][0][id]" type="hidden" value="1" />' + + '<input id="post_comments_attributes_0_name" name="post[comments_attributes][0][name]" size="30" type="text" value="comment #1" />' + + '<input id="post_comments_attributes_1_id" name="post[comments_attributes][1][id]" type="hidden" value="2" />' + + '<input id="post_comments_attributes_1_name" name="post[comments_attributes][1][name]" size="30" type="text" value="comment #2" />' + + '</form>' + + assert_dom_equal expected, output_buffer + end + def test_nested_fields_for_on_a_nested_attributes_collection_association_yields_only_builder @post.comments = [Comment.new(321), Comment.new] yielded_comments = [] diff --git a/activerecord/lib/active_record/associations.rb b/activerecord/lib/active_record/associations.rb index 72061a1b31..266a52d612 100755 --- a/activerecord/lib/active_record/associations.rb +++ b/activerecord/lib/active_record/associations.rb @@ -280,9 +280,10 @@ module ActiveRecord # You can manipulate objects and associations before they are saved to the database, but there is some special behavior you should be # aware of, mostly involving the saving of associated objects. # - # Unless you enable the :autosave option on a <tt>has_one</tt>, <tt>belongs_to</tt>, - # <tt>has_many</tt>, or <tt>has_and_belongs_to_many</tt> association, - # in which case the members are always saved. + # Unless you set the :autosave option on a <tt>has_one</tt>, <tt>belongs_to</tt>, + # <tt>has_many</tt>, or <tt>has_and_belongs_to_many</tt> association. Setting it + # to +true+ will _always_ save the members, whereas setting it to +false+ will + # _never_ save the members. # # === One-to-one associations # diff --git a/activerecord/lib/active_record/autosave_association.rb b/activerecord/lib/active_record/autosave_association.rb index c1bc8423a9..8f37fcd515 100644 --- a/activerecord/lib/active_record/autosave_association.rb +++ b/activerecord/lib/active_record/autosave_association.rb @@ -158,7 +158,7 @@ module ActiveRecord def add_autosave_association_callbacks(reflection) save_method = "autosave_associated_records_for_#{reflection.name}" validation_method = "validate_associated_records_for_#{reflection.name}" - validate validation_method + force_validation = (reflection.options[:validate] == true || reflection.options[:autosave] == true) case reflection.macro when :has_many, :has_and_belongs_to_many @@ -169,7 +169,10 @@ module ActiveRecord after_create save_method after_update save_method - define_method(validation_method) { validate_collection_association(reflection) } + if force_validation || (reflection.macro == :has_many && reflection.options[:validate] != false) + define_method(validation_method) { validate_collection_association(reflection) } + validate validation_method + end else case reflection.macro when :has_one @@ -179,7 +182,11 @@ module ActiveRecord define_method(save_method) { save_belongs_to_association(reflection) } before_save save_method end - define_method(validation_method) { validate_single_association(reflection) } + + if force_validation + define_method(validation_method) { validate_single_association(reflection) } + validate validation_method + end end end end @@ -223,10 +230,8 @@ module ActiveRecord # Validate the association if <tt>:validate</tt> or <tt>:autosave</tt> is # turned on for the association specified by +reflection+. def validate_single_association(reflection) - if reflection.options[:validate] == true || reflection.options[:autosave] == true - if (association = association_instance_get(reflection.name)) && !association.target.nil? - association_valid?(reflection, association) - end + if (association = association_instance_get(reflection.name)) && !association.target.nil? + association_valid?(reflection, association) end end @@ -234,7 +239,7 @@ module ActiveRecord # <tt>:autosave</tt> is turned on for the association specified by # +reflection+. def validate_collection_association(reflection) - if reflection.options[:validate] != false && association = association_instance_get(reflection.name) + if association = association_instance_get(reflection.name) if records = associated_records_to_validate_or_save(association, new_record?, reflection.options[:autosave]) records.each { |record| association_valid?(reflection, record) } end @@ -243,15 +248,15 @@ module ActiveRecord # Returns whether or not the association is valid and applies any errors to # the parent, <tt>self</tt>, if it wasn't. Skips any <tt>:autosave</tt> - # enabled records if they're marked_for_destruction?. + # enabled records if they're marked_for_destruction? or destroyed. def association_valid?(reflection, association) + return true if association.destroyed? || association.marked_for_destruction? + unless valid = association.valid? if reflection.options[:autosave] - unless association.marked_for_destruction? - association.errors.each do |attribute, message| - attribute = "#{reflection.name}_#{attribute}" - errors[attribute] << message if errors[attribute].empty? - end + association.errors.each do |attribute, message| + attribute = "#{reflection.name}_#{attribute}" + errors[attribute] << message if errors[attribute].empty? end else errors.add(reflection.name) @@ -281,9 +286,11 @@ module ActiveRecord if records = associated_records_to_validate_or_save(association, @new_record_before_save, autosave) records.each do |record| + next if record.destroyed? + if autosave && record.marked_for_destruction? association.destroy(record) - elsif @new_record_before_save || record.new_record? + elsif autosave != false && (@new_record_before_save || record.new_record?) if autosave association.send(:insert_record, record, false, false) else @@ -309,14 +316,17 @@ module ActiveRecord # This all happens inside a transaction, _if_ the Transactions module is included into # ActiveRecord::Base after the AutosaveAssociation module, which it does by default. def save_has_one_association(reflection) - if (association = association_instance_get(reflection.name)) && !association.target.nil? + if (association = association_instance_get(reflection.name)) && !association.target.nil? && !association.destroyed? autosave = reflection.options[:autosave] if autosave && association.marked_for_destruction? association.destroy - elsif new_record? || association.new_record? || association[reflection.primary_key_name] != id || autosave - association[reflection.primary_key_name] = id - association.save(!autosave) + else + key = reflection.options[:primary_key] ? send(reflection.options[:primary_key]) : id + if autosave != false && (new_record? || association.new_record? || association[reflection.primary_key_name] != key || autosave) + association[reflection.primary_key_name] = key + association.save(!autosave) + end end end end @@ -330,12 +340,12 @@ module ActiveRecord # This all happens inside a transaction, _if_ the Transactions module is included into # ActiveRecord::Base after the AutosaveAssociation module, which it does by default. def save_belongs_to_association(reflection) - if association = association_instance_get(reflection.name) + if (association = association_instance_get(reflection.name)) && !association.destroyed? autosave = reflection.options[:autosave] if autosave && association.marked_for_destruction? association.destroy - else + elsif autosave != false association.save(!autosave) if association.new_record? || autosave if association.updated? diff --git a/activerecord/lib/active_record/nested_attributes.rb b/activerecord/lib/active_record/nested_attributes.rb index bc4cca7855..3c8140816c 100644 --- a/activerecord/lib/active_record/nested_attributes.rb +++ b/activerecord/lib/active_record/nested_attributes.rb @@ -66,10 +66,10 @@ module ActiveRecord # accepts_nested_attributes_for :avatar, :allow_destroy => true # end # - # Now, when you add the <tt>_delete</tt> key to the attributes hash, with a + # Now, when you add the <tt>_destroy</tt> key to the attributes hash, with a # value that evaluates to +true+, you will destroy the associated model: # - # member.avatar_attributes = { :id => '2', :_delete => '1' } + # member.avatar_attributes = { :id => '2', :_destroy => '1' } # member.avatar.marked_for_destruction? # => true # member.save # member.avatar #=> nil @@ -89,14 +89,14 @@ module ActiveRecord # the attribute hash. # # For each hash that does _not_ have an <tt>id</tt> key a new record will - # be instantiated, unless the hash also contains a <tt>_delete</tt> key + # be instantiated, unless the hash also contains a <tt>_destroy</tt> key # that evaluates to +true+. # # params = { :member => { # :name => 'joe', :posts_attributes => [ # { :title => 'Kari, the awesome Ruby documentation browser!' }, # { :title => 'The egalitarian assumption of the modern citizen' }, - # { :title => '', :_delete => '1' } # this will be ignored + # { :title => '', :_destroy => '1' } # this will be ignored # ] # }} # @@ -144,7 +144,7 @@ module ActiveRecord # By default the associated records are protected from being destroyed. If # you want to destroy any of the associated records through the attributes # hash, you have to enable it first using the <tt>:allow_destroy</tt> - # option. This will allow you to also use the <tt>_delete</tt> key to + # option. This will allow you to also use the <tt>_destroy</tt> key to # destroy existing records: # # class Member < ActiveRecord::Base @@ -153,7 +153,7 @@ module ActiveRecord # end # # params = { :member => { - # :posts_attributes => [{ :id => '2', :_delete => '1' }] + # :posts_attributes => [{ :id => '2', :_destroy => '1' }] # }} # # member.attributes = params['member'] @@ -176,14 +176,14 @@ module ActiveRecord # Supported options: # [:allow_destroy] # If true, destroys any members from the attributes hash with a - # <tt>_delete</tt> key and a value that evaluates to +true+ + # <tt>_destroy</tt> key and a value that evaluates to +true+ # (eg. 1, '1', true, or 'true'). This option is off by default. # [:reject_if] # Allows you to specify a Proc that checks whether a record should be # built for a certain attribute hash. The hash is passed to the Proc # and the Proc should return either +true+ or +false+. When no Proc # is specified a record will be built for all attribute hashes that - # do not have a <tt>_delete</tt> that evaluates to true. + # do not have a <tt>_destroy</tt> value that evaluates to true. # Passing <tt>:all_blank</tt> instead of a Proc will create a proc # that will reject a record where all the attributes are blank. # @@ -236,15 +236,25 @@ module ActiveRecord # destruction of this association. # # See ActionView::Helpers::FormHelper::fields_for for more info. - def _delete + def _destroy marked_for_destruction? end + # Deal with deprecated _delete. + # + def _delete #:nodoc: + ActiveSupport::Deprecation.warn "_delete is deprecated in nested attributes. Use _destroy instead." + _destroy + end + private # Attribute hash keys that should not be assigned as normal attributes. # These hash keys are nested attributes implementation details. - UNASSIGNABLE_KEYS = %w{ id _delete } + # + # TODO Remove _delete from UNASSIGNABLE_KEYS when deprecation warning are + # removed. + UNASSIGNABLE_KEYS = %w( id _destroy _delete ) # Assigns the given attributes to the association. # @@ -253,14 +263,19 @@ module ActiveRecord # record will be built. # # If the given attributes include a matching <tt>:id</tt> attribute _and_ a - # <tt>:_delete</tt> key set to a truthy value, then the existing record + # <tt>:_destroy</tt> key set to a truthy value, then the existing record # will be marked for destruction. def assign_nested_attributes_for_one_to_one_association(association_name, attributes, allow_destroy) attributes = attributes.stringify_keys if attributes['id'].blank? unless reject_new_record?(association_name, attributes) - send("build_#{association_name}", attributes.except(*UNASSIGNABLE_KEYS)) + method = "build_#{association_name}" + if respond_to?(method) + send(method, attributes.except(*UNASSIGNABLE_KEYS)) + else + raise ArgumentError, "Cannot build association #{association_name}. Are you trying to build a polymorphic one-to-one association?" + end end elsif (existing_record = send(association_name)) && existing_record.id.to_s == attributes['id'].to_s assign_to_or_mark_for_destruction(existing_record, attributes, allow_destroy) @@ -272,7 +287,7 @@ module ActiveRecord # Hashes with an <tt>:id</tt> value matching an existing associated record # will update that record. Hashes without an <tt>:id</tt> value will build # a new record for the association. Hashes with a matching <tt>:id</tt> - # value and a <tt>:_delete</tt> key set to a truthy value will mark the + # value and a <tt>:_destroy</tt> key set to a truthy value will mark the # matched record for destruction. # # For example: @@ -280,7 +295,7 @@ module ActiveRecord # assign_nested_attributes_for_collection_association(:people, { # '1' => { :id => '1', :name => 'Peter' }, # '2' => { :name => 'John' }, - # '3' => { :id => '2', :_delete => true } + # '3' => { :id => '2', :_destroy => true } # }) # # Will update the name of the Person with ID 1, build a new associated @@ -292,7 +307,7 @@ module ActiveRecord # assign_nested_attributes_for_collection_association(:people, [ # { :id => '1', :name => 'Peter' }, # { :name => 'John' }, - # { :id => '2', :_delete => true } + # { :id => '2', :_destroy => true } # ]) def assign_nested_attributes_for_collection_association(association_name, attributes_collection, allow_destroy) unless attributes_collection.is_a?(Hash) || attributes_collection.is_a?(Array) @@ -317,25 +332,26 @@ module ActiveRecord end # Updates a record with the +attributes+ or marks it for destruction if - # +allow_destroy+ is +true+ and has_delete_flag? returns +true+. + # +allow_destroy+ is +true+ and has_destroy_flag? returns +true+. def assign_to_or_mark_for_destruction(record, attributes, allow_destroy) - if has_delete_flag?(attributes) && allow_destroy + if has_destroy_flag?(attributes) && allow_destroy record.mark_for_destruction else record.attributes = attributes.except(*UNASSIGNABLE_KEYS) end end - # Determines if a hash contains a truthy _delete key. - def has_delete_flag?(hash) - ConnectionAdapters::Column.value_to_boolean hash['_delete'] + # Determines if a hash contains a truthy _destroy key. + def has_destroy_flag?(hash) + ConnectionAdapters::Column.value_to_boolean(hash['_destroy']) || + ConnectionAdapters::Column.value_to_boolean(hash['_delete']) # TODO Remove after deprecation. end # Determines if a new record should be build by checking for - # has_delete_flag? or if a <tt>:reject_if</tt> proc exists for this + # has_destroy_flag? or if a <tt>:reject_if</tt> proc exists for this # association and evaluates to +true+. def reject_new_record?(association_name, attributes) - has_delete_flag?(attributes) || + has_destroy_flag?(attributes) || self.class.reject_new_nested_attributes_procs[association_name].try(:call, attributes) end end diff --git a/activerecord/test/cases/associations/has_one_associations_test.rb b/activerecord/test/cases/associations/has_one_associations_test.rb index 7140de77ea..cdac86a3b9 100644 --- a/activerecord/test/cases/associations/has_one_associations_test.rb +++ b/activerecord/test/cases/associations/has_one_associations_test.rb @@ -36,6 +36,15 @@ class HasOneAssociationsTest < ActiveRecord::TestCase assert_equal accounts(:rails_core_account), firm.account_using_primary_key end + def test_update_with_foreign_and_primary_keys + firm = companies(:first_firm) + account = firm.account_using_foreign_and_primary_keys + assert_equal Account.find_by_firm_name(firm.name), account + firm.save + firm.reload + assert_equal account, firm.account_using_foreign_and_primary_keys + end + def test_can_marshal_has_one_association_with_nil_target firm = Firm.new assert_nothing_raised do diff --git a/activerecord/test/cases/autosave_association_test.rb b/activerecord/test/cases/autosave_association_test.rb index 271086af8e..9164701601 100644 --- a/activerecord/test/cases/autosave_association_test.rb +++ b/activerecord/test/cases/autosave_association_test.rb @@ -443,6 +443,70 @@ class TestDefaultAutosaveAssociationOnAHasManyAssociation < ActiveRecord::TestCa end end +class TestDefaultAutosaveAssociationOnNewRecord < ActiveRecord::TestCase + def test_autosave_new_record_on_belongs_to_can_be_disabled_per_relationship + new_account = Account.new("credit_limit" => 1000) + new_firm = Firm.new("name" => "some firm") + + assert new_firm.new_record? + new_account.firm = new_firm + new_account.save! + + assert !new_firm.new_record? + + new_account = Account.new("credit_limit" => 1000) + new_autosaved_firm = Firm.new("name" => "some firm") + + assert new_autosaved_firm.new_record? + new_account.unautosaved_firm = new_autosaved_firm + new_account.save! + + assert new_autosaved_firm.new_record? + end + + def test_autosave_new_record_on_has_one_can_be_disabled_per_relationship + firm = Firm.new("name" => "some firm") + account = Account.new("credit_limit" => 1000) + + assert account.new_record? + firm.account = account + firm.save! + + assert !account.new_record? + + firm = Firm.new("name" => "some firm") + account = Account.new("credit_limit" => 1000) + + firm.unautosaved_account = account + + assert account.new_record? + firm.unautosaved_account = account + firm.save! + + assert account.new_record? + end + + def test_autosave_new_record_on_has_many_can_be_disabled_per_relationship + firm = Firm.new("name" => "some firm") + account = Account.new("credit_limit" => 1000) + + assert account.new_record? + firm.accounts << account + + firm.save! + assert !account.new_record? + + firm = Firm.new("name" => "some firm") + account = Account.new("credit_limit" => 1000) + + assert account.new_record? + firm.unautosaved_accounts << account + + firm.save! + assert account.new_record? + end +end + class TestDestroyAsPartOfAutosaveAssociation < ActiveRecord::TestCase self.use_transactional_fixtures = false @@ -480,9 +544,17 @@ class TestDestroyAsPartOfAutosaveAssociation < ActiveRecord::TestCase assert !@pirate.valid? @pirate.ship.mark_for_destruction + @pirate.ship.expects(:valid?).never assert_difference('Ship.count', -1) { @pirate.save! } end + def test_a_child_marked_for_destruction_should_not_be_destroyed_twice + @pirate.ship.mark_for_destruction + assert @pirate.save + @pirate.ship.expects(:destroy).never + assert @pirate.save + end + def test_should_rollback_destructions_if_an_exception_occurred_while_saving_a_child # Stub the save method of the @pirate.ship instance to destroy and then raise an exception class << @pirate.ship @@ -517,9 +589,17 @@ class TestDestroyAsPartOfAutosaveAssociation < ActiveRecord::TestCase assert !@ship.valid? @ship.pirate.mark_for_destruction + @ship.pirate.expects(:valid?).never assert_difference('Pirate.count', -1) { @ship.save! } end + def test_a_parent_marked_for_destruction_should_not_be_destroyed_twice + @ship.pirate.mark_for_destruction + assert @ship.save + @ship.pirate.expects(:destroy).never + assert @ship.save + end + def test_should_rollback_destructions_if_an_exception_occurred_while_saving_a_parent # Stub the save method of the @ship.pirate instance to destroy and then raise an exception class << @ship.pirate @@ -560,9 +640,33 @@ class TestDestroyAsPartOfAutosaveAssociation < ActiveRecord::TestCase children.each { |child| child.name = '' } assert !@pirate.valid? - children.each { |child| child.mark_for_destruction } + children.each do |child| + child.mark_for_destruction + child.expects(:valid?).never + end assert_difference("#{association_name.classify}.count", -2) { @pirate.save! } end + + define_method("test_should_skip_validation_on_the_#{association_name}_association_if_destroyed") do + @pirate.send(association_name).create!(:name => "#{association_name}_1") + children = @pirate.send(association_name) + + children.each { |child| child.name = '' } + assert !@pirate.valid? + + children.each { |child| child.destroy } + assert @pirate.valid? + end + + define_method("test_a_child_marked_for_destruction_should_not_be_destroyed_twice_while_saving_#{association_name}") do + @pirate.send(association_name).create!(:name => "#{association_name}_1") + children = @pirate.send(association_name) + + children.each { |child| child.mark_for_destruction } + assert @pirate.save + children.each { |child| child.expects(:destroy).never } + assert @pirate.save + end define_method("test_should_rollback_destructions_if_an_exception_occurred_while_saving_#{association_name}") do 2.times { |i| @pirate.send(association_name).create!(:name => "#{association_name}_#{i}") } @@ -952,3 +1056,118 @@ class TestAutosaveAssociationOnAHasAndBelongsToManyAssociation < ActiveRecord::T include AutosaveAssociationOnACollectionAssociationTests end + +class TestAutosaveAssociationValidationsOnAHasManyAssocication < ActiveRecord::TestCase + self.use_transactional_fixtures = false + + def setup + @pirate = Pirate.create(:catchphrase => "Don' botharrr talkin' like one, savvy?") + @pirate.birds.create(:name => 'cookoo') + end + + test "should automatically validate associations" do + assert @pirate.valid? + @pirate.birds.each { |bird| bird.name = '' } + + assert !@pirate.valid? + end +end + +class TestAutosaveAssociationValidationsOnAHasOneAssocication < ActiveRecord::TestCase + self.use_transactional_fixtures = false + + def setup + @pirate = Pirate.create(:catchphrase => "Don' botharrr talkin' like one, savvy?") + @pirate.create_ship(:name => 'titanic') + end + + test "should automatically validate associations with :validate => true" do + assert @pirate.valid? + @pirate.ship.name = '' + assert !@pirate.valid? + end + + test "should not automatically validate associations without :validate => true" do + assert @pirate.valid? + @pirate.non_validated_ship.name = '' + assert @pirate.valid? + end +end + +class TestAutosaveAssociationValidationsOnABelongsToAssocication < ActiveRecord::TestCase + self.use_transactional_fixtures = false + + def setup + @pirate = Pirate.create(:catchphrase => "Don' botharrr talkin' like one, savvy?") + end + + test "should automatically validate associations with :validate => true" do + assert @pirate.valid? + @pirate.parrot = Parrot.new(:name => '') + assert !@pirate.valid? + end + + test "should not automatically validate associations without :validate => true" do + assert @pirate.valid? + @pirate.non_validated_parrot = Parrot.new(:name => '') + assert @pirate.valid? + end +end + +class TestAutosaveAssociationValidationsOnAHABTMAssocication < ActiveRecord::TestCase + self.use_transactional_fixtures = false + + def setup + @pirate = Pirate.create(:catchphrase => "Don' botharrr talkin' like one, savvy?") + end + + test "should automatically validate associations with :validate => true" do + assert @pirate.valid? + @pirate.parrots = [ Parrot.new(:name => 'popuga') ] + @pirate.parrots.each { |parrot| parrot.name = '' } + assert !@pirate.valid? + end + + test "should not automatically validate associations without :validate => true" do + assert @pirate.valid? + @pirate.non_validated_parrots = [ Parrot.new(:name => 'popuga') ] + @pirate.non_validated_parrots.each { |parrot| parrot.name = '' } + assert @pirate.valid? + end +end + +class TestAutosaveAssociationValidationMethodsGeneration < ActiveRecord::TestCase + self.use_transactional_fixtures = false + + def setup + @pirate = Pirate.new + end + + test "should generate validation methods for has_many associations" do + assert @pirate.respond_to?(:validate_associated_records_for_birds) + end + + test "should generate validation methods for has_one associations with :validate => true" do + assert @pirate.respond_to?(:validate_associated_records_for_ship) + end + + test "should not generate validation methods for has_one associations without :validate => true" do + assert !@pirate.respond_to?(:validate_associated_records_for_non_validated_ship) + end + + test "should generate validation methods for belongs_to associations with :validate => true" do + assert @pirate.respond_to?(:validate_associated_records_for_parrot) + end + + test "should not generate validation methods for belongs_to associations without :validate => true" do + assert !@pirate.respond_to?(:validate_associated_records_for_non_validated_parrot) + end + + test "should generate validation methods for HABTM associations with :validate => true" do + assert @pirate.respond_to?(:validate_associated_records_for_parrots) + end + + test "should not generate validation methods for HABTM associations without :validate => true" do + assert !@pirate.respond_to?(:validate_associated_records_for_non_validated_parrots) + end +end diff --git a/activerecord/test/cases/nested_attributes_test.rb b/activerecord/test/cases/nested_attributes_test.rb index d033c1e760..721792132c 100644 --- a/activerecord/test/cases/nested_attributes_test.rb +++ b/activerecord/test/cases/nested_attributes_test.rb @@ -68,24 +68,38 @@ class TestNestedAttributesInGeneral < ActiveRecord::TestCase ship = pirate.create_ship(:name => 'Nights Dirty Lightning') assert_no_difference('Ship.count') do - pirate.update_attributes(:ship_attributes => { '_delete' => true }) + pirate.update_attributes(:ship_attributes => { '_destroy' => true }) end end - def test_a_model_should_respond_to_underscore_delete_and_return_if_it_is_marked_for_destruction + def test_a_model_should_respond_to_underscore_destroy_and_return_if_it_is_marked_for_destruction ship = Ship.create!(:name => 'Nights Dirty Lightning') - assert !ship._delete + assert !ship._destroy ship.mark_for_destruction - assert ship._delete + assert ship._destroy + end + + def test_underscore_delete_is_deprecated + ActiveSupport::Deprecation.expects(:warn) + ship = Ship.create!(:name => 'Nights Dirty Lightning') + ship._delete end end class TestNestedAttributesOnAHasOneAssociation < ActiveRecord::TestCase + include AssertRaiseWithMessage + def setup @pirate = Pirate.create!(:catchphrase => "Don' botharrr talkin' like one, savvy?") @ship = @pirate.create_ship(:name => 'Nights Dirty Lightning') end + def test_should_raise_argument_error_if_trying_to_build_polymorphic_belongs_to + assert_raise_with_message ArgumentError, "Cannot build association looter. Are you trying to build a polymorphic one-to-one association?" do + Treasure.new(:name => 'pearl', :looter_attributes => {:catchphrase => "Arrr"}) + end + end + def test_should_define_an_attribute_writer_method_for_the_association assert_respond_to @pirate, :ship_attributes= end @@ -98,9 +112,9 @@ class TestNestedAttributesOnAHasOneAssociation < ActiveRecord::TestCase assert_equal 'Davy Jones Gold Dagger', @pirate.ship.name end - def test_should_not_build_a_new_record_if_there_is_no_id_and_delete_is_truthy + def test_should_not_build_a_new_record_if_there_is_no_id_and_destroy_is_truthy @ship.destroy - @pirate.reload.ship_attributes = { :name => 'Davy Jones Gold Dagger', :_delete => '1' } + @pirate.reload.ship_attributes = { :name => 'Davy Jones Gold Dagger', :_destroy => '1' } assert_nil @pirate.ship end @@ -120,8 +134,8 @@ class TestNestedAttributesOnAHasOneAssociation < ActiveRecord::TestCase assert_equal 'Nights Dirty Lightning', @ship.name end - def test_should_not_replace_an_existing_record_if_there_is_no_id_and_delete_is_truthy - @pirate.reload.ship_attributes = { :name => 'Davy Jones Gold Dagger', :_delete => '1' } + def test_should_not_replace_an_existing_record_if_there_is_no_id_and_destroy_is_truthy + @pirate.reload.ship_attributes = { :name => 'Davy Jones Gold Dagger', :_destroy => '1' } assert_equal @ship, @pirate.ship assert_equal 'Nights Dirty Lightning', @pirate.ship.name @@ -148,29 +162,29 @@ class TestNestedAttributesOnAHasOneAssociation < ActiveRecord::TestCase assert_equal 'Davy Jones Gold Dagger', @pirate.ship.name end - def test_should_delete_an_existing_record_if_there_is_a_matching_id_and_delete_is_truthy + def test_should_destroy_an_existing_record_if_there_is_a_matching_id_and_destroy_is_truthy @pirate.ship.destroy [1, '1', true, 'true'].each do |truth| @pirate.reload.create_ship(:name => 'Mister Pablo') assert_difference('Ship.count', -1) do - @pirate.update_attribute(:ship_attributes, { :id => @pirate.ship.id, :_delete => truth }) + @pirate.update_attribute(:ship_attributes, { :id => @pirate.ship.id, :_destroy => truth }) end end end - def test_should_not_delete_an_existing_record_if_delete_is_not_truthy + def test_should_not_destroy_an_existing_record_if_destroy_is_not_truthy [nil, '0', 0, 'false', false].each do |not_truth| assert_no_difference('Ship.count') do - @pirate.update_attribute(:ship_attributes, { :id => @pirate.ship.id, :_delete => not_truth }) + @pirate.update_attribute(:ship_attributes, { :id => @pirate.ship.id, :_destroy => not_truth }) end end end - def test_should_not_delete_an_existing_record_if_allow_destroy_is_false + def test_should_not_destroy_an_existing_record_if_allow_destroy_is_false Pirate.accepts_nested_attributes_for :ship, :allow_destroy => false, :reject_if => proc { |attributes| attributes.empty? } assert_no_difference('Ship.count') do - @pirate.update_attribute(:ship_attributes, { :id => @pirate.ship.id, :_delete => '1' }) + @pirate.update_attribute(:ship_attributes, { :id => @pirate.ship.id, :_destroy => '1' }) end Pirate.accepts_nested_attributes_for :ship, :allow_destroy => true, :reject_if => proc { |attributes| attributes.empty? } @@ -193,7 +207,7 @@ class TestNestedAttributesOnAHasOneAssociation < ActiveRecord::TestCase def test_should_not_destroy_the_associated_model_until_the_parent_is_saved assert_no_difference('Ship.count') do - @pirate.attributes = { :ship_attributes => { :id => @ship.id, :_delete => '1' } } + @pirate.attributes = { :ship_attributes => { :id => @ship.id, :_destroy => '1' } } end assert_difference('Ship.count', -1) do @pirate.save @@ -224,9 +238,9 @@ class TestNestedAttributesOnABelongsToAssociation < ActiveRecord::TestCase assert_equal 'Arr', @ship.pirate.catchphrase end - def test_should_not_build_a_new_record_if_there_is_no_id_and_delete_is_truthy + def test_should_not_build_a_new_record_if_there_is_no_id_and_destroy_is_truthy @pirate.destroy - @ship.reload.pirate_attributes = { :catchphrase => 'Arr', :_delete => '1' } + @ship.reload.pirate_attributes = { :catchphrase => 'Arr', :_destroy => '1' } assert_nil @ship.pirate end @@ -246,8 +260,8 @@ class TestNestedAttributesOnABelongsToAssociation < ActiveRecord::TestCase assert_equal 'Aye', @pirate.catchphrase end - def test_should_not_replace_an_existing_record_if_there_is_no_id_and_delete_is_truthy - @ship.reload.pirate_attributes = { :catchphrase => 'Arr', :_delete => '1' } + def test_should_not_replace_an_existing_record_if_there_is_no_id_and_destroy_is_truthy + @ship.reload.pirate_attributes = { :catchphrase => 'Arr', :_destroy => '1' } assert_equal @pirate, @ship.pirate assert_equal 'Aye', @ship.pirate.catchphrase @@ -274,29 +288,29 @@ class TestNestedAttributesOnABelongsToAssociation < ActiveRecord::TestCase assert_equal 'Arr', @ship.pirate.catchphrase end - def test_should_delete_an_existing_record_if_there_is_a_matching_id_and_delete_is_truthy + def test_should_destroy_an_existing_record_if_there_is_a_matching_id_and_destroy_is_truthy @ship.pirate.destroy [1, '1', true, 'true'].each do |truth| @ship.reload.create_pirate(:catchphrase => 'Arr') assert_difference('Pirate.count', -1) do - @ship.update_attribute(:pirate_attributes, { :id => @ship.pirate.id, :_delete => truth }) + @ship.update_attribute(:pirate_attributes, { :id => @ship.pirate.id, :_destroy => truth }) end end end - def test_should_not_delete_an_existing_record_if_delete_is_not_truthy + def test_should_not_destroy_an_existing_record_if_destroy_is_not_truthy [nil, '0', 0, 'false', false].each do |not_truth| assert_no_difference('Pirate.count') do - @ship.update_attribute(:pirate_attributes, { :id => @ship.pirate.id, :_delete => not_truth }) + @ship.update_attribute(:pirate_attributes, { :id => @ship.pirate.id, :_destroy => not_truth }) end end end - def test_should_not_delete_an_existing_record_if_allow_destroy_is_false + def test_should_not_destroy_an_existing_record_if_allow_destroy_is_false Ship.accepts_nested_attributes_for :pirate, :allow_destroy => false, :reject_if => proc { |attributes| attributes.empty? } assert_no_difference('Pirate.count') do - @ship.update_attribute(:pirate_attributes, { :id => @ship.pirate.id, :_delete => '1' }) + @ship.update_attribute(:pirate_attributes, { :id => @ship.pirate.id, :_destroy => '1' }) end Ship.accepts_nested_attributes_for :pirate, :allow_destroy => true, :reject_if => proc { |attributes| attributes.empty? } @@ -312,7 +326,7 @@ class TestNestedAttributesOnABelongsToAssociation < ActiveRecord::TestCase def test_should_not_destroy_the_associated_model_until_the_parent_is_saved assert_no_difference('Pirate.count') do - @ship.attributes = { :pirate_attributes => { :id => @ship.pirate.id, '_delete' => true } } + @ship.attributes = { :pirate_attributes => { :id => @ship.pirate.id, '_destroy' => true } } end assert_difference('Pirate.count', -1) { @ship.save } end @@ -380,18 +394,18 @@ module NestedAttributesOnACollectionAssociationTests assert_equal 'Privateers Greed', @pirate.send(@association_name).last.name end - def test_should_not_assign_delete_key_to_a_record + def test_should_not_assign_destroy_key_to_a_record assert_nothing_raised ActiveRecord::UnknownAttributeError do - @pirate.send(association_setter, { 'foo' => { '_delete' => '0' }}) + @pirate.send(association_setter, { 'foo' => { '_destroy' => '0' }}) end end - def test_should_ignore_new_associated_records_with_truthy_delete_attribute + def test_should_ignore_new_associated_records_with_truthy_destroy_attribute @pirate.send(@association_name).destroy_all @pirate.reload.attributes = { association_getter => { 'foo' => { :name => 'Grace OMalley' }, - 'bar' => { :name => 'Privateers Greed', '_delete' => '1' } + 'bar' => { :name => 'Privateers Greed', '_destroy' => '1' } } } @@ -443,7 +457,7 @@ module NestedAttributesOnACollectionAssociationTests ['1', 1, 'true', true].each do |true_variable| record = @pirate.reload.send(@association_name).create!(:name => 'Grace OMalley') @pirate.send(association_setter, - @alternate_params[association_getter].merge('baz' => { :id => record.id, '_delete' => true_variable }) + @alternate_params[association_getter].merge('baz' => { :id => record.id, '_destroy' => true_variable }) ) assert_difference('@pirate.send(@association_name).count', -1) do @@ -454,7 +468,7 @@ module NestedAttributesOnACollectionAssociationTests def test_should_not_destroy_the_associated_model_with_a_non_truthy_argument [nil, '', '0', 0, 'false', false].each do |false_variable| - @alternate_params[association_getter]['foo']['_delete'] = false_variable + @alternate_params[association_getter]['foo']['_destroy'] = false_variable assert_no_difference('@pirate.send(@association_name).count') do @pirate.update_attributes(@alternate_params) end @@ -463,7 +477,7 @@ module NestedAttributesOnACollectionAssociationTests def test_should_not_destroy_the_associated_model_until_the_parent_is_saved assert_no_difference('@pirate.send(@association_name).count') do - @pirate.send(association_setter, @alternate_params[association_getter].merge('baz' => { :id => @child_1.id, '_delete' => true })) + @pirate.send(association_setter, @alternate_params[association_getter].merge('baz' => { :id => @child_1.id, '_destroy' => true })) end assert_difference('@pirate.send(@association_name).count', -1) { @pirate.save } end diff --git a/activerecord/test/cases/reflection_test.rb b/activerecord/test/cases/reflection_test.rb index a164f5e060..0eb2da720e 100644 --- a/activerecord/test/cases/reflection_test.rb +++ b/activerecord/test/cases/reflection_test.rb @@ -176,9 +176,9 @@ class ReflectionTest < ActiveRecord::TestCase def test_reflection_of_all_associations # FIXME these assertions bust a lot - assert_equal 31, Firm.reflect_on_all_associations.size - assert_equal 24, Firm.reflect_on_all_associations(:has_many).size - assert_equal 7, Firm.reflect_on_all_associations(:has_one).size + assert_equal 35, Firm.reflect_on_all_associations.size + assert_equal 26, Firm.reflect_on_all_associations(:has_many).size + assert_equal 9, Firm.reflect_on_all_associations(:has_one).size assert_equal 0, Firm.reflect_on_all_associations(:belongs_to).size end diff --git a/activerecord/test/fixtures/accounts.yml b/activerecord/test/fixtures/accounts.yml index b2d0191900..32583042a8 100644 --- a/activerecord/test/fixtures/accounts.yml +++ b/activerecord/test/fixtures/accounts.yml @@ -2,6 +2,7 @@ signals37: id: 1 firm_id: 1 credit_limit: 50 + firm_name: 37signals unknown: id: 2 diff --git a/activerecord/test/models/company.rb b/activerecord/test/models/company.rb index 9242c209ea..b1a3930e4e 100644 --- a/activerecord/test/models/company.rb +++ b/activerecord/test/models/company.rb @@ -78,7 +78,12 @@ class Firm < Company # added order by id as in fixtures there are two accounts for Rails Core # Oracle tests were failing because of that as the second fixture was selected has_one :account_using_primary_key, :primary_key => "firm_id", :class_name => "Account", :order => "id" + has_one :account_using_foreign_and_primary_keys, :foreign_key => "firm_name", :primary_key => "name", :class_name => "Account" has_one :deletable_account, :foreign_key => "firm_id", :class_name => "Account", :dependent => :delete + + has_one :unautosaved_account, :foreign_key => "firm_id", :class_name => 'Account', :autosave => false + has_many :accounts + has_many :unautosaved_accounts, :foreign_key => "firm_id", :class_name => 'Account', :autosave => false end class DependentFirm < Company @@ -149,6 +154,7 @@ end class Account < ActiveRecord::Base belongs_to :firm + belongs_to :unautosaved_firm, :foreign_key => "firm_id", :class_name => "Firm", :autosave => false def self.destroyed_account_ids @destroyed_account_ids ||= Hash.new { |h,k| h[k] = [] } diff --git a/activerecord/test/models/pirate.rb b/activerecord/test/models/pirate.rb index acf53fce8b..3d7c4bc48a 100644 --- a/activerecord/test/models/pirate.rb +++ b/activerecord/test/models/pirate.rb @@ -1,6 +1,8 @@ class Pirate < ActiveRecord::Base - belongs_to :parrot - has_and_belongs_to_many :parrots + belongs_to :parrot, :validate => true + belongs_to :non_validated_parrot, :class_name => 'Parrot' + has_and_belongs_to_many :parrots, :validate => true + has_and_belongs_to_many :non_validated_parrots, :class_name => 'Parrot' has_and_belongs_to_many :parrots_with_method_callbacks, :class_name => "Parrot", :before_add => :log_before_add, :after_add => :log_after_add, @@ -16,7 +18,8 @@ class Pirate < ActiveRecord::Base has_many :treasure_estimates, :through => :treasures, :source => :price_estimates # These both have :autosave enabled because accepts_nested_attributes_for is used on them. - has_one :ship + has_one :ship, :validate => true + has_one :non_validated_ship, :class_name => 'Ship' has_many :birds has_many :birds_with_method_callbacks, :class_name => "Bird", :before_add => :log_before_add, diff --git a/activerecord/test/models/ship.rb b/activerecord/test/models/ship.rb index 06759d64b8..d0df951622 100644 --- a/activerecord/test/models/ship.rb +++ b/activerecord/test/models/ship.rb @@ -1,7 +1,7 @@ class Ship < ActiveRecord::Base self.record_timestamps = false - belongs_to :pirate + belongs_to :pirate, :validate => true has_many :parts, :class_name => 'ShipPart', :autosave => true accepts_nested_attributes_for :pirate, :allow_destroy => true, :reject_if => proc { |attributes| attributes.empty? } diff --git a/activerecord/test/models/treasure.rb b/activerecord/test/models/treasure.rb index 97c690c110..2a98e74f2c 100644 --- a/activerecord/test/models/treasure.rb +++ b/activerecord/test/models/treasure.rb @@ -3,4 +3,6 @@ class Treasure < ActiveRecord::Base belongs_to :looter, :polymorphic => true has_many :price_estimates, :as => :estimate_of + + accepts_nested_attributes_for :looter end diff --git a/activerecord/test/schema/schema.rb b/activerecord/test/schema/schema.rb index 9ab4cf6f43..15e5e12d03 100644 --- a/activerecord/test/schema/schema.rb +++ b/activerecord/test/schema/schema.rb @@ -22,6 +22,7 @@ ActiveRecord::Schema.define do # unless the ordering matters. In which case, define them below create_table :accounts, :force => true do |t| t.integer :firm_id + t.string :firm_name t.integer :credit_limit end |