[Woo POS][Local Catalog] Skip manual one-time sync on site switch if it's not needed

WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosFullSyncStatusChecker.kt

@@ -23,7 +23,10 @@ class WooPosFullSyncStatusChecker @Inject constructor(
     @Suppress("ReturnCount")
     suspend fun checkSyncRequirement(): WooPosFullSyncRequirement {
         val site = selectedSite.getOrNull()
-            ?: error("No site selected")
+        if (site == null) {
+            wooPosLogWrapper.e("Full sync check failed: No site selected")
+            return WooPosFullSyncRequirement.Error("No site selected")
+        }
 
         if (!isLocalCatalogSupported(site.localId())) {
             wooPosLogWrapper.d("Full sync check skipped: Local catalog not supported for site")
@@ -71,7 +74,7 @@ class WooPosFullSyncStatusChecker @Inject constructor(
                     "Full sync not required: Recent sync at $lastFullSyncTimestamp " +
                         "(${if (catalogIsEmpty) "empty catalog" else "$productCount products"})"
                 )
-                WooPosFullSyncRequirement.NotRequired
+                WooPosFullSyncRequirement.NotRequired(lastFullSyncTimestamp)
             }
         }
     }
@@ -89,7 +92,7 @@ class WooPosFullSyncStatusChecker @Inject constructor(
 }
 
 sealed class WooPosFullSyncRequirement {
-    data object NotRequired : WooPosFullSyncRequirement()
+    data class NotRequired(val lastSyncTimestamp: Long) : WooPosFullSyncRequirement()
     data object Overdue : WooPosFullSyncRequirement()
     data object BlockingRequired : WooPosFullSyncRequirement()
     data class Error(val message: String) : WooPosFullSyncRequirement()

WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosLocalCatalogSyncScheduler.kt

@@ -84,7 +84,7 @@ class WooPosLocalCatalogSyncScheduler @Inject constructor(
                 oneTimeWorkRequest
             )
 
-            logger.d("Manual POS local catalog sync triggered")
+            logger.d("Manual POS local catalog sync work enqueued")
         }
     }
 

WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosLocalCatalogSyncWorker.kt

@@ -10,7 +10,6 @@ import com.woocommerce.android.ui.woopos.tab.WooPosTabShouldBeVisible
 import com.woocommerce.android.ui.woopos.util.datastore.WooPosPreferencesRepository
 import dagger.assisted.Assisted
 import dagger.assisted.AssistedInject
-import org.wordpress.android.fluxc.model.SiteModel
 import kotlin.time.Duration.Companion.milliseconds
 
 @HiltWorker
@@ -26,7 +25,7 @@ constructor(
     private val logger: WooPosLogWrapper,
     private val timeProvider: DateTimeProvider,
     private val wooPosTabShouldBeVisible: WooPosTabShouldBeVisible,
-    private val isLocalCatalogSupported: WooPosIsLocalCatalogSupported,
+    private val syncStatusChecker: WooPosFullSyncStatusChecker,
 ) : CoroutineWorker(appContext, workerParams) {
 
     companion object {
@@ -47,7 +46,13 @@ constructor(
             return Result.success()
         }
 
-        val site = isCatalogSyncSupported() ?: return Result.failure()
+        validateSyncStatus()?.let { return it }
+
+        // Site and catalog support already validated by validateSyncStatus()
+        val site = selectedSite.getOrNull() ?: run {
+            logger.e("Unexpected: Site is null after validation")
+            return Result.failure()
+        }
 
         logger.d("Starting FULL local catalog sync")
 
@@ -84,6 +89,33 @@ constructor(
         }
     }
 
+    @Suppress("ReturnCount")
+    private suspend fun validateSyncStatus(): Result? {
+        val syncRequirement = syncStatusChecker.checkSyncRequirement()
+        return when (syncRequirement) {
+            is WooPosFullSyncRequirement.NotRequired -> {
+                logger.d(
+                    "Local catalog sync NotRequired: Catalog was last synced at " +
+                        "${syncRequirement.lastSyncTimestamp}"
+                )
+                Result.success()
+            }
+            is WooPosFullSyncRequirement.LocalCatalogDisabled -> {
+                logger.d("Local catalog sync LocalCatalogDisabled: ${syncRequirement.message}")
+                Result.success()
+            }
+            is WooPosFullSyncRequirement.Error -> {
+                logger.e("Sync requirement check failed: ${syncRequirement.message}. Retrying...")
+                Result.retry()
+            }
+            is WooPosFullSyncRequirement.BlockingRequired,
+            is WooPosFullSyncRequirement.Overdue -> {
+                logger.d("Proceeding with sync (requirement: $syncRequirement)")
+                null
+            }
+        }
+    }
+
     private suspend fun isPosInactive(): Boolean {
         val lastUsedTimestamp = preferencesRepository.getLastUsedTimestamp() ?: return false
         val daysSinceLastUse = (timeProvider.now() - lastUsedTimestamp).milliseconds.inWholeDays
@@ -97,20 +129,4 @@ constructor(
             false
         }
     }
-
-    @Suppress("ReturnCount")
-    private suspend fun isCatalogSyncSupported(): SiteModel? {
-        val site = selectedSite.getOrNull()
-        if (site == null) {
-            logger.w("No selected WooCommerce site found, skipping local catalog sync")
-            return null
-        }
-
-        if (!isLocalCatalogSupported(site.localId())) {
-            logger.d("Local catalog not supported for site")
-            return null
-        }
-
-        return site
-    }
 }

WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/home/items/products/WooPosProductsDataSourceTest.kt

@@ -56,7 +56,7 @@ class WooPosProductsDataSourceTest {
     fun `given sync not required, when prepopulate cache, then uses local db data source`() = runTest {
         // GIVEN
         whenever(syncStatusChecker.checkSyncRequirement()).thenReturn(
-            WooPosFullSyncRequirement.NotRequired
+            WooPosFullSyncRequirement.NotRequired(lastSyncTimestamp = 0L)
         )
         whenever(localDbDataSource.fetchFirstProductsPage(false)).thenReturn(
             flowOf(ProductsResult.Remote(Result.success(emptyList())))
@@ -175,7 +175,7 @@ class WooPosProductsDataSourceTest {
     fun `given sync not required, when load more products, then delegates to local db data source`() = runTest {
         // GIVEN
         whenever(syncStatusChecker.checkSyncRequirement()).thenReturn(
-            WooPosFullSyncRequirement.NotRequired
+            WooPosFullSyncRequirement.NotRequired(lastSyncTimestamp = 0L)
         )
         whenever(localDbDataSource.loadMoreProducts()).thenReturn(Result.success(emptyList()))
         val sut = createSut()
@@ -247,7 +247,7 @@ class WooPosProductsDataSourceTest {
     fun `given sync not required, when refresh products, then delegates to local db data source`() = runTest {
         // GIVEN
         whenever(syncStatusChecker.checkSyncRequirement()).thenReturn(
-            WooPosFullSyncRequirement.NotRequired
+            WooPosFullSyncRequirement.NotRequired(lastSyncTimestamp = 0L)
         )
         whenever(localDbDataSource.refreshProducts()).thenReturn(
             Result.success(
@@ -292,7 +292,7 @@ class WooPosProductsDataSourceTest {
     fun `given sync not required, when refresh variations, then delegates to local db data source`() = runTest {
         // GIVEN
         whenever(syncStatusChecker.checkSyncRequirement()).thenReturn(
-            WooPosFullSyncRequirement.NotRequired
+            WooPosFullSyncRequirement.NotRequired(lastSyncTimestamp = 0L)
         )
         whenever(localDbDataSource.refreshVariations(123L)).thenReturn(
             Result.success(

WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosFullSyncStatusCheckerTest.kt

@@ -81,18 +81,19 @@ class WooPosFullSyncStatusCheckerTest {
             assertThat(result).isInstanceOf(WooPosFullSyncRequirement.LocalCatalogDisabled::class.java)
         }
 
-    @Test(expected = IllegalStateException::class)
-    fun `given no site selected, when checkSyncRequirement called, then should throw error`() = runTest {
+    @Test
+    fun `given no site selected, when checkSyncRequirement called, then should return Error`() = runTest {
         // GIVEN
         whenever(selectedSite.getOrNull()).thenReturn(null)
 
         val sut = createSut()
 
         // WHEN
-        sut.checkSyncRequirement()
+        val result = sut.checkSyncRequirement()
 
         // THEN
-        // Exception is expected
+        assertThat(result).isInstanceOf(WooPosFullSyncRequirement.Error::class.java)
+        assertThat((result as WooPosFullSyncRequirement.Error).message).isEqualTo("No site selected")
     }
 
     @Test
@@ -193,7 +194,7 @@ class WooPosFullSyncStatusCheckerTest {
             val result = sut.checkSyncRequirement()
 
             // THEN
-            assertThat(result).isEqualTo(WooPosFullSyncRequirement.NotRequired)
+            assertThat(result).isEqualTo(WooPosFullSyncRequirement.NotRequired(recentTimestamp))
         }
 
     @Test
@@ -225,7 +226,7 @@ class WooPosFullSyncStatusCheckerTest {
             val result = sut.checkSyncRequirement()
 
             // THEN
-            assertThat(result).isEqualTo(WooPosFullSyncRequirement.NotRequired)
+            assertThat(result).isInstanceOf(WooPosFullSyncRequirement.NotRequired::class.java)
         }
 
     @Test
@@ -266,7 +267,7 @@ class WooPosFullSyncStatusCheckerTest {
             val result = sut.checkSyncRequirement()
 
             // THEN
-            assertThat(result).isEqualTo(WooPosFullSyncRequirement.NotRequired)
+            assertThat(result).isEqualTo(WooPosFullSyncRequirement.NotRequired(recentTimestamp))
         }
 
     @Test
@@ -286,7 +287,7 @@ class WooPosFullSyncStatusCheckerTest {
             val result = sut.checkSyncRequirement()
 
             // THEN
-            assertThat(result).isEqualTo(WooPosFullSyncRequirement.NotRequired)
+            assertThat(result).isEqualTo(WooPosFullSyncRequirement.NotRequired(recentTimestamp))
         }
 
     @Test

WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosLocalCatalogSyncWorkerTest.kt

@@ -30,7 +30,7 @@ class WooPosLocalCatalogSyncWorkerTest : BaseUnitTest() {
     private lateinit var site: SiteModel
     private var logger: WooPosLogWrapper = mock()
     private var wooPosTabShouldBeVisible: WooPosTabShouldBeVisible = mock()
-    private var isLocalCatalogSupported: WooPosIsLocalCatalogSupported = mock()
+    private var syncStatusChecker: WooPosFullSyncStatusChecker = mock()
     private val mockTimeProvider: DateTimeProvider = mock {
         whenever(it.now()).thenReturn(CURRENT_TIME_MILLIS)
     }
@@ -66,7 +66,7 @@ class WooPosLocalCatalogSyncWorkerTest : BaseUnitTest() {
         }
 
         whenever(selectedSite.getOrNull()).thenReturn(site)
-        whenever(isLocalCatalogSupported(site.localId())).thenReturn(true)
+        whenever(syncStatusChecker.checkSyncRequirement()).thenReturn(WooPosFullSyncRequirement.BlockingRequired)
 
         whenever(wooPosTabShouldBeVisible.invoke()).thenReturn(Result.success(true))
         whenever(preferencesRepository.getLastUsedTimestamp()).thenReturn(null)
@@ -86,7 +86,7 @@ class WooPosLocalCatalogSyncWorkerTest : BaseUnitTest() {
             logger = logger,
             timeProvider = mockTimeProvider,
             wooPosTabShouldBeVisible = wooPosTabShouldBeVisible,
-            isLocalCatalogSupported = isLocalCatalogSupported,
+            syncStatusChecker = syncStatusChecker,
         )
     }
 
@@ -105,30 +105,17 @@ class WooPosLocalCatalogSyncWorkerTest : BaseUnitTest() {
     }
 
     @Test
-    fun `given local catalog not supported, when sync is attempted, then returns failure`() = testBlocking {
+    fun `when no site selected, then returns retry`() = testBlocking {
         // GIVEN
-        whenever(isLocalCatalogSupported(site.localId())).thenReturn(false)
+        whenever(syncStatusChecker.checkSyncRequirement())
+            .thenReturn(WooPosFullSyncRequirement.Error("No site selected"))
         val worker = createWorker()
 
         // WHEN
         val result = worker.doWork()
 
         // THEN
-        assertThat(result).isEqualTo(ListenableWorker.Result.failure())
-        verify(syncRepository, never()).syncLocalCatalogFull(any())
-    }
-
-    @Test
-    fun `when no site selected, then returns failure`() = testBlocking {
-        // GIVEN
-        whenever(selectedSite.getOrNull()).thenReturn(null)
-        val worker = createWorker()
-
-        // WHEN
-        val result = worker.doWork()
-
-        // THEN
-        assertThat(result).isEqualTo(ListenableWorker.Result.failure())
+        assertThat(result).isEqualTo(ListenableWorker.Result.retry())
         verify(syncRepository, never()).syncLocalCatalogFull(any())
     }
 
@@ -273,4 +260,90 @@ class WooPosLocalCatalogSyncWorkerTest : BaseUnitTest() {
         verify(syncRepository).syncLocalCatalogFull(eq(site))
         verify(syncRepository).syncLocalCatalogIncremental(eq(site))
     }
+
+    @Test
+    fun `given sync not required, when validateSyncStatus is called, then returns success without syncing`() = testBlocking {
+        // GIVEN
+        val lastSyncTimestamp = CURRENT_TIME_MILLIS - (3 * 24 * 60 * 60 * 1000L) // 3 days ago
+        whenever(syncStatusChecker.checkSyncRequirement())
+            .thenReturn(WooPosFullSyncRequirement.NotRequired(lastSyncTimestamp))
+
+        val worker = createWorker()
+
+        // WHEN
+        val result = worker.doWork()
+
+        // THEN
+        assertThat(result).isEqualTo(ListenableWorker.Result.success())
+        verify(syncRepository, never()).syncLocalCatalogFull(any())
+        verify(syncRepository, never()).syncLocalCatalogIncremental(any())
+    }
+
+    @Test
+    fun `given local catalog disabled, when validateSyncStatus is called, then returns success without syncing`() = testBlocking {
+        // GIVEN
+        whenever(syncStatusChecker.checkSyncRequirement())
+            .thenReturn(WooPosFullSyncRequirement.LocalCatalogDisabled("Catalog too large"))
+
+        val worker = createWorker()
+
+        // WHEN
+        val result = worker.doWork()
+
+        // THEN
+        assertThat(result).isEqualTo(ListenableWorker.Result.success())
+        verify(syncRepository, never()).syncLocalCatalogFull(any())
+        verify(syncRepository, never()).syncLocalCatalogIncremental(any())
+    }
+
+    @Test
+    fun `given sync requirement check error, when validateSyncStatus is called, then returns retry`() = testBlocking {
+        // GIVEN
+        whenever(syncStatusChecker.checkSyncRequirement())
+            .thenReturn(WooPosFullSyncRequirement.Error("No network connection"))
+
+        val worker = createWorker()
+
+        // WHEN
+        val result = worker.doWork()
+
+        // THEN
+        assertThat(result).isEqualTo(ListenableWorker.Result.retry())
+        verify(syncRepository, never()).syncLocalCatalogFull(any())
+        verify(syncRepository, never()).syncLocalCatalogIncremental(any())
+    }
+
+    @Test
+    fun `given blocking sync required, when validateSyncStatus is called, then proceeds with sync`() = testBlocking {
+        // GIVEN
+        whenever(syncStatusChecker.checkSyncRequirement())
+            .thenReturn(WooPosFullSyncRequirement.BlockingRequired)
+
+        val worker = createWorker()
+
+        // WHEN
+        val result = worker.doWork()
+
+        // THEN
+        assertThat(result).isEqualTo(ListenableWorker.Result.success())
+        verify(syncRepository).syncLocalCatalogFull(eq(site))
+        verify(syncRepository).syncLocalCatalogIncremental(eq(site))
+    }
+
+    @Test
+    fun `given sync overdue, when validateSyncStatus is called, then proceeds with sync`() = testBlocking {
+        // GIVEN
+        whenever(syncStatusChecker.checkSyncRequirement())
+            .thenReturn(WooPosFullSyncRequirement.Overdue)
+
+        val worker = createWorker()
+
+        // WHEN
+        val result = worker.doWork()
+
+        // THEN
+        assertThat(result).isEqualTo(ListenableWorker.Result.success())
+        verify(syncRepository).syncLocalCatalogFull(eq(site))
+        verify(syncRepository).syncLocalCatalogIncremental(eq(site))
+    }
 }