Conversation
WalkthroughAdds a top-level constant DOCS_WITH_BACK listing KYC document types that require a back-side image (ine, dni, residency, matricula_consular). Introduces a Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Files/areas to inspect:
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #394 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 16 16
Lines 1409 1417 +8
=========================================
+ Hits 1409 1417 +8
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/test_requests.py (1)
49-52: Test correctly asserts missinguri_back; consider covering all doc types requiring back.This test covers the
inecase well, butDOCS_WITH_BACKalso includesdni,residency, andmatricula_consular. Parametrizing over those types would better lock in the contract.- def test_update_user_update_govt(): - with pytest.raises(ValueError) as ex: - UserUpdateRequest(govt_id={'type': 'ine', 'uri_front': 'files/123'}) - assert 'uri_back must be provided for type ine' in str(ex.value) +@pytest.mark.parametrize( + 'doc_type', ['ine', 'dni', 'residency', 'matricula_consular'] +) +def test_update_user_govt_requires_uri_back(doc_type: str) -> None: + with pytest.raises(ValueError) as ex: + UserUpdateRequest(govt_id={'type': doc_type, 'uri_front': 'files/123'}) + assert ( + f'uri_back must be provided for type {doc_type}' in str(ex.value) + )cuenca_validations/types/requests.py (2)
97-102: DOCS_WITH_BACK definition is clear; consider typing and immutability.The constant accurately captures document types needing a back image; you could make it more explicit and immutable by adding a type annotation and switching to a tuple.
-DOCS_WITH_BACK = [ - KYCFileType.ine, - KYCFileType.dni, - KYCFileType.residency, - KYCFileType.matricula_consular, -] +DOCS_WITH_BACK: tuple[KYCFileType, ...] = ( + KYCFileType.ine, + KYCFileType.dni, + KYCFileType.residency, + KYCFileType.matricula_consular, +)
555-561: govt_id validator enforcesuri_backcorrectly; minor typing tweak possible.The logic matches the requirement (back image required for the listed types) and error messaging aligns with the test; you can tighten typing and avoid relying on truthiness for a slightly clearer validator.
- @field_validator('govt_id') - @classmethod - def validate_govt_id(cls, govt_id: KYCFile): - if govt_id and govt_id.type in DOCS_WITH_BACK and not govt_id.uri_back: - error = f'uri_back must be provided for type {govt_id.type.value}' - raise ValueError(error) - return govt_id + @field_validator('govt_id') + @classmethod + def validate_govt_id( + cls, govt_id: Optional[KYCFile] + ) -> Optional[KYCFile]: + if ( + govt_id is not None + and govt_id.type in DOCS_WITH_BACK + and not govt_id.uri_back + ): + error = f'uri_back must be provided for type {govt_id.type.value}' + raise ValueError(error) + return govt_id
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cuenca_validations/types/requests.py(2 hunks)cuenca_validations/version.py(1 hunks)tests/test_requests.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: Enforce Relative Imports for Internal ModulesEnsure that any imports referencing internal modules use relative paths. However, if modules reside in the main module directories (for example /src or /library_or_app_name) —and relative imports are not feasible—absolute imports are acceptable. Additionally, if a module is located outside the main module structure (for example, in /tests or /scripts at a similar level), absolute imports are also valid.
Examples and Guidelines:
- If a module is in the same folder or a subfolder of the current file, use relative imports. For instance: from .some_module import SomeClass
- If the module is located under /src or /library_or_app_name and cannot be imported relatively, absolute imports are allowed (e.g., from library_or_app_name.utilities import helper_method).
- If a module is outside the main module directories (for example, in /tests, /scripts, or any similarly placed directory), absolute imports are valid.
- External (third-party) libraries should be imported absolutely (e.g., import requests).
**/*.py:
Rule: Enforce Snake Case in Python Backend
- New or Modified Code: Use snake_case for all variables, functions, methods, and class attributes.
- Exceptions (Pydantic models for API responses):
- Primary fields must be snake_case.
- If older clients expect camelCase, create a computed or alias field that references the snake_case field.
- Mark any camelCase fields as deprecated or transitional.
Examples
Invalid:
class CardConfiguration(BaseModel): title: str subTitle: str # ❌ Modified or new field in camelCaseValid:
class CardConfiguration(BaseModel): title: str subtitle: str # ✅ snake_case for new/modified field @computed_field def subTitle(self) -> str: # camelCase allowed only for compatibility return self.subtitleAny direct use of camelCase in new or updated code outside of these exceptions should be flagged.
`*...
Files:
tests/test_requests.pycuenca_validations/types/requests.pycuenca_validations/version.py
🧬 Code graph analysis (2)
tests/test_requests.py (1)
cuenca_validations/types/requests.py (1)
UserUpdateRequest(509-561)
cuenca_validations/types/requests.py (2)
cuenca_validations/types/enums.py (1)
KYCFileType(241-251)cuenca_validations/types/identities.py (1)
KYCFile(138-175)
🔇 Additional comments (1)
cuenca_validations/version.py (1)
1-1: Version bump is consistent with the new validation; confirm intended impact level.Stricter validation on
govt_id.uri_backcan cause previously accepted requests to fail, so treating this as a patch (2.1.22→2.1.23) is fine only if this is considered a bugfix rather than a behavior change for clients—please confirm and ensure any changelog/release notes call this out.
gmorales96
left a comment
gmorales96
left a comment
There was a problem hiding this comment.
Solo un par de type hints
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
cuenca_validations/types/requests.py (1)
97-102: Usefrozensetfor more efficient membership checks.Since
DOCS_WITH_BACKis used for membership checks (line 558), using afrozensetprovides O(1) lookup instead of O(n) for lists, and conveys immutability.Apply this diff:
-DOCS_WITH_BACK = [ +DOCS_WITH_BACK = frozenset([ KYCFileType.ine, KYCFileType.dni, KYCFileType.residency, KYCFileType.matricula_consular, -] +])tests/test_requests.py (1)
49-52: Expand test coverage for all document types requiringuri_back.The test only validates the INE type. Consider using
@pytest.mark.parametrizeto test all document types inDOCS_WITH_BACK(INE, DNI, residency, matricula_consular), and add a positive test case where providinguri_backallows the request to succeed.Example enhancement:
@pytest.mark.parametrize( 'doc_type', ['ine', 'dni', 'residency', 'matricula_consular'] ) def test_update_user_govt_id_requires_uri_back(doc_type: str) -> None: with pytest.raises(ValueError) as ex: UserUpdateRequest( govt_id=KYCFile(type=doc_type, uri_front='files/123') ) assert f'uri_back must be provided for type {doc_type}' in str(ex.value) @pytest.mark.parametrize( 'doc_type', ['ine', 'dni', 'residency', 'matricula_consular'] ) def test_update_user_govt_id_with_uri_back_succeeds(doc_type: str) -> None: # Should not raise request = UserUpdateRequest( govt_id=KYCFile( type=doc_type, uri_front='files/123', uri_back='files/456' ) ) assert request.govt_id is not None assert request.govt_id.uri_back == 'files/456'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cuenca_validations/types/requests.py(2 hunks)tests/test_requests.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: Enforce Relative Imports for Internal ModulesEnsure that any imports referencing internal modules use relative paths. However, if modules reside in the main module directories (for example /src or /library_or_app_name) —and relative imports are not feasible—absolute imports are acceptable. Additionally, if a module is located outside the main module structure (for example, in /tests or /scripts at a similar level), absolute imports are also valid.
Examples and Guidelines:
- If a module is in the same folder or a subfolder of the current file, use relative imports. For instance: from .some_module import SomeClass
- If the module is located under /src or /library_or_app_name and cannot be imported relatively, absolute imports are allowed (e.g., from library_or_app_name.utilities import helper_method).
- If a module is outside the main module directories (for example, in /tests, /scripts, or any similarly placed directory), absolute imports are valid.
- External (third-party) libraries should be imported absolutely (e.g., import requests).
**/*.py:
Rule: Enforce Snake Case in Python Backend
- New or Modified Code: Use snake_case for all variables, functions, methods, and class attributes.
- Exceptions (Pydantic models for API responses):
- Primary fields must be snake_case.
- If older clients expect camelCase, create a computed or alias field that references the snake_case field.
- Mark any camelCase fields as deprecated or transitional.
Examples
Invalid:
class CardConfiguration(BaseModel): title: str subTitle: str # ❌ Modified or new field in camelCaseValid:
class CardConfiguration(BaseModel): title: str subtitle: str # ✅ snake_case for new/modified field @computed_field def subTitle(self) -> str: # camelCase allowed only for compatibility return self.subtitleAny direct use of camelCase in new or updated code outside of these exceptions should be flagged.
`*...
Files:
tests/test_requests.pycuenca_validations/types/requests.py
🧠 Learnings (1)
📚 Learning: 2025-07-09T00:29:18.195Z
Learnt from: gmorales96
Repo: cuenca-mx/cuenca-validations PR: 376
File: cuenca_validations/types/identities.py:86-86
Timestamp: 2025-07-09T00:29:18.195Z
Learning: The `postal_code_id` field in `AddressRequest` model in `cuenca_validations/types/identities.py` refers to a database record ID that references a postal code record, not the actual postal code value itself. Therefore, it should use standard database ID validation (like `NonEmptyStr`) rather than postal code format validation.
Applied to files:
cuenca_validations/types/requests.py
🧬 Code graph analysis (1)
cuenca_validations/types/requests.py (2)
cuenca_validations/types/enums.py (1)
KYCFileType(241-251)cuenca_validations/types/identities.py (1)
KYCFile(138-175)
🪛 GitHub Actions: test
tests/test_requests.py
[error] 51-51: mypy: Argument "govt_id" to "UserUpdateRequest" has incompatible type "dict[str, str]"; expected "KYCFile | None" [arg-type]
🔇 Additional comments (1)
cuenca_validations/types/requests.py (1)
555-561: LGTM! Validator logic is correct.The validator correctly enforces that government ID documents requiring a back side (INE, DNI, residency, matricula consular) must have
uri_backprovided. The error message is clear and specific.
3 participants
No permite actualizar el documentos de Identidad si no se incluye el frente y reverso para la INE y otros tipos
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.