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(finance): update iso-3166-1 country codes for IBAN/BIC #168

Merged

Conversation

bmenant
Copy link
Contributor

@bmenant bmenant commented Jan 16, 2022

I had posted the issue #1261 on the original repository, followed by a pull request (from this branch).

To put it short: this simply update the list of ISO-3166-1 country codes [1] used for IBAN & BIC generations. As a matter of fact, some codes were nonexistent in the official specifications. Those have caused Faker.js to generate invalid IBAN and BIC codes (notably when checked against validator.js, for example).

FYI, as of today, I’m unable to run tests: Error: Cannot find module './lib'

[1] Both BIC (ISO-9362) and IBAN (ISO-13616) explicitly refers to ISO-3166-1.


It has also been suggested (here: #131) to use fake-numbers for IBAN generation. AFAIK, it uses a restricted list of country codes for IBAN generation (which I believe to be just fine). However, for BIC generation, fake-numbers uses a random string of 2 letters as a country code (which are unlikely to ever produce a valid BIC code IMHO).

@netlify
Copy link

netlify bot commented Jan 16, 2022

✔️ Deploy Preview for vigilant-wescoff-04e480 ready!

🔨 Explore the source changes: c7beecf

🔍 Inspect the deploy log: https://app.netlify.com/sites/vigilant-wescoff-04e480/deploys/61e3f14654ea5200086836b3

😎 Browse the preview: https://deploy-preview-168--vigilant-wescoff-04e480.netlify.app

@Shinigami92 Shinigami92 added the p: 2-high Fix main branch label Jan 16, 2022
@Shinigami92
Copy link
Member

To tun test you have to run npm run build to generate lib content

@prisis prisis added this to the v6.1.0 milestone Jan 16, 2022
@import-brain import-brain requested a review from a team January 20, 2022 17:59
@bmenant bmenant force-pushed the fix/issue-1261-iban-bic-eu-country-code branch from c7beecf to 5ebe630 Compare January 26, 2022 02:12
@bmenant
Copy link
Contributor Author

bmenant commented Jan 26, 2022

Rebased on v6.0.0-alpha.5 and updated tests.

@bmenant bmenant removed their assignment Jan 26, 2022
src/iban.ts Outdated Show resolved Hide resolved
@bmenant bmenant removed their assignment Jan 27, 2022
@bmenant
Copy link
Contributor Author

bmenant commented Jan 27, 2022

I think it’s out of scope over here, but you might consider refactoring countries data (I suppose there are other places Faker uses these).

You might also consider adding a dependencies for those, such as unicode-org/cldr-json, annexare/Countries or node-i18n-iso-countries.

@bmenant bmenant requested a review from ST-DDT January 27, 2022 01:06
@ST-DDT
Copy link
Member

ST-DDT commented Jan 27, 2022

The ISO page seems to still be down. I will review it once it is back up again.

@hertzg
Copy link

hertzg commented Feb 2, 2022

@ST-DDT FYI ISO page seems to be back up

ST-DDT
ST-DDT previously approved these changes Feb 2, 2022
Copy link
Member

@ST-DDT ST-DDT left a comment

Choose a reason for hiding this comment

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

XK doesn't appear in the iso-list, but i found multiple references to it in some other online references, which I would consider good enough.
I also checked the code regarding the usage of the iso codes and it is only used to generate bics so it should be safe.

hertzg
hertzg previously approved these changes Feb 4, 2022
@Shinigami92 Shinigami92 added the needs rebase There is a merge conflict label Mar 10, 2022
test/finance.spec.ts Outdated Show resolved Hide resolved
@bmenant bmenant dismissed stale reviews from hertzg and ST-DDT via 272beba March 11, 2022 08:13
@bmenant bmenant force-pushed the fix/issue-1261-iban-bic-eu-country-code branch from 2aa6acf to 272beba Compare March 11, 2022 08:13
@bmenant bmenant force-pushed the fix/issue-1261-iban-bic-eu-country-code branch from 272beba to 771c70b Compare March 11, 2022 08:33
@bmenant bmenant force-pushed the fix/issue-1261-iban-bic-eu-country-code branch from 771c70b to 1c81357 Compare March 11, 2022 08:50
@bmenant
Copy link
Contributor Author

bmenant commented Mar 11, 2022

Rebased and updated tests as suggested by @Shinigami92

I had got a new failure in finance.spec.ts in a seed/bic test. I don’t know where those seeded values come from since they are hard-coded, but I had to change one of the BIC code (LT to LY). Since the order and the number of country codes has changed, it is likely that the country code selected for this particular seeding value has changed as well. Please let me know if I’m wrong.

@bmenant bmenant removed their assignment Mar 11, 2022
@Shinigami92 Shinigami92 removed the needs rebase There is a merge conflict label Mar 11, 2022
@codecov
Copy link

codecov bot commented Mar 11, 2022

Codecov Report

Merging #168 (f34c2f2) into main (af3f99e) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #168      +/-   ##
==========================================
- Coverage   99.33%   99.33%   -0.01%     
==========================================
  Files        1920     1920              
  Lines      176483   176466      -17     
  Branches      905      905              
==========================================
- Hits       175303   175286      -17     
  Misses       1124     1124              
  Partials       56       56              
Impacted Files Coverage Δ
src/iban.ts 100.00% <100.00%> (ø)

@Shinigami92 Shinigami92 requested review from ST-DDT and a team March 11, 2022 12:12
@Shinigami92
Copy link
Member

(LT to LY).

This happened because you deleted some entries from the definitions, so the indexes changed. The values are "fetched" from the definitions via arrayElement which uses datatype.number as index.
So everything is okay 👌

@bmenant
Copy link
Contributor Author

bmenant commented Mar 11, 2022

@Shinigami92 You reassigned myself, is there anything I can do so far? 🙂

@Shinigami92
Copy link
Member

Nope, we just may have another workflow then you expect
We have a big issue/pr board: https://github.com/orgs/faker-js/projects/2/views/9
And there we can see who is or has worked on something, it's mostly the same as the author of a PR

@ST-DDT ST-DDT requested a review from a team March 11, 2022 19:00
@Shinigami92 Shinigami92 added c: bug Something isn't working p: 1-normal Nothing urgent and removed p: 2-high Fix main branch labels Mar 15, 2022
@Shinigami92 Shinigami92 merged commit 6050d7a into faker-js:main Mar 21, 2022
@ST-DDT ST-DDT added the s: accepted Accepted feature / Confirmed bug label Mar 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: bug Something isn't working p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants