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

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

@@ -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
@@ -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
@@ -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
@@ -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()
@@ -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>> {
@@ -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)
 
@@ -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(
+            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(
+            PosLocalCatalogSyncResult.Failure.UnexpectedError("No site selected")
+        )
+    }
 }
 
 class WooPosProductsRemoteDataSource @Inject constructor(
@@ -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")
                     )
@@ -488,6 +510,24 @@ class WooPosProductsRemoteDataSource @Inject constructor(
             ?.toWooPosVariation(variationMapper)
     }
 
+    override suspend fun refreshProducts(): Result<WooPosSyncResult> = withContext(
+        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

WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/home/items/products/WooPosProductsDataSourceInterface.kt

@@ -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>>
 
@@ -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>
 }

WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/home/items/products/WooPosProductsViewModel.kt

@@ -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
@@ -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
@@ -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() {
 
@@ -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,
@@ -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(
@@ -130,12 +108,7 @@ class WooPosProductsViewModel @Inject constructor(
                 }
             }
 
-            WooPosProductsUIEvent.ProductsLoadingErrorRetryButtonClicked -> {
-                loadProducts(
-                    forceRefreshProducts = false,
-                    withPullToRefresh = false,
-                )
-            }
+            WooPosProductsUIEvent.ProductsLoadingErrorRetryButtonClicked -> loadProducts(forceRefreshProducts = false)
         }
     }
 
@@ -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()
@@ -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(