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

Implemented VAT validation support for english (en-GB) locale #1463

Merged
merged 5 commits into from
Nov 29, 2020

Conversation

CodingNagger
Copy link
Contributor

I introduced the isVAT method based on a feature request spotted in #852. However, it feels like to big a piece to take all at once so I picked a locale which I need that validation for which is "en-GB".

I purposefully excluded generated files as these seem to contain changes that are not currently checked in the main master branch so that this PR only contains my changes.

Checklist

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

@codecov
Copy link

codecov bot commented Oct 5, 2020

Codecov Report

Merging #1463 (2686095) into master (c5a1a22) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1463   +/-   ##
=======================================
  Coverage   99.92%   99.92%           
=======================================
  Files          96       98    +2     
  Lines        1275     1340   +65     
=======================================
+ Hits         1274     1339   +65     
  Misses          1        1           
Impacted Files Coverage Δ
src/index.js 100.00% <100.00%> (ø)
src/lib/isVAT.js 100.00% <100.00%> (ø)
src/lib/alpha.js 100.00% <0.00%> (ø)
src/lib/isFQDN.js 100.00% <0.00%> (ø)
src/lib/isAlpha.js 100.00% <0.00%> (ø)
src/lib/isEmail.js 100.00% <0.00%> (ø)
src/lib/isPostalCode.js 100.00% <0.00%> (ø)
src/lib/isMobilePhone.js 100.00% <0.00%> (ø)
src/lib/isPassportNumber.js 100.00% <0.00%> (ø)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c5a1a22...2686095. Read the comment docs.

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.

Thanks for your contrib! 🎉 Just 2 comment from me:

  1. Don't know if we could just go with country codes only for territorial stuff like VAT, so should this be GB or UK? Just the way we've done for others like Passport...
  2. see below.

src/lib/isVAT.js Outdated
/^GB\d{3} \d{4} ([0-8][0-9]|9[0-6])$/,
/^GB\d{9} \d{3}$/,
/^GBGD[0-4][0-9]{2}$/,
/^GBHA[5-9][0-9]{2}$/,
Copy link
Member

Choose a reason for hiding this comment

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

Possible to make this one-line regex instead, to save on the matching loop later?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep that works

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the country codes, I'm cool going for ISO 3166-1 alpha-2 which would be GB for the UK.

@CodingNagger
Copy link
Contributor Author

ping @profnandaa

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 for your contrib! 🎉

Being a new feature, will request some extra pair of eyes from @ezkemboi @tux-tn @rubiin

@profnandaa
Copy link
Member

ping @ezkemboi @tux-tn

@rubiin
Copy link
Member

rubiin commented Nov 16, 2020

looks good to merge

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.

LGTM

@profnandaa
Copy link
Member

@CodingNagger -- qq, BTW, is VAT for GB different from that of UK?

@CodingNagger
Copy link
Contributor Author

Hey @profnandaa GB is the ISO 3166-1 alpha-2 notation for the UK.

@profnandaa
Copy link
Member

Ok, thanks!

README.md Outdated
@@ -155,6 +155,7 @@ Validator | Description
**isURL(str [, options])** | check if the string is an URL.<br/><br/>`options` is an object which defaults to `{ protocols: ['http','https','ftp'], require_tld: true, require_protocol: false, require_host: true, require_valid_protocol: true, allow_underscores: false, host_whitelist: false, host_blacklist: false, allow_trailing_dot: false, allow_protocol_relative_urls: false, disallow_auth: false }`.<br/><br/>require_protocol - if set as true isURL will return false if protocol is not present in the URL.<br/>require_valid_protocol - isURL will check if the URL's protocol is present in the protocols option.<br/>protocols - valid protocols can be modified with this option.<br/>require_host - if set as false isURL will not check if host is present in the URL.<br/>require_port - if set as true isURL will check if port is present in the URL.<br/>allow_protocol_relative_urls - if set as true protocol relative URLs will be allowed.<br/>validate_length - if set as false isURL will skip string length validation (2083 characters is IE max URL length).
**isUUID(str [, version])** | check if the string is a UUID (version 3, 4 or 5).
**isVariableWidth(str)** | check if the string contains a mixture of full and half-width chars.
**isVAT(str, countryCode)** | checks that the string is a [valid VAT number](https://en.wikipedia.org/wiki/VAT_identification_number) if validation is available for the given country code matching [ISO 3166-1 alpha-2](https://en.wikipedia.org/wiki/ISO_3166-1_alpha-2).
Copy link
Member

Choose a reason for hiding this comment

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

Please list the supported country codes so far in the README.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@profnandaa profnandaa added 🧹 needs-update For PRs that need to be updated before landing and removed 🍿 discussion labels Nov 19, 2020
@profnandaa
Copy link
Member

I think that should be it, just that minor README fix and we should be good to go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧹 needs-update For PRs that need to be updated before landing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants