Skip to content

Conversation

@mahmoudsaeed
Copy link
Contributor

@mahmoudsaeed mahmoudsaeed commented Sep 22, 2020

All Submissions:

Changes proposed in this Pull Request:

The proposed changes allow the taxes endpoint to return an array of postcodes/cities for tax rates with multiple (semi-colon separated) postcodes and/or cities in two new keys, cities and postcodes; the old existing city and postcode keys are kept for compatibility and will return only one of the available values.

Closes #27750 .

How to test the changes in this Pull Request:

  1. In WooCommerce admin go to Settings - Tax - Standard Rates and create 3 rates: one with no cities or postcodes specified, another one with one city and one postcode, and one with two cities and two postcodes (separate them with a semicolon, ;).

  2. Temporarily add this at the ned of your woocommerce.php file, this way you won't have to bother about REST API authentication: add_filter('woocommerce_rest_check_permissions', function() {return true;}, 10, 0 ); (note that this will cause a bunch of unit tests to fail)

  3. Retrieve the taxes via the /wp-json/wc/v3/taxes endpoint and verity that the results are consistent:

For no city and no postcode:

 {
    "postcode": "",
    "city": "",
    "postcodes": [],
    "cities": []
}

For one city and postcode:

 {
    "postcode": "1111",
    "city": "OSAKA",
    "postcodes": [
      "1111"
    ],
    "cities": [
      "OSAKA"
    ],
}

For two cities and postcodes:

{
    "postcode": "2222",
    "city": "KOBE",
    "postcodes": [
      "1111",
      "2222"
    ],
    "cities": [
      "OSAKA",
      "KOBE"
    ],

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully run tests with your changes locally?

Changelog entry

Fix - Add array-typed "cities" and "postcodes" to the response of the "tax" endpoint in the REST API to overcome the limitation of "city" and "postcode" returning always one single value.

@mahmoudsaeed mahmoudsaeed changed the title Fix taxes endpoint not returning multiple postcodes/cities (#27750) Fix taxes endpoint not returning multiple postcodes/cities Sep 22, 2020
@peterfabian peterfabian requested review from a team and Konamiman and removed request for a team January 19, 2021 10:14
Copy link
Contributor

@Konamiman Konamiman left a comment

Choose a reason for hiding this comment

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

Hi @mahmoudsaeed, thank you for your contribution. I've tested your changes and they work as expected, however there's a problem with your approach: modifying the type of an existing field of the response is a breaking change and could cause trouble to clients (which could be assuming that a string is always returned).

Therefore I would suggest the following changes to your pull request:

  1. Leave the current postcode and city fields untouched. We'll mark these as deprecated.
  2. Add two new fields: postcodes and cities - note that they are in plural. Make those always return an array, even when there's just one value to return or there is nothing to return at all (empty array in the later case).

Please note that 2 should be implemented in the v3 class (includes/rest-api/Controllers/Version3/class-wc-rest-taxes-controller.php) instead of the v1 class, since older versions of the API shouldn't change. This will probably mean that you'll need to extract some portions of the original code from v1 into protected methods that will be overriden in the v3 class.

Once you have applied these changes please ping me and I'll be glad to review your pull request again.

Base automatically changed from master to trunk February 25, 2021 22:17
@mahmoudsaeed
Copy link
Contributor Author

Thanks @Konamiman for reviewing this. I have made the requested changes.

@mahmoudsaeed mahmoudsaeed requested a review from Konamiman March 18, 2021 13:03
@Konamiman
Copy link
Contributor

Hi @mahmoudsaeed, sorry for the delay. I've re-reviewed and approved the pull request, also I've edited the description to introduce testing instructions (which replace your "example output") and a changelog entry to conform to our standards. I'll merge as soon as the CI checks are good.

@Konamiman Konamiman merged commit df78a5c into woocommerce:trunk Mar 23, 2021
@woocommercebot woocommercebot added release: add changelog Mark all PRs that have not had their changelog entries added. [auto] release: add testing instructions PRs that have not had testing instructions added to the wiki. [auto] labels Mar 23, 2021
@Konamiman Konamiman added the REST/Store API/Webhooks Issues related to WooCommerce REST API. label Mar 23, 2021
@Konamiman Konamiman added this to the 5.3.0 milestone Mar 23, 2021
@mahmoudsaeed mahmoudsaeed deleted the fix-rest-tax branch March 23, 2021 16:19
@tammullen tammullen added testing instructions added and removed release: add testing instructions PRs that have not had testing instructions added to the wiki. [auto] labels Apr 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release: add changelog Mark all PRs that have not had their changelog entries added. [auto] REST/Store API/Webhooks Issues related to WooCommerce REST API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

REST API: taxes endpoint does not return multiple postcodes/cities

4 participants