-
Notifications
You must be signed in to change notification settings - Fork 136
[POS][Local Catalog] Connect product list to POS database #14636
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[POS][Local Catalog] Connect product list to POS database #14636
Conversation
📲 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.
|
…tch-products-and' into woomob-1072-woo-poslocal-catalog-connect-product-list-to-pos-products
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR integrates local database storage for the POS product catalog, controlled by the WOO_POS_LOCAL_CATALOG_M1 feature flag. When enabled, products are loaded from the local database instead of the existing cache/remote hybrid approach.
- Adds sorting by product name (case-insensitive) at the database level
- Introduces a new database-backed data source for products with search filtering capabilities
- Updates product retrieval logic in cart and totals repositories to support both local catalog and legacy flows
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| WooPosProductsDao.kt | Adds case-insensitive sorting by name to database query |
| WooPosProductsInDbDataSource.kt | New data source implementation for database-backed product loading |
| WooPosProductsDataSourceInterface.kt | Interface to unify different product data source implementations |
| WooPosProductsViewModel.kt | Updates to use feature flag-based data source selection |
| WooPosTotalsRepository.kt | Adds conditional product retrieval from local catalog |
| WooPosCartViewModel.kt | Adds conditional product retrieval from local catalog |
| Test files | Updates and new tests to support the changes |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
...kotlin/com/woocommerce/android/ui/woopos/home/items/products/WooPosProductsInDbDataSource.kt
Outdated
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## trunk #14636 +/- ##
=========================================
Coverage 38.42% 38.42%
- Complexity 9812 9818 +6
=========================================
Files 2086 2087 +1
Lines 116448 116470 +22
Branches 15563 15563
=========================================
+ Hits 44748 44757 +9
- Misses 67549 67562 +13
Partials 4151 4151 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 4 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
...erce/src/main/kotlin/com/woocommerce/android/ui/woopos/home/totals/WooPosTotalsRepository.kt
Outdated
Show resolved
Hide resolved
...erce/src/main/kotlin/com/woocommerce/android/ui/woopos/home/totals/WooPosTotalsRepository.kt
Outdated
Show resolved
Hide resolved
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/home/cart/WooPosCartViewModel.kt
Outdated
Show resolved
Hide resolved
WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/home/cart/WooPosCartViewModel.kt
Outdated
Show resolved
Hide resolved
|
|
||
| init { | ||
| listenEventsFromParent() | ||
| viewModelScope.launch { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this or should the data source object be created every time when the VM is created?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not needed since fetchFirstPage() already handles the state reset internally. Removed: 0f3456a
| private suspend fun handleSimpleProductClicked(productId: Long): WooPosCartItemViewState { | ||
| val product = getProductById(productId)!! | ||
| val product = when { | ||
| wooPosLocalCatalogM1Enabled() -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it'll be better to have this logic in WooPosGetProductById as this reusable component that used in many places. wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, good idea. Done in b1e60e4
| private suspend fun handleVariationClicked(productId: Long, variationId: Long): WooPosCartItemViewState { | ||
| val product = getProductById(productId)!! | ||
| val product = when { | ||
| wooPosLocalCatalogM1Enabled() -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even in this VM you have the same code twice, so imo should be in getProductById
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored in b1e60e4
| else -> getProductById(itemData.productId)!! | ||
| } | ||
| val variationResult = when { | ||
| wooPosLocalCatalogM1Enabled() -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here for getVariationById
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in b1e60e4
| private val resourceProvider: ResourceProvider, | ||
| ) : ViewModel() { | ||
|
|
||
| private val currentDataSource: WooPosProductsDataSourceInterface = when (wooPosLocalCatalogM1Enabled()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have you considered moving this to the place where the instance is created, e.g. in dagger module? So the right implementation will be injected everywhere where needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Moved this to the module: 57b2078
kidinov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I left a couple of comments, please take a look!
WOOMOB-1072
Description
The goal of this PR is to load products from the DB instead of the existing in-memory cache / remote call hybrid store. Currently, the logic that decides on which data source to use is based on the feature flag WOO_POS_LOCAL_CATALOG_M1 state.
Steps to reproduce
—
Testing information
The tests that have been performed
Above
Images/gif
N/A
RELEASE-NOTES.txtif necessary. Use the "[Internal]" label for non-user-facing changes.