diff --git a/backend/app/routes/playlists.py b/backend/app/routes/playlists.py index dfae5af9..3a1bcd12 100644 --- a/backend/app/routes/playlists.py +++ b/backend/app/routes/playlists.py @@ -3,6 +3,8 @@ CRUD (create / rename / delete, add / remove / reorder items) plus YouTube two-w mirror the user's own YouTube playlists in (`/sync-youtube`) and push/revert local edits back out (`/{id}/push-plan`, `/{id}/revert-youtube`), behind the optional write scope.""" +from datetime import datetime, timezone + from fastapi import APIRouter, Depends, HTTPException from pydantic import BaseModel from sqlalchemy import func, select, and_ @@ -52,6 +54,16 @@ def _own_playlist(db: Session, user: User, playlist_id: int) -> Playlist: return pl +def _touch(pl: Playlist) -> None: + """Bump the playlist's `updated_at` for an edit that only writes `playlist_items`. + + `UpdatedAtMixin`'s `onupdate` fires on an UPDATE of the PLAYLIST row, and add / remove / + reorder / move write the item rows instead — so without this the rail's "recently changed" + sort would only ever see renames. The assignment is what makes the column mean "last time + this list changed", which is what the rail promises the reader.""" + pl.updated_at = datetime.now(timezone.utc) + + def _next_playlist_position(db: Session, user_id: int) -> int: """Append position for a new playlist in this user's rail.""" return ( @@ -91,6 +103,7 @@ def _add_item(db: Session, pl: Playlist, video_id: str, *, mark_dirty: bool) -> ) if mark_dirty: recompute_dirty(db, pl) + _touch(pl) db.commit() return True @@ -108,6 +121,7 @@ def _remove_item(db: Session, pl: Playlist, video_id: str, *, mark_dirty: bool) db.delete(item) if mark_dirty: recompute_dirty(db, pl) + _touch(pl) db.commit() return True @@ -129,6 +143,9 @@ def _summary( "item_count": count, "total_duration_seconds": total_duration, "cover_thumbnail": cover, + # When this list last CHANGED (rename, add/remove/move/reorder) — the rail's + # "recently changed first" sort reads it. See _touch for why it isn't automatic. + "updated_at": pl.updated_at, } if has_video is not None: out["has_video"] = has_video @@ -520,6 +537,63 @@ def remove_item( return {"playlist_id": pl.id, "video_id": video_id} +class MoveItemIn(BaseModel): + target_playlist_id: int + + +@router.post("/{playlist_id}/items/{video_id}/move") +def move_item( + playlist_id: int, + video_id: str, + body: MoveItemIn, + user: User = Depends(current_user), + db: Session = Depends(get_db), +) -> dict: + """Move one video from a playlist into another of the caller's playlists, atomically. + + ONE transaction on purpose. The obvious client-side shape — add to the target, then remove + from the source — leaves the video in BOTH lists whenever the second call fails, which is + exactly the kind of half-applied state this codebase treats as a bug rather than an edge + case. Idempotent: if the video is already in the target, the source row is simply dropped + (moving into a list that already has it is a removal, not a duplicate).""" + src = _own_playlist(db, user, playlist_id) + dst = _own_playlist(db, user, body.target_playlist_id) + if src.id == dst.id: + raise HTTPException( + status_code=400, detail={"code": "playlist_move", "reason": "same_playlist"} + ) + item = db.scalar( + select(PlaylistItem).where( + PlaylistItem.playlist_id == src.id, PlaylistItem.video_id == video_id + ) + ) + if item is None: + # A concurrent edit (another tab, or a YouTube re-pull) already took it out. + raise HTTPException( + status_code=404, detail={"code": "playlist_move", "reason": "not_in_playlist"} + ) + already = db.scalar( + select(PlaylistItem).where( + PlaylistItem.playlist_id == dst.id, PlaylistItem.video_id == video_id + ) + ) + if already is not None: + db.delete(item) # the uq_playlist_video constraint would reject a second row anyway + else: + # Re-parent the existing row rather than delete+insert: `added_at` ("when I saved this") + # survives the move, and there is no window where the video belongs to neither list. + item.playlist_id = dst.id + item.position = _next_item_position(db, dst.id) + # Both sides changed, so both get the dirty recompute and the timestamp — the asymmetric + # version of this (touching only the source) is the recurring smell CLAUDE.md warns about. + recompute_dirty(db, src) + recompute_dirty(db, dst) + _touch(src) + _touch(dst) + db.commit() + return {"ok": True, "from_playlist_id": src.id, "to_playlist_id": dst.id, "video_id": video_id} + + class ReorderItemsIn(BaseModel): video_ids: list[str] = [] @@ -555,5 +629,6 @@ def reorder_items( it.position = pos pos += 1 recompute_dirty(db, pl) + _touch(pl) db.commit() return {"playlist_id": pl.id, "count": pos} diff --git a/backend/tests/test_playlist_move.py b/backend/tests/test_playlist_move.py new file mode 100644 index 00000000..8590c2a8 --- /dev/null +++ b/backend/tests/test_playlist_move.py @@ -0,0 +1,190 @@ +"""E10 S2 — the one-step "move this video to another list" endpoint, and the `updated_at` touch +the rail's "recently changed" sort reads. + +Two properties are load-bearing and neither is visible from the happy path alone: + +1. **The move is one transaction.** The client-side shape it replaces (add to the target, then + remove from the source) leaves the video in BOTH lists when the second call fails. So the + tests assert the *pair* of lists after every call, never just the destination. +2. **`updated_at` tracks item edits.** `UpdatedAtMixin.onupdate` only fires on an UPDATE of the + playlist row, and add/remove/reorder/move write `playlist_items` — so the timestamp is bumped + explicitly, and a regression there would silently degrade the sort to "renames only". +""" + +from datetime import datetime, timedelta, timezone + +from app.models import Channel, Playlist, PlaylistItem, Video + + +def _seed_videos(db, ids): + db.add(Channel(id="UCtest", title="Test")) + now = datetime.now(timezone.utc) + for v in ids: + db.add(Video(id=v, channel_id="UCtest", published_at=now)) + db.commit() + + +def _playlist(db, user, name, video_ids, **kw): + pl = Playlist(user_id=user.id, name=name, **kw) + db.add(pl) + db.flush() + for i, vid in enumerate(video_ids): + db.add(PlaylistItem(playlist_id=pl.id, video_id=vid, position=i)) + db.commit() + return pl + + +def _ids(db, pl_id): + return [ + it.video_id + for it in db.execute( + PlaylistItem.__table__.select() + .where(PlaylistItem.playlist_id == pl_id) + .order_by(PlaylistItem.position) + ).all() + ] + + +def _move(client, src, video_id, dst): + return client.post( + f"/api/playlists/{src}/items/{video_id}/move", json={"target_playlist_id": dst} + ) + + +def test_move_leaves_the_video_in_exactly_one_list(db, make_user, as_user): + user = make_user() + _seed_videos(db, ["v1", "v2"]) + src = _playlist(db, user, "Source", ["v1", "v2"]) + dst = _playlist(db, user, "Target", []) + + r = _move(as_user(user), src.id, "v1", dst.id) + assert r.status_code == 200 + assert r.json() == { + "ok": True, + "from_playlist_id": src.id, + "to_playlist_id": dst.id, + "video_id": "v1", + } + db.expire_all() + assert _ids(db, src.id) == ["v2"] + assert _ids(db, dst.id) == ["v1"] + + +def test_move_appends_to_the_end_of_the_target(db, make_user, as_user): + user = make_user() + _seed_videos(db, ["v1", "v2", "v3"]) + src = _playlist(db, user, "Source", ["v3"]) + dst = _playlist(db, user, "Target", ["v1", "v2"]) + + assert _move(as_user(user), src.id, "v3", dst.id).status_code == 200 + db.expire_all() + assert _ids(db, dst.id) == ["v1", "v2", "v3"] + + +def test_move_into_a_list_that_already_has_it_is_a_removal_not_a_duplicate(db, make_user, as_user): + user = make_user() + _seed_videos(db, ["v1"]) + src = _playlist(db, user, "Source", ["v1"]) + dst = _playlist(db, user, "Target", ["v1"]) + + assert _move(as_user(user), src.id, "v1", dst.id).status_code == 200 + db.expire_all() + assert _ids(db, src.id) == [] + assert _ids(db, dst.id) == ["v1"] # exactly one row, not two + + +def test_move_to_the_same_list_is_refused(db, make_user, as_user): + user = make_user() + _seed_videos(db, ["v1"]) + src = _playlist(db, user, "Source", ["v1"]) + + r = _move(as_user(user), src.id, "v1", src.id) + assert r.status_code == 400 + assert r.json()["detail"] == {"code": "playlist_move", "reason": "same_playlist"} + db.expire_all() + assert _ids(db, src.id) == ["v1"] + + +def test_move_of_a_video_that_left_the_source_reports_it(db, make_user, as_user): + user = make_user() + _seed_videos(db, ["v1"]) + src = _playlist(db, user, "Source", []) + dst = _playlist(db, user, "Target", []) + + r = _move(as_user(user), src.id, "v1", dst.id) + assert r.status_code == 404 + assert r.json()["detail"] == {"code": "playlist_move", "reason": "not_in_playlist"} + + +def test_move_into_someone_elses_list_404s_and_changes_nothing(db, make_user, as_user): + user, other = make_user(), make_user() + _seed_videos(db, ["v1"]) + src = _playlist(db, user, "Mine", ["v1"]) + theirs = _playlist(db, other, "Theirs", []) + + assert _move(as_user(user), src.id, "v1", theirs.id).status_code == 404 + db.expire_all() + assert _ids(db, src.id) == ["v1"] + assert _ids(db, theirs.id) == [] + + +def test_move_marks_both_linked_sides_dirty(db, make_user, as_user): + """A move edits two YouTube mirrors at once — the source lost an item, the target gained one. + Marking only the source is the asymmetric fix this repo keeps re-learning.""" + user = make_user() + _seed_videos(db, ["v1"]) + src = _playlist(db, user, "Src", ["v1"], yt_playlist_id="PLsrc", synced_video_ids='["v1"]') + dst = _playlist(db, user, "Dst", [], yt_playlist_id="PLdst", synced_video_ids="[]") + # Baseline: both are in sync with YouTube before the move. + src.dirty = False + dst.dirty = False + db.commit() + + assert _move(as_user(user), src.id, "v1", dst.id).status_code == 200 + db.expire_all() + assert db.get(Playlist, src.id).dirty is True + assert db.get(Playlist, dst.id).dirty is True + + +def test_item_edits_bump_updated_at(db, make_user, as_user): + """The rail's "recently changed first" sort reads this. Adding, moving and reordering all + write `playlist_items` only, so nothing here is covered by the ORM's `onupdate`.""" + user = make_user() + _seed_videos(db, ["v1", "v2"]) + src = _playlist(db, user, "Source", ["v1", "v2"]) + dst = _playlist(db, user, "Target", []) + old = datetime.now(timezone.utc) - timedelta(days=3) + src.updated_at = old + dst.updated_at = old + db.commit() + client = as_user(user) + + assert client.post(f"/api/playlists/{dst.id}/items", json={"video_id": "v1"}).status_code == 200 + db.expire_all() + assert db.get(Playlist, dst.id).updated_at > old + + dst.updated_at = old + db.commit() + assert ( + client.put(f"/api/playlists/{src.id}/order", json={"video_ids": ["v2", "v1"]}).status_code + == 200 + ) + db.expire_all() + assert db.get(Playlist, src.id).updated_at > old + + src.updated_at = old + dst.updated_at = old + db.commit() + assert _move(client, src.id, "v2", dst.id).status_code == 200 + db.expire_all() + # BOTH sides of a move are "recently changed" — the target is where the reader will look. + assert db.get(Playlist, src.id).updated_at > old + assert db.get(Playlist, dst.id).updated_at > old + + +def test_the_list_endpoint_publishes_updated_at(db, make_user, as_user): + user = make_user() + _playlist(db, user, "Solo", []) + rows = as_user(user).get("/api/playlists").json() + assert len(rows) == 1 + assert rows[0]["updated_at"] # the rail cannot sort by a field the API never sends diff --git a/frontend/src/lib/api/core.ts b/frontend/src/lib/api/core.ts index 74b6db05..c507f67e 100644 --- a/frontend/src/lib/api/core.ts +++ b/frontend/src/lib/api/core.ts @@ -77,6 +77,11 @@ function localizeDetail(d: unknown): string { // from that address" — so an unknown reason falls back to the generic refusal rather than a key. if (o?.code === "url_blocked") return t(`downloads.errors.urlBlocked.${o.reason}`, t("downloads.errors.urlBlocked.generic")); + // E10 S2: the one-step "move to another list". Both reasons mean the same thing to a reader — + // the list moved under you — but they are different enough to be worth naming separately, and an + // unknown reason falls back to the plain failure rather than printing a key. + if (o?.code === "playlist_move") + return t(`playlists.moveErrors.${o.reason}`, t("playlists.moveFailed")); // SEC-14: a manual sync trigger on cooldown. Not an error the user caused so much as a pace // limit, and the copy says so. if (o?.code === "sync_cooldown") return t("errors.syncCooldown"); diff --git a/frontend/src/lib/api/playlists.ts b/frontend/src/lib/api/playlists.ts index 4a085532..85846b8e 100644 --- a/frontend/src/lib/api/playlists.ts +++ b/frontend/src/lib/api/playlists.ts @@ -18,6 +18,7 @@ export interface Playlist { item_count: number; total_duration_seconds: number; cover_thumbnail: string | null; + updated_at: string; // ISO; when the list last CHANGED (rename, add/remove/move/reorder) has_video?: boolean; // only set when listed with ?contains= } @@ -74,6 +75,17 @@ export const playlistsApi = { }), removeFromPlaylist: (id: number, videoId: string) => req(`/api/playlists/${id}/items/${videoId}`, { method: "DELETE" }), + // One request on purpose: add-then-remove from the client leaves the video in BOTH lists when + // the second call fails. The server does it in one transaction. + moveToPlaylist: ( + id: number, + videoId: string, + targetPlaylistId: number, + ): Promise<{ ok: true; from_playlist_id: number; to_playlist_id: number; video_id: string }> => + req(`/api/playlists/${id}/items/${videoId}/move`, { + method: "POST", + body: JSON.stringify({ target_playlist_id: targetPlaylistId }), + }), reorderPlaylist: (id: number, videoIds: string[]) => req(`/api/playlists/${id}/order`, { method: "PUT",