diff --git a/backend/app/discover/pins.py b/backend/app/discover/pins.py index b4be5f85..aa79ecd8 100644 --- a/backend/app/discover/pins.py +++ b/backend/app/discover/pins.py @@ -41,28 +41,34 @@ def resolve_ref(db: Session, user: User, kind: str, ref: str) -> tuple[str, obje The client speaks the ids the rest of the API speaks: a video id, a channel id, a Plex `rating_key`, a playlist id. Plex's internal integer keys never leave the backend, so the - translation belongs here rather than in five call sites.""" + translation belongs here rather than in five call sites. + + The COLUMN half of every answer comes from `PIN_COLUMNS`; only the LOOKUP differs per kind + (a direct `db.get`, a rating-key translation, an int parse plus an ownership check). Before the + E24 review the branches spelled the column names out again and `PIN_COLUMNS` had no readers at + all — a constant that called itself the one vocabulary while nothing spoke it.""" + col = PIN_COLUMNS.get(kind) + if col is None: + return None if kind == "video": - return ("video_id", ref) if db.get(Video, ref) is not None else None + return (col, ref) if db.get(Video, ref) is not None else None if kind == "channel": - return ("channel_id", ref) if db.get(Channel, ref) is not None else None + return (col, ref) if db.get(Channel, ref) is not None else None if kind == "plex_item": it = by_rating_key(db, PlexItem, ref) - return ("plex_item_id", it.id) if it is not None else None + return (col, it.id) if it is not None else None if kind == "plex_show": sh = by_rating_key(db, PlexShow, ref) - return ("plex_show_id", sh.id) if sh is not None else None - if kind == "playlist": - try: - pid = int(ref) - except ValueError: - return None - pl = db.get(Playlist, pid) - # A playlist is per-user, unlike the other four (which are shared catalog rows whose PRIVACY - # lives in the state tables). Someone else's list must read as "no such thing", not as a 403 — - # the same reason `owned_or_404` 404s rather than 403s. - return ("playlist_id", pl.id) if pl is not None and pl.user_id == user.id else None - return None + return (col, sh.id) if sh is not None else None + try: + pid = int(ref) + except ValueError: + return None + pl = db.get(Playlist, pid) + # A playlist is per-user, unlike the other four (which are shared catalog rows whose PRIVACY + # lives in the state tables). Someone else's list must read as "no such thing", not as a 403 — + # the same reason `owned_or_404` 404s rather than 403s. + return (col, pl.id) if pl is not None and pl.user_id == user.id else None def list_pins(db: Session, user: User) -> list[ComfortPin]: diff --git a/backend/app/discover/rails.py b/backend/app/discover/rails.py index 4a622898..0598e0a1 100644 --- a/backend/app/discover/rails.py +++ b/backend/app/discover/rails.py @@ -6,8 +6,6 @@ side-by-side rails would just move the choice the user could not make ("which ha level up, which is the opposite of what this epic is for. """ -from datetime import datetime, timezone - from sqlalchemy.orm import Session from app import sysconfig @@ -15,11 +13,7 @@ from app.discover import cards, queries from app.discover.queries import RAIL_LIMIT from app.models import PlexShow, User from app.plex.lookups import by_rating_key - -# Sort sentinel for a row whose timestamp is missing. A plain `(ts is not None, ts)` key looks like it -# handles NULLs and does not: two NULL rows compare equal on the flag and then compare `None < None`, -# which raises. A rail must not be able to 500 the landing page over missing metadata. -_NO_TS = datetime.min.replace(tzinfo=timezone.utc) +from app.plex.rollup import NO_TS def plex_available(db: Session) -> bool: @@ -64,7 +58,7 @@ def continue_watching(db: Session, user: User, limit: int = RAIL_LIMIT) -> list[ ] if plex_available(db): rows += [(ts, cards.from_plex(c)) for ts, c in queries.plex_in_progress(db, user.id, limit)] - rows.sort(key=lambda r: r[0] or _NO_TS, reverse=True) + rows.sort(key=lambda r: r[0] or NO_TS, reverse=True) out = [card for _, card in rows[:limit]] # Resolve AFTER the slice: only the rows that survive it are worth a query. for card in out: diff --git a/backend/app/plex/rollup.py b/backend/app/plex/rollup.py index 4ea9d228..53211250 100644 --- a/backend/app/plex/rollup.py +++ b/backend/app/plex/rollup.py @@ -3,11 +3,25 @@ Drives the show/season grid badges and the Resume / Play-from-start / mark-all buttons. """ +from datetime import datetime, timezone + from sqlalchemy import and_, case, func, or_, select from sqlalchemy.orm import Session, aliased from app.models import PlexItem, PlexState +# Sort sentinel for a rail row whose timestamp is missing, so `key=lambda r: r[0] or NO_TS` reads the +# same at every rail that merges two type-homogeneous lists in Python. +# +# It is NOT a crash fix, whatever the E24 commit that introduced it says. The `(ts is not None, ts)` +# key it replaced cannot raise: tuple comparison finds the first UNEQUAL element via `==`, so two +# `(False, None)` keys compare equal and `None < None` is never reached — verified by execution +# during the E24 epic-end review, over every permutation of null/non-null rows. What the sentinel +# buys is one idiom instead of two, and a key that says "missing sorts last" outright rather than +# encoding it in a boolean flag. A BARE `r[0]` would genuinely raise, which is the mutation the +# rails' tests do guard against. +NO_TS = datetime.min.replace(tzinfo=timezone.utc) + def show_status(total: int, watched: int, inprog: int) -> str: """Roll a show's episodes up into a single watch state: fully watched, partially started, or new.""" diff --git a/backend/app/routes/discover.py b/backend/app/routes/discover.py index fa516c62..7493359a 100644 --- a/backend/app/routes/discover.py +++ b/backend/app/routes/discover.py @@ -29,7 +29,9 @@ router = APIRouter(prefix="/api/discover", tags=["discover"]) class PinIn(BaseModel): - kind: Literal["video", "channel", "plex_item", "plex_show", "playlist"] + # From `pins.PIN_KINDS`, not a second hand-written list: the vocabulary, the schema columns and + # this request contract have to move together, and re-typing them here is how they would not. + kind: Literal[*pins.PIN_KINDS] # The id the rest of the API speaks for that kind: a video id, a channel id, a Plex rating_key, # or a playlist id. `pins.resolve_ref` does the translation to the row's internal key. ref: str diff --git a/backend/app/routes/plex/discover.py b/backend/app/routes/plex/discover.py index 5e8816a7..3bf13054 100644 --- a/backend/app/routes/plex/discover.py +++ b/backend/app/routes/plex/discover.py @@ -14,8 +14,6 @@ grid uses, applied to each rail's movie + show query (a rail may legitimately co the client hides). Genre-language matches the grid; duration is movie-only. """ -from datetime import datetime, timezone - from fastapi import APIRouter, Depends, Query from sqlalchemy import and_, func, or_, select from sqlalchemy.orm import Session @@ -43,12 +41,6 @@ RAIL_LIMIT = 20 GENRE_TILES = 12 SURPRISE_PER_KIND = 3 # Surprise-me: at most this many movies + this many shows (6 total) -# Sort sentinel for a row with no timestamp. The `(ts is not None, ts)` key this replaced LOOKS -# NULL-safe and is not: two null-timestamped rows compare equal on the flag and then compare -# `None < None`, which raises — so a show whose rollup has no `resumed_at`, or a movie state written -# before `progress_updated_at` existed, could 500 a rail rather than sort last (E24). -_NO_TS = datetime.min.replace(tzinfo=timezone.utc) - def _scope_libs(db: Session, scope: str) -> tuple[list[int], list[int]]: """The movie/show library ids visible for this scope (empty list = out of scope).""" @@ -121,10 +113,11 @@ def _recently_added( else [] ) # Merge the two type-homogeneous lists by added_at, then serialize each side in one batch so the - # per-card state lookups stay batched (no N+1). The `(is not None, value)` key sorts a missing - # timestamp last under reverse. + # per-card state lookups stay batched (no N+1). Same `or NO_TS` key as the two rails below — the + # E24 review found this one still on the older `(is not None, value)` form, i.e. one rule in two + # spellings, with the two comments explaining it differently. tagged = [(m.added_at, "movie", m) for m in movies] + [(s.added_at, "show", s) for s in shows] - tagged.sort(key=lambda t: (t[0] is not None, t[0]), reverse=True) + tagged.sort(key=lambda t: t[0] or rollup.NO_TS, reverse=True) top = tagged[:RAIL_LIMIT] # Key the serialized cards back by rating_key (globally unique in Plex — a movie and a show never # share one), so the merge doesn't depend on object identity staying alive. @@ -179,7 +172,7 @@ def _recently_watched( ) for ep, st, sh_title, sh_rk in db.execute(eq).all(): rows.append((st.watched_at, leaf_card(ep, st, show_title=sh_title, show_rk=sh_rk))) - rows.sort(key=lambda r: r[0] or _NO_TS, reverse=True) + rows.sort(key=lambda r: r[0] or rollup.NO_TS, reverse=True) return [card for _, card in rows[:RAIL_LIMIT]] @@ -243,7 +236,7 @@ def _continue_watching( for sh, resumed_at in srows: rows.append((resumed_at, show_card(sh, "in_progress"))) - rows.sort(key=lambda r: r[0] or _NO_TS, reverse=True) + rows.sort(key=lambda r: r[0] or rollup.NO_TS, reverse=True) return [card for _, card in rows[:RAIL_LIMIT]] diff --git a/backend/tests/test_discover.py b/backend/tests/test_discover.py index 4469bf2c..0ae2d56e 100644 --- a/backend/tests/test_discover.py +++ b/backend/tests/test_discover.py @@ -236,10 +236,14 @@ def test_the_resume_rail_obeys_the_channel_blocklist(db): assert hero.pick(db, user, "any", 0) is None -def test_a_null_timestamp_sorts_last_instead_of_raising(db): - """The `(ts is not None, ts)` key this rail's Plex sibling used compares `None < None` when two - rows both lack a timestamp, which raises. A missing progress timestamp must sort last, not 500 - the landing page.""" +def test_a_null_timestamp_sorts_last(db): + """A missing progress timestamp sorts last rather than 500ing the landing page. + + Narrower than it was written to be, and the epic-end review corrected it: the previous key, + `(ts is not None, ts)`, does NOT raise on two nulls — tuple comparison finds the first UNEQUAL + element via `==`, so `None < None` is never reached, and this test passes under that key too. + What it does discriminate is the naive `key=lambda r: r[0]`, which really does raise. See + `plex.rollup.NO_TS` for the corrected rationale.""" user, _ = _seed_yt(db) db.add_all( [ @@ -539,6 +543,45 @@ def test_a_pinned_video_survives_leaving_the_feed_scope(db): assert card["next"]["id"] == "vmed" +def test_the_plex_surprise_obeys_the_time_budget(db): + """The one control on the page has to mean what it says: a 100-minute film is not an answer to + "10 perc". The YouTube half of this rule has had its own test since S1; the Plex half's duration + bounds could be deleted with the whole suite still green, because every test that reached + `plex_surprise` passed budget="any".""" + user, _ = _seed_yt(db) + _seed_plex(db, user) + _enable_plex(db) + drawn = lambda budget: { # noqa: E731 + (c["source"], c["id"]) for s in range(6) for c in rails.surprise(db, user, budget, s, 5) + } + assert ("plex", "pm1") not in drawn("short") + # …and it IS offered where it fits, so this cannot pass by the rail being empty. + assert ("plex", "pm1") in drawn("long") + + +def test_the_plex_surprise_never_offers_something_already_started(db): + """A half-watched film is not a surprise, and the resume tier has already had its chance at it. + The mirror of `test_surprise_never_offers_something_already_started` for the other source — the + asymmetric half of one principle, which the house rules say to fix everywhere.""" + user, _ = _seed_yt(db) + movie, _, _ = _seed_plex(db, user) + _enable_plex(db) + db.add( + PlexState( + user_id=user.id, + item_id=movie.id, + status="new", + position_seconds=600, + progress_updated_at=NOW, + ) + ) + db.commit() + drawn = { + (c["source"], c["id"]) for s in range(6) for c in rails.surprise(db, user, "long", s, 5) + } + assert ("plex", "pm1") not in drawn + + def test_a_new_pin_lands_at_the_end_even_after_a_hole(db): """Positions are never renumbered on removal — `remove_pin` deletes the row, and every FK cascade silently removes a pin when its target disappears — so the append position has to come from @@ -603,11 +646,46 @@ def test_the_resume_rail_says_what_would_actually_play(db): def test_only_a_show_row_pays_for_a_resolution(db): """`next` is the answer to "a show has no Play button". Attaching it to a movie or a video would - both cost a query for nothing and re-open the self-reference cycle the shelf already hit.""" + both cost a query for nothing and re-open the self-reference cycle the shelf already hit. + + The states are what make this test exist: without them every rail query inner-joins its way to + zero rows, the loop body never runs, and the assertion is vacuous — which is exactly how the + epic-end review found it.""" user, _ = _seed_yt(db) - _seed_plex(db, user) + movie, _, ep1 = _seed_plex(db, user) _enable_plex(db) - for card in rails.continue_watching(db, user): + db.add_all( + [ + VideoState( + user_id=user.id, + video_id="vmed", + status="new", + position_seconds=300, + progress_updated_at=NOW, + ), + PlexState( + user_id=user.id, + item_id=movie.id, + status="new", + position_seconds=600, + progress_updated_at=NOW, + ), + PlexState( + user_id=user.id, + item_id=ep1.id, + status="new", + position_seconds=120, + progress_updated_at=NOW, + ), + ] + ) + db.commit() + + rail = rails.continue_watching(db, user) + # A video, a movie and a show — one row of each kind that can appear here, so the assertion below + # actually exercises both sides of the equality. + assert {c["kind"] for c in rail} == {"video", "movie", "show"} + for card in rail: assert ("next" in card) == (card["kind"] == "show"), card["id"] @@ -732,6 +810,63 @@ def test_pinning_twice_is_the_same_shelf(db, as_user): assert len(c.get("/api/discover/pins").json()["items"]) == 1 +def test_pinning_a_plex_show_resolves_to_the_episode_that_would_play(db, as_user): + """Two of the five pinnable kinds are Plex, and the rating_key → internal-id translation is + theirs alone — a wrong column or a wrong model in either arm pins the wrong row, or 404s every + Plex pin. Neither arm had a test before the epic-end review.""" + user, _ = _seed_yt(db) + _, _, ep1 = _seed_plex(db, user) + _enable_plex(db) + db.add(PlexState(user_id=user.id, item_id=ep1.id, status="watched", watched_at=NOW)) + db.commit() + c = as_user(user) + assert c.post("/api/discover/pins", json={"kind": "plex_show", "ref": "ps1"}).status_code == 200 + (tile,) = c.get("/api/discover/pins").json()["items"] + assert (tile["kind"], tile["id"]) == ("show", "ps1") + assert tile["next"]["id"] == "ps1e2" # ep1 is finished + + +def test_pinning_a_plex_movie_plays_itself_without_a_cycle(db, as_user): + """The self-reference that once 500'd the whole shelf, on the other leaf kind: `next` has to be a + distinct dict, not the tile aliased into itself. Covered for a video since S1, never for a movie.""" + user, _ = _seed_yt(db) + _seed_plex(db, user) + _enable_plex(db) + c = as_user(user) + assert c.post("/api/discover/pins", json={"kind": "plex_item", "ref": "pm1"}).status_code == 200 + (tile,) = c.get("/api/discover/pins").json()["items"] + assert tile["id"] == "pm1" and tile["next"]["id"] == "pm1" + + +def test_the_reroll_endpoint_parses_its_whole_exclusion_list_and_its_press_count(db, as_user): + """The reroll's behaviour is covered at the domain level; the ROUTE's query-string translation — + the comma split, the cap, the `start` passthrough — was not covered at all, so a change there + could hand back the same card on every press with the suite green. Excluding all three candidates + is what proves the whole list parsed: a delimiter change leaves one unsplit string in the set, + and a hero comes back.""" + user, _ = _seed_yt(db) + db.add( + VideoState( + user_id=user.id, + video_id="vmed", + status="new", + position_seconds=300, + progress_updated_at=NOW, + ) + ) + db.add(ComfortPin(user_id=user.id, video_id="vlong", position=0)) + db.commit() + c = as_user(user) + + # `start` rotates which tier answers first: 0 leads with resume, 1 with the shelf. + assert c.get("/api/discover/pick?seed=1&start=0").json()["hero"]["tier"] == "resume" + assert c.get("/api/discover/pick?seed=1&start=1").json()["hero"]["tier"] == "comfort" + + skip = ",".join(f"youtube:{v}" for v in ("vshort", "vmed", "vlong")) + assert c.get(f"/api/discover/pick?seed=2&exclude={skip}").json()["hero"] is None + assert c.get("/api/discover/pick?budget=whenever").status_code == 422 + + def test_pinning_something_that_does_not_exist_is_a_404(db, as_user): user, _ = _seed_yt(db) r = as_user(user).post("/api/discover/pins", json={"kind": "video", "ref": "nope"}) diff --git a/frontend/src/components/Home.dom.test.tsx b/frontend/src/components/Home.dom.test.tsx index f444637d..00c41e09 100644 --- a/frontend/src/components/Home.dom.test.tsx +++ b/frontend/src/components/Home.dom.test.tsx @@ -443,6 +443,31 @@ describe("Home", () => { expect(setPage).toHaveBeenCalledWith("plex"); }); + it("sends the ⓘ affordance to the item's page, not to the player", async () => { + // `onInfo` and `onPlay` differ by one ternary arm, and confusing them is the defect the play + // test above was written for — but only `onPlay` was pinned, so swapping ⓘ's "movie" for "play" + // typechecked (both are members of `PlexJumpTarget["type"]`) and shipped green. ⓘ is the one + // action on this surface a user presses precisely because they do NOT want to start watching. + const movie = { + ...ytCard("m1", "A Film"), + source: "plex", + kind: "movie", + aspect: "poster", + video: null, + plex: { id: "m1", type: "movie", title: "A Film", thumb: "" }, + }; + discover.mockResolvedValue({ + budget: "any", + hero: null, + continue_watching: [movie], + comfort: [], + }); + mount(); + fireEvent.click(await screen.findByLabelText("plex.info.title")); + expect(setJumpTo).toHaveBeenCalledWith({ id: "m1", type: "movie", replace: true }); + expect(setPage).toHaveBeenCalledWith("plex"); + }); + it("rotates the tier on each press so the button offers a different KIND of answer", async () => { // Measured 2026-09-06: exclusion alone left it walking the resume rail and cycling nine cards, // never once reaching the shelf. The press count travels to the server as the rotation. @@ -462,7 +487,11 @@ describe("Home", () => { expect(discoverPick).toHaveBeenLastCalledWith( "any", expect.any(Number), - expect.any(Array), + // The exact list, not `expect.any(Array)`: the window has to ACCUMULATE. A one-element list + // is identical whether it accumulates or replaces, so the first press cannot tell them + // apart — and with the loose matcher both "replace, never grow" and SEEN_WINDOW = 1 (which + // reinstate the measured nine-card cycle) left the whole suite green. + ["youtube:v1", "youtube:v9"], 2, ), );