Repository navigation
Fix #1310: drop dangling applyApiResponse refs in admin groups delete/save - #1325
Merged
Merged
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1310.
Summary
Three
then(function (r) { applyApiResponse(r); })callbacks inweb/themes/default/page_admin_groups_list.tplreferenced a global helper that was deleted along withweb/scripts/sourcebans.jsat #1123 D1. The server-sidegroups.remove/groups.editcalls still fired and completed, but the.then(...)callback threw an unhandledReferenceErrorbeforewindow.SBPP.showToastever ran — clicking Delete on a group hung the page with a red console error and no UI confirmation that anything happened.The three call sites:
SbppGroupsSave(line 401, save / edit path)SbppGroupsDelete(line 408, the path the report exercises)SbppServerGroupsDelete(line 414, server admin / server group delete)The fix replaces the three callbacks with the canonical inline response handler already used by the sibling
page_admin_groups_add.tpl'sSbppGroupsAddcallback:window.SBPP.showToast,sb.message.*fallback for legacy themes that never wired SBPP,r.data.reload(set bygroups.editafter a save), andgroups.removewhose envelope only carriesmessage.redir(nodata.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
SbppGroupsApplyResponsehelper so all three callers share one definition.Out of scope
The defensive
typeof window.applyApiResponse === 'function'guard inpage_admin_bans_email.tpl:123is 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, as the issue itself notes.Docs
No
AGENTS.md/ARCHITECTURE.mdupdates needed per the "Keep the docs in sync" table — this is a pure bug fix that does not change a subsystem, request lifecycle, schema, or convention. Thesourcebans.jsanti-pattern entry already calls out exactly this class of zombie reference.Test plan
Local quality gates (mirror CI):
./sbpp.sh phpstan— pass (228/228 files, no errors)../sbpp.sh test— pass (403 tests, 1765 assertions; the one PHPUnit deprecation pre-exists this PR)../sbpp.sh ts-check— pass (no.jssource changes; the inline JS in.tplis not in scope oftsc --checkJs)../sbpp.sh e2e --grep "admin groups" --project=chromium --workers=1— pass:flows/admin-groups-delete.spec.ts(new, this PR) — deletes the seeded group, asserts nopageerror, asserts the success toast surfaces, asserts the row is gone after the redirect.flows/admin-groups-bitmask.spec.ts(Web admin groups bitmask preview goes negative when high-bit flags toggled — JS|=returns Int32 #1272) — still green../sbpp.sh e2e specs/smoke/admin/groups.spec.ts --project=chromium --workers=1— pass.The
--workers=1flag mirrors CI; with the local default ofworkers > 1, the sibling bitmask spec races itself against the truncate-and-reseed in this spec'sbeforeEach(the documentedworkers: 1constraint in AGENTS.md "Playwright E2E specifics").