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

Fix french ID parser #8

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Fix french ID parser #8

wants to merge 10 commits into from

Conversation

lukesolo
Copy link

@lukesolo lukesolo commented Jun 1, 2020

https://en.wikipedia.org/wiki/National_identity_card_(France)#Machine-readable_zone

Some French IDs don't have issuance code, so it's not correct to pick MRZ type by it's presence.

@@ -59,7 +59,7 @@ module.exports = [
},
Object.assign({}, documentNumberTemplate, {
line: 1,
start: 7,
start: 0,
Copy link
Member

Choose a reason for hiding this comment

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

why this change? The 3 fields are separated in the description of wikipedia. They can always be reconcatenated later

Copy link
Author

Choose a reason for hiding this comment

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

https://www.consilium.europa.eu/prado/en/FRA-BO-02002/image-7890.html
First 12 symbols represent complete Document Number. I don't think there is a case to use just part of it somewhere. Without this change all library clients should check if it's France then concatenate 3 fields to get the doc number.

I can make a pull request without this change if you don't want to break the backward compatibility.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the explanation. This LGTM then. No problem, we'll make a major release with all the changes.

@@ -5,7 +5,8 @@ const formats = {
TD2: 'TD2',
TD3: 'TD3',
SWISS_DRIVING_LICENSE: 'SWISS_DRIVING_LICENSE',
FRENCH_NATIONAL_ID: 'FRENCH_NATIONAL_ID'
FRENCH_NATIONAL_ID: 'FRENCH_NATIONAL_ID',
FRENCH_DRIVING_LICENSE: 'FRENCH_DRIVING_LICENSE'
Copy link
Member

Choose a reason for hiding this comment

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

Could we call it EU_DRIVING_LICENSE or the french one has non-standard parts?

Copy link
Author

Choose a reason for hiding this comment

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

There is no standard in EU driving license. As I know, French, Swiss and Netherlands driving licenses are all has unique format.

@targos
Copy link
Member

targos commented Jun 11, 2020

Thank you for the contribution!

Comment on lines +2 to +3
"name": "@lukesolo/mrz",
"version": "3.1.3",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"name": "@lukesolo/mrz",
"version": "3.1.3",
"name": "mrz",
"version": "3.1.1",


module.exports = {
testURL: 'http://localhost',
};
Copy link
Member

Choose a reason for hiding this comment

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

what does this fix?

Copy link
Author

Choose a reason for hiding this comment

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

Hmm, as I remember I had some problems with Jest and after googling this helped me.

Copy link
Author

Choose a reason for hiding this comment

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

Without it I get

 FAIL  src/parse/__tests__/td3.js
  ● Test suite failed to run

    SecurityError: localStorage is not available for opaque origins
      
      at Window.get localStorage [as localStorage] (node_modules/jsdom/lib/jsdom/browser/Window.js:257:15)
          at Array.forEach (<anonymous>)

Copy link
Member

Choose a reason for hiding this comment

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

could you try with testEnvironment: "node" instead?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, it helps.
Thanks!

@lpatiny
Copy link
Member

lpatiny commented Sep 7, 2021

@targos any idea why this branch was not merged =

@targos
Copy link
Member

targos commented Sep 7, 2021

There are open discussions

@fominv
Copy link
Contributor

fominv commented Nov 19, 2024

For those interested in a much smaller scope:

I created a PR that only adds support for french national IDs with no administrative code.

#61

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

Successfully merging this pull request may close these issues.

6 participants