Feature/tos and user agreement #418
Conversation
WalkthroughThe changes introduce two new entities, 📜 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 #418 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 49 51 +2
Lines 1103 1134 +31
=========================================
+ Hits 1103 1134 +31
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:
|
cuenca/resources/terms_catalog.py
Outdated
| from .base import Queryable, Retrievable | ||
|
|
||
|
|
||
| class TermsCatalog(Retrievable, Queryable): |
There was a problem hiding this comment.
TermsOfService
El objeto como tal no es un catalogo, el catalogo sería todos los objetos
There was a problem hiding this comment.
ok, entonces para los tos aceptados puede ser UserAgreement o TOSAgreement ?
There was a problem hiding this comment.
UserTOSAgreement queda
| created_at: dt.datetime | ||
| type: TermsOfService | ||
| version: str | ||
| uri: SerializableHttpUrl |
There was a problem hiding this comment.
Creo que necesita un campo deactivated_at o is_active para saber si esta activo o no.
| from .base import Creatable, Queryable, Retrievable | ||
|
|
||
|
|
||
| class TermsOfService(Creatable, Retrievable, Queryable): |
There was a problem hiding this comment.
Como este ya es la aceptación de los terminos qiueda mejor UserTOSAgreement
| id: str | ||
| created_at: dt.datetime | ||
| user_id: str | ||
| tos_id: str |
There was a problem hiding this comment.
Creo que es mejor que el modelo ya tenga ya los campos, para no hacer más llamadas
type: TermsOfService
version: str
gmorales96
force-pushed
the
feature/tos-and-user-agreement
branch
from
5f4e881 to
a7564d8
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
cuenca/resources/terms_of_service.py (1)
9-19: Consider adding class documentation.While the implementation is solid, adding a docstring to the
TermsOfServiceclass would improve code maintainability by explaining the purpose and usage of this class, especially since it's a newly introduced entity as mentioned in the PR objectives.class TermsOfService(Retrievable, Queryable): + """ + Represents a Terms of Service document that users can agree to. + + This class handles the retrieval and querying of terms of service + agreements available in the system. + """ _resource: ClassVar = 'terms_of_service'cuenca/resources/users_tos_agreements.py (2)
13-21: Consider documenting the class and attributes.While the class implementation follows snake_case naming conventions as required, adding docstrings to explain the purpose of this class and its attributes would improve maintainability and developer onboarding.
class UserTOSAgreement(Creatable, Retrievable, Queryable): + """ + Represents a user's agreement to terms of service. + + This class tracks when and how a user has agreed to a specific + version of the terms of service. + """ _resource: ClassVar = 'users_tos_agreements' id: str created_at: dt.datetime user_id: str type: TermsOfService version: str ip: str location: str hash: str url: SerializableHttpUrl
20-21: Consider adding validation for security-sensitive fields.The
hashandurlfields appear to store security-sensitive information related to the terms acceptance. Consider adding validation to ensure these fields contain properly formatted data.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (4)
cuenca/resources/__init__.py(3 hunks)cuenca/resources/terms_of_service.py(1 hunks)cuenca/resources/users_tos_agreements.py(1 hunks)cuenca/version.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- cuenca/version.py
🚧 Files skipped from review as they are similar to previous changes (1)
- cuenca/resources/init.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).
cuenca/resources/terms_of_service.pycuenca/resources/users_tos_agreements.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.
cuenca/resources/terms_of_service.pycuenca/resources/users_tos_agreements.py
🧬 Code Definitions (1)
cuenca/resources/users_tos_agreements.py (1)
cuenca/resources/terms_of_service.py (1)
TermsOfService(10-18)
🔇 Additional comments (6)
cuenca/resources/terms_of_service.py (4)
1-6: Appropriate import structure for external libraries.The imports are correctly structured with standard libraries first (datetime, typing), followed by external libraries (cuenca_validations). The use of absolute imports for external modules follows the coding guidelines.
7-8: Correct usage of relative imports for internal modules.The relative import for base classes (
Queryable, Retrievable) follows the coding guidelines which require relative imports for modules in the same directory.
10-12: Clean class definition with appropriate resource path.The
TermsOfServiceclass properly inherits fromRetrievableandQueryablebase classes, and correctly defines the resource endpoint with a ClassVar type annotation.
13-18: Well-structured class attributes with proper type annotations.All attributes follow snake_case naming convention as required in the coding guidelines. Type annotations are appropriately used, enhancing code readability and enabling static type checking.
cuenca/resources/users_tos_agreements.py (2)
1-8: Imports follow the coding guidelines correctly.The imports are well-organized, using absolute imports for external libraries and relative imports for internal modules, which aligns with the project's coding guidelines.
10-12: Class inheritance looks appropriate.The
UserTOSAgreementclass inherits fromCreatable,Retrievable, andQueryable, which aligns with its purpose to create, retrieve, and query user agreements to terms of service. The resource name uses snake_case as required by the guidelines.
| id: str | ||
| created_at: dt.datetime | ||
| user_id: str | ||
| type: TermsOfService |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify type annotation consistency.
The type attribute is annotated as TermsOfService here, but in the TermsOfService class it's TermsOfServiceEnum. Ensure this is intentional and not a potential type mismatch.
🌐 Web query:
What's the difference between importing a class as a type annotation vs using an enum in Python?
💡 Result:
In Python, importing a class for type annotations and using an enum serve fundamentally different purposes, though both relate to type enforcement:
Type Annotations
Type annotations specify expected data types for variables, parameters, or return values, enabling static type checkers (like mypy) to validate correctness.
- Usage: Define general type constraints for objects (e.g., integers, custom classes).
from typing import Type class User: pass def process_user(user: User) -> None: # Annotates "user" as a User instance ...
- Flexibility: Allows values to be any valid instance of the annotated class.
Enums (Enumerations)
Enums define a fixed set of named constants, enforcing that values must be one of the predefined members.
- Usage: Restrict values to a specific group (e.g., status codes, days of the week).
from enum import Enum class Day(Enum): MONDAY = 1 TUESDAY = 2 def set_day(day: Day) -> None: # Only accepts Day.MONDAY, Day.TUESDAY, etc. ...
- Key Features:
- Type Safety: Prevents invalid values (e.g.,
42cannot replaceDay.MONDAY). - Readability: Names (e.g.,
Day.MONDAY) replace "magic numbers." - Utilities: Access members via
.name,.value, or iteration[^1][^2].
- Type Safety: Prevents invalid values (e.g.,
Key Differences
| Feature | Type Annotations | Enums |
|---|---|---|
| Purpose | General type checking | Restrict to predefined constants |
| Allowed Values | Any valid instance of the type | Only enum members (e.g., Day.MONDAY) |
| Metaclass | Standard class behavior | Special EnumType for member management[^7] |
| Static Analysis | Validates instance type | Validates specific enum literals[^3] |
| Flexibility | Broad (any instance) | Narrow (fixed members) |
Example with Both
from enum import Enum
from typing import Type
class Status(Enum):
SUCCESS = 200
ERROR = 400
def validate_response(code: int) -> Status: # Return must be Status.SUCCESS/ERROR
return Status(code)
UserType = Type[Status] # Type annotation for the Status class itselfHere, Status ensures only valid HTTP codes are used, while Type[Status] annotates a class reference[^9].
When to Use Each
- Use type annotations for general type enforcement (e.g.,
int, custom classes). - Use enums for constrained constants (e.g., states, categories)[^5][^8].
Enums enhance code safety and readability, while type annotations provide broader type validation.
Citations:
- 1: https://docs.python.org/3/howto/enum.html
- 2: https://mimo.org/glossary/python/enum
- 3: https://mypy.readthedocs.io/en/stable/literal_types.html
- 4: https://florian-dahlitz.de/articles/why-you-should-use-more-enums-in-python
- 5: https://www.kdnuggets.com/python-enum-how-to-build-enumerations-in-python
- 6: https://discuss.python.org/t/now-that-weve-promoted-generics-and-type-aliases-to-syntax-why-not-do-the-same-for-enums-structs-and-abstract-classes/35584
- 7: https://docs.python.org/3/library/enum.html
- 8: https://realpython.com/python-enum/
- 9: https://docs.pydantic.dev/2.2/usage/types/enums/
- 10: https://discuss.python.org/t/enum-classes-and-self/56233
- 11: https://arjancodes.com/blog/python-enum-classes-for-managing-constants/
- 12: https://youtrack.jetbrains.com/issue/PY-44632/Enum-value-with-annotation-confuses-PyCharms-type-inference
- 13: https://discuss.python.org/t/best-practices-for-placing-common-enumeration-constants-in-a-python-package/38519
- 14: https://www.youtube.com/watch?v=AoTBsBBPtOs
Action Required: Align Type Annotation with Enum Usage
The current annotation in line 16 of cuenca/resources/users_tos_agreements.py uses TermsOfService, but the actual type defined in the TermsOfService class is TermsOfServiceEnum. In Python, type annotations and enums have distinct roles: type annotations inform static type checkers, whereas enums enforce a constrained set of values. If the intention is to restrict allowed values to those defined in TermsOfServiceEnum, the annotation should reflect that directly. Either update the annotation to use TermsOfServiceEnum or document the aliasing rationale explicitly to avoid confusion.
- File: cuenca/resources/users_tos_agreements.py
- Issue: Mismatch between annotation (
TermsOfService) and actual enum type (TermsOfServiceEnum) - Action: Update the type annotation or add clarifying comments if intentional
gmorales96
force-pushed
the
feature/tos-and-user-agreement
branch
from
64d351b to
2ff921b
Compare
Description
This PR introduces
TermsOfServiceandUserTOSAgreement, to manage terms of service agreements.The
TermsOfServicerepresents available terms, whileUserTOSAgreementtracks user acceptance.Summary by CodeRabbit
New Features
TermsOfServiceandUserTOSAgreement.Chores