diff --git a/core/watchlist/source_picker.py b/core/watchlist/source_picker.py new file mode 100644 index 00000000..666a5a91 --- /dev/null +++ b/core/watchlist/source_picker.py @@ -0,0 +1,67 @@ +"""Active-source-aware artist ID picker for bulk watchlist add. + +The bulk "Add unwatched library artists to watchlist" endpoint used +to drop artists silently whenever they didn't carry an ID for the +user's currently active metadata source. A Spotify-primary user with +library artists matched only against iTunes/Deezer would see them +counted as ``skipped_no_id`` and never make it onto the watchlist — +surfacing on Discord as "Library and Watchlist not syncing +correctly". The per-artist Enhanced View sync sometimes "fixed" them +because it re-ran enrichment that occasionally populated the missing +ID, but that workaround couldn't help artists Spotify simply doesn't +have. + +This helper picks the active source's ID first, then falls back +through every other supported source so an artist makes it onto the +watchlist as long as ANY metadata source can identify them. +""" + +from typing import Any, Dict, Optional, Tuple + + +# (source_name, library-artist-row column). Order is also the +# fallback priority — Spotify > iTunes > Deezer > Discogs by default +# coverage. The active source moves to the front of the queue inside +# pick_artist_id_for_watchlist. +SOURCE_ID_COLUMNS = ( + ('spotify', 'spotify_artist_id'), + ('itunes', 'itunes_artist_id'), + ('deezer', 'deezer_id'), + ('discogs', 'discogs_id'), +) + + +def pick_artist_id_for_watchlist( + artist: Dict[str, Any], + active_source: Optional[str], +) -> Tuple[Optional[str], Optional[str]]: + """Pick a (source-id, source-name) pair for adding ``artist`` to + the watchlist. + + Tries ``active_source`` first when it appears in + ``SOURCE_ID_COLUMNS``, then falls back through every other source + in registration order. Empty strings count as missing. Returns + ``(None, None)`` only when the artist truly has no usable source + ID — that's the only legitimate skip reason for the bulk-add + flow. + + The returned ID is always coerced to ``str`` because watchlist + columns are TEXT and SQLite will happily store the original int + type otherwise (which then breaks ID-based equality checks + between watchlist and library code paths). + """ + preferred = next( + ((src, col) for src, col in SOURCE_ID_COLUMNS if src == active_source), + None, + ) + ordered = [preferred] if preferred else [] + ordered.extend( + (src, col) for src, col in SOURCE_ID_COLUMNS if (src, col) != preferred + ) + for src, col in ordered: + if not src: + continue + value = artist.get(col) + if value: + return str(value), src + return None, None diff --git a/tests/test_watchlist_bulk_add.py b/tests/test_watchlist_bulk_add.py new file mode 100644 index 00000000..b193dd7c --- /dev/null +++ b/tests/test_watchlist_bulk_add.py @@ -0,0 +1,130 @@ +"""Regression tests for the bulk "Add unwatched library artists to +watchlist" endpoint. + +Discord report: bulk add silently skipped library artists that didn't +have an ID for the user's currently active metadata source. A +Spotify-primary user with library artists matched only against iTunes +or Deezer would see them counted as ``skipped_no_id`` and never make +it onto the watchlist — the user perceived this as "Library and +Watchlist not syncing correctly". + +These tests pin the new behaviour: try the active source first, then +fall back to any other source ID the artist carries. Drop only when +the artist has zero source IDs. +""" + +from core.watchlist.source_picker import pick_artist_id_for_watchlist + + +def _make_picker(active_source): + """Tiny adapter so test bodies stay readable as ``pick(artist)``.""" + return lambda artist: pick_artist_id_for_watchlist(artist, active_source) + + +# --------------------------------------------------------------------------- +# Happy paths +# --------------------------------------------------------------------------- + + +def test_active_source_id_takes_priority_when_present() -> None: + """When the artist has the active source's ID, that one wins — + other sources don't override it.""" + pick = _make_picker('spotify') + artist = { + 'spotify_artist_id': 'sp-123', + 'itunes_artist_id': 'it-456', + 'deezer_id': 'dz-789', + } + assert pick(artist) == ('sp-123', 'spotify') + + +def test_falls_back_to_itunes_when_active_spotify_missing() -> None: + """Spotify-primary user with an iTunes-only library artist must + still get the artist on the watchlist instead of being silently + skipped (the Discord-reported regression).""" + pick = _make_picker('spotify') + artist = { + 'itunes_artist_id': 'it-456', + 'deezer_id': 'dz-789', + } + assert pick(artist) == ('it-456', 'itunes') + + +def test_falls_back_to_deezer_when_active_and_itunes_missing() -> None: + """Order matters — iTunes is preferred over Deezer when both + fallbacks exist, matching the real-world catalogue coverage + ranking the picker uses.""" + pick = _make_picker('spotify') + artist = { + 'deezer_id': 'dz-789', + } + assert pick(artist) == ('dz-789', 'deezer') + + +def test_falls_back_to_discogs_as_last_resort() -> None: + pick = _make_picker('spotify') + artist = { + 'discogs_id': 'dg-999', + } + assert pick(artist) == ('dg-999', 'discogs') + + +def test_returns_none_when_artist_has_zero_source_ids() -> None: + """Drop only when the artist has no source IDs at all — that's + the only legitimate skip reason now.""" + pick = _make_picker('spotify') + assert pick({'name': 'Some Artist'}) == (None, None) + + +def test_active_source_itunes_picks_itunes_first() -> None: + """Active source ordering must work for non-Spotify primary too.""" + pick = _make_picker('itunes') + artist = { + 'spotify_artist_id': 'sp-123', + 'itunes_artist_id': 'it-456', + } + assert pick(artist) == ('it-456', 'itunes') + + +def test_active_source_deezer_picks_deezer_first() -> None: + pick = _make_picker('deezer') + artist = { + 'spotify_artist_id': 'sp-123', + 'deezer_id': 'dz-789', + } + assert pick(artist) == ('dz-789', 'deezer') + + +def test_unrecognized_active_source_still_falls_back() -> None: + """If active_source is something the picker doesn't know (e.g. + 'hydrabase'), still try every known source — better to add the + artist with whatever ID exists than reject silently.""" + pick = _make_picker('hydrabase') + artist = { + 'spotify_artist_id': 'sp-123', + } + # First fallback is Spotify per source_id_columns order + assert pick(artist) == ('sp-123', 'spotify') + + +def test_empty_string_id_does_not_count_as_present() -> None: + """SQL NULL surfaces as None; defensive check that empty string + also falls through to the next source.""" + pick = _make_picker('spotify') + artist = { + 'spotify_artist_id': '', + 'itunes_artist_id': 'it-456', + } + assert pick(artist) == ('it-456', 'itunes') + + +def test_numeric_id_is_coerced_to_string() -> None: + """Some sources return numeric IDs from SQLite; the watchlist DB + stores them as TEXT, so the picker must coerce to string before + add_artist_to_watchlist sees them.""" + pick = _make_picker('itunes') + artist = {'itunes_artist_id': 12345} + artist_id, src = pick(artist) + assert isinstance(artist_id, str) + assert artist_id == '12345' + assert src == 'itunes' diff --git a/web_server.py b/web_server.py index ef7b5970..3f4b55b7 100644 --- a/web_server.py +++ b/web_server.py @@ -24834,15 +24834,16 @@ def watchlist_all_unwatched_library_artists(): skipped_no_id = 0 skipped_already = 0 + # Try the active source's ID first, fall back through every other + # supported source. Pre-fix this loop required the active source's + # ID and silently dropped library artists that only had iTunes, + # Deezer, or Discogs IDs — surfaced as "Library and Watchlist not + # syncing correctly" on Discord because the bulk add reported + # "Added X" with no breakdown of why others were rejected. + from core.watchlist.source_picker import pick_artist_id_for_watchlist + for artist in unwatched_artists: - # Use only the active source's ID — matches frontend modal filtering - artist_id = None - if active_source == 'spotify': - artist_id = artist.get('spotify_artist_id') - elif active_source == 'itunes': - artist_id = artist.get('itunes_artist_id') - elif active_source == 'deezer': - artist_id = artist.get('deezer_id') + artist_id, picked_source = pick_artist_id_for_watchlist(artist, active_source) if not artist_id: skipped_no_id += 1 @@ -24857,8 +24858,7 @@ def watchlist_all_unwatched_library_artists(): skipped_already += 1 continue - src = active_source if active_source in ('spotify', 'itunes', 'deezer') else _get_metadata_fallback_source() - success = database.add_artist_to_watchlist(artist_id, artist_name, profile_id=get_current_profile_id(), source=src) + success = database.add_artist_to_watchlist(artist_id, artist_name, profile_id=get_current_profile_id(), source=picked_source) if success: added += 1 # Use library thumb_url if available (no HTTP calls needed)