fix: Address second code review findings (test isolation, frontend type safety)

This commit is contained in:
Yunxiao Xu
2026-02-17 02:11:04 -08:00
parent 96e2634053
commit ece12f951a
6 changed files with 31 additions and 24 deletions

View File

@@ -7,7 +7,9 @@ from ea_chatbot.history.models import User
# We will need to mock HistoryManager and get_db dependencies later # We will need to mock HistoryManager and get_db dependencies later
# For now, we define the expected behavior of the auth endpoints. # For now, we define the expected behavior of the auth endpoints.
client = TestClient(app) @pytest.fixture
def client():
return TestClient(app)
@pytest.fixture @pytest.fixture
def mock_user(): def mock_user():
@@ -19,7 +21,7 @@ def mock_user():
theme_preference="light" theme_preference="light"
) )
def test_register_user_success(): def test_register_user_success(client):
"""Test successful user registration.""" """Test successful user registration."""
# We mock it where it is used in the router # We mock it where it is used in the router
with patch("ea_chatbot.api.routers.auth.history_manager") as mock_hm: with patch("ea_chatbot.api.routers.auth.history_manager") as mock_hm:
@@ -34,7 +36,7 @@ def test_register_user_success():
assert response.status_code == 201 assert response.status_code == 201
assert response.json()["email"] == "new@example.com" assert response.json()["email"] == "new@example.com"
def test_login_success(): def test_login_success(client):
"""Test successful login and JWT return.""" """Test successful login and JWT return."""
with patch("ea_chatbot.api.routers.auth.history_manager") as mock_hm: with patch("ea_chatbot.api.routers.auth.history_manager") as mock_hm:
mock_hm.authenticate_user.return_value = User(id="1", username="test@example.com") mock_hm.authenticate_user.return_value = User(id="1", username="test@example.com")
@@ -48,7 +50,7 @@ def test_login_success():
assert "access_token" in response.json() assert "access_token" in response.json()
assert response.json()["token_type"] == "bearer" assert response.json()["token_type"] == "bearer"
def test_login_invalid_credentials(): def test_login_invalid_credentials(client):
"""Test login with wrong password.""" """Test login with wrong password."""
with patch("ea_chatbot.api.routers.auth.history_manager") as mock_hm: with patch("ea_chatbot.api.routers.auth.history_manager") as mock_hm:
mock_hm.authenticate_user.return_value = None mock_hm.authenticate_user.return_value = None
@@ -61,12 +63,12 @@ def test_login_invalid_credentials():
assert response.status_code == 401 assert response.status_code == 401
assert "detail" in response.json() assert "detail" in response.json()
def test_protected_route_without_token(): def test_protected_route_without_token(client):
"""Test that protected routes require a token.""" """Test that protected routes require a token."""
response = client.get("/api/v1/auth/me") response = client.get("/api/v1/auth/me")
assert response.status_code == 401 assert response.status_code == 401
def test_oidc_login_redirect(): def test_oidc_login_redirect(client):
"""Test that OIDC login returns a redirect URL and sets session cookie.""" """Test that OIDC login returns a redirect URL and sets session cookie."""
with patch("ea_chatbot.api.routers.auth.oidc_client") as mock_oidc: with patch("ea_chatbot.api.routers.auth.oidc_client") as mock_oidc:
mock_oidc.get_auth_data.return_value = { mock_oidc.get_auth_data.return_value = {
@@ -81,7 +83,7 @@ def test_oidc_login_redirect():
assert response.json()["url"] == "https://oidc-provider.com/auth" assert response.json()["url"] == "https://oidc-provider.com/auth"
assert "oidc_session" in response.cookies assert "oidc_session" in response.cookies
def test_oidc_callback_success(): def test_oidc_callback_success(client):
"""Test successful OIDC callback and JWT issuance.""" """Test successful OIDC callback and JWT issuance."""
with patch("ea_chatbot.api.routers.auth.oidc_client") as mock_oidc, \ with patch("ea_chatbot.api.routers.auth.oidc_client") as mock_oidc, \
patch("ea_chatbot.api.routers.auth.OIDCSession.decrypt") as mock_decrypt, \ patch("ea_chatbot.api.routers.auth.OIDCSession.decrypt") as mock_decrypt, \
@@ -105,7 +107,7 @@ def test_oidc_callback_success():
assert response.status_code == 302 assert response.status_code == 302
assert "access_token" in response.cookies assert "access_token" in response.cookies
def test_get_me_success(): def test_get_me_success(client):
"""Test getting current user with a valid token.""" """Test getting current user with a valid token."""
from ea_chatbot.api.utils import create_access_token from ea_chatbot.api.utils import create_access_token
token = create_access_token(data={"sub": "123"}) token = create_access_token(data={"sub": "123"})

View File

@@ -5,7 +5,9 @@ from ea_chatbot.api.main import app
from ea_chatbot.history.models import User from ea_chatbot.history.models import User
from ea_chatbot.api.utils import create_access_token from ea_chatbot.api.utils import create_access_token
client = TestClient(app) @pytest.fixture
def client():
return TestClient(app)
@pytest.fixture @pytest.fixture
def mock_user(): def mock_user():
@@ -17,7 +19,7 @@ def mock_user():
theme_preference="light" theme_preference="light"
) )
def test_v1_prefix(): def test_v1_prefix(client):
"""Test that routes are prefixed with /api/v1.""" """Test that routes are prefixed with /api/v1."""
# This should now be 404 # This should now be 404
response = client.get("/auth/me") response = client.get("/auth/me")
@@ -27,7 +29,7 @@ def test_v1_prefix():
response = client.get("/api/v1/auth/me") response = client.get("/api/v1/auth/me")
assert response.status_code == 401 assert response.status_code == 401
def test_login_sets_cookie(): def test_login_sets_cookie(client):
"""Test that login sets the access_token cookie.""" """Test that login sets the access_token cookie."""
with patch("ea_chatbot.api.routers.auth.history_manager") as mock_hm: with patch("ea_chatbot.api.routers.auth.history_manager") as mock_hm:
mock_hm.authenticate_user.return_value = User(id="1", username="test@example.com") mock_hm.authenticate_user.return_value = User(id="1", username="test@example.com")
@@ -45,7 +47,7 @@ def test_login_sets_cookie():
assert "access_token" in set_cookie assert "access_token" in set_cookie
assert "HttpOnly" in set_cookie assert "HttpOnly" in set_cookie
def test_register_sets_cookie(): def test_register_sets_cookie(client):
"""Test that register sets the access_token cookie.""" """Test that register sets the access_token cookie."""
with patch("ea_chatbot.api.routers.auth.history_manager") as mock_hm: with patch("ea_chatbot.api.routers.auth.history_manager") as mock_hm:
mock_hm.get_user.return_value = None mock_hm.get_user.return_value = None
@@ -59,7 +61,7 @@ def test_register_sets_cookie():
assert response.status_code == 201 assert response.status_code == 201
assert "access_token" in response.cookies assert "access_token" in response.cookies
def test_auth_via_cookie(): def test_auth_via_cookie(client):
"""Test that protected routes work with the access_token cookie.""" """Test that protected routes work with the access_token cookie."""
token = create_access_token(data={"sub": "123"}) token = create_access_token(data={"sub": "123"})
@@ -73,7 +75,7 @@ def test_auth_via_cookie():
assert response.status_code == 200 assert response.status_code == 200
assert response.json()["email"] == "test@example.com" assert response.json()["email"] == "test@example.com"
def test_logout_clears_cookie(): def test_logout_clears_cookie(client):
"""Test that logout endpoint clears the cookie.""" """Test that logout endpoint clears the cookie."""
response = client.post("/api/v1/auth/logout") response = client.post("/api/v1/auth/logout")
assert response.status_code == 200 assert response.status_code == 200
@@ -81,7 +83,7 @@ def test_logout_clears_cookie():
cookie = response.cookies.get("access_token") cookie = response.cookies.get("access_token")
assert not cookie or cookie == "" assert not cookie or cookie == ""
def test_oidc_callback_redirects_with_cookie(): def test_oidc_callback_redirects_with_cookie(client):
"""Test that OIDC callback sets cookie and redirects.""" """Test that OIDC callback sets cookie and redirects."""
with patch("ea_chatbot.api.routers.auth.oidc_client") as mock_oidc, \ with patch("ea_chatbot.api.routers.auth.oidc_client") as mock_oidc, \
patch("ea_chatbot.api.routers.auth.OIDCSession.decrypt") as mock_decrypt, \ patch("ea_chatbot.api.routers.auth.OIDCSession.decrypt") as mock_decrypt, \

