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(pt-BR): tax id, passport and license plates #1613

Merged
merged 6 commits into from
Mar 12, 2021

Conversation

mschunke
Copy link
Contributor

Included Tax ID, passport, and license plate validators for pt-BR (Brazil) locale. Tests written and passed. Locale added to the documentation.

Checklist

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

@renanmontebelo
Copy link
Contributor

There's some overlap with these already opened PRs:

We might consider to join forces if you wish.

@mschunke
Copy link
Contributor Author

mschunke commented Mar 1, 2021

Sure, for what I see @renanmontebelo license plate approach is more complete as "mercosur" plates are available too. My approach for tax Id validation uses the current .isTaxId function and supports also id (CNPJ) validation for entities and well as CPF (persons) validations.

I would suggest to keep his for license plates and mine for tax Ids.

tux-tn
tux-tn approved these changes Mar 1, 2021
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.

Thank you for your contribution @mschunke. Can you please resolve merge conflicts (delete validator.js and validator.min.js files and fix conflict in README.md ) and that would be great if you join forces and decide what to keep in each PR.
MB I approved the PR instead of requesting changes 😅

@tux-tn tux-tn added 🍿 discussion 🧹 needs-update For PRs that need to be updated before landing labels Mar 1, 2021
@codecov
Copy link

codecov bot commented Mar 5, 2021

Codecov Report

Merging #1613 (410ee54) into master (deb1d1e) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #1613   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          100       100           
  Lines         1807      1849   +42     
=========================================
+ Hits          1807      1849   +42     
Impacted Files Coverage Δ
src/lib/isPassportNumber.js 100.00% <ø> (ø)
src/lib/isTaxID.js 100.00% <100.00%> (ø)

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 deb1d1e...a511310. Read the comment docs.

@mschunke
Copy link
Contributor Author

mschunke commented Mar 5, 2021

Hey @tux-tn, all conflicts are solved and build checks passed. Anything else I should do?

tin === '88888888888' ||
tin === '99999999999' ||
tin === '00000000000'
) return false;
Copy link
Member

Choose a reason for hiding this comment

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

This line is not covered by tests, you probably just need to add a testcase with an invalid CPF

tin === '66666666666666' ||
tin === '77777777777777' ||
tin === '88888888888888' ||
tin === '99999999999999') { return false; }
Copy link
Member

Choose a reason for hiding this comment

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

Same here, this condition is not covered, adding a test with an invalid CNPJ should fix it

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.

Thank you @mschunke ! I added two comments concerning untested conditions/code branches. Can you check it?

@tux-tn
Copy link
Member

tux-tn commented Mar 5, 2021

@renanmontebelo @mschunke what have you decided concerning what approach to keep?
Should this one add only tax id and passport and the other one add license plate validation?

@mschunke
Copy link
Contributor Author

mschunke commented Mar 5, 2021

@renanmontebelo @mschunke what have you decided concerning what approach to keep?
Should this one add only tax id and passport and the other one add license plate validation?

Hey, @tux-tn,
Got no reply from @renanmontebelo, but I'm fine with this PR just adding tax id and passport and using the other PR for license plates.

@mschunke
Copy link
Contributor Author

mschunke commented Mar 5, 2021

Thank you @mschunke ! I added two comments concerning untested conditions/code branches. Can you check it?

@tux-tn
Was wondering where those untested lines were... thank you! Code updated with 100% coverage.

@renanmontebelo
Copy link
Contributor

@mschunke sorry for the delay, I can only work on this in the weekends. Selecting your PR for tax id and my PR for license plates sounds good. I'll close #1589 in favor of yours.

My tax id implementation also accepts an optional parameter to require or ignore formatting characters like . and -. Please feel free to copy into yours if you think it's a good addition. Thank you.

@mschunke
Copy link
Contributor Author

mschunke commented Mar 6, 2021

@renanmontebelo np. We all work on these on our spare time. I've removed my license plate validation for we can use yours, and kept my tax ID validator as agreed. Thanks for the cooperation!

btw, my tax id validator accepts both formatted and unformatted input.

@mschunke mschunke requested a review from tux-tn March 6, 2021 19:49
@renanmontebelo
Copy link
Contributor

Sounds good, thank you for the cooperation as well. I missed that yours accept both formatted/unformatted CPFs, that's awesome.

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.

Thank you @mschunke 🎉 ! Can you just remove the isLicensePlate PT-BR entry in README.md? i think it's the only necessary change

@mschunke
Copy link
Contributor Author

mschunke commented Mar 8, 2021

@tux-tn sorry about that, forget about the readme file! File updated. Please let me know if you need anything else 😁

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.

@mschunke that was fast 😅
LGTM 🎉 Thank you again for the fast response and for your collaboration with @renanmontebelo !

@tux-tn tux-tn removed 🍿 discussion 🧹 needs-update For PRs that need to be updated before landing labels Mar 8, 2021
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 @mschunke for the PR and welcome to the project! 🎉

@profnandaa profnandaa merged commit 3c771e8 into validatorjs:master Mar 12, 2021
@murilopulley
Copy link

Thanks @profnandaa! Happy to help :)

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.

5 participants