From a88751c9fa5376a85a22912064d25ad035bb6355 Mon Sep 17 00:00:00 2001 From: malinajirka Date: Wed, 7 Jan 2026 09:28:44 +0100 Subject: [PATCH 1/5] Remove fileUrl from the result --- .../ui/woopos/localcatalog/WooPosFileBasedSyncAction.kt | 4 ---- .../ui/woopos/localcatalog/WooPosFileBasedSyncActionTest.kt | 1 - 2 files changed, 5 deletions(-) diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosFileBasedSyncAction.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosFileBasedSyncAction.kt index bf24729df49d..8f59204510ed 100644 --- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosFileBasedSyncAction.kt +++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosFileBasedSyncAction.kt @@ -68,7 +68,6 @@ class WooPosFileBasedSyncAction @Inject constructor( } data class FileBasedSyncResult( - val fileUrl: String, val downloadedFile: File, val totalProducts: Int?, val completedAt: String?, @@ -125,7 +124,6 @@ class WooPosFileBasedSyncAction @Inject constructor( return Result.success( createFileBasedSyncResult( result = catalogResult, - fileUrl = url, downloadedFile = downloadedFile, productsStored = parsedData.products.size, variationsStored = parsedData.variations.size @@ -140,13 +138,11 @@ class WooPosFileBasedSyncAction @Inject constructor( private fun createFileBasedSyncResult( result: WooPosGenerateCatalogResult, - fileUrl: String, downloadedFile: File, productsStored: Int, variationsStored: Int ): FileBasedSyncResult { return FileBasedSyncResult( - fileUrl = fileUrl, downloadedFile = downloadedFile, totalProducts = result.total, completedAt = result.completedAt, diff --git a/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosFileBasedSyncActionTest.kt b/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosFileBasedSyncActionTest.kt index 9447604f06c2..332bc06cb1f6 100644 --- a/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosFileBasedSyncActionTest.kt +++ b/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosFileBasedSyncActionTest.kt @@ -86,7 +86,6 @@ class WooPosFileBasedSyncActionTest { // THEN assertThat(result.isSuccess).isTrue() val syncResult = result.getOrThrow() - assertThat(syncResult.fileUrl).isEqualTo(defaultUrl) assertThat(syncResult.downloadedFile).isEqualTo(defaultFile) assertThat(syncResult.productsStored).isEqualTo(1) assertThat(syncResult.variationsStored).isEqualTo(1) From dcd6f43133e873155f2c25f2fbffe8c8d6726fd6 Mon Sep 17 00:00:00 2001 From: malinajirka Date: Thu, 8 Jan 2026 13:59:04 +0100 Subject: [PATCH 2/5] Refactor WooPosFileBasedSyncAction to use shared result type --- .../localcatalog/WooPosFileBasedSyncAction.kt | 110 +++++++++--------- .../WooPosFileBasedSyncActionTest.kt | 85 +++++--------- 2 files changed, 83 insertions(+), 112 deletions(-) diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosFileBasedSyncAction.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosFileBasedSyncAction.kt index 824e0f127d0e..8bd6fb170c7b 100644 --- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosFileBasedSyncAction.kt +++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosFileBasedSyncAction.kt @@ -25,17 +25,16 @@ class WooPosFileBasedSyncAction @Inject constructor( } @Suppress("ReturnCount") - suspend fun syncCatalog( - site: SiteModel - ): Result { - logger.d("Starting file-based catalog generation for site ${site.id}") + suspend fun syncCatalog(site: SiteModel): PosLocalCatalogSyncResult { + val startTime = System.currentTimeMillis() + logger.d("WooPosFileBasedSyncAction: Starting file-based catalog generation for site ${site.id}") var failedConsecutiveAttempts = 0 repeat(MAX_POLL_ATTEMPTS) { attemptCount -> if (attemptCount > 0) { val delayMs = computeBackoffDelay(attemptCount) - logger.d("Waiting ${delayMs}ms before poll attempt $attemptCount") + logger.d("WooPosFileBasedSyncAction: Waiting ${delayMs}ms before poll attempt $attemptCount") delay(delayMs) } @@ -43,7 +42,12 @@ class WooPosFileBasedSyncAction @Inject constructor( if (response.isFailure) { if (++failedConsecutiveAttempts >= MAX_CONSECUTIVE_FAILED_ATTEMPTS) { - return Result.failure(response.exceptionOrNull() ?: Exception("Unknown error")) + val error = response.exceptionOrNull() + logger.e("WooPosFileBasedSyncAction: File-based sync failed " + + "after $MAX_CONSECUTIVE_FAILED_ATTEMPTS consecutive failures") + return PosLocalCatalogSyncResult.Failure.NetworkError( + error?.message ?: "API error during catalog sync" + ) } else { logger.w("Poll attempt $attemptCount failed: ${response.exceptionOrNull()?.message}") return@repeat @@ -52,46 +56,42 @@ class WooPosFileBasedSyncAction @Inject constructor( failedConsecutiveAttempts = 0 val result = response.getOrThrow() - logger.d( - "Poll attempt $attemptCount" - ) + logger.d("WooPosFileBasedSyncAction: Poll attempt $attemptCount") - val processedResult = processPollingResult(result, site) + val processedResult = processPollingResult(result, site, startTime = startTime) if (processedResult != null) { return processedResult } } - logger.e("Catalog generation timed out after $MAX_POLL_ATTEMPTS attempts") - return Result.failure(Exception("Catalog generation timed out")) + logger.e("WooPosFileBasedSyncAction: Catalog generation timed out after $MAX_POLL_ATTEMPTS attempts") + return PosLocalCatalogSyncResult.Failure.CatalogGenerationTimeout( + "Catalog generation is taking longer than expected." + ) } - data class FileBasedSyncResult( - val totalProducts: Int?, - val completedAt: String?, - val productsStored: Int, - val variationsStored: Int - ) - @Suppress("ReturnCount") private suspend fun processPollingResult( result: WooPosGenerateCatalogResult, - site: SiteModel - ): Result? { + site: SiteModel, + startTime: Long + ): PosLocalCatalogSyncResult? { return when (result.state) { WooPosGenerateCatalogState.COMPLETED -> { val url = result.url if (url != null) { - logger.d("Catalog available, starting download.") - - processDownloadAndStore(url, result, site) + logger.d("WooPosFileBasedSyncAction: Catalog available, starting download.") + processDownloadAndStore(url, site, startTime) } else { - logger.e("Catalog generation completed but URL is missing") - Result.failure(Exception("Catalog generation completed but URL is missing")) + logger.e("WooPosFileBasedSyncAction: Catalog generation completed but URL is missing") + PosLocalCatalogSyncResult.Failure.InvalidResponse( + "Catalog generation completed but download URL is missing." + ) } } else -> null.also { - logger.d("State: ${result.state}, Progress: ${result.progress}% out of ${result.total} items") + logger.d("WooPosFileBasedSyncAction: State: ${result.state}, Progress: ${result.progress}% " + + "out of ${result.total} items") } } } @@ -99,53 +99,57 @@ class WooPosFileBasedSyncAction @Inject constructor( @Suppress("ReturnCount") private suspend fun processDownloadAndStore( url: String, - catalogResult: WooPosGenerateCatalogResult, - site: SiteModel - ): Result { + site: SiteModel, + startTime: Long + ): PosLocalCatalogSyncResult { val downloadedFile = catalogFileDownloader.downloadCatalogFile(url, site.localId()) .onFailureLog("Failed to download catalog file") - .getOrElse { return Result.failure(it) } + .getOrElse { + return PosLocalCatalogSyncResult.Failure.NetworkError( + it.message ?: "Failed to download catalog file" + ) + } val parsedData = catalogFileParser.parseCatalogFile(downloadedFile, site.localId()) .onFailureLog("Failed to parse catalog file") - .getOrElse { return Result.failure(it) } + .getOrElse { + return PosLocalCatalogSyncResult.Failure.InvalidResponse( + it.message ?: "Failed to parse catalog file" + ) + } posLocalCatalogStore.storeCatalogData( localSiteId = site.localId(), products = parsedData.products, variations = parsedData.variations ).onFailureLog("Failed to store catalog data") - .getOrElse { return Result.failure(it) } + .getOrElse { + return PosLocalCatalogSyncResult.Failure.DatabaseError( + it.message ?: "Failed to store catalog data" + ) + } catalogFileDownloader.cleanupOldCatalogFiles(keepLatest = downloadedFile) - return Result.success( - createFileBasedSyncResult( - result = catalogResult, - productsStored = parsedData.products.size, - variationsStored = parsedData.variations.size - ) + val syncDuration = System.currentTimeMillis() - startTime + logger.d( + "File-based sync completed successfully. " + + "Products: ${parsedData.products.size}, Variations: ${parsedData.variations.size}. " + + "Duration: ${syncDuration}ms." + ) + + return PosLocalCatalogSyncResult.Success( + productsSynced = parsedData.products.size, + variationsSynced = parsedData.variations.size, + syncDurationMs = syncDuration ) } private fun Result.onFailureLog(context: String): Result { - onFailure { logger.e("$context: ${it.message}") } + onFailure { logger.e("WooPosFileBasedSyncAction: $context: ${it.message}") } return this } - private fun createFileBasedSyncResult( - result: WooPosGenerateCatalogResult, - productsStored: Int, - variationsStored: Int - ): FileBasedSyncResult { - return FileBasedSyncResult( - totalProducts = result.total, - completedAt = result.completedAt, - productsStored = productsStored, - variationsStored = variationsStored - ) - } - private fun computeBackoffDelay(attemptCount: Int): Long { val exponent = (attemptCount - 2).coerceAtLeast(0) val raw = INITIAL_POLL_INTERVAL_MS * BACKOFF_MULTIPLIER.pow(exponent.toDouble()) diff --git a/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosFileBasedSyncActionTest.kt b/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosFileBasedSyncActionTest.kt index c9f803af9217..0f82da083b63 100644 --- a/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosFileBasedSyncActionTest.kt +++ b/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosFileBasedSyncActionTest.kt @@ -84,10 +84,10 @@ class WooPosFileBasedSyncActionTest { val result = sut.syncCatalog(site) // THEN - assertThat(result.isSuccess).isTrue() - val syncResult = result.getOrThrow() - assertThat(syncResult.productsStored).isEqualTo(1) - assertThat(syncResult.variationsStored).isEqualTo(1) + assertThat(result).isInstanceOf(PosLocalCatalogSyncResult.Success::class.java) + val syncResult = result as PosLocalCatalogSyncResult.Success + assertThat(syncResult.productsSynced).isEqualTo(1) + assertThat(syncResult.variationsSynced).isEqualTo(1) } @Test @@ -139,7 +139,7 @@ class WooPosFileBasedSyncActionTest { val result = sut.syncCatalog(site) // THEN - assertThat(result.isSuccess).isTrue() + assertThat(result).isInstanceOf(PosLocalCatalogSyncResult.Success::class.java) verify(posLocalCatalogStore, times(3)).generateCatalogOrGetStatus(site) } @@ -152,7 +152,7 @@ class WooPosFileBasedSyncActionTest { val result = sut.syncCatalog(site) // THEN - assertThat(result.isSuccess).isTrue() + assertThat(result).isInstanceOf(PosLocalCatalogSyncResult.Success::class.java) verify(posLocalCatalogStore, times(2)).generateCatalogOrGetStatus(site) } @@ -165,7 +165,7 @@ class WooPosFileBasedSyncActionTest { val result = sut.syncCatalog(site) // THEN - assertThat(result.isSuccess).isTrue() + assertThat(result).isInstanceOf(PosLocalCatalogSyncResult.Success::class.java) } @Test @@ -177,7 +177,7 @@ class WooPosFileBasedSyncActionTest { val result = sut.syncCatalog(site) // THEN - assertThat(result.isFailure).isTrue() + assertThat(result).isInstanceOf(PosLocalCatalogSyncResult.Failure::class.java) } @Test @@ -189,8 +189,7 @@ class WooPosFileBasedSyncActionTest { val result = sut.syncCatalog(site) // THEN - assertThat(result.isFailure).isTrue() - assertThat(result.exceptionOrNull()?.message).contains("timed out") + assertThat(result).isInstanceOf(PosLocalCatalogSyncResult.Failure.CatalogGenerationTimeout::class.java) } @Test @@ -202,8 +201,7 @@ class WooPosFileBasedSyncActionTest { val result = sut.syncCatalog(site) // THEN - assertThat(result.isFailure).isTrue() - assertThat(result.exceptionOrNull()?.message).contains("URL is missing") + assertThat(result).isInstanceOf(PosLocalCatalogSyncResult.Failure.InvalidResponse::class.java) } @Test @@ -215,7 +213,7 @@ class WooPosFileBasedSyncActionTest { val result = sut.syncCatalog(site) // THEN - assertThat(result.isFailure).isTrue() + assertThat(result).isInstanceOf(PosLocalCatalogSyncResult.Failure::class.java) } @Test @@ -239,7 +237,7 @@ class WooPosFileBasedSyncActionTest { val result = sut.syncCatalog(site) // THEN - assertThat(result.isFailure).isTrue() + assertThat(result).isInstanceOf(PosLocalCatalogSyncResult.Failure::class.java) } @Test @@ -263,7 +261,7 @@ class WooPosFileBasedSyncActionTest { val result = sut.syncCatalog(site) // THEN - assertThat(result.isFailure).isTrue() + assertThat(result).isInstanceOf(PosLocalCatalogSyncResult.Failure::class.java) } @Test @@ -278,23 +276,6 @@ class WooPosFileBasedSyncActionTest { verify(catalogFileDownloader, never()).cleanupOldCatalogFiles(any()) } - @Test - fun `given happy path, when syncCatalog, then includes total and completedAt in result`() = runTest { - // GIVEN - val total = 100 - val completedAt = "2024-01-15T10:30:00Z" - givenCatalogGenerationCompletedWithDetails(total = total, completedAt = completedAt) - - // WHEN - val result = sut.syncCatalog(site) - - // THEN - assertThat(result.isSuccess).isTrue() - val syncResult = result.getOrThrow() - assertThat(syncResult.totalProducts).isEqualTo(total) - assertThat(syncResult.completedAt).isEqualTo(completedAt) - } - @Test fun `given empty catalog, when syncCatalog, then returns success with zero counts`() = runTest { // GIVEN @@ -309,10 +290,10 @@ class WooPosFileBasedSyncActionTest { val result = sut.syncCatalog(site) // THEN - assertThat(result.isSuccess).isTrue() - val syncResult = result.getOrThrow() - assertThat(syncResult.productsStored).isEqualTo(0) - assertThat(syncResult.variationsStored).isEqualTo(0) + assertThat(result).isInstanceOf(PosLocalCatalogSyncResult.Success::class.java) + val syncResult = result as PosLocalCatalogSyncResult.Success + assertThat(syncResult.productsSynced).isEqualTo(0) + assertThat(syncResult.variationsSynced).isEqualTo(0) } @Test @@ -329,10 +310,10 @@ class WooPosFileBasedSyncActionTest { val result = sut.syncCatalog(site) // THEN - assertThat(result.isSuccess).isTrue() - val syncResult = result.getOrThrow() - assertThat(syncResult.productsStored).isEqualTo(1) - assertThat(syncResult.variationsStored).isEqualTo(0) + assertThat(result).isInstanceOf(PosLocalCatalogSyncResult.Success::class.java) + val syncResult = result as PosLocalCatalogSyncResult.Success + assertThat(syncResult.productsSynced).isEqualTo(1) + assertThat(syncResult.variationsSynced).isEqualTo(0) } @Test @@ -349,10 +330,10 @@ class WooPosFileBasedSyncActionTest { val result = sut.syncCatalog(site) // THEN - assertThat(result.isSuccess).isTrue() - val syncResult = result.getOrThrow() - assertThat(syncResult.productsStored).isEqualTo(0) - assertThat(syncResult.variationsStored).isEqualTo(1) + assertThat(result).isInstanceOf(PosLocalCatalogSyncResult.Success::class.java) + val syncResult = result as PosLocalCatalogSyncResult.Success + assertThat(syncResult.productsSynced).isEqualTo(0) + assertThat(syncResult.variationsSynced).isEqualTo(1) } @Test @@ -364,7 +345,7 @@ class WooPosFileBasedSyncActionTest { val result = sut.syncCatalog(site) // THEN - assertThat(result.isSuccess).isTrue() + assertThat(result).isInstanceOf(PosLocalCatalogSyncResult.Success::class.java) } private suspend fun givenCatalogGenerationCompleted() { @@ -381,20 +362,6 @@ class WooPosFileBasedSyncActionTest { ) } - private suspend fun givenCatalogGenerationCompletedWithDetails(total: Int, completedAt: String) { - whenever(posLocalCatalogStore.generateCatalogOrGetStatus(site)) - .thenReturn( - Result.success( - WooPosGenerateCatalogResult( - state = WooPosGenerateCatalogState.COMPLETED, - url = defaultUrl, - total = total, - completedAt = completedAt - ) - ) - ) - } - private suspend fun givenCatalogGenerationCompletedWithoutUrl() { whenever(posLocalCatalogStore.generateCatalogOrGetStatus(site)) .thenReturn( From b9873d9d371b74af1491bf445322f084167a2cf9 Mon Sep 17 00:00:00 2001 From: malinajirka Date: Thu, 8 Jan 2026 13:59:28 +0100 Subject: [PATCH 3/5] Wire WooPosFileBasedSyncAction when FF enabled --- .../localcatalog/WooPosFileBasedSyncAction.kt | 13 +++++++---- .../WooPosLocalCatalogSyncRepository.kt | 22 ++++++++++++++----- .../WooPosLocalCatalogSyncRepositoryTest.kt | 2 ++ 3 files changed, 27 insertions(+), 10 deletions(-) diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosFileBasedSyncAction.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosFileBasedSyncAction.kt index 8bd6fb170c7b..7965509f35b0 100644 --- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosFileBasedSyncAction.kt +++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosFileBasedSyncAction.kt @@ -43,8 +43,10 @@ class WooPosFileBasedSyncAction @Inject constructor( if (response.isFailure) { if (++failedConsecutiveAttempts >= MAX_CONSECUTIVE_FAILED_ATTEMPTS) { val error = response.exceptionOrNull() - logger.e("WooPosFileBasedSyncAction: File-based sync failed " + - "after $MAX_CONSECUTIVE_FAILED_ATTEMPTS consecutive failures") + logger.e( + "WooPosFileBasedSyncAction: File-based sync failed " + + "after $MAX_CONSECUTIVE_FAILED_ATTEMPTS consecutive failures" + ) return PosLocalCatalogSyncResult.Failure.NetworkError( error?.message ?: "API error during catalog sync" ) @@ -89,9 +91,12 @@ class WooPosFileBasedSyncAction @Inject constructor( ) } } + else -> null.also { - logger.d("WooPosFileBasedSyncAction: State: ${result.state}, Progress: ${result.progress}% " + - "out of ${result.total} items") + logger.d( + "WooPosFileBasedSyncAction: State: ${result.state}, Progress: ${result.progress}% " + + "out of ${result.total} items" + ) } } } diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosLocalCatalogSyncRepository.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosLocalCatalogSyncRepository.kt index eee7c868144f..f6d6f487169d 100644 --- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosLocalCatalogSyncRepository.kt +++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosLocalCatalogSyncRepository.kt @@ -11,6 +11,7 @@ import com.woocommerce.android.ui.woopos.util.analytics.WooPosAnalyticsTracker import com.woocommerce.android.ui.woopos.util.datastore.WooPosPreferencesRepository import com.woocommerce.android.ui.woopos.util.datastore.WooPosSyncTimestampManager import com.woocommerce.android.util.CoroutineDispatchers +import com.woocommerce.android.util.FeatureFlag import kotlinx.coroutines.withContext import org.wordpress.android.fluxc.model.LocalOrRemoteId.LocalId import org.wordpress.android.fluxc.model.SiteModel @@ -21,6 +22,7 @@ import javax.inject.Singleton @Singleton class WooPosLocalCatalogSyncRepository @Inject constructor( private val posSyncAction: WooPosSyncAction, + private val posFileBasedSyncAction: WooPosFileBasedSyncAction, private val posCheckCatalogSizeAction: WooPosCheckCatalogSizeAction, private val syncTimestampManager: WooPosSyncTimestampManager, private val preferencesRepository: WooPosPreferencesRepository, @@ -47,12 +49,16 @@ class WooPosLocalCatalogSyncRepository @Inject constructor( ) ) - return@withContext performSync( - site = site, - pageSize = PAGE_SIZE, - maxPages = MAX_PAGES_PER_FULL_SYNC, - maxTotalItems = MAX_TOTAL_ITEMS_FULL_SYNC - ).also { result -> + return@withContext if (FeatureFlag.WOO_POS_LOCAL_CATALOG_FILE_APPROACH.isEnabled()) { + performFileBasedSync(site) + } else { + performSync( + site = site, + pageSize = PAGE_SIZE, + maxPages = MAX_PAGES_PER_FULL_SYNC, + maxTotalItems = MAX_TOTAL_ITEMS_FULL_SYNC + ) + }.also { result -> when (result) { is PosLocalCatalogSyncResult.Success -> { syncTimestampManager.storeFullSyncLastCompletedTimestamp(dateTimeProvider.now()) @@ -97,6 +103,10 @@ class WooPosLocalCatalogSyncRepository @Inject constructor( } } + private suspend fun performFileBasedSync(site: SiteModel): PosLocalCatalogSyncResult { + return posFileBasedSyncAction.syncCatalog(site) + } + private suspend fun trackSyncCompleted( site: SiteModel, syncType: SyncType, diff --git a/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosLocalCatalogSyncRepositoryTest.kt b/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosLocalCatalogSyncRepositoryTest.kt index 8dbb98ffd51d..a33b67d9768a 100644 --- a/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosLocalCatalogSyncRepositoryTest.kt +++ b/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosLocalCatalogSyncRepositoryTest.kt @@ -29,6 +29,7 @@ import org.wordpress.android.fluxc.store.pos.localcatalog.WooPosLocalCatalogStor class WooPosLocalCatalogSyncRepositoryTest : BaseUnitTest() { private lateinit var sut: WooPosLocalCatalogSyncRepository private var posSyncAction: WooPosSyncAction = mock() + private var posFileBasedSyncAction: WooPosFileBasedSyncAction = mock() private var posCheckCatalogSizeAction: WooPosCheckCatalogSizeAction = mock() private var syncTimestampManager: WooPosSyncTimestampManager = mock() private var preferencesRepository: WooPosPreferencesRepository = mock() @@ -50,6 +51,7 @@ class WooPosLocalCatalogSyncRepositoryTest : BaseUnitTest() { sut = WooPosLocalCatalogSyncRepository( posSyncAction = posSyncAction, + posFileBasedSyncAction = posFileBasedSyncAction, posCheckCatalogSizeAction = posCheckCatalogSizeAction, syncTimestampManager = syncTimestampManager, dispatchers = dispatchers, From 4191e474fef5b3f4b11cb723ef0f95c7486a86af Mon Sep 17 00:00:00 2001 From: malinajirka Date: Thu, 8 Jan 2026 15:35:38 +0100 Subject: [PATCH 4/5] Update WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosFileBasedSyncAction.kt Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- .../android/ui/woopos/localcatalog/WooPosFileBasedSyncAction.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosFileBasedSyncAction.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosFileBasedSyncAction.kt index 297d85255535..1f307341e129 100644 --- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosFileBasedSyncAction.kt +++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosFileBasedSyncAction.kt @@ -135,7 +135,7 @@ class WooPosFileBasedSyncAction @Inject constructor( val syncDuration = System.currentTimeMillis() - startTime logger.d( - "File-based sync completed successfully. " + + "WooPosFileBasedSyncAction: File-based sync completed successfully. " + "Products: ${parsedData.products.size}, Variations: ${parsedData.variations.size}. " + "Duration: ${syncDuration}ms." ) From 50b0e03cd5e1e999c7c408a51bacef1271f20e1f Mon Sep 17 00:00:00 2001 From: malinajirka Date: Thu, 8 Jan 2026 16:57:29 +0100 Subject: [PATCH 5/5] Set last sync timestamps for file based sync --- .../localcatalog/WooPosFileBasedSyncAction.kt | 73 +++++++++++++------ .../WooPosLocalCatalogSyncRepository.kt | 13 +++- .../WooPosFileBasedSyncActionTest.kt | 40 +++++----- 3 files changed, 83 insertions(+), 43 deletions(-) diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosFileBasedSyncAction.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosFileBasedSyncAction.kt index 1f307341e129..0d6d3c7f1b9b 100644 --- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosFileBasedSyncAction.kt +++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosFileBasedSyncAction.kt @@ -23,8 +23,18 @@ class WooPosFileBasedSyncAction @Inject constructor( private const val MAX_POLL_INTERVAL_MS = 30_000L private const val BACKOFF_MULTIPLIER = 1.3 } + sealed class WooPosFileBasedSyncResult { + data class Success( + val result: PosLocalCatalogSyncResult.Success, + val lastModifiedDate: String? + ) : WooPosFileBasedSyncResult() + + data class Failure( + val result: PosLocalCatalogSyncResult.Failure + ) : WooPosFileBasedSyncResult() + } - suspend fun syncCatalog(site: SiteModel): PosLocalCatalogSyncResult { + suspend fun syncCatalog(site: SiteModel): WooPosFileBasedSyncResult { val startTime = System.currentTimeMillis() logger.d("WooPosFileBasedSyncAction: Starting file-based catalog generation for site ${site.id}") @@ -46,8 +56,10 @@ class WooPosFileBasedSyncAction @Inject constructor( "WooPosFileBasedSyncAction: File-based sync failed " + "after $MAX_CONSECUTIVE_FAILED_ATTEMPTS consecutive failures" ) - return PosLocalCatalogSyncResult.Failure.NetworkError( - error?.message ?: "API error during catalog sync" + return WooPosFileBasedSyncResult.Failure( + PosLocalCatalogSyncResult.Failure.NetworkError( + error?.message ?: "API error during catalog sync" + ) ) } else { logger.w("Poll attempt $attemptCount failed: ${response.exceptionOrNull()?.message}") @@ -66,8 +78,10 @@ class WooPosFileBasedSyncAction @Inject constructor( } logger.e("WooPosFileBasedSyncAction: Catalog generation timed out after $MAX_POLL_ATTEMPTS attempts") - return PosLocalCatalogSyncResult.Failure.CatalogGenerationTimeout( - "Catalog generation is taking longer than expected." + return WooPosFileBasedSyncResult.Failure( + PosLocalCatalogSyncResult.Failure.CatalogGenerationTimeout( + "Catalog generation is taking longer than expected." + ) ) } @@ -75,17 +89,18 @@ class WooPosFileBasedSyncAction @Inject constructor( result: WooPosGenerateCatalogResult, site: SiteModel, startTime: Long - ): PosLocalCatalogSyncResult? { + ): WooPosFileBasedSyncResult? { return when (result.state) { WooPosGenerateCatalogState.COMPLETED -> { - val url = result.url - if (url != null) { + if (result.url != null) { logger.d("WooPosFileBasedSyncAction: Catalog available, starting download.") - processDownloadAndStore(url, site, startTime) + processDownloadAndStore(result, site, startTime) } else { logger.e("WooPosFileBasedSyncAction: Catalog generation completed but URL is missing") - PosLocalCatalogSyncResult.Failure.InvalidResponse( - "Catalog generation completed but download URL is missing." + WooPosFileBasedSyncResult.Failure( + PosLocalCatalogSyncResult.Failure.InvalidResponse( + "Catalog generation completed but download URL is missing." + ) ) } } @@ -100,23 +115,27 @@ class WooPosFileBasedSyncAction @Inject constructor( } private suspend fun processDownloadAndStore( - url: String, + result: WooPosGenerateCatalogResult, site: SiteModel, startTime: Long - ): PosLocalCatalogSyncResult { - val downloadedFile = catalogFileDownloader.downloadCatalogFile(url, site.localId()) + ): WooPosFileBasedSyncResult { + val downloadedFile = catalogFileDownloader.downloadCatalogFile(result.url!!, site.localId()) .onFailureLog("Failed to download catalog file") .getOrElse { - return PosLocalCatalogSyncResult.Failure.NetworkError( - it.message ?: "Failed to download catalog file" + return WooPosFileBasedSyncResult.Failure( + PosLocalCatalogSyncResult.Failure.NetworkError( + it.message ?: "Failed to download catalog file" + ) ) } val parsedData = catalogFileParser.parseCatalogFile(downloadedFile, site.localId()) .onFailureLog("Failed to parse catalog file") .getOrElse { - return PosLocalCatalogSyncResult.Failure.InvalidResponse( - it.message ?: "Failed to parse catalog file" + return WooPosFileBasedSyncResult.Failure( + PosLocalCatalogSyncResult.Failure.InvalidResponse( + it.message ?: "Failed to parse catalog file" + ) ) } @@ -126,8 +145,10 @@ class WooPosFileBasedSyncAction @Inject constructor( variations = parsedData.variations ).onFailureLog("Failed to store catalog data") .getOrElse { - return PosLocalCatalogSyncResult.Failure.DatabaseError( - it.message ?: "Failed to store catalog data" + return WooPosFileBasedSyncResult.Failure( + PosLocalCatalogSyncResult.Failure.DatabaseError( + it.message ?: "Failed to store catalog data" + ) ) } @@ -140,10 +161,14 @@ class WooPosFileBasedSyncAction @Inject constructor( "Duration: ${syncDuration}ms." ) - return PosLocalCatalogSyncResult.Success( - productsSynced = parsedData.products.size, - variationsSynced = parsedData.variations.size, - syncDurationMs = syncDuration + return WooPosFileBasedSyncResult.Success( + PosLocalCatalogSyncResult.Success( + productsSynced = parsedData.products.size, + variationsSynced = parsedData.variations.size, + syncDurationMs = syncDuration + ), + // Using scheduledAt (not completedAt) to not miss changes made during generation + lastModifiedDate = result.scheduledAt ) } diff --git a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosLocalCatalogSyncRepository.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosLocalCatalogSyncRepository.kt index 427276ed9f02..0231f6384fbd 100644 --- a/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosLocalCatalogSyncRepository.kt +++ b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosLocalCatalogSyncRepository.kt @@ -104,7 +104,18 @@ class WooPosLocalCatalogSyncRepository @Inject constructor( } private suspend fun performFileBasedSync(site: SiteModel): PosLocalCatalogSyncResult { - return posFileBasedSyncAction.syncCatalog(site) + return when (val result = posFileBasedSyncAction.syncCatalog(site)) { + is WooPosFileBasedSyncAction.WooPosFileBasedSyncResult.Success -> result.result.also { + if (result.lastModifiedDate != null) { + syncTimestampManager.parseTimestampFromApi(result.lastModifiedDate)?.let { timestamp -> + syncTimestampManager.storeProductsLastSyncTimestamp(timestamp) + syncTimestampManager.storeVariationsLastSyncTimestamp(timestamp) + } + } + } + + is WooPosFileBasedSyncAction.WooPosFileBasedSyncResult.Failure -> result.result + } } private suspend fun trackSyncCompleted( diff --git a/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosFileBasedSyncActionTest.kt b/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosFileBasedSyncActionTest.kt index 0f82da083b63..0f154349fc63 100644 --- a/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosFileBasedSyncActionTest.kt +++ b/WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosFileBasedSyncActionTest.kt @@ -84,8 +84,8 @@ class WooPosFileBasedSyncActionTest { val result = sut.syncCatalog(site) // THEN - assertThat(result).isInstanceOf(PosLocalCatalogSyncResult.Success::class.java) - val syncResult = result as PosLocalCatalogSyncResult.Success + assertThat(result).isInstanceOf(WooPosFileBasedSyncAction.WooPosFileBasedSyncResult.Success::class.java) + val syncResult = (result as WooPosFileBasedSyncAction.WooPosFileBasedSyncResult.Success).result assertThat(syncResult.productsSynced).isEqualTo(1) assertThat(syncResult.variationsSynced).isEqualTo(1) } @@ -139,7 +139,7 @@ class WooPosFileBasedSyncActionTest { val result = sut.syncCatalog(site) // THEN - assertThat(result).isInstanceOf(PosLocalCatalogSyncResult.Success::class.java) + assertThat(result).isInstanceOf(WooPosFileBasedSyncAction.WooPosFileBasedSyncResult.Success::class.java) verify(posLocalCatalogStore, times(3)).generateCatalogOrGetStatus(site) } @@ -152,7 +152,7 @@ class WooPosFileBasedSyncActionTest { val result = sut.syncCatalog(site) // THEN - assertThat(result).isInstanceOf(PosLocalCatalogSyncResult.Success::class.java) + assertThat(result).isInstanceOf(WooPosFileBasedSyncAction.WooPosFileBasedSyncResult.Success::class.java) verify(posLocalCatalogStore, times(2)).generateCatalogOrGetStatus(site) } @@ -165,7 +165,7 @@ class WooPosFileBasedSyncActionTest { val result = sut.syncCatalog(site) // THEN - assertThat(result).isInstanceOf(PosLocalCatalogSyncResult.Success::class.java) + assertThat(result).isInstanceOf(WooPosFileBasedSyncAction.WooPosFileBasedSyncResult.Success::class.java) } @Test @@ -177,7 +177,7 @@ class WooPosFileBasedSyncActionTest { val result = sut.syncCatalog(site) // THEN - assertThat(result).isInstanceOf(PosLocalCatalogSyncResult.Failure::class.java) + assertThat(result).isInstanceOf(WooPosFileBasedSyncAction.WooPosFileBasedSyncResult.Failure::class.java) } @Test @@ -189,7 +189,9 @@ class WooPosFileBasedSyncActionTest { val result = sut.syncCatalog(site) // THEN - assertThat(result).isInstanceOf(PosLocalCatalogSyncResult.Failure.CatalogGenerationTimeout::class.java) + assertThat(result).isInstanceOf(WooPosFileBasedSyncAction.WooPosFileBasedSyncResult.Failure::class.java) + val failure = (result as WooPosFileBasedSyncAction.WooPosFileBasedSyncResult.Failure).result + assertThat(failure).isInstanceOf(PosLocalCatalogSyncResult.Failure.CatalogGenerationTimeout::class.java) } @Test @@ -201,7 +203,9 @@ class WooPosFileBasedSyncActionTest { val result = sut.syncCatalog(site) // THEN - assertThat(result).isInstanceOf(PosLocalCatalogSyncResult.Failure.InvalidResponse::class.java) + assertThat(result).isInstanceOf(WooPosFileBasedSyncAction.WooPosFileBasedSyncResult.Failure::class.java) + val failure = (result as WooPosFileBasedSyncAction.WooPosFileBasedSyncResult.Failure).result + assertThat(failure).isInstanceOf(PosLocalCatalogSyncResult.Failure.InvalidResponse::class.java) } @Test @@ -213,7 +217,7 @@ class WooPosFileBasedSyncActionTest { val result = sut.syncCatalog(site) // THEN - assertThat(result).isInstanceOf(PosLocalCatalogSyncResult.Failure::class.java) + assertThat(result).isInstanceOf(WooPosFileBasedSyncAction.WooPosFileBasedSyncResult.Failure::class.java) } @Test @@ -237,7 +241,7 @@ class WooPosFileBasedSyncActionTest { val result = sut.syncCatalog(site) // THEN - assertThat(result).isInstanceOf(PosLocalCatalogSyncResult.Failure::class.java) + assertThat(result).isInstanceOf(WooPosFileBasedSyncAction.WooPosFileBasedSyncResult.Failure::class.java) } @Test @@ -261,7 +265,7 @@ class WooPosFileBasedSyncActionTest { val result = sut.syncCatalog(site) // THEN - assertThat(result).isInstanceOf(PosLocalCatalogSyncResult.Failure::class.java) + assertThat(result).isInstanceOf(WooPosFileBasedSyncAction.WooPosFileBasedSyncResult.Failure::class.java) } @Test @@ -290,8 +294,8 @@ class WooPosFileBasedSyncActionTest { val result = sut.syncCatalog(site) // THEN - assertThat(result).isInstanceOf(PosLocalCatalogSyncResult.Success::class.java) - val syncResult = result as PosLocalCatalogSyncResult.Success + assertThat(result).isInstanceOf(WooPosFileBasedSyncAction.WooPosFileBasedSyncResult.Success::class.java) + val syncResult = (result as WooPosFileBasedSyncAction.WooPosFileBasedSyncResult.Success).result assertThat(syncResult.productsSynced).isEqualTo(0) assertThat(syncResult.variationsSynced).isEqualTo(0) } @@ -310,8 +314,8 @@ class WooPosFileBasedSyncActionTest { val result = sut.syncCatalog(site) // THEN - assertThat(result).isInstanceOf(PosLocalCatalogSyncResult.Success::class.java) - val syncResult = result as PosLocalCatalogSyncResult.Success + assertThat(result).isInstanceOf(WooPosFileBasedSyncAction.WooPosFileBasedSyncResult.Success::class.java) + val syncResult = (result as WooPosFileBasedSyncAction.WooPosFileBasedSyncResult.Success).result assertThat(syncResult.productsSynced).isEqualTo(1) assertThat(syncResult.variationsSynced).isEqualTo(0) } @@ -330,8 +334,8 @@ class WooPosFileBasedSyncActionTest { val result = sut.syncCatalog(site) // THEN - assertThat(result).isInstanceOf(PosLocalCatalogSyncResult.Success::class.java) - val syncResult = result as PosLocalCatalogSyncResult.Success + assertThat(result).isInstanceOf(WooPosFileBasedSyncAction.WooPosFileBasedSyncResult.Success::class.java) + val syncResult = (result as WooPosFileBasedSyncAction.WooPosFileBasedSyncResult.Success).result assertThat(syncResult.productsSynced).isEqualTo(0) assertThat(syncResult.variationsSynced).isEqualTo(1) } @@ -345,7 +349,7 @@ class WooPosFileBasedSyncActionTest { val result = sut.syncCatalog(site) // THEN - assertThat(result).isInstanceOf(PosLocalCatalogSyncResult.Success::class.java) + assertThat(result).isInstanceOf(WooPosFileBasedSyncAction.WooPosFileBasedSyncResult.Success::class.java) } private suspend fun givenCatalogGenerationCompleted() {