diff --git a/.sisyphus/coverage-report.md b/.sisyphus/coverage-report.md deleted file mode 100644 index b12aa09..0000000 --- a/.sisyphus/coverage-report.md +++ /dev/null @@ -1,190 +0,0 @@ -# Test Coverage Improvement Report - -**Date**: 2026-07-10 -**Overall Coverage**: 50.4% of statements -**Test Command**: `go test -tags "test,dev" -coverprofile=coverage.out -covermode=atomic ./...` - ---- - -## Coverage by Package - -| Package | Coverage | Status | -|---------|----------|--------| -| `handlers/webhooks` | 100.0% | ✅ | -| `internal/images` | 100.0% | ✅ | -| `internal/jobs` | 93.5% | ✅ | -| `mw` | 89.3% | ✅ (small gaps) | -| `internal/validators` | 82.9% | ✅ | -| `clock` | 80.0% | ✅ | -| `internal/square` | 74.4% | ✅ | -| `handlers/scheduling` | 73.1% | ✅ | -| `handlers/notifications` | 71.5% | ✅ | -| `handlers/admin` | 68.6% | ✅ | -| `handlers/auth` | 66.0% | ✅ | -| `db` | 66.7% | ✅ | -| `handlers/today` | 64.5% | ✅ | -| `handlers/payments` | 55.4% | 🟡 Moderate | -| `handlers/bookings` | 47.7% | 🟡 Low | -| `handlers/portfolio` | 48.9% | 🟡 Low | -| `auth` | 45.0% | 🟡 Low | -| `handlers/user` | 39.1% | 🔴 Very Low | -| `handlers/services` | 30.3% | 🔴 Very Low | -| `crussell (root)` | 5.4% | 🔴 Critical | -| `internal/dav` | 0.0% | ⚫ Zero | -| `internal/logutil` | 0.0% | ⚫ Zero | -| `internal/s3` | 0.0% | ⚫ Zero | -| `internal/zxcvbnjs` | 0.0% | ⚫ Zero | - ---- - -## Critical Finding: Cross-Package Coverage Blind Spot - -**Several functions show 0% in their own package but HAVE tests in the `admin` package.** Go's per-package coverage (`go test ./handlers/services/`) only counts tests within that package. Cross-package test calls (from `handlers/admin/`) don't count. - -Affected handlers (all tested in `handlers/admin/` but show 0% in their own package): - -| Function | Actual Coverage | Where Tested | -|----------|----------------|-------------| -| `handlers/services`: ToggleService, CreateServiceHandler, DeleteServiceHandler, AllServicesHandler | ✅ Tested | `admin/services_test.go` | -| `handlers/user`: GetAdminUserHandler, ListAdminUsersHandler, GetEligiblePatchTestServicesHandler, AddPatchTestHandler | ✅ Tested | `admin/users_test.go` | -| `handlers/bookings`: GetAllAdminBookingsHandler, GetAdminBookingHandler, GetAllBookingsByUserHandler, SearchAdminBookingsHandler, etc. | ❌ **Untested** | No admin tests written | -| `handlers/bookings`: AdminListPendingBookingsHandler, AdminGetInProgressBookingHandler | ❌ **Dead code** | No routes registered in main.go | - ---- - -## Priority 1: Immediate Wins (<30 min each, no new dependencies) - -### 1.1 `internal/logutil/logutil_test.go` — 70 lines, pure stdlib -**Functions**: `ColoredDuration(d time.Duration) string`, `ColoredRows(n int) string` -**What to test**: -- `ColoredDuration`: 3 branches (<500ms → green, <5s → yellow, >=5s → red) -- `ColoredRows`: singular (n=1) vs plural (n≠1) -- `NO_COLOR` env var behavior (ANSI codes vs empty strings) -**Lines added**: ~40 - -### 1.2 `internal/zxcvbnjs/zxcvbn_test.go` — 68 Go lines, 1 public function -**Function**: `Score(password string) (int, error)` -**What to test**: -- Known weak passwords (`"password"`, `"123456"`) → score 0-1 -- Strong passwords → score 3-4 -- Empty string → handle gracefully -- `sync.Once` lazy init works across repeated calls -**Lines added**: ~30 - -### 1.3 `auth/jwt_test.go` — Refresh token functions (already have handler-level tests) -**Functions**: `generateRefreshTokenString()`, `GenerateRefreshToken(ctx, userID, role)`, `VerifyRefreshToken(ctx, tokenString)` -**What to test**: -- `generateRefreshTokenString`: format (64-char hex) + uniqueness (100 calls) -- `GenerateRefreshToken`: stores in DB, returns non-empty token -- `VerifyRefreshToken`: verify + token rotation (2nd call fails), expired, revoked -**Pattern**: Already works in `handlers/auth/auth_test.go:TestRefreshToken_Generation` — adapt for package-level -**Test infra**: `auth/testmain_test.go` already sets up `InitJWT` + test DB pool — no setup needed - -### 1.4 `handlers/bookings/repo_test.go` — 4 trivial DB helpers (~5 lines each) -**Functions**: `GetBookingStatus`, `GetBookingStartTime`, `BookingExists`, `CountUserBookingsInStatus` -**What to test**: -- Happy path: insert booking → call function → assert result -- Not-found: non-existent ID → assert error -**Pattern**: `ctx, tx := testutils.SetupTestTx(t)` → `fixtures.CreateTestUser(tx)` → `fixtures.CreateTestService(tx)` → `fixtures.CreateTestBooking(tx, userID, svcID)` → call function directly - -### 1.5 `mw/response_test.go` — Trivial helpers -**Functions**: `RespondJSON(w, status, data)`, `RespondError(w, status, message)` -**What to test**: -- `RespondJSON`: basic write, error encoding, nil data -- `RespondError`: confirm JSON shape `{"error": "..."}` -- **Action**: `RespondError` is dead code (never called anywhere) — either remove it or test + start using it -**Ease**: 5-line pure functions, `httptest.ResponseRecorder` + standard assertions - -### 1.6 `internal/dav/types_test.go` — Pure string builders -**Functions**: `GenerateICalEvent(EventInput) string`, `GenerateVCard(ContactInput) string` -**What to test**: -- All-day vs timed events, attendees list, special characters (iCal escaping) -- vCard formatting with various field combinations -- Edge cases: empty fields, long strings -**No DB needed**: Pure functions, no dependencies - ---- - -## Priority 2: Medium Effort (follow existing test patterns, 1-2h each) - -### 2.1 `handlers/services/services_test.go` — Jump 30% → ~90% -**What**: Move/add in-package tests for `ToggleService`, `CreateServiceHandler`, `DeleteServiceHandler`, `AllServicesHandler` -**Why**: These are already tested in `handlers/admin/services_test.go` — just need to replicate in the `services` package -**Pattern**: Add helper `makeAdminContextRequest` (5 lines setting `mw.UserIDKey` + `mw.UserRoleKey` on chi context like `admin/test_helpers.go`) -**Bonus**: Remove dead code on lines 200-208 of `services.go` (unreachable after unconditional `return`) - -### 2.2 `handlers/user/*_test.go` — Jump 39% → ~55% -**What**: Add in-package tests for `GetAdminUserHandler`, `ListAdminUsersHandler`, `GetEligiblePatchTestServicesHandler`, `AddPatchTestHandler` -**Why**: Same cross-package issue — tested in `admin/users_test.go` -**Also add**: -- `CreateGuestUserHandler` success path (currently only validation failure tests) -- `processProfileImage` unit test with a real JPEG file in `testdata/` -- `GetAdminUserHandler` error paths (not-found, social login query error) - -### 2.3 `handlers/bookings/admin_*_test.go` — Admin GET handler tests -**What**: Add HTTP handler tests for uncovered admin GET handlers in `bookings.go`: -- `GetAllAdminBookingsHandler`, `GetAdminBookingHandler`, `GetAllBookingsByUserHandler` -- `SearchAdminBookingsHandler`, `GetOverlappingBookings*`, `GetBookingsBy*Range*` -**Pattern**: `serveChiHandler` with manual admin context injection (already used in `overlap_test.go` and `admin_reserve_test.go`) -**Verify**: `AdminListPendingBookingsHandler` and `AdminGetInProgressBookingHandler` — confirmed unreachable (no routes). Either remove or add routes + tests. - -### 2.4 `mw/ratelimit_test.go` — Middleware wrapper tests -**Functions** (dev stubs): `ProgressiveRateLimit`, `RateLimit` -**What to test**: Wrap handler with middleware in dev mode, verify pass-through behavior -**Pattern**: `httptest.NewRecorder` + `httptest.NewRequest` + standard HTTP test - ---- - -## Priority 3: Heavier Effort (needs mocks or refactoring, 2-4h each) - -### 3.1 `handlers/portfolio/images_test.go` — UploadImage (currently 7.5%) -**Blockers**: `s3.Client` is nil in tests → handler returns "Storage not configured" early -**Fix**: Mock `s3.Uploader` interface (already exists), set `s3.Client = &mockUploader{}` in test setup -**What to test**: Multi-part form with JPEG data, verify 200, verify DB record inserted with correct URLs -**Also**: `ListFilters` 31.7% — add tests for `?filter[category]=value` (dual-query path), `?tag=forest`, combined filters - -### 3.2 `handlers/user/profile_test.go` — UploadProfilePictureHandler (8.8%) -**Blocker**: Same S3 nil issue as portfolio -**Fix**: Same mock approach — `s3.Client = &mockUploader{}` -**Also**: `updateCardDAV` (11.1%) — currently requires `DAV_BASE_URL` env var and HTTP server. Mock the function or use `httptest.NewServer`. - -### 3.3 `internal/dav/shared_test.go` — DB-backed CardDAV operations -**Functions**: 14 `BaseService` methods (CRUD for calendars, contacts, events) -**Requires**: PostgreSQL test DB pool (existing pattern in `testutils/testdb/`) -**Pattern**: Create test fixtures (calendar, contact records), call methods, assert DB state -**Note**: Importing `dav` package triggers `service_dev.go`'s `init()` which tries to connect to Postgres. Either set `GO_TESTING` env var or construct `BaseService` directly. - -### 3.4 `internal/s3/s3_test.go` — S3 storage -**Approach**: -- Unit tests via `Uploader` interface mock -- `Connect()` integration test requires local S3 (RustFS/MinIO) → tag `//go:build integration` -- `GetURL` URL-builder logic can be tested independently (currently 0% on both build tags) - ---- - -## Dead Code Found During Investigation - -| Location | Function | Status | -|----------|----------|--------| -| `mw/response.go:19` | `RespondError` | **Defined but NEVER called** — remove or wire up | -| `bookings/manage.go:309` | `AdminListPendingBookingsHandler` | **No route registered** in main.go | -| `bookings/manage.go:320` | `AdminGetInProgressBookingHandler` | **No route registered** in main.go | -| `services/services.go:200-208` | Duplicate-key error block | **Unreachable** — after unconditional `return` on line 198 | - ---- - -## Summary: Coverage Impact by Action - -| Action | Estimated Coverage Gain | -|--------|----------------------| -| Fix cross-package gap (services) | +15-20% in `handlers/services` | -| Fix cross-package gap (user admin handlers) | +10-15% in `handlers/user` | -| Add repo.go tests | +2-3% in `handlers/bookings` | -| Add admin booking GET handler tests | +15-20% in `handlers/bookings` | -| Auth refresh token tests | +15-20% in `auth` | -| logutil + zxcvbnjs tests | 0% → 80-100% in those packages | -| RespondJSON test | 0% → 100% in `mw/response.go` | -| Portfolio S3 mock + tests | 7.5% → 60-70% in `handlers/portfolio` | -| dav/types pure func tests | 0% → ~25% in `internal/dav` | -| Remove dead code | Removes false-negative 0% entries | -| **Total estimated improvement** | **~50% → ~65-70% overall** | diff --git a/.sisyphus/plans/cron-jobs-package.md b/.sisyphus/plans/cron-jobs-package.md deleted file mode 100644 index 9cacca1..0000000 --- a/.sisyphus/plans/cron-jobs-package.md +++ /dev/null @@ -1,431 +0,0 @@ -# Plan: Consolidated `jobs` Package for Background Cron Tasks - -**Goal:** Extract 9 side-effect cleanup functions from `GetAvailableHours` HTTP handler into a dedicated `jobs` package with cron-scheduled, parallel execution. Remove the lazy-cleanup antipattern. - -**Current state:** -- 9 cleanup functions in `backend/handlers/scheduling/time-blockers.go` — all called synchronously in `default-hours.go:364-407` on every `GET /available-hours` request -- Only `CleanupOldReservations` ALSO runs on a background goroutine (`main.go:432-450`, 5min ticker) -- 4 other ad-hoc background goroutines exist (JWT cleanup, GDPR cache, login state, rate limiter) -- `robfig/cron/v3` v3.0.1 is already a `go.mod` dependency (currently used only for parsing time-blocker cron expressions) - ---- - -## Phase 1: Create `backend/internal/jobs/` Package - -**New package:** `crussell/internal/jobs` - -### File: `scheduler.go` - -Core scheduler abstraction built on `robfig/cron/v3`: - -```go -package jobs - -import ( - "context" - "log" - "time" - "github.com/robfig/cron/v3" -) - -// Job defines a single periodic task. -type Job struct { - Name string // Human-readable name (for logging) - Schedule string // Standard cron expression ("*/5 * * * *") - Timeout time.Duration // Per-execution timeout - Concurrency int // Max concurrent runs (0 = unlimited, 1 = serial) - Handler func(context.Context) error // The actual work -} - -// Scheduler manages all registered cron jobs with parallel execution. -type Scheduler struct { - cron *cron.Cron - entries []cron.EntryID - registry []Job - baseCtx context.Context - cancel context.CancelFunc - semaphores map[string]chan struct{} // Per-job concurrency limit -} -``` - -**Key design decisions:** -1. Each job runs in its own goroutine (cron v3 default) — parallel by nature -2. Per-job concurrency control via channel semaphore — prevents overlapping runs of the same job -3. Panic recovery wrapper — matches existing pattern in `jwt.go`, `gdpr_export.go`, `auth/local.go` -4. Timeout via `context.WithTimeout` — matches existing `main.go` pattern -5. Graceful shutdown via `baseCtx` — the cron scheduler is stopped, then active jobs drain - -```go -func New() *Scheduler { - ctx, cancel := context.WithCancel(context.Background()) - return &Scheduler{ - cron: cron.New(cron.WithLocation(londonLocation)), - baseCtx: ctx, - cancel: cancel, - semaphores: make(map[string]chan struct{}), - } -} - -func (s *Scheduler) Register(job Job) { - // Validate cron expression at registration time - parser := cron.NewParser(cron.Minute | cron.Hour | cron.Dom | cron.Month | cron.Dow) - if _, err := parser.Parse(job.Schedule); err != nil { - log.Fatalf("jobs: invalid cron schedule %q for job %q: %v", job.Schedule, job.Name, err) - } - s.registry = append(s.registry, job) -} - -func (s *Scheduler) Start() { - for _, job := range s.registry { - j := job // capture - entryID, err := s.cron.AddFunc(j.Schedule, s.wrapJob(j)) - if err != nil { - log.Fatalf("jobs: failed to register %q: %v", j.Name, err) - } - s.entries = append(s.entries, entryID) - } - s.cron.Start() -} - -func (s *Scheduler) Shutdown() <-chan struct{} { - ctx := s.cron.Stop() // Stop scheduler (returns ctx that completes when all jobs finish) - s.cancel() // Cancel base context so in-flight jobs know to stop - return ctx.Done() // Caller can wait for this -} - -// wrapJob adds panic recovery, timeout, concurrency control, and logging. -func (s *Scheduler) wrapJob(job Job) func() { - // Set up semaphore if concurrency limited - var sem chan struct{} - if job.Concurrency > 0 { - sem = make(chan struct{}, job.Concurrency) - sem <- struct{}{} // Initial slot filled - } - - return func() { - // Concurrency guard - if sem != nil { - select { - case <-sem: - // Acquired — proceed - default: - log.Printf("jobs: %q skipped (previous run still in progress)", job.Name) - return - } - defer func() { sem <- struct{}{} }() - } - - // Panic recovery - defer func() { - if r := recover(); r != nil { - log.Printf("jobs: panic recovered in %q: %v", job.Name, r) - } - }() - - // Timeout - ctx, cancel := context.WithTimeout(s.baseCtx, job.Timeout) - defer cancel() - - start := time.Now() - if err := job.Handler(ctx); err != nil { - log.Printf("jobs: %q failed: %v (duration: %v)", job.Name, err, time.Since(start)) - } else { - log.Printf("jobs: %q completed (duration: %v)", job.Name, time.Since(start)) - } - } -} -``` - -### File: `cleanup.go` - -Registration of all 9 scheduling cleanup functions + JWT cleanup. - -```go -package jobs - -import ( - "context" - "crussell/auth" - "crussell/handlers/scheduling" - "time" -) - -// RegisterAll registers every background job. -func RegisterAll(s *Scheduler) { - // === HIGH FREQUENCY (every 5 min) === - - s.Register(Job{ - Name: "cleanup-reservations", - Schedule: "*/5 * * * *", - Timeout: 30 * time.Second, - Concurrency: 1, - Handler: scheduling.CleanupOldReservations, - }) - - s.Register(Job{ - Name: "cleanup-expired-deposits", - Schedule: "*/5 * * * *", - Timeout: 30 * time.Second, - Concurrency: 1, - Handler: scheduling.CleanupExpiredDeposits, - }) - - // === MID FREQUENCY (hourly) === - - s.Register(Job{ - Name: "cleanup-expired-loyalty-redemptions", - Schedule: "0 * * * *", - Timeout: 30 * time.Second, - Concurrency: 1, - Handler: scheduling.CleanupExpiredLoyaltyRedemptions, - }) - - s.Register(Job{ - Name: "cleanup-old-idempotency-keys", - Schedule: "0 * * * *", - Timeout: 30 * time.Second, - Concurrency: 1, - Handler: scheduling.CleanupOldIdempotencyKeys, - }) - - s.Register(Job{ - Name: "cleanup-revoked-jtis", - Schedule: "0 * * * *", - Timeout: 30 * time.Second, - Concurrency: 1, - Handler: func(ctx context.Context) error { - auth.CleanupRevokedJTIs(ctx) - return nil - }, - }) - - // === LOW FREQUENCY (daily, off-peak) === - - s.Register(Job{ - Name: "anonymize-stale-guest-accounts", - Schedule: "0 3 * * *", // 3am - Timeout: 5 * time.Minute, - Concurrency: 1, - Handler: scheduling.AnonymizeStaleGuestAccounts, - }) - - s.Register(Job{ - Name: "cleanup-expired-financial-records", - Schedule: "0 4 * * *", // 4am - Timeout: 10 * time.Minute, - Concurrency: 1, - Handler: scheduling.CleanupExpiredFinancialRecords, - }) - - s.Register(Job{ - Name: "cleanup-expired-gift-cards", - Schedule: "0 5 * * *", // 5am - Timeout: 5 * time.Minute, - Concurrency: 1, - Handler: scheduling.CleanupExpiredGiftCards, - }) - - s.Register(Job{ - Name: "cleanup-idle-accounts", - Schedule: "30 3 * * *", // 3:30am (offset from financial) - Timeout: 5 * time.Minute, - Concurrency: 1, - Handler: scheduling.CleanupIdleAccounts, - }) - - s.Register(Job{ - Name: "cleanup-old-name-history", - Schedule: "30 4 * * *", // 4:30am - Timeout: 30 * time.Second, - Concurrency: 1, - Handler: scheduling.CleanupOldNameHistory, - }) -} -``` - -**Staggering rationale:** Daily jobs at 3am/3:30am/4am/4:30am/5am spread the load so heavy operations (financial aggregation, idle account scans) don't contend with each other or with the hourly jobs. High-frequency jobs (reservations, deposits) are decoupled on their own 5-min schedules. - ---- - -## Phase 2: Wire into `main.go` - -### Before: -```go -// main.go:142 -auth.StartJTICleanup() - -// main.go:432-450 (background goroutine for reservations) -cleanupCtx, cleanupStop := context.WithCancel(context.Background()) -go func() { - ticker := time.NewTicker(reservationCleanupInterval) - ... -}() - -// main.go:457-468 (shutdown) -cleanupStop() -``` - -### After: -```go -// At package level, after constants but before init() -var sched *jobs.Scheduler - -// In main(), after initDB()/initSquare()/etc but before r := chi.NewRouter(): -sched = jobs.New() -jobs.RegisterAll(sched) -sched.Start() - -// In main(), shutdown block (replace the old cleanupStop() call): -quit := make(chan os.Signal, 1) -signal.Notify(quit, syscall.SIGTERM, syscall.SIGINT) -go func() { - <-quit - log.Println("Shutting down server...") - - // Graceful HTTP shutdown - ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) - defer cancel() - if err := srv.Shutdown(ctx); err != nil { - log.Printf("Server forced to shutdown: %v", err) - } - - // Graceful job scheduler shutdown - <-sched.Shutdown() - log.Println("Background jobs stopped") -}() -``` - -**Remove:** -- `reservationCleanupInterval` and `reservationCleanupTimeout` constants (`main.go:71-72`) -- `cleanupCtx`/`cleanupStop` variables (`main.go:433`) -- The goroutine block (`main.go:434-450`) -- Import `scheduling` from `main.go` (no longer needed there) -- `auth.StartJTICleanup()` call (`main.go:142`) - ---- - -## Phase 3: Remove Side-Effects from `GetAvailableHours` - -**File: `backend/handlers/scheduling/default-hours.go`** - -Delete lines 364-407 (the entire cleanup block): - -```diff -- // Clean up old reservations (older than 1 hour) -- if err := CleanupOldReservations(r.Context()); err != nil { -- log.Printf("Failed to cleanup old reservations: %v", err) -- } -- -- // Anonymize stale guest accounts (6+ months after last booking) -- if err := AnonymizeStaleGuestAccounts(r.Context()); err != nil { -- log.Printf("Failed to anonymize stale guest accounts: %v", err) -- } -- -- // Clean up expired loyalty redemptions (pending past expires_at) -- if err := CleanupExpiredLoyaltyRedemptions(r.Context()); err != nil { -- log.Printf("Failed to cleanup expired loyalty redemptions: %v", err) -- } -- -- // Clean up expired financial records (aggregate + delete granular data) -- if err := CleanupExpiredFinancialRecords(r.Context()); err != nil { -- log.Printf("Failed to cleanup expired financial records: %v", err) -- } -- -- // Clean up bookings past deposit deadline (no deposit paid) -- if err := CleanupExpiredDeposits(r.Context()); err != nil { -- log.Printf("Failed to cleanup expired deposits: %v", err) -- } -- -- // Clean up expired gift cards (unused for 24+ months) -- if err := CleanupExpiredGiftCards(r.Context()); err != nil { -- log.Printf("Failed to cleanup expired gift cards: %v", err) -- } -- -- // Clean up idle accounts (2yr no money, 5yr with money) -- if err := CleanupIdleAccounts(r.Context()); err != nil { -- log.Printf("Failed to cleanup idle accounts: %v", err) -- } -- -- // Clean up old idempotency keys (24h+ and non-pending) -- if err := CleanupOldIdempotencyKeys(r.Context()); err != nil { -- log.Printf("Failed to cleanup old idempotency keys: %v", err) -- } -- -- // Clean up old name history (6+ months) -- if err := CleanupOldNameHistory(r.Context()); err != nil { -- log.Printf("Failed to cleanup old name history: %v", err) -- } -``` - -Also remove `"log"` import from `default-hours.go` if it becomes unused after removal. - ---- - -## Phase 4: Remove `auth.StartJTICleanup()` (Optional but Recommended) - -**File: `backend/auth/jwt.go`** - -- Keep `CleanupRevokedJTIs(ctx)` — it's used by tests and now by the jobs package -- Delete `StartJTICleanup()` entirely (no longer needed, replaced by the jobs package) -- Remove the ticker goroutine - ---- - -## Phase 5: Update Documentation - -**File: `obsidian/Crussell/Technical Manual.md`** -- Update lines ~545 and ~999: change "lazy cleanup triggered on availability fetch" to "all cleanup runs on cron schedules via the `jobs` package" -- Remove the lazy-cleanup justification -- Add a new section documenting the `jobs` package and its job registry - -**File: `obsidian/Crussell/Future Work - Gap Backlog.md`** -- Mark item #2 as fully complete (strikethrough) -- Remove the note about remaining functions still running on `GET /api/availability` - -**File: `README.md`** (optional) -- Add a line about the background job scheduler - ---- - -## Effort Estimate - -| Step | Files Changed | Effort | -|------|--------------|--------| -| Phase 1a: `scheduler.go` | 1 new file | 2-3h | -| Phase 1b: `cleanup.go` | 1 new file | 1h | -| Phase 2: Wire `main.go` | 1 file | 30min | -| Phase 3: Remove side-effects from handler | 1 file | 15min | -| Phase 4: Remove `StartJTICleanup` | 1 file | 15min | -| Phase 5: Update docs | 2-3 files | 30min | -| Testing & go vet | — | 1h | -| **Total** | **4-5 files (+2 new)** | **~6h** | - ---- - -## Risks & Mitigations - -| Risk | Mitigation | -|------|------------| -| **Concurrent DB load** from multiple jobs at the same cron tick | Stagger daily jobs across 3am-5am range. High-frequency jobs (5min) are lightweight. Per-job `Concurrency: 1` prevents overlapping runs. | -| **Job takes longer than interval** (e.g., financial aggregation >5min while scheduled every 5min) | `Concurrency: 1` + skip-logic: if previous run still in-flight, the new invocation is skipped and logged. | -| **Cron expression parsing differs** from existing time-blocker parser | Both use `cron.NewParser(cron.Minute \| cron.Hour \| cron.Dom \| cron.Month \| cron.Dow)` — identical. | -| **Tests expect cleanup side-effects** from `GetAvailableHours` | Search for tests that rely on the side-effect calls. Some may need an explicit cleanup call before assertions. | -| **`CleanupExpiredDeposits` timing gap** — deposit-lapsed slots not freed until next cron tick | 5-min frequency is acceptable. The previous behavior freed on the *next* availability fetch, which could be minutes or hours apart depending on user activity. 5-min max latency is actually *better* than the lazy approach during quiet periods. | -| **Panic in one job takes down the scheduler** | `wrapJob` has defer-recover. One job's panic cannot affect others (separate goroutines). | - ---- - -## Edge Cases & Exclusions - -**Not in scope (Phase 1):** -- `gdpr_export.go:init()` — GDPR cache cleanup (5min ticker). Tightly coupled to package-level state. Leave as-is. -- `auth/local.go:init()` — Login state cleanup. Tightly coupled. Leave as-is. -- `mw/ratelimit.go` goroutines — Rate limiter cleanup. Leave as-is. -- These can be migrated to the scheduler in a follow-up if desired, but require more refactoring. - -**Not a concern:** -- `backend/handlers/scheduling/time-blockers.go` already uses `robfig/cron/v3` — no new dependency, no version conflict. -- All 9 cleanup functions have signature `func(context.Context) error` — perfectly uniform for the `Job.Handler` type. -- The `scheduling` package is already imported in `main.go` — no new import needed for that. - -**Key invariant:** All cleanup functions are idempotent (documented in code). Running them on cron instead of on-demand has zero correctness impact — they produce the same result regardless of how often they run. diff --git a/.sisyphus/plans/holiday-hours-conflict-resolution.md b/.sisyphus/plans/holiday-hours-conflict-resolution.md deleted file mode 100644 index 750fff0..0000000 --- a/.sisyphus/plans/holiday-hours-conflict-resolution.md +++ /dev/null @@ -1,397 +0,0 @@ -# Holiday Hours Conflict Resolution Plan - -## Overview - -Add a conflict resolution flow to the "Create Exception Schedule" modal (similar to the Time Blocker flow) that prevents saving holiday hours until all affected client bookings have been moved out of the newly-closed/rescheduled time periods. - ---- - -## Key Differences from Time Blocker Flow - -| Aspect | Time Blockers | Holiday Hours | -|---|---|---| -| Scope | Single day, specific time range | Entire weeks, full daily schedules | -| Available-for-reschedule logic | Same working hours apply (no change) | **Dual mode**: For same-week affected days → use **new holiday hours**; for other weeks → use **default hours** | -| Bypass mechanism | N/A (blocker is hard block) | `out_of_hours` flag on admin booking create/reserve | -| Existing conflict check | Client-side overlap against one date | Need per-week overlap against up to N weeks | -| Placeholder blocker pattern | `RESERVATION:placeholder:{bookingId}` | New pattern needed (likely holiday-specific placeholder) | - ---- - -## Files to Modify - -### Frontend -1. **`frontend/src/lib/components/admin/HolidayHours.svelte`** — Add conflict detection UI -2. **`frontend/src/routes/admin/+page.svelte`** — Wire `openUserModal`, `openBookingModal`, `rescheduleVersion` props to `HolidayHours` - -### Backend -3. **`backend/handlers/scheduling/exceptional-hours.go`** — Add conflict-checking API or expand existing -4. **`backend/handlers/scheduling/default-hours.go`** — Add "preview" mode to GetAvailableHours that accepts proposed exceptional hours for conflict-specific available-slot preview -5. **`backend/main.go`** — Register new route(s) - ---- - -## Frontend Design - -### A. Props Interface (HolidayHours.svelte) - -```typescript -interface Props { - openUserModal?: (userId: string) => void; - openBookingModal?: (bookingId: string) => void; - rescheduleVersion?: number; -} -``` - -Wire these from `admin/+page.svelte`: -```svelte - -``` - -### B. New State Variables - -```typescript -let overlappingBookings = $state([]); -let checkingOverlap = $state(false); -let hasOverlap = $state(false); -let conflictWeeks = $state>(new Map()); - -type OverlappingBooking = { - id: string; - start_time: string; - duration_minutes: number; - status: string; - user: { id: string; full_name: string; email: string | null; phone: string | null } | null; - services: string[]; - week_start: string; // Which Monday this booking falls in - day_of_week: number; // 0=Mon..6=Sun - is_same_week_as_change: boolean; // Whether this week is being modified -}; -``` - -### C. Guard Condition - -```typescript -const canSave = $derived.by(() => { - return isFormValid && !hasOverlap && !checkingOverlap; -}); -``` - -### D. Conflict Checking Flow - -When admin opens the modal or changes the week selection: - -1. **Collect all affected dates**: For each week_start in `weekStarts`, expand to the 7 days (Mon-Sun). Figure out which days have changes (different hours from default, or newly closed). - -2. **Fetch conflicting bookings**: Call a new backend endpoint: - ``` - GET /api/admin/bookings/conflicting-for-exception - ?week_starts[]=2026-08-03&week_starts[]=2026-08-10 - &proposed_hours=[{weekday:1,start:"10:00",end:"18:00",isOpen:true},...] - ``` - - This returns ONLY bookings that fall within the affected hours (e.g., if Mon is now closed, all Monday bookings in that week conflict; if Mon hours changed from 9-5 to 12-8, bookings at 9am-12pm conflict since they'd be in the newly-closed part). - -3. **Client-side display**: Group conflicting bookings by week, show in amber warning section with: - - Week label ("Week of 3 Aug 2026") - - For each conflicting booking: client name, time, services - - "View Booking" / "View Client" buttons - -### E. Available-For-Reschedule Display - -For each conflicting booking, the admin needs to know where they **can** reschedule to. This requires a **preview endpoint** that shows available hours **as-if** the holiday hours were already applied: - -- If the booking is in a week being modified by this exception → show available hours **using the new proposed holiday hours** (the hours that will apply once saved) -- If the booking is in any other week → show available hours **using the existing working hours** (default + any already-applied exceptions) - -Backend endpoint: -``` -GET /api/scheduling/preview-available-hours - ?start=2026-08-03&end=2026-08-10 - &proposed_hours=[{weekday:1,start:"10:00",end:"18:00",isOpen:true},...] - &proposed_weeks=["2026-08-03"] - // proposed_hours + proposed_weeks simulate the new exception group -``` - -This is like `GetAvailableHours` but overrides the working hours resolution with the proposed exception for the specified weeks. - -### F. Save Flow Changes - -When admin clicks "Create Schedule": - -1. Check if conflicts exist → prevent save -2. Admin must click "View Booking" to cancel/reschedule each conflicting booking -3. After each booking is resolved, re-check conflicts -4. When no conflicts remain → "Create Schedule" button enables -5. On save: Optionally create placeholder blockers for the conflicting bookings (similar to `RESERVATION:placeholder:` pattern but adapted for holiday hours). These would block the affected SLOTS (not the full week) to prevent re-booking during the transition. - -**Placeholder strategy**: For each affected booking time, create a `RESERVATION:holiday_placeholder:{bookingId}:{weekStart}` entry in time_blockers with TTL = 24 hours (enough for the resolution process). - ---- - -## Backend Design - -### A. New Endpoint: `GET /api/admin/bookings/conflicting-for-exception` - -Purpose: Return all bookings that would conflict with a proposed exception group. - -Input: -``` -week_starts: string[] // Mondays of affected weeks (YYYY-MM-DD) -proposed_hours: [{ - weekday: number // 0=Mon..6=Sun - startTime: string // HH:MM - endTime: string // HH:MM - isOpen: boolean -}] -``` - -Logic: -1. For each week_start, expand to 7 days (Mon-Sun) -2. For each day, look up the proposed hours for that weekday -3. Query bookings table for bookings on those dates -4. Filter bookings that fall OUTSIDE the proposed open hours (if the day is closed → all bookings conflict; if hours changed → bookings before new start or after new end conflict) -5. Return filtered bookings grouped by week_start with user info - -SQL approach: -```sql -SELECT b.id, b.start_time, b.total_duration_minutes as duration, b.status, - u.id as user_id, u.fn as full_name, u.email, u.phone -FROM bookings b -LEFT JOIN users u ON b.user_id = u.id -WHERE b.status NOT IN ('completed','client_cancelled','we_cancelled','no_show','deposit_lapsed') - AND b.start_time >= $1 -- range start - AND b.start_time < $2 -- range end - AND ( - (b.start_time::time < $3) -- starts before new opening time - OR - (b.start_time::time + (b.total_duration_minutes || ' minutes')::interval > $4) -- ends after new closing - ) -ORDER BY b.start_time -``` - -### B. New/Modified Endpoint: `GET /api/scheduling/preview-available-hours` - -Purpose: Show available hours as-if the proposed exception group were already applied. - -This is a modified version of `GetAvailableHours` that: -1. Accepts the same parameters (start, end) -2. Accepts additional `proposed_hours` and `proposed_weeks` parameters -3. For days in proposed_weeks, uses proposed_hours instead of actual exceptional hours -4. Otherwise, behaves identically to the current logic (including time blocker subtraction, late-night lock, etc.) - -### C. Admin Override Mechanism - -The existing `out_of_hours` flag on `AdminCreateBookingForUserRequest` and `AdminReserveSlotRequest` already exists. When an admin is rescheduling a booking during the conflict resolution flow: - -- The "available hours" preview from step B shows valid slots -- If the admin needs to place a booking outside those hours (e.g., there's no good time in the holiday hours), they can use the existing **admin booking creation** with `out_of_hours: true` to bypass working hours restrictions -- This is already supported in: - - `AdminCreateBookingForUserHandler` (manage.go:622) — skips exceptional-hours closed check when `out_of_hours=true` - - `AdminReserveSlotHandler` (admin_reserve.go:118) — skips closing-hours check when `out_of_hours=true` -- The frontend could expose this via a "Place outside working hours" checkbox on the booking modal (only visible during admin conflict resolution) - -### D. Time Blocker Integration - -The holiday hours placeholder pattern (`RESERVATION:holiday_placeholder:*`) should use the existing `CleanupOldReservations` mechanism with a custom TTL. Since these are conceptually similar to `RESERVATION:edit_request:*` (24h TTL), we can add a new TTL category: - -```go -// In CleanupOldReservations, add: -// RESERVATION:holiday_placeholder:* -> 24 hours -``` - ---- - -## Implementation Steps - -### Step 1: Backend — Add conflicting bookings endpoint - -1.1 Implement `GetConflictingBookingsForExceptionHandler` in `exceptional-hours.go` -1.2 Register route: `r.Get("/conflicting-for-exception", scheduling.GetConflictingBookingsForExceptionHandler)` in `main.go` -1.3 Handle the logic: iterate weeks → expand to days → join with proposed hours → find bookings outside new hours - -### Step 2: Backend — Add preview available hours endpoint - -2.1 Add `GetPreviewAvailableHours` in `default-hours.go` -2.2 Accept `proposed_hours` and `proposed_weeks` as query params (JSON-encoded arrays) -2.3 Override working hours resolution for days in proposed_weeks - -### Step 3: Frontend — Add conflict UI to HolidayHours.svelte - -3.1 Add `openUserModal`, `openBookingModal`, `rescheduleVersion` props -3.2 Add `checkConflictingBookings()` function -3.3 Add amber warning section for conflicts (reuse pattern from TimeBlockers.svelte) -3.4 Wire `canSave` guard to `hasOverlap` -3.5 Group conflicts by week_start for clarity - -### Step 4: Frontend — Wire props in admin/+page.svelte - -4.1 Pass `openUserModal`, `openBookingModal`, `rescheduleVersion` to `` - -### Step 5: Backend — Add placeholder cleanup category - -5.1 Add `RESERVATION:holiday_placeholder:*` → 24 hours in `CleanupOldReservations` - -### Step 6: Edge Cases - -6.1 **Existing exception groups stacking**: If multiple exception groups already apply, the new proposed hours should override them for the preview (the newest exception wins) -6.2 **Partial day changes**: If a day goes from 9-5 to 12-8, only bookings starting before 12pm conflict -6.3 **Day closing entirely**: All bookings on that day conflict -6.4 **Week already has an exception**: The proposed exception replaces it entirely for those weeks — check against the new hours, not the current exception -6.5 **Admin override flow**: When admin clicks "View Booking", the booking modal should let them reschedule with `out_of_hours` option if needed (this may already work through admin booking creation) - ---- - -## Sequence Diagram (Text) - -``` -Admin opens "Create Exception Schedule" modal - → Selects weeks + sets per-day hours - → For each clicked change, calls checkConflictingBookings() - → GET /api/admin/bookings/conflicting-for-exception - ?week_starts=2026-08-03,2026-08-10 - &proposed_hours=[{...}] - → Returns list of conflicting bookings with user info - → Shows amber warning with grouped conflicts - → Admin clicks "View Booking" on a conflict - → BookingModal opens, admin cancels or reschedules - → BookingModal calls onReschedule callback - → Re-check conflicts - → When all conflicts resolved → "Create Schedule" enables - → Admin clicks "Create Schedule" - → Creates placeholder blockers for each resolved conflict - → POST /api/scheduling/exceptional-groups (existing) - → Placeholder blockers auto-cleanup after 24h -``` - ---- - -## Compatibility Assessment (from 5 parallel investigations) - -### 1. Timezone & DST Handling — SAFE with one watch - -**Architecture**: UTC-normalised. `clock.Now()` returns UTC. PostgreSQL runs with `timezone = "UTC"`. London conversion applied only where wall-clock rules matter. - -**BST midnight pattern** (critical): Every date-boundary query converts London-midnight to UTC using: -```go -startTime = time.Date(startLondon.Year(), startLondon.Month(), startLondon.Day(), 0, 0, 0, 0, londonLocation).UTC() -``` -This ensures a booking at 00:30 BST (23:30 UTC previous day) is included in the correct London date. Found in **9 locations**. The new endpoints MUST follow this pattern. - -**`expandCronOccurrences`** is DST-safe — extracts hour/minute from London time, pins occurrences to London wall-clock. - -**Watch**: Late-night lock (default-hours.go:594-611) uses `now.AddDate(0, 0, 1)` on **UTC** time, not London time, then formats the date string. If `now` is 22:30 BST (21:30 UTC), this is practically safe since 22:30 London doesn't cross a calendar boundary. But for the preview endpoint, replicate the London-midnight boundary pattern directly rather than copying the late-night lock's UTC date logic. - -### 2. Weekday Mapping — MUST MATCH EXACTLY - -**7 different conversion patterns exist.** The holiday hours code must use the **same patterns** as the existing booking handlers: - -**For Go time → DB weekday** (0=Monday): -```go -weekday := int((localStart.Weekday() + 6) % 7) -``` -This is used in all booking handlers (reserve.go, admin_reserve.go, bookings.go, manage.go). - -**For date → weekStart (Monday at UTC midnight)**: -```go -localStart := req.StartTime.In(londonLocation) -daysToMonday := int(localStart.Weekday()) -if daysToMonday == 0 { daysToMonday = 7 } -tm := localStart.AddDate(0, 0, -daysToMonday+1) -weekStart := time.Date(tm.Year(), tm.Month(), tm.Day(), 0, 0, 0, 0, time.UTC) -``` -Used in 4 booking handler locations. The UTC-midnight trick is essential — `tm` -has Location=London from `.In()`, so `.Year()/Month()/Day()` return London calendar values. `time.Date(..., time.UTC)` creates a UTC midnight at that London date, which pgx's DATE codec maps correctly regardless of BST/GMT. - -**Do NOT use** the `daysSinceMonday := int(d.Weekday()) - 1` pattern from `GetWorkingHours`/`GetAvailableHours` — that's used for date iteration loops, not individual booking lookups. - -### 3. Booking Statuses — Match time-blocker flow (includes `pending_release`) - -**For conflict detection**, use the same filter as the time-blocker flow: - -```sql -status NOT IN ('completed', 'client_cancelled', 'we_cancelled', 'no_show', 'deposit_lapsed') -``` - -This **includes `pending_release`** as a conflicting status. Rationale: `pending_release` represents a client who owes a deposit but hasn't paid on time. The system allows other users to book over the slot until payment is received (guaranteeing the slot for the business — if the original client is a no-show, another booking might fill it). However, if a holiday hours change is applied, the slot will be blocked entirely, defeating that safety mechanism. These clients should be contacted too — they may want to pay or choose a different time before the holiday hours take effect. - -Status breakdown: -- `pending` — active (needs admin contact) -- `confirmed` — active (needs admin contact) -- `in_progress` — active (might overlap with change) -- `pending_release` — active (client owes deposit, slot still reclaimable via payment) -- `completed` — excluded (already done) -- `client_cancelled`/`we_cancelled` — excluded (no client to contact) -- `no_show` — excluded (no active booking) -- `deposit_lapsed` — excluded (slot was already reclaimed by another booking) - -### 4. Overlap SQL Pattern — Use the overlap condition, not the date-range condition - -**Critical distinction**: -- `GetBookingsByDateRangeHandler` uses `start_time >= $1 AND start_time < $2` — this only finds bookings that **start** within the range. It misses bookings that started before but extend into it. -- Real overlap checks use `start_time < $2 AND end_time > $1` — this catches all bookings that occupy any part of the range. - -For holiday hours conflict detection, the holiday hours cover FULL DAYS (00:00-23:59 London). Active bookings on those days overlap by definition. The filter should find bookings whose time falls **OUTSIDE** the proposed new open window: - -```sql -WHERE status NOT IN ('client_cancelled','we_cancelled','no_show','pending_release','deposit_lapsed') - AND start_time < day_end_utc - AND end_time > day_start_utc - AND ( - start_time::time AT TIME ZONE 'Europe/London' < proposed_open -- starts before opening - OR - end_time::time AT TIME ZONE 'Europe/London' > proposed_close -- ends after closing - OR - proposed_is_open = false -- day is closed entirely - ) -``` - -### 5. GetAvailableHours Pipeline — Injection Point for Preview - -The `preview-available-hours` endpoint must mirror `GetAvailableHours` exactly, with proposed hours injected at step 7a: - -``` -Priority order (highest to lowest): - 1. Proposed holiday hours (new) <-- inject here - 2. Existing exceptional hours <-- existing `applied` check - 3. Default working_hours <-- existing `defaultMap` fallback - 4. Closed (00:00-00:00, isOpen=false) <-- existing fallback -``` - -Injection point is at `default-hours.go` lines 549-564 (inside the per-day loop). Add BEFORE the `if applied != nil` block: - -```go -if proposed, ok := proposedHours[day.Date]; ok { - baseStart = proposed.StartTime - baseEnd = proposed.EndTime - isOpen = proposed.IsOpen - day.Source = "proposed" -} else if applied != nil { - // existing exceptional hours check... -``` - -All downstream steps (booking subtraction, blocker subtraction, late-night lock, `out_of_hours` override) remain unchanged — they operate on the resolved `baseStart`/`baseEnd`/`isOpen` values. - -### Summary: No blockers found - -All 5 investigations confirmed the plan is compatible with the existing timezone, weekday mapping, status filtering, and slot calculation systems. The critical invariants to maintain are: - -1. **London-midnight → UTC conversion** for all date boundaries -2. **`(weekday + 6) % 7`** for Go→DB weekday mapping -3. **UTC-midnight weekStart** trick for exceptional hours lookups -4. **time-blocker status filter** (includes `pending_release`) for conflict detection -5. **Overlap condition** (`start_time < end AND end_time > start`) not date-range condition - ---- - -## Risk Assessment - -| Risk | Mitigation | -|---|---| -| Race condition: booking created while admin is resolving conflicts | Placeholder blockers prevent this (Step 5) | -| Admin creates overlapping exceptional hours across multiple groups | Each week_start is unique per group application (DB unique index on `exceptional_group_applications(week_start)`) | -| Large number of affected weeks overwhelms UI | Paginate conflicts, show week-by-week summary | -| Same-day bookings at BST midnight boundary fall in wrong date | Use existing London-midnight → UTC boundary pattern (not `GetBookingsByDateRange` approach) | -| Wrong weekday mapping at DST boundary | Use `(localStart.Weekday() + 6) % 7` + UTC-midnight weekStart trick (same as all booking handlers) | -| `pending_release` clients contacted unnecessarily | They owe a deposit — treating their booking as active is correct. If they don't pay, the slot would be evictable anyway, but contacting them ensures they have a chance to respond before holiday hours lock it | -| Late-night lock (22:00-11:00) uses UTC dates inconsistently | Preview endpoint should use London-midnight for all date comparisons, not UTC `AddDate` | diff --git a/.sisyphus/plans/payments-review-consolidated.md b/.sisyphus/plans/payments-review-consolidated.md deleted file mode 100644 index 0f8de05..0000000 --- a/.sisyphus/plans/payments-review-consolidated.md +++ /dev/null @@ -1,235 +0,0 @@ -# Payments Work Review — Consolidated Report (Round 1) - -**Scope:** commits `503c326..HEAD` (110 commits, ~45k insertions, 178 files) — replacing the dev-only Square mock with a realistic mock + real Square integration, plus hardening. - -**Review method:** 8 specialist agents (Square API contract, Square usage surface, UK consumer law, UK GDPR/PCI, mobile parity, testing gaps, pattern consistency, fix-wide-application) + 5 review-work agents (goal/constraint verification, QA execution, code quality, security, context mining). - ---- - -## Resolution Status (updated 13 Aug 2026) - -The payments overhaul is now complete (batch 1, 88 commits, code fixed). This section records the resolution status of every round-1 finding as verified in the batch-1 review, with spot-checks re-run against the current tree on 13 Aug 2026. The round-1 findings text below is left untouched. - -| ID | Status | Note | -|----|--------|------| -| C1 | RESOLVED | till-sale no-key fallback now derives a deterministic base key (`till-` + sha256) and reuses pending rows | -| C2 | RESOLVED | `ValidateAmount`/£10,000 cap applied on till sales and gift-card create/top-up/transfer | -| C3 | RESOLVED | `CancelGiftCard` / `AdminCancelGiftCard` implemented: in-app 14-day cancel, refund to original payment method, reg 34(9) partial-use handling | -| H1 | RESOLVED | `square_request_snapshot` scrubbed (`NULL`) in all erasure paths; comments/wording per operator intent | -| H2 | RESOLVED | `CleanupIdleAccounts` snapshots Square IDs pre-anonymize and disables cards + deletes the Square customer (with retry) | -| H3 | RESOLVED | Square-side deletion retried via scheduled job and surfaced on failure | -| H4 | RESOLVED | 2FA gate now also on BuyGiftCard SaveCard + CreatePaymentMethod | -| H5 | RESOLVED | env vars documented, checker fixed | -| M1 | RESOLVED | 16px font in Square hosted fields (no iOS auto-zoom) | -| M2 | RESOLVED | dialogs bottom-sheet/max-height on `` held across read-modify-write. -10. **TopUpGiftCard could top up an EXPIRED card** (reviving forfeited balance). **Fixed:** expiry gate added to admin top-up (till already had it). -11. **UserPaymentModal infinite fetch loop** — `$effect` re-triggered on store `loading` toggle → 983+ API calls. **Fixed:** one-shot guard + idempotent store. -12. **Money displayed 100× too small** — `formatCurrency(totalPaid / 100)` where totalPaid already pounds. **Fixed:** `formatCurrency(totalPaid)`. -13. **postChargeRecheck stranded money silently** — no refund row, no notification. **Fixed:** flood-capped critical notification raised. -14. **`payment.completed` orphan detection could fail a legit completed charge** (delayed webhook). **Fixed in two passes:** (a) `SweepKeyedReplayAge` age gate; (b) **B1-EVIDENCE GATE** (round 3): failed-mark now requires the sweep's actual B1 markers (`b1_attempts > 0` or a `duplicate charge — sweep replay` refund row); no evidence → origin left pending for the sweep to reconcile. New test locks the no-evidence path. - -### MEDIUM -15. **Online card SAVE unusable in enforced env** — gate matched only mock's fictional `cnon:sca-`. **Fixed:** genuineness derived from Square acceptance of any token-like source. -16. **Payment 2FA gate dead UI + fallback machinery** — frontend sent codes backend never read; `insertTwoFAFallbackAudit`/`reissueTwoFACodeAfterFailedCharge`/`enforceSCAFallbackConsent` unreachable. **Fixed:** dead machinery deleted, `verification_code` removed from charge structs, TwoFactorCodeInput removed from payment flows (account flows intact). -17. **Privacy policy false statements + Art 13 gaps** — "never store card expiry" (false: last_4/exp_month/exp_year stored), missing processors (Cloudflare/R2/CardDAV/Google Fonts/CARTO), no international transfers. **Fixed:** reworded + added sections. -18. **No right-to-cancel at gift-card purchase + self-purchase forfeits 14-day right without disclosure.** **Fixed:** disclosure + explicit acknowledgement checkbox + links to gift-card-terms. -19. **Terms page omitted Distance Contracts & Right to Cancel section.** **Fixed:** ported from obsidian §5. -20. **Business cancellation full-refund policy vs notice-tier code.** **Fixed:** policies rewritten to the accurate "full refund or free reschedule; refunds under standard tiers unless waived". -21. **Terms §2.2 contradicted privacy policy on deletion balance.** **Fixed:** rewritten to "balance retained on anonymised record". -22. **Idle-account warnings promised but commented out.** **Fixed:** docs corrected to "not yet implemented". -23. **Till responses hand-rolled JSON** (~50 `http.Error` + 6 `json.NewEncoder`) instead of `mw.RespondJSON/RespondError`. **Fixed:** migrated. -24. **Dev/mock new-card charges broken** — `verify_mock_` overwrote the cnon nonce; mock token ≠ backend contract. **Fixed:** mock mints `cnon:sca-...`; `tokenizeWithVerification` returns `verificationToken: null` for new cards (real-SDK parity) so `save_card` works in dev. -25. **Gift-card buy (account page) kept the old overwrite pattern + sent dead consent fields.** **Fixed:** explicit precedence + `scaFallbackConsentFields` removed. -26. **`GetBookingRemainingBalancePence` counted `on_the_house`/discounts as paid.** **Fixed:** excluded. -27. **refund_failed notification dedup missing `acknowledged_at IS NULL`.** **Fixed.** -28. **Verification codes stored plaintext + brute-force budget keyed per code value.** **Fixed:** HMAC-peppered hashing at rest (CHAR(64)), per-user budget, `[VERIFY]` log relay in dev / fail-closed in prod. - -### MEDIUM-HIGH (adversarial round 3) -29. **£250 online-tip cap bypassable via the overflow-tip carve** — `confirm_overflow_tip=true` + £10,000 minted uncapped tip rows. **Fixed:** gate rejects `req.Amount − realRemaining > maxOnlineTipPence` (£250) in BOTH carve paths + `buildSplitRecords` belt-and-braces returns an error (signature `([]PaymentRecord, error)`). Runtime-verified: £10k → 400; £250 → succeeds with £215 tip carve. -30. **Completion campaign auto-apply over-redemption race** — read-then-write + non-conditional increments. **Fixed:** all 4 increments atomic reserve-first (`... AND times_redeemed < max_redemptions RETURNING id`), skip-on-ErrNoRows; per-user milestone + anniversary get `ON CONFLICT DO NOTHING`; schema backstops added (`chk_times_redeemed` CHECK + partial unique index `uq_booking_discounts_user_milestone_campaign`). Deterministic race tests added. - -### LOW / MINOR / NITPICK -31. **4 gift-card admin handlers lacked in-handler `isAdminRequest` backstop.** **Fixed** (CreateGiftCard/TopUpGiftCard/TransferGiftCard/ClaimExpiredBalance). -32. **Tip lock key inconsistent** (`crussell:tip:` vs `crussell:payment:`). **Fixed:** aligned. -33. **Float→int64 overflow in gift-card cap checks.** **Fixed:** NaN/Inf/oversize guard before pence conversion. -34. **Mock SCA error-shape parity unpinned + prod 2FA branches never compiled** (dead under `test,dev`). **Fixed:** `run-prod-tag-tests.sh` + handler-level verification_required tests + mock parity tests. -35. **Test-gap fixes:** ApplyScheduledDefaultHours test; webhook tests hardcoded-2025 dates → clock-relative; ValidateCardInfo table tests; listCards 20-page guard test; penceLess/roundingEpsilon boundaries; main.go startup-check tests; policy.ts↔refund_policy.go cross-check tests; vitest svelte-component project (happy-dom) + SCA dialog test. -36. **Mobile parity (26 findings):** touch targets ≥44px on UserBookingModal/PaymentModal/TipPayment/BookingFlow/OverflowTipConfirm/TillPurchases; dialog close 44px; `active:` pressed feedback; TimeSlotPicker 50dvh+44px; `.no-scrollbar` utility; receipt document.write fields escaped; policyPopover mobile anchor; CSP meta added to app.html. -37. **Docs parity (17 findings):** refresh-token grace 60s→20s; session 30d→90d; notification reasons 16→17; test counts recomputed; deposit advance 24h→36h; gift_card_expiry default 12→24; patch-test 24-48h→24h; phantom `no_deposit` reason removed; line-ref drift → function-name refs; 5-min header mislabel; route tables + cancellation/gift-card-terms; cash-refund claim verified; flood-cap enumeration; idle-account warnings; M1 VAT undercount; Testing Architecture package counts; `.env` stale `TWO_FACTOR_FALLBACK=true` removed; verification-code delivery channel documented. -38. **Policy minors:** cancel-confirm dialog overstatement; deposit 20–50% phrasing; obsidian User/Admin Manual inconsistencies (24h/36h, forfeit vs full-refund); gift-card expiry reminders; last-updated date format; footer links; deletion balance in obsidian. -39. **stale comment/struct-field cleanup:** `twofa.go:197` clock sentinel; sweep-cap coordination comment; `releaseBookingPaymentLock` note; `ConsentVersion/ConsentAccepted` dead struct fields removed from charge structs; stale references to deleted 2FA machinery. - ---- - -## ADVERSARIAL PASSES — CLEARED (no fix needed) -- Refund capacity/over-refund/pence rounding — **CLEARED** (exact pence math, per-payment + per-booking caps, idempotency replay returns stored result). -- Refund webhook replay double-apply — **CLEARED** (status-guarded UPDATEs, event_id dedup). -- Webhook signature verification — **CLEARED** (HMAC-SHA256 URL+body, constant-time compare). -- Sweep refund of a just-succeeded payment / cancel of a just-completed checkout — **CLEARED** (age gates + replay-window discriminator). -- Sweep notification flood caps — **CLEARED** at every insert site. -- Gift-card transfer TOCTOU / clawback / expiry boundary / per-admin cap — **CLEARED**. -- Loyalty stamps — **CLEARED** (atomic daily-cap + once-per-booking UPDATE). -- Route guards — **CLEARED** (admin group + in-handler backstops; all 4 missing backstops now added). -- SCA/2FA boundary — **VERIFIED-OK**: SCA-only for charges (token required in enforced env), homegrown 2FA only for account/admin actions, fail-closed default, no header-based gate bypass. -- `snapshot` encryption plaintext fallback — **KNOWN/ACCEPTED** (documented money-safety-over-PII tradeoff; CRITICAL startup warning). -- App-clock vs DB-clock money gates — **THEORETICAL/LOW**, no exploit path (server clock not client-controlled). - -## REMAINING KNOWLEDGE (intentional, documented) -- `SNAPSHOT_ENC_KEY` unset → plaintext snapshot fallback (startup CRITICAL; ops must set it in prod). -- **2FA/verification-code delivery**: intended channel is email/SMS (P6), not yet wired. Stdout-log delivery (`[2FA]`/`[VERIFY]`) is a **local DEV ONLY feature** — dev/test builds only. Production builds have no delivery channel; code issuance FAILS CLOSED (503) until email/SMS ships. The `TWO_FACTOR_ALLOW_LOG_DELIVERY` production opt-in was **removed entirely**. -- TRUST_PROXY_HEADERS=false default means rate limiting sees the proxy IP behind nginx (startup warning; ops decision). -- LocalStorage tokens + no hard CSP on the SvelteKit shell beyond the added meta (residual XSS-exposure risk; documented). - -**Session verdict:** review FAILED at start (2 critical wire-contract breaks + 30+ issues) → after 4 fix/review loops: **all issues fixed, all tests green, all keystone journeys runtime-verified.** diff --git a/.sisyphus/plans/staged-default-hours-change.md b/.sisyphus/plans/staged-default-hours-change.md deleted file mode 100644 index 4825f5e..0000000 --- a/.sisyphus/plans/staged-default-hours-change.md +++ /dev/null @@ -1,282 +0,0 @@ -# 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 |