Repository navigation
Fix #1301: require non-empty reason + confirm modal on banlist unban / commslist unmute & ungag - #1323
Merged
Conversation
…unmute / ungag (#1301) v1.x had two safeguards before lifting a ban or comm block: a confirm modal and a non-empty unblock-reason prompt. Both lived in the deleted `web/scripts/sourcebans.js` (UnbanBan / UnMuteBan / UnGagBan), and the v2.0 cutover left the row's affordance as a single click that silently sent an empty `ureason`. The audit log lost the *why* behind every block lift for ~18 months. This restores both safeguards on the v2.0 chrome: - New `<dialog id="bans-unban-dialog">` and `<dialog id="comms-unblock-dialog">` carrying a confirm prompt + required-reason textarea (`aria-required` rather than the native `required`, so the JS submit handler owns the empty-reason inline-error UX rather than ceding to the browser's validation popover). - New `bans.unban` JSON action mirrors the legacy GET handler's per-row precision check (Owner / Unban-all / Unban-own / Unban-group) and rejects empty `ureason` server-side. The existing `comms.unblock` gains the same empty-reason guard. Both legacy GET fallbacks (`?p=banlist&a=unban` / `?p=commslist&a=ungag` / `&a=unmute`) bounce empty reasons too so a hand-edited URL can't slip through the back door. - All four `Log::add(LogType::Message, ...)` audit-trail calls now carry the unblock reason in the message column, restoring v1.x parity. Tests: new PHPUnit coverage for `bans.unban` (anonymous reject, missing bid, empty reason, unknown bid, success path, already-lifted, audit log contents) + `comms.unblock` empty-reason rejection + audit log contents. PermissionMatrix locks the new dispatcher gate. The `admin-ban-lifecycle` E2E spec now exercises the modal (empty submit shows inline error, filled submit flips row in place + success toast); `comms-affordances` adopts the same modal interaction.
The handler was tightened in #1301 to reject empty `ureason` server-side but the docblock (and therefore the regenerated api-contract.js JSDoc) still claimed `(string, optional)`. Sync the prose with the runtime contract so the api-contract reader and any future caller doesn't miss the validation gate.
rumblefrog
pushed a commit
that referenced
this pull request
May 10, 2026
Resolve AGENTS.md conflict by keeping both new anti-pattern entries: the #1308 stopPropagation-on-data-copy + unconditional-success-toast entries from this PR, AND the #1301 reason-less-unban + native-required entries from #1323 (merged to main). Both pairs append to the same "Anti-patterns" list and don't overlap semantically.
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 #1301.
Summary
v1.x had two safeguards before lifting a ban or comm block: a confirm modal and a non-empty unblock-reason prompt. Both lived in the deleted
web/scripts/sourcebans.js(UnbanBan / UnMuteBan / UnGagBan), and the v2.0 cutover left the row's affordance as a single click that silently sent an emptyureason. The audit log lost the why behind every block lift for ~18 months.This PR restores both safeguards on the v2.0 chrome:
<dialog id="bans-unban-dialog">inpage_bans.tpland<dialog id="comms-unblock-dialog">inpage_comms.tpl. The textarea usesaria-required="true"(NOT the nativerequired) so the JS submit handler owns the empty-reason inline-error UX rather than ceding to the browser's validation popover. The dialog title + verb adapt to the comm-block type (mute / gag / silence). On success, the row flips in place via the existingflipRowToUnbanned/flipRowToUnmutedhelpers and a success toast fires.bans.unbanJSON action mirrors the legacy GET handler's per-row precision check (Owner / Unban-all / Unban-own / Unban-group), rejects emptyureasonserver-side, archives matching protests, fanssm_unbanto enabled servers, and records the reason in the audit log. The existingcomms.unblockgains the same empty-reason guard.?p=banlist&a=unbanand?p=commslist&a=ungag/&a=unmuteshort-circuit withShowBoxwhenureasonis empty, so a hand-edited URL can't slip through the back door.Log::add(LogType::Message, ...)audit-trail calls (legacy GET unban, legacy GET ungag/unmute, JSONbans.unban, JSONcomms.unblock) now carryReason: <ureason>in the message column, restoring v1.x parity.requiredon a<dialog>form's textarea.Test plan
./sbpp.sh phpstan./sbpp.sh ts-check./sbpp.sh composer api-contract./sbpp.sh test --filter="Bans|Comms|PermissionMatrix"./sbpp.sh test(full suite)./sbpp.sh e2e --workers=1 --grep "comms|banlist|ban" --project=chromiumSpecifically covered by new PHPUnit tests:
bans.unban: anonymous reject, missing/badbid, emptyureasonreject, unknownbid(404), success path (state persists in:prefix_bans.RemoveType+RemovedBy), already-lifted reject (409), audit-log message containsReason: ….comms.unblock(additions on top of the existing tests): emptyureasonreject, audit-log message containsReason: ….PermissionMatrixTest: locksbans.unban's dispatcher gate toADMIN_OWNER | ADMIN_UNBAN | ADMIN_UNBAN_OWN_BANS | ADMIN_UNBAN_GROUP_BANS(mirrors the legacy GET).Specifically covered by updated E2E specs:
admin-ban-lifecycle.spec.ts: drives the new#bans-unban-dialogmodal (empty submit surfaces inline[data-testid="bans-unban-error"], filled submit flips row'sdata-statefrompermanenttounbannedin place + success toast).comms-affordances.spec.ts(desktop + mobile variants): drives the new#comms-unblock-dialogmodal before asserting the in-placeunmutedflip.Anti-patterns avoided
sourcebans.js,xajax, or ADOdb.Sbpp\…) PHP class.LogType::Message/BanRemoval::Unbanned->valueat every bind site.:prefix_*literal —Sbpp\Db\Database::query()rewrites the placeholder.setTimeoutwaits in E2E specs — the modal'sdata-testidhooks + the row'sdata-stateattribute are the deterministic settle signals.requiredon the dialog form's textarea — the JS submit handler is the inline-error UX, the server is the load-bearing gate.onclick="event.stopPropagation()"on the trigger button (document.addEventListener('click')is how the dialog opener picks the click up — stopPropagation would silently swallow it; the action button isn't inside any[data-drawer-href]ancestor anyway).