Repository navigation
Fix #1303: wrap admin/admins advanced-search form in collapsible <details> - #1318
Merged
Merged
Conversation
…#1303) The post-#1207 ADM-4 advanced-search card on Admin > Admin Management always rendered fully expanded above the admin list. Nine filter rows pushed the actual list well below the fold for the common unfiltered case while filtering is occasional. Wrap the form in a default-collapsed `<details class="card filters-details">` disclosure so the unfiltered list reaches above the fold; auto-expand on a post-submit paint so the filter chrome (and the Clear-filters affordance) stays visible while the user iterates. - View DTO carries paired `int $active_filter_count` + `bool $has_active_filters` properties; the page handler increments the count once per populated value slot. Match-mode toggles (`name_match` / `steam_match` / `admemail_match`) deliberately don't count: they always carry a default ('0' or '1') and only refine the matching filter, they don't filter on their own. - `<summary>` mirrors `core/admin_sidebar.tpl`'s mobile accordion vocabulary (filter icon + label + count badge + chevron with 180° rotation on `[open]`). The `prefers-reduced-motion: reduce` global rule already collapses the chevron transition to ~0ms; an extra per-rule override in the new `.filters-details` block stays defensive parity. - Reusable `.filters-details` shape — the public banlist / commslist filter bars (`page_bans.tpl` / `page_comms.tpl`) are candidates for the same shape per the issue body's notes; left for a follow-up. - Tests: 4 new PHPUnit cases lock the disclosure contract end-to-end (default-closed, auto-open with active filter, count matches populated slots and ignores match-mode toggles, empty multi-select arrays don't lift the count). 4 new Playwright specs lock the browser chrome (default-closed, opens on toggle click, auto-opens on URL with active filters, multi-filter URL count badge tracks). The existing density spec opens the disclosure before driving the form (the `[open]` attribute flips synchronously on click — no animation-driven sentinel). - AGENTS.md "Where to find what" gets a row for the new collapsible filter pattern; the existing admin-admins advanced-search row is updated to flag the `$active_filter_count` increment as a paired edit when adding new filter slots. Fixes #1303
…ount by perm) Two small fixes on top of the #1303 collapsible advanced-search disclosure: - The `<summary>` already paints the "Advanced search" title (+ chevron + count badge), so the form's `card__header` emitting another `<h3>Advanced search</h3>` directly under it stacked two identical headings the moment a user opened the disclosure. Drop the H3; keep the explanatory paragraph (the load-bearing AND-semantics copy from the #1207 ADM-4 audit has no analog in the summary). - The `admemail` filter is permission-gated by `EditAdmins | Owner` in both the form template AND the page handler (`admin.admins.php` ignores `?admemail=` from a user without the perm). The active-filter count was happily incrementing for that slot regardless of perm, so URL forgery (or a stale tab from a permission downgrade) painted "1 active" on the disclosure summary while every visible filter row read empty AND the disclosure auto-opened exposing nothing actionable. Mirror the gate locally so the count stays an honest summary of what the visible form actually filters on. Regression guard: `testDisclosureCountIgnoresPermissionGatedEmailSlot` (logs in as `ADMIN_LIST_ADMINS`-only and forges `?admemail=alice`, asserts zero count + closed disclosure + no e-mail input row).
rumblefrog
added a commit
that referenced
this pull request
May 10, 2026
…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.
rumblefrog
added a commit
that referenced
this pull request
May 10, 2026
…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.
Rushaway
pushed a commit
to srcdslab/sourcebans-pp
that referenced
this pull request
May 15, 2026
…ban-meta + Re-apply + collapsible adv search) (sbpp#1330) * fix(banlist+commslist): restore unban-meta line + Re-apply + collapsible advanced search (sbpp#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 sbpp#1301 / sbpp#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 sbpp#1303 introduced for admin-admins (CSS rules ship in this PR; will dedupe harmlessly when sbpp#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. * fix(tests): update integration tests after rebase on main Two test-only fixes surfaced when the PR rebased on top of sbpp#1320 + - `BanListIpColumnTest::renderBanList` (from sbpp#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 sbpp#1326) asserted a literal `"'mapimg' => GetMapImage("` substring in `web/api/handlers/servers.php`. sbpp#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. * fix(tests): login as admin in BanListIpColumnTest hideplayerips=false case sbpp#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 sbpp#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.
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 #1303.
Summary
The post-#1207 ADM-4 advanced-search card on Admin → Admin Management
always rendered fully expanded above the admin list. Nine filter rows
(login + match-mode, SteamID + match-mode, e-mail + match-mode, web
group, SourceMod admin group, server group, web-permissions
multi-select, server-permissions multi-select, server) plus a footer
pushed the actual list well below the fold for the common unfiltered
case while filtering is occasional.
This wraps the form in a default-collapsed
<details class="card filters-details">disclosure so the unfiltered list reaches above thefold; the disclosure auto-expands on a post-submit paint so the filter
chrome (and the Clear-filters affordance) stays visible while the user
iterates. Wire format and AND semantics are unchanged; only the chrome
wrapping the form is new.
AdminAdminsSearchView) gains pairedint $active_filter_count+bool $has_active_filtersproperties; thepage handler increments the count once per populated value slot.
Match-mode toggles (
name_match/steam_match/admemail_match)deliberately don't count: they always carry a default ('0' or '1')
and only refine the matching filter, they don't filter on their own.
<summary>mirrorscore/admin_sidebar.tpl's mobile accordionvocabulary (filter icon + label + count badge + chevron with 180°
rotation on
[open]). Theprefers-reduced-motion: reduceglobalrule already collapses the chevron transition to ~0ms; an explicit
per-rule override in the new
.filters-detailsblock staysdefensive parity.
.filters-detailsis intentionally generic sothe public banlist / commslist filter bars (
page_bans.tpl/page_comms.tpl) can adopt the same pattern per the issue body'snotes; left for a follow-up.
collapsible filter pattern; the existing admin-admins
advanced-search row is updated to flag the
$active_filter_countincrement as a paired edit when adding new filter slots.
Test plan
All gates run locally against the isolated
sbpp-task-1303stack:./sbpp.sh phpstan— pass (228 files, no errors)./sbpp.sh test— pass (407 tests, 1779 assertions; the 15AdminAdminsSearchTest cases include 4 new disclosure-shape tests:
default-closed, auto-open with active filter, count matches
populated slots and ignores match-mode toggles, empty multi-select
arrays don't lift the count)
./sbpp.sh ts-check— pass (no errors)./sbpp.sh composer api-contract— clean (no diff; the patchdoesn't touch any handler)
./sbpp.sh e2e --grep "admin/admins"— pass (12 tests; 4 newdisclosure E2E specs + the existing density specs, including the
one updated to open the disclosure before driving the form)
Selectors anchor on the new
data-testidhooks(
search-admins-disclosure,search-admins-toggle,search-admins-active-count) per AGENTS.md's "Selectors must use#1123's testability hooks" rule. The native
<details>[open]attribute flips synchronously on click, so no
setTimeout/animation-driven sentinel is needed.