-
Notifications
You must be signed in to change notification settings - Fork 120
[Local Catalog] Update barcode scanning to use local catalog #16263
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
[Local Catalog] Update barcode scanning to use local catalog #16263
Conversation
Introduces `POSCatalogModeProvider` to centralize all eligibility checks for determining whether local GRDB catalog should be used. The provider consolidates and evaluates: - Feature flag (.pointOfSaleLocalCatalogi1) - Country support (US, GB only) - Currency support (USD for US, GBP for GB) - Catalog size limits (≤1000 items) Eligibility is checked once per site and cached, since these criteria don't change during a session. This provides a single source of truth that can be used for catalog sync decisions, barcode scanning, and other local catalog features.
Adds query methods to PersistedProduct and PersistedProductVariation for searching by barcode (global unique ID or SKU). Product queries: - posProductByGlobalUniqueID: Search by global unique ID - posProductBySKU: Search by SKU Variation queries: - posVariationByGlobalUniqueID: Search by global unique ID - posVariationBySKU: Search by SKU All queries apply the same POS filters as the existing posProductsRequest and posVariationsRequest methods (non-downloadable, supported product types). This ensures barcode scanning respects the same product visibility rules as the POS catalog display. The service layer will use these methods sequentially: first searching by global unique ID, then falling back to SKU if no match is found.
Introduces `PointOfSaleLocalBarcodeScanService` that implements barcode scanning using the local GRDB catalog instead of remote API calls. Also adds a `parentProduct` relationship to `PersistedProductVariation` to fetch variations with their parent products in a single query. Key features: - Implements `PointOfSaleBarcodeScanServiceProtocol` for drop-in compatibility - Sequential search: globalUniqueID first, then SKU fallback (for both products and variations) - Uses GRDB relationships to fetch variations with parent products efficiently - Reuses existing `PointOfSaleItemMapper` for POSItem conversion - Returns same error types as remote service for consistent UX - Validates products are POS-compatible (non-downloadable, supported types) The search strategy: 1. Search products by globalUniqueID 2. Search variations by globalUniqueID (with parent product) 3. Search products by SKU 4. Search variations by SKU (with parent product) 5. Throw notFound error if no matches This allows barcode scanning to work entirely offline when local catalog is synced, matching the same filters as the POS product list.
Updates `POSTabCoordinator` to use local barcode scanning service when local catalog infrastructure is available, with automatic fallback to remote service. Changes: - Replace lazy `barcodeScanService` property with `createBarcodeScanService` method that chooses the appropriate service based on infrastructure availability - Checks for both `grdbManager` AND `catalogSyncCoordinator` to determine if local catalog is properly initialized - When both are available, creates `PointOfSaleLocalBarcodeScanService` for offline barcode scanning using local GRDB catalog - Otherwise, creates `PointOfSaleBarcodeScanService` for remote API-based barcode scanning - Service selection happens at POS view presentation time, ensuring the correct service is used based on current feature flag and infrastructure state This completes the local catalog barcode scanning feature. When the feature flag is enabled and infrastructure is available, barcode scanning works entirely offline using GRDB. When disabled or not available, it falls back to the existing remote API behavior seamlessly.
The POSCatalogModeProvider was created but not used in the final implementation. Barcode service selection is done by checking for grdbManager and catalogSyncCoordinator availability directly. This can be properly refactored in a future issue to consolidate eligibility checking across POS features.
Adds test coverage for local catalog barcode scanning functionality: 1. PointOfSaleLocalBarcodeScanServiceTests: - Tests finding products by global unique ID and SKU - Tests finding variations with parent products - Tests search priority (global unique ID before SKU) - Tests error cases (not found, downloadable, unsupported types) - Uses actual GRDBManager for integration testing 2. PersistedProductBarcodeQueryTests: - Tests posProductByGlobalUniqueID query method - Tests posProductBySKU query method - Verifies POS filters (non-downloadable, supported types) - Tests site isolation 3. PersistedProductVariationBarcodeQueryTests: - Tests posVariationByGlobalUniqueID query method - Tests posVariationBySKU query method - Verifies variation filters (non-downloadable) - Tests parent product relationship fetching - Tests site isolation
Simplified barcode scanning to only use global unique IDs. The previous implementation also searched by SKU, but this was incorrect as barcode scanning should only match barcodes (global unique IDs), not SKUs. SKU matching is still available through search. Changes: - Removed posProductBySKU and posVariationBySKU query methods - Simplified PointOfSaleLocalBarcodeScanService to only search by global unique ID - Removed all SKU-related tests and updated site isolation tests
Variable parent products cannot be added to the cart directly - only their variations can be added. Updated the local barcode scan service to throw an unsupportedProductType error when a variable parent product barcode is scanned, matching the behavior of the remote barcode scan service. This ensures an error row is displayed in the cart UI when scanning a variable parent product's barcode.
|
|
staskus
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.
Works well 👍 Great to see UX benefits coming alive due to this project.
Only left minor comments. Also, I assume PersistedProductBarcodeQueryTests needs to be edited since the assertions are wrong, we do expect barcode queries not to filter out specific product types.
Modules/Sources/Yosemite/PointOfSale/Items/PointOfSaleLocalBarcodeScanService.swift
Outdated
Show resolved
Hide resolved
Modules/Sources/Yosemite/PointOfSale/Items/PointOfSaleLocalBarcodeScanService.swift
Outdated
Show resolved
Hide resolved
| } | ||
| // Fetch parent product using the relationship | ||
| guard let parentProduct = try variation.request(for: PersistedProductVariation.parentProduct).fetchOne(db) else { | ||
| return nil |
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 a minor detail, but in the remote implementation, we throw PointOfSaleBarcodeScanError.noParentProductForVariation in case the parent product is not found
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 catch 😊
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.
As far as I can tell, it's not actually possible due to integrity constraints. If the parent is deleted, or not added in the first place, the variation would be deleted as well. I couldn't even make a test (involving the db) that the error is thrown... but I've added one that documents it shouldn't happen. b5f0a1f
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.
Okay, got it! Thanks for checking.
| } | ||
|
|
||
| @Test("posProductByGlobalUniqueID filters out downloadable products") | ||
| func test_global_unique_id_query_filters_downloadable() async throws { |
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 assume this needs to be removed, since we don't filter out downloadable from the query. We don't want to filter them out to show an appropriate error.
| } | ||
|
|
||
| @Test("posProductByGlobalUniqueID filters out unsupported product types") | ||
| func test_global_unique_id_query_filters_unsupported_types() async throws { |
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 as with downloadable.
Co-authored-by: Povilas Staskus <[email protected]>
Tests that variations are automatically deleted when their parent product is deleted due to foreign key constraints with CASCADE delete. This documents that the noParentProductForVariation error thrown in searchVariationByGlobalUniqueID is defensive code that won't be triggered in normal operation, since the database prevents orphaned variations from existing.

Description
This PR updates barcode scanning to use the local catalog when it's available.
Apologies for the length – most of this is tests!
Testing information
I've tested the following:
Unknown scanned itemerror rowUnsupported item typeerror rowUnsupported item typeerror row and tracks the specific downloadable errorScreenshots
local.catalog.barcode.scanning.mp4
RELEASE-NOTES.txtif necessary.