[Woo POS][Cash & Receipts] Navigation via navigation component to the cash payment screen #13069

kidinov · 2024-12-04T16:28:53Z

Closes: #13063

Description

The PR moves mock of the cash payment state to a separate screen. The screen now opened using compose navigation

Testing information

More -> POS -> Add product -> Checkout
Click on take payment button
Notice a new screen is opened
Navigate back

The tests that have been performed

Above

Images/gif

12-05--14-46.mp4

I have considered if this change warrants release notes and have added them to RELEASE-NOTES.txt if necessary. Use the "[Internal]" label for non-user-facing changes.

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:

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.
Ensure Adequate Unit Test Coverage: The changes are reasonably covered by unit tests or an explanation is provided in the PR description.
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 big (tablet) and small (phone) in case of UI changes, and no regressions are added.

dangermattic · 2024-12-04T16:29:37Z

	5 Warnings
⚠️	This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.
⚠️	Class `WooPosCashPaymentRepository` is missing tests, but `unit-tests-exemption` label was set to ignore this.
⚠️	Class `WooPosCashPaymentState` is missing tests, but `unit-tests-exemption` label was set to ignore this.
⚠️	Class `WooPosCashPaymentUIEvent` is missing tests, but `unit-tests-exemption` label was set to ignore this.
⚠️	Class `WooPosCashPaymentViewModel` is missing tests, but `unit-tests-exemption` label was set to ignore this.

Generated by 🚫 Danger

wpmobilebot · 2024-12-04T16:33:16Z

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

	App Name	WooCommerce-Wear Android
	Platform	⌚️ Wear OS
	Flavor	Jalapeno
	Build Type	Debug
	Commit	`a2b4711`
	Direct Download	`woocommerce-wear-prototype-build-pr13069-a2b4711.apk`

wpmobilebot · 2024-12-04T16:48:40Z

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

	App Name	WooCommerce Android
	Platform	📱 Mobile
	Flavor	Jalapeno
	Build Type	Debug
	Commit	`a2b4711`
	Direct Download	`woocommerce-prototype-build-pr13069-a2b4711.apk`

kidinov · 2024-12-05T13:45:01Z

...rce/src/main/kotlin/com/woocommerce/android/ui/woopos/cashpayment/WooPosCashPaymentScreen.kt

