diff --git a/App/.env.example b/App/.env.example index 3b7c413..a92b171 100644 --- a/App/.env.example +++ b/App/.env.example @@ -78,8 +78,9 @@ FORGEJO_TOKEN= # Let submitters (TECHNICAL/MANNING) read & export every PO and open the History # page (read-only). Opt-in — on only when exactly "true". # NEXT_PUBLIC_SUBMITTER_VIEW_ALL_ENABLED=true -# Let a CLOSED PO's own submitter (plus Accounts/Manager/SuperUser) add attachments -# to it — remediation for POs whose uploads were lost to the document-upload bug. +# Let a PO's own submitter (plus Accounts/Manager/SuperUser) add attachments to it +# in any state except rejected/cancelled — remediation for POs whose uploads were +# lost to the document-upload bug, and the general "attach after the fact" affordance. # Opt-in — on only when exactly "true". # NEXT_PUBLIC_CLOSED_PO_ATTACHMENTS_ENABLED=true diff --git a/App/CLAUDE.md b/App/CLAUDE.md index 4d02d19..26ad355 100644 --- a/App/CLAUDE.md +++ b/App/CLAUDE.md @@ -295,7 +295,7 @@ APP_INTERNAL_URL # Base URL PdfService reaches the app at (falls back to NEXT_PUBLIC_INVENTORY_ENABLED # Inventory feature flag NEXT_PUBLIC_SUBMITTER_VIEW_ALL_ENABLED # Opt-in ("true"): submitters (TECHNICAL/MANNING) read & export every PO + History (read-only) NEXT_PUBLIC_CREWING_ENABLED # Crewing module feature flag (opt-in "true"; off by default) -NEXT_PUBLIC_CLOSED_PO_ATTACHMENTS_ENABLED # Opt-in ("true"): a CLOSED PO's own submitter + Accounts/Manager/SuperUser may add attachments (remediation for the upload bug). Off by default. +NEXT_PUBLIC_CLOSED_PO_ATTACHMENTS_ENABLED # Opt-in ("true"): a PO's own submitter + Accounts/Manager/SuperUser may add attachments in any state except rejected/cancelled (upload-bug remediation + general "attach after the fact"). Off by default. NEXT_PUBLIC_ENV_LABEL # When set, shows a non-prod banner (EnvBanner). Leave unset in prod. ``` diff --git a/App/app/actions/upload-po-documents.ts b/App/app/actions/upload-po-documents.ts index 1dbabbd..3e1f8f5 100644 --- a/App/app/actions/upload-po-documents.ts +++ b/App/app/actions/upload-po-documents.ts @@ -3,7 +3,8 @@ import { auth } from "@/auth"; import { db } from "@/lib/db"; import { buildStorageKey, uploadBuffer } from "@/lib/storage"; -import { canAddClosedPoAttachment } from "@/lib/permissions"; +import { canAddPoAttachment } from "@/lib/permissions"; +import { CLOSED_PO_ATTACHMENTS_ENABLED } from "@/lib/feature-flags"; import { revalidatePath } from "next/cache"; // Matches the FileUploader hint ("up to 10 MB each") and @@ -43,16 +44,25 @@ export async function uploadPoDocuments( }); if (!po) return { error: "PO not found" }; - // A CLOSED PO is otherwise immutable; attaching to one is only allowed via the - // feature-flagged remediation path (canAddClosedPoAttachment). The normal create - // and receipt flows upload while the PO is pre-CLOSED, so they're unaffected. - if (po.status === "CLOSED") { - const allowed = canAddClosedPoAttachment(session.user.role, { + // A voided PO never accepts attachments, regardless of the flag. + if (po.status === "REJECTED" || po.status === "CANCELLED") { + return { error: "Attachments can't be added to a rejected or cancelled purchase order." }; + } + + if (CLOSED_PO_ATTACHMENTS_ENABLED) { + // Feature on: only the PO's submitter + Accounts / Manager / SuperUser may + // attach, in any non-voided state. The normal create / receipt flows are run + // by exactly those actors, so they keep working. + const allowed = canAddPoAttachment(session.user.role, po.status, { isSubmitter: po.submitterId === session.user.id, }); if (!allowed) { - return { error: "Adding attachments to a closed purchase order isn't allowed." }; + return { error: "Adding attachments to this purchase order isn't allowed." }; } + } else if (po.status === "CLOSED") { + // Feature off: a closed PO stays immutable (legacy behaviour). The create / + // receipt flows upload while the PO is pre-CLOSED, so they're unaffected. + return { error: "Adding attachments to a closed purchase order isn't allowed." }; } for (const file of files) { diff --git a/App/components/po/closed-po-attachment-uploader.tsx b/App/components/po/po-attachment-uploader.tsx similarity index 77% rename from App/components/po/closed-po-attachment-uploader.tsx rename to App/components/po/po-attachment-uploader.tsx index c0eff65..da57000 100644 --- a/App/components/po/closed-po-attachment-uploader.tsx +++ b/App/components/po/po-attachment-uploader.tsx @@ -6,12 +6,12 @@ import { FileUploader } from "@/components/po/file-uploader"; import { uploadPoDocuments } from "@/app/actions/upload-po-documents"; /** - * Feature-flagged uploader shown on a CLOSED PO's detail page so its submitter (or - * Accounts / Manager / SuperUser) can attach documents that were lost to the upload - * bug. Gating is decided server-side in po-detail.tsx; the server action re-checks - * the permission, so this component is only the UI. + * Feature-flagged uploader shown on a PO's detail page so its submitter (or + * Accounts / Manager / SuperUser) can add documents after the fact — in any state + * except rejected/cancelled. Gating is decided server-side in po-detail.tsx; the + * server action re-checks the permission, so this component is only the UI. */ -export function ClosedPoAttachmentUploader({ poId }: { poId: string }) { +export function PoAttachmentUploader({ poId }: { poId: string }) { const router = useRouter(); const [files, setFiles] = useState([]); const [busy, setBusy] = useState(false); @@ -36,7 +36,7 @@ export function ClosedPoAttachmentUploader({ poId }: { poId: string }) {

