Task by ma-narijore · Pull Request #263 · mate-academy/py-fastapi-homework-4-task

ma-narijore · 2026-02-17T19:43:58Z

No description provided.

mateacademy-ai-mentor

Review Summary

I am requesting changes because there are several critical, requirement-breaking issues that will prevent the task from being considered complete.

Critical blockers that must be fixed:

The profile feature is incomplete: the avatar field in src/schemas/profiles.py is declared but not validated, even though the requirements explicitly demand JPG/JPEG/PNG type and ≤1 MB using validate_image, and the Pydantic config mixes v1 (Config) and v2 (field_validator) styles, which can cause from_attributes to be ignored. Add an @field_validator("avatar") that calls validate_image (and safely handles None), and switch to the correct v2 configuration via model_config = {"from_attributes": True} (or the project’s chosen pattern).
In src/routes/accounts.py, the token helper and dependencies do not follow the required contract: there is no proper get_token(request: Request) that centrally extracts the Bearer token and distinguishes an expired token with the exact message "Token has expired.", and jwt_manager/s3_client are not wired via Depends(get_jwt_auth_manager) and Depends(get_s3_storage_client) as required. Also, you use payload['sub'] even though tokens are created with user_id, and you set avatar=None after upload instead of storing the real avatar_url, all of which will cause runtime errors or incorrect behavior.
The email flows are partially wrong: in account activation you call send_activation_email after activation completes instead of send_activation_complete_email, and in password reset completion you call send_password_reset_email instead of send_password_reset_complete_email, so users will receive the wrong templates for completion flows.
The required DI factory get_accounts_email_notificator is missing in src/notifications/emails.py, even though it is needed for dependency injection and tests; implement get_accounts_email_notificator(settings: BaseAppSettings = Depends(get_settings)) -> EmailSenderInterface and clean up or correctly use the related imports.
The PostgreSQL data volume mapping in docker-compose.yml points to /var/lib/postgresql/ instead of /var/lib/postgresql/data, which will likely break persistence and migrations; update that volume to target /var/lib/postgresql/data.

Minor but important improvements:

Clean up unused imports in src/schemas/profiles.py (Form, File, HTTPException) and in src/notifications/emails.py once the DI helper is implemented. Also, adjust the MailHog host in src/tests/test_e2e/test_email_notification.py to use settings.EMAIL_HOST instead of hardcoded localhost to avoid environment-specific flakiness.

Once you address these issues, your implementation will be much closer to the checklist requirements and should behave correctly end-to-end; the overall structure is good, so these are mainly targeted fixes to align with the spec and avoid runtime problems.

✨ Please rate this review in the Platform's notification center¹. 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 button² 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 tag³.
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.

mateacademy-ai-mentor · 2026-02-17T19:49:14Z

src/schemas/profiles.py

+    gender: str
+    date_of_birth: date
+    info: str
+    avatar: Optional[UploadFile] = None


The avatar field is declared but there is no validator for it. Per the requirements you must validate uploaded avatars (file type and size) by using validate_image (and handle the case when avatar is None). Add an @field_validator("avatar") to enforce the JPG/JPEG/PNG and 1MB limit and raise a ValueError on invalid files.

mateacademy-ai-mentor · 2026-02-17T19:49:14Z

src/schemas/profiles.py


+from typing import Optional
+
 from fastapi import UploadFile, Form, File, HTTPException


Form, File, and HTTPException are imported but not used in this module. Remove unused imports or use HTTPException/File/Form only if needed. Unused imports may be confusing and should be cleaned up.

mateacademy-ai-mentor · 2026-02-17T19:49:14Z

src/schemas/profiles.py

+    class Config:
+        from_attributes = True


You use field_validator (pydantic v2 style) but the response model sets class Config: from_attributes = True (pydantic v1 style). If the project uses pydantic v2, replace the Config class with model_config = {"from_attributes": True} or the v2-appropriate configuration. The current mix may lead to the from_attributes setting being ignored.

mateacademy-ai-mentor · 2026-02-17T19:49:14Z

src/routes/accounts.py

+async def get_current_user_payload(
+    authorization: str | None,
+    jwt_manager: JWTAuthManagerInterface,
+):


The function is named get_current_user_payload but the checklist requires a get_token style function that extracts the Bearer token from the request and raises the exact error messages/status codes. Either implement get_token(request: Request) as specified or rename/adjust to match the requirement and usage throughout the project.

mateacademy-ai-mentor · 2026-02-17T19:49:14Z

src/routes/accounts.py

+    except Exception:
+        raise HTTPException(401, "Invalid token.")


