Settings fixes from a full UI test pass

core/system_utils.py

@@ -6,6 +6,7 @@
 import os
 import platform
 import posixpath
+import re
 import shutil
 import subprocess
 import atexit
@@ -319,27 +320,52 @@ def parse_size_bytes(size_str: str) -> int:
     if not size_str or size_str.strip() == "0":
         return 0
     size_str = size_str.strip().upper()
+    units = (('TB', 1024**4), ('GB', 1024**3), ('MB', 1024**2),
+             ('T', 1024**4), ('G', 1024**3), ('M', 1024**2))
+    number, multiplier = size_str, 1024**3  # Bare numbers default to GB
+    for suffix, factor in units:
+        if size_str.endswith(suffix):
+            number, multiplier = size_str[:-len(suffix)], factor
+            break
     try:
-        if size_str.endswith('TB'):
-            return int(float(size_str[:-2]) * 1024**4)
-        elif size_str.endswith('GB'):
-            return int(float(size_str[:-2]) * 1024**3)
-        elif size_str.endswith('MB'):
-            return int(float(size_str[:-2]) * 1024**2)
-        elif size_str.endswith('T'):
-            return int(float(size_str[:-1]) * 1024**4)
-        elif size_str.endswith('G'):
-            return int(float(size_str[:-1]) * 1024**3)
-        elif size_str.endswith('M'):
-            return int(float(size_str[:-1]) * 1024**2)
-        else:
-            return int(float(size_str) * 1024**3)  # Default to GB
+        value = int(float(number) * multiplier)
     except ValueError:
+        value = None
+    # Callers treat a negative result as a percentage (see
+    # ConfigManager._parse_cache_limit), so a negative size must never get through.
+    if value is None or value < 0:
         # 0 means "no limit" to every caller, so an unparseable value silently
         # removes the cap the user thought they set. Say so; callers that can
         # name the setting add their own message on top.
         logging.warning(f"Could not read '{size_str}' as a size. Expected e.g. 500GB, 1.5T or 250. Treating as unset.")
         return 0
+    return value
+
+
+def size_setting_error(value: str, allow_percent: bool = True) -> Optional[str]:
+    """Why a size setting (cache_limit, min_free_space, ...) can't be read, or None.
+
+    Accepts the same formats as parse_size_bytes() and, when allow_percent,
+    a percentage from just above 0 to 100. Empty or "0" means unset and is valid.
+    Used by the Settings page to refuse a value the engine would ignore.
+    """
+    text = (value or "").strip()
+    if not text or text == "0":
+        return None
+    hint = "Use a size like 500GB or 1.5T" + (", or a percentage like 75%" if allow_percent else "")
+    if text.endswith("%"):
+        if not allow_percent:
+            return f"'{text}' is a percentage; this needs a size. {hint}."
+        try:
+            percent = float(text[:-1])
+        except ValueError:
+            return f"'{text}' isn't a percentage. {hint}."
+        if not 0 < percent <= 100:
+            return f"'{text}' must be more than 0% and at most 100%."
+        return None
+    if not re.fullmatch(r"\d+(\.\d+)?\s*(TB|GB|MB|T|G|M)?", text, re.IGNORECASE):
+        return f"'{text}' isn't a size. {hint}."
+    return None
 
 
 def format_bytes(bytes_value: int) -> str:

tests/test_badge_icons.py

@@ -0,0 +1,28 @@
+"""Badge icon sizing and alignment.
+
+Lucide replaces <i data-lucide> with <svg>, so `.badge i` never applied and
+badge icons rendered at 24px. That made icon badges taller, and with baseline
+alignment a text-only badge beside one (Recently Added: "Returns when watched"
++ "OnDeck") sat lower.
+"""
+
+import re
+from pathlib import Path
+
+CSS = (Path(__file__).resolve().parents[1] / "web" / "static" / "css" / "plex-theme.css").read_text(encoding="utf-8")
+
+
+def _rule(selector):
+    match = re.search(r"(?m)^" + re.escape(selector) + r"\s*\{([^}]*)\}", CSS)
+    assert match, selector
+    return match.group(1)
+
+
+def test_badge_icons_are_sized_as_svg():
+    body = _rule(".badge svg")
+    assert "width: 12px" in body and "height: 12px" in body
+    assert not re.search(r"(?m)^\.badge i\s*\{", CSS)
+
+
+def test_badges_align_by_middle():
+    assert "vertical-align: middle" in _rule(".badge")

