Agregar metamap session #386
Conversation
…thly_spending_type to UserRequest model
Walkthrough
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 📜 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)
✨ Finishing Touches
🧪 Generate unit tests
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #386 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 16 16
Lines 1391 1400 +9
=========================================
+ Hits 1391 1400 +9
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:
|
…eat/eduardo-garcia18/AddMetamapSession
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
cuenca_validations/types/requests.py (2)
550-551: Hardenresource_idtype (non-empty when provided).Use
NonEmptyStrso empty strings are rejected wheneverresource_idis present.- resource_id: Optional[str] = None + resource_id: Optional[NonEmptyStr] = None
566-571: Align JSON schema example withresource_idusage.Including
resource_idwhiletypeissession.registrationis confusing. Either switchtypetosession.metamap_verificationor dropresource_id.- 'type': 'session.registration', + 'type': 'session.metamap_verification', ... - 'resource_id': 'some_resource_id', + 'resource_id': 'some_resource_id',tests/test_requests.py (1)
61-71: Assert on the actual exception message value.Use
exception.valueto avoid relying onExceptionInfo’s repr.- assert 'Resource id expected for this session' in str(exception) + assert 'Resource id expected for this session' in str(exception.value)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cuenca_validations/types/enums.py(1 hunks)cuenca_validations/types/requests.py(1 hunks)cuenca_validations/version.py(1 hunks)tests/test_requests.py(2 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:
cuenca_validations/version.pytests/test_requests.pycuenca_validations/types/enums.pycuenca_validations/types/requests.py
🧬 Code graph analysis (2)
tests/test_requests.py (2)
cuenca_validations/types/enums.py (1)
SessionType(584-591)cuenca_validations/types/requests.py (1)
SessionRequest(545-573)
cuenca_validations/types/requests.py (1)
cuenca_validations/types/enums.py (1)
SessionType(584-591)
🪛 Ruff (0.12.2)
cuenca_validations/types/requests.py
560-560: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (5)
cuenca_validations/version.py (1)
1-1: Version bump looks good.No issues.
cuenca_validations/types/enums.py (1)
591-591: New session type added correctly.Enum member name and value are consistent with existing conventions.
tests/test_requests.py (3)
4-8: Imports are appropriate.Absolute imports from the package are fine in tests.
38-46: Good baseline test.Covers non-metamap session without extra fields.
48-59: Happy-path test looks good.Validates mapping and presence of
resource_id.
cuenca_validations/types/enums.py
Outdated
| account_entries = 'session.account_entries' | ||
| download_file = 'session.download_file' | ||
| upload_file = 'session.upload_file' | ||
| metamap_verification = 'session.metamap_verification' |
There was a problem hiding this comment.
Renombrar a onboarding_verification para que haga mas sentido con el recurso
There was a problem hiding this comment.
no tiene sentido, porque renombrar a onboarding si no se hace el onboarding con la sesión, simplemente es una sesión para poder consultar una verificación de metamap?
There was a problem hiding this comment.
El recurso se llama onboarding_verifications 🤡
cuenca_validations/types/requests.py
Outdated
| if values[ | ||
| 'type' | ||
| ] == SessionType.metamap_verification and not values.get( | ||
| 'resource_id' | ||
| ): | ||
| raise ValueError('Resource id expected for this session') |
There was a problem hiding this comment.
Por legibilidad sugiero ponerlo así
def validate_metadata(cls, values: dict) -> dict:
type, resource_id = values['type'], values.get('resource_id')
if type == SessionType.onboarding_verification and not resource_id:
raise ValueError('Resource id expected for this session')
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
cuenca_validations/types/requests.py (1)
550-559: Fix enum/str comparison in before-validator; avoid KeyError; don’t shadow built-intype.In a
mode='before'validator,values['type']is likely a raw str. Comparing it toSessionType.onboarding_verificationwill silently fail; also direct indexing can raiseKeyError. Normalize toSessionType, guard missing/invalid input, and prefer a non-empty type forresource_id. This keeps existing error text (to avoid breaking tests).Apply:
- resource_id: Optional[str] = None + resource_id: Optional[NonEmptyStr] = None @@ - def validate_metadata(cls, values: dict) -> dict: - type, resource_id = values['type'], values.get('resource_id') - if type == SessionType.onboarding_verification and not resource_id: - raise ValueError('Resource id expected for this session') - return values + def validate_metadata(cls, values: DictStrAny) -> DictStrAny: + # In 'before' validators, 'type' is often a raw string; normalize to Enum + session_type_raw = values.get('type') + if session_type_raw is None: + return values # let field validation handle missing/invalid type + try: + session_type = ( + session_type_raw + if isinstance(session_type_raw, SessionType) + else SessionType(session_type_raw) + ) + except Exception: + # Defer invalid enum value errors to field validation + return values + if ( + session_type is SessionType.onboarding_verification + and not values.get('resource_id') + ): + raise ValueError('Resource id expected for this session') + return values
🧹 Nitpick comments (1)
cuenca_validations/types/requests.py (1)
560-569: Align example withresource_idusage.The example shows
resource_idbuttypeissession.registration. Suggest switching the example type to the flow that requiresresource_idfor clarity.- 'type': 'session.registration', + 'type': 'session.onboarding_verification',
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cuenca_validations/types/enums.py(1 hunks)cuenca_validations/types/requests.py(1 hunks)cuenca_validations/version.py(1 hunks)tests/test_requests.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- cuenca_validations/types/enums.py
- tests/test_requests.py
- cuenca_validations/version.py
🧰 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:
cuenca_validations/types/requests.py
🧬 Code graph analysis (1)
cuenca_validations/types/requests.py (1)
cuenca_validations/types/enums.py (1)
SessionType(584-591)
🪛 Ruff (0.12.2)
cuenca_validations/types/requests.py
557-557: Avoid specifying long messages outside the exception class
(TRY003)
|
@eduardo-garcia18 revisa el stack trace del PR, pareciera que se están incluyendo commits de @gmorales96 |
Summary by CodeRabbit
New Features
Tests
Chores