[Local Catalog] Add GRDB data source for item list #16231
Conversation
joshheald
added
type: task
|
|
Moves mock from YosemiteTests to PointOfSaleTests/Mocks where it can be used by PointOfSale module tests. Changes: - Moved file to Modules/Tests/PointOfSaleTests/Mocks/ - Made properties mutable for test control - Removed public access (internal to test module) - Simplified methods (tests set properties directly) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
joshheald
force-pushed
the
issue/woomob-1088-use-GRDB-in-itemlist
branch
from
4e79a59 to
a84aebc
Compare
Follow the pattern from PointOfSaleItemService by accepting CurrencySettings and creating the PointOfSaleItemMapper internally. This avoids exposing PointOfSaleItemMapper publicly from Yosemite. Changes: - GRDBObservableDataSource now takes currencySettings parameter - Creates PointOfSaleItemMapper internally with optional override - Updated tests to use new initializer
|
@staskus I'll have the other PR in the stack up this morning, which will let you see it working in the views. You may want to delay testing until then. I know you don't have all the context of this yet, so happy to have a call or async chat to go over anything |
Replace sleep-based polling with proper Swift Observation tracking. The waitForCondition helper now: - Uses withObservationTracking to detect changes to observable properties - Recursively re-observes until the condition is met - Has a 2-second timeout as a backstop to prevent hanging - Checks both loading completion AND expected item count - No primary polling - purely event-driven with timeout safety This fixes intermittent failures in: - test_load_products_maps_to_pos_items_correctly - test_load_products_sets_loading_state_and_fetches_items
Created reusable request methods on PersistedProduct and PersistedProductVariation: - posProductsRequest: Filters to simple/variable, non-downloadable products, sorts by name - posVariationsRequest: Filters to non-downloadable variations, sorts by ID This fixes the pagination issue where unsupported/downloadable product types caused fewer items than pageSize to be returned, stopping pagination early. Before: 20 fetched → 18 mapped → pagination stopped (18 < 20) After: 20 fetched → 20 mapped → pagination continues correctly Changes: - Add ProductType enum to PersistedProduct for type-safe filtering - Filter out unsupported product types at DB level (not in mapper) - Filter out downloadable products/variations - Products sorted alphabetically by name (case-insensitive, localized) - Variations sorted consistently by ID - Use weak self capture in Task blocks to avoid retain cycles - Centralized query logic for maintainability Benefits: - All supported products now load correctly with proper pagination - Cleaner separation of concerns (filtering in DB, not mapper) - More efficient (don't fetch items we'll filter out anyway)
Ensure the observe() function and condition check always run on MainActor to prevent data races when accessing observable properties. This fixes intermittent 'Index out of range' crashes on line 72 where the continuation would resume before the productItems array was actually populated on the main actor.
Use .receive(on: DispatchQueue.main) to ensure ValueObservation results
are delivered on the main queue, and update properties synchronously
rather than dispatching via Task { @mainactor }.
This simplifies the threading model and ensures property updates happen
synchronously when observations fire, which should improve reliability
and reduce potential race conditions.
Applied to all three observations: products, variations, and statistics.
Generated by 🚫 Danger |
| .tracking { [weak self] database -> [POSProduct] in | ||
| guard let self else { return [] } | ||
|
|
||
| let persistedProducts = try PersistedProduct |
There was a problem hiding this comment.
It's a minor detail, but I wonder if this observation and the statistics observation should happen in the same observable to avoid any data inconsistencies. I imagine both updates should happen at the same time, but theoretically, there can be a gap between setting productItems and totalProductCount, making hasMoreProducts value wrong for a brief moment. 🤔
| .fetchAll(database) | ||
|
|
||
| return try persistedProducts.map { persistedProduct in | ||
| let images = try persistedProduct.request(for: PersistedProduct.images).fetchAll(database) |
There was a problem hiding this comment.
For each product, we request images and attributes. Is this an effective way?
Could we flatten it all by:
- Getting all products
- Getting all attributes (for product ids) and make dictionary
- Getting all images (for product ids) and make dictionary
Map into POSProduct with attributes and images.
Or I see there also .including method which maybe could be used.
Same for variations
| .fetchCount(database) | ||
|
|
||
| let variationCount = try PersistedProductVariation | ||
| .filter(PersistedProductVariation.Columns.siteID == siteID) |
There was a problem hiding this comment.
Should it be using posVariationsRequest or least .filter(Columns.downloadable == false) so we would get the same count in case there are downloadable variations?
I think it also affects hasMoreVariations since it checks totalVariationCount which is not filtered per-product and compares it to variationItems is is loaded per-product.
| }, | ||
| receiveValue: { [weak self] observedProducts in | ||
| guard let self else { return } | ||
| let posItems = itemMapper.mapProductsToPOSItems(products: observedProducts) |
There was a problem hiding this comment.
You mentioned the performance with a lot of items which may be connected to mapping, could we do the mapping outside the main thread and only receive on main thread?
.publisher(in: grdbManager.databaseConnection)
.map { [itemMapper] observedProducts in
// Mapping happens on background Combine scheduler
itemMapper.mapProductsToPOSItems(products: observedProducts)
}
.receive(on: DispatchQueue.main)We could also leave such performance considerations for the future. I'm sure there are even more things that we could do if we reach use cases with extremely large catalogs.
4 participants
Part of: WOOMOB-1088
Description
This PR adds a new datasource, analagous to
PointOfSaleItemService, but with observation of the database so that any changes result in updates to the observable properties thatGRDBObservableDataSourceprovides.Steps to reproduce
This isn't used in the app yet. Unit tests are all you have! This PR is the base of a stack, and the top PR should allow you to use it in the item list, so functional testing can wait until then.
RELEASE-NOTES.txtif necessary.