fix(transactions): categorize fails closed when the verifikat cannot be created (#1990)
* fix(transactions): categorize fails closed when the verifikat cannot be created (#1947) Booking into a locked period refused the verifikat but still wrote is_business/category, so the row left "Att bokföra" and the nav badge while journal_entry_id stayed NULL (canonical worklist predicate: is_business IS NULL). The verifikat is the booking: when it cannot be created nothing is written and the request returns a typed 409 TX_CATEGORIZE_JOURNAL_ENTRY_FAILED (Swedish reason preserved, details.cause = underlying code); a null engine return maps to 400 NO_OPEN_PERIOD_FOR_DATE. Same shape on the dashboard route, the v1 single route and per item in v1 batch-categorize. journal_entry_error stays in the 200 body, always null, for client compatibility. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FkUfWtuFCUkNtRAgMQCse2 * fix(transactions): fail closed on the engine's null return in the MCP/bulk door too Review findings on #1990: categorizeMatchedTransaction (pending-op approval, Underlag bulk-book) still wrote is_business/category with journal_entry_id NULL when createTransactionJournalEntry returned null (closed year or missing period return null without throwing), recreating the exact #1947 stranding while the tool reported success. The core now refuses before the transactions update with a structured 400 whose errorCode (PERIOD_LOCKED or NO_OPEN_PERIOD_FOR_DATE, told apart via checkPeriodLock) flows into result_data.error_code; the bulk driver skips such items with reason no_open_period. The dashboard route's null guard gets the same disambiguation: a closed covering year answers PERIOD_LOCKED (reason period_is_closed) instead of claiming the rakenskapsar does not exist, and the thrown-error branch now pairs messageSv with messageEn per the errorResponseFromCode contract. TX_CATEGORIZE_JOURNAL_ENTRY_FAILED message_en no longer embeds API-doc prose (details.cause guidance lives in remediation). DECISIONS line corrected: the MCP door was fail-closed only for thrown engine errors, not the null return. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FkUfWtuFCUkNtRAgMQCse2 --------- Co-authored-by: Jakob Wennberg <311770904+jakobwennberg-oss@users.noreply.github.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Jakob Wennberg
co-authored by
Claude Fable 5
Jakob Wennberg
Claude Fable 5
Jakob Wennberg
parent
cb9ae15d46
commit
f0af4ad4ee
@@ -7,7 +7,7 @@ import {
|
||||
makeTransaction,
|
||||
} from '@/tests/helpers'
|
||||
import { eventBus } from '@/lib/events'
|
||||
import { JournalEntryNotBalancedError } from '@/lib/bookkeeping/errors'
|
||||
import { BookkeepingDatabaseError, JournalEntryNotBalancedError } from '@/lib/bookkeeping/errors'
|
||||
|
||||
const { supabase: mockSupabase, enqueue, reset, findCalls } = createQueuedMockSupabase()
|
||||
vi.mock('@/lib/supabase/server', () => ({
|
||||
@@ -86,6 +86,13 @@ vi.mock('@/lib/bookkeeping/cancel-orphaned-entry', () => ({
|
||||
reverseOrphanedJournalEntry: (...args: unknown[]) => mockReverseOrphanedJournalEntry(...args),
|
||||
}))
|
||||
|
||||
// Null-return disambiguation (issue #1947): the route asks checkPeriodLock
|
||||
// whether the engine's null was a closed covering period or a missing one.
|
||||
const mockCheckPeriodLock = vi.fn()
|
||||
vi.mock('@/lib/api/v1/check-period-lock', () => ({
|
||||
checkPeriodLock: (...args: unknown[]) => mockCheckPeriodLock(...args),
|
||||
}))
|
||||
|
||||
const mockFindMissingActiveAccounts = vi.fn()
|
||||
vi.mock('@/lib/bookkeeping/account-validation', async () => {
|
||||
const actual = await vi.importActual<typeof import('@/lib/bookkeeping/account-validation')>(
|
||||
@@ -126,6 +133,8 @@ describe('POST /api/transactions/[id]/categorize', () => {
|
||||
mockDetectDup.mockResolvedValue(null)
|
||||
mockAppendProcessingHistory.mockResolvedValue('evt-1')
|
||||
mockReverseOrphanedJournalEntry.mockResolvedValue(undefined)
|
||||
// Default: no covering period at all. The closed-period test overrides this.
|
||||
mockCheckPeriodLock.mockResolvedValue({ locked: false, reason: 'no_fiscal_period' })
|
||||
})
|
||||
|
||||
it('delegates the CAS-race orphan to engine-backed storno compensation', async () => {
|
||||
@@ -228,35 +237,6 @@ describe('POST /api/transactions/[id]/categorize', () => {
|
||||
).toHaveLength(2)
|
||||
})
|
||||
|
||||
it('returns a race conflict when the guarded update matches no row without creating an entry', async () => {
|
||||
const tx = makeTransaction({
|
||||
id: 'tx-1',
|
||||
amount: -500,
|
||||
merchant_name: null,
|
||||
journal_entry_id: null,
|
||||
})
|
||||
|
||||
enqueue({ data: tx, error: null })
|
||||
enqueue({ data: { entity_type: 'enskild_firma', fiscal_year_start_month: 1 }, error: null })
|
||||
enqueue({ data: [], error: null }) // resolveSettlementAccount: no enabled cash accounts -> 1930
|
||||
enqueue({ data: [{ id: 'period-1' }], error: null })
|
||||
mockCreateTransactionJournalEntry.mockResolvedValueOnce(null)
|
||||
enqueue({ data: [], error: null })
|
||||
|
||||
const response = await POST(
|
||||
createMockRequest('/api/transactions/tx-1/categorize', {
|
||||
method: 'POST',
|
||||
body: { is_business: false },
|
||||
}),
|
||||
createMockRouteParams({ id: 'tx-1' }),
|
||||
)
|
||||
const { status, body } = await parseJsonResponse<{ error: { code: string } }>(response)
|
||||
|
||||
expect(status).toBe(409)
|
||||
expect(body.error.code).toBe('TX_CATEGORIZE_RACE')
|
||||
expect(mockReverseOrphanedJournalEntry).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('creates journal entry for business expense', async () => {
|
||||
const tx = makeTransaction({
|
||||
id: 'tx-1',
|
||||
@@ -574,80 +554,133 @@ describe('POST /api/transactions/[id]/categorize', () => {
|
||||
expect(mockSupabase.from).not.toHaveBeenCalledWith('document_attachments')
|
||||
})
|
||||
|
||||
it('returns success with error when journal entry creation fails (non-blocking)', async () => {
|
||||
const tx = makeTransaction({
|
||||
id: 'tx-1',
|
||||
amount: -500,
|
||||
merchant_name: 'Test',
|
||||
journal_entry_id: null,
|
||||
describe('fails closed when the verifikat cannot be created (issue #1947)', () => {
|
||||
// Queue order up to the engine call: tx fetch, settings, settlement
|
||||
// accounts, ensureFiscalPeriod. Nothing after that: a refused verifikat
|
||||
// must not reach the transactions update.
|
||||
const enqueueUpToEngine = () => {
|
||||
const tx = makeTransaction({
|
||||
id: 'tx-1',
|
||||
amount: -500,
|
||||
merchant_name: 'Test',
|
||||
journal_entry_id: null,
|
||||
})
|
||||
enqueue({ data: tx, error: null })
|
||||
enqueue({ data: { entity_type: 'enskild_firma', fiscal_year_start_month: 1 }, error: null })
|
||||
enqueue({ data: [], error: null }) // resolveSettlementAccount: no enabled cash accounts -> 1930
|
||||
enqueue({ data: [{ id: 'period-1' }], error: null })
|
||||
}
|
||||
|
||||
const categorize = async () => {
|
||||
const request = createMockRequest('/api/transactions/tx-1/categorize', {
|
||||
method: 'POST',
|
||||
body: { is_business: true, category: 'expense_software' },
|
||||
})
|
||||
const response = await POST(request, createMockRouteParams({ id: 'tx-1' }))
|
||||
return parseJsonResponse<{
|
||||
error: {
|
||||
code: string
|
||||
message: string
|
||||
message_en?: string
|
||||
details?: { cause?: string; reason?: string; fiscal_period_id?: string }
|
||||
}
|
||||
}>(response)
|
||||
}
|
||||
|
||||
it('refuses the booking and leaves the row untouched when the period is locked', async () => {
|
||||
enqueueUpToEngine()
|
||||
mockCreateTransactionJournalEntry.mockRejectedValue(
|
||||
new BookkeepingDatabaseError('commit_entry', 'Cannot write to locked/closed fiscal period "2025"'),
|
||||
)
|
||||
|
||||
const { status, body } = await categorize()
|
||||
|
||||
expect(status).toBe(409)
|
||||
expect(body.error.code).toBe('TX_CATEGORIZE_JOURNAL_ENTRY_FAILED')
|
||||
expect(body.error.message).toBe(
|
||||
'Perioden är låst. Verifikationen kan inte skapas i en stängd eller låst period.',
|
||||
)
|
||||
// The sv/en pair is derived from the SAME underlying error, per the
|
||||
// errorResponseFromCode contract (provide both or neither): the English
|
||||
// side must not stay on the generic registry text while the Swedish
|
||||
// side names the period lock, and must never carry envelope-field prose.
|
||||
expect(body.error.message_en).toBe('Bookkeeping database operation failed.')
|
||||
expect(body.error.message_en).not.toContain('details.cause')
|
||||
expect(body.error.details?.cause).toBe('BOOKKEEPING_DATABASE_ERROR')
|
||||
// Nothing persisted: is_business/category stay NULL so the row keeps
|
||||
// matching the worklist predicate and stays in "Att bokföra".
|
||||
expect(findCalls('transactions', 'update')).toEqual([])
|
||||
expect(mockSaveUserMappingRule).not.toHaveBeenCalled()
|
||||
expect(mockReverseOrphanedJournalEntry).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
enqueue({ data: tx, error: null })
|
||||
enqueue({ data: { entity_type: 'enskild_firma', fiscal_year_start_month: 1 }, error: null })
|
||||
enqueue({ data: [], error: null }) // resolveSettlementAccount: no enabled cash accounts -> 1930
|
||||
enqueue({ data: [{ id: 'period-1' }], error: null })
|
||||
it('translates typed engine errors to Swedish in the refusal (issue #337)', async () => {
|
||||
enqueueUpToEngine()
|
||||
mockCreateTransactionJournalEntry.mockRejectedValue(new JournalEntryNotBalancedError(100, 80))
|
||||
|
||||
mockCreateTransactionJournalEntry.mockRejectedValue(new Error('Period locked'))
|
||||
const { status, body } = await categorize()
|
||||
|
||||
// Update transaction
|
||||
enqueue({ data: [{ ...tx, is_business: true, category: 'expense_software' }], error: null })
|
||||
|
||||
const request = createMockRequest('/api/transactions/tx-1/categorize', {
|
||||
method: 'POST',
|
||||
body: { is_business: true, category: 'expense_software' },
|
||||
})
|
||||
const response = await POST(request, createMockRouteParams({ id: 'tx-1' }))
|
||||
const { status, body } = await parseJsonResponse<{
|
||||
success: boolean
|
||||
journal_entry_created: boolean
|
||||
journal_entry_error: string
|
||||
}>(response)
|
||||
|
||||
expect(status).toBe(200)
|
||||
expect(body.success).toBe(true)
|
||||
expect(body.journal_entry_created).toBe(false)
|
||||
// Untyped errors no longer leak their raw English message (issue #337):
|
||||
// they map to the Swedish transaction-context fallback.
|
||||
expect(body.journal_entry_error).toBe('Kunde inte hantera transaktionen. Försök igen.')
|
||||
})
|
||||
|
||||
it('translates typed engine errors to Swedish in journal_entry_error (issue #337)', async () => {
|
||||
const tx = makeTransaction({
|
||||
id: 'tx-1',
|
||||
amount: -500,
|
||||
merchant_name: 'Test',
|
||||
journal_entry_id: null,
|
||||
expect(status).toBe(409)
|
||||
expect(body.error.code).toBe('TX_CATEGORIZE_JOURNAL_ENTRY_FAILED')
|
||||
expect(body.error.message).toContain('balanserar inte')
|
||||
expect(body.error.message).toMatch(/100/)
|
||||
expect(body.error.message).toMatch(/80/)
|
||||
expect(body.error.message).not.toContain('not balanced')
|
||||
expect(body.error.message).not.toContain('check constraint')
|
||||
expect(findCalls('transactions', 'update')).toEqual([])
|
||||
})
|
||||
|
||||
enqueue({ data: tx, error: null })
|
||||
enqueue({ data: { entity_type: 'enskild_firma', fiscal_year_start_month: 1 }, error: null })
|
||||
enqueue({ data: [], error: null }) // resolveSettlementAccount: no enabled cash accounts -> 1930
|
||||
enqueue({ data: [{ id: 'period-1' }], error: null })
|
||||
it('maps untyped errors to the Swedish transaction fallback without leaking the raw text', async () => {
|
||||
enqueueUpToEngine()
|
||||
mockCreateTransactionJournalEntry.mockRejectedValue(new Error('boom'))
|
||||
|
||||
mockCreateTransactionJournalEntry.mockRejectedValue(new JournalEntryNotBalancedError(100, 80))
|
||||
const { status, body } = await categorize()
|
||||
|
||||
// Update transaction
|
||||
enqueue({ data: [{ ...tx, is_business: true, category: 'expense_software' }], error: null })
|
||||
|
||||
const request = createMockRequest('/api/transactions/tx-1/categorize', {
|
||||
method: 'POST',
|
||||
body: { is_business: true, category: 'expense_software' },
|
||||
expect(status).toBe(409)
|
||||
expect(body.error.code).toBe('TX_CATEGORIZE_JOURNAL_ENTRY_FAILED')
|
||||
expect(body.error.message).toBe('Kunde inte hantera transaktionen. Försök igen.')
|
||||
expect(body.error.message).not.toContain('boom')
|
||||
expect(findCalls('transactions', 'update')).toEqual([])
|
||||
})
|
||||
const response = await POST(request, createMockRouteParams({ id: 'tx-1' }))
|
||||
const { status, body } = await parseJsonResponse<{
|
||||
success: boolean
|
||||
journal_entry_created: boolean
|
||||
journal_entry_error: string
|
||||
}>(response)
|
||||
|
||||
expect(status).toBe(200)
|
||||
expect(body.success).toBe(true)
|
||||
expect(body.journal_entry_created).toBe(false)
|
||||
expect(body.journal_entry_error).toContain('balanserar inte')
|
||||
expect(body.journal_entry_error).toMatch(/100/)
|
||||
expect(body.journal_entry_error).toMatch(/80/)
|
||||
expect(body.journal_entry_error).not.toContain('not balanced')
|
||||
expect(body.journal_entry_error).not.toContain('check constraint')
|
||||
it('returns NO_OPEN_PERIOD_FOR_DATE when the engine finds no covering period (null entry)', async () => {
|
||||
enqueueUpToEngine()
|
||||
mockCreateTransactionJournalEntry.mockResolvedValue(null)
|
||||
mockCheckPeriodLock.mockResolvedValue({ locked: false, reason: 'no_fiscal_period' })
|
||||
|
||||
const { status, body } = await categorize()
|
||||
|
||||
expect(status).toBe(400)
|
||||
expect(body.error.code).toBe('NO_OPEN_PERIOD_FOR_DATE')
|
||||
expect(body.error.details?.reason).toBe('no_fiscal_period')
|
||||
// Refused before the CAS write: no update, no orphan, so no storno.
|
||||
expect(findCalls('transactions', 'update')).toEqual([])
|
||||
expect(mockSaveUserMappingRule).not.toHaveBeenCalled()
|
||||
expect(mockReverseOrphanedJournalEntry).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('returns PERIOD_LOCKED (reason period_is_closed) when the covering year is closed', async () => {
|
||||
// findFiscalPeriod filters is_closed = false, so a klarmarkerad year
|
||||
// also surfaces as the engine's null return. The route must not claim
|
||||
// the räkenskapsår does not exist when it exists and is closed.
|
||||
enqueueUpToEngine()
|
||||
mockCreateTransactionJournalEntry.mockResolvedValue(null)
|
||||
mockCheckPeriodLock.mockResolvedValue({
|
||||
locked: true,
|
||||
reason: 'period_is_closed',
|
||||
fiscal_period_id: 'fp-2024',
|
||||
})
|
||||
|
||||
const { status, body } = await categorize()
|
||||
|
||||
expect(status).toBe(400)
|
||||
expect(body.error.code).toBe('PERIOD_LOCKED')
|
||||
expect(body.error.details?.reason).toBe('period_is_closed')
|
||||
expect(body.error.details?.fiscal_period_id).toBe('fp-2024')
|
||||
expect(findCalls('transactions', 'update')).toEqual([])
|
||||
expect(mockSaveUserMappingRule).not.toHaveBeenCalled()
|
||||
expect(mockReverseOrphanedJournalEntry).not.toHaveBeenCalled()
|
||||
})
|
||||
})
|
||||
|
||||
it('returns 500 when transaction update fails', async () => {
|
||||
|
||||
@@ -7,13 +7,14 @@ import { getTemplateById, buildMappingResultFromTemplate, validateTemplateForEnt
|
||||
import { createTransactionJournalEntry } from '@/lib/bookkeeping/transaction-entries'
|
||||
import { reverseOrphanedJournalEntry } from '@/lib/bookkeeping/cancel-orphaned-entry'
|
||||
import { getEarliestFiscalPeriodStart } from '@/lib/core/bookkeeping/period-service'
|
||||
import { checkPeriodLock } from '@/lib/api/v1/check-period-lock'
|
||||
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'
|
||||
import { errorResponse, errorResponseFromCode, getStructuredError } from '@/lib/errors/get-structured-error'
|
||||
import {
|
||||
DUPLICATE_AMOUNT_TOLERANCE_PCT,
|
||||
DUPLICATE_DATE_WINDOW_DAYS,
|
||||
@@ -782,7 +783,6 @@ export const POST = withRouteContext(
|
||||
|
||||
let journalEntryCreated = false
|
||||
let journalEntryId: string | null = null
|
||||
let journalEntryError: string | null = null
|
||||
let documentLinkWarning: string | null = null
|
||||
|
||||
try {
|
||||
@@ -809,13 +809,53 @@ export const POST = withRouteContext(
|
||||
if (err instanceof AccountsNotInChartError) {
|
||||
return accountsNotInChartResponse(err)
|
||||
}
|
||||
// All errors map to Swedish via getErrorMessage: the raw message is
|
||||
// already logged above and must never reach the user verbatim (issue
|
||||
// #337). The categorization is preserved either way so the user can
|
||||
// still re-book the verifikation manually.
|
||||
journalEntryError = getErrorMessage(err, { context: 'transaction' })
|
||||
// Fail closed (issue #1947): the verifikat IS the booking. Writing
|
||||
// is_business/category without one used to drop the row out of the
|
||||
// canonical worklist predicate in lib/worklist/types.ts (is_business IS
|
||||
// NULL) while it was still unbooked, so it vanished from "Att bokföra"
|
||||
// and the nav badge with no reminder to finish it. Nothing is persisted
|
||||
// when the entry fails: the row stays uncategorized and visible. Both
|
||||
// message locales come from getErrorMessage (sv/en from the same error,
|
||||
// per the errorResponseFromCode contract: provide both or neither); the
|
||||
// raw message is already logged above and must never reach the user
|
||||
// verbatim (issue #337).
|
||||
const structured = getStructuredError(err)
|
||||
return errorResponseFromCode('TX_CATEGORIZE_JOURNAL_ENTRY_FAILED', txLog, {
|
||||
requestId,
|
||||
messageSv: getErrorMessage(err, { context: 'transaction' }),
|
||||
messageEn: getErrorMessage(err, { context: 'transaction', locale: 'en' }),
|
||||
details: { cause: structured.code },
|
||||
})
|
||||
}
|
||||
|
||||
// createTransactionJournalEntry returns null (no throw) when
|
||||
// findFiscalPeriod sees no OPEN period covering the date and the pre-FY
|
||||
// clamp does not apply: either no fiscal period exists there at all, or
|
||||
// the covering period exists but is closed (is_closed = true; findFiscalPeriod
|
||||
// filters is_closed = false). Same fail-closed rule either way: refuse
|
||||
// rather than mark the row categorized-but-unbooked. checkPeriodLock tells
|
||||
// the two apart so a closed year answers PERIOD_LOCKED (reason
|
||||
// period_is_closed) instead of claiming the räkenskapsår does not exist.
|
||||
if (!journalEntryId) {
|
||||
const periodLock = await checkPeriodLock(supabase, companyId, transaction.date)
|
||||
if (periodLock.locked) {
|
||||
return errorResponseFromCode('PERIOD_LOCKED', txLog, {
|
||||
requestId,
|
||||
details: {
|
||||
transaction_date: transaction.date,
|
||||
reason: periodLock.reason,
|
||||
fiscal_period_id: periodLock.fiscal_period_id,
|
||||
},
|
||||
})
|
||||
}
|
||||
return errorResponseFromCode('NO_OPEN_PERIOD_FOR_DATE', txLog, {
|
||||
requestId,
|
||||
details: { transaction_date: transaction.date, reason: 'no_fiscal_period' },
|
||||
})
|
||||
}
|
||||
|
||||
// Learning writes (mapping rule, counterparty template) run only after a
|
||||
// posted verifikat, so they never learn from a booking that did not happen.
|
||||
// direction_mismatch = a mirrored refund/repayment booking; learning it
|
||||
// as a rule would store backwards accounts for the merchant.
|
||||
if (is_business && transaction.merchant_name && !mappingResult.direction_mismatch) {
|
||||
@@ -1027,21 +1067,13 @@ export const POST = withRouteContext(
|
||||
},
|
||||
})
|
||||
|
||||
if (journalEntryError) {
|
||||
// Categorization stuck but the verifikation didn't make it through.
|
||||
// Surface as a structured warning: the response below carries the
|
||||
// user-facing message in `journal_entry_error`.
|
||||
txLog.warn('partial outcome: journal entry creation failed', {
|
||||
reason: 'journal_entry_creation_failed',
|
||||
message: journalEntryError,
|
||||
})
|
||||
}
|
||||
|
||||
// journal_entry_error is always null here: a failed verifikat now returns
|
||||
// a typed 409 above (issue #1947). The field stays for client compatibility.
|
||||
return NextResponse.json({
|
||||
success: true,
|
||||
journal_entry_created: journalEntryCreated,
|
||||
journal_entry_id: journalEntryId,
|
||||
journal_entry_error: journalEntryError,
|
||||
journal_entry_error: null,
|
||||
document_link_warning: documentLinkWarning,
|
||||
category: finalCategory,
|
||||
})
|
||||
|
||||
Reference in New Issue
Block a user