Share the checks and the job builders of the orchestrator

One function checks a whole number of a request, and one checks the
metadata field names. save_settings takes the settings by name. A rebuild
job and a story from an epub each have one builder.

bindery/orchestrator.py

@@ -66,6 +66,25 @@ def default_locked_fields() -> list[str]:
     return [f for f in config.DEFAULT_LOCKED_FIELDS if f in METADATA_FIELDS]
 
 
+def whole_number(value: Any, name: str, most: int | None = None, negative: bool = False) -> int:
+    """A value of a request as a whole number. A wrong value is an error of the request."""
+    try:
+        number = int(value)
+    except (TypeError, ValueError):
+        raise ValueError(f"{name} must be a whole number") from None
+    if number < 0 and not negative:
+        raise ValueError(f"{name} cannot be negative")
+    if most is not None and number > most:
+        raise ValueError(f"{name} cannot be more than {most}")
+    return number
+
+
+def _check_fields(fields: Any) -> None:
+    bad = [f for f in fields if f not in METADATA_FIELDS]
+    if bad:
+        raise ValueError(f"Unknown metadata fields: {bad}")
+
+
 # Queueing
 
 
@@ -104,9 +123,7 @@ def queue_story(
         raise ValueError("Use the library import for this mode")
     if mode_enum == DownloadMode.RECHECK and not chapter_ids:
         raise ValueError("Select at least one chapter to recheck")
-    bad = [f for f in force_fields or [] if f not in METADATA_FIELDS]
+    _check_fields(force_fields or [])
-    if bad:
-        raise ValueError(f"Unknown metadata fields: {bad}")
     job = Job(
         mode=mode_enum,
         story_id=story_id,
@@ -118,6 +135,10 @@ def queue_story(
     return job_queue.add(job).to_dict()
 
 
+def _queue_rebuild(story: dict[str, Any]) -> dict[str, Any]:
+    return job_queue.add(Job(mode=DownloadMode.REBUILD, story_id=story["id"], title=story["title"])).to_dict()
+
+
 def queue_import(books: list[dict[str, Any]], check: bool = True) -> list[dict[str, Any]]:
     """Queue imports of library books, given as {"id": ..., "uuid": ..., "title": ...}."""
     jobs = []
@@ -136,10 +157,15 @@ def queue_import(books: list[dict[str, Any]], check: bool = True) -> list[dict[s
     return jobs
 
 
+def _epub_story_url(path: Path) -> tuple[str, str] | None:
+    """The story URL and the site of an epub from FanFicFare, or None."""
+    return next((n for n in map(fff.normalize_url, epub_urls(path)) if n), None)
+
+
 def queue_epub_file(path: Path, filename: str, story_id: int | None = None, check: bool = True) -> dict[str, Any]:
     """Queue an import of an uploaded epub. A file without a story URL, or of another story, fails at once."""
     try:
-        normal = next((n for n in map(fff.normalize_url, epub_urls(path)) if n), None)
+        normal = _epub_story_url(path)
         if not normal:
             raise StoryError(f"{filename} has no supported story URL. Only epubs from FanFicFare can be imported.")
         story = store.get_story_by_url(normal[0])
@@ -266,15 +292,11 @@ def update_story_settings(
     """Change the settings that are given. A recheck_days of 0 turns the full rechecks off."""
     story = _require_story(story_id)
     if recheck_days is not None:
-        days = int(recheck_days)
+        days = whole_number(recheck_days, "The number of days")
-        if days < 0:
-            raise ValueError("The number of days cannot be negative")
         store.set_setting(story_id, "recheck_days", days or None)
     changes: dict[str, Any] = {}
     if locked_fields is not None:
-        bad = [f for f in locked_fields if f not in METADATA_FIELDS]
+        _check_fields(locked_fields)
-        if bad:
-            raise ValueError(f"Unknown metadata fields: {bad}")
         changes["locked_fields"] = sorted(set(locked_fields))
         # An unlocked field takes the site value again.
         unlocked = set(story["locked_fields"]) - set(locked_fields)
@@ -317,20 +339,18 @@ def edit_metadata(story_id: int, values: dict[str, Any]) -> dict[str, Any]:
     build uses the values.
     """
     story = _require_story(story_id)
-    bad = [f for f in values if f not in METADATA_FIELDS]
+    _check_fields(values)
-    if bad:
-        raise ValueError(f"Unknown metadata fields: {bad}")
     pinned = dict(story["pinned"])
     for field, value in values.items():
         if value is None:
             pinned.pop(field, None)
         else:
             pinned[field] = _edited_value(field, value)
-    title = _display_title(story, pinned, story["site_values"].get("title") or story["title"])
+    title = _display_title(pinned, story["site_values"].get("title") or story["title"])
     store.update_story(story_id, pinned=pinned, title=title)
     queued = None
     if story["metahtml"] and (story["calibre_id"] or story["ingested_at"]):
-        queued = job_queue.add(Job(mode=DownloadMode.REBUILD, story_id=story_id, title=title)).to_dict()
+        queued = _queue_rebuild({**story, "title": title})
     return {"story": story_view(story_id), "job": queued}
 
 
@@ -364,58 +384,38 @@ def get_settings() -> dict[str, Any]:
     }
 
 
-def _validate_job_list_value(value: Any, name: str, maximum: int) -> int:
+# The settings that save_settings takes.
-    try:
+SETTINGS = ("recent_chapters", "personal_ini", "link_books", "hold_new_links", "pause_uploads", *JOB_LIST_DEFAULTS)
-        n = int(value)
-    except (TypeError, ValueError) as e:
-        raise ValueError(f"{name} must be a whole number") from e
-    if n < 0:
-        raise ValueError(f"{name} cannot be negative")
-    if n > maximum:
-        raise ValueError(f"{name} cannot be more than {maximum}")
-    return n
 
 
-def save_settings(
+def save_settings(**changes: Any) -> dict[str, Any]:
-    recent_chapters: Any = None,
+    """Change the settings that are given, by the names in SETTINGS. A value of None changes nothing.
-    personal_ini: str | None = None,
-    hold_new_links: bool | None = None,
-    pause_uploads: bool | None = None,
-    story_jobs_hours: Any = None,
-    story_jobs_count: Any = None,
-    main_jobs_hours: Any = None,
-    main_jobs_count: Any = None,
-    link_books: bool | None = None,
-) -> dict[str, Any]:
-    """Change the settings that are given. Return all settings, and FanFicFare's warnings about personal.ini.
 
-    Every value is checked before the first one is stored, so a bad value changes nothing.
+    Returns all settings, and FanFicFare's warnings about personal.ini. Every value is checked before the first
+    one is stored, so a bad value changes nothing.
     """
+    unknown = set(changes) - set(SETTINGS)
+    if unknown:
+        raise ValueError(f"Unknown settings: {sorted(unknown)}")
+    changes = {name: value for name, value in changes.items() if value is not None}
+    link_books, personal_ini = changes.get("link_books"), changes.get("personal_ini")
     if link_books is not None and not isinstance(link_books, bool):
         raise ValueError("Link books to CWA library must be true or false")
     numbers: dict[str, int] = {}
-    if recent_chapters is not None:
+    if "recent_chapters" in changes:
-        try:
+        numbers["recent_chapters"] = whole_number(changes["recent_chapters"], "The number of recent chapters")
-            numbers["recent_chapters"] = int(recent_chapters)
+    for name in JOB_LIST_DEFAULTS:
-        except (TypeError, ValueError) as e:
+        if name in changes:
-            raise ValueError("The number of recent chapters must be a whole number") from e
+            most = config.JOB_LIST_MAX_HOURS if name.endswith("hours") else config.JOB_LIST_MAX_COUNT
-        if numbers["recent_chapters"] < 0:
+            numbers[name] = whole_number(changes[name], name, most)
-            raise ValueError("The number of recent chapters cannot be negative")
-    for name, value, maximum in (
-        ("story_jobs_hours", story_jobs_hours, config.JOB_LIST_MAX_HOURS),
-        ("story_jobs_count", story_jobs_count, config.JOB_LIST_MAX_COUNT),
-        ("main_jobs_hours", main_jobs_hours, config.JOB_LIST_MAX_HOURS),
-        ("main_jobs_count", main_jobs_count, config.JOB_LIST_MAX_COUNT),
-    ):
-        if value is not None:
-            numbers[name] = _validate_job_list_value(value, name, maximum)
     warnings = fff.check_personal_ini(personal_ini) if personal_ini is not None else []
 
     released = False
     if link_books is not None and _linking_available():
         released = _link_books() != link_books
         store.set_setting(None, "link_books", link_books)
-    for name, value in (("hold_new_links", hold_new_links), ("pause_uploads", pause_uploads)):
+    for name in ("hold_new_links", "pause_uploads"):
+        value = changes.get(name)
         if value is None or (name == "pause_uploads" and config.INGEST_DIR is None):
             continue
         released = released or (bool(store.get_setting(None, name)) and not value)
@@ -508,7 +508,7 @@ def set_chapter_revision(chapter_id: int, rev: int) -> dict[str, Any]:
     if not chapter or store.revision_html(chapter_id, rev) is None:
         raise KeyError("No such chapter revision")
     store.update_chapter(chapter_id, current_rev=rev, changed_at=now())
-    return queue_story(chapter["story_id"], DownloadMode.REBUILD.value)
+    return _queue_rebuild(_require_story(chapter["story_id"]))
 
 
 def delete_revision(chapter_id: int, rev: int) -> list[dict[str, Any]]:
@@ -532,7 +532,7 @@ def set_chapters_included(story_id: int, chapter_ids: list[int], included: bool)
         chapter = store.get_chapter(cid)
         if chapter and chapter["story_id"] == story_id:
             store.update_chapter(cid, included=int(included))
-    return queue_story(story_id, DownloadMode.REBUILD.value)
+    return _queue_rebuild(_require_story(story_id))
 
 
 def move_chapters(story_id: int, chapter_ids: list[int], after_id: int | None) -> dict[str, Any]:
@@ -551,7 +551,7 @@ def move_chapters(story_id: int, chapter_ids: list[int], after_id: int | None) -
         return rest[:at] + moving + rest[at:]
 
     store.reorder_chapters(story_id, arrange)
-    return queue_story(story_id, DownloadMode.REBUILD.value)
+    return _queue_rebuild(_require_story(story_id))
 
 
 def delete_chapters(story_id: int, chapter_ids: list[int]) -> dict[str, Any]:
@@ -563,7 +563,7 @@ def delete_chapters(story_id: int, chapter_ids: list[int]) -> dict[str, Any]:
     if len(deleting) == len(order):
         raise ValueError("A story must keep at least one chapter. To remove the story, stop tracking it")
     store.delete_chapters(story_id, deleting)
-    return queue_story(story_id, DownloadMode.REBUILD.value)
+    return _queue_rebuild(_require_story(story_id))
 
 
 def link_story(story_id: int, book_uuid: str | None) -> None:
@@ -596,7 +596,7 @@ def approve_upload(story_id: int, book_uuid: str | None) -> dict[str, Any] | Non
         raise ValueError("The library link of this story changed. Reload the page, and check the link again.")
     store.set_setting(story_id, "unapproved_link", None)
     if _can_upload() and store.get_setting(story_id, "upload_held") and story["metahtml"]:
-        return job_queue.add(Job(mode=DownloadMode.REBUILD, story_id=story_id, title=story["title"])).to_dict()
+        return _queue_rebuild(story)
     return None
 
 
@@ -627,7 +627,7 @@ def build_for_download(story_id: int) -> tuple[BinaryIO, str]:
     """The epub of a story for a browser download, as an open file. Bindery builds it only if the book changed
     after the last build."""
     story = _require_story(story_id)
-    overrides = _epub_overrides(story, story["pinned"])
+    overrides = _epub_overrides(story["pinned"])
     key = _build_key(story, overrides, story["site_values"])
     # The file is opened under the lock, so a new build cannot delete it first.
     with _build_lock:
@@ -664,11 +664,11 @@ def _resolve_metadata(story: dict[str, Any], site: dict[str, Any], force_fields:
     return pinned
 
 
-def _epub_overrides(story: dict[str, Any], pinned: dict[str, Any]) -> dict[str, Any]:
+def _epub_overrides(pinned: dict[str, Any]) -> dict[str, Any]:
     return {f: v for f, v in pinned.items() if f in METADATA_FIELDS}
 
 
-def _display_title(story: dict[str, Any], pinned: dict[str, Any], site_title: str) -> str:
+def _display_title(pinned: dict[str, Any], site_title: str) -> str:
     return pinned.get("title") or site_title
 
 
@@ -864,7 +864,7 @@ def _release_held_uploads() -> None:
     for story_id in store.story_settings("upload_held"):
         story = store.get_story(story_id)
         if story and story["metahtml"]:
-            job_queue.add(Job(mode=DownloadMode.REBUILD, story_id=story_id, title=story["title"]))
+            _queue_rebuild(story)
 
 
 def _hold_reason(story: dict[str, Any], book: dict[str, Any] | None) -> str | None:
@@ -943,7 +943,7 @@ def adopt_build_keys() -> None:
         story = _require_story(story_id)
         if not story["metahtml"]:
             continue
-        key = _build_key(story, _epub_overrides(story, story["pinned"]), story["site_values"])
+        key = _build_key(story, _epub_overrides(story["pinned"]), story["site_values"])
         store.set_setting(story_id, "built_key", key)
         if (story["calibre_id"] or story["ingested_at"]) and not held.get(story_id):
             store.set_setting(story_id, "sent_key", key)
@@ -997,6 +997,16 @@ def _import_epub(story_id: int, contents: fff.EpubContents) -> int:
     return len(new)
 
 
+def _story_from_epub(
+    url: str, site: str, title: str, meta: dict[str, Any], book_id: int | None = None, book_uuid: str | None = None
+) -> int:
+    """Create a story with the metadata of its epub. The locked fields keep the values of the epub."""
+    locked = default_locked_fields()
+    story_id = store.create_story(url, site, title, " & ".join(meta["authors"]), locked, book_id, book_uuid)
+    store.update_story(story_id, pinned={f: _field_value(f, meta) for f in locked}, status="ok")
+    return story_id
+
+
 def _run_import(job: Job) -> str:
     tracked = store.get_story_by_calibre_id(job.calibre_id)
     # A tracked story without chapters is left from an import that stopped, so the import goes on.
@@ -1009,7 +1019,7 @@ def _run_import(job: Job) -> str:
     _progress(job, "Downloading epub from CWA")
     epub = cwa().download_epub(source, config.TMP_DIR / f"import-{job.calibre_id}-{uuid.uuid4().hex[:8]}.epub")
     try:
-        normal = next((n for n in map(fff.normalize_url, epub_urls(epub)) if n), None)
+        normal = _epub_story_url(epub)
         if not normal:
             raise StoryError(f"{job.title} has no supported story URL")
         meta = epub_metadata(epub)
@@ -1021,12 +1031,8 @@ def _run_import(job: Job) -> str:
             return f"Linked to tracked story {story['title']}"
         contents = _read_epub(epub, title)
         if story is None:
-            locked = default_locked_fields()
+            story_id = _story_from_epub(normal[0], normal[1], title, meta, job.calibre_id, book_uuid or None)
-            story_id = store.create_story(
+            store.update_story(story_id, ingested_at=now())
-                normal[0], normal[1], title, " & ".join(meta["authors"]), locked, job.calibre_id, book_uuid or None
-            )
-            pinned = {f: _field_value(f, meta) for f in locked}
-            store.update_story(story_id, pinned=pinned, ingested_at=now(), status="ok")
             story = _require_story(story_id)
         story_id = story["id"]
         job_queue.update(job, story_id=story_id)
@@ -1047,11 +1053,7 @@ def _run_import_file(job: Job) -> str:
         story = store.get_story(job.story_id) if job.story_id else store.get_story_by_url(job.url)
         if story is None:
             meta = epub_metadata(path)
-            locked = default_locked_fields()
+            story_id = _story_from_epub(job.url, fff.normalize_url(job.url)[1], meta["title"] or job.url, meta)
-            story_id = store.create_story(
-                job.url, fff.normalize_url(job.url)[1], meta["title"] or job.url, " & ".join(meta["authors"]), locked
-            )
-            store.update_story(story_id, pinned={f: _field_value(f, meta) for f in locked}, status="ok")
             _link_changed(story_id)
             story = _require_story(story_id)
         job_queue.update(job, story_id=story["id"], title=story["title"])
@@ -1069,7 +1071,7 @@ def _run_import_file(job: Job) -> str:
             Job(mode=DownloadMode.NEW, story_id=story["id"], title=story["title"], options={"build": bool(added)})
         )
     elif added and story["metahtml"]:
-        job_queue.add(Job(mode=DownloadMode.REBUILD, story_id=story["id"], title=story["title"]))
+        _queue_rebuild(story)
     return message
 
 
@@ -1077,7 +1079,7 @@ def _run_rebuild(job: Job) -> str:
     story = _require_story(job.story_id)
     job_queue.update(job, title=story["title"])
     pinned = _resolve_metadata(story, story["site_values"], [])
-    overrides = _epub_overrides(story, pinned)
+    overrides = _epub_overrides(pinned)
     key = _build_key(story, overrides, story["site_values"])
     force = bool(job.options.get("force"))
     if not force and key == _done_key(story["id"]):
@@ -1319,8 +1321,8 @@ def _run_site_job(job: Job) -> str:
     # The book is built and sent only when its chapters, rules or metadata changed. A job that resumes, or that
     # follows a failed job, finds the changes of the earlier run through the key.
     pinned = _resolve_metadata(story, snap.calibre, job.force_fields)
-    title = _display_title(story, pinned, snap.title)
+    title = _display_title(pinned, snap.title)
-    overrides = _epub_overrides(story, pinned)
+    overrides = _epub_overrides(pinned)
     key = _build_key(story, overrides, snap.calibre)
     checked: dict[str, Any] = {"pinned": pinned, "site_values": snap.calibre, "title": title, "last_checked": now()}
     if key == _done_key(story_id):

bindery/web.py

@@ -105,10 +105,7 @@ def _list(data: dict[str, Any], name: str) -> list[Any]:
 
 
 def _int(value: Any, name: str) -> int:
-    try:
+    return orchestrator.whole_number(value, name, negative=True)
-        return int(value)
-    except (TypeError, ValueError):
-        raise ValueError(f"{name} must be a whole number") from None
 
 
 def _ids(data: dict[str, Any], name: str = "chapter_ids") -> list[int]:
@@ -280,17 +277,7 @@ def settings() -> Any:
     if request.method == "PUT":
         _admin_only()
         data = _json()
-        return orchestrator.save_settings(
+        return orchestrator.save_settings(**{name: data.get(name) for name in orchestrator.SETTINGS})
-            data.get("recent_chapters"),
-            data.get("personal_ini"),
-            data.get("hold_new_links"),
-            data.get("pause_uploads"),
-            data.get("story_jobs_hours"),
-            data.get("story_jobs_count"),
-            data.get("main_jobs_hours"),
-            data.get("main_jobs_count"),
-            link_books=data.get("link_books"),
-        )
     settings = orchestrator.get_settings()
     if not auth.is_admin(g.identity):
         del settings["personal_ini"]  # it can hold passwords for story sites

tests/test_api_flows.py

@@ -199,3 +199,17 @@ def test_a_story_change_does_not_ask_cwa_for_the_book(client, story, monkeypatch
     assert response.json["recheck_days"] == 4
     assert "chapters" not in response.json
     cwa.get_book.assert_not_called()
+
+
+def test_settings_are_saved_by_name(client):
+    with pytest.raises(ValueError, match="Unknown settings: \\['recent'\\]"):
+        o.save_settings(recent=3)
+    # The route passes only the known settings on.
+    response = client.put("/api/settings", json={"recent": 3, "recent_chapters": 4, "main_jobs_count": 9}, headers=H)
+    assert (response.status_code, response.json["recent_chapters"], response.json["main_jobs_count"]) == (200, 4, 9)
+    for data, error in [
+        ({"recent_chapters": -1}, "The number of recent chapters cannot be negative"),
+        ({"main_jobs_count": 501}, "main_jobs_count cannot be more than 500"),
+        ({"story_jobs_hours": "x"}, "story_jobs_hours must be a whole number"),
+    ]:
+        assert client.put("/api/settings", json=data, headers=H).json == {"error": error}