Catching a generic Exception and returning "Invalid token." does not match the checklist which requires treating an expired token specifically with the detail "Token has expired.". Inspect jwt_manager.decode_access_token exceptions (e.g., BaseSecurityError) and map expiration errors to the required message/status (401).

mateacademy-ai-mentor · 2026-02-17T19:49:15Z

src/notifications/emails.py

 from email.mime.multipart import MIMEMultipart

 import aiosmtplib
+from fastapi import Depends


Depends is imported but not used in this file. The dependency injection factory get_accounts_email_notificator required by the task should use Depends(get_settings) and therefore Depends should be used there (or remove this import if the DI function will be implemented in another module).

mateacademy-ai-mentor · 2026-02-17T19:49:15Z

src/notifications/emails.py

 from exceptions import BaseEmailError
 from notifications.interfaces import EmailSenderInterface

+# from config.dependencies import get_settings


A commented-out import for get_settings is present. The task explicitly requires a DI function get_accounts_email_notificator that depends on get_settings; ensure you implement that function (here or in the intended module) instead of leaving this commented out.

mateacademy-ai-mentor · 2026-02-17T19:49:15Z

src/notifications/emails.py

 from notifications.interfaces import EmailSenderInterface

+# from config.dependencies import get_settings
+from config.settings import BaseAppSettings


BaseAppSettings is imported but not used. The DI function get_accounts_email_notificator(settings: BaseAppSettings = Depends(get_settings)) should accept this type; either use it in that function or remove this import.

mateacademy-ai-mentor · 2026-02-17T19:49:15Z

src/notifications/emails.py


+# from config.dependencies import get_settings
+from config.settings import BaseAppSettings
+


This looks like a placeholder line — the file currently lacks the required get_accounts_email_notificator dependency provider described in the task. Implement that factory here (using get_settings) so other modules can Depends(get_accounts_email_notificator) as required by the checklist.

mateacademy-ai-mentor · 2026-02-17T19:49:15Z

src/tests/test_e2e/test_email_notification.py

    assert activated_user.is_active, f"User {user_email} is not active!"

-    mailhog_url = f"http://{settings.EMAIL_HOST}:{settings.MAILHOG_API_PORT}/api/v2/messages"
+    mailhog_url = f"http://localhost:{settings.MAILHOG_API_PORT}/api/v2/messages"


In the activation test you build MailHog URL using literal localhost while other tests use settings.EMAIL_HOST. This inconsistency can break when MailHog runs under a different host (e.g., in Docker Compose where the host is mailhog). Use the same settings.EMAIL_HOST as in other tests to avoid environment-dependent failures (e.g., replace localhost with settings.EMAIL_HOST).

mateacademy-ai-mentor

Review Summary

I am requesting changes because there are critical issues that block correct functionality. Specifically, in src/routes/accounts.py, you are using except jwt_manager.decode_access_token(token): as an exception type, which is invalid Python syntax and will cause a runtime error—this must be replaced with a proper exception class (such as ExpiredSignatureError or your JWT manager's custom exception). Additionally, the authentication dependency injection is not implemented as required: you should use a get_token(request: Request) helper to extract and validate the Bearer token, handling all required error cases and returning the user payload directly. There is also an unnecessary use of global file_name in the avatar upload handler, which should be removed to avoid concurrency and scoping bugs. Finally, after a password reset, you are sending the wrong email template—use send_password_reset_complete_email instead of send_password_reset_email. Please address these issues to ensure your code meets the requirements and runs without errors. The rest of your implementation is strong, so once these blockers are fixed, your submission should be ready for approval!

✨ Please rate this review in the Platform's notification center¹. 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 button² 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 tag³.
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.

mateacademy-ai-mentor · 2026-02-17T20:45:38Z

src/routes/accounts.py

+
+    try:
+        payload = jwt_manager.decode_access_token(token)
+    except jwt_manager.decode_access_token(token):


You cannot use except jwt_manager.decode_access_token(token): as an exception type. This will raise a syntax/runtime error. Instead, catch the relevant exception class (likely defined in your JWT manager/interface) that represents token expiration (e.g., ExpiredSignatureError or custom error) and handle with the exact required messages.

mateacademy-ai-mentor · 2026-02-17T20:45:38Z

src/routes/accounts.py

+    db: AsyncSession = Depends(get_db),
+    jwt_manager: JWTAuthManagerInterface = Depends(get_jwt_auth_manager),
+    s3_client: S3StorageInterface = Depends(get_s3_storage_client),
+    authorization: str | None = Depends(get_current_user_payload),


This dependency/double injection pattern is messy: authorization: str | None = Depends(get_current_user_payload) and then immediately calling payload = await get_current_user_payload(...). Instead, implement and use a get_token(request: Request) function (see requirements) that directly extracts and validates the Bearer token from the header, returning the user payload (or raising the exact required messages for missing, bad format, or expired). Then, use its output as a direct dependency here.

mateacademy-ai-mentor · 2026-02-17T20:45:38Z

src/routes/accounts.py

+        # generate filename
+        ext = avatar.filename.split(".")[-1]
+        file_name = f"{user_id}_{uuid.uuid4().hex}.{ext}"
+        global file_name


You do not need nor want to use global file_name here; file_name is scoped within the request handler. Remove this statement to avoid potential concurrency/scope bugs.

mateacademy-ai-mentor · 2026-02-17T20:45:38Z

src/routes/accounts.py

        await db.run_sync(lambda s: s.delete(token_record))
+
+        background_tasks.add_task(
+            email_sender.send_password_reset_email,


After password reset is complete, requirements say to send the password reset complete email template, but you are calling send_password_reset_email instead of send_password_reset_complete_email. Change to the correct template for spec compliance.

mateacademy-ai-mentor

Review Summary

I am approving your solution as an educational override so you can keep progressing while you address the remaining issues. You’ve done solid work wiring up the account routes, email notifications, and profile schemas, and you’re correctly using BackgroundTasks and the provided validation helpers for most fields. The main things to improve are: (1) token handling in src/routes/accounts.py — introduce a dedicated get_token(request: Request) helper and avoid calling get_current_user_payload both as a dependency and manually; (2) build activation and reset URLs that actually embed the generated tokens instead of using static links, so users can complete those flows; and (3) move avatar validation fully into ProfileCreateSchema and fix the optional avatar validator in src/schemas/profiles.py so it safely handles None and returns an Optional[UploadFile] [CHECKLIST ITEM #X]. Additionally, guard calls to s3_client.get_file_url so it’s only used when an avatar was uploaded, and clean up the flake8 issues (line length, blank lines, boolean comparisons, EOF newline) that are currently blocking CI. Overall, your structure and intent are good; tightening these details will make your implementation both robust and fully aligned with the task requirements.

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

Rate AI review example ↩

mateacademy-ai-mentor · 2026-02-17T21:10:29Z

src/routes/accounts.py

+async def get_current_user_payload(
+    authorization: str | None,
+    jwt_manager: JWTAuthManagerInterface,
+):
+    if not authorization:
+        raise HTTPException(401, "Authorization header is missing")
+
+    if not authorization.startswith("Bearer "):
+        raise HTTPException(
+            401,
+            "Invalid Authorization header format. Expected 'Bearer <token>'"
+        )
+
+    token = authorization.split(" ")[1]
+
+    try:
+        payload = jwt_manager.decode_access_token(token)
+    except TokenExpiredError:
+        raise HTTPException(401, "Token has expired.")
+    except Exception:
+        raise HTTPException(401, "Invalid token.")
+
+    return payload


This helper both extracts the header and decodes the token, but the task explicitly requires a separate helper that extracts the Bearer token from the Request (e.g. get_token(request: Request)) and the exact error messages/status codes. FastAPI will not inject the raw Authorization header string into this parameter automatically — consider implementing get_token(request: Request) that reads request.headers['Authorization'] (or use Header) and returns the token string, then use jwt_manager in a separate step to decode. Also keep responsibilities separated: token extraction vs token decoding.

mateacademy-ai-mentor · 2026-02-17T21:10:29Z

src/routes/accounts.py

+    authorization: str | None = Depends(get_current_user_payload),
+):
+    # ---------------- TOKEN VALIDATION ----------------
+    payload = await get_current_user_payload(authorization, jwt_manager)


You are using Depends(get_current_user_payload) in the endpoint signature (so FastAPI will call the dependency and pass its return value), but then you call get_current_user_payload(...) again manually (line 675). This is incorrect: when used as a Depends, you should accept the dependency return value (payload) directly and NOT call the dependency function again. Better: have a get_token(request: Request) dependency that returns the token string and then decode that token here with jwt_manager (or make a dependency that returns the decoded payload). The current double-call will cause type errors / runtime exceptions.

mateacademy-ai-mentor · 2026-02-17T21:10:29Z

src/routes/accounts.py

+
+        background_tasks.add_task(
+            email_sender.send_activation_email,
+            str(new_user.email),
+            reg_link
+        )


When sending activation emails you pass a static reg_link. The email must include a valid activation URL that contains the activation token (and typically the user's email or token param) so the user can activate their account. Construct a URL that embeds the activation token (e.g. f"{base_url}/accounts/activate/?token={activation_token.token}&email={new_user.email}") and pass that to the email sender.

mateacademy-ai-mentor · 2026-02-17T21:10:29Z

src/routes/accounts.py

+    background_tasks.add_task(
+        email_sender.send_password_reset_email,
+        str(user.email),
+        req_pass_link
+    )


Same issue for password reset: you send a static req_pass_link. The reset email must include a link that contains the password reset token (so the user can perform the reset). Build a link with the generated reset_token.token (and email) and pass it to the email sender instead of a static URL.

mateacademy-ai-mentor · 2026-02-17T21:10:29Z

src/routes/accounts.py

+    if avatar:
+        try:
+            await validate_image(avatar)
+        except Exception:
+            raise HTTPException(400, "Invalid avatar image")


You call await validate_image(avatar) here in the route. The task requires that profile validation (including image validation) be implemented in schemas/profiles.py using the validation functions imported from the validation package. Validate image either in the ProfileCreateSchema or ensure the schema is responsible for validating avatar. Also verify whether validate_image is async — if it is synchronous you should not await it.

mateacademy-ai-mentor · 2026-02-17T21:10:30Z

src/schemas/profiles.py

+    @field_validator("avatar")
+    @classmethod
+    def validate_avatar(cls, v) -> str:
+        validate_image(v)
+        return v


The validate_avatar method currently has a signature/return hint of -> str and immediately calls validate_image(v). Because avatar is optional, this validator will be called with v = None and validate_image(None) will likely raise. Change the validator to: 1) accept an Optional[UploadFile], 2) if v is None return None, 3) otherwise call validate_image(v) and return the UploadFile. Also update the return type to Optional[UploadFile] (or UploadFile) to match the field.

