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

Meaningful constants #86

Closed
DragonBe opened this issue Nov 11, 2019 · 5 comments
Closed

Meaningful constants #86

DragonBe opened this issue Nov 11, 2019 · 5 comments
Labels
easy pick good first issue If this is the first time you want to contribute, this issue might help you getting started help wanted We could really use your help quality refactoring Improving existing code for the better

Comments

@DragonBe
Copy link
Owner

When we first started out creating this service, we only looked at validation of VAT ID's for EU countries and named our constants in such a way that we weren't ready for other validation types.

Now with issue #82 a new type of validation was presented for companies outside of EU that require a VAT ID to perform business within the EU. We have implemented this validation already in PR #85 but the naming convention of the constants VIES_EU_COUNTRY_TOTAL and VIES_EU_COUNTRY_LIST no longer represent their true values.

These constants require to be changed into a more meaningful label that reflects this new situation.

@DragonBe DragonBe added quality easy pick help wanted We could really use your help good first issue If this is the first time you want to contribute, this issue might help you getting started refactoring Improving existing code for the better labels Nov 11, 2019
@Seldaek
Copy link
Contributor

Seldaek commented Nov 11, 2019

Moreover, the listEuropeanCountries function should not return 'EU' as a european country code IMO, regardless of what the constant is called.

@krzaczek
Copy link
Collaborator

krzaczek commented Nov 11, 2019

@Seldaek yes the listEuropeanCountries should not return EU.

We could do this two ways

  1. We could leave EU in the list of VIES_EU_COUNTRY_LIST and just update the listEuropeanCountries to not return EU as a "country" by simply filtering the return array.
    or
  2. We could remove EU from VIES_EU_COUNTRY_LIST and write a new method to validate requestor prefix (that should return true for all 28 countries and EU

I think we should go with option 1) since probably (don't know why it's not working this way now) VIES will allow to validate EU MOSS VAT numbers in the near future, and we will just need to rewrite the ValidateEU class to make it work.

@Seldaek
Copy link
Contributor

Seldaek commented Dec 17, 2019

@krzaczek implemented 1) in #87 - it doesn't fix the const naming issue but at least the public API is now fixed which I hope makes it ready to release.

@krzaczek
Copy link
Collaborator

krzaczek commented Jan 4, 2020

@Seldaek Could You fix the PHPCS errors so we could merge it and release it #87 (comment)

@DragonBe
Copy link
Owner Author

DragonBe commented Jan 7, 2020

Closing this issue as PR #87 is now integrated in master code and will be released soon.

@DragonBe DragonBe closed this as completed Jan 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
easy pick good first issue If this is the first time you want to contribute, this issue might help you getting started help wanted We could really use your help quality refactoring Improving existing code for the better
Projects
None yet
Development

No branches or pull requests

3 participants