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: corrected the Costa Rican IBAN format #646

Merged
merged 5 commits into from
Mar 23, 2022
Merged

Conversation

MrGussio
Copy link
Contributor

Fixes #643 by correcting the Costa Rican IBAN formatting

@MrGussio MrGussio requested a review from a team as a code owner March 21, 2022 15:02
@ST-DDT ST-DDT added c: bug Something isn't working p: 1-normal Nothing urgent s: accepted Accepted feature / Confirmed bug labels Mar 21, 2022
@ST-DDT ST-DDT added this to the v6.1 - First bugfixes milestone Mar 21, 2022
@codecov
Copy link

codecov bot commented Mar 21, 2022

Codecov Report

Merging #646 (d0519e6) into main (a759c87) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head d0519e6 differs from pull request most recent head ff816b9. Consider uploading reports for the commit ff816b9 to get more accurate results

@@           Coverage Diff           @@
##             main     #646   +/-   ##
=======================================
  Coverage   99.33%   99.33%           
=======================================
  Files        1923     1923           
  Lines      176853   176857    +4     
  Branches      906      908    +2     
=======================================
+ Hits       175681   175687    +6     
+ Misses       1116     1114    -2     
  Partials       56       56           
Impacted Files Coverage Δ
src/iban.ts 100.00% <100.00%> (ø)
src/vendor/unique.ts 95.16% <0.00%> (+1.61%) ⬆️

ST-DDT
ST-DDT previously approved these changes Mar 21, 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.

Checked and seems to be working according to: https://www.iban.de/iban-pruefen.html

Copy link
Member

@Shinigami92 Shinigami92 left a comment

Choose a reason for hiding this comment

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

Don't we have a test case for that? Nothing failed...
But I think we should add a test case for that because we also have tests for other ibans

See: https://github.com/faker-js/faker/blob/main/test/finance_iban.spec.ts

@Shinigami92 Shinigami92 added the needs test More tests are needed label Mar 21, 2022
@MrGussio
Copy link
Contributor Author

Don't we have a test case for that? Nothing failed... But I think we should add a test case for that because we also have tests for other ibans

See: https://github.com/faker-js/faker/blob/main/test/finance_iban.spec.ts

I added the test for Costa Rica to the test file and made issue #660 for the other tests of missing IBAN country codes.

In the latest commit I also splitted the reserved character from the banking digits of the Costa Rican IBAN formatting, to really cohere to the definitions I have found online.

test/finance_iban.spec.ts Outdated Show resolved Hide resolved
@MrGussio MrGussio requested a review from ST-DDT March 23, 2022 07:08
@ST-DDT ST-DDT requested a review from a team March 23, 2022 08:09
@ST-DDT ST-DDT removed the needs test More tests are needed label Mar 23, 2022
@Shinigami92 Shinigami92 changed the title fix: corrected the Costa Rican IBAN format (#643) fix: corrected the Costa Rican IBAN format Mar 23, 2022
@Shinigami92 Shinigami92 enabled auto-merge (squash) March 23, 2022 08:26
@Shinigami92 Shinigami92 merged commit 3f3de78 into faker-js:main Mar 23, 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.

Wrong IBAN formatting for Costa Rica
3 participants