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

Brexit and VAT refactor #24943

Merged
merged 14 commits into from Feb 5, 2020
Merged

Brexit and VAT refactor #24943

merged 14 commits into from Feb 5, 2020

Conversation

kloon
Copy link
Member

@kloon kloon commented Oct 31, 2019

All Submissions:

Changes proposed in this Pull Request:

This PR does a couple of things. First it refactors the VAT functionality currently built into the WC_Countries::get_european_union_countries method by deprecating it and introducing a new method WC_Countries::get_vat_countries and updating all calls to the get_european_union_countries method for VAT purposes with the new method.

Secondly, it removes GB from the get_european_union_countries method and then add them to the get_vat_countries method since GB still uses VAT.

It is important to note that this PR is still in progress and should not be merged before Brexit officially happens. From what I could see there should be no effect on having GB remain as part of the get_european_union_countries in core since we only use that for determining to use VAT or TAX, but it could have an effect on plugins that relies on the method for specific functionality relating to EU countries, for that there is a filter currently in place that can be utilised to remove GB should it be needed in these plugins.

Closes #24666

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

Dev - Introduce WC_Countries::get_vat_countries for returning a list of countries that uses VAT and refactor WC_Countries::get_european_union_countries with backward compatibility and deprecation to remove the VAT functionality from there.
Dev - Brexit, remove GB from WC_Countries::get_european_union_countries

… VAT handling.

Add new WC_Countries::get_vat_countries for returning countries supporting VAT.
@kloon kloon added the status: in progress This is being worked on. label Oct 31, 2019
Copy link
Contributor

@rodrigoprimo rodrigoprimo left a comment

Choose a reason for hiding this comment

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

Looks good to me overall, but there is one problem that needs to be addressed that I mentioned in an inline comment.

Besides that, I wonder if we should keep GB in the list of european countries in this PR. This way, we can merge it for WC 3.9 as just the refactor is already an improvement and should make it easier for those that need to adjust their store using the filters provided here when Brexit happens and before we release a new WooCommerce version. Then we can address removing GB on a separate PR when the time comes. What do you think?

includes/class-wc-countries.php Outdated Show resolved Hide resolved
@claudiosanches claudiosanches marked this pull request as ready for review January 27, 2020 14:50
@claudiosanches claudiosanches added status: needs review and removed status: in progress This is being worked on. labels Jan 27, 2020
@claudiosanches
Copy link
Contributor

I'm fixing the conflicts.

@rodrigoprimo rodrigoprimo added this to the 4.0.0 milestone Jan 27, 2020
@claudiosanches
Copy link
Contributor

I have updated this PR to use WC_Countries::countries_using_vat() introduced in #24999

Copy link
Contributor

@peterfabian peterfabian left a comment

Choose a reason for hiding this comment

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

I think we can improve this one a bit, please see below.

includes/class-wc-countries.php Show resolved Hide resolved
tests/unit-tests/countries/countries.php Show resolved Hide resolved
@peterfabian peterfabian added needs: author feedback The issue/PR needs a response from any of the parties involved in the issue. and removed status: needs review labels Feb 5, 2020
@claudiosanches claudiosanches added status: needs review and removed needs: author feedback The issue/PR needs a response from any of the parties involved in the issue. labels Feb 5, 2020
@peterfabian peterfabian merged commit d3a33f8 into master Feb 5, 2020
@peterfabian peterfabian deleted the refactor/eu-vat-brexit branch February 5, 2020 18:41
@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 Feb 5, 2020
@peterfabian peterfabian removed release: add testing instructions PRs that have not had testing instructions added to the wiki. [auto] release: add changelog Mark all PRs that have not had their changelog entries added. [auto] labels Feb 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Brexit - October 31 - November 1
5 participants