fix: address the feature review — accurate copy, reachable controls, real coverage

Round 1 on the feature diff found 15; the substantive ones:

- The delete control VANISHED on any invite whose address had registered, which
  is the ordinary sign-up shape (register writes the User and the pending invite
  together). The guard stays broad — that row is the account's approval gate, and
  orphaning it strands someone who can then neither sign in nor be approved — but
  the control is now a disabled button carrying the reason, on both lists, so an
  admin never sees a row silently missing a button its neighbours have.
- Deleting an entry is not always a revoke: is_allowed falls back to the env
  ALLOWED_EMAILS / ADMIN_EMAILS lists once no row exists, and an ADMIN_EMAILS
  address is force-promoted on its next Google sign-in. The route now reports
  still_allowed_by_env and the UI says so instead of claiming access was removed.
- "Deny keeps the row so they can't immediately re-ask" was simply false —
  upsert_pending_invite reopens a denied row. Corrected everywhere it was stated.
- The Configuration warning reads me.email_configured, which save() did not
  invalidate, so it kept warning about fields the admin had just filled in.
- The read-only variant was an aria-hidden span explained only by a Tooltip that
  the hints toggle can switch off; now a disabled button with title + aria-label.
- The delete error path threw away the 409's reason (the stale-list case) and
  left the stale row on screen.
- The throttle window was spent even when no notice was written, silencing the
  next hour on the path whose job is to stop silent drops.
- email_enabled() opened a second session and decrypted the SMTP password on
  every /api/me poll; it now takes the caller's session.
- welcomeMode.dom.test.ts never rendered the card, so reverting the history
  wiring to useState left it green. A render test now drives the real links and
  fails on that revert (verified).

