mirror of
https://github.com/got-feedBack/feedBack.git
synced 2026-07-22 21:01:40 +00:00
v3 library: artist aliases + Tidy-up merge UI — P4 (#705)
* v3 library: artist aliases + Tidy-up merge UI — P4 Fixes the "ACDC vs AC/DC" split without touching a single file or row: a never-purged artist_alias table (raw_name -> canonical_name) applied at DISPLAY time. The scanner keeps writing whatever the pack says; one alias row fixes every matching song. - query_artists dedupes/groups/orders on the effective artist, with a zero-cost fast path when no aliases exist; the artist filter expands a canonical name to its raw variants (index-friendly, keyset-safe); query_page re-labels row artists through the alias map. - CRUD + merge API: list aliases, list raw artists (variants + counts for the picker), set/merge/remove; a self-alias clears (= un-merge). - "Tidy up artists..." in the filter drawer (local library only): a searchable raw-variant checklist, merge-into-canonical, and a current-merges list with per-row un-merge. The artist dropdown + tree pick up canonical names with no dropdown code changes. - Sort + A-Z rail stay on the RAW artist (keyset-safe): a cross-letter alias shows its canonical label but buckets under the raw letter until effective columns are materialized (the grouping engine's work_key already resolves aliases when this table exists, so merged artists group correctly there). 11 tests. tailwind.min.css regenerated (generated file - on a merge conflict, re-run scripts/build-tailwind.sh). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Nm7tHs1Yvjjtnnu4nzJgdN * fix(v3): flatten transitive artist-alias chains + cycle guard so sequential merges unify (PR #705 review) merge_artists looped set_artist_alias which stored one hop, so sequential merges (ACDC->AC/DC then AC/DC->AC-DC) left a two-hop chain that the single-hop effective_artist/grouping/filtering split into two groups. Add _single_hop_canonical + _terminal_canonical (visited-set cycle break), resolve the canonical to its terminal before storing, forward-flatten existing rows that pointed at the raw name, and reject cycles (409). Batch merge now runs under one lock + one commit for atomicity. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: byrongamatos <xasiklas@gmail.com>
This commit is contained in:
ChrisBeWithYou
GitHub
parent
77e5a4982b
commit
a47accd894
282
server.py
282
server.py
@ -598,6 +598,22 @@ class MetadataDB:
|
||||
)
|
||||
""")
|
||||
self.conn.execute("CREATE INDEX IF NOT EXISTS idx_song_tags_tag ON song_tags(tag COLLATE NOCASE)")
|
||||
# Artist-name aliases (P4): "ACDC" → "AC/DC", "the beatles" → "The Beatles".
|
||||
# A CANONICALIZATION OVERRIDE applied AT DISPLAY only — the scanner-derived
|
||||
# `songs.artist` and the feedpak files are never rewritten (a rescan can't
|
||||
# fight the user; one alias row fixes every matching song at once). Keyed by
|
||||
# the raw artist string (COLLATE NOCASE so case variants collapse), so it is
|
||||
# NOT filename-keyed → never touched by delete_missing/delete_song (an alias
|
||||
# outlives the songs that motivated it, ready for re-import). mb_artist_id is
|
||||
# reserved for a future confident MusicBrainz match (unused now).
|
||||
self.conn.execute("""
|
||||
CREATE TABLE IF NOT EXISTS artist_alias (
|
||||
raw_name TEXT PRIMARY KEY COLLATE NOCASE,
|
||||
canonical_name TEXT NOT NULL,
|
||||
mb_artist_id TEXT,
|
||||
updated_at TEXT
|
||||
)
|
||||
""")
|
||||
self.conn.execute("""
|
||||
CREATE TABLE IF NOT EXISTS loops (
|
||||
id INTEGER PRIMARY KEY AUTOINCREMENT,
|
||||
@ -1554,6 +1570,173 @@ class MetadataDB:
|
||||
def _existing_song_filter(self) -> str:
|
||||
return self._EXISTING_SONG_FILTER
|
||||
|
||||
# ── Artist-name canonicalization (P4) ─────────────────────────────────────
|
||||
# "Apply at display": resolve songs.artist through the artist_alias override
|
||||
# for the deduped dropdown/tree (query_artists) — else keep the raw name. The
|
||||
# correlated PK-lookup subquery is fine for the offset-paged catalog; the grid
|
||||
# FILTER instead expands a canonical name to its raw variants (index-friendly,
|
||||
# keyset-safe), and the grid DISPLAY re-labels rows in Python via alias_map().
|
||||
_EFFECTIVE_ARTIST_SQL = (
|
||||
"COALESCE((SELECT aa.canonical_name FROM artist_alias aa "
|
||||
"WHERE aa.raw_name = songs.artist COLLATE NOCASE), songs.artist)"
|
||||
)
|
||||
|
||||
def alias_map(self) -> dict:
|
||||
"""{raw_name_lower: canonical_name} for every alias — one read to re-label
|
||||
a page of grid rows without an N+1. Lowercased keys so the lookup matches
|
||||
the raw artist case-insensitively (the table is COLLATE NOCASE)."""
|
||||
return {r[0].lower(): r[1] for r in self.conn.execute(
|
||||
"SELECT raw_name, canonical_name FROM artist_alias").fetchall()}
|
||||
|
||||
def effective_artist(self, raw: str, amap: dict | None = None) -> str:
|
||||
"""Canonical display name for a raw artist (alias override else itself)."""
|
||||
if raw is None:
|
||||
return raw
|
||||
amap = self.alias_map() if amap is None else amap
|
||||
return amap.get(raw.lower(), raw)
|
||||
|
||||
def _single_hop_canonical(self, name: str) -> str | None:
|
||||
"""The stored canonical for a raw name (a SINGLE hop), or None if `name`
|
||||
is not itself an alias key. Case-insensitive (the table is COLLATE NOCASE)
|
||||
— the shared primitive the chain-flatteners reuse."""
|
||||
if not name:
|
||||
return None
|
||||
row = self.conn.execute(
|
||||
"SELECT canonical_name FROM artist_alias WHERE raw_name = ? COLLATE NOCASE",
|
||||
(name,)).fetchone()
|
||||
return row[0] if row else None
|
||||
|
||||
def _terminal_canonical(self, name: str) -> str:
|
||||
"""Follow the alias chain from `name` to its TERMINAL canonical — the first
|
||||
name that is not itself an alias key — so transitive chains (raw → mid →
|
||||
… → terminal) collapse to one hop. A visited-set breaks cycles: if we come
|
||||
back to a name already seen we return the last name reached rather than
|
||||
looping. Reuses the single-hop primitive."""
|
||||
seen: set = set()
|
||||
cur = name
|
||||
while True:
|
||||
key = (cur or "").lower()
|
||||
if key in seen:
|
||||
return cur # cycle — stop, return where we are
|
||||
seen.add(key)
|
||||
nxt = self._single_hop_canonical(cur)
|
||||
if nxt is None or (nxt or "").lower() == key:
|
||||
return cur # not an alias key (or self) → terminal
|
||||
cur = nxt
|
||||
|
||||
def _raw_variants_for(self, canonical: str) -> list:
|
||||
"""Every raw artist string that should match a filter on `canonical`: the
|
||||
canonical name itself plus all raw names aliased to it (case-insensitive).
|
||||
Lets the artist filter be `artist IN (...)` — uses the artist index and is
|
||||
keyset-safe, instead of a per-row COALESCE subquery."""
|
||||
rows = self.conn.execute(
|
||||
"SELECT raw_name FROM artist_alias WHERE canonical_name = ? COLLATE NOCASE",
|
||||
(canonical,)).fetchall()
|
||||
seen, out = set(), []
|
||||
for name in [canonical, *[r[0] for r in rows]]:
|
||||
k = (name or "").lower()
|
||||
if name and k not in seen:
|
||||
seen.add(k)
|
||||
out.append(name)
|
||||
return out
|
||||
|
||||
def list_artist_aliases(self) -> list:
|
||||
"""All alias rows (raw → canonical), canonical then raw, for the Tidy-up
|
||||
'current merges' list."""
|
||||
rows = self.conn.execute(
|
||||
"SELECT raw_name, canonical_name, mb_artist_id FROM artist_alias "
|
||||
"ORDER BY canonical_name COLLATE NOCASE, raw_name COLLATE NOCASE").fetchall()
|
||||
return [{"raw_name": r[0], "canonical_name": r[1], "mb_artist_id": r[2]} for r in rows]
|
||||
|
||||
def _set_artist_alias_locked(self, raw_name: str, canonical_name: str,
|
||||
mb_artist_id: str | None = None) -> dict:
|
||||
"""Core upsert — assumes self._lock is HELD and does NOT commit (so the
|
||||
single set and the batch merge can share one transaction). Flattens chains
|
||||
and guards cycles:
|
||||
|
||||
* A self-alias (raw == canonical) DROPs any existing row (the UI un-merge).
|
||||
* Otherwise `canonical` is resolved to its TERMINAL canonical, so setting a
|
||||
new hop onto an existing chain collapses to one hop rather than growing a
|
||||
two-hop chain that grouping/filtering would then split.
|
||||
* Cycle guard: if that terminal IS `raw`, storing would loop the chain back
|
||||
on itself — we no-op and report it so the caller can surface a failure.
|
||||
* Forward-flatten: any existing rows whose canonical == `raw` are re-pointed
|
||||
to the new terminal, so previously-merged variants follow `raw` onward.
|
||||
|
||||
Returns a result dict {ok, raw_name, canonical_name, ...}."""
|
||||
raw = (raw_name or "").strip()
|
||||
canon = (canonical_name or "").strip()
|
||||
if not raw or not canon:
|
||||
raise ValueError("raw_name and canonical_name are required")
|
||||
if raw.lower() == canon.lower():
|
||||
self.conn.execute("DELETE FROM artist_alias WHERE raw_name = ? COLLATE NOCASE", (raw,))
|
||||
return {"ok": True, "raw_name": raw, "canonical_name": raw, "unmerged": True}
|
||||
terminal = self._terminal_canonical(canon)
|
||||
if (terminal or "").lower() == raw.lower():
|
||||
# raw → … → raw would be a cycle; refuse rather than corrupt the chain.
|
||||
return {"ok": False, "reason": "cycle", "raw_name": raw,
|
||||
"canonical_name": canon, "terminal": terminal}
|
||||
self.conn.execute(
|
||||
"INSERT INTO artist_alias (raw_name, canonical_name, mb_artist_id, updated_at) "
|
||||
"VALUES (?, ?, ?, datetime('now')) "
|
||||
"ON CONFLICT(raw_name) DO UPDATE SET "
|
||||
"canonical_name = excluded.canonical_name, "
|
||||
"mb_artist_id = excluded.mb_artist_id, updated_at = excluded.updated_at",
|
||||
(raw, terminal, mb_artist_id))
|
||||
# Re-point any variants that were previously merged INTO raw onto the new
|
||||
# terminal (raw itself now aliases onward, so it can't stay a canonical).
|
||||
self.conn.execute(
|
||||
"UPDATE artist_alias SET canonical_name = ?, updated_at = datetime('now') "
|
||||
"WHERE canonical_name = ? COLLATE NOCASE AND raw_name != ? COLLATE NOCASE",
|
||||
(terminal, raw, terminal))
|
||||
return {"ok": True, "raw_name": raw, "canonical_name": terminal}
|
||||
|
||||
def set_artist_alias(self, raw_name: str, canonical_name: str,
|
||||
mb_artist_id: str | None = None) -> dict:
|
||||
"""Upsert one raw→canonical override (chain-flattened, cycle-guarded — see
|
||||
_set_artist_alias_locked). Returns the result dict."""
|
||||
with self._lock:
|
||||
result = self._set_artist_alias_locked(raw_name, canonical_name, mb_artist_id)
|
||||
self.conn.commit()
|
||||
return result
|
||||
|
||||
def remove_artist_alias(self, raw_name: str) -> None:
|
||||
with self._lock:
|
||||
self.conn.execute("DELETE FROM artist_alias WHERE raw_name = ? COLLATE NOCASE", (raw_name,))
|
||||
self.conn.commit()
|
||||
|
||||
def merge_artists(self, raw_names, canonical_name: str) -> int:
|
||||
"""Point several raw artist names at one canonical (the Tidy-up merge).
|
||||
Skips the canonical's own self-alias. Returns the count of aliases written.
|
||||
ATOMIC: the whole batch runs under one lock and one commit, so a mid-batch
|
||||
cycle rejection can't leave a half-applied merge."""
|
||||
canon = (canonical_name or "").strip()
|
||||
if not canon:
|
||||
raise ValueError("canonical_name is required")
|
||||
n = 0
|
||||
with self._lock:
|
||||
for raw in (raw_names or []):
|
||||
r = (raw or "").strip()
|
||||
if r and r.lower() != canon.lower():
|
||||
result = self._set_artist_alias_locked(r, canon)
|
||||
if result.get("ok"):
|
||||
n += 1
|
||||
self.conn.commit()
|
||||
return n
|
||||
|
||||
def raw_artists(self, limit: int = 2000) -> list:
|
||||
"""Distinct RAW artist names in the library with song counts + their
|
||||
current canonical (for the Tidy-up picker — you merge raw variants). Raw,
|
||||
not effective, so both 'ACDC' and 'AC/DC' show as separate mergeable rows."""
|
||||
limit = max(1, min(10000, int(limit)))
|
||||
amap = self.alias_map()
|
||||
rows = self.conn.execute(
|
||||
"SELECT artist, COUNT(*) c FROM songs WHERE artist IS NOT NULL AND artist != '' "
|
||||
"GROUP BY artist COLLATE NOCASE ORDER BY c DESC, artist COLLATE NOCASE LIMIT ?",
|
||||
(limit,)).fetchall()
|
||||
return [{"name": r[0], "count": r[1],
|
||||
"canonical": amap.get((r[0] or "").lower(), r[0])} for r in rows]
|
||||
|
||||
def record_session(self, filename: str, arrangement: int, *, score: int,
|
||||
accuracy: float, last_position=None) -> dict:
|
||||
"""Record a scored play: plays += 1, best_* = max, last_* = new."""
|
||||
@ -2222,8 +2405,14 @@ class MetadataDB:
|
||||
where += " AND format = ?"
|
||||
params.append(format_filter)
|
||||
if artist_filter:
|
||||
where += " AND artist = ? COLLATE NOCASE"
|
||||
params.append(artist_filter)
|
||||
# The dropdown/tree list CANONICAL names (query_artists), so a filter
|
||||
# value is canonical — expand it to every raw variant aliased to it so
|
||||
# picking "AC/DC" returns songs tagged "ACDC" too. `artist IN (...)`
|
||||
# keeps the artist index (keyset-safe), unlike a per-row COALESCE.
|
||||
variants = self._raw_variants_for(artist_filter)
|
||||
ph = ",".join(["?"] * len(variants))
|
||||
where += f" AND artist COLLATE NOCASE IN ({ph})"
|
||||
params += variants
|
||||
if album_filter:
|
||||
where += " AND album = ? COLLATE NOCASE"
|
||||
params.append(album_filter)
|
||||
@ -2550,9 +2739,17 @@ class MetadataDB:
|
||||
fns = [s["filename"] for s in songs]
|
||||
udm = self.user_meta_map(fns)
|
||||
tgm = self.tags_map(fns)
|
||||
# Canonical artist at display (P4): re-label the card's artist through the
|
||||
# alias override so "ACDC" reads as "AC/DC". Display-only — the row's sort
|
||||
# position (raw artist) is untouched, so a card can show a canonical name
|
||||
# that differs from its A–Z bucket for cross-letter aliases; the full
|
||||
# sort/rail reindex under aliases is the P5a materialization pass.
|
||||
amap = self.alias_map()
|
||||
for s in songs:
|
||||
s["user_difficulty"] = udm.get(s["filename"])
|
||||
s["tags"] = tgm.get(s["filename"], [])
|
||||
if amap:
|
||||
s["artist"] = amap.get((s.get("artist") or "").lower(), s.get("artist"))
|
||||
return songs, total
|
||||
|
||||
def query_artists(self, letter: str = "", q: str = "",
|
||||
@ -2576,19 +2773,26 @@ class MetadataDB:
|
||||
stems_has=stems_has, stems_lacks=stems_lacks,
|
||||
has_lyrics=has_lyrics, tunings=tunings, naming_mode=naming_mode,
|
||||
)
|
||||
# Canonicalize artists at display when aliases exist (P4): dedupe / group /
|
||||
# letter / order on the EFFECTIVE artist so "ACDC" + "AC/DC" list as one
|
||||
# entry. With no aliases, `art_expr` stays the plain (indexed) `artist`
|
||||
# column, so the common case pays zero subquery cost.
|
||||
has_aliases = self.conn.execute("SELECT 1 FROM artist_alias LIMIT 1").fetchone() is not None
|
||||
art_expr = self._EFFECTIVE_ARTIST_SQL if has_aliases else "artist"
|
||||
|
||||
if letter == "#":
|
||||
where += " AND artist NOT GLOB '[A-Za-z]*'"
|
||||
where += f" AND ({art_expr}) NOT GLOB '[A-Za-z]*'"
|
||||
elif letter:
|
||||
where += " AND UPPER(SUBSTR(artist, 1, 1)) = ?"
|
||||
where += f" AND UPPER(SUBSTR(({art_expr}), 1, 1)) = ?"
|
||||
params.append(letter.upper())
|
||||
|
||||
# Get paginated distinct artists
|
||||
# Get paginated distinct (effective) artists
|
||||
total_artists = self.conn.execute(
|
||||
f"SELECT COUNT(DISTINCT artist COLLATE NOCASE) FROM songs {where}", params
|
||||
f"SELECT COUNT(DISTINCT ({art_expr}) COLLATE NOCASE) FROM songs {where}", params
|
||||
).fetchone()[0]
|
||||
|
||||
artist_rows = self.conn.execute(
|
||||
f"SELECT DISTINCT artist COLLATE NOCASE as a FROM songs {where} ORDER BY a LIMIT ? OFFSET ?",
|
||||
f"SELECT DISTINCT ({art_expr}) COLLATE NOCASE as a FROM songs {where} ORDER BY a LIMIT ? OFFSET ?",
|
||||
params + [size, page * size]
|
||||
).fetchall()
|
||||
artist_names = [r[0] for r in artist_rows]
|
||||
@ -2596,15 +2800,15 @@ class MetadataDB:
|
||||
if not artist_names:
|
||||
return [], total_artists
|
||||
|
||||
# Fetch songs for these artists only
|
||||
# Fetch songs for these (effective) artists only
|
||||
placeholders = ",".join(["?"] * len(artist_names))
|
||||
song_where = f"{where} AND artist COLLATE NOCASE IN ({placeholders})"
|
||||
song_where = f"{where} AND ({art_expr}) COLLATE NOCASE IN ({placeholders})"
|
||||
song_params = params + artist_names
|
||||
|
||||
rows = self.conn.execute(
|
||||
f"SELECT filename, title, artist, album, year, duration, tuning, arrangements, has_lyrics, "
|
||||
f"SELECT filename, title, ({art_expr}) as artist, album, year, duration, tuning, arrangements, has_lyrics, "
|
||||
f"format, stem_count, stem_ids, tuning_name "
|
||||
f"FROM songs {song_where} ORDER BY artist COLLATE NOCASE, album COLLATE NOCASE, title COLLATE NOCASE",
|
||||
f"FROM songs {song_where} ORDER BY ({art_expr}) COLLATE NOCASE, album COLLATE NOCASE, title COLLATE NOCASE",
|
||||
song_params
|
||||
).fetchall()
|
||||
|
||||
@ -5545,6 +5749,62 @@ def list_tags():
|
||||
return {"tags": meta_db.all_tags()}
|
||||
|
||||
|
||||
# ── Artist aliases / Tidy-up (P4) ────────────────────────────────────────────
|
||||
# Canonicalize messy artist tags at DISPLAY ("ACDC" → "AC/DC") without touching
|
||||
# the feedpak files or the scanner-derived songs.artist. All DB-only.
|
||||
|
||||
@app.get("/api/artist-aliases")
|
||||
def list_artist_aliases():
|
||||
"""Existing raw→canonical overrides (the Tidy-up 'current merges' list)."""
|
||||
return {"aliases": meta_db.list_artist_aliases()}
|
||||
|
||||
|
||||
@app.get("/api/artists/raw")
|
||||
def list_raw_artists(limit: int = 2000):
|
||||
"""Distinct RAW artist names + song counts + current canonical — the Tidy-up
|
||||
picker (you merge raw variants into one canonical)."""
|
||||
return {"artists": meta_db.raw_artists(limit)}
|
||||
|
||||
|
||||
@app.post("/api/artist-aliases")
|
||||
def set_artist_alias(data: dict):
|
||||
"""Upsert one override: {raw_name, canonical_name, mb_artist_id?}. A self-alias
|
||||
(raw == canonical) clears the row instead (un-merge)."""
|
||||
raw = (data.get("raw_name") or "").strip()
|
||||
canon = (data.get("canonical_name") or "").strip()
|
||||
if not raw or not canon:
|
||||
return JSONResponse({"error": "raw_name and canonical_name are required"}, 400)
|
||||
result = meta_db.set_artist_alias(raw, canon, (data.get("mb_artist_id") or None))
|
||||
if not result.get("ok"):
|
||||
# Would form a cycle (raw → … → raw) — refuse rather than corrupt the chain.
|
||||
return JSONResponse(
|
||||
{"error": "alias would create a cycle", "raw_name": raw, "canonical_name": canon},
|
||||
409)
|
||||
return {"ok": True, "raw_name": raw, "canonical_name": result.get("canonical_name", canon)}
|
||||
|
||||
|
||||
@app.post("/api/artist-aliases/merge")
|
||||
def merge_artist_aliases(data: dict):
|
||||
"""Merge several raw artist variants into one canonical:
|
||||
{raw_names: [...], canonical_name}. The canonical's own self-alias is skipped.
|
||||
Returns {merged: N}."""
|
||||
canon = (data.get("canonical_name") or "").strip()
|
||||
raws = data.get("raw_names")
|
||||
if not canon:
|
||||
return JSONResponse({"error": "canonical_name is required"}, 400)
|
||||
if not isinstance(raws, list) or not raws:
|
||||
return JSONResponse({"error": "raw_names must be a non-empty array"}, 400)
|
||||
n = meta_db.merge_artists(raws, canon)
|
||||
return {"merged": n, "canonical_name": canon}
|
||||
|
||||
|
||||
@app.delete("/api/artist-aliases/{raw_name:path}")
|
||||
def delete_artist_alias(raw_name: str):
|
||||
"""Remove one override so that raw artist stands on its own again."""
|
||||
meta_db.remove_artist_alias(raw_name)
|
||||
return {"ok": True}
|
||||
|
||||
|
||||
# ── Player profile / unified XP / streak (fee[dB]ack v0.3.0) ──────────────────
|
||||
|
||||
def _list_bundled_avatars() -> list[str]:
|
||||
|
||||
2
static/tailwind.min.css
vendored
2
static/tailwind.min.css
vendored
File diff suppressed because one or more lines are too long
@ -1713,6 +1713,11 @@
|
||||
(state.provider === 'local' && Object.keys(currentFilterRules()).length
|
||||
? '<div class="pt-3 border-t border-fb-border/50"><button data-drawer-save class="w-full text-sm text-fb-primary hover:text-fb-primaryHi border border-fb-primary/40 rounded-md py-2">＋ Save as collection</button></div>'
|
||||
: '') +
|
||||
// Artist canonicalization (P4) — local library only (aliases apply to
|
||||
// the local catalog). Opens the Tidy-up modal to merge "ACDC"/"AC/DC".
|
||||
(state.provider === 'local'
|
||||
? '<div class="pt-3 border-t border-fb-border/50"><button data-drawer-tidy class="w-full text-sm text-fb-textDim hover:text-fb-text border border-fb-border/50 rounded-md py-2">Tidy up artists…</button></div>'
|
||||
: '') +
|
||||
'<div class="flex justify-between pt-3 border-t border-fb-border/50"><button data-drawer-clear class="text-sm text-fb-textDim hover:text-fb-text">Clear all</button>' +
|
||||
'<button data-drawer-apply class="bg-fb-primary hover:bg-fb-primaryHi text-white px-4 py-2 rounded-md text-sm">Done</button></div></div>';
|
||||
|
||||
@ -1726,6 +1731,7 @@
|
||||
d.querySelectorAll('[data-lyrics]').forEach((b) => b.addEventListener('click', () => { f.lyrics = b.getAttribute('data-lyrics'); renderDrawer(); }));
|
||||
d.querySelectorAll('[data-mastery]').forEach((b) => b.addEventListener('click', () => { const v = b.getAttribute('data-mastery'); const i = f.mastery.indexOf(v); if (i >= 0) f.mastery.splice(i, 1); else f.mastery.push(v); renderDrawer(); }));
|
||||
d.querySelector('[data-drawer-save]')?.addEventListener('click', saveCurrentAsCollection);
|
||||
d.querySelector('[data-drawer-tidy]')?.addEventListener('click', openArtistTidyUp);
|
||||
d.querySelector('[data-drawer-close]')?.addEventListener('click', closeDrawer);
|
||||
d.querySelector('[data-drawer-clear]')?.addEventListener('click', async () => {
|
||||
state.filters = { arr_has: [], arr_lacks: [], stem_has: [], stem_lacks: [], lyrics: '', tunings: [], mastery: [] };
|
||||
@ -1978,6 +1984,116 @@
|
||||
// (it falls back to the legacy modal where this isn't defined).
|
||||
window.__fbOpenSongDetails = openDetails;
|
||||
|
||||
// ── Artist Tidy-up (P4) ────────────────────────────────────────────────--
|
||||
// Merge messy artist variants ("ACDC" + "AC/DC") into one canonical name.
|
||||
// Aliases apply AT DISPLAY only (no file rewrite); the dropdown/tree/grid pick
|
||||
// up the change on the next load. Self-contained centered modal.
|
||||
function openArtistTidyUp() {
|
||||
const st = { raw: [], aliases: [], sel: new Set(), query: '', mergeInto: '', mergeTouched: false, busy: false };
|
||||
|
||||
const overlay = document.createElement('div');
|
||||
overlay.className = 'fixed inset-0 z-[200] bg-black/60 backdrop-blur-sm flex items-center justify-center p-4';
|
||||
const onKey = (e) => { if (e.key === 'Escape') { e.preventDefault(); done(); } };
|
||||
function done() { overlay.remove(); document.removeEventListener('keydown', onKey); }
|
||||
document.addEventListener('keydown', onKey);
|
||||
overlay.addEventListener('click', (e) => { if (e.target === overlay) done(); });
|
||||
document.body.appendChild(overlay);
|
||||
|
||||
async function refresh() {
|
||||
const [raw, aliases] = await Promise.all([jget('/api/artists/raw'), jget('/api/artist-aliases')]);
|
||||
st.raw = (raw && raw.artists) || [];
|
||||
st.aliases = (aliases && aliases.aliases) || [];
|
||||
render();
|
||||
}
|
||||
|
||||
// Default canonical = the highest-count selected variant (the one most of
|
||||
// your library already uses), unless the user typed their own.
|
||||
function defaultCanonical() {
|
||||
let best = '', bestC = -1;
|
||||
st.raw.forEach((a) => { if (st.sel.has(a.name) && a.count > bestC) { bestC = a.count; best = a.name; } });
|
||||
return best;
|
||||
}
|
||||
|
||||
function render() {
|
||||
if (!st.mergeTouched) st.mergeInto = defaultCanonical();
|
||||
const q = st.query.trim().toLowerCase();
|
||||
const list = st.raw.filter((a) => !q || (a.name || '').toLowerCase().includes(q));
|
||||
const rows = list.map((a) => {
|
||||
const checked = st.sel.has(a.name) ? ' checked' : '';
|
||||
const mapped = (a.canonical && a.canonical.toLowerCase() !== (a.name || '').toLowerCase())
|
||||
? '<span class="text-[11px] text-fb-primary ml-1">→ ' + esc(a.canonical) + '</span>' : '';
|
||||
return '<label class="flex items-center gap-2 px-2 py-1 rounded hover:bg-fb-card/50 cursor-pointer">' +
|
||||
'<input type="checkbox" data-tidy-sel="' + esc(a.name) + '"' + checked + ' class="w-4 h-4 accent-fb-primary shrink-0">' +
|
||||
'<span class="text-sm text-fb-text truncate flex-1">' + esc(a.name) + mapped + '</span>' +
|
||||
'<span class="text-xs text-fb-textDim shrink-0">' + a.count + '</span></label>';
|
||||
}).join('') || '<div class="text-xs text-fb-textDim px-2 py-3">No artists</div>';
|
||||
|
||||
const canReady = st.mergeInto.trim() && st.sel.size && !st.busy;
|
||||
const mergeBar = st.sel.size
|
||||
? '<div class="pt-2 mt-2 border-t border-fb-border/50 space-y-2">' +
|
||||
'<div class="text-xs text-fb-textDim">' + st.sel.size + ' selected — merge into:</div>' +
|
||||
'<div class="flex gap-1"><input type="text" data-tidy-canon value="' + esc(st.mergeInto) + '" placeholder="Canonical name" class="flex-1 bg-fb-card border border-fb-border/60 rounded-lg px-3 py-1.5 text-sm text-fb-text outline-none focus:border-fb-primary/60">' +
|
||||
'<button data-tidy-merge ' + (canReady ? '' : 'disabled') + ' class="text-sm px-3 rounded-lg ' + (canReady ? 'bg-fb-primary hover:bg-fb-primaryHi text-white' : 'bg-fb-card/50 text-fb-textDim cursor-not-allowed') + '">Merge</button></div></div>'
|
||||
: '';
|
||||
|
||||
const aliasRows = st.aliases.length
|
||||
? st.aliases.map((al) => '<div class="flex items-center gap-2 px-2 py-1 text-sm"><span class="text-fb-textDim truncate flex-1">' + esc(al.raw_name) + ' <span class="text-fb-textDim/60">→</span> ' + esc(al.canonical_name) + '</span>' +
|
||||
'<button data-tidy-unmerge="' + esc(al.raw_name) + '" class="text-xs text-fb-textDim hover:text-fb-accent shrink-0">un-merge</button></div>').join('')
|
||||
: '<div class="text-xs text-fb-textDim px-2 py-2">No merges yet</div>';
|
||||
|
||||
overlay.innerHTML =
|
||||
'<div class="bg-fb-sidebar border border-fb-border/60 rounded-2xl w-full max-w-md shadow-2xl max-h-[85vh] flex flex-col">' +
|
||||
'<div class="p-5 pb-3 flex items-center justify-between"><h3 class="text-base font-semibold text-fb-text">Tidy up artists</h3>' +
|
||||
'<button data-tidy-x aria-label="Close" class="text-fb-textDim hover:text-fb-text text-xl leading-none">✕</button></div>' +
|
||||
'<div class="px-5"><input type="text" data-tidy-search value="' + esc(st.query) + '" placeholder="Search artists…" class="w-full bg-fb-card border border-fb-border/60 rounded-lg px-3 py-1.5 text-sm text-fb-text outline-none focus:border-fb-primary/60 mb-1"></div>' +
|
||||
'<div class="px-3 overflow-y-auto v3-scroll flex-1 min-h-[6rem]">' + rows + '</div>' +
|
||||
'<div class="px-5">' + mergeBar + '</div>' +
|
||||
'<div class="px-5 pt-2"><div class="text-xs font-semibold uppercase tracking-wider text-fb-textDim mb-1">Current merges</div><div class="max-h-32 overflow-y-auto v3-scroll">' + aliasRows + '</div></div>' +
|
||||
'<div class="p-5 pt-3 flex justify-end"><button data-tidy-x class="text-sm px-4 py-2 bg-fb-card/60 hover:bg-fb-card border border-fb-border/50 rounded-xl text-fb-text">Done</button></div>' +
|
||||
'</div>';
|
||||
|
||||
overlay.querySelectorAll('[data-tidy-x]').forEach((b) => b.addEventListener('click', done));
|
||||
const search = overlay.querySelector('[data-tidy-search]');
|
||||
if (search) search.addEventListener('input', () => { st.query = search.value; render(); const s = overlay.querySelector('[data-tidy-search]'); if (s) { s.focus(); const n = s.value.length; try { s.setSelectionRange(n, n); } catch (_) { /* */ } } });
|
||||
overlay.querySelectorAll('[data-tidy-sel]').forEach((cb) => cb.addEventListener('change', () => {
|
||||
const n = cb.getAttribute('data-tidy-sel');
|
||||
if (cb.checked) st.sel.add(n); else st.sel.delete(n);
|
||||
render();
|
||||
}));
|
||||
const canon = overlay.querySelector('[data-tidy-canon]');
|
||||
if (canon) canon.addEventListener('input', () => {
|
||||
st.mergeInto = canon.value; st.mergeTouched = true;
|
||||
const btn = overlay.querySelector('[data-tidy-merge]');
|
||||
if (btn) btn.disabled = !(st.mergeInto.trim() && st.sel.size && !st.busy);
|
||||
});
|
||||
overlay.querySelector('[data-tidy-merge]')?.addEventListener('click', doMerge);
|
||||
overlay.querySelectorAll('[data-tidy-unmerge]').forEach((b) => b.addEventListener('click', () => doUnmerge(b.getAttribute('data-tidy-unmerge'))));
|
||||
}
|
||||
|
||||
async function doMerge() {
|
||||
const canonical = st.mergeInto.trim();
|
||||
if (!canonical || !st.sel.size || st.busy) return;
|
||||
st.busy = true;
|
||||
await jsend('POST', '/api/artist-aliases/merge', { raw_names: [...st.sel], canonical_name: canonical });
|
||||
st.sel.clear(); st.mergeTouched = false; st.busy = false;
|
||||
await afterChange();
|
||||
}
|
||||
|
||||
async function doUnmerge(raw) {
|
||||
if (!raw) return;
|
||||
try { await fetch('/api/artist-aliases/' + enc(raw), { method: 'DELETE' }); } catch (_) { /* */ }
|
||||
await afterChange();
|
||||
}
|
||||
|
||||
async function afterChange() {
|
||||
await refresh();
|
||||
// Reflect canonical names in the toolbar dropdown + grid immediately.
|
||||
try { await loadArtistCatalog(); refreshArtistAlbumSelects(); reload(); } catch (_) { /* not on the songs grid */ }
|
||||
}
|
||||
|
||||
refresh();
|
||||
}
|
||||
|
||||
// The host loads the Folder Library plugin's screen.js at startup (defining
|
||||
// window.folderLibrary). If it isn't present yet, inject it once; the
|
||||
// plugin's IIFEs are idempotent so a redundant evaluation is a no-op. The
|
||||
|
||||
211
tests/test_artist_alias.py
Normal file
211
tests/test_artist_alias.py
Normal file
@ -0,0 +1,211 @@
|
||||
"""Tests for artist-name canonicalization (P4): the artist_alias override that
|
||||
merges messy artist tags ("ACDC" → "AC/DC") AT DISPLAY — the deduped dropdown/
|
||||
tree (query_artists), the artist filter (canonical matches all raw variants),
|
||||
and the grid card label — without rewriting songs.artist or the feedpak files.
|
||||
Sort/A–Z stay on the raw artist (keyset-safe); that reindex is deferred to P5a."""
|
||||
|
||||
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:
|
||||
conn.close()
|
||||
sys.modules.pop("server", None)
|
||||
|
||||
|
||||
@pytest.fixture()
|
||||
def client(server):
|
||||
return TestClient(server.app)
|
||||
|
||||
|
||||
def _seed(server, fn, artist, album="Alb"):
|
||||
server.meta_db.put(fn, 0, 0, {"title": fn.split(".")[0], "artist": artist, "album": album})
|
||||
|
||||
|
||||
def _artist_names(client):
|
||||
data = client.get("/api/library/artists?size=100").json()
|
||||
return [a["name"] for a in data["artists"]]
|
||||
|
||||
|
||||
def _grid_artist(client, fn):
|
||||
row = next(s for s in client.get("/api/library").json()["songs"] if s["filename"] == fn)
|
||||
return row["artist"]
|
||||
|
||||
|
||||
def _alias(client, raw, canonical):
|
||||
return client.post("/api/artist-aliases", json={"raw_name": raw, "canonical_name": canonical})
|
||||
|
||||
|
||||
# ── No aliases: raw names, unchanged behaviour ───────────────────────────────
|
||||
|
||||
def test_no_aliases_lists_raw_distinct(client, server):
|
||||
_seed(server, "a.archive", "ACDC")
|
||||
_seed(server, "b.archive", "AC/DC")
|
||||
assert set(_artist_names(client)) == {"ACDC", "AC/DC"}
|
||||
|
||||
|
||||
# ── Dropdown/tree dedupe on the canonical name ───────────────────────────────
|
||||
|
||||
def test_alias_dedupes_artist_list(client, server):
|
||||
_seed(server, "a.archive", "ACDC")
|
||||
_seed(server, "b.archive", "AC/DC")
|
||||
_alias(client, "ACDC", "AC/DC")
|
||||
data = client.get("/api/library/artists?size=100").json()
|
||||
assert [a["name"] for a in data["artists"]] == ["AC/DC"]
|
||||
assert data["total_artists"] == 1
|
||||
|
||||
|
||||
# ── Grid card shows the canonical label ──────────────────────────────────────
|
||||
|
||||
def test_grid_shows_canonical_artist(client, server):
|
||||
_seed(server, "a.archive", "ACDC")
|
||||
_alias(client, "ACDC", "AC/DC")
|
||||
assert _grid_artist(client, "a.archive") == "AC/DC"
|
||||
|
||||
|
||||
# ── Filtering by the canonical matches every raw variant ─────────────────────
|
||||
|
||||
def test_filter_by_canonical_matches_all_variants(client, server):
|
||||
_seed(server, "a.archive", "ACDC")
|
||||
_seed(server, "b.archive", "AC/DC")
|
||||
_seed(server, "c.archive", "Other")
|
||||
_alias(client, "ACDC", "AC/DC")
|
||||
got = {s["filename"] for s in
|
||||
client.get("/api/library", params={"artist": "AC/DC"}).json()["songs"]}
|
||||
assert got == {"a.archive", "b.archive"}
|
||||
|
||||
|
||||
# ── Merge endpoint ───────────────────────────────────────────────────────────
|
||||
|
||||
def test_merge_endpoint(client, server):
|
||||
_seed(server, "a.archive", "Beatles")
|
||||
_seed(server, "b.archive", "The Beatles")
|
||||
r = client.post("/api/artist-aliases/merge",
|
||||
json={"raw_names": ["Beatles", "The Beatles"], "canonical_name": "The Beatles"})
|
||||
assert r.json()["merged"] == 1 # "The Beatles" self-skip
|
||||
assert _artist_names(client) == ["The Beatles"]
|
||||
|
||||
|
||||
def test_merge_requires_canonical_and_list(client, server):
|
||||
assert client.post("/api/artist-aliases/merge", json={"raw_names": ["x"]}).status_code == 400
|
||||
assert client.post("/api/artist-aliases/merge", json={"canonical_name": "y"}).status_code == 400
|
||||
|
||||
|
||||
# ── Un-merge: self-alias clears + DELETE ─────────────────────────────────────
|
||||
|
||||
def test_self_alias_clears(client, server):
|
||||
_seed(server, "a.archive", "ACDC")
|
||||
_alias(client, "ACDC", "AC/DC")
|
||||
_alias(client, "ACDC", "ACDC") # self → un-merge
|
||||
assert client.get("/api/artist-aliases").json()["aliases"] == []
|
||||
assert _grid_artist(client, "a.archive") == "ACDC"
|
||||
|
||||
|
||||
def test_delete_alias_unmerges(client, server):
|
||||
_seed(server, "a.archive", "ACDC")
|
||||
_alias(client, "ACDC", "AC/DC")
|
||||
assert client.delete("/api/artist-aliases/ACDC").json()["ok"] is True
|
||||
assert _grid_artist(client, "a.archive") == "ACDC"
|
||||
|
||||
|
||||
# ── Never purged when songs churn (separate, non-filename-keyed table) ────────
|
||||
|
||||
def test_alias_survives_song_reindex(client, server):
|
||||
_seed(server, "a.archive", "ACDC")
|
||||
_alias(client, "ACDC", "AC/DC")
|
||||
# A rescan re-indexes the song (INSERT OR REPLACE INTO songs).
|
||||
server.meta_db.put("a.archive", 1, 1, {"title": "a", "artist": "ACDC", "album": "Alb"})
|
||||
assert _grid_artist(client, "a.archive") == "AC/DC"
|
||||
assert len(client.get("/api/artist-aliases").json()["aliases"]) == 1
|
||||
|
||||
|
||||
# ── Raw-artist picker (Tidy-up source) ───────────────────────────────────────
|
||||
|
||||
def test_raw_artists_lists_counts_and_canonical(client, server):
|
||||
_seed(server, "a.archive", "ACDC")
|
||||
_seed(server, "b.archive", "ACDC")
|
||||
_seed(server, "c.archive", "AC/DC")
|
||||
_alias(client, "ACDC", "AC/DC")
|
||||
by_name = {a["name"]: a for a in client.get("/api/artists/raw").json()["artists"]}
|
||||
assert by_name["ACDC"]["count"] == 2
|
||||
assert by_name["ACDC"]["canonical"] == "AC/DC" # shows where it maps
|
||||
assert by_name["AC/DC"]["count"] == 1
|
||||
|
||||
|
||||
# ── Transitive chains flatten so sequential merges unify (PR #705 P2) ─────────
|
||||
|
||||
def test_sequential_merge_flattens_transitive_chain(client, server):
|
||||
"""merge ACDC→AC/DC then AC/DC→AC-DC must unify ALL variants onto the terminal
|
||||
canonical ("AC-DC") — not leave a two-hop chain that grouping/filtering split."""
|
||||
_seed(server, "a.archive", "ACDC")
|
||||
_seed(server, "b.archive", "AC/DC")
|
||||
_seed(server, "c.archive", "AC-DC")
|
||||
client.post("/api/artist-aliases/merge",
|
||||
json={"raw_names": ["ACDC"], "canonical_name": "AC/DC"})
|
||||
client.post("/api/artist-aliases/merge",
|
||||
json={"raw_names": ["AC/DC"], "canonical_name": "AC-DC"})
|
||||
# Both original variants resolve (single hop) to the terminal canonical.
|
||||
assert server.meta_db.effective_artist("ACDC") == "AC-DC"
|
||||
assert server.meta_db.effective_artist("AC/DC") == "AC-DC"
|
||||
# The song tagged "ACDC" displays the terminal, not the intermediate.
|
||||
assert _grid_artist(client, "a.archive") == "AC-DC"
|
||||
# Grouping shows ONE canonical, not two split groups.
|
||||
assert _artist_names(client) == ["AC-DC"]
|
||||
# Stored rows are already terminal (forward-flattened), never AC/DC.
|
||||
canon = {a["canonical_name"] for a in client.get("/api/artist-aliases").json()["aliases"]}
|
||||
assert canon == {"AC-DC"}
|
||||
# Filtering by the terminal matches every original variant.
|
||||
got = {s["filename"] for s in
|
||||
client.get("/api/library", params={"artist": "AC-DC"}).json()["songs"]}
|
||||
assert got == {"a.archive", "b.archive", "c.archive"}
|
||||
|
||||
|
||||
def test_cycle_is_refused_and_state_intact(client, server):
|
||||
"""A→B then B→A would close a cycle: the second set is refused (409) and the
|
||||
existing A→B mapping is left intact — no loop, no corruption."""
|
||||
_seed(server, "a.archive", "A")
|
||||
_seed(server, "b.archive", "B")
|
||||
client.post("/api/artist-aliases/merge", json={"raw_names": ["A"], "canonical_name": "B"})
|
||||
r = _alias(client, "B", "A") # B → A closes the cycle
|
||||
assert r.status_code == 409
|
||||
# State unchanged: exactly one alias row, A → B.
|
||||
assert client.get("/api/artist-aliases").json()["aliases"] == [
|
||||
{"raw_name": "A", "canonical_name": "B", "mb_artist_id": None}]
|
||||
assert server.meta_db.effective_artist("A") == "B"
|
||||
assert server.meta_db.effective_artist("B") == "B"
|
||||
|
||||
|
||||
def test_terminal_resolution_survives_a_stored_cycle(server):
|
||||
"""Even if the table somehow holds a direct cycle (P↔Q), the visited-set makes
|
||||
_terminal_canonical terminate instead of looping forever."""
|
||||
db = server.meta_db
|
||||
with db._lock:
|
||||
db.conn.execute("INSERT INTO artist_alias (raw_name, canonical_name, updated_at) "
|
||||
"VALUES ('P', 'Q', datetime('now'))")
|
||||
db.conn.execute("INSERT INTO artist_alias (raw_name, canonical_name, updated_at) "
|
||||
"VALUES ('Q', 'P', datetime('now'))")
|
||||
db.conn.commit()
|
||||
assert db._terminal_canonical("P") in ("P", "Q")
|
||||
assert db._terminal_canonical("Q") in ("P", "Q")
|
||||
|
||||
|
||||
def test_list_aliases_sorted(client, server):
|
||||
_seed(server, "a.archive", "ACDC")
|
||||
_seed(server, "b.archive", "guns n roses")
|
||||
_alias(client, "ACDC", "AC/DC")
|
||||
_alias(client, "guns n roses", "Guns N' Roses")
|
||||
aliases = client.get("/api/artist-aliases").json()["aliases"]
|
||||
assert {a["raw_name"] for a in aliases} == {"ACDC", "guns n roses"}
|
||||
Loading…
Reference in New Issue
Block a user