-
Notifications
You must be signed in to change notification settings - Fork 0
Update phone and email internal #391
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThis pull request adds two optional fields to UserUpdateRequest: Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Changes are localized (one main model plus tests and a version bump). The new validator is simple and the field additions are repetitive; review mainly for validator placement and test coverage. Possibly related PRs
Suggested reviewers
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.py⚙️ CodeRabbit configuration file
Files:
🪛 Ruff (0.14.0)cuenca_validations/types/requests.py529-529: Avoid specifying long messages outside the exception class (TRY003) 🔇 Additional comments (1)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #391 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 16 16
Lines 1396 1404 +8
=========================================
+ Hits 1396 1404 +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:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
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/types/requests.pycuenca_validations/version.pytests/test_requests.py
🧬 Code graph analysis (1)
tests/test_requests.py (1)
cuenca_validations/types/requests.py (2)
UserTOSAgreementRequest(442-445)UserUpdateRequest(502-546)
🪛 GitHub Actions: test
cuenca_validations/types/requests.py
[error] 522-522: TypeError: unsupported operand type(s) for |: 'type' and 'NoneType' while defining UserUpdateRequest (likely due to using 'PhoneNumber | None' syntax on Python 3.9).
🪛 Ruff (0.14.0)
cuenca_validations/types/requests.py
529-529: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (4)
cuenca_validations/version.py (1)
1-1: LGTM!Version bump to a dev release is appropriate for this feature addition.
cuenca_validations/types/requests.py (1)
525-530: Validator logic looks good.The
check_at_least_one_paramvalidator correctly enforces that at least one parameter must be provided when creating an update request. This pattern is consistent with other validators in the class.Note: Ruff flagged the error message as a long message outside the exception class (TRY003), but this pattern is used consistently throughout the codebase (e.g., lines 187, 494, 538), so addressing it here would be inconsistent unless the entire codebase adopts a different pattern.
tests/test_requests.py (2)
4-7: LGTM!The import statement is correctly updated to include
UserUpdateRequest.
37-46: Excellent test coverage!The two test cases comprehensively validate:
- The new
check_at_least_one_paramvalidator ensures at least one parameter is provided- The
extra="forbid"configuration fromBaseRequestrejects unknown parametersBoth tests are well-structured with clear assertions.
Summary by CodeRabbit
New Features
Tests