Add attachments

- This purchase order is closed. Attach any documents that are missing. + Attach any documents that are missing from this purchase order.

diff --git a/App/components/po/po-detail.tsx b/App/components/po/po-detail.tsx index d8511c1..77d811d 100644 --- a/App/components/po/po-detail.tsx +++ b/App/components/po/po-detail.tsx @@ -5,14 +5,13 @@ import { DiscardDraftButton } from "@/components/po/discard-draft-button"; import { SubmitDraftButton } from "@/components/po/submit-draft-button"; import { CancelPoButton, SupersedeForm } from "@/components/po/cancel-po-controls"; import { EmailVendorButton } from "@/components/po/email-vendor-button"; -import { ClosedPoAttachmentUploader } from "@/components/po/closed-po-attachment-uploader"; +import { PoAttachmentUploader } from "@/components/po/po-attachment-uploader"; import { formatCurrency, formatDate, formatDateTime } from "@/lib/utils"; import { generateDownloadUrl } from "@/lib/storage"; import { groupAttachments } from "@/lib/attachments"; -import { canAddClosedPoAttachment } from "@/lib/permissions"; +import { canAddPoAttachment, hasPermission } from "@/lib/permissions"; import { TC_FIXED_LINE } from "@/lib/validations/po"; import { parsePoTerms } from "@/lib/terms"; -import { hasPermission } from "@/lib/permissions"; import type { LineItemInput } from "@/lib/validations/po"; import type { Role } from "@prisma/client"; @@ -174,12 +173,12 @@ export async function PoDetail({ po, currentUserId, currentRole, readOnly = fals ); const attachmentGroups = groupAttachments(docsWithUrls); - // Feature-flagged remediation: a closed PO's submitter (or Accounts / Manager / - // SuperUser) may attach documents that the upload bug dropped. Never in readOnly. - const canAddClosedAttachment = + // Feature-flagged: the PO's submitter (or Accounts / Manager / SuperUser) may add + // attachments after the fact, in any state except rejected/cancelled. Never in + // readOnly. The server action re-checks this permission. + const canAddAttachment = !readOnly && - po.status === "CLOSED" && - canAddClosedPoAttachment(currentRole, { isSubmitter: po.submitter.id === currentUserId }); + canAddPoAttachment(currentRole, po.status, { isSubmitter: po.submitter.id === currentUserId }); const canConfirmReceipt = (po.status === "PAID_DELIVERED" || po.status === "PARTIALLY_CLOSED" || po.status === "PARTIALLY_PAID") && @@ -518,7 +517,7 @@ export async function PoDetail({ po, currentUserId, currentRole, readOnly = fals })()} {/* Documents — grouped by lifecycle stage (submission / payment / delivery) */} - {(attachmentGroups.length > 0 || canAddClosedAttachment) && ( + {(attachmentGroups.length > 0 || canAddAttachment) && (

Attachments

{attachmentGroups.length === 0 && ( @@ -554,7 +553,7 @@ export async function PoDetail({ po, currentUserId, currentRole, readOnly = fals
))}
- {canAddClosedAttachment && } + {canAddAttachment && }
)} diff --git a/App/lib/feature-flags.ts b/App/lib/feature-flags.ts index c1fe01a..b875545 100644 --- a/App/lib/feature-flags.ts +++ b/App/lib/feature-flags.ts @@ -16,11 +16,12 @@ * keeping it dark by default leaves production unchanged. See lib/permissions.ts (§6 matrix) * and wiki Crewing-Implementation-Spec. * - * NEXT_PUBLIC_CLOSED_PO_ATTACHMENTS_ENABLED=true → lets a CLOSED PO's own submitter, plus - * Accounts / Manager / SuperUser, add attachments to it. Remediation path for the upload - * bug where documents never persisted (no PODocument row): closed POs whose files were lost - * can be fixed without reopening them. Opt-in (off unless "true") so production is unchanged - * until enabled. See lib/permissions.ts (canAddClosedPoAttachment). + * NEXT_PUBLIC_CLOSED_PO_ATTACHMENTS_ENABLED=true → lets a PO's own submitter, plus + * Accounts / Manager / SuperUser, add attachments to it in any state EXCEPT + * rejected/cancelled. Remediation path for the upload bug where documents never persisted + * (no PODocument row), and the general "attach a document after the fact" affordance. + * Opt-in (off unless "true") so production is unchanged until enabled. + * See lib/permissions.ts (canAddPoAttachment). */ export const INVENTORY_ENABLED = diff --git a/App/lib/permissions.ts b/App/lib/permissions.ts index 95be0ca..b094d1e 100644 --- a/App/lib/permissions.ts +++ b/App/lib/permissions.ts @@ -1,4 +1,4 @@ -import type { Role } from "@prisma/client"; +import type { Role, POStatus } from "@prisma/client"; import { SUBMITTER_VIEW_ALL_ENABLED, CLOSED_PO_ATTACHMENTS_ENABLED } from "./feature-flags"; export type Permission = @@ -279,23 +279,30 @@ export function canViewAllPos(role: Role): boolean { return hasPermission(role, "view_all_pos") || submitterCanViewAll(role); } -// ── Closed-PO attachments (feature-flagged remediation) ─────────────────────── -// Roles that may attach to ANY closed PO (the PO's own submitter is allowed too, -// regardless of role) when NEXT_PUBLIC_CLOSED_PO_ATTACHMENTS_ENABLED=true. -const CLOSED_PO_ATTACHMENT_ROLES: Role[] = ["ACCOUNTS", "MANAGER", "SUPERUSER"]; +// ── PO attachments (feature-flagged) ────────────────────────────────────────── +// Roles that may attach to a PO (besides the PO's own submitter, who is always +// allowed) when NEXT_PUBLIC_CLOSED_PO_ATTACHMENTS_ENABLED=true. +const PO_ATTACHMENT_ROLES: Role[] = ["ACCOUNTS", "MANAGER", "SUPERUSER"]; + +// A PO in one of these terminal/voided states never accepts new attachments. +const NO_ATTACHMENT_STATUSES: POStatus[] = ["REJECTED", "CANCELLED"]; /** - * Feature-flagged: whether the current user may add attachments to a CLOSED PO. - * This is the remediation path for the upload bug where documents never persisted - * — closed POs that lost their files can be re-attached without reopening them. + * Feature-flagged: whether the current user may add attachments to a PO. * - * Allowed (only when the flag is on) for the PO's own submitter, or for - * Accounts / Manager / SuperUser. Off by default ⇒ closed POs stay immutable. + * When `NEXT_PUBLIC_CLOSED_PO_ATTACHMENTS_ENABLED` is on, the PO's own submitter + * — plus Accounts / Manager / SuperUser — may attach documents to a PO in **any + * state except `REJECTED` / `CANCELLED`** (a voided PO is never editable). This + * is the remediation path for the upload bug where documents never persisted, and + * the general "add a document after the fact" affordance. Off by default ⇒ no + * post-hoc attachment UI and closed POs stay immutable. */ -export function canAddClosedPoAttachment( +export function canAddPoAttachment( role: Role, + status: POStatus, opts: { isSubmitter: boolean } ): boolean { if (!CLOSED_PO_ATTACHMENTS_ENABLED) return false; - return opts.isSubmitter || CLOSED_PO_ATTACHMENT_ROLES.includes(role); + if (NO_ATTACHMENT_STATUSES.includes(status)) return false; + return opts.isSubmitter || PO_ATTACHMENT_ROLES.includes(role); } diff --git a/App/tests/integration/closed-po-attachments.test.ts b/App/tests/integration/po-attachment-permissions.test.ts similarity index 58% rename from App/tests/integration/closed-po-attachments.test.ts rename to App/tests/integration/po-attachment-permissions.test.ts index 017f5dc..0a541c8 100644 --- a/App/tests/integration/closed-po-attachments.test.ts +++ b/App/tests/integration/po-attachment-permissions.test.ts @@ -1,8 +1,9 @@ /** - * Integration test for the feature-flagged closed-PO attachment remediation - * (NEXT_PUBLIC_CLOSED_PO_ATTACHMENTS_ENABLED). With the flag ON, a CLOSED PO's own - * submitter — plus Accounts / Manager / SuperUser — may attach documents; everyone - * else is refused. (The flag-OFF case lives in po-document-upload.test.ts.) + * Integration test for the feature-flagged PO-attachment permission + * (NEXT_PUBLIC_CLOSED_PO_ATTACHMENTS_ENABLED). With the flag ON, a PO's own + * submitter — plus Accounts / Manager / SuperUser — may attach documents to a PO + * in **any state except REJECTED / CANCELLED**; everyone else, and any voided PO, + * is refused. (The flag-OFF behaviour lives in po-document-upload.test.ts.) */ import { vi, describe, it, expect, beforeAll, afterEach } from "vitest"; @@ -12,7 +13,7 @@ vi.mock("@/lib/storage", async (importOriginal) => { const actual = await importOriginal(); return { ...actual, uploadBuffer: vi.fn().mockResolvedValue(undefined) }; }); -// Flip ONLY the remediation flag on; everything else stays real. +// Flip ONLY the attachment flag on; everything else stays real. vi.mock("@/lib/feature-flags", async (importOriginal) => { const actual = await importOriginal(); return { ...actual, CLOSED_PO_ATTACHMENTS_ENABLED: true }; @@ -21,13 +22,16 @@ vi.mock("@/lib/feature-flags", async (importOriginal) => { import { auth } from "@/auth"; import { db } from "@/lib/db"; import { uploadBuffer } from "@/lib/storage"; -import type { Role } from "@prisma/client"; +import type { Role, POStatus } from "@prisma/client"; import { createPo } from "@/app/(portal)/po/new/actions"; import { uploadPoDocuments } from "@/app/actions/upload-po-documents"; import { makeSession, getSeedUser, getSeedVessel, getSeedAccount, makePoForm, deletePosByTitle } from "./helpers"; -const PREFIX = "INTTEST_CLOSEDPO_"; -let techId: string; // the PO's submitter +const PREFIX = "INTTEST_POATTACH_"; +const VOID_ERROR = "Attachments can't be added to a rejected or cancelled purchase order."; +const DENY_ERROR = "Adding attachments to this purchase order isn't allowed."; + +let techId: string; // the PO's submitter let vesselId: string; let accountId: string; const userIds: Record = {}; @@ -66,19 +70,21 @@ function pdf(name: string): File { return new File(["%PDF-1.4 hello"], name, { type: "application/pdf" }); } -// A CLOSED PO submitted by the TECHNICAL user. -async function makeClosedPo(title: string): Promise { +// A PO submitted by the TECHNICAL user, forced into `status`. +async function makePo(title: string, status: POStatus): Promise { as(techId, "TECHNICAL"); const result = await createPo(makePoForm({ title, vesselId, accountId, intent: "draft" })); expect(result).not.toHaveProperty("error"); const poId = (result as { id: string }).id; - await db.purchaseOrder.update({ where: { id: poId }, data: { status: "CLOSED" } }); + if (status !== "DRAFT") { + await db.purchaseOrder.update({ where: { id: poId }, data: { status } }); + } return poId; } -describe("closed-PO attachments (flag on)", () => { - it("lets the PO's own submitter attach to their closed PO", async () => { - const poId = await makeClosedPo(`${PREFIX}Submitter`); +describe("PO attachment permissions (flag on)", () => { + it("lets the PO's own submitter attach to their PO", async () => { + const poId = await makePo(`${PREFIX}Submitter`, "CLOSED"); as(techId, "TECHNICAL"); const err = await uploadPoDocuments(poId, [pdf("missing-invoice.pdf")]); @@ -91,7 +97,7 @@ describe("closed-PO attachments (flag on)", () => { ["MANAGER", "MANAGER"], ["SUPERUSER", "SUPERUSER"], ])("lets %s attach to a closed PO they did not submit", async (key, role) => { - const poId = await makeClosedPo(`${PREFIX}${key}`); + const poId = await makePo(`${PREFIX}${key}`, "CLOSED"); as(userIds[key], role); const err = await uploadPoDocuments(poId, [pdf("doc.pdf")]); @@ -99,29 +105,62 @@ describe("closed-PO attachments (flag on)", () => { expect(await db.pODocument.count({ where: { poId } })).toBe(1); }); + // The headline of this change: not just CLOSED — any live state. + it.each(["MGR_REVIEW", "MGR_APPROVED", "SENT_FOR_PAYMENT", "PAID_DELIVERED", "EDITS_REQUESTED"])( + "lets Manager attach to a PO in %s", + async (status) => { + const poId = await makePo(`${PREFIX}${status}`, status); + as(userIds.MANAGER, "MANAGER"); + + const err = await uploadPoDocuments(poId, [pdf("doc.pdf")]); + expect(err).toBeNull(); + expect(await db.pODocument.count({ where: { poId } })).toBe(1); + } + ); + + it.each(["REJECTED", "CANCELLED"])( + "refuses attachments to a %s PO, even for Manager", + async (status) => { + const poId = await makePo(`${PREFIX}${status}`, status); + as(userIds.MANAGER, "MANAGER"); + + const err = await uploadPoDocuments(poId, [pdf("doc.pdf")]); + expect(err).toEqual({ error: VOID_ERROR }); + expect(uploadBuffer).not.toHaveBeenCalled(); + expect(await db.pODocument.count({ where: { poId } })).toBe(0); + } + ); + + it("refuses a voided PO even for its own submitter", async () => { + const poId = await makePo(`${PREFIX}SubmitterRejected`, "REJECTED"); + as(techId, "TECHNICAL"); + + const err = await uploadPoDocuments(poId, [pdf("doc.pdf")]); + expect(err).toEqual({ error: VOID_ERROR }); + expect(await db.pODocument.count({ where: { poId } })).toBe(0); + }); + it("refuses a submitter-role user who is not this PO's submitter", async () => { - const poId = await makeClosedPo(`${PREFIX}OtherSubmitter`); + const poId = await makePo(`${PREFIX}OtherSubmitter`, "MGR_APPROVED"); as(userIds.MANNING, "MANNING"); // a submitter role, but not the PO's submitter const err = await uploadPoDocuments(poId, [pdf("doc.pdf")]); - expect(err).toEqual({ error: "Adding attachments to a closed purchase order isn't allowed." }); + expect(err).toEqual({ error: DENY_ERROR }); expect(uploadBuffer).not.toHaveBeenCalled(); expect(await db.pODocument.count({ where: { poId } })).toBe(0); }); it("refuses a role outside the allow-list (auditor)", async () => { - const poId = await makeClosedPo(`${PREFIX}Auditor`); + const poId = await makePo(`${PREFIX}Auditor`, "CLOSED"); as(userIds.AUDITOR, "AUDITOR"); const err = await uploadPoDocuments(poId, [pdf("doc.pdf")]); - expect(err).toEqual({ error: "Adding attachments to a closed purchase order isn't allowed." }); + expect(err).toEqual({ error: DENY_ERROR }); expect(await db.pODocument.count({ where: { poId } })).toBe(0); }); - it("still allows uploads to a non-closed PO (normal flow unaffected)", async () => { - as(techId, "TECHNICAL"); - const result = await createPo(makePoForm({ title: `${PREFIX}Draft`, vesselId, accountId, intent: "draft" })); - const poId = (result as { id: string }).id; // stays DRAFT + it("still allows the normal create flow (DRAFT submitter)", async () => { + const poId = await makePo(`${PREFIX}Draft`, "DRAFT"); as(techId, "TECHNICAL"); const err = await uploadPoDocuments(poId, [pdf("draft-doc.pdf")]);