feat(user): add user services endpoints from user service docs #11
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThese changes introduce user profile management endpoints to the backend API. They add JWT-based authentication using OAuth2 password bearer tokens, implement user profile retrieval, update, and deletion endpoints, define related Pydantic schemas, and encapsulate user database operations in a new service class. The main application now includes the new user router. Additionally, MongoDB connection management is refactored to use a lifespan context manager, and datetime handling is updated to use timezone-aware UTC timestamps. Test coverage for the new user features and service logic is added with mocking and fixtures. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant FastAPI
participant Security
participant UserService
participant MongoDB
Client->>FastAPI: GET /users/me (with Bearer token)
FastAPI->>Security: get_current_user(token)
Security->>Security: Decode JWT, extract user_id
Security-->>FastAPI: user_id
FastAPI->>UserService: get_user_by_id(user_id)
UserService->>MongoDB: Find user by ID
MongoDB-->>UserService: User document
UserService-->>FastAPI: User data
FastAPI-->>Client: UserProfileResponse
Client->>FastAPI: PATCH /users/me (updates, Bearer token)
FastAPI->>Security: get_current_user(token)
Security-->>FastAPI: user_id
FastAPI->>UserService: update_user_profile(user_id, updates)
UserService->>MongoDB: Update user document
MongoDB-->>UserService: Updated document
UserService-->>FastAPI: Updated user data
FastAPI-->>Client: Updated user response
Client->>FastAPI: DELETE /users/me (Bearer token)
FastAPI->>Security: get_current_user(token)
Security-->>FastAPI: user_id
FastAPI->>UserService: delete_user(user_id)
UserService->>MongoDB: Delete user by ID
MongoDB-->>UserService: Deletion result
UserService-->>FastAPI: Success flag
FastAPI-->>Client: DeleteUserResponse
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 (
|
✅ Deploy Preview for splitwizer canceled.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #11 +/- ##
===========================================
+ Coverage 65.39% 76.27% +10.88%
===========================================
Files 12 17 +5
Lines 575 864 +289
===========================================
+ Hits 376 659 +283
- Misses 199 205 +6 ☔ View 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 (7)
backend/app/auth/security.py (1)
17-17: Fix formatting issueMissing blank line before variable declaration.
+ oauth2_scheme = OAuth2PasswordBearer(tokenUrl="/auth/login/email")backend/app/user/schemas.py (1)
5-5: Fix formatting issuesMissing blank lines before class definitions as required by PEP 8.
+ class UserProfileResponse(BaseModel):+ class UserProfileUpdateRequest(BaseModel):+ class DeleteUserResponse(BaseModel):Also applies to: 17-17, 22-22
backend/app/user/service.py (2)
1-1: Remove unused importsSeveral imports are not used in this file and should be removed.
-from fastapi import HTTPException, status, Depends +from fastapi import HTTPException, status from app.database import get_database from bson import ObjectId from datetime import datetime -from typing import Optional, Dict, Any +from typing import OptionalAlso applies to: 5-5
7-7: Fix formatting issuesMissing blank lines before class definition and after class/function definitions.
+ class UserService:user_service = UserService() +Also applies to: 47-47
backend/app/user/routes.py (3)
1-1: Remove unused importThe
statusimport is not used in this file.-from fastapi import APIRouter, Depends, HTTPException, status +from fastapi import APIRouter, Depends, HTTPException
9-9: Fix formatting issuesMissing blank lines before function definitions.
+ @router.get("/me", response_model=UserProfileResponse)+ @router.patch("/me", response_model=Dict[str, Any])+ @router.delete("/me", response_model=DeleteUserResponse)Also applies to: 16-16, 29-29
16-27: Improve response consistencyThe PATCH endpoint returns a wrapped response while GET returns the user object directly. Consider using a consistent response schema.
-@router.patch("/me", response_model=Dict[str, Any]) +@router.patch("/me", response_model=UserProfileResponse) async def update_user_profile( updates: UserProfileUpdateRequest, current_user: Dict[str, Any] = Depends(get_current_user) ): update_data = updates.dict(exclude_unset=True) if not update_data: raise HTTPException(status_code=400, detail="No update fields provided.") updated_user = await user_service.update_user_profile(current_user["_id"], update_data) if not updated_user: raise HTTPException(status_code=404, detail="User not found") - return {"user": updated_user} + return updated_user
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/app/auth/security.py(3 hunks)backend/app/user/routes.py(1 hunks)backend/app/user/schemas.py(1 hunks)backend/app/user/service.py(1 hunks)backend/main.py(2 hunks)
🧰 Additional context used
🪛 Flake8 (7.2.0)
backend/app/auth/security.py
[error] 102-102: expected 2 blank lines, found 1
(E302)
backend/app/user/service.py
[error] 1-1: 'fastapi.HTTPException' imported but unused
(F401)
[error] 1-1: 'fastapi.status' imported but unused
(F401)
[error] 1-1: 'fastapi.Depends' imported but unused
(F401)
[error] 5-5: 'typing.Dict' imported but unused
(F401)
[error] 5-5: 'typing.Any' imported but unused
(F401)
[error] 7-7: expected 2 blank lines, found 1
(E302)
[error] 47-47: expected 2 blank lines after class or function definition, found 1
(E305)
backend/app/user/schemas.py
[error] 5-5: expected 2 blank lines, found 1
(E302)
[error] 17-17: expected 2 blank lines, found 1
(E302)
[error] 22-22: expected 2 blank lines, found 1
(E302)
backend/app/user/routes.py
[error] 1-1: 'fastapi.status' imported but unused
(F401)
[error] 9-9: expected 2 blank lines, found 1
(E302)
[error] 16-16: expected 2 blank lines, found 1
(E302)
[error] 29-29: expected 2 blank lines, found 1
(E302)
🪛 Ruff (0.11.9)
backend/app/auth/security.py
122-122: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
backend/app/user/service.py
1-1: fastapi.HTTPException imported but unused
Remove unused import
(F401)
1-1: fastapi.status imported but unused
Remove unused import
(F401)
1-1: fastapi.Depends imported but unused
Remove unused import
(F401)
5-5: typing.Dict imported but unused
Remove unused import
(F401)
5-5: typing.Any imported but unused
Remove unused import
(F401)
backend/app/user/routes.py
1-1: fastapi.status imported but unused
Remove unused import: fastapi.status
(F401)
10-10: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
19-19: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
30-30: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
🪛 Pylint (3.3.7)
backend/app/user/schemas.py
[refactor] 14-14: Too few public methods (0/2)
(R0903)
[refactor] 5-5: Too few public methods (0/2)
(R0903)
[refactor] 17-17: Too few public methods (0/2)
(R0903)
[refactor] 22-22: Too few public methods (0/2)
(R0903)
🔇 Additional comments (4)
backend/app/auth/security.py (1)
5-6: LGTM on OAuth2 integrationThe addition of OAuth2PasswordBearer with proper imports looks good for JWT token authentication.
backend/main.py (1)
6-6: LGTM on router integrationThe user router is properly imported and registered with the FastAPI application. The integration follows the existing pattern used for the auth router.
Also applies to: 105-105
backend/app/user/schemas.py (1)
5-25: Well-designed schemas for user operationsThe Pydantic schemas are well-structured:
UserProfileResponseproperly handles field aliasing and validationUserProfileUpdateRequestenables partial updates with optional fieldsDeleteUserResponseprovides consistent response formatThe use of
EmailStrfor email validation and proper field aliasing withpopulate_by_name = Truedemonstrates good practices.backend/app/user/routes.py (1)
9-34: Well-implemented authenticated user endpointsThe API endpoints are well-designed with:
- Proper JWT authentication using
get_current_userdependency- Appropriate HTTP methods and status codes
- Good error handling for missing users and invalid inputs
- Consistent use of the user service layer
The endpoints follow REST conventions and provide a clean API for user profile management.
- Resolved ModuleNotFoundErrors in tests by adding project root to sys.path in conftest.py. - Fixed ResponseValidationError in user routes tests by aligning mock data with schema (camelCase for datetime fields). - Addressed Pydantic V2 deprecations (Config class to model_config, .dict() to .model_dump()). - Replaced deprecated FastAPI on_event handlers with lifespan manager. - Replaced deprecated datetime.utcnow() with datetime.now(timezone.utc). - Updated test invocation to `python -m pytest` to resolve plugin discovery issues in the environment. (#12) * feat: Add tests for user service routes and methods Implemented tests for the user service, covering both API routes and underlying service logic. Key changes and steps taken: 1. Created `backend/tests/user/` directory. 2. Added `test_user_routes.py` for testing `/users/me` endpoints (GET, PATCH, DELETE). - Initialized with `httpx.AsyncClient`, later refactored to `fastapi.testclient.TestClient` for compatibility with FastAPI app instance. - Corrected import paths for the FastAPI `app` instance (from `main.py`). - Ensured mock data for `created_at` and `updated_at` fields uses `datetime` objects to match Pydantic model validation. - Added missing `datetime` import. 3. Added `test_user_service.py` for testing `UserService` methods. - Implemented tests for `get_user_by_id`, `update_user_profile`, `delete_user`, and `transform_user_document`. - Used `pytest-mock` for mocking database interactions. - Corrected an `InvalidId` error by using a validly formatted ObjectId string for a non-existent user test case. 4. Updated `backend/requirements.txt` to include `pytest-mock`. 5. Modified `backend/app/user/schemas.py` to add an alias for `avatar` to `imageUrl` in `UserProfileResponse`. 6. Iteratively debugged test execution issues: - Resolved `pytest-asyncio` import problems by using `python -m pytest`. - Fixed `ModuleNotFoundError` for the `app` module by running pytest from the `backend` directory. - Addressed various `TypeError` and `NameError` issues during test setup and execution. The tests should now be in a state where they are close to passing or all passing. The primary focus was on setting up the test structure, writing comprehensive tests, and resolving environment and import-related issues. * Fix test errors and address deprecation warnings - Resolved ModuleNotFoundErrors in tests by adding project root to sys.path in conftest.py. - Fixed ResponseValidationError in user routes tests by aligning mock data with schema (camelCase for datetime fields). - Addressed Pydantic V2 deprecations (Config class to model_config, .dict() to .model_dump()). - Replaced deprecated FastAPI on_event handlers with lifespan manager. - Replaced deprecated datetime.utcnow() with datetime.now(timezone.utc). - Updated test invocation to `python -m pytest` to resolve plugin discovery issues in the environment. --------- Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
backend/app/auth/security.py (1)
102-122: Address code duplication and exception handling issues.The
get_current_userfunction duplicates JWT decoding logic from the existingverify_tokenfunction and has suboptimal exception handling, as previously noted.+ def get_current_user(token: str = Depends(oauth2_scheme)) -> Dict[str, Any]: """ Retrieves the current user based on the provided JWT token. Args: token: The JWT token from which to extract the user information. Returns: A dictionary containing the current user's information. Raises: HTTPException: If the token is invalid or user information cannot be extracted. """ try: - payload = jwt.decode(token, settings.secret_key, algorithms=[settings.algorithm]) + payload = verify_token(token) # Reuse existing function user_id = payload.get("sub") if user_id is None: - raise HTTPException(status_code=status.HTTP_401_UNAUTHORIZED, detail="Invalid token payload") + raise HTTPException( + status_code=status.HTTP_401_UNAUTHORIZED, + detail="Invalid token payload" + ) from None return {"_id": user_id} - except JWTError: - raise HTTPException(status_code=status.HTTP_401_UNAUTHORIZED, detail="Could not validate credentials") + except HTTPException: + raise # Re-raise HTTPException from verify_token + except Exception as e: + raise HTTPException( + status_code=status.HTTP_401_UNAUTHORIZED, + detail="Could not validate credentials" + ) from ebackend/app/user/service.py (4)
14-25: Address field mapping inconsistency and add error handling.The past review comment about field mapping inconsistency between
avatar(database) andimageUrl(API schema) remains unresolved. Additionally, the ObjectId conversion needs error handling.def transform_user_document(self, user: dict) -> dict: if not user: return None + try: + user_id = str(user["_id"]) + except (KeyError, TypeError): + return None return { - "_id": str(user["_id"]), + "_id": user_id, "name": user.get("name"), "email": user.get("email"), - "avatar": user.get("avatar"), + "imageUrl": user.get("imageUrl") or user.get("avatar"), # Handle both field names "currency": user.get("currency", "USD"), "createdAt": user.get("created_at"), "updatedAt": user.get("updated_at") or user.get("created_at"), }
27-30: Add error handling for invalid ObjectId conversion.The past review comment about ObjectId error handling remains unresolved and is critical for preventing runtime exceptions.
async def get_user_by_id(self, user_id: str) -> Optional[dict]: db = self.get_db() + try: + object_id = ObjectId(user_id) + except Exception: + return None - user = await db.users.find_one({"_id": ObjectId(user_id)}) + user = await db.users.find_one({"_id": object_id}) return self.transform_user_document(user)
32-40: Add error handling for invalid ObjectId conversion.The update method also needs ObjectId error handling as mentioned in past review comments.
async def update_user_profile(self, user_id: str, updates: dict) -> Optional[dict]: db = self.get_db() + try: + object_id = ObjectId(user_id) + except Exception: + return None updates["updated_at"] = datetime.now(timezone.utc) result = await db.users.find_one_and_update( - {"_id": ObjectId(user_id)}, + {"_id": object_id}, {"$set": updates}, return_document=True ) return self.transform_user_document(result)
42-45: Add error handling for invalid ObjectId conversion.The delete method also needs ObjectId error handling as mentioned in past review comments.
async def delete_user(self, user_id: str) -> bool: db = self.get_db() + try: + object_id = ObjectId(user_id) + except Exception: + return False - result = await db.users.delete_one({"_id": ObjectId(user_id)}) + result = await db.users.delete_one({"_id": object_id}) return result.deleted_count == 1
🧹 Nitpick comments (8)
backend/tests/conftest.py (1)
6-12: Fix comment spacing to follow PEP 8 standards.The implementation correctly adds the project root to sys.path for test imports, but there are formatting issues with inline comments.
-import sys # Added -from pathlib import Path # Added +import sys # Added +from pathlib import Path # Addedbackend/tests/auth/test_auth_routes.py (1)
138-138: Fix comment spacing and approve timezone-aware datetime usage.The change to use timezone-aware datetime is correct and consistent with the overall datetime handling updates in this PR.
- "created_at": datetime.now(timezone.utc), # Ensure datetime is used + "created_at": datetime.now(timezone.utc), # Ensure datetime is usedbackend/main.py (1)
10-21: Excellent modernization with lifespan context manager.The replacement of separate startup/shutdown handlers with a lifespan context manager follows FastAPI best practices and provides better resource management.
+ @asynccontextmanager async def lifespan(app: FastAPI):backend/app/user/schemas.py (1)
1-24: Well-designed schemas with proper validation and aliasing.The Pydantic models are correctly structured with appropriate field validation, aliasing, and optional fields for partial updates.
from datetime import datetime + class UserProfileResponse(BaseModel): id: str = Field(alias="_id") name: str email: EmailStr imageUrl: Optional[str] = Field(default=None, alias="avatar") currency: str = "USD" createdAt: datetime updatedAt: datetime model_config = {"populate_by_name": True} + class UserProfileUpdateRequest(BaseModel): name: Optional[str] = None imageUrl: Optional[str] = None currency: Optional[str] = None + class DeleteUserResponse(BaseModel): success: bool = True message: Optional[str] = Nonebackend/app/user/service.py (1)
1-5: Remove unused imports to clean up the code.Several imports are not used in this service class and should be removed for cleaner code.
-from fastapi import HTTPException, status, Depends from app.database import get_database from bson import ObjectId from datetime import datetime, timezone -from typing import Optional, Dict, Any +from typing import Optionalbackend/tests/user/test_user_service.py (2)
3-3: Remove unused import.The
get_databaseimport is not used in the test file since it's mocked.-from app.database import get_database # To mock the database dependency
137-137: Remove unused variable assignment.The
expected_transformedvariable is assigned but never used.- # Expected transformed output - expected_transformed = user_service.transform_user_document(updated_user_doc_from_db) -backend/app/user/routes.py (1)
1-1: Remove unused import.The
statusimport is not used in this file.-from fastapi import APIRouter, Depends, HTTPException, status +from fastapi import APIRouter, Depends, HTTPException
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
backend/app/auth/security.py(4 hunks)backend/app/auth/service.py(7 hunks)backend/app/user/routes.py(1 hunks)backend/app/user/schemas.py(1 hunks)backend/app/user/service.py(1 hunks)backend/main.py(2 hunks)backend/requirements.txt(1 hunks)backend/tests/auth/test_auth_routes.py(3 hunks)backend/tests/conftest.py(1 hunks)backend/tests/user/test_user_routes.py(1 hunks)backend/tests/user/test_user_service.py(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- backend/requirements.txt
- backend/app/auth/service.py
🧰 Additional context used
🧬 Code Graph Analysis (4)
backend/app/auth/security.py (1)
backend/app/dependencies.py (1)
get_current_user(10-55)
backend/app/user/routes.py (3)
backend/app/user/schemas.py (3)
UserProfileResponse(5-14)UserProfileUpdateRequest(16-19)DeleteUserResponse(21-23)backend/app/auth/security.py (1)
get_current_user(102-122)backend/app/user/service.py (3)
get_user_by_id(27-30)update_user_profile(32-40)delete_user(42-45)
backend/app/user/service.py (3)
backend/app/database.py (1)
get_database(31-37)backend/app/auth/service.py (1)
get_db(58-62)backend/app/user/routes.py (1)
update_user_profile(17-27)
backend/main.py (1)
backend/app/database.py (2)
connect_to_mongo(10-18)close_mongo_connection(20-29)
🪛 Flake8 (7.2.0)
backend/tests/conftest.py
[error] 6-6: at least two spaces before inline comment
(E261)
[error] 7-7: at least two spaces before inline comment
(E261)
backend/tests/user/test_user_service.py
[error] 3-3: 'app.database.get_database' imported but unused
(F401)
[error] 3-3: at least two spaces before inline comment
(E261)
[error] 6-6: at least two spaces before inline comment
(E261)
[error] 13-13: expected 2 blank lines, found 1
(E302)
[error] 17-17: at least two spaces before inline comment
(E261)
[error] 20-20: expected 2 blank lines, found 1
(E302)
[error] 54-54: expected 2 blank lines, found 1
(E302)
[error] 58-58: expected 2 blank lines, found 1
(E302)
[error] 69-69: at least two spaces before inline comment
(E261)
[error] 70-70: at least two spaces before inline comment
(E261)
[error] 72-72: at least two spaces before inline comment
(E261)
[error] 77-77: expected 2 blank lines, found 1
(E302)
[error] 98-98: expected 2 blank lines, found 1
(E302)
[error] 103-103: expected 2 blank lines, found 1
(E302)
[error] 112-112: expected 2 blank lines, found 1
(E302)
[error] 123-123: expected 2 blank lines, found 1
(E302)
[error] 137-137: local variable 'expected_transformed' is assigned to but never used
(F841)
[error] 146-146: at least two spaces before inline comment
(E261)
[error] 147-147: at least two spaces before inline comment
(E261)
[error] 159-159: at least two spaces before inline comment
(E261)
[error] 175-175: expected 2 blank lines, found 1
(E302)
[error] 186-186: expected 2 blank lines, found 1
(E302)
backend/tests/user/test_user_routes.py
[error] 2-2: at least two spaces before inline comment
(E261)
[error] 5-5: at least two spaces before inline comment
(E261)
[error] 6-6: at least two spaces before inline comment
(E261)
[error] 9-9: at least two spaces before inline comment
(E261)
[error] 12-12: expected 2 blank lines, found 1
(E302)
[error] 13-13: at least two spaces before inline comment
(E261)
[error] 14-14: at least two spaces before inline comment
(E261)
[error] 17-17: expected 2 blank lines, found 1
(E302)
[error] 26-26: expected 2 blank lines, found 1
(E302)
[error] 37-37: at least two spaces before inline comment
(E261)
[error] 46-46: at least two spaces before inline comment
(E261)
[error] 71-71: expected 2 blank lines, found 1
(E302)
[error] 71-71: at least two spaces before inline comment
(E261)
[error] 74-74: at least two spaces before inline comment
(E261)
[error] 82-82: expected 2 blank lines, found 1
(E302)
[error] 82-82: at least two spaces before inline comment
(E261)
[error] 86-86: at least two spaces before inline comment
(E261)
[error] 92-92: expected 2 blank lines, found 1
(E302)
[error] 92-92: at least two spaces before inline comment
(E261)
[error] 100-100: at least two spaces before inline comment
(E261)
[error] 102-102: at least two spaces before inline comment
(E261)
[error] 104-104: at least two spaces before inline comment
(E261)
[error] 108-108: expected 2 blank lines, found 1
(E302)
[error] 108-108: at least two spaces before inline comment
(E261)
[error] 115-115: at least two spaces before inline comment
(E261)
[error] 116-116: at least two spaces before inline comment
(E261)
[error] 120-120: at least two spaces before inline comment
(E261)
[error] 124-124: at least two spaces before inline comment
(E261)
[error] 126-126: expected 2 blank lines, found 1
(E302)
[error] 126-126: at least two spaces before inline comment
(E261)
[error] 128-128: at least two spaces before inline comment
(E261)
[error] 132-132: expected 2 blank lines, found 1
(E302)
[error] 132-132: at least two spaces before inline comment
(E261)
[error] 136-136: at least two spaces before inline comment
(E261)
[error] 142-142: expected 2 blank lines, found 1
(E302)
[error] 142-142: at least two spaces before inline comment
(E261)
[error] 145-145: at least two spaces before inline comment
(E261)
[error] 151-151: expected 2 blank lines, found 1
(E302)
[error] 151-151: at least two spaces before inline comment
(E261)
[error] 154-154: at least two spaces before inline comment
(E261)
backend/app/auth/security.py
[error] 102-102: expected 2 blank lines, found 1
(E302)
backend/app/user/routes.py
[error] 1-1: 'fastapi.status' imported but unused
(F401)
[error] 9-9: expected 2 blank lines, found 1
(E302)
[error] 16-16: expected 2 blank lines, found 1
(E302)
[error] 29-29: expected 2 blank lines, found 1
(E302)
backend/app/user/schemas.py
[error] 5-5: expected 2 blank lines, found 1
(E302)
[error] 16-16: expected 2 blank lines, found 1
(E302)
[error] 21-21: expected 2 blank lines, found 1
(E302)
backend/app/user/service.py
[error] 1-1: 'fastapi.HTTPException' imported but unused
(F401)
[error] 1-1: 'fastapi.status' imported but unused
(F401)
[error] 1-1: 'fastapi.Depends' imported but unused
(F401)
[error] 5-5: 'typing.Dict' imported but unused
(F401)
[error] 5-5: 'typing.Any' imported but unused
(F401)
[error] 7-7: expected 2 blank lines, found 1
(E302)
[error] 47-47: expected 2 blank lines after class or function definition, found 1
(E305)
backend/main.py
[error] 10-10: expected 2 blank lines, found 1
(E302)
backend/tests/auth/test_auth_routes.py
[error] 138-138: at least two spaces before inline comment
(E261)
🪛 Ruff (0.11.9)
backend/tests/user/test_user_service.py
3-3: app.database.get_database imported but unused
Remove unused import: app.database.get_database
(F401)
137-137: Local variable expected_transformed is assigned to but never used
Remove assignment to unused variable expected_transformed
(F841)
backend/app/auth/security.py
122-122: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
backend/app/user/routes.py
1-1: fastapi.status imported but unused
Remove unused import: fastapi.status
(F401)
10-10: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
19-19: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
30-30: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
backend/app/user/service.py
1-1: fastapi.HTTPException imported but unused
Remove unused import
(F401)
1-1: fastapi.status imported but unused
Remove unused import
(F401)
1-1: fastapi.Depends imported but unused
Remove unused import
(F401)
5-5: typing.Dict imported but unused
Remove unused import
(F401)
5-5: typing.Any imported but unused
Remove unused import
(F401)
🪛 Pylint (3.3.7)
backend/app/user/schemas.py
[refactor] 5-5: Too few public methods (0/2)
(R0903)
[refactor] 16-16: Too few public methods (0/2)
(R0903)
[refactor] 21-21: Too few public methods (0/2)
(R0903)
🔇 Additional comments (11)
backend/tests/auth/test_auth_routes.py (1)
176-176: LGTM! Consistent timezone-aware datetime usage.The datetime handling is properly updated to use timezone-aware UTC timestamps.
backend/main.py (1)
104-104: LGTM! User router successfully integrated.The user router inclusion enables the new user profile management endpoints.
backend/app/auth/security.py (2)
59-61: LGTM! Consistent timezone-aware datetime usage.The updates to use
datetime.now(timezone.utc)ensure consistent UTC timestamp handling across the application.
17-17: LGTM! OAuth2 scheme properly configured.The OAuth2PasswordBearer scheme is correctly configured with the login endpoint URL.
backend/tests/user/test_user_service.py (1)
54-196: Excellent test coverage!The test suite comprehensively covers all UserService methods with various scenarios including edge cases, error conditions, and data transformations. The use of fixtures and mocking is appropriate and well-structured.
backend/app/user/routes.py (2)
9-28: Well-structured user profile endpoints.The endpoints are properly structured with:
- Appropriate use of dependency injection for authentication
- Proper error handling with meaningful HTTP status codes
- Consistent response patterns
- Good separation of concerns
The Ruff warnings about
Depends()calls are false positives - this is the standard FastAPI pattern for dependency injection.
29-34: Consider the implications of user deletion.The delete endpoint returns a success message but doesn't actually verify if cascading deletions (user's expenses, groups, etc.) are handled properly. Ensure the service layer or database triggers handle related data cleanup.
#!/bin/bash # Search for references to user deletion and related data cleanup rg -A 5 -B 5 "delete.*user|remove.*user" --type pybackend/tests/user/test_user_routes.py (4)
1-68: Well-structured test setup with appropriate fixtures.The test setup uses proper fixtures for client and authentication, with effective mocking of the service layer. The use of TestClient is appropriate for synchronous testing of FastAPI routes.
71-89: Comprehensive test coverage for GET endpoint.The tests properly verify both success and error scenarios for the profile retrieval endpoint, checking response codes and data structure.
92-139: Thorough testing of PATCH endpoint scenarios.The update tests cover:
- Full profile updates
- Partial updates
- Empty payload validation
- User not found scenarios
The field mapping verification (imageUrl → avatar) correctly tests the schema transformation.
142-157: Complete test coverage for DELETE endpoint.The deletion tests verify both successful deletion and user not found scenarios with appropriate response validation.
) - Creates a new /auth/token endpoint that accepts OAuth2PasswordRequestForm (form-data) as expected by Swagger UI for the tokenUrl. - Updates OAuth2PasswordBearer to use this new /auth/token endpoint. - The existing /auth/login/email endpoint (expecting JSON) remains unchanged for direct client use. This change ensures that Swagger UI can correctly authenticate and obtain a bearer token without a 422 Unprocessable Entity error, as it now communicates with an endpoint that handles the expected 'application/x-www-form-urlencoded' content type for token acquisition. Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/app/auth/security.py (1)
102-122: Address code duplication and architectural concernsThis function has several issues:
- Code duplication: It duplicates JWT decoding logic already present in the
verify_tokenfunction- Architectural conflict: There's already a comprehensive
get_current_userfunction inbackend/app/dependencies.pythat includes database lookup- Exception handling: Missing proper exception chaining
- Formatting: Missing blank lines before function definition
The existing function in
dependencies.pyprovides complete user retrieval with database validation, while this simpler version only extracts the user ID from the token.Consider refactoring to reuse existing logic and clarify the architectural pattern:
+ + def get_current_user(token: str = Depends(oauth2_scheme)) -> Dict[str, Any]: """ Retrieves the current user based on the provided JWT token. Args: token: The JWT token from which to extract the user information. Returns: A dictionary containing the current user's information. Raises: HTTPException: If the token is invalid or user information cannot be extracted. """ try: - payload = jwt.decode(token, settings.secret_key, algorithms=[settings.algorithm]) + payload = verify_token(token) # Reuse existing function user_id = payload.get("sub") if user_id is None: - raise HTTPException(status_code=status.HTTP_401_UNAUTHORIZED, detail="Invalid token payload") + raise HTTPException( + status_code=status.HTTP_401_UNAUTHORIZED, + detail="Invalid token payload" + ) from None return {"_id": user_id} + except HTTPException: + raise # Re-raise HTTPException from verify_token - except JWTError: - raise HTTPException(status_code=status.HTTP_401_UNAUTHORIZED, detail="Could not validate credentials") + except Exception as e: + raise HTTPException( + status_code=status.HTTP_401_UNAUTHORIZED, + detail="Could not validate credentials" + ) from ePlease clarify the intended usage pattern - should this simple
get_current_userfunction coexist with the comprehensive one independencies.py, or should there be a unified approach?
🧹 Nitpick comments (2)
backend/app/auth/routes.py (1)
9-9: Remove unused importThe
oauth2_schemeimport is not used in this file and should be removed.-from app.auth.security import create_access_token, oauth2_scheme # Import oauth2_scheme +from app.auth.security import create_access_tokenbackend/app/auth/security.py (1)
17-17: Fix inline comment formattingThe OAuth2 scheme configuration is correct, but the inline comment needs proper spacing.
-oauth2_scheme = OAuth2PasswordBearer(tokenUrl="/auth/token") # Updated tokenUrl +oauth2_scheme = OAuth2PasswordBearer(tokenUrl="/auth/token") # Updated tokenUrl
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/app/auth/routes.py(1 hunks)backend/app/auth/security.py(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/app/auth/security.py (1)
backend/app/dependencies.py (1)
get_current_user(10-55)
🪛 Ruff (0.11.9)
backend/app/auth/routes.py
9-9: app.auth.security.oauth2_scheme imported but unused
Remove unused import: app.auth.security.oauth2_scheme
(F401)
17-17: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
41-44: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
backend/app/auth/security.py
122-122: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🪛 GitHub Check: codecov/patch
backend/app/auth/routes.py
[warning] 23-23: backend/app/auth/routes.py#L23
Added line #L23 was not covered by tests
[warning] 26-26: backend/app/auth/routes.py#L26
Added line #L26 was not covered by tests
[warning] 31-31: backend/app/auth/routes.py#L31
Added line #L31 was not covered by tests
[warning] 36-39: backend/app/auth/routes.py#L36-L39
Added lines #L36 - L39 were not covered by tests
[warning] 41-41: backend/app/auth/routes.py#L41
Added line #L41 was not covered by tests
backend/app/auth/security.py
[warning] 61-61: backend/app/auth/security.py#L61
Added line #L61 was not covered by tests
[warning] 119-119: backend/app/auth/security.py#L119
Added line #L119 was not covered by tests
[warning] 121-122: backend/app/auth/security.py#L121-L122
Added lines #L121 - L122 were not covered by tests
🪛 Flake8 (7.2.0)
backend/app/auth/routes.py
[error] 9-9: 'app.auth.security.oauth2_scheme' imported but unused
(F401)
[error] 9-9: at least two spaces before inline comment
(E261)
[error] 10-10: at least two spaces before inline comment
(E261)
[error] 16-16: expected 2 blank lines, found 1
(E302)
[error] 16-16: at least two spaces before inline comment
(E261)
[error] 27-27: at least two spaces before inline comment
(E261)
backend/app/auth/security.py
[error] 17-17: at least two spaces before inline comment
(E261)
[error] 102-102: expected 2 blank lines, found 1
(E302)
🔇 Additional comments (1)
backend/app/auth/security.py (1)
59-61: LGTM - Timezone-aware datetime usageThe update to use
datetime.now(timezone.utc)instead of the deprecateddatetime.utcnow()is a good improvement for timezone handling.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
backend/app/auth/security.py (2)
17-17: Fix inline comment formatting.The inline comment needs proper spacing according to PEP 8 standards.
-oauth2_scheme = OAuth2PasswordBearer(tokenUrl="/auth/token") # Updated tokenUrl +oauth2_scheme = OAuth2PasswordBearer(tokenUrl="/auth/token") # Updated tokenUrl
102-119: Well-implemented user authentication function - addresses past review concerns.The current implementation successfully addresses the past review comment about code duplication by properly using
verify_token()for centralized JWT validation. The error handling is clean and appropriate.However, there's a formatting issue with blank lines.
+ def get_current_user(token: str = Depends(oauth2_scheme)) -> Dict[str, Any]:backend/app/user/service.py (3)
1-5: Remove unused imports to clean up the code.Several imports are not used in this file and should be removed to keep the code clean.
-from fastapi import HTTPException, status, Depends from app.database import get_database from bson import ObjectId from datetime import datetime, timezone from typing import Optional -from typing import Optional, Dict, Any +from typing import Optional
7-13: Consider adding proper spacing and improve constructor.The class definition needs proper spacing and the empty constructor could be simplified.
+ + class UserService: - def __init__(self): - pass + """Service class for user-related database operations.""" def get_db(self): return get_database()
63-63: Add proper spacing after class definition.The module-level service instance needs proper spacing according to PEP 8.
return result.deleted_count == 1 + user_service = UserService()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/app/auth/security.py(4 hunks)backend/app/user/service.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
backend/app/auth/security.py (2)
backend/app/dependencies.py (1)
get_current_user(10-55)backend/app/auth/routes.py (1)
verify_token(201-221)
backend/app/user/service.py (3)
backend/app/database.py (1)
get_database(31-37)backend/app/auth/service.py (1)
get_db(58-62)backend/app/user/routes.py (1)
update_user_profile(17-27)
🪛 Flake8 (7.2.0)
backend/app/auth/security.py
[error] 17-17: at least two spaces before inline comment
(E261)
[error] 102-102: expected 2 blank lines, found 1
(E302)
backend/app/user/service.py
[error] 1-1: 'fastapi.HTTPException' imported but unused
(F401)
[error] 1-1: 'fastapi.status' imported but unused
(F401)
[error] 1-1: 'fastapi.Depends' imported but unused
(F401)
[error] 5-5: 'typing.Dict' imported but unused
(F401)
[error] 5-5: 'typing.Any' imported but unused
(F401)
[error] 7-7: expected 2 blank lines, found 1
(E302)
[error] 63-63: expected 2 blank lines after class or function definition, found 1
(E305)
🪛 Ruff (0.11.9)
backend/app/user/service.py
1-1: fastapi.HTTPException imported but unused
Remove unused import
(F401)
1-1: fastapi.status imported but unused
Remove unused import
(F401)
1-1: fastapi.Depends imported but unused
Remove unused import
(F401)
5-5: typing.Dict imported but unused
Remove unused import
(F401)
5-5: typing.Any imported but unused
Remove unused import
(F401)
🔇 Additional comments (6)
backend/app/auth/security.py (2)
1-1: Good addition of timezone-aware datetime and OAuth2 support.The imports for
timezoneandOAuth2PasswordBearerproperly support the new authentication functionality being added.Also applies to: 5-6
59-59: Excellent update to timezone-aware timestamps.Replacing
datetime.utcnow()withdatetime.now(timezone.utc)follows current best practices and addresses the deprecation warning in Python 3.12+.Also applies to: 61-61
backend/app/user/service.py (4)
14-29: Excellent implementation addressing past review concerns.The
transform_user_documentmethod properly handles:
- Invalid ObjectId conversion with try-catch (line 18-20)
- Field mapping between
avatarandimageUrl(line 25)- Graceful handling of missing data
This successfully addresses the past review comments about error handling and field mapping inconsistency.
31-38: Good ObjectId validation implementation.The method properly handles invalid ObjectId conversion as suggested in past reviews, returning
Nonegracefully when the user_id cannot be converted to a valid ObjectId.
40-52: Excellent timezone-aware timestamp handling.The use of
datetime.now(timezone.utc)on line 46 is a best practice and maintains consistency with the timezone-aware changes made elsewhere in the codebase.
54-61: Well-implemented delete method with proper error handling.The method correctly handles ObjectId validation and returns a clear boolean result based on the deletion count.
Pull Request Description
requirements.txtSummary by CodeRabbit