move_item checked "already in the target?" and then re-parented without a lock, so a second tab's add or the scheduler's YouTube merge committing the same video in between hit uq_playlist_video and answered a raw 500. The re-parent now runs in a savepoint and a conflict falls back to the already-present outcome. The target is touched only when the row landed in it (an untouched list was promoted in the rail's "recently changed" sort), and _touch stamps the database clock like the mixin's own onupdate. The canonical-patterns table names owned_or_404 against ad-hoc ownership helpers; six were left (_own_playlist, _own_playlist_or_404, _own_view, _own_tag, notifications._owned, downloads._own_job), two of them hand-rolled with a bare English 404. All call sites now use owned_or_404, and the download/link/notification 404s are structured like the rest. The contract test reads every "what is gone" the routes name, and the YouTube refusal reasons, and requires both languages to name each.
291 lines
11 KiB
Python
291 lines
11 KiB
Python
"""E10 S2 — the one-step "move this video to another list" endpoint, and the `updated_at` touch
|
|
the rail's "recently changed" sort reads.
|
|
|
|
Two properties are load-bearing and neither is visible from the happy path alone:
|
|
|
|
1. **The move is one transaction.** The client-side shape it replaces (add to the target, then
|
|
remove from the source) leaves the video in BOTH lists when the second call fails. So the
|
|
tests assert the *pair* of lists after every call, never just the destination.
|
|
2. **`updated_at` tracks item edits.** `UpdatedAtMixin.onupdate` only fires on an UPDATE of the
|
|
playlist row, and add/remove/reorder/move write `playlist_items` — so the timestamp is bumped
|
|
explicitly, and a regression there would silently degrade the sort to "renames only".
|
|
"""
|
|
|
|
from datetime import datetime, timedelta, timezone
|
|
|
|
from app.models import Channel, Playlist, PlaylistItem, Video
|
|
|
|
|
|
def _seed_videos(db, ids):
|
|
db.add(Channel(id="UCtest", title="Test"))
|
|
now = datetime.now(timezone.utc)
|
|
for v in ids:
|
|
db.add(Video(id=v, channel_id="UCtest", published_at=now))
|
|
db.commit()
|
|
|
|
|
|
def _playlist(db, user, name, video_ids, **kw):
|
|
pl = Playlist(user_id=user.id, name=name, **kw)
|
|
db.add(pl)
|
|
db.flush()
|
|
for i, vid in enumerate(video_ids):
|
|
db.add(PlaylistItem(playlist_id=pl.id, video_id=vid, position=i))
|
|
db.commit()
|
|
return pl
|
|
|
|
|
|
def _ids(db, pl_id):
|
|
return [
|
|
it.video_id
|
|
for it in db.execute(
|
|
PlaylistItem.__table__.select()
|
|
.where(PlaylistItem.playlist_id == pl_id)
|
|
.order_by(PlaylistItem.position)
|
|
).all()
|
|
]
|
|
|
|
|
|
def _move(client, src, video_id, dst):
|
|
return client.post(
|
|
f"/api/playlists/{src}/items/{video_id}/move", json={"target_playlist_id": dst}
|
|
)
|
|
|
|
|
|
def test_move_leaves_the_video_in_exactly_one_list(db, make_user, as_user):
|
|
user = make_user()
|
|
_seed_videos(db, ["v1", "v2"])
|
|
src = _playlist(db, user, "Source", ["v1", "v2"])
|
|
dst = _playlist(db, user, "Target", [])
|
|
|
|
r = _move(as_user(user), src.id, "v1", dst.id)
|
|
assert r.status_code == 200
|
|
assert r.json() == {
|
|
"ok": True,
|
|
"from_playlist_id": src.id,
|
|
"to_playlist_id": dst.id,
|
|
"video_id": "v1",
|
|
}
|
|
db.expire_all()
|
|
assert _ids(db, src.id) == ["v2"]
|
|
assert _ids(db, dst.id) == ["v1"]
|
|
|
|
|
|
def test_move_appends_to_the_end_of_the_target(db, make_user, as_user):
|
|
user = make_user()
|
|
_seed_videos(db, ["v1", "v2", "v3"])
|
|
src = _playlist(db, user, "Source", ["v3"])
|
|
dst = _playlist(db, user, "Target", ["v1", "v2"])
|
|
|
|
assert _move(as_user(user), src.id, "v3", dst.id).status_code == 200
|
|
db.expire_all()
|
|
assert _ids(db, dst.id) == ["v1", "v2", "v3"]
|
|
|
|
|
|
def test_move_into_a_list_that_already_has_it_is_a_removal_not_a_duplicate(db, make_user, as_user):
|
|
user = make_user()
|
|
_seed_videos(db, ["v1"])
|
|
src = _playlist(db, user, "Source", ["v1"])
|
|
dst = _playlist(db, user, "Target", ["v1"])
|
|
|
|
assert _move(as_user(user), src.id, "v1", dst.id).status_code == 200
|
|
db.expire_all()
|
|
assert _ids(db, src.id) == []
|
|
assert _ids(db, dst.id) == ["v1"] # exactly one row, not two
|
|
|
|
|
|
def test_move_to_the_same_list_is_refused(db, make_user, as_user):
|
|
user = make_user()
|
|
_seed_videos(db, ["v1"])
|
|
src = _playlist(db, user, "Source", ["v1"])
|
|
|
|
r = _move(as_user(user), src.id, "v1", src.id)
|
|
assert r.status_code == 400
|
|
assert r.json()["detail"] == {"code": "playlist_move", "reason": "same_playlist"}
|
|
db.expire_all()
|
|
assert _ids(db, src.id) == ["v1"]
|
|
|
|
|
|
def test_move_of_a_video_that_left_the_source_reports_it(db, make_user, as_user):
|
|
user = make_user()
|
|
_seed_videos(db, ["v1"])
|
|
src = _playlist(db, user, "Source", [])
|
|
dst = _playlist(db, user, "Target", [])
|
|
|
|
r = _move(as_user(user), src.id, "v1", dst.id)
|
|
assert r.status_code == 404
|
|
assert r.json()["detail"] == {"code": "playlist_move", "reason": "not_in_playlist"}
|
|
|
|
|
|
def test_move_into_someone_elses_list_404s_and_changes_nothing(db, make_user, as_user):
|
|
user, other = make_user(), make_user()
|
|
_seed_videos(db, ["v1"])
|
|
src = _playlist(db, user, "Mine", ["v1"])
|
|
theirs = _playlist(db, other, "Theirs", [])
|
|
|
|
assert _move(as_user(user), src.id, "v1", theirs.id).status_code == 404
|
|
db.expire_all()
|
|
assert _ids(db, src.id) == ["v1"]
|
|
assert _ids(db, theirs.id) == []
|
|
|
|
|
|
def test_a_list_that_is_not_yours_answers_with_a_reason_the_ui_can_translate(
|
|
db, make_user, as_user
|
|
):
|
|
# The one 404 a user meets in practice (the list went away in another tab). It used to be a bare
|
|
# English "Unknown playlist", which the Hungarian UI then showed verbatim.
|
|
user, other = make_user(), make_user()
|
|
_seed_videos(db, ["v1"])
|
|
theirs = _playlist(db, other, "Theirs", ["v1"])
|
|
|
|
r = as_user(user).delete(f"/api/playlists/{theirs.id}/items/v1")
|
|
assert r.status_code == 404
|
|
assert r.json()["detail"] == {"code": "not_found", "reason": "playlist"}
|
|
db.expire_all()
|
|
assert _ids(db, theirs.id) == ["v1"]
|
|
|
|
|
|
def test_a_move_into_a_list_that_is_gone_names_the_target_not_the_list_on_screen(
|
|
db, make_user, as_user
|
|
):
|
|
# Both lists 404'd with the same reason, which the UI reads as "THIS playlist doesn't exist any
|
|
# more" — said about the source the reader is looking at, which is still there.
|
|
user = make_user()
|
|
_seed_videos(db, ["v1"])
|
|
src = _playlist(db, user, "Mine", ["v1"])
|
|
gone = _playlist(db, user, "Gone", [])
|
|
gone_id = gone.id
|
|
db.delete(gone)
|
|
db.commit()
|
|
|
|
r = _move(as_user(user), src.id, "v1", gone_id)
|
|
assert r.status_code == 404
|
|
assert r.json()["detail"] == {"code": "not_found", "reason": "move_target"}
|
|
db.expire_all()
|
|
assert _ids(db, src.id) == ["v1"]
|
|
|
|
|
|
def test_a_refused_playlist_action_answers_with_a_reason_the_ui_can_translate(
|
|
db, make_user, as_user
|
|
):
|
|
# The rest of the router's refusals were bare English too ("The Watch later list can't be
|
|
# deleted.", "YouTube delete failed.", ...), shown verbatim under the Hungarian UI.
|
|
user = make_user()
|
|
client = as_user(user)
|
|
wl = client.post("/api/playlists/watch-later", json={"video_id": "nope"})
|
|
assert wl.status_code == 404
|
|
assert wl.json()["detail"] == {"code": "not_found", "reason": "video"}
|
|
_seed_videos(db, ["v1"])
|
|
saved = client.post("/api/playlists/watch-later", json={"video_id": "v1"})
|
|
wl_id = saved.json()["playlist_id"]
|
|
r = client.delete(f"/api/playlists/{wl_id}")
|
|
assert r.status_code == 400
|
|
assert r.json()["detail"] == {"code": "playlist", "reason": "watch_later_undeletable"}
|
|
|
|
|
|
def test_move_marks_both_linked_sides_dirty(db, make_user, as_user):
|
|
"""A move edits two YouTube mirrors at once — the source lost an item, the target gained one.
|
|
Marking only the source is the asymmetric fix this repo keeps re-learning."""
|
|
user = make_user()
|
|
_seed_videos(db, ["v1"])
|
|
src = _playlist(db, user, "Src", ["v1"], yt_playlist_id="PLsrc", synced_video_ids='["v1"]')
|
|
dst = _playlist(db, user, "Dst", [], yt_playlist_id="PLdst", synced_video_ids="[]")
|
|
# Baseline: both are in sync with YouTube before the move.
|
|
src.dirty = False
|
|
dst.dirty = False
|
|
db.commit()
|
|
|
|
assert _move(as_user(user), src.id, "v1", dst.id).status_code == 200
|
|
db.expire_all()
|
|
assert db.get(Playlist, src.id).dirty is True
|
|
assert db.get(Playlist, dst.id).dirty is True
|
|
|
|
|
|
def test_item_edits_bump_updated_at(db, make_user, as_user):
|
|
"""The rail's "recently changed first" sort reads this. Adding, moving and reordering all
|
|
write `playlist_items` only, so nothing here is covered by the ORM's `onupdate`."""
|
|
user = make_user()
|
|
_seed_videos(db, ["v1", "v2"])
|
|
src = _playlist(db, user, "Source", ["v1", "v2"])
|
|
dst = _playlist(db, user, "Target", [])
|
|
old = datetime.now(timezone.utc) - timedelta(days=3)
|
|
src.updated_at = old
|
|
dst.updated_at = old
|
|
db.commit()
|
|
client = as_user(user)
|
|
|
|
assert client.post(f"/api/playlists/{dst.id}/items", json={"video_id": "v1"}).status_code == 200
|
|
db.expire_all()
|
|
assert db.get(Playlist, dst.id).updated_at > old
|
|
|
|
dst.updated_at = old
|
|
db.commit()
|
|
assert (
|
|
client.put(f"/api/playlists/{src.id}/order", json={"video_ids": ["v2", "v1"]}).status_code
|
|
== 200
|
|
)
|
|
db.expire_all()
|
|
assert db.get(Playlist, src.id).updated_at > old
|
|
|
|
src.updated_at = old
|
|
dst.updated_at = old
|
|
db.commit()
|
|
assert _move(client, src.id, "v2", dst.id).status_code == 200
|
|
db.expire_all()
|
|
# BOTH sides of a move are "recently changed" — the target is where the reader will look.
|
|
assert db.get(Playlist, src.id).updated_at > old
|
|
assert db.get(Playlist, dst.id).updated_at > old
|
|
|
|
|
|
def test_the_list_endpoint_publishes_updated_at(db, make_user, as_user):
|
|
user = make_user()
|
|
_playlist(db, user, "Solo", [])
|
|
rows = as_user(user).get("/api/playlists").json()
|
|
assert len(rows) == 1
|
|
assert rows[0]["updated_at"] # the rail cannot sort by a field the API never sends
|
|
|
|
|
|
def test_a_target_that_already_had_it_is_not_recently_changed(db, make_user, as_user):
|
|
"""Moving into a list that already holds the video only removes it from the source: the
|
|
target did not change, and bumping it promoted an untouched list in the rail's sort."""
|
|
user = make_user()
|
|
_seed_videos(db, ["v1"])
|
|
src = _playlist(db, user, "Source", ["v1"])
|
|
dst = _playlist(db, user, "Target", ["v1"])
|
|
old = datetime.now(timezone.utc) - timedelta(days=3)
|
|
src.updated_at = old
|
|
dst.updated_at = old
|
|
db.commit()
|
|
|
|
assert _move(as_user(user), src.id, "v1", dst.id).status_code == 200
|
|
db.expire_all()
|
|
assert db.get(Playlist, src.id).updated_at > old
|
|
assert db.get(Playlist, dst.id).updated_at == old
|
|
|
|
|
|
def test_a_concurrent_add_to_the_target_lands_as_the_already_present_move(
|
|
db, make_user, as_user, monkeypatch
|
|
):
|
|
"""The already-in-target check is a read, not a lock: another tab's add (or the scheduler's
|
|
YouTube merge) can commit the same video to the target in between, and the re-parent then
|
|
hits uq_playlist_video. That is the idempotent outcome arriving late — not a raw 500."""
|
|
from app.db import SessionLocal
|
|
from app.routes import playlists as plroutes
|
|
|
|
user = make_user()
|
|
_seed_videos(db, ["v1"])
|
|
src = _playlist(db, user, "Source", ["v1"])
|
|
dst = _playlist(db, user, "Target", [])
|
|
real = plroutes._next_item_position
|
|
|
|
def add_first(session, playlist_id):
|
|
with SessionLocal() as other: # the other tab, committing between check and re-parent
|
|
other.add(PlaylistItem(playlist_id=dst.id, video_id="v1", position=0))
|
|
other.commit()
|
|
return real(session, playlist_id)
|
|
|
|
monkeypatch.setattr(plroutes, "_next_item_position", add_first)
|
|
assert _move(as_user(user), src.id, "v1", dst.id).status_code == 200
|
|
db.expire_all()
|
|
assert _ids(db, src.id) == []
|
|
assert _ids(db, dst.id) == ["v1"]
|