Merge pull request #217 from stripe/bk-hashes

Fix updating for nested hashes

lib/stripe/api_operations/update.rb

@@ -2,11 +2,7 @@ module Stripe
   module APIOperations
     module Update
       def save(params={})
-        values = serialize_params(self).merge(params)
-
-        if @values[:metadata]
-          values[:metadata] = serialize_metadata
-        end
+        values = self.class.serialize_params(self).merge(params)
 
         if values.length > 0
           values.delete(:id)
@@ -16,42 +12,6 @@ module Stripe
         end
         self
       end
-
-      def serialize_metadata
-        if @unsaved_values.include?(:metadata)
-          # the metadata object has been reassigned
-          # i.e. as object.metadata = {key => val}
-          metadata_update = @values[:metadata]  # new hash
-          new_keys = metadata_update.keys.map(&:to_sym)
-          # remove keys at the server, but not known locally
-          keys_to_unset = @previous_metadata.keys - new_keys
-          keys_to_unset.each {|key| metadata_update[key] = ''}
-
-          metadata_update
-        else
-          # metadata is a StripeObject, and can be serialized normally
-          serialize_params(@values[:metadata])
-        end
-      end
-
-      def serialize_params(obj)
-        case obj
-        when nil
-          ''
-        when StripeObject
-          unsaved_keys = obj.instance_variable_get(:@unsaved_values)
-          obj_values = obj.instance_variable_get(:@values)
-          update_hash = {}
-
-          unsaved_keys.each do |k|
-            update_hash[k] = serialize_params(obj_values[k])
-          end
-
-          update_hash
-        else
-          obj
-        end
-      end
     end
   end
 end

lib/stripe/stripe_object.rb

@@ -43,7 +43,7 @@ module Stripe
 
     def refresh_from(values, opts, partial=false)
       @opts = opts
-      @previous_metadata = values[:metadata]
+      @original_values = Marshal.load(Marshal.dump(values)) # deep copy
       removed = partial ? Set.new : Set.new(@values.keys - values.keys)
       added = Set.new(values.keys - @values.keys)
       # Wipe old state before setting new.  This is useful for e.g. updating a
@@ -118,6 +118,75 @@ module Stripe
       end
     end
 
+
+    def serialize_nested_object(key)
+      new_value = @values[key]
+      if @unsaved_values.include?(key)
+        # the object has been reassigned
+        # e.g. as object.key = {foo => bar}
+        update = new_value
+        new_keys = update.keys.map(&:to_sym)
+        # remove keys at the server, but not known locally
+        keys_to_unset = @original_values[key].keys - new_keys
+        keys_to_unset.each {|key| update[key] = ''}
+
+        update
+      else
+        # can be serialized normally
+        self.class.serialize_params(new_value)
+      end
+    end
+
+    def self.serialize_params(obj)
+      case obj
+      when nil
+        ''
+      when StripeObject
+        unsaved_keys = obj.instance_variable_get(:@unsaved_values)
+        obj_values = obj.instance_variable_get(:@values)
+        update_hash = {}
+
+        unsaved_keys.each do |k|
+          update_hash[k] = serialize_params(obj_values[k])
+        end
+
+        obj_values.each do |k, v|
+          if v.is_a?(StripeObject) || v.is_a?(Hash)
+            update_hash[k] = obj.serialize_nested_object(k)
+          elsif v.is_a?(Array)
+            original_value = obj.instance_variable_get(:@original_values)[k]
+            if original_value && original_value.length > v.length
+              # url params provide no mechanism for deleting an item in an array,
+              # just overwriting the whole array or adding new items. So let's not
+              # allow deleting without a full overwrite until we have a solution.
+              raise ArgumentError.new(
+                "You cannot delete an item from an array, you must instead set a new array"
+              )
+            end
+            update_hash[k] = serialize_params(v)
+          end
+        end
+
+        update_hash
+      when Array
+        update_hash = {}
+        obj.each_with_index do |value, index|
+          update = serialize_params(value)
+          if update != {}
+            update_hash[index] = update
+          end
+        end
+
+        if update_hash == {}
+          nil
+        else
+          update_hash
+        end
+      else
+        obj
+      end
+    end
+
     protected
 
     def metaclass

test/stripe/account_test.rb

@@ -26,6 +26,31 @@ module Stripe
       assert !a.details_submitted
     end
 
+    should "be updatable" do
+      resp = {
+        :id => 'acct_foo',
+        :legal_entity => {
+          :address => {
+            :line1 => '1 Two Three'
+          }
+        }
+      }
+      @mock.expects(:get).
+        once.
+        with('https://api.stripe.com/v1/accounts/acct_foo', nil, nil).
+        returns(test_response(resp))
+
+      @mock.expects(:post).
+        once.
+        with('https://api.stripe.com/v1/accounts/acct_foo', nil, 'legal_entity[first_name]=Bob&legal_entity[address][line1]=2%20Three%20Four').
+        returns(test_response(resp))
+
+      a = Stripe::Account.retrieve('acct_foo')
+      a.legal_entity.first_name = 'Bob'
+      a.legal_entity.address.line1 = '2 Three Four'
+      a.save
+    end
+
     should "be able to deauthorize an account" do
       resp = {:id => 'acct_1234', :email => "test+bindings@stripe.com", :charge_enabled => false, :details_submitted => false}
       @mock.expects(:get).once.returns(test_response(resp))

test/stripe/api_resource_test.rb

@@ -416,6 +416,115 @@ module Stripe
           assert_equal true, rescued
         end
       end
+
+      should 'add key to nested objects' do
+        acct = Stripe::Account.construct_from({
+          :id => 'myid',
+          :legal_entity => {
+            :size => 'l',
+            :score => 4,
+            :height => 10
+          }
+        })
+
+        @mock.expects(:post).once.with("#{Stripe.api_base}/v1/accounts/myid", nil, 'legal_entity[first_name]=Bob').returns(test_response({"id" => "myid"}))
+
+        acct.legal_entity.first_name = 'Bob'
+        acct.save
+      end
+
+      should 'save nothing if nothing changes' do
+        acct = Stripe::Account.construct_from({
+          :id => 'myid',
+          :metadata => {
+            :key => 'value'
+          }
+        })
+
+        @mock.expects(:post).once.with("#{Stripe.api_base}/v1/accounts/myid", nil, '').returns(test_response({"id" => "myid"}))
+
+        acct.save
+      end
+
+      should 'correctly handle replaced nested objects' do
+        acct = Stripe::Account.construct_from({
+          :id => 'myid',
+          :legal_entity => {
+            :last_name => 'Smith',
+          }
+        })
+
+        @mock.expects(:post).once.with("#{Stripe.api_base}/v1/accounts/myid", nil, 'legal_entity[first_name]=Bob&legal_entity[last_name]=').returns(test_response({"id" => "myid"}))
+
+        acct.legal_entity = {:first_name => 'Bob'}
+        acct.save
+      end
+
+      should 'correctly handle array setting' do
+        acct = Stripe::Account.construct_from({
+          :id => 'myid',
+          :legal_entity => {}
+        })
+
+        @mock.expects(:post).once.with("#{Stripe.api_base}/v1/accounts/myid", nil, 'legal_entity[additional_owners][0][first_name]=Bob').returns(test_response({"id" => "myid"}))
+
+        acct.legal_entity.additional_owners = [{:first_name => 'Bob'}]
+        acct.save
+      end
+
+      should 'correctly handle array insertion' do
+        acct = Stripe::Account.construct_from({
+          :id => 'myid',
+          :legal_entity => {
+            :additional_owners => []
+          }
+        })
+
+        @mock.expects(:post).once.with("#{Stripe.api_base}/v1/accounts/myid", nil, 'legal_entity[additional_owners][0][first_name]=Bob').returns(test_response({"id" => "myid"}))
+
+        acct.legal_entity.additional_owners << {:first_name => 'Bob'}
+        acct.save
+      end
+
+      should 'correctly handle array updates' do
+        acct = Stripe::Account.construct_from({
+          :id => 'myid',
+          :legal_entity => {
+            :additional_owners => [{:first_name => 'Bob'}, {:first_name => 'Jane'}]
+          }
+        })
+
+        @mock.expects(:post).once.with("#{Stripe.api_base}/v1/accounts/myid", nil, 'legal_entity[additional_owners][1][first_name]=Janet').returns(test_response({"id" => "myid"}))
+
+        acct.legal_entity.additional_owners[1].first_name = 'Janet'
+        acct.save
+      end
+
+      should 'correctly handle array noops' do
+        acct = Stripe::Account.construct_from({
+          :id => 'myid',
+          :legal_entity => {
+            :additional_owners => [{:first_name => 'Bob'}]
+          }
+        })
+
+        @mock.expects(:post).once.with("#{Stripe.api_base}/v1/accounts/myid", nil, '').returns(test_response({"id" => "myid"}))
+
+        acct.save
+      end
+
+      should 'correctly handle hash noops' do
+        acct = Stripe::Account.construct_from({
+          :id => 'myid',
+          :legal_entity => {
+            :address => {:line1 => '1 Two Three'}
+          }
+        })
+
+        @mock.expects(:post).once.with("#{Stripe.api_base}/v1/accounts/myid", nil, '').returns(test_response({"id" => "myid"}))
+
+        acct.save
+      end
     end
   end
 end