View File

@@ -5,7 +5,9 @@ from ea_chatbot.api.main import app
from ea_chatbot.history.models import User from ea_chatbot.history.models import User
from ea_chatbot.api.utils import create_access_token from ea_chatbot.api.utils import create_access_token
client = TestClient(app) @pytest.fixture
def client():
return TestClient(app)
@pytest.fixture @pytest.fixture
def test_user(): def test_user():
@@ -20,7 +22,7 @@ def test_user():
def auth_token(): def auth_token():
return create_access_token(data={"sub": "user-123"}) return create_access_token(data={"sub": "user-123"})
def test_get_me_includes_theme(test_user, auth_token): def test_get_me_includes_theme(client, test_user, auth_token):
"""Test that /auth/me returns the theme_preference.""" """Test that /auth/me returns the theme_preference."""
with patch("ea_chatbot.api.dependencies.history_manager") as mock_hm: with patch("ea_chatbot.api.dependencies.history_manager") as mock_hm:
mock_hm.get_user_by_id.return_value = test_user mock_hm.get_user_by_id.return_value = test_user
@@ -33,7 +35,7 @@ def test_get_me_includes_theme(test_user, auth_token):
assert "theme_preference" in data assert "theme_preference" in data
assert data["theme_preference"] == "light" assert data["theme_preference"] == "light"
def test_update_theme_success(test_user, auth_token): def test_update_theme_success(client, test_user, auth_token):
"""Test successful theme update via PATCH /auth/theme.""" """Test successful theme update via PATCH /auth/theme."""
updated_user = User( updated_user = User(
id="user-123", id="user-123",
@@ -62,7 +64,7 @@ def test_update_theme_success(test_user, auth_token):
assert data["theme_preference"] == "dark" assert data["theme_preference"] == "dark"
mock_hm_router.update_user_theme.assert_called_once_with("user-123", "dark") mock_hm_router.update_user_theme.assert_called_once_with("user-123", "dark")
def test_update_theme_unauthorized(): def test_update_theme_unauthorized(client):
"""Test that theme update requires authentication.""" """Test that theme update requires authentication."""
response = client.patch( response = client.patch(
"/api/v1/auth/theme", "/api/v1/auth/theme",

View File

@@ -37,7 +37,7 @@ function App() {
setUser(userData) setUser(userData)
setIsAuthenticated(true) setIsAuthenticated(true)
if (userData.theme_preference) { if (userData.theme_preference) {
setThemeLocal(userData.theme_preference as "light" | "dark") setThemeLocal(userData.theme_preference)
} }
// Load history after successful auth // Load history after successful auth
loadHistory() loadHistory()
@@ -67,7 +67,7 @@ function App() {
setUser(userData) setUser(userData)
setIsAuthenticated(true) setIsAuthenticated(true)
if (userData.theme_preference) { if (userData.theme_preference) {
setThemeLocal(userData.theme_preference as "light" | "dark") setThemeLocal(userData.theme_preference)
} }
loadHistory() loadHistory()
} catch (err: unknown) { } catch (err: unknown) {

View File

@@ -1,7 +1,7 @@
import { createContext, useContext, useEffect, useState } from "react" import { createContext, useContext, useEffect, useState } from "react"
import { AuthService } from "@/services/auth" import { AuthService } from "@/services/auth"
type Theme = "light" | "dark" export type Theme = "light" | "dark"
interface ThemeContextType { interface ThemeContextType {
theme: Theme theme: Theme

View File

@@ -1,4 +1,5 @@
import api from "./api" import api from "./api"
import type { Theme } from "@/components/theme-provider"
export interface AuthResponse { export interface AuthResponse {
access_token: string access_token: string
@@ -9,7 +10,7 @@ export interface UserResponse {
id: string id: string
email: string email: string
display_name?: string display_name?: string
theme_preference: string theme_preference: Theme
} }
export const AuthService = { export const AuthService = {
@@ -51,7 +52,7 @@ export const AuthService = {
await api.post("/auth/logout") await api.post("/auth/logout")
}, },
async updateTheme(theme: string): Promise<UserResponse> { async updateTheme(theme: Theme): Promise<UserResponse> {
const response = await api.patch<UserResponse>("/auth/theme", { theme }) const response = await api.patch<UserResponse>("/auth/theme", { theme })
return response.data return response.data
}, },