Add suggested-products product endpoint #43720
Conversation
retrofox
added
focus: new product ux
github-actions
bot
added
the
plugin: woocommerce
|
Hi , @woocommerce/mothra Apart from reviewing the code changes, please make sure to review the testing instructions as well. You can follow this guide to find out what good testing instructions should look like: |
Test Results SummaryCommit SHA: 7286f43
To view the full API test report, click here. To view the full E2E test report, click here. To view all test reports, visit the WooCommerce Test Reports Dashboard. |
...includes/rest-api/Controllers/Version3/class-wc-rest-product-related-products-controller.php
Outdated
Show resolved
Hide resolved
retrofox
changed the title
retrofox
force-pushed
the
update/add-product-related-products-endpoint
branch
from
69b70a6
to
2a0b44f
Compare
...includes/rest-api/Controllers/Version3/class-wc-rest-product-related-products-controller.php
Outdated
Show resolved
Hide resolved
| ), | ||
| ), | ||
| array( | ||
| 'methods' => WP_REST_Server::READABLE, |
There was a problem hiding this comment.
If we are supporting the passing in of the attributes/categories/tags and such should we turn this into a POST instead? So that data can be passed in as data versus a query?
There was a problem hiding this comment.
I am not opposed to leaving it as a GET request for now.
There was a problem hiding this comment.
I believe that using GET method is appropriate in this case as we do not intend to make any changes to the data.
There was a problem hiding this comment.
Yeah, I suppose that is fine, it just seems like it may get messy when we also support the passing in of attributes, as attributes contain a key and value.
There was a problem hiding this comment.
It's a good point. It doesn't seem pretty easy to pass it in the query. right?
...includes/rest-api/Controllers/Version3/class-wc-rest-product-related-products-controller.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Nice work @retrofox, this is heading in the right direction I think, and your right it is pretty clean :)
I did leave several inline suggestions and noticed two bugs, one around the combining of the tags/categories and another around a caching issue with transients ( I left inline comments around the relevant code for each of those ).
| 'methods' => WP_REST_Server::READABLE, | ||
| 'callback' => array( $this, 'get_related_items' ), | ||
| 'permission_callback' => array( $this, 'get_items_permissions_check' ), | ||
| 'args' => $this->get_collection_params(), |
There was a problem hiding this comment.
I think we should probably have a unique function for this, I left another comment below.
There was a problem hiding this comment.
The related-products endpoint supports almost all of them. Maybe we should rake over the include one.
I do imagine a scenario where we want to pick all related products except a few:
products/100/related-products?related_categories=10?exclude=1,2,3
There was a problem hiding this comment.
now I got it.
Let me update the PR.
| * overwriting the post__in parameter. | ||
| */ | ||
| if ( ! empty( $this->related_producs_ids ) ) { | ||
| $args['post__in'] = $this->related_producs_ids; |
There was a problem hiding this comment.
Aaah I was curious how the parent::get_items( $request ); was still working, but I guess that is because of this.
Do you think it would make more sense if we just call wc_get_product here with array_map, I saw something similar in one of the template functions ( don't have to copy this, just an example of array_map:
$args['related_products'] = array_filter( array_map( 'wc_get_product', wc_get_related_products( $product->get_id(), $args['posts_per_page'], $product->get_upsell_ids() ) ), 'wc_products_array_filter_visible' );
// Handle orderby.
$args['related_products'] = wc_products_array_orderby( $args['related_products'], $args['orderby'], $args['order'] );
There was a problem hiding this comment.
The (good, I think) thing about using the parent get_items() is that it will inherit the whole behavior.
For instance, could I use the _fields, per_page, and other params with this approach? I think we could, but it will require addressing them.
There was a problem hiding this comment.
Do you see any issue by using the get_items() method? In the end, what we need to return is a products collection
...ins/woocommerce/includes/rest-api/Controllers/Version3/class-wc-rest-products-controller.php
Show resolved
Hide resolved
...ins/woocommerce/includes/rest-api/Controllers/Version3/class-wc-rest-products-controller.php
Outdated
Show resolved
Hide resolved
...ins/woocommerce/includes/rest-api/Controllers/Version3/class-wc-rest-products-controller.php
Outdated
Show resolved
Hide resolved
retrofox
changed the title
...ins/woocommerce/includes/rest-api/Controllers/Version3/class-wc-rest-products-controller.php
Outdated
Show resolved
Hide resolved
| */ | ||
| if ( ! empty( $this->suggested_products_ids ) ) { | ||
| $args['post__in'] = $this->suggested_products_ids; | ||
| } |
There was a problem hiding this comment.
Sorry for not noticing this earlier, but this can also be removed right?
There was a problem hiding this comment.
Don't sorry at all. It's my mistake. Thanks for reviewing. Addressed.
Actually, the endpoint keeps relying on the parent get_items() method. If we take this path, these changes are needed.
retrofox
force-pushed
the
update/add-product-related-products-endpoint
branch
from
97f84c0
to
7286f43
Compare
github-actions
bot
added
the
needs: analysis
alopezari
added
needs: internal testing
* init approach of related products endpoint * register related prodducts endpoint * rename response prop with ids * filter categoris from endpoint params * filter categories when pulling related products * changelog * extend from WC_REST_CRUD_Controller class * iterate over defininf endpoint params * doc * merge product terms with params * introduce `combine` param * return products collection instead of plain array * fixinf eslint issues * pass proper fn args to bound fns * do not call parent register method * post type is defined by parent controller class * remove related products controller * extend products controller with `related-products`endpoint * filter post__in param * when no related products found return empty array * change and register endpoint params * remove commented line * add a custom method to extend the params * update * fix typo * ensure to respect the products limit * fix eslint issue * remove unrequired endpoint id param
Submission Review Guidelines:
Changes proposed in this Pull Request:
This PR introduces a new Suggested Products endpoint:
Params
The endpoint accepts three parameters:
categories,tags, andlimit.Also, it accepts the
combineparam to combine or not the current product items (related_categories, related_tags, ...) with the provided ones.Closes #43714
How to test the changes in this Pull Request:
Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:
A simple way to check this endpoint if by using the apiFetch library which is exposed globally in the new Product Editor app:
Now, you can start to perform requests by using the apiFetch lib:
Get default suggested products
21, and limit3Changelog entry
Significance
Type
Message
Comment