feat(playlists): remove and move many videos at once, and undo either, in one transaction

backend/app/reader/service.py

@@ -482,6 +482,119 @@ def move_item(db: Session, src: Playlist, dst: Playlist, video_id: str) -> None:
     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."""
+    lock_playlists(db, pl.id)
+    wanted = set(video_ids)
+    rows = [it for it in queries.items(db, pl.id) if it.video_id in wanted]
+    for row in rows:
+        db.delete(row)
+    if rows:
+        recompute_dirty(db, pl)
+        touch(pl)
+    db.flush()
+    return len(rows)
+
+
+def _insert_at(db: Session, pl: Playlist, placed: list[tuple[int, str]]) -> int:
+    """Add videos at their indices (ascending, so each index is where the row stood when the rows
+    before it were back) and renumber the list 0..n-1, as positions can have gaps. A video already in
+    the list stays where it is, and one the catalog has lost is left out. Says how many were added;
+    the caller holds the playlist's lock."""
+    rows = queries.items(db, pl.id)
+    present = {r.video_id for r in rows}
+    known = queries.known_video_ids(db, [vid for _, vid in placed])
+    added = 0
+    for at, vid in sorted(placed, key=lambda p: p[0]):
+        if vid in present or vid not in known:
+            continue
+        row = PlaylistItem(playlist_id=pl.id, video_id=vid, position=0)
+        db.add(row)
+        rows.insert(min(at, len(rows)), row)
+        present.add(vid)
+        added += 1
+    for pos, row in enumerate(rows):
+        row.position = pos
+    return added
+
+
+def restore_items(db: Session, pl: Playlist, video_ids: list[str], positions: list[int]) -> int:
+    """Put removed videos back at their places, in ONE request: an append plus a whole-list reorder
+    leaves them at the end whenever the second call fails. Says how many came back."""
+    lock_playlists(db, pl.id)
+    added = _insert_at(db, pl, list(zip(positions, video_ids, strict=True)))
+    if added:
+        recompute_dirty(db, pl)
+        touch(pl)
+    db.flush()
+    return added
+
+
+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."""
+    if src.id == dst.id:
+        raise MoveRefused("same_playlist")
+    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]
+    held = queries.in_playlist(db, dst.id, [r.video_id for r in rows])
+    position = next_item_position(db, dst.id)
+    moved: list[str] = []
+    already_there: list[str] = []
+    for row in rows:
+        if row.video_id in held:
+            already_there.append(row.video_id)
+            db.delete(row)
+        else:
+            moved.append(row.video_id)
+            # Re-parented, as `move_item` says why: `added_at` survives the move.
+            row.playlist_id = dst.id
+            row.position = position
+            position += 1
+    if rows:
+        recompute_dirty(db, src)
+        touch(src)
+    if moved:
+        recompute_dirty(db, dst)
+        touch(dst)
+    db.flush()
+    return moved, already_there
+
+
+def undo_move(
+    db: Session,
+    src: Playlist,
+    dst: Playlist,
+    moved: list[str],
+    already_there: list[str],
+    positions: list[int],
+) -> int:
+    """Take a move back in one transaction: the `moved` videos leave `dst` and every video returns to
+    its place in `src`, while the `already_there` ones stay in `dst`, which held them before.
+    `positions` pairs with `moved` + `already_there`. Says how many came back."""
+    lock_playlists(db, src.id, dst.id)
+    wanted = set(moved)
+    rows = [it for it in queries.items(db, dst.id) if it.video_id in wanted]
+    for row in rows:
+        db.delete(row)
+    db.flush()
+    back = _insert_at(db, src, list(zip(positions, [*moved, *already_there], strict=True)))
+    if rows:
+        recompute_dirty(db, dst)
+        touch(dst)
+    if back:
+        recompute_dirty(db, src)
+        touch(src)
+    db.flush()
+    return back
+
+
 def reorder_items(db: Session, pl: Playlist, order: list[str]) -> int:
     """Number the listed videos 0..n in the listed order. Items the list left out (added in another
     tab meanwhile) keep their relative order after them, so no two items share a position. The

backend/app/routes/playlists.py

@@ -6,7 +6,7 @@ out (`/{id}/push-plan`, `/{id}/revert-youtube`), behind the optional write scope
 from typing import TYPE_CHECKING, Literal
 
 from fastapi import APIRouter, Depends, HTTPException
