[Woo POS] Make cash payment events independent from card payment events #15769
Conversation
Turn cash and card relationship from OR (enum) to AND (struct), allowing them to co-exist together. Keeping both cash and card payment state in a single variable can cause occasional issues when card payment event unexpectedly arrives and changes the state while cash payment was in progress.
Generated by 🚫 Danger |
…ndent-from-card-payment
There was a problem hiding this comment.
Pull Request Overview
This PR refactors PointOfSalePaymentState from an enum into a struct with separate card and cash states, adds an activePaymentMethod helper, and updates all dependent helpers, views, models, mocks, and tests accordingly.
- Replace
PointOfSalePaymentStateenum with a struct containingcardandcashproperties - Introduce
activePaymentMethodand route logic in helpers/views through it - Update aggregate model, mocks, and tests to initialize and compare the new struct state
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| WooCommerceTests/POS/ViewHelpers/TotalsViewHelperTests.swift | Updated tests to wrap card state in new struct initializer |
| WooCommerceTests/POS/ViewHelpers/CartViewHelperTests.swift | Updated tests to initialize PointOfSalePaymentState(card:…) |
| WooCommerceTests/POS/Models/PointOfSaleAggregateModelTests.swift | Adjusted expected state comparisons to new struct with card and cash |
| WooCommerceTests/POS/Mocks/MockPointOfSaleAggregateModel.swift | Changed mock default paymentState to use empty PointOfSalePaymentState() |
| Classes/POS/ViewHelpers/TotalsViewHelper.swift | Switched helper logic to use activePaymentMethod and separate .card and .cash branches |
| Classes/POS/ViewHelpers/CartViewHelper.swift | Updated guard to reference paymentState.card and moved allowsCartEditing into PointOfSaleCardPaymentState |
| Classes/POS/Presentation/TotalsView.swift | Rewrote UI switches to branch on activePaymentMethod and nested card/cash states |
| Classes/POS/Presentation/PointOfSaleDashboardView.swift | Updated background environment binding to check .card state only |
| Classes/POS/Presentation/POSFloatingControlView.swift | Changed disabling to use paymentState.card |
| Classes/POS/Models/PointOfSalePaymentState.swift | Converted enum to struct, added activePaymentMethod, moved initializers |
| Classes/POS/Models/PointOfSaleAggregateModel.swift | Adjusted default initializer, state mutations for card vs. cash events |
Comments suppressed due to low confidence (2)
WooCommerce/WooCommerceTests/POS/ViewHelpers/CartViewHelperTests.swift:12
- Add tests for
shouldPreventCartEditingwhenpaymentState.cashis.collectingCashand.paymentSuccessto ensure cart editing is correctly prevented for cash payment scenarios.
#expect(sut.shouldPreventCartEditing(orderState: .syncing,
WooCommerce/Classes/POS/ViewHelpers/TotalsViewHelper.swift:4
- Consider adding tests for cash payment states to
shouldShowTotalsFields, verifying it returnsfalseforcashstates like.collectingCashand.paymentSuccess.
func shouldShowTotalsFields(for paymentState: PointOfSalePaymentState) -> Bool {
|
|
| @@ -177,7 +177,7 @@ extension PointOfSaleAggregateModel { | |||
|
|
|||
| private func setStateForEditing() { | |||
| orderStage = .building | |||
| paymentState = .card(.idle) | |||
| paymentState = PointOfSalePaymentState() | |||
There was a problem hiding this comment.
Question:
Should we remove the .idle states as default params when we initialize a PointOfSalePaymentState instance? While it makes sense in this function, being able to instantiate it without having to explicitly pass any initial values could make us mistakenly override current payment state if we call it in the wrong place, and reset the payment state to .idle when it shouldn't.
There was a problem hiding this comment.
Not necessarily for this PR but I wonder if there's a benefit of making TotalsViewHelper just hold static functions rather than instance functions, it's currently stateless and has no dependencies, we're not mocking anything specific, and doesn't need to conform to anything. (same case for CartViewHelper)
The question comes from seeing it initialized in each test, rather than the actual number of real allocations when the app runs, so it seems trivial either way, but happy to hear you thoughts in general about these 👍
| @@ -80,7 +80,7 @@ struct PointOfSaleDashboardView: View { | |||
| .environment(\.floatingControlAreaSize, | |||
| CGSizeMake(floatingSize.width + Constants.floatingControlHorizontalOffset, | |||
| floatingSize.height + Constants.floatingControlVerticalOffset)) | |||
| .environment(\.posBackgroundAppearance, posModel.paymentState != .card(.processingPayment) ? .primary : .secondary) | |||
| .environment(\.posBackgroundAppearance, posModel.paymentState.card != .processingPayment ? .primary : .secondary) | |||
There was a problem hiding this comment.
Since we're already updating this let's extract the bg color:
private var backgroundAppearance: POSBackgroundAppearanceKey.Appearance {
posModel.paymentState.card != .processingPayment ? .primary : .secondary
}
WOOMOB-120
Description
Keeping
paymentStateenumin a single variable can introduce unwanted issues. For example, having.collectCashstate active when a latercardpayment event arrives and changes a view state.The solution is to switch from
enum'tostruct` when managing a payment state. It allows changing card and cash states without affecting the overall payment state by mistake:PointOfSalePaymentStateas structactivePaymentMethodvariable that could be used for clarity within viewspaymentState.cashorpaymentState.cardSteps to reproduce
These changes shouldn't change the behavior. Verify:
Testing information
Tested on:
Screenshots
cash.payment.events.MP4
RELEASE-NOTES.txtif necessary.