Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on implementing the email notifications using BackgroundTasks and setting up the core logic for profile creation. Your code is well-structured, but there are two critical requirements that need to be addressed before this can be approved.
- Validation Logic Location: The validation for profile fields (name, gender, birth date, etc.) is currently implemented in the route handler at
src/routes/profiles.py. The task requires this logic to be centralized within the Pydantic schema insrc/schemas/profiles.pyusing@field_validatordecorators. - Avatar URL Format: The API response for a successful profile creation returns the S3 object key for the avatar (e.g.,
avatars/1_avatar.jpg) instead of a full URL. The response must provide a complete, usable URL (e.g.,http://minio-theater/avatars/1_avatar.jpg) as specified in the requirements.
Please move the validation logic and correct the avatar URL format. I'm looking forward to seeing your updated submission!
✨ 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
| try: | ||
| validate_name(data.first_name) | ||
| validate_name(data.last_name) | ||
| validate_gender(data.gender) | ||
| validate_birth_date(data.date_of_birth) | ||
|
|
||
| if not data.info.strip(): | ||
| raise ValueError("Info field cannot be empty or contain only spaces.") | ||
|
|
||
| validate_image(data.avatar) | ||
|
|
||
| except ValueError as e: | ||
| raise HTTPException( | ||
| status_code=422, | ||
| detail=str(e) | ||
| ) |
There was a problem hiding this comment.
According to the task description, the validation for profile creation fields should be implemented within the ProfileCreateSchema in schemas/profiles.py, likely using Pydantic's @field_validator decorators. While your validation logic here is correct, moving it to the schema will centralize validation rules and keep the route handler focused on business logic.
| avatar_url=avatar_key, | ||
| ) | ||
|
|
||
| return profile |
There was a problem hiding this comment.
The task requirements specify that the response should contain the full URL for the user's avatar, not just the S3 object key. You are currently storing and returning the key (avatars/{user_id}_avatar.jpg). You'll need to construct the full URL (e.g., by combining the S3 endpoint/bucket from settings with this key) before sending the response.
| class ProfileCreateSchema(BaseModel): | ||
| first_name: str | ||
| last_name: str | ||
| gender: str | ||
| date_of_birth: date | ||
| info: str | ||
| avatar: UploadFile |
There was a problem hiding this comment.
This schema is missing the required validation logic. According to the task description, validation for fields like first_name, last_name, gender, date_of_birth, info, and avatar should be implemented within this class. Consider using Pydantic's @field_validator decorator along with the imported validation functions to enforce the rules specified in the requirements.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent progress on this! You've successfully moved most of the validation logic into the Pydantic schema, which is a great improvement. I'm requesting one change to fully meet the task requirements.
The validation for the avatar image is still located in the route handler (src/routes/profiles.py). For consistency and to meet the requirements, all profile validation, including the avatar, must be centralized in src/schemas/profiles.py.
Please create a @field_validator for the avatar field within the ProfileCreateSchema and call validate_image from there. Once that's done, this will be ready for approval. Great work so far!
✨ 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 config import get_jwt_auth_manager | ||
| from storages.interfaces import S3StorageInterface | ||
| from config.dependencies import get_s3_storage_client | ||
| from validation import validate_image, validate_name, validate_gender, validate_birth_date |
There was a problem hiding this comment.
These validation functions are no longer used in this file since the logic has been moved to the Pydantic schema. It's good practice to remove unused imports to keep the code clean.
src/routes/profiles.py
Outdated
| avatar_key = f"avatars/{user_id}_avatar.jpg" | ||
|
|
||
| try: | ||
| validate_image(data.avatar) |
There was a problem hiding this comment.
According to the task requirements, all validation for the profile creation, including the avatar image, should be handled within the ProfileCreateSchema in src/schemas/profiles.py. While this works, please move the image validation into the schema using a @field_validator to centralize all the validation logic as required.
| gender: str | ||
| date_of_birth: date | ||
| info: str | ||
| avatar: UploadFile |
There was a problem hiding this comment.
Excellent work on adding validators for the profile fields! However, the validation for the avatar is missing from this schema. The task requires using the validate_image function within a @field_validator here to ensure the file type and size are correct, just as you've done for the other fields.
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 done a fantastic job moving all the validation logic into the Pydantic schema, which was the main goal here. I am approving your solution so you can move forward, but I have a couple of minor suggestions for cleanup in src/routes/profiles.py.
- You can remove the unused validation imports, as that logic is now correctly handled in the schema.
- The
try...except ValueErrorblock is now redundant. Your Pydantic model validation will catch anyValueErrorand raise a singleHTTPException, so thatexceptblock is unreachable.
These are just small cleanups to keep the code tidy. Overall, this is a very strong submission. Great job!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| from config import get_jwt_auth_manager | ||
| from storages.interfaces import S3StorageInterface | ||
| from config.dependencies import get_s3_storage_client | ||
| from validation import validate_image, validate_name, validate_gender, validate_birth_date |
There was a problem hiding this comment.
These validation imports are no longer needed in this file, as all validation logic has been correctly moved to src/schemas/profiles.py. It's good practice to remove unused imports to keep the code clean.
| except ValueError as e: | ||
| raise HTTPException(status_code=422, detail=str(e)) |
There was a problem hiding this comment.
This except ValueError block appears to be unreachable. The validation is now handled within the ProfileCreateSchema, which catches any ValueError from the validators and raises a single HTTPException with a 422 status code. This block can be safely removed.
2 participants
No description provided.