mateacademy-ai-mentor · 2026-02-17T21:10:30Z

src/tests/test_e2e/test_email_notification.py


    response = await e2e_client.post("/api/v1/accounts/register/", json=user_data)
-    assert response.status_code == 201, f"Expected 201, got {response.status_code}"
+    assert response.status_code == 201, f"Expected 201, got {response.json()}"


Don't call response.json() directly inside the assertion message. If the response body is not valid JSON this will raise an exception while building the assertion message and hide the real failure. Use response.text or guard the JSON parse (e.g., try/except) to provide a safe debug output.

mateacademy-ai-mentor · 2026-02-17T21:10:30Z

src/tests/test_e2e/test_email_notification.py


-    await e2e_db_session.commit()
-    e2e_db_session.expire_all()
+    await setup_e2e_db.commit()


You call await setup_e2e_db.commit() then setup_e2e_db.expire_all() without awaiting the latter. If setup_e2e_db is an AsyncSession, commit() is awaitable (ok) but check whether expire_all() is a coroutine in your session implementation; if it is await it as well. Also consider committing/expiring before querying MailHog or document why commit is done after — committing earlier may reduce race conditions between app writes and test assertions.

mateacademy-ai-mentor · 2026-02-17T21:10:30Z

src/tests/test_e2e/test_email_notification.py

-    await e2e_db_session.commit()
-    e2e_db_session.expire_all()
+    await setup_e2e_db.commit()
+    setup_e2e_db.expire_all()


If expire_all() is async in your session implementation you must await setup_e2e_db.expire_all(). If it's synchronous (regular expire_all()), current usage is fine — still add a brief comment to avoid confusion and prevent future regressions when switching session types.

mateacademy-ai-mentor · 2026-02-17T21:10:30Z

src/tests/test_e2e/test_email_notification.py

    - Verify the email sent confirms the activation and contains the expected details.
    """
-    user_email = "test@mate.com"
+    user_email = "test1@mate.com"


You're setting user_email here but earlier registration test used a similar email (test1@mate.com). Keep test data consistent or centralize test emails into fixtures/constants to avoid accidental mismatches across tests and make the flow clearer.

Oleh-Fr added 2 commits February 17, 2026 18:20

add email notifications

86c5e63

finish task

a23133f

mateacademy-ai-mentor suggested changes Feb 17, 2026

View reviewed changes

fix with AI

da4e149

ma-narijore requested a review from mateacademy-ai-mentor February 17, 2026 20:30

ma-narijore marked this pull request as draft February 17, 2026 20:43

ma-narijore marked this pull request as ready for review February 17, 2026 20:43

mateacademy-ai-mentor suggested changes Feb 17, 2026

View reviewed changes

fix with ai

9b7b030

ma-narijore requested a review from mateacademy-ai-mentor February 17, 2026 21:04

mateacademy-ai-mentor approved these changes Feb 17, 2026

View reviewed changes


		from typing import Optional

		from fastapi import UploadFile, Form, File, HTTPException


		# from config.dependencies import get_settings
		from config.settings import BaseAppSettings

Conversation

ma-narijore commented Feb 17, 2026

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants