Uh oh!
There was an error while loading. Please reload this page.
Fix A-z ranges - #1625
Conversation
Codecov Report
@@ Coverage Diff @@## master #1625 +/- ##
=========================================
Coverage 100.00% 100.00% =========================================
Files 100 100 Lines 1798 1846 +48 =========================================
+ Hits 1798 1846 +48
Continue to review full report at Codecov.
|
Uh oh!
There was an error while loading. Please reload this page.
tux-tn
left a comment
There was a problem hiding this comment.
Good catch ! Thank you @bmacnaughton and congrats for your first contribution 🎉
profnandaa
commented
Mar 12, 2021
Will recheck something before landing. Thanks for your contrib! |
bmacnaughton
commented
Mar 15, 2021
can i help with any information for additional review? this seems like the validator passes some potentially troublesome characters. |
bmacnaughton
commented
Mar 20, 2021
@tux-tn, @profnandaa - is there anything i can do to wrap this up? if you need to check something i'm happy to research it for you. i just want to close this out while i'm still fresh with it. and it's a pretty serious gap for a validator. thanks. |
| const isBICReg = /^[A-z]{4}[A-z]{2}\w{2}(\w{3})?$/; | ||
| // https://en.wikipedia.org/wiki/ISO_9362 | ||
| const isBICReg = /^[A-Za-z]{6}[A-Za-z0-9]{2}([A-Za-z0-9]{3})?$/; |
There was a problem hiding this comment.
NOTE: This is not part of the changes request. I am just offering suggestions and we can do it in a separate PR. I can also work on it after this PR get merged.
Great work here.
I like the simplicity, but I could offer an alternative based on fact that the second part is country code addition i.e:
- Bank code (A-Z) : 4 letter code.
Country code (A-Z) : 2 letter code.
- Location Code (0-9 or A-Z) : 2 digit code – either letters or numbers.
- Branch Code (0-9 or A-Z) : optional 3 digit code – either letters or numbers*.
-> We could leverage the array in validISO31661Alpha2CountriesCodes for making this a robust validator.
NB: [Editing code based on comments]
// Making validISO31661Alpha2CountriesCodes exportable or just add to new util fileimport{validISO31661Alpha2CountriesCodes}from'./isISO31661Alpha2';//... other codeexportdefaultfunctionisBIC(str){assertString(str);constcountryCode=str.slice(4,6);if(validISO31661Alpha2CountriesCodes.indexOf(countryCode)===-1||countryCode.toUpperCase!=='XK'// for Republic of Kosovo){returnfalse;}returnisBICReg.test(str);}CC. @tux-tn and @profnandaa
There was a problem hiding this comment.
i'm happy to make a change doing this - i think it's a good idea. i would just return false without executing the isBICReg.test(str) if it's not a country code - no reason to execute the test if we already know the answer.
it's not clear to me why using includes is useful though; is there a compatibility issue somewhere that i'm not aware of? this seems like it should be a straight, simple use of indexOf() - some has to call a function each time but even then why replace some() with includes() to does the exact same thing but with an extra layer of a function call.
There was a problem hiding this comment.
That is great. And I am happy that you are willing to make these changes.
First, I would like to say that, the code above was a sample and required more modifications.
- If it not country code, please do that return as you have mentioned
- Take the array of
validISO31661Alpha2CountriesCodesfrom isISO31661Alpha2 and create a new file insidelib/util. Import for both isISO31661Alpha2 and BIC validators. - You can make use of
indexOf()instead ofincludes()andsome()
I am happy to review that and give a go-ahead to merge this PR.
Thanks again for your contributions @bmacnaughton.
There was a problem hiding this comment.
i think this is ready to be merged. it made sense to check that the bank ID was valid.
bmacnaughton
commented
Mar 24, 2021
@tux-tn@profnandaa - any chance of getting this reviewed and merged? the core problem that is fixed is pretty serious. |
tux-tn
commented
Mar 25, 2021
@bmacnaughton sorry can't help on this. Both ezkemboi and i already approved your PR but you still need to wait for profnandaa in order to merge it. He will probably do it when he has time |
ezkemboi
commented
Mar 25, 2021
cc @profnandaa |
profnandaa
left a comment
There was a problem hiding this comment.
Thanks for the good catch; my assumption had been that [A-z] == [A-Za-z]; now just learnt that A-z just meant all ASCII codes for 65 to 122, which include the in between none-alphas (91 - 96)!
A number of files contain ranges of [A-z] which allows the characters
[\]^_\and the back-tick character. Those have been replaced by [A-Za-z].