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

feat(isTaxID): Add EU-UK TIN validation #1446

Merged
merged 42 commits into from
Nov 24, 2020

Conversation

tplessas
Copy link
Contributor

@tplessas tplessas commented Sep 22, 2020

  • Add support for the following locales: [ 'bg-BG', 'cs-CZ', 'de-AT', 'de-DE', 'dk-DK', 'el-CY', 'el-GR', 'en-GB', 'en-IE', 'es-ES', 'et-EE', 'fi-FI', 'fr-BE', 'fr-FR', 'fr-LU', 'hr-HR', 'hu-HU', 'it-IT', 'lb-LU', 'lt-LT', 'lv-LV' 'mt-MT', 'nl-BE', 'nl-NL', 'pl-PL', 'pt-PT', 'ro-RO', 'sk-SK', 'sl-SI', 'sv-SE' ]
  • Add tests for locales
  • Add helper validation functions
  • Update isTaxID() readme description
  • Will close Contribute: Support for more locales to isTaxID #1423

Checklist

  • PR contains only changes related; no stray files, etc.
  • README updated (where applicable)
  • Tests written (where applicable)

tplessas added 30 commits August 25, 2020 06:53
* 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
@profnandaa
Copy link
Member

This is a great contribution, I'll review it this weekend. Thanks!

/cc. @tux-tn @ezkemboi -- please have a look too.

Copy link
Member

@profnandaa profnandaa left a comment

Choose a reason for hiding this comment

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

Really great work on this. LGTM for the most part, just a few comments here.

You can address that as I wait for @ezkemboi @tux-tn @rubiin to have a look.

if (!isDate(date, 'YY/MM/DD')) { return false; }

/*
* Check validity of birth town registry number (codice catastale)
Copy link
Member

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?

* 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) {
Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Member

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.

@profnandaa profnandaa added 🧹 needs-update For PRs that need to be updated before landing and removed 🕑 to-be-reviewed labels Oct 9, 2020
@firlus
Copy link
Contributor

firlus commented Oct 19, 2020

I can confirm the functionality of the de-DE locale.
Implemented it as well (#1489) before seing that it already has been done.

* Remove codice catastale validation subroutine from it-IT
(to be included in another upstream PR) for further review.
* 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`
@tplessas
Copy link
Contributor Author

@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.

@profnandaa
Copy link
Member

@tplessas -- no worries, thanks for sending in.

Copy link
Member

@profnandaa profnandaa left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! 🎉

@profnandaa profnandaa removed the 🧹 needs-update For PRs that need to be updated before landing label Oct 21, 2020
@profnandaa
Copy link
Member

@rubiin @tux-tn or @ezkemboi -- kindly have a look at this, and we can land it soonest.

@profnandaa
Copy link
Member

@tux-tn @ezkemboi -- if you can look at this one as first priority will be awesome. I don't want it to miss the November train.

@ezkemboi
Copy link
Member

ezkemboi commented Nov 16, 2020 via email

@profnandaa
Copy link
Member

@tplessas -- please resolve for conflict; @ezkemboi @tux-tn -- can check this as first priority since it was a huge diff. I would like to get this in in the weekend release.

@ezkemboi
Copy link
Member

@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.

@tplessas
Copy link
Contributor Author

@profnandaa Conflict resolved.

@profnandaa
Copy link
Member

profnandaa commented Nov 19, 2020 via email

Copy link
Member

@tux-tn tux-tn left a 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?

@ezkemboi
Copy link
Member

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.

Copy link
Member

@ezkemboi ezkemboi left a 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.

@profnandaa
Copy link
Member

profnandaa commented Nov 23, 2020 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Contribute: Support for more locales to isTaxID
5 participants