From f7230802209bce15870ef7d840a599bcd609db7b Mon Sep 17 00:00:00 2001 From: Brandur Date: Fri, 4 Mar 2016 19:04:29 -0800 Subject: [PATCH 01/11] Refactor `serialize_params` under `StripeObject` This pull does two major things: 1. Refactors `serialize_params` to be more concise and readable while still complying to our existing test suite. Unfortunately over time this method has become a ball of mud that's very difficult to reason about, as recently evidenced by #384. 2. Moves `serialize_params` from class method to instance method (while still keeping for old class method for backwards compatibility). This is to give it a more sane interface. --- lib/stripe/account.rb | 32 +++-- lib/stripe/api_operations/update.rb | 13 +- lib/stripe/stripe_object.rb | 178 +++++++++++++++++----------- test/stripe/account_test.rb | 12 +- test/stripe/stripe_object_test.rb | 44 +++++++ 5 files changed, 187 insertions(+), 92 deletions(-) diff --git a/lib/stripe/account.rb b/lib/stripe/account.rb index 6be44644..853496db 100644 --- a/lib/stripe/account.rb +++ b/lib/stripe/account.rb @@ -52,15 +52,17 @@ module Stripe # # We're trying to get this overturned on the server side, but for now, # patch in a special allowance. - def self.serialize_params(obj, original_value=nil) - update_hash = StripeObject.serialize_params(obj, original_value) - case obj - when StripeObject - obj_values = obj.instance_variable_get(:@values) - obj_values.each do |k, v| - if k == :additional_owners && v.is_a?(Array) - update_hash[k] = serialize_additional_owners(obj, v) - end + def serialize_params(options = {}) + serialize_params_account(self, super) + end + + def serialize_params_account(obj, update_hash) + obj.instance_variable_get(:@values).each do |k, v| + k = k.to_sym + if k == :additional_owners && v.is_a?(Array) + update_hash[k] = serialize_additional_owners(obj, v) + elsif v.is_a?(StripeObject) && update_hash[k] + serialize_params_account(v, update_hash[k]) end end update_hash @@ -89,7 +91,7 @@ module Stripe private - def self.serialize_additional_owners(obj, value) + def serialize_additional_owners(obj, value) original_value = obj.instance_variable_get(:@original_values)[:additional_owners] if original_value && original_value.length > value.length # url params provide no mechanism for deleting an item in an array, @@ -102,8 +104,14 @@ module Stripe update_hash = {} value.each_with_index do |v, i| - update = StripeObject.serialize_params(v) - if update != {} && (!original_value || update != original_value[i]) + # We will almost always see a StripeObject except in the case of a Hash + # that's been appended to an array of `additional_owners`. We may be + # able to normalize that ugliness by using an array proxy object with + # StripeObjects that can detect appends and replace a hash with a + # StripeObject. + update = v.is_a?(StripeObject) ? v.serialize_params : v + + if update != {} && (!original_value || update != obj.serialize_params_value(original_value[i], nil, false, true)) update_hash[i.to_s] = update end end diff --git a/lib/stripe/api_operations/update.rb b/lib/stripe/api_operations/update.rb index 4a676b0e..8ab5b175 100644 --- a/lib/stripe/api_operations/update.rb +++ b/lib/stripe/api_operations/update.rb @@ -27,14 +27,13 @@ module Stripe values = self.class.serialize_params(self).merge(params) - if values.length > 0 - # note that id gets removed here our call to #url above has already - # generated a uri for this object with an identifier baked in - values.delete(:id) + # note that id gets removed here our call to #url above has already + # generated a uri for this object with an identifier baked in + values.delete(:id) + + response, opts = request(:post, req_url, values) + initialize_from(response, opts) - response, opts = request(:post, req_url, values) - initialize_from(response, opts) - end self end diff --git a/lib/stripe/stripe_object.rb b/lib/stripe/stripe_object.rb index 80d4de9f..291ef192 100644 --- a/lib/stripe/stripe_object.rb +++ b/lib/stripe/stripe_object.rb @@ -72,13 +72,17 @@ module Stripe # # * +values+ - Hash of values to use to update the current attributes of # the object. + # * +:opts+ Options for StripeObject like an API key. # # ==== Options # - # * +:opts+ Options for StripeObject like an API key. - def update_attributes(values, opts = {}) + # * +:dirty+ Whether values should be initiated as "dirty" (unsaved). + # Defaults to true. + def update_attributes(values, opts = nil, options = {}) + dirty = options[:dirty] values.each do |k, v| - @values[k] = Util.convert_to_stripe_object(v, opts) + @values[k] = Util.convert_to_stripe_object(v, opts || {}) + dirty_value!(@values[k]) unless dirty == false @unsaved_values.add(k) end end @@ -142,73 +146,44 @@ module Stripe end end - def serialize_nested_object(key) - new_value = @values[key] - if new_value.is_a?(APIResource) - return {} - end - - if @unsaved_values.include?(key) - # the object has been reassigned - # e.g. as object.key = {foo => bar} - update = new_value - update = update.to_hash if update.is_a?(StripeObject) - new_keys = update.keys.map(&:to_sym) - - # remove keys at the server, but not known locally - if @original_values[key] - keys_to_unset = @original_values[key].keys - new_keys - keys_to_unset.each {|key| update[key] = ''} - end - - update - else - # can be serialized normally - self.class.serialize_params(new_value) + # Sets all keys within the StripeObject as unsaved so that they will be + # included with an update when #serialize_params is called. This method is + # also recursive, so any StripeObjects contained as values or which are + # values in a tenant array are also marked as dirty. + def dirty! + @unsaved_values = Set.new(@values.keys) + @values.each do |k, v| + dirty_value!(v) end end - def self.serialize_params(obj, original_value=nil) - case obj - when nil - '' - when Array - update = obj.map { |v| serialize_params(v) } - if original_value != update - update - else - nil + def serialize_params(options = {}) + update_hash = {} + + @values.each do |k, v| + # There are two reasons we may want to add in a parameter for update: + # + # 1. The `force` option has been set. + # 2. We know that it was modified. + # 3. Its value is a StripeObject. A StripeObject may contain modified + # values within in that its parent StripeObject doesn't know about. + # + unsaved = @unsaved_values.include?(k) + if options[:force] || unsaved || v.is_a?(StripeObject) + update_hash[k.to_sym] = + serialize_params_value(@values[k], @original_values[k], unsaved, options[:force]) end - 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?(Array) - original_value = obj.instance_variable_get(:@original_values)[k] - - # the conditional here tests whether the old and new values are - # different (and therefore needs an update), or the same (meaning - # we can leave it out of the request) - if updated = serialize_params(v, original_value) - update_hash[k] = updated - else - update_hash.delete(k) - end - elsif v.is_a?(StripeObject) || v.is_a?(Hash) - update_hash[k] = obj.serialize_nested_object(k) - end - end - - update_hash - else - obj end + + # a `nil` that makes it out of `#serialize_params_value` signals an empty + # value that we shouldn't appear in the serialized form of the object + update_hash.reject! { |_, v| v == nil } + + update_hash + end + + def self.serialize_params(obj, options = {}) + obj.serialize_params(options) end protected @@ -249,7 +224,8 @@ module Stripe "We interpret empty strings as nil in requests. " \ "You may set #{self}.#{k} = nil to delete the property.") end - @values[k] = v + @values[k] = Util.convert_to_stripe_object(v, @opts) + dirty_value!(@values[k]) @unsaved_values.add(k) end @@ -328,7 +304,7 @@ module Stripe @unsaved_values.delete(k) end - update_attributes(values, opts) + update_attributes(values, opts, :dirty => false) values.each do |k, _| @transient_values.delete(k) @unsaved_values.delete(k) @@ -336,5 +312,73 @@ module Stripe self end + + def serialize_params_value(value, original, unsaved, force) + case value + when nil + '' + + when APIResource + nil + + when Array + update = value.map { |v| serialize_params_value(v, nil, true, force) } + + # This prevents an array that's unchanged from being resent. + if update != serialize_params_value(original, nil, true, force) + update + else + nil + end + + # Handle a Hash for now, but in the long run we should be able to + # eliminate all places where hashes are stored as values internally by + # making sure any time one is set, we convert it to a StripeObject. This + # will simplify our model by making data within an object more + # consistent. + when Hash + Util.convert_to_stripe_object(value, @opts).serialize_params + + when StripeObject + update = value.serialize_params(:force => force) + + # If the entire object was replaced, then we need blank each field of + # the old object that held a value. The new serialized values will + # override an of these empties. + update = empty_values(original).merge(update) if original && unsaved + + update + + else + value + end + end + + private + + def dirty_value!(value) + case value + when Array + value.map { |v| dirty_value!(v) } + when StripeObject + value.dirty! + end + end + + # Returns a hash of empty values for all the values that are in the given + # StripeObject. + def empty_values(obj) + values = case obj + when Hash then obj + when StripeObject then obj.instance_variable_get(:@values) + else + raise ArgumentError, "#empty_values got unexpected object type: #{obj.class.name}" + end + + values.inject({}) do |update, (k, _)| + update[k] = '' + update + end + end end end diff --git a/test/stripe/account_test.rb b/test/stripe/account_test.rb index bf65d420..b89b3a5b 100644 --- a/test/stripe/account_test.rb +++ b/test/stripe/account_test.rb @@ -155,8 +155,8 @@ module Stripe should "#serialize_params an a new additional_owners" do obj = Stripe::Util.convert_to_stripe_object({ :object => "account", - :legal_entity => { - }, + :legal_entity => Stripe::StripeObject.construct_from({ + }), }, {}) obj.legal_entity.additional_owners = [ { :first_name => "Joe" }, @@ -171,7 +171,7 @@ module Stripe } } } - assert_equal(expected, obj.class.serialize_params(obj)) + assert_equal(expected, obj.serialize_params) end should "#serialize_params on an partially changed additional_owners" do @@ -197,7 +197,7 @@ module Stripe } } } - assert_equal(expected, obj.class.serialize_params(obj)) + assert_equal(expected, obj.serialize_params) end should "#serialize_params on an unchanged additional_owners" do @@ -220,7 +220,7 @@ module Stripe :additional_owners => {} } } - assert_equal(expected, obj.class.serialize_params(obj)) + assert_equal(expected, obj.serialize_params) end # Note that the empty string that we send for this one has a special @@ -246,7 +246,7 @@ module Stripe :additional_owners => "" } } - assert_equal(expected, obj.class.serialize_params(obj)) + assert_equal(expected, obj.serialize_params) end end end diff --git a/test/stripe/stripe_object_test.rb b/test/stripe/stripe_object_test.rb index 378bca14..e962d173 100644 --- a/test/stripe/stripe_object_test.rb +++ b/test/stripe/stripe_object_test.rb @@ -214,5 +214,49 @@ module Stripe serialized = Stripe::StripeObject.serialize_params(obj) assert_equal({ :foo => "bar" }, serialized[:metadata]) end + + should "#serialize_params with a StripeObject that's been replaced" do + obj = Stripe::StripeObject.construct_from({ + :metadata => Stripe::StripeObject.construct_from({ :bar => 'foo' }) + }) + + # Here we replace the object wholesale which means that the client must + # be able to blank out the values that were in the old object, but which + # are no longer present in the new one. + obj.metadata = + Stripe::StripeObject.construct_from({ :baz => 'foo' }) + + serialized = Stripe::StripeObject.serialize_params(obj) + assert_equal({ :bar => "", :baz => 'foo' }, serialized[:metadata]) + end + + should "#serialize_params with an array of StripeObjects" do + obj = Stripe::StripeObject.construct_from({}) + obj.metadata = [ + Stripe::StripeObject.construct_from({ :foo => 'bar' }) + ] + + serialized = Stripe::StripeObject.serialize_params(obj) + assert_equal([{ :foo => "bar" }], serialized[:metadata]) + end + + should "#serialize_params and remove embedded APIResources" do + obj = Stripe::StripeObject.construct_from({ + :customer => Customer.construct_from({}) + }) + + serialized = Stripe::StripeObject.serialize_params(obj) + assert_equal({}, serialized) + end + + should "#serialize_params takes a force option" do + obj = Stripe::StripeObject.construct_from({ + :id => 'id', + :metadata => Stripe::StripeObject.construct_from({ :foo => 'bar' }) + }) + + serialized = Stripe::StripeObject.serialize_params(obj, :force => true) + assert_equal({ :id => 'id', :metadata => { :foo => 'bar' } }, serialized) + end end end From fcfef21c77b28a2245422a729fabb20772c3e9b8 Mon Sep 17 00:00:00 2001 From: Brandur Date: Fri, 4 Mar 2016 19:10:59 -0800 Subject: [PATCH 02/11] Add spec for `#dirty!` --- test/stripe/stripe_object_test.rb | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/test/stripe/stripe_object_test.rb b/test/stripe/stripe_object_test.rb index e962d173..d74ac8f6 100644 --- a/test/stripe/stripe_object_test.rb +++ b/test/stripe/stripe_object_test.rb @@ -258,5 +258,19 @@ module Stripe serialized = Stripe::StripeObject.serialize_params(obj, :force => true) assert_equal({ :id => 'id', :metadata => { :foo => 'bar' } }, serialized) end + + should "#dirty! forces an object and its subobjects to be saved" do + obj = Stripe::StripeObject.construct_from({ + :id => 'id', + :metadata => Stripe::StripeObject.construct_from({ :foo => 'bar' }) + }) + + # note that `force` and `dirty!` are for different things, but are + # functionally equivalent + obj.dirty! + + serialized = Stripe::StripeObject.serialize_params(obj) + assert_equal({ :id => 'id', :metadata => { :foo => 'bar' } }, serialized) + end end end From f215827e2f4bf8cb7eee8f61f3e1da66785b3122 Mon Sep 17 00:00:00 2001 From: Brandur Date: Fri, 4 Mar 2016 19:18:26 -0800 Subject: [PATCH 03/11] Remove uses and deprecate `.serialize_params` --- lib/stripe/api_operations/update.rb | 2 +- lib/stripe/stripe_object.rb | 10 +++++++-- test/stripe/stripe_object_test.rb | 32 ++++++++++++++--------------- 3 files changed, 25 insertions(+), 19 deletions(-) diff --git a/lib/stripe/api_operations/update.rb b/lib/stripe/api_operations/update.rb index 8ab5b175..c832adc5 100644 --- a/lib/stripe/api_operations/update.rb +++ b/lib/stripe/api_operations/update.rb @@ -25,7 +25,7 @@ module Stripe # Now remove any parameters that look like object attributes. params = params.reject { |k, _| respond_to?(k) } - values = self.class.serialize_params(self).merge(params) + values = self.serialize_params(self).merge(params) # note that id gets removed here our call to #url above has already # generated a uri for this object with an identifier baked in diff --git a/lib/stripe/stripe_object.rb b/lib/stripe/stripe_object.rb index 291ef192..df27c9cb 100644 --- a/lib/stripe/stripe_object.rb +++ b/lib/stripe/stripe_object.rb @@ -182,8 +182,14 @@ module Stripe update_hash end - def self.serialize_params(obj, options = {}) - obj.serialize_params(options) + class << self + # This class method has been deprecated in favor of the instance method + # of the same name. + def serialize_params(obj, options = {}) + obj.serialize_params(options) + end + extend Gem::Deprecate + deprecate :serialize_params, "#serialize_params", 2016, 9 end protected diff --git a/test/stripe/stripe_object_test.rb b/test/stripe/stripe_object_test.rb index d74ac8f6..2c8c925b 100644 --- a/test/stripe/stripe_object_test.rb +++ b/test/stripe/stripe_object_test.rb @@ -121,20 +121,20 @@ module Stripe should "#serialize_params on an empty object" do obj = Stripe::StripeObject.construct_from({}) - assert_equal({}, Stripe::StripeObject.serialize_params(obj)) + assert_equal({}, obj.serialize_params) end should "#serialize_params on a new object with a subobject" do obj = Stripe::StripeObject.new obj.metadata = { :foo => "bar" } assert_equal({ :metadata => { :foo => "bar" } }, - Stripe::StripeObject.serialize_params(obj)) + obj.serialize_params) end should "#serialize_params on a basic object" do obj = Stripe::StripeObject.construct_from({ :foo => nil }) obj.update_attributes(:foo => "bar") - assert_equal({ :foo => "bar" }, Stripe::StripeObject.serialize_params(obj)) + assert_equal({ :foo => "bar" }, obj.serialize_params) end should "#serialize_params on a more complex object" do @@ -146,7 +146,7 @@ module Stripe }) obj.foo.bar = "newbar" assert_equal({ :foo => { :bar => "newbar" } }, - Stripe::StripeObject.serialize_params(obj)) + obj.serialize_params) end should "#serialize_params on an array" do @@ -155,7 +155,7 @@ module Stripe }) obj.foo = ["new-value"] assert_equal({ :foo => ["new-value"] }, - Stripe::StripeObject.serialize_params(obj)) + obj.serialize_params) end should "#serialize_params on an array that shortens" do @@ -164,7 +164,7 @@ module Stripe }) obj.foo = ["new-value"] assert_equal({ :foo => ["new-value"] }, - Stripe::StripeObject.serialize_params(obj)) + obj.serialize_params) end should "#serialize_params on an array that lengthens" do @@ -173,7 +173,7 @@ module Stripe }) obj.foo = ["new-value"] * 4 assert_equal({ :foo => ["new-value"] * 4 }, - Stripe::StripeObject.serialize_params(obj)) + obj.serialize_params) end should "#serialize_params on an array of hashes" do @@ -187,12 +187,12 @@ module Stripe ] obj.foo[0].bar = "baz" assert_equal({ :foo => [{ :bar => "baz" }] }, - Stripe::StripeObject.serialize_params(obj)) + obj.serialize_params) end should "#serialize_params doesn't include unchanged values" do obj = Stripe::StripeObject.construct_from({ :foo => nil }) - assert_equal({}, Stripe::StripeObject.serialize_params(obj)) + assert_equal({}, obj.serialize_params) end should "#serialize_params on an array that is unchanged" do @@ -200,7 +200,7 @@ module Stripe :foo => ["0-index", "1-index", "2-index"], }) obj.foo = ["0-index", "1-index", "2-index"] - assert_equal({}, Stripe::StripeObject.serialize_params(obj)) + assert_equal({}, obj.serialize_params) end should "#serialize_params with a StripeObject" do @@ -211,7 +211,7 @@ module Stripe obj.metadata = Stripe::StripeObject.construct_from({ :foo => 'bar' }) - serialized = Stripe::StripeObject.serialize_params(obj) + serialized = obj.serialize_params assert_equal({ :foo => "bar" }, serialized[:metadata]) end @@ -226,7 +226,7 @@ module Stripe obj.metadata = Stripe::StripeObject.construct_from({ :baz => 'foo' }) - serialized = Stripe::StripeObject.serialize_params(obj) + serialized = obj.serialize_params assert_equal({ :bar => "", :baz => 'foo' }, serialized[:metadata]) end @@ -236,7 +236,7 @@ module Stripe Stripe::StripeObject.construct_from({ :foo => 'bar' }) ] - serialized = Stripe::StripeObject.serialize_params(obj) + serialized = obj.serialize_params assert_equal([{ :foo => "bar" }], serialized[:metadata]) end @@ -245,7 +245,7 @@ module Stripe :customer => Customer.construct_from({}) }) - serialized = Stripe::StripeObject.serialize_params(obj) + serialized = obj.serialize_params assert_equal({}, serialized) end @@ -255,7 +255,7 @@ module Stripe :metadata => Stripe::StripeObject.construct_from({ :foo => 'bar' }) }) - serialized = Stripe::StripeObject.serialize_params(obj, :force => true) + serialized = obj.serialize_params(:force => true) assert_equal({ :id => 'id', :metadata => { :foo => 'bar' } }, serialized) end @@ -269,7 +269,7 @@ module Stripe # functionally equivalent obj.dirty! - serialized = Stripe::StripeObject.serialize_params(obj) + serialized = obj.serialize_params assert_equal({ :id => 'id', :metadata => { :foo => 'bar' } }, serialized) end end From adcb806aac71f7dc47fa5851150a592e9abffd4f Mon Sep 17 00:00:00 2001 From: Brandur Date: Fri, 4 Mar 2016 19:22:17 -0800 Subject: [PATCH 04/11] Add test to check .serialize_params deprecation --- test/stripe/stripe_object_test.rb | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/test/stripe/stripe_object_test.rb b/test/stripe/stripe_object_test.rb index 2c8c925b..453a7405 100644 --- a/test/stripe/stripe_object_test.rb +++ b/test/stripe/stripe_object_test.rb @@ -272,5 +272,19 @@ module Stripe serialized = obj.serialize_params assert_equal({ :id => 'id', :metadata => { :foo => 'bar' } }, serialized) end + + should "warn that .serialize_params is deprecated" do + old_stderr = $stderr + $stderr = StringIO.new + begin + obj = Stripe::StripeObject.construct_from({}) + Stripe::StripeObject.serialize_params(obj) + message = "NOTE: Stripe::StripeObject.serialize_params is " + + "deprecated; use #serialize_params instead" + assert_match Regexp.new(message), $stderr.string + ensure + $stderr = old_stderr + end + end end end From c173f802a69dfdf1582e599d45c3d702a636a5f7 Mon Sep 17 00:00:00 2001 From: Brandur Date: Fri, 4 Mar 2016 19:24:15 -0800 Subject: [PATCH 05/11] Add comment about `APIResource` exclusion for clarity --- lib/stripe/stripe_object.rb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lib/stripe/stripe_object.rb b/lib/stripe/stripe_object.rb index df27c9cb..23076faf 100644 --- a/lib/stripe/stripe_object.rb +++ b/lib/stripe/stripe_object.rb @@ -324,6 +324,11 @@ module Stripe when nil '' + # The logic here is that essentially any object embedded in another + # object that had a `type` is actually an API resource of a different + # type that's been included in the response. These other resources must + # be updated from their proper endpoints, and therefore they are not + # included when serializing even if they've been modified. when APIResource nil From 06cbe6239aa9055f5ea8b933bc9c92ca737e820d Mon Sep 17 00:00:00 2001 From: Brandur Date: Fri, 4 Mar 2016 19:26:08 -0800 Subject: [PATCH 06/11] Improve the accuracy on the comment above `Hash` serialization --- lib/stripe/stripe_object.rb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/lib/stripe/stripe_object.rb b/lib/stripe/stripe_object.rb index 23076faf..b3703aa2 100644 --- a/lib/stripe/stripe_object.rb +++ b/lib/stripe/stripe_object.rb @@ -347,6 +347,11 @@ module Stripe # making sure any time one is set, we convert it to a StripeObject. This # will simplify our model by making data within an object more # consistent. + # + # For now, you can still run into a hash if someone appends one to an + # existing array being held by a StripeObject. This could happen for + # example by appending a new hash onto `additional_owners` for an + # account. when Hash Util.convert_to_stripe_object(value, @opts).serialize_params From eba117d5e81602e6ac75261f0f79c93665f44b7d Mon Sep 17 00:00:00 2001 From: Brandur Date: Mon, 7 Mar 2016 09:56:08 -0800 Subject: [PATCH 07/11] Kill outdated "two" --- lib/stripe/stripe_object.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/stripe/stripe_object.rb b/lib/stripe/stripe_object.rb index b3703aa2..9325ba0e 100644 --- a/lib/stripe/stripe_object.rb +++ b/lib/stripe/stripe_object.rb @@ -161,7 +161,8 @@ module Stripe update_hash = {} @values.each do |k, v| - # There are two reasons we may want to add in a parameter for update: + # There are a few reasons that we may want to add in a parameter for + # update: # # 1. The `force` option has been set. # 2. We know that it was modified. From 27ebcbb6974fe39bda7ba68d0f8ebaac8040e5d3 Mon Sep 17 00:00:00 2001 From: Brandur Date: Mon, 7 Mar 2016 09:59:33 -0800 Subject: [PATCH 08/11] Revert to `opts = {}` and rename to `method_options` --- lib/stripe/stripe_object.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/stripe/stripe_object.rb b/lib/stripe/stripe_object.rb index 9325ba0e..11987bf9 100644 --- a/lib/stripe/stripe_object.rb +++ b/lib/stripe/stripe_object.rb @@ -78,10 +78,10 @@ module Stripe # # * +:dirty+ Whether values should be initiated as "dirty" (unsaved). # Defaults to true. - def update_attributes(values, opts = nil, options = {}) - dirty = options[:dirty] + def update_attributes(values, opts = {}, method_options = {}) + dirty = method_options[:dirty] values.each do |k, v| - @values[k] = Util.convert_to_stripe_object(v, opts || {}) + @values[k] = Util.convert_to_stripe_object(v, opts) dirty_value!(@values[k]) unless dirty == false @unsaved_values.add(k) end From dd11f4b2977e1b01d968d7d3e9e8d3b4269f0e0c Mon Sep 17 00:00:00 2001 From: Brandur Date: Mon, 7 Mar 2016 11:11:57 -0800 Subject: [PATCH 09/11] Remove the ugly `dirty == false` --- lib/stripe/stripe_object.rb | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/lib/stripe/stripe_object.rb b/lib/stripe/stripe_object.rb index 11987bf9..7a6f5bf5 100644 --- a/lib/stripe/stripe_object.rb +++ b/lib/stripe/stripe_object.rb @@ -76,13 +76,17 @@ module Stripe # # ==== Options # - # * +:dirty+ Whether values should be initiated as "dirty" (unsaved). - # Defaults to true. + # * +:dirty+ Whether values should be initiated as "dirty" (unsaved) and + # which applies only to new StripeObjects being ininiated under this + # StripeObject. Defaults to true. def update_attributes(values, opts = {}, method_options = {}) - dirty = method_options[:dirty] + # Default to true. TODO: Convert to optional arguments after we're off + # 1.9 which will make this quite a bit more clear. + dirty = method_options.fetch(:dirty, true) + values.each do |k, v| @values[k] = Util.convert_to_stripe_object(v, opts) - dirty_value!(@values[k]) unless dirty == false + dirty_value!(@values[k]) if dirty @unsaved_values.add(k) end end From 592bc89688fe63f61dfe4d906d8a3678b4e7c934 Mon Sep 17 00:00:00 2001 From: Brandur Date: Mon, 7 Mar 2016 11:27:45 -0800 Subject: [PATCH 10/11] Refactor special case serialization for clarity --- lib/stripe/account.rb | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/lib/stripe/account.rb b/lib/stripe/account.rb index 853496db..07872d94 100644 --- a/lib/stripe/account.rb +++ b/lib/stripe/account.rb @@ -57,12 +57,11 @@ module Stripe end def serialize_params_account(obj, update_hash) - obj.instance_variable_get(:@values).each do |k, v| - k = k.to_sym - if k == :additional_owners && v.is_a?(Array) - update_hash[k] = serialize_additional_owners(obj, v) - elsif v.is_a?(StripeObject) && update_hash[k] - serialize_params_account(v, update_hash[k]) + if entity = @values[:legal_entity] + if owners = entity[:additional_owners] + entity_update = update_hash[:legal_entity] ||= {} + entity_update[:additional_owners] = + serialize_additional_owners(entity, owners) end end update_hash @@ -91,9 +90,9 @@ module Stripe private - def serialize_additional_owners(obj, value) - original_value = obj.instance_variable_get(:@original_values)[:additional_owners] - if original_value && original_value.length > value.length + def serialize_additional_owners(legal_entity, additional_owners) + original_value = legal_entity.instance_variable_get(:@original_values)[:additional_owners] + if original_value && original_value.length > additional_owners.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. @@ -103,7 +102,7 @@ module Stripe end update_hash = {} - value.each_with_index do |v, i| + additional_owners.each_with_index do |v, i| # We will almost always see a StripeObject except in the case of a Hash # that's been appended to an array of `additional_owners`. We may be # able to normalize that ugliness by using an array proxy object with @@ -111,8 +110,9 @@ module Stripe # StripeObject. update = v.is_a?(StripeObject) ? v.serialize_params : v - if update != {} && (!original_value || update != obj.serialize_params_value(original_value[i], nil, false, true)) - update_hash[i.to_s] = update + if update != {} && (!original_value || + update != legal_entity.serialize_params_value(original_value[i], nil, false, true)) + update_hash[i.to_s] = update end end update_hash From b452835344279e6e307895f57543b182d7d730e7 Mon Sep 17 00:00:00 2001 From: Brandur Date: Mon, 7 Mar 2016 11:32:50 -0800 Subject: [PATCH 11/11] Fix spelling problem --- lib/stripe/stripe_object.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/stripe/stripe_object.rb b/lib/stripe/stripe_object.rb index 7a6f5bf5..b593f089 100644 --- a/lib/stripe/stripe_object.rb +++ b/lib/stripe/stripe_object.rb @@ -365,7 +365,7 @@ module Stripe # If the entire object was replaced, then we need blank each field of # the old object that held a value. The new serialized values will - # override an of these empties. + # override any of these empty values. update = empty_values(original).merge(update) if original && unsaved update