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

Fix Source creation so that TypeData is passed properly #461

Merged
merged 3 commits into from
Sep 20, 2017

Conversation

remi-stripe
Copy link
Contributor

The TypeData values are specific to each source type. When the parameters are sent, they have to be passed to the API under the source's type's name so for 3DS you pass three_d_secure[card] for example while for Sofort you pass sofort[country].

This uses a custom AppendTo for SourceObjectParams so that the TypeData properties are looped over and encoded with the right level based on the current Source's type.

r? @brandur-stripe

I've confirmed it works for 3DS now but I was not sure if there was an easier way that looping over p.TypeData ourselves.

Fixes #460

The TypeData values are specific to each source type. When the parameters
are sent, they have to be passed to the API under the source's type's name
so for 3DS you pass `three_d_secure[card]` for example while for Sofort
you pass `sofort[country]`.
@kostaskoukouvis
Copy link

kostaskoukouvis commented Sep 20, 2017

Wow, that was fast!
Sorry for commenting here and being a bit off-topic probably, but since you touched that file i'll go ahead and take the liberty to ask. Why is the Amount in the SourceObjectParams struct an uint64 while in the Source struct an int64?

@remi-stripe
Copy link
Contributor Author

@kostaskoukouvis Mostly for historical reasons I think. Most amounts should be uint64 though there are exceptions (invoice items can have a negative amount). We likely chose the wrong type originally and wanted to avoid changing the type silently. I'm mostly guessing though.

@brandur-stripe
Copy link
Contributor

@remi-stripe Thanks for the fix!

The one problem I see with this one is that if Type is left empty, then you're going to see some really weird encoding like [][foo] (an empty string sent to form.FormatKey will be treated as []). Should we maybe wrap that for loop in a conditional that checks if Type != ""?

@brandur-stripe
Copy link
Contributor

Mostly for historical reasons I think. Most amounts should be uint64 though there are exceptions (invoice items can have a negative amount). We likely chose the wrong type originally and wanted to avoid changing the type silently. I'm mostly guessing though.

Yep :/ all legacy. I see that uint64 probably seemed like a good idea, but we probably should have just standardized on one type like int64.

@remi-stripe
Copy link
Contributor Author

The one problem I see with this one is that if Type is left empty, then you're going to see some really weird encoding like [][foo] (an empty string sent to form.FormatKey will be treated as []). Should we maybe wrap that for loop in a conditional that checks if Type != ""?

Ha nice catch, https://stripe.com/docs/api#create_source says it's required except for shared Sources. But in that case TypeData would have to be empty I think.

@brandur-stripe
Copy link
Contributor

Ha nice catch, https://stripe.com/docs/api#create_source says it's required except for shared Sources. But in that case TypeData would have to be empty I think.

Yeah I'm just thinking that if you left Type unset and filled in TypeData because you didn't 100% know what you're doing, you might get a weird error message from the API as it gets the wrong data type and goes crazy (I'm actually not exactly sure what would happen, but it might not like it).

@remi-stripe
Copy link
Contributor Author

@brandur-stripe pushed a fix, is this the right approach?

@brandur-stripe
Copy link
Contributor

@brandur-stripe pushed a fix, is this the right approach?

Yeah, it's usually preferable not to panic of course, but it's probably helpful in this sort of case where we find an obvious usage error.

@brandur-stripe brandur-stripe merged commit fe61e22 into master Sep 20, 2017
@brandur-stripe brandur-stripe deleted the remi-fix-source-creation branch September 20, 2017 17:44
nadaismail-stripe pushed a commit that referenced this pull request Oct 18, 2024
* Fix tooltip wrapping

Looks like the metadata table wasn't using the same class definition

* Attempting to zero in on test bug

* Fix table name

* Check for single user
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants