diff --git a/backend/app/reader/service.py b/backend/app/reader/service.py index 1041b822..93deffb9 100644 --- a/backend/app/reader/service.py +++ b/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 diff --git a/backend/app/routes/playlists.py b/backend/app/routes/playlists.py index f27ea526..a4ca9d41 100644 --- a/backend/app/routes/playlists.py +++ b/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] = [] diff --git a/backend/tests/test_playlist_bulk.py b/backend/tests/test_playlist_bulk.py new file mode 100644 index 00000000..31c3a167 --- /dev/null +++ b/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"]