fix(transactions): allow re-linking a bank tx stranded on a reversed verifikat (#988) (#1009)

* fix(transactions): allow re-linking a bank tx stranded on a reversed verifikat (#988) A transaction whose journal_entry_id points at a reversed/cancelled entry reads as "utan koppling" in the UI (the transactions page enriches only status='posted' links), yet the re-booking guards treated ANY non-null pointer as "already linked". So a storno'd/corrected transaction could never be linked to another verifikat or re-categorized: the exact symptom in issue #988. Add a shared hasLiveJournalEntryLink() predicate used by every re-booking guard (linkTransactionToJournalEntry, manualLink, categorize-core, and the MCP link stage-check): a pointer at a non-posted entry is treated as re-linkable, and the two optimistic-locked writes now lock on the exact previous pointer (null OR the stale id) instead of always .is(null), so the overwrite goes through race-safely. hasLiveJournalEntryLink fails closed on a read error so a transient blip can't detach a genuinely live link. The source was fixed in #726 (reverseEntry/correctEntry now detach/re-point the tx); this makes the guards self-heal for the pre-#726 backlog and any future best-effort miss. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(transactions): detect 0-row CAS before invoice effects; fix categorize commit test Addresses PR review (CodeRabbit Critical + CI): - link-journal-entry.ts: the tx UPDATE now .select('id') and treats a 0-row result as LINK_TX_TX_ALREADY_LINKED, failing BEFORE any invoice settlement / invoice_payments insert. Without this, a concurrent re-link that lost the CAS would still mark the invoice paid against a transaction we didn't link (same optimistic-lock contract manualLink already enforces). - pending-operations commit route test: the categorize_transaction "already categorized" case now enqueues the hasLiveJournalEntryLink status read (posted = live) so it still returns 409. This was the core-only CI failure: the new liveness read in categorize-core consumed a queued response. - Updated the link happy-path / invoice-race test enqueues to return a row for the now-selecting tx UPDATE. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2026-07-12 22:43:02 +02:00
parent dcd33997b7
commit f04dc4c4e0
9 changed files with 249 additions and 26 deletions
@@ -0,0 +1,54 @@
+/**
+ * Unit tests for hasLiveJournalEntryLink.
+ *
+ * This is the predicate the re-booking guards (linkTransactionToJournalEntry,
+ * manualLink, categorize-core, the MCP stage-check) share to decide whether a
+ * transaction's journal_entry_id is a LIVE link that should block re-linking,
+ * or a stale pointer at a reversed/cancelled entry that the UI already shows as
+ * "utan koppling" and must stay re-linkable (issue #988).
+ *
+ * The end-to-end re-link behaviour is covered by the route test
+ * (app/api/transactions/[id]/link-journal-entry/__tests__/route.test.ts) and
+ * the pending-op commit test.
+ */
+import { describe, it, expect } from 'vitest'
+import { createQueuedMockSupabase } from '@/tests/helpers'
+import { hasLiveJournalEntryLink } from '../link-journal-entry'
+
+describe('hasLiveJournalEntryLink', () => {
+  it('returns false for a null/undefined pointer without querying', async () => {
+    const { supabase } = createQueuedMockSupabase()
+    expect(await hasLiveJournalEntryLink(supabase as never, 'company-1', null)).toBe(false)
+    expect(await hasLiveJournalEntryLink(supabase as never, 'company-1', undefined)).toBe(false)
+  })
+
+  it('returns true when the entry is posted (a live link)', async () => {
+    const { supabase, enqueue } = createQueuedMockSupabase()
+    enqueue({ data: { status: 'posted' }, error: null })
+    expect(await hasLiveJournalEntryLink(supabase as never, 'company-1', 'je-1')).toBe(true)
+  })
+
+  it('returns false when the entry is reversed (stale link, #988)', async () => {
+    const { supabase, enqueue } = createQueuedMockSupabase()
+    enqueue({ data: { status: 'reversed' }, error: null })
+    expect(await hasLiveJournalEntryLink(supabase as never, 'company-1', 'je-1')).toBe(false)
+  })
+
+  it('returns false when the entry is cancelled', async () => {
+    const { supabase, enqueue } = createQueuedMockSupabase()
+    enqueue({ data: { status: 'cancelled' }, error: null })
+    expect(await hasLiveJournalEntryLink(supabase as never, 'company-1', 'je-1')).toBe(false)
+  })
+
+  it('returns false when the referenced entry row is missing', async () => {
+    const { supabase, enqueue } = createQueuedMockSupabase()
+    enqueue({ data: null, error: null })
+    expect(await hasLiveJournalEntryLink(supabase as never, 'company-1', 'je-gone')).toBe(false)
+  })
+
+  it('fails closed (returns true) on a read error so a live link is never clobbered', async () => {
+    const { supabase, enqueue } = createQueuedMockSupabase()
+    enqueue({ data: null, error: { message: 'statement timeout' } })
+    expect(await hasLiveJournalEntryLink(supabase as never, 'company-1', 'je-1')).toBe(true)
+  })
+})
@@ -29,6 +29,7 @@ import { upsertCounterpartyTemplate } from '@/lib/bookkeeping/counterparty-templ
 import { isBookkeepingError } from '@/lib/bookkeeping/errors'
 import { linkToJournalEntry } from '@/lib/core/documents/document-service'
 import { detectBookingDuplicate, type BookingDuplicateExclusions } from '@/lib/transactions/booking-duplicate-detection'
+import { hasLiveJournalEntryLink } from '@/lib/transactions/link-journal-entry'
 import { appendProcessingHistory } from '@/lib/processing-history/append'
 import { roundOre } from '@/lib/money'
 import { createLogger } from '@/lib/logger'
@@ -165,7 +166,17 @@ export async function categorizeMatchedTransaction(
  if (fetchError || !transaction) {
    return { error: 'Transaction not found: it may have been deleted.', status: 404 }
  }
-  if (transaction.journal_entry_id) {
+  // A stale pointer at a 'reversed' entry (storno/correction left it behind)
+  // must not block re-categorization: the row reads as "utan koppling" in the
+  // UI, so a fresh booking has to be allowed (issue #988). Only a live posted
+  // link means it was genuinely categorized in the meantime. The UPDATE below
+  // is unconditional (no null-lock), so it overwrites the stale pointer; the
+  // duplicate guard still catches an existing live correction and steers the
+  // user to link instead.
+  if (
+    transaction.journal_entry_id &&
+    (await hasLiveJournalEntryLink(supabase, companyId, transaction.journal_entry_id))
+  ) {
    return { error: 'Transaction already has a journal entry: it was categorized in the meantime.', status: 409 }
  }

@@ -79,6 +79,43 @@ export function formatVoucherLabel(
  return num === '' ? series : `${series}-${num}`
 }

+/**
+ * Is `journalEntryId` a LIVE link, i.e. does it reference a posted verifikat?
+ *
+ * A transaction can carry a non-null `journal_entry_id` that no longer points
+ * at a live booking: reversing (storno) or correcting an entry marks the
+ * original `reversed`, and while both flows try to detach or re-point the
+ * transaction (engine.ts `reverseEntry`, storno-service `relinkTransactions-
+ * ToEntry`), those re-links are best-effort and rows reversed before #726
+ * (2026-06-15) were never touched at all. Such a transaction reads as "utan
+ * koppling" in the UI: the transactions page enriches only `status='posted'`
+ * links, so a reversed pointer renders as no link, yet the raw column is still
+ * set.
+ *
+ * The "already linked" guards on the re-booking paths must mirror that same
+ * posted-only predicate. If they treat any non-null pointer as linked, a
+ * transaction the UI shows as free can never be re-linked or re-categorized
+ * (issue #988). Returns true ONLY when the pointer references a posted entry;
+ * null / missing / reversed / cancelled / draft all count as no live link, so
+ * the caller may overwrite the stale pointer. Fails closed (returns true) on a
+ * read error so a transient lookup blip can never detach a genuinely live link.
+ */
+export async function hasLiveJournalEntryLink(
+  supabase: SupabaseClient,
+  companyId: string,
+  journalEntryId: string | null | undefined,
+): Promise<boolean> {
+  if (!journalEntryId) return false
+  const { data, error } = await supabase
+    .from('journal_entries')
+    .select('status')
+    .eq('id', journalEntryId)
+    .eq('company_id', companyId)
+    .maybeSingle()
+  if (error) return true
+  return data?.status === 'posted'
+}
+
 export async function linkTransactionToJournalEntry(
  supabase: SupabaseClient,
  userId: string,
@@ -103,7 +140,15 @@ export async function linkTransactionToJournalEntry(
    return { ok: false, code: 'TX_CATEGORIZE_TX_NOT_FOUND' }
  }

-  if (transaction.journal_entry_id) {
+  // Only a LIVE (posted) pointer blocks re-linking. A pointer left behind by a
+  // storno/correction references a 'reversed' entry: the UI already shows the
+  // row as "utan koppling", so the guard must agree and let the user re-link it
+  // to another verifikat (issue #988). The stale pointer is overwritten by the
+  // optimistic-locked UPDATE below.
+  if (
+    transaction.journal_entry_id &&
+    (await hasLiveJournalEntryLink(supabase, companyId, transaction.journal_entry_id as string))
+  ) {
    return {
      ok: false,
      code: 'LINK_TX_TX_ALREADY_LINKED',
@@ -211,14 +256,20 @@ export async function linkTransactionToJournalEntry(
  // if a subsequent step fails: otherwise a partial state would persist
  // (tx linked, invoice unchanged, no payment row).
  const priorTxState = {
-    journal_entry_id: transaction.journal_entry_id, // validated null above
+    // null, or a stale 'reversed'-entry id we're clearing (validated not-live above)
+    journal_entry_id: transaction.journal_entry_id,
    invoice_id: transaction.invoice_id,
    potential_invoice_id: transaction.potential_invoice_id,
    potential_supplier_invoice_id: transaction.potential_supplier_invoice_id,
    is_business: transaction.is_business,
  }

-  const { error: updateTxError } = await supabase
+  // Optimistic lock on the pointer we validated: null for a free row, or the
+  // exact stale id for one we're detaching from a reversed entry. Locking on
+  // the known value (rather than always .is(null)) lets the stale-pointer
+  // overwrite through while still turning a concurrent re-link into a no-op.
+  const previousJournalEntryId = (transaction.journal_entry_id as string | null) ?? null
+  const txUpdate = supabase
    .from('transactions')
    .update({
      journal_entry_id: journalEntryId,
@@ -229,11 +280,26 @@ export async function linkTransactionToJournalEntry(
    })
    .eq('id', transactionId)
    .eq('company_id', companyId)
-    .is('journal_entry_id', null)
+  const { data: updatedTxRows, error: updateTxError } = await (previousJournalEntryId === null
+    ? txUpdate.is('journal_entry_id', null)
+    : txUpdate.eq('journal_entry_id', previousJournalEntryId)
+  ).select('id')

  if (updateTxError) {
    return { ok: false, code: 'LINK_TX_DB_ERROR', details: { reason: updateTxError.message } }
  }
+  // CAS lost: a concurrent linker changed the pointer between the liveness
+  // check and this write, so 0 rows matched. Fail BEFORE any invoice side
+  // effects: otherwise we'd settle the invoice + insert an invoice_payments row
+  // for a transaction we didn't actually link (same optimistic-lock contract as
+  // manualLink in lib/reconciliation/bank-reconciliation.ts).
+  if (!updatedTxRows || updatedTxRows.length === 0) {
+    return {
+      ok: false,
+      code: 'LINK_TX_TX_ALREADY_LINKED',
+      details: { existingJournalEntryId: previousJournalEntryId },
+    }
+  }

  async function rollbackTxLink(reason: string): Promise<void> {
    // SOC 2 PI1.3 (processing integrity): if a rollback itself fails, the