fix(admin): match the list windows the auth API enforces

.changeset/steady-lions-page.md

@@ -0,0 +1,22 @@
+---
+"seamless-cli": minor
+---
+
+Match the list windows the auth API now enforces, and let `org list` page.
+
+The API validates the window on its admin list routes: `limit` is 1 to 100 and
+`offset` is 0 or more. The CLI checked both flags against one range with a floor
+of zero, which was right for `--offset` and wrong for `--limit`, so
+`users list --limit 0` and `users list --limit 500` were sent and came back as a
+400 naming neither the flag nor the bound. Each flag is checked against its own
+range now, and the message says which one was wrong and what it accepts.
+
+`--limit 0` is therefore an error rather than a request for nothing. Asking the
+server for zero rows and reporting "No users." said there were none when the CLI
+had not looked, which is worse than saying the flag is out of range.
+
+`org list` gains `--limit`, `--offset` and `--search`. It sent no window at all,
+so once the API started defaulting to 50 it printed the first 50 organizations
+and then a count of every organization, claiming rows it had not shown. It now
+reports where the page sits, the way `users list` already did, and `--search`
+matches the name and slug server-side.

src/commands/adminShared.ts

@@ -1,6 +1,10 @@
 import kleur from "kleur";
 import { ReauthRequiredError } from "../core/authClient.js";
 import { AdminApiError, PermissionError } from "../core/admin.js";
+import { extractFlag } from "../core/args.js";
+
+const DEFAULT_LIMIT = 50;
+const LIMIT_MAX = 100;
 
 export function reportAdminError(err: unknown): never {
   if (err instanceof ReauthRequiredError) {
@@ -21,3 +25,69 @@ export function parseList(value?: string): string[] | undefined {
     .map((item) => item.trim())
     .filter((item) => item.length > 0);
 }
+
+// The two flags do not share a range. `--offset 0` is the first page, but the API
+// takes a limit of 1 to 100 and answers anything outside that with a 400 naming
+// neither the flag nor the bound. Checking here says which flag was wrong and what
+// it accepts, and a non-numeric page stays a typo the server would reinterpret.
+function pageNumber(
+  raw: string | undefined,
+  flag: string,
+  fallback: number,
+  bounds: { min: number; max?: number },
+): number {
+  if (raw === undefined) return fallback;
+
+  const value = Number(raw);
+  const { min, max } = bounds;
+
+  if (!Number.isInteger(value) || value < min || (max !== undefined && value > max)) {
+    console.error(
+      kleur.red(
+        max === undefined
+          ? `--${flag} must be a whole number of ${min} or more.`
+          : `--${flag} must be a whole number between ${min} and ${max}.`,
+      ),
+    );
+    process.exit(1);
+  }
+
+  return value;
+}
+
+/** Reads `--limit` and `--offset` off a list command, and the arguments left over. */
+export function parseWindow(args: string[]): {
+  limit: number;
+  offset: number;
+  rest: string[];
+} {
+  const limitFlag = extractFlag(args, "limit");
+  const offsetFlag = extractFlag(limitFlag.rest, "offset");
+
+  return {
+    limit: pageNumber(limitFlag.value, "limit", DEFAULT_LIMIT, {
+      min: 1,
+      max: LIMIT_MAX,
+    }),
+    offset: pageNumber(offsetFlag.value, "offset", 0, { min: 0 }),
+    rest: offsetFlag.rest,
+  };
+}
+
+/**
+ * The position of the rows on screen within the whole result set.
+ *
+ * Printed rather than a bare count, because a list route returns one page and a
+ * count of every match. Reporting the count alone reads as though every row is on
+ * screen, which is how `org list` came to claim a total it had not shown.
+ */
+export function pagePosition(
+  offset: number,
+  shown: number,
+  total: number,
+  noun: string,
+): string {
+  return `Showing ${offset + 1}-${offset + shown} of ${total} ${noun}${
+    total === 1 ? "" : "s"
+  }.`;
+}

src/commands/helpTopics.ts

@@ -356,7 +356,9 @@ config oauth-providers <list|add|update|remove>
         body: `Admin user management (requires an admin role).
 
 users list [--limit <n>] [--offset <n>] [--json]
-  • List users
+  • List users, 50 at a time by default
+  • --limit is 1 to 100, --offset is 0 or more; total counts every user, not
+    just the page shown
 users delete <id> [--force]
   • Delete a user (asks for confirmation)
 users credentials <id> [--json]
@@ -383,7 +385,10 @@ users prepare-device-replacement <id> [--force] [--keep-sessions] [--keep-passke
           "org <list|create|get|update>, org members <list|add|update|remove>",
         body: `Admin organization management (requires an admin role).
 
-org list [--json]
+org list [--limit <n>] [--offset <n>] [--search <text>] [--json]
+  • Lists 50 at a time; --limit is 1 to 100, --offset is 0 or more
+  • --search matches the name and slug, and total counts every match rather
+    than the page shown
 org create <name> [--slug <slug>]
 org get <id> [--json]
 org update <id> [--name <name>] [--slug <slug>]

src/commands/org.test.ts

@@ -128,7 +128,10 @@ describe("runOrg list", () => {
       total: 1,
     });
     await runOrg(["list", "--json"]);
-    expect(vi.mocked(listOrgs)).toHaveBeenCalledWith(fakeClient);
+    expect(vi.mocked(listOrgs)).toHaveBeenCalledWith(fakeClient, {
+      limit: 50,
+      offset: 0,
+    });
     expect(logs()).toContain(JSON.stringify([{ id: "o1", name: "Acme" }], null, 2));
   });
 
@@ -160,6 +163,39 @@ describe("runOrg list", () => {
     expect(logs()).toContain("(no name)");
     expect(logs()).toContain("2 organizations.");
   });
+
+  // The endpoint returns one page and a count of every match, so printing the
+  // count alone claimed rows that were never shown.
+  it("reports where the page sits in the result set", async () => {
+    vi.mocked(listOrgs).mockResolvedValue({
+      organizations: [{ id: "o1", name: "Acme" }],
+      total: 140,
+    });
+    await runOrg(["list", "--limit", "1", "--offset", "50"]);
+    expect(vi.mocked(listOrgs)).toHaveBeenCalledWith(fakeClient, {
+      limit: 1,
+      offset: 50,
+    });
+    expect(logs()).toContain("Showing 51-51 of 140 organizations.");
+  });
+
+  it("sends a search term when one is given", async () => {
+    vi.mocked(listOrgs).mockResolvedValue({
+      organizations: [{ id: "o1", name: "Acme" }],
+      total: 1,
+    });
+    await runOrg(["list", "--search", "acme"]);
+    expect(vi.mocked(listOrgs)).toHaveBeenCalledWith(fakeClient, {
+      limit: 50,
+      offset: 0,
+      search: "acme",
+    });
+  });
+
+  it("rejects a window the API would refuse", async () => {
+    await expect(runOrg(["list", "--limit", "0"])).rejects.toBeInstanceOf(ExitError);
+    expect(vi.mocked(listOrgs)).not.toHaveBeenCalled();
+  });
 });
 
 describe("runOrg create", () => {

src/commands/org.ts

@@ -12,7 +12,12 @@ import {
   updateOrg,
   type Json,
 } from "../core/admin.js";
-import { parseList, reportAdminError } from "./adminShared.js";
+import {
+  pagePosition,
+  parseList,
+  parseWindow,
+  reportAdminError,
+} from "./adminShared.js";
 import {
   confirmDestructive,
   hasForceFlag,
@@ -83,7 +88,14 @@ async function runOrgMembers(args: string[]): Promise<void> {
 
 async function orgList(client: AuthClient, rest: string[]): Promise<void> {
   const json = rest.includes("--json");
-  const { organizations, total } = await listOrgs(client);
+  const { limit, offset, rest: remaining } = parseWindow(rest);
+  const { value: search } = extractFlag(remaining, "search");
+
+  const { organizations, total } = await listOrgs(client, {
+    limit,
+    offset,
+    ...(search ? { search } : {}),
+  });
 
   if (json) {
     console.log(JSON.stringify(organizations, null, 2));
@@ -96,7 +108,9 @@ async function orgList(client: AuthClient, rest: string[]): Promise<void> {
   for (const org of organizations) {
     printOrgRow(org);
   }
-  console.log(kleur.dim(`${total} organization${total === 1 ? "" : "s"}.`));
+  console.log(
+    kleur.dim(pagePosition(offset, organizations.length, total, "organization")),
+  );
 }
 
 async function orgCreate(client: AuthClient, rest: string[]): Promise<void> {

src/commands/users.test.ts

@@ -183,13 +183,45 @@ describe("runUsers list", () => {
     ["--limit", "abc"],
     ["--limit", "-1"],
     ["--offset", "1.5"],
+    ["--offset", "-1"],
   ])("rejects %s %s", async (flag, value) => {
     await expect(runUsers(["list", flag, value])).rejects.toBeInstanceOf(ExitError);
     expect(errorSpy).toHaveBeenCalledWith(
-      expect.stringContaining("must be a non-negative whole number"),
+      expect.stringContaining("must be a whole number"),
     );
     expect(vi.mocked(listUsers)).not.toHaveBeenCalled();
   });
+
+  // The two flags do not share a range, so the message has to name the one that
+  // was wrong and the bound it broke.
+  it.each(["0", "101"])(
+    "rejects --limit %s outside the range the API accepts",
+    async (value) => {
+      await expect(runUsers(["list", "--limit", value])).rejects.toBeInstanceOf(ExitError);
+      expect(errorSpy).toHaveBeenCalledWith(
+        "--limit must be a whole number between 1 and 100.",
+      );
+      expect(vi.mocked(listUsers)).not.toHaveBeenCalled();
+    },
+  );
+
+  it("still accepts an offset of zero", async () => {
+    vi.mocked(listUsers).mockResolvedValue({ users: [{ id: "u1" }], total: 1 });
+    await runUsers(["list", "--offset", "0"]);
+    expect(vi.mocked(listUsers)).toHaveBeenCalledWith(fakeClient, {
+      limit: 50,
+      offset: 0,
+    });
+  });
+
+  it("accepts the largest window the API allows", async () => {
+    vi.mocked(listUsers).mockResolvedValue({ users: [{ id: "u1" }], total: 1 });
+    await runUsers(["list", "--limit", "100"]);
+    expect(vi.mocked(listUsers)).toHaveBeenCalledWith(fakeClient, {
+      limit: 100,
+      offset: 0,
+    });
+  });
 });
 
 describe("runUsers delete", () => {

src/commands/users.ts

@@ -8,7 +8,11 @@ import {
   prepareDeviceReplacement,
   type Json,
 } from "../core/admin.js";
-import { reportAdminError } from "./adminShared.js";
+import {
+  pagePosition,
+  parseWindow,
+  reportAdminError,
+} from "./adminShared.js";
 import {
   confirmDestructive,
   hasForceFlag,
@@ -45,27 +49,9 @@ export async function runUsers(args: string[]): Promise<void> {
   }
 }
 
-const DEFAULT_LIMIT = 50;
-
-// `--limit 0` is a legitimate answer (ask for nothing), so the floor is 0 rather
-// than 1, but a negative or non-numeric page is a typo the server would silently
-// reinterpret.
-function pageNumber(raw: string | undefined, flag: string, fallback: number): number {
-  if (raw === undefined) return fallback;
-  const value = Number(raw);
-  if (!Number.isInteger(value) || value < 0) {
-    console.error(kleur.red(`--${flag} must be a non-negative whole number.`));
-    process.exit(1);
-  }
-  return value;
-}
-
 async function usersList(client: AuthClient, rest: string[]): Promise<void> {
   const json = rest.includes("--json");
-  const limitFlag = extractFlag(rest, "limit");
-  const offsetFlag = extractFlag(limitFlag.rest, "offset");
-  const limit = pageNumber(limitFlag.value, "limit", DEFAULT_LIMIT);
-  const offset = pageNumber(offsetFlag.value, "offset", 0);
+  const { limit, offset } = parseWindow(rest);
 
   const { users, total } = await listUsers(client, { limit, offset });
 
@@ -82,13 +68,7 @@ async function usersList(client: AuthClient, rest: string[]): Promise<void> {
   for (const user of users) {
     printUserRow(user);
   }
-  console.log(
-    kleur.dim(
-      `Showing ${offset + 1}-${offset + users.length} of ${total} user${
-        total === 1 ? "" : "s"
-      }.`,
-    ),
-  );
+  console.log(kleur.dim(pagePosition(offset, users.length, total, "user")));
 }
 
 async function usersDelete(client: AuthClient, rest: string[]): Promise<void> {

src/core/admin.ts

@@ -142,13 +142,29 @@ export interface OrgList {
   total: number;
 }
 
-export async function listOrgs(client: AuthClient): Promise<OrgList> {
+export interface ListOrgsOptions {
+  limit?: number;
+  offset?: number;
+  search?: string;
+}
+
+export async function listOrgs(
+  client: AuthClient,
+  opts: ListOrgsOptions = {},
+): Promise<OrgList> {
+  const query = new URLSearchParams();
+  if (opts.limit !== undefined) query.set("limit", String(opts.limit));
+  if (opts.offset !== undefined) query.set("offset", String(opts.offset));
+  if (opts.search) query.set("search", opts.search);
+  const suffix = query.size ? `?${query}` : "";
+
   const res = await call<{ organizations?: unknown; total?: number }>(
     client,
     "GET",
-    "/admin/organizations",
+    `/admin/organizations${suffix}`,
   );
   if (!res.ok) throw new AdminApiError(`Could not list organizations (${res.status}).`);
+  // `total` counts every match, not just this page, so callers can report position.
   return {
     organizations: arr(res.data?.organizations),
     total: res.data?.total ?? 0,