-
Notifications
You must be signed in to change notification settings - Fork 10.7k
API: Add endpoint for fetching location data #15162
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
Conversation
claudiosanches
left a comment
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.
Missing schema in all classes too.
| * @author WooThemes | ||
| * @category API | ||
| * @package WooCommerce/API | ||
| * @since 2.6.0 |
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.
Should be 3.1.0
| * @package WooCommerce/API | ||
| * @extends WC_REST_Controller | ||
| */ | ||
| class WC_REST_Data_Index_Controller extends WC_REST_Data_Controller { |
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.
Filename should match the class name.
The abstract class does not have anything special, so could remove the abstract class and use this class to be extended by WC_REST_Data_Location_Controller.
| * @author WooThemes | ||
| * @category API | ||
| * @package WooCommerce/API | ||
| * @since 2.6.0 |
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.
3.1.0
| 'type' => 'string', | ||
| ), | ||
| 'country' => array( | ||
| 'description' => __( 'Country name to restrict states list', 'woocommerce' ), |
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 think should change Continent name and Country name to 2 letter code or ISO 3166 Code.
| * @package WooCommerce/API | ||
| * @extends WC_REST_Controller | ||
| */ | ||
| class WC_REST_Data_Location_Controller extends WC_REST_Data_Controller { |
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.
Class name should be WC_REST_Data_Locations_Controller
mikejolley
left a comment
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.
@justinshreve This could use your eyes too.
| * @return WP_Error|WP_REST_Response | ||
| */ | ||
| public function get_items( $request ) { | ||
| $data = array(); |
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.
nitpick; alignment
| 'description' => __( 'List of supported continents, countries, and states; restricted to the given continent', 'woocommerce' ), | ||
| ), | ||
| array( | ||
| 'slug' => 'locations/<continent>/<country>', |
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.
Cc'ing @justinshreve here too for feedback. How important is continent when it comes to country codes? It feels kind of weird to require it just to get a list of states for a country - all countries are unique, ISO standard codes...
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.
Yeah I think we could probably drop it.
I know settings will mostly be using /data/locations that returns everything, but in terms of other use cases where we might need specific states for a country - it does seem easier to not require the continent.
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.
Is having just continent useful? We could just get rid of /data/locations/<continent> and only have /data/locations/<country>.
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.
Although existing use cases don't need it (?) I think it can be useful. It might be needed in the shipping zone implementation.
| ), | ||
| 'country' => array( | ||
| 'description' => __( 'Country name to restrict states list', 'woocommerce' ), | ||
| 'type' => 'string', |
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.
Correct me if I'm wrong, but I believe we can define valid values for these can we not?
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'm not sure what you mean - do you mean whitelisting countries?
|
|
||
| $data = array(); | ||
| foreach ( $continents as $continent_code => $continent_list ) { | ||
| if ( isset( $request['continent'] ) && ( strtolower( $continent_code ) !== strtolower( $request['continent'] ) ) ) { |
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.
Since codes are always upper case (?) wouldn't it be simpler to just sanitize and uppercase the user input?
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.
Since these codes are filterable, I don't think we can rely on them always being uppercase
| @@ -0,0 +1,134 @@ | |||
| <?php | |||
| /** | |||
| * Tests for Data API. | |||
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 think I've addressed the feedback here — renamed the classes and files, fixed my copy-paste issues 😣 , added schemas, and changed the endpoints so a continent code is not required to get a states list for one country. I'll update the main description to reflect this. |
emdashcodes
left a comment
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.
Looking good! I left a few notes. Most minor coding standards things, but I did note a few small improvements.
| /** | ||
| * REST API Data controller | ||
| * | ||
| * Handles requests to the /data/location endpoint. |
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.
Minor typo. Should be /data/locations.
| * | ||
| * Handles requests to the /data/location endpoint. | ||
| * | ||
| * @author WooThemes |
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.
Minor: Should be Automattic now.
(@mikejolley Looks like we still need to run a script on existing files at some point).
| } | ||
|
|
||
| /** | ||
| * Check whether a given request has permission to read site data |
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.
Minor: Doc comments should end w/ punctuation for consistency.
| } | ||
|
|
||
| /** | ||
| * Return the list of data resources |
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.
Minor: Doc comments should end w/ punctuation for consistency.
| @@ -0,0 +1,146 @@ | |||
| <?php | |||
| /** | |||
| * REST API Data controller | |||
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.
Punctuation.
|
|
||
| $context = ! empty( $request['context'] ) ? $request['context'] : 'view'; | ||
| $data = $this->add_additional_fields_to_object( $data, $request ); | ||
| $data = $this->filter_response_by_context( $data, $context ); |
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 think we can just pass in view here since there won't end up being an edit context, like we do in the system status controller. That would allow us to drop line 109.
| 'slug' => 'locations', | ||
| 'description' => __( 'List of supported continents, countries, and states', 'woocommerce' ), | ||
| ), | ||
| array( |
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'm not sure if this second should be returned here at the root. I think the locations resource and in the future currencies would be here, but not anything one level down.
| * @return WP_Error|WP_REST_Response | ||
| */ | ||
| public function get_items( $request ) { | ||
| $continents = WC()->countries->get_continents(); |
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.
Alignment needed for all 5 of these lines.
tests/unit-tests/api/data.php
Outdated
| $this->assertEquals( 200, $response->get_status() ); | ||
| $this->assertTrue( is_array( $locations ) ); | ||
| $this->assertGreaterThan( 1, count( $locations ) ); | ||
| $this->assertTrue( isset( $locations[0]['code'] ) ); |
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 can use assertNotEmpty( $locations[0]['code']) here instead, and on other similar checks.
tests/unit-tests/api/data.php
Outdated
| $response = $this->server->dispatch( new WP_REST_Request( 'GET', '/wc/v2/data' ) ); | ||
| $index = $response->get_data(); | ||
| $this->assertEquals( 200, $response->get_status() ); | ||
| $this->assertEquals( 2, count( $index ) ); |
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 could use assertCount here instead: $this->assertCount( 2, $index );
(I know other API tests are doing assertEquals too, but would be good to fix those in the future).
cb5e494 to
22a9c70
Compare
|
Some quick notes on responses: When requesting a specific country (say We would return something like When requesting something like (Same issue in the CA response above too). |
|
How would changing the response like that affect the schema? I can see removing the array wrapper for continent & country responses, because we know we're only requesting one item, and that wouldn't affect the schema as it is now. (Honestly I'm not sure I see the relationship of schema for this data, since we're not updating anything on this endpoint) |
Maybe better write first the schema, since it will make easy to code the data that should return. |
|
@justinshreve Good point from @ryelle ^ #15162 (comment) If the schema differs based on the input, how do you represent that in the response? To have 2 different schemas you'd need /country/ /continent/? Which I think brings this full circle 😱 Unless the schema was abstracted somewhat? Then it applies to both? Let's discuss before making further changes so we have a solution. |
|
Yeah good point about the schema @ryelle & @mikejolley. I like your suggestion Mike. I think returning |
|
So with the abstracted schema, would |
|
Yeah I think we can/should still do that. |
|
@justinshreve How is that represented in schema? Does that mean locations have to be objects too? What props? |
|
Wouldn't it just be similar to what it is now, following the schema updates? |
|
Then if you requested /US, the country part would drop off. |
|
Ah I get you. @ryelle can that be represented in the schema without issue? |
|
These latest changes updates the response object based on what filter is passed in (country, continent, or none). I've also tried to update the schema to reflect that the items at different levels may be countries, continents, states, or absent (it makes more sense if you look at the schema and some response objects). |
|
I could imagine this with some other endpoints, like: All results:
[
{
"code": "AF",
"name": "Africa",
"countries": [...],
"_links": {
"self": [
{
"href": "https://example.com/wp-json/wc/v2/data/locations/af"
}
],
"collection": [
{
"href": "https://example.com/wp-json/wc/v2/data/locations"
}
]
}
},
{
"code": "AN",
"name": "Antarctica",
"countries": [...],
"_links": {
"self": [
{
"href": "https://example.com/wp-json/wc/v2/data/locations/an"
}
],
"collection": [
{
"href": "https://example.com/wp-json/wc/v2/data/locations"
}
]
}
},
...
]Specific continent:
{
"code": "AF",
"name": "Africa",
"countries": [...],
"_links": {
"self": [
{
"href": "https://example.com/wp-json/wc/v2/data/locations/af"
}
],
"collection": [
{
"href": "https://example.com/wp-json/wc/v2/data/locations"
}
]
}
}Specific country:
{
"code": "ZA",
"name": "South Africa",
"states": [
{
"code": "EC",
"name": "Eastern Cape"
},
{
"code": "FS",
"name": "Free State"
},
{
"code": "GP",
"name": "Gauteng"
},
{
"code": "KZN",
"name": "KwaZulu-Natal"
},
{
"code": "LP",
"name": "Limpopo"
},
{
"code": "MP",
"name": "Mpumalanga"
},
{
"code": "NC",
"name": "Northern Cape"
},
{
"code": "NW",
"name": "North West"
},
{
"code": "WC",
"name": "Western Cape"
}
],
"_links": {
"self": [
{
"href": "https://example.com/wp-json/wc/v2/data/locations/af/za"
}
],
"up": [
{
"href": "https://example.com/wp-json/wc/v2/data/locations/af"
}
]
}
}This should require a class for But your currently schema is correct and we can go with it. |
|
I dropped the I've added periods to all returned doc strings, and I think I've addressed all the code & doc style feedback. |
emdashcodes
left a comment
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.
Did another pass over the code. Tests pass. Responses look good.
I don't have anything else to add here. LGTM!
|
First off @ryelle, really sorry for going backwards and forwards on this. I appreciate the work you're doing here and acknowledge we're lacking good guidelines for new endpoints and I'm trying to rectify that this week. The schema of this is bothering me, in particular this part:
I feel we may just be trying to do too much in that one endpoint. Going back to @claudiosanches comment above (#15162 (comment)) whilst we should definitely not add the nesting back for continents (because it would be hard to consume) we could make a minor change here and have 2 separate endpoints. So this structure:
So a specific continent looks like: And a specific country looks like: Does that sound acceptable? @claudiosanches I'll be talking to Todd and @mattyza later about what version these endpoints can realistically go in, since we're going to need to version bump the API for discovery. With that in mind, if you could add a |
|
@mikejolley sounds good. Countries inside the |
9dfe840 to
1607338
Compare
|
Added a new branch |
|
@ryelle still missing handle each single item as an individual result, so you'll be able to set |
1607338 to
1f81464
Compare
…t_* helper function. This also adds the `_links` data to each item in the collection.
|
@claudiosanches Can you be a little more detailed? What is missing? Is it there now? |
|
@mikejolley Kelly already fixed it, but I was talking about treating each element as an element in individual, so including |
| * @param array $data The original list of continent(s), countries, and states. | ||
| * @param WP_REST_Request $request Request used to generate the response. | ||
| */ | ||
| return apply_filters( 'woocommerce_rest_prepare_data_continent', $response, $continent, $request ); |
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.
This filter should be included into prepare_item_for_response.
| } | ||
|
|
||
| $continent['countries'] = $local_countries; | ||
| $response = $this->prepare_item_for_response( $continent, $request ); |
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.
$response = $this->prepare_item_for_response( $continent, $request );
$response = $this->prepare_response_for_collection( $response );Both lines should be part of get_item and get_items and not while getting the "row data".
| if ( ! is_wp_error( $item ) ) { | ||
| $response->add_links( $this->prepare_links( $item ) ); | ||
| } | ||
| return $response; |
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.
Missing woocommerce_rest_prepare_* filter.
| } | ||
| } | ||
| $country['states'] = $local_states; | ||
| $response = $this->prepare_item_for_response( $country, $request ); |
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.
Need the same change here, putting both into get_item and get_items.
| public function prepare_item_for_response( $item, $request ) { | ||
| $response = rest_ensure_response( $item ); | ||
| $response->add_links( $this->prepare_links( $item ) ); | ||
| return $response; |
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.
Missing woocommerce_rest_prepare_* filter.
|
|
||
| // Wrap the data in a response object. | ||
| $response = rest_ensure_response( $data ); | ||
| $response->add_links( array( |
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.
This class should have a prepare_links method like all other classes in our REST API.
| */ | ||
| protected function prepare_links( $item ) { | ||
| $links = array(); | ||
| if ( array_key_exists( 'code', $item ) ) { |
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.
Not sure if need to check for array_key_exists( 'code', $item ) since now all items should have self and collection.
| public function get_item( $request ) { | ||
| $data = $this->get_continent( strtoupper( $request['location'] ), $request ); | ||
| if ( empty( $data ) ) { | ||
| $data = new WP_Error( 'woocommerce_rest_data_invalid_location', __( 'There are no locations matching these parameters.', 'woocommerce' ), array( 'status' => 404 ) ); |
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.
No need to pass WP_Error to variables, you can just use return and stop the code here.
This will avoid the need to check for is_wp_error in prepare_item_for_response too.
| * @return WP_REST_Response $response Response data. | ||
| */ | ||
| public function prepare_item_for_response( $item, $request ) { | ||
| $response = rest_ensure_response( $item ); |
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.
Can change it to:
$data = $this->add_additional_fields_to_object( $item, $request );
$data = $this->filter_response_by_context( $data, 'view' );
$response = rest_ensure_response( $data );
$response->add_links( $this->prepare_links( $item ) );add_additional_fields_to_object will allow extend results using register_rest_field.
|
@justinshreve When those last items are done, are you going to cherry-pick this into the wc-api-dev repo? |
|
@mikejolley Yeah, I can get these into the plugin. Are we good to go? |
claudiosanches
left a comment
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.
Looks good, great work :)
|
Merged into the feature plugin over at woocommerce/wc-api-dev#3. Thanks @ryelle! I think we can close this, since we are not going to be using the v3 branch. |
Fixes #15080 by adding a new endpoint
/data/locationsreturning all available continents, countries, and states; with options to restrict to a continent or country.This returns data that looks like the following. It uses
WC()->countries->get_*etc, so any filtered content should be reflected in the output.To test:
phpunit tests/unit-tests/api/data.php/data/locationsoptionally with continent & country codes, ex/data/locations/eu,/data/locations/us, or fake data to see an error/data/locations/aa/datalists an "index" view