Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 13 commits into from
Jan 16, 2025
Merged

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

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
3534925
Remove price filter from products screen
AnirudhBhat Jan 13, 2025
1695808
Remove price filter from variations screen
AnirudhBhat Jan 13, 2025
6be3917
Remove price filter unit test from products screen
AnirudhBhat Jan 13, 2025
cc201eb
Remove price filter unit test from variation screen
AnirudhBhat Jan 13, 2025
f62b5fe
Add a boolean param to indicate whether the order is free or not
AnirudhBhat Jan 15, 2025
8271098
Update view state when there is free order added to cart
AnirudhBhat Jan 15, 2025
a61c31e
Hide card payment screen for free orders
AnirudhBhat Jan 15, 2025
5e38fc5
Change logic to identify whether the order is free or not.
AnirudhBhat Jan 15, 2025
243579b
Merge branch 'refs/heads/trunk' into issue/13293-remove-null-price-check
AnirudhBhat Jan 15, 2025
601ad2e
Remove unnecessary tests
AnirudhBhat Jan 15, 2025
9dfff65
Remove unused import
AnirudhBhat Jan 15, 2025
57c3d64
Add test to verify free products in the cart updates the checkout sta…
AnirudhBhat Jan 16, 2025
a953166
Add test to verify non-free products in the cart updates the checkout…
AnirudhBhat Jan 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 2 additions & 10 deletions .../kotlin/com/woocommerce/android/ui/woopos/home/items/products/WooPosProductsDataSource.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package com.woocommerce.android.ui.woopos.home.items.products

import com.woocommerce.android.model.Product
import com.woocommerce.android.ui.products.ProductStatus
import com.woocommerce.android.ui.products.ProductType.VARIABLE
import com.woocommerce.android.ui.products.selector.ProductListHandler
import com.woocommerce.android.util.WooLog
import kotlinx.coroutines.Dispatchers
Expand Down Expand Up @@ -46,7 +45,7 @@ class WooPosProductsDataSource @Inject constructor(
)

if (result.isSuccess) {
val remoteProducts = handler.productsFlow.first().applyPosProductFilter()
val remoteProducts = handler.productsFlow.first()
updateProductCache(remoteProducts)
emit(ProductsResult.Remote(Result.success(productCache)))
} else {
Expand All @@ -64,7 +63,7 @@ class WooPosProductsDataSource @Inject constructor(
suspend fun loadMore(): Result<List<Product>> = withContext(Dispatchers.IO) {
val result = handler.loadMore()
if (result.isSuccess) {
val moreProducts = handler.productsFlow.first().applyPosProductFilter()
val moreProducts = handler.productsFlow.first()
updateProductCache(moreProducts)
Result.success(productCache)
} else {
Expand All @@ -83,13 +82,6 @@ class WooPosProductsDataSource @Inject constructor(
WooLog.e(WooLog.T.POS, "Loading products failed - $errorMessage", error)
}

private fun List<Product>.applyPosProductFilter() = this.filter { product ->
isProductHasAPrice(product) || product.productType == VARIABLE
}

private fun isProductHasAPrice(product: Product) =
(product.price != null)

sealed class ProductsResult {
data class Cached(val products: List<Product>) : ProductsResult()
data class Remote(val productsResult: Result<List<Product>>) : ProductsResult()
Expand Down
2 changes: 1 addition & 1 deletion ...lin/com/woocommerce/android/ui/woopos/home/items/variations/WooPosVariationsDataSource.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,5 +105,5 @@ sealed class FetchResult {
}

private fun List<ProductVariation>.applyFilter(): List<ProductVariation> {
return filter { it.price != null && !it.isDownloadable }
return filter { !it.isDownloadable }
}
71 changes: 49 additions & 22 deletions ...merce/src/main/kotlin/com/woocommerce/android/ui/woopos/home/totals/WooPosTotalsScreen.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,26 +151,28 @@ private fun TotalsLoaded(
horizontalAlignment = Alignment.CenterHorizontally,
verticalArrangement = Arrangement.Center,
) {
Column(
modifier = Modifier
.fillMaxWidth()
.weight(1.1f)
.background(WooPosTheme.colors.totalsErrorBackground),
horizontalAlignment = Alignment.CenterHorizontally,
verticalArrangement = Arrangement.Center,
) {
when (val readerStatus = state.readerStatus) {
is WooPosTotalsViewState.ReaderStatus.Disconnected -> {
ReaderDisconnected(modifier = Modifier, status = readerStatus, onUIEvent = onUIEvent)
}

is WooPosTotalsViewState.ReaderStatus.Preparing,
is WooPosTotalsViewState.ReaderStatus.CheckingOrder -> {
PreparingReader(readerStatus)
}

is WooPosTotalsViewState.ReaderStatus.ReadyForPayment -> {
ReaderReadyForPayment(readerStatus)
if (!state.isFreeOrder) {
Column(
modifier = Modifier
.fillMaxWidth()
.weight(1.1f)
.background(WooPosTheme.colors.totalsErrorBackground),
horizontalAlignment = Alignment.CenterHorizontally,
verticalArrangement = Arrangement.Center,
) {
when (val readerStatus = state.readerStatus) {
is WooPosTotalsViewState.ReaderStatus.Disconnected -> {
ReaderDisconnected(modifier = Modifier, status = readerStatus, onUIEvent = onUIEvent)
}

is WooPosTotalsViewState.ReaderStatus.Preparing,
is WooPosTotalsViewState.ReaderStatus.CheckingOrder -> {
PreparingReader(readerStatus)
}

is WooPosTotalsViewState.ReaderStatus.ReadyForPayment -> {
ReaderReadyForPayment(readerStatus)
}
}
}
}
Expand Down Expand Up @@ -418,6 +420,7 @@ fun WooPosTotalsScreenPreview(modifier: Modifier = Modifier) {
title = "Ready for payment",
subtitle = "Tap, swipe or insert card"
),
isFreeOrder = false
),
onUIEvent = {},
)
Expand All @@ -438,7 +441,8 @@ fun WooPosTotalsScreenPreviewReaderNotConnected(modifier: Modifier = Modifier) {
title = "Reader not connected",
subtitle = "To process this payment, please connect your reader.",
actionButtonLabel = "Connect to a reader",
)
),
isFreeOrder = false
),
onUIEvent = {},
)
Expand All @@ -459,7 +463,30 @@ fun WooPosTotalsScreenPreviewWithCashPaymentAvailable() {
title = "Reader not connected",
subtitle = "To process this payment, please connect your reader.",
actionButtonLabel = "Connect to a reader",
)
),
isFreeOrder = false
),
onUIEvent = {},
)
}
}

