From 0506086533b566e55f12c6c613b6e723e08feba6 Mon Sep 17 00:00:00 2001 From: Timothy Allen Date: Fri, 2 Oct 2026 21:14:28 +0200 Subject: [PATCH] Check every setting before the first one is stored A bad value in a settings request no longer leaves the other values of that request saved. --- bindery/orchestrator.py | 46 ++++++++++++++++++++++------------------- tests/test_api.py | 27 ++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 21 deletions(-) diff --git a/bindery/orchestrator.py b/bindery/orchestrator.py index 58ca4a5..04a36a0 100644 --- a/bindery/orchestrator.py +++ b/bindery/orchestrator.py @@ -377,30 +377,20 @@ def save_settings( 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.""" - warnings: list[str] = [] - released = False - if link_books is not None: - if not isinstance(link_books, bool): - raise ValueError("Link books to CWA library must be true or false") - if _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)): - 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) - store.set_setting(None, name, True if value else None) - if released and not _uploads_paused(): - _release_held_uploads() + """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. + """ + 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: - count = int(recent_chapters) + 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 count < 0: + if numbers["recent_chapters"] < 0: raise ValueError("The number of recent chapters cannot be negative") - store.set_setting(None, "recent_chapters", count) 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), @@ -408,13 +398,27 @@ def save_settings( ("main_jobs_count", main_jobs_count, config.JOB_LIST_MAX_COUNT), ): if value is not None: - store.set_setting(None, name, _validate_job_list_value(value, name, maximum)) + 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)): + 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) + store.set_setting(None, name, True if value else None) + for name, number in numbers.items(): + store.set_setting(None, name, number) if personal_ini is not None: - warnings = fff.check_personal_ini(personal_ini) # Write a new file, then replace the old one, so a job never reads half a file. partial = config.PERSONAL_INI.with_name("personal.ini.part") partial.write_text(personal_ini, encoding="utf-8") partial.replace(config.PERSONAL_INI) + if released and not _uploads_paused(): + _release_held_uploads() return {**get_settings(), "warnings": warnings} diff --git a/tests/test_api.py b/tests/test_api.py index 81a2340..82e87fa 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -280,6 +280,33 @@ def test_delete_revisions(client): assert [r["rev"] for r in client.get(f"/api/chapters/{chapter_id}/diff").json["revisions"]] == [1, 3] +@pytest.mark.parametrize( + "bad", + [ + {"recent_chapters": "many"}, + {"main_jobs_count": 100000}, + {"personal_ini": "[defaults\nfoo"}, + {"link_books": "yes"}, + ], +) +def test_a_bad_setting_changes_no_setting(client, monkeypatch, tmp_path, bad): + ini = tmp_path / "personal.ini" + ini.write_text("[defaults]\n") + monkeypatch.setattr(config, "PERSONAL_INI", ini) + monkeypatch.setattr(config, "CWA_URL", "https://cwa.invalid") + before = client.get("/api/settings").json + good = { + "link_books": False, + "hold_new_links": True, + "pause_uploads": True, + "recent_chapters": 4, + "story_jobs_hours": 1, + "personal_ini": "[defaults]\nis_adult:true\n", + } + assert client.put("/api/settings", json={**good, **bad}, headers=H).status_code == 400 + assert client.get("/api/settings").json == before + + def test_settings(client, monkeypatch, tmp_path): ini = tmp_path / "personal.ini" ini.write_text("[defaults]\nis_adult:true\n")