-
-
Notifications
You must be signed in to change notification settings - Fork 2.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
feat(isTaxID): Add EU-UK TIN validation #1446
Conversation
* Added TIN validation for Austrian numbers * Added source of validation algorithms to header
* Refactored TIN unit tests to support more locales * Added unit tests for de-AT TINs * Added comments to isTaxID.js to explain behaviour of deAtCheck(tin)
* Added TIN validation for Greek numbers * Added relevant tests
* Added TIN validation for UK numbers * Added relevant tests
* Certain EU TINs might be entered with special characters which should not affect their validity/be omitted according to the specification. Such TINs are now sanitized before running checks in isTaxID(str, locale). * Removed en-GB validity check function (not needed, case already covered in isTaxID(str, locale). * Updated/simplified structure of regexes * Updated de-AT tests as some previously invalid TINs are now considered valid in line with the specification.
* Added TIN validation for Belgian numbers * Added relevant tests * Added local TIN names and validation scope (person/entity) to comments
* Countries with more than one locale should now have only one entry in the taxIdFormat and taxIdCheck objects, all others added as aliases below the objects. This should help avoid repetition. * Refactored nl-BE as alias to fr-BE * Renamed frNlBeCheck to frNlCheck to reflect changes.
* Added validation for French TINs * Added relevant tests
* Added validation for Cypriot TINs * Added relevant tests * Added tests for previously uncovered cases (calling isTaxID without a locale, frBeCheck invalid checksum)
* Added validation for Hungarian TINs * Added relevant tests * Refactored return statements to be one-liners where possible
Different locales might have specific needs wrt acceptable symbols in TINs- in some all are omitted during validation, while others only allow to skip a subset. A new object `sanitizeRegexes` has replaced the previous array, where locale-specific skippable symbol classes are to be placed. When all symbols can be omitted the new variable `allsymbols` is referenced. Aliases have also been added for the new object in line with the others and the isTaxID function checks for the locale's inclusion in `sanitizeRegexes`.
* Add TIN validation for German numbers * Add relevant tests
* Moved de-DE check digit calculation routine to new function `iso7064Check()` to be used for other conforming locales. * Add TIN validation for Croatian numbers * Add relevant tests * Refactor deDeCheck() to use iso7064Check()
* Add TIN validation for Bulgarian numbers * Add relevant tests
* Add TIN validation for Czech numbers * Add relevant tests
* Add validation for first digit of Greek TINs * Refactor el-GR tests * Add info for testable en-GB TINs
* Add TIN validation for Slovakian numbers * Add relevant tests
* Add TIN validation for Danish numbers * Add relevant tests
* Add TIN validation for Estonian numbers * Add relevant tests
* Add TIN validation for Lithuanian numbers (as alias of et-EE) * Add relevant tests
* Add TIN validation for Finnish numbers * Add relevant tests
* Add TIN validation for Italian numbers * Add relevant tests
* Add TIN validation for Irish numbers * Add relevant tests
* Add TIN validation for Latvian numbers * Add relevant tests
* Remove parseInt() calls in year extraction procedures of functions to improve performance and readability
* Add luhnCheck() to be used by conforming locale TINs * Refactor deAtCheck() to use luhnCheck()
* Add reverseMultiplyAndSum() to support new locale check functions
* Add TIN validation for Swedish numbers * Add relevant tests
* Add TIN validation for Dutch numbers * Add relevant tests * Refactor enIeCheck() to use reverseMultiplyAndSum()
* Add TIN validation for Portugese numbers * Add relevant tests
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.
src/lib/isTaxID.js
Outdated
if (!isDate(date, 'YY/MM/DD')) { return false; } | ||
|
||
/* | ||
* Check validity of birth town registry number (codice catastale) |
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.
May be we separate this section as a different PR so that we give it more deliberation; but then allow the rest to land?
src/lib/isTaxID.js
Outdated
* Called with an array of single-digit integers by locale-specific functions | ||
* to validate TINs according to ISO 7064 (MOD 11, 10). | ||
*/ | ||
function iso7064Check(digits) { |
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.
We can move these general-purpose algo checks (up to L118) into the src/lib/util/algorithms.js
and export and re-use here and any other place in future.
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.
agreed, will push both changes this weekend
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.
Let me know once you are done with the changes.
I can confirm the functionality of the de-DE locale. |
* Remove codice catastale validation subroutine from it-IT (to be included in another upstream PR) for further review.
713c7a9
to
b725ca3
Compare
* General-purpose validation algorithms have benn moved to `src/lib/util/algoritms.js` to support further use. * Validation algorithms have been refactored to use strings as input * isTaxID has been refactored to use `algorithms.js`
@profnandaa Both changes have been pushed- sorry for the delay, just moved to a new country. Also sorry for the force push- very bad habit of mine. Algo checks now use strings as input in line with the rest of the project. I will open a new PR with the codice catastale procedure once this has been merged. |
@tplessas -- no worries, thanks for sending in. |
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.
LGTM, thanks! 🎉
Alright. Taking a look at this one in the evening.
Thanks.
…On Mon, Nov 16, 2020 at 2:37 AM Anthony Nandaa ***@***.***> wrote:
@tux-tn <https://github.com/tux-tn> @ezkemboi
<https://github.com/ezkemboi> -- if you can look at this one as first
priority will be awesome. I don't want it to miss the November train.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1446 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ALVWPH22VIHFTV3PNMVOVTTSQBQZZANCNFSM4RV5LGHQ>
.
--
Regards,
Ezrqn Kemboi. <http://www.ezrqnkemboi.dev>
Software Engineer | Innovator | Open Source Enthusiast
|
@profnandaa I have been busy a little but let me look at it by the end of tomorrow before you make a release on weekend. |
@profnandaa Conflict resolved. |
@ez - no pressure, thanks!
On Thu, Nov 19, 2020 at 9:15 PM Theodoros Plessas ***@***.***> wrote:
@profnandaa <https://github.com/profnandaa> Conflict resolved.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1446 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAB7ZEKN3Y4HMP247LCC4JTSQVOEJANCNFSM4RV5LGHQ>
.
--
Sent from a tiny device while on the move.
|
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.
Huge work @tplessas ! Really good, nicely documented and structured.
I have just one question, should we keep the algorithms
separated from the validator? Are they useful somewhere else than TaxID validation?
It might be used in the future. |
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.
We can land this @profnandaa. Looks good.
Thanks, @tplessas for the contribution.
Sure. I was actually the one that asked that we separate the algos just in
case we reuse later, since they are general purpose.
-na
On Fri, Nov 20, 2020 at 7:04 PM Ezrqn Kemboi ***@***.***> wrote:
***@***.**** approved this pull request.
We can land this @profnandaa <https://github.com/profnandaa>. Looks good.
Thanks, @tplessas <https://github.com/tplessas> for the contribution.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1446 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAB7ZEIZLICRL2TG6VU7BHDSQ2HPXANCNFSM4RV5LGHQ>
.
--
Sent from a tiny device while on the move.
|
Checklist