diff --git a/.changeset/steady-lions-page.md b/.changeset/steady-lions-page.md new file mode 100644 index 0000000..65a7df9 --- /dev/null +++ b/.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. diff --git a/src/commands/adminShared.ts b/src/commands/adminShared.ts index 1cb09e2..933a27c 100644 --- a/src/commands/adminShared.ts +++ b/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" + }.`; +} diff --git a/src/commands/helpTopics.ts b/src/commands/helpTopics.ts index e98fd76..bbef1d2 100644 --- a/src/commands/helpTopics.ts +++ b/src/commands/helpTopics.ts @@ -356,7 +356,9 @@ config oauth-providers body: `Admin user management (requires an admin role). users list [--limit ] [--offset ] [--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 [--force] • Delete a user (asks for confirmation) users credentials [--json] @@ -383,7 +385,10 @@ users prepare-device-replacement [--force] [--keep-sessions] [--keep-passke "org , org members ", body: `Admin organization management (requires an admin role). -org list [--json] +org list [--limit ] [--offset ] [--search ] [--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 [--slug ] org get [--json] org update [--name ] [--slug ] diff --git a/src/commands/org.test.ts b/src/commands/org.test.ts index cb3b4d5..f98e873 100644 --- a/src/commands/org.test.ts +++ b/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", () => { diff --git a/src/commands/org.ts b/src/commands/org.ts index d98a73b..700d8da 100644 --- a/src/commands/org.ts +++ b/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 { async function orgList(client: AuthClient, rest: string[]): Promise { 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 { 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 { diff --git a/src/commands/users.test.ts b/src/commands/users.test.ts index 698528f..db3a238 100644 --- a/src/commands/users.test.ts +++ b/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", () => { diff --git a/src/commands/users.ts b/src/commands/users.ts index ab501f4..51cf8f4 100644 --- a/src/commands/users.ts +++ b/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 { } } -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 { 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 { 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 { diff --git a/src/core/admin.ts b/src/core/admin.ts index 7df157a..ff881e9 100644 --- a/src/core/admin.ts +++ b/src/core/admin.ts @@ -142,13 +142,29 @@ export interface OrgList { total: number; } -export async function listOrgs(client: AuthClient): Promise { +export interface ListOrgsOptions { + limit?: number; + offset?: number; + search?: string; +} + +export async function listOrgs( + client: AuthClient, + opts: ListOrgsOptions = {}, +): Promise { + 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,