-
Notifications
You must be signed in to change notification settings - Fork 116
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
[Woo POS][Products Search] Add POSProduct in Networking #15441
Conversation
|
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 Tested both WPCOM login and application password login, products were loaded as before including variable products!
/// ListMapper: Maps generic WooCommerce REST API Lists | ||
/// | ||
struct ListMapper<Output: Decodable>: Mapper { |
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.
👏 no more copying mapper for new use case! We can create an issue to replace existing mappers and let the mobile team know.
@@ -102,7 +102,7 @@ public final class PointOfSaleItemService: PointOfSaleItemServiceProtocol { | |||
// Maps result to POSItem, and populate the output with: | |||
// - Formatted price based on store's currency settings. | |||
// - Product thumbnail, if any. | |||
private func mapProductsToPOSItems(products: [Product]) -> [POSItem] { | |||
private func mapProductsToPOSItems(products: [POSProduct]) -> [POSItem] { |
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 that this is the only change in POS 👍
@@ -202,11 +202,11 @@ public final class ProductsRemote: Remote, ProductsRemoteProtocol { | |||
/// - Parameters: | |||
/// - siteID: Site for which we'll fetch remote products. | |||
/// - productTypes: A list of product types to be included in the results. | |||
/// - pageNumber: Number of page that should be retrieved. | |||
/// - pageNumber: Number of pages that should be retrieved. |
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.
nit: I think this parameter is the page index (starting with 1), not the number of pages (total page count)?
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.
Yep, you're absolutely right! I was obviously not thinking straight, I'll fix it.
Closes: #15439
Description
Adds a
POSProduct
model and updates theProductsRemote.loadProductsForPointOfSale
function to use it.Adds a generic
ListMapper
which can be used for mapping any standard list from the REST API – however it's only used forPOSProduct
at this point.We decided to make a new copy of Product for Point of Sale to support the future persistence of the product catalogue: pdfdoF-6Ha-p2#comment-7880, pdfdoF-6Pb-p2. This version of the Product will only contain what we need for Point of Sale.
Steps to reproduce
Testing information
I've tested with both site credentials and Jetpack – the mapping behaviour is slightly different between the two as only the Jetpack-tunnelled version of the API response is wrapped in a
data
envelope.I checked that we could load multiple pages, reload, create orders, edit orders, and use variable products the same as previously.
I've also checked that the existing Orders and Products features work correctly.
I've tested on an iPad Air running iPadOS 17.7.3, and a simulator running iPadOS 18.2.
Screenshots
POSProduct.in.use.mp4
RELEASE-NOTES.txt
if necessary.Reviewer (or Author, in the case of optional code reviews):
Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement: