[Woo POS][Non-Simple Products] Remove null price check on Products and Variations screen #13312

AnirudhBhat · 2025-01-15T08:58:02Z

Description

This PR removes null price filtering we've had for some time on client side. Since we have decided to allow free products to be displayed on POS, we are allowing products whose price are not set to be allowed to display as well considering those to be free products.

Along with this, this PR also hides card payment screen on checkout if the cart items contain only free products.

discussion: p1736504688523299/1736484154.214279-slack-C070SJRA8DP

Steps to reproduce

Empty price

Ensure you have products and variations whose price are not set (empty price)
Navigate to POS (more menu -> POS)
Ensure you see the product whose price is empty in the list
Ensure you see the variation whose price is empty in the variations screen

Zero price

Ensure you have products and variations whose price are set to zero
Navigate to POS (more menu -> POS)
Ensure you see the product whose price is zero in the list
Ensure you see the variation whose price is zero in the variations screen

Checkout screen for free products

Ensure you have products and variations whose price are set to zero
Navigate to POS (more menu -> POS)
Add product whose price is set to 0 to cart
Click on checkout and navigate to checkout screen
Ensure you don't see card payment UI here. Just the cash payment screen
Ensure you can complete cash payment successfully

Checkout screen for non-free products

Navigate to POS (more menu -> POS)
Add product whose price is set to other than 0 to cart
Click on checkout and navigate to checkout screen
Ensure you see card payment UI along with the cash payment screen
Ensure you can complete card payment successfully

The tests that have been performed

Tested the above scenarios

Images/gif

free_products.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.

# Conflicts: # WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/home/items/products/WooPosProductsDataSource.kt

wpmobilebot · 2025-01-15T09:08:47Z

📲 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	`a953166`
	Direct Download	`woocommerce-wear-prototype-build-pr13312-a953166.apk`

wpmobilebot · 2025-01-15T09:23:57Z

📲 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	`a953166`
	Direct Download	`woocommerce-prototype-build-pr13312-a953166.apk`

codecov-commenter · 2025-01-15T09:28:06Z

Codecov Report

Attention: Patch coverage is 42.85714% with 4 lines in your changes missing coverage. Please review.

Project coverage is 41.09%. Comparing base (888b2e9) to head (a953166).
Report is 44 commits behind head on trunk.

Files with missing lines	Patch %	Lines
...os/home/items/products/WooPosProductsDataSource.kt	0.00%	0 Missing and 2 partials ⚠️
...ome/items/variations/WooPosVariationsDataSource.kt	0.00%	0 Missing and 1 partial ⚠️
...oid/ui/woopos/home/totals/WooPosTotalsViewModel.kt	66.66%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##              trunk   #13312      +/-   ##
============================================
- Coverage     41.10%   41.09%   -0.01%     
+ Complexity     6421     6417       -4     
============================================
  Files          1321     1321              
  Lines         77177    77176       -1     
  Branches      10643    10645       +2     
============================================
- Hits          31722    31718       -4     
  Misses        42646    42646              
- Partials       2809     2812       +3

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

samiuelson

Works well, @AnirudhBhat! Please take a look at a few code improvement ideas I left.

samiuelson · 2025-01-15T18:17:14Z

...merce/src/main/kotlin/com/woocommerce/android/ui/woopos/home/totals/WooPosTotalsViewState.kt

