Enhance onboarding #426
Enhance onboarding #426
Conversation
|
""" WalkthroughThis change updates the user management API by removing several parameters ( Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #426 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 54 54
Lines 1180 1180
=========================================
Hits 1180 1180
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: 3
🧹 Nitpick comments (2)
tests/resources/cassettes/test_session_create.yaml (1)
142-146: Optional: add a positive-case forpronouns
pronounsis always recorded asnull. A dedicated test that sets and retrieves a non-null value would prove the new field is wired end-to-end and survives PATCH round-trips.tests/resources/cassettes/test_user_update.yaml (1)
3-6: Consider adding non-nullpronounsscenarioAs with the session cassette, only
nullpronouns are covered. A positive example would harden regression detection for that field.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
cuenca/resources/users.py(2 hunks)requirements.txt(1 hunks)tests/conftest.py(2 hunks)tests/resources/cassettes/test_session_create.yaml(1 hunks)tests/resources/cassettes/test_user_update.yaml(1 hunks)tests/resources/test_sessions.py(2 hunks)tests/resources/test_users.py(2 hunks)
🧰 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/resources/test_sessions.pytests/resources/test_users.pytests/conftest.pycuenca/resources/users.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
🔇 Additional comments (12)
requirements.txt (1)
2-2: Verify that using a development version is intentional.The
cuenca-validationspackage has been updated to a development pre-release version (2.1.14.dev1). Development versions can introduce instability and should typically be avoided in production environments.Please confirm this is intentional for the current development phase and ensure it's updated to a stable release before production deployment.
tests/resources/test_sessions.py (2)
2-2: LGTM!The import addition for
Professionis appropriate for the updated test logic.
39-40: LGTM!The test logic has been appropriately updated to use the
professionfield instead ofemail_address, which aligns with the API refactoring that removed email parameters from theUser.updatemethod.tests/resources/test_users.py (3)
5-5: LGTM!The import addition for
SATRegimeCodeis appropriate for the updated test logic that uses this enum.
32-32: LGTM!The user ID has been updated to align with the test data changes.
33-36: LGTM!The test changes appropriately reflect the API refactoring:
- Removal of
phone_numberandgovt_idparameters fromUser.update- Addition of
fiscal_regime_codeparameter using theSATRegimeCodeenumThese changes are consistent with the simplified user update interface.
tests/conftest.py (2)
39-40: LGTM!The fixture data updates for birth date and state are appropriate for the test data alignment.
56-60: LGTM!The fixture updates appropriately reflect the API refactoring:
- Removal of direct
phone_numberandemail_addressfields- Replacement with
phone_verification_idandemail_verification_idparameters- Updated
postal_code_idfor test data consistencyThese changes align with the simplified
User.createinterface that now uses verification IDs instead of direct contact information.cuenca/resources/users.py (3)
19-19: LGTM!The import change from
TOSRequesttoAddressRequestaligns with the updated parameter types in the create and update methods.
106-123: LGTM!The
User.createmethod has been appropriately simplified:
- Parameter type changed from
AddresstoAddressRequestfor better request modeling- Removed complex parameters that were likely causing interface bloat
- Maintains essential functionality with
curp,profession,address, and verification IDsThe simplified interface should be easier to use and maintain.
125-158: LGTM!The
User.updatemethod changes are well-structured:
- Parameter type changed from
AddresstoAddressRequestfor consistency- Added
pronounsparameter to support inclusive user profiles- Maintains all essential update functionality
- Proper integration of the new parameter into the
UserUpdateRequestThe changes enhance the API while maintaining a clean interface.
tests/resources/cassettes/test_user_update.yaml (1)
3-6: Good alignment with new update signatureThe expanded PATCH body mirrors the freshly supported fields (
fiscal_regime_code,pronouns, etc.). Looks correct.
gmorales96
changed the title
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores