From 2ada3b3d7ee0bee6f7659a8598a54b72d55b7581 Mon Sep 17 00:00:00 2001 From: Timothy Allen Date: Fri, 2 Oct 2026 23:46:34 +0200 Subject: [PATCH] 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 | 162 ++++++++++++++++++++-------------------- bindery/web.py | 17 +---- tests/test_api_flows.py | 14 ++++ 3 files changed, 98 insertions(+), 95 deletions(-) diff --git a/bindery/orchestrator.py b/bindery/orchestrator.py index a0a7782..76abeb2 100644 --- a/bindery/orchestrator.py +++ b/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] - if bad: - raise ValueError(f"Unknown metadata fields: {bad}") + _check_fields(force_fields or []) 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) - if days < 0: - raise ValueError("The number of days cannot be negative") + days = whole_number(recheck_days, "The number of days") 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] - if bad: - raise ValueError(f"Unknown metadata fields: {bad}") + _check_fields(locked_fields) 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] - if bad: - raise ValueError(f"Unknown metadata fields: {bad}") + _check_fields(values) 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: - try: - 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 +# The settings that save_settings takes. +SETTINGS = ("recent_chapters", "personal_ini", "link_books", "hold_new_links", "pause_uploads", *JOB_LIST_DEFAULTS) -def save_settings( - recent_chapters: Any = None, - 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. +def save_settings(**changes: Any) -> dict[str, Any]: + """Change the settings that are given, by the names in SETTINGS. A value of None changes nothing. - 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: - try: - numbers["recent_chapters"] = int(recent_chapters) - except (TypeError, ValueError) as e: - raise ValueError("The number of recent chapters must be a whole number") from e - if numbers["recent_chapters"] < 0: - 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) + if "recent_chapters" in changes: + numbers["recent_chapters"] = whole_number(changes["recent_chapters"], "The number of recent chapters") + for name in JOB_LIST_DEFAULTS: + if name in changes: + most = config.JOB_LIST_MAX_HOURS if name.endswith("hours") else config.JOB_LIST_MAX_COUNT + numbers[name] = whole_number(changes[name], name, most) 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 = store.create_story( - 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_id = _story_from_epub(normal[0], normal[1], title, meta, job.calibre_id, book_uuid or None) + store.update_story(story_id, ingested_at=now()) 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 = 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") + story_id = _story_from_epub(job.url, fff.normalize_url(job.url)[1], meta["title"] or job.url, meta) _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) - overrides = _epub_overrides(story, pinned) + title = _display_title(pinned, snap.title) + 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): diff --git a/bindery/web.py b/bindery/web.py index d6c5671..289e8d2 100644 --- a/bindery/web.py +++ b/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 int(value) - except (TypeError, ValueError): - raise ValueError(f"{name} must be a whole number") from None + return orchestrator.whole_number(value, name, negative=True) 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( - 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"), - ) + return orchestrator.save_settings(**{name: data.get(name) for name in orchestrator.SETTINGS}) settings = orchestrator.get_settings() if not auth.is_admin(g.identity): del settings["personal_ini"] # it can hold passwords for story sites diff --git a/tests/test_api_flows.py b/tests/test_api_flows.py index 64ec00e..be8fea1 100644 --- a/tests/test_api_flows.py +++ b/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}