restoreOriginalsForItem rethrew anything that was not NoOriginalToRestoreError, which handed asyncRoute a bare 500 and discarded how far the restore had already got. That breaks the invariant the feature is built on: photos restored before the failure really are back, and an admin standing in front of the modal needs the count to decide whether pressing the button again is worth anything. RestoreSummary now carries `failed` and the loop stops and reports, exactly the shape and the reasoning removeBackgroundsForItem already had. The restore-originals route gains the missing 404 for an item that does not exist — remove-backgrounds always had it, and the two handlers are copy-paste rather than a shared helper, so nothing would have caught them diverging. draftingWorker's .catch is now only reachable if the image-listing query itself throws, since removeBackgroundsForItem no longer rejects over a single photo; its comment says so rather than describing behaviour that has moved. The `failed` branch is covered by a unit test that stubs the database module in its own module registry. It cannot honestly be an integration test: the only failure the function can report is a database fault, and the only way to inject one into a real run is to interfere with the single pool every integration suite in the --runInBand process shares and that afterAll calls pool.end() on. Two tests that did exactly that are removed here — they left the suite reporting a failure against its own afterAll and leaking a handle that stopped it exiting. Nor is the fault reachable through data alone: the swap's WHERE original_image_path IS NOT NULL guarantees the value it writes into the NOT NULL image_path, and item_images carries no unique, check or foreign-key constraint on either column, so no row can be seeded that makes the statement fail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
99 lines
4.0 KiB
TypeScript
99 lines
4.0 KiB
TypeScript
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
|
|
// original restorable at all — and what makes the JPEG-to-PNG change free,
|
|
// since no existing path is renamed.
|
|
it('sits beside the original with a -cutout suffix and a .png extension', () => {
|
|
expect(cutoutPathFor('/uploads/abc-123.jpg')).toBe('/uploads/abc-123-cutout.png');
|
|
});
|
|
|
|
// image_path is a contract, not a string: #103 made the stored value the
|
|
// path uploadUrl joins an origin onto.
|
|
it('keeps the /uploads/ prefix', () => {
|
|
expect(cutoutPathFor('/uploads/x.webp')).toBe('/uploads/x-cutout.png');
|
|
});
|
|
|
|
// The extension is replaced rather than appended, so a second pass cannot
|
|
// produce `.png.png`.
|
|
it('replaces the extension rather than appending to it', () => {
|
|
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);
|
|
});
|
|
});
|