Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
This submission requires a few changes. While you've done a great job implementing the email notifications and setting up the profile schemas, there are some critical issues with the new profile creation endpoint that block its functionality.
Here are the key issues to address:
- Incorrect Authentication Logic: In
src/routes/profiles.py, theget_current_userdependency is trying to authenticate using a refresh token from the database. Authentication for API endpoints should be done by validating a short-lived JWT access token passed in theAuthorizationheader. The logic for parsing this header is also incorrect. - Critical Typo: In
src/schemas/profiles.py, the dependency function is namedprofile_sсhemawith a Cyrillic 'с' instead of a Latin 'c'. This will cause aNameErrorat runtime and must be corrected toprofile_schema. - Incorrect Import Paths: Several files, like
src/database/__init__.pyandsrc/tests/conftest.py, use absolute imports starting withsrc.(e.g.,from src.database...). Sincesrcis the source root, these should be changed to relative imports (e.g.,from database...) to preventModuleNotFoundError.
Please fix these issues to get the profile endpoint working as expected.
✨ 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 bearer != "Bearer" or len(bearer) != 2: | ||
| raise HTTPException(status_code=status.HTTP_401_UNAUTHORIZED, detail="Invalid Authorization header format. Expected 'Bearer <token>'") | ||
|
|
||
| stmt = select(RefreshTokenModel).where(RefreshTokenModel.token == bearer[1]) |
There was a problem hiding this comment.
The authentication logic is trying to validate a RefreshToken from the Authorization header. According to standard OAuth2/JWT patterns, this header should contain a short-lived access token. Refresh tokens are typically used only on a specific endpoint (e.g., /refresh) to get a new access token and should not be used to authenticate general API requests.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great progress on this task! The email notifications and profile schemas are well-implemented. However, there are a couple of critical issues in src/routes/profiles.py that are blocking the main functionality and need to be addressed before this can be approved.
- Incorrect Authentication Logic: The
get_current_userdependency is still trying to find a token in the database by querying for anAccessTokenModel. Authentication must be stateless. Please update this to parse theBearertoken from the header and validate it usingjwt_manager.decode_access_token(). - Redundant Data Fetching: In the
create_userendpoint, the dependencydata = Depends(profile_schema)already provides the parsed profile and avatar. The subsequent call to the dependency function is incorrect and will cause an error. You should use thedatavariable directly:profile, avatar = data.
Once these two blockers are fixed, the profile creation feature should work correctly. 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
| await db.delete(token_record) | ||
| await db.commit() | ||
|
|
||
| activate_account_link = "http://127.0.0.1/accounts/activate_account/" |
There was a problem hiding this comment.
This URL for the activation completion email seems to point to a non-existent endpoint. For a confirmation email like this one, it's generally more helpful for the user to provide a link to the login page so they can proceed with their next action.
| try: | ||
| user.password = data.password | ||
|
|
||
| reset_link = f"http://127.0.0.1/accounts/complete-reset-password/{token_record.token}" |
There was a problem hiding this comment.
This URL includes the password reset token, but the token is invalidated and deleted just a few lines below. This means the link sent to the user will be broken. Since this is a confirmation email, consider providing a link to the login page instead, which would be a more useful and functional URL for the user.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your submission as an educational override so you can proceed, but there are a couple of important points to address. You've done an excellent job implementing the email notifications and the profile validation schemas! The main issue is in src/routes/profiles.py, where JWTAuthManager.decode_access_token is being called incorrectly as a static method. This method requires an instance of the JWTAuthManager to access the secret key. You can fix this by injecting it as a dependency into get_current_user, just as you did in routes/accounts.py. Additionally, the error detail for an expired token should be updated to exactly match the requirement: "Token has expired.". Great progress overall!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
No description provided.