diff --git a/DECISIONS.md b/DECISIONS.md index 09b79cdf..7ff58d59 100644 --- a/DECISIONS.md +++ b/DECISIONS.md @@ -49,3 +49,4 @@ One line per decision: `[YYYY-MM-DD] : `. Appended by agents and [2026-07-08] Bedrock prod outage + Docker build failure both root-caused to dependabot #884 (a1fad319, 2026-07-06) bumping @anthropic-ai/bedrock-sdk 0.29.1->0.32.0. Runtime: 0.32.0 streaming returns an empty event stream ("request ended without sending any chunks", no HTTP status) - proven NOT a creds/region issue (prod diagnostic logged AKIA key + eu-west-1). Two prior sessions mis-diagnosed it as an AWS_* env collision and shipped/reverted #937 (BEDROCK_AWS_* rename) with no effect. "Works locally, fails on prod/CI" because local node_modules was stale at 0.29.1 while prod/Docker build fresh from the lockfile (0.32.0). Fix: pin back to ^0.29.1 + regenerate lockfile. FOLLOW-UP: add a dependabot ignore/exact-pin so it does not re-bump to 0.32.x and re-break both. [2026-07-08] One reconciliation PR adopts 3 prod-orphaned migrations (20260707113729 enrichment + 20260708120000/130000 ledger-stats RPCs) plus their pg-tests/fixtures onto main, instead of waiting on #927+#935 to merge: prod ledger was 3 versions ahead of the repo, leaving the default Supabase branch MIGRATIONS_FAILED and blocking every preview branch from being created. SQL committed byte-identical under the exact apply-time versions -> no-op on prod (idempotent), clean on fresh replays, and a no-op on #927/#935's next rebase. Carries #935's DB layer only (migrations + pg-tests + fixtures), not its UI/lib/i18n. Root anti-pattern: all three applied to prod via MCP apply_migration without committing the file (CLAUDE.md "never leave the remote DB ahead of the repo"). [2026-07-08] Pinned @anthropic-ai/bedrock-sdk to exact 0.29.1 (dependabot #884 auto-bumped it to 0.32.0, which broke Bedrock streaming in prod: empty stream / "request ended without sending any chunks"). Guarded three ways against accidental re-bump: exact pin in package.json, dependabot ignore, and a pinned-dep check in scripts/checks/no-new-antipatterns.mjs (check:guards). Unpin only once 0.32.x streaming is verified against Bedrock. +[2026-07-09] Issue #916 (disconnect orphans ledger accounts): release claims by demoting cash_accounts rows to manual (bank_connection_id = null), never deleting: transactions.cash_account_id and ledger history reference the rows, and upsertFromPsd2 promotes a manual holder in place on reconnect so the bank lands back on its original BAS slot. Orphans predating the fix self-heal via a revoked-status filter in the allocator + collision guard (not data repair). When a promote collides with a duplicate row for the same connection+uid (callback mirrored onto an overflow slot pre-fix), the duplicate is deleted only if it has zero linked transactions, otherwise demoted: preserves FK links while freeing the slot. Picker-save rejections now render inline in the picker instead of routing to the sync-progress modal, whose parent-unmount-on-close made every save outcome invisible. diff --git a/extensions/general/enable-banking/__tests__/accounts-route.test.ts b/extensions/general/enable-banking/__tests__/accounts-route.test.ts index 93d50330..7d11c841 100644 --- a/extensions/general/enable-banking/__tests__/accounts-route.test.ts +++ b/extensions/general/enable-banking/__tests__/accounts-route.test.ts @@ -7,13 +7,15 @@ vi.mock('../lib/sync', () => ({ // Mock the cash-accounts service (dynamically imported by the route) so the // mirror + allocation passes are deterministic and observable. -const { mockUpsertFromPsd2, mockAllocate } = vi.hoisted(() => ({ +const { mockUpsertFromPsd2, mockAllocate, mockGetRevokedConnectionIds } = vi.hoisted(() => ({ mockUpsertFromPsd2: vi.fn(), mockAllocate: vi.fn(), + mockGetRevokedConnectionIds: vi.fn(), })) vi.mock('@/lib/cash-accounts/service', () => ({ upsertFromPsd2: (...args: unknown[]) => mockUpsertFromPsd2(...args), allocatePsd2LedgerAccount: (...args: unknown[]) => mockAllocate(...args), + getRevokedConnectionIds: (...args: unknown[]) => mockGetRevokedConnectionIds(...args), })) import { enableBankingExtension } from '../index' @@ -157,6 +159,9 @@ describe('PATCH /accounts (enable-banking)', () => { beforeEach(() => { vi.clearAllMocks() mockUpsertFromPsd2.mockResolvedValue(undefined) + // Default: no revoked connections; individual tests override to exercise + // the self-heal path. + mockGetRevokedConnectionIds.mockResolvedValue(new Set()) // Allocator stand-in mirroring the real behavior: currency default first, // then the next free 1931–1959 slot (skipping other currency defaults). mockAllocate.mockImplementation( @@ -916,6 +921,57 @@ describe('PATCH /accounts (enable-banking)', () => { expect(body.conflicting_accounts).toEqual(['1935']) }) + it('allows a mapping onto a ledger held only by a REVOKED connection (self-heal after disconnect)', async () => { + // Issue #916: rows orphaned by a disconnect that predates the ledger + // claim release still point at the revoked connection. They must not + // count as foreign claims: the save goes through and upsertFromPsd2 + // promotes the orphaned row in place. + mockedSync.mockResolvedValue({ imported: 0, duplicates: 0, errors: 0 }) + mockGetRevokedConnectionIds.mockResolvedValue(new Set(['conn-REVOKED'])) + + const stub: SupabaseStub = { + authUser: { id: 'user-1' }, + chartAccountNumbers: ['1930'], + cashAccountRows: [ + { external_uid: 'old-acc', bank_connection_id: 'conn-REVOKED', ledger_account: '1930' }, + ], + connectionRow: { + id: 'conn-1', + status: 'pending_selection', + accounts_data: [{ uid: 'acc-1', currency: 'SEK', enabled: true }], + }, + } + const supabase = buildSupabase(stub) + const ctx = makeContext(supabase) + + const res = await accountsRoute.handler( + makeRequest({ + connection_id: 'conn-1', + enabled_uids: ['acc-1'], + account_mappings: [{ uid: 'acc-1', ledger_account: '1930' }], + }), + ctx + ) + + expect(res.status).toBe(200) + // The revoked-status lookup was scoped to the foreign connection ids. + expect(mockGetRevokedConnectionIds).toHaveBeenCalledWith( + expect.anything(), + 'company-1', + ['conn-REVOKED'] + ) + // The mirror received the user's pick, not an overflow slot. + expect(mockUpsertFromPsd2).toHaveBeenCalledWith( + expect.anything(), + 'company-1', + expect.objectContaining({ + bank_connection_id: 'conn-1', + external_uid: 'acc-1', + ledger_account: '1930', + }) + ) + }) + it('allocates distinct ledgers for legacy accounts with no mapping at all', async () => { mockedSync.mockResolvedValue({ imported: 0, duplicates: 0, errors: 0 }) diff --git a/extensions/general/enable-banking/__tests__/disconnect-route.test.ts b/extensions/general/enable-banking/__tests__/disconnect-route.test.ts new file mode 100644 index 00000000..baa3242f --- /dev/null +++ b/extensions/general/enable-banking/__tests__/disconnect-route.test.ts @@ -0,0 +1,219 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest' + +// Mock the Enable Banking API client so revoking the PSD2 session never makes +// a network call. SessionExpiredError must stay a real class: index.ts uses it +// in an instanceof check. +vi.mock('../lib/api-client', () => ({ + startAuthorization: vi.fn(), + getASPSPs: vi.fn(), + getPreferredAuthMethod: vi.fn(), + deleteSession: vi.fn().mockResolvedValue(undefined), + isSandboxMode: vi.fn(() => true), + SessionExpiredError: class SessionExpiredError extends Error {}, +})) + +import { enableBankingExtension } from '../index' +import { deleteSession } from '../lib/api-client' +import type { ExtensionContext } from '@/lib/extensions/types' + +const mockedDeleteSession = vi.mocked(deleteSession) + +const disconnectRoute = enableBankingExtension.apiRoutes?.find( + r => r.method === 'DELETE' && r.path === '/disconnect' +) + +if (!disconnectRoute) { + throw new Error('DELETE /disconnect route not registered on enable-banking extension') +} + +interface DisconnectStub { + authUser: { id: string } | null + connectionRow: { + id: string + session_id: string | null + status: string + bank_name?: string | null + } | null + connectionError?: { message: string } | null + connUpdateError?: { message: string } | null + cashUpdateError?: { message: string } | null + /** Captured bank_connections update payloads. */ + connUpdates: Array> + /** Captured cash_accounts update payloads + their eq() filters, in order. */ + cashUpdates: Array<{ payload: Record; filters: Array<[string, unknown]> }> +} + +function makeStub(partial: Partial = {}): DisconnectStub { + return { + authUser: { id: 'user-1' }, + connectionRow: { + id: 'conn-1', + session_id: 'sess-1', + status: 'active', + bank_name: 'Lunar', + }, + connUpdates: [], + cashUpdates: [], + ...partial, + } +} + +function buildSupabase(stub: DisconnectStub) { + return { + auth: { + getUser: vi.fn().mockResolvedValue({ data: { user: stub.authUser }, error: null }), + }, + from: vi.fn((table: string) => { + if (table === 'cash_accounts') { + return { + update: vi.fn((payload: Record) => { + const filters: Array<[string, unknown]> = [] + stub.cashUpdates.push({ payload, filters }) + const result = Promise.resolve({ error: stub.cashUpdateError ?? null }) + const builder = { + eq: vi.fn((col: string, val: unknown) => { + filters.push([col, val]) + return builder + }), + then: result.then.bind(result), + catch: result.catch.bind(result), + } + return builder + }), + } + } + // bank_connections + return { + select: vi.fn().mockReturnThis(), + eq: vi.fn().mockReturnThis(), + single: vi.fn().mockResolvedValue({ + data: stub.connectionRow, + error: stub.connectionError ?? null, + }), + update: vi.fn((payload: Record) => { + stub.connUpdates.push(payload) + return { eq: vi.fn().mockResolvedValue({ error: stub.connUpdateError ?? null }) } + }), + } + }), + } +} + +function makeContext(supabase: ReturnType): ExtensionContext { + return { + userId: 'user-1', + companyId: 'company-1', + extensionId: 'enable-banking', + requestId: 'req_test', + // eslint-disable-next-line @typescript-eslint/no-explicit-any + supabase: supabase as any, + emit: vi.fn().mockResolvedValue(undefined), + settings: { get: vi.fn(), set: vi.fn(), getAll: vi.fn() } as never, + storage: {} as never, + log: { + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + debug: vi.fn(), + } as never, + services: {} as never, + } +} + +function makeRequest(body: unknown): Request { + return new Request('http://localhost/api/extensions/ext/enable-banking/disconnect', { + method: 'DELETE', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify(body), + }) +} + +describe('DELETE /disconnect (enable-banking)', () => { + beforeEach(() => { + vi.clearAllMocks() + mockedDeleteSession.mockResolvedValue(undefined) + }) + + it('returns 401 when unauthenticated', async () => { + const stub = makeStub({ authUser: null }) + const ctx = makeContext(buildSupabase(stub)) + + const res = await disconnectRoute.handler(makeRequest({ connection_id: 'conn-1' }), ctx) + expect(res.status).toBe(401) + }) + + it('returns 404 when the connection is not found', async () => { + const stub = makeStub({ connectionRow: null, connectionError: { message: 'not found' } }) + const ctx = makeContext(buildSupabase(stub)) + + const res = await disconnectRoute.handler(makeRequest({ connection_id: 'conn-1' }), ctx) + expect(res.status).toBe(404) + }) + + it('revokes the connection AND releases its cash_accounts ledger claims (issue #916)', async () => { + const stub = makeStub() + const ctx = makeContext(buildSupabase(stub)) + + const res = await disconnectRoute.handler(makeRequest({ connection_id: 'conn-1' }), ctx) + + expect(res.status).toBe(200) + const body = await res.json() + expect(body.success).toBe(true) + + // PSD2 consent revoked upstream. + expect(mockedDeleteSession).toHaveBeenCalledWith('sess-1') + + // Connection marked revoked. + expect(stub.connUpdates).toEqual([{ status: 'revoked', session_id: null }]) + + // The connection's cash_accounts rows are demoted to manual (NOT deleted): + // transactions and ledger history reference them, and upsertFromPsd2 + // promotes manual holders in place on reconnect so the same bank lands + // back on its original BAS account. + expect(stub.cashUpdates).toHaveLength(1) + expect(stub.cashUpdates[0].payload).toEqual({ bank_connection_id: null }) + expect(stub.cashUpdates[0].filters).toEqual([ + ['company_id', 'company-1'], + ['bank_connection_id', 'conn-1'], + ]) + + expect(ctx.emit).toHaveBeenCalledWith( + expect.objectContaining({ + type: 'bank_connection.revoked', + payload: expect.objectContaining({ connectionId: 'conn-1', companyId: 'company-1' }), + }) + ) + }) + + it('still succeeds when the ledger claim release fails (self-heal covers it)', async () => { + // The connection is already revoked at that point; the allocator and the + // picker-save collision guard both skip revoked connections, so orphaned + // rows recover on the next picker save. + const stub = makeStub({ cashUpdateError: { message: 'transient' } }) + const ctx = makeContext(buildSupabase(stub)) + + const res = await disconnectRoute.handler(makeRequest({ connection_id: 'conn-1' }), ctx) + + expect(res.status).toBe(200) + const body = await res.json() + expect(body.success).toBe(true) + expect(ctx.log.error).toHaveBeenCalledWith( + expect.stringContaining('release cash_accounts'), + expect.objectContaining({ connectionId: 'conn-1' }) + ) + }) + + it('skips PSD2 session revocation when the connection has no session', async () => { + const stub = makeStub({ + connectionRow: { id: 'conn-1', session_id: null, status: 'expired', bank_name: 'Lunar' }, + }) + const ctx = makeContext(buildSupabase(stub)) + + const res = await disconnectRoute.handler(makeRequest({ connection_id: 'conn-1' }), ctx) + + expect(res.status).toBe(200) + expect(mockedDeleteSession).not.toHaveBeenCalled() + // Ledger claims are still released. + expect(stub.cashUpdates).toHaveLength(1) + }) +}) diff --git a/extensions/general/enable-banking/components/AccountPickerDialog.tsx b/extensions/general/enable-banking/components/AccountPickerDialog.tsx index d168b88a..5652e6be 100644 --- a/extensions/general/enable-banking/components/AccountPickerDialog.tsx +++ b/extensions/general/enable-banking/components/AccountPickerDialog.tsx @@ -86,6 +86,10 @@ export function AccountPickerDialog({ const [selected, setSelected] = useState>(new Set()) const [isSaving, setIsSaving] = useState(false) + // Server-side save rejection (validation / ledger conflict). Shown inline in + // the dialog: a rejected save persisted nothing and started no sync, so the + // user must see why and be able to correct the picks. + const [saveError, setSaveError] = useState(null) const [sieLastDate, setSieLastDate] = useState(null) const [chartAccounts, setChartAccounts] = useState([]) const [chartError, setChartError] = useState(false) @@ -114,6 +118,7 @@ export function AccountPickerDialog({ accounts.filter(a => a.enabled !== false).map(a => a.uid) ) setSelected(initial) + setSaveError(null) setLookbackMode('fiscal-year') setCustomSubMode('date') setCustomDate('') @@ -285,6 +290,7 @@ export function AccountPickerDialog({ } setIsSaving(true) + setSaveError(null) // Cap the client wait at the route's 300s budget so a hung backfill can't // leave the progress modal in 'syncing' forever. The save+backfill is one @@ -295,12 +301,15 @@ export function AccountPickerDialog({ // For the initial-selection path, open the progress modal up-front so the // user has visible feedback during the 30-60s backfill. Selection edits - // (no backfill) keep the existing toast-only feedback. + // (no backfill) keep the existing toast-only feedback. Do NOT signal the + // parent to close here (issue #916): the parent unmounts this component on + // close, which would tear down the progress modal too and swallow every + // outcome, including a rejected save. The picker Dialog hides itself while + // progressOpen is true and comes back if the save is rejected. if (isInitialSelection) { setSyncAttempt((n) => n + 1) setProgressState({ kind: 'syncing' }) setProgressOpen(true) - onOpenChange(false) } try { @@ -326,7 +335,17 @@ export function AccountPickerDialog({ const data = await response.json() if (!response.ok) { - throw new Error(data.error || 'Kunde inte spara kontoval') + // Rejected save (400 conflicting_accounts / duplicate_accounts / other + // validation): nothing was persisted and no sync started. Surface the + // server's message inside the still-open picker; the progress modal's + // failed state would wrongly claim "we retry in the background". + setSaveError( + typeof data?.error === 'string' && data.error + ? data.error + : 'Kunde inte spara kontoval' + ) + if (isInitialSelection) setProgressOpen(false) + return } if (isInitialSelection && data.initial_sync) { @@ -407,16 +426,27 @@ export function AccountPickerDialog({ open={progressOpen} onOpenChange={(next) => { setProgressOpen(next) - // When the user closes the summary, propagate the saved/refresh - // signal to the parent (it would have been emitted on success earlier; - // this just guards the failure case where we still want a refresh). - if (!next) onSaved() + // When the user dismisses the progress modal the initial-selection + // flow is over: propagate the saved/refresh signal and close the + // picker. The parent unmounts this whole component on close, which is + // exactly why the picker must stay open until this point: closing it + // earlier would unmount the progress modal mid-flight and swallow the + // sync summary or error. (onSaved was already emitted on success; + // repeating it just guards the failure case where we still refresh.) + if (!next) { + onSaved() + onOpenChange(false) + } }} bankName={bankName} accounts={accounts.filter((a) => selected.has(a.uid))} state={progressState} /> - + {/* Visually yield to the progress modal while it is up, but WITHOUT + signaling the parent (open stays true): the parent unmounts the + component on close, and a rejected save must return to a live picker + with the user's picks intact. */} + Välj konton att synka: {bankName} @@ -596,6 +626,15 @@ export function AccountPickerDialog({ )} + {saveError && ( +
+ Kontovalet sparades inte: {saveError} +
+ )} +
{sortedAccounts.map(account => { const isChecked = selected.has(account.uid) diff --git a/extensions/general/enable-banking/index.ts b/extensions/general/enable-banking/index.ts index baba10e8..e897dde1 100644 --- a/extensions/general/enable-banking/index.ts +++ b/extensions/general/enable-banking/index.ts @@ -837,7 +837,7 @@ export const enableBankingExtension: Extension = { // cash_accounts, whose UNIQUE (company_id, ledger_account) constraint // would otherwise fail per-account and get swallowed, leaving accounts // silently unmirrored. - const { allocatePsd2LedgerAccount, upsertFromPsd2 } = await import( + const { allocatePsd2LedgerAccount, upsertFromPsd2, getRevokedConnectionIds } = await import( '@/lib/cash-accounts/service' ) @@ -857,10 +857,32 @@ export const enableBankingExtension: Extension = { ) // Slots held by OTHER connections' PSD2 accounts — an explicit mapping // onto one of those would violate the unique constraint. Manual rows - // are not foreign: upsertFromPsd2 promotes them in place. + // are not foreign: upsertFromPsd2 promotes them in place. Rows held by + // a REVOKED connection are not foreign either: those are orphaned + // leftovers (disconnect predating the claim release, or a lost demote) + // and upsertFromPsd2 promotes them in place too. Excluding them here + // is the self-heal path for companies whose bank was disconnected + // before disconnect started releasing ledger claims. + const foreignConnectionIds = [ + ...new Set( + cashRows + .filter(r => r.bank_connection_id !== null && r.bank_connection_id !== connection.id) + .map(r => r.bank_connection_id as string) + ), + ] + const revokedConnectionIds = await getRevokedConnectionIds( + supabase, + companyId, + foreignConnectionIds + ) const foreignConnectedLedgers = new Set( cashRows - .filter(r => r.bank_connection_id !== null && r.bank_connection_id !== connection.id) + .filter( + r => + r.bank_connection_id !== null && + r.bank_connection_id !== connection.id && + !revokedConnectionIds.has(r.bank_connection_id) + ) .map(r => r.ledger_account) ) @@ -1227,6 +1249,30 @@ export const enableBankingExtension: Extension = { return NextResponse.json({ error: 'Failed to disconnect' }, { status: 500 }) } + // Release the connection's ledger claims by demoting its cash_accounts + // rows to manual (bank_connection_id = null). The rows themselves stay: + // transactions.cash_account_id and the ledger history reference them, + // and upsertFromPsd2 promotes a manual holder in place on reconnect so + // the same bank lands back on its original BAS account (e.g. 1930) + // instead of overflowing to the next free slot. + const { error: releaseError } = await supabase + .from('cash_accounts') + .update({ bank_connection_id: null }) + .eq('company_id', companyId) + .eq('bank_connection_id', connection.id) + + if (releaseError) { + // Don't fail the disconnect: the connection is already revoked, and + // the allocator / collision guard also skip revoked connections, so + // the orphaned rows self-heal on the next picker save. + log.error('[enable-banking] Failed to release cash_accounts ledger claims on disconnect', { + errorMessage: releaseError.message, + connectionId: connection.id, + userId: user.id, + companyId, + }) + } + try { const emit = ctx?.emit ?? (await import('@/lib/events/bus')).eventBus.emit.bind((await import('@/lib/events/bus')).eventBus) await emit({ diff --git a/lib/cash-accounts/__tests__/service.test.ts b/lib/cash-accounts/__tests__/service.test.ts index e3a1e9f6..50eaebf8 100644 --- a/lib/cash-accounts/__tests__/service.test.ts +++ b/lib/cash-accounts/__tests__/service.test.ts @@ -14,16 +14,46 @@ import { findFreeLedgerAccount, allocatePsd2LedgerAccount, defaultLedgerForCurrency, + getRevokedConnectionIds, + upsertFromPsd2, } from '../service' type CashRow = { ledger_account: string; bank_connection_id: string | null } +type ConnRow = { id: string; status: string } -function makeSupabase(rows: CashRow[], error: { message: string } | null = null) { +interface MakeSupabaseOpts { + error?: { message: string } | null + /** bank_connections rows for the status lookup. Missing ids = not revoked. */ + connections?: ConnRow[] + connectionsError?: { message: string } | null +} + +function makeSupabase(rows: CashRow[], opts: MakeSupabaseOpts = {}) { return { - from: vi.fn(() => ({ - select: vi.fn().mockReturnThis(), - eq: vi.fn(() => Promise.resolve({ data: error ? null : rows, error })), - })), + from: vi.fn((table: string) => { + if (table === 'bank_connections') { + return { + select: vi.fn().mockReturnThis(), + eq: vi.fn().mockReturnThis(), + in: vi.fn((_col: string, ids: string[]) => + Promise.resolve( + opts.connectionsError + ? { data: null, error: opts.connectionsError } + : { + data: (opts.connections ?? []).filter(c => ids.includes(c.id)), + error: null, + }, + ), + ), + } + } + return { + select: vi.fn().mockReturnThis(), + eq: vi.fn(() => + Promise.resolve({ data: opts.error ? null : rows, error: opts.error ?? null }), + ), + } + }), } as unknown as SupabaseClient } @@ -48,6 +78,32 @@ describe('defaultLedgerForCurrency', () => { }) }) +describe('getRevokedConnectionIds', () => { + it('returns only the ids whose connection is revoked', async () => { + const supabase = makeSupabase([], { + connections: [ + { id: 'conn-a', status: 'revoked' }, + { id: 'conn-b', status: 'active' }, + ], + }) + const revoked = await getRevokedConnectionIds(supabase, 'c1', ['conn-a', 'conn-b']) + expect(revoked).toEqual(new Set(['conn-a'])) + }) + + it('returns an empty set without querying when no ids are given', async () => { + const supabase = makeSupabase([]) + const revoked = await getRevokedConnectionIds(supabase, 'c1', []) + expect(revoked.size).toBe(0) + expect((supabase as unknown as { from: ReturnType }).from).not.toHaveBeenCalled() + }) + + it('treats every connection as active when the lookup fails (conservative)', async () => { + const supabase = makeSupabase([], { connectionsError: { message: 'boom' } }) + const revoked = await getRevokedConnectionIds(supabase, 'c1', ['conn-a']) + expect(revoked.size).toBe(0) + }) +}) + describe('findFreeLedgerAccount', () => { it('returns the currency default when nothing holds it', async () => { const supabase = makeSupabase([]) @@ -62,11 +118,50 @@ describe('findFreeLedgerAccount', () => { expect(await findFreeLedgerAccount(supabase, 'c1', 'SEK')).toBe('1930') }) + it('returns the default when it is held only by a REVOKED connection (issue #916)', async () => { + // Disconnecting a bank releases its ledger claims. Rows orphaned before + // that fix still point at the revoked connection; they must count as + // manual holders so a reconnect lands back on 1930, not 1939. + const supabase = makeSupabase( + [{ ledger_account: '1930', bank_connection_id: 'conn-revoked' }], + { connections: [{ id: 'conn-revoked', status: 'revoked' }] }, + ) + expect(await findFreeLedgerAccount(supabase, 'c1', 'SEK')).toBe('1930') + }) + it('overflows to 1931 when a CONNECTED row holds the default', async () => { - const supabase = makeSupabase([{ ledger_account: '1930', bank_connection_id: 'conn-1' }]) + const supabase = makeSupabase([{ ledger_account: '1930', bank_connection_id: 'conn-1' }], { + connections: [{ id: 'conn-1', status: 'active' }], + }) expect(await findFreeLedgerAccount(supabase, 'c1', 'SEK')).toBe('1931') }) + it('still overflows when the revoked-status lookup fails (conservative)', async () => { + const supabase = makeSupabase( + [{ ledger_account: '1930', bank_connection_id: 'conn-revoked' }], + { connectionsError: { message: 'boom' } }, + ) + expect(await findFreeLedgerAccount(supabase, 'c1', 'SEK')).toBe('1931') + }) + + it('keeps revoked-held rows blocking OVERFLOW slots (like manual rows)', async () => { + // The revoked-held row on 1931 keeps its history on that slot; handing the + // slot to a different account would steal it via promote-in-place. + const supabase = makeSupabase( + [ + { ledger_account: '1930', bank_connection_id: 'conn-active' }, + { ledger_account: '1931', bank_connection_id: 'conn-revoked' }, + ], + { + connections: [ + { id: 'conn-active', status: 'active' }, + { id: 'conn-revoked', status: 'revoked' }, + ], + }, + ) + expect(await findFreeLedgerAccount(supabase, 'c1', 'SEK')).toBe('1935') + }) + it('never hands out another currency default as an overflow slot', async () => { const supabase = makeSupabase([ { ledger_account: '1930', bank_connection_id: 'conn-1' }, @@ -102,7 +197,7 @@ describe('findFreeLedgerAccount', () => { }) it('returns null when the lookup fails', async () => { - const supabase = makeSupabase([], { message: 'boom' }) + const supabase = makeSupabase([], { error: { message: 'boom' } }) expect(await findFreeLedgerAccount(supabase, 'c1', 'SEK')).toBeNull() }) }) @@ -167,3 +262,297 @@ describe('allocatePsd2LedgerAccount', () => { expect(mockSyncMappedAccounts).not.toHaveBeenCalled() }) }) + +// --------------------------------------------------------------------------- +// upsertFromPsd2: promote-in-place + duplicate merge (issue #916) +// --------------------------------------------------------------------------- + +interface UpsertStub { + /** Row currently holding (company_id, ledger_account), if any. */ + holder?: { id: string; bank_connection_id: string | null } | null + /** bank_connections rows for the revoked-status lookup. */ + connections?: ConnRow[] + /** Existing row for (company_id, bank_connection_id, external_uid) on another ledger. */ + ownRow?: { id: string; is_primary: boolean } | null + /** Whether the duplicate ownRow has linked transactions. */ + ownHasTransactions?: boolean + upsertError?: { message: string } | null + // Captured writes: + updates: Array<{ payload: Record; id: unknown }> + deletes: unknown[] + upserts: Array> + rpcCalls: Array<{ fn: string; args: Record }> + /** .eq() filters applied to the linked-transactions probe. */ + transactionFilters: Array<{ col: string; value: unknown }> +} + +function makeUpsertStub(partial: Partial = {}): UpsertStub { + return { + updates: [], + deletes: [], + upserts: [], + rpcCalls: [], + transactionFilters: [], + ...partial, + } +} + +function makeUpsertSupabase(stub: UpsertStub) { + return { + rpc: vi.fn((fn: string, args: Record) => { + stub.rpcCalls.push({ fn, args }) + return Promise.resolve({ error: null }) + }), + from: vi.fn((table: string) => { + if (table === 'bank_connections') { + return { + select: vi.fn().mockReturnThis(), + eq: vi.fn().mockReturnThis(), + in: vi.fn((_col: string, ids: string[]) => + Promise.resolve({ + data: (stub.connections ?? []).filter(c => ids.includes(c.id)), + error: null, + }), + ), + } + } + if (table === 'transactions') { + const chain = { + select: vi.fn(() => chain), + eq: vi.fn((col: string, value: unknown) => { + stub.transactionFilters.push({ col, value }) + return chain + }), + limit: vi.fn(() => + Promise.resolve({ + data: stub.ownHasTransactions ? [{ id: 'tx-1' }] : [], + error: null, + }), + ), + } + return chain + } + // cash_accounts + return { + select: vi.fn((cols: string) => { + const chain = { + eq: vi.fn(() => chain), + neq: vi.fn(() => chain), + maybeSingle: vi.fn(() => { + // Holder lookup selects bank_connection_id; duplicate lookup + // selects is_primary. Route by the requested columns. + if (cols.includes('bank_connection_id')) { + return Promise.resolve({ data: stub.holder ?? null, error: null }) + } + return Promise.resolve({ data: stub.ownRow ?? null, error: null }) + }), + } + return chain + }), + update: vi.fn((payload: Record) => ({ + eq: vi.fn((_col: string, id: unknown) => { + stub.updates.push({ payload, id }) + const result = Promise.resolve({ data: null, error: null }) + return { + select: vi.fn(() => Promise.resolve({ data: [{ id }], error: null })), + then: result.then.bind(result), + catch: result.catch.bind(result), + } + }), + })), + delete: vi.fn(() => ({ + eq: vi.fn((_col: string, id: unknown) => { + stub.deletes.push(id) + return Promise.resolve({ error: null }) + }), + })), + upsert: vi.fn((payload: Record) => { + stub.upserts.push(payload) + return Promise.resolve({ error: stub.upsertError ?? null }) + }), + } + }), + } as unknown as SupabaseClient +} + +const UPSERT_INPUT = { + bank_connection_id: 'conn-new', + external_uid: 'uid-1', + currency: 'SEK', + ledger_account: '1930', +} + +describe('upsertFromPsd2', () => { + it('plain-upserts when no row holds the target ledger', async () => { + const stub = makeUpsertStub({ holder: null }) + await upsertFromPsd2(makeUpsertSupabase(stub), 'c1', UPSERT_INPUT) + + expect(stub.upserts).toHaveLength(1) + expect(stub.upserts[0]).toMatchObject({ + company_id: 'c1', + bank_connection_id: 'conn-new', + external_uid: 'uid-1', + ledger_account: '1930', + }) + expect(stub.updates).toHaveLength(0) + }) + + it('promotes a MANUAL holder row in place (seed row or demoted-on-disconnect row)', async () => { + const stub = makeUpsertStub({ holder: { id: 'row-manual', bank_connection_id: null } }) + await upsertFromPsd2(makeUpsertSupabase(stub), 'c1', UPSERT_INPUT) + + expect(stub.updates).toHaveLength(1) + expect(stub.updates[0].id).toBe('row-manual') + expect(stub.updates[0].payload).toMatchObject({ + bank_connection_id: 'conn-new', + external_uid: 'uid-1', + ledger_account: '1930', + }) + expect(stub.upserts).toHaveLength(0) + }) + + it('promotes a holder owned by a REVOKED connection (orphan self-heal, issue #916)', async () => { + const stub = makeUpsertStub({ + holder: { id: 'row-old', bank_connection_id: 'conn-old' }, + connections: [{ id: 'conn-old', status: 'revoked' }], + }) + await upsertFromPsd2(makeUpsertSupabase(stub), 'c1', UPSERT_INPUT) + + // The orphaned row keeps its id (transaction links survive) and is + // re-bound to the new connection on its original ledger account. + expect(stub.updates).toHaveLength(1) + expect(stub.updates[0].id).toBe('row-old') + expect(stub.updates[0].payload).toMatchObject({ + bank_connection_id: 'conn-new', + external_uid: 'uid-1', + ledger_account: '1930', + }) + expect(stub.upserts).toHaveLength(0) + expect(stub.deletes).toHaveLength(0) + }) + + it('does NOT promote a holder owned by an ACTIVE foreign connection', async () => { + const stub = makeUpsertStub({ + holder: { id: 'row-other', bank_connection_id: 'conn-other' }, + connections: [{ id: 'conn-other', status: 'active' }], + }) + await upsertFromPsd2(makeUpsertSupabase(stub), 'c1', UPSERT_INPUT) + + // Falls through to the plain upsert; the DB unique constraint is the + // final arbiter for a genuine conflict. + expect(stub.updates).toHaveLength(0) + expect(stub.upserts).toHaveLength(1) + }) + + it('routes a holder owned by the SAME connection through the plain upsert', async () => { + const stub = makeUpsertStub({ + holder: { id: 'row-self', bank_connection_id: 'conn-new' }, + }) + await upsertFromPsd2(makeUpsertSupabase(stub), 'c1', UPSERT_INPUT) + + expect(stub.updates).toHaveLength(0) + expect(stub.upserts).toHaveLength(1) + }) + + it('deletes an empty duplicate row for the same connection+uid before promoting', async () => { + // Stuck-user recovery: the reconnect callback mirrored uid-1 onto 1939 + // while 1930 was wrongly blocked. On remap to 1930 the empty 1939 + // duplicate is removed and the orphaned holder is promoted, freeing 1939. + const stub = makeUpsertStub({ + holder: { id: 'row-old', bank_connection_id: 'conn-old' }, + connections: [{ id: 'conn-old', status: 'revoked' }], + ownRow: { id: 'row-dup', is_primary: false }, + ownHasTransactions: false, + }) + await upsertFromPsd2(makeUpsertSupabase(stub), 'c1', UPSERT_INPUT) + + expect(stub.deletes).toEqual(['row-dup']) + expect(stub.updates).toHaveLength(1) + expect(stub.updates[0].id).toBe('row-old') + expect(stub.rpcCalls).toHaveLength(0) + }) + + it('demotes (not deletes) a duplicate that has linked transactions', async () => { + const stub = makeUpsertStub({ + holder: { id: 'row-old', bank_connection_id: 'conn-old' }, + connections: [{ id: 'conn-old', status: 'revoked' }], + ownRow: { id: 'row-dup', is_primary: false }, + ownHasTransactions: true, + }) + await upsertFromPsd2(makeUpsertSupabase(stub), 'c1', UPSERT_INPUT) + + expect(stub.deletes).toHaveLength(0) + expect(stub.updates).toHaveLength(2) + // First write releases the duplicate's PSD2 binding, preserving the row + // (and its transactions.cash_account_id links) as a manual account. + expect(stub.updates[0].id).toBe('row-dup') + expect(stub.updates[0].payload).toEqual({ bank_connection_id: null, external_uid: null }) + // Second write promotes the holder. + expect(stub.updates[1].id).toBe('row-old') + expect(stub.updates[1].payload).toMatchObject({ bank_connection_id: 'conn-new' }) + }) + + it('scopes the duplicate linked-transactions probe by company (service-role defense in depth)', async () => { + const stub = makeUpsertStub({ + holder: { id: 'row-old', bank_connection_id: 'conn-old' }, + connections: [{ id: 'conn-old', status: 'revoked' }], + ownRow: { id: 'row-dup', is_primary: false }, + ownHasTransactions: false, + }) + await upsertFromPsd2(makeUpsertSupabase(stub), 'c1', UPSERT_INPUT) + + expect(stub.transactionFilters).toEqual( + expect.arrayContaining([ + { col: 'company_id', value: 'c1' }, + { col: 'cash_account_id', value: 'row-dup' }, + ]), + ) + }) + + it('transfers the primary flag when the deleted duplicate was primary', async () => { + const stub = makeUpsertStub({ + holder: { id: 'row-old', bank_connection_id: 'conn-old' }, + connections: [{ id: 'conn-old', status: 'revoked' }], + ownRow: { id: 'row-dup', is_primary: true }, + ownHasTransactions: false, + }) + await upsertFromPsd2(makeUpsertSupabase(stub), 'c1', UPSERT_INPUT) + + expect(stub.deletes).toEqual(['row-dup']) + expect(stub.rpcCalls).toEqual([ + { + fn: 'set_cash_account_primary', + args: { p_company_id: 'c1', p_cash_account_id: 'row-old' }, + }, + ]) + }) + + it('transfers the primary flag when the DEMOTED duplicate was primary', async () => { + // Otherwise the stale manual row keeps is_primary=true and the + // __PRIMARY_SEK__ sentinel resolves to the wrong row. + const stub = makeUpsertStub({ + holder: { id: 'row-old', bank_connection_id: 'conn-old' }, + connections: [{ id: 'conn-old', status: 'revoked' }], + ownRow: { id: 'row-dup', is_primary: true }, + ownHasTransactions: true, + }) + await upsertFromPsd2(makeUpsertSupabase(stub), 'c1', UPSERT_INPUT) + + expect(stub.deletes).toHaveLength(0) + expect(stub.updates[0].id).toBe('row-dup') + expect(stub.updates[0].payload).toEqual({ bank_connection_id: null, external_uid: null }) + expect(stub.rpcCalls).toEqual([ + { + fn: 'set_cash_account_primary', + args: { p_company_id: 'c1', p_cash_account_id: 'row-old' }, + }, + ]) + }) + + it('throws when the plain upsert fails', async () => { + const stub = makeUpsertStub({ holder: null, upsertError: { message: 'duplicate key' } }) + await expect( + upsertFromPsd2(makeUpsertSupabase(stub), 'c1', UPSERT_INPUT), + ).rejects.toThrow(/duplicate key/) + }) +}) diff --git a/lib/cash-accounts/service.ts b/lib/cash-accounts/service.ts index 9c256589..472b2149 100644 --- a/lib/cash-accounts/service.ts +++ b/lib/cash-accounts/service.ts @@ -131,6 +131,41 @@ export async function findByIban( return (data as CashAccount | null) ?? null } +/** + * Of the given bank_connection ids, return the subset whose connection row has + * status 'revoked'. A revoked connection no longer holds a live claim on its + * cash_accounts rows: the allocator, the picker-save collision guard, and + * upsertFromPsd2's promote-in-place path all treat those rows like manual + * holders so a reconnect can land back on its original ledger account. + * + * On lookup failure this returns an empty set (treat every connection as + * active): the conservative pre-fix behavior. + */ +export async function getRevokedConnectionIds( + supabase: SupabaseClient, + companyId: string, + connectionIds: readonly string[], +): Promise> { + if (connectionIds.length === 0) return new Set() + + const { data, error } = await supabase + .from('bank_connections') + .select('id, status') + .eq('company_id', companyId) + .in('id', [...connectionIds]) + + if (error) { + log.warn('getRevokedConnectionIds lookup failed', { companyId, error: error.message }) + return new Set() + } + + return new Set( + ((data ?? []) as Array<{ id: string; status: string }>) + .filter(c => c.status === 'revoked') + .map(c => c.id), + ) +} + /** * Find a free BAS class-19 slot for a new PSD2 cash account, respecting the * UNIQUE (company_id, ledger_account) constraint. A bank returning N @@ -141,6 +176,9 @@ export async function findByIban( * - The currency default (1930/1932/1933/1934) is available when no * PSD2-backed row holds it. A manual holder (the seeded 1930 row) does * not block it — upsertFromPsd2 promotes that row in place. + * Rows held by a REVOKED connection count as manual too: disconnecting a + * bank releases its ledger claims, so reconnecting the same bank gets its + * original slot back instead of overflowing to 1939. * - Overflow walks the free-use 1931–1959 sub-account slots, skipping the * four currency defaults (reserved as suggestions for their currencies) * and any slot held by ANY existing row — promoting an unrelated manual @@ -169,11 +207,20 @@ export async function findFreeLedgerAccount( return null } + const typedRows = (rows ?? []) as Array<{ ledger_account: string; bank_connection_id: string | null }> + const revokedConnectionIds = await getRevokedConnectionIds( + supabase, + companyId, + [...new Set(typedRows.map(r => r.bank_connection_id).filter((id): id is string => id !== null))], + ) + const anyTaken = new Set() const connectedTaken = new Set() - for (const row of (rows ?? []) as Array<{ ledger_account: string; bank_connection_id: string | null }>) { + for (const row of typedRows) { anyTaken.add(row.ledger_account) - if (row.bank_connection_id !== null) connectedTaken.add(row.ledger_account) + if (row.bank_connection_id !== null && !revokedConnectionIds.has(row.bank_connection_id)) { + connectedTaken.add(row.ledger_account) + } } if (!exclude.has(preferred) && !connectedTaken.has(preferred)) return preferred @@ -265,30 +312,127 @@ export async function upsertFromPsd2( // create_company_with_owner and the seed_default_cash_account migration plant // a manual (bank_connection_id IS NULL) row on the same ledger_account so - // reconciliation routes work before any PSD2 connection exists. The first - // PSD2 sync for that BAS slot has to promote that row in place: a plain - // upsert on (company_id, bank_connection_id, external_uid) wouldn't match it - // (NULL ≠ NULL) and the INSERT path then trips the (company_id, - // ledger_account) UNIQUE constraint. - const { data: seedRow, error: seedLookupError } = await supabase + // reconciliation routes work before any PSD2 connection exists, and the + // disconnect handler demotes a revoked connection's rows to manual the same + // way. Rows still pointing at a REVOKED connection (orphans from before the + // disconnect handler released claims) no longer hold a live claim either. + // In all three cases the PSD2 sync claiming that BAS slot has to promote the + // holder row in place: a plain upsert on (company_id, bank_connection_id, + // external_uid) wouldn't match it and the INSERT path then trips the + // (company_id, ledger_account) UNIQUE constraint. Promoting (instead of + // inserting) keeps the row id stable so transactions.cash_account_id links + // and the ledger's history stay attached. + const { data: holderRow, error: holderLookupError } = await supabase .from('cash_accounts') - .select('id') + .select('id, bank_connection_id') .eq('company_id', companyId) .eq('ledger_account', input.ledger_account) - .is('bank_connection_id', null) .maybeSingle() - if (seedLookupError) { - log.error('upsertFromPsd2 seed lookup failed', { + if (holderLookupError) { + log.error('upsertFromPsd2 holder lookup failed', { companyId, bankConnectionId: input.bank_connection_id, externalUid: input.external_uid, - error: seedLookupError.message, + error: holderLookupError.message, }) - throw new Error(`cash_accounts upsert failed: ${seedLookupError.message}`) + throw new Error(`cash_accounts upsert failed: ${holderLookupError.message}`) } - if (seedRow) { + const typedHolder = holderRow as { id: string; bank_connection_id: string | null } | null + let promotableRowId: string | null = null + if (typedHolder) { + if (typedHolder.bank_connection_id === null) { + promotableRowId = typedHolder.id + } else if (typedHolder.bank_connection_id !== input.bank_connection_id) { + const revoked = await getRevokedConnectionIds(supabase, companyId, [ + typedHolder.bank_connection_id, + ]) + if (revoked.has(typedHolder.bank_connection_id)) { + promotableRowId = typedHolder.id + } + } + // Holder owned by the input connection itself (or by another ACTIVE + // connection): fall through to the plain upsert. For the former the upsert + // matches on (company_id, bank_connection_id, external_uid) and updates in + // place; for the latter the UNIQUE constraint rejects the write and the + // error surfaces to the caller (the picker-save collision guard should + // have caught it earlier). + } + + if (promotableRowId) { + // Promoting the holder makes it THE row for this (bank_connection_id, + // external_uid). If this connection + uid already has a row on another + // ledger (the reconnect callback mirrored it onto an overflow slot while + // the target slot was still wrongly blocked by a revoked connection), that + // duplicate must be resolved first or the promote trips the UNIQUE + // (company_id, bank_connection_id, external_uid) constraint. + const { data: ownRow, error: ownLookupError } = await supabase + .from('cash_accounts') + .select('id, is_primary') + .eq('company_id', companyId) + .eq('bank_connection_id', input.bank_connection_id) + .eq('external_uid', input.external_uid) + .neq('id', promotableRowId) + .maybeSingle() + + if (ownLookupError) { + log.error('upsertFromPsd2 duplicate lookup failed', { + companyId, + bankConnectionId: input.bank_connection_id, + externalUid: input.external_uid, + error: ownLookupError.message, + }) + throw new Error(`cash_accounts upsert failed: ${ownLookupError.message}`) + } + + const typedOwn = ownRow as { id: string; is_primary: boolean } | null + let transferPrimary = false + if (typedOwn) { + // With linked transactions the duplicate is demoted to a plain manual + // row (deleting it would SET NULL those transactions' cash_account_id + // links). Without any, it is a leftover mirror from the broken reconnect + // and is deleted outright so its overflow slot frees up. + const { data: linkedTx, error: linkedTxError } = await supabase + .from('transactions') + .select('id') + .eq('company_id', companyId) + .eq('cash_account_id', typedOwn.id) + .limit(1) + + if (linkedTxError) { + log.error('upsertFromPsd2 duplicate transaction check failed', { + companyId, + bankConnectionId: input.bank_connection_id, + externalUid: input.external_uid, + error: linkedTxError.message, + }) + throw new Error(`cash_accounts upsert failed: ${linkedTxError.message}`) + } + + if ((linkedTx ?? []).length > 0) { + const { error: demoteError } = await supabase + .from('cash_accounts') + .update({ bank_connection_id: null, external_uid: null }) + .eq('id', typedOwn.id) + if (demoteError) { + throw new Error(`cash_accounts upsert failed: ${demoteError.message}`) + } + } else { + const { error: deleteError } = await supabase + .from('cash_accounts') + .delete() + .eq('id', typedOwn.id) + if (deleteError) { + throw new Error(`cash_accounts upsert failed: ${deleteError.message}`) + } + } + // A primary duplicate must hand the flag to the promoted row either way: + // deleted, it would leave the __PRIMARY_SEK__ sentinel unresolvable; + // demoted, the sentinel would keep resolving to the stale manual row. + transferPrimary = typedOwn.is_primary + } + // .select() so we can detect a 0-row UPDATE: Supabase's update().eq() returns // { error: null, data: [] } if the row was deleted between the SELECT above // and this UPDATE (rare but theoretically possible under concurrent ops). @@ -297,10 +441,10 @@ export async function upsertFromPsd2( const { data: promoted, error: promoteError } = await supabase .from('cash_accounts') .update(payload) - .eq('id', seedRow.id) + .eq('id', promotableRowId) .select('id') if (promoteError) { - log.error('upsertFromPsd2 promote-seed failed', { + log.error('upsertFromPsd2 promote-holder failed', { companyId, bankConnectionId: input.bank_connection_id, externalUid: input.external_uid, @@ -309,9 +453,22 @@ export async function upsertFromPsd2( throw new Error(`cash_accounts upsert failed: ${promoteError.message}`) } if (promoted && promoted.length > 0) { + if (transferPrimary) { + try { + await setPrimary(supabase, companyId, promotableRowId) + } catch (primaryError) { + // The promote itself succeeded; losing the primary flag is + // recoverable via the AccountPicker, so log instead of unwinding. + log.error('upsertFromPsd2 primary transfer failed', { + companyId, + cashAccountId: promotableRowId, + error: primaryError instanceof Error ? primaryError.message : String(primaryError), + }) + } + } return } - // Seed row vanished between SELECT and UPDATE: fall through to upsert. + // Holder row vanished between SELECT and UPDATE: fall through to upsert. } const { error } = await supabase