-
Notifications
You must be signed in to change notification settings - Fork 111
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
[HACK week] Adds favorite products filter #14001
Conversation
📲 You can test the changes from this Pull Request in WooCommerce iOS by scanning the QR code below to install the corresponding build.
|
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.
Thank you, Sharma, for opening this PR.
While testing it, I found two issues:
- If I filter by favourite products and then remove one of these products from being a favourite, it will not be removed from the list until I refresh it.
- This is less obvious. If I filter by favourite products and then add a product to favourites from another point (e.g., order detail or dashboard), the product list does not refresh with the new products until I do it manually using pull to refresh. I noticed that for status changes of a product (e.g., from published to draft), they work as expected.
My suggestion would be to add an observer on Core Data so that it can observe changes in the favourites, but I believe .
RELEASE-NOTES.txt
Outdated
@@ -1,6 +1,10 @@ | |||
*** PLEASE FOLLOW THIS FORMAT: [<priority indicator, more stars = higher priority>] <description> [<PR URL>] | |||
*** Use [*****] to indicate smoke tests of all critical flows should be run on the final IPA before release (e.g. major library or OS update). | |||
|
|||
20.6 | |||
----- | |||
- [internal] Ability to mark and filter favorite products. [https://github.com/woocommerce/woocommerce-ios/pull/14001] |
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 worth mentioning that this has not been released yet, but it affects product filtering logic.
@@ -34,11 +35,14 @@ private extension FavoriteProductsFilter { | |||
struct DefaultFavoriteProductsUseCase: FavoriteProductsUseCase { |
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.
Do you believe it's worth writing some unit tests for FavouriteProductsUseCase
or the one in ProductSelectorViewModelTests
should be enough?
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.
Great catch. I added tests in a previous PR. It looks like I removed that file during a merge conflict. Added it back in 5563db9
Thanks for the review and testing, Paolo!
We can't listen to core data to solve this issue because the favorite product IDs are stored in I will check other options to address the issues that you reported. |
Thanks for the review, @pmusolino! The PR is ready for another look. 🙇
Kindly let me know your thoughts. |
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, Sharma, for fixing them. Now everything works as expected 🎉
Part of: #12274
Description
Adds new option to filter favorite products in product lists screen and product selector screen.
I plan to enable the feature flag after a few weeks of testing.
Changes
Steps to reproduce
Prerequisites
Products list
Product selector
Testing information
Screenshots
Gif
iPhone
iPad
Video recording
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: