[Woo POS] Add test coverage for PointOfSaleDashboardViewModel #13310

soundsokay · 2024-07-12T05:57:36Z

Expands test coverage for PointOfSaleDashboardViewModel to verify the observePaymentStateForButtonDisabledProperties method correctly sets isAddMoreDisabled accounting for both payment state and order syncing state.

Closes: #13306

Description

The Add More button was disabled in #13270 but tests weren't updated. This PR adds coverage for that.

Steps to reproduce

Testing information

Invert the values returned and used in the assignment to isAddMoreDisabled, run the PointOfSaleDashboardViewModelTests, and observe failures in the newly added tests.

Screenshots

N/A

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

Verify isAddMoreDisabled is set appropriately for payment state and order syncing state.

dangermattic · 2024-07-12T05:58:07Z

	2 Warnings
⚠️	View files have been modified, but no screenshot or video is included in the pull request. Consider adding some for clarity.
⚠️	This PR is larger than 500 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.

Generated by 🚫 Danger

wpmobilebot · 2024-07-12T06:07:40Z

📲 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	pr13310-7f1cb1a
	Version	19.6
	Bundle ID	com.automattic.alpha.woocommerce
	Commit	`7f1cb1a`
	App Center Build	WooCommerce - Prototype Builds #10106

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

bozidarsevo · 2024-07-12T15:22:48Z

WooCommerce/Classes/POS/ViewModels/PointOfSaleDashboardViewModel.swift

