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 with BackgroundTasks—that part of the submission is solid. However, this requires changes because the profile creation functionality is currently broken due to how file uploads are handled.
The main issue is in src/routes/profiles.py, where the endpoint is configured to accept a JSON body, but file uploads require multipart/form-data. You'll need to update the function signature to accept form fields and the avatar file separately using Form() and File().
This is connected to an issue in src/schemas/profiles.py. The avatar field is defined as a str in the base schema, which causes a type conflict for the creation request that needs an UploadFile. To fix this, please define the avatar field with the appropriate type directly within the ProfileCreateRequestSchema and ProfileResponseSchema instead of the shared base class.
Lastly, please correct the hardcoded login URL in src/routes/accounts.py to include the port :8000 for consistency in the development environment.
✨ 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
| detail="An error occurred while resetting the password." | ||
| ) | ||
|
|
||
| login_link = "http://127.0.0.1/accounts/login/" |
There was a problem hiding this comment.
This hardcoded URL is inconsistent with the others in this file (e.g., in activate_account on line 233), as it is missing the port number :8000. This could prevent the link from working correctly in the development environment. Please ensure all generated URLs are valid and consistent.
src/routes/profiles.py
Outdated
| @router.post("/users/{user_id}/profile/", status_code=status.HTTP_201_CREATED) | ||
| async def user_profile_creation( | ||
| user_id: int, | ||
| profile_data: schemas.ProfileCreateRequestSchema, |
There was a problem hiding this comment.
This function signature expects the profile_data to be a JSON object in the request body. However, since you are uploading an avatar file, the request content type will be multipart/form-data, not application/json. FastAPI cannot automatically parse a multipart request into a single Pydantic model like this.
You should modify the signature to accept individual form fields and the file separately. For example:
from fastapi import Form, File, UploadFile
from datetime import date
async def user_profile_creation(
...,
first_name: str = Form(),
last_name: str = Form(),
...,
avatar: UploadFile = File(),
...
):
# Then you can manually create a dictionary or Pydantic model
# from these fields to perform validation.| class UserProfileBase(BaseModel): | ||
| first_name: str | ||
| last_name: str | ||
| gender: str | ||
| date_of_birth: date | ||
| info: str | ||
| avatar: str | ||
|
|
||
| @field_validator("first_name", "last_name") | ||
| @classmethod | ||
| def validate_fullname(cls, value): | ||
| return validate_name(value) | ||
|
|
||
| @field_validator("gender") | ||
| @classmethod | ||
| def validate_gender(cls, value): | ||
| return validate_gender(value) | ||
|
|
||
| @field_validator("date_of_birth") | ||
| @classmethod | ||
| def validate_bdate(cls, value): | ||
| return validate_birth_date(value) | ||
|
|
||
| @field_validator("info") | ||
| @classmethod | ||
| def validate_info(cls, value): | ||
| if not value.strip(): | ||
| raise ValueError("Info cannot be empty or consist only of spaces.") | ||
| return value | ||
|
|
||
| @field_validator("avatar") | ||
| @classmethod | ||
| def validate_avatar(cls, value): | ||
| return validate_image(value) | ||
|
|
There was a problem hiding this comment.
The structure of this base class creates a conflict for the avatar field, which needs to be an UploadFile for requests but a str (URL) for responses.
- The
avatarfield is typed asstr(line 23). This is incorrect forProfileCreateRequestSchema, which must handle a file upload as per the requirements. - The
@field_validator("avatar")(line 47) callsvalidate_image, which expects anUploadFile. Because the field is typed asstr, the validator will receive the wrong data type, causing an error.
To resolve this, consider defining avatar with the appropriate type within each specific schema (ProfileCreateRequestSchema and ProfileResponseSchema) rather than in this shared base class.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent progress on implementing the profile creation endpoint and email notifications! The use of BackgroundTasks and handling of multipart/form-data are well done. However, a couple of changes are needed before this can be approved.
First, in src/routes/profiles.py, the validation for the authenticated user is incomplete. While you check if the user exists, you also need to ensure they are active by adding a check for db_current_user.is_active. An inactive user should not be able to create a profile. Please also update the error detail "User not found." to one that is specified in the task's API contract.
Additionally, the build is failing because of a Python version issue. To fix this, please modify src/schemas/profiles.py to import Self from the typing_extensions library instead of typing, as our CI environment uses Python 3.10.
# In src/schemas/profiles.py
from typing_extensions import Self # Add this line
from typing import Any # Remove Self from this lineOnce these issues are resolved, your submission 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/profiles.py
Outdated
| if not db_current_user: | ||
| raise HTTPException(status_code=401, detail="User not found.") |
There was a problem hiding this comment.
This check for the authenticated user is a good safeguard. However, it has two points to address:
- The error detail
"User not found."is not listed in the task's "Full List of Possible Errors". The API contract should be consistent. - The check only verifies if the user exists but doesn't check if they are active. An inactive user might still be able to proceed and create a profile for themselves.
Please consider adding a check for db_current_user.is_active and using an error detail that aligns with the project's API specification.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I'm providing an educational approval to help you progress, but there are a couple of critical issues from the automated tests that need your attention. You've done a fantastic job with the email notifications and the Pydantic schemas! The main problems are in src/routes/profiles.py.
The endpoint isn't handling Pydantic validation errors, which results in a 500 error instead of a 422. You can fix this by wrapping the ProfileCreateRequestSchema instantiation in a try...except ValidationError block. Additionally, the error message for an inactive user is incorrect; it should be "User not found or not active." to match the test's expectation.
Addressing these two error handling issues will get all your tests to pass. Keep up the great work, you're almost there!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.