Skip to content

fix(admin): backport upstream mods-list + groups select-all polish #41

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Rushaway merged 4 commits into from
Sep 26, 2026
Merged

fix(admin): backport upstream mods-list + groups select-all polish #41

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bfcc898
fix(admin-mods): decode entity-encoded mod metadata and add proper em…
Rushaway Sep 22, 2026
234ce1a
feat(admin-groups): reflect exhausted select-all/select-none state
Rushaway Sep 22, 2026
ac754b9
fix(admin-groups): refresh select-all/none state after bulk toggle
cmer81 Sep 26, 2026
84e17a2
test(e2e): force the idempotent Select all press on an exhausted grid
cmer81 Sep 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion web/includes/View/AdminModsListView.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@
* Property names keep the historical `permission_*` shape (rather than
* the newer `can_*` convention `Sbpp\View\Perms::for()` produces) because
* the template references `{$permission_listmods}` /
* `{$permission_editmods}` / `{$permission_deletemods}`. See
* `{$permission_addmods}` / `{$permission_editmods}` /
* `{$permission_deletemods}`. See
* `AdminServersListView` / `AdminBansAddView` for the canonical
* `can_*` convention on Views that don't have to honour a historical
* variable name.
Expand All @@ -30,6 +31,7 @@ final class AdminModsListView extends View
*/
public function __construct(
public readonly bool $permission_listmods,
public readonly bool $permission_addmods,
public readonly bool $permission_editmods,
public readonly bool $permission_deletemods,
public readonly int $mod_count,
Expand Down
4 changes: 3 additions & 1 deletion web/pages/admin.mods.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,13 @@
return;
}

// mid=0 is the reserved Web pseudo-mod, not a configurable game mod.
$mod_list = $GLOBALS['PDO']->query("SELECT * FROM `:prefix_mods` WHERE mid > 0 ORDER BY name ASC")->resultset();
$mod_count = (int) $GLOBALS['PDO']->query("SELECT COUNT(mid) AS cnt FROM `:prefix_mods`")->single()['cnt'];
$mod_count = (int) $GLOBALS['PDO']->query("SELECT COUNT(mid) AS cnt FROM `:prefix_mods` WHERE mid > 0")->single()['cnt'];

