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 configure_additional_bank Functionality and Bump Version to 1.2.17 #169

Merged
merged 30 commits into from
Dec 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
02458a1
ci: add Codecov token to workflow configuration
Dec 10, 2024
fe79bec
feat: add error handling for existing bank codes and names
Dec 11, 2024
7ae5528
ci: update GitHub Actions workflow to use new action versions
Dec 11, 2024
e2e4cb9
chore: bump version to 1.2.17.dev1
Dec 11, 2024
2a4566e
chore: update GitHub Actions workflows for release and testing
Dec 11, 2024
b356cd1
fix: update PyPI token reference in GitHub Actions workflow
Dec 11, 2024
982d131
fix: update PyPI token reference in release workflow to use PYPI_API_…
Dec 11, 2024
5a13284
Merge branch 'chore/update-github-actions' into feature/add-editable-…
Dec 13, 2024
cf757b3
refactor: rename and update error classes for bank code validation
Dec 13, 2024
04b96f1
feat: add configure_additional_bank function and corresponding tests
Dec 13, 2024
b3fc823
docs: update README to include instructions for add a new bank
Dec 16, 2024
17056cd
chore: bump version to 1.2.17.dev2
Dec 16, 2024
63aa59d
chore: bump version to 1.2.17
Dec 16, 2024
8c0a105
chore: update GitHub Actions workflows for release and testing
Dec 11, 2024
e7d7e47
fix: update PyPI token reference in GitHub Actions workflow
Dec 11, 2024
224adf2
fix: update PyPI token reference in release workflow to use PYPI_API_…
Dec 11, 2024
7c03593
refactor: rename and update error classes for bank code validation
Dec 13, 2024
eeeaa7d
feat: add configure_additional_bank function and corresponding tests
Dec 13, 2024
bab2612
docs: update README to include instructions for add a new bank
Dec 16, 2024
4bb61cf
chore: bump version to 1.2.17.dev2
Dec 16, 2024
247f3b4
chore: bump version to 1.2.17
Dec 16, 2024
4168501
Merge branch 'feature/add-editable-banks-config' of github.com:cuenca…
Dec 16, 2024
49d22b8
Merge branch 'main' into feature/add-editable-banks-config
gmorales96 Dec 16, 2024
51856ed
feat: enhance configure_additional_bank with input validation
Dec 17, 2024
e4c5146
refactor: streamline bank configuration by replacing error classes wi…
Dec 18, 2024
4eb15b9
refactor: update BankConfigRequest model and validation logic for ban…
Dec 23, 2024
d84ee8a
feat: add remove_bank function to manage bank entries
Dec 23, 2024
acb4644
docs: update README to include instructions for adding a new bank
Dec 23, 2024
c94a35a
refactor: rename configure_additional_bank to add_bank and update val…
Dec 24, 2024
2cec6f6
docs: update README to reflect function name change from configure_ad…
Dec 24, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -57,3 +57,21 @@ Para generar nuevo válido CLABES
import clabe
clabe.generate_new_clabes(10, '002123456')
```

## Para agregar un nuevo banco

A partir de la versión 2.0.0, el paquete se actualizará a **Pydantic v2**, lo que significa que las versiones anteriores ya no recibirán soporte.

Sin embargo, hemos añadido una función para agregar bancos adicionales a la lista, en caso de que sea necesario. Esto se puede hacer sin necesidad de crear un PR. Para agregar un banco, simplemente llama a la siguiente función con el código de Banxico y el nombre del banco:

```python
import clabe
clabe.add_bank('12345', 'New Bank')
```
Comment on lines +65 to +70
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Expand the add_bank documentation

The documentation should provide more details about:

  1. The persistence of added banks (are they temporary or permanent?)
  2. Valid format requirements for bank_code and bank_name
  3. Error handling scenarios
  4. Use cases for when this functionality would be needed

Example:

 Sin embargo, hemos añadido una función para agregar bancos adicionales a la lista, en caso de que sea necesario. Esto se puede hacer sin necesidad de crear un PR. Para agregar un banco, simplemente llama a la siguiente función con el código de Banxico y el nombre del banco:

 ```python
 import clabe
 clabe.add_bank('12345', 'New Bank')

