fix(e24): the review's three backend defects — blocklist, shelf order, next episode
The resume rail built its own SELECT instead of going through `filtered_query` and copied only three of its predicates, so it dropped the per-user channel blocklist the module docstring claims it inherits. Blocking a channel never touches `video_states`, so a half-watched video from it kept its resume position and headed the rail — and, at the default budget with the resume tier leading, became the landing page's hero: the app answering "what should I watch" with the thing the user had just banned. The clause is now `feedquery.not_blocked_cond`, shared with `filtered_query` rather than copied a second time; the feed's SCOPE clauses stay deliberately unapplied, for the reason `yt_video_row` already gives. `next_position` returned the shelf's row COUNT while nothing ever renumbers positions — `remove_pin` deletes, and every FK cascade removes a pin when its target disappears. Past the first hole the count under-counts, so a new pin landed in the middle of the shelf and, past the second, on a position another pin already held. `max(position) + 1`, the idiom `routes/playlists` already uses. `next_episode` took the FIRST in-progress episode in broadcast order while `plex.rollup`, which drives the show page's Resume button, takes the last. One show therefore had two different "continue watching" answers, with the Home tile ranking it by its most recent touch and then offering the episode abandoned weeks earlier. The new test asserts the two implementations against each other rather than against a literal. All three mutation-checked: each new test goes red for the defect it names.
This commit is contained in:
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user