admin/accounted
3
0
Fork 0
Code Issues 1 Pull Requests Actions Packages Projects Releases Wiki Activity

fix(invoices): roll back the header row when a recurring-schedule item replace fails (#1312)

Browse Source 
* fix(invoices): roll back the header row when a recurring-schedule item replace fails

PATCH /api/invoices/recurring/[id] and the update_recurring_schedule commit
executor wrote the schedule header first, then replaced the items. An item
insert failure restored the items snapshot but left the header update
committed, so a combined edit half-applied: a new day_of_month or
default_dimensions stayed while the line edit was undone.

Both write paths now go through one shared helper,
lib/invoices/apply-recurring-schedule-update.ts, which snapshots the header
before writing it (only for a combined edit, the only case with something to
undo) and compensates it on any items failure. The rollback update is filtered
on the updated_at stamp our own write produced, so a concurrent writer (the
hourly cron, a second edit) wins instead of being clobbered from a stale
snapshot: audit finding C2 in lib/invoices/voucher-matching.ts.

A compensation that itself fails is no longer swallowed. The helper reports
itemsRestored / headerRestored, logs the unrecoverable rows and the intended
restore payload, and both call sites then return the new
INVOICE_RECURRING_UPDATE_PARTIAL registry entry, which tells the user in
Swedish that the schedule may be half-saved and to check fields and items
before retrying. A clean rollback keeps the PG-mapped error so a CHECK
violation still surfaces its specific message.

Also in the rewritten block:
- the items DELETE error is checked, so a failed delete no longer proceeds to
  an insert that would duplicate every line;
- the 404 existence check moved above every write, so a PATCH with items for a
  missing or cross-tenant id writes nothing;
- the items snapshot uses select('*') with id/created_at stripped on restore
  (same idiom as replaceInvoiceItems), so a column added later is carried
  through instead of silently dropped;
- NewRecurringScheduleDialog unwraps the nested { error: { message } } envelope
  the route returns, which otherwise reached the toast as "[object Object]".

The cron's no-empty-items invariant holds on every failure path: the items are
either untouched, restored, or the failure is reported explicitly.

Fixes #1275

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(invoices): never write when the compensating snapshot is unavailable

Follow-up on the recurring-schedule rollback: the helper still performed two
writes it already knew it could not compensate.

- The header snapshot read now checks its error and a missing row, and the
  header UPDATE is skipped entirely when either holds, so no header change is
  committed that we already know can never be rolled back.
- An unreadable item snapshot now aborts BEFORE the delete (rolling the header
  back) instead of deleting first and reporting itemsRestored: false, so the
  cron invariant "a schedule always has items" holds on every failure path.
- That header read now runs whenever items are replaced and is scoped by
  company_id, so it doubles as the ownership proof the schedule_id-only item
  delete/insert lacks (the commit executor runs with RLS off). Stated in the
  JSDoc as well.
- The item snapshot is paginated via fetchAllRows: a schedule with more than
  1000 lines could otherwise restore partially while reporting a clean
  rollback.
- The executor now returns errorCode INVOICE_RECURRING_UPDATE_PARTIAL,
  surfaced as CommitResult.code and persisted as result_data.error_code, so a
  staged-op caller can detect the partial state without substring-matching the
  Swedish sentence.
- Route: details keys are camelCase throughout, and an item failure is logged
  once, with the repair context kept on the partial path only.

Tests: the unreadable-snapshot branches are exercised (including the
previously unused itemsSnapshotError harness hook), and the test that pinned
"header written with no possibility of rollback" now asserts that nothing is
written at all.

Fixes #1275

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Jakob Wennberg
2026-07-30 18:50:56 +02:00
committed by GitHub
co-authored by Claude Opus 5
parent 4a38fa30ed
commit f3bf50d862
8 changed files with 1234 additions and 144 deletions
Download Patch File Download Diff File Expand all files Collapse all files
lib/pending-operations/__tests__/recurring-schedule-executors.test.ts
+95 -2
View File
@@ -206,11 +206,16 @@ describe('commitPendingOperation: update_recurring_schedule', () => {
const { supabase, enqueue } = createQueuedMockSupabase()
enqueue({ data: { id: 'op-recurring-1' } }) // claim
enqueue({ data: existingRow }) // existing schedule
// The helper re-reads the header row scoped by company_id before touching
// the items: the items table has no company_id of its own, so this read is
// what keeps the schedule_id-scoped delete/insert inside the tenant on the
// service-role (RLS-off) executor path.
enqueue({ data: existingRow }) // header ownership read
enqueue({
data: [
{ sort_order: 0, description: 'Old', quantity: 1, unit: 'st', unit_price: 100, vat_rate: null },
],
}) // snapshot
}) // items snapshot
enqueue({ data: null }) // delete old items
enqueue({ data: null }) // insert new items
enqueue({ data: null }) // finalize
@@ -236,9 +241,10 @@ describe('commitPendingOperation: update_recurring_schedule', () => {
items_replaced: true,
item_count: 2,
})
expect(supabase.from).toHaveBeenNthCalledWith(3, 'recurring_invoice_schedule_items')
expect(supabase.from).toHaveBeenNthCalledWith(3, 'recurring_invoice_schedules')
expect(supabase.from).toHaveBeenNthCalledWith(4, 'recurring_invoice_schedule_items')
expect(supabase.from).toHaveBeenNthCalledWith(5, 'recurring_invoice_schedule_items')
expect(supabase.from).toHaveBeenNthCalledWith(6, 'recurring_invoice_schedule_items')
})
it('keeps existing items when items are omitted', async () => {
@@ -384,6 +390,93 @@ describe('commitPendingOperation: update_recurring_schedule', () => {
expect(result.http_status).toBe(404)
})
/**
* A combined edit writes the header and then replaces the items, which
* PostgREST cannot do atomically. Issue #1275: an item-insert failure used to
* leave the header update committed, so the edit half-applied.
*/
const fullExistingRow = {
...existingRow,
name: 'Månadsavgift',
updated_at: '2026-07-01T00:00:00Z',
}
const snapshotItem = {
id: 'item-old-1',
created_at: '2026-07-01T00:00:00Z',
schedule_id: SCHEDULE_ID,
sort_order: 0,
description: 'Old',
quantity: 1,
unit: 'st',
unit_price: 100,
vat_rate: null,
dimensions: {},
}
const combinedChanges = {
schedule_id: SCHEDULE_ID,
changes: {
name: 'Nytt namn',
items: [{ description: 'Ny rad', quantity: 2, unit: 'tim', unit_price: 1200 }],
},
}
it('restores the prior header field when the items insert fails on a combined edit', async () => {
const { supabase, enqueue, findCalls } = createQueuedMockSupabase()
enqueue({ data: { id: 'op-recurring-1' } }) // claim
enqueue({ data: fullExistingRow }) // existing schedule
enqueue({ data: fullExistingRow }) // header snapshot
enqueue({ data: { updated_at: '2026-07-30T09:00:00.000Z' } }) // header update
enqueue({ data: [snapshotItem] }) // items snapshot
enqueue({ data: null }) // delete old items
enqueue({ error: { message: 'items insert failed', code: '23514' } }) // items insert
enqueue({ data: null }) // items restore insert
enqueue({ data: [{ id: SCHEDULE_ID }] }) // header restore update
enqueue({ data: null }) // finalize
const result = await commitPendingOperation(
supabase as never,
'user-1',
'company-1',
makePendingOp('update_recurring_schedule', combinedChanges),
)
expect(result.status).toBe('failed')
expect(result.http_status).toBe(500)
const updates = findCalls('recurring_invoice_schedules', 'update')
expect(updates).toHaveLength(2)
// The rollback puts the prior name back rather than leaving it half-saved.
expect(updates[1][0]).toEqual({ name: 'Månadsavgift' })
})
it('reports the partial-state message when the items restore also fails', async () => {
const { supabase, enqueue } = createQueuedMockSupabase()
enqueue({ data: { id: 'op-recurring-1' } }) // claim
enqueue({ data: fullExistingRow }) // existing schedule
enqueue({ data: fullExistingRow }) // header snapshot
enqueue({ data: { updated_at: '2026-07-30T09:00:00.000Z' } }) // header update
enqueue({ data: [snapshotItem] }) // items snapshot
enqueue({ data: null }) // delete old items
enqueue({ error: { message: 'items insert failed', code: '23514' } }) // items insert
enqueue({ error: { message: 'restore boom' } }) // items restore insert fails
enqueue({ data: [{ id: SCHEDULE_ID }] }) // header restore update
enqueue({ data: null }) // finalize
const result = await commitPendingOperation(
supabase as never,
'user-1',
'company-1',
makePendingOp('update_recurring_schedule', combinedChanges),
)
expect(result.status).toBe('failed')
expect(result.http_status).toBe(500)
// Same registry sentence the PATCH route returns.
expect(result.error).toMatch(/halvsparat/)
// And the same machine-readable code, so an MCP caller can detect the
// partial state without substring-matching the Swedish prose.
expect(result.code).toBe('INVOICE_RECURRING_UPDATE_PARTIAL')
})
it('rejects tampered change fields at the commit boundary', async () => {
const { supabase, enqueue } = createQueuedMockSupabase()
enqueue({ data: { id: 'op-recurring-1' } }) // claim
lib/pending-operations/commit.ts
+49 -71
View File
@@ -125,6 +125,7 @@ import {
} from '@/lib/invoices/build-invoice-write'
import { isEditableInvoiceDraft } from '@/lib/invoices/is-editable-draft'
import { replaceInvoiceItems } from '@/lib/invoices/replace-invoice-items'
import { applyRecurringScheduleUpdate } from '@/lib/invoices/apply-recurring-schedule-update'
import { BulkBookInboxSchema } from '@/lib/api/schemas'
import { ensureArticleNumber } from '@/lib/articles/ensure-article-number'
import { isValidRevenueAccount } from '@/lib/articles/validate-revenue-account'
@@ -159,10 +160,13 @@ export interface CommitResult {
error?: string
http_status?: number
auto_rejected?: boolean
// Set when the commit failed because the booking posts to BAS accounts not
// active in the company chart. Recoverable: the op is left 'pending' so the
// caller can activate the accounts and retry. Lets the route rebuild the
// structured ACCOUNTS_NOT_IN_CHART envelope (code + account_numbers).
// Structured-error registry code for the failure, when one is known, so a
// caller can branch on the failure mode instead of parsing `error` text.
// ACCOUNTS_NOT_IN_CHART is the recoverable case: the booking posts to BAS
// accounts not active in the company chart, the op is left 'pending', and
// the route rebuilds the structured envelope (code + account_numbers).
// Other codes (e.g. INVOICE_RECURRING_UPDATE_PARTIAL) are informational:
// callers that do not recognize the code fall back to `error`.
code?: string
account_numbers?: string[]
}
@@ -238,6 +242,10 @@ async function recordSkippedInvoiceJournalEntry(
type ExecutorResult = {
data?: Record<string, unknown>
error?: string
// Structured-error registry code for `error`, when the executor has one.
// Surfaced as CommitResult.code and persisted in result_data.error_code so a
// caller can branch on the failure mode instead of parsing the message text.
errorCode?: string
status?: number
// Set when the executor already performed an irreversible side-effect
// (posted voucher, persisted credit note) before the failure in `error`:
@@ -691,75 +699,40 @@ async function commitUpdateRecurringSchedule(
}
}
if (Object.keys(updateRow).length > 0) {
const { error: updateError } = await supabase
.from('recurring_invoice_schedules')
.update(updateRow)
.eq('id', scheduleId)
.eq('company_id', companyId)
if (updateError) return { error: updateError.message, status: 500 }
}
let itemsReplaced = false
if (items) {
// Provided = replace all; omitted = keep existing (the schema contract).
// Snapshot first so a failed insert can restore the previous lines: an
// item-less schedule makes every cron run throw "schedule has no items"
// and silently skip billing dates.
const { data: previousItems } = await supabase
.from('recurring_invoice_schedule_items')
.select('sort_order, description, quantity, unit, unit_price, vat_rate, dimensions')
.eq('schedule_id', scheduleId)
await supabase
.from('recurring_invoice_schedule_items')
.delete()
.eq('schedule_id', scheduleId)
const itemRows = items.map((item, idx) => ({
schedule_id: scheduleId,
sort_order: idx,
description: item.description,
quantity: item.quantity,
unit: item.unit,
unit_price: item.unit_price,
vat_rate: item.vat_rate ?? null,
dimensions: item.dimensions ?? {},
}))
const { error: itemsError } = await supabase
.from('recurring_invoice_schedule_items')
.insert(itemRows)
if (itemsError) {
// Restore the snapshot so the schedule stays valid for the cron.
if (previousItems && previousItems.length > 0) {
const restoreRows = previousItems.map((row) => ({
schedule_id: scheduleId,
sort_order: row.sort_order,
description: row.description,
quantity: row.quantity,
unit: row.unit,
unit_price: row.unit_price,
vat_rate: row.vat_rate,
dimensions: row.dimensions ?? {},
}))
const { error: restoreError } = await supabase
.from('recurring_invoice_schedule_items')
.insert(restoreRows)
if (restoreError) {
log.error(
'failed to restore schedule items after failed replace: schedule may be left empty',
restoreError,
{ scheduleId },
)
}
// Items provided = replace all; omitted = keep existing (the schema
// contract). The shared helper compensates BOTH writes on failure, so an
// item failure cannot leave the header fields half-saved.
const result = await applyRecurringScheduleUpdate(supabase, {
scheduleId,
companyId,
fields: updateRow,
items,
log,
})
if (!result.ok) {
if (result.stage !== 'header' && (!result.itemsRestored || !result.headerRestored)) {
// Same registry sentence the PATCH route returns, so the two surfaces
// cannot drift on what the user is told.
const partial = getErrorEntry('INVOICE_RECURRING_UPDATE_PARTIAL')
log.error('recurring schedule update left a partial state', result.error, {
scheduleId,
companyId,
stage: result.stage,
itemsRestored: result.itemsRestored,
headerRestored: result.headerRestored,
})
return {
error: `${partial?.message_sv ?? 'Ändringen kunde inte slutföras.'} (${result.error.message})`,
// Machine-readable twin of the PATCH route's envelope code, so an
// MCP/staged-op caller can detect the partial state without
// substring-matching the Swedish sentence.
errorCode: 'INVOICE_RECURRING_UPDATE_PARTIAL',
status: 500,
}
return { error: itemsError.message, status: 500 }
}
itemsReplaced = true
return { error: result.error.message, status: 500 }
}
const itemsReplaced = Boolean(items)
return {
data: {
@@ -5652,7 +5625,11 @@ async function commitPendingOperationInner(
resolved_at: new Date().toISOString(),
result_data: isAutoReject
? { auto_rejected: true, reason: result.error }
: { error: result.error, http_status: result.status },
: {
error: result.error,
http_status: result.status,
...(result.errorCode ? { error_code: result.errorCode } : {}),
},
})
.eq('id', pendingOp.id)
if (isAutoReject) {
@@ -5667,6 +5644,7 @@ async function commitPendingOperationInner(
status: 'failed',
error: result.error,
http_status: result.status ?? 500,
...(result.errorCode ? { code: result.errorCode } : {}),
}
}