-
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
fix: Native code audit, mostly Android #832
Conversation
@michelleb-stripe requesting your review since you did the Android audit (here). Really the only commit that I think could use your attention is this one |
when (paymentResult) { | ||
is PaymentResult.Completed -> { | ||
clientSecret?.let { | ||
retrievePaymentIntent(it, stripeAccountId) |
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.
How is the result of of retrievePaymentIntent being used? I don't think it should be needed because the PaymentFlowFailureMessageFactory contains similar logic. It is called from the PaymentFlowResultProcessor in the PaymentLauncherViewModel.
We have an example of how to use the PaymentLauncher here.
It would also be good to have @ccen-stripe take a final look at it too
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.
retrievePaymentIntent
allows us to get the relevant paymentIntent
(or the error) and pass it back to the React Native app via promise.resolve
. Went with stripe.retrievePaymentIntent
based on this comment
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.
I am not sure I am understanding. It looks like the retrievePaymentIntent is only be retrieved in the "Complete" case and isn't being returned.
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.
That's correct- retrievePaymentIntent
is only called in the complete case. In the case of cancellation or failure, we send an error object back to the user (L46 and L50)
In the case of a completed payment result, we retrieve the payment intent, and resolve
the promise that was passed in via Javascript. We aren't explicitly calling return
here with the payment intent object, but the promise.resolve
call is passing the data back over the React Native bridge into "Javascript land".
The previous code functioned the same way, see here. But, if there's a better way to fetch the payment intent associated with the result then let me know!
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.
I gotchya, that is ok to preserve behavior. The PaymentSheet on Android and iOS do not return the payment intent though, so it is a difference of behavior there.
I don't think an immediate action is needed.
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.
PaymentLauncher
actually processes the status of the Intent internally and returns Completed
/Failed
/Cancelled
according to the processed Outcome
s.
Compared the logic with retrievePaymentIntent
a bit, it seems we can just avoid the retrievePaymentIntent call and do the following mapping:
PaymentResult.Completed-> {
val paymentIntent = createResult("paymentIntent", mapFromPaymentIntentResult(result))
promise.resolve(paymentIntent)}
PaymentResult.Failed-> {
val error = result.lastPaymentError
promise.resolve(createError(ConfirmPaymentErrorType.Failed.toString(), error))
}
PaymentResult.Cancelled-> {
if (isPaymentIntentNextActionVoucherBased(result.nextActionType)) {
val paymentIntent = createResult("paymentIntent", mapFromPaymentIntentResult(result))
promise.resolve(paymentIntent)
} else {
(result.lastPaymentError)?.let {
promise.resolve(createError(ConfirmPaymentErrorType.Canceled.toString(), it))
} ?: run {
promise.resolve(createError(ConfirmPaymentErrorType.Canceled.toString(), "The payment has been 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.
Not sure I understand- mapFromPaymentIntentResult
accepts a PaymentIntent
, but the result
we're getting here from PaymentLauncher
is a PaymentResult
. I thought in order to retrieve the full PaymentIntent
object, we pretty much needed to use Stripe.retrievePaymentIntent
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.
yeah, @ccen-stripe I think this api is a little different in that it returns the full payment intent to clients.
android/src/main/java/com/reactnativestripesdk/PaymentMethodCreateParamsFactory.kt
Outdated
Show resolved
Hide resolved
android/src/main/java/com/reactnativestripesdk/PaymentMethodCreateParamsFactory.kt
Show resolved
Hide resolved
android/src/main/java/com/reactnativestripesdk/PaymentSheetFragment.kt
Outdated
Show resolved
Hide resolved
android/src/main/java/com/reactnativestripesdk/PaymentSheetFragment.kt
Outdated
Show resolved
Hide resolved
@@ -271,6 +215,7 @@ class StripeSdkModule(private val reactContext: ReactApplicationContext) : React | |||
} | |||
|
|||
@ReactMethod | |||
@SuppressWarnings("unused") | |||
fun initialise(params: ReadableMap, promise: Promise) { | |||
val publishableKey = getValOr(params, "publishableKey", null) as String | |||
val appInfo = getMapOrNull(params, "appInfo") as ReadableMap |
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.
On line 278: setUrlSchemeOnAndroid, it appears this is the returnUrl on Android. I wonder if we can deprecate setUrlSchemeOnAndroid and move to setReturnUrlSchemeOnAndroid?
Let's check with @ccen-stripe, but I think we recommend that user's don't set this, but not sure where we indicate that to users. (Stripe.kt has some commentary on the effect of the returnUrl, but doesn't say anything about recommending it is not set.)
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.
renamed to setReturnUrlSchemeOnAndroid
and deprecated setUrlSchemeOnAndroid
.
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.
Lots of great changes and cleanup in here! Very excited to see this all implemented.
Co-authored-by: michelleb-stripe <77996191+michelleb-stripe@users.noreply.github.com>
I just sent @ccen-stripe a message this morning to get some feedback on one remaining comment: https://github.com/stripe/stripe-react-native/pull/832/files#r822856777 |
Summary / Motivation
Get rid of warnings, fix some code styling, better
null
checks, remove usage of deprecated Android features, better file naming, usePaymentLauncher
instead ofstripe.onPaymentResult
For reviewing- it's probably most efficient to review this PR commit by commit
Testing
Documentation
Select one: