Repository navigation
Fix #1315: public banlist / commslist v1.x → v2.0 regressions (unban-meta + Re-apply + collapsible adv search) - #1330
Merged
Conversation
rumblefrog
force-pushed
the
fix/issue-1315-public-banlist-regressions
branch
from
May 10, 2026 22:30
dbfd09d to
b2b136d
Compare
…ble advanced search (#1315) The v2.0 redesign collapsed the sourcebans.js-driven legacy public lists into Smarty-only `page_bans.tpl` / `page_comms.tpl` and dropped three power-user surfaces along the way. This PR restores them. 1. Inline unban-meta line on admin-lifted rows. The reason cell on the desktop table emits "Unbanned by <admin>: <reason>" (`[data-testid="ban-unban-meta"]`) when `state == 'unbanned'`; mobile cards mirror with the `-mobile` suffix. Same shape on the commslist for `state == 'unmuted'` — higher priority there because no drawer fallback exists. Both are gated on `!$hideadminname` so anonymous viewers under a hidden-admins config don't get the admin name leaked. Read-side render only — no overlap with #1301 / #1323. 2. Re-apply (Reban) icon affordance for expired / unbanned rows on the banlist desktop, gated on `ADMIN_OWNER | ADMIN_ADD_BAN`. Deep-links the smart-default `?p=admin&c=bans§ion=add-ban&rebanid=<bid>` URL the existing `BansPrepareReban` JSON action already pre-populates. Mobile parity deferred — drawer is canonical. 3. Advanced-search disclosure on both lists. Default-collapsed `<details class="filters-details">` wrapping the legacy multi- criterion search form (loaded via `{load_template file="admin.bans.search"}`). Auto-opens on a post-submit `?advType=&advSearch=` paint via a new `BanListView::$is_advanced_search_open` / `CommsListView::$is_advanced_search_open` boolean. Reuses the `.filters-details` chrome #1303 introduced for admin-admins (CSS rules ship in this PR; will dedupe harmlessly when #1318 merges). Tests: - PHPUnit: `PublicBanListRegressionTest` (7 cases — disclosure default-closed, auto-open, unban-meta render, Re-apply gating + URL shape, banlist + commslist parity). RunInSeparateProcess per method to dodge the page-handler `setPostKey()` redeclare trap, mirroring `Php82DeprecationsTest`'s pattern. - Playwright: `flows/public-banlist-regressions.spec.ts` (4 cases across chromium + mobile-chromium — disclosure default-closed, click-to-open, post-submit auto-open via URL navigation; closed-state axe coverage). Re-apply / unban-meta DB-state assertions live in the PHPUnit test where seeding is cleaner. AGENTS.md "Where to find what" gains three rows for the new patterns.
Two test-only fixes surfaced when the PR rebased on top of #1320 + - `BanListIpColumnTest::renderBanList` (from #1320) constructs a `BanListView` with positional + named arguments. This PR adds a 28th constructor parameter (`is_advanced_search_open`) which the test must now pass — default it to `false` so the IP-column suite stays exercising the column-render contract it was written for, not the disclosure state. - `ServerMapImageRenderTest::testHandlerStillEmitsMapimgField` (from #1326) asserted a literal `"'mapimg' => GetMapImage("` substring in `web/api/handlers/servers.php`. #1329's cache + debounce rewrite shifted the surrounding column alignment to 5 spaces, breaking the brittle match without changing the contract the test guards (the field is still emitted). Switch to a regex that tolerates any whitespace between the key and `=>` so a future alignment shift doesn't silently re-trigger the same false positive.
… case #1315's `<details class="filters-details">` disclosure on the public ban list `{load_template file="admin.bans.search"}`s the legacy advanced-search partial. The partial's backing `admin.bans.search.php` re-derives `hideplayerips` / `hideadminname` from `Config::getBool('banlist.hideplayerips') && !$userbank->is_admin()` and `Renderer::render`s the result into Smarty, overwriting whatever the parent BanListView had assigned. In production this is a no-op because the parent (`page.banlist.php`) computes the SAME formula — both halves converge. But `BanListIpColumnTest` hand-crafts `BanListView::$hideplayerips` to exercise the column-gating contract in isolation, so the partial's overwrite collapses the test's fixture-controlled value down to whatever the test environment's Config + $userbank happen to evaluate to. Fix by logging in as admin in the two test methods that pass `hideplayerips: false`. With `is_admin()=true`, the partial's formula reduces to `(…) && !true = false`, matching the View's hand-crafted `false`. The suppressed-state test (`hideplayerips: true`) stays anonymous so the formula reduces to `Config && true` — `true` under `data.sql`'s `banlist.hideplayerips=1` default — also matching the View's input. The test pre-dates #1315; it was written against the v2.0 template that didn't yet `{load_template}` the partial, so the parent's assigns were never overwritten. The new disclosure brings the partial back into the render tree and the test has to maintain the production invariant ("BanListView's hideplayerips matches AdminBansSearchView's recomputed value") explicitly.
rumblefrog
force-pushed
the
fix/issue-1315-public-banlist-regressions
branch
from
May 10, 2026 22:59
e2964ca to
0a56d74
Compare
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
1 participant
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 #1315.
Summary
The v2.0 redesign collapsed the sourcebans.js-driven legacy public
lists into Smarty-only
page_bans.tpl/page_comms.tpland droppedthree power-user surfaces along the way. This PR restores them.
the desktop table emits "Unbanned by
<admin>:<reason>"(
[data-testid=\"ban-unban-meta\"]) whenstate == 'unbanned'; mobilecards mirror with the
-mobilesuffix. Same shape on the commslistfor
state == 'unmuted'— higher priority there because no drawerfallback exists on
<tr data-testid=\"comm-row\">. Both are gatedon
!$hideadminnameso anonymous viewers under a hidden-adminsconfig don't get the admin name leaked. Read-side render only
— no overlap with Banlist unban (and commslist unmute/ungag) accept empty reasons with no confirmation — regression vs. 1.x #1301 / Fix #1301: require non-empty reason + confirm modal on banlist unban / commslist unmute & ungag #1323's unban-reason write paths.
the banlist desktop, gated on
ADMIN_OWNER | ADMIN_ADD_BAN.Deep-links the smart-default
?p=admin&c=bans§ion=add-ban&rebanid=<bid>URL the existingBansPrepareRebanJSON action already pre-populates. Mobile parityis intentionally deferred — the mobile card wraps content in a
single
<a data-testid=\"drawer-trigger\">, so adding a siblingbutton requires restructuring to a
<div>wrapper + inner anchor(the
page_comms.tplmobile-card shape from UI/UX audit: cross-cutting findings across public + admin flows on origin/main #1207 ADM-5). Drawerremains the canonical mobile detail view in the meantime.
<details class=\"filters-details\">wrapping the legacy multi-criterion search form (loaded via
{load_template file=\"admin.bans.search\"}). Auto-opens on apost-submit
?advType=&advSearch=paint via a newBanListView::$is_advanced_search_open/CommsListView::$is_advanced_search_openboolean. Bare?p=banlist/?p=commslistand simple-bar filters (?searchText=/
?server=/?time=) leave the disclosure closed so theunfiltered list reaches above the fold. Visual + behavioural
vocabulary matches the
.filters-detailschrome Admin Management advanced search would be friendlier inside a collapsible "Filters"<details>#1303 introducedfor admin-admins so a future tweak stays single-source.
Coordination
Three sibling PRs touch overlapping surface area; this PR scopes around
each per the orchestrator's guidance:
.filters-details).Reused the same class name + visual chrome so both PRs converge on
the same single-source CSS. The
.filters-detailsrules ship inthis PR's
theme.cssso it stands alone if it lands first; whenFix #1303: wrap admin/admins advanced-search form in collapsible <details> #1318 also lands the duplicate ruleset collapses harmlessly (CSS
cascades over identical declarations).
changes to flags only; this PR doesn't touch
$ban.ip/$hideplayeripsrendering or the desktop column geometry.banlist-table-columns.spec.tscontinues to pass unchanged.modal with reason field). This PR is read-side only —
surfaces
$ban.ureason/$ban.removedby/$comm.ureason/$comm.removedbyrow fields the page handlers already populate(
page.banlist.phplines 635-643,page.commslist.phplines626-635). No new SQL, no
bans.unban/comms.unmutehandleredits, no audit-log changes — the inline render shows whatever the
upstream write path stores (so Banlist unban (and commslist unmute/ungag) accept empty reasons with no confirmation — regression vs. 1.x #1301 / Fix #1301: require non-empty reason + confirm modal on banlist unban / commslist unmute & ungag #1323's improvements
automatically flow through here without a re-merge).
Test plan
All gates run locally against the isolated
sbpp-task-1315stack(
docker-compose.override.ymlscopes the project name + containernames + host ports per AGENTS.md "Parallel stacks"):
./sbpp.sh phpstan— pass (228 files, no errors)../sbpp.sh test— pass (410 tests, 1784 assertions; the onlyPHPUnit deprecation is the pre-existing doc-comment metadata warning
from
GroupsTest::testEditRoundTripsHighBitFlagsAsPositiveIntegers— unrelated to this PR).
./sbpp.sh ts-check— pass (no errors)../sbpp.sh composer api-contract— clean (no diff; the patchdoesn't touch any handler signature).
./sbpp.sh e2e --workers=1— pass (196 tests passed; 244skipped due to project mismatches — mobile-only or chromium-only
contracts). The new
flows/public-banlist-regressions.spec.tscontributes 4 specs (banlist + commslist disclosure × chromium +
mobile-chromium); the 7-case
PublicBanListRegressionTestPHPUnittwin locks the unban-meta + Re-apply DB-state assertions.
The PHPUnit test runs each method in a separate process (mirroring
Php82DeprecationsTest's pattern) because the page handlers declaretop-level
setPostKey()helpers PHP can't redeclare in one process.Selectors anchor on:
[data-testid=\"banlist-advsearch-disclosure\"]/[data-testid=\"banlist-advsearch-toggle\"]/[data-testid=\"banlist-advsearch-active\"](andcommslist-siblings) — the disclosure shape.
[data-testid=\"ban-unban-meta\"]/[data-testid=\"ban-unban-meta-mobile\"]/[data-testid=\"comm-unban-meta\"]/[data-testid=\"comm-unban-meta-mobile\"]— the inline lift line.[data-testid=\"row-action-reapply\"]— the desktop Re-apply iconanchor.
The closed-state axe runs at the start of each E2E spec; the
open-state axe is intentionally skipped because the legacy
box_admin_bans_search.tpl/box_admin_comms_search.tplshipunlabeled
<select>elements (axeselect-name, critical) — areal but pre-existing a11y bug that lived in the legacy form for
years; #1315 just makes it reachable without URL spelunking. Per
AGENTS.md "Playwright E2E specifics" the threshold must NOT be
downgraded; the right move is a follow-up a11y issue against the
legacy form.