-
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: create token from bank account #591
Conversation
Please add this :( |
Hey guys, Can you please add this feature? Thanks ;) |
I need you to approve this |
Please approve |
It would be great if this could be approved |
android/src/main/java/com/reactnativestripesdk/StripeSdkModule.kt
Outdated
Show resolved
Hide resolved
android/src/main/java/com/reactnativestripesdk/StripeSdkModule.kt
Outdated
Show resolved
Hide resolved
accountHolderType = mapToBankAccountType(accountHolderType) | ||
) | ||
|
||
runBlocking { |
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 done on a background thread?
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.
As I just take it over from Arek I'm not sure I get the comment @michelleb-stripe. runBlocking
is needed here because without it we need an error Suspend function 'createBankAccountToken' should be called only from a coroutine or another suspend function
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.
@souhe When the ReactMethod is called is it being called from a UI thread. The reason the routine is marked as suspend is to help indicate that it should not be called from a UI thread.
android/src/main/java/com/reactnativestripesdk/StripeSdkModule.kt
Outdated
Show resolved
Hide resolved
Please approve |
Please approve the PR is necessary to move forward with a project :( |
@arekkubaczkowski are you planning to update this PR based on the comments you got? |
Any updates? |
Any movement on this? 🙏 |
android/src/main/java/com/reactnativestripesdk/StripeSdkModule.kt
Outdated
Show resolved
Hide resolved
val type = getValOr(params, "type", null)?.let { | ||
if (it != "Card") { | ||
val type = getValOr(params, "type", null) | ||
type?.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.
You can use:
when(type){ "BankAccount" -> {...} null -> {...} }
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.
Good idea. I just refactored this function into a big when
expression
android/src/main/java/com/reactnativestripesdk/StripeSdkModule.kt
Outdated
Show resolved
Hide resolved
ios/StripeSdk.swift
Outdated
bankAccountParams.routingNumber = routingNumber | ||
|
||
if let holderType = Mappers.mapToBankAccountHolderType(accountHolderType) { | ||
bankAccountParams.accountHolderType = holderType |
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.
@charliecruzan-stripe .accountHolderType
defaults to .individual
on iOS. What does it default to on Android? Do we want consistent defaults across for both platforms?
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.
fixed in bf363b8
I am just headed out, but I can take a look at this in the morning. |
Thanks for making those updates. It now look good to me, just need a final approval from @ramont-stripe |
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.
Thank you for updating. Changes look good 👍
closes #575