fix(e5): deep-review round — combobox Enter, parse_person collision, enrich hardening + coverage
Full-epic /deep-review (11 finders + 12 adversarial verifiers) surfaced no HIGH; every mechanism finding calibrated to LOW on verification. Fix all 13: Correctness / reliability: - Combobox: Enter now commits the VISUALLY highlighted row (highlightIndex ?? activeIndex), not the bare keyboard activeIndex — so arrow-then-hover-then-Enter no longer toggles a different option than the one painted, and hover-with-no-arrow is no longer a silent no-op. - parse_person: dedup the known_for pool by (id, media_type), not the bare id — TMDB movie and tv id spaces are independent, so a same-id movie+show pair is two distinct credits (matches the collision-safe key the rest of the module already uses). - person-enrich: a per-person in-flight + 60s cooldown guard (people._should_schedule_enrich) dedups concurrent opens and rate-limits re-triggering while TMDB is degraded (a transient TmdbError leaves the watermark NULL), so bursts can't pile 8s tasks onto the shared threadpool. - PlexInfo compact()/money(): format with i18n.language (keeping currency USD) instead of a hardcoded en/en-US, matching the app's Intl convention. - delete the dead `plex.filter.add` i18n key (both locales) — the +Add button that used it is gone. Reuse: - _continue_watching reuses rollup.agg_subquery(include_resumed=True) + query.apply_show_state_filter instead of a third hand-inlined copy of the per-show rollup + in_progress predicate, so the Discover rail can't drift from the grid's definition. Test coverage (the gaps a regression would ship through): - resolve_library_guids scope-exclusion (a credit in a non-scoped/disabled library is dropped) + the []-vs-None distinction; parse_person cross-media-id retention; enrich_person transient (NULL watermark) and 404 (stamp) branches; _enrich_person_bg body (fresh session close/rollback, None-guard) + the cooldown dedup; continue_watching fully-watched-show exclusion; Combobox Enter-commits-highlighted + async/controlled-query mode; PopoverItem aria-checked contract; RatingsRow missing-IMDb + imdb==audience branches. Backend lane 413→420, FE vitest 351→357; all gates green. No migration.
This commit is contained in:
@@ -47,6 +47,22 @@ def test_parse_person_empty_payload():
|
||||
assert p["biography"] is None and p["known_for"] is None
|
||||
|
||||
|
||||
def test_parse_person_keeps_same_id_across_media_types():
|
||||
# TMDB movie ids and tv ids are independent, so a movie-42 and a tv-42 credit are two distinct works
|
||||
# and BOTH must survive the dedup (keyed on (id, media_type), not the bare id).
|
||||
payload = {
|
||||
"combined_credits": {
|
||||
"cast": [
|
||||
{"id": 42, "media_type": "movie", "title": "Movie 42", "popularity": 90},
|
||||
{"id": 42, "media_type": "tv", "name": "Show 42", "popularity": 80},
|
||||
{"id": 42, "media_type": "movie", "title": "dup movie 42", "popularity": 5}, # real dup
|
||||
]
|
||||
}
|
||||
}
|
||||
kf = plex_tmdb.parse_person(payload)["known_for"]
|
||||
assert [(k["tmdb_id"], k["media_type"]) for k in kf] == [(42, "movie"), (42, "tv")] # both kept, dup dropped
|
||||
|
||||
|
||||
class _FakeTmdb:
|
||||
def __init__(self, payload):
|
||||
self.payload = payload
|
||||
@@ -73,6 +89,49 @@ def test_enrich_person_persists_and_stamps(db, monkeypatch):
|
||||
assert [k["tmdb_id"] for k in person.known_for] == [20, 10, 30]
|
||||
|
||||
|
||||
class _FakeTmdbResult:
|
||||
"""Context-manager TmdbClient stand-in whose fetch_person raises / returns a fixed payload."""
|
||||
|
||||
def __init__(self, *, raises=False, payload=None):
|
||||
self._raises = raises
|
||||
self._payload = payload
|
||||
|
||||
def __enter__(self):
|
||||
return self
|
||||
|
||||
def __exit__(self, *a):
|
||||
return False
|
||||
|
||||
def fetch_person(self, pid):
|
||||
if self._raises:
|
||||
raise plex_tmdb.TmdbError("429 rate limited")
|
||||
return self._payload
|
||||
|
||||
|
||||
def test_enrich_person_transient_error_leaves_watermark_null(db, monkeypatch):
|
||||
# A transient TmdbError must NOT stamp the watermark, so the next open retries (never permanently
|
||||
# blank a person after one blip).
|
||||
person = PlexPerson(name="Transient P", tmdb_person_id=555)
|
||||
db.add(person)
|
||||
db.commit()
|
||||
monkeypatch.setattr(plex_tmdb.sysconfig, "get_str", lambda d, k: "key" if k == "tmdb_api_key" else "")
|
||||
monkeypatch.setattr(plex_tmdb, "TmdbClient", lambda d, timeout=None: _FakeTmdbResult(raises=True))
|
||||
assert plex_tmdb.enrich_person(db, person) is False
|
||||
assert person.tmdb_enriched_at is None # left NULL → retried next open
|
||||
|
||||
|
||||
def test_enrich_person_404_stamps_watermark_without_bio(db, monkeypatch):
|
||||
# A 404 (deleted person → None payload) stamps the watermark so it isn't refetched every open.
|
||||
person = PlexPerson(name="Gone P", tmdb_person_id=555)
|
||||
db.add(person)
|
||||
db.commit()
|
||||
monkeypatch.setattr(plex_tmdb.sysconfig, "get_str", lambda d, k: "key" if k == "tmdb_api_key" else "")
|
||||
monkeypatch.setattr(plex_tmdb, "TmdbClient", lambda d, timeout=None: _FakeTmdbResult(payload=None))
|
||||
assert plex_tmdb.enrich_person(db, person) is True
|
||||
assert person.tmdb_enriched_at is not None # stamped
|
||||
assert person.biography is None # nothing fetched
|
||||
|
||||
|
||||
def test_enrich_person_noop_without_key(db, monkeypatch):
|
||||
person = PlexPerson(name="Actor Y", tmdb_person_id=555)
|
||||
db.add(person)
|
||||
@@ -116,6 +175,7 @@ def test_person_detail_schedules_enrich_off_request_path(db, monkeypatch):
|
||||
|
||||
scheduled: list[int] = []
|
||||
monkeypatch.setattr(people, "_enrich_person_bg", lambda pid: scheduled.append(pid))
|
||||
people._enrich_inflight.clear() # ensure the per-person cooldown guard doesn't suppress this open
|
||||
|
||||
resp = _get(db, user, monkeypatch, "/api/plex/person/Lazy P")
|
||||
assert resp.status_code == 200, resp.text
|
||||
@@ -123,6 +183,62 @@ def test_person_detail_schedules_enrich_off_request_path(db, monkeypatch):
|
||||
assert len(scheduled) == 1 # the enrich was scheduled off the request path
|
||||
|
||||
|
||||
def test_enrich_person_bg_closes_session_and_isolates_errors(monkeypatch):
|
||||
# The background-task body must open a fresh session, fetch the person, and ALWAYS close it — rolling
|
||||
# back (never propagating) if enrich_person raises. Pure-unit via a fake session (no DB needed).
|
||||
from app.routes.plex import people
|
||||
|
||||
calls = {"rollback": 0, "close": 0, "enriched": 0}
|
||||
|
||||
class _FakeSession:
|
||||
def __init__(self, person):
|
||||
self._person = person
|
||||
|
||||
def get(self, model, pid):
|
||||
return self._person
|
||||
|
||||
def rollback(self):
|
||||
calls["rollback"] += 1
|
||||
|
||||
def close(self):
|
||||
calls["close"] += 1
|
||||
|
||||
# Success path: person found → enrich called, session closed, no rollback.
|
||||
monkeypatch.setattr(people, "SessionLocal", lambda: _FakeSession(object()))
|
||||
monkeypatch.setattr(
|
||||
people.plex_tmdb, "enrich_person", lambda db, p: calls.__setitem__("enriched", calls["enriched"] + 1)
|
||||
)
|
||||
people._enrich_person_bg(5)
|
||||
assert calls == {"rollback": 0, "close": 1, "enriched": 1}
|
||||
|
||||
# Error path: enrich raises → rollback + close, no exception escapes.
|
||||
calls.update({"rollback": 0, "close": 0, "enriched": 0})
|
||||
|
||||
def _boom(db, p):
|
||||
raise RuntimeError("enrich blew up")
|
||||
|
||||
monkeypatch.setattr(people.plex_tmdb, "enrich_person", _boom)
|
||||
people._enrich_person_bg(5) # must not raise
|
||||
assert calls["rollback"] == 1 and calls["close"] == 1
|
||||
|
||||
# Missing person (deleted id): None-guard skips enrich, session still closed.
|
||||
calls.update({"rollback": 0, "close": 0, "enriched": 0})
|
||||
monkeypatch.setattr(people, "SessionLocal", lambda: _FakeSession(None))
|
||||
people._enrich_person_bg(9)
|
||||
assert calls == {"rollback": 0, "close": 1, "enriched": 0}
|
||||
|
||||
|
||||
def test_should_schedule_enrich_dedups_within_cooldown():
|
||||
# Concurrent/rapid opens of the same person must schedule the enrich only once per cooldown window.
|
||||
from app.routes.plex import people
|
||||
|
||||
people._enrich_inflight.clear()
|
||||
assert people._should_schedule_enrich(12345) is True # first open reserves
|
||||
assert people._should_schedule_enrich(12345) is False # within cooldown → deduped
|
||||
assert people._should_schedule_enrich(99999) is True # a different person is independent
|
||||
people._enrich_inflight.clear()
|
||||
|
||||
|
||||
def test_person_detail_no_enrich_when_already_enriched(db, monkeypatch):
|
||||
# An already-enriched person (tmdb_enriched_at set) must NOT schedule another background fetch.
|
||||
from datetime import datetime, timezone
|
||||
|
||||
Reference in New Issue
Block a user