Merge pull request #453 from Nezreka/fix/watchlist-bulk-fallback-source

Bulk watchlist add: fall back through every source ID, not just active

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

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'

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)