@Composable
@WooPosPreview
fun WooPosTotalsScreenPreviewForFreeOrders() {
WooPosTheme {
WooPosTotalsScreen(
modifier = Modifier,
state = WooPosTotalsViewState.Totals(
orderSubtotalText = "$420.00",
orderTotalText = "$462.00",
orderTaxText = "$42.00",
readerStatus = WooPosTotalsViewState.ReaderStatus.Disconnected(
title = "Reader not connected",
subtitle = "To process this payment, please connect your reader.",
actionButtonLabel = "Connect to a reader",
),
isFreeOrder = true
),
onUIEvent = {},
)
Expand Down
7 changes: 6 additions & 1 deletion ...ce/src/main/kotlin/com/woocommerce/android/ui/woopos/home/totals/WooPosTotalsViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ class WooPosTotalsViewModel @Inject constructor(
uiState.value = state.copy(readerStatus = buildTotalsReaderNotConnectedError())
cancelPaymentAction()
}

is Connected -> {
val state = uiState.value
if (state !is WooPosTotalsViewState.Totals) return@collect
Expand Down Expand Up @@ -253,7 +254,10 @@ class WooPosTotalsViewModel @Inject constructor(
} else {
val orderId = dataState.value.orderId
check(orderId != EMPTY_ORDER_ID)
if (cardReaderFacade.readerStatus.value is Connected) {
if (
cardReaderFacade.readerStatus.value is Connected &&
dataState.value.orderTotal?.compareTo(BigDecimal.ZERO) == 1
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

) {
val state = uiState.value
check(state is WooPosTotalsViewState.Totals)
check(uiState.value is WooPosTotalsViewState.Totals)
Expand Down Expand Up @@ -456,6 +460,7 @@ class WooPosTotalsViewModel @Inject constructor(
orderTaxText = priceFormat(taxAmount),
orderTotalText = priceFormat(totalAmount),
readerStatus = readerStatus,
isFreeOrder = totalAmount.compareTo(BigDecimal.ZERO) == 0
)
}

Expand Down
1 change: 1 addition & 0 deletions ...ce/src/main/kotlin/com/woocommerce/android/ui/woopos/home/totals/WooPosTotalsViewState.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ sealed class WooPosTotalsViewState : Parcelable {
val orderTaxText: String,
val orderTotalText: String,
val readerStatus: ReaderStatus,
val isFreeOrder: Boolean,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@AnirudhBhat AnirudhBhat Jan 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. Introduces complexity in managing a Hidden status, as it’s an artificial state not tied to the actual reader.
  2. Requires careful handling in the WooPosTotalsViewModel to ensure the Hidden status transitions correctly, especially when toggling between free orders and paid orders.
  3. 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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to take the approach you think is best 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

) : WooPosTotalsViewState()

data class PaymentSuccess(val orderTotalText: String) : WooPosTotalsViewState()
Expand Down
36 changes: 0 additions & 36 deletions .../test/kotlin/com/woocommerce/android/ui/woopos/home/items/WooPosProductsDataSourceTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,42 +297,6 @@ class WooPosProductsDataSourceTest {
assertFalse(cachedResult.products.any { it.remoteId == 1L })
}

@Test
fun `given remote products, when loadSimpleProducts called, then filter in only products that has price`() =
runTest {
// GIVEN
whenever(handler.canLoadMore).thenReturn(AtomicBoolean(true))
whenever(handler.productsFlow).thenReturn(
flowOf(
listOf(
ProductTestUtils.generateProduct(
productId = 1,
productName = "Product 1",
amount = "",
productType = "simple",
isDownloadable = false,
),
ProductTestUtils.generateProduct(
productId = 2,
productName = "Product 2",
amount = "20.0",
productType = "simple",
isDownloadable = false
).copy(firstImageUrl = "https://test.com")
)
)
)
whenever(handler.loadFromCacheAndFetch(any(), any(), any(), any(), any())).thenReturn(Result.success(Unit))
val sut = WooPosProductsDataSource(handler)

// WHEN
val flow = sut.loadSimpleProducts(forceRefreshProducts = true).toList()

// THEN
val remoteResult = flow[1] as WooPosProductsDataSource.ProductsResult.Remote
assertThat(remoteResult.productsResult.getOrNull()?.any { it.remoteId == 1L }).isFalse()
}

@Test
fun `given cached products, when loadSimpleProducts called, then filter out downloadable products`() =
runTest {
Expand Down
102 changes: 0 additions & 102 deletions ...otlin/com/woocommerce/android/ui/woopos/home/variations/WooPosVariationsDataSourceTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,38 +302,6 @@ class WooPosVariationsDataSourceTest {
assertFalse(cachedResult.data.any { it.remoteVariationId == 1L })
}

@Test
fun `given remote variations, when fetchFirstPage called, then filter in only variations that have price`() = runTest {
// GIVEN
val productId = 1L
whenever(handler.canLoadMore(5)).thenReturn(true)
whenever(handler.getVariationsFlow(productId)).thenReturn(
flowOf(
listOf(
ProductTestUtils.generateProductVariation(
variationId = 1,
amount = "",
),
ProductTestUtils.generateProductVariation(
variationId = 2,
amount = "20.0",
)
)
)
)
whenever(variationsCache.get(productId)).thenReturn(sampleProducts)
whenever(handler.fetchVariations(productId, forceRefresh = true)).thenReturn(Result.success(Unit))
val sut = WooPosVariationsDataSource(handler, variationsCache)

// WHEN
val flow = sut.fetchFirstPage(productId, forceRefresh = true).toList()

// THEN
val remoteResult = flow[1] as FetchResult.Remote

assertThat(remoteResult.result.getOrNull()?.any { it.remoteVariationId == 1L }).isFalse()
}

@Test
fun `given cached variations, when fetchFirstPage called, then filter out virtual variations`() = runTest {
// GIVEN
Expand Down Expand Up @@ -367,74 +335,4 @@ class WooPosVariationsDataSourceTest {

assertFalse(cachedResult.data.any { it.remoteVariationId == 1L })
}

@Test
fun `given cached variations, when fetchFirstPage called, then filter out downloadable variations`() = runTest {
// GIVEN
val productId = 1L
whenever(handler.canLoadMore(5)).thenReturn(false)
whenever(handler.getVariationsFlow(productId)).thenReturn(
flowOf(
listOf(
ProductTestUtils.generateProductVariation(
variationId = 1,
amount = "0",
isDownloadable = true
),
ProductTestUtils.generateProductVariation(
variationId = 2,
amount = "20.0",
isVirtual = false,
isDownloadable = false
)
)
)
)
whenever(handler.fetchVariations(productId, forceRefresh = false)).thenReturn(Result.success(Unit))
whenever(variationsCache.get(productId)).thenReturn(sampleProducts)
val sut = WooPosVariationsDataSource(handler, variationsCache)

// WHEN
val flow = sut.fetchFirstPage(productId, forceRefresh = false).toList()

// THEN
val cachedResult = flow[0] as FetchResult.Cached

assertFalse(cachedResult.data.any { it.remoteVariationId == 1L })
}

@Test
fun `given remote variations, when fetchFirstPage called, then filter out downloadable variations`() = runTest {
// GIVEN
val productId = 1L
whenever(handler.canLoadMore(5)).thenReturn(true)
whenever(handler.getVariationsFlow(productId)).thenReturn(
flowOf(
listOf(
ProductTestUtils.generateProductVariation(
variationId = 1,
amount = "0",
isDownloadable = true
),
ProductTestUtils.generateProductVariation(
variationId = 2,
amount = "20.0",
isVirtual = false,
isDownloadable = false
)
)
)
)
whenever(handler.fetchVariations(productId, forceRefresh = true)).thenReturn(Result.success(Unit))
whenever(variationsCache.get(productId)).thenReturn(sampleProducts)
val sut = WooPosVariationsDataSource(handler, variationsCache)

// WHEN
val flow = sut.fetchFirstPage(productId, forceRefresh = true).toList()

// THEN
val remoteResult = flow[1] as FetchResult.Remote

assertThat(remoteResult.result.getOrNull()?.any { it.remoteVariationId == 1L }).isFalse()
}
}
Loading