+### Consideraciones importantes
+
+- Los bancos agregados son temporales y se reiniciarán cuando se reinicie la aplicación
+- El código del banco debe ser un string de 5 dígitos
+- El nombre del banco no debe estar vacío
+
+### Manejo de errores
+
+```python
+try:

  • clabe.add_bank('12345', 'New Bank')
    +except ValueError as e:
  • print(f"Error al agregar banco: {e}")
    +```

<!-- This is an auto-generated comment by CodeRabbit -->


Para eliminar un banco

```python
import clabe
clabe.remove_bank('12345')
```
Comment on lines +72 to +77
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Document remove_bank behavior and validation

The remove_bank documentation should clarify:

  1. What happens when trying to remove a non-existent bank
  2. Whether removing a bank affects existing CLABE numbers
  3. Error handling scenarios

Example:

 Para eliminar un banco
 
 ```python
 import clabe
 clabe.remove_bank('12345')

+### Consideraciones importantes
+
+- No se pueden eliminar bancos predeterminados del sistema
+- Intentar eliminar un banco inexistente generará un error
+
+### Manejo de errores
+
+```python
+try:

  • clabe.remove_bank('12345')
    +except ValueError as e:
  • print(f"Error al eliminar banco: {e}")
    +```

<!-- This is an auto-generated comment by CodeRabbit -->

4 changes: 4 additions & 0 deletions clabe/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,18 @@
'generate_new_clabes',
'get_bank_name',
'validate_clabe',
'add_bank',
'remove_bank',
]

from .banks import BANK_NAMES, BANKS
from .types import Clabe
from .validations import (
add_bank,
compute_control_digit,
generate_new_clabes,
get_bank_name,
remove_bank,
validate_clabe,
)
from .version import __version__
53 changes: 53 additions & 0 deletions clabe/validations.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import random
from typing import List, Union

from pydantic import BaseModel, Field

from .banks import BANK_NAMES, BANKS

CLABE_LENGTH = 18
Expand Down Expand Up @@ -61,3 +63,54 @@ def generate_new_clabes(number_of_clabes: int, prefix: str) -> List[str]:
assert validate_clabe(clabe)
clabes.append(clabe)
return clabes


class BankConfigRequest(BaseModel):
"""
Validates and processes bank configuration requests.

The class handles validation of bank names and codes, ensuring:
- Bank names are non-empty strings
- Banxico codes are exactly 5 digits
"""

bank_name: str = Field(
min_length=1,
strip_whitespace=True,
description="Bank name must have at least 1 character.",
)

bank_code_banxico: str = Field(
regex=r"^\d{5}$", description="Banxico code must be a 5-digit string."
)

@property
def bank_code_abm(self):
return self.bank_code_banxico[-3:]

Comment on lines +68 to +90
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Add thread safety and validation for derived ABM code.

The class handles basic validation well using Pydantic, but there are a few improvements needed:

  1. Add validation that the derived ABM code doesn't conflict with existing codes
  2. Make the ABM code derivation rules explicit in the docstring
 class BankConfigRequest(BaseModel):
     """
     Validates and processes bank configuration requests.
 
     The class handles validation of bank names and codes, ensuring:
     - Bank names are non-empty strings
     - Banxico codes are exactly 5 digits
+    - ABM codes (derived as last 3 digits of Banxico code) don't conflict
     """
 
     bank_name: str = Field(
         min_length=1,
         strip_whitespace=True,
         description="Bank name must have at least 1 character.",
     )
 
     bank_code_banxico: str = Field(
         regex=r"^\d{5}$", description="Banxico code must be a 5-digit string."
     )
 
     @property
     def bank_code_abm(self) -> str:
+        """Derive the 3-digit ABM code from the last 3 digits of the Banxico code."""
         return self.bank_code_banxico[-3:]
+
+    def validate_abm_code(self) -> None:
+        """Validate that the derived ABM code doesn't already exist."""
+        if self.bank_code_abm in BANKS:
+            raise ValueError(f"ABM code {self.bank_code_abm} already exists")
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
class BankConfigRequest(BaseModel):
"""
Validates and processes bank configuration requests.
The class handles validation of bank names and codes, ensuring:
- Bank names are non-empty strings
- Banxico codes are exactly 5 digits
"""
bank_name: str = Field(
min_length=1,
strip_whitespace=True,
description="Bank name must have at least 1 character.",
)
bank_code_banxico: str = Field(
regex=r"^\d{5}$", description="Banxico code must be a 5-digit string."
)
@property
def bank_code_abm(self):
return self.bank_code_banxico[-3:]
class BankConfigRequest(BaseModel):
"""
Validates and processes bank configuration requests.
The class handles validation of bank names and codes, ensuring:
- Bank names are non-empty strings
- Banxico codes are exactly 5 digits
- ABM codes (derived as last 3 digits of Banxico code) don't conflict
"""
bank_name: str = Field(
min_length=1,
strip_whitespace=True,
description="Bank name must have at least 1 character.",
)
bank_code_banxico: str = Field(
regex=r"^\d{5}$", description="Banxico code must be a 5-digit string."
)
@property
def bank_code_abm(self) -> str:
"""Derive the 3-digit ABM code from the last 3 digits of the Banxico code."""
return self.bank_code_banxico[-3:]
def validate_abm_code(self) -> None:
"""Validate that the derived ABM code doesn't already exist."""
if self.bank_code_abm in BANKS:
raise ValueError(f"ABM code {self.bank_code_abm} already exists")


def add_bank(bank_code_banxico: str, bank_name: str) -> None:
"""
Add a bank configuration.

Args:
bank_code_banxico: 5-digit Banxico bank code
bank_name: Bank name
"""
request = BankConfigRequest(
bank_code_banxico=bank_code_banxico,
bank_name=bank_name,
)
BANKS[request.bank_code_abm] = request.bank_code_banxico
BANK_NAMES[request.bank_code_banxico] = request.bank_name


def remove_bank(bank_code_banxico: str) -> None:
"""
Remove a bank configuration by its Banxico code.

Args:
bank_code_banxico: 5-digit Banxico bank code
"""
BANKS.pop(bank_code_banxico[-3:], None)
BANK_NAMES.pop(bank_code_banxico, None)
Comment on lines +108 to +116
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Add thread safety for bank removal.

The function needs thread safety when modifying global state:

 def remove_bank(bank_code_banxico: str) -> None:
     """
     Remove a bank configuration by its Banxico code.
 
     Args:
         bank_code_banxico: 5-digit Banxico bank code
     """
+    with _bank_lock:
         BANKS.pop(bank_code_banxico[-3:], None)
         BANK_NAMES.pop(bank_code_banxico, None)

Committable suggestion skipped: line range outside the PR's diff.

2 changes: 1 addition & 1 deletion clabe/version.py
Original file line number Diff line number Diff line change
@@ -1 +1 @@
__version__ = '1.2.16'
__version__ = '1.2.17'
54 changes: 54 additions & 0 deletions tests/test_clabe.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,13 @@
import pytest

from clabe import (
BANK_NAMES,
BANKS,
add_bank,
compute_control_digit,
generate_new_clabes,
get_bank_name,
remove_bank,
validate_clabe,
)

Expand Down Expand Up @@ -36,3 +40,53 @@ def test_generate_new_clabes():
for clabe in clabes:
assert clabe.startswith(prefix)
assert validate_clabe(clabe)


@pytest.mark.parametrize(
'abm_code, banxico_code, name',
[
('713', '90713', 'Cuenca DMZ'),
('714', '90714', 'Cuenca Gem DMZ'),
('715', '90715', 'Cuenca Gem Beta'),
],
)
def test_add_bank_success(abm_code, banxico_code, name):
add_bank(banxico_code, name)
assert get_bank_name(abm_code) == name


@pytest.mark.parametrize(
'banxico_code, name',
[
('1234', 'Test Bank'), # invalid Banxico code 4 digits
('123456', 'Test Bank'), # invalid Banxico code 6 digits
('12345', ''), # Valid code, empty name
('123AT', 'Test Bank'), # Non-numeric codes
],
)
def test_add_bank_invalid_inputs(banxico_code, name):
with pytest.raises(ValueError):
add_bank(banxico_code, name)


def test_remove_bank():
test_code = "90716"
test_name = "To Delete Bank"
add_bank(test_code, test_name)

assert test_code[-3:] in BANKS
assert test_code in BANK_NAMES

remove_bank(test_code)

assert test_code[-3:] not in BANKS
assert test_code not in BANK_NAMES


def test_remove_nonexistent_bank():
nonexistent_code = "99999"

remove_bank(nonexistent_code)

assert nonexistent_code[-3:] not in BANKS
assert nonexistent_code not in BANK_NAMES
Loading