[WOOMOB-1949] Wire file-based sync action and use shared result type#15174
Conversation
…and-paginated-download' into issue/woomob-1949-poslocal-catalog-switch-between-file-and-paginated-download # Conflicts: # WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosFileBasedSyncAction.kt # WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosFileBasedSyncActionTest.kt
…etween-file-and-paginated-download # Conflicts: # WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosFileBasedSyncAction.kt
…and-paginated-download' into issue/woomob-1949-poslocal-catalog-switch-between-file-and-paginated-download # Conflicts: # WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosFileBasedSyncAction.kt
📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
|
|
📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.
|
There was a problem hiding this comment.
Pull request overview
This PR wires the WooPosFileBasedSyncAction into the WooPosLocalCatalogSyncRepository and refactors it to return PosLocalCatalogSyncResult directly instead of wrapping results in Result<FileBasedSyncResult>. This eliminates error mapping at the repository level and makes the file-based sync action consistent with the existing paginated sync action.
Key changes:
- Feature flag-based conditional routing between file-based and paginated sync approaches
- Direct return of
PosLocalCatalogSyncResulttypes with specific failure subtypes (NetworkError, InvalidResponse, DatabaseError, CatalogGenerationTimeout) - Removal of intermediate
FileBasedSyncResultdata class
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
WooPosLocalCatalogSyncRepository.kt |
Added dependency injection for WooPosFileBasedSyncAction and feature flag-based conditional logic to route sync requests |
WooPosFileBasedSyncAction.kt |
Refactored return type from Result<FileBasedSyncResult> to PosLocalCatalogSyncResult with improved error handling and logging consistency |
WooPosFileBasedSyncActionTest.kt |
Updated test assertions to match new return type, removed obsolete test for fields no longer in the result |
WooPosLocalCatalogSyncRepositoryTest.kt |
Added mock for WooPosFileBasedSyncAction |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
.../src/main/kotlin/com/woocommerce/android/ui/woopos/localcatalog/WooPosFileBasedSyncAction.kt
Outdated
Show resolved
Hide resolved
…localcatalog/WooPosFileBasedSyncAction.kt Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## trunk #15174 +/- ##
=========================================
Coverage 38.65% 38.65%
Complexity 10520 10520
=========================================
Files 2189 2189
Lines 124369 124400 +31
Branches 17189 17196 +7
=========================================
+ Hits 48069 48091 +22
- Misses 71449 71455 +6
- Partials 4851 4854 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| class WooPosLocalCatalogSyncRepositoryTest : BaseUnitTest() { | ||
| private lateinit var sut: WooPosLocalCatalogSyncRepository | ||
| private var posSyncAction: WooPosSyncAction = mock() | ||
| private var posFileBasedSyncAction: WooPosFileBasedSyncAction = mock() |
There was a problem hiding this comment.
The new file-based sync path added to syncLocalCatalogFull when the feature flag is enabled lacks test coverage. Consider adding test cases that verify:
- The file-based sync action is called when the feature flag is enabled
- Timestamps are properly stored from the lastModifiedDate when file-based sync succeeds
- Failure scenarios from file-based sync are properly handled
- The regular paginated sync is still used when the feature flag is disabled
…etween-file-and-paginated-download
Fixes WOOMOB-1949
Description
Wires
WooPosFileBasedSyncActioninto the repository whenWOO_POS_LOCAL_CATALOG_FILE_APPROACHfeature flag is enabled, and refactors it to return wrapper around PosLocalCatalogSyncResult instead ofResult<FileBasedSyncResult>.This eliminates the need for error mapping in the repository layer - the action now handles all error cases and returns the appropriate
PosLocalCatalogSyncResult.Failuretypes directly (NetworkError, InvalidResponse, DatabaseError, CatalogGenerationTimeout).Note: There are still a lot of things that need to be done before this code is ready for production. For example:
Test Steps
IMPORTANT - Make sure to use supported version of Woo on your site. You can use Jetpack Beta plugin and select
issue/woomob-1894-add-pos-visibility-taxonomy-to-corebranch of Woo core.Test file-based sync (new behavior):
WOO_POS_LOCAL_CATALOG_FILE_APPROACHfeature flagTest for regressions (paginated sync):
WOO_POS_LOCAL_CATALOG_FILE_APPROACHfeature flagImages/gif
N/A - Internal sync logic changes
RELEASE-NOTES.txtif necessary. Use the "[Internal]" label for non-user-facing changes.