109 lines
14 KiB
Markdown
109 lines
14 KiB
Markdown
# GFIBER-683 — Allow admins to correct offer data (accepted/rejected override + history)
|
|
|
|
## Context
|
|
|
|
Jira **GFIBER-683**: "Agents in the pilot group have submitted incorrect upsells that compromised reporting metrics calculation. We need a mechanism to allow admins to validate, edit and correct these wrong entries."
|
|
|
|
Concretely: at the end of the Upsell Evaluator flow, an agent records whether the customer **Accepted** or **Rejected** the offer (`upsell_tickets.state`, mapped from the enum: `accepted` / `declined_hard` in practice — the only two "declined" the current UI can produce). Agents sometimes pick the wrong one by mistake. Since every dashboard KPI, rate tile, and export re-reads `state` live on every request, one wrong pick silently skews reporting metrics with no way to fix it today.
|
|
|
|
A detailed design already exists in Docmost (space **GFiber** → page *Upsell_State_Override_—_Plan_Historial_de_Cambios_(Accepted-Rejected)*, ES/EN subpages, written 2026-07-09) and was refined further in this conversation against the current state of the repo. This plan supersedes the Docmost draft on two points where the repo/requirements moved since it was written (see below); the rest of the Docmost design (migration, RPC, RLS, server actions, tests) still holds and an Explore agent re-verified every file path/helper name against today's code.
|
|
|
|
**What changed vs. the Docmost draft, confirmed with the user in this session:**
|
|
|
|
1. **Table scope widened.** Today `app/(protected)/dashboard/page.tsx` fetches only `state = "accepted"` tickets for the "Offer Verification" table (`ValidationsCard` → `AcceptedTicketsVerification`). An agent who mis-marked an accepted offer as Rejected produces a `declined_hard` ticket that never appears in this table — there'd be nothing to click "History"/override on. Fix: broaden the query to `state IN ('accepted', 'declined_hard')` — the only two states this feature's toggle ever produces or corrects. Not all 6 enum values — `no_pitch`, `declined_soft`, `no_offer`, `in_progress` stay out of this table's scope.
|
|
2. **Menu design simplified.** The Docmost draft split this into two menu items — "Verify" (existing modal + a new embedded "Override Outcome" section) and "History" (separate read-only modal). The user's clarified intent is simpler: leave the existing verification modal **completely untouched**, and add **one** new second menu item that opens a **single new modal** combining both the Accepted⇄Rejected override control *and* the change history in one view.
|
|
3. `no_pitch` stays excluded from reclassification, per the user — confirmed as-is from the Docmost draft.
|
|
|
|
## Design
|
|
|
|
### 1. Migration — new audit table + atomic RPC
|
|
|
|
New file `supabase/migrations/<timestamp>_add_upsell_state_changes.sql` (timestamp must sort after the latest existing migration, `20260714120000_add_sales_coach_notes_to_upsell_tickets.sql`):
|
|
|
|
- **Table `upsell_state_changes`**: `id uuid pk`, `ticket_id uuid references upsell_tickets(id) on delete cascade`, `changed_by uuid references profiles(id) on delete set null`, `changed_at timestamptz default now()`, `old_state upsell_state not null`, `new_state upsell_state not null`, `reason text`. Index on `(ticket_id, changed_at desc)`.
|
|
- Modeled directly on the existing audit-table precedent `supabase/migrations/20260612120000_create_eligibility_checks.sql`, but **no INSERT/UPDATE/DELETE policy for any client role at all** (unlike `eligibility_checks`, which lets any authenticated user insert) — every write goes through the RPC below. RLS: `select` policy gated to `public.current_profile_role() in ('admin','owner')` (reuse the existing helper from `supabase/migrations/20260707130000_fix_profiles_update_policy_recursion.sql:29-42` — do not write a new one).
|
|
- **RPC `change_upsell_ticket_state(p_ticket_id uuid, p_new_state upsell_state, p_reason text default null) returns upsell_tickets`**, `security definer`, `set search_path = ''`: checks caller role is admin/owner via `auth.uid()` → `profiles.role` (or `current_profile_role()`), checks `p_new_state in ('accepted','declined_hard')`, row-locks the ticket (`for update`), checks its current `state` is one of the terminal states `('accepted','declined_hard','declined_soft','no_offer')` (excludes `in_progress` and `no_pitch`), checks it isn't already in `p_new_state`, then in one transaction: `update upsell_tickets set state = p_new_state, initiation_type = null where id = ...` and `insert into upsell_state_changes (...)`. `revoke ... from public; grant execute ... to authenticated`.
|
|
- **Critical gotcha to get right**: this RPC must be called via the **session-scoped client** (`createClient()` from `utils/supabase/server.ts`), not `createAdminClient()` (`utils/supabase/admin.ts`) — every other action in `dashboard/actions.ts` today uses `createAdminClient()` after an app-layer `requireRole` check, so this is a real deviation; call it out in the PR description/code review since it's easy to copy-paste the wrong client and have `auth.uid()` silently resolve to null inside the function.
|
|
- Regenerate `utils/supabase/database.types.ts` after applying the migration to DEV.
|
|
|
|
### 2. Server Actions — `app/(protected)/dashboard/actions.ts`
|
|
|
|
Add two new actions, following the exact import/helper pattern already used in this file (`getCurrentUserWithProfile` from `utils/supabase/get-current-user.ts`, `requireRole`/`RoleError` from `utils/roles.ts`):
|
|
|
|
- `changeTicketState(ticketId, newState: "accepted" | "declined_hard", reason?)`: app-layer `requireRole(profile, "admin", "owner")` as defense-in-depth, then `createClient().rpc("change_upsell_ticket_state", {...})`, surface any raised exception as `{ error }`, `revalidatePath("/dashboard")` on success.
|
|
- `getTicketStateHistory(ticketId)`: same role gate, then a plain read via `createAdminClient()` (no RLS-sensitive write here) joining `profiles!changed_by(display_name, email)`, ordered `changed_at desc`.
|
|
|
|
### 3. Widen the verification table's query — `app/(protected)/dashboard/page.tsx`
|
|
|
|
- Change the `acceptedTicketsResult` query (~line 128) from `.eq("state", "accepted")` to `.in("state", ["accepted", "declined_hard"])`, and add `state` to the `.select(...)` column list.
|
|
- `AcceptedTicket` type (`components/dashboard/AcceptedTicketsVerification.tsx:17-30`) gets a new `state: UpsellState` field.
|
|
- No other change needed here: `ValidationsCard`'s "Information by Agent" tab (`buildValidationSummaryByAgent`) derives its pending/valid/rejected tally from `full_flow_completed`/`initiation_type`/`objection_handled_correctly` only, not from `state`, so it picks up the wider ticket set automatically and correctly.
|
|
|
|
### 4. UI — kebab becomes a 2-item menu; second item opens one combined modal
|
|
|
|
**New DS component `KebabMenu`** (`packages/design-system/src/components/KebabMenu/`): wraps the existing `IconButton` (⋯ trigger) and portal-renders (`createPortal` to `document.body`, positioned from the trigger's `getBoundingClientRect()` — same technique already used by `AttendanceQuickModal`/`TicketVerificationModal`, needed because this table can scroll horizontally and a non-portaled menu risks clipping, the same overflow-bug class already hit twice in this repo for the Attendance grid). Closes on Escape/outside-click/item-select. `role="menu"` / `role="menuitem"`, arrow-key navigation. Props: `{ ariaLabel: string; items: { label: string; onSelect: () => void }[] }`. Ships with `.stories.tsx`, `.test.tsx`, `.test.cy.tsx` per this repo's DS-first convention.
|
|
|
|
`components/dashboard/AcceptedTicketsVerification.tsx` Actions column (currently a single `IconButton` at lines 179-194 calling `setManagedTicket(ticket)`) becomes:
|
|
|
|
```tsx
|
|
<KebabMenu
|
|
ariaLabel={`Actions for ${agentName}'s ticket`}
|
|
items={[
|
|
{ label: "Verify", onSelect: () => setManagedTicket(ticket) },
|
|
{ label: "Change Status", onSelect: () => setStateModalTicket(ticket) },
|
|
]}
|
|
/>
|
|
```
|
|
|
|
`TicketVerificationModal` is **not modified** — it keeps rendering exactly as it does today (the 4 existing sections: full_flow_completed, initiation_type, objection_handled_correctly, sales coach notes).
|
|
|
|
**New DS component `TicketStateModal`** (`packages/design-system/src/components/TicketStateModal/`), opened by the new "Change Status" menu item, built on the same portal/focus-trap/Escape-to-close shell as `TicketVerificationModal`/`AttendanceQuickModal`:
|
|
|
|
- Header: agent name + Case ID + current `OUTCOME_BADGE` (reused from `utils/upsell/outcomeDisplay.ts`).
|
|
- **Override section**: "Mark Accepted" / "Mark Rejected" buttons, disabled on whichever is currently active, plus an optional reason `TextField` (DS component already used elsewhere, e.g. `StageClose.tsx`'s Order ID field). On submit, calls `changeTicketState` via the existing `startTransition` + `router.refresh()` pattern already used for the other actions in this file.
|
|
- **History section** (same view, below the override controls): fetches `getTicketStateHistory(ticketId)` on open, renders a `DataTable` (same component `SessionHistoryView` uses): `changed_at` → `old_state → new_state` (two `OUTCOME_BADGE`s with an arrow between) → `changed_by_name` → `reason`. Empty state: "No manual overrides yet."
|
|
- Footer: Close button.
|
|
- Ships with `.stories.tsx`, `.test.tsx`, `.test.cy.tsx`.
|
|
|
|
`AcceptedTicketsVerification.tsx` gets one new local state (`stateModalTicket`) alongside the existing `managedTicket`, each independently rendering its own modal — mirrors the existing `managed` state pattern already used for `TicketVerificationModal`.
|
|
|
|
### Side effects to call out in the PR description (no code change, just visibility)
|
|
|
|
- Reclassifying a client's most-recent ticket changes that client's live cooloff/eligibility on the next Upsell Evaluator lookup (`lookupClient()` derives eligibility from the latest ticket's `state` live, no cache).
|
|
- Dashboard KPIs, rate tiles, Session History, and the XLSX export all shift on next load — nothing else to invalidate.
|
|
|
|
## Files touched
|
|
|
|
| File | Change |
|
|
|---|---|
|
|
| `supabase/migrations/<ts>_add_upsell_state_changes.sql` | new table + RLS (admin/owner select-only, no insert/update/delete policy) + `change_upsell_ticket_state()` RPC |
|
|
| `utils/supabase/database.types.ts` | regenerated |
|
|
| `app/(protected)/dashboard/actions.ts` | new `changeTicketState`, new `getTicketStateHistory` |
|
|
| `app/(protected)/dashboard/page.tsx` | widen `acceptedTicketsResult` query to `state IN ('accepted','declined_hard')`, add `state` to select |
|
|
| `components/dashboard/AcceptedTicketsVerification.tsx` | `AcceptedTicket.state` field; swap lone kebab `IconButton` for `KebabMenu` (Verify / Change Status); new `stateModalTicket` local state rendering `TicketStateModal` |
|
|
| `packages/design-system/src/components/KebabMenu/*` | new — menu trigger + story + tests |
|
|
| `packages/design-system/src/components/TicketStateModal/*` | new — combined override+history modal + story + tests |
|
|
| `packages/design-system/src/components/index.tsx` | export `KebabMenu` + `TicketStateModal` |
|
|
|
|
## Tests (TDD)
|
|
|
|
- `__tests__/dashboard-state-change.test.ts` — mock `.rpc()` (this action calls the RPC, not `.from().update()`), role gate, success path, RPC exception messages surfaced verbatim, `revalidatePath` only on success.
|
|
- `__tests__/dashboard-state-history.test.ts` — role gate, empty history → `[]`, `profiles!changed_by` join mapping, DB error → `{ error }`.
|
|
- `KebabMenu.test.tsx` + `.test.cy.tsx` — open/close (click, Escape, outside-click, item-select), `role="menu"`/`"menuitem"`, arrow-key nav, correct `onSelect` fired.
|
|
- `TicketStateModal.test.tsx` + `.test.cy.tsx` — override buttons disabled on current state, click inactive button calls `changeTicketState` with right args, history rows render (old→new badges, changed_by_name, reason), empty state, focus-trap/Escape (mirror `AttendanceQuickModal.test.tsx`).
|
|
- Extend `__tests__/accepted-tickets-verification.test.tsx`: Actions column renders `KebabMenu` with exactly two items; "Verify" opens the (unmodified) verification modal; "Change Status" opens `TicketStateModal`; mutually exclusive modal states.
|
|
- Extend/add a page-level test or integration check confirming the widened query actually returns `declined_hard` tickets alongside `accepted` ones.
|
|
- No Vitest can exercise the Postgres function body (no pgTAP in this repo) — the RPC's internal guards (role check, terminal-state check, atomicity) can only be verified against a real DEV database; call this out as a manual verification step, not a gap for more unit tests.
|
|
|
|
## Verification (end-to-end)
|
|
|
|
1. `npm run lint && npm run test`.
|
|
2. Apply migration to DEV (`supabase db push` against `vshcimexazocttjmyrqy`), regenerate types.
|
|
3. Exercise the RPC directly against DEV Postgres (SQL editor / `supabase db execute`): non-admin `auth.uid()` rejected; `in_progress`/`no_pitch` ticket rejected; same-state no-op rejected; a successful call leaves exactly one new `upsell_state_changes` row **and** the ticket's `state`/`initiation_type` updated together in the same query check.
|
|
4. `npm run dev` as admin/owner: confirm the Offer Verification table now lists both accepted and declined_hard tickets. Open the kebab on an accepted ticket → "Change Status" → override to Rejected with a reason → confirm the row's status updates on refresh → reopen "Change Status" → confirm the new history row (old→new, your name, reason) appears above/below the override controls → reopen "Verify" → confirm `initiation_type` reset (Check 2 chips unselected).
|
|
5. Confirm the cooloff side effect: pick a client whose latest ticket is the one just reclassified, run a fresh Upsell Evaluator lookup for that GAID, confirm the eligibility banner reflects the new cooloff window.
|
|
|
|
## Git workflow
|
|
|
|
New branch from `dev`: `feat/gfiber-683-offer-state-override`, per this repo's convention (branch from `main`... actually per `CLAUDE.md` this repo branches feature work from `main`, and periodically `main`→`dev` is merged for DEV testing — confirm target base branch matches how GFIBER-689/GFIBER-648 branches were structured before opening the PR).
|