Merge pull request 'feat(po): allow attachments in any PO state except rejected/cancelled' (#146) from claude/po-attachments-any-state into master

Reviewed-on: #146

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
 

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.
 ```
 

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) {

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<File[]>([]);
   const [busy, setBusy] = useState(false);
@@ -36,7 +36,7 @@ export function ClosedPoAttachmentUploader({ poId }: { poId: string }) {
     <div className="mt-5 border-t border-neutral-100 pt-4">
       <p className="text-xs font-semibold uppercase tracking-wide text-neutral-500">Add attachments</p>
       <p className="mt-0.5 text-xs text-neutral-400">
-        This purchase order is closed. Attach any documents that are missing.
+        Attach any documents that are missing from this purchase order.
       </p>
       <div className="mt-3">
         <FileUploader files={files} onChange={setFiles} disabled={busy} />

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) && (
         <div className="rounded-lg border border-neutral-200 bg-white p-6">
           <h3 className="text-sm font-semibold text-neutral-900 mb-4">Attachments</h3>
           {attachmentGroups.length === 0 && (
@@ -554,7 +553,7 @@ export async function PoDetail({ po, currentUserId, currentRole, readOnly = fals
               </div>
             ))}
           </div>
-          {canAddClosedAttachment && <ClosedPoAttachmentUploader poId={po.id} />}
+          {canAddAttachment && <PoAttachmentUploader poId={po.id} />}
         </div>
       )}
 

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 =

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);
 }

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<typeof import("@/lib/storage")>();
   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<typeof import("@/lib/feature-flags")>();
   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<string, string> = {};
@@ -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<string> {
+// A PO submitted by the TECHNICAL user, forced into `status`.
+async function makePo(title: string, status: POStatus): Promise<string> {
   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<POStatus>(["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<POStatus>(["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")]);