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

Add support for 8 digit IINs and 2 digit last_digits #123

Merged
merged 3 commits into from
Jan 20, 2022

Conversation

ugexe
Copy link
Contributor

@ugexe ugexe commented Nov 11, 2021

Previously issuer_id_number was expected to be 6 digits and
last_4_digits to be 4 digits. This changes the validation to
additionally allow for 8 digit issuer_id_numbers and 2 digit
last_4_digits.

Additionally last_4_digits has been deprecated in favor of the
more appropriately named last_digits.

@ugexe ugexe force-pushed the nlogan/8-digit-iin-2-digit-lastdigits branch from 48b7b0e to 7738b8a Compare November 15, 2021 13:51
));
parent::__construct(
v::allOf(
v::when(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This doesn't feel like it belongs in the parameter list, but I wasn't familiar enough with this to determine how to express it more clearly. I assumed I would be able to apply the 2|4 valuation on last_digits in the parameter list and do additional validation based on the iin inside the function body but I couldn't get it to DWIM

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that is a tricky one. I thought OneOf might work well here, but on reflection, we have to also handle the case where the keys aren't present. One possible improvement might be switching the When to:

             v::when(
                    v::key('issuer_id_number', v::stringType()->length(8, 8), false),
                    v::key('last_digits', v::stringType()->length(2, 2), false),
                    v::key('last_digits', v::stringType()->length(4, 4), false),
                ),

And moving the last_digits validation down. This should improve the error message and might make it a little bit easier to understand the logic.

Copy link
Member

@oschwald oschwald left a comment

Choose a reason for hiding this comment

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

One comment. I'll leave it up to you whether you want to make the change.

));
parent::__construct(
v::allOf(
v::when(
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that is a tricky one. I thought OneOf might work well here, but on reflection, we have to also handle the case where the keys aren't present. One possible improvement might be switching the When to:

             v::when(
                    v::key('issuer_id_number', v::stringType()->length(8, 8), false),
                    v::key('last_digits', v::stringType()->length(2, 2), false),
                    v::key('last_digits', v::stringType()->length(4, 4), false),
                ),

And moving the last_digits validation down. This should improve the error message and might make it a little bit easier to understand the logic.

Previously issuer_id_number was expected to be 6 digits and
last_4_digits to be 4 digits. This changes the validation to
additionally allow for 8 digit issuer_id_numbers and 2 digit
last_4_digits.

Additionally last_4_digits has been deprecated in favor of the
more appropriately named last_digits.
@ugexe ugexe force-pushed the nlogan/8-digit-iin-2-digit-lastdigits branch from 55865b9 to 1e4aa83 Compare January 19, 2022 14:36
@ugexe ugexe force-pushed the nlogan/8-digit-iin-2-digit-lastdigits branch from 8aa20b6 to 5375e27 Compare January 19, 2022 14:45
@oschwald oschwald merged commit b49abb7 into main Jan 20, 2022
@oschwald oschwald deleted the nlogan/8-digit-iin-2-digit-lastdigits branch January 20, 2022 16:53
@mrtimp
Copy link

mrtimp commented Jan 20, 2022

@oschwald our credit card provider only returns the last 2 not 4 digits of a masked credit card so this PR is of interest for us. Do you have any idea when you will be tagging a new release?

@oschwald
Copy link
Member

@mrtimp, I suspect we will be releasing this in the very near future, perhaps the next week or two.

@mrtimp
Copy link

mrtimp commented Jan 24, 2022

Fantastic @oschwald! Look forward to it.

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

Successfully merging this pull request may close these issues.

None yet

3 participants