#922: import search labelled Spotify Free users' primary source as 'Deezer'
A Spotify Free (no-auth) user saw 'Showing Discogs results - not from your primary source (Deezer)' on the manual album-import search. Root cause: get_primary_source() deliberately downgrades an unauthenticated Spotify to the working fallback (deezer) so client routing always yields a usable client - and the import payload reused that FUNCTIONAL value for the LABEL. The free source has no album-name search (SpotifyFreeMetadataClient.search_albums() returns []), so falling back for results is correct; only the label was wrong. Fix: get_primary_source_label() preserves the user's configured intent (Spotify Free reads as 'spotify') without touching client routing or the search chain. The import album/track/suggestions payloads now return the label; the functional source still drives the hydrabase-enqueue + fallback chain. Banner now reads 'not from your primary source (Spotify)'. Tests: seam tests for get_primary_source_label + route regression pinning the label/functional decoupling; updated 4 existing import-route tests.
This commit is contained in:
parent
d647fc8ad1
commit
4c47c01076
6 changed files with 141 additions and 8 deletions
|
|
@ -16,6 +16,7 @@ from core.imports.staging import (
|
||||||
AUDIO_EXTENSIONS,
|
AUDIO_EXTENSIONS,
|
||||||
get_import_suggestions_cache,
|
get_import_suggestions_cache,
|
||||||
get_primary_source as _get_primary_source,
|
get_primary_source as _get_primary_source,
|
||||||
|
get_primary_source_label as _get_primary_source_label,
|
||||||
get_staging_path as _get_staging_path,
|
get_staging_path as _get_staging_path,
|
||||||
read_staging_file_metadata as _read_staging_file_metadata,
|
read_staging_file_metadata as _read_staging_file_metadata,
|
||||||
refresh_import_suggestions_cache as _refresh_import_suggestions_cache,
|
refresh_import_suggestions_cache as _refresh_import_suggestions_cache,
|
||||||
|
|
@ -48,6 +49,7 @@ class ImportRouteRuntime:
|
||||||
read_staging_file_metadata: Callable[[str, str], Dict[str, Any]] = _read_staging_file_metadata
|
read_staging_file_metadata: Callable[[str, str], Dict[str, Any]] = _read_staging_file_metadata
|
||||||
read_tags: Callable[[str], Any] = _default_read_tags
|
read_tags: Callable[[str], Any] = _default_read_tags
|
||||||
get_primary_source: Callable[[], str] = _get_primary_source
|
get_primary_source: Callable[[], str] = _get_primary_source
|
||||||
|
get_primary_source_label: Callable[[], str] = _get_primary_source_label
|
||||||
search_import_albums: Callable[..., list] = _search_import_albums
|
search_import_albums: Callable[..., list] = _search_import_albums
|
||||||
search_import_tracks: Callable[..., list] = _search_import_tracks
|
search_import_tracks: Callable[..., list] = _search_import_tracks
|
||||||
build_album_import_match_payload: Callable[..., Dict[str, Any]] = build_album_import_match_payload
|
build_album_import_match_payload: Callable[..., Dict[str, Any]] = build_album_import_match_payload
|
||||||
|
|
@ -222,7 +224,7 @@ def staging_suggestions() -> tuple[Dict[str, Any], int]:
|
||||||
"success": True,
|
"success": True,
|
||||||
"suggestions": cache["suggestions"],
|
"suggestions": cache["suggestions"],
|
||||||
"ready": cache["built"],
|
"ready": cache["built"],
|
||||||
"primary_source": _get_primary_source(),
|
"primary_source": _get_primary_source_label(),
|
||||||
}, 200
|
}, 200
|
||||||
|
|
||||||
|
|
||||||
|
|
@ -239,7 +241,10 @@ def search_albums(runtime: ImportRouteRuntime, query: str, limit: int = 12) -> t
|
||||||
runtime.hydrabase_worker.enqueue(query, "albums")
|
runtime.hydrabase_worker.enqueue(query, "albums")
|
||||||
|
|
||||||
albums = runtime.search_import_albums(query, limit=limit)
|
albums = runtime.search_import_albums(query, limit=limit)
|
||||||
return {"success": True, "albums": albums, "primary_source": primary_source}, 200
|
# The label names the user's CONFIGURED source (Spotify Free reads as
|
||||||
|
# 'spotify', not the deezer fallback the functional source downgrades to).
|
||||||
|
return {"success": True, "albums": albums,
|
||||||
|
"primary_source": runtime.get_primary_source_label()}, 200
|
||||||
except Exception as exc:
|
except Exception as exc:
|
||||||
runtime.logger.error("Error searching albums for import: %s", exc)
|
runtime.logger.error("Error searching albums for import: %s", exc)
|
||||||
return {"success": False, "error": str(exc)}, 500
|
return {"success": False, "error": str(exc)}, 500
|
||||||
|
|
@ -385,7 +390,8 @@ def search_tracks(runtime: ImportRouteRuntime, query: str, limit: int = 10) -> t
|
||||||
runtime.hydrabase_worker.enqueue(query, "tracks")
|
runtime.hydrabase_worker.enqueue(query, "tracks")
|
||||||
|
|
||||||
tracks = runtime.search_import_tracks(query, limit=limit)
|
tracks = runtime.search_import_tracks(query, limit=limit)
|
||||||
return {"success": True, "tracks": tracks, "primary_source": primary_source}, 200
|
return {"success": True, "tracks": tracks,
|
||||||
|
"primary_source": runtime.get_primary_source_label()}, 200
|
||||||
except Exception as exc:
|
except Exception as exc:
|
||||||
runtime.logger.error("Error searching tracks for import: %s", exc)
|
runtime.logger.error("Error searching tracks for import: %s", exc)
|
||||||
return {"success": False, "error": str(exc)}, 500
|
return {"success": False, "error": str(exc)}, 500
|
||||||
|
|
|
||||||
|
|
@ -56,6 +56,12 @@ def get_primary_source() -> str:
|
||||||
return _get_primary_source()
|
return _get_primary_source()
|
||||||
|
|
||||||
|
|
||||||
|
def get_primary_source_label() -> str:
|
||||||
|
from core.metadata_service import get_primary_source_label as _get_primary_source_label
|
||||||
|
|
||||||
|
return _get_primary_source_label()
|
||||||
|
|
||||||
|
|
||||||
def get_source_priority(preferred_source: str):
|
def get_source_priority(preferred_source: str):
|
||||||
from core.metadata_service import get_source_priority as _get_source_priority
|
from core.metadata_service import get_source_priority as _get_source_priority
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -368,6 +368,24 @@ def get_primary_source(spotify_client_factory: Optional[MetadataClientFactory] =
|
||||||
return source
|
return source
|
||||||
|
|
||||||
|
|
||||||
|
def get_primary_source_label() -> str:
|
||||||
|
"""Configured primary source for UI that *names* "your primary source" (the
|
||||||
|
import-search fallback banner, etc.).
|
||||||
|
|
||||||
|
Identical to ``get_primary_source()`` except it does NOT downgrade a no-auth
|
||||||
|
Spotify Free user to the working fallback: Spotify Free (fallback_source=
|
||||||
|
'spotify' + metadata.spotify_free) is reported as 'spotify', because that IS
|
||||||
|
their configured source — even though free-text album search itself has no
|
||||||
|
free-path implementation and legitimately falls back to another provider.
|
||||||
|
``get_primary_source()`` keeps the downgrade so client routing always yields a
|
||||||
|
usable client; labels want the user's actual intent, not the fallback."""
|
||||||
|
_default = METADATA_SOURCE_PRIORITY[0]
|
||||||
|
source = _get_config_value("metadata.fallback_source", _default) or _default
|
||||||
|
if source == "spotify" and _get_config_value("metadata.spotify_free", False):
|
||||||
|
return "spotify"
|
||||||
|
return get_primary_source()
|
||||||
|
|
||||||
|
|
||||||
def get_spotify_disconnect_source(configured_source: Optional[str] = None) -> str:
|
def get_spotify_disconnect_source(configured_source: Optional[str] = None) -> str:
|
||||||
"""Return the active metadata source after Spotify is disconnected."""
|
"""Return the active metadata source after Spotify is disconnected."""
|
||||||
_default = METADATA_SOURCE_PRIORITY[0]
|
_default = METADATA_SOURCE_PRIORITY[0]
|
||||||
|
|
|
||||||
|
|
@ -51,6 +51,7 @@ from core.metadata.registry import (
|
||||||
get_itunes_client,
|
get_itunes_client,
|
||||||
get_primary_client,
|
get_primary_client,
|
||||||
get_primary_source,
|
get_primary_source,
|
||||||
|
get_primary_source_label,
|
||||||
get_spotify_client_for_profile,
|
get_spotify_client_for_profile,
|
||||||
get_registered_runtime_client,
|
get_registered_runtime_client,
|
||||||
get_source_priority,
|
get_source_priority,
|
||||||
|
|
@ -117,6 +118,7 @@ __all__ = [
|
||||||
"get_musicmap_similar_artists",
|
"get_musicmap_similar_artists",
|
||||||
"get_primary_client",
|
"get_primary_client",
|
||||||
"get_primary_source",
|
"get_primary_source",
|
||||||
|
"get_primary_source_label",
|
||||||
"get_spotify_client_for_profile",
|
"get_spotify_client_for_profile",
|
||||||
"get_registered_runtime_client",
|
"get_registered_runtime_client",
|
||||||
"get_spotify_client",
|
"get_spotify_client",
|
||||||
|
|
|
||||||
|
|
@ -207,7 +207,7 @@ def test_staging_suggestions_returns_cache_payload(monkeypatch):
|
||||||
"get_import_suggestions_cache",
|
"get_import_suggestions_cache",
|
||||||
lambda: {"suggestions": [{"album": "Album"}], "built": True},
|
lambda: {"suggestions": [{"album": "Album"}], "built": True},
|
||||||
)
|
)
|
||||||
monkeypatch.setattr(import_routes, "_get_primary_source", lambda: "deezer")
|
monkeypatch.setattr(import_routes, "_get_primary_source_label", lambda: "deezer")
|
||||||
|
|
||||||
payload, status = staging_suggestions()
|
payload, status = staging_suggestions()
|
||||||
|
|
||||||
|
|
@ -296,6 +296,7 @@ def test_search_albums_enqueues_hydrabase_and_caps_limit():
|
||||||
calls = []
|
calls = []
|
||||||
runtime = ImportRouteRuntime(
|
runtime = ImportRouteRuntime(
|
||||||
get_primary_source=lambda: "hydrabase",
|
get_primary_source=lambda: "hydrabase",
|
||||||
|
get_primary_source_label=lambda: "hydrabase",
|
||||||
hydrabase_worker=worker,
|
hydrabase_worker=worker,
|
||||||
dev_mode_enabled=True,
|
dev_mode_enabled=True,
|
||||||
search_import_albums=lambda query, limit: calls.append((query, limit)) or [{"id": "album-1"}],
|
search_import_albums=lambda query, limit: calls.append((query, limit)) or [{"id": "album-1"}],
|
||||||
|
|
@ -327,10 +328,15 @@ def test_search_albums_exposes_primary_source_when_chain_falls_back():
|
||||||
# serves results from a different source, the response must carry both
|
# serves results from a different source, the response must carry both
|
||||||
# `primary_source` (what the user configured) and per-album `source`
|
# `primary_source` (what the user configured) and per-album `source`
|
||||||
# (what actually served the result) so the UI can warn the user.
|
# (what actually served the result) so the UI can warn the user.
|
||||||
|
#
|
||||||
|
# The configured source for the BANNER is the label, NOT the functional
|
||||||
|
# source (issue #922): a Spotify Free user's functional source downgrades to
|
||||||
|
# the deezer fallback, but the banner must still name what they configured.
|
||||||
runtime = ImportRouteRuntime(
|
runtime = ImportRouteRuntime(
|
||||||
get_primary_source=lambda: "musicbrainz",
|
get_primary_source=lambda: "deezer", # functional (downgraded fallback)
|
||||||
|
get_primary_source_label=lambda: "spotify", # configured intent (Spotify Free)
|
||||||
search_import_albums=lambda query, limit: [
|
search_import_albums=lambda query, limit: [
|
||||||
{"id": "deezer-1", "name": "Album", "source": "deezer"},
|
{"id": "discogs-1", "name": "Album", "source": "discogs"},
|
||||||
],
|
],
|
||||||
logger=_FakeLogger(),
|
logger=_FakeLogger(),
|
||||||
)
|
)
|
||||||
|
|
@ -339,8 +345,8 @@ def test_search_albums_exposes_primary_source_when_chain_falls_back():
|
||||||
|
|
||||||
assert status == 200
|
assert status == 200
|
||||||
assert payload["success"] is True
|
assert payload["success"] is True
|
||||||
assert payload["primary_source"] == "musicbrainz"
|
assert payload["primary_source"] == "spotify" # label, not the deezer fallback
|
||||||
assert payload["albums"][0]["source"] == "deezer"
|
assert payload["albums"][0]["source"] == "discogs"
|
||||||
|
|
||||||
|
|
||||||
def test_search_tracks_enqueues_hydrabase_and_caps_limit():
|
def test_search_tracks_enqueues_hydrabase_and_caps_limit():
|
||||||
|
|
@ -348,6 +354,7 @@ def test_search_tracks_enqueues_hydrabase_and_caps_limit():
|
||||||
calls = []
|
calls = []
|
||||||
runtime = ImportRouteRuntime(
|
runtime = ImportRouteRuntime(
|
||||||
get_primary_source=lambda: "hydrabase",
|
get_primary_source=lambda: "hydrabase",
|
||||||
|
get_primary_source_label=lambda: "hydrabase",
|
||||||
hydrabase_worker=worker,
|
hydrabase_worker=worker,
|
||||||
dev_mode_enabled=True,
|
dev_mode_enabled=True,
|
||||||
search_import_tracks=lambda query, limit: calls.append((query, limit)) or [{"id": "track-1"}],
|
search_import_tracks=lambda query, limit: calls.append((query, limit)) or [{"id": "track-1"}],
|
||||||
|
|
|
||||||
94
tests/test_import_primary_source_label.py
Normal file
94
tests/test_import_primary_source_label.py
Normal file
|
|
@ -0,0 +1,94 @@
|
||||||
|
"""The import-search 'primary source' label must name the user's CONFIGURED source.
|
||||||
|
|
||||||
|
Bug #922: a Spotify Free (no-auth) user saw "Showing Discogs results - not from your
|
||||||
|
primary source (Deezer)" on the manual album-import search. Root cause: get_primary_source()
|
||||||
|
deliberately downgrades an unauthenticated Spotify to the working fallback (deezer) so
|
||||||
|
client routing always yields a usable client — and the import payload reused that
|
||||||
|
FUNCTIONAL value for the LABEL. The free source has no album-name search (SpotifyFree
|
||||||
|
.search_albums() returns []), so falling back for results is correct; only the label was
|
||||||
|
wrong. get_primary_source_label() preserves the configured intent (Spotify Free reads as
|
||||||
|
'spotify') without touching client routing, and the import route returns the label.
|
||||||
|
"""
|
||||||
|
|
||||||
|
from __future__ import annotations
|
||||||
|
|
||||||
|
import core.metadata.registry as registry
|
||||||
|
from core.imports.routes import ImportRouteRuntime, search_albums
|
||||||
|
|
||||||
|
|
||||||
|
class _AuthedSpotify:
|
||||||
|
def is_spotify_authenticated(self):
|
||||||
|
return True
|
||||||
|
|
||||||
|
|
||||||
|
class _UnauthedSpotify:
|
||||||
|
"""No-auth Spotify (free tier): officially unauthenticated."""
|
||||||
|
|
||||||
|
def is_spotify_authenticated(self):
|
||||||
|
return False
|
||||||
|
|
||||||
|
|
||||||
|
def _patch_cfg(monkeypatch, cfg, *, client=None):
|
||||||
|
monkeypatch.setattr(registry, "_get_config_value", lambda k, d=None: cfg.get(k, d))
|
||||||
|
monkeypatch.setattr(registry, "get_spotify_client", lambda client_factory=None: client)
|
||||||
|
|
||||||
|
|
||||||
|
# --- get_primary_source_label seam -------------------------------------------
|
||||||
|
|
||||||
|
def test_label_spotify_free_reads_as_spotify(monkeypatch):
|
||||||
|
"""THE FIX: no-auth Spotify Free is labelled 'spotify', not the deezer fallback."""
|
||||||
|
_patch_cfg(
|
||||||
|
monkeypatch,
|
||||||
|
{"metadata.fallback_source": "spotify", "metadata.spotify_free": True},
|
||||||
|
client=_UnauthedSpotify(),
|
||||||
|
)
|
||||||
|
assert registry.get_primary_source_label() == "spotify"
|
||||||
|
|
||||||
|
|
||||||
|
def test_label_spotify_authed_reads_as_spotify(monkeypatch):
|
||||||
|
_patch_cfg(
|
||||||
|
monkeypatch,
|
||||||
|
{"metadata.fallback_source": "spotify", "metadata.spotify_free": False},
|
||||||
|
client=_AuthedSpotify(),
|
||||||
|
)
|
||||||
|
assert registry.get_primary_source_label() == "spotify"
|
||||||
|
|
||||||
|
|
||||||
|
def test_label_spotify_unauthed_no_free_downgrades(monkeypatch):
|
||||||
|
"""Spotify configured but neither authed nor free → genuinely on the fallback,
|
||||||
|
so the label honestly reports the working default (not a misleading 'spotify')."""
|
||||||
|
_patch_cfg(
|
||||||
|
monkeypatch,
|
||||||
|
{"metadata.fallback_source": "spotify", "metadata.spotify_free": False},
|
||||||
|
client=_UnauthedSpotify(),
|
||||||
|
)
|
||||||
|
label = registry.get_primary_source_label()
|
||||||
|
assert label == registry.METADATA_SOURCE_PRIORITY[0] # deezer default
|
||||||
|
assert label != "spotify"
|
||||||
|
|
||||||
|
|
||||||
|
def test_label_non_spotify_source_unchanged(monkeypatch):
|
||||||
|
_patch_cfg(
|
||||||
|
monkeypatch,
|
||||||
|
{"metadata.fallback_source": "deezer", "metadata.spotify_free": False},
|
||||||
|
)
|
||||||
|
assert registry.get_primary_source_label() == "deezer"
|
||||||
|
|
||||||
|
|
||||||
|
# --- import route regression: label decoupled from functional source ----------
|
||||||
|
|
||||||
|
def test_search_albums_payload_uses_label_not_functional_source():
|
||||||
|
"""REGRESSION (#922): the payload's primary_source is the LABEL ('spotify'),
|
||||||
|
even though the functional source the search chain used downgraded to 'deezer'."""
|
||||||
|
runtime = ImportRouteRuntime(
|
||||||
|
get_primary_source=lambda: "deezer", # functional (downgraded)
|
||||||
|
get_primary_source_label=lambda: "spotify", # configured intent
|
||||||
|
search_import_albums=lambda q, limit=12: [{"name": "X", "source": "discogs"}],
|
||||||
|
hydrabase_worker=None,
|
||||||
|
dev_mode_enabled=False,
|
||||||
|
)
|
||||||
|
payload, status = search_albums(runtime, "some album")
|
||||||
|
assert status == 200
|
||||||
|
assert payload["primary_source"] == "spotify"
|
||||||
|
# The functional source is still free to differ (the chain genuinely used a fallback).
|
||||||
|
assert runtime.get_primary_source() == "deezer"
|
||||||
Loading…
Reference in a new issue