From 703052d1d282a8648a857b4b9f3298fbb2bea02e Mon Sep 17 00:00:00 2001 From: mARTin-B78 Date: Tue, 22 Sep 2026 00:50:33 +0200 Subject: [PATCH] fix(magazine): adding a region crashed after a byline split; undo could roll back later changes original_text is now a Block field, so paths that rebuild blocks keep it instead of crashing. Undo snapshots are sealed with a digest of the saved state and refused once anything else changed; actions without their own undo point clear the Undo button. v1.41.1. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 6 +++++ VERSION | 2 +- architecture/magazine-ingestion.md | 5 +++- core/magazine.py | 9 ++++++- routes/magazine.py | 33 +++++++++++++++++++++-- static/index.html | 12 ++++----- static/js/magazine.js | 8 ++++++ tests/magazine.spec.js | 12 +++++++++ tests/test_magazine_routes.py | 42 ++++++++++++++++++++++++++++++ 9 files changed, 118 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index dde2a0d..9ce99cb 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.41.1] — 2026-09-22 + +### Fixed +- **Magazine: "Could not add region: … unexpected keyword argument 'original_text'".** After *Clean up boxes* split an author line, drawing a region (and re-OCR of a page) crashed on the extra field kept with that author line. The field is now part of the region model and survives every path. +- **Undo could roll back more than the change it belonged to.** An Undo offered for a delete stayed valid while other changes that record no undo point of their own (drawing a region, fixing the reading order, clean-up, approving) happened afterwards — pressing it would have reverted those too. Undo now refuses as soon as anything else has changed, and those actions clear the Undo button. + ## [1.41.0] — 2026-09-21 ### Changed diff --git a/VERSION b/VERSION index d61cd4d..11c831f 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.41.0 \ No newline at end of file +1.41.1 \ No newline at end of file diff --git a/architecture/magazine-ingestion.md b/architecture/magazine-ingestion.md index bcd0bb1..fb7cb7d 100644 --- a/architecture/magazine-ingestion.md +++ b/architecture/magazine-ingestion.md @@ -347,7 +347,10 @@ membership like the single-region routes. Every destructive route (delete, type split, bulk) writes `undo.json` = {token, label, layout, articles} *after* validating and *before* saving (the files on disk still hold the old state), and returns the token; `POST .../undo {token}` restores both files only if that token is still the latest (409 otherwise), so an old Undo can never roll back a newer -change. One level. UI: *Select boxes* mode (click or drag a rectangle; a box ≥ 50% inside is selected), +change. One level. After the change is saved the snapshot is sealed with a digest of layout.json + +articles.json (`_seal_undo`); undo also requires the current state to match that digest, so a later change that +records no undo point (drawing, fix reading order, clean-up, approve) makes the old Undo invalid instead of being +rolled back with it. UI: *Select boxes* mode (click or drag a rectangle; a box ≥ 50% inside is selected), a bulk bar, and an Undo bar replacing the per-delete confirmation dialog (per NN/g-style guidance: frequent, reversible actions get Undo, not "are you sure?"). diff --git a/core/magazine.py b/core/magazine.py index 65929ad..ac6465c 100644 --- a/core/magazine.py +++ b/core/magazine.py @@ -116,9 +116,16 @@ class Block: region_label: str = "" continues_from_previous_page: bool = False source: str = "native" # "native" | "ocr" | "vision_ocr" | "vision_label" + # The author line's text before an author line that ran into the first sentence was split + # (clean_up_regions). Part of the model so every path that rebuilds blocks keeps it -- and does + # not crash on it (drawing a region did, "unexpected keyword argument 'original_text'"). + original_text: str = "" def to_json(self) -> dict[str, Any]: - return asdict(self) + data = asdict(self) + if not data["original_text"]: + del data["original_text"] + return data @dataclass diff --git a/routes/magazine.py b/routes/magazine.py index 1fee5ab..2c2a707 100644 --- a/routes/magazine.py +++ b/routes/magazine.py @@ -19,6 +19,7 @@ from __future__ import annotations import asyncio import base64 import html +import hashlib import json import logging import re @@ -756,13 +757,36 @@ def _add_split_paragraphs_to_articles(articles: list[dict], report: dict) -> boo def _snapshot_for_undo(d: Path, label: str) -> str: """Keep the layout and articles as they are now, so the change about to happen can be undone. - One level: the next snapshot replaces this one. Returns the token the client undoes with.""" + One level: the next snapshot replaces this one. Returns the token the client undoes with. + The caller seals it with _seal_undo once the change is saved.""" token = uuid.uuid4().hex snapshot = {"token": token, "label": label, "layout": mag.load_layout(d), "articles": mag.load_articles(d)} (d / "undo.json").write_text(json.dumps(snapshot, ensure_ascii=False), "utf-8") return token +def _state_digest(d: Path) -> str: + """A fingerprint of the saved layout and articles.""" + h = hashlib.sha256() + for name in ("layout.json", "articles.json"): + path = d / name + h.update(path.read_bytes() if path.is_file() else b"-") + return h.hexdigest() + + +def _seal_undo(d: Path | None, token: str | None) -> None: + """Record what the state looks like right after the undoable change. Undo is refused once the + state differs from that -- any later change (drawing a region, fixing the reading order, + approving) would otherwise be rolled back silently with it.""" + path = d / "undo.json" if d is not None and token else None + if path is None or not path.is_file(): + return + snapshot = json.loads(path.read_text("utf-8")) + if snapshot.get("token") == token: + snapshot["after"] = _state_digest(d) + path.write_text(json.dumps(snapshot, ensure_ascii=False), "utf-8") + + @router.post("/api/magazine/{doc_id}/undo") async def magazine_undo(doc_id: str, request: Request): """Undo the last region change (delete, type change, merge, split, bulk). Body: {token}. @@ -774,7 +798,7 @@ async def magazine_undo(doc_id: str, request: Request): if not isinstance(token, str) or not path.is_file(): raise HTTPException(409, "Nothing to undo") snapshot = json.loads(path.read_text("utf-8")) - if snapshot.get("token") != token: + if snapshot.get("token") != token or snapshot.get("after") != _state_digest(d): raise HTTPException(409, "Something else has changed since -- this can no longer be undone") mag.save_layout(d, snapshot["layout"]) mag.save_articles(d, snapshot["articles"]) @@ -840,6 +864,7 @@ async def magazine_bulk_regions(doc_id: str, page: int, request: Request): changed = True if changed: mag.save_articles(d, articles) + _seal_undo(d, token) return {"ok": True, "undo": token, "remap": remap, "count": len(ids)} @@ -866,6 +891,7 @@ async def magazine_merge_region(doc_id: str, page: int, block_id: str): changed = True if changed: mag.save_articles(d, articles) + _seal_undo(d, token) return {"ok": True, "id": block_id, "removed": removed, "undo": token} @@ -890,6 +916,7 @@ async def magazine_split_region(doc_id: str, page: int, block_id: str, request: articles = mag.load_articles(d) if _add_split_paragraphs_to_articles(articles, {"byline_split": [{"id": block_id, "new_id": new_id}]}): mag.save_articles(d, articles) + _seal_undo(d, token) return {"ok": True, "id": block_id, "new_id": new_id, "undo": token} @@ -1095,6 +1122,7 @@ async def magazine_update_region(doc_id: str, page: int, block_id: str, request: mag.save_layout(d, layout) if region_id != block_id: _move_article_membership(d, block_id, region_id, to_image=new_type == "image") + _seal_undo(d, token) return {"ok": True, "text": text, "id": region_id, "type": new_type, "undo": token} @@ -1142,6 +1170,7 @@ async def magazine_delete_region(doc_id: str, page: int, block_id: str): changed = True if changed: mag.save_articles(d, articles) + _seal_undo(d, token) return {"ok": True, "undo": token} diff --git a/static/index.html b/static/index.html index 47dab1d..338dfa6 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 ca06dd3..49febdc 100644 --- a/static/js/magazine.js +++ b/static/js/magazine.js @@ -1217,6 +1217,7 @@ async function magOrderPut(order) { } async function magOrderSave() { + magOfferUndo(null); // this change records no undo point, so an older Undo no longer applies const seq = (state.orderSeq || []).slice(); if (!seq.length) return; try { @@ -1231,6 +1232,7 @@ async function magOrderSave() { } async function magOrderMove(id, value) { + magOfferUndo(null); // this change records no undo point, so an older Undo no longer applies const order = (state.reviewRegionOrder || []).filter((x) => x !== id); const to = Math.min(Math.max(1, parseInt(value, 10) || 1), order.length + 1) - 1; order.splice(to, 0, id); @@ -1245,6 +1247,7 @@ async function magOrderMove(id, value) { } async function magOrderReset() { + magOfferUndo(null); // this change records no undo point, so an older Undo no longer applies try { await api('/api/magazine/' + state.doc.id + '/pages/' + state.reviewPage + '/reading-order', { method: 'DELETE' }); } catch (e) { @@ -1596,6 +1599,7 @@ async function magReviewSubmitEdit(reocr) { } async function magCleanUpBoxes() { + magOfferUndo(null); // this change records no undo point, so an older Undo no longer applies const ok = await confirmDialog('Clean up the boxes of this whole issue: text inside pictures, OCR noise (boxes of one or two characters) ' + 'and boxes that repeat a bigger box\'s text stop being read aloud, and author lines that run into the first sentence are split. ' + 'Nothing is deleted — every change shows in the review list and can be undone with the type picker — and a backup of the layout is kept.', @@ -1620,6 +1624,7 @@ async function magCleanUpBoxes() { } async function magFixReadingOrder() { + magOfferUndo(null); // this change records no undo point, so an older Undo no longer applies const ok = await confirmDialog('Re-read the reading order of every page in this issue, column by column. ' + 'Text, boxes and approvals stay as they are, but the order in which approved articles are read aloud can change. ' + 'Pages you ordered by hand are kept.', { title: 'Fix reading order?', okLabel: 'Fix reading order' }); @@ -1911,6 +1916,7 @@ function magReviewOpenDrawPanel(box) { } async function magReviewSubmitDraw() { + magOfferUndo(null); // this change records no undo point, so an older Undo no longer applies const box = $('mag-draw-rect'); const pageMeta = (state.layout.pages || []).find((p) => p.page === state.reviewPage); if (!box || !pageMeta) return; @@ -2020,6 +2026,7 @@ function selectedBlockIds() { } async function magReocrPage() { + magOfferUndo(null); // this change records no undo point, so an older Undo no longer applies const page = state.reviewPage; if (page == null) return; const btn = $('mag-reocr-btn'); @@ -2102,6 +2109,7 @@ async function magRefineFigures() { } async function magApprove() { + magOfferUndo(null); // this change records no undo point, so an older Undo no longer applies const a = state.article; if (!a) return; const blocks = selectedBlockIds(); diff --git a/tests/magazine.spec.js b/tests/magazine.spec.js index 3904ca2..9916fcf 100644 --- a/tests/magazine.spec.js +++ b/tests/magazine.spec.js @@ -629,3 +629,15 @@ test('clicking boxes in select mode builds a selection whose type changes at onc await page.locator('#mag-bulk-type').selectOption('ocr_noise'); await expect.poll(() => sent).toEqual({ ids: ['b1', 'b2'], action: 'type', type: 'ocr_noise' }); }); + +test('an action without its own undo point clears the Undo button', async ({ page }) => { + await mockBackend(page); + await page.route(`**/api/magazine/${DOC_ID}/pages/3/regions/b3`, route => route.fulfill({ json: { ok: true, undo: 'tok-9' } })); + await page.route(`**/api/magazine/${DOC_ID}/reading-order`, route => route.fulfill({ json: { ok: true, pages_changed: [], pages_locked: 0 } })); + await openReview(page); + await page.locator('[data-block-row="b3"]').getByRole('button', { name: /Delete region/ }).click(); + await expect(page.locator('#mag-undo-bar')).toContainText('Region deleted.'); + await page.getByRole('button', { name: 'Fix reading order', exact: true }).click(); + await page.getByRole('alertdialog').getByRole('button', { name: 'Fix reading order', exact: true }).click(); + await expect(page.locator('#mag-undo-bar')).toBeEmpty(); +}); diff --git a/tests/test_magazine_routes.py b/tests/test_magazine_routes.py index 7c983a9..e50cea6 100644 --- a/tests/test_magazine_routes.py +++ b/tests/test_magazine_routes.py @@ -1123,3 +1123,45 @@ class BulkUndoFoldTests(unittest.TestCase): self.assertEqual(err.exception.status_code, 400) self.assertEqual(len(mag.load_layout(self.dir)["blocks"]), 4) self.assertFalse((self.dir / "undo.json").exists()) + + +class UndoSafetyAndExtraFieldTests(BulkUndoFoldTests): + """Undo never rolls back a later change; a split author line's extra field survives every path.""" + + def test_undo_is_refused_once_anything_else_changed(self): + from core import magazine as mag + from routes.magazine import magazine_bulk_regions, magazine_undo + out = self.run_route(magazine_bulk_regions, 1, body={"ids": ["lg1"], "action": "delete"}) + # A change that records no undo point of its own, e.g. drawing a region or fixing the order. + layout = mag.load_layout(self.dir) + layout["blocks"][0]["text"] = "edited later" + mag.save_layout(self.dir, layout) + with self.assertRaises(HTTPException) as err: + self.run_route(magazine_undo, body={"token": out["undo"]}) + self.assertEqual(err.exception.status_code, 409) + self.assertEqual(mag.load_layout(self.dir)["blocks"][0]["text"], "edited later") + + def test_an_unsealed_old_snapshot_is_refused(self): + import json + from routes.magazine import magazine_undo + from core import magazine as mag + (self.dir / "undo.json").write_text(json.dumps({"token": "t", "label": "x", "layout": mag.load_layout(self.dir), + "articles": mag.load_articles(self.dir)}), "utf-8") + with self.assertRaises(HTTPException): + self.run_route(magazine_undo, body={"token": "t"}) + + def test_drawing_a_region_on_a_layout_with_a_split_author_line_works(self): + """"Could not add region: Block.__init__() got an unexpected keyword argument 'original_text'".""" + from core.magazine import add_manual_region, clean_up_regions + self.layout["blocks"].append({"id": "by", "page": 1, "bbox": [50, 20, 300, 30], "type": "byline", + "text": "Von Jo Bager KI gilt als Antwort", "reading_order": 9}) + cleaned, report = clean_up_regions(self.layout) + self.assertEqual(len(report["byline_split"]), 1) + after = add_manual_region(self.dir, cleaned, 1, [400, 700, 100, 40], "text", "New text") + byline = next(b for b in after["blocks"] if b["id"] == "by") + self.assertIn("KI gilt als Antwort", byline["original_text"]) + self.assertTrue(any(b["text"] == "New text" for b in after["blocks"])) + + def test_blocks_without_the_field_do_not_gain_it(self): + from core.magazine import Block + self.assertNotIn("original_text", Block(id="x", page=1, bbox=[0, 0, 1, 1], type="body", text="t").to_json())