diff --git a/CHANGELOG.md b/CHANGELOG.md index 6a91faa..a9d8af5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,12 @@ Follows [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) · versioned wi ## [Unreleased] +## [1.38.1] — 2026-09-21 + +### Fixed +- **Magazine: "Could not approve: Unknown block ids".** After deleting a region, merging it, or changing a text region into an image, the review page still had the old id selected and Approve sent it; worse, it sent a stale image list that dropped a region just turned into an image. The review selection now follows every such change (a converted region keeps its selected state under its new id), Approve only sends ids that still exist, and deleting a region also removes it from every article that listed it. +- Keyboard focus in the review list now lands on the row that is focused when the list is redrawn, not the one focused when a change started. + ## [1.38.0] — 2026-09-21 ### Added diff --git a/VERSION b/VERSION index 4afb1d2..7bf334d 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.38.0 \ No newline at end of file +1.38.1 \ No newline at end of file diff --git a/routes/magazine.py b/routes/magazine.py index eebcbe2..2de97aa 100644 --- a/routes/magazine.py +++ b/routes/magazine.py @@ -1037,6 +1037,17 @@ async def magazine_delete_region(doc_id: str, page: int, block_id: str): raise HTTPException(404, "Region not found") layout = mag.delete_region(layout, block_id) mag.save_layout(d, layout) + # Also drop it from articles: an article still listing a deleted id made Approve fail with + # "Unknown block ids". + articles = mag.load_articles(d) + changed = False + for article in articles: + for key in ("blocks", "images"): + if block_id in (article.get(key) or []): + article[key] = [x for x in article[key] if x != block_id] + changed = True + if changed: + mag.save_articles(d, articles) return {"ok": True} diff --git a/static/index.html b/static/index.html index 3845709..5475b2f 100644 --- a/static/index.html +++ b/static/index.html @@ -27,12 +27,12 @@ - - - - - - + + + + + + diff --git a/static/js/magazine.js b/static/js/magazine.js index f0af3ff..c34932f 100644 --- a/static/js/magazine.js +++ b/static/js/magazine.js @@ -701,9 +701,10 @@ async function magReviewChangeType(regionId, type, select) { if (select) select.disabled = true; say(fromImage && !toImage ? 'Reading the region as text…' : 'Changing region type…', 'info'); try { - await api('/api/magazine/' + state.doc.id + '/pages/' + state.reviewPage + '/regions/' + encodeURIComponent(regionId), { + const r = await api('/api/magazine/' + state.doc.id + '/pages/' + state.reviewPage + '/regions/' + encodeURIComponent(regionId), { method: 'PATCH', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ type }), }); + if (r && r.id && r.id !== regionId) magRemapSelection(regionId, r.id, toImage); } catch (e) { if (select) select.disabled = false; return say('Could not change the region type: ' + e.message, 'err'); @@ -1536,10 +1537,11 @@ function magRegionUrl(id) { } async function magReloadReview(message) { - // Keep a keyboard user's place: the row that had focus gets it back after the list is redrawn. - const had = document.activeElement?.closest?.('[data-block-row]')?.dataset.blockRow; + // Keep a keyboard user's place: the row focused when the redraw happens (not when the change + // started -- the user may have moved on meanwhile) gets focus back. state.layout = null; await ensureLayout(); + const had = document.activeElement?.closest?.('[data-block-row]')?.dataset.blockRow; renderReview(); if (had) document.querySelector('#mag-review [data-block-row="' + CSS.escape(had) + '"] input[type="checkbox"]')?.focus(); if (message) say(message, 'ok'); @@ -1573,7 +1575,8 @@ function magRowEdit(id) { return magReloadReview('Text saved'); } if (act === 'split') { - await api(magRegionUrl(id) + '/split', { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ at: area.selectionStart }) }); + const r = await api(magRegionUrl(id) + '/split', { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ at: area.selectionStart }) }); + if (r?.new_id) magSelectionAddAlongside(id, r.new_id); return magReloadReview('Region split in two'); } magReloadReview(); @@ -1596,7 +1599,8 @@ async function magRowReocr(id) { async function magRowMerge(id) { try { - await api(magRegionUrl(id) + '/merge-next', { method: 'POST' }); + const r = await api(magRegionUrl(id) + '/merge-next', { method: 'POST' }); + if (r?.removed) magRemapSelection(r.removed, null, false); } catch (e) { return say('Could not merge: ' + e.message, 'err'); } @@ -1635,6 +1639,7 @@ async function magReviewDeleteRegion(blockId) { await api('/api/magazine/' + state.doc.id + '/pages/' + state.reviewPage + '/regions/' + encodeURIComponent(blockId), { method: 'DELETE', }); + magRemapSelection(blockId, null, false); } catch (e) { return say('Could not delete region: ' + e.message, 'err'); } @@ -1846,10 +1851,35 @@ async function magReviewSubmitDraw() { renderReview(); } +// Keep the review's unsaved selection in step with a region change made on the server. Without this, +// Approve sent ids that no longer existed ("Unknown block ids") after a delete, merge or type change, +// and sent a stale image list that dropped a text box just turned into an image. +function magRemapSelection(oldId, newId, newIsImage) { + const a = state.article; + if (!a) return; + if (!_magReviewChecked) _magReviewChecked = new Set(a.blocks || []); + const wasOn = _magReviewChecked.has(oldId) || (a.images || []).includes(oldId); + _magReviewChecked.delete(oldId); + a.blocks = (a.blocks || []).filter((id) => id !== oldId); + a.images = (a.images || []).filter((id) => id !== oldId); + if (!newId || !wasOn) return; + if (newIsImage) a.images = a.images.concat([newId]); + else { _magReviewChecked.add(newId); a.blocks = a.blocks.concat([newId]); } +} + +// A region split in two: the new part is included wherever the original was. +function magSelectionAddAlongside(existingId, newId) { + const a = state.article; + if (!a) return; + if (!_magReviewChecked) _magReviewChecked = new Set(a.blocks || []); + if (_magReviewChecked.has(existingId)) { _magReviewChecked.add(newId); a.blocks = (a.blocks || []).concat([newId]); } +} + function selectedBlockIds() { - if (_magReviewChecked) return Array.from(_magReviewChecked); - return Array.from(document.querySelectorAll('#mag-review input[data-block]:checked')) - .map((el) => el.dataset.block); + const exists = new Set((state.layout?.blocks || []).map((b) => b.id)); + const ids = _magReviewChecked ? Array.from(_magReviewChecked) + : Array.from(document.querySelectorAll('#mag-review input[data-block]:checked')).map((el) => el.dataset.block); + return state.layout ? ids.filter((id) => exists.has(id)) : ids; } async function magReocrPage() { @@ -1945,7 +1975,7 @@ async function magApprove() { headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ blocks, - images: a.images || [], + images: (a.images || []).filter((id) => !state.layout || (state.layout.images || []).some((img) => img.id === id)), status: 'approved', title: ($('mag-rev-title')?.value || a.title).trim(), author: ($('mag-rev-author')?.value || a.author).trim(), diff --git a/tests/magazine.spec.js b/tests/magazine.spec.js index bdedd8b..66fc6cd 100644 --- a/tests/magazine.spec.js +++ b/tests/magazine.spec.js @@ -545,3 +545,39 @@ test('the review list can be worked from the keyboard', async ({ page }) => { await page.keyboard.press('Enter'); await expect(page.locator('[data-block-row="b3"]').getByRole('textbox', { name: 'Text of this region' })).toBeFocused(); }); + +test('approving after turning a ticked text box into an image sends the new image, not the old id', async ({ page }) => { + await mockBackend(page); + let converted = false; + await page.route(`**/api/magazine/${DOC_ID}/layout`, route => route.fulfill({ json: converted + ? { ...LAYOUT, blocks: LAYOUT.blocks.filter(b => b.id !== 'b3'), images: [{ id: 'p0003-manual-000', page: 3, bbox: [50, 150, 200, 300], filename: 'x.png', reading_order: 2 }] } + : LAYOUT })); + await page.route(`**/api/magazine/${DOC_ID}/pages/3/regions/b3`, route => { + converted = true; + return route.fulfill({ json: { ok: true, id: 'p0003-manual-000', type: 'image' } }); + }); + await openReview(page); + await expect(page.locator('#mag-review input[data-block="b3"]')).toBeChecked(); + await page.locator('[data-block-row="b3"] select.mag-block-type-select').selectOption('image'); + await expect(page.locator('[data-block-row="p0003-manual-000"]')).toBeVisible(); + const request = page.waitForRequest(r => r.method() === 'PATCH' && r.url().includes('/articles/article-001')); + await page.getByRole('button', { name: 'Approve', exact: true }).click(); + const body = (await request).postDataJSON(); + expect(body.blocks).not.toContain('b3'); + expect(body.images).toContain('p0003-manual-000'); +}); + +test('approving after deleting a ticked box does not send its id', async ({ page }) => { + await mockBackend(page); + let deleted = false; + await page.route(`**/api/magazine/${DOC_ID}/layout`, route => route.fulfill({ json: deleted + ? { ...LAYOUT, blocks: LAYOUT.blocks.filter(b => b.id !== 'b3') } : LAYOUT })); + await page.route(`**/api/magazine/${DOC_ID}/pages/3/regions/b3`, route => { deleted = true; return route.fulfill({ json: { ok: true } }); }); + await openReview(page); + await page.locator('[data-block-row="b3"]').getByRole('button', { name: /Delete region/ }).click(); + await page.getByRole('alertdialog').getByRole('button', { name: 'Delete', exact: true }).click(); + await expect(page.locator('[data-block-row="b3"]')).toHaveCount(0); + const request = page.waitForRequest(r => r.method() === 'PATCH' && r.url().includes('/articles/article-001')); + await page.getByRole('button', { name: 'Approve', exact: true }).click(); + expect((await request).postDataJSON().blocks).not.toContain('b3'); +}); diff --git a/tests/test_magazine_routes.py b/tests/test_magazine_routes.py index 3bfe7a5..7edb988 100644 --- a/tests/test_magazine_routes.py +++ b/tests/test_magazine_routes.py @@ -544,7 +544,8 @@ class DeleteRegionRouteTests(unittest.IsolatedAsyncioTestCase): layout = {"blocks": [{"id": "b1", "page": 1}], "images": []} with patch('routes.magazine._require_magazine', return_value=(None, {})), \ patch('routes.magazine.mag.load_layout', return_value=layout), \ - patch('routes.magazine.mag.save_layout') as save: + patch('routes.magazine.mag.save_layout') as save, \ + patch('routes.magazine.mag.load_articles', return_value=[]): result = await magazine_delete_region('a' * 32, 1, 'b1') self.assertTrue(result['ok']) save.assert_called_once() @@ -555,7 +556,8 @@ class DeleteRegionRouteTests(unittest.IsolatedAsyncioTestCase): layout = {"blocks": [], "images": [{"id": "img1", "page": 1}]} with patch('routes.magazine._require_magazine', return_value=(None, {})), \ patch('routes.magazine.mag.load_layout', return_value=layout), \ - patch('routes.magazine.mag.save_layout'): + patch('routes.magazine.mag.save_layout'), \ + patch('routes.magazine.mag.load_articles', return_value=[]): result = await magazine_delete_region('a' * 32, 1, 'img1') self.assertTrue(result['ok']) @@ -578,6 +580,23 @@ class DeleteRegionRouteTests(unittest.IsolatedAsyncioTestCase): await magazine_delete_region('a' * 32, 1, 'b1') self.assertEqual(err.exception.status_code, 404) + async def test_a_deleted_region_leaves_every_article_that_listed_it(self): + """An article still listing a deleted id made Approve fail with "Unknown block ids".""" + from unittest.mock import patch + from routes.magazine import magazine_delete_region + layout = {"blocks": [{"id": "b1", "page": 1}, {"id": "b2", "page": 1}], "images": [{"id": "img", "page": 1}]} + articles = [{"id": "a1", "blocks": ["b1", "b2"], "images": ["img"]}, {"id": "a2", "blocks": ["b2"], "images": []}] + saved = {} + for rid, expect in (("b1", [["b2"], ["img"], ["b2"]]), ("img", [["b1", "b2"], [], ["b2"]])): + with self.subTest(rid=rid), \ + patch('routes.magazine._require_magazine', return_value=(None, {})), \ + patch('routes.magazine.mag.load_layout', return_value=layout), \ + patch('routes.magazine.mag.save_layout'), \ + patch('routes.magazine.mag.load_articles', return_value=[dict(a) for a in articles]), \ + patch('routes.magazine.mag.save_articles', side_effect=lambda _d, a: saved.__setitem__('a', a)): + await magazine_delete_region('a' * 32, 1, rid) + self.assertEqual([saved['a'][0]['blocks'], saved['a'][0]['images'], saved['a'][1]['blocks']], expect) + class ChangeRegionTypeTests(unittest.TestCase): """Changing what a detected region is, from the review list."""