Skip to content

Solution#251

Open
Ann-Riabykina wants to merge 4 commits intomate-academy:mainfrom
Ann-Riabykina:develop
Open

Solution#251
Ann-Riabykina wants to merge 4 commits intomate-academy:mainfrom
Ann-Riabykina:develop

Conversation

@Ann-Riabykina
Copy link

No description provided.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

Excellent work on implementing the email notifications with background tasks and setting up the profile creation endpoint. The overall structure is solid. However, a critical change is needed before this can be approved.

The age validation logic in src/validation/profile.py is incorrect. Calculating age with (date.today() - birth_date).days // 365 doesn't account for leap years and can fail to enforce the 18+ requirement. Please implement a more robust check that correctly compares the full birth date against today's date.

Additionally, a smaller issue to address is in src/routes/profiles.py, where the avatar file extension is hardcoded to .jpg. It would be better to preserve the original file's extension from the upload to handle different image types like PNG correctly. Once the age validation is fixed, this will be ready for approval.


✨ 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

detail="User already has a profile.",
)

avatar_key = f"avatars/{user_id}_avatar.jpg"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The file extension for the avatar key is hardcoded as .jpg. The validation logic allows for other image types like PNG. This might lead to a content-type mismatch where a PNG file is served with a .jpg extension. It would be more robust to preserve the original file's extension. You could extract it from payload.avatar.filename.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

Good progress on the profile creation and email notifications. However, changes are needed because several critical issues are causing the automated tests to fail and a key requirement is not met.

Please address the following blockers:

  1. Incorrect Age Validation: The age calculation in src/validation/profile.py is still inaccurate and doesn't reliably enforce the 18+ requirement. This critical issue from the previous review must be fixed.
  2. Failing Account Tests: The normalize_email validators in src/schemas/accounts.py are causing a TypeError. They should return the simple lowercased string, like return value.lower(), not return EmailStr(...).
  3. Incorrect Avatar Filename: In src/routes/profiles.py, you're creating filenames with a double dot (e.g., _avatar..jpg). The os.path.splitext() function already includes the dot, so you should change your f-string to f"avatars/{user_id}_avatar{ext.lower()}".

Once these are fixed, the tests should pass.


✨ 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

_, ext = os.path.splitext(filename)
ext = ext.lower()

avatar_key = f"avatars/{user_id}_avatar.{ext}"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The os.path.splitext() function returns the extension with a leading dot (e.g., '.jpg'). By adding another dot in this f-string, the resulting filename will be incorrect, such as ...avatar..jpg. Please remove the extra dot to fix the key generation.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

Great job on implementing the email notifications and the profile creation logic! Your work on using BackgroundTasks and handling file uploads is excellent.

I am approving this submission as an educational override to help you move forward, but there are two critical issues from the previous review that still need your attention. First, the age validation in src/validation/profile.py is inaccurate because dividing by 365 doesn't account for leap years. Second, all five normalize_email validators in src/schemas/accounts.py are still causing a TypeError. Please ensure they return a simple string by using return str(value).lower() and updating the type hint to -> str.

Please make sure to apply these fixes in your next steps. Keep up the great work!


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Footnotes

  1. Rate AI review example

Comment on lines +16 to +17
def normalize_email(cls, value: EmailStr) -> EmailStr:
return EmailStr(str(value).lower())

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This validator was not fixed as requested in the previous review. To resolve the TypeError, it should return a simple string, not an EmailStr object. Please change the implementation to return str(value).lower() and update the return type hint to -> str.

Comment on lines +39 to +40
def normalize_email(cls, value: EmailStr) -> EmailStr:
return EmailStr(str(value).lower())

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the same issue as in the UserRegistrationRequestSchema. This validator should return a simple string and the type hint should be updated to str to avoid the TypeError.

Comment on lines +48 to +49
def normalize_email(cls, value: EmailStr) -> EmailStr:
return EmailStr(str(value).lower())

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This validator also needs to be corrected. Please return a simple lowercase string (str) instead of an EmailStr object.

Comment on lines +59 to +60
def normalize_email(cls, value: EmailStr) -> EmailStr:
return EmailStr(str(value).lower())

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please apply the same fix here as for the other normalize_email validators: return a simple str.

Comment on lines +75 to +76
def normalize_email(cls, value: EmailStr) -> EmailStr:
return EmailStr(str(value).lower())

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This normalize_email validator also needs to be fixed. It should return a simple lowercase string to prevent the TypeError.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants