diff --git a/.sisyphus/plans/staged-default-hours-change.md b/.sisyphus/plans/staged-default-hours-change.md new file mode 100644 index 0000000..4825f5e --- /dev/null +++ b/.sisyphus/plans/staged-default-hours-change.md @@ -0,0 +1,282 @@ +# Staged Default Hours Change with Conflict Resolution + +## Overview + +When an admin edits default working hours, instead of applying changes immediately: +1. Run conflict resolution (same pattern as holiday hours/time blockers) +2. Stage the change with a future effective date (admin picks, default +2 weeks) +3. Current hours continue to apply until the switch-over date +4. At midnight on the effective date, the change auto-applies and triggers a notification +5. Contact page shows "These opening hours will change from [date]" +6. Available hours use current hours before the date, new hours after + +--- + +## What Changes + +### New DB Table: `default_hours_scheduled_changes` + +```sql +CREATE TABLE default_hours_scheduled_changes ( + id SERIAL PRIMARY KEY, + effective_date DATE NOT NULL, -- London midnight date to switch over + created_by CHAR(12) NOT NULL REFERENCES users(id), + created_at TIMESTAMPTZ DEFAULT NOW(), + applied_at TIMESTAMPTZ, -- NULL until cron applies it + cancelled_at TIMESTAMPTZ, -- NULL unless admin cancels + hours JSONB NOT NULL -- [{weekday, startTime, endTime, isOpen}] +); +``` + +Only ONE pending change is allowed at a time. If a pending change exists and the admin tries to create another, they must cancel the existing one first. + +### New `admin_notification_reason` enum value + +```sql +ALTER TYPE admin_notification_reason ADD VALUE 'default_hours_changed'; +``` + +Used by the cron job when it applies the change — creates a single notification for admin review. + +--- + +## Files to Create/Modify + +### Backend + +| # | File | Action | +|---|---|---| +| 1 | `init-scripts/init-script.sql` | Add new table and enum value | +| 2 | `backend/handlers/scheduling/default-hours.go` | Add `ScheduleDefaultHoursChange`, `GetScheduledDefaultHoursChange`, `CancelScheduledDefaultHoursChange` handlers | +| 3 | `backend/handlers/scheduling/default-hours.go` | Modify `GetDefaultHours` — return both current and pending future hours | +| 4 | `backend/handlers/scheduling/default-hours.go` | Modify `computeAvailableHours` / `GetWorkingHours` — inject future hours for dates >= effective_date | +| 5 | `backend/handlers/scheduling/scheduled-cleanup.go` | Add `ApplyScheduledDefaultHours` cron handler | +| 6 | `backend/internal/jobs/cleanup.go` | Register `apply-default-hours` cron job (daily at 00:05) | +| 7 | `backend/main.go` | Register new routes | + +### Frontend + +| # | File | Action | +|---|---|---| +| 8 | `frontend/src/lib/components/admin/WeeklySchedule.svelte` | Add conflict resolution UI + date picker + staged save flow | +| 9 | `frontend/src/lib/components/layout/BusinessHours.svelte` | Show upcoming hours change with date | +| 10 | `frontend/src/routes/admin/+page.svelte` | Wire modal props if needed | + +--- + +## Detail: Backend Design + +### Handler: `ScheduleDefaultHoursChange` (POST) + +`PUT /api/admin/default-hours` → replaced with a staging flow: + +``` +POST /api/admin/default-hours/schedule +Content-Type: application/json + +{ + "hours": [{"weekday": 0, "startTime": "10:00", "endTime": "18:00", "isOpen": true}, ...], + "effective_date": "2026-08-17" // optional, defaults to +14 days from today London +} +``` + +**Flow:** +1. Parse + validate input +2. Check for existing pending change — return 409 if one exists +3. Run conflict detection between current bookings and the proposed hours +4. If conflicts exist → return 409 with `{error: "conflicts exist", bookings: [...]}` (same format as holiday hours) +5. If no conflicts → store the pending change, return 201 with `{effective_date: "2026-08-17"}` + +### Handler: `GetScheduledDefaultHoursChange` (GET) + +``` +GET /api/admin/default-hours/scheduled +``` + +Returns the pending change or 404: +```json +{ + "effective_date": "2026-08-17", + "hours": [...], + "created_at": "...", + "created_by": "..." +} +``` + +### Handler: `CancelScheduledDefaultHoursChange` (DELETE) + +``` +DELETE /api/admin/default-hours/scheduled +``` + +Sets `cancelled_at` on the pending change. Returns 200. + +### Handler: `ApplyScheduledDefaultHours` (cron) + +Runs at "5 0 * * *" (00:05 daily — 5 minutes after midnight to avoid midnight race conditions). + +**Flow:** +1. Query `default_hours_scheduled_changes` where `effective_date <= CURRENT_DATE` (London time) AND `applied_at IS NULL` AND `cancelled_at IS NULL` +2. For each due change: + a. BEGIN transaction + b. DELETE all existing `working_hours` rows + c. INSERT new rows from the change's hours JSON + d. INSERT an `admin_notifications` with reason `default_hours_changed` (details: "Default hours changed from [old summary] to [new summary]") + e. SET `applied_at = NOW()` + f. COMMIT + +### Modified: `GetDefaultHours` + +Current: returns `SELECT weekday, start_time, end_time, is_open FROM working_hours ORDER BY weekday` + +New: if a pending change exists with `effective_date > today`, include an extra field: +```json +[ + {"weekday": 0, "startTime": "00:00", "endTime": "00:00", "isOpen": false, ...}, + ... +] +``` +Response changes to include optional `scheduled_change` field: +```json +{ + "current": [...], + "scheduled_change": { + "effective_date": "2026-08-17", + "hours": [...] + } +} +``` + +This is backwards-compatible for existing consumers — they read the array from `current`. + +### Modified: `computeAvailableHours` / `GetWorkingHours` + +The hours resolution currently goes: proposed > exceptional > default > closed. + +For the staged hours feature, I need to inject the **scheduled future default hours** for dates >= effective_date: + +Priority order: proposed > exceptional > scheduled_future_default > default > closed + +In `computeAvailableHours` (the per-day loop), after looking up the `defaultMap` entry: + +```go +// Before falling through to default/closed, check if there's a staged future change +if scheduledChangeHours != nil && d.In(londonLocation).Format("2006-01-02") >= scheduledEffectiveDate { + if fh, ok := scheduledChangeHours[weekday]; ok { + baseStart = fh.StartTime + baseEnd = fh.EndTime + isOpen = fh.IsOpen + day.Source = "scheduled_change" + } +} else if def, ok := defaultMap[weekday]; ok { + // existing default logic... +} +``` + +This means: +- Today → current working hours apply +- Between today and effective_date → current working hours apply (no change) +- On/after effective_date → new scheduled hours apply +- Exceptional hours always override (higher priority) + +For `GetWorkingHours`, same logic applies — it needs to return the correct hours for each date in the range. + +--- + +## Detail: Frontend Design + +### WeeklySchedule.svelte — "Edit Schedule" Modal + +The modal gets a new top section (same pattern as HolidayHours conflict resolution): + +**Step 1: Admin opens modal, edits hours** +Same time picker interface as today. No change to the editing UX. + +**Step 2: Conflict resolution (new)** +- A date picker for "Apply from" (defaults to +14 days from today in London) +- A `checkConflictingBookings()` function that: + - Takes the proposed hours + effective date range (from effective_date to effective_date + 90 days or so) + - Calls `POST /api/admin/default-hours/schedule` with a dry-run flag or a dedicated conflict endpoint + - Shows amber warning with conflicting bookings + "View Booking" / "View Client" buttons +- Guard: submit button disabled while conflicts exist + +**Step 3: Submit** +- Button text: `Schedule Change for [date]` +- On success toast: `Default hours will change at 23:59 on 17/08/2026` +- The modal closes, admin sees a "Pending change" indicator on the WeeklySchedule card + +**Step 4: Pending change indicator** +- After a change is scheduled, the WeeklySchedule card shows: + - An amber banner: "Default hours are scheduled to change on 17/08/2026" + - A "Cancel" button that calls `DELETE /api/admin/default-hours/scheduled` +- The `fetchDefaultHours` response now includes `scheduled_change` — display it + +### BusinessHours.svelte — Contact Page + +Add a new section below the current "Upcoming Holiday Hours" section: + +```svelte +{#if scheduledChange} +
+

+ Opening hours will change from {formattedDate} +

+ {#each scheduledChange.hours as h} +
+ {dayNames[h.weekday]} + + {#if h.isOpen} + {formatTime(h.startTime)} – {formatTime(h.endTime)} + {:else} + Closed + {/if} + +
+ {/each} +{/if} +``` + +To load this, the `fetchData` function needs an additional API call: +``` +GET /api/admin/default-hours/scheduled (public, or a new public endpoint) +``` + +Since this is shown on the public contact page, the endpoint should be under the public read-only group (OptionalAuth) — similar to how `GET /scheduling/working-hours` is public. + +### admin/+page.svelte + +If `WeeklySchedule` needs `openUserModal` / `openBookingModal` for conflict resolution, wire the same props. Currently `WeeklySchedule` does not accept these props, but the conflict resolution flow needs "View Client" and "View Booking" buttons. + +--- + +## Conflict Resolution vs Holiday Hours — Reusing the Pattern + +The conflict detection for default hours changes reuses the EXACT same pattern as the holiday hours conflict handler, but with a date range instead of week starts: + +``` +POST /api/admin/default-hours/conflicting-bookings +{ + "proposed_hours": [{"weekday": 0, "startTime": "10:00", "endTime": "18:00", "isOpen": true}, ...], + "start_date": "2026-08-17", + "end_date": "2026-11-17" // default: +90 days from effective date +} +``` + +This reuses the same `parseTimeToMinutes` comparison logic and the `ActiveBookingStatuses` filter. The response is the same `OverlappingBookingsResponse` format. + +**Note:** The availability-for-reschedule logic is simpler than holiday hours because: +- Default hours changes apply PERMANENTLY (not per-week like holiday hours) +- The "what hours apply for rescheduling" is just: current hours until effective_date, future hours after +- No per-week mapping needed + +--- + +## Risks + +| Risk | Mitigation | +|---|---| +| Admin sets effective_date in the past | Validate: must be >= tomorrow (London date + 1) | +| Cron miss at midnight doesn't apply change | Cron runs 00:05 to avoid midnight race. Query uses `<= CURRENT_DATE` so it catches any missed days | +| Two admins try to schedule simultaneously | Unique constraint on `(applied_at IS NULL AND cancelled_at IS NULL)` — use a partial unique index | +| Notification is an admin-only todo placeholder | Add `default_hours_changed` to the enum, create one notification. Future work: user notification | +| Scheduling a change far in the future (6+ months) | Conflicts only checked against existing bookings in the window. Long-range changes may need re-checking when new bookings are made — acceptable for v1 | diff --git a/README.md b/README.md index ad7b2b2..040ba2a 100644 --- a/README.md +++ b/README.md @@ -76,7 +76,7 @@ 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 ./... # ~1,642 tests passed (~13s) +cd backend && go test -tags "test,dev" -count=1 -parallel 8 ./... # 1,716 tests passed (4 skipped, ~13s) cd backend && go test -tags "test,dev" -count=1 -race -timeout 480s ./... # race detector (all packages, ~4min) cd backend && go test -tags "test,dev" -count=10 -parallel 8 ./... # thorough verification (~2-3min) ``` diff --git a/obsidian/Crussell/Overview.md b/obsidian/Crussell/Overview.md index 2f5dd48..0c24d8a 100644 --- a/obsidian/Crussell/Overview.md +++ b/obsidian/Crussell/Overview.md @@ -218,7 +218,7 @@ npm run dev # Dev server with HMR ```bash cd backend -go test -tags "test,dev" ./... # ~1,642 tests passed +go test -tags "test,dev" ./... # 1,716 tests passed (4 skipped) go test -tags "test,dev" -v -run TestName ./... # Single test ``` diff --git a/obsidian/Crussell/Technical Manual.md b/obsidian/Crussell/Technical Manual.md index 20c7104..6d4ddf1 100644 --- a/obsidian/Crussell/Technical Manual.md +++ b/obsidian/Crussell/Technical Manual.md @@ -1267,7 +1267,7 @@ Files with this pattern: `bookings.go` (4 handlers), `custom_services.go`, `user ### Test Coverage -**~1,642 tests run** across all packages (0 failures). Coverage improved from 50.4% to 65.0% via 56 new test files covering booking handlers, user handlers, payments (giftcards, till, refunds), DAV, auth, middleware, validators, zxcvbn, and scheduling. Key additions: coverage improvement tests (bookings_coverage_test.go, user_coverage_test.go, payments coverage expansion — all meaningful error-path tests, not padding), split-lunch detection tests, savepoint/transaction-context tests for time-sensitive operations, VAT lifecycle and parallel-deadlock regression tests, and cleanup of 10 dead test functions flagged by staticcheck U1000. +**1,716 tests run** across all packages (4 skipped, 0 failures). Coverage improved from 50.4% to 65.0% via 56 new test files covering booking handlers, user handlers, payments (giftcards, till, refunds), DAV, auth, middleware, validators, zxcvbn, and scheduling. Key additions: coverage improvement tests (bookings_coverage_test.go, user_coverage_test.go, payments coverage expansion — all meaningful error-path tests, not padding), split-lunch detection tests, savepoint/transaction-context tests for time-sensitive operations, VAT lifecycle and parallel-deadlock regression tests, and cleanup of 10 dead test functions flagged by staticcheck U1000. | Package | Coverage Area | |---------|--------------| diff --git a/obsidian/Crussell/Testing Architecture & DB Management.md b/obsidian/Crussell/Testing Architecture & DB Management.md index feb1ebc..1cdf2cd 100644 --- a/obsidian/Crussell/Testing Architecture & DB Management.md +++ b/obsidian/Crussell/Testing Architecture & DB Management.md @@ -1,6 +1,6 @@ # Testing Architecture & DB Management -**Last Updated:** July 2026 (v5 — coverage 50.4%→65.0%, ~1,642 tests) +**Last Updated:** July 2026 (v5 — coverage 50.4%→65.0%, 1,716 tests) --- @@ -501,7 +501,7 @@ This appears in `TestAccount_DeleteGuest` and `TestLoyalty_Get`. The `dav.Servic |--------|-------| | Quick check (`-count=1`) | **~13s** | | Packages | 19 tested, 0 failures | -| Tests | ~1,642 passed, 0 failing | +| Tests | 1,716 passed, 4 skipped, 0 failing | New test additions in this batch: | Test | Coverage | @@ -520,7 +520,7 @@ New test additions in this batch: | `TestCancelReservation_DoesNotTouchAnonReservations` | Inverse-isolation test — user cancel ignores `RESERVATION:anon:%` (defensive — the WHERE clause only matches `RESERVATION:user:%`) | | `TestCancelReservation_DoesNotTouchAdminReservations` | Inverse-isolation test — user cancel ignores `RESERVATION:admin:%`. Pairs with the admin-side test that verifies admin cancel ignores `RESERVATION:user:%`. Proves the two endpoints are properly partitioned. | -**Total tests:** ~1,642 passed across all packages. 0 failures. Growth driven by: coverage improvement pass (new test files for bookings, user, payments, giftcards, till, refunds, DAV, auth, middleware, validators, zxcvbn — 56 new files, coverage 50.4%→65.0%), VAT lifecycle and parallel-deadlock regression tests, savepoint/transaction-context pattern for time-sensitive tests, split-lunch detection tests, and removal of 10 dead test functions flagged by staticcheck U1000. +**Total tests:** 1,716 passed across all packages (4 skipped). 0 failures. Growth driven by: coverage improvement pass (new test files for bookings, user, payments, giftcards, till, refunds, DAV, auth, middleware, validators, zxcvbn — 56 new files, coverage 50.4%→65.0%), VAT lifecycle and parallel-deadlock regression tests, savepoint/transaction-context pattern for time-sensitive tests, split-lunch detection tests, and removal of 10 dead test functions flagged by staticcheck U1000. ### What Drives Test Time @@ -637,7 +637,7 @@ This shouldn't appear anymore — the auth package's TestMain was updated to use ### Q: What's the total test count? -~1,642 tests run across all packages. 0 failures. +1,716 tests run across all packages (4 skipped). 0 failures. **Notable new tests:** Centralised job scheduler tests (3 — RegisterAll count, schedules, handler signatures), scheduled-cleanup handler tests (21 — NotifyUnpaidOneWeek/Month, TransitionDiscountCampaigns, CleanupExpiredVerificationCodes/RefreshTokens), GDPR export cache cleanup (4), stale login entry cleanup (4), rate limiter cleanup tests (6), rate limiter production behavior tests (6). 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`). New closing_time tests (3), content-type middleware tests (2), clock package tests, expanded admin reserve overlap tests, expanded gift card buy flow tests with VAT, and full admin reservation cancel coverage (12 tests covering walkin + callin + isolation + no-op + idempotency + response format parity).