diff --git a/CHANGELOG.md b/CHANGELOG.md index 76a1e8f5..601ec2fb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,17 @@ edit the release notes and re-run the generator. Upgrading, pinning a version and rolling back: [docs/self-hosting.md](docs/self-hosting.md#upgrading). +## 0.116.0 — 2026-11-03 + +### New + +- In a playlist, tick rows with their box, Shift+click or x, then move or remove them together. An Undo puts them back at their places, and a single row's move has one too. +- Drag rows onto another list in the rail: one row, or all the selected ones, also under a sort or a filter. + +### Fixed + +- Removing a video from inside the player no longer loses your place in the list when the player closes. + ## 0.115.0 — 2026-11-01 ### New diff --git a/VERSION b/VERSION index bdc80994..4c08787e 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.115.0 +0.116.0 diff --git a/backend/app/reader/service.py b/backend/app/reader/service.py index 1041b822..930f2321 100644 --- a/backend/app/reader/service.py +++ b/backend/app/reader/service.py @@ -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.`, 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): @@ -406,30 +406,6 @@ def remove_item(db: Session, pl: Playlist, video_id: str, *, mark_dirty: bool) - return True -def place_item(db: Session, pl: Playlist, video_id: str, position: int) -> None: - """Put a video already in the list at `position` (clamped to the end), renumbering the whole - list: positions can have gaps after removals, so "index N" is only meaningful over the - ordered rows.""" - rows = queries.items(db, pl.id) - new = next(r for r in rows if r.video_id == video_id) - rows.remove(new) - rows.insert(min(position, len(rows)), new) - for pos, row in enumerate(rows): - row.position = pos - db.flush() - - -def add_to_playlist(db: Session, pl: Playlist, video_id: str, position: int | None) -> None: - """Add a video to one of the user's lists, at `position` when given (the list page's "Undo" of - a removal puts the row back where it was in ONE request), else at the end. A video already in - the list stays where it is.""" - added = add_item(db, pl, video_id, mark_dirty=False) - if added and position is not None: - place_item(db, pl, video_id, position) - recompute_dirty(db, pl) - db.flush() - - def rename_playlist(db: Session, pl: Playlist, name: str) -> None: """A new name moves `dirty`; the caller holds the playlist's lock (the scheduled mirror refresh reads `dirty` under it).""" @@ -447,39 +423,163 @@ 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. +def remove_items(db: Session, pl: Playlist, video_ids: list[str]) -> list[str]: + """Remove these videos from a playlist, in one transaction; a video not in it any more is simply + not among them. Says which went: an Undo must not bring back one this removal never took.""" + 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 [row.video_id for row in rows] - 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.""" + +def _insert_at( + db: Session, + pl: Playlist, + placed: list[tuple[int, str]], + leaving: dict[str, PlaylistItem] | None = None, + staying_out: frozenset[str] = frozenset(), +) -> 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 or in `staying_out` is left out, the + ones after it closing up. A video with a row in `leaving` brings that row along, for the reason + `move_items` re-parents. Says how many were added; the caller holds the playlist's lock.""" + leaving = leaving or {} + 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 + left_out = 0 + for at, vid in sorted(placed, key=lambda p: p[0]): + if vid in present or vid not in known or vid in staying_out: + left_out += 1 + continue + row = leaving.pop(vid, None) + if row is None: + row = PlaylistItem(playlist_id=pl.id, video_id=vid, position=0) + db.add(row) + else: + row.playlist_id = pl.id + rows.insert(min(at - left_out, 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 + + +class ListGone(Exception): + """A list of a two-list write was deleted between the route's read and the lock.""" + + def __init__(self, *, source: bool): + super().__init__("source" if source else "target") + self.source = source + + +def _lock_both(db: Session, src: Playlist, dst: Playlist) -> None: + """Both lists, in id order: a move A→B beside a move B→A would otherwise lock crosswise. A list + already gone is said here: a write into it would answer with its foreign key's 500.""" + found = lock_playlists(db, src.id, dst.id) + if src.id not in found: + raise ListGone(source=True) + if dst.id not in found: + raise ListGone(source=False) + + +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 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) - 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: + _lock_both(db, src, dst) + 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 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) 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 return to their places + in `src`, keeping their rows, as the move did. The `already_there` ones return too and stay in + `dst`, which held them before. A moved video `dst` no longer holds has gone on from there, and + bringing it back would leave it in lists the reader never put it in, so it stays out, as a + move skips a video a concurrent edit took out. `positions` pairs with `moved` + `already_there`. + Says how many came back. Raises `MoveRefused` for the same list twice, as a move does.""" + if src.id == dst.id: + raise MoveRefused("same_playlist") + _lock_both(db, src, dst) + wanted = set(moved) + rows = {it.video_id: it for it in queries.items(db, dst.id) if it.video_id in wanted} + changed = bool(rows) + placed = list(zip(positions, [*moved, *already_there], strict=True)) + back = _insert_at(db, src, placed, leaving=rows, staying_out=frozenset(wanted - rows.keys())) + # What `src` holds again by now only leaves `dst`. + for row in rows.values(): + db.delete(row) + db.flush() + if changed: + 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: diff --git a/backend/app/routes/playlists.py b/backend/app/routes/playlists.py index f27ea526..5622dd2f 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, model_validator from sqlalchemy.orm import Session from app import quota @@ -386,24 +386,17 @@ def delete_playlist( return {"deleted": playlist_id} -class AddItemIn(VideoRefIn): - # Where the video lands among the list's items, in their stored order (0 = first). Omitted, it - # is appended. The list page's "Undo" of a removal puts the row back where it was with this — - # ONE request, because append-then-reorder leaves it at the end whenever the second call fails. - position: int | None = Field(default=None, ge=0) - - @router.post("/{playlist_id}/items") def add_item( playlist_id: int, - body: AddItemIn, + body: VideoRefIn, user: User = Depends(current_user), db: Session = Depends(get_db), ) -> dict: - pl = _own_playlist(db, user, playlist_id) + pl = _own_playlist(db, user, playlist_id, lock=True) video_id = body.video_id _video_or_404(db, video_id) - service.add_to_playlist(db, pl, video_id, body.position) + service.add_item(db, pl, video_id, mark_dirty=True) db.commit() return {"ok": True, "playlist_id": pl.id, "video_id": video_id} @@ -415,58 +408,158 @@ def remove_item( user: User = Depends(current_user), db: Session = Depends(get_db), ) -> dict: - pl = _own_playlist(db, user, playlist_id) + pl = _own_playlist(db, user, playlist_id, lock=True) service.remove_item(db, pl, video_id, mark_dirty=True) db.commit() return {"ok": True, "playlist_id": pl.id, "video_id": video_id} -class MoveItemIn(BaseModel): +class VideoIdsIn(BaseModel): + # Required: with a default, a misnamed key answered 200 having acted on no video. + video_ids: list[str] + + +def _paired(video_count: int, positions: list[int]) -> None: + """One place per video, none before the start and none twice: both undo bodies come back from + the client, and the insert's arithmetic counts on places that only rise.""" + 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") + if len(set(positions)) != len(positions): + raise ValueError("positions cannot repeat") + + +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 -@router.post("/{playlist_id}/items/{video_id}/move") -def move_item( +class UndoMoveIn(BaseModel): + from_playlist_id: int + # Required, as `video_ids` is. An empty one is still a real body: the target held them all. + 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 _list_gone(e: service.ListGone) -> HTTPException: + """Each list of a two-list write under the reason the route's own read gives it.""" + return not_found("playlist") if e.source else not_found("move_target") + + +def _move_refused(e: service.MoveRefused) -> HTTPException: + return HTTPException(status_code=400, detail={"code": "playlist_move", "reason": e.reason}) + + +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, - video_id: str, - body: MoveItemIn, + body: VideoIdsIn, 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. + """Remove many videos from a playlist at once. Returns the ones that went.""" + pl = _own_playlist(db, user, playlist_id, lock=True) + 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, lock=True) + 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 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 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).""" + 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) - # 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") + dst = _other_list(db, user, body.target_playlist_id) try: - service.move_item(db, src, dst, video_id) + moved, already_there = service.move_items(db, src, dst, body.video_ids) 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 + raise _move_refused(e) from None + except service.ListGone as e: + raise _list_gone(e) from None db.commit() - return {"ok": True, "from_playlist_id": src.id, "to_playlist_id": dst.id, "video_id": video_id} + return { + "ok": True, + "from_playlist_id": src.id, + "to_playlist_id": dst.id, + "moved": moved, + "already_there": already_there, + } -class ReorderItemsIn(BaseModel): - video_ids: list[str] = [] +@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) + try: + added = service.undo_move(db, src, dst, body.moved, body.already_there, body.positions) + except service.MoveRefused as e: + raise _move_refused(e) from None + except service.ListGone as e: + raise _list_gone(e) from None + db.commit() + return {"ok": True, "playlist_id": src.id, "added": added} @router.put("/{playlist_id}/order") def reorder_items( playlist_id: int, - body: ReorderItemsIn, + body: VideoIdsIn, user: User = Depends(current_user), db: Session = Depends(get_db), ) -> dict: diff --git a/backend/app/routes/plex/playlists.py b/backend/app/routes/plex/playlists.py index 71b5d453..3c782494 100644 --- a/backend/app/routes/plex/playlists.py +++ b/backend/app/routes/plex/playlists.py @@ -183,7 +183,9 @@ def playlist_remove_item( class PlexItemKeysIn(BaseModel): - item_rating_keys: list[str] = [] + # Required: with a default, a misnamed key answered 200 having acted on no item, and a reorder + # took it for an empty order. + item_rating_keys: list[str] class PlexItemAddBulkIn(PlexItemKeysIn): diff --git a/backend/app/routes/search.py b/backend/app/routes/search.py index bc1bf444..3f96cd4b 100644 --- a/backend/app/routes/search.py +++ b/backend/app/routes/search.py @@ -314,7 +314,8 @@ def search_youtube( class ClearSearchIn(BaseModel): - video_ids: list[str] = [] + # Required: a default read a misnamed key as an empty list, a 200 that cleared nothing. + video_ids: list[str] @router.post("/clear") diff --git a/backend/tests/test_playlist_add_position.py b/backend/tests/test_playlist_add_position.py index 33532ad2..72584c38 100644 --- a/backend/tests/test_playlist_add_position.py +++ b/backend/tests/test_playlist_add_position.py @@ -1,4 +1,4 @@ -"""Adding a video AT a position: the list page's "Undo" of a removal. +"""Putting a video back AT a position: the list page's "Undo" of a removal. The row has to come back where it was in ONE request. The two-call shape (append, then send the whole order) leaves it at the end of the list whenever the second call fails, and the reorder would @@ -38,14 +38,17 @@ def _ids(db, pl_id): ] -def _add(client, pl_id, body): - return client.post(f"/api/playlists/{pl_id}/items", json=body) +def _restore(client, pl_id, video_id, position): + return client.post( + f"/api/playlists/{pl_id}/items/restore", + json={"video_ids": [video_id], "positions": [position]}, + ) def test_a_position_puts_the_video_back_in_its_place(db, make_user, as_user): user = make_user() pl = _seed(db, user, ["a", "b", "c"]) - r = _add(as_user(user), pl.id, {"video_id": "vX", "position": 1}) + r = _restore(as_user(user), pl.id, "vX", 1) assert r.status_code == 200 assert _ids(db, pl.id) == ["a", "vX", "b", "c"] @@ -54,36 +57,37 @@ def test_the_position_counts_rows_not_stored_numbers(db, make_user, as_user): # A removal leaves a gap (0, 5, 9): index 2 is before the THIRD row, not "position 2". user = make_user() pl = _seed(db, user, ["a", "b", "c"], positions=[0, 5, 9]) - _add(as_user(user), pl.id, {"video_id": "vX", "position": 2}) + _restore(as_user(user), pl.id, "vX", 2) assert _ids(db, pl.id) == ["a", "b", "vX", "c"] def test_a_position_past_the_end_appends(db, make_user, as_user): user = make_user() pl = _seed(db, user, ["a", "b"]) - _add(as_user(user), pl.id, {"video_id": "vX", "position": 40}) + _restore(as_user(user), pl.id, "vX", 40) assert _ids(db, pl.id) == ["a", "b", "vX"] -def test_no_position_still_appends(db, make_user, as_user): +def test_an_add_appends(db, make_user, as_user): user = make_user() pl = _seed(db, user, ["a", "b"]) - _add(as_user(user), pl.id, {"video_id": "vX"}) + r = as_user(user).post(f"/api/playlists/{pl.id}/items", json={"video_id": "vX"}) + assert r.status_code == 200 assert _ids(db, pl.id) == ["a", "b", "vX"] def test_a_video_already_in_the_list_does_not_move(db, make_user, as_user): - # The add is idempotent; a position must not turn it into a reorder. + # A restore is idempotent; a position must not turn it into a reorder. user = make_user() pl = _seed(db, user, ["a", "b", "c"]) - _add(as_user(user), pl.id, {"video_id": "c", "position": 0}) + _restore(as_user(user), pl.id, "c", 0) assert _ids(db, pl.id) == ["a", "b", "c"] def test_a_negative_position_is_refused(db, make_user, as_user): user = make_user() pl = _seed(db, user, ["a"]) - r = _add(as_user(user), pl.id, {"video_id": "vX", "position": -1}) + r = _restore(as_user(user), pl.id, "vX", -1) assert r.status_code == 422 assert _ids(db, pl.id) == ["a"] @@ -101,6 +105,6 @@ def test_putting_a_removed_video_back_leaves_a_linked_list_clean(db, make_user, assert client.delete(f"/api/playlists/{pl.id}/items/b").status_code == 200 db.expire_all() assert db.get(Playlist, pl.id).dirty is True - _add(client, pl.id, {"video_id": "b", "position": 1}) + _restore(client, pl.id, "b", 1) db.expire_all() assert db.get(Playlist, pl.id).dirty is False diff --git a/backend/tests/test_playlist_bulk.py b/backend/tests/test_playlist_bulk.py new file mode 100644 index 00000000..53ddcb90 --- /dev/null +++ b/backend/tests/test_playlist_bulk.py @@ -0,0 +1,640 @@ +"""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 queries, service +from app.reader.models import Playlist, PlaylistItem +from app.sync.playlists import current_fingerprint + + +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": ["b", "d"]} + 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"] == [] + 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_keeps_when_each_video_was_saved(db, make_user, as_user): + user = make_user() + _seed_videos(db, ["a"]) + src = _playlist(db, user, "Source", []) + dst = _playlist(db, user, "Target", ["a"]) + saved = datetime(2020, 1, 2, tzinfo=timezone.utc) + row = db.scalar(select(PlaylistItem).where(PlaylistItem.playlist_id == dst.id)) + row.added_at = saved + db.commit() + + r = as_user(user).post( + f"/api/playlists/{src.id}/items/undo-move", + json={"from_playlist_id": dst.id, "moved": ["a"], "positions": [0]}, + ) + + assert r.json()["added"] == 1 + db.expire_all() + back = db.scalar(select(PlaylistItem).where(PlaylistItem.playlist_id == src.id)) + assert back.added_at == saved + + +def test_undo_move_leaves_a_video_that_moved_on_where_it_is(db, make_user, as_user): + """A to B, then B to C, then the first Undo: the video is not in the list the Undo takes it from, + and coming back would leave it in two lists the reader never put it in.""" + user = make_user() + _seed_videos(db, ["a", "b"]) + src = _playlist(db, user, "Source", []) + dst = _playlist(db, user, "Target", ["b"]) + third = _playlist(db, user, "Third", ["a"]) + + r = as_user(user).post( + f"/api/playlists/{src.id}/items/undo-move", + json={"from_playlist_id": dst.id, "moved": ["a", "b"], "positions": [0, 1]}, + ) + + assert r.json()["added"] == 1 + assert _ids(db, src.id) == ["b"] + assert _ids(db, dst.id) == [] + assert _ids(db, third.id) == ["a"] + + +def test_an_undo_closes_up_the_places_of_a_video_that_stayed_out(db, make_user, as_user): + """Each index counts the rows put back before it, so one that stays out moves the rest up.""" + user = make_user() + _seed_videos(db, ["a", "b", "c", "d"]) + src = _playlist(db, user, "Source", ["c", "d"]) + dst = _playlist(db, user, "Target", ["b"]) + + r = as_user(user).post( + f"/api/playlists/{src.id}/items/undo-move", + json={"from_playlist_id": dst.id, "moved": ["a", "b"], "positions": [0, 1]}, + ) + + assert r.json()["added"] == 1 + assert _ids(db, src.id) == ["b", "c", "d"] + + +def test_a_restore_closes_up_the_places_of_a_video_the_catalog_lost(db, make_user, as_user): + user = make_user() + _seed_videos(db, ["b", "c", "d"]) + pl = _playlist(db, user, "L", ["c", "d"]) + + r = as_user(user).post( + f"/api/playlists/{pl.id}/items/restore", + json={"video_ids": ["lost", "b"], "positions": [0, 1]}, + ) + + assert r.json()["added"] == 1 + assert _ids(db, pl.id) == ["b", "c", "d"] + + +def test_undo_move_takes_a_video_out_of_the_target_when_the_source_has_it_again( + db, make_user, as_user +): + user = make_user() + _seed_videos(db, ["a"]) + src = _playlist(db, user, "Source", ["a"]) + dst = _playlist(db, user, "Target", ["a"]) + + r = as_user(user).post( + f"/api/playlists/{src.id}/items/undo-move", + json={"from_playlist_id": dst.id, "moved": ["a"], "positions": [0]}, + ) + + assert r.json()["added"] == 0 + assert _ids(db, src.id) == ["a"] + assert _ids(db, dst.id) == [] + + +@pytest.mark.parametrize( + ("path", "body", "gone", "reason"), + [ + ("move-bulk", {"video_ids": ["a"], "target_playlist_id": "dst"}, "dst", "move_target"), + ("move-bulk", {"video_ids": ["a"], "target_playlist_id": "dst"}, "src", "playlist"), + ( + "undo-move", + {"from_playlist_id": "dst", "moved": ["a"], "positions": [0]}, + "src", + "playlist", + ), + ( + "undo-move", + {"from_playlist_id": "dst", "moved": ["a"], "positions": [0]}, + "dst", + "move_target", + ), + ], +) +def test_a_list_deleted_before_the_lock_is_not_found( + db, make_user, as_user, monkeypatch, path, body, gone, reason +): + user = make_user() + _seed_videos(db, ["a"]) + src = _playlist(db, user, "Source", ["a"]) + dst = _playlist(db, user, "Target", ["a"]) + ids = {"src": src.id, "dst": dst.id} + body = {k: (dst.id if v == "dst" else v) for k, v in body.items()} + # Another tab's delete committed between the route's read and this lock. + monkeypatch.setattr(service, "lock_playlists", lambda _db, *got: set(got) - {ids[gone]}) + + r = as_user(user).post(f"/api/playlists/{src.id}/items/{path}", json=body) + + assert r.status_code == 404 + assert reason in str(r.json()["detail"]) + + +@pytest.mark.parametrize( + ("method", "path", "body"), + [ + ("post", "items", {"video_id": "a"}), + ("delete", "items/a", None), + ("post", "items/remove-bulk", {"video_ids": ["a"]}), + ("post", "items/restore", {"video_ids": ["a"], "positions": [0]}), + ], +) +def test_a_one_list_write_finds_its_list_under_the_lock( + db, make_user, as_user, monkeypatch, method, path, body +): + """Read with its lock, so a delete that won the row answers 404 rather than a foreign key's 500.""" + user = make_user() + _seed_videos(db, ["a"]) + pl = _playlist(db, user, "L", []) + real = queries.own_playlist + + def locked_read_finds_nothing(db_, user_, playlist_id, *, lock=False): + return None if lock else real(db_, user_, playlist_id) + + monkeypatch.setattr(queries, "own_playlist", locked_read_finds_nothing) + kw = {"json": body} if body is not None else {} + + r = getattr(as_user(user), method)(f"/api/playlists/{pl.id}/{path}", **kw) + + assert r.status_code == 404 + + +@pytest.mark.parametrize( + ("path", "body"), + [ + ("items/remove-bulk", {"ids": ["a"]}), + ("items/move-bulk", {"ids": ["a"], "target_playlist_id": 1}), + ("items/undo-move", {"ids": ["a"], "from_playlist_id": 1}), + ], +) +def test_a_body_without_its_videos_is_refused_rather_than_acting_on_none( + db, make_user, as_user, path, body +): + user = make_user() + pl = _playlist(db, user, "L", []) + + assert as_user(user).post(f"/api/playlists/{pl.id}/{path}", json=body).status_code == 422 + + +def test_an_undo_of_only_videos_the_target_held_names_no_moved_ones(db, make_user, as_user): + """An empty `moved` is a real body: every video of the move was already in the target.""" + user = make_user() + _seed_videos(db, ["a"]) + src = _playlist(db, user, "Source", []) + dst = _playlist(db, user, "Target", ["a"]) + + r = as_user(user).post( + f"/api/playlists/{src.id}/items/undo-move", + json={"from_playlist_id": dst.id, "moved": [], "already_there": ["a"], "positions": [0]}, + ) + + assert r.json()["added"] == 1 + assert _ids(db, src.id) == ["a"] + assert _ids(db, dst.id) == ["a"] + + +def test_an_undo_within_one_list_is_refused_and_keeps_its_videos(db, make_user, as_user): + """A move refuses the same list twice, so no Undo of one exists; a row must not go for asking.""" + user = make_user() + _seed_videos(db, ["a", "b"]) + pl = _playlist(db, user, "L", ["a", "b"]) + + r = as_user(user).post( + f"/api/playlists/{pl.id}/items/undo-move", + json={"from_playlist_id": pl.id, "moved": ["a"], "positions": [0]}, + ) + + assert r.status_code == 400 + assert r.json()["detail"] == {"code": "playlist_move", "reason": "same_playlist"} + assert _ids(db, pl.id) == ["a", "b"] + + +@pytest.mark.parametrize( + ("path", "key", "extra"), + [ + ("items/restore", "video_ids", {}), + ("items/undo-move", "moved", {"from_playlist_id": None}), + ], +) +def test_two_videos_cannot_ask_for_one_place(db, make_user, as_user, path, key, extra): + user = make_user() + _seed_videos(db, ["a", "b", "c"]) + pl = _playlist(db, user, "L", ["c"]) + other = _playlist(db, user, "Other", ["a", "b"]) + body = {key: ["a", "b"], "positions": [0, 0]} + body.update({k: other.id for k in extra}) + + r = as_user(user).post(f"/api/playlists/{pl.id}/{path}", json=body) + + assert r.status_code == 422 + assert _ids(db, pl.id) == ["c"] + + +def test_a_reorder_without_its_videos_is_refused(db, make_user, as_user): + user = make_user() + pl = _playlist(db, user, "L", []) + + assert as_user(user).put(f"/api/playlists/{pl.id}/order", json={"ids": []}).status_code == 422 + + +def _synced(db, pl): + pl.synced_fingerprint = current_fingerprint(db, pl) + pl.dirty = False + db.commit() + + +def test_a_bulk_remove_and_its_undo_leave_a_linked_list_as_it_was(db, make_user, as_user): + user = make_user() + _seed_videos(db, ["a", "b"]) + pl = _playlist(db, user, "L", ["a", "b"], yt_playlist_id="PLx") + _synced(db, pl) + client = as_user(user) + + r = client.post(f"/api/playlists/{pl.id}/items/remove-bulk", json={"video_ids": ["a"]}) + assert r.status_code == 200 + db.expire_all() + assert db.get(Playlist, pl.id).dirty is True + + r = client.post( + f"/api/playlists/{pl.id}/items/restore", json={"video_ids": ["a"], "positions": [0]} + ) + assert r.status_code == 200 + db.expire_all() + assert db.get(Playlist, pl.id).dirty is False + + +def test_a_move_and_its_undo_leave_both_linked_lists_as_they_were(db, make_user, as_user): + user = make_user() + _seed_videos(db, ["a", "b"]) + src = _playlist(db, user, "Source", ["a", "b"], yt_playlist_id="PLs") + dst = _playlist(db, user, "Target", ["b"], yt_playlist_id="PLd") + _synced(db, src) + _synced(db, dst) + client = as_user(user) + + moved = client.post( + f"/api/playlists/{src.id}/items/move-bulk", + json={"video_ids": ["a"], "target_playlist_id": dst.id}, + ).json() + assert moved["moved"] == ["a"] + db.expire_all() + assert db.get(Playlist, src.id).dirty is True + assert db.get(Playlist, dst.id).dirty is True + + r = client.post( + f"/api/playlists/{src.id}/items/undo-move", + json={"from_playlist_id": dst.id, "moved": ["a"], "positions": [0]}, + ) + assert r.status_code == 200 + db.expire_all() + assert db.get(Playlist, src.id).dirty is False + assert db.get(Playlist, dst.id).dirty is False + + +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"] diff --git a/backend/tests/test_playlist_move.py b/backend/tests/test_playlist_move.py index d0b74c58..c430f19e 100644 --- a/backend/tests/test_playlist_move.py +++ b/backend/tests/test_playlist_move.py @@ -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): diff --git a/backend/tests/test_playlist_position_lock.py b/backend/tests/test_playlist_position_lock.py index 308c1b7d..50810fec 100644 --- a/backend/tests/test_playlist_position_lock.py +++ b/backend/tests/test_playlist_position_lock.py @@ -102,16 +102,24 @@ PLEX = PlexPlaylist.__table__.name "call", [ lambda c, pid, other: c.post(f"/api/playlists/{pid}/items", json={"video_id": "v2"}), - lambda c, pid, other: c.post( - f"/api/playlists/{pid}/items", json={"video_id": "v2", "position": 0} - ), 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"]}), + lambda c, pid, other: c.post( + f"/api/playlists/{pid}/items/remove-bulk", json={"video_ids": ["v1"]} + ), + lambda c, pid, other: c.post( + f"/api/playlists/{pid}/items/restore", json={"video_ids": ["v2"], "positions": [0]} + ), + lambda c, pid, other: c.post( + f"/api/playlists/{pid}/items/undo-move", + json={"from_playlist_id": other, "moved": ["v2"], "positions": [0]}, + ), ], - ids=["add", "add-at", "watch-later", "move", "reorder"], + ids=["add", "watch-later", "move", "reorder", "remove-bulk", "restore", "undo-move"], ) def test_a_youtube_playlist_write_locks_the_playlist(db, make_user, as_user, locks, call): user = make_user() @@ -168,18 +176,35 @@ PLEX_ITEMS = PlexPlaylistItem.__table__.name "call", [ lambda c, pid, other: c.post(f"/api/playlists/{pid}/items", json={"video_id": "v1"}), - lambda c, pid, other: c.post( - f"/api/playlists/{pid}/items", json={"video_id": "v2", "position": 0} - ), 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 lambda c, pid, other: c.delete(f"/api/playlists/{pid}"), + lambda c, pid, other: c.post( + f"/api/playlists/{pid}/items/remove-bulk", json={"video_ids": ["v1"]} + ), + lambda c, pid, other: c.post( + f"/api/playlists/{pid}/items/restore", json={"video_ids": ["v2"], "positions": [0]} + ), + lambda c, pid, other: c.post( + f"/api/playlists/{pid}/items/undo-move", + json={"from_playlist_id": other, "moved": ["v2"], "positions": [0]}, + ), + ], + ids=[ + "add-present", + "remove", + "move", + "reorder", + "delete", + "remove-bulk", + "restore", + "undo-move", ], - ids=["add-present", "add-at", "remove", "move", "reorder", "delete"], ) def test_a_youtube_item_write_locks_the_playlist_before_any_item( db, make_user, as_user, statements, call @@ -193,14 +218,23 @@ def test_a_youtube_item_write_locks_the_playlist_before_any_item( assert _locked_before_items(statements, YT, YT_ITEMS), statements -def test_a_move_locks_both_lists_in_one_ordered_statement(db, make_user, as_user, statements): - """Two moves A→B and B→A each locking "its" list first would lock crosswise.""" +@pytest.mark.parametrize( + ("path", "body"), + [ + ("move-bulk", lambda other: {"video_ids": ["v1"], "target_playlist_id": other}), + ("undo-move", lambda other: {"from_playlist_id": other, "moved": ["v2"], "positions": [0]}), + ], + ids=["move", "undo"], +) +def test_a_move_locks_both_lists_in_one_ordered_statement( + db, make_user, as_user, statements, path, body +): + """Two moves A→B and B→A each locking "its" list first would lock crosswise, and an Undo runs the + other way round from a move by its nature.""" user = make_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} - ) + r = as_user(user).post(f"/api/playlists/{pid}/items/{path}", json=body(other)) assert r.status_code == 200, r.text lock = _locked_before_items(statements, YT, YT_ITEMS) assert lock and "ORDER BY" in lock, lock diff --git a/backend/tests/test_plex_playlist_add_positions.py b/backend/tests/test_plex_playlist_add_positions.py index 04b1552b..4d4989ff 100644 --- a/backend/tests/test_plex_playlist_add_positions.py +++ b/backend/tests/test_plex_playlist_add_positions.py @@ -6,6 +6,8 @@ rows, since positions have gaps after a removal. One request: an append plus a s leaves the rows at the end whenever that call fails, and would overwrite another tab's changes. """ +import pytest + from app.plex.models import PlexItem, PlexLibrary, PlexPlaylist, PlexPlaylistItem @@ -101,6 +103,22 @@ def test_positions_must_pair_with_the_keys(db, make_user, as_user): assert _order(db, pl.id)[0] == ["a"] +@pytest.mark.parametrize( + ("method", "path"), + [("post", "items/bulk"), ("post", "items/remove-bulk"), ("put", "order")], +) +def test_a_body_without_its_keys_is_refused_rather_than_acting_on_none( + db, make_user, as_user, method, path +): + user = make_user() + pl = _seed(db, user, ["a"], extra=()) + + r = getattr(as_user(user), method)(f"/api/plex/playlists/{pl.id}/{path}", json={"keys": ["a"]}) + + assert r.status_code == 422 + assert _order(db, pl.id)[0] == ["a"] + + def test_remove_then_put_back_is_no_change(db, make_user, as_user): user = make_user() pl = _seed(db, user, ["a", "b", "c", "d"], extra=()) diff --git a/backend/tests/test_reader_service.py b/backend/tests/test_reader_service.py index 63e4c0b8..fa8b540e 100644 --- a/backend/tests/test_reader_service.py +++ b/backend/tests/test_reader_service.py @@ -210,9 +210,8 @@ def test_an_add_is_idempotent_and_lands_where_asked(db, make_user): pl = service.create_playlist(db, user, "L") assert service.add_item(db, pl, "a", mark_dirty=False) is True assert service.add_item(db, pl, "a", mark_dirty=False) is False - service.add_to_playlist(db, pl, "b", None) - service.add_to_playlist(db, pl, "c", 0) - service.add_to_playlist(db, pl, "a", 2) # already there: stays put + assert service.add_item(db, pl, "b", mark_dirty=True) is True + assert service.restore_items(db, pl, ["c", "a"], [0, 2]) == 1 # "a" is there: it stays put assert _uncommitted(db) db.commit() assert _ids(db, pl) == ["c", "a", "b"] @@ -235,14 +234,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): diff --git a/backend/tests/test_route_commits.py b/backend/tests/test_route_commits.py index 6491fec5..8f83c3e0 100644 --- a/backend/tests/test_route_commits.py +++ b/backend/tests/test_route_commits.py @@ -310,6 +310,14 @@ def test_clearing_leaves_a_video_the_caller_never_found(db, make_user, as_user): assert r.json() == {"ok": True, "removed": 0} +def test_clearing_refuses_a_body_without_its_list(db, make_user, as_user): + # A misnamed key read as an empty list: 200, and nothing cleared. + user = make_user() + r = as_user(user).post("/api/search/clear", json={"ids": ["s1"]}) + assert r.status_code == 422 + assert as_user(user).post("/api/search/clear", json={"video_ids": []}).status_code == 200 + + # --- Plex show and season marks --------------------------------------------------------------- diff --git a/docs/DESIGN.md b/docs/DESIGN.md index c173a62e..281e8f77 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -536,6 +536,26 @@ of a full-width panel and nothing on the right. shows them as four buttons with the item's place beside the selected view; a side panel in edit mode gives each group an Up / Down pair (`ui/OrderButtons`, `compact`). A move that would go nowhere is shown off, not hidden. A grip also picks up from the keyboard (space, the arrows, space). +- **Selecting rows:** a list whose rows take bulk actions shows a box first in every row, always, and one + over the column for the rows shown. Shift+click selects a range, `x` toggles the row under the keyboard, + Esc clears. A selected row wears the accent tint as well as its tick. An action covers the selected rows + shown; a row a filter hides stays selected and comes back ticked; another list starts over. The row's own + menu still acts on that row alone. While anything is selected the toolbar's second row is the action band: + the count (and how many a filter hides), the last move's target as one click, Move to, Remove, Clear + selection. An action clears the selection, undoes by its toast and, when it fails, ticks its rows again. + An Undo brings back what is still there to bring: a video the move put in the target and the target no + longer holds stays where it went, and a warning says how many came back. One the target held already only + left the source, so it returns there. + Delete takes the selection when there is one, else the row under the keyboard, with no confirm. With every + tick hidden by a filter it removes nothing and says so. An Esc during a drag puts the drag down, and only that. + Below `xl` the band's buttons are glyphs named in their tooltips, and a filter's hidden count is "+N" with + its sentence in the tooltip. The row the band replaces keeps its height, invisible, so a tick moves no row. + On a phone the count is its number (the words read out, and in the tooltip) and the undo pair stays out. + A row's grip also carries it onto a list in the rail, under a sort or a filter too (the grip then says + so, and no key lifts it); a selected row carries every selected one, named on the drag image below the + pointer. The target row lights up (`ring-accent`); the open list is none; an unpinned rail peeks by the + drag's own hover and scrolls at its edges. Dropped inside the list, one row takes its new place and many + stay. A key never leaves the list and moves its own row: the band and the row's menu are the other way. - **Motion:** 150–300ms micro-interactions, transform/opacity only; `prefers-reduced-motion` is respected. [WCAG 2.3.3] @@ -833,4 +853,12 @@ of a full-width panel and nothing on the right. | 2026-11-01 | Toolbar group glyphs (owner D9 a, D10 a, D11 a) | The feed's, channel page's and Plex Browse's filter pills name their group by a glyph (`Eye` watch state, `Shapes` content types, `ArrowDownUp` sort), the word in the tooltip and the accessible name; Sort + View end the toolbar's row, the chip row shows only with a chip; a channel page has no count pill | The value alone; Sort + View on a row of their own | "Watch state:" and "Sort:" cost a tablet row its room; the value says enough once the glyph names the group; the channel header already counts the videos | | 2026-11-01 | Channel header on a tablet (owner E2) | Below `xl` the action row drops its two words: "Prio" (the stepper's tooltip names it) and "Subscribed" (a ✓ glyph; its name in the aria-label, Unsubscribe in the tooltip); the tags keep the row's rest, folding into "+N" | The tag lane on a full line of its own below ~12rem; the tags under the channel name | On an iPad (820 px, 44 px targets) the lane got 29 px, a sliver; now 130 px, and every tag shows at 1180 | | 2026-11-01 | Clear watch progress (owner E a) | A video menu's Reset reads "Clear watch progress" in English (HU "Megtekintés visszaállítása" stays); "Reset view" is only a page's view reset (sort, filters, grouping) | "Reset view" for both; the playlist page's view reset renamed instead | On a playlist page the two stood one right-click apart under one name, and one of them erases the video's progress with no undo | -| 2026-11-01 | Clear watch progress clears only that (owner A) | The row shows only on a video with a resume point and clears that alone: a hidden video stays hidden, a watched one watched. Un-hiding and un-watching have rows of their own | +| 2026-11-01 | Clear watch progress clears only that (owner A) | The row shows only on a video with a resume point and clears that alone: a hidden video stays hidden, a watched one watched. Un-hiding and un-watching have rows of their own | The old Reset, which also un-hid and un-watched | A hidden video stays hidden: what the row's name says is all it does | +| 2026-11-01 | Selecting a list's rows (owner D2 a) | An always visible box first in the row and one over the column for the rows shown; Shift+click a range, `x` the row under the keyboard, Esc clears; the selected row wears `accent/12` with its tick. Rows a filter hides stay selected, an action covers those shown; Watch later offers no selecting. | A "Select" button that shows the boxes on demand | Found at once and works on touch, with no mode to leave; costs about 2 rem a row | +| 2026-11-01 | Selection band (owner D1 a, D3 a, D4 a) | The toolbar's second row while rows are selected, no floating bar: move (the last target as one click) and remove, no copy; both undo back to the places, a video the target held before stays there; a single row's move undoes too. | A floating bar at the bottom; copying to a list too; no undo on a move | It covers no row, and the player and the notices live at the bottom; twenty videos sent to the wrong list need a way back to their places | +| 2026-11-01 | Grip on a phone (owner A) | Below a 24rem table a playlist row has no grip: the title keeps its width, and the row menu keeps the moves. | The grip and its drag kept, the title at 44 px | The title gets its width back, and the rail is shut on a phone, so a drag had nowhere to go | +| 2026-11-01 | Selection band on a tablet (owner A) | Below `xl` the band is glyphs on one row, the names in tooltip and aria-label, as on the channel header; the replaced row stays in the same grid cell, invisible, so the first tick moves no row under the second tap. | The buttons in words, wrapping to a second row | The first tick pushed the list from under the second tap | +| 2026-11-01 | Playlist heading on a narrow window (owner A) | The list's name had no width left: below `xl` Unsynced is its dot (the word read out, the hint in the tooltip); on a phone the cover is gone and Play all is its glyph, named in the tooltip. | Every word kept, the list's name at no width | A heading without its name says nothing | +| 2026-11-02 | Dragging onto a list (owner D5 a, D6 a) | The grip carries a row, or every selected row with a selected one, onto a list in the rail; the toast undoes. Many dropped inside the list stay; a coarse pointer's drawer never peeks, so its rail is a target only when pinned. | Only the grabbed row going along; the grip off under a view | One row moving out of three lit ones misleads, and a filter is how the rows to move are found | +| 2026-11-02 | Delete with every tick hidden (owner a) | It removes nothing and a warning says the filter hides them | Taking the row under the keyboard | While the band shows a selection, Delete means that selection: a row no one ticked must not go | +| 2026-11-02 | Undo of a video gone further (owner a) | A video the move put in the target comes back only if the target still holds it, and a warning says how many | Everything back, whatever it did since | A move skips a video that left the source; the same rule back keeps a video out of lists the reader never put it in | diff --git a/docs/gates.md b/docs/gates.md index b0e23f5f..f0cf46db 100644 --- a/docs/gates.md +++ b/docs/gates.md @@ -108,8 +108,8 @@ project checks needs a row. | Scope | Narrowed | Why | Measured | | --- | --- | --- | --- | -| check | e2e (Playwright) not in `check` | It needs the :8080 stack built from the tree (minutes). It runs in `check-full`, and `release` rebuilds the stack and requires it (E32 D2) — nothing ships without it. | 9 specs: 6 × 3 engines, 3 on chromium only (the row below) | -| e2e | `a11y.spec.ts` `member.spec.ts` `devices.spec.ts` run on chromium only (`CHROMIUM_ONLY`, the webkit and ipad projects' `testIgnore`) | What they hold does not change with the engine: the axe scan reads the DOM and the accessibility tree, which the engines build alike; the member spec checks which pages and requests a role gets; the devices spec opens its own desktop and iPad contexts. Run three times they would triple their time and catch nothing more. What an engine does change (layout, events) the other specs hold on all three. | 3 of 9 specs | +| check | e2e (Playwright) not in `check` | It needs the :8080 stack built from the tree (minutes). It runs in `check-full`, and `release` rebuilds the stack and requires it (E32 D2) — nothing ships without it. | 12 specs: 9 × 3 engines, 3 on chromium only (the row below) | +| e2e | `a11y.spec.ts` `member.spec.ts` `devices.spec.ts` run on chromium only (`CHROMIUM_ONLY`, the webkit and ipad projects' `testIgnore`) | What they hold does not change with the engine: the axe scan reads the DOM and the accessibility tree, which the engines build alike; the member spec checks which pages and requests a role gets; the devices spec opens its own desktop and iPad contexts. Run three times they would triple their time and catch nothing more. What an engine does change (layout, events) the other specs hold on all three. | 3 of 12 specs | | pushes | no server-side gate (no CI) | Owner decision 2026-10-04. The gate (`scripts/check.sh`) runs on the dev machine before work is handed over, and `release` runs it, then e2e, before anything is merged and Trivy before the image is pushed — the CI only repeated that, after the merge. Its runner shared the git host's container, and one run there starved Gitea of memory until it stopped answering. What it added — a fresh `npm ci` on Linux — the image build does at publish. | — | | coverage | not in the gate; no threshold | Owner decision E32 D4: never enforced; "every regression test seen red" is the bar, and a percentage invites tests written to move it. Out of the lanes since E35: read once, by hand, it found 5 bugs; as a lane it was a number nobody acted on, and the slowest vitest run (the one that flaked). On demand: docs/development.md. | frontend 80% / backend 78% of lines (E32 S1) | | CVE gate | HIGH/CRITICAL with a fix available only | `--ignore-unfixed` + `--severity HIGH,CRITICAL`: a finding with no fix cannot be acted on by an upgrade; it shows in the scan output. A fixable one fails the publish, and the only way past it is `.trivyignore.yaml`. | 0 open (E32 S1) | diff --git a/frontend/e2e/playlists.spec.ts b/frontend/e2e/playlists.spec.ts new file mode 100644 index 00000000..08cc28c9 --- /dev/null +++ b/frontend/e2e/playlists.spec.ts @@ -0,0 +1,313 @@ +import { test, expect, type Locator, type Page } from "@playwright/test"; +import { openApp } from "./helpers"; + +// A video dragged onto a list in the rail, with a real pointer: the rail and the page are separate +// React trees, and jsdom lays out nothing to drag across. The browser projects share one account, so +// each test holds lists of its own, named for it, and takes them away after. + +interface Listed { + id: number; + items: { id: string; title: string }[]; +} + +const made: number[] = []; + +async function newList(page: Page, name: string, videos: string[] = []): Promise { + const res = await page.request.post("/api/playlists", { data: { name } }); + const { id } = (await res.json()) as { id: number }; + made.push(id); + for (const video_id of videos) { + const added = await page.request.post(`/api/playlists/${String(id)}/items`, { + data: { video_id }, + }); + expect(added.ok()).toBe(true); + } + return id; +} + +const contents = async (page: Page, id: number) => + ((await (await page.request.get(`/api/playlists/${String(id)}`)).json()) as Listed).items.map( + (v) => v.id, + ); + +async function setUp(page: Page, name: string, project: string) { + await pinOnTouch(page, project); + const feed = (await (await page.request.get("/api/feed?limit=3")).json()) as { + items: { id: string; title: string }[]; + }; + expect(feed.items.length).toBe(3); + const videos = feed.items; + const from = await newList( + page, + `${name} from`, + videos.map((v) => v.id), + ); + const to = await newList(page, `${name} to`); + await openApp(page, `/?page=playlists&pl=${String(from)}`); + await expect(page.getByRole("heading", { name: `${name} from` })).toBeVisible(); + await expect(page.locator("tbody tr")).toHaveCount(3); + return { videos, from, to, target: page.locator(`[data-playlist-drop="${String(to)}"]`) }; +} + +const centre = async (el: Locator) => { + const box = await el.boundingBox(); + if (!box) throw new Error("nothing to point at"); + return { x: box.x + box.width / 2, y: box.y + box.height / 2 }; +}; + +/** Pick the row up by its grip and carry it to a point, the button still held. */ +async function carry(page: Page, grip: Locator, to: { x: number; y: number }) { + const from = await centre(grip); + await page.mouse.move(from.x, from.y); + await page.mouse.down(); + await page.mouse.move(from.x + 8, from.y + 8, { steps: 3 }); + await page.mouse.move(to.x, to.y, { steps: 12 }); +} + +/** The same onto the rail. An unpinned one is a tab until the drag's own hover peeks it open. */ +async function carryToRail(page: Page, grip: Locator, target: Locator) { + if (await target.isVisible()) { + await carry(page, grip, await centre(target)); + return; + } + const tab = page.getByRole("button", { name: "Pin Playlists open" }); + await carry(page, grip, await centre(tab)); + await expect(target).toBeVisible(); + await page.mouse.move(...(Object.values(await centre(target)) as [number, number]), { steps: 6 }); +} + +/** What dnd-kit tells a screen reader: said once the drag has taken the step in, so a drop after it + * is the one a person makes, not one that beats the row to its new place. */ +const said = (page: Page, text: RegExp) => + expect(page.locator('[id^="DndLiveRegion"]').filter({ hasText: text })).toBeAttached(); + +/** A coarse pointer's rail never peeks, a drawer instead: on the tablet it is pinned, for these. */ +async function pinOnTouch(page: Page, project: string) { + if (project !== "ipad") return; + await page.route("**/api/me", async (route) => { + const res = await route.fetch(); + const me = (await res.json()) as { preferences?: Record }; + await route.fulfill({ + response: res, + json: { ...me, preferences: { ...me.preferences, playlistsCollapsed: false } }, + }); + }); +} + +test.describe("acting on the selected rows", { tag: "@playlists" }, () => { + test.afterEach(async ({ page }) => { + for (const id of made.splice(0)) await page.request.delete(`/api/playlists/${String(id)}`); + }); + + const box = (page: Page, title: string) => + page.getByRole("checkbox", { name: `Select “${title}”` }); + + test("a Shift range, the band's Remove, and its Undo back to the places", async ({ + page, + }, info) => { + const { videos, from } = await setUp(page, `band ${info.project.name}`, info.project.name); + await box(page, videos[0]!.title).click(); + await box(page, videos[1]!.title).click({ modifiers: ["Shift"] }); + await expect(box(page, videos[1]!.title)).toBeChecked(); + await expect(box(page, videos[2]!.title)).not.toBeChecked(); + await page.getByRole("button", { name: "Remove", exact: true }).click(); + await expect(page.locator("tbody tr")).toHaveCount(1); + await expect.poll(() => contents(page, from)).toEqual([videos[2]!.id]); + await page.getByRole("button", { name: "Undo", exact: true }).last().click(); + await expect.poll(() => contents(page, from)).toEqual(videos.map((v) => v.id)); + await expect(page.locator("tbody tr")).toHaveCount(3); + }); + + test("x ticks the walked row, Esc clears, and Delete takes the selection and goes on", async ({ + page, + }, info) => { + const { videos, from } = await setUp(page, `keys ${info.project.name}`, info.project.name); + const firstRow = page.locator("tbody tr").first(); + await page.keyboard.press("j"); + await expect(firstRow).toBeFocused(); + await page.keyboard.press("x"); + await expect(box(page, videos[0]!.title)).toBeChecked(); + await page.keyboard.press("Escape"); + await expect(box(page, videos[0]!.title)).not.toBeChecked(); + await page.keyboard.press("x"); + await page.keyboard.press("j"); + await page.keyboard.press("x"); + await page.keyboard.press("Delete"); + await expect.poll(() => contents(page, from)).toEqual([videos[2]!.id]); + await expect(page.locator("tbody tr")).toHaveCount(1); + await expect(firstRow).toBeFocused(); + }); +}); + +test.describe("dragging onto a list", { tag: "@playlists" }, () => { + test.afterEach(async ({ page }) => { + await page.mouse.up(); + for (const id of made.splice(0)) await page.request.delete(`/api/playlists/${String(id)}`); + }); + + test("a row leaves for the list it is dropped on, lit up on the way, and Undo brings it back", async ({ + page, + }, info) => { + const { videos, from, to, target } = await setUp( + page, + `drag one ${info.project.name}`, + info.project.name, + ); + const grip = page.getByRole("button", { name: `Reorder “${videos[1]!.title}”`, exact: false }); + await carryToRail(page, grip, target); + await expect(target).toHaveClass(/ring-2/); + await page.mouse.up(); + await expect(target).not.toHaveClass(/ring-2/); + await expect(page.locator("tbody tr")).toHaveCount(2); + // The grip that held the focus has left: the keyboard keeps its place in the list. + await expect + .poll(() => page.evaluate(() => document.activeElement?.closest("tbody") != null)) + .toBe(true); + await expect.poll(() => contents(page, to)).toEqual([videos[1]!.id]); + expect(await contents(page, from)).toEqual([videos[0]!.id, videos[2]!.id]); + await page.getByRole("button", { name: "Undo", exact: true }).last().click(); + await expect.poll(() => contents(page, from)).toEqual(videos.map((v) => v.id)); + expect(await contents(page, to)).toEqual([]); + }); + + test("a selected row carries the selected ones, named on the drag image", async ({ + page, + }, info) => { + const { videos, from, to, target } = await setUp( + page, + `drag many ${info.project.name}`, + info.project.name, + ); + await page.getByRole("checkbox", { name: videos[0]!.title, exact: false }).click(); + await page.getByRole("checkbox", { name: videos[2]!.title, exact: false }).click(); + const grip = page.getByRole("button", { name: `Reorder “${videos[2]!.title}”`, exact: false }); + await carryToRail(page, grip, target); + const image = page.getByText("2 videos", { exact: true }); + await expect(image).toBeVisible(); + // Above every layer, by the tooltip's token: the overlay wrapping it takes the level. + expect(await image.evaluate((el) => getComputedStyle(el.parentElement!).zIndex)).toBe("100"); + // Below the pointer, so it leaves the list's name to read. + const { y } = await centre(target); + expect((await image.boundingBox())!.y).toBeGreaterThan(y); + await page.mouse.up(); + await expect.poll(() => contents(page, to)).toEqual([videos[0]!.id, videos[2]!.id]); + expect(await contents(page, from)).toEqual([videos[1]!.id]); + await expect(page.getByRole("checkbox", { name: "Select all" })).not.toBeChecked(); + }); + + test("many rows dropped inside the list stay as they were", async ({ page }, info) => { + const { videos, from } = await setUp( + page, + `drag inside ${info.project.name}`, + info.project.name, + ); + await page.getByRole("checkbox", { name: videos[0]!.title, exact: false }).click(); + await page.getByRole("checkbox", { name: videos[1]!.title, exact: false }).click(); + const grip = page.getByRole("button", { name: `Reorder “${videos[0]!.title}”`, exact: false }); + await carry(page, grip, await centre(page.locator("tbody tr").nth(2))); + await page.mouse.up(); + await expect(page.locator("tbody tr")).toHaveCount(3); + expect(await contents(page, from)).toEqual(videos.map((v) => v.id)); + }); + + test("one row dropped inside the list takes the new place", async ({ page }, info) => { + const { videos, from } = await setUp( + page, + `drag order ${info.project.name}`, + info.project.name, + ); + const grip = page.getByRole("button", { name: `Reorder “${videos[0]!.title}”`, exact: false }); + await carry(page, grip, await centre(page.locator("tbody tr").nth(2))); + await said(page, /position 3/); + await page.mouse.up(); + await expect + .poll(() => contents(page, from)) + .toEqual([videos[1]!.id, videos[2]!.id, videos[0]!.id]); + }); + + test("Escape puts the drag down, with nothing moved and nothing lit", async ({ page }, info) => { + const { videos, from, to, target } = await setUp( + page, + `drag cancel ${info.project.name}`, + info.project.name, + ); + // A tick on another row: the Esc is the drag's, and leaves the selection alone. + const ticked = page.getByRole("checkbox", { name: `Select “${videos[2]!.title}”` }); + await ticked.click(); + const grip = page.getByRole("button", { name: `Reorder “${videos[0]!.title}”`, exact: false }); + await carryToRail(page, grip, target); + await expect(target).toHaveClass(/ring-2/); + await page.keyboard.press("Escape"); + await expect(target).not.toHaveClass(/ring-2/); + await page.mouse.up(); + expect(await contents(page, from)).toEqual(videos.map((v) => v.id)); + expect(await contents(page, to)).toEqual([]); + await expect(ticked).toBeChecked(); + }); + + test("a key still reorders a row, alone", async ({ page }, info) => { + const { videos, from } = await setUp(page, `drag key ${info.project.name}`, info.project.name); + await page.getByRole("checkbox", { name: videos[0]!.title, exact: false }).click(); + await page.getByRole("checkbox", { name: videos[2]!.title, exact: false }).click(); + await page.getByRole("button", { name: `Reorder “${videos[0]!.title}”`, exact: false }).focus(); + await page.keyboard.press("Space"); + await said(page, /picked up/i); + // A person's pause: dnd-kit drops an arrow pressed before it has measured the lifted row, which + // nothing on the page announces (11 of 31 runs without it, none of 31 with it). + await page.waitForTimeout(300); + await page.keyboard.press("ArrowDown"); + await said(page, /position 2/); + await page.keyboard.press("Space"); + await expect + .poll(() => contents(page, from)) + .toEqual([videos[1]!.id, videos[0]!.id, videos[2]!.id]); + }); + + test("under a filter a row still goes to another list, and none is reordered", async ({ + page, + }, info) => { + const { videos, from, to, target } = await setUp( + page, + `drag filter ${info.project.name}`, + info.project.name, + ); + await page.getByRole("columnheader", { name: "Title" }).getByRole("button").last().click(); + await page.getByPlaceholder("Title").fill(videos[1]!.title); + await page.keyboard.press("Escape"); + await expect(page.locator("tbody tr")).toHaveCount(1); + const grip = page.getByRole("button", { + name: `Drag “${videos[1]!.title}” onto a list`, + exact: false, + }); + await carryToRail(page, grip, target); + await page.mouse.up(); + await expect.poll(() => contents(page, to)).toEqual([videos[1]!.id]); + expect(await contents(page, from)).toEqual([videos[0]!.id, videos[2]!.id]); + }); + + test("the rail scrolls while the drag holds at its edge", async ({ page }, info) => { + const name = `drag scroll ${info.project.name}`; + for (let i = 0; i < 12; i++) await newList(page, `${name} ${String(i)}`); + await page.setViewportSize({ width: 1280, height: 520 }); + const { videos } = await setUp(page, name, info.project.name); + const grip = page.getByRole("button", { name: `Reorder “${videos[0]!.title}”`, exact: false }); + const area = page.locator("[data-playlist-drop-area]"); + await carryToRail(page, grip, area.locator("[data-playlist-drop]").first()); + const scroller = () => + area.evaluate((el) => { + for (let p = el.parentElement; p; p = p.parentElement) { + const { overflowY } = getComputedStyle(p); + if ((overflowY === "auto" || overflowY === "scroll") && p.scrollHeight > p.clientHeight) { + const { left, right, bottom } = p.getBoundingClientRect(); + return { top: p.scrollTop, x: (left + right) / 2, bottom }; + } + } + return { top: -1, x: 0, bottom: 0 }; + }); + const before = await scroller(); + expect(before.top).toBeGreaterThanOrEqual(0); + await page.mouse.move(before.x, before.bottom - 8, { steps: 4 }); + await expect.poll(async () => (await scroller()).top).toBeGreaterThan(before.top); + await page.keyboard.press("Escape"); + }); +}); diff --git a/frontend/src/components/ui/NativeControls.tsx b/frontend/src/components/ui/NativeControls.tsx index bc2db63b..07e234c3 100644 --- a/frontend/src/components/ui/NativeControls.tsx +++ b/frontend/src/components/ui/NativeControls.tsx @@ -1,5 +1,5 @@ import clsx from "clsx"; -import type { InputHTMLAttributes, ReactNode, Ref } from "react"; +import type { InputHTMLAttributes, ReactNode, RefObject } from "react"; // A6 (docs/DESIGN.md): checkbox, slider and colour well are the BROWSER's controls, tinted // with the chosen colour (the accent; the media amber on the player chrome) — keyboard, screen @@ -10,18 +10,30 @@ import type { InputHTMLAttributes, ReactNode, Ref } from "react"; type Native = Omit, "type" | "onChange">; -/** The bare checkbox: inside your own `