[Woo POS] Extract totals logic to `TotalsViewModel` #13211

iamgabrielma · 2024-07-01T10:16:16Z

Description

This PR is part 3 (and last) of splitting PointOfSaleDashboardViewModel responsibilities based on the 3 main views in POS:

Item selector
Cart view
Totals view (this PR)

As order syncing and payment collection work hand in hand, the move of both parts is in the same PR to also unblock other work affected by the view models like Bozidar's UI updates #13166.

Most of the diffs move code from PointOfSaleDashboardViewModel to TotalsViewModel with minimum changes and reorganization in the file to privatize the methods as much as possible.

TotalsView still depends on PointOfSaleDashboardViewModel for starting a new transaction and to provide the cart items & POS items for calculating the total amount manually after an order sync failure, we can consider refactoring in the future.

Changes

Created TotalsViewModel, which encapsulates most of totals and payments logic
CurrencyFormatter is injected from the app to the pos through the entry point
Cleanup of unused code
Added empty preliminary list of unit test, that will be tackled on a separate PR

Testing

Prerequisite: the store is eligible for POS and the feature switch is enabled in Menu > Settings > Experimental Features > POS

Launch app
Go to Menu > Point of Sale
Add an item (with a non-zero price and ideally with tax applied on checkout) to cart
Tap Checkout --> the order total should be updated to a correct, non-zero value
Tap Collect Payment and proceed to tap a card to pay --> the transaction should succeed
Tap New transaction and repeat the above steps --> the second transaction should succeed
Tap Exit POS and go to the orders tab --> the two orders should be shown as completed

dangermattic · 2024-07-01T10:16:51Z

	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.

	1 Message
📖	This PR is still a Draft: some checks will be skipped.

Generated by 🚫 Danger

wpmobilebot · 2024-07-01T10:25:42Z

📲 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	pr13211-2e8b444
	Version	19.3
	Bundle ID	com.automattic.alpha.woocommerce
	Commit	`2e8b444`
	App Center Build	WooCommerce - Prototype Builds #9830

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

* trunk: Add a test case on `CartViewModel.removeItemFromCart`. Update tests Always fetch GLA from remote instead of relying on storage Update slug for GLA Update tests for DefaultGoogleAdsEligibilityChecker Add a comment regarding the plugin version limit Add tests for DefaultGoogleAdsEligibilityChecker Update MockFeatureFlagService Check google ads connection for Google ads Add GoogleAdsEligibilityChecker Add new feature flag googleAdsCampaignCreationOnWebView

…talsViewModel`.

…h as possible.

jaclync · 2024-07-01T17:58:29Z

WooCommerce/Classes/POS/ViewModels/TotalsViewModel.swift

+ var isShimmering: Bool {
+ isSyncingOrder
+ }
+
+ var isPriceFieldRedacted: Bool {
+ formattedOrderTotalTaxPrice == nil || isSyncingOrder
+ }
+
+ var isTotalPriceFieldRedacted: Bool {
+ formattedOrderTotalPrice == nil || isSyncingOrder
+ }


I like these descriptive view properties 💯

jaclync · 2024-07-01T18:01:50Z

WooCommerce/Classes/POS/Presentation/TotalsView.swift

@@ -78,10 +95,6 @@ struct TotalsView: View {
 ]
 }
 }
-
- private var paymentButtonsDisabled: Bool {


This appears to be unused, thanks for cleaning it up.

jaclync · 2024-07-01T18:05:22Z

WooCommerce/WooCommerceTests/POS/ViewModels/TotalsViewModelTests.swift

@@ -0,0 +1,15 @@
+import XCTest
+
+final class TotalsViewModelTests: XCTestCase {


@iamgabrielma 👋 since the PR is already pretty big, maybe we can add the test cases in a separate PR?

That sounds good, I've opened a separate issue so we can handle those: #13217

iamgabrielma · 2024-07-02T01:50:10Z

WooCommerce/Classes/POS/ViewModels/TotalsViewModel.swift

+ allProducts: allItems)
+ self.order = order
+ isSyncingOrder = false
+ // TODO: this is temporary solution


@jaclync Do you know if there is any P2/thread/reference to what the non-temporary solution should be? We can add a link here for context.

Good question, this TODO comment was in trunk before this PR. I just checked the history, It looks like it's from this PR comment: https://github.com/woocommerce/woocommerce-ios/pull/13099/files#r1647370089

@bozidarsevo would you like to share a link here if there is a GitHub issue or some reference about the following plan?

bozidarsevo

Looks good! :)

staskus

Looks good, test cases also work well 👍

bozidarsevo · 2024-07-02T13:12:17Z

WooCommerce/Classes/POS/Presentation/PointOfSaleDashboardView.swift

@@ -39,19 +39,6 @@ struct PointOfSaleDashboardView: View {
 }
 .toolbarBackground(Color.toolbarBackground, for: .bottomBar)
 .toolbarBackground(.visible, for: .bottomBar)
- .sheet(isPresented: $viewModel.showsCardReaderSheet, content: {


This needs to be here because now the "Connect now" button in toolbar does not work.

now it only works if the totals are presented.

Ohh great catch! I will fix this today.

I was looking at this now and even if I change to how it was the sheet does not show, did we change something behind the scenes on how we set showsCardReaderSheet?

I was looking at this now and even if I change to how it was the sheet does not show, did we change something behind the scenes on how we set showsCardReaderSheet?

It does work for me with the changes on #13226, can you check if you can still reproduce the issue on that branch?

iamgabrielma added 2 commits July 1, 2024 16:50

Create TotalsViewModel and extract sync. Unit tests

a1f5322

Move methods from dashboard to TotalsViewModel

701dda3

iamgabrielma added type: task An internally driven task. feature: POS labels Jul 1, 2024

Pass items from cart to calculate and sync order

2944fa9

jaclync mentioned this pull request Jul 1, 2024

[Woo POS] Extract cart logic to CartViewModel #13191

Merged

Base automatically changed from task/12998-pos-extract-cart-vm to trunk July 1, 2024 15:01

jaclync added 6 commits July 1, 2024 11:17

Move derived variables from TotalsView to TotalsViewModel.

4fe2aed

Move payment related code from PointOfSaleDashboardViewModel to `To…

