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 33f2aa3aeab2..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,18 +23,27 @@ 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 - ): Result { - logger.d("Starting file-based catalog generation for site ${site.id}") + suspend fun syncCatalog(site: SiteModel): WooPosFileBasedSyncResult { + 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) } @@ -42,7 +51,16 @@ 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 WooPosFileBasedSyncResult.Failure( + PosLocalCatalogSyncResult.Failure.NetworkError( + error?.message ?: "API error during catalog sync" + ) + ) } else { logger.w("Poll attempt $attemptCount failed: ${response.exceptionOrNull()?.message}") return@repeat @@ -51,99 +69,114 @@ 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 WooPosFileBasedSyncResult.Failure( + 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 - ) - private suspend fun processPollingResult( result: WooPosGenerateCatalogResult, - site: SiteModel - ): Result? { + site: SiteModel, + startTime: Long + ): WooPosFileBasedSyncResult? { return when (result.state) { WooPosGenerateCatalogState.COMPLETED -> { - val url = result.url - if (url != null) { - logger.d("Catalog available, starting download.") - - processDownloadAndStore(url, result, site) + if (result.url != null) { + logger.d("WooPosFileBasedSyncAction: Catalog available, starting download.") + processDownloadAndStore(result, 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") + WooPosFileBasedSyncResult.Failure( + 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" + ) } } } - @Suppress("ReturnCount") private suspend fun processDownloadAndStore( - url: String, - catalogResult: WooPosGenerateCatalogResult, - site: SiteModel - ): Result { - val downloadedFile = catalogFileDownloader.downloadCatalogFile(url, site.localId()) + result: WooPosGenerateCatalogResult, + site: SiteModel, + startTime: Long + ): WooPosFileBasedSyncResult { + val downloadedFile = catalogFileDownloader.downloadCatalogFile(result.url!!, site.localId()) .onFailureLog("Failed to download catalog file") - .getOrElse { return Result.failure(it) } + .getOrElse { + 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 Result.failure(it) } + .getOrElse { + return WooPosFileBasedSyncResult.Failure( + 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 WooPosFileBasedSyncResult.Failure( + 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( + "WooPosFileBasedSyncAction: File-based sync completed successfully. " + + "Products: ${parsedData.products.size}, Variations: ${parsedData.variations.size}. " + + "Duration: ${syncDuration}ms." + ) + + 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 ) } 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/main/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosLocalCatalogSyncRepository.kt b/WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosLocalCatalogSyncRepository.kt index d6f9dc14d068..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 @@ -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,21 @@ class WooPosLocalCatalogSyncRepository @Inject constructor( } } + private suspend fun performFileBasedSync(site: SiteModel): PosLocalCatalogSyncResult { + 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( site: SiteModel, syncType: SyncType, 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..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,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(WooPosFileBasedSyncAction.WooPosFileBasedSyncResult.Success::class.java) + val syncResult = (result as WooPosFileBasedSyncAction.WooPosFileBasedSyncResult.Success).result + 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(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.isSuccess).isTrue() + 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.isSuccess).isTrue() + assertThat(result).isInstanceOf(WooPosFileBasedSyncAction.WooPosFileBasedSyncResult.Success::class.java) } @Test @@ -177,7 +177,7 @@ class WooPosFileBasedSyncActionTest { val result = sut.syncCatalog(site) // THEN - assertThat(result.isFailure).isTrue() + assertThat(result).isInstanceOf(WooPosFileBasedSyncAction.WooPosFileBasedSyncResult.Failure::class.java) } @Test @@ -189,8 +189,9 @@ class WooPosFileBasedSyncActionTest { val result = sut.syncCatalog(site) // THEN - assertThat(result.isFailure).isTrue() - assertThat(result.exceptionOrNull()?.message).contains("timed out") + 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 @@ -202,8 +203,9 @@ class WooPosFileBasedSyncActionTest { val result = sut.syncCatalog(site) // THEN - assertThat(result.isFailure).isTrue() - assertThat(result.exceptionOrNull()?.message).contains("URL is missing") + 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 @@ -215,7 +217,7 @@ class WooPosFileBasedSyncActionTest { val result = sut.syncCatalog(site) // THEN - assertThat(result.isFailure).isTrue() + assertThat(result).isInstanceOf(WooPosFileBasedSyncAction.WooPosFileBasedSyncResult.Failure::class.java) } @Test @@ -239,7 +241,7 @@ class WooPosFileBasedSyncActionTest { val result = sut.syncCatalog(site) // THEN - assertThat(result.isFailure).isTrue() + assertThat(result).isInstanceOf(WooPosFileBasedSyncAction.WooPosFileBasedSyncResult.Failure::class.java) } @Test @@ -263,7 +265,7 @@ class WooPosFileBasedSyncActionTest { val result = sut.syncCatalog(site) // THEN - assertThat(result.isFailure).isTrue() + assertThat(result).isInstanceOf(WooPosFileBasedSyncAction.WooPosFileBasedSyncResult.Failure::class.java) } @Test @@ -278,23 +280,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 +294,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(WooPosFileBasedSyncAction.WooPosFileBasedSyncResult.Success::class.java) + val syncResult = (result as WooPosFileBasedSyncAction.WooPosFileBasedSyncResult.Success).result + assertThat(syncResult.productsSynced).isEqualTo(0) + assertThat(syncResult.variationsSynced).isEqualTo(0) } @Test @@ -329,10 +314,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(WooPosFileBasedSyncAction.WooPosFileBasedSyncResult.Success::class.java) + val syncResult = (result as WooPosFileBasedSyncAction.WooPosFileBasedSyncResult.Success).result + assertThat(syncResult.productsSynced).isEqualTo(1) + assertThat(syncResult.variationsSynced).isEqualTo(0) } @Test @@ -349,10 +334,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(WooPosFileBasedSyncAction.WooPosFileBasedSyncResult.Success::class.java) + val syncResult = (result as WooPosFileBasedSyncAction.WooPosFileBasedSyncResult.Success).result + assertThat(syncResult.productsSynced).isEqualTo(0) + assertThat(syncResult.variationsSynced).isEqualTo(1) } @Test @@ -364,7 +349,7 @@ class WooPosFileBasedSyncActionTest { val result = sut.syncCatalog(site) // THEN - assertThat(result.isSuccess).isTrue() + assertThat(result).isInstanceOf(WooPosFileBasedSyncAction.WooPosFileBasedSyncResult.Success::class.java) } private suspend fun givenCatalogGenerationCompleted() { @@ -381,20 +366,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( 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,