Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions app/api/reimbursable/route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 })
Expand Down
18 changes: 8 additions & 10 deletions app/api/transactions/categorize/route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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.
Expand All @@ -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
Expand All @@ -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.
Expand Down
12 changes: 10 additions & 2 deletions components/CategoryPicker.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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.
Expand Down
30 changes: 18 additions & 12 deletions tests/unit/categorize-route.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand All @@ -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)
})

Expand Down Expand Up @@ -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)
})

Expand Down Expand Up @@ -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
Expand All @@ -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)
})

Expand Down
55 changes: 51 additions & 4 deletions tests/unit/category-picker.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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 }) }))

Expand All @@ -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,
Expand All @@ -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(<CategoryPicker {...props} source="rule" />)
fireEvent.change(select(), { target: { value: 'Shopping' } })
expect(await screen.findByRole('alert')).toBeTruthy()
Expand Down Expand Up @@ -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',
Expand Down Expand Up @@ -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(<CategoryPicker {...props} />)
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(<CategoryPicker {...props} />)
fireEvent.change(select(), { target: { value: 'Shopping' } })
await waitFor(() => expect(screen.getByRole('alert')).toBeTruthy())
rerender(<CategoryPicker {...props} options={['Food & Drink', 'Travel']} />)
expect(screen.getByRole('alert').textContent).toBe(STALE[1][1])
expect([...select().options].map((o) => o.value)).toEqual(['Food & Drink', 'Travel'])
rerender(<CategoryPicker {...props} options={['Food & Drink', 'Travel']} value="Travel" source="pick" />)
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(<CategoryPicker {...props} />)
fireEvent.change(select(), { target: { value: 'Shopping' } })
await waitFor(() => expect(screen.getByRole('alert')).toBeTruthy())
expect(refresh).not.toHaveBeenCalled()
})
})
6 changes: 3 additions & 3 deletions tests/unit/reimbursable-checkbox.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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],
Expand Down
10 changes: 5 additions & 5 deletions tests/unit/reimbursable-editor.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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],
Expand Down Expand Up @@ -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()
Expand Down
Loading
Loading