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

feat: add support for ACHv2 on Android #879

Merged
merged 13 commits into from
Apr 15, 2022

Conversation

charliecruzan-stripe
Copy link
Collaborator

@charliecruzan-stripe charliecruzan-stripe commented Apr 7, 2022

Summary

Added native implementations for

  • collectBankAccountForPayment
  • collectBankAccountForSetup
  • verifyMicrodepositsForPayment
  • verifyMicrodepositsForSetup

Adds support for USBankAccount type to confirmPayment and confirmSetupIntent

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 way

This PR also cleans up a lot of our PaymentLauncherFragment.kt code:

  • added setup intent handling and removed that from StripeSdkModule.kt
  • wrapped the .confirm and .handleNextAction methods so that it's harder for us to forget to assign the appropriate promise and client secret properties

Until 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

  • I tested this manually
  • I added automated tests

Documentation

Select one:

  • I have added relevant documentation for my changes.
  • This PR does not result in any developer-facing changes.

@charliecruzan-stripe charliecruzan-stripe changed the title [WIP] feat: add support for ACHv2 on Android feat: add support for ACHv2 on Android Apr 8, 2022
@@ -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.+'
Copy link
Collaborator Author

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

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

Copy link
Collaborator

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.

Copy link
Collaborator Author

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.

@charliecruzan-stripe charliecruzan-stripe marked this pull request as ready for review April 8, 2022 14:41
@charliecruzan-stripe charliecruzan-stripe removed the request for review from acomley-stripe April 8, 2022 14:41
@@ -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 {
Copy link
Collaborator Author

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

Copy link

@jameswoo-stripe jameswoo-stripe left a 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.+'

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."))

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?

Copy link
Collaborator Author

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")

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?

Copy link
Collaborator Author

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 resolved 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)

Copy link
Collaborator

@carlosmuvi-stripe carlosmuvi-stripe left a 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.+'
Copy link
Collaborator

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
Copy link
Collaborator

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?

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

Copy link

@jameswoo-stripe jameswoo-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

Copy link
Collaborator

@carlosmuvi-stripe carlosmuvi-stripe left a comment

Choose a reason for hiding this comment

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

:shipit:

@charliecruzan-stripe charliecruzan-stripe merged commit 337bb84 into master Apr 15, 2022
@charliecruzan-stripe charliecruzan-stripe deleted the charliecruzan-achv2-android branch April 15, 2022 19:24
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