\Sbpp\View\Renderer::render($theme, new \Sbpp\View\AdminModsListView(
permission_listmods: $canList,
permission_addmods: $canAdd,
permission_editmods: $userbank->HasAccess(WebPermission::mask(WebPermission::Owner, WebPermission::EditMods)),
permission_deletemods: $userbank->HasAccess(WebPermission::mask(WebPermission::Owner, WebPermission::DeleteMods)),
mod_count: $mod_count,
Expand Down
28 changes: 25 additions & 3 deletions web/tests/e2e/specs/flows/admin-groups-select-all-flags.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,13 @@ test.describe('flow: admin groups select-all permission flags (upstream #1436)',
await expect(selectAll).toHaveAttribute('type', 'button');
await expect(selectNone).toHaveAttribute('type', 'button');
await expect(bitmaskBadge).toHaveText(/^0 bitmask$/);
// Nothing checked yet: "Select none" starts out reflecting that
// there's nothing left to clear (upstream #1573 parity — see the
// SbppGroupsRefreshSelectAllButtons docblock for why this fork
// uses aria-disabled + a dimmed style instead of a real indeterminate
// checkbox or the native `disabled` attribute).
await expect(selectNone).toHaveAttribute('aria-disabled', 'true');
await expect(selectAll).toHaveAttribute('aria-disabled', 'false');

const total = await checkboxes.count();
expect(total).toBeGreaterThan(0);
Expand All @@ -146,10 +153,17 @@ test.describe('flow: admin groups select-all permission flags (upstream #1436)',
}
await expect(bitmaskBadge).toHaveText(`${expected} bitmask`);
await expect(bitmaskBadge).not.toContainText('-');
// Everything is now checked: "Select all" reflects there's nothing
// left to add, "Select none" flips back to actionable.
await expect(selectAll).toHaveAttribute('aria-disabled', 'true');
await expect(selectNone).toHaveAttribute('aria-disabled', 'false');

// Idempotent: a second press changes nothing and must not corrupt
// the preview.
await selectAll.click();
// the preview. The button now carries aria-disabled="true", which
// Playwright's actionability check treats as disabled, so force
// the click: a mouse user can still press it (no native
// `disabled`), and that press must stay a no-op.
await selectAll.click({ force: true });
await expect(bitmaskBadge).toHaveText(`${expected} bitmask`);

// ---- Save round-trips the folded OR-sum --------------------------
Expand Down Expand Up @@ -181,13 +195,21 @@ test.describe('flow: admin groups select-all permission flags (upstream #1436)',
for (let i = 0; i < reloadedTotal; i++) {
await expect(reloadedChecks.nth(i)).toBeChecked();
}
// Fresh page load re-derives the same state from the SSR-checked
// grid, not just from in-memory JS state carried across the click.
const reloadedSelectAll = reloadedDetail.locator('[data-testid="flag-select-all"]');
const reloadedSelectNone = reloadedDetail.locator('[data-testid="flag-select-none"]');
await expect(reloadedSelectAll).toHaveAttribute('aria-disabled', 'true');
await expect(reloadedSelectNone).toHaveAttribute('aria-disabled', 'false');

// ---- Select none → every checkbox cleared, badge back to 0 -------
await reloadedDetail.locator('[data-testid="flag-select-none"]').click();
await reloadedSelectNone.click();
for (let i = 0; i < reloadedTotal; i++) {
await expect(reloadedChecks.nth(i)).not.toBeChecked();
}
await expect(reloadedBadge).toHaveText(/^0 bitmask$/);
await expect(reloadedSelectNone).toHaveAttribute('aria-disabled', 'true');
await expect(reloadedSelectAll).toHaveAttribute('aria-disabled', 'false');
});

test('a bulk toggle arms the unsaved-changes guard', async ({ page }) => {
Expand Down
66 changes: 66 additions & 0 deletions web/themes/default/page_admin_groups_list.tpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -613,6 +613,63 @@ function SbppGroupsToggleAllFlags(checked) {

var preview = document.querySelector('[data-testid="flag-bitmask"]');
if (preview) preview.textContent = SbppFoldFlags(grid) + ' bitmask';
// The `change` above targets the grid itself, which the grid listener
// filters out (it only reacts to `input[name="flags[]"]` targets), so
// refresh the Select all / Select none state explicitly here.
SbppGroupsRefreshSelectAllButtons();
}

/**
* Port of upstream sbpp/sourcebans-pp#1573's tri-state "select all"
* checkbox, adapted to this fork's two-button shape instead of a
* single `<input type="checkbox">`: a checkbox here would have lost
* the explicit Select-all/Select-none affordance the #1436 spec
* (admin-groups-select-all-flags.spec.ts) already locks in — including
* the unsigned bit-31 OR-fold guard (#1272) and the dirty-tracker-arming
* contract, both of which are exercised through `SbppGroupsToggleAllFlags`
* and would need re-proving from scratch against a checkbox's native
* `indeterminate` handling. Reflecting the grid's current state via each
* button's `aria-disabled` + dimmed style gives the same "you can see
* there's nothing left to do" feedback without touching that logic.
*
* Deliberately uses `aria-disabled` + a visual dim, NOT the native
* `disabled` attribute: `SbppGroupsToggleAllFlags` is already a safe
* no-op when everything is already on/off (it bails before dispatching
* `change`, per the comment above it), and the #1436 spec exercises a
* redundant "Select all" press on a fully-selected grid as a deliberate
* idempotency check. A native `disabled` button can't receive a click at
* all; `aria-disabled` communicates the same "nothing left to do" state
* to assistive tech and sighted users while the button stays pressable
* (the spec forces that click, since Playwright's actionability check
* treats aria-disabled="true" as disabled).
*
* Call after anything that can change the grid's checked state without
* going through a user click on an individual checkbox: the bulk toggle
* (`SbppGroupsToggleAllFlags` dispatches `change` on the grid itself,
* which the grid listener ignores), the master-detail `paintGroup()`
* repaint, and the bootstrap call below. Manual clicks on a checkbox are
* covered by the `change` listener wired further down this file.
*/
function SbppGroupsRefreshSelectAllButtons() {
var grid = document.querySelector('[data-testid="flag-grid"]');
var selectAll = document.querySelector('[data-testid="flag-select-all"]');
var selectNone = document.querySelector('[data-testid="flag-select-none"]');
if (!grid || !(selectAll || selectNone)) return;

var checks = grid.querySelectorAll('input[name="flags[]"]:not([disabled])');
var total = checks.length;
var checked = grid.querySelectorAll('input[name="flags[]"]:not([disabled]):checked').length;

/** @param {Element|null} btn @param {boolean} exhausted */
function reflect(btn, exhausted) {
if (!btn) return;
btn.setAttribute('aria-disabled', exhausted ? 'true' : 'false');
/** @type {HTMLElement} */ (btn).style.opacity = exhausted ? '0.5' : '';
/** @type {HTMLElement} */ (btn).style.cursor = exhausted ? 'default' : '';
}

reflect(selectAll, total === 0 || checked === total);
reflect(selectNone, checked === 0);
}

/**
Expand Down Expand Up @@ -769,7 +826,12 @@ function SbppServerGroupsDelete(gid, name, type, btn) {
if (!target || !target.matches || !target.matches('input[name="flags[]"]')) return;

preview.textContent = SbppFoldFlags(grid) + ' bitmask';
SbppGroupsRefreshSelectAllButtons();
});

// Reflect the SSR-rendered initial state (e.g. a group whose flags
// already cover every checkbox loads with "Select all" disabled).
SbppGroupsRefreshSelectAllButtons();
})();

// --- Client-side master-detail selection ---
Expand Down Expand Up @@ -852,6 +914,10 @@ function SbppServerGroupsDelete(gid, name, type, btn) {
if (bitmaskEl) {
bitmaskEl.textContent = flags + ' bitmask';
}
// paintGroup() sets .checked directly (no change event), so the
// Select all/Select none disabled state needs an explicit refresh
// here too — the grid's `change` listener alone won't catch it.
SbppGroupsRefreshSelectAllButtons();

var rows = list.querySelectorAll('[data-testid="group-row"]');
for (var r = 0; r < rows.length; r++) {
Expand Down
54 changes: 37 additions & 17 deletions web/themes/default/page_admin_mods_list.tpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

Variable contract (kept in sync by SmartyTemplateRule):
- $permission_listmods — gate the whole tab body.
- $permission_addmods — gate the empty-state "Add mod" CTA.
- $permission_editmods — gate the per-row "Edit" link.
- $permission_deletemods — gate the per-row "Delete" button.
- $mod_count — total mods configured.
Expand Down Expand Up @@ -55,7 +56,7 @@

<div class="card" style="overflow:hidden">
{if $mod_count > 0}
<div class="table-scroll">
<div class="table-scroll" data-testid="mods-table-wrap">
<table class="table table--compact" data-testid="mods-table">
<thead>
<tr>
Expand All @@ -69,20 +70,23 @@
</tr>
</thead>
<tbody>
{* Mod metadata is entity-encoded on store. Decode before
Smarty's automatic final escape at every text and
attribute sink so values stay readable without raw HTML. *}
{foreach from=$mod_list item=mod}
<tr id="mid_{$mod.mid}" data-testid="mod-row" data-id="{$mod.mid}">
<td>
<div class="flex items-center gap-2">
<img src="images/games/{$mod.icon}"
<img src="images/games/{$mod.icon|unescape:'html'}"
alt=""
width="18"
height="18"
loading="lazy"
onerror="this.style.visibility='hidden'">
<span class="font-medium">{$mod.name}</span>
<span class="font-medium">{$mod.name|unescape:'html'}</span>
</div>
</td>
<td><span class="font-mono text-xs">{$mod.modfolder}</span></td>
<td><span class="font-mono text-xs">{$mod.modfolder|unescape:'html'}</span></td>
<td class="tabular-nums">{$mod.steam_universe}</td>
<td>
{if $mod.enabled}
Expand All @@ -103,7 +107,7 @@
href="index.php?p=admin&c=mods&o=edit&id={$mod.mid|escape:'url'}"
data-testid="editmod-link"
data-tooltip="Edit"
aria-label="Edit mod {$mod.name|escape}">
aria-label="Edit mod {$mod.name|unescape:'html'}">
<i data-lucide="pencil" style="width:14px;height:14px"></i>
</a>
{/if}
Expand All @@ -127,11 +131,11 @@
type="button"
data-action="mod-delete"
data-mid="{$mod.mid}"
data-name="{$mod.name|escape}"
data-name="{$mod.name|unescape:'html'}"
data-fallback-href="index.php?p=admin&amp;c=mods"
data-testid="deletemod-btn"
data-tooltip="Delete"
aria-label="Delete mod {$mod.name|escape}">
aria-label="Delete mod {$mod.name|unescape:'html'}">
<i data-lucide="trash-2" style="width:14px;height:14px;color:var(--danger)"></i>
</button>
{/if}
Expand All @@ -151,17 +155,19 @@
{foreach from=$mod_list item=mod}
<div class="mods-list-card" data-testid="mods-list-card" data-id="{$mod.mid}">
<div class="mods-list-card__body flex items-center gap-3">
<img src="images/games/{$mod.icon}"
<img src="images/games/{$mod.icon|unescape:'html'}"
alt=""
width="28"
height="28"
loading="lazy"
onerror="this.style.visibility='hidden'">
<div style="flex:1;min-width:0">
<div class="font-medium text-sm truncate">{$mod.name}</div>
<div class="text-xs text-muted truncate" style="margin-top:0.125rem">
<span class="font-mono">{$mod.modfolder}</span>
· SU {$mod.steam_universe}
<div class="font-medium text-sm truncate">{$mod.name|unescape:'html'}</div>
<div class="flex items-center gap-2 text-xs text-muted" style="margin-top:0.125rem">
<span class="font-mono truncate"
style="min-width:0"
title="{$mod.modfolder|unescape:'html'}">{$mod.modfolder|unescape:'html'}</span>
<span class="tabular-nums" style="flex-shrink:0">SU {$mod.steam_universe}</span>
</div>
<div style="margin-top:0.35rem">
{if $mod.enabled}
Expand All @@ -179,7 +185,7 @@
href="index.php?p=admin&amp;c=mods&amp;o=edit&amp;id={$mod.mid|escape:'url'}"
data-testid="editmod-link-mobile"
data-tooltip="Edit"
aria-label="Edit mod {$mod.name|escape}">
aria-label="Edit mod {$mod.name|unescape:'html'}">
<i data-lucide="pencil" style="width:14px;height:14px"></i>
</a>
{/if}
Expand All @@ -188,11 +194,11 @@
type="button"
data-action="mod-delete"
data-mid="{$mod.mid}"
data-name="{$mod.name|escape}"
data-name="{$mod.name|unescape:'html'}"
data-fallback-href="index.php?p=admin&amp;c=mods"
data-testid="deletemod-btn-mobile"
data-tooltip="Delete"
aria-label="Delete mod {$mod.name|escape}">
aria-label="Delete mod {$mod.name|unescape:'html'}">
<i data-lucide="trash-2" style="width:14px;height:14px;color:var(--danger)"></i>
</button>
{/if}
Expand All @@ -202,8 +208,22 @@
{/foreach}
</div>
{else}
<div class="card__body">
<p class="text-muted">No mods configured yet.</p>
<div class="empty-state" data-testid="mods-empty" data-filtered="false">
<div class="empty-state__icon" aria-hidden="true">
<i data-lucide="puzzle"></i>
</div>
<h2 class="empty-state__title">No mods configured yet</h2>
<p class="empty-state__body">Add a game mod before assigning it to bans or servers.</p>
{if $permission_addmods}
<div class="empty-state__actions">
<a class="btn btn--primary btn--sm"
href="index.php?p=admin&amp;c=mods&amp;section=add"
data-testid="mods-empty-add">
<i data-lucide="plus"></i>
Add mod
</a>
</div>
{/if}
</div>
{/if}
</div>
Expand Down
Loading