Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job implementing the email notification system with background tasks! However, some changes are needed before this can be approved, as several core requirements are not yet met.
Here are the main issues to address:
-
Profile Creation Logic: The biggest issue is that the profile creation is split into two endpoints. The task requires a single
POSTrequest to/users/{user_id}/profile/that handles amultipart/form-datapayload containing both the user's profile data (likefirst_name) and their avatar file. You'll need to refactor the endpoint insrc/routes/profiles.pyto acceptForm()fields and anUploadFile. -
Incorrect Email URLs: The links generated in the notification emails are not functional. For example, in
src/routes/accounts.py, theactivation_linkpoints to/register/instead of the correct/activate/endpoint. Please review and correct all the generated links for activation, login, and password resets. -
Incorrect HTTP Status Codes: Several error responses in
src/routes/profiles.pydon't match the requirements. For instance, an expired token should return401 Unauthorized(not400), and an attempt to create an existing profile should return400 Bad Request(not403). -
Incomplete Authorization: The authorization check in
src/routes/profiles.pyis missing the logic to allow admin users to create profiles for others.
Please focus on these points. Once they are resolved, your solution will be in great shape!
✨ 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
| sa.UniqueConstraint("user_id"), | ||
| sa.UniqueConstraint("user_id"), |
There was a problem hiding this comment.
You have a duplicated UniqueConstraint("user_id") here. One of these is redundant and should be removed to keep the table definition clean and correct.
| ) | ||
| user: Mapped[UserModel] = relationship("UserModel", back_populates="profile") | ||
|
|
||
| __table_args__ = (UniqueConstraint("user_id"),) |
There was a problem hiding this comment.
You've defined a unique constraint on the user_id column twice: once here in __table_args__ and also with unique=True in the mapped_column definition on line 153. While this works, it's redundant. It's generally better to define it in only one place. I'd recommend removing this __table_args__ definition and keeping the unique=True on the column itself.
src/routes/accounts.py
Outdated
| @@ -1,24 +1,30 @@ | |||
| from datetime import datetime, timezone | |||
| from typing import cast | |||
| from urllib import request | |||
There was a problem hiding this comment.
This urllib.request import is not used anywhere in the file and should be removed to keep the code clean.
src/routes/accounts.py
Outdated
| detail="An error occurred during user creation.", | ||
| ) from e | ||
| else: | ||
| activation_link = f"{BASE_URL}/register/" |
There was a problem hiding this comment.
The activation_link currently points back to the /register/ endpoint. According to the task requirements, you need to generate a valid URL for the action. A more appropriate link would direct the user towards the activation process, which is handled by the /activate/ endpoint.
src/routes/accounts.py
Outdated
| await db.delete(token_record) | ||
| await db.commit() | ||
|
|
||
| login_link = f"{BASE_URL}/activate/" |
There was a problem hiding this comment.
The login_link here points back to the /activate/ endpoint. After an account is successfully activated, the user should be guided to the login page. Consider changing this link to point to the /login/ endpoint to improve the user experience.
src/routes/accounts.py
Outdated
| db.add(reset_token) | ||
| await db.commit() | ||
|
|
||
| reset_link = f"{BASE_URL}/password-reset/request/" |
There was a problem hiding this comment.
The reset_link points to the endpoint for requesting a password reset, but it should guide the user to where they can complete the reset. This link should ideally point towards the page or endpoint for completing the password reset, which is /reset-password/complete/.
src/routes/accounts.py
Outdated
| detail="An error occurred while resetting the password.", | ||
| ) | ||
|
|
||
| login_link = f"{BASE_URL}/reset-password/complete/" |
There was a problem hiding this comment.
This login_link points to the /reset-password/complete/ endpoint. After a successful password reset, the user's next step is typically to log in. The link should probably point to the /login/ endpoint.
src/routes/profiles.py
Outdated
| @router.post("/users/me/avatar/", response_model=ProfileResponseSchema) | ||
| async def upload_avatar( | ||
| file: UploadFile = File(...), | ||
| db: AsyncSession = Depends(get_db), | ||
| token: str = Depends(get_token), | ||
| jwt_manager: JWTAuthManagerInterface = Depends(get_jwt_auth_manager), | ||
| s3_storage_client: S3StorageInterface = Depends(get_s3_storage_client), | ||
| ): |
There was a problem hiding this comment.
This endpoint for uploading an avatar is not what the task requires. The requirement is to create a complete user profile, including the avatar, via a single POST request to /users/{user_id}/profile/. The logic within this function for handling file validation and upload should be moved into the user_profile function.
src/routes/profiles.py
Outdated
| async def user_profile( | ||
| user_id: str, | ||
| user_data: ProfileRequestSchema, | ||
| db: AsyncSession = Depends(get_db), | ||
| token: str = Depends(get_token), | ||
| jwt_manager: JWTAuthManagerInterface = Depends(get_jwt_auth_manager), | ||
| ): |
There was a problem hiding this comment.
The function signature for user_profile needs to be adjusted to handle a multipart/form-data request, which is necessary for file uploads. Instead of receiving a single Pydantic model user_data, you should use Form() for each text field (e.g., first_name: str = Form(...)) and UploadFile for the avatar (e.g., avatar: UploadFile = File(...)).
src/routes/profiles.py
Outdated
| except TokenExpiredError: | ||
| raise HTTPException(status_code=400, detail="Token has expired.") | ||
| except InvalidTokenError: | ||
| raise HTTPException(status_code=400, detail="Invalid token") |
There was a problem hiding this comment.
The task requirements state that expired or invalid tokens should return a 401 Unauthorized status code. This implementation incorrectly returns 400 Bad Request.
src/routes/profiles.py
Outdated
| if int(user_id) != decode_token["user_id"]: | ||
| raise HTTPException( | ||
| status_code=403, detail="You don't have permission to edit this profile." | ||
| ) |
There was a problem hiding this comment.
This authorization check is a good start, but it's incomplete. The requirements specify that a user can only create their own profile unless they have elevated permissions (e.g., are an admin). You need to add a check here to see if the user making the request is an admin, and if so, allow them to proceed even if the user_id doesn't match their own.
src/routes/profiles.py
Outdated
| user_profile_db = result.scalar_one_or_none() | ||
|
|
||
| if user_profile_db: | ||
| raise HTTPException(status_code=403, detail="User already has a profile.") |
There was a problem hiding this comment.
According to the task requirements, if a profile already exists, the server should respond with a 400 Bad Request. This implementation incorrectly returns a 403 Forbidden.
src/routes/profiles.py
Outdated
| gender=user_data.gender, | ||
| date_of_birth=user_data.date_of_birth, | ||
| info=user_data.info, | ||
| avatar="", |
There was a problem hiding this comment.
This line demonstrates that the avatar upload functionality is missing from the profile creation endpoint. The avatar file should be received, uploaded to MinIO storage, and the resulting URL should be stored here instead of an empty string.
src/schemas/profiles.py
Outdated
| class ProfileRequestSchema(BaseModel): | ||
| first_name: str | ||
| last_name: str | ||
| gender: GenderEnum | ||
| date_of_birth: date | ||
| info: str | ||
| avatar: str |
There was a problem hiding this comment.
This schema is defined to parse a JSON request body. However, the requirement to upload an avatar image means the endpoint must handle multipart/form-data. A Pydantic model used as a request body cannot handle file uploads. You should adjust the endpoint signature to accept form fields and a file upload separately using Form() and File() from FastAPI.
src/schemas/profiles.py
Outdated
| gender: GenderEnum | ||
| date_of_birth: date | ||
| info: str | ||
| avatar: str |
There was a problem hiding this comment.
The avatar field is defined as a str, which is incorrect for a file upload. The endpoint should expect an UploadFile object for the avatar. This field type needs to be reconsidered as part of handling multipart/form-data.
src/schemas/profiles.py
Outdated
| # @field_validator("avatar") | ||
| # @classmethod | ||
| # def check_date_of_birth(cls, value): | ||
| # return validate_image(value) |
There was a problem hiding this comment.
This commented-out code appears to be an attempt at avatar validation. However, there are two issues here:
- The function name
check_date_of_birthis a copy-paste error. - More importantly,
validate_imageexpects anUploadFileobject. Since theavatarfield is defined as astr, this validator would fail if it were active because it would receive a string, not a file object.
| "id": 1, | ||
| "genre": "Comedy" | ||
| } | ||
| genre_schema_example = {"id": 1, "genre": "Comedy"} |
There was a problem hiding this comment.
The key in this dictionary should be "name" instead of "genre" to match the GenreSchema definition in schemas/movies.py.
| Key=file_name, | ||
| Body=file_data, | ||
| ContentType="image/jpeg" | ||
| ContentType="image/jpeg", |
There was a problem hiding this comment.
The ContentType is hardcoded to "image/jpeg". While this will work for JPEG files, the requirements also specify support for PNG images. To handle different file types correctly, it would be better to pass the file's content type from the route handler to this method. This ensures the correct metadata is stored in MinIO, which can be important for how browsers render the image.
| if re.search(r'^[A-Za-z]*$', name) is None: | ||
| raise ValueError(f'{name} contains non-english letters') | ||
| def validate_name(name: str) -> str: | ||
| if re.search(r"^[A-Za-z]*$", name) is None: |
There was a problem hiding this comment.
The regex r"^[A-Za-z]*$" allows an empty string as a valid name because * matches zero or more characters. A name field should likely not be empty. Consider using + instead (r"^[A-Za-z]+$") to require at least one letter.
| raise ValueError('Invalid birth date - year must be greater than 1900.') | ||
| raise ValueError("Invalid birth date - year must be greater than 1900.") | ||
|
|
||
| age = (date.today() - birth_date).days // 365 |
There was a problem hiding this comment.
Calculating age by dividing days by 365 can be inaccurate because of leap years. A more precise method involves comparing the year, month, and day directly. Consider this approach:
today = date.today()
age = today.year - birth_date.year - ((today.month, today.day) < (birth_date.month, birth_date.day))
src/validation/profile.py
Outdated
| def validate_info(info: str) -> str: | ||
| if not info: | ||
| raise ValueError("Info cannot be empty") | ||
| if " " * len(info) == info: |
There was a problem hiding this comment.
While this check for an all-whitespace string is functional, a more idiomatic and readable way to do this in Python is to use the str.isspace() method. For example: if info.isspace():.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great work on refactoring the profile creation endpoint to handle multipart/form-data! However, this requires a few changes as some critical functionality is not working as required, which is causing the tests to fail.
Please address the following blockers:
- Incorrect Email URLs: The notification links in
src/routes/accounts.pyare still pointing to the wrong endpoints. For example, the activation link should go to/activate/, not/register/. - Incorrect Avatar URL: The profile creation response in
src/routes/profiles.pyreturns the internal file path for the avatar. It needs to return the full public URL, which you can generate usings3_storage_client.get_file_url(). - Profile Response Error: The API will fail to serialize the profile response because
ProfileResponseSchemainsrc/schemas/profiles.pyis missingmodel_config = {"from_attributes": True}. - Flawed Gender Validation: The logic in
src/validation/profile.pyfor validating the gender is incorrect and will reject valid inputs.
Once these issues are resolved, your solution should 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
| sa.ForeignKeyConstraint(["user_id"], ["users.id"], ondelete="CASCADE"), | ||
| sa.PrimaryKeyConstraint("id"), | ||
| sa.UniqueConstraint("user_id"), | ||
| sa.UniqueConstraint("user_id"), |
There was a problem hiding this comment.
There's a duplicate UniqueConstraint defined for the user_id column. You should remove one of them to avoid redundancy in the table definition.
| ) | ||
| user: Mapped[UserModel] = relationship("UserModel", back_populates="profile") | ||
|
|
||
| __table_args__ = (UniqueConstraint("user_id"),) |
There was a problem hiding this comment.
You've defined the uniqueness for user_id in two different ways: once with unique=True inside mapped_column and again using UniqueConstraint in __table_args__. This is redundant.
For a single-column unique constraint, simply using unique=True on the column definition is sufficient and more concise. You can safely remove the __table_args__ tuple.
| await db.commit() | ||
| await db.refresh(user_profile) | ||
|
|
||
| return user_profile |
There was a problem hiding this comment.
The function currently returns the SQLAlchemy user_profile model directly. In this model, the avatar attribute holds the filename (e.g., avatars/1_avatar.jpg), not the full URL.
The task requires the final response to include the complete, publicly accessible URL for the avatar. You should use the s3_storage_client.get_file_url() method to generate this URL and include it in the response. A simple way to fix this is to update the avatar attribute on the user_profile object with the full URL before returning it.
src/routes/accounts.py
Outdated
| detail="An error occurred during user creation.", | ||
| ) from e | ||
| else: | ||
| activation_link = f"{BASE_URL}/register/" |
There was a problem hiding this comment.
The activation_link is pointing to the registration endpoint (/register/). According to the requirements and user flow, this link should direct the user to the account activation endpoint. Please correct this to point to the /activate/ route.
src/routes/accounts.py
Outdated
| await db.delete(token_record) | ||
| await db.commit() | ||
|
|
||
| login_link = f"{BASE_URL}/activate/" |
There was a problem hiding this comment.
After successful activation, the login_link should direct the user to the login page. Currently, it points back to the /activate/ endpoint, which isn't logical for a user who has just completed activation. Please update this to point to the /login/ endpoint.
src/routes/accounts.py
Outdated
| detail="An error occurred while resetting the password.", | ||
| ) | ||
|
|
||
| login_link = f"{BASE_URL}/reset-password/complete/" |
There was a problem hiding this comment.
The login_link sent after a successful password reset is pointing to the password reset completion endpoint (/reset-password/complete/). After resetting their password, the user should be directed to the /login/ endpoint to sign in with their new credentials.
| Key=file_name, | ||
| Body=file_data, | ||
| ContentType="image/jpeg" | ||
| ContentType="image/jpeg", |
There was a problem hiding this comment.
The ContentType is currently hardcoded to image/jpeg. According to the requirements in validation/profile.py, you also support PNG files. Uploading a PNG file with an incorrect content type might cause rendering issues in some browsers.
Consider making the ContentType dynamic. You can get the content type from the UploadFile object in the route handler (e.g., avatar.content_type) and pass it to this upload_file method.
| def validate_name(name: str): | ||
| if re.search(r'^[A-Za-z]*$', name) is None: | ||
| raise ValueError(f'{name} contains non-english letters') | ||
| if re.search(r"^[A-Za-z]*$", name) is None: |
There was a problem hiding this comment.
The current regular expression r"^[A-Za-z]*$" allows an empty string as a valid name because * matches zero or more characters. While Form(...) makes the field required, an empty string can still be submitted. To ensure the name is not empty, you should use + instead of * (e.g., r"^[A-Za-z]+$").
|
|
||
| def validate_gender(gender: str) -> None: | ||
| def validate_gender(gender: str): | ||
| if gender not in GenderEnum.__members__.values(): |
There was a problem hiding this comment.
The check gender not in GenderEnum.__members__.values() is logically incorrect. It compares the input string gender with the enum member objects (e.g., <GenderEnum.MAN: 'man'>), not their string values. Since GenderEnum inherits from str, you can perform this check more simply and correctly with if gender not in GenderEnum:.
| raise ValueError('Invalid birth date - year must be greater than 1900.') | ||
| raise ValueError("Invalid birth date - year must be greater than 1900.") | ||
|
|
||
| age = (date.today() - birth_date).days // 365 |
There was a problem hiding this comment.
Calculating age by dividing the total number of days by 365 is not entirely accurate because it doesn't account for leap years. A more precise way to calculate age is to compare the year, month, and day components of the dates.
Consider this more robust approach:
today = date.today()
age = today.year - birth_date.year - ((today.month, today.day) < (birth_date.month, birth_date.day))
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent progress on the user profile and notification features! I am approving this submission as an educational override to help you move forward, but there are a few critical issues that need your attention.
Your profile creation endpoint will fail at runtime because the ProfileResponseSchema in src/schemas/profiles.py is missing model_config = {"from_attributes": True}. Additionally, the gender validation in src/validation/profile.py is flawed; comparing a string to GenderEnum.__members__.values() will always fail. A correct approach is to check if the string value exists in the enum itself, for example if gender not in GenderEnum:.
Lastly, the email notification links in src/routes/accounts.py point to the wrong endpoints, which prevents the activation and password reset flows from working. You've built a solid foundation, and addressing these points will make your features fully functional. Keep up the great work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| sa.ForeignKeyConstraint(["user_id"], ["users.id"], ondelete="CASCADE"), | ||
| sa.PrimaryKeyConstraint("id"), | ||
| sa.UniqueConstraint("user_id"), | ||
| sa.UniqueConstraint("user_id"), |
There was a problem hiding this comment.
This unique constraint on user_id is a duplicate of the one on the line above. While it might not cause issues in all database systems, it's redundant and should be removed to keep the migration clean.
| ) | ||
| user: Mapped[UserModel] = relationship("UserModel", back_populates="profile") | ||
|
|
||
| __table_args__ = (UniqueConstraint("user_id"),) |
There was a problem hiding this comment.
You have a unique constraint defined here in __table_args__, but you also have unique=True on the user_id column definition above. This is redundant. It's best practice to define the constraint in only one place. I'd recommend removing this line and relying on the unique=True parameter on the mapped_column for clarity.
| validate_name(first_name) | ||
| validate_name(last_name) | ||
| validate_birth_date(date_of_birth) | ||
| validate_gender(gender) |
There was a problem hiding this comment.
The validation logic in the validate_gender function you are calling here is incorrect. As mentioned in the previous review, it will reject valid inputs like 'man' or 'woman'. Please review the validation function to ensure it correctly compares the input string against the allowed enum values.
|
|
||
| @router.post( | ||
| "/users/{user_id}/profile/", | ||
| response_model=ProfileResponseSchema, |
There was a problem hiding this comment.
This endpoint will fail with a serialization error because the ProfileResponseSchema is not correctly configured to work with ORM models. As pointed out in the previous review, you must add model_config = {"from_attributes": True} to the schema definition in src/schemas/profiles.py to allow Pydantic to create the response from your UserProfileModel instance.
src/routes/profiles.py
Outdated
| token: str = Depends(get_token), | ||
| jwt_manager: JWTAuthManagerInterface = Depends(get_jwt_auth_manager), | ||
| s3_storage_client: S3StorageInterface = Depends(get_s3_storage_client), | ||
| user_data: str = Depends(check_field), |
There was a problem hiding this comment.
The type hint for user_data should be tuple instead of str, as the check_field dependency function returns a tuple of the validated form fields.
src/routes/accounts.py
Outdated
| db.add(reset_token) | ||
| await db.commit() | ||
|
|
||
| reset_link = f"{BASE_URL}/password-reset/request/" |
There was a problem hiding this comment.
The link for a password reset request should point to the endpoint where the user can complete the password reset (e.g., /reset-password/complete/). This URL incorrectly points back to the endpoint that requests the reset.
src/routes/accounts.py
Outdated
| detail="An error occurred while resetting the password.", | ||
| ) | ||
|
|
||
| login_link = f"{BASE_URL}/reset-password/complete/" |
There was a problem hiding this comment.
After successfully resetting a password, the user should be directed to the login page. This URL currently points back to the password reset completion endpoint itself.
| class ProfileResponseSchema(BaseModel): | ||
| id: int | ||
| user_id: int | ||
| first_name: str | ||
| last_name: str | ||
| gender: GenderEnum | ||
| date_of_birth: date | ||
| info: str | ||
| avatar: str |
There was a problem hiding this comment.
This schema is used to serialize a database model (UserProfileModel) into a JSON response. To allow Pydantic to read the data from the model's attributes, you need to enable from_attributes mode. As mentioned in the previous review, you can fix this by adding model_config = {"from_attributes": True} inside the class.
| Key=file_name, | ||
| Body=file_data, | ||
| ContentType="image/jpeg" | ||
| ContentType="image/jpeg", |
There was a problem hiding this comment.
The ContentType is hardcoded as image/jpeg. According to the requirements and validation logic, PNG files are also supported. To ensure browsers can correctly render all uploaded image types, the content type should be set dynamically. You could pass the avatar.content_type from the UploadFile object in the route handler down to this upload_file method.
|
|
||
| def validate_gender(gender: str) -> None: | ||
| def validate_gender(gender: str): | ||
| if gender not in GenderEnum.__members__.values(): |
There was a problem hiding this comment.
The logic to validate the gender is still incorrect. GenderEnum.__members__.values() returns a collection of enum members (e.g., <GenderEnum.MAN: 'man'>), not the string values themselves. As a result, checking if a string like 'man' is in this collection will always fail.
A simpler and correct way to perform this check is if gender not in GenderEnum:, because GenderEnum inherits from str.
No description provided.