-
Notifications
You must be signed in to change notification settings - Fork 115
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] Bump minimum required WC version to 9.6, replace local products filters with downloadable parameter, page size 25 #14843
Conversation
…rtual` and `downloadable` API support.
|
… to include virtual product in the API response.
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.
This works as described. However, I think based on pdfdoF-62l-p2#comment-7285 it could be also expanded or done in another PR:
- Allow no-price products
- Restrict POS entry from 9.6+
func isNotDownloadable(product: Product) -> Bool { | ||
!product.downloadable | ||
} | ||
|
||
func hasPrice(product: Product) -> Bool { |
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.
Based on pdfdoF-62l-p2#comment-7285, this one can be removed as well 👍
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.
Removed in b192a0c.
Potential latest decision: p1736504688523299/1736484154.214279-slack-C070SJRA8DP
Based on the proposed decision above, I will wait for WC 9.6 to be released this week before making all the changes:
|
WC 9.6 is still not publicly available as of today. As I'm AFK after this Friday, I'm trying to get the PR ready for merge first and it can be merged when 9.6 is in GA. @staskus it's not urgent, this PR is ready for review again with the changes in #14843 (comment). |
Unit tests are failing, please skip the review for now and I'll update when it's ready for review again. |
TTP education tests should hopefully succeed if you merge now with trunk. |
Ready for review now! A test case failing from the removal of the price filter was updated in 8916a21. |
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.
- POS only available from 9.6
- Downloadable filtered
- No price products shown
We'll need to implement additional handling for these no price or zero price products to finish the order.
@staskus thanks for the review; I think we're waiting to merge until we're sure 9.6 is released.
We discussed this last week – I think all that's needed is:
In future, we could optimise by adding a Context: p1736742009959369-slack-C070SJRA8DP |
👀 Let me know if I'm holding this one by having the flag disabled, it can be enabled right away as soon as we merge this PR, as the rest it's just minor UI details. |
To retain the ability to turn off the feature flag for variable products, we should only use the higher minimum WC version when the flag is enabled. If we disable the flag, downloadable products are still filtered out by local filters, and it’s no longer necessary to remove virtual products.
Closes: #1483
Description
This pull request updates products and variations and filtering criteria based on the latest decision pdfdoF-62l-p2#comment-7224. It removes the virtual local filter (as we want to show virtual products), replaces the downloadable local filter with a parameter in the API request that works for WC versions 9.6+, and updates the page size from 100 to 25 for products and variations requests.
Regarding the page size, I tested with the smallest font size on the tallest iOS device available - iPad Pro 13 in' so far in iOS 18.2. The minimum amount of items to fill the scroll view in portrait is 10. Even so, I picked 25 to match Android. Please feel free to share any thoughts on the page size.
🗒️ It's likely we will update the minimum WC version requirement to 9.6 for POS based on p1736485809856869/1736484154.214279-slack-C070SJRA8DP. Since 9.6 is still in beta 2 at the time of writing, we will postpone the version requirement bump until the version is publicly released.
Pagination and Product Parameters:
Networking/Networking/Remote/ProductVariationsRemote.swift
: Changed thevariationsPerPage
constant from 100 to 25.Networking/Networking/Remote/ProductsRemote.swift
: Added thedownloadable
parameter to the request and updated theproductsPerPage
constant from 100 to 25. [1] [2] [3]Test Updates:
Networking/NetworkingTests/Remote/ProductsRemoteTests.swift
: Added a new test to verify that thedownloadable
parameter is set correctly in theloadProductsForPointOfSale
method.Filtering Criteria:
Yosemite/Yosemite/PointOfSale/PointOfSaleItemService.swift
: Removed theisNotVirtual
andisNotDownloadable
filtering functions and criteria. [1] [2]Steps to reproduce
Testing information
Screenshots
Notice that the downloadable products are filtered out.
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: