Keep a move or a delete of chapters that the user makes during a job

A job placed each new chapter with the chapter order from its start, so
a move by hand during the job went back. The store now places a new
chapter against the order that the story has at that moment, in one
transaction. A recheck of a chapter that the user deleted no longer
stops the job.

bindery/orchestrator.py

@@ -530,7 +530,8 @@ def set_chapters_included(story_id: int, chapter_ids: list[int], included: bool)
 
 def move_chapters(story_id: int, chapter_ids: list[int], after_id: int | None) -> dict[str, Any]:
     """Move chapters, in their current order, to directly after the chapter `after_id`, or to the start with None."""
-    order = [c["id"] for c in store.list_chapters(story_id)]
+
+    def arrange(order: list[int]) -> list[int]:
         moving = [cid for cid in order if cid in set(chapter_ids)]
         if not moving:
             raise ValueError("Select the chapters to move")
@@ -540,7 +541,9 @@ def move_chapters(story_id: int, chapter_ids: list[int], after_id: int | None) -
             raise ValueError("The chapters cannot go after a chapter that moves with them")
         rest = [cid for cid in order if cid not in moving]
         at = rest.index(after_id) + 1 if after_id is not None else 0
-    store.set_positions(story_id, rest[:at] + moving + rest[at:])
+        return rest[:at] + moving + rest[at:]
+
+    store.reorder_chapters(story_id, arrange)
     return queue_story(story_id, DownloadMode.REBUILD.value)
 
 
@@ -940,8 +943,9 @@ def _import_epub(story_id: int, contents: fff.EpubContents) -> int:
     for img in contents.images:
         store.put_image(story_id, img["url"], img["ext"], img["mime"], img["data"])
     if new:
-        order = merge_chapter_lists(list(stored), epub_order, newest_last=False)
-        store.add_chapters(story_id, new, "import", [stored.get(u, u) for u in order])
+        # The store keeps a chapter under the URL that it had at its download.
+        in_epub = [stored.get(u, u) for u in epub_order]
+        store.add_chapters(story_id, new, "import", lambda have: merge_chapter_lists(have, in_epub, newest_last=False))
     return len(new)
 
 
@@ -1180,7 +1184,6 @@ def _run_site_job(job: Job) -> str:
     total = len(new_urls) + len(refetch)
     done = 0
     failed: list[str] = []
-    order = merge_chapter_lists([c["url"] for c in chapters], list(site_titles))
     stored_images = store.image_urls(story_id)
 
     def store_images() -> None:
@@ -1205,8 +1208,8 @@ def _run_site_job(job: Job) -> str:
             continue
         store_images()
         chapter = {"url": url, "title": site_titles[url], "html": html, "text_hash": text_hash(html)}
-        by_url[url] = chapter
-        store.add_chapters(story_id, [chapter], mode.value, [u for u in order if u in by_url])
+        # The chapter goes in the order that the story has now. A move or a delete by hand during this job stays.
+        store.add_chapters(story_id, [chapter], mode.value, lambda have: merge_chapter_lists(have, list(site_titles)))
         new_count += 1
         save_progress()
 
@@ -1220,6 +1223,10 @@ def _run_site_job(job: Job) -> str:
             done += 1
             continue
         store_images()
+        revisions = store.list_revisions(c["id"])
+        if not revisions:  # the user deleted the chapter during this job
+            done += 1
+            continue
         if c["id"] in renamed:
             c["title"] = renamed[c["id"]]
             store.update_chapter(c["id"], title=c["title"])
@@ -1227,7 +1234,7 @@ def _run_site_job(job: Job) -> str:
         new_hash = text_hash(html)
         # The site changed when its text differs from the text that it sent last. Until then, the book keeps
         # a revision that the user chose.
-        newest = store.list_revisions(c["id"])[-1]
+        newest = revisions[-1]
         if new_hash != newest["text_hash"]:
             if _rules_explain_change(story_id, newest, html):
                 store.replace_revision(c["id"], newest["rev"], html, new_hash)

bindery/store.py

