Skip to content
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

Added actions, reducers & selectors to fetch the locations from the WooCommerce host. #15032

Merged
merged 5 commits into from
Jun 18, 2017

Conversation

DanReyLop
Copy link
Contributor

Fixes #14266

This PR adds logic to fetch locations data from the WooCommerce server. It uses the new v3/data/continents endpoint (from here). Since each continent already is returned with all the info for all its countries, and every country is returned with all the info for all its states, there's no need to hit any other endpoint.

The data is stored as it comes from the server, but I've added a few selectors to "query" it. Since there are quite a few countries in the world, maybe I've been too negligent with performance. When this is hooked to an actual UI, I'll measure and if needed optimize the data structures or add memoizing to the selectors.

There is no way to test it in the UI, so just run the tests.

@DanReyLop DanReyLop added Store [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Tests] Includes Tests labels Jun 13, 2017
@DanReyLop DanReyLop self-assigned this Jun 13, 2017
@matticbot
Copy link
Contributor

@matticbot matticbot added the [Size] XL Probably needs to be broken down into multiple smaller issues label Jun 13, 2017
* @return {Array} The locations tree, as retrieved from the server. It can also be the string "LOADING"
* if the locations are currently being fetched, or a "falsy" value if that haven't been fetched at all.
*/
export const getRawLocations = ( state, siteId = getSelectedSiteId( state ) ) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you need the Raw here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this being used somewhere else? From this PR it is not evident why it is being exported

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, it doesn't need to be exported. Fixed.

@matticbot
Copy link
Contributor

@DanReyLop This PR needs a rebase

],
},
{
code: "AM",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor but this should be NA and North America.

},
],
},
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NA and North America


const locations = [
{
code: 'AM',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NA and North America

expect( hasStates( loadedState, 'SA' ) ).to.be.false;
} );

it( 'should return true if the countri has states', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: country

Copy link
Contributor

@justinshreve justinshreve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good to me. The main thing is replacing AM/America in the docs and tests with NA and North America which match what would be returned.

@DanReyLop
Copy link
Contributor Author

Overall looks good to me. The main thing is replacing AM/America in the docs and tests with NA and North America which match what would be returned.

🤦‍♂️ fixed. It was just for an example, but I should have looked at the real values anyway :)

@matticbot
Copy link
Contributor

@DanReyLop This PR needs a rebase

@DanReyLop
Copy link
Contributor Author

@belcherj @justinshreve Can I please get another quick 👀? I've fixed the comments you had.

Copy link
Contributor

@justinshreve justinshreve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved - your latest change LGTM.

@justinshreve justinshreve added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jun 16, 2017
@DanReyLop DanReyLop merged commit 062fccd into master Jun 18, 2017
@DanReyLop DanReyLop deleted the add/14266-get-locations branch June 18, 2017 07:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Size] XL Probably needs to be broken down into multiple smaller issues Store [Tests] Includes Tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants