From e74986cc9ad4e4d65faf61cc341fc17660b5b126 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sat, 19 Sep 2026 07:01:18 +0200 Subject: [PATCH] fix(playlists): a racing move is not a 500; ownership through owned_or_404 everywhere 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. --- backend/app/routes/downloads.py | 35 +++++----- backend/app/routes/notifications.py | 14 ++-- backend/app/routes/playlists.py | 64 +++++++++++-------- backend/app/routes/plex/playlists.py | 20 +++--- backend/app/routes/saved_views.py | 8 +-- backend/app/routes/tags.py | 11 ++-- backend/tests/test_playlist_move.py | 46 +++++++++++++ frontend/src/i18n/backendIds.contract.test.ts | 43 ++++++++++++- frontend/src/i18n/locales/en/errors.json | 5 +- frontend/src/i18n/locales/hu/errors.json | 5 +- 10 files changed, 166 insertions(+), 85 deletions(-) diff --git a/backend/app/routes/downloads.py b/backend/app/routes/downloads.py index 3077f192..ed479700 100644 --- a/backend/app/routes/downloads.py +++ b/backend/app/routes/downloads.py @@ -38,7 +38,7 @@ from app.models import ( MediaAsset, User, ) -from app.routes._common import get_or_404, not_found +from app.routes._common import get_or_404, not_found, owned_or_404 from app.security import hash_password from app.userscope import is_messageable_user, messageable_clauses from app.utils import iso @@ -166,15 +166,8 @@ def _serialize_job(db: Session, job: DownloadJob) -> dict: return _serialize(job, asset) -def _own_job(db: Session, user: User, job_id: int) -> DownloadJob: - job = db.get(DownloadJob, job_id) - if job is None or job.user_id != user.id: - raise HTTPException(status_code=404, detail="Unknown download") - return job - - def _own_job_for_update(db: Session, user: User, job_id: int) -> DownloadJob: - """`_own_job` for a transition that moves its asset's ref count (resume, cancel, delete, redownload) + """The ownership check (`owned_or_404`) for a transition that moves its asset's ref count (resume, cancel, delete, redownload) or writes the status those decide on (pause). The job is re-read under a row lock, so two overlapping requests for one job run one after the other and the second decides on the first's committed result — never both releasing the same hold, and never a pause turning a job a cancel @@ -183,7 +176,7 @@ def _own_job_for_update(db: Session, user: User, job_id: int) -> DownloadJob: The asset is locked BEFORE the job: the order a finishing worker child takes them in (`service.finalize_if_running`), so the two can never deadlock.""" while True: - asset_id = _own_job(db, user, job_id).asset_id + asset_id = owned_or_404(db, DownloadJob, job_id, user, what="download").asset_id if asset_id is not None: db.execute(select(MediaAsset.id).where(MediaAsset.id == asset_id).with_for_update()) job = db.execute( @@ -193,7 +186,7 @@ def _own_job_for_update(db: Session, user: User, job_id: int) -> DownloadJob: .execution_options(populate_existing=True) ).scalar_one_or_none() if job is None: - raise HTTPException(status_code=404, detail="Unknown download") + raise not_found("download") if job.asset_id == asset_id: return job db.rollback() # another request moved the job to another asset meanwhile: lock that one @@ -614,7 +607,7 @@ def update_download_meta( ) -> dict: """Update a download's display metadata: title, channel name + link, and extra reference URLs. All are display-only overrides — the on-disk file keeps its system-decided, id-bound name.""" - job = _own_job(db, user, job_id) + job = owned_or_404(db, DownloadJob, job_id, user, what="download") sent = body.model_fields_set if "display_name" in sent: job.display_name = ((body.display_name or "").strip())[:255] or None @@ -699,7 +692,7 @@ def delete_download( def _accessible_job(db: Session, user: User, job_id: int) -> DownloadJob: job = db.get(DownloadJob, job_id) if job is None: - raise HTTPException(status_code=404, detail="Unknown download") + raise not_found("download") if job.user_id == user.id: return job shared = db.execute( @@ -708,7 +701,7 @@ def _accessible_job(db: Session, user: User, job_id: int) -> DownloadJob: ) ).first() if shared is None: - raise HTTPException(status_code=404, detail="Unknown download") + raise not_found("download") return job @@ -820,7 +813,7 @@ def share_download( user: User = Depends(require_human), db: Session = Depends(get_db), ) -> dict: - job = _own_job(db, user, job_id) + job = owned_or_404(db, DownloadJob, job_id, user, what="download") if job.status != "done" or job.asset_id is None: raise HTTPException(status_code=409, detail="Only a finished download can be shared.") target = body.email.strip().lower() @@ -863,7 +856,9 @@ def unshare_download( user: User = Depends(require_human), db: Session = Depends(get_db), ) -> dict: - _own_job(db, user, job_id) # ownership guard (404s if not the caller's job) + owned_or_404( + db, DownloadJob, job_id, user, what="download" + ) # ownership guard (404s if not the caller's job) recipient = db.execute( select(User).where(func.lower(User.email) == email.strip().lower()) ).scalar_one_or_none() @@ -925,10 +920,10 @@ def share_recipients( def _own_link(db: Session, user: User, link_id: int) -> DownloadLink: link = db.get(DownloadLink, link_id) if link is None: - raise HTTPException(status_code=404, detail="Unknown link") + raise not_found("link") job = db.get(DownloadJob, link.job_id) if job is None or job.user_id != user.id: - raise HTTPException(status_code=404, detail="Unknown link") + raise not_found("link") return link @@ -944,7 +939,7 @@ def _link_expiry(days: int | None) -> datetime | None: def list_links( job_id: int, user: User = Depends(require_human), db: Session = Depends(get_db) ) -> list[dict]: - _own_job(db, user, job_id) + owned_or_404(db, DownloadJob, job_id, user, what="download") rows = ( db.execute( select(DownloadLink) @@ -975,7 +970,7 @@ def create_link( user: User = Depends(require_human), db: Session = Depends(get_db), ) -> dict: - job = _own_job(db, user, job_id) + job = owned_or_404(db, DownloadJob, job_id, user, what="download") if job.status != "done" or job.asset_id is None: raise HTTPException(status_code=409, detail="Only a finished download can be shared.") pw = body.password.strip() diff --git a/backend/app/routes/notifications.py b/backend/app/routes/notifications.py index f5d43aca..33fb61d4 100644 --- a/backend/app/routes/notifications.py +++ b/backend/app/routes/notifications.py @@ -5,7 +5,7 @@ All endpoints use `current_user` (not `require_human`): notifications and their state are personal UI state with no quota or YouTube involvement, so the shared demo account manages its own inbox normally.""" -from fastapi import APIRouter, Depends, HTTPException, Query +from fastapi import APIRouter, Depends, Query from sqlalchemy import func, select, update from sqlalchemy.orm import Session @@ -14,6 +14,7 @@ from app.db import get_db from app.models import Notification, User from app.notifications import trim_read from app.utils import iso +from app.routes._common import owned_or_404 router = APIRouter(prefix="/api/me/notifications", tags=["notifications"]) @@ -72,20 +73,13 @@ def unread_count(user: User = Depends(current_user), db: Session = Depends(get_d return {"count": count or 0} -def _owned(db: Session, notification_id: int, user: User) -> Notification: - n = db.get(Notification, notification_id) - if n is None or n.user_id != user.id: - raise HTTPException(status_code=404, detail="Notification not found") - return n - - @router.post("/{notification_id}/read") def mark_read( notification_id: int, user: User = Depends(current_user), db: Session = Depends(get_db), ) -> dict: - n = _owned(db, notification_id, user) + n = owned_or_404(db, Notification, notification_id, user, what="notification") n.read = True db.commit() trim_read(db, user.id) @@ -110,7 +104,7 @@ def dismiss( user: User = Depends(current_user), db: Session = Depends(get_db), ) -> dict: - n = _owned(db, notification_id, user) + n = owned_or_404(db, Notification, notification_id, user, what="notification") # Dismiss implies read (it no longer counts toward the unread badge). n.dismissed = True n.read = True diff --git a/backend/app/routes/playlists.py b/backend/app/routes/playlists.py index 718de2a9..646dcf3d 100644 --- a/backend/app/routes/playlists.py +++ b/backend/app/routes/playlists.py @@ -3,11 +3,10 @@ CRUD (create / rename / delete, add / remove / reorder items) plus YouTube two-w mirror the user's own YouTube playlists in (`/sync-youtube`) and push/revert local edits back out (`/{id}/push-plan`, `/{id}/revert-youtube`), behind the optional write scope.""" -from datetime import datetime, timezone - from fastapi import APIRouter, Depends, HTTPException from pydantic import BaseModel from sqlalchemy import func, select, and_ +from sqlalchemy.exc import IntegrityError from sqlalchemy.orm import Session, aliased from app import quota @@ -48,10 +47,6 @@ def _check_pull_cooldown(user: User) -> None: raise HTTPException(status_code=429, detail={"code": "sync_cooldown"}) -def _own_playlist(db: Session, user: User, playlist_id: int) -> Playlist: - return owned_or_404(db, Playlist, playlist_id, user, what="playlist") - - def _refused(status: int, reason: str) -> HTTPException: """A playlist action the server refuses, as a reason the UI translates (`playlists.errors.`) — a bare English detail reached the Hungarian UI verbatim.""" @@ -64,8 +59,11 @@ def _touch(pl: Playlist) -> None: `UpdatedAtMixin`'s `onupdate` fires on an UPDATE of the PLAYLIST row, and add / remove / reorder / move write the item rows instead — so without this the rail's "recently changed" sort would only ever see renames. The assignment is what makes the column mean "last time - this list changed", which is what the rail promises the reader.""" - pl.updated_at = datetime.now(timezone.utc) + this list changed", which is what the rail promises the reader. + + The DATABASE's clock, like the mixin's own `onupdate`: one sort key written from two clocks + (this process's and Postgres's) could order a rename and an item edit the wrong way round.""" + pl.updated_at = func.now() def _next_playlist_position(db: Session, user_id: int) -> int: @@ -339,7 +337,7 @@ def revert_youtube( """Reset one YouTube-linked playlist to its current YouTube state, discarding local edits (and clearing the dirty flag). The user-facing 'undo all my changes' for a list once the in-session undo history is gone.""" - pl = _own_playlist(db, user, playlist_id) + pl = owned_or_404(db, Playlist, playlist_id, user, what="playlist") if not pl.yt_playlist_id: raise _refused(400, "not_on_youtube") if not has_read_scope(user): @@ -356,7 +354,7 @@ def revert_youtube( def _pushable(db: Session, user: User, playlist_id: int) -> Playlist: """A local playlist the user may push to YouTube (create or update). The built-in Watch later and read-only YouTube mirrors are never pushable.""" - pl = _own_playlist(db, user, playlist_id) + pl = owned_or_404(db, Playlist, playlist_id, user, what="playlist") if pl.kind == "watch_later": raise _refused(400, "watch_later_not_pushable") return pl @@ -419,7 +417,7 @@ def get_playlist( user: User = Depends(current_user), db: Session = Depends(get_db), ) -> dict: - pl = _own_playlist(db, user, playlist_id) + pl = owned_or_404(db, Playlist, playlist_id, user, what="playlist") state = aliased(VideoState) rows = db.execute( select(*feed_columns(user, state)) @@ -451,7 +449,7 @@ def rename_playlist( user: User = Depends(current_user), db: Session = Depends(get_db), ) -> dict: - pl = _own_playlist(db, user, playlist_id) + pl = owned_or_404(db, Playlist, playlist_id, user, what="playlist") if "name" in body.model_fields_set: name = (body.name or "").strip() if not name: @@ -488,7 +486,7 @@ def delete_playlist( user: User = Depends(current_user), db: Session = Depends(get_db), ) -> dict: - pl = _own_playlist(db, user, playlist_id) + pl = owned_or_404(db, Playlist, playlist_id, user, what="playlist") if pl.kind == "watch_later": raise _refused(400, "watch_later_undeletable") if on_youtube: @@ -514,7 +512,7 @@ def add_item( user: User = Depends(current_user), db: Session = Depends(get_db), ) -> dict: - pl = _own_playlist(db, user, playlist_id) + pl = owned_or_404(db, Playlist, playlist_id, user, what="playlist") video_id = body.video_id get_or_404(db, Video, video_id, what="video") _add_item(db, pl, video_id, mark_dirty=True) @@ -528,7 +526,7 @@ def remove_item( user: User = Depends(current_user), db: Session = Depends(get_db), ) -> dict: - pl = _own_playlist(db, user, playlist_id) + pl = owned_or_404(db, Playlist, playlist_id, user, what="playlist") _remove_item(db, pl, video_id, mark_dirty=True) return {"playlist_id": pl.id, "video_id": video_id} @@ -552,7 +550,7 @@ def move_item( exactly the kind of half-applied state this codebase treats as a bug rather than an edge case. Idempotent: if the video is already in the target, the source row is simply dropped (moving into a list that already has it is a removal, not a duplicate).""" - src = _own_playlist(db, user, playlist_id) + src = owned_or_404(db, Playlist, playlist_id, user, what="playlist") # 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 = owned_or_404(db, Playlist, body.target_playlist_id, user, what="move_target") @@ -575,19 +573,31 @@ def move_item( PlaylistItem.playlist_id == dst.id, PlaylistItem.video_id == video_id ) ) - if already is not None: - db.delete(item) # the uq_playlist_video constraint would reject a second row anyway - else: + dst_changed = False + if already 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. - item.playlist_id = dst.id - item.position = _next_item_position(db, dst.id) - # Both sides changed, so both get the dirty recompute and the timestamp — the asymmetric - # version of this (touching only the source) is the recurring smell CLAUDE.md warns about. + # survives the move, and there is no window where the video belongs to neither list. The + # check above is not a lock: a second tab's add or the scheduler's YouTube merge can insert + # the same (playlist, video) in between, and uq_playlist_video then refuses the re-parent. + # That is the already-present outcome arriving late, not a failure — answer it the same way + # instead of a raw 500. + try: + with db.begin_nested(): + item.playlist_id = dst.id + item.position = _next_item_position(db, dst.id) + db.flush() + dst_changed = True + except IntegrityError: + pass # the savepoint put the row back in the source list; drop it below + if not dst_changed: + db.delete(item) + # The source always changed; the target only when the row landed in it. `_touch` on a target + # that already had the video promoted an untouched list in the rail's "recently changed" sort. recompute_dirty(db, src) - recompute_dirty(db, dst) _touch(src) - _touch(dst) + if dst_changed: + recompute_dirty(db, dst) + _touch(dst) db.commit() return {"ok": True, "from_playlist_id": src.id, "to_playlist_id": dst.id, "video_id": video_id} @@ -603,7 +613,7 @@ def reorder_items( user: User = Depends(current_user), db: Session = Depends(get_db), ) -> dict: - pl = _own_playlist(db, user, playlist_id) + pl = owned_or_404(db, Playlist, playlist_id, user, what="playlist") order = body.video_ids items = { it.video_id: it diff --git a/backend/app/routes/plex/playlists.py b/backend/app/routes/plex/playlists.py index 022af3ae..9728b1d3 100644 --- a/backend/app/routes/plex/playlists.py +++ b/backend/app/routes/plex/playlists.py @@ -24,10 +24,6 @@ from app.routes._common import owned_or_404 router = APIRouter(prefix="/api/plex", tags=["plex"]) -def _own_playlist_or_404(db: Session, pid: int, user: User) -> PlexPlaylist: - return owned_or_404(db, PlexPlaylist, pid, user, what="playlist") - - def _playlist_card(pl: PlexPlaylist, count: int, thumb_rk: str | None) -> dict: return { "id": pl.id, @@ -140,7 +136,7 @@ def create_playlist( def playlist_detail( pid: int, user: User = Depends(current_user), db: Session = Depends(get_db) ) -> dict: - pl = _own_playlist_or_404(db, pid, user) + pl = owned_or_404(db, PlexPlaylist, pid, user, what="playlist") rows = db.execute(_playlist_item_query(pid)).all() items = [it for (_pi, it) in rows] states = { @@ -183,7 +179,7 @@ def rename_playlist( user: User = Depends(current_user), db: Session = Depends(get_db), ) -> dict: - pl = _own_playlist_or_404(db, pid, user) + pl = owned_or_404(db, PlexPlaylist, pid, user, what="playlist") title = body.title.strip() if not title: raise HTTPException(status_code=400, detail="title is required") @@ -203,7 +199,7 @@ def rename_playlist( def delete_playlist( pid: int, user: User = Depends(current_user), db: Session = Depends(get_db) ) -> dict: - pl = _own_playlist_or_404(db, pid, user) + pl = owned_or_404(db, PlexPlaylist, pid, user, what="playlist") db.delete(pl) db.commit() return {"deleted": pid} @@ -220,7 +216,7 @@ def playlist_add_item( user: User = Depends(current_user), db: Session = Depends(get_db), ) -> dict: - pl = _own_playlist_or_404(db, pid, user) + pl = owned_or_404(db, PlexPlaylist, pid, user, what="playlist") it = by_rating_key(db, PlexItem, body.item_rating_key) if it is None: raise HTTPException(status_code=404, detail="Unknown item") @@ -252,7 +248,7 @@ def playlist_remove_item( user: User = Depends(current_user), db: Session = Depends(get_db), ) -> dict: - pl = _own_playlist_or_404(db, pid, user) + pl = owned_or_404(db, PlexPlaylist, pid, user, what="playlist") it = by_rating_key(db, PlexItem, item_rating_key) if it is not None: db.execute( @@ -297,7 +293,7 @@ def playlist_add_bulk( ) -> dict: """Append many items at once (a whole season/show). Already-present items are skipped; the input order is preserved for the newly-added ones. Returns how many were added + the new total.""" - pl = _own_playlist_or_404(db, pid, user) + pl = owned_or_404(db, PlexPlaylist, pid, user, what="playlist") items = _items_by_rks(db, body.item_rating_keys) existing = set( db.execute( @@ -340,7 +336,7 @@ def playlist_remove_bulk( ) -> dict: """Remove many items at once (a whole season/show, or a multi-select). Returns how many were removed + the new total.""" - pl = _own_playlist_or_404(db, pid, user) + pl = owned_or_404(db, PlexPlaylist, pid, user, what="playlist") ids = [it.id for it in _items_by_rks(db, body.item_rating_keys)] removed = 0 if ids: @@ -370,7 +366,7 @@ def reorder_playlist( db: Session = Depends(get_db), ) -> dict: """Set the playlist order from an explicit list of item rating_keys (0-based positions).""" - pl = _own_playlist_or_404(db, pid, user) + pl = owned_or_404(db, PlexPlaylist, pid, user, what="playlist") order = body.item_rating_keys id_by_rk = { it.rating_key: it.id diff --git a/backend/app/routes/saved_views.py b/backend/app/routes/saved_views.py index e2d6f68e..130b7aec 100644 --- a/backend/app/routes/saved_views.py +++ b/backend/app/routes/saved_views.py @@ -90,10 +90,6 @@ def create_view( return _serialize(view) -def _own_view(db: Session, user: User, view_id: int) -> SavedView: - return owned_or_404(db, SavedView, view_id, user, what="view") - - class UpdateViewIn(BaseModel): # PATCH: presence decides what's touched (read via model_fields_set). filters, when sent, # must be a real object — an explicit null is rejected (a view can't have null filters). @@ -109,7 +105,7 @@ def update_view( user: User = Depends(require_human), db: Session = Depends(get_db), ) -> dict: - view = _own_view(db, user, view_id) + view = owned_or_404(db, SavedView, view_id, user, what="view") sent = body.model_fields_set if "name" in sent: name = (body.name or "").strip() @@ -148,7 +144,7 @@ def delete_view( user: User = Depends(require_human), db: Session = Depends(get_db), ) -> dict: - view = _own_view(db, user, view_id) + view = owned_or_404(db, SavedView, view_id, user, what="view") db.delete(view) db.commit() return {"deleted": view_id} diff --git a/backend/app/routes/tags.py b/backend/app/routes/tags.py index d5b8c8d8..32165fd3 100644 --- a/backend/app/routes/tags.py +++ b/backend/app/routes/tags.py @@ -78,11 +78,6 @@ def create_tag( return {"id": tag.id, "name": tag.name, "color": tag.color, "category": tag.category} -def _own_tag(db: Session, user: User, tag_id: int) -> Tag: - # System tags (user_id NULL) are not user-editable → owned_or_404 404s them too. - return owned_or_404(db, Tag, tag_id, user, what="tag") - - class TagUpdateIn(BaseModel): # PATCH: presence matters, and `color: null` explicitly CLEARS it — so the endpoint reads # model_fields_set rather than testing for None (which couldn't tell absent from cleared). @@ -97,7 +92,8 @@ def update_tag( user: User = Depends(current_user), db: Session = Depends(get_db), ) -> dict: - tag = _own_tag(db, user, tag_id) + # System tags (user_id NULL) are not user-editable: owned_or_404 404s them too. + tag = owned_or_404(db, Tag, tag_id, user, what="tag") sent = body.model_fields_set if "name" in sent: name = (body.name or "").strip() @@ -120,7 +116,8 @@ def delete_tag( user: User = Depends(current_user), db: Session = Depends(get_db), ) -> dict: - tag = _own_tag(db, user, tag_id) + # System tags (user_id NULL) are not user-editable: owned_or_404 404s them too. + tag = owned_or_404(db, Tag, tag_id, user, what="tag") db.delete(tag) # ChannelTag rows cascade on tag delete. db.commit() return {"deleted": tag_id} diff --git a/backend/tests/test_playlist_move.py b/backend/tests/test_playlist_move.py index 9a936d32..8bf1de21 100644 --- a/backend/tests/test_playlist_move.py +++ b/backend/tests/test_playlist_move.py @@ -242,3 +242,49 @@ def test_the_list_endpoint_publishes_updated_at(db, make_user, as_user): 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"] diff --git a/frontend/src/i18n/backendIds.contract.test.ts b/frontend/src/i18n/backendIds.contract.test.ts index 867c7658..d286a5d2 100644 --- a/frontend/src/i18n/backendIds.contract.test.ts +++ b/frontend/src/i18n/backendIds.contract.test.ts @@ -1,4 +1,4 @@ -import { readFileSync } from "node:fs"; +import { readdirSync, readFileSync } from "node:fs"; import { describe, expect, it } from "vitest"; // Every id the BACKEND owns and the UI names must have its name in both languages. The UI looks these @@ -86,6 +86,32 @@ const failDetails = [ ]; const DETAIL = "downloads.errors.fail.catalog."; +// What a structured 404 says is gone → `errors.notFound.`: every `not_found("x")` and every +// `owned_or_404(..., what="x")` / `get_or_404(..., what="x")` under routes/, read from the sources. +const routeSources = (dir = "routes"): string[] => + readdirSync(new URL(`../../../backend/app/${dir}/`, import.meta.url), { + withFileTypes: true, + }).flatMap((e) => + e.isDirectory() + ? routeSources(`${dir}/${e.name}`) + : e.name.endsWith(".py") + ? [backend(`${dir}/${e.name}`)] + : [], + ); +const goneWhats = [ + ...new Set( + routeSources().flatMap((src) => + [...src.matchAll(/not_found\("(\w+)"\)|what="(\w+)"/g)].map((m) => (m[1] ?? m[2])!), + ), + ), +]; +// A refused YouTube action → `errors.youtube.`, the reasons read from their Literal. +const youtubeReasons = [ + ...( + backend("youtube/client.py").match(/^YouTubeRefusal = Literal\[([^\]]*)\]/m)?.[1] ?? "" + ).matchAll(/"(\w+)"/g), +].map((m) => m[1]!); + /** The ids under `prefix` a locale names, `prefix.` or `prefix..`. */ const idsUnder = (keys: Set, prefix: string) => new Set( @@ -106,6 +132,8 @@ describe("backend ids have their names in both languages", () => { expect(quotaActions.length).toBeGreaterThan(0); expect(adminCodes).toEqual(expect.arrayContaining(["activate", "setpw"])); expect(failDetails).toEqual(expect.arrayContaining(["facebook.login_wall"])); + expect(goneWhats).toEqual(expect.arrayContaining(["playlist", "download", "job"])); + expect(youtubeReasons).toEqual(["quota", "reconnect", "unavailable"]); // Every code the frontend localizes is raised somewhere the scan can see. expect(adminCodes.filter((c) => !adminRefusals.some((m) => m[1] === c))).toEqual([]); }); @@ -159,6 +187,19 @@ describe("backend ids have their names in both languages", () => { expect([...named].filter((d) => !failDetails.includes(d))).toEqual([]); }); + it(`${lang}: every "that is gone" a 404 names, and every YouTube refusal`, () => { + expect(missing(goneWhats.map((w) => `errors.notFound.${w}`))).toEqual([]); + expect( + [...idsUnder(keys, "errors.notFound")].filter( + (w) => w !== "generic" && !goneWhats.includes(w), + ), + ).toEqual([]); + expect(missing(youtubeReasons.map((r) => `errors.youtube.${r}`))).toEqual([]); + expect( + [...idsUnder(keys, "errors.youtube")].filter((r) => !youtubeReasons.includes(r)), + ).toEqual([]); + }); + it(`${lang}: every admin refusal reason`, () => { expect(missing(adminReasons.map((r) => `errors.admin.${r}`))).toEqual([]); expect([...idsUnder(keys, "errors.admin")].filter((r) => !adminReasons.includes(r))).toEqual( diff --git a/frontend/src/i18n/locales/en/errors.json b/frontend/src/i18n/locales/en/errors.json index 6d4287f2..06f42c1b 100644 --- a/frontend/src/i18n/locales/en/errors.json +++ b/frontend/src/i18n/locales/en/errors.json @@ -18,7 +18,10 @@ "channel": "This channel isn't in the library any more.", "subscription": "You're not subscribed to this channel any more.", "recipient": "There's no user with that email address.", - "job": "This job doesn't exist any more." + "job": "This job doesn't exist any more.", + "download": "This download doesn't exist any more.", + "link": "This share link doesn't exist any more.", + "notification": "This notification doesn't exist any more." }, "admin": { "not_allowed": "Their address isn't approved any more. Add it to the whitelist first.", diff --git a/frontend/src/i18n/locales/hu/errors.json b/frontend/src/i18n/locales/hu/errors.json index 13be5034..6a5c8887 100644 --- a/frontend/src/i18n/locales/hu/errors.json +++ b/frontend/src/i18n/locales/hu/errors.json @@ -18,7 +18,10 @@ "channel": "Ez a csatorna már nincs a könyvtárban.", "subscription": "Már nem vagy feliratkozva erre a csatornára.", "recipient": "Nincs felhasználó ezzel az e-mail címmel.", - "job": "Ez a feladat már nem létezik." + "job": "Ez a feladat már nem létezik.", + "download": "Ez a letöltés már nem létezik.", + "link": "Ez a megosztási link már nem létezik.", + "notification": "Ez az értesítés már nem létezik." }, "admin": { "not_allowed": "A címe már nincs jóváhagyva. Előbb vedd fel a fehérlistára.",