From 619e2206220e55d6316c76f6323b8820f28c4873 Mon Sep 17 00:00:00 2001 From: Kyle Pope Date: Fri, 27 Feb 2026 05:41:16 +0800 Subject: [PATCH] Fix QA review #2: W-03/W-04, S-01 through S-04 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit W-03: Unify split transactions — _create_db_session() now uses flush() instead of commit(), callers own the final commit. W-04: Time-bound dedup key fetch to 7-day purge window. S-01: Type admin dashboard response with RecentLoginItem/RecentAuditItem. S-02: Convert starred events index to partial index WHERE is_starred = true. S-03: EventTemplate.created_at default changed to func.now() for consistency. S-04: Add single-worker scaling note to weather cache. Co-Authored-By: Claude Opus 4.6 --- .../versions/035_add_performance_indexes.py | 6 ++++-- backend/app/jobs/notifications.py | 8 ++++++-- backend/app/models/event_template.py | 2 +- backend/app/routers/auth.py | 4 +++- backend/app/routers/weather.py | 2 ++ backend/app/schemas/admin.py | 20 +++++++++++++++++-- 6 files changed, 34 insertions(+), 8 deletions(-) diff --git a/backend/alembic/versions/035_add_performance_indexes.py b/backend/alembic/versions/035_add_performance_indexes.py index c7fb34b..e36baae 100644 --- a/backend/alembic/versions/035_add_performance_indexes.py +++ b/backend/alembic/versions/035_add_performance_indexes.py @@ -27,11 +27,13 @@ def upgrade() -> None: ["calendar_id", "start_datetime", "end_datetime"], ) - # Partial index for starred events dashboard query + # Partial index for starred events dashboard query — only rows where + # is_starred = true are ever queried, so a partial index is smaller and faster. op.create_index( "ix_calendar_events_calendar_starred", "calendar_events", - ["calendar_id", "is_starred"], + ["calendar_id", "start_datetime"], + postgresql_where="is_starred = true", ) # FK lookup index for recurring children DELETE diff --git a/backend/app/jobs/notifications.py b/backend/app/jobs/notifications.py index 045a4ab..cc97c35 100644 --- a/backend/app/jobs/notifications.py +++ b/backend/app/jobs/notifications.py @@ -41,9 +41,13 @@ UMBRA_URL = "http://10.0.69.35" # ── Dedup helpers ───────────────────────────────────────────────────────────── async def _get_sent_keys(db: AsyncSession, user_id: int) -> set[str]: - """Batch-fetch all notification keys for a user in one query.""" + """Batch-fetch recent notification keys for a user (within the 7-day purge window).""" + cutoff = datetime.now() - timedelta(days=7) result = await db.execute( - select(NtfySent.notification_key).where(NtfySent.user_id == user_id) + select(NtfySent.notification_key).where( + NtfySent.user_id == user_id, + NtfySent.sent_at >= cutoff, + ) ) return set(result.scalars().all()) diff --git a/backend/app/models/event_template.py b/backend/app/models/event_template.py index 78f8a2d..d4cb662 100644 --- a/backend/app/models/event_template.py +++ b/backend/app/models/event_template.py @@ -24,4 +24,4 @@ class EventTemplate(Base): Integer, ForeignKey("locations.id", ondelete="SET NULL"), nullable=True ) is_starred: Mapped[bool] = mapped_column(Boolean, default=False) - created_at: Mapped[datetime] = mapped_column(default=datetime.now, server_default=func.now()) + created_at: Mapped[datetime] = mapped_column(default=func.now(), server_default=func.now()) diff --git a/backend/app/routers/auth.py b/backend/app/routers/auth.py index eda49c0..69038f2 100644 --- a/backend/app/routers/auth.py +++ b/backend/app/routers/auth.py @@ -195,7 +195,7 @@ async def _create_db_session( user_agent=(user_agent or "")[:255] if user_agent else None, ) db.add(db_session) - await db.commit() + await db.flush() token = create_session_token(user.id, session_id) return session_id, token @@ -335,6 +335,7 @@ async def login( user_agent = request.headers.get("user-agent") _, token = await _create_db_session(db, user, client_ip, user_agent) _set_session_cookie(response, token) + await db.commit() return { "authenticated": True, "must_change_password": True, @@ -415,6 +416,7 @@ async def register( _, token = await _create_db_session(db, new_user, ip, user_agent) _set_session_cookie(response, token) + await db.commit() return {"message": "Registration successful", "authenticated": True} diff --git a/backend/app/routers/weather.py b/backend/app/routers/weather.py index bed8724..8568b37 100644 --- a/backend/app/routers/weather.py +++ b/backend/app/routers/weather.py @@ -21,6 +21,8 @@ router = APIRouter() # SEC-15: Bounded LRU cache keyed by (user_id, location) — max 100 entries. # OrderedDict preserves insertion order; move_to_end on hit, popitem(last=False) # to evict the oldest when capacity is exceeded. +# NOTE: This cache is process-local. With multiple workers each process would +# maintain its own copy, wasting API quota. Currently safe — single Uvicorn worker. _CACHE_MAX = 100 _cache: OrderedDict = OrderedDict() diff --git a/backend/app/schemas/admin.py b/backend/app/schemas/admin.py index 958a26c..7688bc5 100644 --- a/backend/app/schemas/admin.py +++ b/backend/app/schemas/admin.py @@ -99,14 +99,30 @@ class SystemConfigUpdate(BaseModel): # Admin dashboard # --------------------------------------------------------------------------- +class RecentLoginItem(BaseModel): + username: str + last_login_at: Optional[datetime] = None + + model_config = ConfigDict(from_attributes=True) + + +class RecentAuditItem(BaseModel): + action: str + actor_username: Optional[str] = None + target_username: Optional[str] = None + created_at: datetime + + model_config = ConfigDict(from_attributes=True) + + class AdminDashboardResponse(BaseModel): total_users: int active_users: int admin_count: int active_sessions: int mfa_adoption_rate: float - recent_logins: list[dict] - recent_audit_entries: list[dict] + recent_logins: list[RecentLoginItem] + recent_audit_entries: list[RecentAuditItem] # ---------------------------------------------------------------------------