-from pydantic import BaseModel, Field
+from pydantic import BaseModel, Field, model_validator
 from sqlalchemy.orm import Session
 
 from app import quota
@@ -459,6 +459,124 @@ def move_item(
     return {"ok": True, "from_playlist_id": src.id, "to_playlist_id": dst.id, "video_id": video_id}
 
 
+class VideoIdsIn(BaseModel):
+    video_ids: list[str] = []
+
+
+def _paired(video_count: int, positions: list[int]) -> None:
+    """One place per video, none before the start: both undo bodies come back from the client."""
+    if len(positions) != video_count:
+        raise ValueError("positions must pair one-to-one with the videos")
+    if any(at < 0 for at in positions):
+        raise ValueError("positions cannot be negative")
+
+
+class RestoreItemsIn(VideoIdsIn):
+    # Where each video goes back: an index over the ordered rows, one per video.
+    positions: list[int] = []
+
+    @model_validator(mode="after")
+    def _one_position_per_video(self):
+        _paired(len(self.video_ids), self.positions)
+        return self
+
+
+class MoveItemsIn(VideoIdsIn):
+    target_playlist_id: int
+
+
+class UndoMoveIn(BaseModel):
+    from_playlist_id: int
+    moved: list[str] = []
+    already_there: list[str] = []
+    # One per video of `moved` + `already_there`, in that order.
+    positions: list[int] = []
+
+    @model_validator(mode="after")
+    def _one_position_per_video(self):
+        _paired(len(self.moved) + len(self.already_there), self.positions)
+        return self
+
+
+def _other_list(db: Session, user: User, playlist_id: int) -> "Playlist":
+    """The other list of a move. Its own reason: "this playlist doesn't exist any more" about a list
+    that is gone was read as the one on screen, which is still there."""
+    other = queries.own_playlist(db, user, playlist_id)
+    if other is None:
+        raise not_found("move_target")
+    return other
+
+
+@router.post("/{playlist_id}/items/remove-bulk")
+def remove_items(
+    playlist_id: int,
+    body: VideoIdsIn,
+    user: User = Depends(current_user),
+    db: Session = Depends(get_db),
+) -> dict:
+    """Remove many videos from a playlist at once. Returns how many went."""
+    pl = _own_playlist(db, user, playlist_id)
+    removed = service.remove_items(db, pl, body.video_ids)
+    db.commit()
+    return {"ok": True, "playlist_id": pl.id, "removed": removed}
+
+
+@router.post("/{playlist_id}/items/restore")
+def restore_items(
+    playlist_id: int,
+    body: RestoreItemsIn,
+    user: User = Depends(current_user),
+    db: Session = Depends(get_db),
+) -> dict:
+    """The "Undo" of a removal: the videos back at their places. Returns how many came back."""
+    pl = _own_playlist(db, user, playlist_id)
+    added = service.restore_items(db, pl, body.video_ids, body.positions)
+    db.commit()
+    return {"ok": True, "playlist_id": pl.id, "added": added}
+
+
+@router.post("/{playlist_id}/items/move-bulk")
+def move_items(
+    playlist_id: int,
+    body: MoveItemsIn,
+    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"."""
+    src = _own_playlist(db, user, playlist_id)
+    dst = _other_list(db, user, body.target_playlist_id)
+    try:
+        moved, already_there = service.move_items(db, src, dst, body.video_ids)
+    except service.MoveRefused as e:
+        raise HTTPException(
+            status_code=400, detail={"code": "playlist_move", "reason": e.reason}
+        ) from None
+    db.commit()
+    return {
+        "ok": True,
+        "from_playlist_id": src.id,
+        "to_playlist_id": dst.id,
+        "moved": moved,
+        "already_there": already_there,
+    }
+
+
+@router.post("/{playlist_id}/items/undo-move")
+def undo_move(
+    playlist_id: int,
+    body: UndoMoveIn,
+    user: User = Depends(current_user),
+    db: Session = Depends(get_db),
+) -> dict:
+    """The "Undo" of a move, back into this playlist in one transaction. Returns how many came back."""
+    src = _own_playlist(db, user, playlist_id)
+    dst = _other_list(db, user, body.from_playlist_id)
+    added = service.undo_move(db, src, dst, body.moved, body.already_there, body.positions)
+    db.commit()
+    return {"ok": True, "playlist_id": src.id, "added": added}
+
+
 class ReorderItemsIn(BaseModel):
     video_ids: list[str] = []
 

backend/tests/test_playlist_bulk.py

@@ -0,0 +1,352 @@
+"""Many videos at once: remove, move, and the Undo of each. Every test reads both lists after the call,
+as `test_playlist_move` does: a half-applied move is the failure these routes exist to rule out."""
+
+from datetime import datetime, timedelta, timezone
+
+import pytest
+from sqlalchemy import select
+
+from app.catalog.models import Channel, Video
+from app.reader import service
+from app.reader.models import Playlist, PlaylistItem
+
+
+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):
+    db.expire_all()
+    return list(
+        db.execute(
+            select(PlaylistItem.video_id)
+            .where(PlaylistItem.playlist_id == pl_id)
+            .order_by(PlaylistItem.position)
+        ).scalars()
+    )
+
+
+def _positions(db, pl_id):
+    db.expire_all()
+    return list(
+        db.execute(
+            select(PlaylistItem.position)
+            .where(PlaylistItem.playlist_id == pl_id)
+            .order_by(PlaylistItem.position)
+        ).scalars()
+    )
+
+
+def test_remove_bulk_takes_out_the_named_videos_and_counts_what_was_there(db, make_user, as_user):
+    user = make_user()
+    _seed_videos(db, ["a", "b", "c", "d"])
+    pl = _playlist(db, user, "L", ["a", "b", "c", "d"])
+
+    r = as_user(user).post(
+        f"/api/playlists/{pl.id}/items/remove-bulk", json={"video_ids": ["b", "d", "gone"]}
+    )
+
+    assert r.status_code == 200
+    assert r.json() == {"ok": True, "playlist_id": pl.id, "removed": 2}
+    assert _ids(db, pl.id) == ["a", "c"]
+
+
+def test_remove_bulk_of_nothing_leaves_the_list_untouched(db, make_user, as_user):
+    user = make_user()
+    _seed_videos(db, ["a"])
+    pl = _playlist(db, user, "L", ["a"])
+    stamp = datetime.now(timezone.utc) - timedelta(days=3)
+    pl.updated_at = stamp
+    db.commit()
+
+    r = as_user(user).post(
+        f"/api/playlists/{pl.id}/items/remove-bulk", json={"video_ids": ["gone"]}
+    )
+
+    assert r.json()["removed"] == 0
+    db.expire_all()
+    assert db.get(Playlist, pl.id).updated_at == stamp
+
+
+def test_restore_puts_the_videos_back_at_their_places(db, make_user, as_user):
+    user = make_user()
+    _seed_videos(db, ["a", "b", "c", "d"])
+    pl = _playlist(db, user, "L", ["b", "d"])
+
+    r = as_user(user).post(
+        f"/api/playlists/{pl.id}/items/restore",
+        json={"video_ids": ["c", "a"], "positions": [2, 0]},
+    )
+
+    assert r.json() == {"ok": True, "playlist_id": pl.id, "added": 2}
+    assert _ids(db, pl.id) == ["a", "b", "c", "d"]
+    assert _positions(db, pl.id) == [0, 1, 2, 3]
+
+
+def test_restore_leaves_a_video_already_back_where_it_is_and_skips_one_the_catalog_lost(
+    db, make_user, as_user
+):
+    user = make_user()
+    _seed_videos(db, ["a", "b"])
+    pl = _playlist(db, user, "L", ["b", "a"])
+
+    r = as_user(user).post(
+        f"/api/playlists/{pl.id}/items/restore",
+        json={"video_ids": ["a", "lost"], "positions": [0, 1]},
+    )
+
+    assert r.json()["added"] == 0
+    assert _ids(db, pl.id) == ["b", "a"]
+
+
+@pytest.mark.parametrize(
+    "body",
+    [
+        {"video_ids": ["a"], "positions": []},
+        {"video_ids": ["a"], "positions": [0, 1]},
+        {"video_ids": ["a"], "positions": [-1]},
+    ],
+)
+def test_restore_refuses_positions_that_do_not_pair(db, make_user, as_user, body):
+    user = make_user()
+    pl = _playlist(db, user, "L", [])
+
+    assert as_user(user).post(f"/api/playlists/{pl.id}/items/restore", json=body).status_code == 422
+
+
+def test_move_bulk_lands_in_the_sources_order_whatever_order_they_were_picked_in(
+    db, make_user, as_user
+):
+    user = make_user()
+    _seed_videos(db, ["a", "b", "c", "x"])
+    src = _playlist(db, user, "Source", ["a", "b", "c"])
+    dst = _playlist(db, user, "Target", ["x"])
+
+    r = as_user(user).post(
+        f"/api/playlists/{src.id}/items/move-bulk",
+        json={"video_ids": ["c", "a"], "target_playlist_id": dst.id},
+    )
+
+    assert r.status_code == 200
+    assert r.json() == {
+        "ok": True,
+        "from_playlist_id": src.id,
+        "to_playlist_id": dst.id,
+        "moved": ["a", "c"],
+        "already_there": [],
+    }
+    assert _ids(db, src.id) == ["b"]
+    assert _ids(db, dst.id) == ["x", "a", "c"]
+
+
+def test_move_bulk_says_which_the_target_already_held_and_skips_one_already_gone(
+    db, make_user, as_user
+):
+    user = make_user()
+    _seed_videos(db, ["a", "b", "c"])
+    src = _playlist(db, user, "Source", ["a", "b"])
+    dst = _playlist(db, user, "Target", ["b"])
+
+    r = as_user(user).post(
+        f"/api/playlists/{src.id}/items/move-bulk",
+        json={"video_ids": ["a", "b", "c"], "target_playlist_id": dst.id},
+    )
+
+    assert r.json()["moved"] == ["a"]
+    assert r.json()["already_there"] == ["b"]
+    assert _ids(db, src.id) == []
+    assert _ids(db, dst.id) == ["b", "a"]
+
+
+def test_move_bulk_keeps_when_each_video_was_saved(db, make_user, as_user):
+    user = make_user()
+    _seed_videos(db, ["a"])
+    src = _playlist(db, user, "Source", ["a"])
+    dst = _playlist(db, user, "Target", [])
+    saved = datetime(2020, 1, 2, tzinfo=timezone.utc)
+    row = db.scalar(select(PlaylistItem).where(PlaylistItem.playlist_id == src.id))
+    row.added_at = saved
+    db.commit()
+
+    r = as_user(user).post(
+        f"/api/playlists/{src.id}/items/move-bulk",
+        json={"video_ids": ["a"], "target_playlist_id": dst.id},
+    )
+    assert r.status_code == 200
+
+    db.expire_all()
+    moved = db.scalar(select(PlaylistItem).where(PlaylistItem.playlist_id == dst.id))
+    assert moved.added_at == saved
+
+
+def test_a_target_that_gained_nothing_does_not_rise_in_the_recently_changed_sort(
+    db, make_user, as_user
+):
+    user = make_user()
+    _seed_videos(db, ["a"])
+    src = _playlist(db, user, "Source", ["a"])
+    dst = _playlist(db, user, "Target", ["a"])
+    stamp = datetime.now(timezone.utc) - timedelta(days=3)
+    dst.updated_at = stamp
+    db.commit()
+
+    r = as_user(user).post(
+        f"/api/playlists/{src.id}/items/move-bulk",
+        json={"video_ids": ["a"], "target_playlist_id": dst.id},
+    )
+    assert r.status_code == 200
+
+    db.expire_all()
+    assert db.get(Playlist, dst.id).updated_at == stamp
+
+
+def test_move_bulk_into_the_same_list_is_refused(db, make_user, as_user):
+    user = make_user()
+    _seed_videos(db, ["a"])
+    pl = _playlist(db, user, "L", ["a"])
+
+    r = as_user(user).post(
+        f"/api/playlists/{pl.id}/items/move-bulk",
+        json={"video_ids": ["a"], "target_playlist_id": pl.id},
+    )
+
+    assert r.status_code == 400
+    assert r.json()["detail"] == {"code": "playlist_move", "reason": "same_playlist"}
+    assert _ids(db, pl.id) == ["a"]
+
+
+def test_move_bulk_marks_a_linked_list_on_both_sides_as_unsynced(db, make_user, as_user):
+    user = make_user()
+    _seed_videos(db, ["a"])
+    src = _playlist(db, user, "Source", ["a"], yt_playlist_id="PLs")
+    dst = _playlist(db, user, "Target", [], yt_playlist_id="PLd")
+
+    r = as_user(user).post(
+        f"/api/playlists/{src.id}/items/move-bulk",
+        json={"video_ids": ["a"], "target_playlist_id": dst.id},
+    )
+    assert r.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_undo_move_restores_both_lists_as_they_stood(db, make_user, as_user):
+    user = make_user()
+    _seed_videos(db, ["a", "b", "c", "d"])
+    src = _playlist(db, user, "Source", ["a", "b", "c", "d"])
+    dst = _playlist(db, user, "Target", ["c"])
+    client = as_user(user)
+
+    moved = client.post(
+        f"/api/playlists/{src.id}/items/move-bulk",
+        json={"video_ids": ["a", "c"], "target_playlist_id": dst.id},
+    ).json()
+    assert _ids(db, src.id) == ["b", "d"]
+    assert _ids(db, dst.id) == ["c", "a"]
+
+    r = client.post(
+        f"/api/playlists/{src.id}/items/undo-move",
+        json={
+            "from_playlist_id": dst.id,
+            "moved": moved["moved"],
+            "already_there": moved["already_there"],
+            "positions": [0, 2],
+        },
+    )
+
+    assert r.json() == {"ok": True, "playlist_id": src.id, "added": 2}
+    assert _ids(db, src.id) == ["a", "b", "c", "d"]
+    # The one the target held before the move stays there.
+    assert _ids(db, dst.id) == ["c"]
+
+
+def test_undo_move_is_one_transaction(db, make_user, as_user, monkeypatch):
+    user = make_user()
+    _seed_videos(db, ["a"])
+    src = _playlist(db, user, "Source", [])
+    dst = _playlist(db, user, "Target", ["a"])
+
+    def boom(*a, **kw):
+        raise RuntimeError("second step failed")
+
+    monkeypatch.setattr(service, "_insert_at", boom)
+    client = as_user(user)
+    body = {"from_playlist_id": dst.id, "moved": ["a"], "already_there": [], "positions": [0]}
+    with pytest.raises(RuntimeError):
+        _ = client.post(f"/api/playlists/{src.id}/items/undo-move", json=body)
+
+    db.rollback()
+    assert _ids(db, dst.id) == ["a"]
+    assert _ids(db, src.id) == []
+
+
+def test_undo_move_refuses_positions_that_do_not_pair(db, make_user, as_user):
+    user = make_user()
+    src = _playlist(db, user, "Source", [])
+    dst = _playlist(db, user, "Target", [])
+
+    r = as_user(user).post(
+        f"/api/playlists/{src.id}/items/undo-move",
+        json={"from_playlist_id": dst.id, "moved": ["a"], "already_there": ["b"], "positions": [0]},
+    )
+
+    assert r.status_code == 422
+
+
+@pytest.mark.parametrize(
+    ("path", "body"),
+    [
+        ("remove-bulk", {"video_ids": ["a"]}),
+        ("restore", {"video_ids": ["a"], "positions": [0]}),
+        ("move-bulk", {"video_ids": ["a"], "target_playlist_id": 0}),
+        ("undo-move", {"from_playlist_id": 0, "moved": ["a"], "positions": [0]}),
+    ],
+)
+def test_another_accounts_list_is_not_found(db, make_user, as_user, path, body):
+    owner, other = make_user(), make_user()
+    _seed_videos(db, ["a"])
+    theirs = _playlist(db, owner, "Theirs", ["a"])
+    mine = _playlist(db, other, "Mine", [])
+    body = {
+        k: (mine.id if k in ("target_playlist_id", "from_playlist_id") else v)
+        for k, v in body.items()
+    }
+
+    r = as_user(other).post(f"/api/playlists/{theirs.id}/items/{path}", json=body)
+
+    assert r.status_code == 404
+    assert _ids(db, theirs.id) == ["a"]
+
+
+@pytest.mark.parametrize(
+    ("path", "key"), [("move-bulk", "target_playlist_id"), ("undo-move", "from_playlist_id")]
+)
+def test_the_other_list_of_a_move_must_be_the_callers_too(db, make_user, as_user, path, key):
+    owner, other = make_user(), make_user()
+    _seed_videos(db, ["a"])
+    theirs = _playlist(db, owner, "Theirs", ["a"])
+    mine = _playlist(db, other, "Mine", ["a"])
+    body = {"video_ids": ["a"], "moved": ["a"], "positions": [0], key: theirs.id}
+
+    r = as_user(other).post(f"/api/playlists/{mine.id}/items/{path}", json=body)
+
+    assert r.status_code == 404
+    assert "move_target" in str(r.json()["detail"])
+    assert _ids(db, theirs.id) == ["a"]
+    assert _ids(db, mine.id) == ["a"]