diff --git a/docs/architecture.qmd b/docs/architecture.qmd index 7dfd136..1079e53 100644 --- a/docs/architecture.qmd +++ b/docs/architecture.qmd @@ -102,18 +102,16 @@ Here are some patterns we've considered for server-side error handling: border-collapse: collapse; } -.styled-table th:nth-child(1) { width: 5%; } -.styled-table th:nth-child(2) { width: 50%; } -.styled-table th:nth-child(3), -.styled-table th:nth-child(4), -.styled-table th:nth-child(5) { width: 15%; } -.styled-table th:nth-child(6) { width: 10%; } +.styled-table th:nth-child(1) { width: 50%; } +.styled-table th:nth-child(2), +.styled-table th:nth-child(3), +.styled-table th:nth-child(4) { width: 15%; } +.styled-table th:nth-child(5) { width: 10%; } - @@ -123,7 +121,6 @@ Here are some patterns we've considered for server-side error handling: - @@ -131,7 +128,6 @@ Here are some patterns we've considered for server-side error handling: - @@ -139,7 +135,6 @@ Here are some patterns we've considered for server-side error handling: - @@ -147,7 +142,6 @@ Here are some patterns we've considered for server-side error handling: - @@ -155,7 +149,6 @@ Here are some patterns we've considered for server-side error handling: - diff --git a/docs/static/documentation.txt b/docs/static/documentation.txt index f15677b..15713bb 100644 --- a/docs/static/documentation.txt +++ b/docs/static/documentation.txt @@ -322,18 +322,16 @@ Here are some patterns we've considered for server-side error handling: border-collapse: collapse; } -.styled-table th:nth-child(1) { width: 5%; } -.styled-table th:nth-child(2) { width: 50%; } -.styled-table th:nth-child(3), -.styled-table th:nth-child(4), -.styled-table th:nth-child(5) { width: 15%; } -.styled-table th:nth-child(6) { width: 10%; } +.styled-table th:nth-child(1) { width: 50%; } +.styled-table th:nth-child(2), +.styled-table th:nth-child(3), +.styled-table th:nth-child(4) { width: 15%; } +.styled-table th:nth-child(5) { width: 10%; }
ID Approach Returns to same page Preserves form inputs
1 Validate with Pydantic dependency, catch and redirect from middleware (with exception message as context) to an error page with "go back" button No YesLow
2 Validate in FastAPI endpoint function body, redirect to origin page with error message query param Yes NoMedium
3 Validate in FastAPI endpoint function body, redirect to origin page with error message query param and form inputs as context so we can re-render page with original form inputs Yes YesHigh
4 Validate with Pydantic dependency, use session context to get form inputs from request, redirect to origin page from middleware with exception message and form inputs as context so we can re-render page with original form inputs Yes YesHigh
5 Validate in either Pydantic dependency or function endpoint body and directly return error message or error toast HTML partial in JSON, then mount error toast with HTMX or some simple layout-level Javascript Yes Yes
- @@ -343,7 +341,6 @@ Here are some patterns we've considered for server-side error handling: - @@ -351,7 +348,6 @@ Here are some patterns we've considered for server-side error handling: - @@ -359,7 +355,6 @@ Here are some patterns we've considered for server-side error handling: - @@ -367,7 +362,6 @@ Here are some patterns we've considered for server-side error handling: - @@ -375,7 +369,6 @@ Here are some patterns we've considered for server-side error handling: - diff --git a/docs/static/schema.png b/docs/static/schema.png index b47a3a1..49cacfe 100644 Binary files a/docs/static/schema.png and b/docs/static/schema.png differ diff --git a/routers/user.py b/routers/user.py index 2188c10..504d8d1 100644 --- a/routers/user.py +++ b/routers/user.py @@ -1,9 +1,10 @@ -from fastapi import APIRouter, Depends, HTTPException, Form -from fastapi.responses import RedirectResponse +from fastapi import APIRouter, Depends, Form, UploadFile, File +from fastapi.responses import RedirectResponse, Response from pydantic import BaseModel, EmailStr from sqlmodel import Session -from utils.models import User -from utils.auth import get_session, get_authenticated_user, verify_password +from typing import Optional +from utils.models import User, DataIntegrityError +from utils.auth import get_session, get_authenticated_user, verify_password, PasswordValidationError router = APIRouter(prefix="/user", tags=["user"]) @@ -15,16 +16,29 @@ class UpdateProfile(BaseModel): """Request model for updating user profile information""" name: str email: EmailStr - avatar_url: str + avatar_file: Optional[bytes] = None + avatar_content_type: Optional[str] = None @classmethod async def as_form( cls, name: str = Form(...), email: EmailStr = Form(...), - avatar_url: str = Form(...), + avatar_file: Optional[UploadFile] = File(None), ): - return cls(name=name, email=email, avatar_url=avatar_url) + avatar_data = None + avatar_content_type = None + + if avatar_file: + avatar_data = await avatar_file.read() + avatar_content_type = avatar_file.content_type + + return cls( + name=name, + email=email, + avatar_file=avatar_data, + avatar_content_type=avatar_content_type + ) class UserDeleteAccount(BaseModel): @@ -44,15 +58,20 @@ async def as_form( @router.post("/update_profile", response_class=RedirectResponse) async def update_profile( user_profile: UpdateProfile = Depends(UpdateProfile.as_form), - current_user: User = Depends(get_authenticated_user), + user: User = Depends(get_authenticated_user), session: Session = Depends(get_session) ): # Update user details - current_user.name = user_profile.name - current_user.email = user_profile.email - current_user.avatar_url = user_profile.avatar_url + user.name = user_profile.name + user.email = user_profile.email + + # Handle avatar update + if user_profile.avatar_file: + user.avatar_data = user_profile.avatar_file + user.avatar_content_type = user_profile.avatar_content_type + session.commit() - session.refresh(current_user) + session.refresh(user) return RedirectResponse(url="/profile", status_code=303) @@ -60,27 +79,43 @@ async def update_profile( async def delete_account( user_delete_account: UserDeleteAccount = Depends( UserDeleteAccount.as_form), - current_user: User = Depends(get_authenticated_user), + user: User = Depends(get_authenticated_user), session: Session = Depends(get_session) ): - if not current_user.password: - raise HTTPException( - status_code=500, - detail="User password not found in database; please contact a system administrator" + if not user.password: + raise DataIntegrityError( + resource="User password" ) if not verify_password( user_delete_account.confirm_delete_password, - current_user.password.hashed_password + user.password.hashed_password ): - raise HTTPException( - status_code=400, - detail="Password is incorrect" + raise PasswordValidationError( + field="confirm_delete_password", + message="Password is incorrect" ) # Delete the user - session.delete(current_user) + session.delete(user) session.commit() # Log out the user return RedirectResponse(url="/auth/logout", status_code=303) + + +@router.get("/avatar") +async def get_avatar( + user: User = Depends(get_authenticated_user), + session: Session = Depends(get_session) +): + """Serve avatar image from database""" + if not user.avatar_data: + raise DataIntegrityError( + resource="User avatar" + ) + + return Response( + content=user.avatar_data, + media_type=user.avatar_content_type + ) diff --git a/templates/users/organization.html b/templates/users/organization.html index 2e75fa9..913598b 100644 --- a/templates/users/organization.html +++ b/templates/users/organization.html @@ -63,11 +63,7 @@