tests/test_custom_path_mappings.py

@@ -0,0 +1,251 @@
+"""Custom path mappings (not linked to a Plex library) on Settings → Libraries.
+
+Delete used to target #path-mappings, which only existed on the retired Paths
+page, so htmx dropped the request. Add targeted a container that was only
+rendered once a custom mapping existed. Cards address mappings by list index,
+which shifts whenever a mapping is deleted, so they also send the paths they
+were rendered with and the server refuses a stale card.
+
+Mounts only the settings router on a minimal app, backed by a real
+SettingsService over a temp settings file, with Plex library discovery mocked.
+"""
+
+import html
+import json
+import re
+import sys
+from unittest.mock import MagicMock, patch
+
+import pytest
+from fastapi import FastAPI
+from fastapi.testclient import TestClient
+
+sys.modules.setdefault('fcntl', MagicMock())
+for _mod in [
+    'apscheduler', 'apscheduler.schedulers', 'apscheduler.schedulers.background',
+    'apscheduler.triggers', 'apscheduler.triggers.cron', 'apscheduler.triggers.interval',
+    'plexapi', 'plexapi.server',
+]:
+    sys.modules.setdefault(_mod, MagicMock())
+
+
+def _force_real_modules():
+    for name in ["web.config", "web.routers", "web.routers.settings",
+                 "web.services", "web.services.settings_service", "web"]:
+        mod = sys.modules.get(name)
+        if isinstance(mod, MagicMock):
+            del sys.modules[name]
+    import web  # noqa: F401
+    import web.config  # noqa: F401
+    import web.routers.settings  # noqa: F401
+    import web.services.settings_service  # noqa: F401
+
+
+_force_real_modules()
+
+_MOVIES_LIB = {
+    "id": 4, "title": "Movies", "type": "movie", "type_label": "Movies",
+    "locations": ["/data/Movies/"],
+}
+
+
+def _mapping(name, plex, real, section_id=None):
+    m = {"name": name, "plex_path": plex, "real_path": real,
+         "cache_path": "/mnt/cache" + real[len("/mnt/user"):],
+         "cacheable": True, "enabled": True}
+    if section_id is not None:
+        m["section_id"] = section_id
+    return m
+
+
+MOVIES = _mapping("Movies", "/data/Movies/", "/mnt/user/Movies/", section_id=4)
+MOVIES_UHD = _mapping("Movies UHD", "/data/UHD/", "/mnt/user/UHD/", section_id=4)
+MUSIC = _mapping("Music", "/data/Music/", "/mnt/user/Music/")
+AUDIOBOOKS = _mapping("Audiobooks", "/data/Books/", "/mnt/user/Books/")
+
+
+@pytest.fixture
+def make_client(tmp_path):
+    def make(mappings):
+        settings_file = tmp_path / "plexcache_settings.json"
+        settings_file.write_text(json.dumps({
+            "PLEX_URL": "http://localhost:32400", "PLEX_TOKEN": "abc",
+            "valid_sections": [4], "path_mappings": mappings, "cache_dir": "/mnt/cache",
+        }, indent=2), encoding="utf-8")
+        return settings_file
+
+    patches = []
+
+    def client(mappings):
+        settings_file = make(mappings)
+        p1 = patch("web.services.settings_service.SETTINGS_FILE", settings_file)
+        p2 = patch("web.services.settings_service.DATA_DIR", tmp_path)
+        p1.start(); p2.start(); patches.extend([p1, p2])
+        from web.services.settings_service import SettingsService
+        svc = SettingsService()
+        svc._cached_settings = None
+        p3 = patch.object(svc, "get_plex_libraries", return_value=[_MOVIES_LIB])
+        p3.start(); patches.append(p3)
+        from web.routers import settings as settings_router
+        p4 = patch("web.routers.settings.get_settings_service", return_value=svc)
+        p4.start(); patches.append(p4)
+        app = FastAPI()
+        app.include_router(settings_router.router, prefix="/settings")
+        return TestClient(app), svc
+
+    yield client
+    for p in reversed(patches):
+        p.stop()
+
+
+def _names(svc):
+    return [m["name"] for m in svc._load_raw()["path_mappings"]]
+
+
+def _expected(m):
+    return {"expected_plex_path": m["plex_path"], "expected_real_path": m["real_path"]}
+
+
+def _edit_form(m, **changes):
+    form = {"name": m["name"], "plex_path": m["plex_path"], "real_path": m["real_path"],
+            "cache_path": m["cache_path"], "cacheable": "on", "enabled": "on", **_expected(m)}
+    form.update(changes)
+    return form
+
+
+# -- page wiring ------------------------------------------------------------
+
+def test_every_hx_target_on_the_libraries_page_exists(make_client):
+    """The Delete button pointed at an element this page never renders."""
+    client, _ = make_client([MOVIES, MOVIES_UHD, MUSIC])
+    page = client.get("/settings/libraries").text
+    # The alert container lives in settings/index.html's layout.
+    ids = set(re.findall(r'\bid="([^"]+)"', page))
+    targets = set(re.findall(r'hx-target="#([^"]+)"', page))
+    assert targets, "expected hx-target attributes on the page"
+    missing = sorted(t for t in targets if t not in ids)
+    assert not missing, missing
+
+
+def test_delete_url_carries_the_expected_paths(make_client):
+    """In the URL, not hx-vals: htmx 1.9 sends DELETE values in the body."""
+    kids = _mapping("Kids & Family", "/data/Kids & Family/", "/mnt/user/Kids/")
+    client, svc = make_client([MOVIES, MUSIC, kids])
+    page = client.get("/settings/libraries").text
+
+    def delete_url(index):
+        found = re.search(r'hx-delete="(/settings/paths/%d\?[^"]+)"' % index, page)
+        return html.unescape(found.group(1))   # what the browser sends
+
+    # The page's own URL deletes the right mapping, '&' in the path included.
+    r = client.delete(delete_url(2))
+    assert r.status_code == 200
+    assert _names(svc) == ["Movies", "Music"]
+
+    # Music's card is now stale-safe too: after removing Music elsewhere,
+    # its old URL is refused instead of deleting whatever sits at index 1.
+    svc.delete_path_mapping(1)
+    svc.add_path_mapping(AUDIOBOOKS)
+    r = client.delete(delete_url(1))
+    assert "changed since this page loaded" in r.text
+    assert _names(svc) == ["Movies", "Audiobooks"]
+
+
+def test_custom_section_rendered_even_when_empty(make_client):
+    client, _ = make_client([MOVIES])
+    page = client.get("/settings/libraries").text
+    assert 'id="custom-mappings"' in page
+    assert 'hx-target="#custom-mappings"' in page          # the Add form
+    assert 'id="orphan-mappings"' not in page              # no heading or list without cards
+
+
+# -- add --------------------------------------------------------------------
+
+def test_add_first_custom_mapping_returns_the_section(make_client):
+    client, svc = make_client([MOVIES])
+    r = client.post("/settings/paths", data={
+        "name": "Music", "plex_path": "/data/Music/", "real_path": "/mnt/user/Music/",
+        "cache_path": "/mnt/cache/Music/", "cacheable": "on", "enabled": "on"})
+    assert r.status_code == 200
+    assert r.text.lstrip().startswith('<div id="custom-mappings"')
+    assert "Custom Mappings" in r.text and "Music" in r.text
+    assert 'hx-delete="/settings/paths/1?' in r.text
+    assert _names(svc) == ["Movies", "Music"]
+
+
+# -- delete -----------------------------------------------------------------
+
+def test_delete_custom_mapping_rerenders_with_fresh_indices(make_client):
+    client, svc = make_client([MUSIC, MOVIES, AUDIOBOOKS])
+    r = client.delete("/settings/paths/0", params=_expected(MUSIC))
+    assert r.status_code == 200
+    assert _names(svc) == ["Movies", "Audiobooks"]
+    assert "Music" not in r.text
+    # Audiobooks moved from index 2 to 1; the re-rendered card says so.
+    assert 'hx-delete="/settings/paths/1?' in r.text
+    assert 'hx-delete="/settings/paths/2?' not in r.text
+    assert 'id="custom-mappings"' in r.text
+
+
+def test_delete_last_custom_mapping_leaves_an_empty_section(make_client):
+    client, svc = make_client([MOVIES, MUSIC])
+    r = client.delete("/settings/paths/1", params=_expected(MUSIC))
+    assert _names(svc) == ["Movies"]
+    assert 'id="custom-mappings"' in r.text and 'id="orphan-mappings"' not in r.text
+
+
+def test_stale_delete_is_refused(make_client):
+    """Card rendered for Audiobooks at index 2, but the list has since shifted."""
+    client, svc = make_client([MOVIES, AUDIOBOOKS])
+    r = client.delete("/settings/paths/1", params=_expected(MUSIC))
+    assert _names(svc) == ["Movies", "Audiobooks"]
+    assert r.headers["HX-Retarget"] == "#settings-alert-container"
+    assert "changed since this page loaded" in r.text
+
+    r = client.delete("/settings/paths/5", params=_expected(AUDIOBOOKS))
+    assert _names(svc) == ["Movies", "Audiobooks"]
+    assert "changed since this page loaded" in r.text
+
+
+# -- edit -------------------------------------------------------------------
+
+def test_edit_custom_mapping(make_client):
+    client, svc = make_client([MOVIES, MUSIC])
+    r = client.put("/settings/paths/1", data=_edit_form(MUSIC, name="Music FLAC"))
+    assert r.status_code == 200
+    assert _names(svc) == ["Movies", "Music FLAC"]
+
+
+def test_stale_edit_is_refused(make_client):
+    """Without the check this would overwrite Audiobooks with Music's form."""
+    client, svc = make_client([MOVIES, AUDIOBOOKS])
+    r = client.put("/settings/paths/1", data=_edit_form(MUSIC, name="Music FLAC"))
+    assert _names(svc) == ["Movies", "Audiobooks"]
+    assert r.headers["HX-Retarget"] == "#settings-alert-container"
+
+
+# -- library card save that removes a mapping --------------------------------
+
+def test_library_save_with_removal_refreshes_custom_cards(make_client):
+    client, svc = make_client([MOVIES, MOVIES_UHD, MUSIC])
+    form = {
+        "name_0": "Movies", "plex_path_0": MOVIES["plex_path"], "real_path_0": MOVIES["real_path"],
+        "cache_path_0": MOVIES["cache_path"], "host_cache_path_0": "", "cacheable_0": "on",
+        "name_1": "Movies UHD", "plex_path_1": MOVIES_UHD["plex_path"],
+        "real_path_1": MOVIES_UHD["real_path"], "cache_path_1": MOVIES_UHD["cache_path"],
+        "host_cache_path_1": "", "delete_1": "1",
+    }
+    r = client.put("/settings/libraries/4/paths", data=form)
+    assert _names(svc) == ["Movies", "Music"]
+    assert 'id="library-card-4"' in r.text
+    # Music moved from index 2 to 1; its card is replaced out-of-band.
+    assert '<div id="custom-mappings" hx-swap-oob="true">' in r.text
+    assert 'hx-delete="/settings/paths/1?' in r.text
+
+
+def test_library_save_without_removal_leaves_custom_cards_alone(make_client):
+    client, _ = make_client([MOVIES, MUSIC])
+    form = {"name_0": "Movies", "plex_path_0": MOVIES["plex_path"], "real_path_0": MOVIES["real_path"],
+            "cache_path_0": MOVIES["cache_path"], "host_cache_path_0": "", "cacheable_0": "on"}
+    r = client.put("/settings/libraries/4/paths", data=form)
+    assert "hx-swap-oob" not in r.text