c56f34e

…talsViewModel`.

Remove unused POSOrderService.updateOrderStatus.

f3c427a

Minor polishes to privatize and remove dashboard VM dependency as muc…

85a0626

…h as possible.

Fix unit testing build failure.

2e8b444

jaclync reviewed Jul 1, 2024

View reviewed changes

jaclync added this to the 19.4 milestone Jul 1, 2024

iamgabrielma mentioned this pull request Jul 2, 2024

[Woo POS] Unit tests for TotalsViewModel #13217

Open

iamgabrielma commented Jul 2, 2024

View reviewed changes

iamgabrielma marked this pull request as ready for review July 2, 2024 01:53

iamgabrielma requested review from bozidarsevo and staskus July 2, 2024 02:34

iamgabrielma mentioned this pull request Jul 2, 2024

[WOO POS] update UI for payment processing content in checkout #13166

Merged

1 task

bozidarsevo approved these changes Jul 2, 2024

View reviewed changes

staskus approved these changes Jul 2, 2024

View reviewed changes

iamgabrielma merged commit 1d36ce4 into trunk Jul 2, 2024
23 checks passed

iamgabrielma deleted the task/12998-pos-extract-totals-vm branch July 2, 2024 08:45

bozidarsevo reviewed Jul 2, 2024

View reviewed changes

jaclync mentioned this pull request Jul 2, 2024

[Woo POS] Fix reader connection alerts not showing up when connecting reader from the bottom bar #13226

Merged

3 tasks

This was referenced Jul 4, 2024

[Woo POS] Split PointOfSaleDashboardViewModel responsibilities #12998

Closed

[Woo POS] Extract CartViewModel and TotalsViewModel #13179

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Woo POS] Extract totals logic to `TotalsViewModel` #13211

[Woo POS] Extract totals logic to `TotalsViewModel` #13211

iamgabrielma commented Jul 1, 2024 •

edited

Loading

dangermattic commented Jul 1, 2024 •

edited

Loading

wpmobilebot commented Jul 1, 2024 •

edited

Loading

jaclync Jul 1, 2024

jaclync Jul 1, 2024

jaclync Jul 1, 2024

iamgabrielma Jul 2, 2024

iamgabrielma Jul 2, 2024

jaclync Jul 2, 2024

bozidarsevo left a comment

staskus left a comment

bozidarsevo Jul 2, 2024

bozidarsevo Jul 2, 2024

jaclync Jul 2, 2024

bozidarsevo Jul 2, 2024

iamgabrielma Jul 3, 2024

		@@ -0,0 +1,15 @@
		import XCTest

		final class TotalsViewModelTests: XCTestCase {

[Woo POS] Extract totals logic to TotalsViewModel #13211

[Woo POS] Extract totals logic to TotalsViewModel #13211

Conversation

iamgabrielma commented Jul 1, 2024 • edited Loading

Description

Changes

Testing

dangermattic commented Jul 1, 2024 • edited Loading

wpmobilebot commented Jul 1, 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

bozidarsevo left a comment

Choose a reason for hiding this comment

staskus left a 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

[Woo POS] Extract totals logic to `TotalsViewModel` #13211

[Woo POS] Extract totals logic to `TotalsViewModel` #13211

iamgabrielma commented Jul 1, 2024 •

edited

Loading

dangermattic commented Jul 1, 2024 •

edited

Loading

wpmobilebot commented Jul 1, 2024 •

edited

Loading