From 7d10519875834e1447fc8f3b0bf33b78f6860654 Mon Sep 17 00:00:00 2001
From: rumblefrog <contact@zhenyangli.me>
Date: Sun, 10 May 2026 13:20:28 -0400
Subject: [PATCH] fix(admin/groups): drop dangling applyApiResponse refs
 (#1310)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Three `then(function (r) { applyApiResponse(r); })` callbacks in
`page_admin_groups_list.tpl` referenced a global helper that was
deleted along with `web/scripts/sourcebans.js` at #1123 D1. The
server-side `groups.remove` / `groups.edit` calls still fired and
completed, but the `.then(...)` callback threw an unhandled
`ReferenceError` before `window.SBPP.showToast` ever ran — so
clicking Delete on a group hung the page with a red console error
and no UI confirmation.

Replace the three callbacks with the canonical inline response
handler already used by the sibling `page_admin_groups_add.tpl`:

- success/error toast via `window.SBPP.showToast`,
- `sb.message.*` fallback for legacy themes that never wired SBPP,
- honours `r.data.reload` (set by `groups.edit` after a save), and
- — for handlers like `groups.remove` whose envelope only carries
  `message.redir` (no `data.reload`) — navigates to that URL after
  the toast so the master-detail editor stops pointing at the row
  that just got deleted.

The handler is factored into a small `SbppGroupsApplyResponse`
helper so all three callers share one definition.

Regression test: `web/tests/e2e/specs/flows/admin-groups-delete.spec.ts`
seeds a web admin group via `Actions.GroupsAdd`, deletes it, asserts
(a) no `pageerror` (the pre-fix `ReferenceError`), (b) a `success`
toast titled "Group Deleted" surfaces, and (c) the row is gone after
the post-delete redirect. Also tightens the `applyApiResponse`
comment in the existing #1272 bitmask spec — the toast contract is
now its own spec, this one stays focused on the wire-level
round-trip.

The defensive `typeof window.applyApiResponse === 'function'` guard
in `page_admin_bans_email.tpl:123` is a separate dead-code cleanup
(it never throws because of the guard, just no-ops a fallback path)
and is intentionally left out of scope here.
---
 .../specs/flows/admin-groups-bitmask.spec.ts  |  14 +-
 .../specs/flows/admin-groups-delete.spec.ts   | 178 ++++++++++++++++++
 web/themes/default/page_admin_groups_list.tpl |  58 +++++-
 3 files changed, 239 insertions(+), 11 deletions(-)
 create mode 100644 web/tests/e2e/specs/flows/admin-groups-delete.spec.ts

diff --git a/web/tests/e2e/specs/flows/admin-groups-bitmask.spec.ts b/web/tests/e2e/specs/flows/admin-groups-bitmask.spec.ts
index 1c48bd313..bafcede89 100644
--- a/web/tests/e2e/specs/flows/admin-groups-bitmask.spec.ts
+++ b/web/tests/e2e/specs/flows/admin-groups-bitmask.spec.ts
@@ -142,14 +142,12 @@ test.describe('flow: admin groups bitmask round-trip (#1272)', () => {
         // The Save button posts `Actions.GroupsEdit` with
         // `web_flags: <SbppFoldFlags result>`. We wait on the
         // network response (the deterministic terminal state for
-        // "save succeeded") rather than a UI toast — the inline
-        // success-toast wiring on this template is a separate concern
-        // outside #1272's scope (`SbppGroupsSave` calls a now-
-        // undefined `applyApiResponse`, a pre-existing dangling
-        // reference from the sourcebans.js removal at #1123 D1; the
-        // server-side save still completes, just without UI
-        // confirmation). The wire-level signal is what locks in the
-        // round-trip contract.
+        // "save succeeded") rather than a UI toast — the toast
+        // contract is locked in separately by
+        // `admin-groups-delete.spec.ts` (#1310 — replacement for the
+        // pre-#1310 dangling `applyApiResponse` reference); this
+        // spec stays focused on the wire-level round-trip that
+        // #1272 fixes.
         const saveButton = detail.locator('[data-testid="group-save"]');
         await expect(saveButton).toBeVisible();
 
diff --git a/web/tests/e2e/specs/flows/admin-groups-delete.spec.ts b/web/tests/e2e/specs/flows/admin-groups-delete.spec.ts
new file mode 100644
index 000000000..bfffe1a87
--- /dev/null
+++ b/web/tests/e2e/specs/flows/admin-groups-delete.spec.ts
@@ -0,0 +1,178 @@
+/**
+ * Flow spec — issue #1310: Delete-group on the admin groups list
+ * surfaces a success toast and navigates back to the list, with NO
+ * `ReferenceError: applyApiResponse is not defined` in the console.
+ *
+ * What this locks in
+ * ------------------
+ * Pre-#1310, all three inline `then(function (r) { applyApiResponse(r); })`
+ * callbacks in `web/themes/default/page_admin_groups_list.tpl` referenced
+ * a global helper that was deleted wholesale at #1123 D1 along with
+ * `web/scripts/sourcebans.js`. The actual server-side `groups.remove`
+ * fired and completed, but the `.then(...)` callback threw an unhandled
+ * `ReferenceError` before `window.SBPP.showToast` ever ran — so from the
+ * operator's POV the page just hung with a red console error and no
+ * confirmation that the group was deleted (see issue #1310 repro). #1310
+ * replaces the three callbacks with the canonical inline response handler
+ * already used by the sibling `page_admin_groups_add.tpl` (success/error
+ * toast via `window.SBPP.showToast`, fall back to `sb.message.*` for
+ * legacy themes, honour `data.reload` and — for handlers like
+ * `groups.remove` whose envelope only carries `message.redir` — navigate
+ * there after the toast so the master-detail editor stops pointing at
+ * the row that just got deleted).
+ *
+ * The two acceptance criteria asserted below:
+ *   1. Clicking Delete must NOT emit a `pageerror` (no `ReferenceError`).
+ *   2. A `success` toast titled "Group Deleted" must surface, and the
+ *      group row must be gone from the list after the post-delete redirect.
+ *
+ * Selectors
+ * ---------
+ * Per AGENTS.md "Selectors must use #1123's testability hooks":
+ *   - `[data-testid="web-groups-section"]`  — page mount signal.
+ *   - `[data-testid="group-list"]`          — left-rail row container.
+ *   - `[data-testid="group-row"]`           — per-group anchor row.
+ *   - `[data-testid="group-detail"]`        — right-pane editor form.
+ *   - `[data-testid="group-delete"]`        — the Delete button.
+ *   - `.toast[data-kind="success"]`         — toast wrapper (theme.js).
+ *
+ * Project gating
+ * --------------
+ * Pin to chromium (desktop). The reference-error contract holds at
+ * every viewport, but a flow spec mutating the shared `sourcebans_e2e`
+ * DB races with itself across projects (`workers: 1` is the suite-wide
+ * mitigation per AGENTS.md). The mobile chrome is structurally the same
+ * `<button onclick="SbppGroupsDelete(...)">` shape; doubling it would
+ * just retread the wire contract.
+ */
+
+import { expect, test } from '../../fixtures/auth.ts';
+import { truncateE2eDb } from '../../fixtures/db.ts';
+
+const GROUPS_LIST_ROUTE = '/index.php?p=admin&c=groups&section=list';
+
+const FIXTURE = {
+    groupName: 'e2e-delete-target',
+};
+
+test.describe('flow: admin groups delete (#1310 — applyApiResponse zombie)', () => {
+    test.skip(({ isMobile }) => isMobile, 'flow spec runs only on desktop chromium');
+
+    test.beforeEach(async () => {
+        await truncateE2eDb();
+    });
+
+    test('Delete group surfaces a success toast and emits no ReferenceError', async ({ page }) => {
+        // ---- 0. Capture every uncaught exception for the run -------------
+        // The pre-#1310 bug throws at the `.then()` callback as an
+        // uncaught Promise rejection that bubbles to `pageerror`.
+        // We assert the array is empty at the end of the test.
+        const consoleErrors: string[] = [];
+        page.on('pageerror', (err) => consoleErrors.push(err.message));
+
+        // ---- 1. Seed a web admin group via the JSON API ------------------
+        // Use `Actions.GroupsAdd` so the seed path mirrors the live
+        // dispatcher (CSRF + permissions + handler stack). type='1' is a
+        // web admin group; bitmask=0 so we land at a known empty state.
+        await page.goto('/');
+
+        const seedEnvelope = await page.evaluate(async (groupName) => {
+            const w = window as unknown as {
+                sb: {
+                    api: {
+                        call: (
+                            action: string,
+                            params: Record<string, unknown>,
+                        ) => Promise<{
+                            ok: boolean;
+                            error?: { code: string; message: string };
+                        }>;
+                    };
+                };
+                Actions: Record<string, string>;
+            };
+            return await w.sb.api.call(w.Actions.GroupsAdd, {
+                name: groupName,
+                type: '1',
+                bitmask: 0,
+                srvflags: '',
+            });
+        }, FIXTURE.groupName);
+
+        expect(
+            seedEnvelope.ok,
+            `groups.add must succeed: ${JSON.stringify(seedEnvelope)}`,
+        ).toBe(true);
+
+        // ---- 2. Navigate to the groups list -------------------------------
+        // Master-detail auto-selects the only row (admin.groups.php
+        // falls back to `web_group_list[0]` when ?gid= is missing or
+        // unknown). The right-pane editor mounts with the Delete button
+        // because the seeded admin/admin user has DeleteGroups.
+        await page.goto(GROUPS_LIST_ROUTE);
+
+        const detail = page.locator('[data-testid="group-detail"]');
+        await expect(detail).toBeVisible();
+
+        const seededRow = page
+            .locator('[data-testid="group-row"]')
+            .filter({ hasText: FIXTURE.groupName });
+        await expect(seededRow).toHaveCount(1);
+
+        // ---- 3. Click Delete + accept the native confirm() ----------------
+        // The inline JS calls `window.confirm()` before issuing the
+        // destructive call. Playwright dismisses dialogs by default;
+        // wire an accept handler so the click resolves the API path.
+        page.once('dialog', (d) => {
+            void d.accept();
+        });
+
+        const deleteResponsePromise = page.waitForResponse(
+            (response) =>
+                response.url().includes('api.php') &&
+                response.request().method() === 'POST' &&
+                response.status() === 200,
+        );
+
+        await detail.locator('[data-testid="group-delete"]').click();
+
+        const deleteResponse = await deleteResponsePromise;
+        const deleteEnvelope = await deleteResponse.json();
+        expect(
+            deleteEnvelope.ok,
+            `groups.remove must succeed: ${JSON.stringify(deleteEnvelope)}`,
+        ).toBe(true);
+
+        // ---- 4. Success toast appears ------------------------------------
+        // The post-#1310 handler routes `r.data.message` through
+        // `window.SBPP.showToast`. Anchor on `data-kind="success"`
+        // (set by theme.js's `showToast`) plus a hasText filter on
+        // the title the API returns ("Group Deleted").
+        const successToast = page
+            .locator('.toast[data-kind="success"]')
+            .filter({ hasText: /group deleted/i });
+        await expect(successToast).toBeVisible();
+
+        // ---- 5. After the post-delete redirect, the group is gone --------
+        // The handler's envelope has `message.redir =
+        // 'index.php?p=admin&c=groups'` (no &gid=). The post-#1310
+        // helper `setTimeout`s a navigation there 1500ms after the
+        // toast; we wait on the URL change rather than a wall-clock
+        // timer so a slower run doesn't flake.
+        await page.waitForURL(/\?p=admin&c=groups(?!.*&gid=)/);
+
+        const remainingRow = page
+            .locator('[data-testid="group-row"]')
+            .filter({ hasText: FIXTURE.groupName });
+        await expect(remainingRow).toHaveCount(0);
+
+        // ---- 6. Final regression assertion: no console errors ------------
+        // Pre-#1310 this would contain
+        // `ReferenceError: applyApiResponse is not defined` from the
+        // unhandled Promise rejection at the `.then(...)` callback.
+        expect(
+            consoleErrors,
+            `unexpected console errors:\n${consoleErrors.join('\n')}`,
+        ).toEqual([]);
+    });
+});
diff --git a/web/themes/default/page_admin_groups_list.tpl b/web/themes/default/page_admin_groups_list.tpl
index 8775dcd54..8a0785de0 100644
--- a/web/themes/default/page_admin_groups_list.tpl
+++ b/web/themes/default/page_admin_groups_list.tpl
@@ -386,6 +386,58 @@ function SbppFoldFlags(root) {
     return bitmask >>> 0;
 }
 
+/**
+ * Inline replacement for the legacy `applyApiResponse` global from
+ * `web/scripts/sourcebans.js` (deleted at #1123 D1, see AGENTS.md
+ * "Anti-patterns"). Mirrors the shape from `page_admin_groups_add.tpl`'s
+ * `SbppGroupsAdd` callback (#1310): error toast on `r.ok === false`,
+ * success toast from `r.data.message`, auto-reload on `r.data.reload`,
+ * and — for handlers whose envelope only carries `message.redir` (e.g.
+ * `groups.remove`, which does NOT set `data.reload`) — navigate there
+ * after the toast so the master-detail editor stops pointing at the
+ * row that just got deleted.
+ *
+ * `sb.api.call` already honours `r.redirect` (top-level navigation
+ * envelope) by setting `window.location.href` itself, so we just bail
+ * if the envelope had it set.
+ *
+ * @param {object|null|undefined} r The envelope from `sb.api.call`.
+ * @param {{defaultTitle?: string}} [opts]
+ */
+function SbppGroupsApplyResponse(r, opts) {
+    if (!r) return;
+    if (r.redirect) return;
+
+    var fallback = (opts && opts.defaultTitle) || 'Done';
+
+    if (r.ok === false) {
+        var em = (r.error && r.error.message) || 'Unknown error';
+        if (window.SBPP && typeof window.SBPP.showToast === 'function') {
+            window.SBPP.showToast({ kind: 'error', title: 'Error', body: em });
+        } else if (window.sb && window.sb.message) {
+            window.sb.message.error('Error', em);
+        }
+        return;
+    }
+
+    var data = r.data || {};
+    var msg = data.message || {};
+    var title = msg.title || fallback;
+    var body = msg.body || '';
+
+    if (window.SBPP && typeof window.SBPP.showToast === 'function') {
+        window.SBPP.showToast({ kind: 'success', title: title, body: body });
+    } else if (window.sb && window.sb.message) {
+        window.sb.message.success(title, body, msg.redir || '');
+    }
+
+    if (data.reload) {
+        setTimeout(function () { window.location.reload(); }, 1500);
+    } else if (msg.redir) {
+        setTimeout(function () { window.location.href = msg.redir; }, 1500);
+    }
+}
+
 function SbppGroupsSave(event) {
     event.preventDefault();
     var form = event.target;
@@ -398,20 +450,20 @@ function SbppGroupsSave(event) {
         web_flags: bitmask,
         srv_flags: '',
         type: 'web'
-    }).then(function (r) { applyApiResponse(r); });
+    }).then(function (r) { SbppGroupsApplyResponse(r, { defaultTitle: 'Group updated' }); });
     return false;
 }
 
 function SbppGroupsDelete(gid, name) {
     if (!confirm('Delete group "' + name + '"?')) return;
     sb.api.call(Actions.GroupsRemove, { gid: Number(gid), type: 'web' })
-        .then(function (r) { applyApiResponse(r); });
+        .then(function (r) { SbppGroupsApplyResponse(r, { defaultTitle: 'Group deleted' }); });
 }
 
 function SbppServerGroupsDelete(gid, name, type) {
     if (!confirm('Delete group "' + name + '"?')) return;
     sb.api.call(Actions.GroupsRemove, { gid: Number(gid), type: String(type) })
-        .then(function (r) { applyApiResponse(r); });
+        .then(function (r) { SbppGroupsApplyResponse(r, { defaultTitle: 'Group deleted' }); });
 }
 
 // --- Live bitmask preview (#1258) ---