diff --git a/src/components/ProtectedRoute.test.tsx b/src/components/ProtectedRoute.test.tsx index 19bb17b..27e2313 100644 --- a/src/components/ProtectedRoute.test.tsx +++ b/src/components/ProtectedRoute.test.tsx @@ -34,9 +34,19 @@ describe('ProtectedRoute', () => { expect(screen.getByText('Login page')).toBeInTheDocument() }) - it('renders the protected content when there is a session', () => { - adminAuthMock.mockReturnValue({ session: { user: { id: 'admin' } }, loading: false }) + it('renders the protected content when there is an admin session', () => { + adminAuthMock.mockReturnValue({ + session: { user: { id: '21c90224-e0b2-412e-a431-8e7f097c1cef' } }, + loading: false, + }) renderWithRoute() expect(screen.getByText('Products page')).toBeInTheDocument() }) + + it('redirects to /admin/login when the session belongs to a non-admin user', () => { + adminAuthMock.mockReturnValue({ session: { user: { id: 'some-other-user-id' } }, loading: false }) + renderWithRoute() + expect(screen.getByText('Login page')).toBeInTheDocument() + expect(screen.queryByText('Products page')).not.toBeInTheDocument() + }) }) diff --git a/src/components/ProtectedRoute.tsx b/src/components/ProtectedRoute.tsx index aef24ab..62a53d5 100644 --- a/src/components/ProtectedRoute.tsx +++ b/src/components/ProtectedRoute.tsx @@ -1,11 +1,20 @@ import { Navigate, Outlet } from 'react-router-dom' import useAdminAuth from '../hooks/useAdminAuth' +// The single admin account's UUID, matched throughout this project's RLS +// policies. Public signup is disabled (see supabase/config.toml), and RLS +// blocks every write from a non-admin session regardless, but this check is +// defense-in-depth: it ensures the admin UI itself never renders for an +// authenticated session that isn't the admin's, even if signup were ever +// re-enabled or a session existed for some other reason. +const ADMIN_USER_ID = '21c90224-e0b2-412e-a431-8e7f097c1cef' + function ProtectedRoute() { const { session, loading } = useAdminAuth() if (loading) return null if (!session) return + if (session.user.id !== ADMIN_USER_ID) return return } diff --git a/src/lib/friendlyError.test.ts b/src/lib/friendlyError.test.ts new file mode 100644 index 0000000..fa0f368 --- /dev/null +++ b/src/lib/friendlyError.test.ts @@ -0,0 +1,19 @@ +import { describe, it, expect } from 'vitest' +import { friendlyError } from './friendlyError' + +describe('friendlyError', () => { + it('maps a foreign-key violation (23503) to a friendly message', () => { + expect( + friendlyError({ + code: '23503', + message: + 'update or delete on table "products" violates foreign key constraint "inquiries_product_id_fkey" on table "inquiries"', + }) + ).toBe("This can't be deleted because it's referenced by an existing booking or inquiry.") + }) + + it('falls back to the raw message for any other error', () => { + expect(friendlyError({ code: '23505', message: 'duplicate key value' })).toBe('duplicate key value') + expect(friendlyError({ message: 'network error' })).toBe('network error') + }) +}) diff --git a/src/lib/friendlyError.ts b/src/lib/friendlyError.ts new file mode 100644 index 0000000..5d7adda --- /dev/null +++ b/src/lib/friendlyError.ts @@ -0,0 +1,14 @@ +// Postgres error code for a foreign-key violation. bookings.service_id and +// inquiries.product_id reference products/services with no cascade, so +// deleting a referenced row fails with a raw, unfriendly Postgres message +// (e.g. `update or delete on table "products" violates foreign key +// constraint "inquiries_product_id_fkey"...`). This maps that case (and any +// other error) to admin-friendly text. +const FOREIGN_KEY_VIOLATION = '23503' + +export function friendlyError(error: { code?: string; message: string }): string { + if (error.code === FOREIGN_KEY_VIOLATION) { + return "This can't be deleted because it's referenced by an existing booking or inquiry." + } + return error.message +} diff --git a/src/lib/storage.test.ts b/src/lib/storage.test.ts index 2690ac8..5d47abf 100644 --- a/src/lib/storage.test.ts +++ b/src/lib/storage.test.ts @@ -20,16 +20,16 @@ describe('uploadSalonImage', () => { fromMock.mockClear() }) - it('uploads to the salon-images bucket and returns the public URL', async () => { + it('uploads to the salon-images bucket and returns the storage-relative path', async () => { uploadMock.mockResolvedValue({ error: null }) - getPublicUrlMock.mockReturnValue({ data: { publicUrl: 'https://example.supabase.co/storage/v1/object/public/salon-images/products/edge-control.jpg' } }) const file = new File(['fake'], 'edge-control.jpg', { type: 'image/jpeg' }) - const url = await uploadSalonImage(file, 'products/edge-control.jpg') + const path = await uploadSalonImage(file, 'products/edge-control.jpg') expect(fromMock).toHaveBeenCalledWith('salon-images') expect(uploadMock).toHaveBeenCalledWith('products/edge-control.jpg', file, { upsert: true }) - expect(url).toBe('https://example.supabase.co/storage/v1/object/public/salon-images/products/edge-control.jpg') + expect(getPublicUrlMock).not.toHaveBeenCalled() + expect(path).toBe('products/edge-control.jpg') }) it('throws when the upload fails', async () => { diff --git a/src/lib/storage.ts b/src/lib/storage.ts index 7940302..04894aa 100644 --- a/src/lib/storage.ts +++ b/src/lib/storage.ts @@ -1,8 +1,12 @@ import { supabase } from './supabase' +// Returns the storage-relative path (not a public URL) so products.image / +// services.image always hold the same format regardless of whether the row +// was seeded (scripts/seed.ts stores relative paths) or uploaded through the +// admin panel. Callers that need to render the image should derive the +// public URL from this path at render time. export async function uploadSalonImage(file: File, path: string): Promise { const { error } = await supabase.storage.from('salon-images').upload(path, file, { upsert: true }) if (error) throw error - const { data } = supabase.storage.from('salon-images').getPublicUrl(path) - return data.publicUrl + return path } diff --git a/src/pages/admin/AdminApp.test.tsx b/src/pages/admin/AdminApp.test.tsx index 2642705..ed09032 100644 --- a/src/pages/admin/AdminApp.test.tsx +++ b/src/pages/admin/AdminApp.test.tsx @@ -3,7 +3,7 @@ import { MemoryRouter, Routes, Route } from 'react-router-dom' import { describe, it, expect, vi } from 'vitest' vi.mock('../../hooks/useAdminAuth', () => ({ - default: () => ({ session: { user: { id: 'admin' } }, loading: false }), + default: () => ({ session: { user: { id: '21c90224-e0b2-412e-a431-8e7f097c1cef' } }, loading: false }), })) vi.mock('../../lib/supabase', () => ({ supabase: { @@ -39,4 +39,26 @@ describe('AdminApp routing', () => { expect(await screen.findByRole('heading', { name: /^products$/i })).toBeInTheDocument() expect(screen.getByRole('link', { name: /bookings/i })).toBeInTheDocument() }) + + it('redirects /admin to /admin/products when authenticated', async () => { + render( + + + } /> + + , + ) + expect(await screen.findByRole('heading', { name: /^products$/i })).toBeInTheDocument() + }) + + it('redirects an unmatched /admin/* path to /admin/products when authenticated', async () => { + render( + + + } /> + + , + ) + expect(await screen.findByRole('heading', { name: /^products$/i })).toBeInTheDocument() + }) }) diff --git a/src/pages/admin/AdminApp.tsx b/src/pages/admin/AdminApp.tsx index 2aadf97..049fbb5 100644 --- a/src/pages/admin/AdminApp.tsx +++ b/src/pages/admin/AdminApp.tsx @@ -1,4 +1,4 @@ -import { Routes, Route } from 'react-router-dom' +import { Routes, Route, Navigate } from 'react-router-dom' import ProtectedRoute from '../../components/ProtectedRoute' import AdminLogin from './AdminLogin' import AdminLayout from './AdminLayout' @@ -14,11 +14,13 @@ function AdminApp() { } /> }> }> + } /> } /> } /> } /> } /> } /> + } /> diff --git a/src/pages/admin/AdminBookings.test.tsx b/src/pages/admin/AdminBookings.test.tsx index c39b038..14f8fc8 100644 --- a/src/pages/admin/AdminBookings.test.tsx +++ b/src/pages/admin/AdminBookings.test.tsx @@ -33,6 +33,7 @@ describe('AdminBookings', () => { eqUpdateMock.mockReset().mockResolvedValue({ error: null }) updateMock.mockReset().mockReturnValue({ eq: eqUpdateMock }) fromMock.mockReset().mockReturnValue({ select: selectMock, update: updateMock }) + vi.spyOn(window, 'confirm').mockReturnValue(true) }) it('lists bookings with the related service name', async () => { @@ -42,22 +43,46 @@ describe('AdminBookings', () => { expect(fromMock).toHaveBeenCalledWith('bookings') }) - it('cancels a booking', async () => { + it('cancels a booking after confirmation', async () => { render() await screen.findByText('Jane Doe') await userEvent.click(screen.getByRole('button', { name: /cancel/i })) + expect(window.confirm).toHaveBeenCalled() await waitFor(() => expect(updateMock).toHaveBeenCalledWith({ status: 'cancelled' })) expect(eqUpdateMock).toHaveBeenCalledWith('id', 'booking-1') }) - it('marks a booking completed', async () => { + it('does not cancel a booking when the confirmation is dismissed', async () => { + vi.spyOn(window, 'confirm').mockReturnValue(false) + render() + await screen.findByText('Jane Doe') + + await userEvent.click(screen.getByRole('button', { name: /cancel/i })) + + expect(window.confirm).toHaveBeenCalled() + expect(updateMock).not.toHaveBeenCalled() + }) + + it('marks a booking completed after confirmation', async () => { render() await screen.findByText('Jane Doe') await userEvent.click(screen.getByRole('button', { name: /mark completed/i })) + expect(window.confirm).toHaveBeenCalled() await waitFor(() => expect(updateMock).toHaveBeenCalledWith({ status: 'completed' })) }) + + it('does not mark a booking completed when the confirmation is dismissed', async () => { + vi.spyOn(window, 'confirm').mockReturnValue(false) + render() + await screen.findByText('Jane Doe') + + await userEvent.click(screen.getByRole('button', { name: /mark completed/i })) + + expect(window.confirm).toHaveBeenCalled() + expect(updateMock).not.toHaveBeenCalled() + }) }) diff --git a/src/pages/admin/AdminBookings.tsx b/src/pages/admin/AdminBookings.tsx index 3e1e4be..c140306 100644 --- a/src/pages/admin/AdminBookings.tsx +++ b/src/pages/admin/AdminBookings.tsx @@ -85,8 +85,26 @@ function AdminBookings() { {booking.status === 'confirmed' && ( <> - setStatus(booking.id, 'cancelled')}>Cancel - setStatus(booking.id, 'completed')}>Mark completed + { + if (window.confirm('Cancel this booking? This will free up its slot for other bookings.')) { + void setStatus(booking.id, 'cancelled') + } + }} + > + Cancel + + { + if (window.confirm('Mark this booking completed?')) { + void setStatus(booking.id, 'completed') + } + }} + > + Mark completed + > )} diff --git a/src/pages/admin/AdminProducts.tsx b/src/pages/admin/AdminProducts.tsx index 826f454..4969962 100644 --- a/src/pages/admin/AdminProducts.tsx +++ b/src/pages/admin/AdminProducts.tsx @@ -1,6 +1,7 @@ import { useEffect, useState, type FormEvent } from 'react' import { supabase } from '../../lib/supabase' import { uploadSalonImage } from '../../lib/storage' +import { friendlyError } from '../../lib/friendlyError' import styles from './AdminProducts.module.css' interface AdminProduct { @@ -78,12 +79,14 @@ function AdminProducts() { if (!window.confirm('Delete this product?')) return const { error: deleteError } = await supabase.from('products').delete().eq('id', id) if (deleteError) { - setError(deleteError.message) + setError(friendlyError(deleteError)) return } await loadProducts() } + const isEditing = Boolean(form.id) && products.some((p) => p.id === form.id) + return ( Products @@ -114,7 +117,7 @@ function AdminProducts() { - {form.id && products.some((p) => p.id === form.id) ? 'Edit product' : 'New product'} + {isEditing ? 'Edit product' : 'New product'} Id setForm((f) => ({ ...f, id: e.target.value }))} required + disabled={isEditing} /> diff --git a/src/pages/admin/AdminServices.tsx b/src/pages/admin/AdminServices.tsx index 1add643..940d784 100644 --- a/src/pages/admin/AdminServices.tsx +++ b/src/pages/admin/AdminServices.tsx @@ -1,6 +1,7 @@ import { useEffect, useState, type FormEvent } from 'react' import { supabase } from '../../lib/supabase' import { uploadSalonImage } from '../../lib/storage' +import { friendlyError } from '../../lib/friendlyError' import styles from './AdminServices.module.css' interface AdminService { @@ -75,12 +76,14 @@ function AdminServices() { if (!window.confirm('Delete this service?')) return const { error: deleteError } = await supabase.from('services').delete().eq('id', id) if (deleteError) { - setError(deleteError.message) + setError(friendlyError(deleteError)) return } await loadServices() } + const isEditing = Boolean(form.id) && servicesList.some((s) => s.id === form.id) + return ( Services @@ -109,7 +112,7 @@ function AdminServices() { - {form.id && servicesList.some((s) => s.id === form.id) ? 'Edit service' : 'New service'} + {isEditing ? 'Edit service' : 'New service'} Id setForm((f) => ({ ...f, id: e.target.value }))} required + disabled={isEditing} /> diff --git a/supabase/config.toml b/supabase/config.toml index de7965a..059099e 100644 --- a/supabase/config.toml +++ b/supabase/config.toml @@ -173,7 +173,7 @@ enable_refresh_token_rotation = true # Requires enable_refresh_token_rotation = true. refresh_token_reuse_interval = 10 # Allow/disallow new user signups to your project. -enable_signup = true +enable_signup = false # Allow/disallow anonymous sign-ins to your project. enable_anonymous_sign_ins = false # Allow/disallow testing manual linking of accounts @@ -218,7 +218,7 @@ web3 = 30 [auth.email] # Allow/disallow new user signups via email to your project. -enable_signup = true +enable_signup = false # If enabled, a user will be required to confirm any email change on both the old, and new email # addresses. If disabled, only the new email is required to confirm. double_confirm_changes = true diff --git a/supabase/migrations/20260812110508_completed_bookings_hold_slot.sql b/supabase/migrations/20260812110508_completed_bookings_hold_slot.sql new file mode 100644 index 0000000..1e6f350 --- /dev/null +++ b/supabase/migrations/20260812110508_completed_bookings_hold_slot.sql @@ -0,0 +1,75 @@ +-- Follow-up to 20260812105003 (deferred from the final whole-branch review +-- before merge). Postgres migrations are append-only history, so this +-- replaces the trigger function and the partial unique index rather than +-- editing the already-applied 20260812000002/20260812000003 files. +-- +-- Problem: 20260812105003 widened bookings.status to allow 'completed', and +-- the admin panel gained a "Mark completed" action, but sync_booked_slot() +-- (20260812000003) and bookings_active_slot_idx (20260812000002) still only +-- treat status = 'confirmed' as "slot occupied". As a result: +-- 1. confirmed -> completed deletes the booked_slots mirror row for that +-- slot, exactly as if the booking had been cancelled. +-- 2. The partial unique index (where status = 'confirmed') no longer +-- blocks a second confirmed booking for a now-completed slot. +-- A completed appointment did happen and must continue to occupy its slot; +-- only 'cancelled' should free it. +-- +-- Fix: every place sync_booked_slot() tested status = 'confirmed' now tests +-- status in ('confirmed', 'completed'), and the partial unique index is +-- rebuilt with the same predicate. +create or replace function public.sync_booked_slot() +returns trigger +language plpgsql +security definer +set search_path = '' +as $$ +begin + if tg_op = 'DELETE' then + if old.status in ('confirmed', 'completed') then + delete from public.booked_slots + where date = old.date and time = old.time + and not exists ( + select 1 from public.bookings b + where b.date = old.date and b.time = old.time and b.status in ('confirmed', 'completed') + ); + end if; + return old; + end if; + + if tg_op = 'UPDATE' then + -- Covers both a status change away from 'confirmed'/'completed' + -- (cancellation) and a reschedule of a still-occupying booking to a + -- different (date, time): either way, the OLD slot may no longer have + -- any confirmed/completed booking holding it and its mirror row needs + -- to go. + if old.status in ('confirmed', 'completed') + 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 ( + select 1 from public.bookings b + where b.date = old.date and b.time = old.time and b.status in ('confirmed', 'completed') + ); + end if; + if new.status in ('confirmed', 'completed') then + insert into public.booked_slots (date, time) + values (new.date, new.time) + on conflict do nothing; + end if; + return new; + end if; + + -- INSERT + if new.status in ('confirmed', 'completed') then + insert into public.booked_slots (date, time) + values (new.date, new.time) + on conflict do nothing; + end if; + return new; +end; +$$; + +drop index if exists public.bookings_active_slot_idx; +create unique index bookings_active_slot_idx + on public.bookings (date, time) + where status in ('confirmed', 'completed'); diff --git a/supabase/tests/database/rls.test.sql b/supabase/tests/database/rls.test.sql index d0cebbd..85da329 100644 --- a/supabase/tests/database/rls.test.sql +++ b/supabase/tests/database/rls.test.sql @@ -1,5 +1,5 @@ begin; -select plan(42); +select plan(47); -- Seed reference data as service_role (bypasses RLS) set local role service_role; @@ -242,8 +242,15 @@ select lives_ok( 'admin can upload to salon-images' ); +-- Scoped to a single booking (not a blanket service_id filter): several +-- other knotless bookings above share a slot across statuses (e.g. two +-- cancelled bookings for 2026-10-01 2:00 PM), and a blanket update would +-- try to move both of them to 'completed' at once, colliding on the +-- occupied-slot unique index below -- that's a test-setup hazard, not the +-- behavior under test here. select lives_ok( - $$ update public.bookings set status = 'completed' where service_id = 'knotless' $$, + $$ update public.bookings set status = 'completed' + where date = '2026-10-04' and time = '1:00 PM' and status = 'confirmed' $$, 'admin can mark a booking completed' ); @@ -254,6 +261,39 @@ select throws_ok( 'booking status is still constrained to known values' ); +-- A completed appointment actually happened and must keep occupying its +-- slot: marking a confirmed booking completed must NOT free booked_slots +-- (regression test for the confirmed -> completed double-booking bug). +select lives_ok( + $$ insert into public.bookings (service_id, date, time, contact_name, contact_phone, contact_email, contact_notes, status) + values ('knotless', '2026-10-05', '10:00 AM', 'Complete Client', '555-0301', 'complete@example.com', '', 'confirmed') $$, + 'confirmed booking for the completed-slot test succeeds' +); + +select isnt_empty( + $$ select * from public.booked_slots where date = '2026-10-05' and time = '10:00 AM' $$, + 'booked_slots mirror has an entry for the confirmed slot before completion' +); + +select lives_ok( + $$ update public.bookings set status = 'completed' + where date = '2026-10-05' and time = '10:00 AM' and status = 'confirmed' $$, + 'marking the booking completed succeeds' +); + +select isnt_empty( + $$ select * from public.booked_slots where date = '2026-10-05' and time = '10:00 AM' $$, + 'booked_slots mirror row STILL holds the slot after confirmed -> completed' +); + +select throws_ok( + $$ insert into public.bookings (service_id, date, time, contact_name, contact_phone, contact_email, contact_notes, status) + values ('knotless', '2026-10-05', '10:00 AM', 'Double Book Client', '555-0302', 'double@example.com', '', 'confirmed') $$, + '23505', + null, + 'a second confirmed booking for a now-completed slot still fails with a uniqueness violation' +); + select isnt_empty( $$ select * from public.notification_failures $$, 'admin can read notification_failures'