Bindings to support Custom Connect onboarding in Europe #492
Conversation
ksun-stripe
force-pushed
the
ksun-connect-bindings
branch
from
4622d37 to
0086c36
Compare
|
<3 |
| throws AuthenticationException, | ||
| InvalidRequestException, | ||
| APIConnectionException, | ||
| CardException, |
There was a problem hiding this comment.
The method signature also says this throws CardException, but it's not covered by the documentation block. The createPii methods have the same problem. Maybe just a copy/paste error from createToken?
There was a problem hiding this comment.
I made it deeper into the code, looks like this is because StripeAPIHandler.createToken() can throw CardException. For PII & Connect Account creation, I think you should probably "catch & swallow" CardException (only if it cannot happen, obviously). This prevents the underlying implementation (that you're sharing a method) from leaking to clients of the SDK. Otherwise I think it'd be good to document under what conditions it'd be thrown.
There was a problem hiding this comment.
Also, you might want to update variable names, etc, in StripeAPIHandler.createToken() - things like cardParams are now more general than just cards.
| */ | ||
| public Token createAccountTokenSynchronous( | ||
| @NonNull final AccountParams accountParams, | ||
| @Nullable String publishableKey) |
There was a problem hiding this comment.
You might document what this does if publishableKey is null.
| validateKey(publishableKey); | ||
| RequestOptions requestOptions = RequestOptions.builder( | ||
| publishableKey, | ||
| null, |
There was a problem hiding this comment.
The other create token methods pass in mStripeAccount. Why don't we want to do that here? (also, looks like there're constructors for RequestOptions that just take the key and request type, which seems possibly better than null)
There was a problem hiding this comment.
My thinking is that this is the function for creating a Stripe account so there should never be a pre-existing Stripe Account passed to it...since we're creating it with this function. In particular, I was concerned this would be cause unexpected server behavior since the token can be used for updates as well. Ex: somehow the user has an only StripeAccount string saved in the Stripe object and goes to create a new account with a new legal entity and accidentally changes their old legal entity.
EDIT: I checked and it doesn't cause issues.
On account updates:
https://docs.google.com/document/d/1mmDgLjhbV8AuR_Ulxu6KULF4kemz3SNoUHTxco8b0u8/edit
There was a problem hiding this comment.
Then I like this method better:
stripe-android/stripe/src/main/java/com/stripe/android/RequestOptions.java
Lines 112 to 114 in 4163e57
| @@ -17,11 +17,12 @@ | |||
| public class Token implements StripePaymentSource { | |||
There was a problem hiding this comment.
The documentation of the Token class should be updated. It's no longer just a card. I'd also link to the docs for Token, which is probably a good place to borrow text from. https://stripe.com/docs/api#tokens
| * and links to Stripe's terms of service. Tokens will only generated | ||
| * when this is true. | ||
| * @param legalEntity map that specifies the legal entity for which the connect account is being | ||
| * created. Can contain any of the field specified by legal_entity is the API |
There was a problem hiding this comment.
typos: "any of the fields specified by legal_entity in the API"
Same typos below
| * @return {@code this}, for chaining purposes | ||
| */ | ||
| public AccountParams setLegalEntity(Map<String, Object> legalEntity) { | ||
| mLegalEntity = legalEntity; |
There was a problem hiding this comment.
Any concerns about validating that the user hasn't stuck something non-serializable into the legalEntity?
For example, I'm not sure what happens if they try to use com.stripe.android.model.Address objects instead of a Map<String, Object> for legal_entity.address.
I followed some of the code down, and it looked like the networking code was going to eventually call to flattenParamsValue(), which I don't think handles model objects.
There was a problem hiding this comment.
hrm...yes...that is a good concern. we should check that.
Add bindings to support custom connect onboarding in Europe. I opted against creating a custom object for legal entity because of longer term maintainability concerns.
r? @danj-stripe
cc: @basta-stripe @timpeacock-stripe @bg-stripe