-
Notifications
You must be signed in to change notification settings - Fork 210
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 handling for PR as a US state in terminal locations API #4044
base: develop
Are you sure you want to change the base?
Conversation
I'm not very familiar with Stripe terminal, so I'd like to learn more about how to test the PR properly before leaving a review. I followed the testing instructions from #2144 and installed Basic Auth. Here are the results: curl --user admin:admin \
-X POST \
-H "Content-Type: application/json" \
-d '{
"display_name": "PR Test Location",
"address": {
"line1": "123 Main St",
"city": "San Juan",
"postal_code": "00901",
"country": "PR"
}
}' \
http://localhost:8072/wp-json/wc/v3/wc_stripe/terminal/locations {"id":"tml_F9EpAAFAj6DXV9","object":"terminal.location","address":{"city":"San Juan","country":"PR","line1":"123 Main St","line2":"","postal_code":"00901","state":""},"display_name":"PR Test Location","livemode":false,"metadata":{}} curl --user admin:admin \
http://localhost:8072/wp-json/wc/v3/wc_stripe/terminal/locations ✅ Locations listed appear to be transformed correctly. [{"id":"tml_F9EpAAFAj6DXV9","object":"terminal.location","address":{"city":"San Juan","country":"US","line1":"123 Main St","line2":"","postal_code":"00901","state":"PR"},"display_name":"PR Test Location","livemode":false,"metadata":{}}] curl --user admin:admin \
http://localhost:8072/wp-json/wc/v3/wc_stripe/terminal/locations/tml_F9EpAAFAj6DXV9 ✅ Single location appears to be transformed correctly. {"id":"tml_F9EpAAFAj6DXV9","object":"terminal.location","address":{"city":"San Juan","country":"US","line1":"123 Main St","line2":"","postal_code":"00901","state":"PR"},"display_name":"Updated PR Location","livemode":false,"metadata":{}} As I'll be AFK the next couple of days, I'll reassign the reviewer. |
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.
@deepakpathania it looks like the PR is not fully complete yet. I found the same issue as Ricardo in the wc/v3/wc_stripe/terminal/locations
endpoint, where PR
is still being returned as a country when it should be returned as a state, with the country set to US
.
Additionally, if a PR
address is updated with the country correctly set as US
and the state as PR
, it gets misconfigured, returning the country as PR
and leaving the state empty.
Existing address
{
"id": "tml_F9VZ1g0NhsHBGD",
"object": "terminal.location",
"address": {
"city": "San juan",
"country": "US",
"line1": "1 Example St.",
"line2": "",
"postal_code": "00907",
"state": "PR"
},
"display_name": "Test Location 1",
"livemode": false,
"metadata": {}
}
After updating the address
curl -X POST https://wcstripe.test/wp-json/wc/v3/wc_stripe/terminal/locations/tml_F9VZ1g0NhsHBGD \
-H "Content-Type: application/json" \
-d '{
"line1": "12 Example St.",
"city": "San juan",
"country": "PR",
"postal_code": "00907"
}' \
-u ck_foo:cs_bar
{
"id": "tml_F9VZ1g0NhsHBGD",
"object": "terminal.location",
"address": {
"city": "San juan",
"country": "PR",
"line1": "1 Example St.",
"line2": "",
"postal_code": "00907",
"state": ""
},
"display_name": "Test Location 1",
"livemode": false,
"metadata": {}
}
Notice that the country and state were changed incorrectly.
Please give a look at these issues.
@asumaran I have added handling for the create and update cases as well. Could you check once again? Also would you mind sharing credentials for the testing setup so that I can test out with the curl commands you shared as well? Thanks. |
@deepakpathania I’m testing on my local site. You can also test the REST API using cURL by generating an API key from WooCommerce > Settings > Advanced > REST API. If you don't have a local setup you could use Jurassic Ninja. |
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 update @deepakpathania. It seems to be working fine with all endpoints I tested:
POST /wp-json/wc/v3/wc_stripe/terminal/locations
GET /wp-json/wc/v3/wc_stripe/terminal/locations
GET /wp-json/wc/v3/wc_stripe/terminal/locations/tml_abcxyz
DELETE /wp-json/wc/v3/wc_stripe/terminal/locations/tml_abcxyz
POST /wp-json/wc/v3/wc_stripe/terminal/locations/tml_abcxyz
GET /wp-json/wc/v3/wc_stripe/terminal/locations/store
Waiting for @iamgabrielma to test before merging. |
Fixes #4061
Similar to: Automattic/woocommerce-payments#10349
Changes proposed in this Pull Request:
Testing instructions
PR
as state andUS
as country in casePR
is the location.Changelog entry
Changelog Entry Comment
Comment
Post merge