fix(plex): resolve deep-review findings on the Discover/Collections batch
Round of USER-triggered deep-review fixes (11 CONFIRMED + 1 PLAUSIBLE, all verified) on the 22-commit branch: - collections: split the mutate-then-resync error surface. A resync READ that fails after a successful Plex mutation no longer reports a false 422 "Plex write failed" + rollback; it returns 502 "sync pending" (the next full sync reconciles the mirror). New _write_or_pending_error helper; delete is exempt (no Plex read). Covered by new happy-path / 422-rollback / 502 tests. - New-collection dup-name guard is now advisory only: existingNames is the search-scoped, all-library union, but the backend enforces uniqueness PER library — so it no longer blocks a valid same-name collection in another library, and the backend 409 is localized to nameTaken. - member-remove now confirms before the write-through (matches delete-collection). - add-title / new-collection library search render an error state on request failure instead of a misleading "No matches". - history: seed navCounter past the current entry's persisted sfScroll on load so post-reload ids can't collide with back-stack entries and mis-restore scroll. - PageScroller: LRU-cap the navId-keyed savedScroll map (was unbounded). - share isIntrinsicReadOnly between the browse tile and the info page so the lock/edit affordance can't drift.
This commit is contained in:
@@ -208,6 +208,23 @@ def _reject_duplicate_name(
|
||||
raise HTTPException(status_code=409, detail="A collection with this name already exists")
|
||||
|
||||
|
||||
def _write_or_pending_error(wrote: bool, e: Exception) -> HTTPException:
|
||||
"""Map a PlexError raised inside a mutate-then-resync block to the honest status. If the mutating
|
||||
call itself failed (``wrote`` False) the write never landed → 422 "Plex write failed". If the
|
||||
write succeeded but the follow-up resync READ (Plex metadata/children) failed, the change IS on
|
||||
the Plex server → don't claim a write failure; report 502 "sync pending" (the next full sync
|
||||
reconciles the local mirror)."""
|
||||
if wrote:
|
||||
return HTTPException(
|
||||
status_code=502,
|
||||
detail=(
|
||||
"Collection updated in Plex, but refreshing the local copy failed; "
|
||||
f"it will reconcile on the next sync ({e})"
|
||||
),
|
||||
)
|
||||
return HTTPException(status_code=422, detail=f"Plex write failed: {e}")
|
||||
|
||||
|
||||
class CreateCollectionIn(BaseModel):
|
||||
library: str = ""
|
||||
title: str = ""
|
||||
@@ -239,15 +256,17 @@ def create_collection(
|
||||
if lib is None:
|
||||
raise HTTPException(status_code=404, detail="Unknown library")
|
||||
_reject_duplicate_name(db, lib.id, title)
|
||||
wrote = False
|
||||
try:
|
||||
with PlexClient(db) as plex:
|
||||
new = plex.create_collection(lib.plex_key, title, seed)
|
||||
wrote = True
|
||||
col = plex_sync.resync_collection(db, plex, lib, str(new.get("ratingKey")))
|
||||
col.editable = True
|
||||
db.commit()
|
||||
except (PlexError, PlexNotConfigured) as e:
|
||||
db.rollback()
|
||||
raise HTTPException(status_code=422, detail=f"Plex write failed: {e}")
|
||||
raise _write_or_pending_error(wrote, e)
|
||||
return _collection_card(col)
|
||||
|
||||
|
||||
@@ -260,14 +279,16 @@ def collection_add_item(
|
||||
) -> dict:
|
||||
col = _editable_collection_or_403(db, rating_key)
|
||||
lib = db.get(PlexLibrary, col.library_id)
|
||||
wrote = False
|
||||
try:
|
||||
with PlexClient(db) as plex:
|
||||
plex.add_collection_item(rating_key, str(item_rating_key))
|
||||
wrote = True
|
||||
col = plex_sync.resync_collection(db, plex, lib, rating_key)
|
||||
db.commit()
|
||||
except (PlexError, PlexNotConfigured) as e:
|
||||
db.rollback()
|
||||
raise HTTPException(status_code=422, detail=f"Plex write failed: {e}")
|
||||
raise _write_or_pending_error(wrote, e)
|
||||
return _collection_card(col)
|
||||
|
||||
|
||||
@@ -280,14 +301,16 @@ def collection_remove_item(
|
||||
) -> dict:
|
||||
col = _editable_collection_or_403(db, rating_key)
|
||||
lib = db.get(PlexLibrary, col.library_id)
|
||||
wrote = False
|
||||
try:
|
||||
with PlexClient(db) as plex:
|
||||
plex.remove_collection_item(rating_key, str(item_rating_key))
|
||||
wrote = True
|
||||
col = plex_sync.resync_collection(db, plex, lib, rating_key)
|
||||
db.commit()
|
||||
except (PlexError, PlexNotConfigured) as e:
|
||||
db.rollback()
|
||||
raise HTTPException(status_code=422, detail=f"Plex write failed: {e}")
|
||||
raise _write_or_pending_error(wrote, e)
|
||||
return _collection_card(col)
|
||||
|
||||
|
||||
@@ -308,14 +331,16 @@ def collection_rename(
|
||||
raise HTTPException(status_code=400, detail="title is required")
|
||||
_reject_duplicate_name(db, col.library_id, title, exclude_rk=rating_key)
|
||||
lib = db.get(PlexLibrary, col.library_id)
|
||||
wrote = False
|
||||
try:
|
||||
with PlexClient(db) as plex:
|
||||
plex.rename_collection(lib.plex_key, rating_key, title)
|
||||
wrote = True
|
||||
col = plex_sync.resync_collection(db, plex, lib, rating_key)
|
||||
db.commit()
|
||||
except (PlexError, PlexNotConfigured) as e:
|
||||
db.rollback()
|
||||
raise HTTPException(status_code=422, detail=f"Plex write failed: {e}")
|
||||
raise _write_or_pending_error(wrote, e)
|
||||
return _collection_card(col)
|
||||
|
||||
|
||||
|
||||
@@ -5,12 +5,15 @@ DB lane (`db` fixture) — skipped without Postgres.
|
||||
from datetime import datetime, timezone
|
||||
|
||||
from fastapi.testclient import TestClient
|
||||
from sqlalchemy import select
|
||||
|
||||
import app.state as state
|
||||
from app.auth import admin_user, current_user
|
||||
from app.db import get_db
|
||||
from app.main import app
|
||||
from app.models import PlexCollection, PlexItem, PlexLibrary, PlexShow, User
|
||||
from app.plex.client import PlexError
|
||||
from app.routes.plex import collections as coll_routes
|
||||
|
||||
|
||||
def _seed(db):
|
||||
@@ -142,6 +145,130 @@ def test_collection_detail_hero_and_ordered_members(db, monkeypatch):
|
||||
assert [i["id"] for i in d["items"]] == ["early", "later"] # release-year order
|
||||
|
||||
|
||||
# --- Write-through collection mutations (create/add/remove/rename write to Plex, then resync the local
|
||||
# mirror). We stub PlexClient so the DB lane can drive the happy path, the 422-on-mutation-failure
|
||||
# rollback, and the 502-on-resync-failure split (a successful write whose follow-up refresh read fails
|
||||
# must NOT be reported as a write failure). ------------------------------------------------------------
|
||||
class _CollStub:
|
||||
"""A PlexClient stand-in recording calls; `fail_on` makes one method raise PlexError."""
|
||||
|
||||
def __init__(self, fail_on: str | None = None):
|
||||
self.fail_on = fail_on
|
||||
self.calls: list[str] = []
|
||||
|
||||
def __enter__(self):
|
||||
return self
|
||||
|
||||
def __exit__(self, *a):
|
||||
return False
|
||||
|
||||
def _maybe(self, name: str):
|
||||
self.calls.append(name)
|
||||
if self.fail_on == name:
|
||||
raise PlexError("boom")
|
||||
|
||||
def create_collection(self, section_key, title, item_rating_key):
|
||||
self._maybe("create_collection")
|
||||
return {"ratingKey": "NEW"}
|
||||
|
||||
def add_collection_item(self, rk, item):
|
||||
self._maybe("add_collection_item")
|
||||
|
||||
def remove_collection_item(self, rk, item):
|
||||
self._maybe("remove_collection_item")
|
||||
|
||||
def rename_collection(self, section_key, rk, title):
|
||||
self._maybe("rename_collection")
|
||||
|
||||
def delete_collection(self, rk):
|
||||
self._maybe("delete_collection")
|
||||
|
||||
|
||||
def _resync_ok(db, plex, lib, rk):
|
||||
"""Stand in for the post-write refresh: return the existing local row (no live Plex read)."""
|
||||
return db.execute(select(PlexCollection).where(PlexCollection.rating_key == rk)).scalars().first()
|
||||
|
||||
|
||||
def _seed_editable(db):
|
||||
lib = PlexLibrary(plex_key="1", title="Movies", kind="movie", enabled=True)
|
||||
db.add(lib)
|
||||
db.flush()
|
||||
# Plain, admin-editable collection (title isn't a provider word, not smart → source "collection").
|
||||
db.add(PlexCollection(rating_key="A", library_id=lib.id, title="My Coll", editable=True,
|
||||
child_count=1))
|
||||
db.add(PlexItem(rating_key="m1", library_id=lib.id, kind="movie", title="M1",
|
||||
collection_keys=["A"]))
|
||||
user = User(email="mut@example.com")
|
||||
db.add(user)
|
||||
db.commit()
|
||||
return lib, user
|
||||
|
||||
|
||||
def _admin_client(db, user, monkeypatch, stub, resync=_resync_ok):
|
||||
monkeypatch.setattr(state, "_configured_cache", True)
|
||||
monkeypatch.setattr(coll_routes, "PlexClient", lambda db: stub)
|
||||
monkeypatch.setattr(coll_routes.plex_sync, "resync_collection", resync)
|
||||
app.dependency_overrides[get_db] = lambda: db
|
||||
app.dependency_overrides[current_user] = lambda: user
|
||||
app.dependency_overrides[admin_user] = lambda: user
|
||||
return TestClient(app)
|
||||
|
||||
|
||||
def test_rename_writes_through_to_plex(db, monkeypatch):
|
||||
_lib, user = _seed_editable(db)
|
||||
stub = _CollStub()
|
||||
client = _admin_client(db, user, monkeypatch, stub)
|
||||
try:
|
||||
r = client.patch("/api/plex/collections/A", json={"title": "Renamed"})
|
||||
assert r.status_code == 200, r.text
|
||||
assert "rename_collection" in stub.calls
|
||||
finally:
|
||||
app.dependency_overrides.clear()
|
||||
|
||||
|
||||
def test_remove_item_writes_through_to_plex(db, monkeypatch):
|
||||
_lib, user = _seed_editable(db)
|
||||
stub = _CollStub()
|
||||
client = _admin_client(db, user, monkeypatch, stub)
|
||||
try:
|
||||
r = client.delete("/api/plex/collections/A/items/m1")
|
||||
assert r.status_code == 200, r.text
|
||||
assert "remove_collection_item" in stub.calls
|
||||
finally:
|
||||
app.dependency_overrides.clear()
|
||||
|
||||
|
||||
def test_mutation_failure_returns_422_and_keeps_local_row(db, monkeypatch):
|
||||
_lib, user = _seed_editable(db)
|
||||
stub = _CollStub(fail_on="rename_collection")
|
||||
client = _admin_client(db, user, monkeypatch, stub)
|
||||
try:
|
||||
r = client.patch("/api/plex/collections/A", json={"title": "Renamed"})
|
||||
assert r.status_code == 422, r.text
|
||||
finally:
|
||||
app.dependency_overrides.clear()
|
||||
row = db.execute(select(PlexCollection).where(PlexCollection.rating_key == "A")).scalars().first()
|
||||
assert row.title == "My Coll" # rolled back — the write never landed
|
||||
|
||||
|
||||
def test_resync_failure_after_successful_write_returns_502(db, monkeypatch):
|
||||
# The mutation landed on Plex; only the follow-up refresh READ failed → 502 "sync pending", NOT a
|
||||
# false 422 "write failed". Regression guard for the mutate-then-resync error split.
|
||||
_lib, user = _seed_editable(db)
|
||||
stub = _CollStub()
|
||||
|
||||
def _resync_boom(db_, plex, lib_, rk):
|
||||
raise PlexError("read failed")
|
||||
|
||||
client = _admin_client(db, user, monkeypatch, stub, resync=_resync_boom)
|
||||
try:
|
||||
r = client.patch("/api/plex/collections/A", json={"title": "Renamed"})
|
||||
assert r.status_code == 502, r.text
|
||||
assert "rename_collection" in stub.calls # the write DID happen
|
||||
finally:
|
||||
app.dependency_overrides.clear()
|
||||
|
||||
|
||||
def test_show_collection_gets_montage_art_from_shows(db, monkeypatch):
|
||||
# A show-library collection must get its montage from PlexShow members (else it renders blank) — the
|
||||
# A3 fix that unions movie + show art. Runtime stays 0 (shows have no single duration).
|
||||
|
||||
Reference in New Issue
Block a user