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 27 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
7 changes: 7 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -57,3 +57,10 @@ Para generar nuevo válido CLABES
import clabe
clabe.generate_new_clabes(10, '002123456')
```

Para generar un nuevo banco

```python
import clabe
clabe.configure_additional_bank('777', '713', 'New Bank')
```
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',
'configure_additional_bank',
'remove_bank',
]

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

from pydantic import BaseModel, Field, validator

from .banks import BANK_NAMES, BANKS

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


class BankConfigRequest(BaseModel):
bank_name: str = Field(
min_length=1,
strip_whitespace=True,
description="The name of the bank - cannot be empty",
)

bank_code_banxico: str = Field(
min_length=5, max_length=5, description="The Banxico code for the bank"
)

@validator("bank_code_banxico")
def validate_bank_code(cls, value):
if not re.fullmatch(r"\d{5}", value):
raise ValueError(
"bank_code_banxico must be a string of exactly 5 digits"
)
return value

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

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 class documentation and strengthen validation.

The class needs documentation explaining its purpose and the relationship between Banxico and ABM codes. Additionally, the bank_code_abm property makes assumptions about code derivation.

Add docstring and improve validation:

 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 from Banxico codes) are valid 3-digit codes
+    """
     bank_name: str = Field(
         min_length=1,
         strip_whitespace=True,
         description="The name of the bank - cannot be empty",
     )

     bank_code_banxico: str = Field(
         min_length=5, max_length=5, description="The Banxico code for the bank"
     )

     @validator("bank_code_banxico")
     def validate_bank_code(cls, value):
         if not re.fullmatch(r"\d{5}", value):
             raise ValueError(
                 "bank_code_banxico must be a string of exactly 5 digits"
             )
+        # Validate that the last 3 digits form a valid ABM code
+        abm_code = value[-3:]
+        if abm_code == "000":
+            raise ValueError("Invalid ABM code derived from bank_code_banxico")
         return value

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


def configure_additional_bank(bank_code_banxico: str, bank_name: str) -> None:

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:
bank_code_abm = next(
(
abm
for abm, banxico in BANKS.items()
if banxico == bank_code_banxico
),
None,
)

if bank_code_abm:
BANKS.pop(bank_code_abm)
BANK_NAMES.pop(bank_code_banxico)
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Improve remove_bank implementation.

The function needs thread safety, documentation, and could be simplified.

 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
+    """
-    bank_code_abm = next(
-        (
-            abm
-            for abm, banxico in BANKS.items()
-            if banxico == bank_code_banxico
-        ),
-        None,
-    )
+    with _bank_lock:
+        bank_code_abm = next(
+            (abm for abm, banxico in BANKS.items() if banxico == bank_code_banxico),
+            None,
+        )
 
-    if bank_code_abm:
-        BANKS.pop(bank_code_abm)
-        BANK_NAMES.pop(bank_code_banxico)
+        if bank_code_abm:
+            BANKS.pop(bank_code_abm)
+            BANK_NAMES.pop(bank_code_banxico)
📝 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
def remove_bank(bank_code_banxico: str) -> None:
bank_code_abm = next(
(
abm
for abm, banxico in BANKS.items()
if banxico == bank_code_banxico
),
None,
)
if bank_code_abm:
BANKS.pop(bank_code_abm)
BANK_NAMES.pop(bank_code_banxico)
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:
bank_code_abm = next(
(abm for abm, banxico in BANKS.items() if banxico == bank_code_banxico),
None,
)
if bank_code_abm:
BANKS.pop(bank_code_abm)
BANK_NAMES.pop(bank_code_banxico)

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'
35 changes: 35 additions & 0 deletions tests/test_clabe.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@

from clabe import (
compute_control_digit,
configure_additional_bank,
generate_new_clabes,
get_bank_name,
remove_bank,
validate_clabe,
)

Expand Down Expand Up @@ -36,3 +38,36 @@ 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_configure_additional_bank_success(abm_code, banxico_code, name):
configure_additional_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_configure_additional_bank_invalid_inputs(banxico_code, name):
with pytest.raises(ValueError):
configure_additional_bank(banxico_code, name)


def test_remove_bank():
remove_bank('40138')
with pytest.raises(ValueError):
get_bank_name('138')
Loading