Skip to content

Fix taxes endpoint returning incorrect X-WP-Total and X-WP-TotalPages headers #30890

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.

Sign up for GitHub

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

Jump to bottom
Merged
barryhughes merged 1 commit into from
Nov 24, 2021
Merged

Fix taxes endpoint returning incorrect X-WP-Total and X-WP-TotalPages headers #30890

barryhughes merged 1 commit into from
Nov 24, 2021

Conversation

@mahmoudsaeed
Copy link
Contributor

@mahmoudsaeed mahmoudsaeed commented Oct 5, 2021 •
edited by barryhughes
Loading

All Submissions:

Changes proposed in this Pull Request:

Closes #30838 .

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 - REST API now supplies the correct pagination information when fetching tax rates.

FOR PR REVIEWER ONLY:

  • I have reviewed that everything is sanitized/escaped appropriately for any SQL or XSS injection possibilities. I made sure Linting is not ignored or disabled.

@mahmoudsaeed mahmoudsaeed mentioned this pull request Oct 5, 2021
Closed
5 tasks
@barryhughes
Copy link
Member

barryhughes commented Nov 10, 2021 •
edited
Loading

Thanks, @mahmoudsaeed!

We do see some related test fails, including:

  • WC_REST_Taxes_Controller_Tests::test_get_tax_response_includes_cities_and_postcodes_as_arrays()
  • WC_REST_Taxes_Controller_Tests::test_get_tax_response_can_be_sorted_by_priority()

Do you have space to look at those before we move forward on this?

@barryhughes
Copy link
Member

barryhughes commented Nov 10, 2021

💡 At this point it would also be worth rebasing on latest trunk, to resolve the conflicts and align with the new repo structure :-)

@mahmoudsaeed
Copy link
Contributor Author

mahmoudsaeed commented Nov 24, 2021

@barryhughes Thanks for following up! I rebased and fixed the test fails.

@barryhughes
Copy link
Member

barryhughes commented Nov 24, 2021

Thanks: the previous fails (relating to WC_REST_Taxes_Controller_Tests) seem to be fixed as of rebasing and I believe the latest fails (consistent connect ECONNREFUSED 127.0.0.1:80 issues in the API tests) are unrelated and perhaps temporary. I'll trigger a further re-run, though.

barryhughes
barryhughes approved these changes Nov 24, 2021
View reviewed changes
Copy link
Member

@barryhughes barryhughes left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution!

@barryhughes barryhughes merged commit ca40a48 into woocommerce:trunk Nov 24, 2021
@github-actions github-actions bot added this to the 6.1.0 milestone Nov 24, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Nov 24, 2021 •
edited by barryhughes
Loading

Hi @barryhughes, thanks for merging this pull request. Please take a look at these follow-up tasks you may need to perform:

  • Add the status: needs changelog label
  • Add the status: needs testing instructions label

@barryhughes barryhughes added the release: add changelog Mark all PRs that have not had their changelog entries added. [auto] label Nov 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@barryhughes barryhughes barryhughes approved these changes

Assignees

No one assigned

Labels

release: add changelog Mark all PRs that have not had their changelog entries added. [auto]

Projects

None yet

Milestone

6.1.0

Development

Successfully merging this pull request may close these issues.

Taxes endpoint pagination

2 participants

@mahmoudsaeed @barryhughes