From 4a27f3c245777c634261b64c5c714fa2a99d6002 Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Wed, 6 May 2026 09:30:33 -0700 Subject: [PATCH] Source-agnostic Enhance Quality flow + reject empty matches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Discord report: clicking Enhance Quality on an artist with neither Spotify nor Deezer connected added tracks to the wishlist as "unknown artist - unknown album - unknown track". Root cause was structural. core/artists/quality.py had a hardcoded Spotify-direct → Spotify-search → iTunes-fallback chain that ignored the user's configured primary metadata source. When Spotify wasn't connected, every track fell through to an iTunes-only fallback that occasionally returned matches with empty fields (cleared the 0.7 confidence threshold but missing artist / album / title). Those empty strings propagated through the wishlist payload normalizer's truthy-check passthrough at core/wishlist/payloads.py:77-80 and the UI rendered them as "Unknown" defaults. Rewrote the flow source-agnostic: - ArtistQualityDeps gains get_metadata_fallback_source. Flow resolves the user's active primary source once up front. - New _build_payload_from_track helper produces the Spotify-shaped wishlist payload from any source's Track object — single place that knows how to construct it (replaces the duplicate construction in the Spotify-search and iTunes-fallback paths). - New _search_match helper does generic confidence-scored search against any client implementing search_tracks(query, limit). Same 0.7 threshold, same album-bonus weighting as before. - New _has_complete_metadata validator rejects matches with empty title / album / artists before they reach the wishlist. - _spotify_direct_lookup kept as a Spotify-only optimization (only Spotify exposes get_track_details(id) returning rich raw payload); other sources fall through to search. - Failure reason now names the active source: "No usable {source} match — connect another metadata source for better coverage". Result: Discogs users get a Discogs search. Hydrabase users get a Hydrabase search. iTunes users get an iTunes search with empty-field rejection. Spotify keeps its direct-lookup fast path. 6 new tests pin the architectural change: - Primary-source dispatch routes to the configured client (Discogs, not Spotify) when Spotify isn't primary - Spotify direct-lookup is gated on Spotify being the active primary (skipped when Discogs is configured even if track has spotify_track_id) - Empty title / album / artists fields all reject the match - Failure reason names the active source --- core/artists/quality.py | 461 ++++++++++++++++++++-------------- tests/artists/test_quality.py | 206 ++++++++++++++- web_server.py | 1 + webui/static/helper.js | 1 + 4 files changed, 469 insertions(+), 200 deletions(-) diff --git a/core/artists/quality.py b/core/artists/quality.py index 3bf6dec6..de2a660f 100644 --- a/core/artists/quality.py +++ b/core/artists/quality.py @@ -2,23 +2,40 @@ `enhance_artist_quality(artist_id, track_ids, deps)` is the route-handler body for the `/api/library/artist//enhance` endpoint. It walks -the user's selected tracks, finds the best Spotify (preferred) or iTunes -(fallback) match for each, and queues high-quality re-downloads on the +the user's selected tracks, finds the best metadata match against the +configured primary source, and queues high-quality re-downloads on the wishlist with `source_type='enhance'`. -Per-track flow: +Per-track flow (source-agnostic): 1. Resolve the existing track via the artist's full detail map (built up front from `database.get_artist_full_detail`). 2. Read current quality tier from the file extension. 3. Build `matched_track_data` for the wishlist entry, in priority order: - - Direct Spotify lookup via stored `spotify_track_id` (preferred). - - Spotify search fallback using matching_engine queries. - - iTunes/fallback source search. -4. Add to wishlist via `wishlist_service.add_spotify_track_to_wishlist` + - Direct Spotify lookup via stored `spotify_track_id` (only when + Spotify is the active primary source — Spotify exposes + `get_track_details(id)` returning rich raw data; other sources + don't have an equivalent stored-ID-to-track API today). + - Search match against the primary metadata source (Spotify / + iTunes / Deezer / Discogs / Hydrabase — whichever the user has + configured as their primary). Confidence threshold is 0.7. +4. Validate the match has non-empty title, album, and artists. Reject + matches with empty fields — those propagated as + "unknown artist - unknown album - unknown track" wishlist entries + pre-fix because the wishlist payload normalizer's truthy-check + passthrough accepted dicts with empty string fields. +5. Add to wishlist via `wishlist_service.add_spotify_track_to_wishlist` with `source_type='enhance'` and a `source_context` carrying the original file path, format tier, bitrate, and artist name. -5. Tally `enhanced_count` / `failed_count` / per-track failure reasons. +6. Tally `enhanced_count` / `failed_count` / per-track failure reasons. + +The flow originally had Spotify-only logic for steps 1 and 2 with iTunes +hardcoded as the only fallback. That broke for users with neither +Spotify nor Deezer connected — iTunes returned sparse / no matches and +the failure mode was silent. Now everything dispatches through the +configured primary source via ``deps.get_metadata_fallback_client()`` +(which respects `metadata.fallback_source` in config). Spotify keeps +its direct-lookup optimization; everything else goes through search. Returns `(payload_dict, http_status_code)` so the route wrapper can `jsonify()` and return. @@ -29,7 +46,7 @@ from __future__ import annotations import logging import os from dataclasses import dataclass -from typing import Any, Callable +from typing import Any, Callable, Optional logger = logging.getLogger(__name__) @@ -44,6 +61,205 @@ class ArtistQualityDeps: get_current_profile_id: Callable[[], int] get_quality_tier_from_extension: Callable get_metadata_fallback_client: Callable[[], Any] + get_metadata_fallback_source: Callable[[], str] + + +def _has_complete_metadata(payload: Optional[dict]) -> bool: + """Reject matches with empty / missing core fields. Pre-fix, iTunes + returned matches that cleared the 0.7 confidence threshold while + having empty artist / album / title — those propagated as junk + wishlist entries displayed as 'unknown artist - unknown album - + unknown track'.""" + if not payload: + return False + if not (payload.get('name') or '').strip(): + return False + artists = payload.get('artists') or [] + has_artist = any( + (a.get('name') or '').strip() if isinstance(a, dict) else (a or '').strip() + for a in artists + ) + if not has_artist: + return False + album = payload.get('album') or {} + if isinstance(album, dict): + if not (album.get('name') or '').strip(): + return False + elif not (album or '').strip(): + return False + return True + + +def _build_payload_from_track(track_obj) -> dict: + """Build a Spotify-shaped wishlist payload from any metadata source's + Track-shaped object (Spotify Track, iTunes Track, Deezer Track, + Discogs Track — they all have the same .id / .name / .artists / + .album / .duration_ms / etc shape because each client mimics + Spotify's surface). + + The wishlist's downstream pipeline expects Spotify shape; this helper + is the single place that knows how to produce it. Replaces the + duplicated payload construction that used to live in the Spotify + search path AND the iTunes fallback path. + + Does NOT substitute defaults for missing artists / album / title — + ``_has_complete_metadata`` rejects empty matches downstream so the + user sees a clear failure instead of a junk wishlist entry with + fabricated values. + """ + image_url = getattr(track_obj, 'image_url', '') or '' + album_images = ( + [{'url': image_url, 'height': 600, 'width': 600}] + if image_url else [] + ) + artist_names = list(getattr(track_obj, 'artists', None) or []) + return { + 'id': getattr(track_obj, 'id', ''), + 'name': getattr(track_obj, 'name', '') or '', + 'artists': [{'name': a} for a in artist_names], + 'album': { + 'name': getattr(track_obj, 'album', '') or '', + 'artists': [{'name': a} for a in artist_names], + 'album_type': getattr(track_obj, 'album_type', None) or 'album', + 'images': album_images, + 'release_date': getattr(track_obj, 'release_date', '') or '', + 'total_tracks': 1, + }, + 'duration_ms': getattr(track_obj, 'duration_ms', 0) or 0, + 'track_number': getattr(track_obj, 'track_number', None) or 1, + 'disc_number': getattr(track_obj, 'disc_number', None) or 1, + 'popularity': getattr(track_obj, 'popularity', None) or 0, + 'preview_url': getattr(track_obj, 'preview_url', None), + 'external_urls': getattr(track_obj, 'external_urls', None) or {}, + } + + +def _spotify_direct_lookup(spotify_client, spotify_tid: str, + fallback_artist_name: str, + fallback_album_name: str, + fallback_title: str) -> Optional[dict]: + """Spotify-only direct-lookup optimization. Spotify's + ``get_track_details(id)`` returns rich `raw_data` already in the + wishlist payload shape; other sources don't have an equivalent + stored-ID-to-track API today, so we fall through to search for + them. + """ + try: + track_details = spotify_client.get_track_details(spotify_tid) + if not track_details: + return None + if track_details.get('raw_data'): + return track_details['raw_data'] + # Enhanced format — rebuild with images + album_data = track_details.get('album', {}) + album_images = [] + if album_data.get('id'): + try: + full_album = spotify_client.get_album(album_data['id']) + if full_album and full_album.get('images'): + album_images = full_album['images'] + except Exception: + pass + return { + 'id': spotify_tid, + 'name': track_details.get('name', fallback_title), + 'artists': [ + {'name': a} + for a in track_details.get('artists', [fallback_artist_name]) + ], + 'album': { + 'id': album_data.get('id', ''), + 'name': album_data.get('name', fallback_album_name), + 'album_type': album_data.get('album_type', 'album'), + 'release_date': album_data.get('release_date', ''), + 'total_tracks': album_data.get('total_tracks', 1), + 'artists': [ + {'name': a} + for a in album_data.get('artists', [fallback_artist_name]) + ], + 'images': album_images, + }, + 'duration_ms': track_details.get('duration_ms', 0), + 'track_number': track_details.get('track_number', 1), + 'disc_number': track_details.get('disc_number', 1), + 'popularity': 0, + 'preview_url': None, + 'external_urls': {}, + } + except Exception as exc: + logger.error(f"[Enhance] Spotify direct lookup failed for {spotify_tid}: {exc}") + return None + + +def _search_match(client, matching_engine, title: str, artist_name: str, + album_title: str) -> Optional[dict]: + """Search the configured primary source for a track matching + title + artist. Confidence threshold 0.7; album-tracks get a + small bonus over singles. Returns a wishlist payload built from + the best match, or None if nothing clears threshold. + + Source-agnostic — works for any client implementing + ``search_tracks(query, limit)`` returning Track-shaped objects. + """ + if not client: + return None + + temp_track = type('TempTrack', (), { + 'name': title, 'artists': [artist_name], 'album': album_title, + })() + try: + queries = matching_engine.generate_download_queries(temp_track) + except Exception: + return None + + best = None + best_conf = 0.0 + for query in queries[:3]: + try: + results = client.search_tracks(query, limit=5) + except Exception: + continue + if not results: + continue + for cand in results: + artist_conf = max( + (matching_engine.similarity_score( + matching_engine.normalize_string(artist_name), + matching_engine.normalize_string(a), + ) for a in (cand.artists or [artist_name])), + default=0, + ) + title_conf = matching_engine.similarity_score( + matching_engine.normalize_string(title), + matching_engine.normalize_string(cand.name), + ) + combined = artist_conf * 0.5 + title_conf * 0.5 + # Small bonus for album tracks over singles. + album_type = getattr(cand, 'album_type', None) or '' + if album_type == 'album': + combined += 0.02 + elif album_type == 'ep': + combined += 0.01 + if combined > best_conf and combined >= 0.7: + best_conf = combined + best = cand + if best_conf >= 0.9: + break + + if not best: + return None + + # Spotify search returns a richer dict via get_track_details — try + # to upgrade the match if the search-results client exposes that. + if hasattr(client, 'get_track_details'): + try: + full_details = client.get_track_details(best.id) + if full_details and full_details.get('raw_data'): + return full_details['raw_data'] + except Exception: + pass + + return _build_payload_from_track(best) def enhance_artist_quality(artist_id, track_ids, deps: ArtistQualityDeps): @@ -73,6 +289,12 @@ def enhance_artist_quality(artist_id, track_ids, deps: ArtistQualityDeps): track['_album_id'] = album.get('id') track_lookup[tid] = track + # Resolve the primary metadata source ONCE up front. All matching + # routes through this. Replaces the legacy hardcoded + # Spotify-direct → Spotify-search → iTunes-fallback chain. + primary_source = deps.get_metadata_fallback_source() + primary_client = deps.get_metadata_fallback_client() + enhanced_count = 0 failed_count = 0 failed_tracks = [] @@ -95,200 +317,51 @@ def enhance_artist_quality(artist_id, track_ids, deps: ArtistQualityDeps): title = track.get('title', '') or '' if not title.strip(): title = os.path.splitext(os.path.basename(file_path))[0] - spotify_tid = track.get('spotify_track_id') + album_title = track.get('_album_title', '') - # Build Spotify track data for wishlist matched_track_data = None - if spotify_tid and deps.spotify_client: - # Direct lookup via stored Spotify ID — raw_data has full Spotify API format - try: - track_details = deps.spotify_client.get_track_details(spotify_tid) - if track_details and track_details.get('raw_data'): - matched_track_data = track_details['raw_data'] - elif track_details: - # Enhanced format — rebuild with images for wishlist compatibility - album_data = track_details.get('album', {}) - album_images = [] - # Try to get album art from a full album lookup - if album_data.get('id'): - try: - full_album = deps.spotify_client.get_album(album_data['id']) - if full_album and full_album.get('images'): - album_images = full_album['images'] - except Exception: - pass - matched_track_data = { - 'id': spotify_tid, - 'name': track_details.get('name', title), - 'artists': [{'name': a} for a in track_details.get('artists', [artist_name])], - 'album': { - 'id': album_data.get('id', ''), - 'name': album_data.get('name', track.get('_album_title', '')), - 'album_type': album_data.get('album_type', 'album'), - 'release_date': album_data.get('release_date', ''), - 'total_tracks': album_data.get('total_tracks', 1), - 'artists': [{'name': a} for a in album_data.get('artists', [artist_name])], - 'images': album_images, - }, - 'duration_ms': track_details.get('duration_ms', track.get('duration', 0)), - 'track_number': track_details.get('track_number', track.get('track_number', 1)), - 'disc_number': track_details.get('disc_number', 1), - 'popularity': 0, - 'preview_url': None, - 'external_urls': {}, - } - except Exception as e: - logger.error(f"[Enhance] Spotify lookup failed for {spotify_tid}: {e}") - - if not matched_track_data and deps.spotify_client: - # Fallback: Spotify search matching — need full track data for wishlist - try: - temp_track = type('TempTrack', (), { - 'name': title, 'artists': [artist_name], - 'album': track.get('_album_title', '') - })() - search_queries = deps.matching_engine.generate_download_queries(temp_track) - best_match = None - best_match_raw = None - best_confidence = 0.0 - - for search_query in search_queries[:3]: # Limit queries - try: - results = deps.spotify_client.search_tracks(search_query, limit=5) - if not results: - continue - for sp_track in results: - artist_conf = max( - (deps.matching_engine.similarity_score( - deps.matching_engine.normalize_string(artist_name), - deps.matching_engine.normalize_string(a) - ) for a in (sp_track.artists or [artist_name])), - default=0 - ) - title_conf = deps.matching_engine.similarity_score( - deps.matching_engine.normalize_string(title), - deps.matching_engine.normalize_string(sp_track.name) - ) - combined = artist_conf * 0.5 + title_conf * 0.5 - # Small bonus for album tracks over singles - _at = getattr(sp_track, 'album_type', None) or '' - if _at == 'album': - combined += 0.02 - elif _at == 'ep': - combined += 0.01 - if combined > best_confidence and combined >= 0.7: - best_confidence = combined - best_match = sp_track - if best_confidence >= 0.9: - break - except Exception: - continue - - if best_match: - # Fetch full track data from Spotify for proper wishlist format - try: - full_details = deps.spotify_client.get_track_details(best_match.id) - if full_details and full_details.get('raw_data'): - matched_track_data = full_details['raw_data'] - else: - raise ValueError("No raw_data from get_track_details") - except Exception: - # Build from Track dataclass with image - album_images = [{'url': best_match.image_url}] if best_match.image_url else [] - matched_track_data = { - 'id': best_match.id, - 'name': best_match.name, - 'artists': [{'name': a} for a in best_match.artists], - 'album': { - 'name': best_match.album, - 'artists': [{'name': a} for a in best_match.artists], - 'album_type': 'album', - 'release_date': getattr(best_match, 'release_date', '') or '', - 'images': album_images, - }, - 'duration_ms': best_match.duration_ms, - 'popularity': best_match.popularity or 0, - 'preview_url': best_match.preview_url, - 'external_urls': best_match.external_urls or {}, - } - except Exception as e: - logger.error(f"[Enhance] Search match failed for {title}: {e}") - - # Fallback source when Spotify unavailable or no match found - if not matched_track_data: - try: - fallback_client = deps.get_metadata_fallback_client() - itunes_best = None - itunes_best_conf = 0.0 - - itunes_queries = deps.matching_engine.generate_download_queries( - type('TempTrack', (), { - 'name': title, 'artists': [artist_name], - 'album': track.get('_album_title', '') - })() + # 1. Spotify-only direct-lookup optimization. Other sources + # don't have a stored-ID-to-track API today. + if primary_source == 'spotify' and deps.spotify_client: + spotify_tid = track.get('spotify_track_id') + if spotify_tid: + matched_track_data = _spotify_direct_lookup( + deps.spotify_client, spotify_tid, + artist_name, album_title, title, ) - for search_query in itunes_queries[:3]: - try: - itunes_results = fallback_client.search_tracks(search_query, limit=5) - if not itunes_results: - continue - for it_track in itunes_results: - artist_conf = max( - (deps.matching_engine.similarity_score( - deps.matching_engine.normalize_string(artist_name), - deps.matching_engine.normalize_string(a) - ) for a in (it_track.artists or [artist_name])), - default=0 - ) - title_conf = deps.matching_engine.similarity_score( - deps.matching_engine.normalize_string(title), - deps.matching_engine.normalize_string(it_track.name) - ) - combined = artist_conf * 0.5 + title_conf * 0.5 - # Small bonus for album tracks over singles - _at = getattr(it_track, 'album_type', None) or '' - if _at == 'album': - combined += 0.02 - elif _at == 'ep': - combined += 0.01 - if combined > itunes_best_conf and combined >= 0.7: - itunes_best_conf = combined - itunes_best = it_track - if itunes_best_conf >= 0.9: - break - except Exception: - continue + # 2. Search match against the primary source (works for any + # source that implements search_tracks — Spotify, iTunes, + # Deezer, Discogs, Hydrabase). + if not matched_track_data: + try: + matched_track_data = _search_match( + primary_client, deps.matching_engine, + title, artist_name, album_title, + ) + except Exception as exc: + logger.error(f"[Enhance] {primary_source} search failed for {title}: {exc}") - if itunes_best: - album_images = [{'url': itunes_best.image_url, 'height': 600, 'width': 600}] if itunes_best.image_url else [] - matched_track_data = { - 'id': itunes_best.id, - 'name': itunes_best.name, - 'artists': [{'name': a} for a in itunes_best.artists], - 'album': { - 'name': itunes_best.album, - 'artists': [{'name': a} for a in itunes_best.artists], - 'album_type': 'album', - 'images': album_images, - 'release_date': itunes_best.release_date or '', - 'total_tracks': 1, - }, - 'duration_ms': itunes_best.duration_ms, - 'track_number': itunes_best.track_number or 1, - 'disc_number': itunes_best.disc_number or 1, - 'popularity': itunes_best.popularity or 0, - 'preview_url': itunes_best.preview_url, - 'external_urls': itunes_best.external_urls or {}, - } - logger.warning(f"[Enhance] Fallback match for {title}: {itunes_best.artists[0]} - {itunes_best.name} (conf: {itunes_best_conf:.3f})") - except Exception as e: - logger.error(f"[Enhance] Fallback source failed for {title}: {e}") + # 3. Reject matches with empty / missing core fields. + if not _has_complete_metadata(matched_track_data): + if matched_track_data: + logger.warning( + f"[Enhance] {primary_source} match for '{title}' rejected — " + f"empty title / album / artists (would render as 'unknown')" + ) + matched_track_data = None if not matched_track_data: failed_count += 1 - failed_tracks.append({'track_id': track_id, 'title': title, 'reason': 'No Spotify or fallback match'}) + failed_tracks.append({ + 'track_id': track_id, + 'title': title, + 'reason': ( + f'No usable {primary_source} match — ' + f'connect another metadata source for better coverage' + ), + }) continue # Add to wishlist with enhance source diff --git a/tests/artists/test_quality.py b/tests/artists/test_quality.py index 6d7574e8..bdba465d 100644 --- a/tests/artists/test_quality.py +++ b/tests/artists/test_quality.py @@ -91,9 +91,21 @@ def _build_deps( artist_detail=None, wishlist=None, fallback_client=None, + fallback_source=None, profile_id=1, quality_tier=('mp3_320', 4), ): + # Default source mirrors the runtime helper: 'spotify' when a Spotify + # client is wired, otherwise 'itunes'. Tests override via the + # ``fallback_source`` kwarg when they need a specific source name. + if fallback_source is None: + fallback_source = 'spotify' if spotify is not None else 'itunes' + # When primary is Spotify, the Spotify client serves both the direct + # lookup AND the search match — so the fallback_client doubles as the + # primary search client. Other sources use whatever fallback_client + # the test passes in. + if fallback_client is None and fallback_source == 'spotify': + fallback_client = spotify deps = aq.ArtistQualityDeps( spotify_client=spotify, matching_engine=matching_engine or _FakeMatchingEngine(), @@ -102,6 +114,7 @@ def _build_deps( get_current_profile_id=lambda: profile_id, get_quality_tier_from_extension=lambda fp: quality_tier, get_metadata_fallback_client=lambda: fallback_client, + get_metadata_fallback_source=lambda: fallback_source, ) return deps @@ -240,6 +253,85 @@ def test_fallback_source_when_spotify_none(): assert md['album']['images'] == [{'url': 'http://it', 'height': 600, 'width': 600}] +def test_dispatches_through_primary_source_not_spotify_specific(): + """Architectural pin: when the user's primary metadata source is + Discogs (or any non-Spotify source), the enhance flow searches + THAT source's client, not Spotify. Pre-fix the flow had a + hardcoded Spotify-direct → Spotify-search → iTunes-fallback chain + that ignored the user's actual configured primary source. + """ + discogs_track = _SpotifyTrack(id='dc-1', name='Track One', + artists=['Artist Name']) + discogs_track.track_number = 1 + discogs_track.disc_number = 1 + discogs_track.album = 'Album X' + discogs_calls = [] + + class _DiscogsStub: + def search_tracks(self, q, limit=5): + discogs_calls.append(q) + return [discogs_track] + + wishlist = _FakeWishlist() + deps = _build_deps( + spotify=None, + artist_detail=_artist_with_track(), + wishlist=wishlist, + fallback_client=_DiscogsStub(), + fallback_source='discogs', + ) + + payload, _ = aq.enhance_artist_quality('artist-1', ['t1'], deps) + + # Discogs (the configured primary) was the one searched; match queued. + assert discogs_calls, "Primary source (Discogs) was not searched" + assert payload['enhanced_count'] == 1 + assert wishlist.added[0]['spotify_track_data']['id'] == 'dc-1' + + +def test_spotify_direct_lookup_not_used_when_spotify_not_primary(): + """Architectural pin: even if the user has Spotify configured AND a + track has a stored spotify_track_id, the direct-lookup optimization + only runs when Spotify is the ACTIVE primary source. Otherwise it + must search the active primary instead. + """ + spotify_calls = [] + + class _SpotifyStub: + def get_track_details(self, tid): + spotify_calls.append(('details', tid)) + return None + def search_tracks(self, q, limit=5): + spotify_calls.append(('search', q)) + return [] + + discogs_track = _SpotifyTrack(id='dc-1', name='Track One', + artists=['Artist Name']) + discogs_track.track_number = 1 + discogs_track.disc_number = 1 + discogs_track.album = 'Album X' + + class _DiscogsStub: + def search_tracks(self, q, limit=5): + return [discogs_track] + + wishlist = _FakeWishlist() + deps = _build_deps( + spotify=_SpotifyStub(), + artist_detail=_artist_with_track(spotify_tid='sp-stored'), + wishlist=wishlist, + fallback_client=_DiscogsStub(), + fallback_source='discogs', + ) + + payload, _ = aq.enhance_artist_quality('artist-1', ['t1'], deps) + + # Spotify direct lookup must NOT be invoked when primary is discogs. + assert ('details', 'sp-stored') not in spotify_calls + # Discogs match was queued instead. + assert payload['enhanced_count'] == 1 + + # --------------------------------------------------------------------------- # Failure modes # --------------------------------------------------------------------------- @@ -266,21 +358,123 @@ def test_track_with_no_file_path_marked_failed(): def test_no_match_anywhere_marked_failed(): - """No Spotify match AND no fallback match → failed reason 'No Spotify or fallback match'.""" + """No primary-source match → failed reason names the source so the + user knows which one returned nothing.""" spotify = _FakeSpotify(track_details=None, search_results=[]) - fallback = type('FB', (), { - 'search_tracks': lambda self, q, limit=5: [], - })() deps = _build_deps( spotify=spotify, artist_detail=_artist_with_track(), - fallback_client=fallback, ) payload, _ = aq.enhance_artist_quality('artist-1', ['t1'], deps) assert payload['failed_count'] == 1 - assert 'No Spotify or fallback match' in payload['failed_tracks'][0]['reason'] + reason = payload['failed_tracks'][0]['reason'] + assert 'No usable spotify match' in reason + assert 'connect another metadata source' in reason + + +def test_no_match_without_spotify_uses_source_specific_reason(): + """When Spotify isn't connected and the active primary (e.g. iTunes) + finds nothing, the failure reason names iTunes specifically. Discord + case: user with no Spotify / Deezer saw enhance silently produce + 'unknown artist - unknown album - unknown track' wishlist entries + instead of a clear failure.""" + fallback = type('FB', (), { + 'search_tracks': lambda self, q, limit=5: [], + })() + deps = _build_deps( + spotify=None, + artist_detail=_artist_with_track(), + fallback_client=fallback, + fallback_source='itunes', + ) + + payload, _ = aq.enhance_artist_quality('artist-1', ['t1'], deps) + assert payload['failed_count'] == 1 + reason = payload['failed_tracks'][0]['reason'] + assert 'No usable itunes match' in reason + assert 'connect another metadata source' in reason + + +def test_fallback_match_with_empty_artist_rejected(): + """Per the user-reported bug: an iTunes match that clears the 0.7 + confidence threshold but has empty/missing artists is rejected + instead of producing a wishlist entry with empty artist field + (which the wishlist payload normalizer happily accepts and the + UI then displays as 'unknown artist').""" + fallback_track = _SpotifyTrack(id='it-empty', name='Track One', + artists=[], # empty artists list + image_url='') + fallback_track.track_number = 1 + fallback_track.disc_number = 1 + fallback_track.album = 'Album X' + fallback = type('FB', (), { + 'search_tracks': lambda self, q, limit=5: [fallback_track], + })() + wishlist = _FakeWishlist() + deps = _build_deps( + spotify=None, + artist_detail=_artist_with_track(), + wishlist=wishlist, + fallback_client=fallback, + ) + + payload, _ = aq.enhance_artist_quality('artist-1', ['t1'], deps) + + # No wishlist entry with empty fields — match was rejected. + assert payload['enhanced_count'] == 0 + assert payload['failed_count'] == 1 + assert wishlist.added == [] + + +def test_fallback_match_with_empty_album_rejected(): + """Empty album field on iTunes match → reject (was producing + 'unknown album' wishlist entries).""" + fallback_track = _SpotifyTrack(id='it-no-album', name='Track One', + artists=['Artist Name']) + fallback_track.track_number = 1 + fallback_track.disc_number = 1 + fallback_track.album = '' # empty + fallback = type('FB', (), { + 'search_tracks': lambda self, q, limit=5: [fallback_track], + })() + wishlist = _FakeWishlist() + deps = _build_deps( + spotify=None, + artist_detail=_artist_with_track(), + wishlist=wishlist, + fallback_client=fallback, + ) + + payload, _ = aq.enhance_artist_quality('artist-1', ['t1'], deps) + assert payload['enhanced_count'] == 0 + assert payload['failed_count'] == 1 + assert wishlist.added == [] + + +def test_fallback_match_with_empty_name_rejected(): + """Empty title on iTunes match → reject.""" + fallback_track = _SpotifyTrack(id='it-no-name', name='', + artists=['Artist Name']) + fallback_track.track_number = 1 + fallback_track.disc_number = 1 + fallback_track.album = 'Album X' + fallback = type('FB', (), { + 'search_tracks': lambda self, q, limit=5: [fallback_track], + })() + wishlist = _FakeWishlist() + deps = _build_deps( + spotify=None, + artist_detail=_artist_with_track(), + wishlist=wishlist, + fallback_client=fallback, + ) + + payload, _ = aq.enhance_artist_quality('artist-1', ['t1'], deps) + assert payload['enhanced_count'] == 0 + assert payload['failed_count'] == 1 + assert wishlist.added == [] # --------------------------------------------------------------------------- diff --git a/web_server.py b/web_server.py index 8355ea95..0c212ff6 100644 --- a/web_server.py +++ b/web_server.py @@ -9411,6 +9411,7 @@ def _build_artist_quality_deps(): get_current_profile_id=get_current_profile_id, get_quality_tier_from_extension=_get_quality_tier_from_extension, get_metadata_fallback_client=_get_metadata_fallback_client, + get_metadata_fallback_source=_get_metadata_fallback_source, ) diff --git a/webui/static/helper.js b/webui/static/helper.js index 607ba6c8..0907a7bd 100644 --- a/webui/static/helper.js +++ b/webui/static/helper.js @@ -3432,6 +3432,7 @@ const WHATS_NEW = { '2.4.2': [ // --- post-2.4.1 dev work — entries hidden by _getLatestWhatsNewVersion until the build version bumps --- { date: 'Unreleased — 2.4.2 dev cycle' }, + { title: 'Fix: Enhance Quality Producing "Unknown" Wishlist Entries Without Spotify', desc: 'discord report: clicking enhance quality on an artist with neither spotify nor deezer connected added tracks to the wishlist as "unknown artist - unknown album - unknown track". root cause was structural: `core/artists/quality.py` had a hardcoded spotify-direct → spotify-search → itunes-fallback chain that ignored the user\'s configured primary metadata source. when spotify wasn\'t connected, every track fell through to a brittle itunes-only fallback that occasionally returned matches with empty fields (cleared the 0.7 confidence threshold but missing artist / album / title), and those empty strings propagated through the wishlist payload normalizer\'s truthy-check passthrough as "Unknown". rewrote the flow source-agnostic: it now resolves the user\'s active primary metadata source once via `_get_metadata_fallback_source()` and dispatches direct-lookup + search through whichever client (spotify / itunes / deezer / discogs / hydrabase) the user has configured. spotify-direct-lookup remains as a per-source optimization (only spotify exposes `get_track_details(id)` returning rich raw payload), but otherwise every metadata source is treated equally — discogs users get a discogs search, hydrabase users get a hydrabase search, etc. matches with empty title / album / artists are rejected before the wishlist add so users see a clear "no usable {source} match — connect another metadata source" error instead of junk entries. 6 new tests pin: empty-field rejection paths (3), primary-source dispatch (2), source-named failure reason (1).', page: 'library' }, { title: 'Internal: Media Server Engine Cin/JohnBaumb Pass', desc: 'internal — applied the same architectural cleanups the download engine PR went through to the media server engine PR before review. (1) every server client (Plex / Jellyfin / Navidrome / SoulSync) now explicitly inherits `MediaServerClient` instead of relying on structural typing — drift in any class fails at the conformance test boundary. (2) generic accessors on the engine: `configured_clients()` (replaces per-server `if X and X.is_connected(): clients[name] = X` chains in web_server.py) and `reload_config(name=None)` (generic dispatch instead of per-client reload calls). (3) singleton factory: `get_media_server_engine()` / `set_media_server_engine()` matching the metadata + download engine shape. web_server.py boots via `set_media_server_engine(...)` so factory + global handle share state. (4) ~70 direct `plex_client.X` / `jellyfin_client.X` / `navidrome_client.X` / `soulsync_library_client.X` attribute reaches in web_server.py migrated to `media_server_engine.client(\'\').X`. ~60 standalone refs (truthy checks, media_client assignments, source-name tuples) also routed through the engine. (5) the per-server `plex_client` / `jellyfin_client` / `navidrome_client` / `soulsync_library_client` globals in web_server.py are gone entirely — engine owns the client instances now, every caller reaches via `media_server_engine.client(\'\')`. four multi-client consumers (`PlaylistSyncService`, `ListeningStatsWorker`, `WebScanManager`, discovery `SyncDeps`) refactored to take the engine instead of separate per-server kwargs. (6) `TrackInfo` and `PlaylistInfo` lifted out of `core/plex_client.py` / `jellyfin_client.py` / `navidrome_client.py` (each was defining a near-identical copy) into the neutral `core/media_server/types.py` module — same lift Cin caught on the download `TrackResult`/`AlbumResult`/`DownloadStatus` situation. consumers (matching engine, sync service) get one import. zero behavior change.' }, { title: 'Internal: Media Server Engine Foundation', desc: 'internal — companion to the download engine refactor. introduces a media-server engine + plugin contract on top of the four server clients (plex / jellyfin / navidrome / soulsync standalone). pre-refactor web_server.py held four separate per-server client globals that every dispatch site reached individually. new `core/media_server/` package provides `MediaServerEngine` that owns the per-server clients + a small set of generic accessors (`client(name)`, `active_client()`, `configured_clients()`, `reload_config(name)`) so call sites use one canonical lookup pattern. plugin contract requires the four methods every server actually implements (is_connected, ensure_connection, get_all_artists, get_all_album_ids) — methods that exist on most-but-not-all servers (search_tracks, trigger_library_scan, get_library_stats, etc.) are listed in `KNOWN_PER_SERVER_METHODS` for discoverability and reached directly via `engine.client(name).` since there\'s no uniform safe-default that fits every method. honest scope: the four uniform-shape `is_connected` dispatches were lifted into `engine.is_connected()` (the one cross-server wrapper kept on the engine); the ~18 server-specific chains that do genuinely different per-server work (playlist track replace, metadata sync, scan strategies) stay explicit at the call site per the "lift what\'s truly shared" standard, but reach the per-server client through the engine. 42 tests pin: per-server observable behavior (4 server pinning files, 20 tests), engine surface + accessor contracts (15 tests), structural conformance + explicit-inheritance (9 tests). zero behavior change for users.' }, { title: 'Drop SoundCloud Preview Snippets at Search Time', desc: 'soundcloud serves a ~30s preview clip for tracks that are gated behind go+ / login (very common for major-label uploads — official content basically doesn\'t exist on soundcloud, so what shows up is bootlegs, fan uploads, type beats, and these 30s previews). yt-dlp accepts the preview as the download payload, the post-download integrity check catches the duration mismatch and quarantines the file, but the user just sees "all candidates failed" with no explanation. previews also showed up in the candidate-review modal where clicking one bypassed validation and downloaded the same broken file. now `filter_soundcloud_previews` drops these candidates at every entry point — auto-search scoring, modal-cache fallback, AND the not-found raw-results path — so previews never reach the matcher OR the user. drops candidates < 35s or below half the expected duration, gated on expected being non-trivially long (>60s) so genuine short tracks still pass. also fixed a silent regression in the hybrid-fallback path where the per-source attribute removal left `getattr(orch, \'youtube\', None)` returning None for every source — fallback never fired. now resolves through the orchestrator\'s `client(name)` accessor.', page: 'downloads' },