@@ -8,7 +8,7 @@ site without losing text.
 import json
 import re
 import sqlite3
-from collections.abc import Iterator
+from collections.abc import Callable, Iterator
 from contextlib import contextmanager
 from datetime import datetime
 from pathlib import Path
@@ -282,13 +282,27 @@ class Store:
             )
             return chapter_id
 
-    def add_chapters(self, story_id: int, chapters: list[dict[str, str]], source: str, order: list[str]) -> list[int]:
+    def add_chapters(
+        self,
+        story_id: int,
+        chapters: list[dict[str, str]],
+        source: str,
+        order: list[str] | Callable[[list[str]], list[str]],
+    ) -> list[int]:
         """Add chapters and put the story in the order of `order`, a list of chapter URLs, in one transaction.
 
         Each chapter needs url, title, html and text_hash. Chapters that `order`
         leaves out go after the others and keep their order.
+
+        `order` can be a function. It gets the URLs of the stored chapters in book
+        order, and returns the list. It runs in the transaction, so it sees a move or
+        a delete that the user made a moment before.
         """
         with self._connect() as conn:
+            conn.execute("BEGIN IMMEDIATE")
+            if callable(order):
+                stored = conn.execute("SELECT url FROM chapters WHERE story_id = ? ORDER BY position, id", (story_id,))
+                order = order([r["url"] for r in stored])
             ids = []
             for c in chapters:
                 cur = conn.execute(
@@ -338,11 +352,20 @@ class Store:
                 (*fields.values(), chapter_id),
             )
 
-    def set_positions(self, story_id: int, chapter_ids: list[int]) -> None:
+    def reorder_chapters(self, story_id: int, arrange: Callable[[list[int]], list[int]]) -> None:
+        """Give the chapters of a story a new order.
+
+        `arrange` gets the chapter ids in book order, and returns them in the new
+        order. The read and the write are one transaction, so a job that adds a
+        chapter at the same time cannot come between them.
+        """
         with self._connect() as conn:
+            conn.execute("BEGIN IMMEDIATE")
+            rows = conn.execute("SELECT id FROM chapters WHERE story_id = ? ORDER BY position, id", (story_id,))
+            order = arrange([r["id"] for r in rows])
             conn.executemany(
                 "UPDATE chapters SET position = ? WHERE id = ? AND story_id = ?",
-                [(i, cid, story_id) for i, cid in enumerate(chapter_ids)],
+                [(i, cid, story_id) for i, cid in enumerate(order)],
             )
 
     def delete_chapters(self, story_id: int, chapter_ids: list[int]) -> None:

tests/test_job_races.py

