-
Notifications
You must be signed in to change notification settings - Fork 46
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
PayPal: Remove Checkout Listener Pattern #307
Conversation
* Remove `CardClient.approveOrderListener` property | ||
* Remove `CardClient.cardVaultListener` property |
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 realized these should be added to CardClient
as well.
@@ -173,6 +154,25 @@ class PayPalWebViewModel @Inject constructor( | |||
} | |||
|
|||
fun handleBrowserSwitchResult(activity: ComponentActivity) { | |||
authState?.let { paypalClient?.completeAuthChallenge(activity.intent, it) } | |||
val result = authState?.let { paypalClient?.finishStart(activity.intent, 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.
I had to do a double take on this because our checkout main function is "start"
But it goes with the pattern. finish[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.
Yeah honestly I wouldn't mind renaming it to either checkout()
or confirmPaymentSource()
. It might be nice to have CardClient.confirmPaymentSource()
as well instead of approveOrder()
to align with PPaaS.
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.
Then it would be finishCheckout()
or finishConfirmPaymentSource()
.
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 like checkout(
) but maybe it's misleading because we provide intermediate steps in our SDK.
In CardClient
, we have approveOrder
, which is what we do. maybe that makes sense
approveOrder
for both CardClient
and PayPalwebCheckout
but let's hold on changing that
so we can address it on different PRs. wdyt?
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.
My bad I missed this. I agree we should hold off on re-naming until we're closer to GA.
} | ||
} else { | ||
PayPalWebCheckoutFinishStartResult.NoResult |
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 c. this is possible although just theoretically, right?
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.
NoResult
really means the SDK doesn't have enough information to determine the state of the browser switched flow. We don't receive a "close" event with Chrome Custom Tabs or anything, so when the merchant app comes back into the foreground we have to parse the deep link to determine if it was successful.
Some merchants may choose to support other browser-switch enabled payment methods in the same view. E.g. we have to make sure that we don't provide a false-postive result for PayPal success when the deep link indicates that the success event is actually for Card. So if the deep link is for another payment method other than PayPal, PayPalWebCheckoutClient
returns NoResult
.
Summary of changes
PayPalClient
checkout flow away from the listener pattern.PayPalWebCheckoutClient.start()
with calls toPayPalWebCheckoutClient.finishStart()
to complete a vault flowChecklist
Authors