diff --git a/supabase/functions/on-booking-created/index.ts b/supabase/functions/on-booking-created/index.ts index 8632ae5..ef23563 100644 --- a/supabase/functions/on-booking-created/index.ts +++ b/supabase/functions/on-booking-created/index.ts @@ -28,12 +28,27 @@ Deno.serve(async (req) => { }) } - try { - const supabaseAdmin = createClient( - Deno.env.get('SUPABASE_URL')!, - Deno.env.get('SUPABASE_SERVICE_ROLE_KEY')!, - ) + const supabaseAdmin = createClient( + Deno.env.get('SUPABASE_URL')!, + Deno.env.get('SUPABASE_SERVICE_ROLE_KEY')!, + ) + // Best-effort: records a failure so it's visible somewhere a human could + // plausibly check (e.g. a future admin panel view), rather than only in + // Edge Function logs nobody reads. Swallow any error from this insert + // itself -- we're already in a failure path and must not let logging the + // failure become a second, louder failure. + const recordFailure = async (message: string) => { + try { + await supabaseAdmin + .from('notification_failures') + .insert({ kind: 'booking', record_id: booking.id, error_message: message }) + } catch (err) { + console.error('on-booking-created: failed to record notification_failures row', err instanceof Error ? err.message : err) + } + } + + try { const { data: service } = await supabaseAdmin .from('services') .select('name') @@ -54,7 +69,9 @@ Deno.serve(async (req) => { const resendApiKey = Deno.env.get('RESEND_API_KEY') if (!resendApiKey) { - console.error(`on-booking-created: RESEND_API_KEY is not set, skipping email for service ${booking.service_id}`) + const message = 'RESEND_API_KEY is not set' + console.error(`on-booking-created: ${message}, skipping email for service ${booking.service_id}`) + await recordFailure(message) } else { const resendRes = await fetch('https://api.resend.com/emails', { method: 'POST', @@ -66,23 +83,28 @@ Deno.serve(async (req) => { }) if (!resendRes.ok) { + const body = await resendRes.text() console.error( `on-booking-created: Resend send failed for service ${booking.service_id} (status ${resendRes.status})`, - await resendRes.text(), + body, ) + await recordFailure(`Resend responded ${resendRes.status}: ${body}`) } } } catch (err) { // The booking itself already succeeded (this handler only runs after // the row was inserted) -- a failure here means the notification email - // didn't go out, not that the booking failed. Log for debugging and - // still return 200: a non-200 would make pg_net treat this as a - // retry-worthy failure, but retrying re-sends against the same booking - // row and won't fix a Resend outage or bad payload. - console.error( - `on-booking-created: unexpected error for service ${booking?.service_id}`, - err instanceof Error ? err.message : err, - ) + // didn't go out, not that the booking failed. We still return 200: + // pg_net is fire-and-forget (it doesn't inspect the response status or + // retry on non-2xx -- the response just lands in net._http_response), + // so returning 500 wouldn't trigger a retry or help anyone; nothing + // consumes this status code. What it *would* do is misrepresent the + // request as failed when the booking itself succeeded. The actual gap + // this creates -- a failed send being invisible -- is what + // notification_failures (recorded above/below) exists to close. + const message = err instanceof Error ? err.message : String(err) + console.error(`on-booking-created: unexpected error for service ${booking?.service_id}`, message) + await recordFailure(message) } return new Response(JSON.stringify({ ok: true }), { headers: { 'Content-Type': 'application/json' } }) diff --git a/supabase/functions/on-inquiry-created/index.ts b/supabase/functions/on-inquiry-created/index.ts index 8075a66..20a3fd7 100644 --- a/supabase/functions/on-inquiry-created/index.ts +++ b/supabase/functions/on-inquiry-created/index.ts @@ -28,12 +28,27 @@ Deno.serve(async (req) => { }) } - try { - const supabaseAdmin = createClient( - Deno.env.get('SUPABASE_URL')!, - Deno.env.get('SUPABASE_SERVICE_ROLE_KEY')!, - ) + const supabaseAdmin = createClient( + Deno.env.get('SUPABASE_URL')!, + Deno.env.get('SUPABASE_SERVICE_ROLE_KEY')!, + ) + // Best-effort: records a failure so it's visible somewhere a human could + // plausibly check (e.g. a future admin panel view), rather than only in + // Edge Function logs nobody reads. Swallow any error from this insert + // itself -- we're already in a failure path and must not let logging the + // failure become a second, louder failure. + const recordFailure = async (message: string) => { + try { + await supabaseAdmin + .from('notification_failures') + .insert({ kind: 'inquiry', record_id: inquiry.id, error_message: message }) + } catch (err) { + console.error('on-inquiry-created: failed to record notification_failures row', err instanceof Error ? err.message : err) + } + } + + try { const { data: product } = await supabaseAdmin .from('products') .select('name') @@ -47,7 +62,9 @@ Deno.serve(async (req) => { const resendApiKey = Deno.env.get('RESEND_API_KEY') if (!resendApiKey) { - console.error(`on-inquiry-created: RESEND_API_KEY is not set, skipping email for product ${inquiry.product_id}`) + const message = 'RESEND_API_KEY is not set' + console.error(`on-inquiry-created: ${message}, skipping email for product ${inquiry.product_id}`) + await recordFailure(message) } else { const resendRes = await fetch('https://api.resend.com/emails', { method: 'POST', @@ -59,23 +76,28 @@ Deno.serve(async (req) => { }) if (!resendRes.ok) { + const body = await resendRes.text() console.error( `on-inquiry-created: Resend send failed for product ${inquiry.product_id} (status ${resendRes.status})`, - await resendRes.text(), + body, ) + await recordFailure(`Resend responded ${resendRes.status}: ${body}`) } } } catch (err) { // The inquiry itself already succeeded (this handler only runs after // the row was inserted) -- a failure here means the notification email - // didn't go out, not that the inquiry failed. Log for debugging and - // still return 200: a non-200 would make pg_net treat this as a - // retry-worthy failure, but retrying re-sends against the same inquiry - // row and won't fix a Resend outage or bad payload. - console.error( - `on-inquiry-created: unexpected error for product ${inquiry?.product_id}`, - err instanceof Error ? err.message : err, - ) + // didn't go out, not that the inquiry failed. We still return 200: + // pg_net is fire-and-forget (it doesn't inspect the response status or + // retry on non-2xx -- the response just lands in net._http_response), + // so returning 500 wouldn't trigger a retry or help anyone; nothing + // consumes this status code. What it *would* do is misrepresent the + // request as failed when the inquiry itself succeeded. The actual gap + // this creates -- a failed send being invisible -- is what + // notification_failures (recorded above/below) exists to close. + const message = err instanceof Error ? err.message : String(err) + console.error(`on-inquiry-created: unexpected error for product ${inquiry?.product_id}`, message) + await recordFailure(message) } return new Response(JSON.stringify({ ok: true }), { headers: { 'Content-Type': 'application/json' } }) diff --git a/supabase/migrations/20260812000002_bookings_slot_uniqueness.sql b/supabase/migrations/20260812000002_bookings_slot_uniqueness.sql index 885ccf0..475e9e6 100644 --- a/supabase/migrations/20260812000002_bookings_slot_uniqueness.sql +++ b/supabase/migrations/20260812000002_bookings_slot_uniqueness.sql @@ -11,6 +11,54 @@ -- 'cancelled': a cancelled booking must not permanently block the slot it -- once held, and multiple cancelled bookings may legitimately share a -- (date, time) (e.g. several people asked for the same slot, all declined). +-- +-- SELF-HEALING: on `supabase db push` against the live project, this +-- `create unique index` could fail outright if any existing confirmed +-- booking already shares a (date, time) with another -- exactly the bug +-- being fixed here, so entirely plausible in already-live data. Rather than +-- have the migration fail and block deploy, dedupe_confirmed_booking_slots() +-- below collapses each duplicate group down to the earliest-created row +-- (keeping it 'confirmed') and cancels the rest before the index is +-- created, so index creation is guaranteed to succeed either way. It's +-- defined as a standalone function (called once below, but also directly +-- callable) so the pgTAP test can exercise the exact same logic the +-- migration runs, rather than a copy that could drift from it. +create or replace function public.dedupe_confirmed_booking_slots() +returns void +language plpgsql +security definer +set search_path = '' +as $$ +declare + dup record; +begin + for dup in + select date, time, count(*) as cnt + from public.bookings + where status = 'confirmed' + group by date, time + having count(*) > 1 + loop + update public.bookings + set status = 'cancelled' + where date = dup.date and time = dup.time and status = 'confirmed' + and id <> ( + select id from public.bookings + where date = dup.date and time = dup.time and status = 'confirmed' + order by created_at asc, id asc + limit 1 + ); + raise notice 'dedupe_confirmed_booking_slots: cancelled % duplicate confirmed booking(s) for slot % %, keeping the earliest-created one', + dup.cnt - 1, dup.date, dup.time; + end loop; +end; +$$; + +revoke execute on function public.dedupe_confirmed_booking_slots() from public, anon, authenticated; +grant execute on function public.dedupe_confirmed_booking_slots() to service_role; + +select public.dedupe_confirmed_booking_slots(); + create unique index bookings_active_slot_idx on public.bookings (date, time) where status = 'confirmed'; diff --git a/supabase/migrations/20260812000003_booked_slots_cleanup.sql b/supabase/migrations/20260812000003_booked_slots_cleanup.sql index 4c4d0ae..6aafda7 100644 --- a/supabase/migrations/20260812000003_booked_slots_cleanup.sql +++ b/supabase/migrations/20260812000003_booked_slots_cleanup.sql @@ -4,7 +4,7 @@ -- already-applied 20260808000002 file. -- -- Problem: sync_booked_slot() only ran `after insert`, and unconditionally --- mirrored every new booking regardless of status. Two consequences: +-- mirrored every new booking regardless of status. Three consequences: -- 1. If a confirmed booking was later cancelled (status update) or -- deleted, its booked_slots mirror row was never removed, so the slot -- stayed permanently blocked even though it was actually free again. @@ -12,14 +12,22 @@ -- insert policy allows any status, not just the 'confirmed' default) -- would still create a booked_slots row, incorrectly blocking a slot -- nobody had actually confirmed. +-- 3. Rescheduling a confirmed booking (changing date/time while it stays +-- 'confirmed') didn't fire the trigger at all under an `of status` +-- qualifier, leaving the OLD slot permanently blocked and the NEW slot +-- showing as free even though it's now taken. Caught in review: a +-- status-qualified trigger silently misses any update that doesn't +-- touch the status column. -- --- Fix: the trigger function now branches on TG_OP. On insert/update it only --- mirrors the slot when the new row is 'confirmed' (fixing #2 above). On --- update-away-from-confirmed or delete, it removes the mirror row, but only --- if no *other* confirmed booking still holds that (date, time) -- the --- partial unique index added in 20260812000002 guarantees there can be at --- most one, but this guard keeps the function correct even if that --- assumption ever changes. +-- Fix: the trigger function now branches on TG_OP and the trigger fires on +-- every UPDATE (not just `of status`). On insert/update it only mirrors the +-- slot when the new row is 'confirmed' (fixing #2 above). On update where +-- the (date, time, status) triple actually changed and the OLD row was +-- confirmed, or on delete of a confirmed row, it removes the OLD slot's +-- mirror row, but only if no *other* confirmed booking still holds that +-- (date, time) -- the partial unique index added in 20260812000002 +-- guarantees there can be at most one, but this guard keeps the function +-- correct even if that assumption ever changes. create or replace function public.sync_booked_slot() returns trigger language plpgsql @@ -40,7 +48,12 @@ begin end if; if tg_op = 'UPDATE' then - if old.status = 'confirmed' and new.status <> 'confirmed' then + -- Covers both a status change away from 'confirmed' (cancellation) and a + -- reschedule of a still-confirmed booking to a different (date, time): + -- either way, the OLD slot may no longer have any confirmed booking + -- holding it and its mirror row needs to go. + if old.status = 'confirmed' + and (old.date, old.time, old.status) is distinct from (new.date, new.time, new.status) then delete from public.booked_slots where date = old.date and time = old.time and not exists ( @@ -66,8 +79,12 @@ begin end; $$; -create trigger booking_status_updated_sync_slot - after update of status on public.bookings +-- Unqualified `after update` (not `of status`): a reschedule that changes +-- date/time while status stays 'confirmed' must also resync the mirror -- +-- the old slot needs freeing and the new slot needs blocking -- and an +-- `of status` trigger would never fire for that case. +create trigger booking_updated_sync_slot + after update on public.bookings for each row execute function public.sync_booked_slot(); diff --git a/supabase/migrations/20260812000004_notification_failures.sql b/supabase/migrations/20260812000004_notification_failures.sql new file mode 100644 index 0000000..c6b5f84 --- /dev/null +++ b/supabase/migrations/20260812000004_notification_failures.sql @@ -0,0 +1,31 @@ +-- Follow-up to the final review of this fast-follow-up branch. The Edge +-- Functions (on-booking-created, on-inquiry-created) already log a failed +-- Resend send via console.error, but Edge Function logs are not something +-- Mira (the salon owner) would ever look at -- a failed notification email +-- was effectively invisible. This table gives failures a place a human +-- could plausibly check (e.g. surfaced later in an admin panel), without +-- making the booking/inquiry request itself fail or retry (see the +-- reasoning in the Edge Functions' try/catch blocks). +create table public.notification_failures ( + id uuid primary key default gen_random_uuid(), + kind text not null check (kind in ('booking', 'inquiry')), + record_id uuid not null, + error_message text not null, + created_at timestamptz not null default now() +); + +alter table public.notification_failures enable row level security; + +-- Admin-only select; no insert/update/delete policy exists for anyone -- +-- only the Edge Functions' service-role client writes here, and +-- service_role bypasses RLS entirely (same pattern as booked_slots' +-- security-definer-trigger-only write path). +create policy "notification_failures_admin_read" on public.notification_failures + for select using ((select auth.uid()) = '21c90224-e0b2-412e-a431-8e7f097c1cef'::uuid); + +-- No grant to anon at all (these rows can reference booking/inquiry PII via +-- error_message, same protection level as bookings/inquiries themselves). +-- authenticated gets select (narrowed to the admin UID by the RLS policy +-- above); only service_role can write. +grant select on public.notification_failures to authenticated; +grant select, insert, update, delete on public.notification_failures to service_role; diff --git a/supabase/tests/database/rls.test.sql b/supabase/tests/database/rls.test.sql index 5d83f96..186ea16 100644 --- a/supabase/tests/database/rls.test.sql +++ b/supabase/tests/database/rls.test.sql @@ -1,5 +1,5 @@ begin; -select plan(23); +select plan(37); -- Seed reference data as service_role (bypasses RLS) set local role service_role; @@ -171,5 +171,112 @@ select is_empty( 'booked_slots mirror row is removed after the booking is deleted' ); +-- Rescheduling: an update that changes date/time while status stays +-- 'confirmed' must free the OLD slot's mirror row and create one for the +-- NEW slot. A trigger scoped to `after update of status` would never fire +-- for this (status doesn't change), which was the bug caught in review. +select lives_ok( + $$ insert into public.bookings (service_id, date, time, contact_name, contact_phone, contact_email, contact_notes) + values ('knotless', '2026-10-03', '11:00 AM', 'F Client', '555-0106', 'f@example.com', '') $$, + 'confirmed booking for the reschedule test succeeds' +); + +select lives_ok( + $$ update public.bookings set date = '2026-10-04', time = '1:00 PM' + where date = '2026-10-03' and time = '11:00 AM' and status = 'confirmed' $$, + 'rescheduling the confirmed booking to a new date/time succeeds' +); + +select is_empty( + $$ select * from public.booked_slots where date = '2026-10-03' and time = '11:00 AM' $$, + 'booked_slots mirror row for the OLD slot is removed after a reschedule' +); + +select isnt_empty( + $$ select * from public.booked_slots where date = '2026-10-04' and time = '1:00 PM' $$, + 'booked_slots mirror row for the NEW slot is created after a reschedule' +); + +-- notification_failures: admin-only select; only service_role writes. +select lives_ok( + $$ insert into public.notification_failures (kind, record_id, error_message) + values ('booking', gen_random_uuid(), 'Resend responded 401: unauthorized') $$, + 'service_role can insert into notification_failures' +); + +set local role anon; +select throws_ok( + $$ select * from public.notification_failures $$, + '42501', + null, + 'anon cannot read notification_failures' +); + +select throws_ok( + $$ insert into public.notification_failures (kind, record_id, error_message) + values ('booking', gen_random_uuid(), 'hack') $$, + '42501', + null, + 'anon cannot insert into notification_failures' +); + +set local role authenticated; +set local request.jwt.claim.sub = '21c90224-e0b2-412e-a431-8e7f097c1cef'; +select isnt_empty( + $$ select * from public.notification_failures $$, + 'admin can read notification_failures' +); + +-- Self-healing unique index migration: replay the exact dedupe logic +-- 20260812000002 runs before creating bookings_active_slot_idx, proving +-- that pre-existing duplicate confirmed bookings for a slot get collapsed +-- to a single confirmed row (the earliest-created one) rather than making +-- the index creation fail outright. +-- DDL (drop/create index) requires table ownership, which service_role +-- doesn't have -- reset to the connecting (owner) role for this block. The +-- dedupe_confirmed_booking_slots() call further down still exercises the +-- service_role grant path via an explicit role switch. +reset role; + +select lives_ok( + $$ drop index if exists public.bookings_active_slot_idx $$, + 'temporarily dropping the unique index to simulate pre-migration data succeeds' +); + +select lives_ok( + $$ insert into public.bookings (service_id, date, time, contact_name, contact_phone, contact_email, contact_notes, status, created_at) + values + ('knotless', '2026-11-01', '4:00 PM', 'Dup Earliest', '555-0201', 'dup1@example.com', '', 'confirmed', now() - interval '1 hour'), + ('knotless', '2026-11-01', '4:00 PM', 'Dup Later', '555-0202', 'dup2@example.com', '', 'confirmed', now()) $$, + 'seeding two duplicate confirmed bookings for the same slot succeeds without the unique index' +); + +-- Exercises the same execute grant the migration relies on: only +-- service_role can call this function (revoked from anon/authenticated). +set local role service_role; +select lives_ok( + $$ select public.dedupe_confirmed_booking_slots() $$, + 'dedupe_confirmed_booking_slots() runs without error' +); + +select is( + (select count(*)::int from public.bookings where date = '2026-11-01' and time = '4:00 PM' and status = 'confirmed'), + 1, + 'the self-heal dedupe leaves exactly one confirmed booking per duplicated slot' +); + +select results_eq( + $$ select contact_name from public.bookings where date = '2026-11-01' and time = '4:00 PM' and status = 'confirmed' $$, + $$ values ('Dup Earliest'::text) $$, + 'the self-heal dedupe keeps the earliest-created booking as confirmed' +); + +-- Back to the owner role for the final DDL step. +reset role; +select lives_ok( + $$ create unique index bookings_active_slot_idx on public.bookings (date, time) where status = 'confirmed' $$, + 'the unique index can be created after the self-heal dedupe removes the duplicate' +); + select * from finish(); rollback;