diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosFullSyncStatusChecker.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosFullSyncStatusChecker.kt index b88b30b1f58..38f2a7a79b0 100644 --- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosFullSyncStatusChecker.kt +++ b/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() diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosLocalCatalogSyncScheduler.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosLocalCatalogSyncScheduler.kt index 62f55f446c1..d1ba7c601ec 100644 --- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosLocalCatalogSyncScheduler.kt +++ b/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") } } diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosLocalCatalogSyncWorker.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosLocalCatalogSyncWorker.kt index 80298a22e8a..fed7e4a3f49 100644 --- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosLocalCatalogSyncWorker.kt +++ b/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 - } } diff --git a/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/home/items/products/WooPosProductsDataSourceTest.kt b/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/home/items/products/WooPosProductsDataSourceTest.kt index a991bb1ede3..299863d0177 100644 --- a/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/home/items/products/WooPosProductsDataSourceTest.kt +++ b/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( diff --git a/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosFullSyncStatusCheckerTest.kt b/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosFullSyncStatusCheckerTest.kt index 06f49d699e0..4f0c08ba033 100644 --- a/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosFullSyncStatusCheckerTest.kt +++ b/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 diff --git a/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosLocalCatalogSyncWorkerTest.kt b/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosLocalCatalogSyncWorkerTest.kt index 2a6282ce70b..6eabdd9622a 100644 --- a/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosLocalCatalogSyncWorkerTest.kt +++ b/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)) + } }