diff --git a/app/api/reimbursable/route.ts b/app/api/reimbursable/route.ts index 9ff3a6f..af0b1d6 100644 --- a/app/api/reimbursable/route.ts +++ b/app/api/reimbursable/route.ts @@ -50,12 +50,14 @@ export async function PATCH(req: Request) { console.error('[reimbursable] transaction read failed', { id: transactionId, code: readError.code, message: readError.message }) return fail(500, TRY_AGAIN) } - if (!txn) return fail(404, 'That transaction no longer exists. Refresh and try again.') + if (!txn) return fail(404, 'That transaction no longer exists. Showing the latest.') // `removed` is a soft flag (a Plaid repost), not a delete, so nothing stops a mark being written // to a row that no longer renders anywhere. That mark would then be unreachable while still - // counting toward what you are owed. - if (txn.removed) return fail(400, 'Your bank removed that transaction. Refresh and try again.') + // counting toward what you are owed. A 409, like the other stale-row refusals here, so the + // editors reload and the removed row goes. These used to say "Refresh and try again", which the + // installed app has no way to do (#139). + if (txn.removed) return fail(409, 'Your bank removed that transaction. Showing the latest.') // Guards #31. A credit-card payment is already excluded from both spending and income — the // purchases were counted when they happened — so marking it reduces nothing while still inflating @@ -94,7 +96,7 @@ export async function PATCH(req: Request) { if (!count) { // A stale-card refusal: the editors reload on a 409 to show the latest. console.error('[reimbursable] update matched no rows (row changed, or the update policy is missing)', { id: transactionId }) - return fail(409, 'This transaction just changed. Refresh and try again.') + return fail(409, 'This transaction just changed. Showing the latest.') } return NextResponse.json({ ok: true }) diff --git a/app/api/transactions/categorize/route.ts b/app/api/transactions/categorize/route.ts index 9d2b338..a0a8ba0 100644 --- a/app/api/transactions/categorize/route.ts +++ b/app/api/transactions/categorize/route.ts @@ -27,6 +27,10 @@ import { unknownIfLost } from '@/lib/lost-reply' // // It is not the only writer of user_category: app/api/categories/route.ts also writes it in bulk on // rename and delete (a delete clears picks), outside these guards (#100). +// +// 404 and 409 mean the picker's row is stale: the transaction is gone, the bank removed it, the +// category was deleted, or the row changed. CategoryPicker reloads on either, so these say the latest +// is showing. They used to say "Refresh and try again", which the installed app has no way to do (#139). const SAVE_FAILED = 'That could not be saved. Please try again.' export async function POST(req: Request) { @@ -68,13 +72,10 @@ export async function POST(req: Request) { return NextResponse.json({ error: SAVE_FAILED }, { status: 500 }) } if (!row) { - return NextResponse.json({ error: 'That transaction no longer exists. Refresh and try again.' }, { status: 404 }) + return NextResponse.json({ error: 'That transaction no longer exists. Showing the latest.' }, { status: 404 }) } if (row.removed) { - return NextResponse.json( - { error: 'Your bank removed that transaction. Refresh and try again.' }, - { status: 400 } - ) + return NextResponse.json({ error: 'Your bank removed that transaction. Showing the latest.' }, { status: 409 }) } // 4. Card payments, picked or not. @@ -98,10 +99,7 @@ export async function POST(req: Request) { } const picked = (cats ?? []).find((c) => c.name === category) if (!picked) { - return NextResponse.json( - { error: 'That category no longer exists. Refresh and try again.' }, - { status: 400 } - ) + return NextResponse.json({ error: 'That category no longer exists. Showing the latest.' }, { status: 409 }) } // 6. Write, with the guards INSIDE the update, so Plaid re-tagging or removing the row between the @@ -125,7 +123,7 @@ export async function POST(req: Request) { // Usually the row changed underneath us. A missing update policy would also land here, which is // why the log says both. console.error('[categorize] update matched no rows (row changed, or the update policy is missing)', transactionId) - return NextResponse.json({ error: 'This transaction just changed. Refresh and try again.' }, { status: 409 }) + return NextResponse.json({ error: 'This transaction just changed. Showing the latest.' }, { status: 409 }) } // 7. A record of every change, so a mistaken pick can be put back from the logs. diff --git a/components/CategoryPicker.tsx b/components/CategoryPicker.tsx index dbf7590..0654bb6 100644 --- a/components/CategoryPicker.tsx +++ b/components/CategoryPicker.tsx @@ -59,8 +59,8 @@ export function CategoryPicker({ setPending(category) working(true) setError(null) - // What to do once the request has settled: reload after a save, or after one whose outcome is - // unknown. Never while it is in flight, and never after a refusal. + // What to do once the request has settled: reload after a save, after one whose outcome is + // unknown, or after a refusal that says this row is stale. Never while it is in flight. let reload = false try { // The shared rule (lib/save-response.ts): a redirect to /login or an OK reply that is not JSON @@ -78,6 +78,14 @@ export function CategoryPicker({ setPending(null) if (result.outcome === 'refused') { setError(result.error) + // 404 and 409: the route says this row is stale. The transaction is gone or the bank + // removed it, the category was deleted, or the bank changed the row during the save. So + // show the latest, as the message says. The installed app has no other way to reload + // (#139). For a deleted category the row stays and so does the message, and the options + // lose the category. A row that's gone takes its message with it, and a pending + // transaction that posted shows as the posted one, to pick again (#252). Any other refusal + // wrote nothing, and the server still holds what the picker shows. + reload = result.status === 404 || result.status === 409 } else { // Failed in flight, timed out, or a reply that could not be read. None of them means the // request never reached the server. diff --git a/tests/unit/categorize-route.test.ts b/tests/unit/categorize-route.test.ts index 03e32ef..f018f14 100644 --- a/tests/unit/categorize-route.test.ts +++ b/tests/unit/categorize-route.test.ts @@ -226,9 +226,13 @@ describe('POST /api/transactions/categorize', () => { expect(calls.updates).toHaveLength(0) }) - it('answers 404 when the transaction does not exist', async () => { + // A stale row answers 404 or 409, which CategoryPicker reloads on, so the words say the latest is + // showing. "Refresh and try again" asked for something the installed app has no way to do (#139). + it('answers 404 when the transaction does not exist, saying the latest is showing', async () => { makeSupabase({ row: null }) - expect((await post({ transactionId: 'txn-1', category: 'Grocery' })).status).toBe(404) + const res = await post({ transactionId: 'txn-1', category: 'Grocery' }) + expect(res.status).toBe(404) + expect((await res.json()).error).toBe('That transaction no longer exists. Showing the latest.') }) it('reads every column the later steps need', async () => { @@ -239,11 +243,12 @@ describe('POST /api/transactions/categorize', () => { } }) - it('answers 400 for a row the bank removed, and writes nothing', async () => { + // 409, not 400, so the picker reloads and the removed row goes from the list (#139). + it('answers 409 for a row the bank removed, and writes nothing', async () => { const calls = makeSupabase({ row: { ...ROW, removed: true } }) const res = await post({ transactionId: 'txn-1', category: 'Grocery' }) - expect(res.status).toBe(400) - expect((await res.json()).error).toBe('Your bank removed that transaction. Refresh and try again.') + expect(res.status).toBe(409) + expect((await res.json()).error).toBe('Your bank removed that transaction. Showing the latest.') expect(calls.updates).toHaveLength(0) }) @@ -282,12 +287,13 @@ describe('POST /api/transactions/categorize', () => { }) // A name that is not one of the household's categories, including the display fallback - // 'Uncategorized' (spec decision 7: clearing is not supported). - it.each(['Uncategorized', 'Not A Category'])('answers 400 for %s, and writes nothing', async (category) => { + // 'Uncategorized' (spec decision 7: clearing is not supported). From the picker, that's a category + // deleted since the list loaded, so 409, not 400: the picker reloads its options (#139). + it.each(['Uncategorized', 'Not A Category'])('answers 409 for %s, and writes nothing', async (category) => { const calls = makeSupabase({ row: ROW }) const res = await post({ transactionId: 'txn-1', category }) - expect(res.status).toBe(400) - expect((await res.json()).error).toBe('That category no longer exists. Refresh and try again.') + expect(res.status).toBe(409) + expect((await res.json()).error).toBe('That category no longer exists. Showing the latest.') expect(calls.updates).toHaveLength(0) }) @@ -336,7 +342,7 @@ describe('POST /api/transactions/categorize', () => { makeSupabase({ row: ROW, update: { error: null, count: 0 } }) const res = await post({ transactionId: 'txn-1', category: 'Grocery' }) expect(res.status).toBe(409) - expect((await res.json()).error).toBe('This transaction just changed. Refresh and try again.') + expect((await res.json()).error).toBe('This transaction just changed. Showing the latest.') }) // A count the server did not send is not a count of one. Treating it as a write would report a @@ -348,10 +354,10 @@ describe('POST /api/transactions/categorize', () => { }) // Category names match exactly, as stored. A near-miss is not that category. - it('answers 400 for a category in the wrong case, and writes nothing', async () => { + it('answers 409 for a category in the wrong case, and writes nothing', async () => { const calls = makeSupabase({ row: ROW }) const res = await post({ transactionId: 'txn-1', category: 'grocery' }) - expect(res.status).toBe(400) + expect(res.status).toBe(409) expect(calls.updates).toHaveLength(0) }) diff --git a/tests/unit/category-picker.test.tsx b/tests/unit/category-picker.test.tsx index 4d246c4..ab8156d 100644 --- a/tests/unit/category-picker.test.tsx +++ b/tests/unit/category-picker.test.tsx @@ -14,7 +14,8 @@ afterEach(() => { // A real spy, not a bare closure. With `refresh: () => {}` no test can tell whether the list is ever // told to reload, so deleting the call, or making it run after a refused save, leaves the suite // green. A refused save writes nothing: the server holds the old value, which the picker already -// shows while the error explains why, so a reload there is a wasted round trip. +// shows while the error explains why, so a reload there is a wasted round trip. The exception is a +// refusal that says the row is stale, where showing the latest is the point (#139). const refresh = vi.hoisted(() => vi.fn()) vi.mock('next/navigation', () => ({ useRouter: () => ({ refresh }) })) @@ -30,8 +31,9 @@ const select = () => screen.getByRole('combobox') as HTMLSelectElement // The route always answers in JSON, so every stand-in for its reply carries that content type. const JSON_TYPE = { 'content-type': 'application/json' } -const reply = (ok: boolean, body: unknown) => ({ +const reply = (ok: boolean, body: unknown, status = ok ? 200 : 400) => ({ ok, + status, redirected: false, headers: new Headers(JSON_TYPE), json: async () => body, @@ -41,7 +43,7 @@ describe('CategoryPicker', () => { // #28 spec §8.2: keyed on source and value, so a row whose REASON for a name changes (a rule equal // to the bank's category is removed: rule → bank, same name) still drops a stale alert. it('clears its alert when the source changes and the name does not', async () => { - vi.stubGlobal('fetch', vi.fn().mockResolvedValue(reply(false, { error: 'This transaction just changed. Refresh and try again.' }))) + vi.stubGlobal('fetch', vi.fn().mockResolvedValue(reply(false, { error: 'A credit-card payment is already kept out of spending and income.' }))) const { rerender } = render() fireEvent.change(select(), { target: { value: 'Shopping' } }) expect(await screen.findByRole('alert')).toBeTruthy() @@ -95,7 +97,8 @@ describe('CategoryPicker', () => { }) // A refused save wrote nothing, so the server still holds the old value. The picker already shows - // it while the error explains why; a refresh would only re-render the same value. + // it while the error explains why; a refresh would only re-render the same value. A refusal that + // says the row is stale (404 or 409) is the exception, below. it('does not reload the page when the save is refused', async () => { vi.stubGlobal( 'fetch', @@ -429,3 +432,47 @@ describe('CategoryPicker', () => { expect(refresh).toHaveBeenCalledTimes(1) }) }) + + +// #139: the route's 404 and 409 mean this row is stale: the transaction is gone or the bank removed +// it, the category was deleted, or the bank changed the row during the save. The route used to say +// "Refresh and try again", and in the installed app there's nothing to refresh with. So the picker +// reloads itself, and the words now say the latest is showing. +describe('CategoryPicker: a refusal that says this row is stale (#139)', () => { + const STALE = [ + [404, 'That transaction no longer exists. Showing the latest.'], + [409, 'That category no longer exists. Showing the latest.'], + ] as const + + it.each(STALE)('reloads after a %s, and shows its words', async (status, words) => { + vi.stubGlobal('fetch', vi.fn(async () => reply(false, { error: words }, status))) + render() + fireEvent.change(select(), { target: { value: 'Shopping' } }) + await waitFor(() => expect(screen.getByRole('alert').textContent).toBe(words)) + expect(select().value).toBe('Food & Drink') + expect(refresh).toHaveBeenCalledTimes(1) + }) + + // A deleted category: the reload leaves the row as it was, so the message stays while the options + // lose the category. Like any refusal's, it goes when the server's value next changes. + it('keeps its message while the reload leaves the row as it was', async () => { + vi.stubGlobal('fetch', vi.fn(async () => reply(false, { error: STALE[1][1] }, 409))) + const { rerender } = render() + fireEvent.change(select(), { target: { value: 'Shopping' } }) + await waitFor(() => expect(screen.getByRole('alert')).toBeTruthy()) + rerender() + expect(screen.getByRole('alert').textContent).toBe(STALE[1][1]) + expect([...select().options].map((o) => o.value)).toEqual(['Food & Drink', 'Travel']) + rerender() + expect(screen.queryByRole('alert')).toBeNull() + }) + + // Any other refusal wrote nothing and left the row as it was, so there's nothing to reload. + it.each([400, 500, 503])('does not reload after a %s', async (status) => { + vi.stubGlobal('fetch', vi.fn(async () => reply(false, { error: 'nope' }, status))) + render() + fireEvent.change(select(), { target: { value: 'Shopping' } }) + await waitFor(() => expect(screen.getByRole('alert')).toBeTruthy()) + expect(refresh).not.toHaveBeenCalled() + }) +}) diff --git a/tests/unit/reimbursable-checkbox.test.tsx b/tests/unit/reimbursable-checkbox.test.tsx index 8e5f29d..2267443 100644 --- a/tests/unit/reimbursable-checkbox.test.tsx +++ b/tests/unit/reimbursable-checkbox.test.tsx @@ -135,9 +135,9 @@ describe('ReimbursableCheckbox', () => { // nothing and needs no reload, unless it says this row is stale (404, 409). An unknown outcome may // have written, so it reloads to show what the server has. it.each([ - ['a refusal', () => refused(400, 'Your bank removed that transaction. Refresh and try again.'), 'Your bank removed that transaction. Refresh and try again.', 0], - ['a 404', () => refused(404, 'That transaction no longer exists. Refresh and try again.'), 'That transaction no longer exists. Refresh and try again.', 1], - ['a 409', () => refused(409, 'This transaction just changed. Refresh and try again.'), 'This transaction just changed. Refresh and try again.', 1], + ['a refusal', () => refused(400, 'A credit-card payment is already kept out of spending and income.'), 'A credit-card payment is already kept out of spending and income.', 0], + ['a 404', () => refused(404, 'That transaction no longer exists. Showing the latest.'), 'That transaction no longer exists. Showing the latest.', 1], + ['a 409', () => refused(409, 'This transaction just changed. Showing the latest.'), 'This transaction just changed. Showing the latest.', 1], ['a redirect to the login page', redirectedToLogin, 'Your session ended. Sign in again.', 0], // The #103 bug: any OK reply used to count as saved. ['a 200 without ok: true', () => new Response('{}', { headers: { 'content-type': 'application/json' } }), 'That could not be saved.', 0], diff --git a/tests/unit/reimbursable-editor.test.tsx b/tests/unit/reimbursable-editor.test.tsx index ade8132..278b460 100644 --- a/tests/unit/reimbursable-editor.test.tsx +++ b/tests/unit/reimbursable-editor.test.tsx @@ -208,9 +208,9 @@ describe('ReimbursableEditor', () => { // what was typed, so a retry is one tap, and says why. A refusal wrote nothing and needs no reload // unless it says this row is stale (404, 409); an unknown outcome may have written, so it reloads. it.each([ - ['a refusal', () => refused(400, 'Your bank removed that transaction. Refresh and try again.'), 'Your bank removed that transaction. Refresh and try again.', 0], - ['a 404', () => refused(404, 'That transaction no longer exists. Refresh and try again.'), 'That transaction no longer exists. Refresh and try again.', 1], - ['a 409', () => refused(409, 'This transaction just changed. Refresh and try again.'), 'This transaction just changed. Refresh and try again.', 1], + ['a refusal', () => refused(400, 'A credit-card payment is already kept out of spending and income.'), 'A credit-card payment is already kept out of spending and income.', 0], + ['a 404', () => refused(404, 'That transaction no longer exists. Showing the latest.'), 'That transaction no longer exists. Showing the latest.', 1], + ['a 409', () => refused(409, 'This transaction just changed. Showing the latest.'), 'This transaction just changed. Showing the latest.', 1], ['a redirect to the login page', redirectedToLogin, 'Your session ended. Sign in again.', 0], // The #103 bug: any OK reply used to close the dialog as if saved. ['a 200 without ok: true', () => new Response('{}', { headers: { 'content-type': 'application/json' } }), 'That could not be saved.', 0], @@ -276,8 +276,8 @@ describe('ReimbursableEditor', () => { const escape = () => fireEvent(screen.getByRole('dialog'), new Event('cancel', { bubbles: false, cancelable: true })) escape() expect(screen.getByRole('dialog')).toBeTruthy() - settle(refused(400, 'Your bank removed that transaction. Refresh and try again.')) - expect((await screen.findByRole('alert')).textContent).toBe('Your bank removed that transaction. Refresh and try again.') + settle(refused(400, 'A credit-card payment is already kept out of spending and income.')) + expect((await screen.findByRole('alert')).textContent).toBe('A credit-card payment is already kept out of spending and income.') expect(screen.getByRole('dialog')).toBeTruthy() // Settled, so Escape is the way out again. escape() diff --git a/tests/unit/reimbursable-route.test.ts b/tests/unit/reimbursable-route.test.ts index 39c7f19..6bc1a8d 100644 --- a/tests/unit/reimbursable-route.test.ts +++ b/tests/unit/reimbursable-route.test.ts @@ -179,17 +179,17 @@ describe('PATCH /api/reimbursable', () => { const calls = makeSupabase({ row: null }) const res = await mark() expect(res.status).toBe(404) - expect(await errorOf(res)).toBe('That transaction no longer exists. Refresh and try again.') + expect(await errorOf(res)).toBe('That transaction no longer exists. Showing the latest.') expect(calls.updates).toHaveLength(0) }) // `removed` is a soft flag (a Plaid repost), not a delete. A mark written to a removed row would be - // unreachable while still counting toward what you're owed. - it('answers 400 for a transaction the bank removed, and writes nothing', async () => { + // unreachable while still counting toward what you're owed. 409, so the editors reload (#139). + it('answers 409 for a transaction the bank removed, and writes nothing', async () => { const calls = makeSupabase({ row: { ...TXN, removed: true } }) const res = await mark() - expect(res.status).toBe(400) - expect(await errorOf(res)).toBe('Your bank removed that transaction. Refresh and try again.') + expect(res.status).toBe(409) + expect(await errorOf(res)).toBe('Your bank removed that transaction. Showing the latest.') expect(calls.updates).toHaveLength(0) }) @@ -262,7 +262,7 @@ describe('PATCH /api/reimbursable', () => { makeSupabase({ row, rowAtWrite }) const res = await mark() expect(res.status).toBe(409) - expect(await errorOf(res)).toBe('This transaction just changed. Refresh and try again.') + expect(await errorOf(res)).toBe('This transaction just changed. Showing the latest.') expect(logs).toContainEqual([ '[reimbursable] update matched no rows (row changed, or the update policy is missing)', { id: 'txn-1' }, @@ -309,7 +309,7 @@ describe('PATCH /api/reimbursable', () => { makeSupabase({ update: { error: null, count: 0 } }) const res = await mark() expect(res.status).toBe(409) - expect(await errorOf(res)).toBe('This transaction just changed. Refresh and try again.') + expect(await errorOf(res)).toBe('This transaction just changed. Showing the latest.') expect(logs).toContainEqual([ '[reimbursable] update matched no rows (row changed, or the update policy is missing)', { id: 'txn-1' }, diff --git a/tests/unit/transaction-card.test.tsx b/tests/unit/transaction-card.test.tsx index ee41f8b..df87516 100644 --- a/tests/unit/transaction-card.test.tsx +++ b/tests/unit/transaction-card.test.tsx @@ -738,12 +738,14 @@ describe('TransactionCard learned marker (#28)', () => { // CategoryPicker keys its stale-alert reset on source as well as name, so the sheet must hand it // the source: a rule equal to the bank's category being removed changes the reason, not the name. + // A refusal that isn't about a stale row, since a stale one's words stay through the reload (#139). it("passes the category's source to the picker, so a change of reason clears its alert", async () => { vi.stubGlobal('fetch', vi.fn().mockResolvedValue({ ok: false, + status: 400, redirected: false, headers: new Headers({ 'content-type': 'application/json' }), - json: async () => ({ error: 'This transaction just changed. Refresh and try again.' }), + json: async () => ({ error: 'A credit-card payment is already kept out of spending and income.' }), })) const agreeing = { name: 'Food', source: 'rule' as const, ruleId: 'r1', bankName: 'Food' } const { rerender } = render() diff --git a/tests/unit/transaction-row.test.tsx b/tests/unit/transaction-row.test.tsx index c113309..c93a0ce 100644 --- a/tests/unit/transaction-row.test.tsx +++ b/tests/unit/transaction-row.test.tsx @@ -229,7 +229,7 @@ describe('TransactionRow learned marker (#28)', () => { // A save error renders inside the picker container, so it cannot push the marker off the line. it('keeps the marker beside the select while an error shows', async () => { - vi.stubGlobal('fetch', vi.fn().mockResolvedValue({ ok: false, redirected: false, headers: new Headers({ 'content-type': 'application/json' }), json: async () => ({ error: 'This transaction just changed. Refresh and try again.' }) })) + vi.stubGlobal('fetch', vi.fn().mockResolvedValue({ ok: false, status: 400, redirected: false, headers: new Headers({ 'content-type': 'application/json' }), json: async () => ({ error: 'A credit-card payment is already kept out of spending and income.' }) })) const { container } = renderRow({}, LEARNED) fireEvent.change(screen.getByRole('combobox'), { target: { value: 'Food' } }) const alert = await screen.findByRole('alert') @@ -269,14 +269,16 @@ describe('TransactionRow suggested marker (#28 §14.6)', () => { }) // CategoryPicker keys its stale-alert reset on source as well as name, so the row must hand it the -// source: a rule equal to the bank's category being removed changes the reason, not the name. +// source: a rule equal to the bank's category being removed changes the reason, not the name. A +// refusal that isn't about a stale row, since a stale one's words stay through the reload (#139). describe('TransactionRow category source', () => { it("passes the category's source to the picker, so a change of reason clears its alert", async () => { vi.stubGlobal('fetch', vi.fn().mockResolvedValue({ ok: false, + status: 400, redirected: false, headers: new Headers({ 'content-type': 'application/json' }), - json: async () => ({ error: 'This transaction just changed. Refresh and try again.' }), + json: async () => ({ error: 'A credit-card payment is already kept out of spending and income.' }), })) const agreeing: ResolvedCategory = { name: 'Food', source: 'rule', ruleId: 'r1', bankName: 'Food' } const row = (category: ResolvedCategory) => (