Skip to content

[Woo POS][Local Catalog] Refactor and fix pull-to-refresh logic in products and variations #14816

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

Open
wants to merge 12 commits into
base: trunk
Choose a base branch
from
+369 −220
Open

[Woo POS][Local Catalog] Refactor and fix pull-to-refresh logic in products and variations #14816

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
57f4048
Refactor and fix pull-to-refresh logic in products and variations vie…
samiuelson Oct 24, 2025
a656b2f
Merge branch `trunk` and resolve conflicts
samiuelson Oct 24, 2025
5e911ae
Merge branch 'trunk' into woomob-1566-woo-poslocal-catalog-ptr-runs-i…
samiuelson Oct 24, 2025
4df733c
Remove comment
samiuelson Oct 27, 2025
9e08a48
Rename function
samiuelson Oct 27, 2025
801786d
Fix implementation of `refreshProducts` and `refreshVariations` in `W…
samiuelson Oct 27, 2025
a7fd7db
Refactor: Unify sync result types and observe product/variation lists
samiuelson Oct 27, 2025
1e0e98c
Merge branch 'trunk' into woomob-1566-woo-poslocal-catalog-ptr-runs-i…
samiuelson Oct 27, 2025
eb180f2
Remove unreachable code
samiuelson Oct 28, 2025
22bb958
Remove redundant code
samiuelson Oct 28, 2025
e238460
Remove unreachable code
samiuelson Oct 28, 2025
8c895ce
Update tests
samiuelson Oct 28, 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
74 changes: 57 additions & 17 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 @@ -13,11 +13,15 @@ import com.woocommerce.android.ui.woopos.common.data.models.WooPosProductModel
import com.woocommerce.android.ui.woopos.common.data.models.WooPosProductModelMapper
import com.woocommerce.android.ui.woopos.common.data.models.WooPosWCProductToWooPosProductModelMapper
import com.woocommerce.android.ui.woopos.common.data.toWooPosVariation
import com.woocommerce.android.ui.woopos.home.items.products.WooPosProductsDataSource.VariationsResult
import com.woocommerce.android.ui.woopos.home.items.variations.WooPosVariationsLRUCache
import com.woocommerce.android.ui.woopos.localcatalog.PosLocalCatalogSyncResult
import com.woocommerce.android.ui.woopos.localcatalog.ProductsResult
import com.woocommerce.android.ui.woopos.localcatalog.VariationsResult
import com.woocommerce.android.ui.woopos.localcatalog.WooPosFullSyncRequirement
import com.woocommerce.android.ui.woopos.localcatalog.WooPosFullSyncStatusChecker
import com.woocommerce.android.ui.woopos.localcatalog.WooPosLocalCatalogSyncRepository
import com.woocommerce.android.ui.woopos.localcatalog.WooPosPerformInstantCatalogFullSync
import com.woocommerce.android.ui.woopos.localcatalog.WooPosSyncResult
import com.woocommerce.android.util.WooLog
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.async
Expand All @@ -27,6 +31,7 @@ import kotlinx.coroutines.flow.first
import kotlinx.coroutines.flow.firstOrNull
import kotlinx.coroutines.flow.flow
import kotlinx.coroutines.flow.flowOn
import kotlinx.coroutines.flow.last
import kotlinx.coroutines.flow.map
import kotlinx.coroutines.flow.take
import kotlinx.coroutines.withContext
Expand Down Expand Up @@ -127,6 +132,14 @@ class WooPosProductsDataSource @Inject constructor(
activeSource?.canLoadMoreVariations(numOfVariations)
?: error("canLoadMoreVariations - Data source not selected")

suspend fun refreshProducts(): Result<WooPosSyncResult> =
activeSource?.refreshProducts()
?: error("refreshProducts - Data source not selected")

suspend fun refreshVariations(productId: Long): Result<WooPosSyncResult> =
activeSource?.refreshVariations(productId)
?: error("refreshVariations - Data source not selected")

suspend fun getVariationById(
productId: Long,
variationId: Long
Expand All @@ -135,16 +148,6 @@ class WooPosProductsDataSource @Inject constructor(
?: error("GetVariationById - Data source not selected")
}

sealed class ProductsResult {
data class Cached(val products: List<WooPosProductModel>) : ProductsResult()
data class Remote(val productsResult: Result<List<WooPosProductModel>>) : ProductsResult()
}

sealed class VariationsResult {
data class Cached(val data: List<WooPosVariation>) : VariationsResult()
data class Remote(val result: Result<List<WooPosVariation>>) : VariationsResult()
}

sealed class WooPosPrepopulatingDataStatus {
data object Syncing : WooPosPrepopulatingDataStatus()
data object Completed : WooPosPrepopulatingDataStatus()
Expand All @@ -158,6 +161,7 @@ class WooPosProductsInDbDataSource @Inject constructor(
private val productMapper: WooPosProductModelMapper,
private val variationMapper: WooPosVariationMapper,
private val performInstantCatalogFullSync: WooPosPerformInstantCatalogFullSync,
private val localCatalogSyncRepository: WooPosLocalCatalogSyncRepository,
) : WooPosProductsDataSourceInterface {

private fun getProductsFromDatabaseFlow(): Flow<List<WooPosProductModel>> {
Expand All @@ -174,9 +178,9 @@ class WooPosProductsInDbDataSource @Inject constructor(

override fun fetchFirstProductsPage(
forceRefresh: Boolean
): Flow<WooPosProductsDataSource.ProductsResult> = getProductsFromDatabaseFlow()
): Flow<ProductsResult> = getProductsFromDatabaseFlow()
.map { products ->
WooPosProductsDataSource.ProductsResult.Remote(Result.success(products))
ProductsResult.Remote(Result.success(products))
}
.flowOn(Dispatchers.IO)

Expand Down Expand Up @@ -232,6 +236,24 @@ class WooPosProductsInDbDataSource @Inject constructor(
)
return result.getOrNull()?.toWooPosVariation(variationMapper)
}

override suspend fun refreshProducts(): Result<WooPosSyncResult> = withContext(Dispatchers.IO) {
selectedSite.getOrNull()?.let { site ->
val syncResult = localCatalogSyncRepository.syncLocalCatalogIncremental(site)
Result.success(syncResult)
} ?: Result.success(
Copy link
Contributor

Choose a reason for hiding this comment

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

❓ Is this intentionally returning Success when site is not selected?

PosLocalCatalogSyncResult.Failure.UnexpectedError("No site selected")
)
}

override suspend fun refreshVariations(productId: Long): Result<WooPosSyncResult> = withContext(Dispatchers.IO) {
selectedSite.getOrNull()?.let { site ->
val syncResult = localCatalogSyncRepository.syncLocalCatalogIncremental(site)
Result.success(syncResult)
} ?: Result.success(
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

PosLocalCatalogSyncResult.Failure.UnexpectedError("No site selected")
)
}
}

class WooPosProductsRemoteDataSource @Inject constructor(
Expand Down Expand Up @@ -297,22 +319,22 @@ class WooPosProductsRemoteDataSource @Inject constructor(

override fun fetchFirstProductsPage(
forceRefresh: Boolean
): Flow<WooPosProductsDataSource.ProductsResult> = flow {
): Flow<ProductsResult> = flow {
offset.set(0)
productsIndex.clearCache()

if (!forceRefresh) {
val cachedProducts = sortProducts(productsCache.getAll()).take(NORMAL_PAGE_SIZE)
emit(WooPosProductsDataSource.ProductsResult.Cached(cachedProducts))
emit(ProductsResult.Cached(cachedProducts))
}

val fetchResult = fetchProducts()

if (fetchResult.isSuccess) {
emit(WooPosProductsDataSource.ProductsResult.Remote(Result.success(fetchResult.getOrThrow())))
emit(ProductsResult.Remote(Result.success(fetchResult.getOrThrow())))
} else {
emit(
WooPosProductsDataSource.ProductsResult.Remote(
ProductsResult.Remote(
Result.failure(
fetchResult.exceptionOrNull() ?: Exception("Unknown error")
)
Expand Down Expand Up @@ -488,6 +510,24 @@ class WooPosProductsRemoteDataSource @Inject constructor(
?.toWooPosVariation(variationMapper)
}

override suspend fun refreshProducts(): Result<WooPosSyncResult> = withContext(
Copy link
Contributor

Choose a reason for hiding this comment

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

when forceRefresh = true the in-memory cache gets wiped out

I can't fine the convo right now, but Andrei mentioned we have not been wiping cache on PTR - I remember we used to do it early in the POS but then switched to keeping the in-memory cache until app restart. Do you know at which point we started calling productsIndex.clearCache() in the data source implementation? I mean, why we decided to do it?

Copy link
Contributor Author

@samiuelson samiuelson Oct 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I just found out that the only place the in-memory cache is cleared is in WooPosProductsDataSource::prepopulateProductsCache which is called in WooPosSplashViewModel. On PTR only the productsIndex is cleared (responsible for pagination) — not the in-memory cache (WooPosProductsCache). This means that we are not clearing the cache and Andrey's issue is related to something else.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh, good find! So this PR is ready to be tested, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, ready 👍

Dispatchers.IO
) {
// We can return the last-emitted result which will always be the instance of [ProductsResult.Remote] because
// when forceRefresh = true the in-memory cache gets wiped out and the flow fetches from remote only.
val productsResult = fetchFirstProductsPage(forceRefresh = true).last()
Result.success(productsResult)
}

override suspend fun refreshVariations(
productId: Long
): Result<WooPosSyncResult> = withContext(Dispatchers.IO) {
// We can return the last-emitted result which will always be the instance of [VariationsResult.Remote] because
// when forceRefresh = true the in-memory cache gets wiped out and the flow fetches from remote only.
val variationsResult = fetchFirstVariationsPage(productId, forceRefresh = true).last()
Result.success(variationsResult)
}

companion object {
private const val NORMAL_PAGE_SIZE = 25
private const val PRE_POPULATION_PAGE_SIZE = 100
Expand Down
11 changes: 9 additions & 2 deletions ...om/woocommerce/android/ui/woopos/home/items/products/WooPosProductsDataSourceInterface.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,16 @@ package com.woocommerce.android.ui.woopos.home.items.products

import com.woocommerce.android.ui.woopos.common.data.WooPosVariation
import com.woocommerce.android.ui.woopos.common.data.models.WooPosProductModel
import com.woocommerce.android.ui.woopos.localcatalog.ProductsResult
import com.woocommerce.android.ui.woopos.localcatalog.VariationsResult
import com.woocommerce.android.ui.woopos.localcatalog.WooPosSyncResult
import kotlinx.coroutines.flow.Flow
import org.wordpress.android.fluxc.model.LocalOrRemoteId

interface WooPosProductsDataSourceInterface {
fun fetchFirstProductsPage(
forceRefresh: Boolean
): Flow<WooPosProductsDataSource.ProductsResult>
): Flow<ProductsResult>

suspend fun loadMoreProducts(): Result<List<WooPosProductModel>>

Expand All @@ -23,11 +26,15 @@ interface WooPosProductsDataSourceInterface {
fun fetchFirstVariationsPage(
productId: Long,
forceRefresh: Boolean
): Flow<WooPosProductsDataSource.VariationsResult>
): Flow<VariationsResult>

suspend fun loadMoreVariations(productId: Long): Result<List<WooPosVariation>>

fun canLoadMoreVariations(numOfVariations: Int): Boolean

suspend fun getVariationById(productId: Long, variationId: Long): WooPosVariation?

suspend fun refreshProducts(): Result<WooPosSyncResult>

suspend fun refreshVariations(productId: Long): Result<WooPosSyncResult>
}
114 changes: 50 additions & 64 deletions ...n/kotlin/com/woocommerce/android/ui/woopos/home/items/products/WooPosProductsViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,7 @@ package com.woocommerce.android.ui.woopos.home.items.products
import androidx.lifecycle.ViewModel
import androidx.lifecycle.viewModelScope
import com.woocommerce.android.R
import com.woocommerce.android.tools.SelectedSite
import com.woocommerce.android.ui.woopos.common.data.models.WooPosProductModel
import com.woocommerce.android.ui.woopos.featureflags.WooPosLocalCatalogM1Enabled
import com.woocommerce.android.ui.woopos.home.ChildToParentEvent
import com.woocommerce.android.ui.woopos.home.ParentToChildrenEvent
import com.woocommerce.android.ui.woopos.home.WooPosChildrenToParentEventSender
Expand All @@ -16,7 +14,8 @@ import com.woocommerce.android.ui.woopos.home.items.WooPosPaginationState
import com.woocommerce.android.ui.woopos.home.items.WooPosProductsViewState
import com.woocommerce.android.ui.woopos.home.items.WooPosPullToRefreshState
import com.woocommerce.android.ui.woopos.localcatalog.PosLocalCatalogSyncResult
import com.woocommerce.android.ui.woopos.localcatalog.WooPosLocalCatalogSyncRepository
import com.woocommerce.android.ui.woopos.localcatalog.ProductsResult
import com.woocommerce.android.ui.woopos.localcatalog.VariationsResult
import com.woocommerce.android.ui.woopos.util.analytics.WooPosAnalyticsEvent
import com.woocommerce.android.ui.woopos.util.analytics.WooPosAnalyticsEvent.Event.PullToRefreshTriggered
import com.woocommerce.android.ui.woopos.util.analytics.WooPosAnalyticsEventConstant
Expand All @@ -39,9 +38,6 @@ class WooPosProductsViewModel @Inject constructor(
private val parentToChildrenEventReceiver: WooPosParentToChildrenEventReceiver,
private val priceFormat: WooPosFormatPrice,
private val analyticsTracker: WooPosAnalyticsTracker,
private val wooPosLocalCatalogM1Enabled: WooPosLocalCatalogM1Enabled,
private val localCatalogSyncRepository: WooPosLocalCatalogSyncRepository,
private val selectedSite: SelectedSite,
private val resourceProvider: ResourceProvider,
) : ViewModel() {

Expand All @@ -62,22 +58,14 @@ class WooPosProductsViewModel @Inject constructor(

init {
listenEventsFromParent()
loadProducts(
forceRefreshProducts = false,
withPullToRefresh = false,
)
loadProducts(forceRefreshProducts = false)
}

private fun listenEventsFromParent() {
viewModelScope.launch {
parentToChildrenEventReceiver.events.collect { event ->
when (event) {
ParentToChildrenEvent.RefreshProductList -> {
loadProducts(
forceRefreshProducts = true,
withPullToRefresh = false,
)
}
ParentToChildrenEvent.RefreshProductList -> loadProducts(forceRefreshProducts = true)

ParentToChildrenEvent.BackFromCheckoutToCartClicked,
is ParentToChildrenEvent.BarcodeEvent,
Expand Down Expand Up @@ -109,17 +97,7 @@ class WooPosProductsViewModel @Inject constructor(
}

WooPosProductsUIEvent.PullToRefreshTriggered -> {
when {
wooPosLocalCatalogM1Enabled() -> {
performIncrementalSync()
}
else -> {
loadProducts(
forceRefreshProducts = true,
withPullToRefresh = true,
)
}
}
handlePullToRefresh()
viewModelScope.launch {
analyticsTracker.track(
PullToRefreshTriggered(
Expand All @@ -130,12 +108,7 @@ class WooPosProductsViewModel @Inject constructor(
}
}

WooPosProductsUIEvent.ProductsLoadingErrorRetryButtonClicked -> {
loadProducts(
forceRefreshProducts = false,
withPullToRefresh = false,
)
}
WooPosProductsUIEvent.ProductsLoadingErrorRetryButtonClicked -> loadProducts(forceRefreshProducts = false)
}
}

Expand Down Expand Up @@ -170,28 +143,21 @@ class WooPosProductsViewModel @Inject constructor(
}
}

private fun loadProducts(
forceRefreshProducts: Boolean,
withPullToRefresh: Boolean,
) {
private fun loadProducts(forceRefreshProducts: Boolean) {
loadProductsJob?.cancel()
loadMoreProductsJob?.cancel()
loadProductsJob = viewModelScope.launch {
_viewState.value = if (withPullToRefresh) {
buildReloadingState()
} else {
WooPosProductsViewState.Loading()
}
_viewState.value = WooPosProductsViewState.Loading()

dataSource.fetchFirstPage(forceRefresh = forceRefreshProducts).collect { result ->
when (result) {
is WooPosProductsDataSource.ProductsResult.Cached -> {
is ProductsResult.Cached -> {
if (result.products.isNotEmpty()) {
_viewState.value = result.products.toContentState()
}
}

is WooPosProductsDataSource.ProductsResult.Remote -> {
is ProductsResult.Remote -> {
_viewState.value = when {
result.productsResult.isSuccess -> {
val products = result.productsResult.getOrThrow()
Expand Down Expand Up @@ -337,32 +303,52 @@ class WooPosProductsViewModel @Inject constructor(
viewModelScope.launch { fromChildToParentEventSender.sendToParent(event) }
}

private fun performIncrementalSync() {
_viewState.value = buildReloadingState()

private fun handlePullToRefresh() {
viewModelScope.launch {
selectedSite.getOrNull()?.let { site ->
val syncResult = localCatalogSyncRepository.syncLocalCatalogIncremental(site)
_viewState.value = buildViewStateForSyncResult(syncResult)
_viewState.value = buildReloadingState()

val result = dataSource.refreshProducts()
result.onSuccess { posSyncResult ->
when (posSyncResult) {
is PosLocalCatalogSyncResult.Success -> {
_viewState.value = hidePTRIndicator()
}
is PosLocalCatalogSyncResult.Failure -> {
handlePTRError()
}
is ProductsResult.Cached -> Unit // PTR is not expected to deliver cached result
is ProductsResult.Remote -> {
if (posSyncResult.productsResult.isSuccess) {
val products = posSyncResult.productsResult.getOrThrow()
_viewState.value = if (products.isNotEmpty()) {
products.toContentState()
} else {
WooPosProductsViewState.Empty()
}
} else {
handlePTRError()
}
}
is VariationsResult -> {
error("Unexpected variations result in products refresh")
}
}
}.onFailure { exception ->
handlePTRError()
}
}
}

private fun buildViewStateForSyncResult(syncResult: PosLocalCatalogSyncResult): WooPosProductsViewState =
when (syncResult) {
is PosLocalCatalogSyncResult.Success -> {
hidePTRIndicator()
}

is PosLocalCatalogSyncResult.Failure -> {
sendEventToParent(
ChildToParentEvent.ToastMessageDisplayed(
message = resourceProvider.getString(R.string.something_went_wrong_try_again)
)
private fun handlePTRError() {
sendEventToParent(
ChildToParentEvent.ToastMessageDisplayed(
message = resourceProvider.getString(
R.string.something_went_wrong_try_again
)
hidePTRIndicator()
}
}
)
)
_viewState.value = hidePTRIndicator()
}

private fun hidePTRIndicator(): WooPosProductsViewState = when (val currentState = _viewState.value) {
is WooPosProductsViewState.Content -> currentState.copy(
Expand Down
Loading