-
Notifications
You must be signed in to change notification settings - Fork 30
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
parse Canadian licenses #6
base: master
Are you sure you want to change the base?
Conversation
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.
This may need some tweaking yet, but now that I have something working I wanted to ask for feedback.
src/keys.js
Outdated
@@ -17,8 +19,10 @@ exports.CodeToKey = { | |||
DAJ: 'addressState', | |||
DAK: 'addressPostalCode', | |||
DAQ: 'documentNumber', | |||
DAT: 'jurisdictionRestrictionCodes', // License Endorsements Code |
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.
WIP: I'm not 100% certain on these yet.
@@ -21,18 +21,27 @@ exports.parse = function parseCode128(str, options = defaultOptions) { | |||
let value = getValue(line); | |||
let key = getKey(code); | |||
if (!key) { | |||
if (options.suppressErrors) { | |||
if (options.suppressErrors || code === "ZNZ") { |
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.
The ZNZ line in the USA sample licenses was ignored by slicing off the last line, but the last line has valid information for my license (eyeColor).
}); | ||
|
||
// date format depends on issuer | ||
const issuer = props["issuer"] || "CAN"; | ||
const getDateFormat = issuer === "USA" ? getDateFormatUSA : getDateFormatCAN; |
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.
An alternative to this would be to utilize the 6-digit Issuer Identification Number to determine the country, since an IIN is always present.
https://www.aamva.org/IIN-and-RID/
Oddly the IIN on my license isn't in the list.
|
||
const isDateField = key => key.indexOf("date") === 0; | ||
|
||
const getDateFormat = value => { | ||
const getDateFormatUSA = value => { | ||
const parts = [value.slice(0, 2), value.slice(2, 4), value.slice(4)]; | ||
return parts.join("/"); |
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.
Should these functions be creating Dates instead? (breaking API change)
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.
personally, i hate Date objects :) They eventually need to be exported to strings, and then the exporter needs to know how it was created, or worse, think. As in:
> new Date('2000-01-01').getTime()
946684800000
> new Date('01-01-2000').getTime()
946702800000
> fuckMe()
string date format wise, i prefer the relatively unambiguous iso format yyyy-mm-dd
. I meant to ask for that on a previous PR, but forgot. It would also be a breaking change, but that's ok.
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.
So you want to change the U.S. format to a string that matches the Canadian yyyy-mm-dd?
@mvayngrib any feedback on this? |
@nathany sorry for the slow response and thanks for the PR! I'll comment on stuff separately near relevant code |
@nathany overall looks great! Take a look at the couple of comments i made |
Thanks Mark. I should be able to address any comments on Thursday. |
We have more changes that I think we need. When @nathany is back tomorrow he probably will want to fix up what we've done and then we will amend the PR. |
There's also the transpile issue #1. |
@nathany yes, I would prefer returning |
A bit late coming into this, but in #9 there is an issue with Ontario driver's licenses (it may be the same in other provinces, but I haven't verified yet). |
I'm not sure if/when I'll be given time for this open source contribution. We don't need support for U.S., so we've dropped trying to support both for our own use. Does anyone want to pick this up? |
@nathany I don't mind having a go. I'll try to finish this off over the next few days. I'll merge my PR into this one. |
OK. I just uploaded a new version of #10. It is similar to this one, and it also addresses @mvayngrib's comments. @mvayngrib are you still planning on maintaining this repository? If not, can I request commit access to it? We are starting to use this quite heavily in my office and there are some other issues that I've seen with it that I'd like to fix. Specifically, I see that Quebec driver's licenses format their dates differently, so they are broken, too. |
happy to review a PR for that :) |
We are fixing the QC licenses in post processing for now, but it would be really nice to move this directly into this library. I'll get something going for that. |
This sample data is based on scanning a physical Alberta driver's license. There are some things that don't seem to meet the specification. https://www.aamva.org/DL-ID-Card-Design-Standard/
Changes: