Skip to content

Conversation

@rogelioLpz
Copy link
Member

@rogelioLpz rogelioLpz commented Oct 16, 2025

Summary by CodeRabbit

  • New Features

    • Users can now update phone number and email via profile update requests.
    • Request validation now enforces that at least one updatable field is provided.
  • Tests

    • Added tests covering validation: missing-parameter rejection and disallowing unknown extra parameters.

@rogelioLpz rogelioLpz self-assigned this Oct 16, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 16, 2025

Walkthrough

This pull request adds two optional fields to UserUpdateRequest: phone_number: Optional[PhoneNumber] and email_address: Optional[EmailStr] in cuenca_validations/types/requests.py. It also introduces a before-model validator check_at_least_one_param that raises a ValueError if no parameters are provided. The package version constant __version__ is updated from '2.1.19' to '2.1.20'. Tests were added to assert that an empty UserUpdateRequest raises the expected error and that unknown extra params are rejected.

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

  • felipao-mx
  • gmorales96

Pre-merge checks and finishing touches

❌ 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 pull request title "Update phone and email internal" is partially related to the changeset. It does reference real aspects of the changes — specifically the addition of phone_number and email_address fields to the UserUpdateRequest class. However, the title lacks clarity and specificity about what is being done. The word "internal" is ambiguous and doesn't clearly convey whether these fields are being added, modified, or refactored. A teammate scanning the history would recognize that phone and email changes are involved but would need to review the details to understand the full scope of the change. The title meets the criteria for partial relatedness but could be more descriptive.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch update-fields

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 87a578f and 8c472ed.

📒 Files selected for processing (2)
  • cuenca_validations/types/requests.py (1 hunks)
  • cuenca_validations/version.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • cuenca_validations/version.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py

⚙️ CodeRabbit configuration file

**/*.py: Enforce Relative Imports for Internal Modules

Ensure 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:

  1. 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
  2. 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).
  3. If a module is outside the main module directories (for example, in /tests, /scripts, or any similarly placed directory), absolute imports are valid.
  4. External (third-party) libraries should be imported absolutely (e.g., import requests).

**/*.py:
Rule: Enforce Snake Case in Python Backend

  1. New or Modified Code: Use snake_case for all variables, functions, methods, and class attributes.
  2. 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 camelCase

Valid:

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.subtitle

Any direct use of camelCase in new or updated code outside of these exceptions should be flagged.

`*...

Files:

  • cuenca_validations/types/requests.py
🪛 Ruff (0.14.0)
cuenca_validations/types/requests.py

529-529: Avoid specifying long messages outside the exception class

(TRY003)

🔇 Additional comments (1)
cuenca_validations/types/requests.py (1)

525-530: LGTM! Validator pattern is consistent with existing code.

The check_at_least_one_param validator correctly ensures that update requests contain at least one field. This pattern is consistent with similar validators in this file (e.g., UserListsRequest.check_request at lines 692-716).

You may optionally verify that the business logic correctly handles the interaction between the new direct phone_number and email_address fields with the existing phone_verification_id and email_verification_id fields (lines 504-505).


Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov
Copy link

codecov bot commented Oct 16, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (237aec0) to head (8c472ed).
⚠️ Report is 1 commits behind head on main.

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     
Flag Coverage Δ
unittests 100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
cuenca_validations/types/requests.py 100.00% <100.00%> (ø)
cuenca_validations/version.py 100.00% <100.00%> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 237aec0...8c472ed. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

📥 Commits

Reviewing files that changed from the base of the PR and between 237aec0 and 87a578f.

📒 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 Modules

Ensure 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:

  1. 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
  2. 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).
  3. If a module is outside the main module directories (for example, in /tests, /scripts, or any similarly placed directory), absolute imports are valid.
  4. External (third-party) libraries should be imported absolutely (e.g., import requests).

**/*.py:
Rule: Enforce Snake Case in Python Backend

  1. New or Modified Code: Use snake_case for all variables, functions, methods, and class attributes.
  2. 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 camelCase

Valid:

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.subtitle

Any direct use of camelCase in new or updated code outside of these exceptions should be flagged.

`*...

Files:

  • cuenca_validations/types/requests.py
  • cuenca_validations/version.py
  • tests/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_param validator 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:

  1. The new check_at_least_one_param validator ensures at least one parameter is provided
  2. The extra="forbid" configuration from BaseRequest rejects unknown parameters

Both tests are well-structured with clear assertions.

@rogelioLpz rogelioLpz merged commit 5ce80db into main Oct 17, 2025
21 checks passed
@rogelioLpz rogelioLpz deleted the update-fields branch October 17, 2025 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants