fix(api): accept fractional progress seconds (R6 S1b code-review round 1)
/code-review high round 1 on S1b — 5 findings, 1 fixed, 4 by-design/deferred: - FIXED: VideoProgressIn / ItemProgressIn position_seconds & duration_seconds typed `int` would 422 a raw float currentTime on the 10s progress checkpoint (the old `int(payload.get(...))` truncated it). Back to `float`, truncated in the handler; +test. The live frontend floors so this was latent, not live. By-design / deferred (no change): the 422 detail-array isn't localized by the FE (filed under R7); PATCH is-not-none (channels/quota, null-safe) vs model_fields_set (nullable-clear) is a deliberate split; missing-field→422 is the point of the epic; me/switch_account's numeric-string coercion is more correct and still session-gated. Gate: ruff clean, pytest 172 green.
This commit is contained in:
@@ -659,8 +659,11 @@ def clear_video_state(
|
|||||||
|
|
||||||
|
|
||||||
class VideoProgressIn(BaseModel):
|
class VideoProgressIn(BaseModel):
|
||||||
position_seconds: int = 0
|
# Accept a float and truncate (below) — the player may post a raw currentTime (a float), which
|
||||||
duration_seconds: int = 0
|
# the old `int(payload.get(...))` silently truncated. A plain `int` field would 422 that on the
|
||||||
|
# 10s progress hot path, losing the resume position; `float` keeps the old tolerance.
|
||||||
|
position_seconds: float = 0
|
||||||
|
duration_seconds: float = 0
|
||||||
|
|
||||||
|
|
||||||
@router.post("/videos/{video_id}/progress")
|
@router.post("/videos/{video_id}/progress")
|
||||||
@@ -674,8 +677,8 @@ def set_video_progress(
|
|||||||
Stores a per-user position on the video_states row without touching watch status, so a
|
Stores a per-user position on the video_states row without touching watch status, so a
|
||||||
partially-watched video can render a progress bar and match the "in progress" filter.
|
partially-watched video can render a progress bar and match the "in progress" filter.
|
||||||
Trivially-early and near-finished positions clear the position rather than store it."""
|
Trivially-early and near-finished positions clear the position rather than store it."""
|
||||||
position = body.position_seconds
|
position = int(body.position_seconds)
|
||||||
duration = body.duration_seconds
|
duration = int(body.duration_seconds)
|
||||||
if position < 0:
|
if position < 0:
|
||||||
position = 0
|
position = 0
|
||||||
|
|
||||||
|
|||||||
@@ -1943,8 +1943,10 @@ def _push_watch(
|
|||||||
|
|
||||||
|
|
||||||
class ItemProgressIn(BaseModel):
|
class ItemProgressIn(BaseModel):
|
||||||
position_seconds: int = 0
|
# float, truncated in the handler — the player may post a raw currentTime (a float); a plain
|
||||||
duration_seconds: int = 0
|
# `int` field would 422 it on the periodic progress checkpoint (the old `int(...)` truncated).
|
||||||
|
position_seconds: float = 0
|
||||||
|
duration_seconds: float = 0
|
||||||
final: bool = False
|
final: bool = False
|
||||||
|
|
||||||
|
|
||||||
@@ -1962,8 +1964,8 @@ def item_progress(
|
|||||||
debounces the Plex resume push: we mirror the settled position to Plex only on a final flush, so
|
debounces the Plex resume push: we mirror the settled position to Plex only on a final flush, so
|
||||||
a single watch doesn't spray `/:/timeline` calls at the server every 10 seconds."""
|
a single watch doesn't spray `/:/timeline` calls at the server every 10 seconds."""
|
||||||
it = _item_or_404(db, rating_key)
|
it = _item_or_404(db, rating_key)
|
||||||
position = max(0, body.position_seconds)
|
position = max(0, int(body.position_seconds))
|
||||||
duration = body.duration_seconds
|
duration = int(body.duration_seconds)
|
||||||
final = body.final
|
final = body.final
|
||||||
now = datetime.now(timezone.utc)
|
now = datetime.now(timezone.utc)
|
||||||
st = db.query(PlexState).filter_by(user_id=user.id, item_id=it.id).first()
|
st = db.query(PlexState).filter_by(user_id=user.id, item_id=it.id).first()
|
||||||
|
|||||||
@@ -6,6 +6,7 @@ from pydantic import ValidationError
|
|||||||
|
|
||||||
from app.routes.channels import AttachTagIn, ChannelUpdateIn
|
from app.routes.channels import AttachTagIn, ChannelUpdateIn
|
||||||
from app.routes.downloads import AdminSetQuotaIn
|
from app.routes.downloads import AdminSetQuotaIn
|
||||||
|
from app.routes.feed import VideoProgressIn
|
||||||
from app.routes.setup import SetupAdminIn
|
from app.routes.setup import SetupAdminIn
|
||||||
from app.routes.tags import TagUpdateIn
|
from app.routes.tags import TagUpdateIn
|
||||||
|
|
||||||
@@ -46,6 +47,15 @@ def test_admin_quota_rejects_non_numeric_limit():
|
|||||||
AdminSetQuotaIn(max_bytes="lots")
|
AdminSetQuotaIn(max_bytes="lots")
|
||||||
|
|
||||||
|
|
||||||
|
def test_progress_accepts_fractional_seconds():
|
||||||
|
# The player may post a raw currentTime (a float). The field must accept it (truncated in the
|
||||||
|
# handler) — a plain `int` field would 422 on the progress hot path. (code-review round 1.)
|
||||||
|
assert VideoProgressIn(position_seconds=12.7).position_seconds == 12.7
|
||||||
|
assert int(VideoProgressIn(position_seconds=12.7).position_seconds) == 12
|
||||||
|
with pytest.raises(ValidationError):
|
||||||
|
VideoProgressIn(position_seconds="not-a-number")
|
||||||
|
|
||||||
|
|
||||||
# --- PATCH models: model_fields_set separates "absent" from "explicit null". ---
|
# --- PATCH models: model_fields_set separates "absent" from "explicit null". ---
|
||||||
|
|
||||||
def test_tag_update_absent_vs_null_color():
|
def test_tag_update_absent_vs_null_color():
|
||||||
|
|||||||
Reference in New Issue
Block a user