From 32af88f9c448407ca2dc1a6144bbc5047cf45f42 Mon Sep 17 00:00:00 2001 From: Mattsson <111893710+mattssonn@users.noreply.github.com> Date: Sat, 6 Jun 2026 15:27:09 +0200 Subject: [PATCH] =?UTF-8?q?feat(invoice):=20handle=20duplicate=20supplier?= =?UTF-8?q?=20invoice=20number=20conflict=20with=E2=80=A6=20(#685)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(invoice): handle duplicate supplier invoice number conflict with detailed error response * fix(invoice): enhance error response for duplicate invoice number to ensure data minimization --- .../supplier-invoices/new/page.tsx | 88 +++++++++++++------ .../__tests__/convert-route.test.ts | 39 ++++++++ extensions/general/invoice-inbox/index.ts | 62 +++++++++++++ 3 files changed, 163 insertions(+), 26 deletions(-) diff --git a/app/(dashboard)/supplier-invoices/new/page.tsx b/app/(dashboard)/supplier-invoices/new/page.tsx index 8fcc9e21..8c515dc7 100644 --- a/app/(dashboard)/supplier-invoices/new/page.tsx +++ b/app/(dashboard)/supplier-invoices/new/page.tsx @@ -37,6 +37,30 @@ interface LineItem { reverse_charge_rate?: number } +// The existing invoice surfaced on a duplicate-number conflict, used to drive +// the resolution dialog (open it / uncredit-and-retry). +interface ExistingSupplierInvoice { + id: string + supplier_invoice_number: string + status: string + credit_note_id: string | null +} + +// Canonical create/convert response. On failure `error` is the structured +// envelope's inner object ({ code, message, details }); a few legacy convert +// paths still return a flat string, so accept both. +interface CreateResult { + data?: { id: string; arrival_number: number } + error?: + | string + | { + code?: string + message?: string + message_en?: string + details?: { existing?: ExistingSupplierInvoice | null } + } +} + interface FormData { supplier_id: string supplier_invoice_number: string @@ -271,7 +295,7 @@ export default function NewSupplierInvoicePage() { // Conflict state for duplicate-supplier-invoice-number const [conflict, setConflict] = useState<{ message: string - existing: { id: string; supplier_invoice_number: string; status: string; credit_note_id: string | null } | null + existing: ExistingSupplierInvoice | null } | null>(null) const [isResolvingConflict, setIsResolvingConflict] = useState(false) const invoiceNumberInputRef = useRef(null) @@ -733,16 +757,13 @@ export default function NewSupplierInvoicePage() { } // Single submit endpoint chooser — convert when we came from inbox, plain - // POST otherwise. Both endpoints validate the same CreateSupplierInvoiceSchema. + // POST otherwise. Both endpoints validate the same CreateSupplierInvoiceSchema + // and return the same canonical error envelope ({ error: { code, message, + // details } }) — including the recoverable duplicate-number 409. async function postCreate(data: FormData): Promise<{ ok: boolean status: number - result: { - data?: { id: string; arrival_number: number } - error?: string - message?: string - existing?: { id: string; supplier_invoice_number: string; status: string; credit_note_id: string | null } - } + result: CreateResult }> { const url = inboxItemId ? `/api/extensions/ext/invoice-inbox/items/${inboxItemId}/convert` @@ -809,7 +830,12 @@ export default function NewSupplierInvoicePage() { const { ok, status, result } = await postCreate(data) if (!ok) { - handleCreateError(status, result) + // EF/direct path also hits the duplicate-number 409 (e.g. converting an + // inbox receipt whose number was already registered) — offer recovery + // instead of a dead-end toast. + if (!tryHandleDuplicateConflict(status, result)) { + handleCreateError(status, result) + } setIsSubmitting(false) return } @@ -892,24 +918,39 @@ export default function NewSupplierInvoicePage() { router.push(afterCreate(invoiceId)) } else { // Treat duplicate-number as a recoverable conflict; everything else as a hard error. - if (status === 409 && result.error === 'duplicate_supplier_invoice_number') { - setShowReview(false) - setConflict({ - message: result.message || t('duplicate_default_message'), - existing: result.existing ?? null, - }) - } else { + if (!tryHandleDuplicateConflict(status, result)) { handleCreateError(status, result) } } setIsSubmitting(false) } + // Detect the recoverable duplicate-supplier-invoice-number conflict and open + // the resolution dialog. Both the inbox `convert` route and the plain create + // route return the same structured 409 envelope, so this works for every + // submit path. Returns true when handled (caller should skip the error toast). + function tryHandleDuplicateConflict(status: number, result: CreateResult): boolean { + const err = result.error + if ( + status !== 409 || + typeof err !== 'object' || + err === null || + err.code !== 'SI_CREATE_DUPLICATE_INVOICE_NUMBER' + ) { + return false + } + // Close the review dialog if it was the path that triggered the conflict; + // a no-op for the EF/direct paths where it was never opened. + setShowReview(false) + setConflict({ + message: err.message || t('duplicate_default_message'), + existing: err.details?.existing ?? null, + }) + return true + } + // Shared error toast for non-conflict failures. - function handleCreateError( - status: number, - result: { error?: string; message?: string }, - ) { + function handleCreateError(status: number, result: CreateResult) { toast({ title: t('register_invoice_failed_title'), description: getErrorMessage(result, { context: 'supplier_invoice', statusCode: status }), @@ -992,12 +1033,7 @@ export default function NewSupplierInvoicePage() { const { ok, status, result } = await postCreate(pendingData) if (!ok || !result.data) { - if (status === 409 && result.error === 'duplicate_supplier_invoice_number') { - setConflict({ - message: result.message || t('duplicate_default_message'), - existing: result.existing ?? null, - }) - } else { + if (!tryHandleDuplicateConflict(status, result)) { handleCreateError(status, result) } setIsSubmitting(false) diff --git a/extensions/general/invoice-inbox/__tests__/convert-route.test.ts b/extensions/general/invoice-inbox/__tests__/convert-route.test.ts index 6448e016..cd32de66 100644 --- a/extensions/general/invoice-inbox/__tests__/convert-route.test.ts +++ b/extensions/general/invoice-inbox/__tests__/convert-route.test.ts @@ -131,6 +131,45 @@ describe('POST /items/:id/convert', () => { expect(status).toBe(404) }) + it('returns 409 (not 500) when the supplier invoice number already exists', async () => { + const { supabase, enqueue } = createQueuedMockSupabase() + enqueue({ data: makeInvoiceInboxItem({ status: 'received' }) }) + enqueue({ data: makeSupplier({ id: SUPPLIER_UUID }) }) + enqueue({ data: 42 }) + // Insert collides with idx_supplier_invoices_company_supplier_number. + enqueue({ + data: null, + error: { + code: '23505', + message: + 'duplicate key value violates unique constraint "idx_supplier_invoices_company_supplier_number"', + }, + }) + // Lookup of the existing (non-credited) invoice for the conflict payload. + enqueue({ + data: { id: 'existing-1', supplier_invoice_number: 'F-2024-001', status: 'approved' }, + }) + + const ctx = buildCtx(supabase) + const request = createMockRequest('/items/item-1/convert', { + method: 'POST', + body: VALID_CONVERT_BODY, + searchParams: { _id: 'item-1' }, + }) + const res = await route.handler(request, ctx) + const { status, body } = await parseJsonResponse<{ + error: { code: string; details?: Record & { existing?: { id: string } } } + }>(res) + + expect(status).toBe(409) + expect(body.error.code).toBe('SI_CREATE_DUPLICATE_INVOICE_NUMBER') + expect(body.error.details?.existing?.id).toBe('existing-1') + // Data minimisation: the raw request body must NOT be echoed back into the + // error envelope — only the server-authoritative `existing` row. + expect(body.error.details).not.toHaveProperty('supplierId') + expect(body.error.details).not.toHaveProperty('supplierInvoiceNumber') + }) + it('successfully converts inbox item to supplier invoice', async () => { const { supabase, enqueue } = createQueuedMockSupabase() const inboxItem = makeInvoiceInboxItem({ status: 'received', document_id: 'doc-1' }) diff --git a/extensions/general/invoice-inbox/index.ts b/extensions/general/invoice-inbox/index.ts index f11b9496..172173bf 100644 --- a/extensions/general/invoice-inbox/index.ts +++ b/extensions/general/invoice-inbox/index.ts @@ -1718,6 +1718,68 @@ export const invoiceInboxExtension: Extension = { .single() if (invoiceError || !invoice) { + // A unique-index hit on (company_id, supplier_id, + // supplier_invoice_number) is a recoverable conflict — the user + // already registered this invoice (often manually, then tried to + // convert the same inbox document). Mirror the main + // /api/supplier-invoices route and return a friendly 409 with the + // existing invoice, instead of letting the raw Postgres message + // surface as a generic 500 ("Ett oväntat serverfel uppstod"). + const pgErr = invoiceError as { code?: string; message?: string } | null + const isDuplicateNumber = + pgErr?.code === '23505' && + (pgErr.message || '').includes('idx_supplier_invoices_company_supplier_number') + + if (isDuplicateNumber) { + // Tenancy: ctx.supabase is the cookie-scoped RLS client and the + // supplier_invoices SELECT policy is + // `company_id IN (SELECT user_company_ids())`. Combined with the + // explicit company_id filter below, this lookup can only ever + // resolve an invoice the caller's own company owns — the returned + // details are never cross-tenant (OWASP ASVS V8.2.1; ISO 27001 + // A.8.3; GDPR art.25(2)). + const { data: existing } = await ctx.supabase + .from('supplier_invoices') + .select('id, supplier_invoice_number, status') + .eq('company_id', ctx.companyId) + .eq('supplier_id', body.supplier_id) + .eq('supplier_invoice_number', body.supplier_invoice_number) + .maybeSingle() + + let creditNoteId: string | null = null + if (existing?.status === 'credited') { + const { data: creditNote } = await ctx.supabase + .from('supplier_invoices') + .select('id') + .eq('company_id', ctx.companyId) + .eq('credited_invoice_id', existing.id) + .eq('is_credit_note', true) + .maybeSingle() + creditNoteId = creditNote?.id ?? null + } + + // Return ONLY server-authoritative fields the recovery dialog needs + // (the existing row, read under RLS). The raw request body + // (supplier_id / supplier_invoice_number) is deliberately not + // echoed back: the client already holds it from its own form state, + // and reflecting user-supplied values widens the response surface + // for no benefit (GDPR art.5(1)(c) data minimisation; OWASP ASVS + // V4.5). The Postgres constraint name is used only to classify the + // error above and is never placed in the response. + return errorResponseFromCode('SI_CREATE_DUPLICATE_INVOICE_NUMBER', ctx.log, { + details: { + existing: existing + ? { + id: existing.id, + supplier_invoice_number: existing.supplier_invoice_number, + status: existing.status, + credit_note_id: creditNoteId, + } + : null, + }, + }) + } + return NextResponse.json({ error: invoiceError?.message || 'Failed to create invoice' }, { status: 500 }) }