+import com.woocommerce.android.ui.woopos.root.navigation.WooPosNavigationEvent
+
+@Composable
+fun WooPosCashPaymentScreen(onNavigationEvent: (WooPosNavigationEvent) -> Unit) {


Mock of the UI as we still have no design ready

kidinov · 2024-12-05T13:45:33Z

.../src/main/kotlin/com/woocommerce/android/ui/woopos/cashpayment/WooPosCashPaymentViewModel.kt

+    val state: StateFlow<WooPosCashPaymentState> = _state
+
+    init {
+        viewModelScope.launch {


As there is no finilized design yet, I don't know the state needed, therefore not unit tests

…complition-call-from-the-mocked-cash-payment-screen # Conflicts: # WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/home/totals/WooPosTotalsViewModel.kt # WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/home/totals/WooPosTotalsViewModelTest.kt

codecov-commenter · 2024-12-09T11:23:09Z

Codecov Report

Attention: Patch coverage is 8.43373% with 76 lines in your changes missing coverage. Please review.

Project coverage is 40.44%. Comparing base (9731223) to head (a2b4711).
Report is 68 commits behind head on trunk.

Files with missing lines	Patch %	Lines
...i/woopos/cashpayment/WooPosCashPaymentViewModel.kt	0.00%	23 Missing ⚠️
.../woopos/cashpayment/WooPosCashPaymentNavigation.kt	0.00%	20 Missing ⚠️
.../woopos/cashpayment/WooPosCashPaymentRepository.kt	0.00%	9 Missing ⚠️
...id/ui/woopos/cashpayment/WooPosCashPaymentState.kt	0.00%	8 Missing ⚠️
...rce/android/ui/woopos/home/WooPosHomeNavigation.kt	0.00%	6 Missing ⚠️
.../ui/woopos/cashpayment/WooPosCashPaymentUIEvent.kt	0.00%	2 Missing ⚠️
...oid/ui/woopos/home/totals/WooPosTotalsViewModel.kt	60.00%	2 Missing ⚠️
...ui/woopos/root/navigation/WooPosNavigationEvent.kt	0.00%	2 Missing ⚠️
...os/root/navigation/WooPosNavigationEventHandler.kt	0.00%	2 Missing ⚠️
...oid/ui/woopos/home/totals/WooPosTotalsViewState.kt	80.00%	1 Missing ⚠️
... and 1 more

Additional details and impacted files

@@             Coverage Diff              @@
##              trunk   #13069      +/-   ##
============================================
- Coverage     40.49%   40.44%   -0.05%     
+ Complexity     6188     6187       -1     
============================================
  Files          1287     1292       +5     
  Lines         74207    74270      +63     
  Branches      10156    10159       +3     
============================================
- Hits          30050    30039      -11     
- Misses        41549    41623      +74     
  Partials       2608     2608

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

AnirudhBhat

Thanks for working on this @kidinov. The navigation works as expected. I've left few nitpicks which are not blocking for the merge.

I also observed an issue with navigation where successive clicks on "take cash payment" button opens the cash payment screen twice.

cash_payment_issue.mp4

AnirudhBhat · 2024-12-10T05:34:26Z

...erce/src/main/kotlin/com/woocommerce/android/ui/woopos/cashpayment/WooPosCashPaymentState.kt

+        val enteredAmount: String,
+        val changeDue: String,
+        val total: String,
+        val canBeOrderBeCompleted: Boolean,


❓ nitpick: canBeOrderBeCompleted naming sounds confusing. Should it be canOrderBeCompleted?

Good catch. That's val is gone in the following PR where the actual state based on the design apepared

AnirudhBhat · 2024-12-10T07:12:10Z

...src/main/kotlin/com/woocommerce/android/ui/woopos/cashpayment/WooPosCashPaymentNavigation.kt

+private const val ROUTE = "cash_payment/{orderId}"
+
+fun NavController.navigateToCashPaymentScreen(orderId: Long) {
+    navigate("cash_payment/$orderId")


nitpick: Using ROUTE.replace("{orderId}", orderId.toString()) would improve maintainability, avoiding mismatches between the navigation declaration and usage.

Good point. Done here already https://github.com/woocommerce/woocommerce-android/pull/13086/files#diff-2ce0f6b51524bb4e82ea8632029e81796be47e5ab5b0c82cafeccb32e4df103dL16

AnirudhBhat · 2024-12-10T09:14:10Z

...rce/src/main/kotlin/com/woocommerce/android/ui/woopos/cashpayment/WooPosCashPaymentScreen.kt

+    ) {
+        Column(
+            modifier = Modifier
+                .width(540.dp)


❓ Do we need to hardcode this width here?

Nope. But as I mentioned this is a mock. The actual UI is implemented in the following PR

AnirudhBhat · 2024-12-10T09:15:46Z

...rce/src/main/kotlin/com/woocommerce/android/ui/woopos/cashpayment/WooPosCashPaymentScreen.kt

+
+@WooPosPreview
+@Composable
+fun WooPosTotalsPaymentCashScreenScreen() {


❓ Should the name be WooPosTotalsPaymentCashScreenPreview?

Also, since it's actually previewing WooPosCashPaymentScreen. What do you think about calling it WooPosCashPaymentScreenPreview instead?

Good catch! Fixed here f3412b4

AnirudhBhat · 2024-12-10T09:21:03Z

.../src/main/kotlin/com/woocommerce/android/ui/woopos/cashpayment/WooPosCashPaymentViewModel.kt

+        viewModelScope.launch {
+            val order = repository.getOrderById(orderId)!!
+            _state.value = WooPosCashPaymentState.Collecting(
+                enteredAmount = "",


nitpick: enteredAmount is initialised as an empty string, and changeDue as BigDecimal.ZERO, which might not align with realistic scenarios. We could consider initialising these values based on prior state.

Yeah, it's all mocks, as there were not design. It's redone all completely now in here

https://github.com/woocommerce/woocommerce-android/pull/13086/files#diff-14edc5b8f39e9641072949511e60cc8208bb196a6bbab3b8b5283bb2ab052e88

kidinov added 5 commits December 4, 2024 15:51

Cash payments toolbar

67ceb72

Moved cash payment related things

71409df

Steps towards navigation to the cash payments screen

64cfcb3

Returned back OnTakeCashPaymentClicked event

58bca21

Compilable navigation

500ecb2

kidinov marked this pull request as draft December 4, 2024 16:28

kidinov added 7 commits December 5, 2024 09:37

Pass order id as navigation route

ced0c7e

Show mocked ui on the new screen

03317bc

Navigation back

dc9ea59

Padding to match cart toolbar

0ecc857

Improved animation

b092157

Fixed formatting

66c5917

Fixed broken tests

d923603

kidinov added feature: point of sale POS project unit-tests-exemption labels Dec 5, 2024

kidinov commented Dec 5, 2024

View reviewed changes

kidinov marked this pull request as ready for review December 5, 2024 13:47

kidinov requested a review from AnirudhBhat December 5, 2024 13:47

Base automatically changed from 13004-woo-poscash-receipts-add-optional-take-payment-button-and-empty-state-for-cash to trunk December 6, 2024 08:04

kidinov linked an issue Dec 6, 2024 that may be closed by this pull request

[Woo POS][Cash & Receipts] Navigation to the cash payment screen via compose navigation #13063

Closed

kidinov added this to the 21.3 milestone Dec 9, 2024

AnirudhBhat self-assigned this Dec 9, 2024

Fixed a unit test

a2b4711

AnirudhBhat approved these changes Dec 10, 2024

View reviewed changes

kidinov merged commit c891053 into trunk Dec 11, 2024
15 checks passed

kidinov deleted the 13063-woo-poscash-receipts-implement-order-complition-call-from-the-mocked-cash-payment-screen branch December 11, 2024 09:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Woo POS][Cash & Receipts] Navigation via navigation component to the cash payment screen #13069

[Woo POS][Cash & Receipts] Navigation via navigation component to the cash payment screen #13069

kidinov commented Dec 4, 2024 •

edited

Loading

dangermattic commented Dec 4, 2024 •

edited

Loading

wpmobilebot commented Dec 4, 2024 •

edited

Loading

wpmobilebot commented Dec 4, 2024 •

edited

Loading

kidinov Dec 5, 2024

kidinov Dec 5, 2024

codecov-commenter commented Dec 9, 2024 •

edited

Loading

AnirudhBhat left a comment

AnirudhBhat Dec 10, 2024

kidinov Dec 11, 2024

AnirudhBhat Dec 10, 2024

kidinov Dec 11, 2024

AnirudhBhat Dec 10, 2024

kidinov Dec 11, 2024

AnirudhBhat Dec 10, 2024

kidinov Dec 11, 2024

AnirudhBhat Dec 10, 2024

kidinov Dec 11, 2024

[Woo POS][Cash & Receipts] Navigation via navigation component to the cash payment screen #13069

[Woo POS][Cash & Receipts] Navigation via navigation component to the cash payment screen #13069

Conversation

kidinov commented Dec 4, 2024 • edited Loading

Description

Testing information

The tests that have been performed

Images/gif

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

dangermattic commented Dec 4, 2024 • edited Loading

wpmobilebot commented Dec 4, 2024 • edited Loading

wpmobilebot commented Dec 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Dec 9, 2024 • edited Loading

Codecov Report

AnirudhBhat 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

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

kidinov commented Dec 4, 2024 •

edited

Loading

dangermattic commented Dec 4, 2024 •

edited

Loading

wpmobilebot commented Dec 4, 2024 •

edited

Loading

wpmobilebot commented Dec 4, 2024 •

edited

Loading

codecov-commenter commented Dec 9, 2024 •

edited

Loading