-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Fixes incorrect country code being used for Greek VAT numbers, should… #20548
Fixes incorrect country code being used for Greek VAT numbers, should… #20548
Conversation
… be 'EL' instead of 'GR'.
Hi @hostep. Thank you for your contribution
For more details, please, review the Magento Contributor Assistant documentation |
@@ -179,18 +179,21 @@ public function checkVatNumber($countryCode, $vatNumber, $requesterCountryCode = | |||
return $gatewayResponse; | |||
} | |||
|
|||
$countryCodeForVatNumber = $this->getCountryCodeForVatNumber($countryCode); | |||
$requesterCountryCodeForVatNumber = $this->getCountryCodeForVatNumber($requesterCountryCode); | |||
|
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.
Since Codacity blames about variable name length, you can try renaming them into $ctryVatCode
and $requesterCtryVatCode
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.
Hi @aleron75, thanks for the review!
I don't agree with this check on length of variable names by Codacy, I like to keep them readable and understandable, we aren't living in the 1990's anymore where disk space was limited and code was supposed to be as small as possible :)
If Magento code standards really wants me to change these to a shorter name, please let me know, and I will change them, but I've send in PR's in the past where the same check failed, and then it wasn't a problem and was just approved as is.
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.
Hello @hostep I will check, if this is not a real issue, your PR will proceed.
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.
Hello @hostep, I confirm you can ignore Codacity blaming about variable name length.
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.
Excellent, thanks! :)
Hi @aleron75, thank you for the review. |
Hi @hostep, thank you for your contribution! |
Thanks for reviewing and merging! |
… be 'EL' instead of 'GR'.
Description (*)
Greek VAT numbers use the prefix 'EL' and not the normal country code 'GR'.
This VAT number is send to the VIES VAT number validation and currently in Magento, the validation of Greek VAT numbers just never works. This PR fixes this.
Quoting from wikipedia:
Fixed Issues (if relevant)
Manual testing scenarios (*)
In admin:
EL1234
is send to VIES, right nowGR1234
is getting sendIn frontend:
EL1234
is send to VIES, right nowGR1234
is getting sendContribution checklist (*)