@@ -12,6 +12,7 @@ sealed class WooPosTotalsViewState : Parcelable {
        val orderTaxText: String,
        val orderTotalText: String,
        val readerStatus: ReaderStatus,
+        val isFreeOrder: Boolean,


💡 The UI doesn’t really care if the order is free; it only cares whether the reader status is shown or hidden. The state model would be cleaner and more readable if, instead of adding an isFreeOrder property to WooPosTotalsViewState, we changed ReaderStatus to reflect when we don’t want it displayed.

For example, we could add a data object Hidden: ReaderStatus() subclass, rather than relying on WooPosTotalsViewState.isFreeOrder. This approach would also simplify the UI code. What do you think?

💡 It would also be great to add a unit test for orders that include only free products.

Thanks for the suggestion. I think this PR of yours already does this change. What do you think of making my change based on your PR?

After thinking more about this, I feel adding a new Hidden status to readerStatus field:

Introduces complexity in managing a Hidden status, as it’s an artificial state not tied to the actual reader.

Requires careful handling in the WooPosTotalsViewModel to ensure the Hidden status transitions correctly, especially when toggling between free orders and paid orders.

Complicates UI logic to correctly display the cash payment screen when the card reader UI is hidden.

We could probably rename isFreeOrder into shouldShowReaderStatus. WDYT?

Feel free to take the approach you think is best 👍

It would also be great to add a unit test for orders that include only free products.

Good suggestion! I've added it here

57c3d64

a953166

samiuelson · 2025-01-15T18:18:40Z

...merce/src/main/kotlin/com/woocommerce/android/ui/woopos/home/totals/WooPosTotalsViewModel.kt

-            if (cardReaderFacade.readerStatus.value is Connected) {
+            if (
+                cardReaderFacade.readerStatus.value is Connected &&
+                dataState.value.orderTotal?.compareTo(BigDecimal.ZERO) == 1


💡 I suggest using > and < operators directly improve code readability.

I believe this approach won't work in all cases when dealing with BigDecimal, because these operators internally rely on the exact scale of the BigDecimal values during comparisons. For example:

If totalAmount is BigDecimal("0.00"), it may not behave as expected when compared to BigDecimal.ZERO using > or <, as the comparison might fail due to scale differences.
Using .compareTo(BigDecimal.ZERO) is more robust because it ignores scale differences and focuses solely on the numerical value, ensuring consistent behaviour regardless of how totalAmount is constructed (e.g., 0.00 vs. 0).

I believe this approach won't work in all cases when dealing with BigDecimal, because these operators internally rely on the exact scale of the BigDecimal values during comparisons.

💡 I think this is not true, according to docs:

All comparisons are translated into calls to compareTo, which is required to return Int.

As a result, using comparison operators translates to the same call to the compareTo() function that is used currently.

You can verify that by CMD-clicking the comparison operator (>) – it will redirect to the underlying compareTo function implementation in Kotlin std lib.

I suggest using comparisons like that:

Suggested change

dataState.value.orderTotal?.compareTo(BigDecimal.ZERO) == 1

dataState.value.orderTotal!! > BigDecimal.ZERO

which compiles to:

dataState.value.orderTotal!!.compareTo(BigDecimal.ZERO) > 0

Let me know what you think or if I'm missing something @AnirudhBhat.

💡 Another improvement would be to add a null check (throw IllegalStateException in case orderTotal) instead of allowing such illegal state in code (comparing potentially null value which will result in NPE thrown from compareTo function anyway, but throwing it earlier would make potential bug easier to debug).

…te appropriately.

… state appropriately.

AnirudhBhat added 8 commits January 13, 2025 08:57

Remove price filter from products screen

3534925

Remove price filter from variations screen

1695808

Remove price filter unit test from products screen

6be3917

Remove price filter unit test from variation screen

cc201eb

Add a boolean param to indicate whether the order is free or not

f62b5fe

Update view state when there is free order added to cart

8271098

Hide card payment screen for free orders

a61c31e

Change logic to identify whether the order is free or not.

5e38fc5

AnirudhBhat added type: task An internally driven task. feature: point of sale POS project labels Jan 15, 2025

AnirudhBhat added this to the 21.5 milestone Jan 15, 2025

AnirudhBhat requested a review from samiuelson January 15, 2025 08:58

AnirudhBhat added 3 commits January 15, 2025 14:30

Merge branch 'refs/heads/trunk' into issue/13293-remove-null-price-check

243579b

# Conflicts: # WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/home/items/products/WooPosProductsDataSource.kt

Remove unnecessary tests

601ad2e

Remove unused import

9dfff65

samiuelson self-assigned this Jan 15, 2025

samiuelson approved these changes Jan 15, 2025

View reviewed changes

AnirudhBhat added 2 commits January 16, 2025 16:26

Add test to verify free products in the cart updates the checkout sta…

57c3d64

…te appropriately.

Add test to verify non-free products in the cart updates the checkout…

a953166

… state appropriately.

AnirudhBhat enabled auto-merge January 16, 2025 11:04

AnirudhBhat merged commit 07de8ee into trunk Jan 16, 2025
15 checks passed

AnirudhBhat deleted the issue/13293-remove-null-price-check branch January 16, 2025 11:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Woo POS][Non-Simple Products] Remove null price check on Products and Variations screen #13312

[Woo POS][Non-Simple Products] Remove null price check on Products and Variations screen #13312

AnirudhBhat commented Jan 15, 2025 •

edited by samiuelson

Loading

wpmobilebot commented Jan 15, 2025 •

edited

Loading

wpmobilebot commented Jan 15, 2025 •

edited

Loading

codecov-commenter commented Jan 15, 2025 •

edited

Loading

samiuelson left a comment

samiuelson Jan 15, 2025

samiuelson Jan 15, 2025

AnirudhBhat Jan 16, 2025 •

edited

Loading

AnirudhBhat Jan 16, 2025

samiuelson Jan 16, 2025

AnirudhBhat Jan 16, 2025

samiuelson Jan 15, 2025

AnirudhBhat Jan 16, 2025

samiuelson Jan 16, 2025

	dataState.value.orderTotal?.compareTo(BigDecimal.ZERO) == 1
	dataState.value.orderTotal!! > BigDecimal.ZERO

[Woo POS][Non-Simple Products] Remove null price check on Products and Variations screen #13312

[Woo POS][Non-Simple Products] Remove null price check on Products and Variations screen #13312

Conversation

AnirudhBhat commented Jan 15, 2025 • edited by samiuelson Loading

Description

Steps to reproduce

Empty price

Zero price

Checkout screen for free products

Checkout screen for non-free products

The tests that have been performed

Images/gif

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

wpmobilebot commented Jan 15, 2025 • edited Loading

wpmobilebot commented Jan 15, 2025 • edited Loading

codecov-commenter commented Jan 15, 2025 • edited Loading

Codecov Report

samiuelson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnirudhBhat Jan 16, 2025 • 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

AnirudhBhat commented Jan 15, 2025 •

edited by samiuelson

Loading

wpmobilebot commented Jan 15, 2025 •

edited

Loading

wpmobilebot commented Jan 15, 2025 •

edited

Loading

codecov-commenter commented Jan 15, 2025 •

edited

Loading

AnirudhBhat Jan 16, 2025 •

edited

Loading