Agregar resource id para sessions create#428
Conversation
WalkthroughAdds an optional Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
📜 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)
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 #428 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 54 54
Lines 1185 1186 +1
=========================================
+ Hits 1185 1186 +1
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 (6)
requirements.txt (1)
2-2: Pre-release pin: confirm necessity or switch to stable before merging to mainIf 2.1.17 (stable) is available and includes SessionRequest.resource_id, prefer pinning the stable release. Otherwise, keep the dev pin but add a short comment explaining why a dev build is required.
Check latest versions on PyPI:
#!/bin/bash curl -s https://pypi.org/pypi/cuenca-validations/json | jq '.info.version, .releases | keys[-5:]'If stable is available, apply:
- cuenca-validations==2.1.17.dev3 + cuenca-validations==2.1.17tests/resources/cassettes/test_session_create_with_resource_id.yaml (1)
13-16: Decouple cassette from client versionRecording the User-Agent ties the cassette to a specific package version and causes churn. Drop this header from the cassette (and/or add it to filter_headers in VCR config).
Apply:
- User-Agent: - - cuenca-python/2.1.11.dev1cuenca/resources/sessions.py (2)
59-59: Avoid sending nulls in create payloadInclude exclude_none to prevent sending keys with null values (success_url, failure_url, resource_id). This reduces server-side ambiguity.
- return cls._create(session=session, **req.model_dump()) + return cls._create(session=session, **req.model_dump(exclude_none=True))
23-23: Consider masking resource_id in logsIf resource_id is sensitive or correlatable, mask it like id using LogConfig. Optional, depending on logging policy.
- resource_id: Optional[str] = None + resource_id: Annotated[Optional[str], LogConfig(masked=True, unmasked_chars_length=4)] = Nonetests/resources/test_sessions.py (2)
44-46: Remove unused fixtures from the new testThey’re not referenced and slow test collection/execution.
-def test_session_create_with_resource_id( - curp_validation_request: dict, user_request: dict -) -> None: +def test_session_create_with_resource_id() -> None:
53-54: Strengthen the assertion setAlso verify the session type to fully exercise the new path.
assert session.user_id == 'USPR4JxMuwSG60u2h4gBpB6Q' assert session.resource_id == '68b887f60c33abad1ea841d3' + assert session.type == SessionType.metamap_verification
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cuenca/resources/sessions.py(4 hunks)cuenca/version.py(1 hunks)requirements.txt(1 hunks)tests/resources/cassettes/test_session_create_with_resource_id.yaml(1 hunks)tests/resources/test_sessions.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/resources/test_sessions.pycuenca/version.pycuenca/resources/sessions.py
🧬 Code graph analysis (1)
tests/resources/test_sessions.py (1)
cuenca/resources/sessions.py (2)
Session(12-59)create(42-59)
🔇 Additional comments (1)
cuenca/version.py (1)
1-1: Align client version with VCR cassette to avoid brittle testsCassette shows User-Agent: cuenca-python/2.1.11.dev1 while version is 2.1.11. Either re-record/scrub the header or align the version string.
Option 1 (align code to cassette):
-__version__ = '2.1.11' +__version__ = '2.1.11.dev1'Option 2 (preferred for release): keep 2.1.11 and remove/mask User-Agent from cassette matching/recording so tests don’t depend on the exact client version.
tests/resources/test_sessions.py
Outdated
| ) | ||
|
|
||
| assert session.user_id == 'USPR4JxMuwSG60u2h4gBpB6Q' | ||
| assert session.resource_id == '68b887f60c33abad1ea841d3' |
There was a problem hiding this comment.
Aqui hay que agregar al test que puedes usar la session para hacer la llamada de tu recurso.
Y tambien test de que con esa session no puedes hacer get de otras verificaciones
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/conftest.py (1)
19-23: Mask session tokens in VCR cassettes.Add
X-Cuenca-SessionId(and optionallyX-Cuenca-LoginToken) tofilter_headersto avoid leaking sensitive tokens in recorded fixtures.config['filter_headers'] = [ ('Authorization', 'DUMMY'), ('X-Cuenca-Token', 'DUMMY'), + ('X-Cuenca-SessionId', 'DUMMY'), + ('X-Cuenca-LoginToken', 'DUMMY'), ]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
requirements.txt(1 hunks)tests/conftest.py(2 hunks)tests/resources/cassettes/test_session_create_with_resource_id.yaml(1 hunks)tests/resources/cassettes/test_session_with_resource_id_authorized.yaml(1 hunks)tests/resources/cassettes/test_session_with_resource_id_unauthorized.yaml(1 hunks)tests/resources/test_sessions.py(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- tests/resources/cassettes/test_session_with_resource_id_authorized.yaml
- tests/resources/cassettes/test_session_with_resource_id_unauthorized.yaml
🚧 Files skipped from review as they are similar to previous changes (3)
- requirements.txt
- tests/resources/test_sessions.py
- tests/resources/cassettes/test_session_create_with_resource_id.yaml
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/conftest.py
**/conftest.py
⚙️ CodeRabbit configuration file
**/conftest.py: Always use the fastapi.testclient.TestClient as a context manager in pytest fixtures to ensure proper cleanup:@pytest.fixture def client(): from myapp.app import app with TestClient(app) as client: yield client❌ Incorrect Pattern (Flag This):
@pytest.fixture def client(): from myapp.app import app return TestClient(app) # Missing context managerSeverity: CRITICAL (Not a Nitpick)
This is a critical issue that must be fixed, not a nitpick or style suggestion. Failing to use the context manager pattern can cause:Closed event loops in tests with multiple requests to the same endpoint, breaking subsequent test execution
Files:
tests/conftest.py
🧬 Code graph analysis (1)
tests/conftest.py (2)
cuenca/resources/sessions.py (2)
Session(12-59)create(42-59)cuenca/http/client.py (2)
Session(24-134)configure(46-78)
🪛 GitHub Actions: test
tests/conftest.py
[error] 85-85: pytest failed with exit code 4. TypeError: Too few arguments for typing.Generator; actual 1, expected 3 (tests/conftest.py:85).
🔇 Additional comments (3)
tests/conftest.py (3)
10-11: Good import choices and naming.Aliasing
cuenca.http.SessiontoClientSessionand importing the resourceSessionseparately improves readability and avoids collisions.
6-6: SessionType import looks correct.Consistent with new tests using
SessionType.onboarding_verification.
84-92: Adjust fixture return annotation toIterator[Session].For pytest fixtures using
yield, preferIterator[T]or fully-parameterizedGenerator[T, None, None]to avoid runtime typing errors.-@pytest.fixture -def session_with_resource_id() -> Generator[Session]: +@pytest.fixture +def session_with_resource_id() -> Iterator[Session]:Likely an incorrect or invalid review comment.
2 participants
Summary by CodeRabbit
New Features
Chores
Tests