Refactored previous changes to nested attributes.

4 files changed, 48 insertions, 47 deletions

diff --git a/activerecord/lib/active_record/nested_attributes.rb b/activerecord/lib/active_record/nested_attributes.rb
index 5143e804d1..ff3a51d5c0 100644
--- a/activerecord/lib/active_record/nested_attributes.rb
+++ b/activerecord/lib/active_record/nested_attributes.rb
@@ -213,9 +213,9 @@ module ActiveRecord
       #   exception is raised. If omitted, any number associations can be processed.
       #   Note that the :limit option is only applicable to one-to-many associations.
       # [:update_only]
-      #   Allows to specify that the an existing record can only be updated.
-      #   A new record in only created when there is no existing record. This
-      #   option only works for on-to-one associations and is ignored for
+      #   Allows you to specify that an existing record may only be updated.
+      #   A new record may only be created when there is no existing record.
+      #   This option only works for one-to-one associations and is ignored for
       #   collection associations. This option is off by default.
       #
       # Examples:
@@ -248,7 +248,7 @@ module ActiveRecord
             end
 
             # def pirate_attributes=(attributes)
-            #   assign_nested_attributes_for_one_to_one_association(:pirate, attributes, false)
+            #   assign_nested_attributes_for_one_to_one_association(:pirate, attributes)
             # end
             class_eval %{
               def #{association_name}_attributes=(attributes)
@@ -289,43 +289,29 @@ module ActiveRecord
 
     # Assigns the given attributes to the association.
     #
-    # If the given attributes include an <tt>:id</tt> that matches the existing
-    # record’s id, then the existing record will be modified. Otherwise a new
-    # record will be built.
-    #
-    # If update_only is true, a new record is only created when no object exists,
-    # otherwise it will be updated
-    #
     # If update_only is false and the given attributes include an <tt>:id</tt>
     # that matches the existing record’s id, then the existing record will be
-    # modified. Otherwise a new record will be built.
+    # modified. If update_only is true, a new record is only created when no
+    # object exists. Otherwise a new record will be built.
     #
-    # If the given attributes include a matching <tt>:id</tt> attribute _and_ a
-    # <tt>:_destroy</tt> key set to a truthy value, then the existing record
-    # will be marked for destruction.
+    # If the given attributes include a matching <tt>:id</tt> attribute, or
+    # update_only is true, and a <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)
       options = self.nested_attributes_options[association_name]
       attributes = attributes.with_indifferent_access
+      check_existing_record = (options[:update_only] || !attributes['id'].blank?)
 
-      if options[:update_only]
-        if existing_record = send(association_name)
-          assign_to_or_mark_for_destruction(existing_record, attributes, options[:allow_destroy])
+      if check_existing_record && (record = send(association_name)) &&
+          (options[:update_only] || record.id.to_s == attributes['id'].to_s)
+        assign_to_or_mark_for_destruction(record, attributes, options[:allow_destroy])
+      elsif !reject_new_record?(association_name, attributes)
+        method = "build_#{association_name}"
+        if respond_to?(method)
+          send(method, attributes.except(*UNASSIGNABLE_KEYS))
         else
-          unless reject_new_record?(association_name, attributes)
-            send("build_#{association_name}", attributes.except(*UNASSIGNABLE_KEYS))
-          end
-        end
-      elsif attributes['id'].blank?
-        unless reject_new_record?(association_name, attributes)
-          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
+          raise ArgumentError, "Cannot build association #{association_name}. Are you trying to build a polymorphic one-to-one association?"
         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, options[:allow_destroy])
       end
     end
 
diff --git a/activerecord/test/cases/nested_attributes_test.rb b/activerecord/test/cases/nested_attributes_test.rb
index 5e4fc2b8d9..60c5bad225 100644
--- a/activerecord/test/cases/nested_attributes_test.rb
+++ b/activerecord/test/cases/nested_attributes_test.rb
@@ -247,34 +247,24 @@ class TestNestedAttributesOnAHasOneAssociation < ActiveRecord::TestCase
   end
 
   def test_should_accept_update_only_option
-    Pirate.accepts_nested_attributes_for :ship, :update_only => true
-    @pirate.update_attribute(:ship_attributes, { :id => @pirate.ship.id, :name => 'Mayflower' })
-
-    Pirate.accepts_nested_attributes_for :ship, :allow_destroy => true, :reject_if => proc { |attributes| attributes.empty? }
+    @pirate.update_attribute(:update_only_ship_attributes, { :id => @pirate.ship.id, :name => 'Mayflower' })
   end
 
   def test_should_create_new_model_when_nothing_is_there_and_update_only_is_true
