-
Notifications
You must be signed in to change notification settings - Fork 263
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
feat: add support for ACHv2 on Android #879
Conversation
@@ -132,7 +132,8 @@ dependencies { | |||
api 'com.facebook.react:react-native:+' | |||
implementation "org.jetbrains.kotlin:kotlin-stdlib:$kotlin_version" | |||
implementation "androidx.lifecycle:lifecycle-runtime-ktx:2.3.1" | |||
implementation 'com.stripe:stripe-android:19.3.+' | |||
implementation 'com.stripe:stripe-android:20.0.+' | |||
implementation 'com.stripe:connections:20.0.+' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Undecided on if this is included by default or we require users to add it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We require our users to add it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We require it for merchants that support ACH, but it's not mandatory for everyone. If you don't want to include the entire library on android for everyone, you can do compileOnly
instead, and mention in the docs that users need to add it to their android/build.gradle
file. CollectBankAccountLauncher
will fail at runtime if the connections code is not available on the classpath.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep- my question here is if the added size of the connections library is large enough to make users add this to their build.gradle manually. Even small additional steps like that can really cause issues for RN users. And then they're in charge of keeping that version up-to-date with the version of com.stripe:stripe-android
we use is stripe-react-native internally. So I lean towards including it in our own library so users don't have to think about it.
@@ -315,6 +350,28 @@ internal fun mapFromPaymentMethod(paymentMethod: PaymentMethod): WritableMap { | |||
|
|||
upi.putString("vpa", paymentMethod.upi?.vpa) | |||
|
|||
// TODO: Remove reflection once USBankAccount is public payment method in stripe-android | |||
try { | |||
PaymentMethod::class.java.getDeclaredField("usBankAccount").let { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reflection for now for testing, won't need this once feature is public
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some concerns around translations
@@ -132,7 +132,8 @@ dependencies { | |||
api 'com.facebook.react:react-native:+' | |||
implementation "org.jetbrains.kotlin:kotlin-stdlib:$kotlin_version" | |||
implementation "androidx.lifecycle:lifecycle-runtime-ktx:2.3.1" | |||
implementation 'com.stripe:stripe-android:19.3.+' | |||
implementation 'com.stripe:stripe-android:20.0.+' | |||
implementation 'com.stripe:connections:20.0.+' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We require our users to add it
is CollectBankAccountResult.Completed -> { | ||
val intent = result.response.intent | ||
if (intent.status === StripeIntent.Status.RequiresPaymentMethod) { | ||
promise.resolve(createError(ErrorType.Canceled.toString(), "Bank account collection was canceled.")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this customer facing? If it is, should this be translated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is developer facing
private fun createUSBankAccountPaymentConfirmParams(): ConfirmPaymentIntentParams { | ||
params.getString("accountNumber")?.let { | ||
if (billingDetailsParams?.name.isNullOrBlank()) { | ||
throw PaymentMethodCreateParamsException("When creating a US bank account payment method, you must provide the following billing details: name") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these going to be user facing? Or will it be the merchants job to forward this to the user and translate it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are developer facing- the exception is caught and then we build a StripeError object which is then resolve
d via the promise to Javascript. And then yeah, up to the developer to decide what action to take (i.e. show an error message or show some retry option)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good!
@@ -132,7 +132,8 @@ dependencies { | |||
api 'com.facebook.react:react-native:+' | |||
implementation "org.jetbrains.kotlin:kotlin-stdlib:$kotlin_version" | |||
implementation "androidx.lifecycle:lifecycle-runtime-ktx:2.3.1" | |||
implementation 'com.stripe:stripe-android:19.3.+' | |||
implementation 'com.stripe:stripe-android:20.0.+' | |||
implementation 'com.stripe:connections:20.0.+' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We require it for merchants that support ACH, but it's not mandatory for everyone. If you don't want to include the entire library on android for everyone, you can do compileOnly
instead, and mention in the docs that users need to add it to their android/build.gradle
file. CollectBankAccountLauncher
will fail at runtime if the connections code is not available on the classpath.
clientSecret = clientSecret, | ||
) | ||
} ?: run { | ||
// Payment method is assumed to be already attached through via collectBankAccountForSetup |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jameswoo-stripe I think it's fine, but can we double check we can make this assumption?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Went through the flow with @charliecruzan-stripe, it looks good to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary
Added native implementations for
Adds support for
USBankAccount
type toconfirmPayment
andconfirmSetupIntent
Adds
com.stripe:connections:20.0.+
as a package dependency. We could also remove it and make this a user step, i don't have strong feelings either wayThis PR also cleans up a lot of our PaymentLauncherFragment.kt code:
.confirm
and.handleNextAction
methods so that it's harder for us to forget to assign the appropriate promise and client secret propertiesUntil Android officially releases ACHv2, I will re-comment the
if (isAndroid) {
Javascript checks before landing this PR.Motivation
Support in iOS was added in #861, this sets us up to be ready for Android's official release
Testing
Documentation
Select one: