Product Query: pass any product taxonomies existing in the URL parameters. #6152
Conversation
|
Size Change: 0 B Total Size: 868 kB ℹ️ View Unchanged
|
cpapazoglou
requested review from
a team,
tarhi-saad and
nielslange
and removed request for
a team,
tarhi-saad and
nielslange
nielslange
requested review from
a team and
dinhtungdu
and removed request for
a team and
dinhtungdu
There was a problem hiding this comment.
Thanks for working on this @cpapazoglou! I definitely agree that's a good addition. I will leave the final review for @opr, I see you already discussed this implementation with him.
I left a couple of questions regarding the docs part below. I also wonder if we could add some tests (maybe here)?
src/StoreApi/docs/products.md
Outdated
| | `sku` | string | no | Limit result set to products with specific SKU(s). Use commas to separate. | | ||
| | `featured` | boolean | no | Limit result set to featured products. | | ||
| | `category` | string | no | Limit result set to products assigned a specific category ID. | | ||
| | `product-taxonomy` | string | no | Limit result set to products assigned to the term ID of that custom product taxonomy. `product-taxonomy` should be the key of the custom product taxonomy registered. | |
There was a problem hiding this comment.
Nitpicking, but I feel it's odd to list product-taxonomy between category and category_operator.
Can we move product-taxonomy above category or, even better, after category_operator?
And two more questions:
- Should we also document
product-category_operator? - And should we make it more explicit that
product-categoryis a placeholder? Maybe enclosing it in square brackets or something along these lines?
| // Map between taxonomy name and arg key. | ||
| $taxonomies = [ | ||
| $default_taxonomies = [ |
There was a problem hiding this comment.
Don't these $default_taxonomies also get returned by get_taxonomies()? If so, is there any need for $default_taxonomies and $taxonomies = array_merge( $default_taxonomies, $all_product_taxonomies );
There was a problem hiding this comment.
Good question @tjcafferkey. get_taxonomies( array( 'object_type' => array( 'product' ) ), 'names' ); returns all product taxonomies including product_cat and product_tag. The thing is that the endpoint currently uses category instead of product_cat and tag instead of product_tag for parameters. So I have opted merging with $default_taxonomies for backwards compatibility. Would be preferable updating the docs and all code references (if any)?
There was a problem hiding this comment.
The thing is that the endpoint currently uses category instead of product_cat and tag instead of product_tag for parameters.
@cpapazoglou it looks like after the merge it uses product_cat and product_tag unless I am wrong?
There was a problem hiding this comment.
🤦, sorry for this and good catch. I was merging with wrong order. Fixed here 6885fd2
Works in my local machine 😁
There was a problem hiding this comment.
@tjcafferkey is the following preferable?
$all_product_taxonomies = get_taxonomies( array( 'object_type' => array( 'product' ) ), 'names' );
$all_product_taxonomies['product_cat'] = 'category';
$all_product_taxonomies['product_tag'] = 'tag';
There was a problem hiding this comment.
Thanks @cpapazoglou this seems to work as expected now. However like @Aljullu mentioned I will leave the final review for @opr
There was a problem hiding this comment.
is the following preferable?
I think as it currently is works fine.
opr
added
status: needs review
focus: rest api
There was a problem hiding this comment.
Hey thanks @Aljullu and @tjcafferkey for reviewing this for me while I was away.
@cpapazoglou it looks great, just wondering about the operator, the AND operator doesn't seem to work for me, could you check it for me? (in and not_in seem to be fine)
- Set two products up assign two custom categories to the first one, to the second one give only one custom category.
- Go to the following URL and expect to only see the product with two categories.
/wp-json/wc/store/products?custom_categories=35,36&custom_categories_operator=and
There was a problem hiding this comment.
Thank you for working on this Harris! I feel this is a much safer option for now while we look into better options into how we filter things. I'd like to request some changes first if that's possible:
- This needs a friendlier changelog, possibly: Allow Store API to filter products by custom taxonomies, unless I'm getting this PR wrong.
- This needs a dev note as well, given it's a change to a developer related page.
- It doesn't seem like we have any protection against core params, what if someone names a taxonomy
featuredorsku, would that conflict? can we make sure all of our core args are prioritized above custom taxonomies.
👋 @opr, I agree the behaviour is very weird and decided to check what happens with the BUT, I notice another bug with custom taxonomies and multiple term ids passed in the arguments. Passing for example 👋 @senadir
Yep, very good suggestion ✅
I am not familiar with dev notes, can you point me to relevant documentation or code?
Very good catch! Yes it would conflict. I am thinking of prefixing the query parameter with |
Thanks @opr for reviewing / testing this!
I hadn't tested
|
|
Added a release note and updated the changelog. |
|
I'm not sure if a |
Good idea @senadir, I replaced Here are the |
cpapazoglou
force-pushed
the
add/all_product_taxonomies_to_product_query
branch
from
ba4b819 to
96ffaef
Compare
cpapazoglou
force-pushed
the
add/all_product_taxonomies_to_product_query
branch
from
96ffaef to
8843a78
Compare
There was a problem hiding this comment.
Thank you for all the effort and back and forth with us @cpapazoglou
Let's merge this!
github-actions
bot
added
status: ready to merge
and removed
status: needs review
labels
While working on 12894-gh-Automattic/woocommerce.com I have noticed that there isn't a way to filter products based on custom product taxonomies. The code only allows a static list of
product_catandproduct_tag. This PR aims at using any product taxonomy present in the URL parameters to build the$query_argsfor theWP_Query.Related convo: p1648465508181829-slack-C8X6Q7XQU
Automated Tests
Manual Testing
How to test the changes in this Pull Request:
custom_categoriestaxonomy/wp-json/wc/store/products?_unstable_tax_custom_categories=[that-category-id]and make sure you get only products where this term was added/wp-json/wc/store/productsand make sure that all products get returned as expectedUser Facing Testing
These are steps for user testing (where "user" is someone interacting with this change that is not editing any code).
Changelog
Enhancements
Bug Fixes
Release Note
Store API
As part of enhancing the Store API, we added a way to filter products by custom defined product taxonomies. The are two new parameters that can be passed to the API and should follow the dynamic patters
_unstable_tax_[product-taxonomy]and_unstable_tax_[product-taxonomy]_operator. They work the same way ascategoryandcategory_operatorbut in this case you should replace[product-taxonomy]with any custom defined product taxonomy. eg?_unstable_tax_hot_categories=1,2,3&_unstable_tax_hot_categories_operator=inwill return all products appearing in
hot_categoriestaxonomy for terms 1 or 2 or 3.