@@ -36,13 +36,12 @@ final class PointOfSaleDashboardViewModel: ObservableObject {
    init(itemProvider: POSItemProvider,
         cardPresentPaymentService: CardPresentPaymentFacade,
         orderService: POSOrderServiceProtocol,
-         currencyFormatter: CurrencyFormatter) {
+         currencyFormatter: CurrencyFormatter,
+         totalsViewModel: TotalsViewModel) {


Since with this change the totalsViewModel is injected and not created, what was the main reason for it? And will we need to change some other parts of the POS code with this?

It was a first-cut attempt to improve testability of this class.

This injection was removed in commit 35370c7

iamgabrielma

Thanks for working on this! I left a couple of comments regarding the decision of injecting the TotalsViewModel, as well as exposing private properties that were only gettable.

Perhaps we can improve the test coverage here by using Mocks/Fakes/Stubs/Spies instead? As an example we do this with the item provider

WooCommerce/Classes/POS/ViewModels/TotalsViewModel.swift

iamgabrielma · 2024-07-15T02:06:58Z

WooCommerce/Classes/POS/ViewModels/TotalsViewModel.swift

+         paymentState: PaymentState = .acceptingCard,
+         isSyncingOrder: Bool = false) {


By setting these two properties and its initial values on initialization we're now setting some default initial state, when before that it was stateless. Note how the only dependencies being injected are abstractions like the payments Façade, utils like the currency formatter, or the POS interfaces.

Do we want it to always default to .acceptingCard and not syncing? Would it be preferable to leave the implementation details to each service? Eg: Payment state being set and updated from the CardPresentPaymentService rather than injected in.

Looks like these have initial values on trunk?

Looks like these have initial values on trunk?

They are, but it's also internal state to the class. A concern here would be:

Imagine we implement a new XYZ that needs the TotalsViewModel to be in .acceptingCard state. Since the property is exposed, and this happens to be its default value then we just ignore setting the property specifically. Later on we update the implementation details of the class to be .notAcceptingCard instead, which will cause XYZ to fail because it relied on default state that is no longer the default case, and would trigger no errors (unless catch by unit tests!) because the compiler would find no issues with it.

To avoid these, we can either force providing a specific initial state by not providing a default value, so the compiler enforces this, or in our case just not expose the state to be modified from outside the class, and leave the responsibility of it to the TotalsViewModel itself, opening some getters so it can be checked (but not modified) for other objects that need to check it.

That's a a good point.

Addressed in commit bb3d973.

iamgabrielma · 2024-07-15T02:13:04Z

WooCommerce/Classes/POS/Presentation/CartView.swift

+    let totalsViewModel = TotalsViewModel(orderService: POSOrderPreviewService(),
+                                              cardPresentPaymentService: CardPresentPaymentPreviewService(),
+                                              currencyFormatter: .init(currencySettings: .init()))


While this happens on the #Preview, declaring a new instance of TotalsViewModel in order to inject it into the VM for the CartView seems to indicate that the design of the dependency graph is not quite right. I'd say the CartView should know nothing about the Totals VM and perhaps this shouldn't be injected here.

Addressed in commit 35370c7.

iamgabrielma · 2024-07-15T02:14:40Z

WooCommerce/Classes/POS/Presentation/PointOfSaleDashboardView.swift

+        let totalsViewModel = TotalsViewModel(orderService: POSOrderPreviewService(),
+                                                      cardPresentPaymentService: CardPresentPaymentPreviewService(),
+                                                      currencyFormatter: .init(currencySettings: .init()))


Same as above, up until now the entry point seems to accept only abstractions either from the payment façade, or the POS interfaces, however now it would require to inject a specific POS VM.

Addressed in commit 35370c7.

iamgabrielma · 2024-07-15T02:20:57Z

WooCommerce/WooCommerceTests/POS/ViewModels/PointOfSaleDashboardViewModelTests.swift

        super.tearDown()
    }

+    private func setupViewModelWithExpectationsExpectingAddMoreMatchesSyncingState(paymentState: TotalsViewModel.PaymentState,


Being helper methods, these could go into an extension. I think there's also no need in the test target for these to be private. Maybe we can also simplify the function names? Since it would seem are specific only to the AddMoreIsDisabled property, however both payment state and syncing state are exposed. Eg:

setupViewModel(paymentState: TotalsViewModel.PaymentState, isSyncingOrder: Bool)

Keep isSyncingOrder and paymentState private.

…en payment state is idle Added PointOfSaleDashboardViewModelProtocol to simulate order syncing for use in the unit test.

…ccess

WooCommerce/Classes/POS/ViewModels/PointOfSaleDashboardViewModel.swift

WooCommerce/WooCommerceTests/POS/ViewModels/PointOfSaleDashboardViewModelTests.swift

Work in progress.

Fixes a compile issue.

Fixes test: test_isAddMoreDisabled_is_true_when_order_is_syncing_and_payment_state_is_idle

Adds: test_isAddMoreDisabled_is_true_for_processingPayment.

Protocol no longer needed after refactoring to new testing approach using mocked dependencies.

Also restore read-only access control for external entities with lazy initialization for cartViewModel.

WooCommerce/Classes/POS/ViewModels/AnyTotalsViewModel.swift

- Converted publishers to computed properties leveraging property wrappers for simplified exposure and automatic initialization. - Introduced a `bindPublishers` method to centralize the assignment of publishers, ensuring all necessary bindings are in one place. - Addressed feedback by eliminating manual publisher initialization in initializers, reducing duplication and potential errors. - Enhanced code maintainability and readability by using property wrappers and centralizing publisher assignments.

…ta-to-go-back-to-product-selector-when-order-is-being-synced-add-test Conflicts: WooCommerce/Classes/POS/Presentation/TotalsView.swift WooCommerce/Classes/POS/ViewModels/TotalsViewModel.swift

- Updated the TotalsViewModel to remove default values for `paymentState` and `isSyncingOrder`, making these parameters mandatory during initialization.

bozidarsevo

I am wondering if we need AnyTotalsViewModel and can just go with TotalsViewModelProtocol and having TotalsViewModel and MockTotalsViewModel conform to it...

WooCommerce/Classes/POS/ViewModels/AnyTotalsViewModel.swift

staskus

I made another suggestion.

I hate the fact that I'm holding this PR for so long but I want to make sure we don't make things harder for ourselves.

I suggest to use concrete implementation in Views, and protocols in View Models. This way we go around any compiler issues that we had and it also transparently exposes our dependency tree that we can refactor in the future.

WooCommerce/Classes/POS/ViewModels/AnyTotalsViewModel.swift

soundsokay · 2024-07-22T17:21:03Z

Thanks for your detailed feedback and WIP solution @staskus.

I too would like to see this moving faster but also want to get this right and not add undesirable patterns while staying pragmatic and mindful of speed of development in the short and long term.

I'll take a look and see if your WIP solution looks like a lower level of effort than the refactoring I had planned.

- Make tests compile. - Required changing access modifiers on `PointOfSaleDashboardViewModel` from `private` to internal access.

- Added test_observeCartAddMoreAction_updates_orderStage_to_building. - Added two tests that were formerly combined and now have been broken out into separate cases. - Cleaned up other tests.

Remove trailing whitespace.

soundsokay · 2024-07-23T06:45:25Z

Your rearchitecture looked good to me.

I merged it in and make adjustments to get the tests to compile in commit e8df637.

One downside is that changed access modifiers for cartViewModel and totalsViewModel on PointOfSaleDashboardViewModel from private to internal access. However the class already uses internal access for itemListViewModel.

I also expanded test coverage and cleaned up PointOfSaleDashboardViewModelTests. Which were my focus before pushing up the changes.

CartViewModelTests are failing. Maybe you can have a look?

Seems the issue is with CartViewModel initializing without any parameters.

staskus · 2024-07-23T06:53:13Z

CartViewModelTests are failing. Maybe you can have a look? Seems the issue is with CartViewModel initializing without any parameters.

@soundsokay yes, it could be! CartViewModel depended on POSDashboardVM passing an observer through the initializer which prevented CartViewModel from being initialized separately. I will take a look 👍

staskus

It builds, I think it's good time to merge this 👍

@bozidarsevo, would be good if you could take one more look yourself, since I also made the last commits.

…t-selector-when-order-is-being-synced-add-test

soundsokay · 2024-07-23T14:15:42Z

Thank you for your contributions @staskus and @bozidarsevo.

I'm taking a look now.

bozidarsevo · 2024-07-23T14:22:10Z

I will fix the build now, might have made a wrong conflict merge commit

Adds verification that orderStage is set to .building when cart becomes empty.

Remove TODO for adding test coverage for PointOfSaleDashboardViewModel now that it has been added.

soundsokay · 2024-07-23T16:08:53Z

I made recent commits that only change test code in PointOfSaleDashboardViewModelTests to expand test coverage and remove comments and whitespace.

With all checks passing and two 👍 I'm going to merge.

Thanks again for your reviews and contributions @staskus and @bozidarsevo.

test: Add test coverage for PointOfSaleDashboardViewModel

d86155d

Verify isAddMoreDisabled is set appropriately for payment state and order syncing state.

soundsokay added category: unit tests Related to unit testing. type: task An internally driven task. labels Jul 12, 2024

soundsokay added this to the 19.6 milestone Jul 12, 2024

bozidarsevo reviewed Jul 12, 2024

View reviewed changes

iamgabrielma reviewed Jul 15, 2024

View reviewed changes

soundsokay added 7 commits July 16, 2024 15:16

test: Remove TotalsViewModel dependency injection

35370c7

test: Restore prior access controls

7fc1fc3

Keep isSyncingOrder and paymentState private.

test: simulate order syncing to verify Add More button is disabled wh…

72f6822

…en payment state is idle Added PointOfSaleDashboardViewModelProtocol to simulate order syncing for use in the unit test.

chore: move protocol to a separate file

aa93c97

test: improve expectation description

f36f8e3

test: verify the Add More button is disabled after collect payment su…

452e807

…ccess

test: improve test name with camelCase

1c48ba7

soundsokay changed the title ~~[DRAFT Woo POS] test: Add test coverage for PointOfSaleDashboardViewModel~~ [Woo POS] Add test coverage for PointOfSaleDashboardViewModel Jul 16, 2024

soundsokay assigned bozidarsevo Jul 16, 2024

staskus requested changes Jul 17, 2024

View reviewed changes

soundsokay added 4 commits July 18, 2024 10:09

test: refactor to make PointOfSaleDashboardViewModel testable

fbb03fc

Work in progress.

chore: remove TotalsViewModelProtocol from test target

52c2b71

Fixes a compile issue.

test: simulate order syncing using mocked dependencies

68aa4dd

Fixes test: test_isAddMoreDisabled_is_true_when_order_is_syncing_and_payment_state_is_idle

test: expand test coverage for PointOfSaleDashboardViewModel

9c7f998

Adds: test_isAddMoreDisabled_is_true_for_processingPayment.

soundsokay modified the milestones: 19.6, 19.7 Jul 18, 2024

soundsokay added 4 commits July 18, 2024 19:28

chore: remove PointOfSaleDashboardViewModelProtocol

2f570f0

Protocol no longer needed after refactoring to new testing approach using mocked dependencies.

test: inject a CartViewModel? into PointOfSaleDashboardViewModel

d682076

Also restore read-only access control for external entities with lazy initialization for cartViewModel.

chore: remove comments

7eca3c7

test: expand test coverage using mocked dependencies

f532537

soundsokay requested review from staskus and bozidarsevo July 19, 2024 07:54

staskus reviewed Jul 19, 2024

View reviewed changes

WooCommerce/Classes/POS/ViewModels/AnyTotalsViewModel.swift Outdated Show resolved Hide resolved

soundsokay added 3 commits July 20, 2024 16:02

Merge remote-tracking branch 'origin/trunk' into feat/13127-disable-c…

0370ce4

…ta-to-go-back-to-product-selector-when-order-is-being-synced-add-test Conflicts: WooCommerce/Classes/POS/Presentation/TotalsView.swift WooCommerce/Classes/POS/ViewModels/TotalsViewModel.swift

refactor: ensure TotalsViewModel initializes without default values

bb3d973

- Updated the TotalsViewModel to remove default values for `paymentState` and `isSyncingOrder`, making these parameters mandatory during initialization.

soundsokay requested a review from staskus July 21, 2024 23:23

bozidarsevo reviewed Jul 22, 2024

View reviewed changes

WooCommerce/Classes/POS/ViewModels/AnyTotalsViewModel.swift Outdated Show resolved Hide resolved

Use protocols in ViewModels and concrete implementations in Views

19bf5a6

staskus reviewed Jul 22, 2024

View reviewed changes

WooCommerce/Classes/POS/ViewModels/AnyTotalsViewModel.swift Outdated Show resolved Hide resolved

soundsokay added 3 commits July 22, 2024 22:12

test: fix compilation issue

e8df637

- Make tests compile. - Required changing access modifiers on `PointOfSaleDashboardViewModel` from `private` to internal access.

test: expanded and refactored PointOfSaleDashboardViewModelTests

837732a

- Added test_observeCartAddMoreAction_updates_orderStage_to_building. - Added two tests that were formerly combined and now have been broken out into separate cases. - Cleaned up other tests.

chore: fix linter issue

57bc0d0

Remove trailing whitespace.

staskus added 2 commits July 23, 2024 10:47

Introduce CartViewModelProtocol to properly mock CartViewModel in tests

0d41ad5

Bind mock orderStageSubect with CartViewModel to fix failing tests

cfe0f20

staskus approved these changes Jul 23, 2024

View reviewed changes

Merge branch 'trunk' into feat/13127-disable-cta-to-go-back-to-produc…

dc80bc7

…t-selector-when-order-is-being-synced-add-test

bozidarsevo approved these changes Jul 23, 2024

View reviewed changes

bozidarsevo and others added 4 commits July 23, 2024 16:23

Update CartViewModel.swift

f6ef4a8

test: expand test coverage

845e85c

Adds verification that orderStage is set to .building when cart becomes empty.

chore: remove TODO

1d36926

Remove TODO for adding test coverage for PointOfSaleDashboardViewModel now that it has been added.

chore: fix linting issues

7f1cb1a

soundsokay merged commit 4e1478a into trunk Jul 23, 2024
22 checks passed

soundsokay deleted the feat/13127-disable-cta-to-go-back-to-product-selector-when-order-is-being-synced-add-test branch July 23, 2024 16:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Woo POS] Add test coverage for PointOfSaleDashboardViewModel #13310

[Woo POS] Add test coverage for PointOfSaleDashboardViewModel #13310

soundsokay commented Jul 12, 2024 •

edited

Loading

dangermattic commented Jul 12, 2024 •

edited

Loading

wpmobilebot commented Jul 12, 2024 •

edited

Loading

bozidarsevo Jul 12, 2024

soundsokay Jul 16, 2024

iamgabrielma left a comment •

edited

Loading

iamgabrielma Jul 15, 2024

soundsokay Jul 16, 2024

iamgabrielma Jul 18, 2024 •

edited

Loading

soundsokay Jul 21, 2024

iamgabrielma Jul 15, 2024

soundsokay Jul 16, 2024

iamgabrielma Jul 15, 2024

soundsokay Jul 16, 2024

iamgabrielma Jul 15, 2024

bozidarsevo left a comment

staskus left a comment

soundsokay commented Jul 22, 2024

soundsokay commented Jul 23, 2024

staskus commented Jul 23, 2024

staskus left a comment

soundsokay commented Jul 23, 2024

bozidarsevo commented Jul 23, 2024

soundsokay commented Jul 23, 2024

		paymentState: PaymentState = .acceptingCard,
		isSyncingOrder: Bool = false) {

[Woo POS] Add test coverage for PointOfSaleDashboardViewModel #13310

[Woo POS] Add test coverage for PointOfSaleDashboardViewModel #13310

Conversation

soundsokay commented Jul 12, 2024 • edited Loading

Description

Steps to reproduce

Testing information

Screenshots

dangermattic commented Jul 12, 2024 • edited Loading

wpmobilebot commented Jul 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamgabrielma left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamgabrielma Jul 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bozidarsevo left a comment

Choose a reason for hiding this comment

staskus left a comment

Choose a reason for hiding this comment

soundsokay commented Jul 22, 2024

soundsokay commented Jul 23, 2024

staskus commented Jul 23, 2024

staskus left a comment

Choose a reason for hiding this comment

soundsokay commented Jul 23, 2024

bozidarsevo commented Jul 23, 2024

soundsokay commented Jul 23, 2024

soundsokay commented Jul 12, 2024 •

edited

Loading

dangermattic commented Jul 12, 2024 •

edited

Loading

wpmobilebot commented Jul 12, 2024 •

edited

Loading

iamgabrielma left a comment •

edited

Loading

iamgabrielma Jul 18, 2024 •

edited

Loading