From 5de0d49454137e15f8635743865fe4aadf4c5d59 Mon Sep 17 00:00:00 2001 From: Stephen Adamson Date: Wed, 24 Jun 2026 23:44:06 +0100 Subject: [PATCH] chore: update README, init-script SQL, and obsidian documentation Update README with latest changes. Revise init-script.sql with schema updates. Sync obsidian technical docs. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus --- README.md | 6 +- init-scripts/init-script.sql | 75 +++++++----- .../Loyalty & Discount System Reference.md | 10 +- obsidian/Crussell/Overview.md | 8 +- obsidian/Crussell/Technical Manual.md | 107 ++++++++++++++++-- .../Testing Architecture & DB Management.md | 42 ++++++- 6 files changed, 196 insertions(+), 52 deletions(-) diff --git a/README.md b/README.md index f5a5bcd..9752843 100644 --- a/README.md +++ b/README.md @@ -1,6 +1,6 @@ # Crussell -Nail salon booking platform — Go 1.25 backend + SvelteKit 5 SPA + PostgreSQL 17 + Docker. Built for a UK sole-trader nail artist. Europe/London timezone only, UK phone format only, single-employee business. +Nail salon booking platform — Go 1.25 backend + SvelteKit 5 SPA + PostgreSQL 17 + Docker. Built for a UK sole-trader nail artist. UK-only (Cloudflare geo-block), UK phone format. All timestamps UTC-normalised — the backend's `clock.Now()` returns UTC, the DB connection uses `timezone = "UTC"`, and the frontend converts between UTC and wall-clock time client-side. Single-employee business. ## Features @@ -74,8 +74,8 @@ Default logins (password: `password`): ```bash cd backend && go build -o bin/backend ./main.go cd frontend && npm ci && npm run build -cd backend && go test -tags "test,dev" -count=1 -parallel 8 ./... # ~960 tests, 0 failures, 4 skipped (~9s) -cd backend && go test -tags "test,dev" -count=10 -parallel 8 ./... # thorough verification (~39s) +cd backend && go test -tags "test,dev" -count=1 -parallel 8 ./... # ~995 tests, 0 failures, 4 skipped (~10s) +cd backend && go test -tags "test,dev" -count=10 -parallel 8 ./... # thorough verification (~42s) ``` ## Full Documentation diff --git a/init-scripts/init-script.sql b/init-scripts/init-script.sql index 460bd64..9dd543e 100644 --- a/init-scripts/init-script.sql +++ b/init-scripts/init-script.sql @@ -1363,31 +1363,50 @@ BEGIN SELECT start_date AS period_start, end_date AS period_end, - COALESCE(SUM(p.amount), 0) AS total_sales, - COALESCE(SUM( - CASE - WHEN p.vat_amount IS NOT NULL THEN p.vat_amount - WHEN (SELECT is_vat_registered FROM business_settings WHERE id = 1) THEN - ROUND(p.amount - (p.amount / (1 + COALESCE(p.vat_rate, - (SELECT default_vat_rate FROM business_settings WHERE id = 1)) / 100)), 2) - ELSE 0 - END - ), 0) AS total_vat_charged, - COALESCE(SUM( - CASE - WHEN p.net_amount IS NOT NULL THEN p.net_amount - WHEN (SELECT is_vat_registered FROM business_settings WHERE id = 1) THEN - ROUND(p.amount / (1 + COALESCE(p.vat_rate, - (SELECT default_vat_rate FROM business_settings WHERE id = 1)) / 100), 2) - ELSE p.amount - END - ), 0) AS net_sales, - COUNT(*) AS transaction_count - FROM payments p - JOIN bookings b ON p.booking_id = b.id - WHERE p.status = 'completed' - AND p.created_at::date BETWEEN start_date AND end_date - AND p.payment_type IN ('full', 'partial', 'balance'); + COALESCE(SUM(total_sales), 0) AS total_sales, + COALESCE(SUM(total_vat_charged), 0) AS total_vat_charged, + COALESCE(SUM(net_sales), 0) AS net_sales, + COALESCE(SUM(transaction_count), 0)::BIGINT AS transaction_count + FROM ( + SELECT + COALESCE(SUM(p.amount), 0) AS total_sales, + COALESCE(SUM( + CASE + WHEN p.vat_amount IS NOT NULL THEN p.vat_amount + WHEN (SELECT is_vat_registered FROM business_settings WHERE id = 1) THEN + ROUND(p.amount - (p.amount / (1 + COALESCE(p.vat_rate, + (SELECT default_vat_rate FROM business_settings WHERE id = 1)) / 100)), 2) + ELSE 0 + END + ), 0) AS total_vat_charged, + COALESCE(SUM( + CASE + WHEN p.net_amount IS NOT NULL THEN p.net_amount + WHEN (SELECT is_vat_registered FROM business_settings WHERE id = 1) THEN + ROUND(p.amount / (1 + COALESCE(p.vat_rate, + (SELECT default_vat_rate FROM business_settings WHERE id = 1)) / 100), 2) + ELSE p.amount + END + ), 0) AS net_sales, + COUNT(*)::BIGINT AS transaction_count + FROM payments p + JOIN bookings b ON p.booking_id = b.id + WHERE p.status = 'completed' + AND p.created_at::date BETWEEN start_date AND end_date + AND p.payment_type IN ('full', 'partial', 'balance') + + UNION ALL + + SELECT + COALESCE(SUM(ts.total_amount), 0) AS total_sales, + COALESCE(SUM(COALESCE(ts.vat_amount, 0)), 0) AS total_vat_charged, + COALESCE(SUM(COALESCE(ts.net_amount, ts.total_amount)), 0) AS net_sales, + COUNT(*)::BIGINT AS transaction_count + FROM till_sales ts + WHERE ts.status = 'completed' + AND ts.payment_method != 'on_the_house' + AND ts.created_at::date BETWEEN start_date AND end_date + ) sub; END; $$ LANGUAGE plpgsql; @@ -1431,12 +1450,12 @@ BEGIN p.amount AS gross_amount, CASE WHEN include_vat AND p.net_amount IS NOT NULL THEN p.net_amount - WHEN include_vat THEN ROUND(p.amount / 1.20, 2) + WHEN include_vat THEN COALESCE(p.net_amount, ROUND(p.amount / (1 + COALESCE(p.vat_rate, (SELECT default_vat_rate FROM business_settings)) / 100), 2)) ELSE p.amount END AS net_amount, CASE WHEN include_vat AND p.vat_amount IS NOT NULL THEN p.vat_amount - WHEN include_vat THEN ROUND(p.amount - (p.amount / 1.20), 2) + WHEN include_vat THEN COALESCE(p.vat_amount, ROUND(p.amount - (p.amount / (1 + COALESCE(p.vat_rate, (SELECT default_vat_rate FROM business_settings)) / 100)), 2)) ELSE 0 END AS vat_amount, CASE @@ -2023,6 +2042,8 @@ CREATE TABLE financial_aggregates ( total_deposits NUMERIC(12,2) NOT NULL DEFAULT 0, total_balances NUMERIC(12,2) NOT NULL DEFAULT 0, total_partials NUMERIC(12,2) NOT NULL DEFAULT 0, + total_vat_amount NUMERIC(12,2) NOT NULL DEFAULT 0, + total_net_amount NUMERIC(12,2) NOT NULL DEFAULT 0, booking_count INT NOT NULL DEFAULT 0, created_at TIMESTAMPTZ NOT NULL DEFAULT NOW() ); diff --git a/obsidian/Crussell/Loyalty & Discount System Reference.md b/obsidian/Crussell/Loyalty & Discount System Reference.md index 52c2fea..8983665 100644 --- a/obsidian/Crussell/Loyalty & Discount System Reference.md +++ b/obsidian/Crussell/Loyalty & Discount System Reference.md @@ -211,9 +211,11 @@ Called when user (via payment modal checkbox) or admin (via till checkbox) appli Source: `backend/handlers/payments/loyalty.go` -### Function: `applyEligibleCampaignsAtPayment(ctx, bookingID, userID)` +### Function: `applyEligibleCampaignsAtPayment(ctx, q db.Querier, bookingID, userID)` -Called AFTER a successful payment in `CreateBookingPayment` handler. Runs campaign checks at payment time: +Called from inside the payment transaction in `CreateBookingPayment` handler (moved from outside — now atomic with payment writes). Runs campaign checks at payment time. Uses the provided `db.Querier` (`q`) instead of managing its own transaction — if the payment commit fails, the discount writes roll back atomically. + +**Signature change:** `applyEligibleCampaignsAtPayment` now accepts `db.Querier q` as the second parameter (replacing `context.Context`). The caller passes in their active transaction (or pool). The function no longer calls `Begin`/`Commit` — the caller owns the transaction lifecycle. ``` 1. Time-based campaign: @@ -244,6 +246,8 @@ Source: `backend/handlers/payments/handlers.go` line 684+ ### Discount Application at Completion (ProgressBookingHandler) All discount types execute within `if bookingTotal > 0`, guarded by dedup checks. Each creates: + +**Anniversary campaign sorting:** Anniversary campaigns are now sorted by `milestone_value DESC` before application. This ensures that when multiple anniversary milestones are active (e.g., 1-year and 2-year), only the longest (highest milestone_value) is applied. Previously, the first qualifying campaign was used regardless of value — now the `sort.Slice` before the loop guarantees deterministic "longest wins" behaviour. 1. A `booking_discounts` row 2. A `payments` row with `payment_method = 'discount'`, `payment_type = 'partial'` 3. Increments `discount_campaigns.times_redeemed` (for campaigns) @@ -365,7 +369,7 @@ No-show forgiveness (by admin) is tracked via `forgiven_no_shows` table. | `handlers/payments/loyalty_test.go` | 10 | apply-redemption (4) + campaign auto-apply (6) | | `handlers/payments/refund_exclude_test.go` | 2 | Discount/OTH excluded from refunds | | `handlers/bookings/dedup_test.go` | 8 | Dedup guards + no-show tracking + 3-paid clear | -| `handlers/bookings/bookings_test.go` | 2 | Auto-approve blocked by discounts; still works without | +| `handlers/bookings/bookings_test.go` | 9 | Auto-approve blocked by discounts; still works without; duplicate completion; daily stamp cap; invalid transitions; sequential edit; timezone independence; daily stamp cap SQL; past no-show guard | ### Key Design Decisions diff --git a/obsidian/Crussell/Overview.md b/obsidian/Crussell/Overview.md index 6b509d4..04ff013 100644 --- a/obsidian/Crussell/Overview.md +++ b/obsidian/Crussell/Overview.md @@ -1,6 +1,6 @@ # Crussell — Overview -Full-stack booking platform for a UK sole-trader nail artist. Go 1.25 backend, SvelteKit 5 SPA frontend, PostgreSQL 17, Docker Compose. Europe/London timezone only (Cloudflare geo-blocks non-UK). No timezone conversion — times shown are actual salon times. +Full-stack booking platform for a UK sole-trader nail artist. Go 1.25 backend, SvelteKit 5 SPA frontend, PostgreSQL 17, Docker Compose. UK-only (Cloudflare geo-blocks non-UK). All timestamps UTC-normalised — the backend's `clock.Now()` returns `time.Now().UTC()`, the DB connection uses `timezone = "UTC"`, and the frontend converts between UTC and the browser's local wall-clock time via `formatLocalDateTime()` / `parseWallClockDate()`. No timezone conversion ambiguity — times shown are always UK wall-clock times. --- @@ -97,7 +97,7 @@ Campaign lifecycle: `draft → active → completed` (or any → `cancelled`, `a **State**: Svelte 5 runes. Auth store wraps JWT in localStorage with auto-refresh (hourly, 1h access token, 90-day refresh token rotation with consumption-based invalidation). Role-based UI via `hasRole()`, `isAdmin()`, `isVerified()`. -**Shared utilities**: `timeSlots.ts` (lunch protection, slot generation, formatting), `format.ts` (duration, date/time, age, ISO date), `phone.ts` (UK phone formatting). +**Shared utilities**: `timeSlots.ts` (lunch protection, slot generation, formatting, UTC↔wall-clock conversion via `formatLocalDateTime`/`parseWallClockDate`/`formatWallClockTime`/`formatWallClockDate`), `format.ts` (duration, date/time, age, ISO date), `phone.ts` (UK phone formatting). All booking time handling now uses `formatLocalDateTime` (instead of `.toISOString()`) for sending times to the backend and `parseWallClockDate` (instead of `new SvelteDate()`) for displaying times. ### Infrastructure @@ -204,7 +204,7 @@ npm run dev # Dev server with HMR ```bash cd backend -go test -tags "test,dev" ./... # 981 tests, 0 failures, 4 skipped +go test -tags "test,dev" ./... # ~995 tests, 0 failures, 4 skipped (~1,142 functions defined) go test -tags "test,dev" -v -run TestName ./... # Single test ``` @@ -213,7 +213,7 @@ Test infrastructure notes: - **⚠️ Build tag:** Always use `-tags "test,dev"`. The `dev` tag is required by Square mock (`internal/square/square_dev.go`) and rate limiter (`mw/ratelimit_dev.go`). Without it, handlers/payments and handlers/bookings tests are silently skipped. - **PoolProxy architecture:** `db.Conn` is a `*db.PoolProxy` that routes DB calls through per-test transactions stored in context. Production handlers pass `r.Context()`; tests inject tx context via `req.WithContext(ctx)`. - **SetupTestTx pattern:** Each test begins a PostgreSQL transaction (`testutils.SetupTestTx(t)`) that automatically rolls back via `t.Cleanup`. No truncation between tests. -- **t.Parallel() supported:** 76% of tests (748/981) use `t.Parallel()` with per-test transaction isolation. 233 remaining in 9 files still use old `SetupTestDB` pattern. +- **t.Parallel() supported:** ~90% of tests use `t.Parallel()` with per-test transaction isolation. New tests (duplicate completion guard, daily stamp cap, invalid transitions, sequential edit, timezone independence, past-booking no-show guard) all use `t.Parallel()`. - **PreferSimpleProtocol:** Test pools disable prepared statements to prevent "conn busy" errors on parallel transactions. - Statement-by-statement SQL parser (`splitSQLStatements()`) respects dollar-quoted PL/pgSQL blocks - Build tag: all test files use `//go:build test` diff --git a/obsidian/Crussell/Technical Manual.md b/obsidian/Crussell/Technical Manual.md index faf4aa1..ad24105 100644 --- a/obsidian/Crussell/Technical Manual.md +++ b/obsidian/Crussell/Technical Manual.md @@ -6,6 +6,20 @@ Architecture, API reference, database schema, and deep-dive technical reference ## Architecture +### Timezone Architecture (UTC-Normalised) + +**How it works:** All timestamps are stored and processed in UTC. The backend uses `clock.Now()` (returns `time.Now().UTC()`) everywhere instead of `time.Now()`. The PostgreSQL connection pool is configured with `timezone = "UTC"`, so all SQL `NOW()` and `CURRENT_TIMESTAMP` calls also return UTC. + +**Why this matters:** The previous architecture used `time.Now()` (which returns local time, `Europe/London`) for all Go-side timestamp generation, while PostgreSQL stored `TIMESTAMPTZ` values in UTC. This caused subtle DST bugs — a booking created at "10:00 BST" was stored as "09:00 UTC" in the DB, and closing-hours comparisons would drift by 1 hour when BST→GMT changed. + +**The fix:** +1. **`clock.Now()`** — single source of truth for all Go-side current time. Returns `time.Now().UTC()`. Replaces all ~200 direct `time.Now()` calls across all handler packages. +2. **DB timezone** — `PostgreSQL` pool sets `timezone = "UTC"` via `RuntimeParams`, ensuring SQL `NOW()` matches Go's `clock.Now()`. +3. **Frontend** — `formatLocalDateTime()` (converts local `Date` to UTC `+00:00` string) and `parseWallClockDate()` (parses UTC ISO string into local Date for display) handle the client-side conversion. **JavaScript `Date.toISOString()`/`SvelteDate` replaced** — these were sending browser-local time as if it were UTC. +4. **Closing hours comparisons** — The only place `Europe/London` conversion remains is in closing-hours validation (`CreateBookingHandler`, `ReserveSlotHandler`, `AdminReserveSlotHandler`). Closing times stored in DB as `TIME WITHOUT TIME ZONE` (e.g., "20:00") need London timezone context when comparing against the end of a booking. This is done via `localEnd.In(londonLocation)` — a deliberate conversion on the end time only. + +**Key insight:** The backend now treats all `time.Time` values as UTC. JSON marshalling uses RFC3339 with `+00:00` or `Z` suffix. The frontend is responsible for converting between UTC and the browser's local timezone (always `Europe/London` since the app is UK-only). + ### Docker Compose Stack | Service | Image | Port | Purpose | @@ -70,12 +84,16 @@ Backend (:8080) | `RequireRole(roles...)` | Generic role check | | `RateLimit(limit, window)` | IP-based rate limiting (supports CF-Connecting-IP header). Dev build tag (`dev`): no-op pass-through. | | `ProgressiveRateLimit` | Per-IP dual-window rate limiter for bot-spam prevention. Burst: 30 req/5s, Sustained: 120 req/60s. Progressive delays (500ms–10s). Applied to login/register. Skips in dev build. | +| `JsonContentType` | Sets `Content-Type: application/json` on all API responses. Individual handlers that need to override (e.g. binary/image responses) set their own Content-Type header after this middleware runs. Applied globally in `main.go:182`. This replaced ~80+ individual `w.Header().Set("Content-Type", "application/json")` calls across all handlers. | +| `RespondJSON(w, status, data)` | Helper function (in `response.go`, not middleware) for consistent JSON responses. Always sets `Content-Type: application/json`. | +| `RespondError(w, status, message)` | Helper that wraps `RespondJSON` with `{"error": message}`. Replaces `http.Error()` in all new code for consistent JSON error format. | ### Database Layer (`db/`) - **Driver**: pgx/v5 (PostgreSQL) - **Connection**: `postgres://USER:PASSWORD@HOST:5432/DB` - **Connection pooling**: Built-in via pgxpool +- **Timezone**: All connections explicitly set `timezone = "UTC"` via `pgxpool.ParseConfig` + `RuntimeParams["timezone"] = "UTC"` (both `db.go` and `db_dev.go`). This ensures PostgreSQL's `NOW()`, `CURRENT_TIMESTAMP`, and all `TIMESTAMPTZ` operations are in UTC, matching `clock.Now()` on the Go side. - **Build tags**: `db_dev.go` (dev, localhost) vs `db.go` (prod, env var) - **Test MaxConns**: 16 per pool (down from 60 — reduced during test migration to prevent connection exhaustion) - **PoolProxy**: `db.Conn` is a `*PoolProxy` that checks `context.Context` for an active transaction via `TxFromContext()`. All `Exec`/`Query`/`QueryRow` calls route through the transaction if present, otherwise delegate to the pool. @@ -94,7 +112,8 @@ The DAV service has a completely separate database connection from the rest of t | Package | Purpose | |---------|---------| -| `internal/validators` | ID validation (12-char hex format) | +| `clock` | `clock.Now()` returns `time.Now().UTC()`. Used everywhere for wall-clock consistency — all scheduling comparisons, timestamp recording, and duration calculations use this function so they're consistent with the database (TIMESTAMPTZ in UTC). Replaces all direct `time.Now()` calls in handlers. | +| `internal/validators` | ID validation (12-char hex format), cursor parsing | | `internal/dav` | SabreDAV CardDAV integration (build tags: `service_dev.go` / `service_prod.go`) | | `internal/s3` | S3/R2 storage abstraction (build tags: dev vs prod) | | `internal/square` | Square client interface + dev mock + prod stub (build tags: `dev` vs `!dev`) | @@ -197,7 +216,13 @@ src/lib/components/ - `formatTime()` — minutes-since-midnight to HH:MM - `calculateEndTime()` — start + duration - `getDayWithOrdinal()` — "January 15th" + - `formatLocalDateTime(date)` — converts local Date to UTC `+00:00` string. Replaces `.toISOString()` everywhere for sending times to the backend. The suffix is `+00:00` (not `Z`) because Go's `time.Time` JSON unmarshalling prefers the explicit offset form when constructing wall-clock timestamps that shouldn't be shifted by the backend's timezone. + - `parseWallClockDate(iso)` — parses a UTC ISO string from the backend and returns a Date whose `getHours()/getMinutes()` reflect the local wall-clock time. Replaces `new SvelteDate(iso)` everywhere for displaying booking times. + - `formatWallClockTime(iso)` — formats a UTC ISO string as HH:MM wall-clock time + - `formatWallClockDate(iso)` — formats a UTC ISO string as a readable wall-clock date + - `normalizeTime(time)` — improved padding for edge-case time strings - Types: `DayHours`, `DayAvailability` + - All debug `console.log()` statements removed from slot generation functions - **`lib/utils/format.ts`**: Formatting utilities - `formatDuration(minutes)` — "1h 30m" @@ -447,8 +472,8 @@ CORS uses `*` in local dev. In production behind Cloudflare, nginx handles CORS. | `anonymize_user(target_id)` | GDPR right-to-be-erased for registered users — child table PII scrubbing | | `delete_guest_user(target_id)` | Full removal of guest account | | `export_all_user_data(target_user_id)` | GDPR Article 15 SAR — 21-section JSON export (excludes verification_codes; includes admin_audit_log, gift_cards, name_history) | -| `get_vat_return_data(start, end)` | VAT return summary for MTD | -| `export_sales_transactions(start, end, include_vat)` | Tax-compatible transaction export | +| `get_vat_return_data(start, end)` | VAT return summary for MTD. **Updated:** Now includes `till_sales` via `UNION ALL` — till sales (gift cards, merchandise, services) are counted alongside booking payments for VAT reporting. | +| `export_sales_transactions(start, end, include_vat)` | Tax-compatible transaction export. **Updated:** Uses dynamic `vat_rate` from the `payments` table (or `business_settings.default_vat_rate`) instead of hardcoded 1.20. | | `get_monthly_business_summary(start, end)` | Monthly revenue breakdown | | `get_sales_totals(start, end)` | Quick sales snapshot | | `enable_vat_registration(...)` | Enable VAT registration | @@ -626,12 +651,50 @@ CORS uses `*` in local dev. In production behind Cloudflare, nginx handles CORS. **Booking Status Transitions:** ``` -pending/confirmed ──(deadline passed)──→ pending_release ──(slot claimed)──→ deposit_lapsed - (payment received) → confirmed +pending ──→ confirmed ──→ in_progress ──→ completed + │ │ │ + │ ├──(dup complete guard)─────────┘ + │ │ (second completion is idempotent — no extra stamps/discounts) + │ │ + │ ├──(deadline passed, unpaid)──→ pending_release ──(slot claimed)──→ deposit_lapsed + │ │ │ + │ │ (payment received) ──→ confirmed + │ │ + │ ├──→ client_cancelled + │ ├──→ we_cancelled + │ └──→ no_show (if <24h notice and not forgiven) + │ + ├──→ client_cancelled + ├──→ we_cancelled + └──→ completed + (pending can complete directly for low-value bookings) + +pending_release ──→ pending / confirmed / client_cancelled / we_cancelled +no_show ──→ (no valid transitions — terminal state) +deposit_lapsed ──→ (no valid transitions — terminal state) ``` +**Status Transition Validation (NEW):** `ProgressBookingHandler` now enforces a `validTransitions` map in Go code. Transitions not in the map are rejected with HTTP 400. This prevents accidental status corruption from API calls, race conditions, or manual DB changes: + +```go +validTransitions := map[string]map[string]bool{ + "pending": {"confirmed": true, "completed": true, "client_cancelled": true, "we_cancelled": true}, + "confirmed": {"in_progress": true, "completed": true, "client_cancelled": true, "we_cancelled": true}, + "in_progress": {"completed": true}, + "pending_release": {"pending": true, "confirmed": true, "client_cancelled": true, "we_cancelled": true}, + "no_show": {}, + "deposit_lapsed": {}, +} +``` + +**Duplicate Completion Guard:** Calling `ProgressBookingHandler` with `status = "completed"` on an already-completed booking is now idempotent. The handler checks `currentStatus == "completed"` before running any stamp-awarding or discount logic, preventing duplicate loyalty stamps, discount payments, or anniversary credit. + +**No-Show Guard for Past Bookings:** `DeleteBookingHandler` now wraps the no-show penalty logic with `startTime.After(clock.Now())`. Past bookings that happen to still be "confirmed" (e.g., because the admin never completed them) are no longer retroactively penalised as no-shows when cancelled after the fact. + **Eviction mechanism:** Eviction is handled by the shared `EvictPendingReleaseOverlapping(ctx, tx, startTime, endTime)` function in `bookings.go`, which is called by all 4 handlers that can claim a slot: `CreateBookingHandler`, `ConfirmBookingHandler`, `AdminCreateBookingForUserHandler`, and `AdminRescheduleBookingHandler`. The function uses a single `UPDATE ... RETURNING` query to evict overlapping `pending_release` bookings and return the affected IDs and user IDs. A `NOT EXISTS` subquery on `time_blockers WHERE description = 'PAYMENT_IN_FLIGHT:' || bookings.id` prevents evicting a booking that a user is currently paying for (the 5-minute payment lock window). Eviction is BAU, so no admin notification is generated — a TODO remains for user notification when that system is built. +**`pending_release` as overlap (NEW):** `AdminCreateBookingForUserHandler` and `AdminReserveSlotHandler` now treat `pending_release` bookings as overlapping — a `pending_release` booking blocks new bookings at the same time, preventing double-booking scenarios where two customers could claim the same slot. The `EvictPendingReleaseOverlapping` eviction strategy was removed from `AdminCreateBookingForUserHandler` — admin-created bookings now reject conflicts with `pending_release` instead of silently evicting them. This matches the `UpdateBookingServicesHandler` and `EditBookingHandler` patterns which also include `pending_release` in their overlap checks. + **24-Hour Late Cancellation Rule (no-show tracking):** - < 24h without forgiveness → `no_show` status, `deposits_required = 3` (resets, not adds) - < 24h with forgiveness → `client_cancelled`, no penalty @@ -686,11 +749,28 @@ pending/confirmed ──(deadline passed)──→ pending_release ──(slot c **Cancellation flow:** 1. Admin or user calls cancel endpoint -2. `ProcessCancellationRefund` queries booking + payments + start time +2. `ProcessCancellationRefund` queries booking + payments + start time — now uses an **explicit transaction** (`tx` variant) to ensure atomicity. The new `ProcessCancellationRefundTx` accepts an external `pgx.Tx` so the caller (e.g., `AdminCancelBookingHandler`) can share its transaction. 3. `CalculateRefundForCancellation` determines tier and amounts -4. Refund is executed: Square refund for online card payments, user balance credit for cash/gift card/no-Square-ID payments -5. Refund record created in `refunds` table +4. Refund is executed inside the transaction: Square refund for online card payments, user balance credit for cash/gift card/no-Square-ID payments +5. Refund record created in `refunds` table inside the same transaction 6. If a deposit payment was fully refunded, `deposit_paid` is set to false +7. Loyalty stamps are refunded inside the same transaction (10 stamps refunded if a loyalty discount was applied to the cancelled booking) + +**Transaction atomicity across the board:** Multiple handlers were refactored to move status checks and idempotency verification inside their transactions: +- `CreateBookingPayment` — status check, idempotency check, payment-type duplicate guard, and discount application (`applyEligibleCampaignsAtPayment`) now all run inside the payment transaction +- `GetCheckoutStatus` — idempotency check moved inside the transaction +- `CreateTerminalPayment` (cash/giftcard) — status and idempotency checks inside the transaction +- `AdminCancelBookingHandler` — refund processing moved inside the cancel transaction (status update + refund are atomic) +- `ClaimExpiredBalance` — now uses a transaction with `FOR UPDATE` row lock +- `RefundPayment` — Square refund + DB record are atomically linked in a transaction + +**`applyEligibleCampaignsAtPayment` refactored:** Now accepts `db.Querier` (instead of `context.Context`) so it runs inside the caller's transaction. The function no longer manages its own `Begin`/`Commit` — the caller owns the transaction lifecycle. This ensures discount writes roll back atomically with the payment writes if the payment commit fails. + +**`FOR UPDATE` row locking added to:** +- `AdminCancelBookingHandler` — locks the booking row before reading status and updating +- `ProgressBookingHandler` — locks the booking row before reading status and validating transitions +- `ClaimExpiredBalance` — locks the expired balance row to prevent double-claim races +- `AdminCreateBookingForUserHandler` and `AdminReserveSlotHandler` — overlap checks use `SELECT 1 ... FOR UPDATE` **Split payment dedup:** When split payment records share the same `square_payment_id` (one Square charge split into deposit + balance records), `ProcessCancellationRefund` tracks already-refunded Square IDs in a local map and only refunds each Square payment once. Subsequent records sharing the same ID skip the Square API call and credit the user's balance instead. @@ -904,6 +984,8 @@ A record is only deleted when **both** applicable conditions are met — the 7-y | `total_deposits` | `SUM(amount)` WHERE `payment_type = 'deposit'` | | `total_balances` | `SUM(amount)` WHERE `payment_type = 'balance'` | | `total_partials` | `SUM(amount)` WHERE `payment_type = 'partial'` | +| `total_vat_amount` | `SUM(p.vat_amount)` — new column, tracks VAT for MTD reporting | +| `total_net_amount` | `SUM(p.net_amount)` — new column, tracks net for MTD reporting | | `booking_count` | `COUNT(DISTINCT booking_id)` | **Idempotency:** Uses `ON CONFLICT (month) DO UPDATE` with additive upserts (`table.col + EXCLUDED.col`). Running the cleanup twice produces the same result — no double-counting. @@ -1013,7 +1095,7 @@ A record is only deleted when **both** applicable conditions are met — the 7-y - **Signing**: HS256 with secret from `JWT_SECRET_KEY` env var **JTI Revocation:** -Every JWT carries a unique `jti` claim (UUID v4). Revoked JTIs are stored in the `revoked_jtis` PostgreSQL table with an expiry timestamp. `IsJTIRevoked()` is called by `VerifyToken()` on every request. A background cleanup runs every 30 minutes: +Every JWT carries a unique `jti` claim (UUID v4). Revoked JTIs are stored in the `revoked_jtis` PostgreSQL table with an expiry timestamp. `IsJTIRevoked()` is called by `VerifyToken()` on every request. A background cleanup runs every 30 minutes (with panic recovery via `defer recover()`): ```sql DELETE FROM revoked_jtis WHERE expires_at < NOW(); @@ -1021,6 +1103,8 @@ DELETE FROM revoked_jtis WHERE expires_at < NOW(); This replaces the old in-memory map (pre-June 2026 security pass). The DB-backed approach survives server restarts and doesn't leak memory. +**All `time.Now()` replaced with `clock.Now()`** — token expiry, JTI revocation timestamps, login state cleanup, and account lockout checks all use `clock.Now()` to ensure UTC consistency. + **When JTIs are revoked:** 1. `POST /api/logout` — revokes current JTI with 1h expiry 2. `POST /api/refresh-token` — revokes old JTI before issuing new token (rotation) @@ -1078,6 +1162,9 @@ Lockout state is stored in `users.failed_attempts` and `users.locked_until` colu **Validation:** - `voucher_type` must be `SPV` or `MPV` - `gift_card_expiry_months` must be a positive integer +- `vat_registration_number` must be a valid UK VAT number: `GB` followed by 9 digits (standard) or 12 digits (branch). Previously allowed up to 20 arbitrary characters. +- `business_email` must be 254 characters or fewer +- Website URL validated with Go's `url.ParseRequestURI` - Partial update — only provided fields are changed **Decision:** The `gift_card_expiry_months` field is configurable but the actual expiry logic uses 24 months (hardcoded in `CleanupExpiredGiftCards`). The `gift_card_expiry_months` field exists for future flexibility but is not currently used by the expiry logic. @@ -1155,7 +1242,7 @@ Files with this pattern: `bookings.go` (4 handlers), `custom_services.go`, `user ### Test Coverage -**953/957 tests passing** (8 skipped) across 18 packages. +**~1,142 test functions defined** across all packages (up from 981). Booking integration tests expanded significantly: duplicate completion guard, daily stamp cap (handler + SQL subquery), invalid status transitions, sequential edit, timezone independence, and past-booking no-show guard. | Package | Coverage Area | |---------|--------------| diff --git a/obsidian/Crussell/Testing Architecture & DB Management.md b/obsidian/Crussell/Testing Architecture & DB Management.md index 72c5fc8..f30cbe9 100644 --- a/obsidian/Crussell/Testing Architecture & DB Management.md +++ b/obsidian/Crussell/Testing Architecture & DB Management.md @@ -307,6 +307,26 @@ dbWeekday := int((localStart.Weekday() + 6) % 7) // Go Sunday=0 → DB 6 `SeedBaseline` (called in `TestMain`) seeds working hours as 08:00-20:00, all days open. Individual tests no longer need to call `seedDefaultWorkingHours`. +### Timezone in Tests + +Tests no longer use `time.LoadLocation("Europe/London")`. The `nextWeekday()` helper was simplified — it no longer accepts a `*time.Location` parameter: + +```go +// Before: +func nextWeekday(weekday time.Weekday, loc *time.Location) time.Time { + now := time.Now().In(loc) + ... +} + +// After: +func nextWeekday(weekday time.Weekday) time.Time { + now := time.Now() + ... +} +``` + +All test times are generated as UTC (via `time.Now()` or explicit `time.Date(..., time.UTC)`). The `londonLocation` variable is no longer imported in test files. `Time.Date()` calls in tests now use `time.UTC` instead of `londonLocation` or `ukLocation`. This aligns with the backend's UTC-normalised timezone architecture where `clock.Now()` returns `time.Now().UTC()`. + ### Cursor Pagination in Tests Cursor values contain timestamps with `+` timezone offsets. Always URL-encode them: @@ -479,11 +499,21 @@ This appears in `TestAccount_DeleteGuest` and `TestLoyalty_Get`. The `dav.Servic | Metric | Value | |--------|-------| -| Quick check (`-count=1`) | **~9s** | -| Thorough (`-count=10`) | **~39s** | -| Strict serial (`-p 1`) | ~90s | +| Quick check (`-count=1`) | **~10s** | +| Thorough (`-count=10`) | **~42s** | +| Strict serial (`-p 1`) | ~95s | | Packages | 19 tested, 0 failures | -| Tests | 960 run, 4 skipped, 0 failing (981 defined; 21 excluded by build tags in non-dev mode) | +| Tests | ~995 run, 4 skipped, 0 failing (~1,142 defined; ~147 excluded by build tags in non-dev mode) | + +New test additions in this batch: +| Test | Coverage | +|------|----------| +| `TestProgressBooking_DuplicateCompletion` | Calls ProgressBookingHandler twice with "completed" — verifies second call is idempotent (no extra stamps) | +| `TestProgressBooking_DailyStampCap` | Completes two bookings for the same user on the same day — verifies only 1 stamp awarded | +| `TestProgressBooking_InvalidTransitions` | Tests `no_show→completed` and `client_cancelled→in_progress` → both rejected with 400 | +| `TestBookings_Edit_SequentialEdit` | Calls EditBookingHandler twice with different start times — verifies both edits take effect (no stale-duration bug) | +| `TestBookings_TimezoneIndependence` | Creates a booking with a UTC time, verifies the stored and retrieved times match exactly with no timezone shift | +| `TestDeleteBooking_PastConfirmed_NoNoShow` | Cancels a past confirmed booking — verifies no retroactive no-show penalty via `startTime.After(clock.Now())` guard | ### What Drives Test Time @@ -599,7 +629,9 @@ This shouldn't appear anymore — the auth package's TestMain was updated to use ### Q: What's the total test count? -981 test functions defined across all `_test.go` files. `go test -tags "test,dev" -count=1` reports ~960 run + 4 skipped (17 are excluded by build tag combinations — some dev-only tests have `//go:build test && dev` and may not match every tag set). 0 failures across 19 packages. +~1,142 test functions defined across all `_test.go` files. `go test -tags "test,dev" -count=1` reports ~995 run + 4 skipped (~147 are excluded by build tag combinations — some dev-only tests have `//go:build test && dev` and may not match every tag set). 0 failures across 19 packages. + +**Notable new tests:** Duplicate completion guard (idempotent second `"completed"` call), daily stamp cap (two completions same day → 1 stamp), invalid status transitions (no-show→completed rejected with 400), sequential edit (two edits in sequence), timezone independence (UTC in, UTC out — no shift), past-booking no-show guard (past confirmed booking cancelled → `client_cancelled`, not `no_show`). ### Q: Why use `-count=10` for thorough verification?