fix(transactions): stop defaulting supplier-invoice payment account to a stale private-funds setting (#985)

* fix(transactions): stop defaulting supplier-invoice payment account to a stale private-funds setting match-supplier-invoice (POST + preview) defaulted the credited cash account from company_settings.last_supplier_payment_account, a sticky setting written by the manual mark-paid "betald med privata medel" flow. Once that setting held 2893 (skuld till aktieägare) from an unrelated private payment, every later match against a real bank transaction reused it instead of the transaction's actual bank account, silently booking genuine bank payments as shareholder-loan repayments. Resolve the credit account from the matched transaction's own cash_account_id -> cash_accounts.ledger_account instead (falling back to 1930 when unlinked), mirroring the existing settlement-account lookup in transactions/[id]/categorize/route.ts. last_supplier_payment_account is no longer read by either route. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Jonas Flodén <jonas@floden.nu> * refactor(transactions): extract shared settlement-account resolution helper Dedupe the identical cash_account_id -> ledger_account lookup across match-supplier-invoice (POST + preview) and categorize into resolveSettlementAccount, per CodeRabbit's nitpick on PR #985. Pure extraction, no behavior change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Jonas Flodén <jonas@floden.nu> * test(transactions): cover settlement-account lookup-error and preview parity gaps Adds the two test cases CodeRabbit flagged as missing on PR #985: - POST match-supplier-invoice: cash_accounts lookup errors, falls back to 1930 and warns (previously unexercised). - preview match-supplier-invoice: linked cash account other than 1930 (parity with the equivalent POST-route test). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Jonas Flodén <jonas@floden.nu> * fix(transactions): thread resolved settlement account into FX/cash-method supplier-payment branches Closes the remaining items from the Swedish-accounting-compliance bot review on PR #985: - match-supplier-invoice/route.ts computed paymentAccount via resolveSettlementAccount but only passed it into the pure-SEK clearing branch; the FX branch (createSupplierInvoicePaymentEntry) and cash-method branch (createSupplierInvoiceCashEntry) still defaulted to 1930 internally even though both already accepted the parameter. - resolveSettlementAccount now also warns (and falls back to 1930) when cash_account_id resolves to a row with no ledger_account, not just on a hard query error. - Documents company_settings.last_supplier_payment_account's scope via a column comment: it must never be read to resolve a matched transaction's settlement account. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Jonas Flodén <jonas@floden.nu> * fix(bookkeeping): abort instead of silently defaulting to 1930 when settlement-account lookup errors Compliance-bot finding on PR #987 (applies equally to #985/#986, shared helper): resolveSettlementAccount treated "no cash_account_id" and "lookup threw a real DB error" the same way -- warn and fall back to 1930. An explicit cash_account_id almost certainly resolves to a non-1930 account, so a transient failure masking it risked the exact class of misbooking this whole PR series exists to fix, just triggered by infra flakiness instead of a stale setting. Now throws BookkeepingDatabaseError on a genuine query error; every caller already runs under withRouteContext/withApiV1 (or the pending- operations dispatcher), whose existing catch-all already converts any isBookkeepingError() throw into the correct structured 500 -- no caller changes needed. The "row found but ledger_account empty" case stays warn+fallback (data-integrity gap, not a query failure). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Jonas Flodén <jonas@floden.nu> * test(bookkeeping): use rejects.toBeInstanceOf for settlement-account error assertion Addresses CodeRabbit nitpick from the 2026-07-12 review round: matching BookkeepingDatabaseError via a `constructor` key in toMatchObject is non-idiomatic; toBeInstanceOf is the standard vitest assertion for this. Signed-off-by: Jonas Flodén <jonas@floden.nu> * docs: scope FX/cash-method paymentAccount gap note to /api/v1 and MCP routes CodeRabbit flagged the #1000 reference on PR #985 as ambiguous — the main match-supplier-invoice route's FX/cash-method branches already thread paymentAccount (per the prior entry), so the still-open gap only applies to the /api/v1 and MCP-facing route. Signed-off-by: Jonas Flodén <jonas@floden.nu> --------- Signed-off-by: Jonas Flodén <jonas@floden.nu> Co-authored-by: Jakob Wennberg <jakob.wennberg@gmail.com>
2026-07-12 21:04:39 +02:00
parent 1e19099945
commit 528c53ffe7
10 changed files with 494 additions and 33 deletions
@@ -8,6 +8,7 @@ import { createTransactionJournalEntry } from '@/lib/bookkeeping/transaction-ent
 import { detectBookingDuplicate } from '@/lib/transactions/booking-duplicate-detection'
 import { appendProcessingHistory } from '@/lib/processing-history/append'
 import { saveUserMappingRule, applySettlementAccount } from '@/lib/bookkeeping/mapping-engine'
+import { resolveSettlementAccount } from '@/lib/bookkeeping/settlement-account'
 import { upsertCounterpartyTemplate, buildMappingResultFromCounterpartyTemplate } from '@/lib/bookkeeping/counterparty-templates'
 import { withRouteContext } from '@/lib/api/with-route-context'
 import { errorResponse, errorResponseFromCode } from '@/lib/errors/get-structured-error'
@@ -314,26 +315,12 @@ export const POST = withRouteContext(
    // real bank line never reconciles. applySettlementAccount only rewrites a
    // 1930 leg and is a no-op when the settlement account is 1930, so legacy
    // rows with no cash_account_id behave exactly as before.
-    let settlementAccount = '1930'
-    if (transaction.cash_account_id) {
-      const { data: txCashAccount, error: cashAccountError } = await supabase
-        .from('cash_accounts')
-        .select('ledger_account')
-        .eq('id', transaction.cash_account_id)
-        .eq('company_id', companyId)
-        .maybeSingle()
-      if (cashAccountError) {
-        // Don't fail the booking: fall back to 1930, but surface the lookup
-        // failure so a silent mis-booking to the wrong bank leg stays auditable.
-        txLog.warn('settlement-account lookup failed; defaulting to 1930', {
-          cashAccountId: transaction.cash_account_id,
-          error: cashAccountError.message,
-        })
-      }
-      if (txCashAccount?.ledger_account) {
-        settlementAccount = txCashAccount.ledger_account as string
-      }
-    }
+    const settlementAccount = await resolveSettlementAccount(
+      supabase,
+      companyId!,
+      transaction.cash_account_id,
+      txLog,
+    )
    mappingResult = applySettlementAccount(mappingResult, settlementAccount)

    txLog.info('mapping resolved', {
@@ -6,6 +6,16 @@ import {
 } from '@/tests/helpers'
 import { AccountsNotInChartError } from '@/lib/bookkeeping/errors'

+const { mockLoggerWarn } = vi.hoisted(() => ({ mockLoggerWarn: vi.fn() }))
+vi.mock('@/lib/logger', () => ({
+  createLogger: () => ({
+    info: vi.fn(),
+    warn: mockLoggerWarn,
+    error: vi.fn(),
+    child: vi.fn().mockReturnThis(),
+  }),
+}))
+
 const { supabase: mockSupabase, enqueue, reset } = createQueuedMockSupabase()
 vi.mock('@/lib/supabase/server', () => ({
  createClient: () => Promise.resolve(mockSupabase),
@@ -202,6 +212,156 @@ describe('POST /api/transactions/[id]/match-supplier-invoice: FX residual', () =
  })
 })

+describe('POST /api/transactions/[id]/match-supplier-invoice: settlement account resolution', () => {
+  // Regression for a real misbooking: a company whose last few supplier
+  // invoices were paid privately (mark-paid sets company_settings
+  // .last_supplier_payment_account = '2893') later matched a genuine bank
+  // transaction to a supplier invoice. The route used to default
+  // paymentAccount from that sticky setting, so the payment credited 2893
+  // (shareholder loan) instead of the transaction's real 1930 bank account.
+  it('credits the account this transaction is linked to, even with a stale last_supplier_payment_account on file', async () => {
+    enqueue({
+      data: {
+        id: TX_UUID,
+        company_id: 'company-1',
+        amount: -1001,
+        currency: 'SEK',
+        amount_sek: null,
+        supplier_invoice_id: null,
+        cash_account_id: 'ca-1930',
+        date: '2026-02-01',
+      },
+      error: null,
+    })
+    enqueue({
+      data: {
+        id: SI_UUID,
+        currency: 'SEK',
+        exchange_rate: null,
+        status: 'registered',
+        remaining_amount: 1001,
+        paid_amount: 0,
+        supplier: { supplier_type: 'swedish_business' },
+        items: [],
+      },
+      error: null,
+    })
+    // Stale sticky setting from an earlier private-funds payment: must be
+    // ignored now that the route no longer selects it.
+    enqueue({ data: { accounting_method: 'accrual', last_supplier_payment_account: '2893' }, error: null })
+    enqueue({ data: { ledger_account: '1930' }, error: null }) // cash_accounts lookup
+    enqueue({ data: [{ id: SI_UUID }], error: null })
+    enqueue({ data: null, error: null })
+    enqueue({ data: null, error: null })
+
+    await POST(makeReq(), createMockRouteParams({ id: TX_UUID }))
+
+    expect(mockCreateJournalEntry).toHaveBeenCalledTimes(1)
+    const input = mockCreateJournalEntry.mock.calls[0][3] as {
+      lines: Array<{ account_number: string; debit_amount: number; credit_amount: number }>
+    }
+    expect(input.lines.find((l) => l.account_number === '1930')?.credit_amount).toBe(1001)
+    expect(input.lines.some((l) => l.account_number === '2893')).toBe(false)
+  })
+
+  it('credits the transaction\'s own linked cash account when it is not the primary 1930', async () => {
+    enqueue({
+      data: {
+        id: TX_UUID,
+        company_id: 'company-1',
+        amount: -500,
+        currency: 'SEK',
+        amount_sek: null,
+        supplier_invoice_id: null,
+        cash_account_id: 'ca-1940',
+        date: '2026-02-01',
+      },
+      error: null,
+    })
+    enqueue({
+      data: {
+        id: SI_UUID,
+        currency: 'SEK',
+        exchange_rate: null,
+        status: 'registered',
+        remaining_amount: 500,
+        paid_amount: 0,
+        supplier: { supplier_type: 'swedish_business' },
+        items: [],
+      },
+      error: null,
+    })
+    enqueue({ data: { accounting_method: 'accrual' }, error: null })
+    enqueue({ data: { ledger_account: '1940' }, error: null }) // cash_accounts lookup
+    enqueue({ data: [{ id: SI_UUID }], error: null })
+    enqueue({ data: null, error: null })
+    enqueue({ data: null, error: null })
+
+    await POST(makeReq(), createMockRouteParams({ id: TX_UUID }))
+
+    const input = mockCreateJournalEntry.mock.calls[0][3] as {
+      lines: Array<{ account_number: string; debit_amount: number; credit_amount: number }>
+    }
+    expect(input.lines.find((l) => l.account_number === '1940')?.credit_amount).toBe(500)
+  })
+
+  it('defaults to 1930 when the transaction has no linked cash account', async () => {
+    enqueueHappyPath({
+      transaction: { amount: -750, currency: 'SEK' },
+      invoice: { currency: 'SEK', remaining_amount: 750 },
+    })
+    await POST(makeReq(), createMockRouteParams({ id: TX_UUID }))
+    const input = mockCreateJournalEntry.mock.calls[0][3] as {
+      lines: Array<{ account_number: string; debit_amount: number; credit_amount: number }>
+    }
+    expect(input.lines.find((l) => l.account_number === '1930')?.credit_amount).toBe(750)
+  })
+
+  it('aborts with 500 BOOKKEEPING_DATABASE_ERROR (mutates nothing) when the cash_accounts lookup errors', async () => {
+    // Regression: an explicit cash_account_id almost certainly resolves to a
+    // non-1930 account, so a transient lookup failure must not silently
+    // degrade to 1930 (same misbooking risk this whole fix exists to close,
+    // just triggered by infra flakiness instead of a stale setting). The
+    // request should fail before any state mutation, not book to a guessed
+    // account.
+    enqueue({
+      data: {
+        id: TX_UUID,
+        company_id: 'company-1',
+        amount: -600,
+        currency: 'SEK',
+        amount_sek: null,
+        supplier_invoice_id: null,
+        cash_account_id: 'ca-broken',
+        date: '2026-02-01',
+      },
+      error: null,
+    })
+    enqueue({
+      data: {
+        id: SI_UUID,
+        currency: 'SEK',
+        exchange_rate: null,
+        status: 'registered',
+        remaining_amount: 600,
+        paid_amount: 0,
+        supplier: { supplier_type: 'swedish_business' },
+        items: [],
+      },
+      error: null,
+    })
+    enqueue({ data: { accounting_method: 'accrual' }, error: null })
+    enqueue({ data: null, error: { message: 'connection reset' } }) // cash_accounts lookup errors
+
+    const res = await POST(makeReq(), createMockRouteParams({ id: TX_UUID }))
+    const { status, body } = await parseJsonResponse<{ error: { code: string } }>(res)
+
+    expect(status).toBe(500)
+    expect(body.error.code).toBe('BOOKKEEPING_DATABASE_ERROR')
+    expect(mockCreateJournalEntry).not.toHaveBeenCalled()
+  })
+})
+
 describe('POST /api/transactions/[id]/match-supplier-invoice: non-FX paths', () => {
  it('returns 200 with the expected body shape on the happy path', async () => {
    enqueueHappyPath({
@@ -0,0 +1,149 @@
+import { describe, it, expect, vi, beforeEach } from 'vitest'
+import {
+  createQueuedMockSupabase,
+  createMockRouteParams,
+  parseJsonResponse,
+} from '@/tests/helpers'
+
+const { supabase: mockSupabase, enqueue, reset } = createQueuedMockSupabase()
+vi.mock('@/lib/supabase/server', () => ({
+  createClient: () => Promise.resolve(mockSupabase),
+}))
+
+vi.mock('@/lib/company/context', () => ({
+  requireCompanyId: vi.fn().mockResolvedValue('company-1'),
+  getActiveCompanyId: vi.fn().mockResolvedValue('company-1'),
+}))
+
+import { GET } from '../route'
+
+const mockUser = { id: 'user-1', email: 'test@test.se' }
+const TX_UUID = '11111111-1111-4111-8111-111111111111'
+const SI_UUID = '22222222-2222-4222-8222-222222222222'
+
+beforeEach(() => {
+  vi.clearAllMocks()
+  reset()
+  mockSupabase.auth.getUser.mockResolvedValue({ data: { user: mockUser } })
+})
+
+function makeReq() {
+  return new Request(
+    `http://localhost/api/transactions/${TX_UUID}/match-supplier-invoice/preview?supplier_invoice_id=${SI_UUID}`,
+  )
+}
+
+// Regression: the sticky company_settings.last_supplier_payment_account
+// (written whenever a supplier invoice is marked paid "with private funds",
+// e.g. crediting 2893) used to be the previewed credit account for ANY
+// matched transaction, including one linked to the company's real 1930 bank
+// account. The preview must credit the transaction's own linked cash
+// account, not that unrelated sticky setting.
+describe('GET /api/transactions/[id]/match-supplier-invoice/preview: settlement account resolution', () => {
+  it('previews a credit to the transaction\'s linked cash account, ignoring a stale last_supplier_payment_account', async () => {
+    enqueue({
+      data: {
+        id: TX_UUID,
+        date: '2026-02-01',
+        amount: -1001,
+        currency: 'SEK',
+        amount_sek: null,
+        cash_account_id: 'ca-1930',
+      },
+      error: null,
+    })
+    enqueue({
+      data: {
+        id: SI_UUID,
+        currency: 'SEK',
+        exchange_rate: null,
+        total: 1001,
+        remaining_amount: 1001,
+        registration_journal_entry_id: 'je-registered',
+        items: [],
+      },
+      error: null,
+    })
+    // Stale sticky setting from an earlier private-funds payment: must be
+    // ignored now that the route resolves the account from the transaction.
+    enqueue({ data: { accounting_method: 'accrual', last_supplier_payment_account: '2893' }, error: null })
+    enqueue({ data: { ledger_account: '1930' }, error: null }) // cash_accounts lookup
+
+    const res = await GET(makeReq(), createMockRouteParams({ id: TX_UUID }))
+    const { body } = await parseJsonResponse<{
+      lines: Array<{ account_number: string; debit_amount: number; credit_amount: number }>
+    }>(res)
+
+    expect(body.lines.find((l) => l.account_number === '1930')?.credit_amount).toBe(1001)
+    expect(body.lines.some((l) => l.account_number === '2893')).toBe(false)
+  })
+
+  it('defaults to 1930 when the transaction has no linked cash account', async () => {
+    enqueue({
+      data: {
+        id: TX_UUID,
+        date: '2026-02-01',
+        amount: -750,
+        currency: 'SEK',
+        amount_sek: null,
+        cash_account_id: null,
+      },
+      error: null,
+    })
+    enqueue({
+      data: {
+        id: SI_UUID,
+        currency: 'SEK',
+        exchange_rate: null,
+        total: 750,
+        remaining_amount: 750,
+        registration_journal_entry_id: 'je-registered',
+        items: [],
+      },
+      error: null,
+    })
+    enqueue({ data: { accounting_method: 'accrual' }, error: null })
+
+    const res = await GET(makeReq(), createMockRouteParams({ id: TX_UUID }))
+    const { body } = await parseJsonResponse<{
+      lines: Array<{ account_number: string; debit_amount: number; credit_amount: number }>
+    }>(res)
+
+    expect(body.lines.find((l) => l.account_number === '1930')?.credit_amount).toBe(750)
+  })
+
+  it('previews a credit to the linked cash account when it is not the primary 1930', async () => {
+    enqueue({
+      data: {
+        id: TX_UUID,
+        date: '2026-02-01',
+        amount: -500,
+        currency: 'SEK',
+        amount_sek: null,
+        cash_account_id: 'ca-1940',
+      },
+      error: null,
+    })
+    enqueue({
+      data: {
+        id: SI_UUID,
+        currency: 'SEK',
+        exchange_rate: null,
+        total: 500,
+        remaining_amount: 500,
+        registration_journal_entry_id: 'je-registered',
+        items: [],
+      },
+      error: null,
+    })
+    enqueue({ data: { accounting_method: 'accrual' }, error: null })
+    enqueue({ data: { ledger_account: '1940' }, error: null }) // cash_accounts lookup
+
+    const res = await GET(makeReq(), createMockRouteParams({ id: TX_UUID }))
+    const { body } = await parseJsonResponse<{
+      lines: Array<{ account_number: string; debit_amount: number; credit_amount: number }>
+    }>(res)
+
+    expect(body.lines.find((l) => l.account_number === '1940')?.credit_amount).toBe(500)
+  })
+})
@@ -13,6 +13,7 @@ import { withRouteContext } from '@/lib/api/with-route-context'
 import { errorResponseFromCode } from '@/lib/errors/get-structured-error'
 import { resolveSekAmount } from '@/lib/bookkeeping/currency-utils'
 import { buildSupplierPaymentClearingLines } from '@/lib/bookkeeping/supplier-payment-lines'
+import { resolveSettlementAccount } from '@/lib/bookkeeping/settlement-account'
 import { ORE_TOLERANCE } from '@/lib/money'
 import type { SupplierInvoice, SupplierInvoiceItem } from '@/types'

@@ -50,7 +51,9 @@ export const GET = withRouteContext(
      // amount_sek is needed for the cash-method preview: a foreign-currency
      // settlement is translated at the payment-date rate (the SEK that left
      // the bank), mirroring the committed verifikat from the POST handler.
-      .select('id, date, amount, currency, amount_sek')
+      // cash_account_id resolves which BAS account this bank line actually
+      // settles from, mirroring the POST handler's settlement-account lookup.
+      .select('id, date, amount, currency, amount_sek, cash_account_id')
      .eq('id', transactionId)
      .eq('company_id', companyId)
      .single()
@@ -70,13 +73,22 @@ export const GET = withRouteContext(

    const { data: settings } = await supabase
      .from('company_settings')
-      .select('accounting_method, last_supplier_payment_account')
+      .select('accounting_method')
      .eq('company_id', companyId)
      .single()

    const accountingMethod = settings?.accounting_method || 'accrual'
-    const paymentAccount =
-      (settings as { last_supplier_payment_account?: string } | null)?.last_supplier_payment_account || '1930'
+
+    // Same resolution as the POST handler: credit the cash account this
+    // transaction is actually linked to, never the sticky
+    // last_supplier_payment_account (that setting reflects the manual
+    // mark-paid/private-funds flow, not a real matched bank transaction).
+    const paymentAccount = await resolveSettlementAccount(
+      supabase,
+      companyId!,
+      transaction.cash_account_id,
+      log,
+    )

    const siAlreadyBooked = !!(invoice as { registration_journal_entry_id?: string | null }).registration_journal_entry_id
    const useCashEntry = !siAlreadyBooked && accountingMethod === 'cash'
@@ -4,6 +4,7 @@ import {
  createSupplierInvoiceCashEntry,
 } from '@/lib/bookkeeping/supplier-invoice-entries'
 import { buildSupplierPaymentClearingLines } from '@/lib/bookkeeping/supplier-payment-lines'
+import { resolveSettlementAccount } from '@/lib/bookkeeping/settlement-account'
 import { cancelOrphanedPaymentEntry } from '@/lib/bookkeeping/cancel-orphaned-entry'
 import { planSupplierPayment } from '@/lib/invoices/apply-supplier-payment'
 import { createJournalEntry, findFiscalPeriod } from '@/lib/bookkeeping/engine'
@@ -104,15 +105,25 @@ export const POST = withRouteContext(

    const { data: settings } = await supabase
      .from('company_settings')
-      .select('accounting_method, last_supplier_payment_account')
+      .select('accounting_method')
      .eq('company_id', companyId)
      .single()

    const accountingMethod = settings?.accounting_method || 'accrual'
-    // Same default the preview route uses, so the committed verifikat credits the
-    // same account the user saw previewed (the old path hardcoded 1930 here).
-    const paymentAccount =
-      (settings as { last_supplier_payment_account?: string } | null)?.last_supplier_payment_account || '1930'
+
+    // Credit the cash account THIS transaction actually belongs to, never a
+    // company-wide "last used" preference: last_supplier_payment_account is
+    // written by the manual mark-paid flow (e.g. a private-funds payment
+    // booked to 2893) and has no relationship to which bank account a real,
+    // matched transaction settled from. Reusing it here silently misbooked a
+    // genuine 1930 bank payment to 2893 once a private payment had set that
+    // sticky default.
+    const paymentAccount = await resolveSettlementAccount(
+      supabase,
+      companyId!,
+      transaction.cash_account_id,
+      txLog,
+    )

    // Route on the supplier invoice's actual booking state: if 2440 was posted
    // at receipt (accrual), the match clears 2440 regardless of the company's
@@ -254,10 +265,11 @@ export const POST = withRouteContext(
          transaction.date,
          invoice.supplier?.supplier_type || 'swedish_business',
          undefined, // supplierName (unchanged default)
-          undefined, // paymentAccount (unchanged default 1930)
-          // Pin a foreign-currency settlement to the payment-date rate so 1930
-          // equals the bank movement (kontantmetoden books the expense at
-          // payment). No-op for SEK invoices and same-rate settlements.
+          paymentAccount,
+          // Pin a foreign-currency settlement to the payment-date rate so the
+          // settlement account equals the bank movement (kontantmetoden books
+          // the expense at payment). No-op for SEK invoices and same-rate
+          // settlements.
          exchangeRateDifference !== 0 && fullSettlement ? actualBankSek : undefined,
        )
        if (journalEntry) journalEntryId = journalEntry.id
@@ -292,6 +304,8 @@ export const POST = withRouteContext(
          supabase, companyId, user.id, invoice as SupplierInvoice,
          paymentAmountSek, transaction.date,
          exchangeRateDifference !== 0 ? exchangeRateDifference : undefined,
+          undefined, // supplierName (unchanged default)
+          paymentAccount,
        )
        if (journalEntry) journalEntryId = journalEntry.id
      }