diff --git a/.env.example b/.env.example index a019c077..199f5e01 100644 --- a/.env.example +++ b/.env.example @@ -96,8 +96,8 @@ WORKFLOW_PROVIDER_CONTROL_TOKEN= CLOCKWORK_CRM_ENABLED=false CRM_PROVIDER_BASE_URL= CRM_PROVIDER_TOKEN= -# HubSpot handoff and pipeline (docs/operations/hubspot-handoff.md): a private -# app token with read-only CRM scopes. Unset leaves HubSpot unavailable; even +# HubSpot handoff and pipeline (docs/operations/hubspot-handoff.md): a HubSpot +# service key (or legacy private-app token) with read-only CRM scopes. Unset leaves HubSpot unavailable; even # when set, nothing is read until an administrator approves the connection. HUBSPOT_ACCESS_TOKEN= ACCOUNTING_PROVIDER_BASE_URL= diff --git a/apps/web/src/features/internal-ops/account-documents/actions.ts b/apps/web/src/features/internal-ops/account-documents/actions.ts new file mode 100644 index 00000000..666306c2 --- /dev/null +++ b/apps/web/src/features/internal-ops/account-documents/actions.ts @@ -0,0 +1,61 @@ +"use server"; + +import { z } from "zod"; + +import { + AccountDocumentTargetSchema, + type AccountDocumentRecord, + type RemovedAccountDocumentRecord, +} from "@clockwork/contracts"; + +import { SessionExpiredError } from "@/src/auth/session"; + +import { attempt, type ActionResult } from "../contracts/action-result"; +import { contractActor } from "../contracts/server"; +import { accountDocumentRepository, accountDocumentStaff } from "./server"; + +async function run(operation: () => Promise): Promise> { + return attempt(async () => { + try { + return await operation(); + } catch (error) { + if (error instanceof SessionExpiredError) + throw new Error("SESSION_EXPIRED"); + throw error; + } + }); +} + +const RequestSchema = z + .object({ target: AccountDocumentTargetSchema }) + .passthrough(); + +/** + * Takes a document off its record, with the reason. Who may attach may + * remove: the record's read permission plus `contract:write`, checked on a + * real staff session with MFA. The file and its history are kept. + */ +export async function removeAccountDocument(raw: unknown) { + return run(async (): Promise => { + const { target, ...input } = RequestSchema.parse(raw); + const session = await accountDocumentStaff(target, true); + return accountDocumentRepository().remove( + target, + input, + contractActor(session), + ); + }); +} + +/** Puts a removed document back on its record. */ +export async function restoreAccountDocument(raw: unknown) { + return run(async (): Promise => { + const { target, ...input } = RequestSchema.parse(raw); + const session = await accountDocumentStaff(target, true); + return accountDocumentRepository().restore( + target, + input, + contractActor(session), + ); + }); +} diff --git a/apps/web/src/features/internal-ops/account-documents/documents.module.css b/apps/web/src/features/internal-ops/account-documents/documents.module.css index d012638d..9be8a013 100644 --- a/apps/web/src/features/internal-ops/account-documents/documents.module.css +++ b/apps/web/src/features/internal-ops/account-documents/documents.module.css @@ -27,6 +27,56 @@ align-items: center; gap: var(--cw-space-2); } +.notice:focus { + outline: none; +} +.notice:empty { + display: none; +} +/* The confirmation sits under the row it removes, across its full width. */ +.removal { + display: grid; + grid-column: 1 / -1; + gap: var(--cw-space-3); + margin-top: var(--cw-space-2); + padding: var(--cw-space-3); + border: 1px solid var(--cw-danger-border, var(--cw-border)); + border-radius: var(--cw-radius-sm); + background: var(--cw-danger-soft, var(--cw-surface)); +} +.removalText { + margin: 0; + color: var(--cw-ink); +} +.removalActions { + display: flex; + flex-wrap: wrap; + gap: var(--cw-space-2); +} +.history { + margin-top: var(--cw-space-4); + border-top: 1px solid var(--cw-border); + padding-top: var(--cw-space-3); +} +.history > summary { + cursor: pointer; + color: var(--cw-muted); + font-weight: 600; +} +.historyList { + display: grid; + gap: var(--cw-space-3); + margin: var(--cw-space-3) 0 0; + padding: 0; + list-style: none; +} +.historyList > li { + display: flex; + flex-wrap: wrap; + align-items: center; + justify-content: space-between; + gap: var(--cw-space-3); +} @media (max-width: 48rem) { .fileRow { display: grid; diff --git a/apps/web/src/features/internal-ops/account-documents/documents.test.tsx b/apps/web/src/features/internal-ops/account-documents/documents.test.tsx index 3e7714c0..38c304dd 100644 --- a/apps/web/src/features/internal-ops/account-documents/documents.test.tsx +++ b/apps/web/src/features/internal-ops/account-documents/documents.test.tsx @@ -1,10 +1,19 @@ import { renderToString } from "react-dom/server"; import { fireEvent, render, screen, waitFor } from "@testing-library/react"; import { beforeEach, expect, it, vi } from "vitest"; -const mocks = vi.hoisted(() => ({ refresh: vi.fn(), fetch: vi.fn() })); +const mocks = vi.hoisted(() => ({ + refresh: vi.fn(), + fetch: vi.fn(), + remove: vi.fn(), + restore: vi.fn(), +})); vi.mock("next/navigation", () => ({ useRouter: () => ({ refresh: mocks.refresh }), })); +vi.mock("./actions", () => ({ + removeAccountDocument: mocks.remove, + restoreAccountDocument: mocks.restore, +})); import { AccountDocuments } from "./documents"; const target = { type: "partner" as const, @@ -19,6 +28,14 @@ const file = { sizeBytes: 1024, uploadedByName: "Iris", createdAt: "2026-10-20T12:00:00Z", + version: 1, +}; +const removedFile = { + ...file, + version: 2, + removedAt: "2026-10-21T12:00:00Z", + removedByName: "Iris", + removalReason: "Attached to the wrong record", }; const choose = (name = "Signed MNDA.pdf", type = "application/pdf") => fireEvent.change(screen.getByLabelText("Choose PDF"), { @@ -100,3 +117,122 @@ it("keeps the file picker disabled until its client handler is ready", () => { container.innerHTML = markup; expect(container.querySelector('input[type="file"]')).toBeDisabled(); }); + +it("removes a document only with a reason, then offers undo", async () => { + mocks.remove.mockResolvedValue({ ok: true, value: removedFile }); + mocks.restore.mockResolvedValue({ ok: true, value: { ...file, version: 3 } }); + render(); + fireEvent.click(screen.getByRole("button", { name: "Remove External MNDA" })); + const confirm = screen.getByRole("button", { name: "Remove document" }); + expect(confirm).toBeDisabled(); + fireEvent.change(screen.getByLabelText("Why remove it?"), { + target: { value: "Attached to the wrong record" }, + }); + fireEvent.click(confirm); + await screen.findByText("“External MNDA” was removed from this record."); + expect(mocks.remove).toHaveBeenCalledWith({ + target, + id: file.id, + expectedVersion: 1, + reason: "Attached to the wrong record", + }); + expect( + screen.queryByRole("link", { name: "Download External MNDA" }), + ).not.toBeInTheDocument(); + expect(screen.getByText("1 removed document")).toBeInTheDocument(); + fireEvent.click(screen.getByRole("button", { name: "Undo" })); + await screen.findByText("“External MNDA” is back on this record."); + expect(mocks.restore).toHaveBeenCalledWith({ + target, + id: file.id, + expectedVersion: 2, + }); + expect( + screen.getByRole("link", { name: "Download External MNDA" }), + ).toBeInTheDocument(); +}); + +it("lists removed documents with who and why, and no way to open them", () => { + render( + , + ); + expect( + screen.getByText(/Removed by Iris: Attached to the wrong record/), + ).toBeInTheDocument(); + expect(screen.queryByRole("link")).not.toBeInTheDocument(); + expect( + screen.queryByRole("button", { name: "Restore External MNDA" }), + ).not.toBeInTheDocument(); + expect( + screen.queryByRole("button", { name: /Remove/ }), + ).not.toBeInTheDocument(); +}); + +it("explains a removal someone else already changed", async () => { + mocks.remove.mockResolvedValue({ + ok: false, + code: "ATTACHMENT_VERSION_CONFLICT", + }); + render(); + fireEvent.click(screen.getByRole("button", { name: "Remove External MNDA" })); + fireEvent.change(screen.getByLabelText("Why remove it?"), { + target: { value: "Replaced" }, + }); + fireEvent.click(screen.getByRole("button", { name: "Remove document" })); + expect( + await screen.findByText(/Someone else changed this document/), + ).toBeInTheDocument(); + expect( + screen.getByRole("link", { name: "Download External MNDA" }), + ).toBeInTheDocument(); +}); + +it("keeps the newest version when the server refresh lags behind an undo", async () => { + mocks.remove.mockResolvedValueOnce({ ok: true, value: removedFile }); + mocks.restore.mockResolvedValue({ ok: true, value: { ...file, version: 3 } }); + const { rerender } = render( + , + ); + fireEvent.click(screen.getByRole("button", { name: "Remove External MNDA" })); + fireEvent.change(screen.getByLabelText("Why remove it?"), { + target: { value: "Wrong record" }, + }); + fireEvent.click(screen.getByRole("button", { name: "Remove document" })); + fireEvent.click(await screen.findByRole("button", { name: "Undo" })); + await screen.findByText("“External MNDA” is back on this record."); + // A refresh that only saw the removal arrives after the undo. + rerender( + , + ); + expect( + screen.getByRole("link", { name: "Download External MNDA" }), + ).toBeInTheDocument(); + mocks.remove.mockResolvedValueOnce({ + ok: true, + value: { ...removedFile, version: 4 }, + }); + fireEvent.click(screen.getByRole("button", { name: "Remove External MNDA" })); + fireEvent.change(screen.getByLabelText("Why remove it?"), { + target: { value: "Wrong record again" }, + }); + fireEvent.click(screen.getByRole("button", { name: "Remove document" })); + await waitFor(() => + expect(mocks.remove).toHaveBeenLastCalledWith( + expect.objectContaining({ expectedVersion: 3 }), + ), + ); + // The undo offered for one removal never outlives the next action. + expect( + screen.queryByText("“External MNDA” is back on this record."), + ).toBeNull(); +}); diff --git a/apps/web/src/features/internal-ops/account-documents/documents.tsx b/apps/web/src/features/internal-ops/account-documents/documents.tsx index 245f4d19..4c4d9241 100644 --- a/apps/web/src/features/internal-ops/account-documents/documents.tsx +++ b/apps/web/src/features/internal-ops/account-documents/documents.tsx @@ -8,6 +8,7 @@ import { type AccountDocumentKind, type AccountDocumentRecord, type AccountDocumentTarget, + type RemovedAccountDocumentRecord, } from "@clockwork/contracts"; import { Button, @@ -29,9 +30,14 @@ import { import { localFileProblem, postDocumentForm } from "../contracts/upload-client"; import { LocalTimestamp } from "../local-timestamp"; import { SessionExpiredReload } from "../session-expiry"; +import { removeAccountDocument, restoreAccountDocument } from "./actions"; import styles from "../contracts/contracts.module.css"; import attachmentStyles from "./documents.module.css"; +type Entry = + | { removed: false; doc: AccountDocumentRecord } + | { removed: true; doc: RemovedAccountDocumentRecord }; + const noSubscription = () => () => {}; const onClient = () => true; const onServer = () => false; @@ -48,16 +54,20 @@ const errors: Record = { ATTACHMENT_NOT_FOUND: "operations.accountDocuments.missing", CONTRACT_FORBIDDEN: "operations.accountDocuments.forbidden", INVALID_INPUT: "operations.accountDocuments.invalid", + ATTACHMENT_VERSION_CONFLICT: "operations.accountDocuments.changed", }; export function AccountDocuments({ target, files, + removed = [], canWrite, demo = false, }: { target: AccountDocumentTarget; files: readonly AccountDocumentRecord[]; + /** Documents taken off this record, kept as history. */ + removed?: readonly RemovedAccountDocumentRecord[]; canWrite: boolean; demo?: boolean; }) { @@ -76,17 +86,97 @@ export function AccountDocuments({ const [busy, setBusy] = useState(false); const [error, setError] = useState(null); const [notice, setNotice] = useState(null); - // Keep the new row visible immediately, even while the server refresh runs. - const [added, setAdded] = useState([]); - const visible = [ - ...added.filter((row) => !files.some((f) => f.id === row.id)), - ...files, - ]; + // What this page just did, shown at once ahead of the server refresh. Each + // entry gives way as soon as the server's copy is at least as new, so a + // change made elsewhere is never hidden by an older local one. + const [local, setLocal] = useState([]); + const remember = (entry: Entry) => + setLocal((entries) => [ + entry, + ...entries.filter((item) => item.doc.id !== entry.doc.id), + ]); + const [removing, setRemoving] = useState<{ + id: string; + reason: string; + } | null>(null); + const [undo, setUndo] = useState(null); + const noticeRef = useRef(null); + const merged = new Map(); + const consider = (entry: Entry) => { + const current = merged.get(entry.doc.id); + if (!current || current.doc.version < entry.doc.version) + merged.set(entry.doc.id, entry); + }; + for (const doc of files) consider({ removed: false, doc }); + for (const doc of removed) consider({ removed: true, doc }); + for (const entry of local) consider(entry); + const entries = [...merged.values()]; + const visible = entries + .flatMap((entry) => (entry.removed ? [] : [entry.doc])) + .sort((a, b) => b.createdAt.localeCompare(a.createdAt)); + const history = entries + .flatMap((entry) => (entry.removed ? [entry.doc] : [])) + .sort((a, b) => b.removedAt.localeCompare(a.removedAt)); + const announce = () => + requestAnimationFrame(() => noticeRef.current?.focus()); + async function remove(document: AccountDocumentRecord, reason: string) { + setBusy(true); + setError(null); + setNotice(null); + try { + const result = await removeAccountDocument({ + target, + id: document.id, + expectedVersion: document.version, + reason, + }).catch(() => ({ ok: false as const, code: "UNEXPECTED" })); + if (!result.ok) { + setError(result.code); + return; + } + setRemoving(null); + remember({ removed: true, doc: result.value }); + setUndo(result.value); + setNotice( + t("operations.accountDocuments.removed", { name: document.title }), + ); + announce(); + router.refresh(); + } finally { + setBusy(false); + } + } + async function restore(document: RemovedAccountDocumentRecord) { + setBusy(true); + setError(null); + setNotice(null); + try { + const result = await restoreAccountDocument({ + target, + id: document.id, + expectedVersion: document.version, + }).catch(() => ({ ok: false as const, code: "UNEXPECTED" })); + if (!result.ok) { + setError(result.code); + return; + } + setUndo(null); + remember({ removed: false, doc: result.value }); + setNotice( + t("operations.accountDocuments.restored", { name: document.title }), + ); + announce(); + router.refresh(); + } finally { + setBusy(false); + } + } const base = `/internal/record-documents/${target.type}/${target.id}`; function changed() { requestId.current = null; setError(null); setNotice(null); + setUndo(null); } async function upload() { if (!file || uploading.current) return; @@ -119,10 +209,8 @@ export function AccountDocuments({ setError(result.code); return; } - setAdded((rows) => [ - result.value, - ...rows.filter((row) => row.id !== result.value.id), - ]); + remember({ removed: false, doc: result.value }); + setUndo(null); setNotice( t("operations.accountDocuments.uploaded", { name: result.value.title }), ); @@ -159,9 +247,30 @@ export function AccountDocuments({ : {})} /> ) : null} - {notice ? ( - - ) : null} +
+ {notice ? ( + void restore(undo)} + > + {t("operations.accountDocuments.undo")} + + ), + } + : {})} + /> + ) : null} +
{demo ? ( {t("operations.accountDocuments.download")} + {canWrite && !demo ? ( + + ) : null} + {removing?.id === document.id ? ( +
{ + event.preventDefault(); + if (removing.reason.trim()) + void remove(document, removing.reason.trim()); + }} + > +

+ {t("operations.accountDocuments.removeConfirm", { + name: document.title, + })} +

+ + setRemoving({ + id: document.id, + reason: event.currentTarget.value, + }) + } + /> + + + + +
+ ) : null} ))} )} + {history.length && !demo ? ( +
+ + {t("operations.accountDocuments.removedTitle", { + count: history.length, + })} + +

+ {t("operations.accountDocuments.removedBody")} +

+
    + {history.map((document) => ( +
  • + + {document.title} + + {document.fileName} ·{" "} + {formatFileSize(document.sizeBytes, locale)} + + + {t("operations.accountDocuments.removedBy", { + name: document.removedByName, + reason: document.removalReason, + })}{" "} + ·{" "} + + + + {canWrite ? ( + + ) : null} +
    • + ))} +
+
+ ) : null} {canWrite && !demo ? (
); diff --git a/apps/web/src/features/internal-ops/hubspot/actions.ts b/apps/web/src/features/internal-ops/hubspot/actions.ts index b4c7ae48..4f9f03bd 100644 --- a/apps/web/src/features/internal-ops/hubspot/actions.ts +++ b/apps/web/src/features/internal-ops/hubspot/actions.ts @@ -192,7 +192,11 @@ export async function processHubSpotDocument(raw: unknown) { const record = await repository.import(partnerId, importId); const item = record.items.find((candidate) => candidate.id === itemId); if (!item) throw new Error("HUBSPOT_ITEM_NOT_FOUND"); - if (!["pending", "failed"].includes(item.status)) + // A duplicate whose identical copy was since removed can be attached. + if ( + !["pending", "failed"].includes(item.status) && + !(item.status === "duplicate" && item.existingAttachment === null) + ) throw new Error("HUBSPOT_ITEM_RESOLVED"); // A previous attempt attached it but its answer was lost. const already = await repository.attachmentOnPartner(partnerId, item.id); diff --git a/apps/web/src/features/internal-ops/hubspot/hubspot.test.tsx b/apps/web/src/features/internal-ops/hubspot/hubspot.test.tsx index 4ecf6ed7..fb7cf4d1 100644 --- a/apps/web/src/features/internal-ops/hubspot/hubspot.test.tsx +++ b/apps/web/src/features/internal-ops/hubspot/hubspot.test.tsx @@ -608,6 +608,7 @@ describe("HubSpotReceipt", () => { errorCode: status === "failed" ? "HUBSPOT_DOCUMENT_UNAVAILABLE" : null, attachmentId: null, existingAttachment: null, + attachmentRemoved: false, excludedReason: null, attempts: status === "failed" ? 1 : 0, updatedAt: now.toISOString(), diff --git a/apps/web/src/features/internal-ops/hubspot/receipt.tsx b/apps/web/src/features/internal-ops/hubspot/receipt.tsx index 38776e33..8d6ba36c 100644 --- a/apps/web/src/features/internal-ops/hubspot/receipt.tsx +++ b/apps/web/src/features/internal-ops/hubspot/receipt.tsx @@ -406,6 +406,16 @@ export function HubSpotReceipt({ })} ) : null} + {item.status === "duplicate" && !item.existingAttachment ? ( + + {t("operations.hubspot.receipt.duplicateRemoved")} + + ) : null} + {item.attachmentRemoved ? ( + + {t("operations.hubspot.receipt.attachmentRemoved")} + + ) : null} {item.status === "excluded" && item.excludedReason ? ( {t("operations.hubspot.receipt.excludedBecause", { @@ -461,7 +471,8 @@ export function HubSpotReceipt({ ) : (
- {item.status === "duplicate" ? ( + {item.status === "duplicate" && + item.existingAttachment ? ( diff --git a/apps/web/src/features/internal-ops/hubspot/setup-guide.tsx b/apps/web/src/features/internal-ops/hubspot/setup-guide.tsx index 0a838374..36e57d8a 100644 --- a/apps/web/src/features/internal-ops/hubspot/setup-guide.tsx +++ b/apps/web/src/features/internal-ops/hubspot/setup-guide.tsx @@ -7,8 +7,11 @@ import type { MessageId, Translator } from "@/src/i18n"; import { hubspotPickerPath, pipelinePath } from "./model"; import styles from "./hubspot.module.css"; -/** The read-only scopes the HubSpot private app needs, as HubSpot names them. */ -export const hubspotPrivateAppScopes = [ +/** + * The read-only scopes the HubSpot service key (or a legacy private app) + * needs, as HubSpot names them. + */ +export const hubspotReadScopes = [ "crm.objects.companies.read", "crm.objects.deals.read", "crm.objects.contacts.read", @@ -111,7 +114,7 @@ export function HubSpotSetupGuide({ {t("operations.hubspot.setup.scopes")}

    - {hubspotPrivateAppScopes.map((scope) => ( + {hubspotReadScopes.map((scope) => (
  • {scope}
    • diff --git a/apps/web/src/i18n/messages/operations-account-documents.ts b/apps/web/src/i18n/messages/operations-account-documents.ts index d3a84b34..def030c5 100644 --- a/apps/web/src/i18n/messages/operations-account-documents.ts +++ b/apps/web/src/i18n/messages/operations-account-documents.ts @@ -35,7 +35,7 @@ export const accountDocumentMessages = defineStaffMessages({ en: "Document attachments are available on saved records in the live staff workspace.", }, "operations.accountDocuments.duplicate": { - en: "This PDF is already attached to this record. You can view or download it below.", + en: "This PDF is already attached to this record. You can view or download it below; to replace it, remove that copy first.", }, "operations.accountDocuments.retry": { en: "The previous upload used different details. Reload the page to check whether it was saved, then try again.", @@ -46,7 +46,44 @@ export const accountDocumentMessages = defineStaffMessages({ "operations.accountDocuments.invalid": { en: "Enter a document title and check the signing date.", }, + "operations.accountDocuments.remove": { en: "Remove" }, + "operations.accountDocuments.removeNamed": { en: "Remove {name}" }, + "operations.accountDocuments.removeConfirm": { + en: "Remove “{name}” from this record? It will no longer be listed or openable here. Commerce keeps the file and this change in the record’s history; you can restore it unless the same PDF has been attached again.", + }, + "operations.accountDocuments.removeReason": { en: "Why remove it?" }, + "operations.accountDocuments.removeReasonHelp": { + en: "For example: attached to the wrong record, or replaced by a corrected copy. Kept with the history.", + }, + "operations.accountDocuments.removeAction": { en: "Remove document" }, + "operations.accountDocuments.removeCancel": { en: "Keep it" }, + "operations.accountDocuments.removing": { en: "Removing…" }, + "operations.accountDocuments.removed": { + en: "“{name}” was removed from this record.", + }, + "operations.accountDocuments.restored": { + en: "“{name}” is back on this record.", + }, + "operations.accountDocuments.undo": { en: "Undo" }, + "operations.accountDocuments.restore": { en: "Restore" }, + "operations.accountDocuments.restoreNamed": { en: "Restore {name}" }, + "operations.accountDocuments.removedTitle": { + count: "count", + en: { + one: "{count} removed document", + other: "{count} removed documents", + }, + }, + "operations.accountDocuments.removedBody": { + en: "Removed documents can’t be opened from this record. They stay in its history for audit, and the same PDF can be attached again.", + }, + "operations.accountDocuments.removedBy": { + en: "Removed by {name}: {reason}", + }, + "operations.accountDocuments.changed": { + en: "Someone else changed this document. Reload the page to see its current state.", + }, "operations.accountDocuments.forbidden": { - en: "You do not have permission to attach documents to this record.", + en: "You do not have permission to change documents on this record.", }, }); diff --git a/apps/web/src/i18n/messages/operations-hubspot.ts b/apps/web/src/i18n/messages/operations-hubspot.ts index 45fd7bc9..bc478abd 100644 --- a/apps/web/src/i18n/messages/operations-hubspot.ts +++ b/apps/web/src/i18n/messages/operations-hubspot.ts @@ -531,8 +531,14 @@ export const hubspotMessages = defineStaffMessages({ }, "operations.hubspot.receipt.excludeConfirm": { en: "Exclude" }, "operations.hubspot.receipt.itemAnnounce": { en: "{title}: {status}" }, + "operations.hubspot.receipt.duplicateRemoved": { + en: "The identical copy was removed from the partner since. Attach this one, or exclude it.", + }, + "operations.hubspot.receipt.attachmentRemoved": { + en: "Removed from the partner’s documents afterwards.", + }, "operations.hubspot.receipt.appendOnly": { - en: "Attachments cannot be removed or moved here. If one is wrong, ask support to correct it.", + en: "To take an attached document off the partner, remove it under Documents; the receipt and the history keep the record of it.", }, "operations.hubspot.continue.title": { en: "Continue in Commerce" }, @@ -763,6 +769,9 @@ export const hubspotMessages = defineStaffMessages({ "operations.hubspot.state.noPipelinesBody": { en: "An administrator chooses which deal pipelines staff may see on the HubSpot connection page.", }, + "operations.hubspot.error.HUBSPOT_DOCUMENT_REMOVED": { + en: "The identical copy was removed from the partner meanwhile. Attach this document instead, or exclude it.", + }, "operations.hubspot.error.HUBSPOT_SIMILAR_PARTNER": { en: "A partner that looks like this company was added meanwhile. It is now listed above; inspect it, then create the partner.", }, @@ -787,14 +796,14 @@ export const hubspotMessages = defineStaffMessages({ "operations.hubspot.setup.current": { en: "Next" }, "operations.hubspot.setup.upcoming": { en: "Later" }, "operations.hubspot.setup.tokenTitle": { - en: "Create a read-only HubSpot private app", + en: "Create a read-only HubSpot service key", }, "operations.hubspot.setup.tokenBody": { - en: "A HubSpot super admin opens Settings → Integrations → Private apps → Create a private app, names it Fil One Commerce, grants only the read scopes below, creates it and copies its access token.", + en: "A HubSpot super admin, or a user with Developer tools access, opens Development → Keys → Service keys → Create service key, names it Fil One Commerce, adds only the read scopes below and copies the key. A legacy private app token with the same scopes works the same way.", }, "operations.hubspot.setup.scopes": { en: "Read scopes to grant:" }, "operations.hubspot.setup.filesScope": { - en: "To bring PDFs across from HubSpot notes, also grant files. Without it, documents show as unavailable and everything else works. Grant no write scopes.", + en: "To bring PDFs across from HubSpot notes, also grant files and files.ui_hidden.read: note attachments are kept in HubSpot’s hidden file folder. Without them, documents show as unavailable and everything else works. Grant no write scopes, and rotate the key in HubSpot about every six months.", }, "operations.hubspot.setup.deployTitle": { en: "Add the token to the deployment", @@ -803,7 +812,7 @@ export const hubspotMessages = defineStaffMessages({ en: "In GitHub, fil-one/clockwork → Settings → Environments, add a secret named HUBSPOT_ACCESS_TOKEN to production (and staging, to try it there first). Then open Actions → Deploy → Run workflow on main. The next deploy gives the app the token.", }, "operations.hubspot.setup.deployCheck": { - en: "This deployment has a token, but HubSpot did not accept it. Check that the token was copied whole and the app was not deleted, then deploy again.", + en: "This deployment has a key, but HubSpot did not accept it. Check that it was copied whole and has not been rotated or revoked, then deploy again.", }, "operations.hubspot.setup.approveTitle": { en: "Approve the connection here", diff --git a/docs/operations/hubspot-handoff.md b/docs/operations/hubspot-handoff.md index 7380d371..142da30c 100644 --- a/docs/operations/hubspot-handoff.md +++ b/docs/operations/hubspot-handoff.md @@ -111,7 +111,9 @@ A document that fails leaves the partner usable with the receipt saying retries only that item; a lost answer is recognised by the attachment ID and never attaches twice. **Exclude this item and finish** needs a reason and records an audited amendment; the receipt then says **Completed with 1 item -excluded**. Attachments are append-only; an incorrect one goes to support. +excluded**. An attachment brought in by mistake is removed under **Documents** +(see [partner records](partner-records.md#removing-a-document)); the receipt +then notes the removal. ## Pipeline @@ -143,13 +145,18 @@ The connection page (**Owner console → Integrations → HubSpot connection**, `/internal/owner/hubspot`) shows this checklist with where the environment stands, and administrators see **Set up HubSpot** on Partners until it is done. -1. **Create a read-only private app in HubSpot.** A HubSpot super admin opens - Settings → Integrations → Private apps → Create a private app, names it Fil - One Commerce and grants only: `crm.objects.companies.read`, - `crm.objects.deals.read`, `crm.objects.contacts.read`, - `crm.objects.owners.read`, `crm.schemas.companies.read`, - `crm.schemas.deals.read`, `crm.schemas.contacts.read`, plus `files` to bring - PDFs across from notes. No write scopes. Copy the access token. +1. **Create a read-only service key in HubSpot.** A HubSpot super admin (or a + user with Developer tools access) opens Development → Keys → Service keys → + Create service key, names it Fil One Commerce and adds only: + `crm.objects.companies.read`, `crm.objects.deals.read`, + `crm.objects.contacts.read`, `crm.objects.owners.read`, + `crm.schemas.companies.read`, `crm.schemas.deals.read`, + `crm.schemas.contacts.read`, plus `files` and `files.ui_hidden.read` to bring + PDFs across from notes (note attachments live in HubSpot's hidden file + folder). No write scopes. Copy the key. A legacy private-app token with the + same scopes works identically: both are sent as a bearer token, and Commerce + identifies the portal from the key itself. Rotate the key in HubSpot about + every six months, then update the secret and redeploy. 2. **Add the token to the deployment.** In GitHub, fil-one/clockwork → Settings → Environments, add the secret `HUBSPOT_ACCESS_TOKEN` to `production` (and `staging` to try it there first). Then run **Actions → Deploy → Run diff --git a/docs/operations/partner-records.md b/docs/operations/partner-records.md index 153a663f..3da7e557 100644 --- a/docs/operations/partner-records.md +++ b/docs/operations/partner-records.md @@ -95,3 +95,23 @@ Uploads and downloads append `account.document_attached` and `account.document_downloaded` audit/outbox events. Retry IDs reuse the original result only for identical content, metadata, target and uploader; an identical PDF already on the same record is reported as a duplicate. + +### Removing a document + +A document attached by mistake, to the wrong record, or superseded by a +corrected copy can be removed by anyone who may attach documents to that record: +**Remove** on its row, give the reason, then **Remove document**. The notice +offers **Undo**, and **Removed documents** at the foot of the section lists each +removal with who made it and why, with **Restore**. + +Removal is an audited state, not an erasure (migration 001484). The row, its +original details and the retained bytes stay; the service role may change only +the removal columns, and a trigger refuses any other change. A removed document +is no longer listed, viewed or downloaded from the record, and no longer counts +as a duplicate, so the corrected or same PDF can be attached again. Restoring is +refused while an identical PDF is on the record. Each change advances the +document's version, so a stale removal or restore is refused with a reload +message. Removals and restores append `account.document_removed` and +`account.document_restored` events with the reason. Permanently purging a file's +bytes is not offered here; it remains a support operation under the retention +rules. diff --git a/packages/contracts/src/account-documents.ts b/packages/contracts/src/account-documents.ts index 1a6d934d..443f72d4 100644 --- a/packages/contracts/src/account-documents.ts +++ b/packages/contracts/src/account-documents.ts @@ -41,4 +41,33 @@ export interface AccountDocumentRecord extends AccountDocumentInput { sizeBytes: number; uploadedByName: string; createdAt: string; + /** Advances with each removal or restore; a stale change is refused. */ + version: number; } + +/** A document taken off its record. It stays in the history, unopenable. */ +export interface RemovedAccountDocumentRecord extends AccountDocumentRecord { + removedAt: string; + removedByName: string; + removalReason: string; +} + +/** + * Removing a document attached by mistake or superseded by a corrected copy. + * The reason is required and kept; the row and its bytes are retained. + */ +export const AccountDocumentRemovalSchema = z + .object({ + id: z.uuid(), + expectedVersion: z.int().min(1), + reason: line(500), + }) + .strict(); +export type AccountDocumentRemoval = z.infer< + typeof AccountDocumentRemovalSchema +>; + +/** Putting a removed document back on its record. */ +export const AccountDocumentRestoreSchema = z + .object({ id: z.uuid(), expectedVersion: z.int().min(1) }) + .strict(); diff --git a/packages/contracts/src/hubspot.ts b/packages/contracts/src/hubspot.ts index ed9e1d06..66e19c50 100644 --- a/packages/contracts/src/hubspot.ts +++ b/packages/contracts/src/hubspot.ts @@ -932,6 +932,8 @@ export interface HubSpotImportItem { attachmentId: string | null; /** The identical PDF already on the record, for a duplicate. */ existingAttachment: { id: string; title: string } | null; + /** The attached document was removed from the partner afterwards. */ + attachmentRemoved: boolean; excludedReason: string | null; attempts: number; updatedAt: string; @@ -1010,6 +1012,7 @@ export const hubspotErrorCodes = [ "HUBSPOT_CONNECTION_VERSION_CONFLICT", "HUBSPOT_NO_PIPELINES", "HUBSPOT_SIMILAR_PARTNER", + "HUBSPOT_DOCUMENT_REMOVED", ] as const; export type HubSpotErrorCode = (typeof hubspotErrorCodes)[number]; diff --git a/packages/db/src/repositories/account-documents.integration.test.ts b/packages/db/src/repositories/account-documents.integration.test.ts index 17fe1389..ce99e653 100644 --- a/packages/db/src/repositories/account-documents.integration.test.ts +++ b/packages/db/src/repositories/account-documents.integration.test.ts @@ -170,4 +170,79 @@ describe("staff account attachments", () => { }); expect((await repo.read(account, file.id, actor)).file.id).toBe(file.id); }); + + it("removes a mistaken attachment with its reason, keeps the evidence, and allows a corrected copy", async () => { + const pdf = upload(); + const file = await repo.add(account, input(), pdf, actor); + await expect( + repo.remove( + account, + { id: file.id, expectedVersion: 2, reason: "Wrong record" }, + actor, + ), + ).rejects.toThrow("ATTACHMENT_VERSION_CONFLICT"); + const removal = { + id: file.id, + expectedVersion: file.version, + reason: "Attached to the wrong record", + }; + const removed = await repo.remove(account, removal, actor); + expect(removed).toMatchObject({ + id: file.id, + version: 2, + removedByName: actor.display, + removalReason: "Attached to the wrong record", + }); + // A retry of the same removal returns it; it is not removed twice. + await expect(repo.remove(account, removal, actor)).resolves.toMatchObject({ + version: 2, + }); + expect((await repo.list(account)).map((row) => row.id)).not.toContain( + file.id, + ); + expect((await repo.removed(account)).map((row) => row.id)).toContain( + file.id, + ); + await expect(repo.read(account, file.id, actor)).rejects.toThrow( + "ATTACHMENT_NOT_FOUND", + ); + await expect( + repo.remove(otherAccount, { ...removal, expectedVersion: 2 }, actor), + ).rejects.toThrow("ATTACHMENT_NOT_FOUND"); + // The bytes and the original row stay for audit. + const [kept] = await client<{ count: number }[]>` + select count(*)::int as count from commerce_account_documents d + join commerce_stored_documents s on s.id::text = d.storage_key + where d.id = ${file.id}`; + expect(kept?.count).toBe(1); + // The same PDF can be attached again, and then the old copy cannot be + // restored over it. + const again = await repo.add(account, input(), pdf, actor); + await expect( + repo.restore(account, { id: file.id, expectedVersion: 2 }, actor), + ).rejects.toThrow("ATTACHMENT_ALREADY_EXISTS"); + await repo.remove( + account, + { id: again.id, expectedVersion: again.version, reason: "Duplicate" }, + actor, + ); + const restored = await repo.restore( + account, + { id: file.id, expectedVersion: 2 }, + actor, + ); + expect(restored).toMatchObject({ id: file.id, version: 3 }); + expect((await repo.read(account, file.id, actor)).bytes).toEqual(pdf.bytes); + const events = await client< + { event_type: string; after: { reason?: string } }[] + >` + select event_type, after from audit_events + where aggregate_id = ${file.id} order by aggregate_version`; + expect(events.map((event) => event.event_type)).toEqual([ + "account.document_attached", + "account.document_removed", + "account.document_restored", + ]); + expect(events[1]?.after.reason).toBe("Attached to the wrong record"); + }); }); diff --git a/packages/db/src/repositories/account-documents.ts b/packages/db/src/repositories/account-documents.ts index 0d81ec6c..41e74f8e 100644 --- a/packages/db/src/repositories/account-documents.ts +++ b/packages/db/src/repositories/account-documents.ts @@ -1,13 +1,16 @@ import { createHash, randomUUID } from "node:crypto"; -import { and, desc, eq, sql } from "drizzle-orm"; +import { and, desc, eq, isNotNull, isNull, sql } from "drizzle-orm"; import { AccountDocumentInputSchema, + AccountDocumentRemovalSchema, + AccountDocumentRestoreSchema, AccountDocumentTargetSchema, ContractFileNameSchema, assertContractPdf, type AccountDocumentRecord, type AccountDocumentTarget, type Actor, + type RemovedAccountDocumentRecord, } from "@clockwork/contracts"; import type { RuntimeDatabase, RuntimeTransaction } from "../client"; import { accounts } from "../schema"; @@ -34,7 +37,31 @@ const view = (row: Row): AccountDocumentRecord => ({ sizeBytes: row.sizeBytes, uploadedByName: row.uploadedByName, createdAt: row.createdAt.toISOString(), + version: row.version, }); +const removedView = (row: Row): RemovedAccountDocumentRecord => ({ + ...view(row), + removedAt: row.removedAt?.toISOString() ?? "", + removedByName: row.removedByName ?? "", + removalReason: row.removalReason ?? "", +}); +const lockRecord = (tx: RuntimeTransaction, target: AccountDocumentTarget) => + tx.execute( + sql`select pg_advisory_xact_lock(hashtextextended(${`attachment:${target.type}:${target.id}`}, 0))`, + ); + +/** An identical PDF still on the record makes a restore a duplicate. */ +function duplicateRefusal(error: unknown): never { + for ( + let current: unknown = error, depth = 0; + current instanceof Error && depth < 4; + current = current.cause, depth += 1 + ) { + const driver = current as Error & { code?: unknown }; + if (driver.code === "23505") throw new Error("ATTACHMENT_ALREADY_EXISTS"); + } + throw error; +} async function requireTarget( tx: RuntimeTransaction, @@ -69,11 +96,153 @@ export class AccountDocumentRepository { await tx .select() .from(accountDocuments) - .where(scope(target)) + .where(and(scope(target), isNull(accountDocuments.removedAt))) .orderBy(desc(accountDocuments.createdAt), desc(accountDocuments.id)) ).map(view); }); } + /** Documents removed from the record, most recently removed first. */ + removed(rawTarget: AccountDocumentTarget) { + const target = AccountDocumentTargetSchema.parse(rawTarget); + return this.tx(async (tx) => { + await requireTarget(tx, target); + return ( + await tx + .select() + .from(accountDocuments) + .where(and(scope(target), isNotNull(accountDocuments.removedAt))) + .orderBy(desc(accountDocuments.removedAt), desc(accountDocuments.id)) + .limit(500) + ).map(removedView); + }); + } + /** + * Takes a document off its record: attached by mistake, to the wrong + * record, or superseded by a corrected copy. The row, its original details + * and the retained bytes stay for audit; the document can no longer be + * opened and no longer blocks attaching the same PDF again. A retry by the + * same person with the same reason returns the removed document. + */ + remove( + rawTarget: AccountDocumentTarget, + raw: unknown, + actor: Actor & { kind: "user" }, + ): Promise { + const target = AccountDocumentTargetSchema.parse(rawTarget); + const input = AccountDocumentRemovalSchema.parse(raw); + return this.tx(async (tx) => { + await lockRecord(tx, target); + const [row] = await tx + .select() + .from(accountDocuments) + .where(and(scope(target), eq(accountDocuments.id, input.id))) + .for("update"); + if (!row) throw new Error("ATTACHMENT_NOT_FOUND"); + if (row.removedAt) { + if ( + row.removedById === actor.id && + row.removalReason === input.reason && + row.version === input.expectedVersion + 1 + ) + return removedView(row); + throw new Error("ATTACHMENT_VERSION_CONFLICT"); + } + if (row.version !== input.expectedVersion) + throw new Error("ATTACHMENT_VERSION_CONFLICT"); + const [updated] = await tx + .update(accountDocuments) + .set({ + removedAt: new Date(), + removedById: actor.id, + // The database keeps a 1–200 character name; an empty or very + // long profile name never blocks a removal. + removedByName: (actor.display ?? "").trim().slice(0, 200) || actor.id, + removalReason: input.reason, + version: row.version + 1, + }) + .where(eq(accountDocuments.id, row.id)) + .returning(); + if (!updated) throw new Error("ATTACHMENT_NOT_FOUND"); + await appendAuditAndOutbox(tx, { + ...(target.type === "account" ? { accountId: target.id } : {}), + aggregateType: "document", + aggregateId: updated.id, + aggregateVersion: updated.version, + eventType: "account.document_removed", + actor, + requestId: randomUUID(), + before: { removed: false }, + after: { + target, + title: updated.title, + fileName: updated.fileName, + sha256: updated.sha256, + reason: input.reason, + }, + }); + return removedView(updated); + }); + } + /** Puts a removed document back, unless an identical PDF is now there. */ + restore( + rawTarget: AccountDocumentTarget, + raw: unknown, + actor: Actor & { kind: "user" }, + ): Promise { + const target = AccountDocumentTargetSchema.parse(rawTarget); + const input = AccountDocumentRestoreSchema.parse(raw); + return this.tx(async (tx) => { + await lockRecord(tx, target); + const [row] = await tx + .select() + .from(accountDocuments) + .where(and(scope(target), eq(accountDocuments.id, input.id))) + .for("update"); + if (!row) throw new Error("ATTACHMENT_NOT_FOUND"); + if (!row.removedAt) { + if (row.version === input.expectedVersion + 1) return view(row); + throw new Error("ATTACHMENT_VERSION_CONFLICT"); + } + if (row.version !== input.expectedVersion) + throw new Error("ATTACHMENT_VERSION_CONFLICT"); + const [twin] = await tx + .select({ id: accountDocuments.id }) + .from(accountDocuments) + .where( + and( + scope(target), + eq(accountDocuments.sha256, row.sha256), + isNull(accountDocuments.removedAt), + ), + ); + if (twin) throw new Error("ATTACHMENT_ALREADY_EXISTS"); + const [updated] = await tx + .update(accountDocuments) + .set({ + removedAt: null, + removedById: null, + removedByName: null, + removalReason: null, + version: row.version + 1, + }) + .where(eq(accountDocuments.id, row.id)) + .returning() + .catch(duplicateRefusal); + if (!updated) throw new Error("ATTACHMENT_NOT_FOUND"); + await appendAuditAndOutbox(tx, { + ...(target.type === "account" ? { accountId: target.id } : {}), + aggregateType: "document", + aggregateId: updated.id, + aggregateVersion: updated.version, + eventType: "account.document_restored", + actor, + requestId: randomUUID(), + before: { removed: true, reason: row.removalReason }, + after: { target, title: updated.title }, + }); + return view(updated); + }); + } async add( rawTarget: AccountDocumentTarget, rawInput: unknown, @@ -99,9 +268,7 @@ export class AccountDocumentRepository { try { const result = await this.tx(async (tx) => { // Serializes uploads and content duplicates for this one record. - await tx.execute( - sql`select pg_advisory_xact_lock(hashtextextended(${`attachment:${target.type}:${target.id}`}, 0))`, - ); + await lockRecord(tx, target); await requireTarget(tx, target); const [retry] = await tx .select() @@ -116,7 +283,8 @@ export class AccountDocumentRepository { retry.kind !== input.kind || retry.signedOn !== input.signedOn || retry.fileName !== fileName || - retry.uploadedById !== actor.id + retry.uploadedById !== actor.id || + retry.removedAt !== null ) throw new Error("ATTACHMENT_RETRY_CONFLICT"); return { row: retry, reused: true }; @@ -124,7 +292,13 @@ export class AccountDocumentRepository { const [duplicate] = await tx .select({ id: accountDocuments.id }) .from(accountDocuments) - .where(and(scope(target), eq(accountDocuments.sha256, hash))); + .where( + and( + scope(target), + eq(accountDocuments.sha256, hash), + isNull(accountDocuments.removedAt), + ), + ); if (duplicate) throw new Error("ATTACHMENT_ALREADY_EXISTS"); const [row] = await tx .insert(accountDocuments) @@ -178,7 +352,13 @@ export class AccountDocumentRepository { const [file] = await tx .select() .from(accountDocuments) - .where(and(scope(target), eq(accountDocuments.id, id))); + .where( + and( + scope(target), + eq(accountDocuments.id, id), + isNull(accountDocuments.removedAt), + ), + ); if (!file) throw new Error("ATTACHMENT_NOT_FOUND"); return file; }); diff --git a/packages/db/src/repositories/hubspot.integration.test.ts b/packages/db/src/repositories/hubspot.integration.test.ts index 35d8b0b3..0a0a6887 100644 --- a/packages/db/src/repositories/hubspot.integration.test.ts +++ b/packages/db/src/repositories/hubspot.integration.test.ts @@ -431,6 +431,83 @@ describe("HubSpot handoff", () => { ).resolves.toBeNull(); }); + it("never reuses or counts a document that was removed from the partner", async () => { + const companyId = "910007"; + const name = `Removed copy ${tag}`; + const plan = newPlan(name, { + documents: ["8201", "8202"].map((fileId) => ({ + fileId, + noteId: "6001", + fileName: `${fileId}.pdf`, + title: `File ${fileId}`, + kind: "other" as const, + size: 100, + updatedAt: null, + })), + }); + await repo.commit( + { + plan, + snapshot: snapshot(companyId, name), + requestDigest: digest(10), + connection, + companyId, + dealId: null, + }, + seller, + ); + const partnerId = plan.destination.partnerId; + const target = { type: "partner" as const, id: partnerId }; + const [first, second] = (await repo.import(partnerId, plan.operationId)) + .items; + if (!first || !second) throw new Error("items missing"); + const bytes = Buffer.from(`%PDF-1.7\nremoved-${tag}\n%%EOF`); + const attachment = await documents.add( + target, + { id: first.id, title: first.title, kind: "other" }, + { fileName: first.fileName, bytes }, + seller, + ); + await repo.recordItem( + partnerId, + plan.operationId, + first.id, + { status: "attached", attachmentId: attachment.id }, + seller, + ); + await repo.recordItem( + partnerId, + plan.operationId, + second.id, + { status: "duplicate", existingAttachmentId: attachment.id }, + seller, + ); + await documents.remove( + target, + { id: attachment.id, expectedVersion: 1, reason: "Wrong file" }, + seller, + ); + const record = await repo.import(partnerId, plan.operationId); + expect(record.items.find((i) => i.id === first.id)).toMatchObject({ + status: "attached", + attachmentRemoved: true, + }); + expect(record.items.find((i) => i.id === second.id)).toMatchObject({ + status: "duplicate", + existingAttachment: null, + }); + await expect( + repo.reuseExisting(partnerId, plan.operationId, second.id, seller), + ).rejects.toThrow("HUBSPOT_DOCUMENT_REMOVED"); + const { createHash } = await import("node:crypto"); + await expect( + repo.attachmentByHash( + partnerId, + createHash("sha256").update(bytes).digest("hex"), + ), + ).resolves.toBeNull(); + }); + it("stops committing reviews read through a connection that changed", async () => { const companyId = "910006"; const name = `Stale ${tag}`; diff --git a/packages/db/src/repositories/hubspot.ts b/packages/db/src/repositories/hubspot.ts index 3edb84b6..9930efd2 100644 --- a/packages/db/src/repositories/hubspot.ts +++ b/packages/db/src/repositories/hubspot.ts @@ -1,5 +1,5 @@ import { randomUUID } from "node:crypto"; -import { and, desc, eq, inArray, sql } from "drizzle-orm"; +import { and, desc, eq, inArray, isNull, sql } from "drizzle-orm"; import { HubSpotConnectionInputSchema, @@ -46,6 +46,11 @@ type UserActor = Actor & { kind: "user" }; type ConnectionRow = typeof commerceHubSpotConnections.$inferSelect; type ImportRow = typeof commerceHubSpotImports.$inferSelect; type ItemRow = typeof commerceHubSpotImportItems.$inferSelect; +interface ItemChange { + values: Partial; + event: string; + detail: Record; +} const connectionView = (row: ConnectionRow): HubSpotConnectionRecord => ({ id: row.id, @@ -66,7 +71,7 @@ const connectionView = (row: ConnectionRow): HubSpotConnectionRecord => ({ const itemView = ( row: ItemRow, - existing: ReadonlyMap, + documents: ReadonlyMap, ): HubSpotImportItem => ({ id: row.id, importId: row.importId, @@ -78,12 +83,18 @@ const itemView = ( status: row.status, errorCode: row.errorCode, attachmentId: row.attachmentId, - existingAttachment: row.existingAttachmentId - ? { - id: row.existingAttachmentId, - title: existing.get(row.existingAttachmentId) ?? "", - } - : null, + // An identical PDF that was since removed is no longer there to reuse. + existingAttachment: + row.existingAttachmentId && + !documents.get(row.existingAttachmentId)?.removed + ? { + id: row.existingAttachmentId, + title: documents.get(row.existingAttachmentId)?.title ?? "", + } + : null, + attachmentRemoved: row.attachmentId + ? (documents.get(row.attachmentId)?.removed ?? false) + : false, excludedReason: row.excludedReason, attempts: row.attempts, updatedAt: row.updatedAt.toISOString(), @@ -816,6 +827,8 @@ export class HubSpotRepository { and( eq(accountDocuments.partnerId, partnerId), eq(accountDocuments.sha256, sha256), + // A removed copy never stands in for a new one. + isNull(accountDocuments.removedAt), ), ); return row ?? null; @@ -884,19 +897,37 @@ export class HubSpotRepository { itemId: string, actor: UserActor, ) { - return this.changeItem(partnerId, importId, itemId, actor, (item) => { - if (item.status !== "duplicate" || !item.existingAttachmentId) - throw new Error("HUBSPOT_ITEM_RESOLVED"); - return { - values: { - status: "reused", - attachmentId: item.existingAttachmentId, - resolvedByName: actor.display ?? actor.id, - }, - event: "hubspot.document_reused", - detail: { attachmentId: item.existingAttachmentId }, - }; - }); + return this.changeItem( + partnerId, + importId, + itemId, + actor, + async (item, tx) => { + if (item.status !== "duplicate" || !item.existingAttachmentId) + throw new Error("HUBSPOT_ITEM_RESOLVED"); + // The identical copy must still be on the partner to be reused. + const [existing] = await tx + .select({ id: accountDocuments.id }) + .from(accountDocuments) + .where( + and( + eq(accountDocuments.id, item.existingAttachmentId), + eq(accountDocuments.partnerId, partnerId), + isNull(accountDocuments.removedAt), + ), + ); + if (!existing) throw new Error("HUBSPOT_DOCUMENT_REMOVED"); + return { + values: { + status: "reused", + attachmentId: item.existingAttachmentId, + resolvedByName: actor.display ?? actor.id, + }, + event: "hubspot.document_reused", + detail: { attachmentId: item.existingAttachmentId }, + }; + }, + ); } /** @@ -928,11 +959,10 @@ export class HubSpotRepository { importId: string, itemId: string, actor: UserActor, - change: (item: ItemRow) => { - values: Partial; - event: string; - detail: Record; - }, + change: ( + item: ItemRow, + tx: RuntimeTransaction, + ) => Promise | ItemChange, attachedAgain?: (item: ItemRow) => boolean, ): Promise { return this.tx(async (tx) => { @@ -967,7 +997,7 @@ export class HubSpotRepository { } throw new Error("HUBSPOT_ITEM_RESOLVED"); } - const { values, event, detail } = change(item); + const { values, event, detail } = await change(item, tx); await tx .update(commerceHubSpotImportItems) .set({ ...values, updatedAt: sql`now()`, version: item.version + 1 }) @@ -1031,16 +1061,26 @@ async function withItems( commerceHubSpotImportItems.fileName, commerceHubSpotImportItems.id, ); - const existingIds = items.flatMap((item) => - item.existingAttachmentId ? [item.existingAttachmentId] : [], - ); - const titles = existingIds.length + const documentIds = items.flatMap((item) => [ + ...(item.existingAttachmentId ? [item.existingAttachmentId] : []), + ...(item.attachmentId ? [item.attachmentId] : []), + ]); + const found = documentIds.length ? await tx - .select({ id: accountDocuments.id, title: accountDocuments.title }) + .select({ + id: accountDocuments.id, + title: accountDocuments.title, + removedAt: accountDocuments.removedAt, + }) .from(accountDocuments) - .where(inArray(accountDocuments.id, existingIds)) + .where(inArray(accountDocuments.id, documentIds)) : []; - const existing = new Map(titles.map((row) => [row.id, row.title])); + const existing = new Map( + found.map((row) => [ + row.id, + { title: row.title, removed: row.removedAt !== null }, + ]), + ); return rows.map((row) => importView( row, diff --git a/packages/db/src/schema/account-documents.ts b/packages/db/src/schema/account-documents.ts index 7b6056c5..f2854952 100644 --- a/packages/db/src/schema/account-documents.ts +++ b/packages/db/src/schema/account-documents.ts @@ -33,15 +33,21 @@ export const accountDocuments = pgTable( createdAt: timestamp("created_at", { withTimezone: true }) .notNull() .defaultNow(), + // 001484: an audited removal from the record; the row and bytes stay. + removedAt: timestamp("removed_at", { withTimezone: true }), + removedById: uuid("removed_by_id"), + removedByName: text("removed_by_name"), + removalReason: text("removal_reason"), + version: integer("version").notNull().default(1), }, (t) => [ uniqueIndex("account_documents_storage").on(t.storageBackend, t.storageKey), uniqueIndex("account_documents_partner_hash") .on(t.partnerId, t.sha256) - .where(sql`${t.partnerId} is not null`), + .where(sql`${t.partnerId} is not null and ${t.removedAt} is null`), uniqueIndex("account_documents_account_hash") .on(t.accountId, t.sha256) - .where(sql`${t.accountId} is not null`), + .where(sql`${t.accountId} is not null and ${t.removedAt} is null`), index("account_documents_partner").on( t.partnerId, t.createdAt.desc(), diff --git a/supabase/migrations/001484_account_document_removal.sql b/supabase/migrations/001484_account_document_removal.sql new file mode 100644 index 00000000..5f5ebca9 --- /dev/null +++ b/supabase/migrations/001484_account_document_removal.sql @@ -0,0 +1,79 @@ +-- Removing an attached document from a partner or account record +-- (docs/operations/partner-records.md#documents-attached-outside-commerce). +-- +-- A PDF attached by mistake, or to the wrong record, or superseded by a +-- corrected copy, can be removed by staff who may attach documents. Removal is +-- an audited state, not an erasure: the row, its original details and the +-- retained bytes stay, so the history of what was attached and why it was +-- taken off remains true. A removed document no longer appears on the record, +-- cannot be viewed or downloaded, and no longer blocks attaching the same PDF +-- again. A removal can be reversed. +-- +-- The staff service gains UPDATE on the removal columns only; every original +-- detail stays immutable, enforced both by the column grant and by a trigger. +set lock_timeout = '5s'; + +alter table public.commerce_account_documents + add column if not exists removed_at timestamptz, + add column if not exists removed_by_id uuid, + add column if not exists removed_by_name text, + add column if not exists removal_reason text, + add column if not exists version integer not null default 1; + +alter table public.commerce_account_documents + drop constraint if exists account_documents_removal_check; + +alter table public.commerce_account_documents + add constraint account_documents_removal_check check ( + version >= 1 + and (removed_at is null) = (removed_by_id is null) + and (removed_at is null) = (removed_by_name is null) + and (removed_at is null) = (removal_reason is null) + and (removal_reason is null or length(btrim(removal_reason)) between 1 and 500) + and (removed_by_name is null or length(btrim(removed_by_name)) between 1 and 200)); + +-- The same PDF may be attached again once the earlier copy is removed. +drop index if exists public.account_documents_partner_hash; + +create unique index account_documents_partner_hash + on public.commerce_account_documents (partner_id, sha256) + where partner_id is not null and removed_at is null; + +drop index if exists public.account_documents_account_hash; + +create unique index account_documents_account_hash + on public.commerce_account_documents (account_id, sha256) + where account_id is not null and removed_at is null; + +create or replace function public.protect_commerce_account_document() returns trigger +language plpgsql set search_path = public as $$ +begin + if to_jsonb(new) - array['removed_at','removed_by_id','removed_by_name','removal_reason','version'] + <> to_jsonb(old) - array['removed_at','removed_by_id','removed_by_name','removal_reason','version'] then + raise exception using errcode = 'P0001', message = 'ATTACHMENT_IMMUTABLE'; + end if; + if new.version <> old.version + 1 then + raise exception using errcode = 'P0001', message = 'ATTACHMENT_VERSION_MUST_ADVANCE'; + end if; + return new; +end $$; + +revoke all on function public.protect_commerce_account_document() from public; + +drop trigger if exists protect_commerce_account_document on public.commerce_account_documents; + +create trigger protect_commerce_account_document before update on public.commerce_account_documents + for each row execute function public.protect_commerce_account_document(); + +grant update (removed_at, removed_by_id, removed_by_name, removal_reason, version) + on public.commerce_account_documents to clockwork_service; + +drop policy if exists account_documents_remove on public.commerce_account_documents; + +create policy account_documents_remove on public.commerce_account_documents + for update to clockwork_service using (true) with check (true); + +comment on column public.commerce_account_documents.removed_at is + 'Set when staff removed the document from its record; the row and bytes are retained for audit.'; + +reset lock_timeout; diff --git a/supabase/tests/1484_account_document_removal.test.sql b/supabase/tests/1484_account_document_removal.test.sql new file mode 100644 index 00000000..915e0f95 --- /dev/null +++ b/supabase/tests/1484_account_document_removal.test.sql @@ -0,0 +1,55 @@ +begin; +select plan(9); + +select has_column('public', 'commerce_account_documents', 'removed_at', 'documents record a removal'); + +select ok(not has_table_privilege('clockwork_service', 'public.commerce_account_documents', 'UPDATE'), + 'staff have no table-wide update'); + +select ok(has_column_privilege('clockwork_service', 'public.commerce_account_documents', 'removed_at', 'UPDATE') + and has_column_privilege('clockwork_service', 'public.commerce_account_documents', 'removal_reason', 'UPDATE'), + 'staff may set the removal columns'); + +select ok(not has_column_privilege('clockwork_service', 'public.commerce_account_documents', 'title', 'UPDATE') + and not has_column_privilege('clockwork_service', 'public.commerce_account_documents', 'sha256', 'UPDATE') + and not has_column_privilege('clockwork_service', 'public.commerce_account_documents', 'storage_key', 'UPDATE'), + 'original details stay immutable for staff'); + +select ok(not has_table_privilege('clockwork_service', 'public.commerce_account_documents', 'DELETE'), + 'documents are never erased by staff'); + +insert into public.commerce_account_documents + (id, account_id, title, kind, file_name, storage_backend, storage_key, sha256, size_bytes, content_type, + uploaded_by_id, uploaded_by_name) +values ('00000000-0000-4000-8000-0000000014f1', '10000000-0000-4000-8000-000000000001', 'Wrong NDA', 'mnda', + 'Wrong.pdf', 'pgtap', 'pgtap-1484-a', repeat('b', 64), 8, 'application/pdf', gen_random_uuid(), 'Staff'); + +select throws_ok($$update public.commerce_account_documents + set removed_at = now(), removed_by_id = gen_random_uuid(), removed_by_name = 'Staff', removal_reason = null, + version = version + 1 + where id = '00000000-0000-4000-8000-0000000014f1'$$, + '23514', null, 'a removal states who and why'); + +select throws_ok($$update public.commerce_account_documents set title = 'Edited', version = version + 1 + where id = '00000000-0000-4000-8000-0000000014f1'$$, + 'P0001', 'ATTACHMENT_IMMUTABLE', 'the trigger keeps original details'); + +update public.commerce_account_documents + set removed_at = now(), removed_by_id = gen_random_uuid(), removed_by_name = 'Staff', + removal_reason = 'Attached to the wrong account', version = version + 1 + where id = '00000000-0000-4000-8000-0000000014f1'; + +select lives_ok($$insert into public.commerce_account_documents + (id, account_id, title, kind, file_name, storage_backend, storage_key, sha256, size_bytes, content_type, + uploaded_by_id, uploaded_by_name) + values (gen_random_uuid(), '10000000-0000-4000-8000-000000000001', 'Same NDA again', 'mnda', + 'Again.pdf', 'pgtap', 'pgtap-1484-b', repeat('b', 64), 8, 'application/pdf', gen_random_uuid(), 'Staff')$$, + 'a removed copy does not block attaching the same PDF again'); + +select throws_ok($$update public.commerce_account_documents + set removed_at = null, removed_by_id = null, removed_by_name = null, removal_reason = null, version = version + 1 + where id = '00000000-0000-4000-8000-0000000014f1'$$, + '23505', null, 'a restore cannot duplicate an identical document on the record'); + +select * from finish(); +rollback;