Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job implementing the email notifications and profile creation features! The use of BackgroundTasks for sending emails is excellent. However, I'm requesting a couple of changes because there are two critical issues that break core functionality.
First, in src/routes/profiles.py, the endpoint for creating a profile is missing a required validation. You need to check if the user_id provided in the URL path corresponds to an existing and active user before proceeding. Currently, it's possible to create a profile for a user who doesn't exist, which should result in a 401 Unauthorized error.
Second, the password reset link generated in src/routes/accounts.py is malformed. It includes ?= which is invalid. The query string should start with ? followed by the key, like ?token=.... This currently prevents the password reset feature from working.
Once these two blockers are fixed, your solution will be ready for approval. 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/accounts.py
Outdated
| f"http://127.0.0.1/accounts/reset-password/complete/token?=" | ||
| f"{quote_plus(reset_token.token)}" |
There was a problem hiding this comment.
The URL generated for the password reset link appears to be malformed. It includes ?= which is not a valid way to start a query string. A typical query string should start with ? followed by a key-value pair like ?token=....
| s3_client: S3StorageInterface = Depends(get_s3_storage_client), | ||
| ): | ||
|
|
||
| is_admin = getattr(current_user, "group_id", None) == 3 |
There was a problem hiding this comment.
Using a hardcoded value like 3 for the admin group ID can make the code harder to maintain. Since you've already imported UserGroupEnum, consider using it here for better readability and to avoid issues if the group IDs ever change, for example: current_user.group.id == UserGroupEnum.ADMIN.value.
| status_code=status.HTTP_403_FORBIDDEN, | ||
| detail="You don't have permission to edit this profile." | ||
| ) | ||
|
|
There was a problem hiding this comment.
According to the task requirements, you need to check if the user for whom the profile is being created (identified by user_id) exists and is active. This check is currently missing. An admin could attempt to create a profile for a non-existent or inactive user, which should result in a 401 Unauthorized error with the detail "User not found or not active."
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job implementing the email notifications and profile schemas! However, this submission requires changes because of a few critical issues that prevent the application from running correctly. Two of these were also mentioned in the previous review.
Please address the following:
- Invalid Docker Image: In
docker-compose-local.yml, the imagepostgres:17.3does not exist. This will prevent the database service from starting. Please use a valid tag, such aspostgres:16.3. - Malformed Password Reset Link: In
src/routes/accounts.py, the URL for the password reset link is still incorrect. A valid query string must start with?token=..., but the current code generates?=which breaks the feature. - Missing User Validation: In
src/routes/profiles.py, you still need to add a check to verify that theuser_idfrom the URL path belongs to an existing and active user before attempting to create a profile. Without this, profiles can be created for invalid users.
✨ 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
docker-compose-local.yml
Outdated
| services: | ||
| db: | ||
| image: 'postgres:latest' | ||
| image: 'postgres:17.3' |
There was a problem hiding this comment.
The Docker image tag postgres:17.3 does not seem to exist. This will prevent the db service from starting. Please use a valid and stable image tag, for example postgres:16.3.
src/routes/accounts.py
Outdated
| f"http://127.0.0.1/accounts/reset-password/complete/token?=" | ||
| f"{quote_plus(reset_token.token)}" |
There was a problem hiding this comment.
The URL for the password reset link is still malformed. A valid query string should start with ? followed by a key, for example: ?token=.... The current implementation ?= is incorrect and will break the link.
| status_code=status.HTTP_403_FORBIDDEN, | ||
| detail="You don't have permission to edit this profile." | ||
| ) | ||
|
|
There was a problem hiding this comment.
This is a good place to add the required check for the target user's existence and status. Before checking if a profile exists, you must first verify that the user_id from the URL path corresponds to an existing and active user in the users table. If not, you should raise a 401 Unauthorized error as per the requirements.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution as an educational override to help you move forward, but there is one critical issue from the last review that still needs to be addressed. You've done an excellent job fixing the Docker images, implementing the email notifications, and correcting the password reset link!
The remaining issue is in src/routes/profiles.py. The profile creation endpoint must first verify that the user_id from the URL path belongs to an existing and active user before proceeding. Currently, this check is missing.
Before checking if a profile already exists, you should query the database for the user by
user_id. If the user is not found or is not active, you must return a401 Unauthorizederror with the detail:"User not found or not active."
Please be sure to implement this validation in your next steps. Keep up the fantastic work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| status_code=status.HTTP_403_FORBIDDEN, | ||
| detail="You don't have permission to edit this profile." | ||
| ) | ||
|
|
There was a problem hiding this comment.
While you've correctly implemented the permission check for the current_user, you're still missing the validation for the user_id from the URL path. You need to query the database to ensure a user with this user_id exists and is active before proceeding.
According to the task requirements, if the user does not exist or is not active, you must return a 401 Unauthorized error with the detail: "User not found or not active."
2 participants
No description provided.