fix(intake): clear remove_background when an admin restores a photo (#281)
item_drafts.remove_background is written once at intake and never updated afterward. Restoring a photo clears original_image_path, which is exactly what makes the row look "never cut out" to removeImageBackground — so a submitter's ticked checkbox, followed by the worker cutting the photo out, followed by an admin restoring a poor result, followed by a click on Regenerate, would silently re-cut the same photo the admin had just put back. Nothing was lost, but the control the design calls "what makes a poor result survivable" was quietly defeated by the button sitting next to it. restoreImageOriginal now swaps the image's paths back and clears item_drafts.remove_background for that item in one transaction, so a restore that succeeds while the flag update fails cannot reintroduce the bug. An admin restoring any photo on an item is treated as overriding the submitter's original request for the whole item — the flag is per-item while the swap is per-photo, so there is no narrower place to record the decision, and turning off the whole item's auto-removal is the conservative direction: the alternative is re-cutting something a person deliberately undid. Added an integration test in draftingBackgroundRemoval.integration.test.ts that drafts a submission with the intent set, cuts it out, restores it, mirrors what the admin's Regenerate button does (state back to queued, attempts cleared), runs the worker again, and asserts the photo is still not cut out. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -110,23 +110,58 @@ export async function removeImageBackground(imageId: number): Promise<void> {
|
||||
}
|
||||
|
||||
/**
|
||||
* Puts the original back.
|
||||
* Puts the original back, and turns off the submitter's auto-removal intent
|
||||
* for the item this photo belongs to.
|
||||
*
|
||||
* The cut-out file is left on disk deliberately. Removing a background is
|
||||
* exactly the operation that produces an occasional bad result on an unusual
|
||||
* object, so somebody restoring one is quite likely to try again — and this
|
||||
* module deletes nothing in any case.
|
||||
*
|
||||
* `item_drafts.remove_background` is written once at intake and otherwise
|
||||
* never updated — without this, a restored photo looks identical to one that
|
||||
* was simply never cut out, and Regenerate reads the same stale `true` and
|
||||
* cuts it out again, quietly undoing the admin's decision. An admin restoring
|
||||
* *any* photo on an item has overridden the submitter's request for that
|
||||
* item: the flag is per-item while the swap is per-photo, so there is no
|
||||
* per-photo place to record "leave this one alone" separately. Turning the
|
||||
* whole item's auto-removal off is the conservative direction — the
|
||||
* alternative is a worker that re-cuts a photo a person deliberately undid,
|
||||
* which is the bug this fixes.
|
||||
*
|
||||
* Both writes happen in one transaction so a restore that succeeded while the
|
||||
* flag update failed cannot reintroduce the bug it exists to close.
|
||||
*/
|
||||
export async function restoreImageOriginal(imageId: number): Promise<void> {
|
||||
const { rowCount } = await pool.query(
|
||||
const client = await pool.connect();
|
||||
try {
|
||||
await client.query('BEGIN');
|
||||
const { rows } = await client.query<{ item_id: number }>(
|
||||
`UPDATE item_images
|
||||
SET image_path = original_image_path, original_image_path = NULL
|
||||
WHERE id = $1 AND original_image_path IS NOT NULL`,
|
||||
WHERE id = $1 AND original_image_path IS NOT NULL
|
||||
RETURNING item_id`,
|
||||
[imageId]
|
||||
);
|
||||
if (rowCount === 0) {
|
||||
const restored = rows[0];
|
||||
if (!restored) {
|
||||
await client.query('ROLLBACK');
|
||||
throw new NoOriginalToRestoreError(`image ${imageId} has no original to restore`);
|
||||
}
|
||||
|
||||
await client.query(`UPDATE item_drafts SET remove_background = false WHERE item_id = $1`, [
|
||||
restored.item_id
|
||||
]);
|
||||
|
||||
await client.query('COMMIT');
|
||||
} catch (err) {
|
||||
if (!(err instanceof NoOriginalToRestoreError)) {
|
||||
await client.query('ROLLBACK');
|
||||
}
|
||||
throw err;
|
||||
} finally {
|
||||
client.release();
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -294,6 +294,10 @@ router.post(
|
||||
* occasional poor result on an unusual object, and this makes that survivable
|
||||
* rather than something to prevent. Nothing is deleted: the cut-out file stays
|
||||
* on disk, because somebody restoring one is quite likely to try again.
|
||||
*
|
||||
* restoreImageOriginal also turns off remove_background for this item, so a
|
||||
* later Regenerate does not silently re-cut a photo the admin just put back —
|
||||
* see the reasoning on that function.
|
||||
*/
|
||||
router.post(
|
||||
'/:itemId/images/:imageId/restore-original',
|
||||
|
||||
@@ -8,6 +8,7 @@ import { resetDb, closeDb } from './setup/testDb';
|
||||
import { draftQueued } from '../../src/intake/draftingWorker';
|
||||
import { resetAnthropicClient } from '../../src/intake/anthropicClient';
|
||||
import { draftListing } from '../../src/intake/draftListing';
|
||||
import { restoreImageOriginal } from '../../src/intake/backgroundRemoval';
|
||||
|
||||
/**
|
||||
* The worker's background-removal step (#281).
|
||||
@@ -150,4 +151,39 @@ describe('background removal after a draft', () => {
|
||||
expect(rows[0]?.state).toBe('ready');
|
||||
expect(await originalPathOf(itemId)).toBeNull();
|
||||
});
|
||||
|
||||
// The bug this closes: remove_background is written once at intake and
|
||||
// otherwise never updated, so a restored photo used to look identical to
|
||||
// one that was simply never cut out — and Regenerate would read the same
|
||||
// stale `true` and cut it out again, quietly undoing what the admin just
|
||||
// did. restoreImageOriginal now clears the flag as part of the restore
|
||||
// itself, so the worker respects the admin's decision on the next pass.
|
||||
it('does not re-cut a photo the admin restored, even after Regenerate', async () => {
|
||||
const itemId = await seedSubmissionWithPhoto({ removeBackground: true });
|
||||
await draftQueued(1);
|
||||
expect(await originalPathOf(itemId)).toBe('/uploads/worker.jpg');
|
||||
|
||||
const { rows: imageRows } = await pool.query<{ id: number }>(
|
||||
`SELECT id FROM item_images WHERE item_id = $1`,
|
||||
[itemId]
|
||||
);
|
||||
await restoreImageOriginal(imageRows[0]!.id);
|
||||
|
||||
const { rows: flagRows } = await pool.query<{ remove_background: boolean }>(
|
||||
`SELECT remove_background FROM item_drafts WHERE item_id = $1`,
|
||||
[itemId]
|
||||
);
|
||||
expect(flagRows[0]?.remove_background).toBe(false);
|
||||
|
||||
// Mirrors what the admin's Regenerate button does: back to queued, with
|
||||
// attempts cleared, so the worker picks the item up again.
|
||||
await pool.query(
|
||||
`UPDATE item_drafts SET state = 'queued', attempts = 0, ai_error = NULL WHERE item_id = $1`,
|
||||
[itemId]
|
||||
);
|
||||
|
||||
await draftQueued(1);
|
||||
|
||||
expect(await originalPathOf(itemId)).toBeNull();
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user