-
Notifications
You must be signed in to change notification settings - Fork 21
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
Free Listings + Paid Ads: Update product feed API to return the bestselling product #1681
Free Listings + Paid Ads: Update product feed API to return the bestselling product #1681
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.
Thanks for the changes, I'm getting the response as expected.
{
"products": [
{
"id": 1695,
"title": "Incredible Steel Coat",
"visible": true,
"status": "not_synced",
"errors": []
}
],
"total": 316,
"page": 1
}
Just double checking if the product status makes a difference? In my case it picks a product that's not synced (excluded).
I just left a few smaller comments but they aren't blocking so I'll go ahead and approve the PR.
src/DB/ProductFeedQueryHelper.php
Outdated
case 'total_sales': | ||
$args['meta_key'] = ProductMetaHandler::KEY_TOTAL_SALES; | ||
$args['orderby'] = [ 'meta_value_num' => $this->get_order() ] + $args['orderby']; | ||
break; | ||
default: | ||
throw InvalidValue::not_in_allowed_list( 'orderby', [ 'title', 'id', 'visible', 'status' ] ); |
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.
We should add total_sales
as that's an allowed value.
src/DB/ProductFeedQueryHelper.php
Outdated
@@ -165,6 +167,10 @@ protected function prepare_query_args(): array { | |||
$args['meta_key'] = $this->prefix_meta_key( ProductMetaHandler::KEY_MC_STATUS ); | |||
$args['orderby'] = [ 'meta_value' => $this->get_order() ] + $args['orderby']; | |||
break; | |||
case 'total_sales': | |||
$args['meta_key'] = ProductMetaHandler::KEY_TOTAL_SALES; |
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.
I personally think we should just use a constant string of total_sales
here. I don't think it really fits within the ProductMetaHandler
class as that's to manage product meta fields we add for the extension, also it means we can call update/get/delete for this field, but it won't target the right entry because it will be adding the prefix. So it can be removed from the ProductMetaHandler
class.
I think for most new users when they reach that step of the onboarding flow their products haven't been synced to GMC, so filtering only for synced products might have some problems. |
Since ProductMetaHandler is to manage product meta fields we add for the extension.
f45ab12
to
7e3caaf
Compare
The original base branch this PR was trying to merge into was |
Changes proposed in this Pull Request:
This PR implements the backend task 4 📌 Return merchants key product data (e.g. bestselling) from the epic issue #1610. Here are what have changed:
mc/product-feed
by adding a new conditiontotal_sales
to the parameterorderby
.total_sales
is one of the meta keys of a product.GET mc/product-feed?per_page=1&orderby=total_sales&order=desc
mc/product-feed
to return price and image url of a product.siteLogoUrl
toglaData
for the frontend.Detailed test instructions:
mc/product-feed
API with parameters:GET mc/product-feed?per_page=1&orderby=total_sales&order=desc
price
andimage_url
.SELECT * FROM wp_postmeta WHERE meta_key = 'total_sales' ORDER BY CAST( meta_value AS unsigned ) DESC;
window.glaData
siteLogoUrl
.null
if you did not upload a custom site logo.siteLogoUrl
isnull
, go towp-admin/admin.php?page=wc-admin&task=appearance
and upload a logo.window.glaData
again, you should see the value ofsiteLogoUrl
.Changelog entry