feat: add user profiles with JWT auth and S3 uploads#248
feat: add user profiles with JWT auth and S3 uploads#248dmytro-malyk-dm wants to merge 3 commits intomate-academy:mainfrom
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
This requires a few changes before it can be approved. You've done an excellent job on the profile creation endpoint—the logic in src/routes/profiles.py and the schemas in src/schemas/profiles.py are very well implemented.
However, the core requirement to send email notifications for account actions is missing from src/routes/accounts.py. You'll need to update the register_user, activate_account, request_password_reset_token, and reset_password functions to inject the EmailSender and use BackgroundTasks to send emails asynchronously, as specified in the task description.
Once that's addressed, please also export ProfileCreateSchema in src/schemas/__init__.py for consistency. You're on the right track, and I look forward to seeing your revisions!
✨ 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 schemas.profiles import ( | ||
| ProfileResponseSchema, | ||
| ) |
There was a problem hiding this comment.
The ProfileCreateSchema is defined in schemas/profiles.py but is not exported here. It's good practice to export all public schemas from the package's __init__.py file for consistency.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent progress on the profile creation and email notifications! However, I'm requesting a couple of changes in the reset_password function in src/routes/accounts.py before this can be approved.
There are two issues that need to be addressed:
- Security Risk: The new password appears to be assigned directly to the user model. This is a security risk as it would store the password in plaintext. Please ensure you use the model's password hashing method (e.g.,
user.set_password()) to securely store the new password. - Missing Argument: The background task that sends the password reset confirmation email is missing the required
login_linkargument. This will cause the email notification to fail. Please add this argument to thesend_password_reset_complete_emailcall.
Once these two issues are fixed, this will be ready to merge. Great work overall!
✨ 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
|
|
||
| try: | ||
| user.password = data.password | ||
| user.password = reset_data.password |
There was a problem hiding this comment.
This line appears to assign the raw password directly to the model's password attribute. Since the application correctly hashes passwords (as seen in register_user and login_user), this will likely store the new password in plaintext, which is a security risk, or cause future logins to fail. Please ensure you are using the model's password hashing mechanism, which might be a method like user.set_password() or a property setter.
| background_tasks.add_task( | ||
| email_sender.send_password_reset_complete_email, | ||
| user.email | ||
| ) |
There was a problem hiding this comment.
The task description provides an example for this email notification that includes a login_link argument. Including a link to the login page is a good user experience after a password reset. Please add the login_link to this background task as shown in the requirements.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on the new profile features and for correctly implementing password hashing! I am approving this as an educational override to help you move forward, but there are a few critical test failures that need your attention before this is fully functional.
The main issues are:
- In
src/routes/accounts.py, thesend_activation_complete_emailcall is missing itslogin_linkargument. - The
reset_passwordfunction callsuser.update_password(), but this method needs to be added to theUserModelinsrc/database/models/accounts.py. - The profile endpoint is failing due to a database lazy-loading error in
src/security/dependencies.py. You can fix this by eager-loading thegrouprelationship withoptions(joinedload(UserModel.group))in your user query.
Fixing these items will get your tests passing. Great progress!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
No description provided.