Agregar resource id para sessions create by eduardo-garcia18 · Pull Request #428 · cuenca-mx/cuenca-python
Summary by CodeRabbit
-
New Features
- Sessions now accept an optional resource_id; it appears in session responses and schema examples.
-
Chores
- Bumped package version to 2.1.11.
- Updated dependency: cuenca-validations to 2.1.17.dev4.
-
Tests
- Added tests and recorded fixtures for session creation with resource_id, authorized access, and unauthorized access.
Walkthrough
Adds an optional resource_id field to the Session model and propagates it through creation: Session now defines resource_id: Optional[str] = None, Session.create accepts resource_id and forwards it into the SessionRequest payload and serialized output. Updates the public JSON schema example for Session. Bumps package version to 2.1.11 and updates cuenca-validations in requirements. Adds VCR cassettes and pytest fixtures/tests to exercise session creation with resource_id and subsequent authorized/unauthorized resource access flows.
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~20 minutes
- Moderate logic change: new field and parameter propagated through model and factory method.
- Tests, fixtures, and multiple VCR cassettes added — increases review surface.
- Version and dependency bumps are straightforward.
- Heterogeneous changes across models, tests, and fixtures require per-file validation.
Suggested reviewers
- alexviquez
- rogelioLpz
- felipao-mx
📜 Recent review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/conftest.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/conftest.py
Pre-merge checks (2 passed, 1 warning)
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | ⚠️ Warning | Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. | You can run @coderabbitai generate docstrings to improve docstring coverage. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title Check | ✅ Passed | The title succinctly describes the key change by indicating that a resource identifier is being added to the session creation functionality, which directly reflects the pull request’s primary modification to the Session.create method. It is concise and clearly tied to the main feature without extraneous detail. |
✨ Finishing Touches
- 📝 Generate Docstrings
🧪 Generate unit tests
- Create PR with unit tests
- Post copyable unit tests in a comment
- Commit unit tests in branch
feat/eduardo-garcia18/AddMetamapSession
Comment @coderabbitai help to get the list of available commands and usage tips.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
| ) | ||
|
|
||
| assert session.user_id == 'USPR4JxMuwSG60u2h4gBpB6Q' | ||
| assert session.resource_id == '68b887f60c33abad1ea841d3' |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters