feat(playlists): act on the selected rows from the toolbar, and undo either

While rows are selected the toolbar's second row is the band: the count,
the last target as one click, Move to, Remove, Clear selection. Both write
in one request and undo back to the places; a video the target held before
stays there. Delete takes the selection when there is one. A failure puts
the rows back ticked.

A row's own move goes through the same write, so it undoes too, and the
single-row move route and service function are gone. A playlist row drops
its grip below a 24rem table: the row menu keeps the moves.
This commit is contained in:
2026-11-01 22:07:09 +01:00
parent 3b85881af4
commit efbbb23525
24 changed files with 1114 additions and 236 deletions
+13 -41
View File
@@ -292,7 +292,7 @@ def set_progress(db: Session, user: User, video_id: str, position: int) -> None:
# Every reason a move is refused; the UI names each one (`playlists.moveErrors.<reason>`, held by the
# frontend's backendIds contract, which reads this Literal).
MoveReason = Literal["same_playlist", "not_in_playlist"]
MoveReason = Literal["same_playlist"]
class MoveRefused(Exception):
@@ -447,41 +447,6 @@ def delete_playlist(db: Session, pl: Playlist) -> None:
db.flush()
def move_item(db: Session, src: Playlist, dst: Playlist, video_id: str) -> None:
"""Move one video from `src` into `dst`, in the caller's one transaction.
The client-side shape — add to the target, then remove from the source — leaves the video in
BOTH lists whenever the second call fails. Idempotent: a video already in the target is simply
dropped from the source (moving into a list that has it is a removal, not a duplicate). Raises
`MoveRefused` for the same list twice, or a video a concurrent edit already took out."""
if src.id == dst.id:
raise MoveRefused("same_playlist")
# Both lists, in id order: a move A→B beside a move B→A would otherwise lock crosswise.
lock_playlists(db, src.id, dst.id)
row = queries.item(db, src.id, video_id)
if row is None:
raise MoveRefused("not_in_playlist")
dst_changed = False
if queries.item(db, dst.id, video_id) is None:
# 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. The
# check above holds: both lists are locked, and an insert into the target (another tab's
# add, a YouTube merge) waits for the lock — its foreign key needs the target row.
row.playlist_id = dst.id
row.position = next_item_position(db, dst.id)
dst_changed = True
if not dst_changed:
db.delete(row)
# The source always changed; the target only when the row landed in it. Touching a target
# that already had the video promoted an untouched list in the rail's "recently changed" sort.
recompute_dirty(db, src)
touch(src)
if dst_changed:
recompute_dirty(db, dst)
touch(dst)
db.flush()
def remove_items(db: Session, pl: Playlist, video_ids: list[str]) -> int:
"""Remove these videos from a playlist, in one transaction; a video not in it any more is simply
not counted. Says how many went."""
@@ -534,12 +499,14 @@ def restore_items(db: Session, pl: Playlist, video_ids: list[str], positions: li
def move_items(
db: Session, src: Playlist, dst: Playlist, video_ids: list[str]
) -> tuple[list[str], list[str]]:
"""Move these videos from `src` into `dst`, in the source's order whatever order they were picked
in, as `move_item` does one. Returns the ones that landed in the target and the ones it already
held, which only left the source: the Undo needs to tell them apart. A video a concurrent edit
already took out of the source is in neither."""
"""Move these videos from `src` into `dst`, in the caller's one transaction, in the source's
order whatever order they were picked in. Returns the ones that landed in the target and the ones
it already held, which only left the source: the Undo needs to tell them apart. A video a
concurrent edit already took out of the source is in neither. Raises `MoveRefused` for the same
list twice."""
if src.id == dst.id:
raise MoveRefused("same_playlist")
# Both lists, in id order: a move A→B beside a move B→A would otherwise lock crosswise.
lock_playlists(db, src.id, dst.id)
wanted = set(video_ids)
rows = [it for it in queries.items(db, src.id) if it.video_id in wanted]
@@ -553,13 +520,18 @@ def move_items(
db.delete(row)
else:
moved.append(row.video_id)
# Re-parented, as `move_item` says why: `added_at` survives the move.
# Re-parented rather than deleted and inserted: `added_at` ("when I saved this") survives
# the move, and there is no window where the video belongs to neither list. `held` stays
# true: both lists are locked, and an insert into the target (another tab's add, a
# YouTube merge) waits for the lock, as its foreign key needs the target row.
row.playlist_id = dst.id
row.position = position
position += 1
if rows:
recompute_dirty(db, src)
touch(src)
# Only when a row landed in it: touching a target that already had the video promoted an untouched
# list in the rail's "recently changed" sort.
if moved:
recompute_dirty(db, dst)
touch(dst)
+7 -40
View File
@@ -421,44 +421,6 @@ def remove_item(
return {"ok": True, "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)
# Its own reason: "this playlist doesn't exist any more" about a TARGET that is gone was read as
# the list on screen — which is still there.
dst = queries.own_playlist(db, user, body.target_playlist_id)
if dst is None:
raise not_found("move_target")
try:
service.move_item(db, src, dst, video_id)
except service.MoveRefused as e:
# A same-list move is a bad request; a video a concurrent edit (another tab, a YouTube
# re-pull) already took out is gone.
status = 400 if e.reason == "same_playlist" else 404
raise HTTPException(
status_code=status, detail={"code": "playlist_move", "reason": e.reason}
) from None
db.commit()
return {"ok": True, "from_playlist_id": src.id, "to_playlist_id": dst.id, "video_id": video_id}
class VideoIdsIn(BaseModel):
video_ids: list[str] = []
@@ -542,8 +504,13 @@ def move_items(
user: User = Depends(current_user),
db: Session = Depends(get_db),
) -> dict:
"""Move many videos into another of the caller's playlists, in one transaction, for the reason
`move_item` gives. Says which landed there and which it already held, for the "Undo"."""
"""Move videos 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 a video in BOTH lists whenever the second call fails. A video the
target already has only leaves the source, and one a concurrent edit (another tab, a YouTube
re-pull) already took out is skipped. Says which landed there and which it already held, for
the "Undo"."""
src = _own_playlist(db, user, playlist_id)
dst = _other_list(db, user, body.target_playlist_id)
try:
+7 -4
View File
@@ -51,7 +51,8 @@ def _ids(db, pl_id):
def _move(client, src, video_id, dst):
return client.post(
f"/api/playlists/{src}/items/{video_id}/move", json={"target_playlist_id": dst}
f"/api/playlists/{src}/items/move-bulk",
json={"video_ids": [video_id], "target_playlist_id": dst},
)
@@ -67,7 +68,8 @@ def test_move_leaves_the_video_in_exactly_one_list(db, make_user, as_user):
"ok": True,
"from_playlist_id": src.id,
"to_playlist_id": dst.id,
"video_id": "v1",
"moved": ["v1"],
"already_there": [],
}
db.expire_all()
assert _ids(db, src.id) == ["v2"]
@@ -116,8 +118,9 @@ def test_move_of_a_video_that_left_the_source_reports_it(db, make_user, as_user)
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"}
assert r.status_code == 200
assert (r.json()["moved"], r.json()["already_there"]) == ([], [])
assert _ids(db, dst.id) == []
def test_move_into_someone_elses_list_404s_and_changes_nothing(db, make_user, as_user):
+6 -3
View File
@@ -107,7 +107,8 @@ PLEX = PlexPlaylist.__table__.name
),
lambda c, pid, other: c.post("/api/playlists/watch-later", json={"video_id": "v2"}),
lambda c, pid, other: c.post(
f"/api/playlists/{pid}/items/v1/move", json={"target_playlist_id": other}
f"/api/playlists/{pid}/items/move-bulk",
json={"video_ids": ["v1"], "target_playlist_id": other},
),
lambda c, pid, other: c.put(f"/api/playlists/{pid}/order", json={"video_ids": ["v1"]}),
],
@@ -173,7 +174,8 @@ PLEX_ITEMS = PlexPlaylistItem.__table__.name
),
lambda c, pid, other: c.delete(f"/api/playlists/{pid}/items/v1"),
lambda c, pid, other: c.post(
f"/api/playlists/{pid}/items/v1/move", json={"target_playlist_id": other}
f"/api/playlists/{pid}/items/move-bulk",
json={"video_ids": ["v1"], "target_playlist_id": other},
),
lambda c, pid, other: c.put(f"/api/playlists/{pid}/order", json={"video_ids": ["v1"]}),
# the ORM cascade deletes the items before the list row
@@ -199,7 +201,8 @@ def test_a_move_locks_both_lists_in_one_ordered_statement(db, make_user, as_user
pid, other = _yt(db, user)
statements.clear()
r = as_user(user).post(
f"/api/playlists/{pid}/items/v1/move", json={"target_playlist_id": other}
f"/api/playlists/{pid}/items/move-bulk",
json={"video_ids": ["v1"], "target_playlist_id": other},
)
assert r.status_code == 200, r.text
lock = _locked_before_items(statements, YT, YT_ITEMS)
+5 -6
View File
@@ -235,14 +235,13 @@ def test_a_move_never_leaves_a_video_in_two_lists(db, make_user):
service.add_item(db, dst, "b", mark_dirty=False)
db.commit()
with pytest.raises(service.MoveRefused, match="same_playlist"):
service.move_item(db, src, src, "a")
service.move_item(db, src, dst, "a")
service.move_item(db, src, dst, "b") # already in the target: dropped from the source
service.move_items(db, src, src, ["a"])
assert service.move_items(db, src, dst, ["a"]) == (["a"], [])
# already in the target: dropped from the source
assert service.move_items(db, src, dst, ["b"]) == ([], ["b"])
db.commit()
assert (_ids(db, src), _ids(db, dst)) == ([], ["b", "a"])
with pytest.raises(service.MoveRefused, match="not_in_playlist"):
service.move_item(db, src, dst, "a")
db.rollback()
assert service.move_items(db, src, dst, ["a"]) == ([], [])
def test_a_reorder_keeps_what_it_left_out_after_the_listed(db, make_user):