diff --git a/backend/app/discover/pins.py b/backend/app/discover/pins.py index 3152cc29..b4be5f85 100644 --- a/backend/app/discover/pins.py +++ b/backend/app/discover/pins.py @@ -16,7 +16,7 @@ Domain module, so nothing here raises HTTP. `resolve_ref` returns None for "no s yours" and the route turns that into its 404 — core never imports routes. """ -from sqlalchemy import select +from sqlalchemy import func, select from sqlalchemy.orm import Session from app.discover import cards, queries @@ -78,8 +78,21 @@ def list_pins(db: Session, user: User) -> list[ComfortPin]: def next_position(db: Session, user: User) -> int: - """New pins land at the end of the shelf.""" - return len(list_pins(db, user)) + """New pins land at the end of the shelf. + + `max(position) + 1`, not the row count. Positions are never renumbered on removal — `remove_pin` + deletes the row, and every FK cascade silently removes a pin when the thing it points at goes + away — so past the first hole a count under-counts and would drop the new pin into the MIDDLE of + the shelf, or onto a position another pin already holds (nothing in the schema forbids that). + Same idiom as `routes/playlists._next_playlist_position`.""" + return ( + db.scalar( + select(func.coalesce(func.max(ComfortPin.position), -1)).where( + ComfortPin.user_id == user.id + ) + ) + + 1 + ) def _shelf_card(db: Session, user: User, pin: ComfortPin) -> dict | None: diff --git a/backend/app/discover/queries.py b/backend/app/discover/queries.py index 8e9b2341..403bd8c9 100644 --- a/backend/app/discover/queries.py +++ b/backend/app/discover/queries.py @@ -21,7 +21,14 @@ from sqlalchemy import and_, func, or_, select from sqlalchemy.orm import Session, aliased from app.discover.budget import band -from app.feedquery import feed_columns, filtered_query, order_by, serialize, sort_keys +from app.feedquery import ( + feed_columns, + filtered_query, + not_blocked_cond, + order_by, + serialize, + sort_keys, +) from app.models import ( Channel, PlaylistItem, @@ -78,7 +85,14 @@ def yt_in_progress(db: Session, user: User, limit: int = RAIL_LIMIT) -> list[tup watched/hidden) — but the query is built here rather than through `filtered_query` because this rail wants an ordering by `progress_updated_at`, and that column lives on the state alias the feed query keeps private. Driving from `video_states` is also the faster plan: it is a per-user table of - a few hundred rows, against a catalog of many thousands.""" + a few hundred rows, against a catalog of many thousands. + + Building it here means the feed's OTHER predicates have to be chosen deliberately rather than + inherited. `not_blocked_cond` is shared, because "never show me this channel" outranks "you + started it" — blocking leaves the state row untouched, so without this the video the user just + banned heads the resume rail. The feed's SCOPE clauses (subscribed / via_explore accessibility) + are deliberately NOT applied, for the reason `yt_video_row` gives: something you have already + started must stay resumable even after you unsubscribe or an exploration expires.""" st = aliased(VideoState) q = ( select(*feed_columns(user, st), st.progress_updated_at.label("progressed_at")) @@ -88,6 +102,7 @@ def yt_in_progress(db: Session, user: User, limit: int = RAIL_LIMIT) -> list[tup st.position_seconds > 0, st.status.notin_(("watched", "hidden")), Video.unavailable_since.is_(None), + not_blocked_cond(user), ) .order_by(st.progress_updated_at.desc().nullslast()) .limit(limit) @@ -281,7 +296,7 @@ _DONE = ("watched", "hidden") def next_episode(db: Session, uid: int, show_id: int) -> dict | None: """What "put that show on" means: the episode in progress, else the first unwatched one, else the - first — in broadcast order, the same `(season, episode)` ordering `detail.py::_episode_nav` uses. + first — over the same `(season, episode)` broadcast ordering `detail.py::_episode_nav` uses. Three ordered `LIMIT 1` probes rather than one scan picked apart in Python. The Python version loaded EVERY episode of the show to choose one of them — 236 rows for Friends, 227 for JAG — which @@ -289,19 +304,28 @@ def next_episode(db: Session, uid: int, show_id: int) -> dict | None: could not, since it resolves a show per rail row on the landing page. Returned as a `leaf_card` so it carries the show title the episode row itself does not.""" + asc = ( + PlexItem.season_number.asc().nullsfirst(), + PlexItem.episode_number.asc().nullsfirst(), + ) base = ( select(PlexItem, PlexState) .outerjoin(PlexState, and_(PlexState.item_id == PlexItem.id, PlexState.user_id == uid)) .where(PlexItem.show_id == show_id, PlexItem.kind == "episode") - .order_by( - PlexItem.season_number.asc().nullsfirst(), - PlexItem.episode_number.asc().nullsfirst(), - ) .limit(1) ) - # In progress. `position_seconds > 0` already requires the state row to exist, so no null guard. + # In progress — the LAST one in broadcast order, which is `plex.rollup.rollup`'s `inprog[-1]`, + # i.e. what the show page's own Resume button plays. Two episodes of one show are in progress at + # once more often than it sounds (`watch_sync._reconcile_inprogress_movies` documents that a + # scrub to the end leaves a stale `status='new', position>0` row until the daily reconcile), and + # taking opposite ends of that set is how one show came to have two different "continue" answers: + # the Home tile ranked it by its LATEST touch and then offered the EARLIEST started episode. + # `position_seconds > 0` already requires the state row to exist, so no null guard. pick = db.execute( - base.where(PlexState.position_seconds > 0, PlexState.status.notin_(_DONE)) + base.where(PlexState.position_seconds > 0, PlexState.status.notin_(_DONE)).order_by( + PlexItem.season_number.desc().nullslast(), + PlexItem.episode_number.desc().nullslast(), + ) ).first() # Unwatched — INCLUDING an episode with no state row at all, which is the common case and the one # a bare `status NOT IN (...)` silently drops (NULL is not false, it is unknown, so the row fails @@ -309,11 +333,11 @@ def next_episode(db: Session, uid: int, show_id: int) -> dict | None: pick = ( pick or db.execute( - base.where(or_(PlexState.id.is_(None), PlexState.status.notin_(_DONE))) + base.where(or_(PlexState.id.is_(None), PlexState.status.notin_(_DONE))).order_by(*asc) ).first() ) # Fully watched show: start it again from the top. - pick = pick or db.execute(base).first() + pick = pick or db.execute(base.order_by(*asc)).first() if pick is None: return None diff --git a/backend/app/feedquery.py b/backend/app/feedquery.py index 80e72d3b..35cd8cfd 100644 --- a/backend/app/feedquery.py +++ b/backend/app/feedquery.py @@ -117,6 +117,19 @@ def tags_by_category(db: Session, tag_ids: list[int]) -> dict[str, list[int]]: return out +def not_blocked_cond(user: User): + """Per-user channel blocklist: never show this user videos from a channel they blocked + (applies to feed, Library, explore, the channel page and the Home resume rail alike). + + A shared expression rather than a clause each surface writes for itself. Blocking does not touch + `video_states`, so a half-watched video from a newly blocked channel survives with its resume + position — and the one query that copied only PART of this predicate set is exactly how that + video came back as the landing page's suggestion after the user had said "never again".""" + return Video.channel_id.notin_( + select(BlockedChannel.channel_id).where(BlockedChannel.user_id == user.id) + ) + + def filtered_query( db: Session, user: User, @@ -207,13 +220,7 @@ def filtered_query( ) ) - # Per-user channel blocklist: never show this user videos from a channel they blocked - # (applies to feed, Library, explore, and the channel page alike). - query = query.where( - Video.channel_id.notin_( - select(BlockedChannel.channel_id).where(BlockedChannel.user_id == user.id) - ) - ) + query = query.where(not_blocked_cond(user)) # Source filter. In the shared Library ("all" scope) it uses the GLOBAL via_search flag # (search-discovered by anyone). In "my" scope it's the user's OWN provenance: diff --git a/backend/tests/test_discover.py b/backend/tests/test_discover.py index 1c83efb2..4469bf2c 100644 --- a/backend/tests/test_discover.py +++ b/backend/tests/test_discover.py @@ -15,7 +15,10 @@ from sqlalchemy.exc import IntegrityError from app.discover import hero, pins, queries, rails from app.discover.budget import BANDS, fits, remaining_seconds +from app.plex import rollup +from app.plex.cards import leaf_card from app.models import ( + BlockedChannel, Channel, ComfortPin, PlexItem, @@ -208,6 +211,31 @@ def test_continue_watching_is_youtube_only_when_plex_is_off(db): assert {c["source"] for c in rails.continue_watching(db, user)} == {"youtube"} +def test_the_resume_rail_obeys_the_channel_blocklist(db): + """Blocking a channel does not touch `video_states`, so a half-watched video from it survives + with its resume position — and this rail builds its own SELECT instead of going through + `filtered_query`. "Never show me this channel" has to outrank "you started it", or the landing + page answers "what should I watch" with the thing the user has just banned.""" + user, ch = _seed_yt(db) + db.add( + VideoState( + user_id=user.id, + video_id="vmed", + status="new", + position_seconds=300, + progress_updated_at=NOW, + ) + ) + db.commit() + assert [c["id"] for c in rails.continue_watching(db, user)] == ["vmed"] + + db.add(BlockedChannel(user_id=user.id, channel_id=ch.id)) + db.commit() + assert rails.continue_watching(db, user) == [] + # And so the hero, whose resume tier draws from that rail, has nothing to offer either. + 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 @@ -511,6 +539,32 @@ def test_a_pinned_video_survives_leaving_the_feed_scope(db): assert card["next"]["id"] == "vmed" +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 + `max(position)`, not from the row count. With a count, the first hole drops the next pin into the + MIDDLE of the shelf and the second makes two pins claim one slot.""" + user, ch = _seed_yt(db) + db.add_all( + [ + ComfortPin(user_id=user.id, video_id="vshort", position=0), + ComfortPin(user_id=user.id, video_id="vmed", position=1), + ComfortPin(user_id=user.id, video_id="vlong", position=2), + ] + ) + db.commit() + db.execute(delete(ComfortPin).where(ComfortPin.video_id.in_(("vshort", "vmed")))) + db.commit() + + db.add(ComfortPin(user_id=user.id, channel_id=ch.id, position=pins.next_position(db, user))) + db.commit() + assert [c["kind"] for c in pins.shelf(db, user)] == ["video", "channel"] + positions = ( + db.execute(select(ComfortPin.position).order_by(ComfortPin.position)).scalars().all() + ) + assert len(set(positions)) == len(positions) # and no two tiles claim the same slot + + def test_a_self_playing_pin_does_not_reference_itself(db): """`next` must be a distinct dict even when the pin plays itself. Aliasing the card into its own `next` is a cycle, and the serializer turns the whole shelf into a 500 over it.""" @@ -583,6 +637,46 @@ def test_a_finished_show_starts_again_from_the_top(db): assert queries.next_episode(db, user.id, show.id)["id"] == "ps1e1" +def test_next_episode_agrees_with_the_show_page_when_two_are_in_progress(db): + """One show, one answer. `plex.rollup.rollup` — which drives the show page's Resume button — + takes the LAST in-progress episode in broadcast order; taking the FIRST here gave the same show + two different "continue" answers, with the rail ranking it by its most recent touch and then + offering the episode abandoned weeks earlier. Two in-progress episodes are not exotic: a scrub to + the end leaves a stale `status='new', position>0` row until the daily reconcile.""" + user, _ = _seed_yt(db) + _, show, ep1 = _seed_plex(db, user) + ep2 = db.execute(select(PlexItem).where(PlexItem.rating_key == "ps1e2")).scalar_one() + db.add_all( + [ + PlexState( + user_id=user.id, + item_id=ep1.id, + status="new", + position_seconds=300, + progress_updated_at=NOW - timedelta(days=9), + ), + PlexState( + user_id=user.id, + item_id=ep2.id, + status="new", + position_seconds=600, + progress_updated_at=NOW, + ), + ] + ) + db.commit() + + got = queries.next_episode(db, user.id, show.id) + assert got["id"] == "ps1e2" + # Asserted against the other implementation rather than against a literal, so the two cannot + # drift apart again without this failing. + ordered = [ + leaf_card(ep, db.execute(select(PlexState).where(PlexState.item_id == ep.id)).scalar_one()) + for ep in (ep1, ep2) + ] + assert rollup.rollup(ordered)["resume"]["id"] == got["id"] + + def test_next_episode_prefers_the_one_in_progress(db): """In progress beats "next unwatched": resuming is cheaper than starting.""" user, _ = _seed_yt(db)