-
Notifications
You must be signed in to change notification settings - Fork 120
[Local Catalog] Add remote for products/variations incremental sync API #16033
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
Conversation
|
|
joshheald
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.
Thanks Jaclyn, looks good 👍
| public func loadProducts(modifiedAfter: Date, siteID: Int64, productTypes: [ProductType] = [.simple, .variable], pageNumber: Int) | ||
| async throws -> PagedItems<POSProduct> { | ||
| let path = "products" | ||
| let dateFormatter = ISO8601DateFormatter() |
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.
| let dateFormatter = ISO8601DateFormatter() | |
| let dateFormatter = ISO8601DateFormatter() |
Perhaps best to have this in a property? I think DateFormatter is reasonably expensive to create, and this could be called quite a few times.
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.
Updated in b1f9850.
| /// - pageNumber: Page number for pagination. | ||
| /// - Returns: Paginated list of POS products. | ||
| /// | ||
| // periphery:ignore - TODO - remove when this endpoint is integrated with catalog sync |
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.
Is the function marked for removal? Why...? I thought we'd need this all the way through?
Or is this just telling periphery to ignore the function and we'll remove that? Why do we need to ignore it if so?
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.
Not the function, just the periphery ignore comment! The periphery:ignore comment is required now since it's not used in the app right now. I'll update the comment to avoid confusion.
Updatead in b0d5574.
| let dateFormatter = ISO8601DateFormatter() | ||
| let parameters = [ | ||
| ParameterKey.modifiedAfter: dateFormatter.string(from: modifiedAfter), | ||
| ParameterKey.productTypes: productTypes.map { $0.rawValue }.joined(separator: ","), |
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.
We'll need to keep this under review, based on what we do with full sync. There are arguments to pull everything down, and filter on the client side... especially if that's easier to generate in the file.
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, right now the full sync does not support filtering on product types. I will remove this product types parameter for now.
Updated in ae63cd3.
| func createPagedItems<T>(items: [T], | ||
| responseHeaders: [String: String]?, | ||
| currentPageNumber: Int) -> PagedItems<T> { | ||
| // Extract total pages from response headers (case insensitive) | ||
| let totalPages = responseHeaders?.first(where: { | ||
| $0.key.lowercased() == PaginationHeaderKey.totalPagesCount.lowercased() | ||
| }).flatMap { Int($0.value) } | ||
|
|
||
| let hasMorePages = totalPages.map { currentPageNumber < $0 } ?? true | ||
|
|
||
| // Extract total count from response headers (case insensitive) | ||
| let totalItems = responseHeaders?.first(where: { | ||
| $0.key.lowercased() == PaginationHeaderKey.totalCount.lowercased() | ||
| }).flatMap { Int($0.value) } | ||
|
|
||
| return PagedItems(items: items, hasMorePages: hasMorePages, totalItems: totalItems) | ||
| } |
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.
Nice 👍
…ncremental sync, as product types filtering is not supported in full sync at least for now.

For WOOMOB-1096
Description
Adds a new
POSCatalogSyncRemoteclass to support incremental synchronization of POS catalog data. This enables merchants to efficiently sync only products and product variations that have been modified after a specific date, significantly reducing bandwidth usage and improving sync performance for large catalogs.The implementation includes:
POSCatalogSyncRemotewith protocol-based design for easy mocking in the futureloadProductsmethod with product type filtering and default valuesloadProductVariationsmethod for fetching variation dataRemotebase classThis PR also includes a feature flag for Local Catalog i1.
Steps to reproduce
Just CI is sufficient as the remote isn't used in the app yet.
Testing information
I tested with
in the app to verify that the remote methods are working as expected.
RELEASE-NOTES.txtif necessary.