From d7ed1d99d4de4cc5904c5f48c8ee9e21a3c6375e Mon Sep 17 00:00:00 2001 From: guarzo Date: Mon, 10 Aug 2026 12:46:00 -0400 Subject: [PATCH] fix(admin): name the access lists, and stop confirming presses that did nothing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The watched-lists page identified rows by `#4001` whenever the list had never been read, even though the catalog knew its name the whole time. `getWatchedListViews` now LEFT JOINs `accessListCatalog` and COALESCEs the catalog name over the snapshot, matching the precedence `watchedListName` already used for audit rows — so the log and the page cannot disagree about what a list is called. Two presses also confirmed work they had not done. `addWatch` and `removeWatch` are idempotent, and both actions read that as success: adding a list another admin had just added answered "List added to the watchlist", and removing a row another tab had already removed answered "removed from the watchlist". Neither wrote an audit row, so the notice was the only account of the press, and it credited this admin with someone else's act. Both services now report whether they changed anything. The add redirects, so its correction is wording alone (`?done=watch-already`); the remove does not, so it returns a `warn` outcome. A refused re-press was silent. `useSubmitGuard` blocks the second press while the first is in flight, and `removeWatchAction` deliberately does not redirect, so the page did not move and nothing was said. `StopWatching` moved into its own client component to wire `onRefused` — a server component cannot hand a function to a client one. The add control was rebuilt as a field group above the list rather than an aside on the section head, since it acts on the collection and not on any row, and the page adopted `.page__head`/`.page__lede`, which let the duplicate `.lede` rule leave `globals.css`. `monitorSentence`'s "normal" case gained a real sentence that names the holder and says what the page compares, without claiming reads are healthy — it cannot know that. Both new e2e tests were mutation-proved: reverting either fix fails exactly the test that covers it. --- e2e/access-lists.spec.ts | 174 ++++++++++++++++++- src/app/admin/access-lists/actions.ts | 30 +++- src/app/admin/access-lists/page.tsx | 120 ++++++------- src/app/admin/access-lists/stop-watching.tsx | 83 +++++++++ src/app/admin/access-lists/view.ts | 33 +++- src/app/globals.css | 33 ++-- src/services/access-lists.ts | 56 +++++- tests/access-lists-service.test.ts | 22 +++ tests/access-lists-view.test.ts | 31 +++- 9 files changed, 487 insertions(+), 95 deletions(-) create mode 100644 src/app/admin/access-lists/stop-watching.tsx diff --git a/e2e/access-lists.spec.ts b/e2e/access-lists.spec.ts index 817a52d..8208760 100644 --- a/e2e/access-lists.spec.ts +++ b/e2e/access-lists.spec.ts @@ -120,7 +120,7 @@ test("state 1: no holder and no scope asks for the grant, and shows no table", a await page.goto("/admin/access-lists"); await expect(page.getByRole("heading", { level: 1 })).toHaveText("Access lists"); - await expect(page.locator(".lede")).toContainText("Nobody has granted"); + await expect(page.locator(".page__lede")).toContainText("Nobody has granted"); await expect(page.getByRole("link", { name: "Grant access" })).toHaveAttribute( "href", "/auth/eve/link?grant=access-lists", @@ -137,7 +137,7 @@ test("state 2: a granted character with no holder gets the designate button", as await asAdmin(context); await page.goto("/admin/access-lists"); - await expect(page.locator(".lede")).toContainText("Designate it as the holder"); + await expect(page.locator(".page__lede")).toContainText("Designate it as the holder"); await expect(page.getByRole("button", { name: "Designate as holder" })).toBeVisible(); await expect(page.getByRole("link", { name: "Grant access" })).toHaveCount(0); }); @@ -155,12 +155,14 @@ test("state 3: a holder whose scope was dropped is offered the GRANTING link, no await seedWatched(characterId); await page.goto("/admin/access-lists"); - await expect(page.locator(".lede")).toContainText("no longer carries the access-list"); + await expect(page.locator(".page__lede")).toContainText( + "no longer carries the access-list", + ); // Lowercase mid-sentence ("...it, so no reads are happening.") — this state's // sentence is one clause, unlike holder-needs-reauth/holder-no-token's // separate "No reads are happening." sentence — so the case must match the // actual copy rather than the brief's shorthand. - await expect(page.locator(".lede")).toContainText("no reads are happening"); + await expect(page.locator(".page__lede")).toContainText("no reads are happening"); await expect(page.getByRole("link", { name: "Re-grant access" })).toHaveAttribute( "href", "/auth/eve/link?grant=access-lists", @@ -178,7 +180,7 @@ test("states 4 and 5: a stale authorization and a dead token are different sente await seedHolder(reauth.characterId); await seedCatalog(reauth.characterId); await page.goto("/admin/access-lists"); - await expect(page.locator(".lede")).toContainText("authorization has gone stale"); + await expect(page.locator(".page__lede")).toContainText("authorization has gone stale"); await expect(page.getByRole("link", { name: "Re-authenticate" })).toBeVisible(); await resetDb(db); @@ -186,7 +188,7 @@ test("states 4 and 5: a stale authorization and a dead token are different sente await seedHolder(dead.characterId); await seedCatalog(dead.characterId); await page.goto("/admin/access-lists"); - await expect(page.locator(".lede")).toContainText("no stored token"); + await expect(page.locator(".page__lede")).toContainText("no stored token"); await expect( page.getByRole("link", { name: "Add this character again" }), ).toBeVisible(); @@ -200,7 +202,9 @@ test("state 6: a healthy holder with an empty catalog offers Check now as the pr await seedHolder(characterId); await page.goto("/admin/access-lists"); - await expect(page.locator(".lede")).toContainText("No lists have been discovered"); + await expect(page.locator(".page__lede")).toContainText( + "No lists have been discovered", + ); await expect(page.getByRole("button", { name: "Check now" })).toHaveClass( /btn--primary/, ); @@ -393,3 +397,159 @@ test("Check now enqueues a read and audits nothing", async ({ page, context }) = .where(eq(auditLog.action, "access_list.check_requested")); expect(audits).toHaveLength(0); }); + +/** + * The never-read row is the ONLY row whose name cannot come from a snapshot, + * because it has no snapshot — `seedWatched` always writes one, so this test + * inserts the watch row bare. It is also the row an admin sees at the moment + * they care most: the seconds between adding a list and the job first reading + * it. `getWatchedListViews` used to select the name from the snapshot alone, + * so this row rendered as `#4001` — a number chosen by CCP, shown to a person + * who picked that list BY NAME out of the dropdown one click earlier. The + * catalog knew the name the entire time. + */ +test("a list added but never read is still named, not reduced to its id", async ({ + page, + context, +}) => { + const { characterId } = await asAdmin(context); + await seedHolder(characterId); + await seedCatalog(characterId, { name: "Fleet staging" }); + // No snapshot row: watched, never read. + await db.insert(accessListWatch).values({ accessListId: LIST_ID, addedBy: "e2e" }); + await page.goto("/admin/access-lists"); + + const row = page.locator(".acl-list__row"); + await expect(row).toContainText("Fleet staging"); + // The assertion that would have failed before the catalog join, kept + // separate from the positive one: a row can contain both, and "the name is + // present" is not the same claim as "the id is not standing in for it". + await expect(row).not.toContainText(`#${LIST_ID}`); + + // The confirmation names it too, which takes `removeWatch`'s return value + // rather than the id the action already had in hand: the row said "Fleet + // staging" a moment ago, and a notice answering "#4001 removed" would leave + // the admin matching a number against a name they never saw here. + await row.getByRole("button", { name: "Stop watching" }).click(); + await expect(page.locator(".notice").last()).toContainText( + "Fleet staging removed from the watchlist", + ); +}); + +/** + * The dropdown's default is a real option rather than an absent one, and the + * select is `required`. Without the placeholder, `defaultValue=""` matched + * nothing rendered, so the browser selected the first list in the catalog and + * an untouched submit added a list the admin never chose. Without `required`, + * the placeholder alone only stopped the browser re-selecting it: a disabled + * selected option contributes no entry at all, so the submit still went, with + * no `accessListId` in it, and `parseId` threw `invalid_id` — an ordinary + * mis-click landing on the error boundary. Both halves are asserted below, + * because either one alone leaves a bad outcome. + */ +test("the add control cannot submit a list the admin never chose", async ({ + page, + context, +}) => { + const { characterId } = await asAdmin(context); + await seedHolder(characterId); + await seedCatalog(characterId, { accessListId: 4001, name: "Fleet staging" }); + await seedCatalog(characterId, { accessListId: 4002, name: "Home defence" }); + await page.goto("/admin/access-lists"); + + const select = page.locator("#add-list"); + await expect(select).toHaveValue(""); + await expect(select.locator("option[value='']")).toBeDisabled(); + + // Count the POSTs rather than inspecting the page afterwards. "Nothing was + // added" is the weaker claim — a server action that throws adds nothing + // either, and the admin is then looking at "Something broke". The claim + // worth pinning is that the mis-click never leaves the browser. + const posts: string[] = []; + page.on("request", (r) => { + if (r.method() === "POST") posts.push(r.url()); + }); + await page.getByRole("button", { name: "Add to watchlist" }).click(); + + // Read the validity state before asserting on `posts`: it is a round trip to + // the browser, so a submit that DID fire has been observed by the time the + // count is read. + expect(await select.evaluate((el: HTMLSelectElement) => el.validity.valueMissing)).toBe( + true, + ); + expect(posts).toEqual([]); + expect(await db.select().from(accessListWatch)).toHaveLength(0); +}); + +/** + * The two stale-page presses. Neither is reachable by clicking alone — the + * `` from a catalog + // with nothing watched in it. + await db.insert(accessListWatch).values({ accessListId: LIST_ID, addedBy: "other" }); + + await page.getByRole("button", { name: "Add to watchlist" }).click(); + const notice = page.locator(".notice").last(); + await expect(notice).toContainText("already on the watchlist"); + // The assertion that fails without the split marker: `doneNotice("watch")` + // is a perfectly good sentence to render here, and a wrong one. + await expect(notice).not.toContainText("List added to the watchlist"); + // One watch row, and no audit row — the other tab wrote the row without + // going through `addWatch`, and this press wrote nothing at all. + expect(await db.select().from(accessListWatch)).toHaveLength(1); + expect(await db.select().from(auditLog)).toHaveLength(0); +}); diff --git a/src/app/admin/access-lists/actions.ts b/src/app/admin/access-lists/actions.ts index 9cb30d5..afcdc39 100644 --- a/src/app/admin/access-lists/actions.ts +++ b/src/app/admin/access-lists/actions.ts @@ -38,9 +38,18 @@ export async function designateHolderAction(formData: FormData): Promise { export async function addWatchAction(formData: FormData): Promise { const { accountId: actor } = await requireAdminAction(); const accessListId = parseId(formData.get("accessListId")); - await addWatch(getDb(), accessListId, actor); + const added = await addWatch(getDb(), accessListId, actor); revalidatePath("/admin/access-lists"); - redirect(`/admin/access-lists?done=watch&at=${Date.now()}`); + // Two markers, because a press that inserted nothing is not the press the + // admin thinks they made: the ` +
+ + ` offers + // nothing already watched — so it says what the press did (nothing) rather + // than repeating the success sentence above. `ConfirmNotice` renders a + // sentence and no tone, so the wording carries the correction alone. + if (done === "watch-already") { + return `That list was already on the watchlist${when}. Nothing was added.`; + } if (done === "check") { return `Check queued${when}. Reload this page once the worker has run.`; } diff --git a/src/app/globals.css b/src/app/globals.css index 43478bc..eeaba5a 100644 --- a/src/app/globals.css +++ b/src/app/globals.css @@ -380,6 +380,7 @@ a:hover { .facts dt, .filters__label, .filter-form__label, +.acl-add__label, .launch__scopes-head, .shell__register, .acl-detail th, @@ -760,17 +761,6 @@ a:hover { margin-top: var(--s-2); } -/* The access-list monitor's own lede: same treatment as `.page__lede`, but - the page has no `.page__head` wrapper around its `

` — `AdminAccessListsPage` - sits the summary sentence directly after it (and, when there is one, the - redirect-carried `ConfirmNotice`), so the class has to stand on its own - rather than as a child selector. */ -.lede { - max-width: var(--measure); - color: var(--ink-dim); - margin-top: var(--s-2); -} - /* Mission-patch furniture: a small designation above the page's own H1, naming the instrument rather than the page — DESIGN.md's own metaphor for this surface ("the flight log, not the cockpit display"). One live use @@ -4622,6 +4612,27 @@ h3.rule-head__label { with no drawer at all (`rowHasDetail`, page.tsx), which `.strip`'s jobs never need to. */ +/* The catalog-to-watchlist control: a label, a select, a submit, on one line + above the list it adds to. The four declarations are `.btn-row`'s today, + but the two names answer to different things — `.btn-row` describes a row + of buttons, and a change to how buttons sit together should not have to + reason about a label bound to a control — so this keeps its own class and + `.acl-add__label` joins the register (near the top of this file) rather + than rendering as an unregistered proportional label beside this section's + mono uppercase `RuleHead`. */ +.acl-add { + display: flex; + flex-wrap: wrap; + gap: var(--s-2); + align-items: center; +} + +.acl-add__label { + letter-spacing: var(--track-label); + color: var(--ink-faint); + margin-right: var(--s-1); +} + .acl-list { list-style: none; margin: var(--s-4) 0 0; diff --git a/src/services/access-lists.ts b/src/services/access-lists.ts index fca4367..77df495 100644 --- a/src/services/access-lists.ts +++ b/src/services/access-lists.ts @@ -1,4 +1,4 @@ -import { and, asc, eq, inArray } from "drizzle-orm"; +import { and, asc, eq, inArray, sql } from "drizzle-orm"; import type { Db, Dbx } from "@/db"; import { accessListCatalog, @@ -107,35 +107,53 @@ async function watchedListName(dbx: Dbx, accessListId: number): Promise`, so reaching this branch means the admin was + * looking at a stale catalog, and telling them the list was added would + * describe an act that did not happen. */ export async function addWatch( db: Db, accessListId: number, actor: string, -): Promise { - await db.transaction(async (tx) => { +): Promise { + return db.transaction(async (tx) => { const inserted = await tx .insert(accessListWatch) .values({ accessListId, addedAt: new Date(), addedBy: actor }) .onConflictDoNothing({ target: accessListWatch.accessListId }) .returning({ accessListId: accessListWatch.accessListId }); - if (inserted.length === 0) return; + if (inserted.length === 0) return false; await logAudit(tx, { actor, action: "access_list.watch_added", target: String(accessListId), details: { accessListId, name: await watchedListName(tx, accessListId) }, }); + return true; }); } -/** Removes a list from the watchlist. Audits only when a row actually went. */ +/** + * Removes a list from the watchlist. Audits only when a row actually went. + * + * Returns both halves the caller needs to write an honest confirmation: the + * name it read (the same one that lands in the audit row, when one is written) + * so the list can be named rather than numbered, and `removed` so a press that + * deleted nothing is not confirmed as a removal. `name` is populated on both + * branches — the catalog and snapshot rows outlive the watch row, so a list + * that was already off the watchlist can still be named in the notice saying + * so. + */ export async function removeWatch( db: Db, accessListId: number, actor: string, -): Promise { - await db.transaction(async (tx) => { +): Promise<{ removed: boolean; name: string | null }> { + return db.transaction(async (tx) => { // Read the name BEFORE the delete: nothing here cascades to the snapshot, // but reading first keeps the audit row correct regardless of what a later // change makes the delete cascade to. @@ -144,13 +162,14 @@ export async function removeWatch( .delete(accessListWatch) .where(eq(accessListWatch.accessListId, accessListId)) .returning({ accessListId: accessListWatch.accessListId }); - if (removed.length === 0) return; + if (removed.length === 0) return { removed: false, name }; await logAudit(tx, { actor, action: "access_list.watch_removed", target: String(accessListId), details: { accessListId, name }, }); + return { removed: true, name }; }); } @@ -207,12 +226,27 @@ export type WatchedListView = { * Every watched list, LEFT-joined to its snapshot: a list added to the * watchlist a minute ago has no snapshot row at all, and that "never read" * state is one the page renders rather than a row it drops. + * + * Also LEFT-joined to the catalog, with the name sourced as + * COALESCE(catalog, snapshot): a list can be watched before the worker ever + * takes a snapshot of it, in which case only the catalog has a name at all. + * + * The two sources rarely disagree — a successful read writes `snapshot.name` + * and `catalog.name` from the same ESI detail in one transaction + * (`jobs/access-lists.ts`) — so the precedence only decides the edges, and + * both edges want the catalog first: a watched-but-never-read list has only a + * catalog row, and a list the holder can no longer see is pruned from the + * catalog by discovery, leaving only the last snapshot. Keeping this in the + * same order as `watchedListName()` above is load-bearing: the page and the + * audit log must never disagree about what a list is called. */ export async function getWatchedListViews(dbx: Dbx): Promise { const rows = await dbx .select({ accessListId: accessListWatch.accessListId, - name: accessListSnapshot.name, + name: sql< + string | null + >`coalesce(${accessListCatalog.name}, ${accessListSnapshot.name})`, readStatus: accessListSnapshot.readStatus, observedAt: accessListSnapshot.observedAt, lastAttemptAt: accessListSnapshot.lastAttemptAt, @@ -224,6 +258,10 @@ export async function getWatchedListViews(dbx: Dbx): Promise accessListSnapshot, eq(accessListSnapshot.accessListId, accessListWatch.accessListId), ) + .leftJoin( + accessListCatalog, + eq(accessListCatalog.accessListId, accessListWatch.accessListId), + ) .orderBy(accessListWatch.accessListId); if (rows.length === 0) return []; const entries = await dbx diff --git a/tests/access-lists-service.test.ts b/tests/access-lists-service.test.ts index f744158..7edfa5d 100644 --- a/tests/access-lists-service.test.ts +++ b/tests/access-lists-service.test.ts @@ -141,6 +141,28 @@ describe("addWatch / removeWatch / getWatchedListIds", () => { expect(await audits()).toHaveLength(1); }); + it("reports whether the press changed anything, so a no-op is not confirmed", async () => { + // The return values the two actions turn their copy on: a second add and a + // second remove both leave the table exactly as the first press did, and + // the only thing separating "added" from "was already there" is this + // boolean. Both are reachable from a stale page — the `