-    Pirate.accepts_nested_attributes_for :ship, :update_only => true
     @ship.delete
-
     assert_difference('Ship.count', 1) do
-      @pirate.reload.update_attribute(:ship_attributes, { :name => 'Mayflower' })
+      @pirate.reload.update_attribute(:update_only_ship_attributes, { :name => 'Mayflower' })
     end
-
-    Pirate.accepts_nested_attributes_for :ship, :allow_destroy => true, :reject_if => proc { |attributes| attributes.empty? }
   end
 
-
   def test_should_update_existing_when_update_only_is_true_and_no_id_is_given
-    Pirate.accepts_nested_attributes_for :ship, :update_only => true
+    @ship.delete
+    @ship = @pirate.create_update_only_ship(:name => 'Nights Dirty Lightning')
 
     assert_no_difference('Ship.count') do
-      @pirate.reload.update_attributes(:ship_attributes => { :name => 'Mayflower' })
+      @pirate.update_attributes(:update_only_ship_attributes => { :name => 'Mayflower' })
     end
-
     assert_equal 'Mayflower', @ship.reload.name
-
-    Pirate.accepts_nested_attributes_for :ship, :allow_destroy => true, :reject_if => proc { |attributes| attributes.empty? }
   end
 end
 
@@ -393,6 +383,27 @@ class TestNestedAttributesOnABelongsToAssociation < ActiveRecord::TestCase
   def test_should_automatically_enable_autosave_on_the_association
     assert Ship.reflect_on_association(:pirate).options[:autosave]
   end
+
+  def test_should_accept_update_only_option
+    @ship.update_attribute(:update_only_pirate_attributes, { :id => @pirate.ship.id, :catchphrase => 'Arr' })
+  end
+
+  def test_should_create_new_model_when_nothing_is_there_and_update_only_is_true
+    @pirate.delete
+    assert_difference('Pirate.count', 1) do
+      @ship.reload.update_attribute(:update_only_pirate_attributes, { :catchphrase => 'Arr' })
+    end
+  end
+
+  def test_should_update_existing_when_update_only_is_true_and_no_id_is_given
+    @pirate.delete
+    @pirate = @ship.create_update_only_pirate(:catchphrase => 'Aye')
+
+    assert_no_difference('Pirate.count') do
+      @ship.update_attributes(:update_only_pirate_attributes => { :catchphrase => 'Arr' })
+    end
+    assert_equal 'Arr', @pirate.reload.catchphrase
+  end
 end
 
 module NestedAttributesOnACollectionAssociationTests
diff --git a/activerecord/test/models/pirate.rb b/activerecord/test/models/pirate.rb
index f2c05dd48f..88c1634717 100644
--- a/activerecord/test/models/pirate.rb
+++ b/activerecord/test/models/pirate.rb
@@ -19,6 +19,7 @@ class Pirate < ActiveRecord::Base
 
   # These both have :autosave enabled because accepts_nested_attributes_for is used on them.
   has_one :ship
+  has_one :update_only_ship, :class_name => 'Ship'
   has_one :non_validated_ship, :class_name => 'Ship'
   has_many :birds
   has_many :birds_with_method_callbacks, :class_name => "Bird",
@@ -35,6 +36,7 @@ class Pirate < ActiveRecord::Base
 
   accepts_nested_attributes_for :parrots, :birds, :allow_destroy => true, :reject_if => proc { |attributes| attributes.empty? }
   accepts_nested_attributes_for :ship, :allow_destroy => true, :reject_if => proc { |attributes| attributes.empty? }
+  accepts_nested_attributes_for :update_only_ship, :update_only => true
   accepts_nested_attributes_for :parrots_with_method_callbacks, :parrots_with_proc_callbacks,
     :birds_with_method_callbacks, :birds_with_proc_callbacks, :allow_destroy => true
   accepts_nested_attributes_for :birds_with_reject_all_blank, :reject_if => :all_blank
diff --git a/activerecord/test/models/ship.rb b/activerecord/test/models/ship.rb
index 06759d64b8..a96e38ab41 100644
--- a/activerecord/test/models/ship.rb
+++ b/activerecord/test/models/ship.rb
@@ -2,9 +2,11 @@ class Ship < ActiveRecord::Base
   self.record_timestamps = false
 
   belongs_to :pirate
+  belongs_to :update_only_pirate, :class_name => 'Pirate'
   has_many :parts, :class_name => 'ShipPart', :autosave => true
 
   accepts_nested_attributes_for :pirate, :allow_destroy => true, :reject_if => proc { |attributes| attributes.empty? }
+  accepts_nested_attributes_for :update_only_pirate, :update_only => true
 
   validates_presence_of :name
 end