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

@@ -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):
+    """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")
-        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()
+    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}
 
 

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")