Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Hi there! I'm requesting a few changes before this can be approved. While your implementation of the email notifications is excellent, the profile creation feature has some critical issues that don't meet the task requirements.
The main problems are in src/routes/profiles.py:
- The endpoint path is
POST /but needs to bePOST /users/{user_id}/profile/. This is crucial because it affects the authorization logic. - You're missing the check to ensure a user can only create their own profile (unless they are an admin).
- The code doesn't validate if the user exists and is active before creating a profile.
Additionally, in src/schemas/profiles.py, the ProfileCreateSchema is missing the required avatar field and its validator. Please address these points by carefully reviewing the requirements for the profile creation endpoint. Keep up the great work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
src/routes/profiles.py
Outdated
| router = APIRouter() | ||
|
|
||
| # Write your code here | ||
| @router.post("/", |
There was a problem hiding this comment.
The endpoint path is defined as / here, but the task requirements specify it should be /users/{user_id}/profile/. This change is crucial as it affects how you'll handle authorization (e.g., allowing an admin to create a profile for another user). The function signature will also need to be updated to accept user_id from the path.
| except Exception: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_401_UNAUTHORIZED, | ||
| detail="Invalid or expired token", | ||
| ) |
There was a problem hiding this comment.
This exception handling is too broad. It catches any error during token decoding and returns a generic message. The requirements specify different error details for different token validation failures, such as "Token has expired.". Consider catching a more specific exception, like BaseSecurityError from the exceptions.security module, to provide more accurate feedback.
src/routes/profiles.py
Outdated
| detail="Invalid or expired token", | ||
| ) | ||
|
|
||
| validate_image(avatar) |
There was a problem hiding this comment.
After extracting the user_id from the token, there's a missing validation step. The requirements state that you must check if the user exists in the database and is active. If not, a 401 Unauthorized error with the detail "User not found or not active." should be returned.
src/routes/profiles.py
Outdated
| detail="Invalid or expired token", | ||
| ) | ||
|
|
||
| validate_image(avatar) |
There was a problem hiding this comment.
The authorization logic is not fully implemented. According to the requirements, a regular user should only be able to create their own profile. If a regular user tries to create a profile for another user (i.e., user_id from the token does not match user_id from the path), a 403 Forbidden error should be returned. This check is currently missing.
src/routes/profiles.py
Outdated
| if result.scalars().first(): | ||
| raise HTTPException( | ||
| status_code=status.HTTP_400_BAD_REQUEST, | ||
| detail="Profile already exists for this user.", |
There was a problem hiding this comment.
The error message here differs from the one specified in the requirements. The task asks for the detail to be "User already has a profile.". It's a good practice to stick to the defined API contract.
src/routes/profiles.py
Outdated
| raise HTTPException( | ||
| status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, | ||
| detail=f"Failed to upload image to storage: {str(e)}" |
There was a problem hiding this comment.
The error message for a failed avatar upload doesn't match the requirements. It should be exactly "Failed to upload avatar. Please try again later.". Including the raw exception (e) in the response can leak internal implementation details.
| class ProfileCreateSchema(BaseModel): | ||
| first_name: str | ||
| last_name: str | ||
| gender: str | ||
| date_of_birth: date | ||
| info: str |
There was a problem hiding this comment.
The task requirements for this file state that the schema should include validation for the avatar using the validate_image function. The ProfileCreateSchema is currently missing the avatar field and its associated validator. Please ensure all required validations are implemented within the schema as described.
src/schemas/profiles.py
Outdated
|
|
||
| @field_validator("first_name", "last_name") | ||
| @classmethod | ||
| def chek_name(cls, v: str): |
There was a problem hiding this comment.
There's a small typo in the method name. It should be check_name to be consistent with the other validators.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great progress on this task! The email notifications and schema validation are implemented perfectly. However, a couple of changes are needed in src/routes/profiles.py before this can be approved.
First, the endpoint needs to validate that the target user (from the user_id in the path) exists and is active. Currently, this check is only performed for the authenticated user, which could allow an admin to create a profile for a user that doesn't exist.
Second, the error message for the authorization check is incorrect. When a user tries to create a profile for someone else without permission, the API should return a 403 Forbidden error with the detail "You don't have permission to edit this profile.", not "Forbidden".
Please make these adjustments. You're very close to completing this!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| if not current_user or not current_user.is_active: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_401_UNAUTHORIZED, | ||
| detail="User not found or not active.", | ||
| ) |
There was a problem hiding this comment.
While it's good to check if the user making the request exists and is active, the requirements also specify that you must check the user for whom the profile is being created (the user_id from the path). An admin, for example, could try to create a profile for a non-existent or inactive user. Please add a check here to ensure the target user exists and is active before proceeding.
src/routes/profiles.py
Outdated
| if token_user_id != user_id and not is_admin: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_403_FORBIDDEN, | ||
| detail="Forbidden", |
There was a problem hiding this comment.
The error detail message here doesn't quite match the requirement. The task specifies it should be "You don't have permission to edit this profile.", but "Forbidden" is used instead.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I'm approving your solution as an educational override to help you progress, as there are a couple of important improvements to consider. Excellent work on the core functionality! The profile creation and email notifications are implemented correctly and meet all requirements. To further improve your code in src/routes/profiles.py, consider two things. First, eager-load the group relationship with .options(joinedload(UserModel.group)) to prevent an inefficient, separate database query when accessing current_user.group.name. Second, catch more specific exceptions like SQLAlchemyError instead of a general Exception to make your error handling more robust and avoid hiding bugs. These are great habits to build for creating performant and reliable applications. Keep up the fantastic effort!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| ) | ||
|
|
||
|
|
||
| user_stmt = select(UserModel).where(UserModel.id == token_user_id) |
There was a problem hiding this comment.
Great job fetching the current_user! However, accessing current_user.group.name on line 69 will trigger a separate database query (lazy loading). To make this more efficient and avoid potential issues with async sessions, it's best to eager-load the group relationship. You can do this by adding .options(joinedload(UserModel.group)) to your select statement.
| await db.commit() | ||
| await db.refresh(new_profile) | ||
| return new_profile | ||
| except Exception: |
There was a problem hiding this comment.
This except block is a bit too broad as it catches any possible exception. For database operations, it's better to be more specific and catch SQLAlchemyError instead. This helps ensure you're only handling expected database errors and not accidentally masking other issues. You will need to import it from sqlalchemy.exc.
2 participants
No description provided.