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;