diff --git a/tests/test_delete_speaker.py b/tests/test_delete_speaker.py new file mode 100644 index 0000000..16c8e2a --- /dev/null +++ b/tests/test_delete_speaker.py @@ -0,0 +1,204 @@ +"""Tests for speaker deletion. + +The speakers page has offered a Delete button for a long time. The endpoint +it called, ``DELETE /api/speakers/{index}``, did not exist: htmx sent the +request and the server answered ``405 Method Not Allowed``, so the button +silently did nothing. + +Deleting a speaker who still has dialogue is also destructive in a way that +is easy to miss - the utterances keep a ``speaker_name`` that no longer +resolves - so the endpoint refuses unless the caller says where that dialogue +should go. +""" +from __future__ import annotations + +import json +import sqlite3 +from collections.abc import Iterator +from pathlib import Path +from typing import Any + +import pytest +from fastapi.testclient import TestClient + +SPEAKERS = [ + {"primary_name": "Alpha", "aliases": ["A-one"], "vocal_traits": ["calm"]}, + {"primary_name": "Beta", "aliases": [], "vocal_traits": ["deep"]}, + {"primary_name": "Silent", "aliases": [], "vocal_traits": []}, +] + + +@pytest.fixture() +def data_dir(tmp_path: Path) -> Iterator[Path]: + """A throwaway project, wired in as the active one.""" + from umbraaudio.config import AppConfig + + d = tmp_path / "proj" / ".umbra" + d.mkdir(parents=True) + (d / "speakers.json").write_text(json.dumps({"speakers": SPEAKERS})) + + # Use the app's own schema: _get_db runs SCHEMA_SQL on connect, and a + # hand-rolled minimal table fails against it. + from umbraaudio.web.routes import SCHEMA_SQL + + conn = sqlite3.connect(str(d / "project.db")) + conn.executescript(SCHEMA_SQL) + conn.execute( + "INSERT INTO utterances (chapter_id, utterance_index, speaker_name, text, type) " + "VALUES (1, 0, 'Beta', 'hello', 'dialogue')" + ) + conn.execute( + "INSERT INTO utterances (chapter_id, utterance_index, speaker_name, text, type) " + "VALUES (1, 1, 'A-one', 'via alias', 'dialogue')" + ) + conn.commit() + conn.close() + + cfg = AppConfig() + cfg.project_dir = tmp_path / "proj" + cfg.data_dir = d + + import umbraaudio.config as config_mod + + original = config_mod.load_config + config_mod.load_config = lambda _slug=None: cfg # type: ignore[assignment] + try: + yield d + finally: + config_mod.load_config = original # type: ignore[assignment] + + +@pytest.fixture() +def client(data_dir: Path, monkeypatch: pytest.MonkeyPatch) -> Iterator[TestClient]: + from umbraaudio.config import AppConfig + from umbraaudio.web import create_app + + cfg = AppConfig() + cfg.project_dir = data_dir.parent + cfg.data_dir = data_dir + + # The app is gated by fail-closed auth middleware, so an unauthenticated + # request is redirected to /login and TestClient follows it - which would + # turn every assertion into a check against the login page. Mint a real + # session with a pinned secret so the tests neither touch the production + # .umbra/session_secret nor need the real password. + import umbraaudio.auth as auth_mod + + monkeypatch.setattr(auth_mod, "_session_secret", lambda: b"k" * 32) + token = auth_mod.issue_session(auth_mod.USERNAME) + + # create_app caches the app in a module-level singleton, so it must be + # cleared between tests or the second test would reuse the first test's + # config - and therefore the first test's temporary directory. + import umbraaudio.web as web_mod + + web_mod._app = None + try: + c = TestClient(create_app(cfg), follow_redirects=False) + c.cookies.set("active_project", "testproj") + c.cookies.set(auth_mod.COOKIE_NAME, token) + yield c + finally: + web_mod._app = None + + +def _spk(data_dir: Path) -> list[dict[str, Any]]: + return json.loads((data_dir / "speakers.json").read_text())["speakers"] + + +def _names(data_dir: Path) -> list[str]: + return [s["primary_name"] for s in _spk(data_dir)] + + +def _rows(data_dir: Path) -> dict[str, str]: + conn = sqlite3.connect(str(data_dir / "project.db")) + rows = dict(conn.execute("SELECT text, speaker_name FROM utterances")) + conn.close() + return rows + + +def test_delete_speaker_without_dialogue_succeeds( + client: TestClient, data_dir: Path +) -> None: + idx = _names(data_dir).index("Silent") + assert client.delete(f"/api/speakers/{idx}").status_code == 200 + assert "Silent" not in _names(data_dir) + + +def test_delete_speaker_with_dialogue_is_refused( + client: TestClient, data_dir: Path +) -> None: + """Otherwise the utterances keep a name that no longer resolves.""" + idx = _names(data_dir).index("Beta") + r = client.delete(f"/api/speakers/{idx}") + assert r.status_code == 409 + body = r.json() + assert body["utterance_count"] == 1 + assert "Beta" in body["error"] + assert "Beta" in _names(data_dir), "must not be removed" + + +def test_refusal_offers_the_other_speakers_as_targets( + client: TestClient, data_dir: Path +) -> None: + idx = _names(data_dir).index("Beta") + options = client.delete(f"/api/speakers/{idx}").json()["options"] + assert "Alpha" in options + assert "Beta" not in options, "a speaker cannot be reassigned to itself" + + +def test_reassigning_moves_the_dialogue(client: TestClient, data_dir: Path) -> None: + idx = _names(data_dir).index("Beta") + assert client.delete(f"/api/speakers/{idx}?reassign_to=Alpha").status_code == 200 + assert _rows(data_dir)["hello"] == "Alpha" + assert "Beta" not in _names(data_dir) + + +def test_reassignment_keeps_the_old_name_as_an_alias( + client: TestClient, data_dir: Path +) -> None: + """So anything still referring to the deleted name keeps resolving.""" + idx = _names(data_dir).index("Beta") + client.delete(f"/api/speakers/{idx}?reassign_to=Alpha") + alpha = next(s for s in _spk(data_dir) if s["primary_name"] == "Alpha") + assert "Beta" in alpha["aliases"] + + +def test_dialogue_stored_under_an_alias_also_counts( + client: TestClient, data_dir: Path +) -> None: + """Utterances are frequently stored under an alias, not the primary name.""" + idx = _names(data_dir).index("Alpha") + r = client.delete(f"/api/speakers/{idx}") + assert r.status_code == 409 + assert r.json()["utterance_count"] == 1 + + +def test_reassignment_target_matches_case_insensitively( + client: TestClient, data_dir: Path +) -> None: + idx = _names(data_dir).index("Alpha") + assert client.delete(f"/api/speakers/{idx}?reassign_to=beta").status_code == 200 + assert _rows(data_dir)["via alias"] == "Beta" + assert "Alpha" not in _names(data_dir) + + +def test_unknown_reassignment_target_is_rejected( + client: TestClient, data_dir: Path +) -> None: + idx = _names(data_dir).index("Beta") + assert client.delete(f"/api/speakers/{idx}?reassign_to=Nobody").status_code == 400 + assert "Beta" in _names(data_dir), "must not delete" + + +def test_out_of_range_index_is_rejected(client: TestClient) -> None: + assert client.delete("/api/speakers/999").status_code == 400 + + +def test_deleting_leaves_other_speakers_untouched( + client: TestClient, data_dir: Path +) -> None: + idx = _names(data_dir).index("Silent") + client.delete(f"/api/speakers/{idx}") + alpha = next(s for s in _spk(data_dir) if s["primary_name"] == "Alpha") + assert alpha["aliases"] == ["A-one"], "unrelated aliases must be untouched" diff --git a/umbraaudio/web/routes.py b/umbraaudio/web/routes.py index f179af6..03cfc9f 100644 --- a/umbraaudio/web/routes.py +++ b/umbraaudio/web/routes.py @@ -2419,6 +2419,126 @@ async def merge_speakers(request: Request, source_index: int, target_index: int) ) +@router.delete("/api/speakers/{index}") +async def delete_speaker(request: Request, index: int, reassign_to: str = ""): + """Delete a speaker. The UI has offered this since before the endpoint existed. + + Deleting a speaker who still has dialogue would leave every one of their + utterances pointing at a name that no longer resolves - the orphaned- + utterance condition this project already has a review page for. So a + speaker with dialogue is refused unless the caller says where that + dialogue should go; ``reassign_to`` re-points it, exactly as a merge does. + A speaker with no dialogue is simply removed. + """ + speakers = _load_speakers(request) + if index < 0 or index >= len(speakers): + return JSONResponse({"error": "invalid index"}, status_code=400) + + speaker = speakers[index] + name = speaker.get("primary_name", "") + old_names = [n for n in ([name] + speaker.get("aliases", [])) if n] + + db = _get_db(request) + placeholders = ",".join("?" for _ in old_names) or "''" + count = db.execute( + f"SELECT COUNT(*) FROM utterances WHERE speaker_name IN ({placeholders})", + old_names, + ).fetchone()[0] + + if count and not reassign_to: + others = [ + s.get("primary_name", "") + for i, s in enumerate(speakers) + if i != index and s.get("primary_name") + ] + return JSONResponse( + { + "error": ( + f"{name} has {count} utterance(s). Choose which speaker " + "should speak them instead." + ), + "utterance_count": count, + "speaker_name": name, + "options": others, + }, + status_code=409, + ) + + if reassign_to: + target = next( + ( + s + for s in speakers + if s.get("primary_name", "").strip().casefold() + == reassign_to.strip().casefold() + ), + None, + ) + if target is None: + return JSONResponse( + {"error": f"no speaker named {reassign_to!r} to reassign to"}, + status_code=400, + ) + target_name = target.get("primary_name", "") + if name and name not in target.get("aliases", []): + target.setdefault("aliases", []).append(name) + moved = 0 + for old_name in old_names: + cur = db.execute( + "UPDATE utterances SET speaker_name = ? WHERE speaker_name = ?", + (target_name, old_name), + ) + moved += cur.rowcount or 0 + db.commit() + else: + target_name = "" + moved = 0 + + # Remove this speaker's own reference audio, but never a shared one. + removed_audio: list[str] = [] + config = _get_active_config(request) + voices_dir = config.data_dir / "voices" + slug = name.strip().lower().replace(" ", "_") + if slug and slug not in ("narrator", "general"): + for candidate in (slug + "_ref.wav", slug + ".wav"): + ref = voices_dir / candidate + if ref.exists(): + try: + ref.unlink() + removed_audio.append(candidate) + except OSError as e: # never fail the delete over a stale file + logger.warning("Could not remove %s: %s", ref, e) + + del speakers[index] + _save_speakers(request, speakers) + + # Drop the approval entry too. Leaving a stale "approved" behind would let a + # speaker re-created later inherit an approval nobody gave for it. + if name: + ledger = config.data_dir / "voice_status.json" + try: + if ledger.exists(): + data = json.loads(ledger.read_text()) + if isinstance(data, dict) and data.pop(name, None) is not None: + ledger.write_text(json.dumps(data, indent=2)) + except (OSError, ValueError) as e: + logger.warning("Could not update voice_status ledger: %s", e) + + speakers = _load_speakers(request) + return templates.TemplateResponse( + request, + "_speakers_list.html", + { + "request": request, + "speakers": speakers, + "deleted": name, + "reassigned_to": target_name, + "utterances_moved": moved, + "removed_audio": removed_audio, + }, + ) + + @router.get("/orphaned-utterances", response_class=HTMLResponse) async def orphaned_utterances_page(request: Request): db = _get_db(request) diff --git a/umbraaudio/web/templates/_speaker_row.html b/umbraaudio/web/templates/_speaker_row.html index e4168b6..0cf047e 100644 --- a/umbraaudio/web/templates/_speaker_row.html +++ b/umbraaudio/web/templates/_speaker_row.html @@ -140,13 +140,15 @@ onclick="toggleEditSpeaker({{ speaker_idx }})"> Edit - + {# Not hx-delete: htmx will not swap a 409, and this has to be + able to answer "this speaker has 12 utterances - where do they + go?". The handler also re-renders the whole list, because + deleting renumbers every speaker after it. #} + +