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

Allow stripe account to be configured on a per-request basis #2379

Merged
merged 7 commits into from
Apr 17, 2020

Conversation

smaskell-stripe
Copy link
Contributor

@smaskell-stripe smaskell-stripe commented Apr 15, 2020

Summary

Adds a stripeAccountId parameter to most methods in Stripe.kt
Note: This does not update

  • confirmPayment/onPaymentResult
  • confirmSetupIntent/onSetupResult
  • authenticateSource/onAuthenticateSourceResult

These will be updated later as they require passing the stripe account id through the intent

Motivation

ANDROID-247

Testing

Unit tests still pass

Update createToken

Add missing JvmOverloads
@@ -258,11 +272,15 @@ class Stripe internal constructor(
* Should be called via `Activity#onActivityResult(int, int, Intent)}}` to handle the
* result of a PaymentIntent automatic confirmation (see [confirmPayment]) or
* manual confirmation (see [handleNextActionForPayment]})
*
* TODO(smaskell): figure out if we can get account id from the intent
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mshafrir-stripe any thoughts here? It looks like the account is added as an extra on the intent in some cases, but it doesn't seem consistent

Copy link
Collaborator

Choose a reason for hiding this comment

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

You can skip adding it here. We should be passing it through the Intent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should be but we aren't. Is that going to be a problem? If we don't add it here then doesn't that mean we have to also not add it in confirmPayment? Otherwise you could do something like this

val stripe = new Stripe(acct_1)
stripe.confirmPayment(this, params, acct_2)
...
stripe.onPaymentResult(requestCode, intent, callback) // uses acc_1 even though payment was for acct_2

Should we avoid adding support here (and other places that use onActivityResult) and follow up on that later?

@@ -1256,6 +1363,7 @@ class Stripe internal constructor(

private fun createToken(
tokenParams: TokenParams,
stripeAccountId: String?,
Copy link
Collaborator

Choose a reason for hiding this comment

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

you can set this to this.stripeAccountId

Copy link
Collaborator

Choose a reason for hiding this comment

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

can we make stripeAccountId come after idempotencyKey in the args?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 on args order. imo, since this is a private method, stripeAccountId should be required so that we don't accidentally forget to supply it somewhere. Let the compiler catch anywhere that might need to be updated, instead of just letting a bunch of places silently continue using the default when they shouldn't be. Don't feel too strongly about that though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh I had it in this order because I feel like we typically put required params before optional ones (except for the callback which should be last)

Copy link
Collaborator

Choose a reason for hiding this comment

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

that makes sense

Copy link
Contributor

Choose a reason for hiding this comment

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

@mshafrir-stripe @smaskell-stripe
This just for discussion sake, do you think it is better or recommended to put idempotencyKey stripeAccount
Or is there any reason we could not re-use something like

internal data class Options internal constructor(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Either way seems fine to me. We're creating the request options in this function anyway but we could make them a parameter.

stripe/src/main/java/com/stripe/android/Stripe.kt Outdated Show resolved Hide resolved
@@ -258,11 +272,15 @@ class Stripe internal constructor(
* Should be called via `Activity#onActivityResult(int, int, Intent)}}` to handle the
* result of a PaymentIntent automatic confirmation (see [confirmPayment]) or
* manual confirmation (see [handleNextActionForPayment]})
*
* TODO(smaskell): figure out if we can get account id from the intent
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can skip adding it here. We should be passing it through the Intent.

stripe/src/main/java/com/stripe/android/Stripe.kt Outdated Show resolved Hide resolved
@smaskell-stripe smaskell-stripe changed the title [WIP] Allow stripe account to be configured on a per-request basis Allow stripe account to be configured on a per-request basis Apr 15, 2020
@anelder-stripe
Copy link
Contributor

@aliriaz-stripe - Would you mind doing a quick pass on this to make sure it aligns with ANDROID-247?

Copy link
Contributor

@aliriaz-stripe aliriaz-stripe left a comment

Choose a reason for hiding this comment

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

LGTM

@smaskell-stripe smaskell-stripe merged commit 506273f into master Apr 17, 2020
@smaskell-stripe smaskell-stripe deleted the smaskell/ANDROID-247 branch April 17, 2020 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants