fix(mods): restore Delete affordance with confirm-reason dialog (#1397) - #1400
Merged
Conversation
The trash-can button on `?p=admin&c=mods` carried an unguarded `onclick="RemoveMod(this.dataset.modName, this.dataset.modId);"` that referenced a v1.x sourcebans.js helper deleted at #1123 D1. Every click threw `ReferenceError: RemoveMod is not defined` and the delete never fired — from the operator's POV "Delete just doesn't work." This is the loud sister of #1352's admins-delete bug. Same anti-pattern (inline `onclick` calling a long-removed helper), same structural fix (mirror the canonical `data-action` + `<dialog>` + inline page-tail dispatcher shape from `page_admin_admins_list.tpl`, route through `Actions.ModsRemove`, capture an optional reason for the audit log). Server: `api_mods_remove` now accepts an optional `ureason`, trims it, and appends `Reason: <reason>` to the audit-log body when non-empty — symmetric with `api_admins_remove` / `api_bans_unban` / `api_comms_unblock`. Empty / omitted reason flows through unchanged so the no-JS / no-dispatcher fallback still works. Chrome: the page-tail script flips the busy contract on the submit button (`window.SBPP.setBusy` with local fallback for themes that strip theme.js), reads / decrements the new `[data-testid="mod-count"]` badge, removes the row in place, and fires `window.SBPP.showToast` for confirmation. Cancel closes without firing the API; missing dispatcher falls back to the mods list landing (no legacy GET handler for `o=remove`). Test plan --------- - ./sbpp.sh phpstan ✓ - ./sbpp.sh test ✓ (651 tests, includes new ModsDeleteDialogTest + extended ModsTest) - ./sbpp.sh ts-check ✓ - ./sbpp.sh composer api-contract ✓ (regenerated + tightened typedef) - CI=1 ./sbpp.sh e2e ✓ (266 passed, includes new mod-delete-confirm spec — 2 tests covering confirm-with-reason + optional-empty-reason) Mirrors #1352's admins-delete framing; the only delta is the optional-reason contract is the same shape (mods being a lifecycle action, not a moderation flip — same justification as admins-delete).
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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 #1397.
Summary
onclick="RemoveMod(this.dataset.modName, this.dataset.modId);"on every mod-row trash button (an unguarded reference to a v1.x sourcebans.js helper deleted at Modernize the default web panel theme (2026 redesign) #1123 D1) with the canonicaldata-action="mod-delete"+<dialog id="mod-delete-dialog">confirm + optional-reason flow. Mirrors the fix(brand): ship default themes/default/logos/sbpp_logo.png brand mark #1352 admins-delete shape byte-for-byte modulo the action name + row testid + counter element — same anti-pattern, same fix shape, just on a sibling surface.api_mods_removeto accept an optionalureason(trimmed, audit-log suffix omitted when empty). Symmetric withapi_admins_remove/api_bans_unban/api_comms_unblock. v2.0 4-line header replaces the legacy 12-line block onmods.php.MOD (X) has been deleted.with an optionalReason: …suffix when the operator filled in the dialog's textarea.Test plan
./sbpp.sh phpstan(level 5, dba) — clean./sbpp.sh test(651 PHPUnit tests, includes newSbpp\Tests\Integration\ModsDeleteDialogTest+ extendedSbpp\Tests\Api\ModswithtestRemoveAppendsReasonToAuditLog/testRemoveOmitsReasonSuffixWhenEmpty/testRemoveAcceptsMissingReasonParam)./sbpp.sh ts-check— clean./sbpp.sh composer api-contract— regenerated; the docblock onapi_mods_removetightened theApiModsRemoveResponsetypedef.CI=1 ./sbpp.sh e2e(266 passed, includes newmod-delete-confirm.spec.ts— covers confirm-with-reason flow + optional-empty-reason variant; both assert no uncaught console errors, audit-log content, in-place row removal, and count-badge decrement).sptouched)docker/files touchedFiles changed
AGENTS.md— extends theonclick="<Helper>(...)"anti-pattern callout with the sisterRemoveModshape, and adds#mod-delete-dialogas the fourth canonical reference under "Add a confirm + reason modal for an irreversible row-level action".web/themes/default/page_admin_mods_list.tpl— deadonclick→data-action; new<dialog>+ inline page-tail script;{$mod_count}wrapped in<span data-testid="mod-count">.web/api/handlers/mods.php—api_mods_removeacceptsureason; v2.0 header replaces legacy block.web/scripts/api-contract.js— regenerated from the tightened docblock.web/tests/api/ModsTest.php— three new ureason-coverage tests + helperlatestLogMessage.web/tests/integration/ModsDeleteDialogTest.php(new) — seven per-attribute regression assertions mirroringAdminsDeleteDialogTest.web/tests/e2e/specs/flows/mod-delete-confirm.spec.ts(new) — end-to-end happy path + optional-reason variant.