-
Notifications
You must be signed in to change notification settings - Fork 120
Update Networking layer with GET /wp-json/wc/v3/customers/<id>
#7746
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
You can test the changes from this Pull Request by:
|
/wp-json/wc/v3/customers/wp-json/wc/v3/customers/<id>
As Fakes is on a different framework, the Customer object cannot be found due to its default internal access control. Catched this when CI ran tests.
joshheald
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, a few suggestions for reducing the amount of test code.
| let firstName = try container.decodeIfPresent(String.self, forKey: .firstName) ?? "" | ||
| let lastName = try container.decodeIfPresent(String.self, forKey: .lastName) ?? "" |
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.
Why the default strings here, given it's an optional property? I think you'd be best off with one or the other: make the property non-optional, or make it nil when we don't have the name.
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.
Oh that's right! firstName and lastName are not required by the API so initially I made them optional, that said, without the name we won't be able to perform a search later on. For now I'll leave them Optional for simplicity, but most likely we'll have to loop back and modify this when we implement the actual search, once we know better what we need. Removed default values at f5c1c3e
| XCTAssertNotNil(customer?.customerID) | ||
| XCTAssertNotNil(customer?.email) | ||
| XCTAssertNotNil(customer?.firstName) | ||
| XCTAssertNotNil(customer?.lastName) | ||
| XCTAssertNotNil(customer?.billing) | ||
| XCTAssertNotNil(customer?.shipping) |
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.
You can remove these asserts at least, or even this whole test, as they are covered by the correctly_parsed test
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.
Gotcha! Done at 2fe9688. I left the rest of the test so we can test the difference between if the data can be loaded from the local file vs if the values are correctly parsed. I see the correctly_parsed test also includes that the data must be loaded first in order to work, but it felt right to keep them separate. Happy to remove it entirely in any case 👍
| // Then | ||
| XCTAssert(result.isSuccess) | ||
| let customer = try XCTUnwrap(result.get()) | ||
| XCTAssertNotNil(customer) |
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 this assert is unnecessary: the previous one will fail and throw if it's nil. You may want to check another property about the customer instead, or nothing at all.
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 see what you mean, done at 32dfe2a
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.
Sorry, I meant the XCTAssertNotNil is unnecessary, though you're right that the XCTAssert(result.isSuccess) is also tested by the XCTUnwrap.
To be honest, and on second thoughts, since you never use the customer other than checking that it's there, unwrapping it isn't needed, I would consider simply doing XCTAssertNotNil(result.get()) in place of all three, though perhaps with the unwrap it's clearer that we're looking at a customer.
This is super trivial, no need to change it, I'm only commenting again because I'd not been clear the first time.
Part of #7741
Description
For merchants to be able to see and choose existing customers from their store when creating or editing orders, we need to know who are these existing customers. At the moment we can only get partial customer data based on other API properties, like Orders.
This PR adds the initial networking layer that we need in order to fetch existing customers from a Store. We do so by performing a GET request to the
/wp-json/wc/v3/customers/<id>API endpointWhy don't we fetch the entirety of
/customers/endpoint?We discovered that this endpoint does not search for first and last names, the proposed workaround is to use
/wc-analytics/customers?search=instead, which returns the customer's ID, then use/customers/<id>to fetch the actual data.For the moment, we're only modelling the Customer entity with a limited number of properties, the ones we actually use within the Order Creation screen (
email,firstName,lastName,billingdetails, andshippingdetails), rather than all available properties. We can include more later as needed.Changes
CustomerRemote,CustomerMapper, andCustomerobjects to the Networking layer. This retrieves, decodes, maps, and models an individual Customer entity from its JSON representation received from the API.Networking.generatedandModels+Copiable.generatedviarake generate, due addingGeneratedCopiableandGeneratedFakeableconformance to the model.customer.jsonfile replicating a dummy/wp-json/wc/v3/customers/<id>API response, so we can Unit Test without hitting the actual network.To avoid this PR to become too big, I'll add further Unit Tests on a different one.
Testing instructions