Plus /api/me flag tests, the registration-shaped pending case, the env-allowlist
case, and a release note that matches what the code does.
This commit is contained in:
2026-09-01 23:31:49 +02:00
parent 44f626f732
commit 85043013f0
14 changed files with 298 additions and 55 deletions
+43 -12
View File
@@ -3,7 +3,7 @@ import { qk } from "../lib/queryKeys";
import { useTranslation } from "react-i18next";
import { useMutation, useQuery, useQueryClient } from "@tanstack/react-query";
import { Ban, Check, RotateCcw, Shield, Trash2, UserPlus, X } from "lucide-react";
import { api, type AdminUserRow, type Invite, type Me } from "../lib/api";
import { api, HttpError, type AdminUserRow, type Invite, type Me } from "../lib/api";
import { notify } from "../lib/notifications";
import { ADMIN_USERS_TAB_KEY } from "../lib/adminUsersTab";
import Tooltip from "./Tooltip";
@@ -301,11 +301,26 @@ function AdminInvites() {
};
const remove = useMutation({
mutationFn: (id: number) => api.deleteInvite(id),
onSuccess: (_d, _id) => {
onSuccess: (d) => {
refresh();
notify({ level: "success", message: t("settings.invites.deleted") });
// Removing the row is not always a revoke: is_allowed falls back to the env allow-lists, so
// for an address on those the person keeps access. Saying "removed" alone would mislead.
if (d.still_allowed_by_env)
notify({ level: "warning", message: t("settings.invites.deletedStillAllowed") });
else notify({ level: "success", message: t("settings.invites.deleted") });
},
onError: (e) => {
// A 409 means the list was stale — someone registered on that address since it loaded. Say
// what to do instead of a generic failure, and refetch so the row re-renders as read-only.
const conflict = e instanceof HttpError && e.status === 409;
notify({
level: "error",
message: t(
conflict ? "settings.invites.deleteHasAccount" : "settings.invites.deleteFailed",
),
});
if (conflict) refresh();
},
onError: () => notify({ level: "error", message: t("settings.invites.deleteFailed") }),
});
const onDelete = async (i: { id: number; email: string }) => {
const ok = await confirm({
@@ -395,11 +410,15 @@ function AdminInvites() {
{/* An entry with an account behind it stays read-only: deleting it would revoke that
user's access, since is_allowed reads this row. Delete the user instead. */}
{i.has_account ? (
<Tooltip hint={t("settings.invites.deleteHasAccount")}>
<span className="shrink-0 p-1 text-muted/50" aria-hidden>
<Trash2 className="w-3.5 h-3.5" />
</span>
</Tooltip>
<button
type="button"
disabled
title={t("settings.invites.deleteHasAccount")}
aria-label={t("settings.invites.deleteHasAccount")}
className="shrink-0 p-1 text-muted/50 cursor-not-allowed"
>
<Trash2 className="w-3.5 h-3.5" />
</button>
) : (
<Tooltip hint={t("settings.invites.deleteHint")}>
<button
@@ -591,9 +610,21 @@ function InviteRow({
<X className="w-4 h-4" />
</button>
</Tooltip>
{/* Deny keeps the row (so the person can't immediately re-ask); delete forgets the address
entirely. Withheld once an account exists — that would revoke a live user's access. */}
{!inv.has_account && (
{/* Deny records a decision and keeps the row; delete forgets the address. Once an account
exists for it the row is that account's approval gate, so it is managed from Users & roles
instead — shown as a disabled control rather than omitted, or the admin sees a row that is
silently missing a button their other rows have. */}
{inv.has_account ? (
<button
type="button"
disabled
title={t("settings.invites.deleteHasAccount")}
aria-label={t("settings.invites.deleteHasAccount")}
className="shrink-0 p-1.5 rounded-lg border border-border/50 text-muted/50 cursor-not-allowed"
>
<Trash2 className="w-4 h-4" />
</button>
) : (
<Tooltip hint={t("settings.invites.deleteHint")}>
<button
onClick={onDelete}
+4
View File
@@ -103,6 +103,10 @@ export default function ConfigPanel() {
}
}
await qc.invalidateQueries({ queryKey: qk.adminConfig() });
// The email-not-configured notice reads me.email_configured, which is otherwise only
// refetched on its own 60s interval — without this it keeps warning about fields the admin
// has just filled in, which reads as the save not having taken.
await qc.invalidateQueries({ queryKey: qk.me() });
setReseedToken((n) => n + 1); // re-seed the draft from the saved server state
setSaveState("saved");
setTimeout(() => setSaveState((s) => (s === "saved" ? "idle" : s)), 2500);
@@ -0,0 +1,78 @@
// @vitest-environment jsdom
//
// The sign-in card's sub-views as the user meets them: clicking a link must leave a history entry,
// so Back — including the mouse's back button — returns to the previous view. They used to be plain
// React state, and Back moved only the address bar while the form on screen stayed put. The
// encode/decode table lives in welcomeMode.dom.test.ts; this file renders the real card, so a revert
// to useState fails here.
//
// i18n is mocked to a key-passthrough so queries read stable keys, and the API module is stubbed —
// the card probes /auth/config on mount and must not reach the network.
import { describe, it, expect, afterEach, beforeEach, vi } from "vitest";
import { render, screen, fireEvent, cleanup, act } from "@testing-library/react";
vi.mock("react-i18next", async (importOriginal) => {
const actual = await importOriginal<typeof import("react-i18next")>();
return {
...actual,
useTranslation: () => ({ t: (key: string) => key, i18n: { language: "en" } }),
};
});
vi.mock("../lib/api", async (importOriginal) => {
const actual = await importOriginal<typeof import("../lib/api")>();
return {
...actual,
api: { ...actual.api, authConfig: () => Promise.resolve({ google_enabled: false }) },
};
});
import Welcome from "./Welcome";
const back = () =>
act(() => {
// jsdom's history.back() is async and does not rewind location, so land on the previous entry
// the way the browser would and fire popstate. The push itself is asserted via history.length.
window.history.replaceState(null, "", "/");
window.dispatchEvent(new PopStateEvent("popstate", { state: null }));
});
beforeEach(() => window.history.replaceState(null, "", "/"));
afterEach(() => {
cleanup();
window.history.replaceState(null, "", "/");
});
describe("sign-in card sub-views and the Back button", () => {
it("opens Forgot password as a history entry and comes back to sign in", async () => {
render(<Welcome />);
expect(await screen.findByText("welcome.auth.signinTitle")).toBeTruthy();
const entries = window.history.length;
fireEvent.click(screen.getByText("welcome.auth.forgotLink"));
expect(screen.getByText("welcome.auth.forgotTitle")).toBeTruthy();
expect(window.history.length).toBe(entries + 1); // a Back now has somewhere to go
expect(new URLSearchParams(window.location.search).get("m")).toBe("forgot");
back();
expect(screen.getByText("welcome.auth.signinTitle")).toBeTruthy();
expect(screen.queryByText("welcome.auth.forgotTitle")).toBeNull();
});
it("does the same for Create an account and the verification-email link", async () => {
render(<Welcome />);
await screen.findByText("welcome.auth.signinTitle");
fireEvent.click(screen.getByText("welcome.auth.createLink"));
expect(screen.getByText("welcome.auth.createTitle")).toBeTruthy();
back();
expect(screen.getByText("welcome.auth.signinTitle")).toBeTruthy();
fireEvent.click(screen.getByText("welcome.auth.resendLink"));
expect(screen.getByText("welcome.auth.resendTitle")).toBeTruthy();
back();
expect(screen.getByText("welcome.auth.signinTitle")).toBeTruthy();
});
});