Task #146: Filter and paginate role permission history

Summary - Backend: GET /api/roles/:id/audit now accepts limit (default 10, max 200), offset, actorUserId (0 = no filter), and from/to (YYYY-MM-DD UTC). The response is now a paginated envelope `{entries, totalCount, limit, offset, nextOffset}` instead of a bare array. - OpenAPI: lib/api-spec/openapi.yaml updated with the new params and a new RolePermissionAuditList schema; client hooks regenerated via `pnpm --filter @workspace/api-spec run codegen`. - Frontend: RolePermissionHistory in artifacts/tx-os/src/pages/admin.tsx is now self-contained — owns its own filter state, fetches the user list for the actor dropdown, applies actor changes immediately, and validates date inputs (rejecting invalid / inverted ranges). - Pagination: switched to TRUE OFFSET PAGINATION. The first page comes from React Query (so live cache invalidations after a save still refresh it), and subsequent "Load more" clicks fetch `offset = nextOffset` imperatively via getRolePermissionAudit() and append the rows to local state. There is no client-side ceiling on how far back an admin can page; we simply stop showing the button when nextOffset is null. A filtersKey effect resets the appended pages whenever any filter (actor / from / to) changes so we never serve overlapping or out-of-order rows. - i18n: added missing keys in artifacts/tx-os/src/locales/{en,ar}.json (historyEmptyFiltered, historyShowing, historyLoadMore, historyFilters.*, historyErrors.*). - Tests: artifacts/api-server/tests/role-permission-audit.test.mjs updated to read the new envelope and now also covers offset-based pagination, actorUserId filtering (including the "0 = no filter" semantic), and date-range filtering (in-range / past-range / inverted / garbage). All 9 audit tests pass; tx-os typecheck clean. - E2E: ran the testing skill end-to-end against /admin → role edit dialog → history panel: created a fresh role through the UI, made 25 permission writes, verified 10 → 20 → 25 pagination with no duplicates and correct hide-on-end behaviour, filter-by-actor reset to first page, empty date range showed empty state, inverted dates surfaced the validation error. Drift / notes - Pre-existing executive-meetings tests in the `test` workflow are still failing — unchanged by this task. - The Arabic language toggle isn't exposed in the page header in this build, so the e2e Arabic step was skipped; locale strings are in place and the test ids do not change with locale. - Code review (initial pass) flagged a 200-row UI ceiling in the previous "growing limit" approach. Replaced with true offset pagination (described above) so admins can scroll back through arbitrarily long histories. Code review follow-ups (round 2) - Tightened parseRoleAuditUtcDate to reject impossible calendar days (2024-02-31, 2025-13-01, etc.) instead of silently rolling forward. - Aligned OpenAPI: actorUserId schema is now `minimum: 0` so the spec matches the runtime "0 = no filter" contract; client regenerated. - Added a targeted backend test that asserts impossible dates → 400. Code review follow-ups (round 3) - UX: when applied filters become invalid, the History list now hides the stale entries and shows the "fix filters first" hint instead, and the Load more button is hidden until filters are valid again. Avoids presenting yesterday's results as the current view. Code review follow-ups (round 4) - Belt-and-braces: also reset the appended history pages when the first page's totalCount + first-row id signature changes, so an external cache invalidation (e.g. another save while the dialog is still open) cannot leave appended pages out of sync with the refreshed first page.
2026-04-30 10:45:07 +00:00
parent 0b5e4c7912
commit ebd553b84a
10 changed files with 819 additions and 79 deletions
@@ -1,5 +1,5 @@
-import { Router, type IRouter } from "express";
-import { desc, eq, inArray, sql } from "drizzle-orm";
+import { Router, type IRouter, type Request, type Response } from "express";
+import { and, desc, eq, gte, inArray, lt, sql, type SQL } from "drizzle-orm";
 import { db } from "@workspace/db";
 import {
  rolesTable,
@@ -62,6 +62,88 @@ router.get("/permissions", requireAdmin, async (_req, res): Promise<void> => {
  res.json(rows.map(serializePermission));
 });

+const ROLE_AUDIT_DATE_RE = /^\d{4}-\d{2}-\d{2}$/;
+
+function parseRoleAuditUtcDate(s: string): Date | null {
+  if (!ROLE_AUDIT_DATE_RE.test(s)) return null;
+  const d = new Date(`${s}T00:00:00.000Z`);
+  if (Number.isNaN(d.getTime())) return null;
+  // Reject impossible calendar days (e.g. 2024-02-31, 2025-13-01) that
+  // would otherwise silently roll forward into a different real date.
+  const [y, m, day] = s.split("-").map(Number);
+  if (
+    d.getUTCFullYear() !== y ||
+    d.getUTCMonth() + 1 !== m ||
+    d.getUTCDate() !== day
+  ) {
+    return null;
+  }
+  return d;
+}
+
+type RoleAuditFilters = {
+  actorUserId: number | null;
+  fromDate: Date | null;
+  toDateExclusive: Date | null;
+};
+
+function parseRoleAuditFilters(
+  req: Request,
+  res: Response,
+): RoleAuditFilters | null {
+  const actorUserIdStr =
+    typeof req.query.actorUserId === "string" ? req.query.actorUserId.trim() : "";
+  const fromStr = typeof req.query.from === "string" ? req.query.from.trim() : "";
+  const toStr = typeof req.query.to === "string" ? req.query.to.trim() : "";
+
+  let actorUserId: number | null = null;
+  if (actorUserIdStr) {
+    const parsed = Number(actorUserIdStr);
+    if (!Number.isFinite(parsed) || !Number.isInteger(parsed) || parsed < 0) {
+      res
+        .status(400)
+        .json({ error: "Invalid 'actorUserId' (expected non-negative integer)" });
+      return null;
+    }
+    // Treat 0 as "no filter" so callers can clear the dropdown without
+    // dropping the query param entirely.
+    actorUserId = parsed > 0 ? parsed : null;
+  }
+  let fromDate: Date | null = null;
+  let toDateExclusive: Date | null = null;
+  if (fromStr) {
+    fromDate = parseRoleAuditUtcDate(fromStr);
+    if (!fromDate) {
+      res.status(400).json({ error: "Invalid 'from' date (expected YYYY-MM-DD)" });
+      return null;
+    }
+  }
+  if (toStr) {
+    const toDate = parseRoleAuditUtcDate(toStr);
+    if (!toDate) {
+      res.status(400).json({ error: "Invalid 'to' date (expected YYYY-MM-DD)" });
+      return null;
+    }
+    toDateExclusive = new Date(toDate.getTime() + 24 * 60 * 60 * 1000);
+  }
+  if (fromDate && toDateExclusive && fromDate >= toDateExclusive) {
+    res.status(400).json({ error: "'from' must be on or before 'to'" });
+    return null;
+  }
+  return { actorUserId, fromDate, toDateExclusive };
+}
+
+function buildRoleAuditWhere(roleId: number, filters: RoleAuditFilters): SQL {
+  const conditions: SQL[] = [eq(rolePermissionAuditTable.roleId, roleId)];
+  if (filters.actorUserId != null)
+    conditions.push(eq(rolePermissionAuditTable.actorUserId, filters.actorUserId));
+  if (filters.fromDate)
+    conditions.push(gte(rolePermissionAuditTable.createdAt, filters.fromDate));
+  if (filters.toDateExclusive)
+    conditions.push(lt(rolePermissionAuditTable.createdAt, filters.toDateExclusive));
+  return and(...conditions) as SQL;
+}
+
 router.get("/roles/:id/audit", requireAdmin, async (req, res): Promise<void> => {
  const id = Number(req.params.id);
  if (!Number.isInteger(id)) {
@@ -69,15 +151,31 @@ router.get("/roles/:id/audit", requireAdmin, async (req, res): Promise<void> =>
    return;
  }
  const limitRaw = Number(req.query.limit);
+  const offsetRaw = Number(req.query.offset);
  const limit =
    Number.isFinite(limitRaw) && limitRaw > 0
-      ? Math.min(Math.floor(limitRaw), 50)
+      ? Math.min(Math.floor(limitRaw), 200)
      : 10;
-  const [role] = await db.select({ id: rolesTable.id }).from(rolesTable).where(eq(rolesTable.id, id));
+  const offset =
+    Number.isFinite(offsetRaw) && offsetRaw >= 0 ? Math.floor(offsetRaw) : 0;
+
+  const [role] = await db
+    .select({ id: rolesTable.id })
+    .from(rolesTable)
+    .where(eq(rolesTable.id, id));
  if (!role) {
    res.status(404).json({ error: "Role not found" });
    return;
  }
+
+  // Filter parsing is intentionally performed AFTER the role existence
+  // check so 404 takes precedence over 400 for unknown roles, matching
+  // the behaviour of the other role sub-routes.
+  const filters = parseRoleAuditFilters(req, res);
+  if (!filters) return;
+
+  const whereClause = buildRoleAuditWhere(id, filters);
+
  const rows = await db
    .select({
      id: rolePermissionAuditTable.id,
@@ -93,9 +191,16 @@ router.get("/roles/:id/audit", requireAdmin, async (req, res): Promise<void> =>
    })
    .from(rolePermissionAuditTable)
    .leftJoin(usersTable, eq(usersTable.id, rolePermissionAuditTable.actorUserId))
-    .where(eq(rolePermissionAuditTable.roleId, id))
+    .where(whereClause)
    .orderBy(desc(rolePermissionAuditTable.createdAt), desc(rolePermissionAuditTable.id))
-    .limit(limit);
+    .limit(limit)
+    .offset(offset);
+
+  const [{ count: totalCount } = { count: 0 }] = await db
+    .select({ count: sql<number>`count(*)::int` })
+    .from(rolePermissionAuditTable)
+    .where(whereClause);
+
  const entries = rows.map((r) => {
    const prev = (r.previousPermissionIds ?? []) as number[];
    const next = (r.newPermissionIds ?? []) as number[];
@@ -121,7 +226,17 @@ router.get("/roles/:id/audit", requireAdmin, async (req, res): Promise<void> =>
          : null,
    };
  });
-  res.json(entries);
+
+  const nextOffset =
+    offset + rows.length < totalCount ? offset + rows.length : null;
+
+  res.json({
+    entries,
+    totalCount,
+    limit,
+    offset,
+    nextOffset,
+  });
 });

 router.get("/roles/:id/usage", requireAdmin, async (req, res): Promise<void> => {
@@ -199,8 +199,12 @@ test("GET /api/roles/:id/audit reports no-op saves with empty added/removed", as
    headers: { Cookie: adminCookie },
  });
  assert.equal(get.status, 200);
-  const entries = await get.json();
+  const body = await get.json();
+  const { entries } = body;
  assert.equal(entries.length, 2);
+  assert.equal(body.totalCount, 2);
+  assert.equal(body.offset, 0);
+  assert.equal(body.nextOffset, null);
  // Latest entry is the no-op save: same set in/out, empty diffs.
  assert.deepEqual(entries[0].previousPermissionIds, [p1]);
  assert.deepEqual(entries[0].newPermissionIds, [p1]);
@@ -236,11 +240,15 @@ test("GET /api/roles/:id/audit returns entries newest first with diff fields and
    headers: { Cookie: adminCookie },
  });
  assert.equal(get.status, 200);
-  const entries = await get.json();
-  assert.ok(Array.isArray(entries));
-  assert.equal(entries.length, 2);
+  const body = await get.json();
+  assert.ok(Array.isArray(body.entries));
+  assert.equal(body.entries.length, 2);
+  assert.equal(body.totalCount, 2);
+  assert.equal(body.offset, 0);
+  assert.equal(body.limit, 10);
+  assert.equal(body.nextOffset, null);

-  const [latest, earlier] = entries;
+  const [latest, earlier] = body.entries;
  assert.deepEqual(latest.previousPermissionIds, [p1]);
  assert.deepEqual(latest.newPermissionIds, [p2]);
  assert.deepEqual(latest.addedPermissionIds, [p2]);
@@ -282,6 +290,179 @@ test("GET /api/roles/:id/audit honours the limit parameter", async () => {
    { headers: { Cookie: adminCookie } },
  );
  assert.equal(limited.status, 200);
-  const entries = await limited.json();
-  assert.equal(entries.length, 2);
+  const body = await limited.json();
+  assert.equal(body.entries.length, 2);
+  assert.equal(body.totalCount, 4);
+  assert.equal(body.limit, 2);
+  assert.equal(body.offset, 0);
+  // Two more rows beyond the page → nextOffset should point at row 2.
+  assert.equal(body.nextOffset, 2);
+});
+
+test("GET /api/roles/:id/audit paginates with offset and stops at the end", async () => {
+  const role = await createRole();
+  const [p1, p2] = await getTwoPermissionIds();
+
+  // 4 distinct permission sets → 4 audit rows, deterministic ordering thanks
+  // to the small inter-write delay.
+  const sets = [[p1], [p2], [p1, p2], []];
+  for (const s of sets) {
+    const r = await fetch(`${API_BASE}/api/roles/${role.id}/permissions`, {
+      method: "PUT",
+      headers: { "Content-Type": "application/json", Cookie: adminCookie },
+      body: JSON.stringify({ permissionIds: s }),
+    });
+    assert.equal(r.status, 200);
+    await new Promise((r) => setTimeout(r, 5));
+  }
+
+  const page1Res = await fetch(
+    `${API_BASE}/api/roles/${role.id}/audit?limit=2&offset=0`,
+    { headers: { Cookie: adminCookie } },
+  );
+  assert.equal(page1Res.status, 200);
+  const page1 = await page1Res.json();
+  assert.equal(page1.entries.length, 2);
+  assert.equal(page1.totalCount, 4);
+  assert.equal(page1.nextOffset, 2);
+
+  const page2Res = await fetch(
+    `${API_BASE}/api/roles/${role.id}/audit?limit=2&offset=2`,
+    { headers: { Cookie: adminCookie } },
+  );
+  assert.equal(page2Res.status, 200);
+  const page2 = await page2Res.json();
+  assert.equal(page2.entries.length, 2);
+  assert.equal(page2.totalCount, 4);
+  // Reached the end of the table — there is nothing more to page through.
+  assert.equal(page2.nextOffset, null);
+
+  // Pages must not overlap.
+  const page1Ids = new Set(page1.entries.map((e) => e.id));
+  for (const e of page2.entries) {
+    assert.equal(page1Ids.has(e.id), false, "page1/page2 overlap");
+  }
+});
+
+test("GET /api/roles/:id/audit filters by actorUserId", async () => {
+  const role = await createRole();
+  const [p1, p2] = await getTwoPermissionIds();
+
+  // Create a SECOND admin so we can attribute audit rows to two different
+  // actors and then verify that filtering returns only one of them.
+  const stamp = `${Date.now().toString(36)}_${Math.random().toString(36).slice(2, 6)}`;
+  const otherUsername = `role_audit_admin_other_${stamp}`;
+  const otherRow = await pool.query(
+    `INSERT INTO users (username, email, password_hash, display_name_en, preferred_language, is_active)
+     VALUES ($1, $2, $3, 'Role Audit Admin Other', 'en', true) RETURNING id`,
+    [otherUsername, `${otherUsername}@example.com`, TEST_PASSWORD_HASH],
+  );
+  const otherId = otherRow.rows[0].id;
+  createdUserIds.push(otherId);
+  await pool.query(
+    `INSERT INTO user_roles (user_id, role_id) SELECT $1, id FROM roles WHERE name = 'admin'`,
+    [otherId],
+  );
+  const otherCookie = await loginAndGetCookie(otherUsername, TEST_PASSWORD);
+
+  // Two writes by adminCookie + one by otherCookie.
+  for (const [cookie, set] of [
+    [adminCookie, [p1]],
+    [otherCookie, [p1, p2]],
+    [adminCookie, [p2]],
+  ]) {
+    const r = await fetch(`${API_BASE}/api/roles/${role.id}/permissions`, {
+      method: "PUT",
+      headers: { "Content-Type": "application/json", Cookie: cookie },
+      body: JSON.stringify({ permissionIds: set }),
+    });
+    assert.equal(r.status, 200);
+    await new Promise((r) => setTimeout(r, 5));
+  }
+
+  const filteredRes = await fetch(
+    `${API_BASE}/api/roles/${role.id}/audit?actorUserId=${otherId}`,
+    { headers: { Cookie: adminCookie } },
+  );
+  assert.equal(filteredRes.status, 200);
+  const filtered = await filteredRes.json();
+  assert.equal(filtered.totalCount, 1);
+  assert.equal(filtered.entries.length, 1);
+  assert.equal(filtered.entries[0].actor.id, otherId);
+
+  // actorUserId=0 means "no filter" — should match all 3 rows.
+  const allRes = await fetch(
+    `${API_BASE}/api/roles/${role.id}/audit?actorUserId=0`,
+    { headers: { Cookie: adminCookie } },
+  );
+  assert.equal(allRes.status, 200);
+  const all = await allRes.json();
+  assert.equal(all.totalCount, 3);
+});
+
+test("GET /api/roles/:id/audit filters by date range", async () => {
+  const role = await createRole();
+  const [p1, p2] = await getTwoPermissionIds();
+
+  // Make a couple of writes "today" (UTC).
+  for (const set of [[p1], [p2]]) {
+    const r = await fetch(`${API_BASE}/api/roles/${role.id}/permissions`, {
+      method: "PUT",
+      headers: { "Content-Type": "application/json", Cookie: adminCookie },
+      body: JSON.stringify({ permissionIds: set }),
+    });
+    assert.equal(r.status, 200);
+    await new Promise((r) => setTimeout(r, 5));
+  }
+
+  const today = new Date().toISOString().slice(0, 10);
+  const tomorrow = new Date(Date.now() + 24 * 60 * 60 * 1000)
+    .toISOString()
+    .slice(0, 10);
+  const yesterday = new Date(Date.now() - 24 * 60 * 60 * 1000)
+    .toISOString()
+    .slice(0, 10);
+
+  // Today is in range → both rows should match.
+  const inRangeRes = await fetch(
+    `${API_BASE}/api/roles/${role.id}/audit?from=${today}&to=${today}`,
+    { headers: { Cookie: adminCookie } },
+  );
+  assert.equal(inRangeRes.status, 200);
+  const inRange = await inRangeRes.json();
+  assert.equal(inRange.totalCount, 2);
+
+  // Yesterday-only window → no rows.
+  const pastRes = await fetch(
+    `${API_BASE}/api/roles/${role.id}/audit?from=${yesterday}&to=${yesterday}`,
+    { headers: { Cookie: adminCookie } },
+  );
+  assert.equal(pastRes.status, 200);
+  const past = await pastRes.json();
+  assert.equal(past.totalCount, 0);
+  assert.equal(past.entries.length, 0);
+
+  // Inverted range → 400.
+  const badRes = await fetch(
+    `${API_BASE}/api/roles/${role.id}/audit?from=${tomorrow}&to=${yesterday}`,
+    { headers: { Cookie: adminCookie } },
+  );
+  assert.equal(badRes.status, 400);
+
+  // Garbage date → 400.
+  const garbageRes = await fetch(
+    `${API_BASE}/api/roles/${role.id}/audit?from=not-a-date`,
+    { headers: { Cookie: adminCookie } },
+  );
+  assert.equal(garbageRes.status, 400);
+
+  // Impossible calendar days are rejected too — without strict validation
+  // these would silently roll forward into a different real date.
+  for (const bad of ["2024-02-31", "2025-13-01", "2025-00-15", "2025-02-30"]) {
+    const r = await fetch(
+      `${API_BASE}/api/roles/${role.id}/audit?from=${bad}`,
+      { headers: { Cookie: adminCookie } },
+    );
+    assert.equal(r.status, 400, `expected 400 for from=${bad}`);
+  }
 });