diff --git a/core/spotify_client.py b/core/spotify_client.py index 0da306af..f04ca6d2 100644 --- a/core/spotify_client.py +++ b/core/spotify_client.py @@ -569,26 +569,43 @@ class SpotifyClient: return self._itunes # Fall back to iTunes if no Discogs token return self._itunes - def _free_enabled(self) -> bool: - """Opt-in switch for the no-creds SpotipyFree ('Spotify Free') source. - Default OFF — the user explicitly enables it (Settings), so there's no - surprise web-scraping for people who simply haven't connected Spotify.""" + def _free_selected(self) -> bool: + """Whether the user picked 'Spotify Free' as their metadata source + (the no-creds option, for when they haven't connected Spotify).""" try: return bool(config_manager.get('metadata.spotify_free', False)) except Exception: return False - def _free_available(self) -> bool: - """Whether the no-creds fallback can be offered at all: enabled in - config AND the SpotipyFree package is installed.""" + def _has_spotify_credentials(self) -> bool: + """Whether Spotify client credentials are configured (a 'connected' + user, even if currently rate-limit-banned). Drives the auto-bridge.""" + try: + cfg = config_manager.get_spotify_config() or {} + return bool(cfg.get('client_id') and cfg.get('client_secret')) + except Exception: + return False + + def _free_installed(self) -> bool: from core.spotify_free_metadata import spotify_free_installed - return self._free_enabled() and spotify_free_installed() + return spotify_free_installed() + + def _free_wanted(self) -> bool: + """Does the user want Spotify metadata at all? Either they have + credentials (connected → eligible for the rate-limit auto-bridge) or + they explicitly chose 'Spotify Free'. This is what keeps the free source + from auto-scraping for someone who never opted into Spotify.""" + return self._has_spotify_credentials() or self._free_selected() + + def _free_available(self) -> bool: + """Free CAN serve: package installed AND the user wants Spotify.""" + return self._free_installed() and self._free_wanted() def is_spotify_metadata_available(self) -> bool: """Whether SoulSync can serve Spotify metadata — real auth OR the - no-creds fallback. Availability gates (search resolve, enrichment - worker, watchlist) use THIS instead of ``is_spotify_authenticated()`` - so the fallback is reachable. Does NOT change auth semantics.""" + no-creds source. Availability gates (search resolve, enrichment worker, + watchlist) use THIS instead of ``is_spotify_authenticated()`` so the + free source is reachable. Does NOT change auth semantics.""" from core.spotify_free_metadata import should_offer_spotify_metadata try: authed = self.is_spotify_authenticated() @@ -597,13 +614,15 @@ class SpotifyClient: return should_offer_spotify_metadata(authed, self._free_available()) def _free_active(self) -> bool: - """Whether the no-creds SpotipyFree fallback may serve THIS request. + """Whether the no-creds source may serve THIS request: free available + AND official can't (no auth, or rate-limited). ``is_spotify_authenticated()`` + already returns False during a rate-limit ban; the explicit rate-limit + term covers the brief window before the auth cache refreshes. When authed + + healthy the official path returns first, so this never opens. - Only when official Spotify can't (no auth, or rate-limited — note - ``is_spotify_authenticated()`` already returns False during a rate-limit - ban) AND the fallback is available. When authed + healthy the official - path returns before any fallback, so this never opens. Delegates the - pure decision to ``core.spotify_free_metadata.should_use_free_fallback``.""" + Two activations fall out of this: a no-auth user who chose Spotify Free + (free is their source), and a connected user mid-rate-limit (free bridges + the ban) — see _free_wanted().""" from core.spotify_free_metadata import should_use_free_fallback if not self._free_available(): return False diff --git a/tests/test_spotify_free_metadata.py b/tests/test_spotify_free_metadata.py index a69f3355..ccfff97e 100644 --- a/tests/test_spotify_free_metadata.py +++ b/tests/test_spotify_free_metadata.py @@ -133,3 +133,56 @@ def test_normalize_artist_skips_imageless_sources(): 'visuals': {'avatarImage': {'sources': [{'height': 1}, {'url': 'u'}]}}} out = normalize_artist(raw) assert out['images'] == [{'url': 'u', 'height': None, 'width': None}] + + +# --------------------------------------------------------------------------- +# SpotifyClient gate model — auto-bridge vs explicit opt-in vs no-surprise +# (constructs the client via __new__ so no network/config init runs) +# --------------------------------------------------------------------------- + +from unittest.mock import patch # noqa: E402 +from core.spotify_client import SpotifyClient # noqa: E402 +import core.spotify_free_metadata as _sfm # noqa: E402 + + +def _gate(authed, has_creds, selected, installed, rate_limited): + c = SpotifyClient.__new__(SpotifyClient) + with patch.object(SpotifyClient, 'is_spotify_authenticated', return_value=authed), \ + patch('core.spotify_client.config_manager') as cm, \ + patch('core.spotify_client._is_globally_rate_limited', return_value=rate_limited), \ + patch.object(_sfm, 'spotify_free_installed', return_value=installed): + cm.get_spotify_config.return_value = ( + {'client_id': 'x', 'client_secret': 'y'} if has_creds else {}) + cm.get.side_effect = lambda k, d=None: selected if k == 'metadata.spotify_free' else d + return c.is_spotify_metadata_available(), c._free_active() + + +def test_connected_healthy_uses_official(): + avail, free = _gate(authed=True, has_creds=True, selected=False, installed=True, rate_limited=False) + assert avail is True and free is False # official; free never opens + + +def test_connected_ratelimited_bridges_to_free(): + avail, free = _gate(authed=False, has_creds=True, selected=False, installed=True, rate_limited=True) + assert avail is True and free is True # auto-bridge, no toggle needed + + +def test_no_auth_picked_spotify_free_serves(): + avail, free = _gate(authed=False, has_creds=False, selected=True, installed=True, rate_limited=False) + assert avail is True and free is True + + +def test_no_auth_not_opted_in_does_nothing(): + # The key no-surprise guarantee: a user who never chose Spotify gets no free. + avail, free = _gate(authed=False, has_creds=False, selected=False, installed=True, rate_limited=False) + assert avail is False and free is False + + +def test_selected_but_package_missing_is_graceful(): + avail, free = _gate(authed=False, has_creds=False, selected=True, installed=False, rate_limited=False) + assert avail is False and free is False + + +def test_connected_ratelimited_but_no_package_no_bridge(): + avail, free = _gate(authed=False, has_creds=True, selected=False, installed=False, rate_limited=True) + assert avail is False and free is False diff --git a/web_server.py b/web_server.py index 0ecd0b03..410579e9 100644 --- a/web_server.py +++ b/web_server.py @@ -2318,8 +2318,11 @@ def get_status(): if t.get('status') in ('downloading', 'searching', 'post_processing', 'queued', 'pending'): active_dl_count += 1 - # Spotify Free: tell the UI whether Spotify metadata is available even - # without auth (so the Settings source selector can offer it). + # Spotify Free: tell the UI (a) whether Spotify metadata is currently + # available (auth or free), and (b) whether the SpotipyFree package is + # installed — the latter is what makes 'Spotify Free' selectable in the + # source dropdown (selecting it is the opt-in, so it can't depend on + # already having selected it). spotify_status = dict(metadata_status['spotify']) try: spotify_status['metadata_available'] = bool( @@ -2327,6 +2330,11 @@ def get_status(): ) except Exception: spotify_status['metadata_available'] = bool(spotify_status.get('authenticated')) + try: + from core.spotify_free_metadata import spotify_free_installed + spotify_status['free_installed'] = spotify_free_installed() + except Exception: + spotify_status['free_installed'] = False status_data = { 'metadata_source': metadata_status['metadata_source'], diff --git a/webui/index.html b/webui/index.html index d5920273..3092c057 100644 --- a/webui/index.html +++ b/webui/index.html @@ -3838,21 +3838,15 @@ -
- -
Fetches Spotify metadata without API credentials. Works two ways: as a full Spotify source if you haven't connected your account, and as a bridge that keeps search & enrichment going when your connected Spotify hits a rate-limit. Unofficial & best-effort (can break if Spotify changes); it can't search albums by name or access your library, and never runs while your authenticated Spotify is working normally.
-
-
Choose the primary source for artist, album, and track metadata. Spotify needs an active session — or enable Spotify Free above. Discogs requires a personal token. MusicBrainz is always available but rate-limited to 1 req/sec.
+
Choose the primary source for artist, album, and track metadata. Spotify needs a connected account. Spotify Free pulls the same Spotify data without credentials (unofficial & best-effort — can break if Spotify changes, can't search albums by name, no library access). Either way, a connected Spotify that hits a rate-limit is automatically bridged by the free source if it's installed. Discogs requires a personal token. MusicBrainz is always available but rate-limited to 1 req/sec.
diff --git a/webui/static/settings.js b/webui/static/settings.js index 94800cde..1cff59a0 100644 --- a/webui/static/settings.js +++ b/webui/static/settings.js @@ -64,9 +64,13 @@ function syncMetadataSourceSelection(source) { function _isMetadataSourceSelectable(source) { if (source === 'spotify') { - // Selectable with real auth OR when Spotify Free (no-creds) is available. - return _lastStatusPayload?.spotify?.authenticated === true - || _lastStatusPayload?.spotify?.metadata_available === true; + // Official Spotify needs a connected session. + return _lastStatusPayload?.spotify?.authenticated === true; + } + if (source === 'spotify_free') { + // No-creds Spotify only needs the SpotipyFree package installed — + // selecting it IS the opt-in, so it must NOT depend on having selected it. + return _lastStatusPayload?.spotify?.free_installed === true; } if (source === 'discogs') { const token = document.getElementById('discogs-token'); @@ -1045,10 +1049,13 @@ async function loadSettingsData() { // Populate Discogs settings document.getElementById('discogs-token').value = settings.discogs?.token || ''; - // Populate Metadata source setting - document.getElementById('metadata-fallback-source').value = settings.metadata?.fallback_source || 'deezer'; - const spotifyFreeToggle = document.getElementById('metadata-spotify-free'); - if (spotifyFreeToggle) spotifyFreeToggle.checked = settings.metadata?.spotify_free === true; + // Populate Metadata source setting. 'Spotify Free' is stored as + // fallback_source='spotify' + spotify_free=true (so all downstream + // 'spotify' routing is unchanged) — map it back to the dropdown value. + const _fbSrc = settings.metadata?.fallback_source || 'deezer'; + const _metaSel = (_fbSrc === 'spotify' && settings.metadata?.spotify_free === true) + ? 'spotify_free' : _fbSrc; + document.getElementById('metadata-fallback-source').value = _metaSel; // Populate Hydrabase settings const hbConfig = settings.hydrabase || {}; @@ -2759,12 +2766,19 @@ async function saveSettings(quiet = false) { const discogsTokenPresent = !!discogsTokenInput?.value?.trim(); let metadataSource = metadataSourceSelect?.value || 'deezer'; const spotifySessionActive = _lastStatusPayload?.spotify?.authenticated === true; + const spotifyFreeInstalled = _lastStatusPayload?.spotify?.free_installed === true; if (metadataSource === 'spotify' && !spotifySessionActive) { metadataSource = _metadataSourceFallback('spotify'); if (metadataSourceSelect) metadataSourceSelect.value = metadataSource; if (!quiet) { showToast('Spotify is disconnected, so the primary metadata source was switched.', 'warning'); } + } else if (metadataSource === 'spotify_free' && !spotifyFreeInstalled) { + metadataSource = _metadataSourceFallback('spotify_free'); + if (metadataSourceSelect) metadataSourceSelect.value = metadataSource; + if (!quiet) { + showToast('Spotify Free needs the SpotipyFree package installed.', 'warning'); + } } else if (metadataSource === 'discogs' && !discogsTokenPresent) { metadataSource = _metadataSourceFallback('discogs'); if (metadataSourceSelect) metadataSourceSelect.value = metadataSource; @@ -2846,8 +2860,10 @@ async function saveSettings(quiet = false) { token: document.getElementById('discogs-token').value, }, metadata: { - fallback_source: metadataSource, - spotify_free: document.getElementById('metadata-spotify-free')?.checked === true + // 'Spotify Free' is stored as the spotify source + a flag, so all + // downstream 'spotify' routing is unchanged. + fallback_source: metadataSource === 'spotify_free' ? 'spotify' : metadataSource, + spotify_free: metadataSource === 'spotify_free' }, hydrabase: { url: document.getElementById('hydrabase-url').value,