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 profile creation endpoint and email notifications! The code is well-structured and the features work correctly.
However, one change is required to fully meet the task specifications. The task requires that all validation logic be associated with the schema file.
- In
src/schemas/profiles.py, you are required to importvalidate_imagefrom thevalidationpackage. This import is currently missing. Please add it to the file to align with the project's design pattern.
Once you've added this import, the submission will meet all requirements. You're very close to finishing this task!
✨ 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
| from validation import ( | ||
| validate_name, | ||
| validate_image, | ||
| validate_gender, | ||
| validate_birth_date | ||
| ) |
There was a problem hiding this comment.
According to the task description, you are required to import validate_image from the validation package in this file. Please add it to the import list.
| # 5. Avatar upload to s3 storage | ||
| try: | ||
| validate_image(avatar) | ||
| ext = avatar.filename.split(".")[-1] |
There was a problem hiding this comment.
This method of getting the file extension might not work as expected for all filenames. For example, if a filename has no extension (e.g., myfile), this will use the entire filename as the extension. For more robust parsing, consider using Python's os.path.splitext or pathlib module.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent progress on the project! The email notifications and profile endpoint logic are implemented correctly. However, one structural change is needed before this can be approved. The project requires all validation logic to be associated with its schema. Currently, the avatar validation (validate_image) is performed in the create_profile route in src/routes/profiles.py. Please move this validation logic into the src/schemas/profiles.py file. A good approach would be to modify the ProfileCreateSchema.as_form class method to accept the avatar and validate it there. 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
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 task! You've successfully implemented all the requirements, and it's great to see that you moved the avatar validation into the schema as requested. Your solution is fully functional and meets all specifications.
To make your code even better, I have a small suggestion for src/schemas/profiles.py. While your current implementation works perfectly, consider using a @field_validator for the avatar field. This would align its validation with the other fields, making your code more consistent and centralizing all the validation logic within the Pydantic pattern.
Since this is a minor point and your code is functionally correct, I am approving your solution to keep you moving forward. Keep up the fantastic work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| info=info, | ||
| avatar=avatar | ||
| ) | ||
| validate_image(avatar) |
There was a problem hiding this comment.
Great work moving the avatar validation into the schema as requested!
To make the implementation even more robust and consistent with how other fields are validated, consider using a @field_validator for the avatar field as well. This would centralize all your validation logic under the same Pydantic pattern.
| except ValueError as e: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, | ||
| detail=str(e) | ||
| ) |
There was a problem hiding this comment.
If you move the image validation to a @field_validator as suggested above, this except ValueError block may no longer be necessary. Pydantic's field validators automatically convert ValueError exceptions into a single ValidationError, which would then be caught by the except ValidationError block. This would help unify and simplify your error handling.
2 participants
No description provided.