{{ organization.name }}

{% for user in role.users %} diff --git a/templates/users/profile.html b/templates/users/profile.html index 0053993..fd4ac0b 100644 --- a/templates/users/profile.html +++ b/templates/users/profile.html @@ -18,8 +18,8 @@

User Profile

Email: {{ user.email }}

- {% if user.avatar_url %} - User Avatar + {% if user.avatar_data %} + User Avatar {% else %} {{ render_silhouette(width=150, height=150) }} {% endif %} @@ -35,7 +35,7 @@

User Profile

Edit Profile
-
+
@@ -45,8 +45,8 @@

User Profile

- - + +
diff --git a/tests/test_user.py b/tests/test_user.py index d6f42d0..674a1c1 100644 --- a/tests/test_user.py +++ b/tests/test_user.py @@ -13,7 +13,9 @@ def test_update_profile_unauthorized(unauth_client: TestClient): data={ "name": "New Name", "email": "new@example.com", - "avatar_url": "https://example.com/avatar.jpg" + }, + files={ + "avatar_file": ("test_avatar.jpg", b"fake image data", "image/jpeg") }, follow_redirects=False ) @@ -23,14 +25,40 @@ def test_update_profile_unauthorized(unauth_client: TestClient): def test_update_profile_authorized(auth_client: TestClient, test_user: User, session: Session): """Test that authorized users can edit their profile""" - + + # Create test image data + test_image_data = b"fake image data" + # Update profile response: Response = auth_client.post( app.url_path_for("update_profile"), data={ "name": "Updated Name", "email": "updated@example.com", - "avatar_url": "https://example.com/new-avatar.jpg" + }, + files={ + "avatar_file": ("test_avatar.jpg", test_image_data, "image/jpeg") + }, + follow_redirects=False + ) + assert response.status_code == 303 + assert response.headers["location"] == app.url_path_for("read_profile") + + # Verify changes in database + session.refresh(test_user) + assert test_user.name == "Updated Name" + assert test_user.email == "updated@example.com" + assert test_user.avatar_data == test_image_data + assert test_user.avatar_content_type == "image/jpeg" + + +def test_update_profile_without_avatar(auth_client: TestClient, test_user: User, session: Session): + """Test that profile can be updated without changing the avatar""" + response: Response = auth_client.post( + app.url_path_for("update_profile"), + data={ + "name": "Updated Name", + "email": "updated@example.com", }, follow_redirects=False ) @@ -41,7 +69,6 @@ def test_update_profile_authorized(auth_client: TestClient, test_user: User, ses session.refresh(test_user) assert test_user.name == "Updated Name" assert test_user.email == "updated@example.com" - assert test_user.avatar_url == "https://example.com/new-avatar.jpg" def test_delete_account_unauthorized(unauth_client: TestClient): @@ -62,7 +89,7 @@ def test_delete_account_wrong_password(auth_client: TestClient, test_user: User) data={"confirm_delete_password": "WrongPassword123!"}, follow_redirects=False ) - assert response.status_code == 400 + assert response.status_code == 422 assert "Password is incorrect" in response.text.strip() @@ -81,3 +108,37 @@ def test_delete_account_success(auth_client: TestClient, test_user: User, sessio # Verify user is deleted from database user = session.get(User, test_user.id) assert user is None + + +def test_get_avatar_authorized(auth_client: TestClient, test_user: User): + """Test getting user avatar""" + # First upload an avatar + test_image_data = b"fake image data" + auth_client.post( + app.url_path_for("update_profile"), + data={ + "name": test_user.name, + "email": test_user.email, + }, + files={ + "avatar_file": ("test_avatar.jpg", test_image_data, "image/jpeg") + }, + ) + + # Then try to retrieve it + response = auth_client.get( + app.url_path_for("get_avatar") + ) + assert response.status_code == 200 + assert response.content == test_image_data + assert response.headers["content-type"] == "image/jpeg" + + +def test_get_avatar_unauthorized(unauth_client: TestClient): + """Test getting avatar for non-existent user""" + response = unauth_client.get( + app.url_path_for("get_avatar"), + follow_redirects=False + ) + assert response.status_code == 303 + assert response.headers["location"] == app.url_path_for("read_login") diff --git a/utils/models.py b/utils/models.py index 63d9ec2..29e468e 100644 --- a/utils/models.py +++ b/utils/models.py @@ -5,7 +5,7 @@ from typing import Optional, List, Union from fastapi import HTTPException from sqlmodel import SQLModel, Field, Relationship -from sqlalchemy import Column, Enum as SQLAlchemyEnum +from sqlalchemy import Column, Enum as SQLAlchemyEnum, LargeBinary from sqlalchemy.orm import Mapped logger = getLogger("uvicorn.error") @@ -180,7 +180,9 @@ class User(SQLModel, table=True): id: Optional[int] = Field(default=None, primary_key=True) name: str email: str = Field(index=True, unique=True) - avatar_url: Optional[str] = None + avatar_data: Optional[bytes] = Field( + default=None, sa_column=Column(LargeBinary)) + avatar_content_type: Optional[str] = None created_at: datetime = Field(default_factory=utc_time) updated_at: datetime = Field(default_factory=utc_time)
ID Approach Returns to same page Preserves form inputs
1 Validate with Pydantic dependency, catch and redirect from middleware (with exception message as context) to an error page with "go back" button No YesLow
2 Validate in FastAPI endpoint function body, redirect to origin page with error message query param Yes NoMedium
3 Validate in FastAPI endpoint function body, redirect to origin page with error message query param and form inputs as context so we can re-render page with original form inputs Yes YesHigh
4 Validate with Pydantic dependency, use session context to get form inputs from request, redirect to origin page from middleware with exception message and form inputs as context so we can re-render page with original form inputs Yes YesHigh
5 Validate in either Pydantic dependency or function endpoint body and directly return error message or error toast HTML partial in JSON, then mount error toast with HTMX or some simple layout-level Javascript Yes Yes
- {% if user.avatar_url %} - User Avatar - {% else %} - {{ render_silhouette(width=40, height=40) }} - {% endif %} + {{ render_silhouette(width=40, height=40) }} {{ user.name }} {{ user.email }}