65 lines
8.1 KiB
Markdown
65 lines
8.1 KiB
Markdown
# Fix: attendance "Present" not saved automatically on login
|
|
|
|
## Context
|
|
|
|
GFIBER-649 shipped "auto-mark attendance Present on login" (merged to `dev` 2026-06-23, confirmed live in both `main` and `dev` branches — they differ by only 2 unrelated commits). The implementation, `markSelfPresentToday()`, has been silently failing for **every non-admin user** since it shipped. Since a missing attendance row degrades to the harmless-looking "Pending" state (see `utils/dashboard/attendance.ts` — no record = assumed present in aggregate counts), nobody saw an error; the bug likely surfaced through the admin "pending attendance" bell/digest email never clearing for otherwise-active agents, which matches what was reported: attendance never gets marked Present at login.
|
|
|
|
The confirmed root cause: **RLS blocks every regular user's self-insert.** `markSelfPresentToday()` (`app/(protected)/upsell-evaluator/attendance-actions.ts`) runs through the anon-key Supabase client (`utils/supabase/server.ts`), so Row-Level Security applies. The only policy on `public.attendance` (`supabase/migrations/20260605130000_create_attendance.sql`) is `FOR ALL ... USING/WITH CHECK (role IN ('admin','owner'))` — it was written for the admin-only dashboard grid and restricts *every* operation, including INSERT, to admins/owners. Regular agents default to `role = 'user'` (`20260603000000_add_role_to_profiles.sql`), so their self-upsert is rejected by RLS. The calling code never reads the `{error}` from `.upsert(...)` — it's discarded entirely — so the failure has been completely invisible. An existing test (`__tests__/dashboard-attendance-actions.test.ts:82-85`) even asserts this silence as current behavior. This can only be fixed with a DB-level policy change (a migration) — no amount of application code can grant a permission Postgres RLS denies.
|
|
|
|
> **Ruled out:** a CHECK-constraint drift between prod/dev on `attendance_status_valid` (migration history shows a prod rollback + dev-only re-narrowing that looked unreconciled). Confirmed with the team that admins can already set status manually without error, which proves the constraint already accepts `'P'`/`'A'` in both environments — no constraint migration needed. This plan sticks to the confirmed RLS fix only.
|
|
|
|
Fixing the RLS policy, plus moving the call site and adding error visibility, closes the bug and prevents this exact silent-failure pattern from recurring.
|
|
|
|
> **Update (post-deploy verification, 2026-07-01):** the first INSERT-only policy (below) was necessary but not sufficient. Live testing in DEV showed `markSelfPresentToday()` still failed to write for a real `role='user'` account even after the migration was applied. Root cause, found by reproducing the exact `INSERT ... ON CONFLICT (agent_id, date) DO NOTHING` statement directly against the DEV database: **Postgres RLS requires SELECT-level visibility into a potentially-conflicting row to resolve an `ON CONFLICT ... DO NOTHING` clause, even though DO NOTHING never reads that row.** With only an INSERT policy and no SELECT policy, Postgres rejects the whole statement with the same generic "violates row-level security policy" error — indistinguishable from the original bug without direct reproduction. A second migration (`20260701000001_attendance_self_checkin_select.sql`) adds a `FOR SELECT` policy scoped to `agent_id = auth.uid()` (a user may see their own attendance rows). Verified end-to-end against the live DEV database: the exact upsert now succeeds and the row persists.
|
|
|
|
## Approach
|
|
|
|
### 1. New migration — self check-in RLS policy
|
|
|
|
`supabase/migrations/20260701000000_attendance_self_checkin_rls.sql`:
|
|
|
|
```sql
|
|
create policy "Users can self-check-in present today"
|
|
on public.attendance
|
|
for insert
|
|
to authenticated
|
|
with check (
|
|
agent_id = (select auth.uid())
|
|
and status = 'P'
|
|
and date = current_date
|
|
);
|
|
```
|
|
|
|
Postgres OR's all applicable permissive policies for a given command, so this adds an allowed INSERT path without touching the existing admin `FOR ALL` policy — admins keep full read/write/delete over every row; regular users still cannot SELECT/UPDATE/DELETE, or insert any row but their own today's `'P'`. Only `WITH CHECK` is needed (no `USING`) since `FOR INSERT` policies don't evaluate `USING`. `markSelfPresentToday()` calls `upsert(..., { ignoreDuplicates: true })`, which compiles to `INSERT ... ON CONFLICT DO NOTHING` — no UPDATE path — so an INSERT-only policy is sufficient and admin-set `'A'` rows are never overwritten.
|
|
|
|
### 2. `attendance-actions.ts` — stop swallowing the error
|
|
|
|
Destructure `{ error }` from the upsert result and `console.error("markSelfPresentToday: upsert failed:", error)` when present, matching the existing repo convention (`app/(protected)/actions.ts`). Function stays best-effort — never throws, return type unchanged. This is what turns the next silent RLS/constraint regression into something visible in logs instead of a months-long undetected bug.
|
|
|
|
### 3. Move the call site from the page to the shared protected layout
|
|
|
|
Today `markSelfPresentToday()` only runs when `app/(protected)/upsell-evaluator/page.tsx` renders — not literally "on login." `app/(protected)/layout.tsx` wraps *every* protected route (dashboard, admin, my-metrics, upsell-evaluator) and already gates on `getCurrentUserWithProfile()`. Move the call there (after the auth gate resolves, before the admin-only pending-agents fetch), and remove it from `page.tsx`. This actually matches the intent already documented in the code comment ("login = auto-present per the GFIBER-649 rule") regardless of which page a post-login redirect lands on.
|
|
|
|
### 4. Tests (TDD — write these alongside/before the code changes)
|
|
|
|
- `__tests__/dashboard-attendance-actions.test.ts`: add a case asserting `console.error` is called with the upsert error (using the existing `wireAnonDb` helper, no changes needed there), and a case asserting no log on success.
|
|
- `__tests__/protected-layout.test.ts`: **must** be updated or it breaks — it currently does not mock `@/app/(protected)/upsell-evaluator/attendance-actions`, confirmed by direct inspection. Add a `vi.fn()` mock (same pattern as `__tests__/upsell-evaluator-page.test.tsx`), assert it's called once for any authenticated role and not called when unauthenticated, add `mockClear()` to the existing `beforeEach`.
|
|
- `__tests__/upsell-evaluator-page.test.tsx`: no required changes (it mocks the whole module and never asserts the call happened).
|
|
|
|
## Risks / edge cases (validated)
|
|
|
|
- **Cross-agent / backdating abuse**: impossible — `WITH CHECK` binds `agent_id = auth.uid()` (server-derived, unspoofable) and `date = current_date` (evaluated server-side).
|
|
- **UTC timezone assumption**: client computes `new Date().toISOString()` (always UTC); Postgres `current_date` resolves in the DB session timezone, which is UTC by default and not overridden in `supabase/config.toml`. A future non-UTC DB timezone change could cause near-midnight false rejects (logged now, not silent) — not a data-safety issue, just worth a code comment.
|
|
|
|
## Verification
|
|
|
|
No pgTAP/DB-level test harness exists in this repo — RLS behavior must be verified against a live Supabase project:
|
|
|
|
1. `npm run test` — full suite green, including the updated `protected-layout` and `dashboard-attendance-actions` tests.
|
|
2. `supabase db push` to **dev** (`vshcimexazocttjmyrqy`); confirm the migration applies cleanly.
|
|
3. Inspect live state: `select policyname, cmd, with_check from pg_policies where tablename='attendance';` — confirm both the new self-check-in policy and the existing admin policy are present.
|
|
4. Log in as a real non-admin (`role='user'`) test account in dev, land on any protected page (not just Upsell Evaluator, to confirm the layout move), then confirm a `status='P'` row exists for that agent/today via the dashboard grid or a direct query.
|
|
5. As an admin, confirm the dashboard's manual attendance marking/clearing still works (the admin `FOR ALL` policy is untouched).
|
|
6. Reload the same session same-day — confirm no duplicate row or error (exercises `ON CONFLICT DO NOTHING`).
|
|
7. Push the same migration to **production** (`hwxkegbdnbrzvekrqxbd`) and repeat step 4 against a real prod account.
|