NextChair/docs/phase-10-summary.md

# Phase 10 — Post-Implementation Fixes and Loyalty Card Enhancements

Follow-up to Phase 9: six issues found during real testing, root-caused and
fixed in dependency order, with full regression coverage and a real Docker
verification pass. This doc is the mandated 19-point summary.

## 1. Root cause of uploaded-image 404s

Next.js 16's `next start` (production mode) resolves `public/` asset
requests against something computed at build time, not a live filesystem
check — files present when the image was built serve `200`; files written
to `public/` at runtime (uploads) serve `404` even though they exist on
disk with correct permissions. Confirmed empirically inside the running
container (`docker exec` showed the file present and readable; the HTTP
request still 404'd). Not a permissions/volume/Docker problem — a Next.js
routing-resolution problem.

## 2. Media delivery architecture after the fix

- `src/lib/media/media-storage.ts` now writes to a storage root **outside**
  `public/` (`storage/media` by default, overridable via
  `MEDIA_STORAGE_ROOT`), resolved with traversal-safe path joining.
- A new Route Handler, `src/app/api/media/[...path]/route.ts`, reads and
  streams the file itself with the correct `content-type`/cache headers,
  404s on anything missing or invalid, and rejects `..` traversal —
  bypassing Next's `public/` resolution entirely.
- `storeMediaFile` verifies the file is readable back (`fs.access`) before
  reporting success, so a silent write failure is never reported as a
  success.
- Storage stays provider-neutral: every caller only ever sees a
  `/api/media/...` URL, never a raw filesystem path, so swapping to S3
  later only touches `media-storage.ts`.
- A hand-written migration (`20260804202400_phase10_rewrite_legacy_media_urls`)
  rewrites every already-stored `/media/...` URL to `/api/media/...` across
  every image column in the schema, so images uploaded before this fix
  keep working without a manual data fix-up.

## 3. Docker media-volume changes

- `Dockerfile`: creates and chowns `./storage/media` (was `./public/media`).
- `docker-compose.local.yaml`: the `next-chair-media` volume now mounts at
  `/app/storage/media` — **same volume name**, deliberately, so the one
  real file already uploaded to the user's business reappears at its new
  path instead of being orphaned. Confirmed in this session: the pre-existing
  uploaded file (`gallery/eecbe189-....jpg`) is present under
  `storage/media/` and resolves `200` through `/api/media/...` after a full
  image rebuild and container recreation.
- Verified end-to-end against the real persistent stack: rebuild →
  recreate → migrations auto-apply → existing upload still resolves →
  `/api/health` and every checked route return the expected status.

## 4. Loyalty schema changes

Added to `LoyaltyProgramme` (migration
`20260804202231_phase10_loyalty_card_and_permission_fixes`):

- `rewardPosition RewardPositionMode @default(AFTER_ALL_STAMPS)` — new enum
  `AFTER_ALL_STAMPS | FINAL_SLOT_IS_REWARD`.
- `cardTopRightText String?`, `cardFont LoyaltyCardFont @default(SANS)`
  (enum `SANS | SERIF | MONO`), `cardTopRightTextSize LoyaltyCardTextSize
  @default(MD)` (enum `SM | MD | LG`), `cardTopRightTextColor String?`,
  `cardBorderColor String?`, `cardMemberNumberColor String?`.
- `CHECK (stampsRequired BETWEEN 1 AND 10)` on `stampsRequired`, confirmed
  genuinely enforced on MySQL 8 (a violating `UPDATE` against the real
  business's row fails with error 3819, verified in this session).
- A separate idempotent migration
  (`20260804202300_phase10_backfill_owner_permissions`) backfills
  `MANAGE_MEDIA_LIBRARY`/`MANAGE_REVIEWS` onto every role already holding
  `MANAGE_STAFF` — see point 10.

No destructive migration was needed: the real business's existing row had
`stampsRequired = 10`, already within range.

## 5. Loyalty typography/colour controls

`src/features/owner/components/loyalty-config-form.tsx` gained:

- **Top-right card text**: plain text input + font (Sans/Serif/Mono, via
  `LoyaltyCardFont` — Tailwind's built-in generic stacks only, deliberately
  no new web fonts loaded) + size (Small/Medium/Large) + colour.
- Font is **card-wide** (applies to all card text), size and colour are
  **specific to the top-right text only** — one clear, documented
  interpretation rather than per-line font controls.
- **Border colour** and **member-number colour**, each via the existing
  `ColorPicker`.
- The live preview renders through the exact same `LoyaltyCard` component
  the customer sees (`src/components/ui/loyalty-card.tsx`) — no
  preview-only rendering path that could drift from the real card.

## 6. Terms-line fix

Two independent root causes, both fixed:

1. `getLoyaltySummary` never selected/returned `termsLine` at all —
   `LoyaltySummary` didn't have the field.
2. `(customer)/loyalty/page.tsx` didn't use it anyway — it always
   synthesized its own `"N more to your reward"` string and only showed it
   when no reward was available, silently discarding whatever the owner
   had actually configured.

Fixed: `LoyaltySummary` now includes `termsLine`, and the page renders
`loyalty.termsLine ?? undefined` — the real configured text, wrapped safely
by the card's existing `text-center` styling, never overlapping the stamp
grid (it sits below it in the flex layout). No configured value → nothing
renders (the card's existing `{termsLine && ...}` guard), matching the
brief's "no placeholder terms" requirement. Verified via a real E2E
assertion against the seeded terms line (customer-journeys.spec.ts, journey 8).

## 7. Stamp-limit enforcement

`MAX_STAMPS_REQUIRED = 10`, defined once in the new
`src/services/loyalty/reward-threshold.ts` and re-exported from
`loyalty.service.ts`, enforced at every layer:

- `validateProgrammeConfig` (domain service).
- The owner form's number input (`max={10}`, clamped on every keystroke).
- The `LoyaltyProgramme_stampsRequired_range` MySQL CHECK constraint.
- Existing integration test data already at or under 10 — no migration of
  bad data was needed (checked first, per the brief's instruction).

## 8. Reward-position mode behaviour

New `resolveRewardThreshold()` (`src/services/loyalty/reward-threshold.ts`)
is the single place this is decided:

- `AFTER_ALL_STAMPS` (default, matches pre-Phase-10 behaviour exactly):
  threshold == `stampsRequired`. Every card position is a plain stamp.
- `FINAL_SLOT_IS_REWARD`: threshold == `stampsRequired - 1` (clamped at 1).
  The last position is the reward itself, not a stamp to collect —
  rendered distinctly (a `Gift` icon outline while locked, filled reward
  styling once unlocked) regardless of whether `stampsRequired` was ever
  literally reached.

Every consumer reads through this one function — never re-derives its own
threshold:

- `addStampTransaction` (shared by both the automatic completed-appointment
  path and the manual staff "Add Stamp" path — same threshold, proven by
  a dedicated integration test that awards one stamp each way and checks
  the reward fires at the crossing point).
- `redeemReward` — deducts the threshold amount, not the raw
  `stampsRequired`, so redemption in `FINAL_SLOT_IS_REWARD` mode can't
  under- or over-deduct relative to what was actually collected.
- `LoyaltyCard` (customer view and owner live preview, same component).
- Every "X/Y stamps" progress display: customer Home, customer Loyalty
  page, staff Current/Clients/Client-detail/Appointment-detail pages, owner
  Appointment-detail page, and the batched staff customer-directory summary
  (`CustomerLoyaltyBrief.rewardThreshold`) — all previously showed
  `currentStamps`/`stampsRequired`, which would have under-represented
  progress in `FINAL_SLOT_IS_REWARD` mode.

The owner's "Reward Timing" control is two clearly labeled option cards
("Reward after all stamps" / "Final position is the reward"), each with
supporting copy computed from the live `stampsRequired` value, not an
unlabeled boolean.

## 9. Google Place ID implementation

The field already existed in the schema and the owner Reviews page
(`reviews-manager.tsx`) — the actual root cause was elsewhere (see point 10).
This session's changes:

- Expanded the field's help text to explain how to find a Place ID via
  Google Maps' own Share → Embed flow, deliberately with **no hardcoded
  external URL** (per the standing "never fabricate a URL" constraint).
- `updateReviewsSettingsAction` now trims the value, detects an actual
  change, and writes an audit log entry (`GOOGLE_PLACE_ID_CHANGED`, with
  `previousPlaceId`/`nextPlaceId` metadata) only when it changed.
- Existing reviews are kept (not archived/deleted) when the Place ID
  changes — matching the recommended, documented behaviour; the "Sync Now"
  button already gave an immediate manual re-sync path.
- Validation stays basic (non-empty, trimmed) — no restrictive regex that
  could reject a valid Place ID.

## 10. Reviews-navigation bug root cause

Not a routing bug at all. A read-only query against the real, live database
showed the installed business's Owner role was **missing** the
`MANAGE_MEDIA_LIBRARY` and `MANAGE_REVIEWS` permissions — both added to the
`Permission` enum in Phase 9, but `syncRolePermissions()` only runs at
install/seed time, so an already-installed business's `RolePermission` rows
never picked them up. The Reviews nav link's `href` and route config were
always correct; the page (and the Place ID field on it) was simply
inaccessible/hidden behind a permission check that silently failed. Fixed
with an idempotent backfill migration
(`20260804202300_phase10_backfill_owner_permissions`) rather than any
client-side workaround — confirmed on the real database in this session
(`SELECT` shows Owner now holds both permissions).

## 11. Home service preselection approach

`preselectServiceAction` (`src/features/booking/actions/draft.ts`):
receives only a `serviceId` from the client; the service's active/bookable
status and business are always re-read from the database. No existing
draft → creates one with the service selected. Existing draft → adds the
service if not already present (no duplicates), clearing a previously
chosen barber/date/time only if the selection actually changed — the same
rule `saveServicesStepAction` already uses. Invalid/inactive/non-bookable
service → a friendly inline error on the `BookServiceButton`, draft
untouched. No price/duration/eligibility ever crosses the URL; everything
is re-validated server-side against the signed booking-draft cookie.

## 12. Routes/components changed

- New: `src/app/api/media/[...path]/route.ts`,
  `src/features/booking/components/book-service-button.tsx`,
  `src/services/loyalty/reward-threshold.ts`.
- Rewritten: `src/lib/media/media-storage.ts`.
- Extended: `src/services/loyalty/loyalty.service.ts` (new fields on
  `LoyaltySummary`/`ProgrammeConfigInput`/`CustomerLoyaltyBrief`,
  `resolveRewardThreshold` re-export), `src/components/ui/loyalty-card.tsx`
  (font/border/member-number/top-right-text props, reward-slot visuals),
  `src/features/owner/components/loyalty-config-form.tsx` (reward-timing
  selector, typography/colour controls), `src/features/booking/actions/draft.ts`
  and `schemas.ts` (`preselectServiceAction`), `src/features/home/components/home-tabs.tsx`
  (uses `BookServiceButton`), `src/features/owner/actions/reviews.ts` and
  `reviews-manager.tsx` (Place ID audit log + help text), `Dockerfile` and
  `docker-compose.local.yaml` (storage path).
- Progress-display denominator fixed (now threshold-aware, not raw
  `stampsRequired`) in: `(customer)/home/page.tsx`, `(customer)/loyalty/page.tsx`,
  `(staff)/staff/current/page.tsx`, `(staff)/staff/clients/page.tsx`,
  `(staff)/staff/clients/[id]/page.tsx`, `(staff)/staff/appointments/[id]/page.tsx`,
  `(owner)/owner/appointments/[id]/page.tsx`, `(owner)/owner/settings/loyalty/page.tsx`.

## 13. Prisma migration

Three migrations, all applied cleanly against the real, persistent
database in this session's Docker verification:

1. `20260804202231_phase10_loyalty_card_and_permission_fixes` — the seven
   new `LoyaltyProgramme` columns + two new enums + the stamps-required
   CHECK constraint (generated via `prisma migrate dev`, CHECK
   hand-appended).
2. `20260804202300_phase10_backfill_owner_permissions` — hand-written,
   idempotent `INSERT IGNORE` permission backfill.
3. `20260804202400_phase10_rewrite_legacy_media_urls` — hand-written,
   idempotent legacy `/media/...` → `/api/media/...` URL rewrite.

## 14. Tests added/updated

- Unit: `src/services/loyalty/__tests__/loyalty-config.test.ts` — 3 new
  `resolveRewardThreshold` cases (both modes + the 1-stamp clamp), plus the
  existing `validateProgrammeConfig` fixtures updated for the widened
  `ProgrammeConfigInput`.
- Integration: `src/services/loyalty/__tests__/loyalty.service.integration.test.ts` —
  3 new cases: `FINAL_SLOT_IS_REWARD` earns via a manual add one stamp
  early; the automatic and manual paths agree on the same threshold;
  redemption deducts the threshold, not the raw `stampsRequired`.
  `loyalty-owner.integration.test.ts` updated for the widened input type.
- E2E: `customer-journeys.spec.ts` — 2 new journeys (9: Home "Book"
  preselects a service, preserves existing selections, doesn't duplicate
  on re-click; 10: the same for a guest) and one new assertion on journey 8
  (the real configured terms line renders on the customer card).
- Removed: an attempted integration test that called
  `preselectServiceAction` directly — it (and every other booking-draft
  Server Action in this codebase) depends on `next/headers`'s
  request-scoped `cookies()` and calls `redirect()` on success, neither of
  which works outside a real Next.js request; this matches this codebase's
  existing, documented convention (see `customer-booking.integration.test.ts`'s
  own header comment) of testing these thin Server Actions via Playwright
  only. The three rejection paths that don't touch `cookies()`/`redirect()`
  were still directly testable, but were dropped in favour of the simpler,
  already-covered E2E path once the client-bundle constraint below made
  clear how easy it is for this module boundary to silently break.

## 15. Full regression results

- **Unit**: 253/253 passing (`npx vitest run`).
- **Integration**: 185/185 passing, against a real MySQL 8 database
  (`npx vitest run --config vitest.integration.config.mts`).
- **E2E — customer-journeys.spec.ts**: 10/10 passing.
- **E2E — owner-journeys.spec.ts**: 13/13 passing (this run caught a real
  bug — see point 18).
- **E2E — staff-journeys.spec.ts**: 6/10 passing. Journey 7 ("barber
  changes their working hours") fails on a clean database; journeys 8–10
  didn't run as a result. Confirmed via `git status` that nothing in this
  phase touches working-hours code — this is a pre-existing issue,
  unrelated to any of the six fixes in this brief, and has been flagged as
  its own follow-up task rather than investigated here (see point 18).
- `npx tsc --noEmit` and `npx eslint .` (whole repo): clean.
- `npm run build` (production build): succeeds. The only console noise
  during the build is Prisma connection-pool timeouts while attempting to
  reach the database from this sandboxed build environment (not reachable
  here by design) — the build still completes and every route lists
  correctly.

## 16. Docker verification

Performed against the user's real, persistent local stack (not a
throwaway container), in this order:

1. `docker compose build next-chair` — image rebuilt with all Phase 10
   code.
2. `docker compose up -d next-chair` — container recreated; entrypoint
   auto-applied the 3 pending migrations against the real database,
   reported "Syncing Super Administrator... skipping (none configured)"
   (expected — no `SUPER_ADMIN_EMAIL` set), then started successfully.
3. `curl` checks: `/api/health` → 200, `/home` → 200, `/loyalty` → 200,
   `/book` → 200, `/owner/settings/loyalty` → 307 (sign-in redirect, not a
   500), `/owner/settings/reviews` → 307. No 500s anywhere.
4. The one real file already uploaded to this business's media library
   (`storage/media/gallery/....jpg`) still resolves `200` with
   `content-type: image/jpeg` through `/api/media/...` after the rebuild —
   confirms the Phase 10.1 media fix and the volume-path change survive a
   real rebuild+recreate cycle.
5. Direct `SELECT` against the real `LoyaltyProgramme` row: the new columns
   are present with correct defaults (`rewardPosition = AFTER_ALL_STAMPS`,
   `cardFont = SANS`, `cardTopRightTextSize = MD`) — the migration didn't
   corrupt or reset the business's existing configuration.
6. A live `UPDATE ... SET stampsRequired = 15` against the real row is
   rejected by the CHECK constraint (`Error 3819`) — confirms the
   constraint is genuinely enforced, not just declared.
7. Direct `SELECT` against `RolePermission`: the real Owner role now holds
   both `MANAGE_MEDIA_LIBRARY` and `MANAGE_REVIEWS`.

Authenticated in-browser verification of the owner loyalty form and both
reward modes was **not** re-run through this same Docker container (curl
can't drive the sign-in form without the real owner's credentials, which
this session doesn't have and won't attempt to bypass). That coverage
comes from the Playwright E2E suite instead, run against a dev server
using the identical application code and a real MySQL 8 database — see
point 15.

## 17. Manual verification performed

Everything in point 16, plus the E2E suite's own real-browser, real-form,
real-database coverage of: the owner loyalty config form (typography,
colours, reward-timing selector, live preview), a customer's card
(terms line, reward-slot visuals), Home's "Book" button preselecting a
service for both a guest and a signed-in customer, and the Reviews/Place ID
settings page loading correctly for the Owner role.

## 18. Known limitations

- **Staff working-hours E2E failure** — pre-existing, unrelated to this
  phase, flagged as its own follow-up task (spawned during this session)
  rather than investigated here, per the brief's scope.
- **Duplicate `mug-saucer` icon-picker React key warning** — pre-existing,
  unrelated, also flagged as its own follow-up task.
- **A real client-bundle bug was found and fixed during this phase's own
  verification**: `loyalty-config-form.tsx` (a Client Component) briefly
  imported `MAX_STAMPS_REQUIRED` from the full `loyalty.service.ts`, which
  transitively pulls in Prisma-touching notification code — this broke the
  owner loyalty settings page in a real browser (caught by the
  owner-journeys E2E suite, not by `tsc`/`eslint`/unit tests, none of which
  exercise the client bundle boundary). Fixed by moving both
  `resolveRewardThreshold` and `MAX_STAMPS_REQUIRED` into the small,
  side-effect-free `reward-threshold.ts` module client components can
  import directly. Documented here because it's a real, general risk
  pattern for this codebase — any future constant or pure helper that
  "happens to live" in a Prisma-calling service file is one import away
  from silently breaking a client bundle, and only an E2E run against a
  real browser will catch it.
- Google Reviews sync itself (calling the real Google Places API) was not
  exercised end-to-end — this session has no real Google API credentials,
  and the brief's own "never call the Places API from every page load"
  constraint means this is only reachable via the 24h cron or the manual
  "Sync Now" button, neither of which this session can trigger against a
  real Google account.
- The staff customer-directory's batched loyalty summary
  (`CustomerLoyaltyBrief`) now shows a threshold-aware denominator like
  every other progress display, but this one specific display wasn't
  separately covered by a new automated test (it's a one-line change,
  reusing the already-tested `resolveRewardThreshold`).

## 19. Anything requiring real external credentials

- A Google Cloud API key + a real Google Business Profile Place ID, to
  exercise the actual Places API sync (see point 18).
- The real owner account's password, to drive an authenticated Docker
  container walkthrough via `curl` rather than relying on the E2E suite's
  own (seeded, non-production) credentials.