diff --git a/server/src/__tests__/helpers/route-test-harness.ts b/server/src/__tests__/helpers/route-test-harness.ts new file mode 100644 index 0000000000..6b704caf6a --- /dev/null +++ b/server/src/__tests__/helpers/route-test-harness.ts @@ -0,0 +1,141 @@ +import { randomUUID } from "node:crypto"; +import express, { type Router } from "express"; +import { isNotNull } from "drizzle-orm"; +import { + companies, + companyMemberships, + createDb, + issues, + principalPermissionGrants, +} from "@paperclipai/db"; +import { afterAll, afterEach, beforeAll, describe } from "vitest"; +import { errorHandler } from "../../middleware/index.js"; +import { ensureHumanRoleDefaultGrants } from "../../services/principal-access-compatibility.js"; +import { + getEmbeddedPostgresTestSupport, + startEmbeddedPostgresTestDatabase, +} from "./embedded-postgres.js"; + +type Db = ReturnType; + +const embeddedPostgresSupport = await getEmbeddedPostgresTestSupport(); + +/** `describe` on hosts that can run embedded Postgres, `describe.skip` elsewhere. */ +export const describeEmbeddedPostgres = embeddedPostgresSupport.supported ? describe : describe.skip; + +export type EmbeddedPostgresContext = { readonly db: Db }; + +/** + * Registers the embedded Postgres lifecycle for the enclosing `describe`. Read + * `.db` from inside a test; it is unavailable until `beforeAll` has run. + */ +export function useEmbeddedPostgres( + prefix: string, + opts: { resetEach?: (db: Db) => Promise } = {}, +): EmbeddedPostgresContext { + let db: Db | null = null; + let tempDb: Awaited> | null = null; + + beforeAll(async () => { + tempDb = await startEmbeddedPostgresTestDatabase(prefix); + db = createDb(tempDb.connectionString); + }, 20_000); + + if (opts.resetEach) { + const resetEach = opts.resetEach; + afterEach(async () => { + if (db) await resetEach(db); + }); + } + + afterAll(async () => { + await tempDb?.cleanup(); + tempDb = null; + db = null; + }); + + return { + get db() { + if (!db) throw new Error("embedded Postgres is not started yet — read .db inside a test"); + return db; + }, + }; +} + +export type BoardActor = { + type: "board"; + source: string; + userId: string; + companyIds: string[]; + memberships: { companyId: string; membershipRole: string; status: string }[]; + isInstanceAdmin: boolean; +}; + +export type SeededCompany = { companyId: string; userId: string; actor: BoardActor }; + +/** Creates a company whose board user holds company-scoped read and write grants. */ +export async function seedCompanyWithBoardAccess(db: Db, name: string): Promise { + const companyId = randomUUID(); + const userId = `user-${randomUUID()}`; + await db.insert(companies).values({ + id: companyId, + name: `${name} ${companyId}`, + issuePrefix: `T${companyId.replaceAll("-", "").slice(0, 6).toUpperCase()}`, + requireBoardApprovalForNewAgents: false, + }); + await db.insert(companyMemberships).values({ + companyId, + principalType: "user", + principalId: userId, + status: "active", + membershipRole: "owner", + updatedAt: new Date(), + }); + await ensureHumanRoleDefaultGrants(db, { + companyId, + principalId: userId, + membershipRole: "owner", + grantedByUserId: null, + }); + return { + companyId, + userId, + actor: { + type: "board", + source: "session", + userId, + companyIds: [companyId], + memberships: [{ companyId, membershipRole: "owner", status: "active" }], + isInstanceAdmin: false, + }, + }; +} + +type RouterFactory = (db: Db, storage: never) => Router; + +/** Mounts route modules under `/api` behind a fixed actor, with the real error handler. */ +export function routeApp(db: Db, actor: BoardActor, ...routerFactories: RouterFactory[]) { + const app = express(); + app.use(express.json()); + app.use((req, _res, next) => { + (req as any).actor = actor; + next(); + }); + for (const factory of routerFactories) { + app.use("/api", factory(db, {} as never)); + } + app.use(errorHandler); + return app; +} + +/** + * Clears the company and issue fixtures seeded by `seedCompanyWithBoardAccess`. + * Suites that seed other tables need their own reset. + */ +export async function resetCompanyIssueFixtures(db: Db) { + await db.delete(issues).where(isNotNull(issues.parentId)); + await db.delete(issues); + await db.delete(principalPermissionGrants); + await db.delete(companyMemberships); + await db.delete(companies); +} diff --git a/server/src/__tests__/issues-list-query-parsing.test.ts b/server/src/__tests__/issues-list-query-parsing.test.ts index f601aa8fc2..91024b93d0 100644 --- a/server/src/__tests__/issues-list-query-parsing.test.ts +++ b/server/src/__tests__/issues-list-query-parsing.test.ts @@ -1,74 +1,59 @@ -import express from "express"; +import { randomUUID } from "node:crypto"; import request from "supertest"; -import { describe, expect, it } from "vitest"; -import { parseStatusFilter } from "../services/issues.ts"; +import { expect, it } from "vitest"; +import { issues } from "@paperclipai/db"; +import { issueRoutes } from "../routes/issues.js"; +import { + describeEmbeddedPostgres, + resetCompanyIssueFixtures, + routeApp, + seedCompanyWithBoardAccess, + useEmbeddedPostgres, +} from "./helpers/route-test-harness.js"; /** - * Regression test for https://github.com/paperclipai/paperclip/issues/4628 - * - * Stands up a minimal Express app whose handler mirrors the parsing path - * `server/src/routes/issues.ts:957-958` uses to forward `req.query.status` - * into `issueService.list({ status })`. Verifies the four shapes Express's - * default `qs` parser can produce all normalize correctly and none crash: - * - * 1. Single value — `?status=todo` - * 2. Comma-separated — `?status=todo,in_progress` (legacy) - * 3. Repeated key (array) — `?status=todo&status=in_progress` (the bug) - * 4. Mixed array + CSV — `?status=todo,in_progress&status=done` - * - * Pre-fix, case 3 returned HTTP 500 with `TypeError: filters.status.split is - * not a function`. We don't spin embedded-postgres here; the helper itself is - * unit-tested in `parse-status-filter.test.ts`, and the route→helper contract - * is what regresses if anyone reverts the fix. + * Regression coverage for https://github.com/paperclipai/paperclip/issues/4628. + * Express's `qs` parser hands the list route either a string or an array for + * `?status=`, and the route normalizes both shapes. */ -function buildApp() { - const app = express(); - app.use(express.json()); - app.get("/api/companies/:companyId/issues", (req, res) => { - // Mirror the cast at routes/issues.ts:958 exactly. Pre-fix this said - // `as string | undefined` and the cast was a lie when qs returned an array. - const statusInput = req.query.status as string | string[] | undefined; - const statuses = parseStatusFilter(statusInput); - res.status(200).json({ statuses }); - }); - return app; -} - -describe("issue list status query parsing", () => { - it("accepts a single ?status=todo and returns one normalized status", async () => { - const res = await request(buildApp()).get("/api/companies/c1/issues?status=todo"); - expect(res.status).toBe(200); - expect(res.body).toEqual({ statuses: ["todo"] }); +describeEmbeddedPostgres("issue list status query parsing", () => { + const ctx = useEmbeddedPostgres("paperclip-issues-list-query-parsing-", { + resetEach: resetCompanyIssueFixtures, }); - it("accepts comma-separated ?status=todo,in_progress (preserves legacy CSV)", async () => { - const res = await request(buildApp()).get( - "/api/companies/c1/issues?status=todo,in_progress", - ); - expect(res.status).toBe(200); - expect(res.body).toEqual({ statuses: ["todo", "in_progress"] }); + async function listStatuses(query: string) { + const company = await seedCompanyWithBoardAccess(ctx.db, "Status parsing"); + const companyId = company.companyId; + await ctx.db.insert(issues).values([ + { id: randomUUID(), companyId, title: "Todo", status: "todo", priority: "medium" }, + { id: randomUUID(), companyId, title: "In progress", status: "in_progress", priority: "medium" }, + { id: randomUUID(), companyId, title: "Done", status: "done", priority: "medium" }, + ]); + const res = await request(routeApp(ctx.db, company.actor, issueRoutes)) + .get(`/api/companies/${companyId}/issues${query}`) + .expect(200); + return (res.body as { status: string }[]).map((issue) => issue.status).sort(); + } + + it("accepts a single ?status=todo", async () => { + expect(await listStatuses("?status=todo")).toEqual(["todo"]); }); - it("accepts repeated ?status=todo&status=in_progress without crashing (the bug fix)", async () => { - const res = await request(buildApp()).get( - "/api/companies/c1/issues?status=todo&status=in_progress", - ); - expect(res.status).toBe(200); - expect(res.body).toEqual({ statuses: ["todo", "in_progress"] }); + it("accepts comma-separated ?status=todo,in_progress", async () => { + expect(await listStatuses("?status=todo,in_progress")).toEqual(["in_progress", "todo"]); }); - it("accepts mixed array + CSV ?status=todo,in_progress&status=done", async () => { - const res = await request(buildApp()).get( - "/api/companies/c1/issues?status=todo,in_progress&status=done", - ); - expect(res.status).toBe(200); - expect(res.body).toEqual({ statuses: ["todo", "in_progress", "done"] }); + it("accepts repeated ?status=todo&status=in_progress", async () => { + expect(await listStatuses("?status=todo&status=in_progress")).toEqual(["in_progress", "todo"]); }); - it("accepts no ?status param and returns an empty status filter", async () => { - const res = await request(buildApp()).get("/api/companies/c1/issues"); - expect(res.status).toBe(200); - expect(res.body).toEqual({ statuses: [] }); + it("accepts mixed array and CSV ?status=todo,in_progress&status=done", async () => { + expect(await listStatuses("?status=todo,in_progress&status=done")) + .toEqual(["done", "in_progress", "todo"]); + }); + + it("returns every status when ?status is absent", async () => { + expect(await listStatuses("")).toEqual(["done", "in_progress", "todo"]); }); }); diff --git a/server/src/__tests__/issues-parent-id-alias.test.ts b/server/src/__tests__/issues-parent-id-alias.test.ts new file mode 100644 index 0000000000..5d1be26202 --- /dev/null +++ b/server/src/__tests__/issues-parent-id-alias.test.ts @@ -0,0 +1,116 @@ +import { randomUUID } from "node:crypto"; +import request from "supertest"; +import { expect, it } from "vitest"; +import { issues } from "@paperclipai/db"; +import { issueRoutes } from "../routes/issues.js"; +import { + describeEmbeddedPostgres, + resetCompanyIssueFixtures, + routeApp, + seedCompanyWithBoardAccess, + useEmbeddedPostgres, +} from "./helpers/route-test-harness.js"; + +describeEmbeddedPostgres("issue list parentIssueId query alias", () => { + const ctx = useEmbeddedPostgres("paperclip-issues-parent-id-alias-", { + resetEach: resetCompanyIssueFixtures, + }); + + async function seed() { + const company = await seedCompanyWithBoardAccess(ctx.db, "Parent alias"); + const companyId = company.companyId; + const parentId = randomUUID(); + const otherParentId = randomUUID(); + const childId = randomUUID(); + const blockedChildId = randomUUID(); + + await ctx.db.insert(issues).values([ + { id: parentId, companyId, title: "Parent", status: "todo", priority: "medium" }, + { id: otherParentId, companyId, title: "Other parent", status: "todo", priority: "medium" }, + ]); + await ctx.db.insert(issues).values([ + { id: childId, companyId, title: "Child", status: "todo", priority: "medium", parentId }, + { id: blockedChildId, companyId, title: "Blocked child", status: "blocked", priority: "medium", parentId }, + { + id: randomUUID(), + companyId, + title: "Other child", + status: "todo", + priority: "medium", + parentId: otherParentId, + }, + { + id: randomUUID(), + companyId, + title: "Blocked other child", + status: "blocked", + priority: "medium", + parentId: otherParentId, + }, + ]); + + return { ...company, parentId, otherParentId, childId, blockedChildId }; + } + + type Seeded = Awaited>; + + function appFor(seeded: Seeded) { + return routeApp(ctx.db, seeded.actor, issueRoutes); + } + + async function listIds(seeded: Seeded, query: Record) { + const res = await request(appFor(seeded)) + .get(`/api/companies/${seeded.companyId}/issues`) + .query(query) + .expect(200); + return (res.body as { id: string }[]).map((issue) => issue.id).sort(); + } + + async function blockedCount(seeded: Seeded, query: Record) { + const res = await request(appFor(seeded)) + .get(`/api/companies/${seeded.companyId}/issues/count`) + .query({ attention: "blocked", ...query }) + .expect(200); + return res.body as { count: number }; + } + + function childrenOfParent(seeded: Seeded) { + return [seeded.childId, seeded.blockedChildId].sort(); + } + + it("filters children by ?parentId=", async () => { + const seeded = await seed(); + expect(await listIds(seeded, { parentId: seeded.parentId })).toEqual(childrenOfParent(seeded)); + }); + + it("filters children by ?parentIssueId=", async () => { + const seeded = await seed(); + expect(await listIds(seeded, { parentIssueId: seeded.parentId })).toEqual(childrenOfParent(seeded)); + }); + + it("prefers ?parentId= when both spellings are present", async () => { + const seeded = await seed(); + const ids = await listIds(seeded, { + parentId: seeded.parentId, + parentIssueId: seeded.otherParentId, + }); + expect(ids).toEqual(childrenOfParent(seeded)); + }); + + it("returns the whole company list when neither spelling is present", async () => { + const seeded = await seed(); + expect(await listIds(seeded, {})).toHaveLength(6); + }); + + it("filters the blocked count by ?parentIssueId=", async () => { + const seeded = await seed(); + expect(await blockedCount(seeded, { parentIssueId: seeded.parentId })).toEqual({ count: 1 }); + }); + + it("returns the same blocked count for both spellings", async () => { + const seeded = await seed(); + const byShortForm = await blockedCount(seeded, { parentId: seeded.parentId }); + const byAlias = await blockedCount(seeded, { parentIssueId: seeded.parentId }); + expect(byAlias).toEqual(byShortForm); + }); +}); diff --git a/server/src/routes/issues.ts b/server/src/routes/issues.ts index 8477bbdd1a..811efef137 100644 --- a/server/src/routes/issues.ts +++ b/server/src/routes/issues.ts @@ -5210,7 +5210,7 @@ export function issueRoutes( projectId: req.query.projectId as string | undefined, workspaceId: req.query.workspaceId as string | undefined, executionWorkspaceId: req.query.executionWorkspaceId as string | undefined, - parentId: req.query.parentId as string | undefined, + parentId: (req.query.parentId ?? req.query.parentIssueId) as string | undefined, descendantOf: req.query.descendantOf as string | undefined, labelId: req.query.labelId as string | undefined, originKind: req.query.originKind as string | undefined, @@ -5402,7 +5402,7 @@ export function issueRoutes( projectId: req.query.projectId as string | undefined, workspaceId: req.query.workspaceId as string | undefined, executionWorkspaceId: req.query.executionWorkspaceId as string | undefined, - parentId: req.query.parentId as string | undefined, + parentId: (req.query.parentId ?? req.query.parentIssueId) as string | undefined, descendantOf: req.query.descendantOf as string | undefined, labelId: req.query.labelId as string | undefined, originKind: req.query.originKind as string | undefined,