[Woo POS] Remove ItemList view model

joshheald commented 2 days ago

Part of: #14458

Description

This PR removes the item list view model, in favour of treating the ItemListView as a viewmodel.

As mentioned in a previous PR, unfortunately using EnvironmentObject for injecting the posModel makes it impractical to write tests for the view(model) functions. That may also be true for AppStorage, I've not tried it yet though... we could try to address these shortcomings, but I haven't here.

If we did it, we would probably have to make private functions internal, so that they could be tested. I think that would be reasonable if the tests are valuable.

My view (🥁) is that the code in the view is now simple enough that the tests we're missing wouldn't add value anyway... but what do you think, @staskus, @iamgabrielma, @jaclync? Is the shouldShowHeaderBanner logic too much to have in the view, untested? Should it be in the posModel instead?

Personally, I don't think so; it's a bit of view presentation logic to decide whether it's appropriate to show the banner, which is primarily a visual thing. The AppStorage macro isn't our code anyway, it's Apple's, so we simply change a bool on tapping the dismiss button.

Testing information

It's worth testing the new way we use User Defaults; via AppStorage. I've checked this by dismissing the simple products banner on trunk, then checking it was still dismissed with this branch.

The other changes relate to pagination, refresh, loading, and selecting items... but they're simply removing various ways of passing these calls through to the posModel, so exhaustive testing might not be the best use of time on this one. Item selection was the most complicated prior to this, the others were simple passthroughs from the view model.

[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 2 days ago

	2 Warnings
:warning:	View files have been modified, but no screenshot or video is included in the pull request. Consider adding some for clarity.
: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 2 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	pr14457-8bcf130
	Version	21.1
	Bundle ID	com.automattic.alpha.woocommerce
	Commit	8bcf13031c2bfdabb9b1dab4021a4bf24bf144af
	App Center Build	WooCommerce - Prototype Builds #11682

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

iamgabrielma commented 2 days ago

using EnvironmentObject for injecting the posModel makes it impractical to write tests for the view(model) functions. That may also be true for AppStorage, I've not tried it yet though... we could try to address these shortcomings, but I haven't here.

Something that we might want to look into is mocking POSAggregateModel, declare the view in the test, inject the mock through .environmentObject, and use ViewInspector to see if all the buttons and other UI elements handled by changes in view state are behaving as they should. This is already part of our dependencies (context: pe5pgL-5Ee-p2) so nothing new we should be adding to the project.

iamgabrielma commented 2 days ago

My view (🥁) is that the code in the view is now simple enough that the tests we're missing wouldn't add value anyway... but what do you think, @staskus, @iamgabrielma, @jaclync? Is the shouldShowHeaderBanner logic too much to have in the view, untested? Should it be in the posModel instead?

Personally, I don't think so; it's a bit of view presentation logic to decide whether it's appropriate to show the banner, which is primarily a visual thing. The AppStorage macro isn't our code anyway, it's Apple's, so we simply change a bool on tapping the dismiss button.

Agree with this. We could make it internal temporarily while we make the rest of changes, or try the ViewInspector approach, but I'm not sure if is a good usage of our time. Adding to it, it would seem that with the new designs we won't have the one-time banner anymore, but we would have only the persistent toggle-able one, so I wouldn't spend much resources trying to work around this one specifically.

staskus commented 2 days ago

My view (🥁) is that the code in the view is now simple enough that the tests we're missing wouldn't add value anyway... but what do you think, @staskus, @iamgabrielma, @jaclync? Is the shouldShowHeaderBanner logic too much to have in the view, untested? Should it be in the posModel instead?

Yes, I think this is fine. If we really want to write tests, one option would be to inject aggregate model through the method parameter. func shouldShowHeaderBanner(eligibleToShowSimpleProductsBanner: Bool) -> Bool. As I mentioned before, ideally I would prefer not having POSModel within View Models at all. However, I don't know if it's always the most reasonable choice. Before making any decisions on this, I want to wait and see what remains out of view models in the end.

joshheald commented 1 day ago

Something that we might want to look into is mocking POSAggregateModel, declare the view in the test, inject the mock through .environmentObject,

Sadly, that's not possible; you have to use a concrete type with EnvironmentObject. This is because it has to conform to ObservableObject, and protocols can't fully do that.

ViewInspector to see if all the buttons and other UI elements handled by changes in view state are behaving as they should. This is already part of our dependencies (context: pe5pgL-5Ee-p2) so nothing new we should be adding to the project.

I didn't know we already had it in the project, that's good to know 😊

woocommerce / woocommerce-ios