Bugfix/release issues #429
Conversation
Walkthrough
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (1 passed, 1 warning, 1 inconclusive)❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 #429 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 54 54
Lines 1185 1185
=========================================
Hits 1185 1185
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cuenca/resources/users.py (2)
173-191: Normalize beneficiaries and handle deprecated curp_document before building request.
Implement conversion and deprecation handling to avoid breaking callers.Apply:
- request = UserUpdateRequest( + # Back-compat: normalize beneficiaries + normalized_beneficiaries = None + if beneficiaries is not None: + normalized_beneficiaries = [] + for b in beneficiaries: + if isinstance(b, BeneficiaryRequest): + normalized_beneficiaries.append(b) + elif isinstance(b, Beneficiary): + normalized_beneficiaries.append( + BeneficiaryRequest(**b.model_dump()) + ) + else: + raise TypeError( + 'beneficiaries items must be BeneficiaryRequest or Beneficiary' + ) + + # Back-compat: deprecated alias + if curp_document_uri is None and curp_document is not None: + warnings.warn( + 'curp_document is deprecated; use curp_document_uri', + DeprecationWarning, + stacklevel=2, + ) + curp_document_uri = curp_document + + request = UserUpdateRequest( profession=profession, address=address, - beneficiaries=beneficiaries, + beneficiaries=normalized_beneficiaries, govt_id=govt_id, proof_of_address=proof_of_address, proof_of_life=proof_of_life, status=status, email_verification_id=email_verification_id, phone_verification_id=phone_verification_id, curp_document_uri=curp_document_uri, fiscal_regime_code=fiscal_regime_code, fiscal_address=fiscal_address, account_use_type=account_use_type, monthly_spending_type=monthly_spending_type, monthly_movements_type=monthly_movements_type, income_type=income_type, pronouns=pronouns, )Add imports outside this hunk:
# at top of file from typing import Union import warnings
192-192: Avoid sending nulls on partial updates.
Use exclude_none to prevent unintended field nulling.Apply:
- return cls._update(id=user_id, **request.model_dump(), session=session) + return cls._update( + id=user_id, **request.model_dump(exclude_none=True), session=session + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cuenca/resources/user_events.py(1 hunks)cuenca/resources/users.py(5 hunks)cuenca/version.py(1 hunks)requirements.txt(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:
cuenca/resources/user_events.pycuenca/version.pycuenca/resources/users.py
🧬 Code graph analysis (1)
cuenca/resources/user_events.py (1)
cuenca/resources/users.py (1)
User(38-196)
🔇 Additional comments (7)
requirements.txt (1)
2-2: Bump looks fine — local checks pass; verify cuenca-validations exports
- No Pydantic v1
.schema()calls found in the repo.- No
curp_documentreferences found.User.update(...)appears only in tests: tests/resources/test_sessions.py:39 and tests/resources/test_users.py:37, 81, 106.- Confirm cuenca-validations==2.1.17 exposes BeneficiaryRequest and
curp_document_uri(check PyPI/GitHub or pip install & inspect).cuenca/version.py (1)
1-1: Version bump vs. potential breaking changes.
Renamingcurp_document→curp_document_uriand changingbeneficiariestype can break keyword-arg callers. Either add a deprecation shim (preferred) or bump version accordingly.cuenca/resources/user_events.py (1)
24-24: Correct Pydantic v2 API usage.
Switch toUser.model_json_schema()is appropriate here.cuenca/resources/users.py (4)
8-9: Importing BeneficiaryRequest is consistent with write-model usage.
No issues; proceed.
105-107: Good migration to model_json_schema() for examples.
This aligns with Pydantic v2. Verify that cuenca-validations models still expose the 'example' key (not only 'examples').Also applies to: 109-111, 114-114
145-145: Forwarding income_type in create() is correct.
Matches field presence on the model and request type.
150-172: Public API change — update docs/changelog & add deprecation/compatibility
- Signature changed (curp_document -> curp_document_uri + new params). Update docs/changelog and call out the breaking change.
- Internal call sites: tests/resources/test_users.py:37, 81, 106 (beneficiaries passed via **request around line ~89).
- No repo usages of curp_document found; definition lives in cuenca/resources/users.py (~lines 150–184).
- Action: either accept both names (map curp_document -> curp_document_uri) for a deprecation period or add a clear migration note in the release docs/changelog.
| profession: Optional[str] = None, | ||
| address: Optional[AddressRequest] = None, | ||
| beneficiaries: Optional[list[Beneficiary]] = None, | ||
| beneficiaries: Optional[list[BeneficiaryRequest]] = None, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Beneficiaries type change is a breaking API for callers. Add a compat path.
Accept both BeneficiaryRequest and Beneficiary, coercing Beneficiary → BeneficiaryRequest, to avoid breaking existing code.
Apply:
- beneficiaries: Optional[list[BeneficiaryRequest]] = None,
+ beneficiaries: Optional[list[Union[BeneficiaryRequest, Beneficiary]]] = None,And normalize before building the request (see insertion below at Lines 173-173).
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In cuenca/resources/users.py around line 155 (and normalize before building the
request at ~173): the function signature changed beneficiaries to
Optional[list[BeneficiaryRequest]] which is a breaking change; accept both
BeneficiaryRequest and Beneficiary by allowing beneficiaries to be
List[Union[BeneficiaryRequest, Beneficiary]] (or keep type as list and handle
runtime) and add a short compat path that iterates over beneficiaries, detects
instances of Beneficiary and converts/coerces them into BeneficiaryRequest
objects (mapping fields appropriately), then replace the list with the coerced
list before the request payload is built at line ~173 so callers using the old
Beneficiary type keep working.
| email_verification_id: Optional[str] = None, | ||
| phone_verification_id: Optional[str] = None, | ||
| curp_document: Optional[SerializableHttpUrl] = None, | ||
| curp_document_uri: Optional[SerializableHttpUrl] = None, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Rename to curp_document_uri: preserve backward compatibility.
Add deprecated alias curp_document and coalesce to curp_document_uri with a warning.
Apply:
- curp_document_uri: Optional[SerializableHttpUrl] = None,
+ curp_document_uri: Optional[SerializableHttpUrl] = None,
+ curp_document: Optional[SerializableHttpUrl] = None, # deprecatedThen coalesce before creating the request (see insertion below at Lines 173-173).
Also applies to: 183-183
Summary by CodeRabbit