mirror of
https://github.com/got-feedBack/feedBack.git
synced 2026-08-11 11:19:24 +00:00
feat(v3 library): "Fix metadata" popup — per-song override + lock, cover picker, MusicBrainz + AcoustID (#777)
* feat(library): metadata override + lock store, enforced by enrichment (popup slices 1–2)
Backend foundation for the Fix-metadata popup. Not yet surfaced in the UI (the
display + 3-tab popup are the next slices); no PR until it's user-visible.
Slice 1 — the store:
- `song_field_override(filename, field, value, locked)` table: a reversible
DISPLAY overlay (never written to the pack), filename-keyed so it survives a
rescan (never purged by delete_missing) and is dropped only with the song.
- DB methods (partial upsert that drops empty+unlocked rows; batch map) +
`GET`/`PUT /api/song/{fn}/overrides` (field allowlist title/artist/album/
year/genre; clearing rides PUT since DELETE /api/song/{path} shadows sub-
routes; PUT demo-blocked).
Slice 2 — locks respected by enrichment:
- The auto-matcher composes a per-song `_compose_lock_filter` onto the global
apply-filter, so a match still applies IDENTITY (mbid/release → art) but never
re-canonicalizes a LOCKED display field.
- Gap-fill (write-to-file) skips locked album/year/genre — writing the matched
value would be exactly the clobber the lock exists to prevent.
- Review/manual picks bypass the filter (an explicit confirm overrides a lock).
Tests: store semantics + rescan-survival + API; the lock filter + reader; an
auto-match leaving a locked field un-canonicalized; gap-fill excluding locked
keys.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* feat(library): show per-song overrides in the grid (popup slice 3)
The grid now displays the user's per-song title/artist/album/year override
in place of the pack value ("grid shows only overrides") — a matched
MusicBrainz canon never silently re-titles a card; canon stays in the
Details drawer + art. Overlaid in Python over the visible window, keyset-safe
like the P4 artist-alias re-label: the seek still runs on the raw column, and
the one overridable keyset column (title) stashes its raw value for the cursor
so paging never skips/dupes. The private stash is dropped from the payload.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* feat(library): 3-tab Fix-metadata popup — Details / Cover art / Match (slice 4)
Turns the thin single-song fix-match modal into the Plex-style metadata
editor reached from a card's "Fix metadata…" menu:
- Details tab: type + lock the displayed title/artist/album/year. Values
ride the reversible override store (GET/PUT /api/song/{fn}/overrides); each
field sits on its pack value (Yours/Pack provenance + revert-to-pack), a lock
pins it against auto-match, and Save repaints the grid via library:changed
(slice-3 overlay). This is the real tool for the blank-artist city-pop pile
MusicBrainz can't surface — you just type the right title.
- Cover art tab: hands off to the shared image picker (its own modal); the
pick refreshes the thumbnail everywhere.
- Match tab: the existing MusicBrainz search + candidate/pick flow, refactored
into shared body/footer helpers (the queue-review flow is untouched).
Backend: GET /overrides now also returns the pack baseline so the Details tab
can pre-fill + show provenance. tailwind.min.css regenerated (build-tailwind.sh)
for the popup's new utility classes.
Identify-by-audio (AcoustID) is deferred: it lives in unmerged PR #759, off
main — the Match tab gains the button once #759 lands and this branch rebases.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(library): wire "Identify by audio" in the tabbed popup's Match tab
The AcoustID Identify button (#759) merged in referencing an out-of-scope
`panel` in the wiring — a leftover from the pre-popup fix-match modal that my
tab refactor renamed to `root`. Under strict mode that threw, so the handler
never attached and the button did nothing. Scope it to `root` (the tab body),
which is where the search-results area it renders into lives.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix(library): make "Identify by audio" outcomes unmistakable
An empty AcoustID result read the same as a broken button. Each state now says
plainly which outcome it is — ✓ fingerprinted-but-no-match vs no-audio vs off vs
unavailable — and, in the popup, points at the manual fallback (Search, or set
the album in Details + cover in Cover art by hand). A ✓ marks the states that
actually ran, so "worked, found nothing" no longer looks like a failure.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
ChrisBeWithYou
co-authored by
Claude Opus 4.8
Claude Opus 4.8
parent
3e036e3db6
commit
af1170cec3
@@ -0,0 +1,232 @@
|
||||
"""Tests for the per-field metadata override + lock store (Fix-metadata popup).
|
||||
|
||||
A reversible DISPLAY overlay, never written to the pack: filename-keyed, so it
|
||||
survives a rescan (never purged by delete_missing) and is dropped only with the
|
||||
song (delete_song). Locks pin a field against a later auto-match.
|
||||
"""
|
||||
|
||||
import importlib
|
||||
import sys
|
||||
|
||||
import pytest
|
||||
from fastapi.testclient import TestClient
|
||||
|
||||
|
||||
@pytest.fixture()
|
||||
def server(tmp_path, monkeypatch, isolate_logging):
|
||||
monkeypatch.setenv("CONFIG_DIR", str(tmp_path))
|
||||
monkeypatch.setenv("FEEDBACK_SKIP_STARTUP_TASKS", "1")
|
||||
sys.modules.pop("server", None)
|
||||
srv = importlib.import_module("server")
|
||||
try:
|
||||
yield srv
|
||||
finally:
|
||||
conn = getattr(getattr(srv, "meta_db", None), "conn", None)
|
||||
if conn is not None:
|
||||
getattr(sys.modules.get("server"), "_join_background_db_threads", lambda: None)()
|
||||
conn.close()
|
||||
sys.modules.pop("server", None)
|
||||
|
||||
|
||||
@pytest.fixture()
|
||||
def client(server):
|
||||
return TestClient(server.app)
|
||||
|
||||
|
||||
def _put(server, fn, **meta):
|
||||
base = {"title": "Song", "artist": "Artist", "album": "", "duration": 100,
|
||||
"arrangements": [{"name": "Lead", "index": 0}]}
|
||||
base.update(meta)
|
||||
server.meta_db.put(fn, 0, 0, base)
|
||||
|
||||
|
||||
# ── store semantics ───────────────────────────────────────────────────────────
|
||||
|
||||
def test_set_get_and_partial_upsert(server):
|
||||
db = server.meta_db
|
||||
assert db.get_song_overrides("a.archive") == {}
|
||||
db.set_song_override("a.archive", "artist", value="AC/DC")
|
||||
assert db.get_song_overrides("a.archive") == {"artist": {"value": "AC/DC", "locked": False}}
|
||||
# partial: lock without touching the value
|
||||
db.set_song_override("a.archive", "artist", locked=True)
|
||||
assert db.get_song_overrides("a.archive")["artist"] == {"value": "AC/DC", "locked": True}
|
||||
# partial: change the value, keep the lock
|
||||
db.set_song_override("a.archive", "artist", value="AC/DC (fixed)")
|
||||
assert db.get_song_overrides("a.archive")["artist"] == {"value": "AC/DC (fixed)", "locked": True}
|
||||
|
||||
|
||||
def test_lock_only_row_persists_without_a_value(server):
|
||||
db = server.meta_db
|
||||
db.set_song_override("a.archive", "year", locked=True)
|
||||
# a pure lock (no override value) is a valid, kept row
|
||||
assert db.get_song_overrides("a.archive") == {"year": {"value": None, "locked": True}}
|
||||
|
||||
|
||||
def test_empty_and_unlocked_drops_the_row(server):
|
||||
db = server.meta_db
|
||||
db.set_song_override("a.archive", "album", value="X", locked=True)
|
||||
db.set_song_override("a.archive", "album", value="", locked=False)
|
||||
assert db.get_song_overrides("a.archive") == {} # no empty shell
|
||||
|
||||
|
||||
def test_clear_one_field_leaves_others(server):
|
||||
db = server.meta_db
|
||||
db.set_song_override("a.archive", "title", value="T")
|
||||
db.set_song_override("a.archive", "artist", value="A")
|
||||
db.clear_song_override("a.archive", "title")
|
||||
assert set(db.get_song_overrides("a.archive")) == {"artist"}
|
||||
|
||||
|
||||
# ── lifecycle: rescan survival vs explicit delete ─────────────────────────────
|
||||
|
||||
def test_rescan_never_purges_overrides_delete_does(server):
|
||||
_put(server, "a.archive")
|
||||
server.meta_db.set_song_override("a.archive", "artist", value="AC/DC", locked=True)
|
||||
server.meta_db.delete_missing(set()) # file vanished from a scan
|
||||
assert server.meta_db.get_song_overrides("a.archive")["artist"]["value"] == "AC/DC"
|
||||
server.meta_db.purge_song_user_data("a.archive") # the delete_song purge
|
||||
assert server.meta_db.get_song_overrides("a.archive") == {}
|
||||
|
||||
|
||||
def test_overrides_map_batches(server):
|
||||
db = server.meta_db
|
||||
db.set_song_override("a.archive", "artist", value="A")
|
||||
db.set_song_override("b.archive", "title", value="B", locked=True)
|
||||
m = db.overrides_map(["a.archive", "b.archive", "missing.archive"])
|
||||
assert m["a.archive"]["artist"]["value"] == "A"
|
||||
assert m["b.archive"]["title"] == {"value": "B", "locked": True}
|
||||
assert "missing.archive" not in m
|
||||
assert db.overrides_map([]) == {}
|
||||
|
||||
|
||||
# ── API ───────────────────────────────────────────────────────────────────────
|
||||
|
||||
def test_api_put_get_and_clear(client, server):
|
||||
_put(server, "a.archive")
|
||||
r = client.put("/api/song/a.archive/overrides",
|
||||
json={"overrides": {"artist": {"value": "AC/DC", "locked": True},
|
||||
"year": {"value": "1979"}}})
|
||||
assert r.status_code == 200
|
||||
ov = r.json()["overrides"]
|
||||
assert ov["artist"] == {"value": "AC/DC", "locked": True}
|
||||
assert ov["year"] == {"value": "1979", "locked": False}
|
||||
assert client.get("/api/song/a.archive/overrides").json()["overrides"]["artist"]["value"] == "AC/DC"
|
||||
# clear via PUT (value null + unlocked) — DELETE is shadowed by /api/song/{path}
|
||||
client.put("/api/song/a.archive/overrides",
|
||||
json={"overrides": {"artist": {"value": None, "locked": False}}})
|
||||
assert "artist" not in client.get("/api/song/a.archive/overrides").json()["overrides"]
|
||||
|
||||
|
||||
def test_api_get_returns_pack_values(client, server):
|
||||
_put(server, "a.archive", title="Pack Title", artist="Pack Artist",
|
||||
album="Pack Album", year="1988")
|
||||
server.meta_db.set_song_override("a.archive", "title", value="Fixed Title")
|
||||
body = client.get("/api/song/a.archive/overrides").json()
|
||||
# the override rides "overrides"; the pack baseline rides "pack" (all 5 fields)
|
||||
assert body["overrides"]["title"]["value"] == "Fixed Title"
|
||||
assert body["pack"] == {"title": "Pack Title", "artist": "Pack Artist",
|
||||
"album": "Pack Album", "year": "1988", "genre": ""}
|
||||
# a song with no row still gets an all-empty pack (popup always has values)
|
||||
assert client.get("/api/song/ghost.archive/overrides").json()["pack"]["title"] == ""
|
||||
|
||||
|
||||
def test_api_rejects_unknown_field(client, server):
|
||||
_put(server, "a.archive")
|
||||
r = client.put("/api/song/a.archive/overrides",
|
||||
json={"overrides": {"tuning": {"value": "Drop D"}}})
|
||||
assert r.status_code == 400
|
||||
assert "unknown field" in r.json()["error"]
|
||||
|
||||
|
||||
# ── lock enforcement (slice 2) ────────────────────────────────────────────────
|
||||
|
||||
def test_locked_fields_reader(server):
|
||||
db = server.meta_db
|
||||
db.set_song_override("a.archive", "artist", value="X", locked=True)
|
||||
db.set_song_override("a.archive", "title", value="Y") # override, not locked
|
||||
db.set_song_override("a.archive", "year", locked=True) # lock only
|
||||
assert db.locked_fields("a.archive") == {"artist", "year"}
|
||||
|
||||
|
||||
def test_compose_lock_filter_strips_locked_cand_keys(server):
|
||||
f = server._compose_lock_filter(None, {"artist", "year"})
|
||||
cand = {"recording_id": "r", "artist": "X", "artist_sort": "X", "title": "T",
|
||||
"year": "1990", "album": "A", "genres": ["rock"]}
|
||||
out = f(cand)
|
||||
# locked display keys stripped (artist maps to artist + artist_sort)…
|
||||
assert not ({"artist", "artist_sort", "year"} & set(out))
|
||||
# …identity + unlocked display fields survive
|
||||
assert out["recording_id"] == "r" and out["title"] == "T" and out["album"] == "A"
|
||||
# no locks → base filter returned unchanged (zero-copy common path)
|
||||
assert server._compose_lock_filter(None, set()) is None
|
||||
|
||||
|
||||
# ── display overlay in the grid (slice 3) ─────────────────────────────────────
|
||||
# "Grid shows only overrides": the effective cell is the user's override else the
|
||||
# pack value. Display-only + keyset-safe — the seek stays on the raw column.
|
||||
|
||||
def _grid(server, **kw):
|
||||
songs, _ = server.meta_db.query_page(**kw)
|
||||
return {s["filename"]: s for s in songs}
|
||||
|
||||
|
||||
def test_grid_shows_override_value_over_pack(server):
|
||||
_put(server, "a.archive", title="Wrong Title", artist="Wrong",
|
||||
album="Pack Album", year="1999")
|
||||
server.meta_db.set_song_override("a.archive", "title", value="Right Title")
|
||||
server.meta_db.set_song_override("a.archive", "artist", value="Right Artist")
|
||||
server.meta_db.set_song_override("a.archive", "year", value="1979")
|
||||
s = _grid(server)["a.archive"]
|
||||
assert s["title"] == "Right Title"
|
||||
assert s["artist"] == "Right Artist"
|
||||
assert s["year"] == "1979"
|
||||
assert s["album"] == "Pack Album" # no override → pack value shows
|
||||
assert s["_sort_title"] == "Wrong Title" # raw title stashed for the cursor
|
||||
|
||||
|
||||
def test_grid_ignores_lock_only_override(server):
|
||||
_put(server, "a.archive", title="Pack Title")
|
||||
server.meta_db.set_song_override("a.archive", "title", locked=True) # lock, no value
|
||||
s = _grid(server)["a.archive"]
|
||||
assert s["title"] == "Pack Title" # a lock without a value never retitles
|
||||
assert "_sort_title" not in s # …and stashes nothing
|
||||
|
||||
|
||||
def test_override_beats_alias_relabel_for_artist(server):
|
||||
_put(server, "a.archive", artist="ACDC")
|
||||
server.meta_db.set_artist_alias("ACDC", "AC/DC") # P4 alias
|
||||
assert _grid(server)["a.archive"]["artist"] == "AC/DC" # alias applies alone
|
||||
server.meta_db.set_song_override("a.archive", "artist", value="AC-DC (mine)")
|
||||
assert _grid(server)["a.archive"]["artist"] == "AC-DC (mine)" # override wins over alias
|
||||
|
||||
|
||||
def test_route_strips_private_sort_title(client, server):
|
||||
_put(server, "a.archive", title="Pack")
|
||||
server.meta_db.set_song_override("a.archive", "title", value="Shown")
|
||||
row = next(s for s in client.get("/api/library?sort=title").json()["songs"]
|
||||
if s["filename"] == "a.archive")
|
||||
assert row["title"] == "Shown"
|
||||
assert "_sort_title" not in row # private keyset stash never leaks to the client
|
||||
|
||||
|
||||
def test_title_keyset_paging_is_complete_with_overrides(client, server):
|
||||
# Raw titles A/B/C → title-sort order is A, B, C on the RAW column.
|
||||
_put(server, "b.archive", title="B")
|
||||
_put(server, "a.archive", title="A")
|
||||
_put(server, "c.archive", title="C")
|
||||
# Overrides that would reshuffle the order IF the cursor wrongly used the
|
||||
# displayed value — the seek must stay on the raw title, so paging still
|
||||
# covers every row exactly once (no skip/dupe).
|
||||
server.meta_db.set_song_override("a.archive", "title", value="ZZZ")
|
||||
server.meta_db.set_song_override("c.archive", "title", value="AAA")
|
||||
seen, cursor = [], None
|
||||
for _ in range(10):
|
||||
url = "/api/library?sort=title&size=1" + (f"&after={cursor}" if cursor else "")
|
||||
data = client.get(url).json()
|
||||
if not data["songs"]:
|
||||
break
|
||||
seen.append(data["songs"][0]["filename"])
|
||||
cursor = data["next_cursor"]
|
||||
if not cursor:
|
||||
break
|
||||
assert sorted(seen) == ["a.archive", "b.archive", "c.archive"] # each exactly once
|
||||
@@ -110,6 +110,19 @@ def test_preview_excludes_author_set_keys(server, client):
|
||||
assert {"genres", "mbid", "isrc"} <= got
|
||||
|
||||
|
||||
def test_preview_excludes_locked_fields(server, client):
|
||||
"""A field LOCKED in the Fix-metadata popup is never gap-filled — writing
|
||||
the matched value would be exactly the clobber the lock prevents — even
|
||||
though the match has a value and the manifest lacks it."""
|
||||
make_dir_sloppak(server, "a.sloppak")
|
||||
seed_match(server, "a.sloppak")
|
||||
server.meta_db.set_song_override("a.sloppak", "album", locked=True)
|
||||
server.meta_db.set_song_override("a.sloppak", "year", locked=True)
|
||||
got = {m["key"] for m in client.get("/api/song/a.sloppak/gap-fill").json()["missing"]}
|
||||
assert "album" not in got and "year" not in got
|
||||
assert {"genres", "mbid", "isrc"} <= got # unlocked keys still offered
|
||||
|
||||
|
||||
def test_preview_excludes_present_but_empty_keys(server, client):
|
||||
"""Gap-fill is append-only, so a present-but-empty value (album: '',
|
||||
year: 0) is NOT a gap the writer can fill — appending would duplicate the
|
||||
|
||||
@@ -185,6 +185,22 @@ def test_enrich_auto_matches_japanese_primary_via_alias(server, monkeypatch):
|
||||
assert row["mb_recording_id"] == "rec-jp"
|
||||
|
||||
|
||||
# ── per-song field locks respected by the auto-matcher ───────────────────────
|
||||
|
||||
def test_locked_field_not_canonicalized_by_auto_match(server, monkeypatch):
|
||||
_put(server, "x.sloppak") # title "Thunderstruck (v2)", artist "ACDC"
|
||||
server.meta_db.set_song_override("x.sloppak", "artist", locked=True)
|
||||
monkeypatch.setattr(server, "_mb_http_get",
|
||||
lambda path, params: {"recordings": [mb_doc()]})
|
||||
monkeypatch.setattr(server, "_enrich_network_enabled", lambda: True)
|
||||
server._background_enrich()
|
||||
row = server.meta_db.get_enrichment("x.sloppak")
|
||||
assert row["match_state"] == "matched" # still matches (identity applies)…
|
||||
assert row["canon_artist"] is None # …but the LOCKED artist isn't canonicalized
|
||||
assert row["canon_title"] == "Thunderstruck" # unlocked display fields still apply
|
||||
assert row["mb_recording_id"] # identity keys still stored (art needs them)
|
||||
|
||||
|
||||
# ── offline safety (the pytest-never-hits-network contract) ──────────────────
|
||||
|
||||
def test_offline_default_skips_matching(server, monkeypatch):
|
||||
|
||||
Reference in New Issue
Block a user