Conversation
WalkthroughThe pull request introduces a new class, 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #370 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 16 16
Lines 1297 1303 +6
=========================================
+ Hits 1297 1303 +6
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: 0
🧹 Nitpick comments (2)
cuenca_validations/types/requests.py (2)
412-428: Improve error handling to preserve original exception context.The location validator implementation is good but should preserve the exception context for better debugging.
Apply this change to improve error handling:
try: lat, lon, alt = map(float, values) TypeAdapter(Latitude).validate_python(lat) TypeAdapter(Longitude).validate_python(lon) - except ValueError: - raise ValueError( + except ValueError as e: + raise ValueError( 'Invalid format. Use lat,lon,alt with float valid values.' - ) + ) from e🧰 Tools
🪛 Ruff (0.8.2)
425-427: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
407-428: Consider adding model configuration with examples.Many other models in this file include a
model_configwith examples to improve API documentation. Consider adding one for this model as well.Example:
class UserTOSAgreementRequest(BaseModel): user_id: str tos_id: str location: str + + model_config = ConfigDict( + json_schema_extra={ + 'example': { + 'user_id': 'USWqY5cvkISJOxHyEKjAKf8w', + 'tos_id': 'TS123456', + 'location': '19.4326,-99.1332,2250', + } + }, + ) @field_validator('location') @classmethod def validate_location(cls, value: str) -> str:🧰 Tools
🪛 Ruff (0.8.2)
425-427: Within an
exceptclause, raise exceptions withraise ... from errorraise ... from Noneto distinguish them from errors in exception handling(B904)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (3)
cuenca_validations/types/requests.py(2 hunks)cuenca_validations/version.py(1 hunks)tests/test_types.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cuenca_validations/version.py
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.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 dir...
**/*.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).
tests/test_types.pycuenca_validations/types/requests.py
`**/*.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...
**/*.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.
tests/test_types.pycuenca_validations/types/requests.py
🧬 Code Definitions (1)
tests/test_types.py (1)
cuenca_validations/types/requests.py (1)
UserTOSAgreementRequest(407-428)
🪛 Ruff (0.8.2)
cuenca_validations/types/requests.py
425-427: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (3)
tests/test_types.py (2)
713-718: Good implementation of test for valid location format.The test correctly validates that the
UserTOSAgreementRequestaccepts a properly formatted location string with valid latitude, longitude, and altitude values.
721-741: Well-structured parameterized test covering invalid location cases.The test comprehensively covers multiple edge cases and invalid formats:
- Out-of-bounds latitude/longitude values
- Non-numeric values
- Incorrect number of components (too few or too many)
This ensures robust validation of the location parameter.
cuenca_validations/types/requests.py (1)
407-411: LGTM - Good model structure with required fields.The class definition properly requires all necessary fields with appropriate types, making all fields mandatory which aligns with the requirements from previous feedback.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_types.py (1)
713-731: Consider adding a test for valid location formatsWhile you've thoroughly tested invalid location formats, it would be beneficial to add a complementary test that verifies valid location formats are accepted correctly.
@pytest.mark.parametrize( 'location', [ (45.0, 90.0), (0, 0), (90, 180), (-90, -180), (23.634501, -102.552784), # Mexico City coordinates ], ) def test_location_validation_valid_format(location): request = UserTOSAgreementRequest( user_id='US123', tos_id='TS123', location=location, ) assert request.location == location
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
tests/test_types.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
`**/*.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 dir...
**/*.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).
tests/test_types.py
`**/*.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...
**/*.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.
tests/test_types.py
🧬 Code Definitions (1)
tests/test_types.py (1)
cuenca_validations/types/requests.py (1)
UserTOSAgreementRequest(406-409)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: pytest (3.10)
- GitHub Check: coverage
- GitHub Check: lint
- GitHub Check: auto-merge
🔇 Additional comments (2)
tests/test_types.py (2)
46-46: Import of UserTOSAgreementRequest looks goodThe import follows the codebase's absolute import conventions for accessing models from the main module.
713-731: Note: This test was previously marked for removalThis new test validates the
locationparameter forUserTOSAgreementRequestagainst various invalid formats. While the test implementation is well-structured and comprehensive, there was a previous review comment suggesting "remover este test, ya no es necesario" (remove this test, it's no longer necessary).Can you clarify whether this test is still needed? If it is, the test provides good coverage for invalid location formats including:
- Out-of-bounds latitude (>90 or <-90)
- Out-of-bounds longitude (>180 or <-180)
- Non-numeric characters in coordinates
- Incorrect number of components in the location tuple (3 values instead of 2)
gmorales96
force-pushed
the
feature/tos-and-user-agreement
branch
from
6b7f2e7 to
deaba5c
Compare
gmorales96
force-pushed
the
feature/tos-and-user-agreement
branch
from
639a4c6 to
8b9eae6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_types.py (1)
713-732: The test cases are comprehensive but consider adding positive tests.The parameterized test function effectively checks various invalid location formats for the
UserTOSAgreementRequestclass. However, it only focuses on validation failures.Consider adding positive test cases to ensure valid coordinates are accepted properly:
@pytest.mark.parametrize( 'location', [ (1500, -99.139584), (-91, 45.1), (45, 181), (45, -181), ('abc', 45), (45, 'abc'), (45, 45, 45), ], ) def test_location_validation_invalid_format(location): with pytest.raises(ValueError): UserTOSAgreementRequest( user_id='US123', tos_id='TS123', location=location, ) +@pytest.mark.parametrize( + 'location', + [ + (19.432608, -99.133209), # Mexico City + (-33.8688, 151.2093), # Sydney + (90, 0), # North Pole (edge case) + (-90, 0), # South Pole (edge case) + (0, 180), # International Date Line (edge case) + (0, -180), # International Date Line (edge case) + ], +) +def test_location_validation_valid_format(location): + request = UserTOSAgreementRequest( + user_id='US123', + tos_id='TS123', + location=location, + ) + assert request.location.lat == location[0] + assert request.location.lng == location[1]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (4)
cuenca_validations/types/enums.py(2 hunks)cuenca_validations/types/requests.py(2 hunks)cuenca_validations/version.py(1 hunks)tests/test_types.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- cuenca_validations/version.py
- cuenca_validations/types/enums.py
- cuenca_validations/types/requests.py
🧰 Additional context used
📓 Path-based instructions (4)
`**/*.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 dir...
**/*.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).
tests/test_types.py
`**/*.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...
**/*.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.
tests/test_types.py
`**/*.py`: Use try/except for concise error handling when accessing nested dictionary keys:
try:
can_ignore_error = data['error']['code'] in ignore_error_codes
excep...</summary>
> `**/*.py`: Use try/except for concise error handling when accessing nested dictionary keys:
>
> ```python
> try:
> can_ignore_error = data['error']['code'] in ignore_error_codes
> except KeyError:
> can_ignore_error = False
> ```
>
> ❌ Avoid Verbose Chained Conditionals:
> ```python
> can_ignore_error = (
> 'code' in data['error']
> and data['error']['code'] in ignore_error_codes
> )
> ```
>
> Explanation:
> The try/except approach:
>
> Reduces code complexity and nesting
> Improves readability by focusing on the "happy path" logic
> Follows Python's "easier to ask forgiveness than permission" (EAFP) idiom
>
> Severity: Important (Not a Nitpick)
> This pattern significantly improves code maintainability and readability, especially as dictionary access patterns become more complex.
- `tests/test_types.py`
</details>
<details>
<summary>`**/*.py`: Context
Agave is our internal Python library for processing SQS messages. The @task decorator can automatically convert JSON to Pydantic models.
Rule
Always annotate @t...</summary>
> `**/*.py`: Context
> Agave is our internal Python library for processing SQS messages. The @task decorator can automatically convert JSON to Pydantic models.
>
> Rule
> Always annotate @task parameters with Pydantic models instead of manually converting dictionaries.
>
> Correct Pattern
> ```python
> from pydantic import BaseModel
> from agave.tasks.sqs_tasks import task
>
> class User(BaseModel):
> name: str
> age: int
>
> @task(queue_url=QUEUE_URL, region_name='us-east-1')
> async def task_validator(message: User) -> None:
> # The message is already a User instance - no conversion needed
> print(message.name) # Direct attribute access
> ```
>
> Incorrect Pattern
> ```python
> from pydantic import BaseModel
> from agave.tasks.sqs_tasks import task
>
> class User(BaseModel):
> name: str
> age: int
>
> @task(queue_url=QUEUE_URL, region_name='us-east-1')
> async def task_validator(message_data: dict) -> None: # or unannotated parameter
> # Unnecessary conversion
> message = User(**message_data)
> print(message.name)
> ```
>
> Explanation
> The Agave @task decorator automatically:
>
> - Reads JSON messages from SQS queues
> - Converts them to Pydantic model instances when the handler parameter is annotated
> - Performs validation based on the Pydantic model
- `tests/test_types.py`
</details>
</details>
</details>
<details>
<summary>🔇 Additional comments (3)</summary><blockquote>
<details>
<summary>tests/test_types.py (3)</summary>
`46-46`: **LGTM: Import of UserTOSAgreementRequest added correctly.**
The import is properly included in the existing import block from the requests module.
---
`727-731`: <details>
<summary>✅ Verification successful</summary>
**Ensure that the UserTOSAgreementRequest is properly tested across the codebase.**
The test verifies that invalid locations raise ValueError, which is good. Verify that there are integration tests for the actual workflow of accepting terms of service.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find any integration tests related to TOS agreement
fd -t f "test.*\.py" | xargs grep -l "TOS" | xargs cat
# Look for usage of the UserTOSAgreementRequest class in the codebase
rg "UserTOSAgreementRequest" --type py -A 5 -B 5
Length of output: 21442
try:
can_ignore_error = data['error']['code'] in ignore_error_codes
excep...</summary>
> `**/*.py`: Use try/except for concise error handling when accessing nested dictionary keys:
>
> ```python
> try:
> can_ignore_error = data['error']['code'] in ignore_error_codes
> except KeyError:
> can_ignore_error = False
> ```
>
> ❌ Avoid Verbose Chained Conditionals:
> ```python
> can_ignore_error = (
> 'code' in data['error']
> and data['error']['code'] in ignore_error_codes
> )
> ```
>
> Explanation:
> The try/except approach:
>
> Reduces code complexity and nesting
> Improves readability by focusing on the "happy path" logic
> Follows Python's "easier to ask forgiveness than permission" (EAFP) idiom
>
> Severity: Important (Not a Nitpick)
> This pattern significantly improves code maintainability and readability, especially as dictionary access patterns become more complex.
- `tests/test_types.py`
</details>
<details>
<summary>`**/*.py`: Context
Agave is our internal Python library for processing SQS messages. The @task decorator can automatically convert JSON to Pydantic models.
Rule
Always annotate @t...</summary>
> `**/*.py`: Context
> Agave is our internal Python library for processing SQS messages. The @task decorator can automatically convert JSON to Pydantic models.
>
> Rule
> Always annotate @task parameters with Pydantic models instead of manually converting dictionaries.
>
> Correct Pattern
> ```python
> from pydantic import BaseModel
> from agave.tasks.sqs_tasks import task
>
> class User(BaseModel):
> name: str
> age: int
>
> @task(queue_url=QUEUE_URL, region_name='us-east-1')
> async def task_validator(message: User) -> None:
> # The message is already a User instance - no conversion needed
> print(message.name) # Direct attribute access
> ```
>
> Incorrect Pattern
> ```python
> from pydantic import BaseModel
> from agave.tasks.sqs_tasks import task
>
> class User(BaseModel):
> name: str
> age: int
>
> @task(queue_url=QUEUE_URL, region_name='us-east-1')
> async def task_validator(message_data: dict) -> None: # or unannotated parameter
> # Unnecessary conversion
> message = User(**message_data)
> print(message.name)
> ```
>
> Explanation
> The Agave @task decorator automatically:
>
> - Reads JSON messages from SQS queues
> - Converts them to Pydantic model instances when the handler parameter is annotated
> - Performs validation based on the Pydantic model
- `tests/test_types.py`
</details>
</details>
</details>
<details>
<summary>🔇 Additional comments (3)</summary><blockquote>
<details>
<summary>tests/test_types.py (3)</summary>
`46-46`: **LGTM: Import of UserTOSAgreementRequest added correctly.**
The import is properly included in the existing import block from the requests module.
---
`727-731`: <details>
<summary>✅ Verification successful</summary>
**Ensure that the UserTOSAgreementRequest is properly tested across the codebase.**
The test verifies that invalid locations raise ValueError, which is good. Verify that there are integration tests for the actual workflow of accepting terms of service.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find any integration tests related to TOS agreement
fd -t f "test.*\.py" | xargs grep -l "TOS" | xargs cat
# Look for usage of the UserTOSAgreementRequest class in the codebase
rg "UserTOSAgreementRequest" --type py -A 5 -B 5Integration tests for TOS acceptance workflow verified
- The invalid location checks for UserTOSAgreementRequest are confirmed in
test_location_validation_invalid_format(i.e. raising a ValueError for various invalid inputs). - Additionally, the TOS workflow is exercised in the integration tests within
test_user_update_requestwhere theterms_of_servicefield (including valid IPv4 and IPv6 cases) is used as part of a full request, effectively simulating the TOS acceptance.
No further changes appear to be necessary regarding TOS testing unless additional explicit positive tests for a standalone UserTOSAgreementRequest are desired.
713-713:
Details
✅ Verification successful
The commented removal instruction is no longer applicable.
There's a comment from a previous review by felipao-mx requesting to remove a test. Since this is a new test implementation, that comment is no longer relevant.
🏁 Script executed:
#!/bin/bash
# Check if there was a previous test at the same location that was removed
git log -p -- tests/test_types.py | grep -A 5 -B 5 "def test_.*" | grep -A 5 "remove" | head -n 20Length of output: 0
Action: Confirm removal of outdated test removal comment
The new parametrized test in tests/test_types.py (line 713) no longer includes or requires the previously flagged removal instruction from felipao-mx. The investigation (via git history inspection) revealed no evidence of a removed test affecting this section, so the old removal comment is now obsolete.
2 participants
Description
This PR adds a new request model,
UserTOSAgreementRequest, to manage the acceptance of terms and services. It also updatesKYCFileTypeby adding thetostype and extendsFileExtensionwith the addition ofhtml.Summary by CodeRabbit
New Features
Chores