Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add update changesets #173

Merged
merged 1 commit into from Jan 2, 2017
Merged

Add update changesets #173

merged 1 commit into from Jan 2, 2017

Conversation

joshsmith
Copy link
Contributor

This adds the concept of a changeset for doing updates.

We should probably go back through and allow something like it for creates, as well. Would also like to remove the specification of value types since value types are (unfortunately) mutable in many cases on Stripe's API (token vs hash, string instead of Unix timestamp, etc).

@coveralls
Copy link

Coverage Status

Coverage increased (+4.2%) to 22.43% when pulling 7b77e09 on add-update-changesets into b9c2262 on 2.0.

begedin
begedin previously requested changes Jan 1, 2017
Copy link
Contributor

@begedin begedin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall, but there's a property missing from the account update schema. As for other types, I'm assuming you're just doing a "rename" for now.

avs_failure: :boolean,
cvc_failure: :boolean
},
email: :string,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems this is missing default_currency which is also listed in the documentation

It's a 3 letter ISO code, so string, I guess

routing_number: :string
},
legal_entity: %{
address: @address_map,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing additional_owners here, but I assume we also aren't yet sure how to implement those? We may want ti add an issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, let's add an issue.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

routing_number: :string,
default_for_currency: :string,
metadata: :map
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API docs specify id, default_for_currency and metadata for managed bank account updates.

For other bank account updates, it's id, customer, account_holder_name, account_holder_type and metadata.

What we have here seems to be a list of all bank account properties. Do we want to handle this later? Seems like you maybe just did a rename for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, let's open an issue for it.

Enum.reduce(keys, %{}, fn key, acc ->
case Map.get(params, key) do
value when is_nil(value) ->
acc # remove nil values
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is introducing a bug since some values in Stripe's API can be unset with null values. Going to make cast/3 instead which accepts an optional list of params that are nullable.

@coveralls
Copy link

Coverage Status

Coverage increased (+7.3%) to 25.561% when pulling 121325e on add-update-changesets into b9c2262 on 2.0.

@coveralls
Copy link

Coverage Status

Coverage increased (+7.1%) to 25.333% when pulling c6d6e7b on add-update-changesets into b9c2262 on 2.0.

@coveralls
Copy link

Coverage Status

Coverage increased (+7.1%) to 25.333% when pulling 88698ce on add-update-changesets into b9c2262 on 2.0.

Copy link
Contributor

@begedin begedin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

Especially like that the coverage is actually going up with some of the recent PRs

@coveralls
Copy link

Coverage Status

Coverage increased (+7.1%) to 25.333% when pulling ee79266 on add-update-changesets into b9c2262 on 2.0.

@joshsmith joshsmith merged commit f1da506 into 2.0 Jan 2, 2017
@joshsmith joshsmith deleted the add-update-changesets branch January 2, 2017 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants