Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great work on implementing the email notifications and profile schemas! The overall structure is solid. However, changes are needed to fully meet the task requirements.
The most critical issue is in src/routes/profiles.py. The endpoint for creating a profile correctly authenticates the user making the request, but it doesn't check if the target user (identified by user_id in the URL) exists and is active. Please add a check to query for the target user and raise an error if they are not found or inactive before attempting to create the profile.
Additionally, in src/storages/s3.py, the ContentType for file uploads is hardcoded as image/jpeg. To properly support other image formats like PNG, you should pass the content_type from the UploadFile object in your route and use it in the S3 client.
Once these two points are addressed, your solution will be ready. Keep up the great effort!
✨ 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
| 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 verify that the user for whom the profile is being created (identified by user_id from the path) exists and is active. The current implementation only validates the authenticated user making the request via the auth_user helper function. You should add a query here to fetch the target user and raise a 401 Unauthorized error if they are not found or not active.
src/storages/s3.py
Outdated
| Bucket=self._bucket_name, | ||
| Key=file_name, | ||
| Body=file_data, | ||
| ContentType="image/jpeg", |
There was a problem hiding this comment.
The ContentType is hardcoded to image/jpeg. While this works for the test case, the requirements state that PNG files should also be supported. Storing a PNG file with a JPEG content type is not ideal and may cause rendering issues in some browsers.
Consider modifying this method to accept a content_type argument and pass the actual content type from the UploadFile object in routes/profiles.py.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on this iteration! I am approving your solution as you've successfully addressed all the critical feedback from the previous review and met all the task requirements. The profile creation endpoint is now robust and handles user validation and file uploads correctly.
I have a few minor suggestions for you to consider for future improvements:
- In
src/routes/profiles.py, the manual validation for theinfofield is redundant since theProfileRequestSchemaalready handles it. You can remove that check for cleaner code. - The docstring for
upload_fileinsrc/storages/interfaces.pycould be updated to match theNonereturn type hint. - There's a leftover
printstatement insrc/tests/test_e2e/test_storage.pythat can be removed to keep the test output clean.
These are just small refinements, and your implementation is solid. Great job!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| if info_raw is None or not str(info_raw).strip(): | ||
| raise HTTPException( | ||
| status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, | ||
| detail="Info field cannot be empty or contain only spaces.", | ||
| ) |
There was a problem hiding this comment.
This manual validation for the info field is redundant. The ProfileRequestSchema on line 104 already performs this check with its own validator. It's a good practice to centralize all input validation within the Pydantic schema for consistency. You can safely remove these lines and let the schema handle it.
| :param file_name: The name of the file to be stored. | ||
| :param file_data: The file data in bytes. | ||
| :param content_type: The content type of the file. Defaults to "application/octet-stream". | ||
| :return: URL of the uploaded file. |
There was a problem hiding this comment.
The docstring indicates that this method returns the URL of the uploaded file, but the type hint correctly specifies None. Since the get_file_url method is responsible for generating the URL, this docstring should be updated to reflect that the method does not return anything.
| } | ||
|
|
||
| profile_response = await e2e_client.post(profile_url, headers=headers, files=files) | ||
| print("Profile creation response:", profile_response.status_code, profile_response.text) |
There was a problem hiding this comment.
This print statement appears to be for debugging purposes. It's a good practice to remove debugging statements like this from the final code to keep test logs clean and focused on test results.
3 participants
No description provided.