Fix QA findings: flush before notify, dedup RSVP, sa_false, validation

- C-02: flush invitations before creating notifications so invitation_id
  is available in notification data; eliminates extra pending fetch
- C-03: skip RSVP notification when status hasn't changed
- C-01: add defensive comments on update/delete endpoints
- W-01: add ge=1, le=2147483647 per-element validation on user_ids
- W-04: deduplicate invited_event_ids query via get_invited_event_ids()
- W-06: replace Python False with sa_false() in or_() clauses
- Frontend: extract resolveInvitationId helper, prefer data.invitation_id

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
Kyle 2026-03-16 14:01:15 +08:00
parent 496666ec5a
commit df857a5719
7 changed files with 81 additions and 91 deletions

View File

@ -1,6 +1,6 @@
from fastapi import APIRouter, Depends, Query from fastapi import APIRouter, Depends, Query
from sqlalchemy.ext.asyncio import AsyncSession from sqlalchemy.ext.asyncio import AsyncSession
from sqlalchemy import select, func, or_, case from sqlalchemy import false as sa_false, select, func, or_, case
from datetime import datetime, date, timedelta from datetime import datetime, date, timedelta
from typing import Optional, List, Dict, Any from typing import Optional, List, Dict, Any
@ -44,8 +44,8 @@ async def get_dashboard(
events_query = select(CalendarEvent).where( events_query = select(CalendarEvent).where(
or_( or_(
CalendarEvent.calendar_id.in_(user_calendar_ids), CalendarEvent.calendar_id.in_(user_calendar_ids),
CalendarEvent.id.in_(invited_event_ids) if invited_event_ids else False, CalendarEvent.id.in_(invited_event_ids) if invited_event_ids else sa_false(),
CalendarEvent.parent_event_id.in_(invited_event_ids) if invited_event_ids else False, CalendarEvent.parent_event_id.in_(invited_event_ids) if invited_event_ids else sa_false(),
), ),
CalendarEvent.start_datetime >= today_start, CalendarEvent.start_datetime >= today_start,
CalendarEvent.start_datetime <= today_end, CalendarEvent.start_datetime <= today_end,
@ -101,8 +101,8 @@ async def get_dashboard(
starred_query = select(CalendarEvent).where( starred_query = select(CalendarEvent).where(
or_( or_(
CalendarEvent.calendar_id.in_(user_calendar_ids), CalendarEvent.calendar_id.in_(user_calendar_ids),
CalendarEvent.id.in_(invited_event_ids) if invited_event_ids else False, CalendarEvent.id.in_(invited_event_ids) if invited_event_ids else sa_false(),
CalendarEvent.parent_event_id.in_(invited_event_ids) if invited_event_ids else False, CalendarEvent.parent_event_id.in_(invited_event_ids) if invited_event_ids else sa_false(),
), ),
CalendarEvent.is_starred == True, CalendarEvent.is_starred == True,
CalendarEvent.start_datetime > today_start, CalendarEvent.start_datetime > today_start,
@ -192,8 +192,8 @@ async def get_upcoming(
events_query = select(CalendarEvent).where( events_query = select(CalendarEvent).where(
or_( or_(
CalendarEvent.calendar_id.in_(user_calendar_ids), CalendarEvent.calendar_id.in_(user_calendar_ids),
CalendarEvent.id.in_(invited_event_ids) if invited_event_ids else False, CalendarEvent.id.in_(invited_event_ids) if invited_event_ids else sa_false(),
CalendarEvent.parent_event_id.in_(invited_event_ids) if invited_event_ids else False, CalendarEvent.parent_event_id.in_(invited_event_ids) if invited_event_ids else sa_false(),
), ),
CalendarEvent.start_datetime >= today_start, CalendarEvent.start_datetime >= today_start,
CalendarEvent.start_datetime <= cutoff_datetime, CalendarEvent.start_datetime <= cutoff_datetime,

View File

@ -1,7 +1,7 @@
import json import json
from fastapi import APIRouter, Depends, HTTPException, Path, Query from fastapi import APIRouter, Depends, HTTPException, Path, Query
from sqlalchemy.ext.asyncio import AsyncSession from sqlalchemy.ext.asyncio import AsyncSession
from sqlalchemy import select, delete, or_ from sqlalchemy import false as sa_false, select, delete, or_
from sqlalchemy.orm import selectinload from sqlalchemy.orm import selectinload
from typing import Optional, List, Any, Literal from typing import Optional, List, Any, Literal
@ -164,8 +164,8 @@ async def get_events(
.where( .where(
or_( or_(
CalendarEvent.calendar_id.in_(all_calendar_ids), CalendarEvent.calendar_id.in_(all_calendar_ids),
CalendarEvent.id.in_(invited_event_ids) if invited_event_ids else False, CalendarEvent.id.in_(invited_event_ids) if invited_event_ids else sa_false(),
CalendarEvent.parent_event_id.in_(invited_event_ids) if invited_event_ids else False, CalendarEvent.parent_event_id.in_(invited_event_ids) if invited_event_ids else sa_false(),
) )
) )
) )
@ -339,8 +339,8 @@ async def get_event(
CalendarEvent.id == event_id, CalendarEvent.id == event_id,
or_( or_(
CalendarEvent.calendar_id.in_(all_calendar_ids), CalendarEvent.calendar_id.in_(all_calendar_ids),
CalendarEvent.id.in_(invited_event_ids) if invited_event_ids else False, CalendarEvent.id.in_(invited_event_ids) if invited_event_ids else sa_false(),
CalendarEvent.parent_event_id.in_(invited_event_ids) if invited_event_ids else False, CalendarEvent.parent_event_id.in_(invited_event_ids) if invited_event_ids else sa_false(),
), ),
) )
) )
@ -359,6 +359,9 @@ async def update_event(
db: AsyncSession = Depends(get_db), db: AsyncSession = Depends(get_db),
current_user: User = Depends(get_current_user), current_user: User = Depends(get_current_user),
): ):
# IMPORTANT: Uses get_accessible_calendar_ids (NOT get_accessible_event_scope).
# Event invitees can VIEW events but must NOT be able to edit them.
# Do not add invited_event_ids to this query.
all_calendar_ids = await get_accessible_calendar_ids(current_user.id, db) all_calendar_ids = await get_accessible_calendar_ids(current_user.id, db)
result = await db.execute( result = await db.execute(
@ -504,6 +507,9 @@ async def delete_event(
db: AsyncSession = Depends(get_db), db: AsyncSession = Depends(get_db),
current_user: User = Depends(get_current_user), current_user: User = Depends(get_current_user),
): ):
# IMPORTANT: Uses get_accessible_calendar_ids (NOT get_accessible_event_scope).
# Event invitees can VIEW events but must NOT be able to delete them.
# Invitees use DELETE /api/event-invitations/{id} to leave instead.
all_calendar_ids = await get_accessible_calendar_ids(current_user.id, db) all_calendar_ids = await get_accessible_calendar_ids(current_user.id, db)
result = await db.execute( result = await db.execute(

View File

@ -1,11 +1,12 @@
from pydantic import BaseModel, ConfigDict, Field from typing import Annotated, Literal, Optional
from typing import Literal, Optional
from datetime import datetime from datetime import datetime
from pydantic import BaseModel, ConfigDict, Field
class EventInvitationCreate(BaseModel): class EventInvitationCreate(BaseModel):
model_config = ConfigDict(extra="forbid") model_config = ConfigDict(extra="forbid")
user_ids: list[int] = Field(..., min_length=1, max_length=20) user_ids: list[Annotated[int, Field(ge=1, le=2147483647)]] = Field(..., min_length=1, max_length=20)
class EventInvitationRespond(BaseModel): class EventInvitationRespond(BaseModel):

View File

@ -13,7 +13,6 @@ from sqlalchemy.ext.asyncio import AsyncSession
from app.models.calendar import Calendar from app.models.calendar import Calendar
from app.models.calendar_member import CalendarMember from app.models.calendar_member import CalendarMember
from app.models.event_lock import EventLock from app.models.event_lock import EventLock
from app.models.event_invitation import EventInvitation
logger = logging.getLogger(__name__) logger = logging.getLogger(__name__)
@ -43,14 +42,10 @@ async def get_accessible_event_scope(
calendar_ids: all calendars the user can access (owned + accepted shared). calendar_ids: all calendars the user can access (owned + accepted shared).
invited_parent_event_ids: event IDs where the user has a non-declined invitation. invited_parent_event_ids: event IDs where the user has a non-declined invitation.
""" """
from app.services.event_invitation import get_invited_event_ids
cal_ids = await get_accessible_calendar_ids(user_id, db) cal_ids = await get_accessible_calendar_ids(user_id, db)
invited_result = await db.execute( invited_event_ids = await get_invited_event_ids(db, user_id)
select(EventInvitation.event_id).where(
EventInvitation.user_id == user_id,
EventInvitation.status != "declined",
)
)
invited_event_ids = [r[0] for r in invited_result.all()]
return cal_ids, invited_event_ids return cal_ids, invited_event_ids

View File

@ -106,15 +106,18 @@ async def send_event_invitations(
db.add(inv) db.add(inv)
invitations.append(inv) invitations.append(inv)
# Create notification # Flush to populate invitation IDs before creating notifications
await db.flush()
for inv in invitations:
start_str = event_start.strftime("%b %d, %I:%M %p") if event_start else "" start_str = event_start.strftime("%b %d, %I:%M %p") if event_start else ""
await create_notification( await create_notification(
db=db, db=db,
user_id=uid, user_id=inv.user_id,
type="event_invite", type="event_invite",
title="Event Invitation", title="Event Invitation",
message=f"{inviter_name} invited you to {event_title}" + (f" · {start_str}" if start_str else ""), message=f"{inviter_name} invited you to {event_title}" + (f" · {start_str}" if start_str else ""),
data={"event_id": event_id, "event_title": event_title}, data={"event_id": event_id, "event_title": event_title, "invitation_id": inv.id},
source_type="event_invitation", source_type="event_invitation",
source_id=event_id, source_id=event_id,
) )
@ -148,8 +151,8 @@ async def respond_to_invitation(
invitation.status = status invitation.status = status
invitation.responded_at = datetime.now() invitation.responded_at = datetime.now()
# Notify the inviter # Notify the inviter only if status actually changed (prevents duplicate notifications)
if invitation.invited_by: if invitation.invited_by and old_status != status:
status_label = {"accepted": "Going", "tentative": "Tentative", "declined": "Declined"} status_label = {"accepted": "Going", "tentative": "Tentative", "declined": "Declined"}
# Fetch responder name # Fetch responder name
responder_settings = await db.execute( responder_settings = await db.execute(

View File

@ -278,14 +278,42 @@ export default function NotificationToaster() {
{ id: `calendar-invite-${inviteId}`, duration: 30000 }, { id: `calendar-invite-${inviteId}`, duration: 30000 },
); );
}; };
const resolveInvitationId = async (notification: AppNotification): Promise<number | null> => {
const data = notification.data as Record<string, unknown> | undefined;
// Prefer invitation_id from notification data (populated after flush fix)
if (data?.invitation_id) return data.invitation_id as number;
// Fallback: fetch pending invitations to resolve by event_id
const eventId = data?.event_id as number | undefined;
if (!eventId) return null;
try {
const { data: pending } = await api.get('/event-invitations/pending');
const inv = (pending as Array<{ id: number; event_id: number }>).find(
(p) => p.event_id === eventId,
);
return inv?.id ?? null;
} catch {
return null;
}
};
const handleEventToastClick = async (
notification: AppNotification,
status: 'accepted' | 'tentative' | 'declined',
toastId: string | number,
) => {
const invId = await resolveInvitationId(notification);
if (invId) {
handleEventInviteRespond(invId, status, toastId, notification.id);
} else {
toast.dismiss(toastId);
markReadRef.current([notification.id]).catch(() => {});
toast.success('Already responded');
}
};
const showEventInviteToast = (notification: AppNotification) => { const showEventInviteToast = (notification: AppNotification) => {
const data = notification.data as Record<string, unknown>;
const eventId = data?.event_id as number;
// Use source_id as a stable ID for dedup (it's the event_id)
const inviteKey = `event-invite-${notification.id}`; const inviteKey = `event-invite-${notification.id}`;
// We need the invitation ID to respond — fetch pending invitations
// For now, use a simplified approach: the toast will query pending invitations
toast.custom( toast.custom(
(id) => ( (id) => (
<div className="w-[356px] rounded-lg border border-border bg-card p-4 shadow-lg"> <div className="w-[356px] rounded-lg border border-border bg-card p-4 shadow-lg">
@ -300,69 +328,21 @@ export default function NotificationToaster() {
</p> </p>
<div className="flex items-center gap-2 mt-3"> <div className="flex items-center gap-2 mt-3">
<button <button
onClick={async () => { onClick={() => handleEventToastClick(notification, 'accepted', id)}
// Fetch the invitation ID from pending invitations
try {
const { data: pending } = await api.get('/event-invitations/pending');
const inv = (pending as Array<{ id: number; event_id: number }>).find(
(p) => p.event_id === eventId
);
if (inv) {
handleEventInviteRespond(inv.id, 'accepted', id, notification.id);
} else {
toast.dismiss(id);
markReadRef.current([notification.id]).catch(() => {});
toast.success('Already responded');
}
} catch {
toast.dismiss(id);
toast.error('Failed to respond');
}
}}
className="flex items-center gap-1 px-3 py-1.5 text-xs font-medium rounded-md bg-accent text-accent-foreground hover:bg-accent/90 transition-colors" className="flex items-center gap-1 px-3 py-1.5 text-xs font-medium rounded-md bg-accent text-accent-foreground hover:bg-accent/90 transition-colors"
> >
<Check className="h-3.5 w-3.5" /> <Check className="h-3.5 w-3.5" />
Accept Accept
</button> </button>
<button <button
onClick={async () => { onClick={() => handleEventToastClick(notification, 'tentative', id)}
try {
const { data: pending } = await api.get('/event-invitations/pending');
const inv = (pending as Array<{ id: number; event_id: number }>).find(
(p) => p.event_id === eventId
);
if (inv) {
handleEventInviteRespond(inv.id, 'tentative', id, notification.id);
} else {
toast.dismiss(id);
markReadRef.current([notification.id]).catch(() => {});
}
} catch {
toast.dismiss(id);
}
}}
className="flex items-center gap-1 px-3 py-1.5 text-xs font-medium rounded-md bg-amber-500/15 text-amber-400 hover:bg-amber-500/25 transition-colors" className="flex items-center gap-1 px-3 py-1.5 text-xs font-medium rounded-md bg-amber-500/15 text-amber-400 hover:bg-amber-500/25 transition-colors"
> >
<Clock className="h-3.5 w-3.5" /> <Clock className="h-3.5 w-3.5" />
Tentative Tentative
</button> </button>
<button <button
onClick={async () => { onClick={() => handleEventToastClick(notification, 'declined', id)}
try {
const { data: pending } = await api.get('/event-invitations/pending');
const inv = (pending as Array<{ id: number; event_id: number }>).find(
(p) => p.event_id === eventId
);
if (inv) {
handleEventInviteRespond(inv.id, 'declined', id, notification.id);
} else {
toast.dismiss(id);
markReadRef.current([notification.id]).catch(() => {});
}
} catch {
toast.dismiss(id);
}
}}
className="flex items-center gap-1 px-3 py-1.5 text-xs font-medium rounded-md text-muted-foreground hover:bg-card-elevated transition-colors" className="flex items-center gap-1 px-3 py-1.5 text-xs font-medium rounded-md text-muted-foreground hover:bg-card-elevated transition-colors"
> >
<X className="h-3.5 w-3.5" /> <X className="h-3.5 w-3.5" />

View File

@ -158,13 +158,18 @@ export default function NotificationsPage() {
setRespondingEventInvite(notification.id); setRespondingEventInvite(notification.id);
try { try {
// Fetch pending invitations to resolve the invitation ID // Prefer invitation_id from notification data; fallback to pending fetch
let invitationId = data?.invitation_id as number | undefined;
if (!invitationId) {
const { data: pending } = await api.get('/event-invitations/pending'); const { data: pending } = await api.get('/event-invitations/pending');
const inv = (pending as Array<{ id: number; event_id: number }>).find( const inv = (pending as Array<{ id: number; event_id: number }>).find(
(p) => p.event_id === eventId, (p) => p.event_id === eventId,
); );
if (inv) { invitationId = inv?.id;
await api.put(`/event-invitations/${inv.id}/respond`, { status }); }
if (invitationId) {
await api.put(`/event-invitations/${invitationId}/respond`, { status });
const successLabel = { accepted: 'Going', tentative: 'Tentative', declined: 'Declined' }; const successLabel = { accepted: 'Going', tentative: 'Tentative', declined: 'Declined' };
toast.success(`Marked as ${successLabel[status]}`); toast.success(`Marked as ${successLabel[status]}`);
queryClient.invalidateQueries({ queryKey: ['calendar-events'] }); queryClient.invalidateQueries({ queryKey: ['calendar-events'] });