Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there are several critical issues that will break functionality or fail the required behavior. First, in src/routes/profiles.py you call fetch_user_by_id but never check if the user exists and is active; this can raise an error and also violates the requirement to return 401 with "User not found or not active." when the target user is missing or inactive. The same profile route also saves and returns the raw avatar storage key instead of generating and returning the final S3/MinIO URL, and the response schema is missing id and user_id, so the API output does not match the specified contract. In src/routes/accounts.py there is an invalid from fastapi import ... requests import that will cause an import-time crash, and both activation and password reset emails send static links without embedding the created tokens, so users (and tests) cannot actually activate accounts or complete password resets. Additionally, get_user in src/routes/dependencies.py does not validate that user_id is present in the decoded token before querying, and several validators in src/validation/profile.py (name regex allowing empty strings, incorrect gender membership check, fragile image format handling) do not fully enforce the stated rules.
To move this forward, please: (1) add the explicit user existence/active check in the profile route and return the correct 401 error when needed; (2) after uploading the avatar, obtain the public URL from your S3 client, assign that to the profile, and ensure ProfileResponseSchema includes id and user_id; (3) fix the accounts.py import and construct activation and reset links that embed the actual tokens you create, passing those full URLs into the background email tasks; (4) in get_user, check that decoded_token["user_id"] exists and raise a clear 401 if missing; and (5) tighten the profile validators as suggested (use re.fullmatch for names, compare gender to the enum .values, normalize and robustly compare image formats). Once these are addressed, your overall structure (dependencies, routes, and use of BackgroundTasks and S3 client) should satisfy the task requirements much more reliably—you're very close, and these fixes will solidify the behavior and make the tests 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
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there are several critical issues that will break required flows and API contracts. Most importantly, the profile creation endpoint returns data built from the incoming form instead of the persisted new_profile, and the ProfileResponseSchema is missing the required id and user_id fields; combined, this means clients/tests will not receive the correct saved profile data. The profile route also relies on a Pydantic model with UploadFile via Form(), which FastAPI does not populate reliably for multipart requests, so you should switch to individual form parameters plus avatar: UploadFile = File(...) (or a small dependency that constructs ProfileCreateSchema). In the password reset flow, the reset URL is malformed (.../token?={token} without a proper query name or email), so users/tests cannot complete the reset via the emailed link, and some token cleanup paths use db.run_sync instead of the async session’s await db.delete(...), which is unsafe. In the validators, the name check allows empty strings, gender comparison is done against enum members rather than their values, image format handling doesn’t normalize Pillow’s image.format, and the birth-date check lacks a future-date guard and uses an imprecise age calculation, all of which can cause valid input to be rejected or invalid input to pass.
To move this forward, you should: (1) update ProfileResponseSchema to include id and user_id and build the response from the saved new_profile while returning the public S3 URL for avatar; (2) adjust the profile route signature to handle form fields and UploadFile correctly so avatar.read() and validation work reliably; (3) fix the password reset URL to something like /reset-password/complete/?email={email}&token={token} and send that in the email; (4) replace run_sync deletions with proper async await db.delete(...) calls; and (5) correct the name, gender, image, and birth-date validators as noted. Once these are addressed, your implementation should satisfy the functional requirements and the end‑to‑end tests are much more likely to pass; the rest of the structure (JWT handling, S3 integration, BackgroundTasks for email) looks solid and close to complete.
✨ 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
|
|
||
| @router.post( | ||
| "/users/{user_id}/profile/", | ||
| response_model=ProfileResponseSchema, |
There was a problem hiding this comment.
The response_model=ProfileResponseSchema is declared here, but the schema currently does not include id and user_id (see src/schemas/profiles.py) — that will make responses not match the required contract. Ensure the response schema includes id and user_id and that the returned object provides those fields.
src/routes/profiles.py
Outdated
| return ProfileResponseSchema( | ||
| **profile_data.model_dump(exclude={"avatar"}), avatar=s3_url |
There was a problem hiding this comment.
Critical: the handler returns a ProfileResponseSchema populated from the incoming profile_data rather than the persisted new_profile. This omits the saved id and user_id and risks returning stale/unaltered data. Construct the response from new_profile (e.g. use values from new_profile and avatar=s3_url) so the response contains id and user_id as required by the task.
| password_reset_complete_link = ( | ||
| f"http://127.0.0.1/accounts/reset-password/complete/token?=" | ||
| f"{quote_plus(reset_token.token)}" |
There was a problem hiding this comment.
The password reset link is built incorrectly: it uses .../token?= followed immediately by the token string. This produces an invalid query parameter name and also omits the user email. Construct a proper URL such as .../reset-password/complete/?email={email}&token={token} so the frontend/tests can parse both email and token.
| @@ -352,17 +403,15 @@ async def reset_password( | |||
| await db.run_sync(lambda s: s.delete(token_record)) | |||
There was a problem hiding this comment.
When deleting the token_record on invalid token you call await db.run_sync(lambda s: s.delete(token_record)). Deleting ORM objects should be done with the same async session (e.g. await db.delete(token_record)), not via run_sync with an object that is bound to an async session — this can cause unexpected behavior. Replace run_sync deletes with await db.delete(...) / proper async-session operations.
| def get_jwt_auth_manager(settings: BaseAppSettings = Depends(get_settings)) -> JWTAuthManagerInterface: | ||
| def get_jwt_auth_manager( | ||
| settings: BaseAppSettings = Depends(get_settings), | ||
| ) -> JWTAuthManagerInterface: |
There was a problem hiding this comment.
Ensure the constructor parameters passed to JWTAuthManager match its actual initializer. If JWTAuthManager expects different keyword names (for example secret_access_key vs secret_key_access) a TypeError will occur at runtime — verify the names secret_key_access, secret_key_refresh, and algorithm are correct.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution as an educational override so you can keep progressing while you refine a few important areas.
You did a solid job wiring up the profile creation flow, S3 upload, and email notifications with BackgroundTasks, and your response schemas correctly use from_attributes=True and return persisted IDs and avatar URLs. The main issues to improve are: (1) multipart handling for avatar — avoid UploadFile inside a Pydantic model with Form() and instead accept individual Form(...) fields plus avatar: UploadFile = File(...) in the route; (2) the password‑reset URL is currently malformed and should include named query parameters like ?email=...&token=...; (3) async DB operations in the accounts routes should use await db.delete(...)/await db.commit() instead of run_sync(...); and (4) profile validators need tightening (non‑empty names with only letters, correct gender enum checking, precise age and future‑date checks, and normalized image formats). Additionally, consider wrapping the profile DB commit in a try/except SQLAlchemyError with rollback, and sanitizing avatar filenames before S3 upload.
Overall, your structure and use of dependencies are on the right track; refining these validation, URL, and async‑DB details will make your implementation much more robust and closer to production‑ready quality.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| from security.interfaces import JWTAuthManagerInterface | ||
|
|
||
|
|
||
| async def fetch_user_by_id(db: AsyncSession, user_id: int) -> UserModel: |
There was a problem hiding this comment.
fetch_user_by_id can return None when the user isn't found. Consider using Optional[UserModel] as the return type and import Optional from typing for clarity and static checking.
| raise HTTPException( | ||
| status_code=status.HTTP_401_UNAUTHORIZED, detail="Token has expired." | ||
| ) | ||
| except InvalidTokenError: |
There was a problem hiding this comment.
Catching InvalidTokenError and returning the message "Invalid Authorization header format. Expected 'Bearer '" may be misleading: that message describes header parsing errors (get_token), while this exception likely means the token contents are invalid. Consider returning a clearer message such as "Invalid token." unless tests expect the current text.
| detail="Invalid Authorization header format. Expected 'Bearer <token>'", | ||
| ) | ||
| user_id = decoded_token.get("user_id") | ||
| if user_id is None: |
There was a problem hiding this comment.
When decoded_token lacks user_id you raise the same "Invalid Authorization header format..." message. It would be clearer to return a message like "Invalid token." because the header format was already validated by get_token. Make sure to align this with the exact messages expected by your tests.
| stmt = ( | ||
| select(UserModel) | ||
| .where(UserModel.id == user_id) | ||
| .options(selectinload(UserModel.group), selectinload(UserModel.profile)) |
There was a problem hiding this comment.
Minor: good use of selectinload to eagerly load group and profile relationships — this avoids N+1 queries when routes access those attributes.
| password_reset_complete_link = ( | ||
| f"http://127.0.0.1/accounts/reset-password/complete/token?=" | ||
| f"{quote_plus(reset_token.token)}" |
There was a problem hiding this comment.
The password reset link built here is malformed: it produces a path like /reset-password/complete/token?=... (no query parameter name and missing email). Tests and users expect a valid URL that includes required parameters (email and token). Use a URL with named query params, for example: f"http://127.0.0.1/accounts/reset-password/complete/?email={quote_plus(str(data.email))}&token={quote_plus(reset_token.token)}" so the reset endpoint can read email and token. This must be corrected to satisfy the requirement that emails include valid URLs.
| user: Annotated[UserModel, Depends(get_user)], | ||
| db: Annotated[AsyncSession, Depends(get_db)], | ||
| s3_client: Annotated[S3StorageInterface, Depends(get_s3_storage_client)], | ||
| profile_data: Annotated[ProfileCreateSchema, Form()], |
There was a problem hiding this comment.
Accepting profile_data: ProfileCreateSchema with Form() is unreliable for multipart/form-data when the schema contains an UploadFile. FastAPI will not reliably construct a Pydantic model that contains an UploadFile this way, and the avatar field (and the field validators) may not run. Change the route to accept individual form parameters and avatar: UploadFile = File(...), or create a small dependency that reads form fields and file and then runs the validation functions before returning a validated structure.
| detail="User already has a profile.", | ||
| ) | ||
|
|
||
| contents = await profile_data.avatar.read() |
There was a problem hiding this comment.
You immediately call await profile_data.avatar.read() assuming avatar exists and is populated. If you switch to the recommended pattern above, read the file from the UploadFile parameter in the route (not from a Pydantic model). Also ensure the validate_image function is invoked on the actual file bytes/UploadFile as required by your validators (currently they may never run).
| ) | ||
|
|
||
| contents = await profile_data.avatar.read() | ||
| filename = f"avatars/{target_user_id}_{profile_data.avatar.filename}" |
There was a problem hiding this comment.
Building the filename directly from profile_data.avatar.filename may include unsafe characters. Sanitize or normalize the filename (or generate a safe random filename) before uploading to S3. Also ensure the validate_image step has checked format and size before upload so large/invalid files aren't sent to storage.
| await s3_client.upload_file(filename, contents) | ||
| s3_url = await s3_client.get_file_url(filename) |
There was a problem hiding this comment.
The S3 upload calls await s3_client.upload_file(filename, contents) and then await s3_client.get_file_url(filename) — that's fine if upload_file expects raw bytes. Confirm the S3StorageInterface expects bytes for contents. You're already catching BaseS3Error and returning the exact required 500 message, which matches the spec.
| new_profile = UserProfileModel( | ||
| **profile_data.model_dump(exclude={"avatar"}), | ||
| avatar=filename, | ||
| user_id=user_id, | ||
| ) | ||
| new_profile.user_id = target_user_id | ||
| db.add(new_profile) | ||
| await db.commit() | ||
| await db.refresh(new_profile) |
There was a problem hiding this comment.
DB persistence (lines creating new_profile, adding and committing) is not wrapped in try/except. If commit/flush fails you should rollback and return a 500. Add error handling (rollback on SQLAlchemyError) to avoid leaving the transaction open or raising unhandled exceptions.
| return ProfileResponseSchema( | ||
| id=new_profile.id, | ||
| user_id=new_profile.user_id, | ||
| first_name=new_profile.first_name, | ||
| last_name=new_profile.last_name, | ||
| gender=new_profile.gender, | ||
| date_of_birth=new_profile.date_of_birth, | ||
| info=new_profile.info, | ||
| avatar=s3_url, |
There was a problem hiding this comment.
Good: the response is built from the persisted new_profile and uses the s3_url for the avatar — this ensures the client gets the saved profile data and final avatar URL as required by the task example response.
| user: Annotated[UserModel, Depends(get_user)], | ||
| db: Annotated[AsyncSession, Depends(get_db)], |
There was a problem hiding this comment.
Authorization / token errors (missing token, invalid format, expired token) must return the exact messages/status codes specified in the task. This route depends on get_user; ensure get_user internally uses the required get_token behaviour (including the exact error messages) so those cases produce the exact responses expected by tests.
|
|
||
|
|
||
| def get_jwt_auth_manager(settings: BaseAppSettings = Depends(get_settings)) -> JWTAuthManagerInterface: | ||
| def get_jwt_auth_manager( |
There was a problem hiding this comment.
The positional-only marker (/) at the start of the parameter list is unnecessary here and may confuse readers. Removing it will keep the signature simpler: def get_jwt_auth_manager(settings: BaseAppSettings = Depends(get_settings)) -> JWTAuthManagerInterface:
|
|
||
| def get_jwt_auth_manager(settings: BaseAppSettings = Depends(get_settings)) -> JWTAuthManagerInterface: | ||
| def get_jwt_auth_manager( | ||
| settings: BaseAppSettings = Depends(get_settings), |
There was a problem hiding this comment.
Minor: the settings parameter uses Depends(get_settings) correctly. No change required, just confirming this matches the dependency injection requirements.
| def validate_name(name: str): | ||
| if re.search(r'^[A-Za-z]*$', name) is None: | ||
| raise ValueError(f'{name} contains non-english letters') | ||
| if re.search(r"^[A-Za-z]*$", name) is None: |
There was a problem hiding this comment.
The regex ^[A-Za-z]*$ allows an empty string (the * quantifier permits zero characters). The requirement says names must contain only English letters — also ensure the name is not empty. Use + (e.g. ^[A-Za-z]+$) and consider trimming whitespace before validation.
| if re.search(r'^[A-Za-z]*$', name) is None: | ||
| raise ValueError(f'{name} contains non-english letters') | ||
| if re.search(r"^[A-Za-z]*$", name) is None: | ||
| raise ValueError(f"{name} contains non-english letters") |
There was a problem hiding this comment.
Error message is fine but you should normalize (strip) name before validation and raise a clearer message for empty names. Example: if name.strip() is empty raise a dedicated error about an empty name.
| image_format = image.format | ||
| if image_format not in supported_image_formats: | ||
| raise ValueError(f"Unsupported image format: {image_format}. Use one of next: {supported_image_formats}") | ||
| raise ValueError( |
There was a problem hiding this comment.
When reporting unsupported format, normalize image.format (e.g. image_format = (image.format or '').upper()) and compare to normalized supported formats. Pillow returns JPEG for JPG files, so treat JPEG/JPG consistently (or list only the canonical names like JPEG, PNG).
| if image_format not in supported_image_formats: | ||
| raise ValueError(f"Unsupported image format: {image_format}. Use one of next: {supported_image_formats}") | ||
| raise ValueError( | ||
| f"Unsupported image format: {image_format}. Use one of next: {supported_image_formats}" |
There was a problem hiding this comment.
The error message listing supported formats is OK, but ensure supported_image_formats and the image.format comparison use the same normalization (both uppercased canonical names) to avoid false negatives for valid images.
| raise ValueError(f"Unsupported image format: {image_format}. Use one of next: {supported_image_formats}") | ||
| raise ValueError( | ||
| f"Unsupported image format: {image_format}. Use one of next: {supported_image_formats}" | ||
| ) |
There was a problem hiding this comment.
Avoid raising the message using un-normalized image_format — normalize before interpolating into the message so it is predictable. Also consider closing the image object (image.close()) after inspection to free resources.
|
|
||
|
|
||
| def validate_gender(gender: str) -> None: | ||
| if gender not in GenderEnum.__members__.values(): |
There was a problem hiding this comment.
The gender membership check is incorrect: GenderEnum.__members__.values() yields enum members, so comparing the input string to those will fail. Instead compare against the members' value (e.g. if gender not in [g.value for g in GenderEnum]: ...) or try GenderEnum(gender) in a try/except to validate.
| def validate_gender(gender: str) -> None: | ||
| if gender not in GenderEnum.__members__.values(): | ||
| raise ValueError(f"Gender must be one of: {', '.join(g.value for g in GenderEnum)}") | ||
| raise ValueError( |
There was a problem hiding this comment.
The raised message is fine, but ensure the join enumerates g.value (you already do) and that the membership check matches that representation — otherwise the message and validation will be inconsistent.
| raise ValueError(f"Gender must be one of: {', '.join(g.value for g in GenderEnum)}") | ||
| raise ValueError( | ||
| f"Gender must be one of: {', '.join(g.value for g in GenderEnum)}" | ||
| ) |
There was a problem hiding this comment.
If GenderEnum members are not too many, prefer computing allowed = [g.value for g in GenderEnum] once and using it both for checking and messaging to keep behaviour consistent and efficient.
| def validate_birth_date(birth_date: date) -> None: | ||
| if birth_date.year < 1900: | ||
| raise ValueError('Invalid birth date - year must be greater than 1900.') | ||
| raise ValueError("Invalid birth date - year must be greater than 1900.") |
There was a problem hiding this comment.
The lower bound year check is okay but the error text says "greater than 1900"; be precise (e.g. "year must be >= 1900"). More importantly, add a future-date guard: if birth_date > date.today() raise an error because future dates must be invalid.
| age = (date.today() - birth_date).days // 365 | ||
| if age < 18: | ||
| raise ValueError('You must be at least 18 years old to register.') | ||
| raise ValueError("You must be at least 18 years old to register.") |
There was a problem hiding this comment.
Age calculation using (date.today() - birth_date).days // 365 is imprecise (leap years and exact birthday). Use a precise calculation: compare year difference and then subtract 1 if the current month/day is before the birth month/day. This ensures the 18-year check is accurate and will match tests that expect exact age semantics.
| if age < 18: | ||
| raise ValueError('You must be at least 18 years old to register.') | ||
| raise ValueError("You must be at least 18 years old to register.") | ||
|
|
There was a problem hiding this comment.
Add an explicit check for birth_date > date.today() (future date) to reject invalid birth dates — currently there's no guard for future dates.
| raise ValueError('You must be at least 18 years old to register.') | ||
| raise ValueError("You must be at least 18 years old to register.") | ||
|
|
||
|
|
There was a problem hiding this comment.
There are blank marker lines here; no action needed except to ensure any future edits keep validators grouped logically.
| raise ValueError("You must be at least 18 years old to register.") | ||
|
|
||
|
|
||
| def validate_info(info: str) -> None: |
There was a problem hiding this comment.
validate_info correctly checks emptiness and whitespace-only strings — this matches the requirement. Consider trimming or normalizing info if you want to store a cleaned value, but validation is correct as-is.
| from fastapi import UploadFile | ||
| from pydantic import BaseModel, field_validator, ConfigDict | ||
|
|
||
| from validation import ( | ||
| validate_name, | ||
| validate_image, | ||
| validate_gender, | ||
| validate_birth_date | ||
| validate_birth_date, | ||
| validate_info, | ||
| ) | ||
|
|
||
| # Write your code here | ||
|
|
||
| class ProfileCreateSchema(BaseModel): | ||
| first_name: str | ||
| last_name: str | ||
| gender: str | ||
| date_of_birth: date | ||
| info: str | ||
| avatar: UploadFile |
There was a problem hiding this comment.
Declaring avatar: UploadFile inside a Pydantic request model is unreliable for multipart/form-data. FastAPI does not reliably populate UploadFile when embedded inside a BaseModel. This will likely cause multipart requests/tests to fail. Instead, accept form fields directly in the route (e.g. first_name: str = Form(...), etc.) and avatar: UploadFile = File(...), or create a small dependency that reads form/file inputs and then runs these validators.
| @field_validator("first_name", "last_name") | ||
| @classmethod | ||
| def validate_a_name(cls, value: str) -> str: | ||
| validate_name(value) | ||
|
|
||
| return value.lower() |
There was a problem hiding this comment.
Validator calls validate_name(value) but does not strip surrounding whitespace. If validate_name does not trim, names containing only spaces or with leading/trailing spaces could bypass/behave unexpectedly. Consider value = value.strip() before calling validate_name(value) so validation matches the requirement that names contain only English letters.
| @field_validator("avatar", mode="before") | ||
| @classmethod | ||
| def validate_avatar(cls, value: UploadFile) -> UploadFile: | ||
| validate_image(value) | ||
|
|
||
| return value |
There was a problem hiding this comment.
The avatar field validator uses validate_image(value) with mode='before'. Ensure validate_image accepts a fastapi.UploadFile (and handles reading bytes, format and size checks) — otherwise validate_image may expect raw bytes/stream. If validate_image consumes the file stream, be aware the route/storage code must handle the consumed stream (or perform validation in the route after await avatar.read()). Clarify and align expectation between the schema validator and the route storage flow to avoid runtime errors.
| class ProfileResponseSchema(BaseModel): | ||
| model_config = ConfigDict(from_attributes=True) | ||
|
|
||
| id: int | ||
| user_id: int |
There was a problem hiding this comment.
ProfileResponseSchema includes id and user_id and uses model_config = ConfigDict(from_attributes=True), which is required so the response can be built from the persisted SQLAlchemy model. This satisfies the requirement that the response include persisted id and user_id values.
No description provided.