@@ -0,0 +1,92 @@
+"""Changes that the user makes to the chapters of a story while a job runs for it."""
+
+import pytest
+from test_deliver import CHAPTER, linked_story, site, titles, update
+from test_deliver import fake as fake
+from test_deliver import jobs as jobs
+from test_deliver import story_epub as story_epub
+
+from bindery import fff, orchestrator
+from bindery.jobs import DownloadMode
+from bindery.store import Store
+
+THREE = [(1, "One", "a"), (2, "Two", "b"), (3, "Three", "c")]
+
+
+def during_download(monkeypatch, number, action):
+    """Run `action` when the job downloads the chapter with this number."""
+    fetch = fff.fetch_chapter
+
+    def fetch_and_act(adapter, url, index):
+        if url == CHAPTER.format(number):
+            action()
+        return fetch(adapter, url, index)
+
+    monkeypatch.setattr(fff, "fetch_chapter", fetch_and_act)
+
+
+def chapter_id(story_id, title):
+    return next(c["id"] for c in orchestrator.store.list_chapters(story_id) if c["title"] == title)
+
+
+def test_a_move_during_a_download_stays(fake, jobs, monkeypatch):
+    story_id = linked_story(THREE)
+    site(monkeypatch, [*THREE, (4, "Four", "d"), (5, "Five", "e")])
+    during_download(monkeypatch, 4, lambda: orchestrator.move_chapters(story_id, [chapter_id(story_id, "Three")], None))
+    assert "2 new chapters" in update(story_id)
+    assert titles(story_id) == ["Three", "One", "Two", "Four", "Five"]
+
+
+def test_a_new_chapter_in_the_middle_goes_after_its_moved_predecessor(fake, jobs, monkeypatch):
+    story_id = linked_story(THREE)
+    site(monkeypatch, [(1, "One", "a"), (9, "Nine", "x"), (2, "Two", "b"), (3, "Three", "c")])
+    during_download(
+        monkeypatch,
+        9,
+        lambda: orchestrator.move_chapters(story_id, [chapter_id(story_id, "One")], chapter_id(story_id, "Three")),
+    )
+    update(story_id)
+    assert titles(story_id) == ["Two", "Three", "One", "Nine"]
+
+
+def test_a_delete_during_a_download_stays(fake, jobs, monkeypatch):
+    story_id = linked_story(THREE)
+    site(monkeypatch, [*THREE, (4, "Four", "d")])
+    during_download(monkeypatch, 4, lambda: orchestrator.delete_chapters(story_id, [chapter_id(story_id, "Two")]))
+    assert "1 new chapter" in update(story_id)
+    assert titles(story_id) == ["One", "Three", "Four"]
+
+
+def test_a_delete_during_a_recheck_does_not_stop_the_job(fake, jobs, monkeypatch):
+    story_id = linked_story(THREE)
+    site(monkeypatch, [(1, "One", "a2"), (2, "Two", "b2"), (3, "Three", "c2")])
+    during_download(monkeypatch, 2, lambda: orchestrator.delete_chapters(story_id, [chapter_id(story_id, "Two")]))
+    assert "2 of 3 rechecked chapters changed" in update(story_id, DownloadMode.RECHECK_ALL)
+    assert titles(story_id) == ["One", "Three"]
+
+
+def test_the_order_function_of_add_chapters_sees_the_stored_order(tmp_path):
+    store = Store(tmp_path / "db.sqlite")
+    sid = store.create_story("u", "s", "T", "A", [])
+    for n, url in enumerate(["u2", "u1"]):
+        store.add_chapter(sid, url, url, "<p/>", "h", "new", n)
+    new = [{"url": "u3", "title": "u3", "html": "<p/>", "text_hash": "h"}]
+    seen = []
+    store.add_chapters(sid, new, "new", lambda stored: seen.append(stored) or [*stored, "u3"])
+    assert seen == [["u2", "u1"]]
+    assert [c["url"] for c in store.list_chapters(sid)] == ["u2", "u1", "u3"]
+
+
+def test_a_refused_reorder_changes_nothing(tmp_path):
+    store = Store(tmp_path / "db.sqlite")
+    sid = store.create_story("u", "s", "T", "A", [])
+    ids = [store.add_chapter(sid, f"u{n}", str(n), "<p/>", "h", "new", n) for n in range(3)]
+
+    def refuse(order):
+        raise ValueError("no")
+
+    with pytest.raises(ValueError, match="no"):
+        store.reorder_chapters(sid, refuse)
+    assert [c["id"] for c in store.list_chapters(sid)] == ids
+    store.reorder_chapters(sid, lambda order: order[::-1])
+    assert [c["id"] for c in store.list_chapters(sid)] == ids[::-1]

tests/test_store.py

@@ -6,7 +6,7 @@ def test_chapters_revisions_and_rules(tmp_path):
     sid = store.create_story("https://example/s/1", "example", "T", "A", ["title"])
     c1 = store.add_chapter(sid, "u1", "One", "<p>1</p>", "h1", "new", 0)
     c2 = store.add_chapter(sid, "u2", "Two", "<p>2</p>", "h2", "new", 1)
-    store.set_positions(sid, [c2, c1])
+    store.reorder_chapters(sid, lambda order: [c2, c1])
     assert [c["url"] for c in store.chapters_for_build(sid)] == ["u2", "u1"]
 
     assert store.add_revision(c1, "<p>1b</p>", "h1b", "recheck") == 2