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 the profile creation endpoint—the implementation is excellent. However, changes are needed for the email notification functionality before this can be approved. Currently, all emails in src/routes/accounts.py are sent synchronously, which blocks the API response; a core requirement is to send these asynchronously using BackgroundTasks. Additionally, the account activation and password reset links are not functional because they are missing the unique tokens. Please ensure the generated token is appended to the URL to make them work. Finally, the validation logic in src/schemas/profiles.py incorrectly reports error locations for name fields, which should also be corrected. Addressing these issues will complete the task requirements.
✨ 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/schemas/profiles.py
Outdated
| status_code=422, | ||
| detail=[{ | ||
| "type": "value_error", | ||
| "loc": ["first_name" if "first_name" in name else "last_name"], |
There was a problem hiding this comment.
The logic for determining the error location (loc) is incorrect. The name variable holds the field's value (e.g., "John"), not its name. This condition checks if the string "first_name" is a substring of the user's provided name, which is not the intended behavior and will lead to incorrect error reporting. Consider splitting this into two separate validator methods, one for first_name and one for last_name, to correctly specify the loc.
| async def register_user( | ||
| user_data: UserRegistrationRequestSchema, | ||
| db: AsyncSession = Depends(get_db), | ||
| email_sender: EmailSenderInterface = Depends(get_accounts_email_notificator), | ||
| ) -> UserRegistrationResponseSchema: |
There was a problem hiding this comment.
According to the task requirements, email notifications must be sent asynchronously. Please add background_tasks: BackgroundTasks to the function parameters and use background_tasks.add_task() later to send the email, instead of calling it directly with await.
src/routes/accounts.py
Outdated
| detail="An error occurred during user creation." | ||
| ) from e | ||
| else: | ||
| activation_link = "http://127.0.0.1/accounts/activate/" |
There was a problem hiding this comment.
The activation_link must include the generated token to be functional for the user. You'll need to retrieve the token from the activation_token object created earlier and append it to this URL, perhaps as a query parameter. You might need to refresh the activation_token object after the database commit to access its value.
| async def activate_account( | ||
| activation_data: UserActivationRequestSchema, | ||
| db: AsyncSession = Depends(get_db), | ||
| email_sender: EmailSenderInterface = Depends(get_accounts_email_notificator), | ||
| ) -> MessageResponseSchema: |
There was a problem hiding this comment.
Similar to the registration endpoint, this function should use BackgroundTasks to send the completion email asynchronously. Please inject BackgroundTasks and use background_tasks.add_task() for sending the email.
| async def request_password_reset_token( | ||
| data: PasswordResetRequestSchema, | ||
| db: AsyncSession = Depends(get_db), | ||
| email_sender: EmailSenderInterface = Depends(get_accounts_email_notificator) | ||
| ) -> MessageResponseSchema: |
There was a problem hiding this comment.
This function should use BackgroundTasks for sending the password reset email asynchronously. Please update the function signature to include background_tasks: BackgroundTasks and use background_tasks.add_task() to dispatch the email.
src/routes/accounts.py
Outdated
| db.add(reset_token) | ||
| await db.commit() | ||
|
|
||
| password_reset_complete_link = "http://127.0.0.1/accounts/password-reset-complete/" |
There was a problem hiding this comment.
This password reset link is not functional because it's missing the unique reset token. To make it work, you need to include the token from the reset_token object in the URL.
| async def reset_password( | ||
| data: PasswordResetCompleteRequestSchema, | ||
| db: AsyncSession = Depends(get_db), | ||
| email_sender: EmailSenderInterface = Depends(get_accounts_email_notificator) | ||
| ) -> MessageResponseSchema: |
There was a problem hiding this comment.
The password reset completion email should be sent in the background. Please inject BackgroundTasks into this function and use background_tasks.add_task() to send the notification asynchronously.
src/schemas/__init__.py
Outdated
| TokenRefreshRequestSchema, | ||
| TokenRefreshResponseSchema | ||
| ) | ||
| ) No newline at end of file |
There was a problem hiding this comment.
Good job exposing the account schemas. However, you've missed exporting the new profile schemas (ProfileCreateSchema, ProfileResponseSchema) from schemas.profiles. To keep the project structure consistent, they should be imported here as well.
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 pull request! I am approving your changes as you've successfully implemented all the core requirements for the profile creation endpoint and fixed the previous issues with asynchronous emails. The validation, authorization, and file upload logic are all working correctly.
One minor point for future reference: please remember to export new schemas from the top-level package. In src/schemas/__init__.py, you should expose the new profile schemas to maintain consistency with the project's structure. This is not a blocker, but it's an important convention.
Great job overall!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
src/schemas/__init__.py
Outdated
| TokenRefreshRequestSchema, | ||
| TokenRefreshResponseSchema | ||
| ) | ||
| ) No newline at end of file |
There was a problem hiding this comment.
The new schemas defined in schemas/profiles.py should be imported and exposed here, just like the schemas from accounts and movies.
2 participants
No description provided.