[Woo POS] Extract payment code to aggregate model

joshheald commented 3 days ago

Closes: #14407

Description

This PR moves payment related code (excluding onboarding) from the TotalsViewModel to the new aggregate model.

Again, apologies that this is so big... but any options to make it smaller appeared to make it less clear and testable as well.

For the most part, this was a lift-and-shift operation, but I made some changes. In particular, I :

removed combine publisher chains (where practical.)
replaced "should display" type flags with direct references to the relevant properties from the View (remember, it's better thought of as a view model!)
added and simplified some tests.

Some things that were tested before, aren't any more. The most complex example of this is TotalsView.isShowingCardReaderStatus – however, it's not substantially different to the existing cardReaderViewLayout on the same View.

I have found that EnvironmentObject makes it impossible to unit test the views which rely on posModel. If we want to go down that path we can, but we'll lose the convenience of the system dependency injection. It's a shame there's no good test hook for this though.

We could potentially keep EnvironmentObject for use by deeply nested but simple views, and pass the posModel in to the top-level views... but it is likely to make it harder to break views in to smaller components, so I've not done it for now.

Testing information

Broadly, this requires testing of payment and navigation between item list, checkout, and payment states.

That should include errors, both in fetching the order and completing the payment, and checking that editing/starting a new order works as expected.

Screenshots

https://github.com/user-attachments/assets/2c0eb97b-eac0-4489-8adc-eb131158470d

https://github.com/user-attachments/assets/ff8ba731-f402-413c-97f2-49a8c3d54847

[x] I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Reviewer (or Author, in the case of optional code reviews):

Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement:

[x] The PR is small and has a clear, single focus, or a valid explanation is provided in the description. If needed, please request to split it into smaller PRs.
[x] Ensure Adequate Unit Test Coverage: The changes are reasonably covered by unit tests or an explanation is provided in the PR description.
[x] Manual Testing: The author listed all the tests they ran, including smoke tests when needed (e.g., for refactorings). The reviewer confirmed that the PR works as expected on all devices (phone/tablet) and no regressions are added.

dangermattic commented 3 days ago

	2 Warnings
:warning:	This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.
:warning:	This PR is assigned to the milestone 21.2. This milestone is due in less than 2 days. Please make sure to get it merged by then or assign it to a milestone with a later deadline.

Generated by :no_entry_sign: Danger

wpmobilebot commented 3 days ago

📲 You can test the changes from this Pull Request in WooCommerce iOS by scanning the QR code below to install the corresponding build.

	App Name	WooCommerce iOS
	Build Number	pr14450-6a545ec
	Version	21.1
	Bundle ID	com.automattic.alpha.woocommerce
	Commit	6a545ec8b81ae1b885862a0609b2810adf2ab630
	App Center Build	WooCommerce - Prototype Builds #11678

Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

joshheald commented 1 day ago

Payment works as expected! Tested payment failure state and success state. :shipit:

Thanks!

I have found that EnvironmentObject makes it impossible to unit test the views which rely on posModel. If we want to go down that path we can, but we'll lose the convenience of the system dependency injection. It's a shame there's no good test hook for this though.

Can you share a simple example just to understand more about this limitation?

Sure – see this branch for an example of testing something on CartView which relies on the posModel

In theory that would all work, but it doesn't because the first time we try to use the posModel in CartView, because our test calls shouldPreventCartEditing, we get this crash:

Fatal error: No ObservableObject of type PointOfSaleAggregateModel found. A View.environmentObject(_:) for PointOfSaleAggregateModel may be missing as an ancestor of this view.

You can see in CartTests:23 there's a commented out injection of the posModel via the environmentObject modifier, but we can't actually inject it that way – if we do, our sut stops being a CartView and becomes some View – no use to us for calling functions on it in the tests.

Attempting to cast it back to a CartView will also fail every time.

Additionally, we can't inject a mock here; EnvironmentObject matches based on concrete types, and protocols won't work. The same is true for @Environment when we get to using the Observation framework.

Unfortunately, SwiftUI's environment cannot be used to look up something which conforms to a protocol rather than a concrete type1. The new @Environment property wrapper introduced with the Observation framework requires an Observable-conforming type, and protocols cannot conform to other protocols1. – Perplexity

We could potentially keep EnvironmentObject for use by deeply nested but simple views, and pass the posModel in to the top-level views... but it is likely to make it harder to break views in to smaller components, so I've not done it for now.

I'm fine with going back to manual DI if the use of EnvironmentObject is making some logic not testable, even though it's quite manual with more code.

Potentially. However, I don't want to do that yet, because we haven't had the chance to really see the benefits of EnvironmentObject injection beyond the initial convenience.

For example, when I did my experiments for this project, one potential route is to have the itemlist show POSDisplayableItems: views which are provided via the Aggregate Model but built by underlying services. By making them views, they can be responsible for adding themselves to the cart or doing whatever else they need to (e.g. expand a variable product selection view)... but that's going to be a whole lot cleaner if they can get the aggregate model using automatic injection.

What do you think about the approach in 6a545ec – where we need tests for presentation logic, extract them to a view model without dependencies, and pass in any relevant states? We would want to avoid ever passing the whole posModel, to keep this clean.

joshheald commented 1 day ago

@jaclync ⬆️ sorry I forgot to tag you

jaclync commented 1 day ago

Thanks for sharing the example, it seems like the challenge is when we want to write unit tests on the SwiftUI view, which we don't do very often.

What do you think about the approach in https://github.com/woocommerce/woocommerce-ios/commit/6a545ec8b81ae1b885862a0609b2810adf2ab630 – where we need tests for presentation logic, extract them to a view model without dependencies, and pass in any relevant states? We would want to avoid ever passing the whole posModel, to keep this clean.

Yea I think keeping logic in a view model makes sense and the view stays lean this way. For cases where the logic depends on the aggregated model (which is pretty likely since the aggregated model has states for multiple views), it'd be great to be able to pass the individual states (hopefully observable, like binding?).

woocommerce / woocommerce-ios