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 `