-
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
Add target audiences and shipping times in the settings/general endpoint. #2342
Add target audiences and shipping times in the settings/general endpoint. #2342
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/google-api-project #2342 +/- ##
==============================================================
+ Coverage 62.5% 64.1% +1.5%
+ Complexity 4466 4450 -16
==============================================================
Files 764 472 -292
Lines 22581 18775 -3806
Branches 543 0 -543
==============================================================
- Hits 14121 12031 -2090
+ Misses 8003 6744 -1259
+ Partials 457 0 -457
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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.
LGTM ✅ Thanks @jorgemd24
I left a comment regarding the since tag, but approved in advance.
* Whether the data should be filtered. | ||
* Register the callbacks. | ||
*/ | ||
protected function register_callbacks() { |
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.
Missed @since
param
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.
Actually since all the class is new maybe we can add it at the top of the class
Changes proposed in this Pull Request:
Part of #2146
As part of the new mechanism for syncing merchant data with Google Merchant Center, we need to make some GLA Options accessible, such as the target audience and shipping times.
This PR adds the target audience and shipping time to the response of
wp-json/wc/v3/settings/general
Initially, my idea was to use the filter
woocommerce_settings- . $group_id
to add a new group of settings. However, this filter can only be used for Options (which should be fine for the target audience but not for the shipping times). Additionally, the filterwoocommerce_settings-
doesn't provide the Request Object param so I decided to use the filterrest_request_after_callbacks
which makes it easier to verify if the request is made from the proxied endpoints.https://github.com/woocommerce/woocommerce/blob/af03815134385c72feb7a70abc597eca57442820/plugins/woocommerce/includes/rest-api/Controllers/Version3/class-wc-rest-setting-options-controller.php#L79
Screenshots:
Detailed test instructions:
GET /wp-json/wc/v3/settings/general?gla_syncable=1
. If using the WPCOM proxy, usewpcom/v2/sites/YOUR_BLOG_ID/wc/v3/settings/general
.gla_target_audience
andgla_shipping_times
.GET /wp-json/wc/v3/settings/general
, it will not add those two fields. Note: If you make the request from WPCOM, it will always add the query parameter.Additional details:
Changelog entry