Introduce CheckoutController state holders and state loader with tests.#13398
Conversation
96b14c2 to
799744c
Compare
Committed-By-Agent: claude
799744c to
788bd12
Compare
| fun bindPaymentElementLoader(loader: DefaultPaymentElementLoader): PaymentElementLoader | ||
|
|
||
| @Binds | ||
| fun bindsElementsSessionRepository(impl: RealElementsSessionRepository): ElementsSessionRepository |
There was a problem hiding this comment.
why do we need this? Don't we get the Elements Session from the Checkout Session?
There was a problem hiding this comment.
It's injected into the loader. So it's not really used in this case. We could probably return a throwing instance to make that more obvious. But realistically, it's probably better to leave as is, ideally model it differently so it's not required here.
There was a problem hiding this comment.
Yeah I feel like better modeling might be for PaymentElementLoader to take a LoadSession parameter which is a fun interface, like RetrieveCustomerEmail. It would do the work in this function: code and in CheckoutSessions implementations we inject the CheckoutSessionLoader and otherwise we inject the ElementsSessionLoader.
Doesn't seem hugely important but I think it'd be better
| configuration: CheckoutController.Configuration.State, | ||
| checkoutSessionResponse: CheckoutSessionResponse, | ||
| ): CheckoutControllerState = CheckoutControllerState( | ||
| key = UUID.randomUUID().toString(), |
There was a problem hiding this comment.
OOC why do we not use the checkout session response ID for this?
There was a problem hiding this comment.
I can't think of a great reason not to use it.
|
|
||
| // Preserve the customer's existing selection across reloads when it's still valid, rather | ||
| // than blindly adopting the loader's recomputed selection (reuses the embedded logic). | ||
| val selection = selectionChooser.choose( |
There was a problem hiding this comment.
should this logic live in PaymentElementLoader?
There was a problem hiding this comment.
We could probably inject it. But it does seem logical that it would go there 🤔 I'll attempt this in a follow up!
There was a problem hiding this comment.
sounds good! Right now I think PaymentElementLoader is doing some work and then we throw out the results of that work
| val resolvedState = state.copy( | ||
| // [state] carries the previously resolved flag images forward (a mutation copies them | ||
| // from the committed state), so they're reused when the currencies haven't changed. | ||
| flagImages = flagImageResolver.resolve(response, cached = state.flagImages), | ||
| ) |
There was a problem hiding this comment.
it feels surprising that we have to make a copy of state here. Does this mean that the state in CheckoutController is out of sync with what we use in load?
There was a problem hiding this comment.
For configure this is a little silly. But I built it to be prepared for mutations, which is where this is actually useful. During a mutation, we'll have the previously configured response + flags, etc -- and we want all the same state, except for the new checkout sessions response + the updated flags (if they're needed).
| val ruleChain: RuleChain = RuleChain | ||
| .outerRule(networkRule) | ||
| .around(PaymentConfigurationTestRule(applicationContext)) |
There was a problem hiding this comment.
should we re-use TestRules here so that other rules are pulled in automatically? E.g. I wonder if we want to use RetryRule for this
There was a problem hiding this comment.
Seems reasonable. I've got a few follow ups already queued, so I'm going to merge this as is to unblock those -- but I'll follow up!
There was a problem hiding this comment.
There's a lot of instrumentation test specific stuff in TestRules, so not sure it makes sense to use here -- given this is a unit test with robolectric instead of running on an emulator.
2 participants
Summary
This PR introduces new internal state holder classes (
CheckoutControllerStateHolder,CheckoutConfirmationStateHolder) and aCheckoutStateLoaderfor the new CheckoutController. These enable atomic, process-death-safe state commits and improved testability for the confirmation flow. Dependency injection modules, DI qualifiers, and supporting classes are included. A full suite of unit tests is added to verify correct behavior and transitions.Motivation
As part of work to introduce and solidify the new CheckoutController, we need robust state management that survives process death and cleanly separates responsibilities for state mutation and observation. Extracting this logic into holders and a loader enables isolated unit testing and better separation of concerns, unlocking easier future maintenance and features.
Testing
Changelog