Fix QA review findings: 3 critical, 5 warnings, 1 suggestion

Critical:
- C-01: Populate member_count in GET /calendars for shared calendars
- C-02: Differentiate 423 lock errors in drag-drop onError (show lock-specific toast)
- C-03: Add expired lock purge to APScheduler housekeeping job

Warnings:
- W-01: Replace setattr loop with explicit field assignment in update_member
- W-02: Cap sync `since` param to 7 days to prevent unbounded scans
- W-05: Remove cosmetic isShared toggle (is_shared is auto-managed by invite flow)
- W-06: Populate preferred_name in _build_member_response from user model
- W-07: Add releaseMutation to release callback dependency array

Suggestion:
- S-06: Remove unused ConvertToSharedRequest schema

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
Kyle 2026-03-06 23:41:08 +08:00
parent 206144d20d
commit dd862bfa48
7 changed files with 58 additions and 23 deletions

View File

@ -21,6 +21,7 @@ from app.models.notification import Notification as AppNotification
from app.models.reminder import Reminder from app.models.reminder import Reminder
from app.models.calendar_event import CalendarEvent from app.models.calendar_event import CalendarEvent
from app.models.calendar import Calendar from app.models.calendar import Calendar
from app.models.event_lock import EventLock
from app.models.todo import Todo from app.models.todo import Todo
from app.models.project import Project from app.models.project import Project
from app.models.ntfy_sent import NtfySent from app.models.ntfy_sent import NtfySent
@ -300,6 +301,18 @@ async def _purge_resolved_requests(db: AsyncSession) -> None:
await db.commit() await db.commit()
async def _purge_expired_locks(db: AsyncSession) -> None:
"""Remove non-permanent event locks that have expired."""
await db.execute(
delete(EventLock).where(
EventLock.is_permanent == False, # noqa: E712
EventLock.expires_at < datetime.now(),
)
)
await db.commit()
# ── Entry point ─────────────────────────────────────────────────────────────── # ── Entry point ───────────────────────────────────────────────────────────────
async def run_notification_dispatch() -> None: async def run_notification_dispatch() -> None:
@ -343,6 +356,7 @@ async def run_notification_dispatch() -> None:
await _purge_expired_sessions(db) await _purge_expired_sessions(db)
await _purge_old_notifications(db) await _purge_old_notifications(db)
await _purge_resolved_requests(db) await _purge_resolved_requests(db)
await _purge_expired_locks(db)
except Exception: except Exception:
# Broad catch: job failure must never crash the scheduler or the app # Broad catch: job failure must never crash the scheduler or the app

View File

@ -1,11 +1,12 @@
from fastapi import APIRouter, Depends, HTTPException, Path from fastapi import APIRouter, Depends, HTTPException, Path
from sqlalchemy.ext.asyncio import AsyncSession from sqlalchemy.ext.asyncio import AsyncSession
from sqlalchemy import select, update from sqlalchemy import func, select, update
from typing import List from typing import List
from app.database import get_db from app.database import get_db
from app.models.calendar import Calendar from app.models.calendar import Calendar
from app.models.calendar_event import CalendarEvent from app.models.calendar_event import CalendarEvent
from app.models.calendar_member import CalendarMember
from app.schemas.calendar import CalendarCreate, CalendarUpdate, CalendarResponse from app.schemas.calendar import CalendarCreate, CalendarUpdate, CalendarResponse
from app.routers.auth import get_current_user from app.routers.auth import get_current_user
from app.models.user import User from app.models.user import User
@ -23,7 +24,28 @@ async def get_calendars(
.where(Calendar.user_id == current_user.id) .where(Calendar.user_id == current_user.id)
.order_by(Calendar.is_default.desc(), Calendar.name.asc()) .order_by(Calendar.is_default.desc(), Calendar.name.asc())
) )
return result.scalars().all() calendars = result.scalars().all()
# Populate member_count for shared calendars
cal_ids = [c.id for c in calendars if c.is_shared]
count_map: dict[int, int] = {}
if cal_ids:
counts = await db.execute(
select(CalendarMember.calendar_id, func.count())
.where(
CalendarMember.calendar_id.in_(cal_ids),
CalendarMember.status == "accepted",
)
.group_by(CalendarMember.calendar_id)
)
count_map = dict(counts.all())
return [
CalendarResponse.model_validate(c, from_attributes=True).model_copy(
update={"member_count": count_map.get(c.id, 0)}
)
for c in calendars
]
@router.post("/", response_model=CalendarResponse, status_code=201) @router.post("/", response_model=CalendarResponse, status_code=201)

View File

@ -4,7 +4,7 @@ Shared calendars router — invites, membership, locks, sync.
All endpoints live under /api/shared-calendars. All endpoints live under /api/shared-calendars.
""" """
import logging import logging
from datetime import datetime from datetime import datetime, timedelta
from typing import Optional from typing import Optional
from fastapi import APIRouter, Depends, HTTPException, Path, Query, Request from fastapi import APIRouter, Depends, HTTPException, Path, Query, Request
@ -60,7 +60,7 @@ def _build_member_response(member: CalendarMember) -> dict:
"calendar_id": member.calendar_id, "calendar_id": member.calendar_id,
"user_id": member.user_id, "user_id": member.user_id,
"umbral_name": member.user.umbral_name if member.user else "", "umbral_name": member.user.umbral_name if member.user else "",
"preferred_name": None, "preferred_name": member.user.preferred_name if member.user else None,
"permission": member.permission, "permission": member.permission,
"can_add_others": member.can_add_others, "can_add_others": member.can_add_others,
"local_color": member.local_color, "local_color": member.local_color,
@ -462,8 +462,10 @@ async def update_member(
if not update_data: if not update_data:
raise HTTPException(status_code=400, detail="No fields to update") raise HTTPException(status_code=400, detail="No fields to update")
for key, value in update_data.items(): if "permission" in update_data:
setattr(member, key, value) member.permission = update_data["permission"]
if "can_add_others" in update_data:
member.can_add_others = update_data["can_add_others"]
await log_audit_event( await log_audit_event(
db, db,
@ -581,6 +583,11 @@ async def sync_shared_calendars(
"""Sync events and member changes since a given timestamp. Cap 500 events.""" """Sync events and member changes since a given timestamp. Cap 500 events."""
MAX_EVENTS = 500 MAX_EVENTS = 500
# Cap since to 7 days ago to prevent unbounded scans
floor = datetime.now() - timedelta(days=7)
if since < floor:
since = floor
cal_id_list: list[int] = [] cal_id_list: list[int] = []
if calendar_ids: if calendar_ids:
for part in calendar_ids.split(","): for part in calendar_ids.split(","):

View File

@ -34,9 +34,6 @@ class UpdateLocalColorRequest(BaseModel):
return v return v
class ConvertToSharedRequest(BaseModel):
model_config = ConfigDict(extra="forbid")
class CalendarMemberResponse(BaseModel): class CalendarMemberResponse(BaseModel):
model_config = ConfigDict(from_attributes=True) model_config = ConfigDict(from_attributes=True)

View File

@ -14,7 +14,6 @@ import {
import { Input } from '@/components/ui/input'; import { Input } from '@/components/ui/input';
import { Label } from '@/components/ui/label'; import { Label } from '@/components/ui/label';
import { Button } from '@/components/ui/button'; import { Button } from '@/components/ui/button';
import { Switch } from '@/components/ui/switch';
import PermissionToggle from './PermissionToggle'; import PermissionToggle from './PermissionToggle';
import { useConnections } from '@/hooks/useConnections'; import { useConnections } from '@/hooks/useConnections';
import { useSharedCalendars } from '@/hooks/useSharedCalendars'; import { useSharedCalendars } from '@/hooks/useSharedCalendars';
@ -35,7 +34,6 @@ export default function CalendarForm({ calendar, onClose }: CalendarFormProps) {
const queryClient = useQueryClient(); const queryClient = useQueryClient();
const [name, setName] = useState(calendar?.name || ''); const [name, setName] = useState(calendar?.name || '');
const [color, setColor] = useState(calendar?.color || '#3b82f6'); const [color, setColor] = useState(calendar?.color || '#3b82f6');
const [isShared, setIsShared] = useState(calendar?.is_shared ?? false);
const [pendingInvite, setPendingInvite] = useState<{ conn: Connection; permission: CalendarPermission } | null>(null); const [pendingInvite, setPendingInvite] = useState<{ conn: Connection; permission: CalendarPermission } | null>(null);
@ -131,7 +129,7 @@ export default function CalendarForm({ calendar, onClose }: CalendarFormProps) {
return ( return (
<Dialog open={true} onOpenChange={onClose}> <Dialog open={true} onOpenChange={onClose}>
<DialogContent className={isShared && showSharing ? 'max-w-3xl' : undefined}> <DialogContent className={calendar?.is_shared && showSharing ? 'max-w-3xl' : undefined}>
<DialogClose onClick={onClose} /> <DialogClose onClick={onClose} />
<DialogHeader> <DialogHeader>
<DialogTitle>{calendar ? 'Edit Calendar' : 'New Calendar'}</DialogTitle> <DialogTitle>{calendar ? 'Edit Calendar' : 'New Calendar'}</DialogTitle>
@ -170,15 +168,7 @@ export default function CalendarForm({ calendar, onClose }: CalendarFormProps) {
{showSharing && ( {showSharing && (
<> <>
<div className="flex items-center justify-between py-3 border-t border-border"> {calendar?.is_shared && (
<Label className="mb-0">Share this calendar</Label>
<Switch
checked={isShared}
onCheckedChange={setIsShared}
/>
</div>
{isShared && (
<div className="space-y-3"> <div className="space-y-3">
<div className="flex items-center justify-between"> <div className="flex items-center justify-between">
<Label className="mb-0">Members</Label> <Label className="mb-0">Members</Label>

View File

@ -9,6 +9,7 @@ import interactionPlugin from '@fullcalendar/interaction';
import type { EventClickArg, DateSelectArg, EventDropArg, DatesSetArg } from '@fullcalendar/core'; import type { EventClickArg, DateSelectArg, EventDropArg, DatesSetArg } from '@fullcalendar/core';
import { ChevronLeft, ChevronRight, Plus, Search } from 'lucide-react'; import { ChevronLeft, ChevronRight, Plus, Search } from 'lucide-react';
import api, { getErrorMessage } from '@/lib/api'; import api, { getErrorMessage } from '@/lib/api';
import axios from 'axios';
import type { CalendarEvent, EventTemplate, Location as LocationType, CalendarPermission } from '@/types'; import type { CalendarEvent, EventTemplate, Location as LocationType, CalendarPermission } from '@/types';
import { useCalendars } from '@/hooks/useCalendars'; import { useCalendars } from '@/hooks/useCalendars';
import { useSettings } from '@/hooks/useSettings'; import { useSettings } from '@/hooks/useSettings';
@ -244,7 +245,11 @@ export default function CalendarPage() {
onError: (error, variables) => { onError: (error, variables) => {
variables.revert(); variables.revert();
queryClient.invalidateQueries({ queryKey: ['calendar-events'] }); queryClient.invalidateQueries({ queryKey: ['calendar-events'] });
toast.error(getErrorMessage(error, 'Failed to update event')); if (axios.isAxiosError(error) && error.response?.status === 423) {
toast.error('Event is locked by another user');
} else {
toast.error(getErrorMessage(error, 'Failed to update event'));
}
}, },
}); });

View File

@ -44,7 +44,7 @@ export function useEventLock(eventId: number | null) {
} catch { } catch {
// Fire-and-forget on release errors // Fire-and-forget on release errors
} }
}, []); }, [releaseMutation]);
// Auto-release on unmount or eventId change // Auto-release on unmount or eventId change
useEffect(() => { useEffect(() => {