From 83224700106fc826d3fe668da5ddec2303100cf9 Mon Sep 17 00:00:00 2001 From: pataniaeli Date: Wed, 9 Sep 2026 20:00:41 -0400 Subject: [PATCH] fix: marking a cancellation Done applies the outcome it asked for (#96) Marking a request Done set status='Done' on the request row and touched nothing else, so the booking stayed Pending Cancellation forever. Applying the outcome is not enough on its own, because the request could not find the reservation it was about. cancellation_requests.occurrence_id names a dated row, and for a weekly booking that row does not survive an edit: the PATCH handler deletes every occurrence and reinserts it on each save, so the ids change while the values are carried across on the date. Every cancellation_requests row in production is in that state -- all four, including the one still Pending. That had a second consequence nobody was looking for. Auto-Cancel finds those reservations anyway, because it scans by status rather than by request, but it could not attribute them back to a request -- so it lost the cancellation_type and fell back to 'Cancelled'. A request that asked to go Virtual would have cancelled the meeting outright. So requests now record occurrence_date, resolved at creation while the id still names a live row, and are matched on (booking_id, occurrence_date) with the id tried first for the cases where it still resolves. The date is the stable identifier the write model actually preserves, which is what 20260829001000 concluded for event_tracking and what issue #69 concluded for calendar UIDs. Verified against production, read-only: the id the one Pending request stored resolves to zero rows, while (booking_id, date) resolves to exactly one -- and it is the row marked Pending Cancellation. The backfill recovers nothing in production, because there is no record anywhere of which date those four requests named. They are left NULL rather than guessed at; the one still Pending needs an administrator's eye, since its booking has two weeks pending and only one request. The backfill is written for environments where the rows are intact, and for a restore. Marking Done includes reservations with no code on file, which Auto-Cancel excludes. A missing code means CSC cannot be asked; it says nothing about whether an administrator has dealt with it, and marking Done by hand is that administrator saying they have. This is why SkippedReservation now carries the identity to act on rather than only the four display fields. Applying the statuses and building the audit rows are now shared with Auto-Cancel, which has to reach the same result: the same request resolved either way should leave the database in the same state. Co-Authored-By: Claude Opus 5 --- .../cancellations/auto-cancel/route.ts | 53 ++----- app/api/administrator/cancellations/route.ts | 72 ++++++++-- app/api/cancellation-requests/route.ts | 27 ++++ lib/pending-cancellations.ts | 130 +++++++++++++++--- ..._cancellation_requests_occurrence_date.sql | 60 ++++++++ ...tion_requests_occurrence_date_rollback.sql | 11 ++ 6 files changed, 287 insertions(+), 66 deletions(-) create mode 100644 supabase/migrations/20260909010000_cancellation_requests_occurrence_date.sql create mode 100644 supabase/migrations/rollback/20260909_cancellation_requests_occurrence_date_rollback.sql diff --git a/app/api/administrator/cancellations/auto-cancel/route.ts b/app/api/administrator/cancellations/auto-cancel/route.ts index 4025049..d51330e 100644 --- a/app/api/administrator/cancellations/auto-cancel/route.ts +++ b/app/api/administrator/cancellations/auto-cancel/route.ts @@ -3,8 +3,14 @@ import { createClient as createAdminClient } from '@supabase/supabase-js' import { NextResponse } from 'next/server' import { checkRateLimit } from '@/lib/check-rate-limit' import { getAuthedUserWithLiveRoles } from '@/lib/authorization' -import { sendCscCancellationRequest, type CancellationLine } from '@/lib/emails/csc-cancellation-request' -import { collectPending, lineKey, requestsFullyCovered } from '@/lib/pending-cancellations' +import { sendCscCancellationRequest } from '@/lib/emails/csc-cancellation-request' +import { + applyCancellationOutcomes, + cancellationAuditRows, + collectPending, + lineKey, + requestsFullyCovered, +} from '@/lib/pending-cancellations' const adminSupabase = createAdminClient( process.env.NEXT_PUBLIC_SUPABASE_URL!, @@ -182,36 +188,10 @@ export async function POST(request: Request) { return NextResponse.json({ error: 'The email could not be sent. No bookings were changed.' }, { status: 502 }) } - // Grouped by table *and* by the status each row is due, so a batch containing - // both kinds writes each its own value. Marking everything 'Cancelled' would - // be wrong for a booking whose request said it was going virtual: that meeting - // still happens, it just does not need the room. - // - // An occurrence whose status was inherited gets its value written onto the - // occurrence itself, which is correct -- only the dates actually sent stop - // being pending, and the rest of the series is untouched. - const TABLE_OF: Record = { - one_time: 'one_time_room_bookings', - occurrence: 'weekly_room_occurrences', - tabling_session: 'tabling_sessions', - } - - const batches = new Map() - for (const l of selected) { - const table = TABLE_OF[l.source] - const bucket = `${table}:${l.resultingStatus}` - if (!batches.has(bucket)) batches.set(bucket, { table, status: l.resultingStatus, ids: [] }) - batches.get(bucket)!.ids.push(l.id) - } - - const failures: string[] = [] - for (const { table, status, ids } of batches.values()) { - const { error } = await adminSupabase.from(table).update({ status }).in('id', ids) - if (error) { - console.error(`Auto-Cancel could not mark ${table} as ${status}:`, error) - failures.push(`${table} (${status})`) - } - } + // Shared with marking a request Done by hand, which has to reach the same + // result: the same request resolved either way should leave the database in + // the same state. + const failures = await applyCancellationOutcomes(selected) // Close the cancellation requests this send acted on, so nobody has to go and // press "Mark as Done" for work Auto-Cancel already did. @@ -241,14 +221,7 @@ export async function POST(request: Request) { // beside every other status change rather than appearing to have happened by // itself. Best effort: the email is out and the statuses are moved, and // failing the request over a missing log would invite a resend. - // Keyed on booking *and* status: one booking can contribute both a cancelled - // week and a virtual one in the same batch, and a single row saying 'Cancelled' - // would misreport the other. - const auditRows = [...new Map( - selected - .filter(l => l.bookingId) - .map(l => [`${l.bookingId}:${l.resultingStatus}`, { booking_id: l.bookingId, admin_id: user.id, new_status: l.resultingStatus }]) - ).values()] + const auditRows = cancellationAuditRows(selected, user.id) if (auditRows.length) { const { error } = await adminSupabase.from('audit_logs').insert(auditRows) if (error) console.error('Auto-Cancel audit log failed:', error) diff --git a/app/api/administrator/cancellations/route.ts b/app/api/administrator/cancellations/route.ts index b502f41..bba3293 100644 --- a/app/api/administrator/cancellations/route.ts +++ b/app/api/administrator/cancellations/route.ts @@ -3,6 +3,11 @@ import { createClient as createAdminClient } from '@supabase/supabase-js' import { NextResponse } from 'next/server' import { checkRateLimit } from '@/lib/check-rate-limit' import { getAuthedUserWithLiveRoles } from '@/lib/authorization' +import { + applyCancellationOutcomes, + cancellationAuditRows, + collectPending, +} from '@/lib/pending-cancellations' const adminSupabase = createAdminClient( process.env.NEXT_PUBLIC_SUPABASE_URL!, @@ -23,7 +28,7 @@ export async function GET() { const { data: cancellations } = await supabase .from('cancellation_requests') .select(` - id, scope, status, created_at, cancellation_type, occurrence_id, booking_id, + id, scope, status, created_at, cancellation_type, occurrence_id, occurrence_date, booking_id, bookings(id, type, purpose, bodies(name)), users(full_name) `) @@ -39,12 +44,20 @@ export async function GET() { if (c.scope === 'occurrence' && c.occurrence_id) { if (bookingType === 'Weekly Room') { - const { data: occ } = await adminSupabase + // Matched on the stored date first, and only then on the id. The id + // does not survive an edit to the booking -- the PATCH handler + // regenerates every occurrence -- which is why these rows showed no + // date at all (issue #96). + const q = adminSupabase .from('weekly_room_occurrences') - .select('occurrence_date, reservation_code') - .eq('id', c.occurrence_id) - .single() - occurrence_date = occ?.occurrence_date ?? null + .select('occurrence_date, reservation_code, weekly_room_bookings!inner(booking_id)') + const { data: occ } = c.occurrence_date + ? await q + .eq('occurrence_date', c.occurrence_date) + .eq('weekly_room_bookings.booking_id', c.booking_id) + .maybeSingle() + : await q.eq('id', c.occurrence_id).maybeSingle() + occurrence_date = occ?.occurrence_date ?? c.occurrence_date ?? null reservation_code = occ?.reservation_code ?? null } else if (bookingType === 'One-Time Room') { const { data: session } = await adminSupabase @@ -88,7 +101,10 @@ export async function GET() { } } - return { ...c, occurrence_date, reservation_code } + // The stored date is the fallback for every type: even where the code + // lookup fails because the row was regenerated, the request still knows + // which date it was about. + return { ...c, occurrence_date: occurrence_date ?? c.occurrence_date ?? null, reservation_code } }) ) @@ -107,6 +123,38 @@ export async function PATCH(request: Request) { if (rateLimitRes) return rateLimitRes const { id } = await request.json() + if (!id) return NextResponse.json({ error: 'A cancellation request id is required.' }, { status: 400 }) + + const { data: req } = await adminSupabase + .from('cancellation_requests') + .select('id') + .eq('id', id) + .maybeSingle() + + if (!req) return NextResponse.json({ error: 'Cancellation request not found.' }, { status: 404 }) + + // Which dated reservations this request covers, and what each is due. Read + // from the same collector Auto-Cancel uses, so the two agree about whose row a + // request owns -- including the inheritance that makes a series-level + // cancellation produce pending occurrences that do not say so themselves. + // + // `skipped` is included here where Auto-Cancel excludes it. A missing + // reservation code means CSC cannot be asked, which is why Auto-Cancel will + // not touch those rows; it says nothing about whether an administrator has + // dealt with it. Marking Done by hand is that administrator saying they have. + const { lines, skipped } = await collectPending() + const covered = [...lines, ...skipped].filter(l => l.cancellationRequestId === id) + + const failures = await applyCancellationOutcomes(covered) + + // One entry per booking touched, so the change shows up in the Audit tab + // beside every other status change rather than appearing to have happened by + // itself. Best effort, as in Auto-Cancel. + const auditRows = cancellationAuditRows(covered, user.id) + if (auditRows.length) { + const { error: auditError } = await adminSupabase.from('audit_logs').insert(auditRows) + if (auditError) console.error('Cancellation done audit log failed:', auditError) + } const { error } = await adminSupabase .from('cancellation_requests') @@ -115,5 +163,13 @@ export async function PATCH(request: Request) { if (error) return NextResponse.json({ error: error.message }, { status: 500 }) - return NextResponse.json({ success: true }) + return NextResponse.json({ + success: true, + applied: covered.length, + cancelled: covered.filter(l => l.resultingStatus === 'Cancelled').length, + virtual: covered.filter(l => l.resultingStatus === 'Virtual').length, + // The request is closed either way -- the admin has said it is handled -- but + // they need to know if a status did not move with it. + ...(failures.length ? { statusUpdateFailed: failures } : {}), + }) } \ No newline at end of file diff --git a/app/api/cancellation-requests/route.ts b/app/api/cancellation-requests/route.ts index 5878c43..f2605bd 100644 --- a/app/api/cancellation-requests/route.ts +++ b/app/api/cancellation-requests/route.ts @@ -28,12 +28,39 @@ export async function POST(request: Request) { const bookingType = guard.row.type + // The date this request is about, resolved now while occurrence_id still + // names a live row (issue #96). + // + // It will not always. The weekly PATCH handler deletes and reinserts every + // occurrence on each save, so an edit to the booking leaves occurrence_id + // pointing at nothing while the values live on under new ids, carried across + // on the date. Storing the date is what lets the request still be matched to + // its reservation afterwards -- without it, marking the request Done finds + // nothing to do, and Auto-Cancel cannot tell which request asked for what. + let occurrenceDate: string | null = null + if (scope === 'occurrence' && occurrence_id) { + if (bookingType === 'One-Time Room') { + const { data } = await adminSupabase + .from('one_time_room_bookings').select('booking_date').eq('id', occurrence_id).maybeSingle() + occurrenceDate = data?.booking_date ?? null + } else if (bookingType === 'Tabling') { + const { data } = await adminSupabase + .from('tabling_sessions').select('session_date').eq('id', occurrence_id).maybeSingle() + occurrenceDate = data?.session_date ?? null + } else { + const { data } = await adminSupabase + .from('weekly_room_occurrences').select('occurrence_date').eq('id', occurrence_id).maybeSingle() + occurrenceDate = data?.occurrence_date ?? null + } + } + // Create cancellation request const { error: requestError } = await adminSupabase .from('cancellation_requests') .insert({ booking_id, occurrence_id: occurrence_id || null, + occurrence_date: occurrenceDate, requested_by: user.id, scope, status: 'Pending', diff --git a/lib/pending-cancellations.ts b/lib/pending-cancellations.ts index 3dfd252..dd6c974 100644 --- a/lib/pending-cancellations.ts +++ b/lib/pending-cancellations.ts @@ -95,11 +95,19 @@ export function hasUsableCode(code: string | null | undefined): boolean { return typeof code === 'string' && code.trim().length > 0 } -export interface SkippedReservation { - date: string - roomOrTable: string - bodyName: string - bookingType: 'One-Time Room' | 'Weekly Room' | 'Tabling' +/** + * Everything a line carries except a code CSC could act on. + * + * It used to carry only the four display fields, because the only thing anyone + * did with a skipped row was print it in the Auto-Cancel preview. Marking a + * cancellation request Done has to *act* on these rows (issue #96): the admin + * saying they have handled it is not conditional on CSC having had a code to + * work from, so the row still needs its status applied and therefore still needs + * its id, its table and its outcome. + */ +export interface SkippedReservation extends Omit { + /** Always null. Its absence is what makes the row skipped. */ + reservationCode: null } /** @@ -191,20 +199,31 @@ export async function collectPending(): Promise { // request beats a resolved one, and the most recent beats an older one. const { data: requests } = await adminSupabase .from('cancellation_requests') - .select('id, booking_id, occurrence_id, scope, status, cancellation_type, created_at') + .select('id, booking_id, occurrence_id, occurrence_date, scope, status, cancellation_type, created_at') .order('created_at', { ascending: false }) // The id travels with the type: sending closes the request it acted on, so // knowing *which* row said 'Virtual' matters as much as the value. const byOccurrence = new Map() + // Keyed on (booking, date), which is what survives an edit. occurrence_id does + // not: the weekly PATCH handler regenerates every occurrence row on each save, + // so a request made before an edit points at nothing afterwards -- and every + // request in production was in exactly that state (issue #96). Falling through + // to bySeriesBooking would have been wrong, and falling through to nothing lost + // the cancellation_type, so a request that asked to go Virtual came out + // Cancelled. + const byBookingDate = new Map() const bySeriesBooking = new Map() const byBooking = new Map() + const dateKey = (bookingId: string, date: string) => `${bookingId}|${date}` + for (const pass of ['Pending', 'other'] as const) { for (const r of (requests ?? []) as { id: string booking_id: string | null occurrence_id: string | null + occurrence_date: string | null scope: string status: string | null cancellation_type: string @@ -217,6 +236,10 @@ export async function collectPending(): Promise { if (r.occurrence_id && !byOccurrence.has(r.occurrence_id)) { byOccurrence.set(r.occurrence_id, ref) } + if (r.booking_id && r.occurrence_date) { + const k = dateKey(r.booking_id, r.occurrence_date) + if (!byBookingDate.has(k)) byBookingDate.set(k, ref) + } if (r.booking_id) { if (r.scope === 'series' && !bySeriesBooking.has(r.booking_id)) { bySeriesBooking.set(r.booking_id, ref) @@ -233,19 +256,17 @@ export async function collectPending(): Promise { request: RequestRef | undefined, ) => { const outcome = outcomeOf(request?.type) - if (hasUsableCode(code)) lines.push({ + const resolved = { ...line, - reservationCode: code!.trim(), resultingStatus: outcome.status, outcomeFromRequest: outcome.fromRequest, cancellationRequestId: request?.id ?? null, - }) - else skipped.push({ - date: line.date, - roomOrTable: line.roomOrTable, - bodyName: line.bodyName, - bookingType: line.bookingType, - }) + } + // Built once and routed, rather than assembled differently on each branch. + // The two used to diverge, and a skipped row lost the id and table that + // marking a request Done now needs (issue #96). + if (hasUsableCode(code)) lines.push({ ...resolved, reservationCode: code!.trim() }) + else skipped.push({ ...resolved, reservationCode: null }) } { @@ -265,7 +286,7 @@ export async function collectPending(): Promise { roomOrTable: r.room_name || 'Not recorded', bodyName: bodyNameOf(r.bookings), bookingType: 'One-Time Room', - }, byBooking.get(r.booking_id)) + }, byBookingDate.get(dateKey(r.booking_id, r.booking_date)) ?? byBooking.get(r.booking_id)) } } @@ -291,7 +312,9 @@ export async function collectPending(): Promise { roomOrTable: r.location || 'Not recorded', bodyName: bodyNameOf(parent?.bookings ?? null), bookingType: 'Tabling', - }, parent?.booking_id ? byBooking.get(parent.booking_id) : undefined) + }, parent?.booking_id + ? byBookingDate.get(dateKey(parent.booking_id, r.session_date)) ?? byBooking.get(parent.booking_id) + : undefined) } } @@ -339,8 +362,14 @@ export async function collectPending(): Promise { bookingType: 'Weekly Room', }, // A request naming this exact week wins over one covering the series. + // The id is tried first because it is exact when it resolves; the + // (booking, date) key is what still works once the row has been + // regenerated, which is the usual case rather than the exception. byOccurrence.get(r.id) - ?? (series?.booking_id ? bySeriesBooking.get(series.booking_id) : undefined) + ?? (series?.booking_id + ? byBookingDate.get(dateKey(series.booking_id, r.occurrence_date)) + ?? bySeriesBooking.get(series.booking_id) + : undefined) ) } } @@ -353,3 +382,68 @@ export async function collectPending(): Promise { } } + +/** Which table each source's `id` belongs to. */ +const TABLE_OF: Record = { + one_time: 'one_time_room_bookings', + occurrence: 'weekly_room_occurrences', + tabling_session: 'tabling_sessions', +} + +/** The rows a cancellation outcome can be written to. */ +export type OutcomeTarget = Pick + +/** + * Writes each reservation the status it is due, and returns the batches that + * failed. + * + * Grouped by table *and* by status, so a batch containing both kinds writes each + * its own value. Marking everything 'Cancelled' would be wrong for a booking + * whose request said it was going virtual: that meeting still happens, it just + * does not need the room. + * + * An occurrence whose status was inherited gets its value written onto the + * occurrence itself, which is correct -- only the dates actually acted on stop + * being pending, and the rest of the series is untouched. + * + * Shared by Auto-Cancel and by marking a request Done by hand, which have to + * agree about this: the same request resolved either way should leave the + * database in the same state. + */ +export async function applyCancellationOutcomes(rows: OutcomeTarget[]): Promise { + const batches = new Map() + for (const l of rows) { + const table = TABLE_OF[l.source] + const bucket = `${table}:${l.resultingStatus}` + if (!batches.has(bucket)) batches.set(bucket, { table, status: l.resultingStatus, ids: [] }) + batches.get(bucket)!.ids.push(l.id) + } + + const failures: string[] = [] + for (const { table, status, ids } of batches.values()) { + const { error } = await adminSupabase.from(table).update({ status }).in('id', ids) + if (error) { + console.error(`Could not mark ${table} as ${status}:`, error) + failures.push(`${table} (${status})`) + } + } + return failures +} + +/** + * Audit rows for a set of reservations, one per booking and status. + * + * Keyed on booking *and* status: one booking can contribute both a cancelled + * week and a virtual one in the same action, and a single row saying 'Cancelled' + * would misreport the other. + */ +export function cancellationAuditRows(rows: OutcomeTarget[], adminId: string) { + return [...new Map( + rows + .filter(l => l.bookingId) + .map(l => [ + `${l.bookingId}:${l.resultingStatus}`, + { booking_id: l.bookingId, admin_id: adminId, new_status: l.resultingStatus }, + ]) + ).values()] +} diff --git a/supabase/migrations/20260909010000_cancellation_requests_occurrence_date.sql b/supabase/migrations/20260909010000_cancellation_requests_occurrence_date.sql new file mode 100644 index 0000000..b4baa7a --- /dev/null +++ b/supabase/migrations/20260909010000_cancellation_requests_occurrence_date.sql @@ -0,0 +1,60 @@ +-- Cancellation requests remember the date, not just the row id (issue #96). +-- +-- cancellation_requests.occurrence_id names the dated row a request is about. +-- For a weekly booking that row does not survive an edit: the weekly PATCH +-- handler deletes every occurrence and reinserts it on each save, so the ids +-- change while the values are carried across on the date. The request is left +-- pointing at nothing. +-- +-- Every cancellation_requests row in production is currently in that state -- +-- all four of them, including the one still Pending. The consequences are worse +-- than they look: +-- +-- * Marking a request Done could not find the reservation it was about, so it +-- closed the request and changed no booking status. That is the reported bug. +-- * Auto-Cancel finds those reservations anyway, because it scans by status +-- rather than by request -- but it cannot attribute them back to a request, +-- so it loses the cancellation_type and falls back to 'Cancelled'. A request +-- that asked to go Virtual would have cancelled the meeting outright. +-- +-- The date is the stable identifier the write model actually preserves, which is +-- the same conclusion 20260829001000_occurrence_events.sql reached for +-- event_tracking, and the same one issue #69 reached for calendar UIDs. This +-- follows it: occurrence_id stays for the rows that still resolve, and +-- occurrence_date becomes the key that survives a regeneration. + +alter table public.cancellation_requests + add column if not exists occurrence_date date; + +comment on column public.cancellation_requests.occurrence_date is + 'The date of the reservation this request is about, for occurrence-scoped requests. The stable key: occurrence_id does not survive an edit to a weekly booking, because the PATCH handler regenerates every occurrence row. NULL for series-scoped requests, and for occurrence-scoped ones created before this column existed whose row had already been regenerated.'; + +-- Backfill from occurrence_id wherever it still resolves. This sets nothing in +-- production, where every row has already been orphaned -- there is no record +-- anywhere of which date those requests named, so they cannot be recovered and +-- are deliberately left NULL rather than guessed at. It is written for the +-- environments where the rows are still intact, and so that applying this +-- migration to a restored backup does the right thing. + +update public.cancellation_requests r +set occurrence_date = o.occurrence_date +from public.weekly_room_occurrences o +where r.occurrence_id = o.id + and r.occurrence_date is null; + +update public.cancellation_requests r +set occurrence_date = s.booking_date +from public.one_time_room_bookings s +where r.occurrence_id = s.id + and r.occurrence_date is null; + +update public.cancellation_requests r +set occurrence_date = t.session_date +from public.tabling_sessions t +where r.occurrence_id = t.id + and r.occurrence_date is null; + +-- Pending requests are looked up by (booking_id, occurrence_date) on every +-- Auto-Cancel preview and every Cancellations tab load. +create index if not exists cancellation_requests_booking_date_idx + on public.cancellation_requests (booking_id, occurrence_date); diff --git a/supabase/migrations/rollback/20260909_cancellation_requests_occurrence_date_rollback.sql b/supabase/migrations/rollback/20260909_cancellation_requests_occurrence_date_rollback.sql new file mode 100644 index 0000000..db1c7e1 --- /dev/null +++ b/supabase/migrations/rollback/20260909_cancellation_requests_occurrence_date_rollback.sql @@ -0,0 +1,11 @@ +-- Rollback for 20260909010000_cancellation_requests_occurrence_date.sql. +-- +-- Drops the column, which returns cancellation requests to being identified only +-- by an occurrence_id that does not survive an edit to a weekly booking. Any +-- dates recorded since the migration are lost, so requests created in between +-- become orphaned in exactly the way the migration was written to stop. + +drop index if exists public.cancellation_requests_booking_date_idx; + +alter table public.cancellation_requests + drop column if exists occurrence_date;