feat(e10): make "move a video to another list" one atomic request
The one-step move S2 needs has no honest client-side shape: "add to the target, then
remove from the source" leaves the video in BOTH lists whenever the second call fails,
and the reverse order loses it entirely. So it is one endpoint in one transaction —
POST /api/playlists/{id}/items/{video_id}/move — which re-parents the existing item row
(so `added_at` survives), recomputes `dirty` on both sides, and is idempotent when the
target already holds the video.
Also publishes `updated_at` on the playlist summary, and bumps it explicitly for the
edits that only write `playlist_items` (add / remove / reorder / move). UpdatedAtMixin's
`onupdate` fires on an UPDATE of the playlist row, so without `_touch` the rail's coming
"recently changed" sort would have seen renames and nothing else.
This commit is contained in:
@@ -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}
|
||||
|
||||
@@ -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
|
||||
@@ -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");
|
||||
|
||||
@@ -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=<video_id>
|
||||
}
|
||||
|
||||
@@ -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",
|
||||
|
||||
Reference in New Issue
Block a user