Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
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,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was missing

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
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'];

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to exclude the pseudo mod


\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
85 changes: 75 additions & 10 deletions web/tests/e2e/specs/flows/mod-delete-confirm.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,11 @@
*
* Project gating
* --------------
* Pin to chromium (desktop). The flow mutates `:prefix_mods` and the
* suite shares a single `sourcebans_e2e` DB across projects
* (`workers: 1` is the suite-wide mitigation per AGENTS.md). Mobile
* coverage doesn't add value for an admin-only chrome that's
* structurally identical at every viewport.
* Pin to chromium. The flow mutates `:prefix_mods` and the suite shares
* a single `sourcebans_e2e` DB across projects (`workers: 1` is the
* suite-wide mitigation per AGENTS.md). The main mutation flow resizes
* that desktop context to exercise the mobile-card mirror without
* adding a second mutating project.
*/

import { expect, test } from '../../fixtures/auth.ts';
Expand All @@ -64,6 +64,10 @@ const FIXTURE = {
};

test.describe('flow: admin mod delete confirm modal (#1397 — RemoveMod zombie)', () => {
// Both tests truncate and mutate the shared E2E database. Keep focused
// local runs serial even though the project enables full parallelism.
test.describe.configure({ mode: 'serial' });

test.skip(({ isMobile }) => isMobile, 'flow spec runs only on desktop chromium');

test.beforeEach(async () => {
Expand Down Expand Up @@ -128,13 +132,25 @@ test.describe('flow: admin mod delete confirm modal (#1397 — RemoveMod zombie)
const targetMid = Number(targetMidAttr);
expect(Number.isFinite(targetMid) && targetMid > 0).toBe(true);

// The count badge text is a bare digit. Read the starting value
// so we can assert it decrements by exactly one after the delete.
// The hidden mobile mirror must be removed by the same successful
// response, otherwise resizing after deletion resurrects the mod.
const targetCard = page
.locator('[data-testid="mods-list-card"]')
.filter({ hasText: FIXTURE.name })
.first();
await expect(targetCard).toHaveCount(1);

// Read the parenthesised starting value so we can assert it
// decrements by exactly one after the delete.
const countBadge = page.locator('[data-testid="mod-count"]').first();
await expect(countBadge).toBeVisible();
const startingCountText = (await countBadge.textContent()) ?? '';
const startingCount = Number(startingCountText.replace(/[^0-9]/g, ''));
expect(Number.isFinite(startingCount) && startingCount > 0).toBe(true);
expect(await page.locator('[data-testid="mod-row"]').count())
.toBe(startingCount);
expect(await page.locator('[data-testid="mods-list-card"]').count())
.toBe(startingCount);

// ---- 3. Click the trash button — dialog must open ----------------
// The delete button is inside `.row-actions` on the row's
Expand Down Expand Up @@ -177,8 +193,18 @@ test.describe('flow: admin mod delete confirm modal (#1397 — RemoveMod zombie)
// The row is still there — cancel didn't delete anything.
await expect(targetRow).toBeVisible();

// ---- 5. Re-open + supply reason + confirm -----------------------
await deleteButton.click();
// ---- 5. Switch to mobile + re-open + confirm --------------------
await page.setViewportSize({ width: 390, height: 844 });
await expect(page.locator('[data-testid="mods-table"]')).toBeHidden();
await expect(page.locator('[data-testid="mods-list-cards"]')).toBeVisible();
await expect(targetCard).toBeVisible();
await expect(targetCard.locator('[data-testid="editmod-link-mobile"]')).toBeVisible();

const mobileDeleteButton = targetCard.locator(
'[data-testid="deletemod-btn-mobile"]',
);
await expect(mobileDeleteButton).toBeVisible();
await mobileDeleteButton.click();
await expect(dialog).toBeVisible();

const reasonInput = dialog.locator('[data-testid="mod-delete-reason"]');
Expand All @@ -204,13 +230,14 @@ test.describe('flow: admin mod delete confirm modal (#1397 — RemoveMod zombie)

// ---- 6. Row removed in place ------------------------------------
await expect(targetRow).toHaveCount(0);
await expect(targetCard).toHaveCount(0);
// Dialog closes after success.
await expect(dialog).toBeHidden();

// Count badge decrements by exactly one (defensive — the
// chrome's `decrementCount` reads the span's text and writes
// back `n - 1`).
await expect(countBadge).toHaveText(String(startingCount - 1));
await expect(countBadge).toHaveText(`(${startingCount - 1})`);

// Success toast surfaces. Anchor on `data-kind="success"` plus
// a hasText filter on the title our handler emits ("Mod
Expand All @@ -221,6 +248,7 @@ test.describe('flow: admin mod delete confirm modal (#1397 — RemoveMod zombie)
await expect(successToast).toBeVisible();

// ---- 7. Audit log carries the reason ----------------------------
await page.setViewportSize({ width: 1280, height: 720 });
// Navigate to the audit log and assert the most recent
// "MOD Deleted" entry's body contains the reason. The audit
// page's row body is `<div class="audit-row__detail">`.
Expand Down Expand Up @@ -272,6 +300,33 @@ test.describe('flow: admin mod delete confirm modal (#1397 — RemoveMod zombie)
.first();
await expect(row).toBeVisible();

// Model the final visible mod without deleting fixture rows from the
// shared DB. This isolates the client-side 1 → 0 transition that must
// reveal the already-mounted empty state after a successful delete.
const targetMid = await row.getAttribute('data-id');
expect(targetMid).not.toBeNull();
await page.locator('[data-testid="mod-row"]').evaluateAll((nodes, keepMid) => {
nodes.forEach((node) => {
if (node.getAttribute('data-id') !== keepMid) node.remove();
});
}, targetMid);
await page.locator('[data-testid="mods-list-card"]').evaluateAll((nodes, keepMid) => {
nodes.forEach((node) => {
if (node.getAttribute('data-id') !== keepMid) node.remove();
});
}, targetMid);

const targetCard = page
.locator('[data-testid="mods-list-card"]')
.filter({ hasText: 'e2e-no-reason-mod' });
const countBadge = page.locator('[data-testid="mod-count"]');
await countBadge.evaluate((node) => {
node.textContent = '(1)';
});
await expect(page.locator('[data-testid="mod-row"]')).toHaveCount(1);
await expect(targetCard).toHaveCount(1);
await expect(page.locator('[data-testid="mods-empty"]')).toBeHidden();

await row.locator('[data-testid="deletemod-btn"]').click();
const dialog = page.locator('[data-testid="mod-delete-dialog"]');
await expect(dialog).toBeVisible();
Expand All @@ -286,7 +341,17 @@ test.describe('flow: admin mod delete confirm modal (#1397 — RemoveMod zombie)
expect(env.ok, JSON.stringify(env)).toBe(true);

await expect(row).toHaveCount(0);
await expect(targetCard).toHaveCount(0);
await expect(countBadge).toHaveText('(0)');
await expect(page.locator('[data-testid="mods-table-wrap"]')).toHaveAttribute('hidden', '');
await expect(page.locator('[data-testid="mods-list-cards"]')).toHaveAttribute('hidden', '');
await expect(page.locator('[data-testid="mods-empty"]')).toBeVisible();

await page.setViewportSize({ width: 390, height: 844 });
await expect(page.locator('[data-testid="mods-list-cards"]')).toBeHidden();
await expect(page.locator('[data-testid="mods-empty"]')).toBeVisible();

await page.setViewportSize({ width: 1280, height: 720 });
await page.goto(AUDIT_ROUTE);
const auditDetail = page
.locator('.audit-row__detail')
Expand Down
37 changes: 37 additions & 0 deletions web/tests/integration/EditAdminTabsChromeTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -103,4 +103,41 @@ public function testEditAdminHandlersSkipOrphanBackStrip(): void
);
}
}

public function testStandaloneEditModKeepsBackStrip(): void
{
$src = (string) file_get_contents(ROOT . 'pages/admin.edit.mod.php');

$this->assertStringContainsString(
'new \\Sbpp\\View\\AdminTabs([],',
$src,
'The standalone mod editor must keep the shared Back strip; it has no in-template section tabs.',
);
}

public function testEditModMetadataDoesNotRenderAsRawHtml(): void
{
$src = (string) file_get_contents(self::THEME . 'page_admin_edit_mod.tpl');

$this->assertStringContainsString(
'Edit mod · {$name|unescape:\'html\'}',
$src,
'Stored entities should be decoded before Smarty applies its final automatic escape.',
);
$this->assertStringContainsString(
'overflow-wrap:anywhere',
$src,
'A schema-width mod name must not overflow the edit heading on narrow viewports.',
);
$this->assertSame(
5,
substr_count($src, '|unescape:\'html\''),
'The title plus every name, folder, and icon sink must decode once before the final escape.',
);
$this->assertStringNotContainsString(
'nofilter',
$src,
'Legacy or externally seeded raw mod metadata must never bypass Smarty escaping.',
);
}
}
86 changes: 76 additions & 10 deletions web/tests/integration/ModsDeleteDialogTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,12 @@ public function testDeleteButtonCarriesDataActionWiring(): void

// Verify the supporting attributes are all present.
$this->assertStringContainsString('data-name="' . $this->targetName . '"', $html);
$this->assertStringContainsString('src="images/games/default&amp;alt.png"', $html);
$this->assertStringContainsString('>delete&amp;target</span>', $html);
$this->assertStringContainsString('title="delete&amp;target"', $html,
'The truncated mobile folder must expose its full value.');
$this->assertStringNotContainsString('&amp;amp;', $html,
'Entity-encoded mod metadata must be decoded before Smarty applies its final escape.');
$this->assertStringContainsString('data-fallback-href="index.php?p=admin&amp;c=mods"', $html,
'The fallback URL lands the no-JS / no-dispatcher operator on the mods list ' .
'(no legacy GET handler exists for `o=remove`).');
Expand Down Expand Up @@ -218,34 +224,94 @@ public function testPageTailScriptUsesActionsConstant(): void
* The mod count badge must carry `data-testid="mod-count"` so the
* page-tail script can decrement it after a delete and the E2E
* spec can read it as the pre / post-delete oracle. Without the
* testid the badge is unreachable in a theme-agnostic way (the
* surrounding `<p>… configured</p>` shape is too brittle to
* regex against).
* testid the badge is unreachable in a theme-agnostic way. The
* rendered text is `(N)` (same shape as the Admins list badge).
*/
public function testCountBadgeCarriesTestid(): void
{
$html = $this->renderModsPage();

$this->assertMatchesRegularExpression(
'/<span[^>]*data-testid="mod-count"[^>]*>\s*\d+\s*<\/span>/',
'/<span[^>]*data-testid="mod-count"[^>]*>\s*\(\d+\)\s*<\/span>/',
$html,
'The mod count number must be wrapped in <span data-testid="mod-count">.'
'The mod count must be parenthesised inside <span data-testid="mod-count">, matching the Admins list badge.'
);
}

/**
* `mid = 0` is the reserved Web pseudo-mod. It is deliberately absent
* from both list renderings, so the heading count must exclude it too.
*/
public function testCountBadgeMatchesDesktopRowsAndMobileCards(): void
{
$html = $this->renderModsPage();

$matched = preg_match(
'/<span[^>]*data-testid="mod-count"[^>]*>\s*\((\d+)\)\s*<\/span>/',
$html,
$countMatch,
);
$this->assertSame(1, $matched, 'The parenthesised mod count must render.');

$desktopRows = preg_match_all('/<tr\b[^>]*data-testid="mod-row"[^>]*>/', $html);
$mobileCards = preg_match_all('/<div\b[^>]*data-testid="mods-list-card"[^>]*>/', $html);

$this->assertNotFalse($desktopRows);
$this->assertNotFalse($mobileCards);
$this->assertSame((int) $countMatch[1], $desktopRows,
'The heading count must equal the visible game-mod rows, excluding the reserved Web pseudo-mod.');
$this->assertSame($desktopRows, $mobileCards,
'Every desktop mod row must have one mobile-card mirror.');
}

public function testCountDecrementKeepsAsciiDigitsForSubsequentDeletes(): void
{
$html = $this->renderModsPage();

$this->assertStringContainsString("String(next) + ')'", $html);
$this->assertStringContainsString('if (next === 0) showEmptyState();', $html);
$this->assertStringNotContainsString(
'(n - 1).toLocaleString()',
$html,
'Localized numerals cannot be parsed by the next ASCII-digit decrement.',
);
}

public function testZeroGameModsRendersActionableEmptyState(): void
{
Fixture::rawPdo()->exec(sprintf(
'DELETE FROM `%s_mods` WHERE mid > 0',
DB_PREFIX,
));

$html = $this->renderModsPage();

$this->assertStringContainsString('data-testid="mod-count">(0)</span>', $html);
$this->assertSame(0, preg_match_all('/<tr\b[^>]*data-testid="mod-row"[^>]*>/', $html));
$this->assertStringContainsString('data-testid="mods-empty-add"', $html);

$matched = preg_match(
'/<div class="empty-state"[^>]*data-testid="mods-empty"[^>]*>/s',
$html,
$emptyTag,
);
$this->assertSame(1, $matched, 'The mods empty state must render.');
$this->assertStringNotContainsString(' hidden', $emptyTag[0],
'The server-rendered empty state must be visible when no game mods remain.');
}

private function seedTargetMod(): void
{
$pdo = Fixture::rawPdo();

// Use a name that's safe through htmlspecialchars and survives
// the data-name attribute round-trip without escaping noise so
// the data-name assertion stays readable.
$this->targetName = 'DeleteTargetMod';
// Mirror api_mods_add's escaping-on-store shape. The rendered
// list should decode this once, then let Smarty escape it once.
$this->targetName = 'Delete &amp; Target';

$pdo->prepare(sprintf(
'INSERT INTO `%s_mods` (name, icon, modfolder, steam_universe, enabled) VALUES (?, ?, ?, ?, ?)',
DB_PREFIX,
))->execute([$this->targetName, 'default.png', 'deletetargetmod', 0, 1]);
))->execute([$this->targetName, 'default&amp;alt.png', 'delete&amp;target', 0, 1]);
$this->targetMid = (int) $pdo->lastInsertId();
}

Expand Down
9 changes: 9 additions & 0 deletions web/themes/default/css/theme.css
Original file line number Diff line number Diff line change
Expand Up @@ -2130,6 +2130,12 @@ details.queue-row > summary > .row-actions {
.admins-list-card:last-child { border-bottom: none; }
.admins-list-card__body { padding: 0.75rem 1rem 0.25rem; }

.mods-list-cards { display: none; }
.mods-list-cards[hidden] { display: none; }
.mods-list-card { border-bottom: 1px solid var(--border); }
.mods-list-card:last-child { border-bottom: none; }
.mods-list-card__body { padding: 0.75rem 1rem 0.25rem; }

/* ---- Responsive ---- */
[data-mobile-menu] { display: none; }
@media (max-width: 1024px) {
Expand All @@ -2148,6 +2154,7 @@ details.queue-row > summary > .row-actions {
dance as `.ban-cards` — hidden at desktop, block at mobile. */
.log-cards { display: block; }
.admins-list-cards { display: block; }
.mods-list-cards { display: block; }
/* #1181: filter chip rows wrap onto multiple lines on mobile
instead of horizontal-scrolling, so every chip is reachable
without a swipe. The .scroll-x desktop affordance is the
Expand All @@ -2159,6 +2166,7 @@ details.queue-row > summary > .row-actions {
.ban-cards { display: none; }
.log-cards { display: none; }
.admins-list-cards { display: none; }
.mods-list-cards { display: none; }
}

/* ---- Utility classes used by templates ---- */
Expand Down Expand Up @@ -2238,6 +2246,7 @@ details.queue-row > summary > .row-actions {
padding: 2.5rem 1.5rem;
color: var(--text-muted);
}
.empty-state[hidden] { display: none; }
.empty-state__icon {
width: 2.5rem;
height: 2.5rem;
Expand Down
Loading
Loading