-
Notifications
You must be signed in to change notification settings - Fork 59
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
Allow MOSS-registered non-EU companies to be used as requester #85
Conversation
Thanks |
@@ -80,6 +80,7 @@ class Vies | |||
'SI' => ['name' => 'Slovenia', 'validator' => Validator\ValidatorSI::class], | |||
'SK' => ['name' => 'Slovakia', 'validator' => Validator\ValidatorSK::class], | |||
'GB' => ['name' => 'United Kingdom', 'validator' => Validator\ValidatorGB::class], | |||
'EU' => ['name' => 'MOSS Number', 'validator' => Validator\ValidatorEU::class], |
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.
The problem with adding it as an entry here is that listEuropeanCountries will then return EU as a country which is incorrect.
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.
True ...
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.
unset($list['EU])
would "fix" that.
But since the const is named VIES_EU_COUNTRY_LIST
there should follow a refactoring.
This could be an easy change of the const name,
or splitting up the array :/
EDIT: const remember the count (which is now also wrong) at VIES_EU_COUNTRY_TOTAL
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.
I agree here with @Seldaek that EU
is not a EU country, but this is a naming convention we made before we knew about MOSS.
In this new situation we should rename the constant into something reflecting the correct representation like VIES_EU_PREFIX_LIST
or something similar, just as @cottton states.
NOTE: Refactoring the constant name should be a separate ticket
Or just take my initial PR which added it only where it's needed with a one
line change? :p
… |
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.
I cannot state how much I appreciate the involvement of all of you.
Up until @Seldaek mentioned this as a ticket I was not even aware of MOSS and it was hard to find any reasonable documentation about the format and how it was assigned, validated and more. A big shout out to @krzaczek to keep digging until he unvailed this information.
I will create two new tickets to rename the constants to stay meaningful within the code.
Awesome job all 👏
@@ -49,7 +49,7 @@ class Vies | |||
const VIES_PROTO = 'http'; | |||
const VIES_DOMAIN = 'ec.europa.eu'; | |||
const VIES_WSDL = '/taxation_customs/vies/checkVatService.wsdl'; | |||
const VIES_EU_COUNTRY_TOTAL = 28; | |||
const VIES_EU_COUNTRY_TOTAL = 29; |
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.
Even though EU
is not a member state, it has given a special purpose within the European VAT regulations, so we have now 29 allowed prefixes. The naming of VIES_EU_COUNTRY_TOTAL
was based upon the understanding only country prefixes were used. Now that this MOSS rule was brought to our attention, we should think about renaming this constant name in something that reflects the true purpose like VIES_EU_TOTAL_ALLOWED_PREFIXES
or something similar.
NOTE: Refactoring the constant name should be a separate ticket
@@ -80,6 +80,7 @@ class Vies | |||
'SI' => ['name' => 'Slovenia', 'validator' => Validator\ValidatorSI::class], | |||
'SK' => ['name' => 'Slovakia', 'validator' => Validator\ValidatorSK::class], | |||
'GB' => ['name' => 'United Kingdom', 'validator' => Validator\ValidatorGB::class], | |||
'EU' => ['name' => 'MOSS Number', 'validator' => Validator\ValidatorEU::class], |
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.
I agree here with @Seldaek that EU
is not a EU country, but this is a naming convention we made before we knew about MOSS.
In this new situation we should rename the constant into something reflecting the correct representation like VIES_EU_PREFIX_LIST
or something similar, just as @cottton states.
NOTE: Refactoring the constant name should be a separate ticket
Thanks! |
see #82
fix #84