diff --git a/backend/src/intake/backgroundRemoval.ts b/backend/src/intake/backgroundRemoval.ts index 2659d68..d19f8d1 100644 --- a/backend/src/intake/backgroundRemoval.ts +++ b/backend/src/intake/backgroundRemoval.ts @@ -189,14 +189,19 @@ export interface RemovalSummary { /** * What a whole-item restore did. * - * No `failed`, because restoring cannot fail the way removing can: it is a - * database swap with no sidecar in it, and a photo that was never cut out is - * skipped rather than being an error. + * Carries `failed` for the same reason `RemovalSummary` does. Restoring is a + * database swap with no sidecar in it, so it fails far less often than + * removing does — but "far less often" is not "never", and a database error + * partway through a four-photo restore is exactly the moment an admin needs + * the count rather than a bare 500. A photo that was never cut out is skipped + * rather than being an error either way. */ export interface RestoreSummary { total: number; /** How many were put back. Photos that were never cut out are not counted. */ restored: number; + /** Whether it stopped early because one of them failed. */ + failed: boolean; } /** Every photo of one item, in order. */ @@ -252,6 +257,26 @@ export async function removeBackgroundsForItem(itemId: number): Promise { const imageIds = await imageIdsFor(itemId); @@ -262,10 +287,14 @@ export async function restoreOriginalsForItem(itemId: number): Promise { // // This is the sender's tick from the submission page, honoured here so // they never waited for it — and a failure must not mark a draft that was - // written correctly as failed. The photo keeps its original in that case, - // and the admin's per-photo control is still there to do it by hand. + // written correctly as failed. + // + // removeBackgroundsForItem no longer rejects over a single photo failing + // (#293 gave it a summary instead, for the admin screen that acts on the + // count) — so this .catch now fires only if the image-listing query + // itself throws, which is rare enough to warrant a log and nothing more. + // A per-photo failure comes back as `failed: true` in the summary, which + // this sweep discards; the photo keeps its original in that case, and the + // admin's per-photo control in the review queue is still there to do it + // by hand. // // Awaited, unlike the notification below, so a sweep that has returned // has finished its work. Nothing is waiting on this: the worker is off diff --git a/backend/src/routes/admin.ts b/backend/src/routes/admin.ts index 6e6c1cd..f6021e0 100755 --- a/backend/src/routes/admin.ts +++ b/backend/src/routes/admin.ts @@ -352,7 +352,7 @@ router.post('/items/:id/mark-available', asyncRoute(async (req: Request, res: Re })); /** - * One item's images, or null when the item does not exist. + * Whether an item with this id exists. * * Checked before acting so an absent item is a 404 rather than a cheerful * summary of nothing. `removeBackgroundsForItem` would happily report @@ -396,6 +396,11 @@ router.post('/items/:id/remove-backgrounds', asyncRoute(async (req: Request, res * The reason removing is safe to try. Photos that were never cut out are * skipped rather than refused, so a half-done item — what a partial failure * leaves behind — is restorable too. + * + * Answers 200 once the id is valid, same as remove-backgrounds and for the + * same reason: `restoreOriginalsForItem` stops at the first genuine failure + * rather than throwing, so there is always a summary to return, never a bare + * 500 that discards how far it got. */ router.post('/items/:id/restore-originals', asyncRoute(async (req: Request, res: Response) => { const itemId = readId(req.params.id); diff --git a/backend/tests/integration/adminItemBackgrounds.integration.test.ts b/backend/tests/integration/adminItemBackgrounds.integration.test.ts index 5093f4b..72e3c0f 100644 --- a/backend/tests/integration/adminItemBackgrounds.integration.test.ts +++ b/backend/tests/integration/adminItemBackgrounds.integration.test.ts @@ -142,7 +142,7 @@ describe('restoring every original on an item', () => { const res = await request(app).post(`/api/admin/items/${itemId}/restore-originals`); expect(res.status).toBe(200); - expect(res.body).toEqual({ total: 2, restored: 2 }); + expect(res.body).toEqual({ total: 2, restored: 2, failed: false }); }); it('answers 404 for an id that cannot be read', async () => { @@ -150,6 +150,15 @@ describe('restoring every original on an item', () => { expect((await request(app).post('/api/admin/items/abc/restore-originals')).status).toBe(404); }); + + // remove-backgrounds has always had this case; restore-originals did not, + // and the two handlers are copy-paste rather than a shared helper — nothing + // would have caught them diverging. + it('answers 404 for an item that does not exist', async () => { + await startStub(answerWithPng); + + expect((await request(app).post('/api/admin/items/999999/restore-originals')).status).toBe(404); + }); }); // Extra scope beyond the endpoints themselves: Task 3 derives its button label diff --git a/backend/tests/integration/backgroundRemoval.integration.test.ts b/backend/tests/integration/backgroundRemoval.integration.test.ts index 4d3ffd1..297ea03 100644 --- a/backend/tests/integration/backgroundRemoval.integration.test.ts +++ b/backend/tests/integration/backgroundRemoval.integration.test.ts @@ -381,7 +381,7 @@ describe('putting every original back', () => { const summary = await restoreOriginalsForItem(itemId); - expect(summary).toEqual({ total: 2, restored: 2 }); + expect(summary).toEqual({ total: 2, restored: 2, failed: false }); const { rows: after } = await pool.query<{ image_path: string }>( `SELECT image_path FROM item_images WHERE item_id = $1 ORDER BY sort_order`, @@ -412,6 +412,6 @@ describe('putting every original back', () => { ); await removeImageBackground(images[0]!.id); - expect(await restoreOriginalsForItem(itemId)).toEqual({ total: 2, restored: 1 }); + expect(await restoreOriginalsForItem(itemId)).toEqual({ total: 2, restored: 1, failed: false }); }); }); diff --git a/backend/tests/unit/backgroundRemoval.test.ts b/backend/tests/unit/backgroundRemoval.test.ts index 23bf41b..a9a9ea5 100644 --- a/backend/tests/unit/backgroundRemoval.test.ts +++ b/backend/tests/unit/backgroundRemoval.test.ts @@ -1,4 +1,41 @@ -import { cutoutPathFor } from '../../src/intake/backgroundRemoval'; +import { pool } from '../../src/db'; +import { cutoutPathFor, restoreOriginalsForItem } from '../../src/intake/backgroundRemoval'; + +// The database module is replaced outright rather than spied on. This has to +// be a unit test: the only failure `restoreOriginalsForItem` can report is a +// database fault, and the only way to inject one is to make a query fail on +// command. Doing that in the integration suite means interfering with the +// single pool every suite in the same `--runInBand` process shares, and that +// `afterAll` calls `pool.end()` on — which made the whole suite unrunnable. +// Here no `Pool` is ever constructed: jest gives this file its own module +// registry, so there is nothing shared to break. +jest.mock('../../src/db', () => ({ pool: { query: jest.fn(), connect: jest.fn() } })); + +const mockPool = pool as unknown as { query: jest.Mock; connect: jest.Mock }; + +/** The distinctive text of the swap that puts one photo back. */ +const SWAP_SQL = 'SET image_path = original_image_path'; + +/** + * A pool over `imageIds` whose nth restore swap fails. + * + * Matching on the swap's SQL rather than counting queries: `restoreImageOriginal` + * also issues BEGIN, the `item_drafts` update and COMMIT on the same client, so + * a bare call counter would break whichever query happened to land nth. + */ +function poolWhoseSwapFailsOn(imageIds: number[], failOnSwap: number): void { + mockPool.query.mockResolvedValue({ rows: imageIds.map((id) => ({ id })) }); + let swaps = 0; + mockPool.connect.mockImplementation(async () => ({ + query: jest.fn(async (sql: string) => { + if (!String(sql).includes(SWAP_SQL)) return { rows: [] }; + swaps += 1; + if (swaps === failOnSwap) throw new Error('database is down'); + return { rows: [{ item_id: 1 }] }; + }), + release: jest.fn() + })); +} describe('where a cut-out is written', () => { // A new file rather than a rewrite of the original, which is what makes the @@ -20,3 +57,42 @@ describe('where a cut-out is written', () => { expect(cutoutPathFor('/uploads/x.png')).toBe('/uploads/x-cutout.png'); }); }); + +describe('when a restore fails partway through an item', () => { + // Logged rather than thrown, so the log line is expected here; silenced to + // keep it out of the suite's output rather than because it does not matter. + let logged: jest.SpyInstance; + + beforeEach(() => { + jest.clearAllMocks(); + logged = jest.spyOn(console, 'error').mockImplementation(() => {}); + }); + + afterEach(() => { + logged.mockRestore(); + }); + + // The case the summary's `failed` field exists for: a genuine database + // failure partway through must not turn into a bare 500 that discards how + // far the restore got. It has to stop, report the count, and let the caller + // decide whether to retry — the same contract removeBackgroundsForItem has. + it('stops at the first genuine failure and reports how far it got', async () => { + poolWhoseSwapFailsOn([11, 22, 33], 2); + + await expect(restoreOriginalsForItem(4)).resolves.toEqual({ + total: 3, + restored: 1, + failed: true + }); + }); + + // The third photo is never attempted, which is what makes pressing Restore + // again worth something: it resumes rather than starting over. + it('does not go on to the photos after the one that failed', async () => { + poolWhoseSwapFailsOn([11, 22, 33], 2); + + await restoreOriginalsForItem(4); + + expect(mockPool.connect).toHaveBeenCalledTimes(2); + }); +});