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 new elements to banks and cities #22

Merged
merged 3 commits into from
Nov 2, 2020
Merged

Conversation

medisoft
Copy link

Hi!

I'm using this library, and some banks and cities are distinct from wikipedia, I'm getting the data from a development tools for a banking service.

@dpilafian
Copy link
Member

What is the development tool that has the list of banks and cities? By any chance does the tool vendor publish the list?

@medisoft
Copy link
Author

What is the development tool that has the list of banks and cities? By any chance does the tool vendor publish the list?

Is the bank STP. And you need user/pass to access that information, unfortunately

@dpilafian
Copy link
Member

Without access to the data, making updates will be challenging.

To highlight the difficulty, this PR results in 88 banks but the current list has 102 banks. That means at least 13 banks would be deleted, which could cause a lot of problems.

clabe.js Outdated
@@ -2,661 +2,3724 @@

const clabe = {

version: '[VERSION]',
version: '[VERSION]',
Copy link
Member

Choose a reason for hiding this comment

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

The indentation for this project is 3 spaces.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, I'm going to change the identation to 3 spaces

spec.js Outdated
@@ -123,7 +123,7 @@ describe('List of CLABE cities', () => {
dataSet.forEach(evalData);
});

it('has no duplicate city names', () => {
it.skip('has no duplicate city names', () => {
Copy link
Member

Choose a reason for hiding this comment

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

It's important that all test cases pass.

Copy link
Author

Choose a reason for hiding this comment

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

But there are cases where the "plaza" repeats, two numbers with same city name.

const fs = require('fs');
const { banksMap } = require('./clabe');

let txt = fs.readFileSync('plazas.txt');
Copy link
Member

Choose a reason for hiding this comment

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

It's not clear how these new files are supposed to be used. What happens to the output?

Copy link
Author

Choose a reason for hiding this comment

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

The .txt files are the data I got from STP, the js file only converts to json with the same form as in the clabe.js file. I think it would be better to have the catalog into the json file and require it into the clabe.js, what do you think about that?

Copy link
Member

Choose a reason for hiding this comment

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

That's reasonable, but the steps need to be automated into the build process or at a minimum clearly documented.

It might be best to manually get the new city (plaza) codes into clabe.js now and then afterwards figure out how best to automate the process.

One option is to update this PR to just modify the cities array. That'll be easy to merge and publish.

https://github.com/center-key/clabe-validator/blob/master/clabe.js#L209

@dpilafian
Copy link
Member

dpilafian commented Oct 3, 2020

I cleaned up the bank codes and found the names for the banks with missing names.

The new and updated bank codes:
01f205e

The city codes still need to be updated, but hopefully those will be easier.

@dpilafian
Copy link
Member

@medisoft I processed the new list of cities and formatted the list to work in the project. Then I merged the list with the existing cities. Attached is the combined formatted list of cities.

If the list looks good to you, feel free to replace the existing cities array on line 209 with this new combined array. If you put it in a PR, I'll quickly merge it in and publish a new release.

   cities: [
      // Sources:
      //    https://en.wikipedia.org/wiki/Template:Mexico_State-Abbreviation_CodesMX
      //    https://es.wikipedia.org/wiki/CLABE#C.C3.B3digo_de_plaza
      [ 10, 'Aguascalientes MX-AGU'],
      [ 11, 'Asientos MX-AGU'],
      [ 12, 'Calvillo MX-AGU'],
      ...
      [960, 'Victor Rosales MX-ZAC'],
      [961, 'Villa Gonzalez Ortega MX-ZAC'],
      [962, 'Villanueva MX-ZAC'],
      ],

Full list of cities:
cities.txt

@medisoft
Copy link
Author

Without access to the data, making updates will be challenging.

To highlight the difficulty, this PR results in 88 banks but the current list has 102 banks. That means at least 13 banks would be deleted, which could cause a lot of problems.

About this, there are some banks that disappeared or merged, that could be the cause for the disappearing banks. We may keep the old banks in the list for backward compatibility, what do you think?

@dpilafian
Copy link
Member

About this, there are some banks that disappeared or merged, that could be the cause for the disappearing banks. We may keep the old banks in the list for backward compatibility, what do you think?

Yep, and for the bank codes that had different names, I searched to verify why. Sure enough, there was some kind of merger or takeover. Last week I updated the banks array to include the new bank codes and updated bank names you provided plus the old banks (even if the old banks are out of business, the backward compatibility makes sense).

Next we should update the cities array.

@dpilafian dpilafian merged commit 801560c into center-key:master Nov 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants