From df857a57193cb4beee1028a410c8dd1a434d20fa Mon Sep 17 00:00:00 2001 From: Kyle Pope Date: Mon, 16 Mar 2026 14:01:15 +0800 Subject: [PATCH] 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 --- backend/app/routers/dashboard.py | 14 +-- backend/app/routers/events.py | 16 +++- backend/app/schemas/event_invitation.py | 7 +- backend/app/services/calendar_sharing.py | 11 +-- backend/app/services/event_invitation.py | 13 ++- .../notifications/NotificationToaster.tsx | 92 ++++++++----------- .../notifications/NotificationsPage.tsx | 19 ++-- 7 files changed, 81 insertions(+), 91 deletions(-) diff --git a/backend/app/routers/dashboard.py b/backend/app/routers/dashboard.py index ec0de48..71ee255 100644 --- a/backend/app/routers/dashboard.py +++ b/backend/app/routers/dashboard.py @@ -1,6 +1,6 @@ from fastapi import APIRouter, Depends, Query 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 typing import Optional, List, Dict, Any @@ -44,8 +44,8 @@ async def get_dashboard( events_query = select(CalendarEvent).where( or_( CalendarEvent.calendar_id.in_(user_calendar_ids), - CalendarEvent.id.in_(invited_event_ids) if invited_event_ids else False, - CalendarEvent.parent_event_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 sa_false(), ), CalendarEvent.start_datetime >= today_start, CalendarEvent.start_datetime <= today_end, @@ -101,8 +101,8 @@ async def get_dashboard( starred_query = select(CalendarEvent).where( or_( CalendarEvent.calendar_id.in_(user_calendar_ids), - CalendarEvent.id.in_(invited_event_ids) if invited_event_ids else False, - CalendarEvent.parent_event_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 sa_false(), ), CalendarEvent.is_starred == True, CalendarEvent.start_datetime > today_start, @@ -192,8 +192,8 @@ async def get_upcoming( events_query = select(CalendarEvent).where( or_( CalendarEvent.calendar_id.in_(user_calendar_ids), - CalendarEvent.id.in_(invited_event_ids) if invited_event_ids else False, - CalendarEvent.parent_event_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 sa_false(), ), CalendarEvent.start_datetime >= today_start, CalendarEvent.start_datetime <= cutoff_datetime, diff --git a/backend/app/routers/events.py b/backend/app/routers/events.py index cc47e12..a01b9fc 100644 --- a/backend/app/routers/events.py +++ b/backend/app/routers/events.py @@ -1,7 +1,7 @@ import json from fastapi import APIRouter, Depends, HTTPException, Path, Query 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 typing import Optional, List, Any, Literal @@ -164,8 +164,8 @@ async def get_events( .where( or_( CalendarEvent.calendar_id.in_(all_calendar_ids), - CalendarEvent.id.in_(invited_event_ids) if invited_event_ids else False, - CalendarEvent.parent_event_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 sa_false(), ) ) ) @@ -339,8 +339,8 @@ async def get_event( CalendarEvent.id == event_id, or_( CalendarEvent.calendar_id.in_(all_calendar_ids), - CalendarEvent.id.in_(invited_event_ids) if invited_event_ids else False, - CalendarEvent.parent_event_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 sa_false(), ), ) ) @@ -359,6 +359,9 @@ async def update_event( db: AsyncSession = Depends(get_db), 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) result = await db.execute( @@ -504,6 +507,9 @@ async def delete_event( db: AsyncSession = Depends(get_db), 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) result = await db.execute( diff --git a/backend/app/schemas/event_invitation.py b/backend/app/schemas/event_invitation.py index 6b120ca..0c68ce2 100644 --- a/backend/app/schemas/event_invitation.py +++ b/backend/app/schemas/event_invitation.py @@ -1,11 +1,12 @@ -from pydantic import BaseModel, ConfigDict, Field -from typing import Literal, Optional +from typing import Annotated, Literal, Optional from datetime import datetime +from pydantic import BaseModel, ConfigDict, Field + class EventInvitationCreate(BaseModel): 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): diff --git a/backend/app/services/calendar_sharing.py b/backend/app/services/calendar_sharing.py index 8b2a112..20bdd03 100644 --- a/backend/app/services/calendar_sharing.py +++ b/backend/app/services/calendar_sharing.py @@ -13,7 +13,6 @@ from sqlalchemy.ext.asyncio import AsyncSession from app.models.calendar import Calendar from app.models.calendar_member import CalendarMember from app.models.event_lock import EventLock -from app.models.event_invitation import EventInvitation 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). 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) - invited_result = await db.execute( - select(EventInvitation.event_id).where( - EventInvitation.user_id == user_id, - EventInvitation.status != "declined", - ) - ) - invited_event_ids = [r[0] for r in invited_result.all()] + invited_event_ids = await get_invited_event_ids(db, user_id) return cal_ids, invited_event_ids diff --git a/backend/app/services/event_invitation.py b/backend/app/services/event_invitation.py index c0ba7ac..e6139a2 100644 --- a/backend/app/services/event_invitation.py +++ b/backend/app/services/event_invitation.py @@ -106,15 +106,18 @@ async def send_event_invitations( db.add(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 "" await create_notification( db=db, - user_id=uid, + user_id=inv.user_id, type="event_invite", title="Event Invitation", 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_id=event_id, ) @@ -148,8 +151,8 @@ async def respond_to_invitation( invitation.status = status invitation.responded_at = datetime.now() - # Notify the inviter - if invitation.invited_by: + # Notify the inviter only if status actually changed (prevents duplicate notifications) + if invitation.invited_by and old_status != status: status_label = {"accepted": "Going", "tentative": "Tentative", "declined": "Declined"} # Fetch responder name responder_settings = await db.execute( diff --git a/frontend/src/components/notifications/NotificationToaster.tsx b/frontend/src/components/notifications/NotificationToaster.tsx index a1cab26..8279b3f 100644 --- a/frontend/src/components/notifications/NotificationToaster.tsx +++ b/frontend/src/components/notifications/NotificationToaster.tsx @@ -278,14 +278,42 @@ export default function NotificationToaster() { { id: `calendar-invite-${inviteId}`, duration: 30000 }, ); }; + const resolveInvitationId = async (notification: AppNotification): Promise => { + const data = notification.data as Record | 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 data = notification.data as Record; - 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}`; - // We need the invitation ID to respond — fetch pending invitations - // For now, use a simplified approach: the toast will query pending invitations toast.custom( (id) => (
@@ -300,69 +328,21 @@ export default function NotificationToaster() {