Draft proposal to update the Google Pay section of the sample application #1469
Conversation
…-android into test/google_pay_beta # Conflicts: # googlepay/src/main/java/com/adyen/checkout/googlepay/internal/provider/GooglePayComponentProvider.kt # googlepay/src/main/java/com/adyen/checkout/googlepay/internal/ui/DefaultGooglePayDelegate.kt
|
Nice work @JlUgia, thanks a lot for the contribution 🚀 We will review this when we can. |
...app/src/main/java/com/adyen/checkout/example/ui/googlepay/compose/SessionsGooglePayEvents.kt
Show resolved
Hide resolved
| ) { | ||
| lateinit var googlePayComponent: GooglePayComponent |
There was a problem hiding this comment.
Another question, is there any case where this field might not get initialized and causes a problem? Asking because you definitely know more than me about compose 😅
There was a problem hiding this comment.
That's a question for us, since the component is only accessed in response to events and state changes under our control. In other words, if we can guarantee that the fetchSession action (triggering the SessionsGooglePayEvents.ComponentData event) completes before any of the following happens:
- A
SessionsGooglePayEvents.ActionorSessionsGooglePayEvents.Intentevent is issued. - The pay button is clicked
- The state becomes
SessionsGooglePayState.ShowButtonorSessionsGooglePayState.ShowComponent.
...then the logic must be safe.
My understanding is that this is the case at least for all Google Pay related events, since Google Pay availability is only requested one the session call returns. Is that also true for Action, or Intent events sent by the component?
On that note, I see that we are handling the SessionsGooglePayState.ShowComponent state in the compose UI, but that value is never set elsewhere.
There was a problem hiding this comment.
The var is function local, so access modifiers are not necessary.
There was a problem hiding this comment.
The tricky part here is with Intent. This is usually triggered after a redirect (custom tabs, external browser, app switch) has completed and we returned to the app through a deep link. With Google Pay that's mostly after the Google Pay component is finished and a 3DS challenge was launched in the browser (which is rare).
Right now we have this code in the SessionsGooglePayActivity:
override fun onNewIntent(intent: Intent) {
..
sessionsGooglePayViewModel.onNewIntent(intent) // this triggers a `SessionsGooglePayEvents.Intent`
}The only case I can think of where this code executes before the SessionsGooglePayEvents.ComponentData event has been called is if the activity got destroyed in the background.
But then even without a redirect, I don't think this sample covers that scenario (app destroyed in the background and recreated), because we are fetching a new session when the view model is created so we lose the previous state of the component completely. I am not sure if any of our samples covers that use case, it's a tricky one to deal with.
There was a problem hiding this comment.
For compose, note that the view model can outlive the view if the view is recreated due to configuration changes (and hence the data there can be reused in subsequent activity recreations).
If the 3DS flow is added to the app, as a developer, I'd like to see one of the following (in order of preference):
- A good practice sample that I can use in my app (including the session renewal)
- A marked commented warning letting developers know about lifecycle challenges
If this is not deemed relevant for a simple sample application, I'd consider removing it otherwise.
There was a problem hiding this comment.
Thanks for the input! I think we definitely would like to provide an example that fully works with all edge case scenarios in mind.
and hence the data there can be reused in subsequent activity recreations
Yes and in this case our samples would work. However (correct me if I'm wrong) if the system decides to kill the app in the background to free up memory (just like when you switch on "Don't Keep Activities" in the developer options) then everything in memory will be destroyed including the view model. Which means the only thing we can use to recover the data is the SaveStateHandle. We already use it on the SDK side but not for all scenarios, so we need to investigate this more.
.../src/main/java/com/adyen/checkout/example/ui/googlepay/compose/SessionsGooglePayViewModel.kt
Outdated
Show resolved
Hide resolved
jreij
force-pushed
the
test/google_pay_beta
branch
from
18d8bd7
to
7cc5181
Compare
…-android into test/google_pay_beta # Conflicts: # googlepay/src/main/java/com/adyen/checkout/googlepay/internal/ui/DefaultGooglePayDelegate.kt # gradle/verification-metadata.xml
…les avoiding the nullifying cycle after actions are handled
…k to the viewModel. Instead, the payment operation can be started right away
…e call with the result does not depend on the Google Pay component)
JlUgia
force-pushed
the
sample_proposals
branch
from
5a3908f
to
4348459
Compare
|
|
||
| package com.adyen.checkout.example.ui.googlepay.compose | ||
|
|
||
| internal abstract class SessionsGooglePayEvents internal constructor() { |
There was a problem hiding this comment.
@JlUgia I just noticed this line, is there any benefit from using this over internal sealed class SessionsGooglePayEvents {?
There was a problem hiding this comment.
The main difference is that sealed classes enforce exhaustive checking (eg.: a switch statement will be forced to include a case statement for each member in the class), which has backwards-compatibility implications (especially for lists that are not meant to be exhaustive).
My rule of thumb is to use sealed class for exhaustive lists that are always meant to be handled in full. For example view states in a layout (loading, complete, error), and abstract class for non-exhaustive lists with unrelated items like SesssionsGooglePayEvents (even more so if they are part of an API that is expected to change).
Let me know if that makes sense.
There was a problem hiding this comment.
This is a really good solution 💯 Especially in library development where adding something to a sealed class can technically cause a breaking change
| SessionsGooglePayContent( | ||
| googlePayComponent = googlePayComponent, |
There was a problem hiding this comment.
Btw I just tested the branch locally and this lateinit field is causing a crash here (and a few other places) when the screen is being created:
kotlin.UninitializedPropertyAccessException: lateinit property googlePayComponent has not been initialized
I'm not sure anymore if this solution will work. I'll try to find a solution but let me know if you have any suggestions.
There was a problem hiding this comment.
I didn't have a client key implementation readily available, and haven't tested it. Let me take a look tomorrow. What's the quickest way to have a client key added?
There was a problem hiding this comment.
Yeah sorry my bad, I should have tested it myself before merging but I didn't want to keep stalling this PR so I thought I'll fix any errors after merging 😅
If you have an Adyen test account already then you can just follow this step in the integration guide. Otherwise don't worry about it, I'll try to figure it out.
Description
Proposal to update sample application to simplify the integration.
Note: This changes don't enforce API correctness, and are only meant to be used as guiding proposals. The code hasn't been run or tested. This review does not include thoughts from our Android counterparts, which we are still seeking.
The changes include:
sealed/abstractclass. This should do without the need tonullify optional parameters after having taken action.componentDatadoes not change). In addition, the component could be created once at the activity level and send as a param to the composable screen. That would allow us to handle the intent right away and free the screen and viewModel from that additional logic and extra UDF cycle.Feel free to comment and modify.