Merge pull request #562 from Nezreka/fix/album-completeness-error-message-navidrome-issue-558
Album Completeness: surface diagnostic when resolver can't find album…
This commit is contained in:
commit
e0d3b8220a
5 changed files with 456 additions and 9 deletions
|
|
@ -32,7 +32,8 @@ from any background worker.
|
||||||
from __future__ import annotations
|
from __future__ import annotations
|
||||||
|
|
||||||
import os
|
import os
|
||||||
from typing import Any, Iterable, Optional
|
from dataclasses import dataclass, field
|
||||||
|
from typing import Any, Iterable, List, Optional, Tuple
|
||||||
|
|
||||||
from utils.logging_config import get_logger
|
from utils.logging_config import get_logger
|
||||||
|
|
||||||
|
|
@ -40,6 +41,33 @@ from utils.logging_config import get_logger
|
||||||
logger = get_logger("library.path_resolver")
|
logger = get_logger("library.path_resolver")
|
||||||
|
|
||||||
|
|
||||||
|
@dataclass
|
||||||
|
class ResolveAttempt:
|
||||||
|
"""Diagnostic record for a single `resolve_library_file_path` call.
|
||||||
|
|
||||||
|
Returned by `resolve_library_file_path_with_diagnostic` so callers
|
||||||
|
that need to surface a useful error message (instead of just a
|
||||||
|
silent None) can describe what was tried. Pure data — no side
|
||||||
|
effects, no rendering opinions.
|
||||||
|
|
||||||
|
Fields:
|
||||||
|
raw_path_existed: True if `os.path.exists(file_path)` returned
|
||||||
|
True at the start of the resolver. When this is True the
|
||||||
|
resolver short-circuits and `base_dirs_tried` will be empty.
|
||||||
|
base_dirs_tried: The ordered list of base directories the
|
||||||
|
resolver suffix-walked against (already filtered by
|
||||||
|
`os.path.isdir`).
|
||||||
|
had_config_manager: Whether a config_manager was supplied. Useful
|
||||||
|
for distinguishing "no candidates discovered" from "couldn't
|
||||||
|
even read config to discover".
|
||||||
|
had_plex_client: Same, for the Plex API probe.
|
||||||
|
"""
|
||||||
|
raw_path_existed: bool = False
|
||||||
|
base_dirs_tried: List[str] = field(default_factory=list)
|
||||||
|
had_config_manager: bool = False
|
||||||
|
had_plex_client: bool = False
|
||||||
|
|
||||||
|
|
||||||
def _docker_resolve_path(path_str: Any) -> Optional[str]:
|
def _docker_resolve_path(path_str: Any) -> Optional[str]:
|
||||||
"""Translate Windows-style paths to the Docker container layout.
|
"""Translate Windows-style paths to the Docker container layout.
|
||||||
|
|
||||||
|
|
@ -149,16 +177,52 @@ def resolve_library_file_path(
|
||||||
The first existing path on disk, or None when no match is found.
|
The first existing path on disk, or None when no match is found.
|
||||||
Never raises — failure is the None return.
|
Never raises — failure is the None return.
|
||||||
"""
|
"""
|
||||||
|
resolved, _ = resolve_library_file_path_with_diagnostic(
|
||||||
|
file_path,
|
||||||
|
transfer_folder=transfer_folder,
|
||||||
|
download_folder=download_folder,
|
||||||
|
config_manager=config_manager,
|
||||||
|
plex_client=plex_client,
|
||||||
|
)
|
||||||
|
return resolved
|
||||||
|
|
||||||
|
|
||||||
|
def resolve_library_file_path_with_diagnostic(
|
||||||
|
file_path: Any,
|
||||||
|
*,
|
||||||
|
transfer_folder: Optional[str] = None,
|
||||||
|
download_folder: Optional[str] = None,
|
||||||
|
config_manager: Any = None,
|
||||||
|
plex_client: Any = None,
|
||||||
|
) -> Tuple[Optional[str], ResolveAttempt]:
|
||||||
|
"""Same as ``resolve_library_file_path`` but also returns a
|
||||||
|
``ResolveAttempt`` describing what the resolver tried.
|
||||||
|
|
||||||
|
Use this when you need to surface a useful "we tried X, Y, Z" error
|
||||||
|
to the user instead of a silent None. Issue #558 (gabistek, Navidrome
|
||||||
|
on Docker): the resolver was returning None because Navidrome doesn't
|
||||||
|
expose library filesystem paths via API (unlike Plex), and the user
|
||||||
|
hadn't configured ``library.music_paths``. The Album Completeness
|
||||||
|
fix endpoint surfaced a generic "Could not determine album folder"
|
||||||
|
error with no diagnostic — user had no way to know what to configure.
|
||||||
|
"""
|
||||||
|
attempt = ResolveAttempt(
|
||||||
|
had_config_manager=config_manager is not None,
|
||||||
|
had_plex_client=plex_client is not None,
|
||||||
|
)
|
||||||
|
|
||||||
if not isinstance(file_path, str) or not file_path:
|
if not isinstance(file_path, str) or not file_path:
|
||||||
return None
|
return None, attempt
|
||||||
|
|
||||||
if os.path.exists(file_path):
|
if os.path.exists(file_path):
|
||||||
return file_path
|
attempt.raw_path_existed = True
|
||||||
|
return file_path, attempt
|
||||||
|
|
||||||
path_parts = file_path.replace("\\", "/").split("/")
|
path_parts = file_path.replace("\\", "/").split("/")
|
||||||
base_dirs = _collect_base_dirs(transfer_folder, download_folder, config_manager, plex_client)
|
base_dirs = _collect_base_dirs(transfer_folder, download_folder, config_manager, plex_client)
|
||||||
|
attempt.base_dirs_tried = list(base_dirs)
|
||||||
if not base_dirs:
|
if not base_dirs:
|
||||||
return None
|
return None, attempt
|
||||||
|
|
||||||
# Skip index 0 to avoid drive-letter / leading-slash artifacts
|
# Skip index 0 to avoid drive-letter / leading-slash artifacts
|
||||||
# (e.g. "E:" or "" from a leading "/").
|
# (e.g. "E:" or "" from a leading "/").
|
||||||
|
|
@ -166,8 +230,12 @@ def resolve_library_file_path(
|
||||||
for i in range(1, len(path_parts)):
|
for i in range(1, len(path_parts)):
|
||||||
candidate = os.path.join(base, *path_parts[i:])
|
candidate = os.path.join(base, *path_parts[i:])
|
||||||
if os.path.exists(candidate):
|
if os.path.exists(candidate):
|
||||||
return candidate
|
return candidate, attempt
|
||||||
return None
|
return None, attempt
|
||||||
|
|
||||||
|
|
||||||
__all__ = ["resolve_library_file_path"]
|
__all__ = [
|
||||||
|
"ResolveAttempt",
|
||||||
|
"resolve_library_file_path",
|
||||||
|
"resolve_library_file_path_with_diagnostic",
|
||||||
|
]
|
||||||
|
|
|
||||||
|
|
@ -1922,6 +1922,54 @@ class RepairWorker:
|
||||||
# Default
|
# Default
|
||||||
return '{num:02d} - {title}'
|
return '{num:02d} - {title}'
|
||||||
|
|
||||||
|
def _build_unresolvable_album_folder_error(self, attempt, sample_db_path):
|
||||||
|
"""Render a diagnostic error string for the Album Completeness
|
||||||
|
"couldn't find existing track on disk" failure mode.
|
||||||
|
|
||||||
|
Pre-fix this returned a flat
|
||||||
|
"Could not determine album folder from existing tracks"
|
||||||
|
which left users (especially Navidrome / Jellyfin Docker setups
|
||||||
|
where the resolver can't auto-discover library mounts) with no
|
||||||
|
way to know what to fix. The new message names the active media
|
||||||
|
server, shows one sample DB-recorded path, and lists the base
|
||||||
|
directories the resolver actually probed.
|
||||||
|
|
||||||
|
Args:
|
||||||
|
attempt: ``ResolveAttempt`` from the last resolver call.
|
||||||
|
May be ``None`` if no attempt was recorded (defensive).
|
||||||
|
sample_db_path: One example ``tracks.file_path`` value from
|
||||||
|
the album. Helps the user see what their media server is
|
||||||
|
reporting so they know what to mount / configure.
|
||||||
|
"""
|
||||||
|
active_server = 'unknown'
|
||||||
|
if self._config_manager is not None:
|
||||||
|
try:
|
||||||
|
getter = getattr(self._config_manager, 'get_active_media_server', None)
|
||||||
|
if callable(getter):
|
||||||
|
active_server = getter() or 'unknown'
|
||||||
|
else:
|
||||||
|
active_server = self._config_manager.get('active_media_server', 'unknown') or 'unknown'
|
||||||
|
except Exception as e:
|
||||||
|
logger.debug("active media server lookup failed: %s", e)
|
||||||
|
|
||||||
|
lines = [
|
||||||
|
"Could not find any existing track from this album on disk.",
|
||||||
|
f"Active media server: {active_server}.",
|
||||||
|
]
|
||||||
|
if sample_db_path:
|
||||||
|
lines.append(f"Example DB-recorded path: {sample_db_path}")
|
||||||
|
if attempt is not None:
|
||||||
|
if attempt.base_dirs_tried:
|
||||||
|
joined = ', '.join(attempt.base_dirs_tried)
|
||||||
|
lines.append(f"Probed base directories: {joined}")
|
||||||
|
else:
|
||||||
|
lines.append("No base directories were available to probe.")
|
||||||
|
lines.append(
|
||||||
|
"Fix: Settings → Library → Music Paths → add the path where "
|
||||||
|
"this container can read your library files."
|
||||||
|
)
|
||||||
|
return ' '.join(lines)
|
||||||
|
|
||||||
def _fix_incomplete_album(self, entity_type, entity_id, file_path, details):
|
def _fix_incomplete_album(self, entity_type, entity_id, file_path, details):
|
||||||
"""Auto-fill an incomplete album by finding missing tracks in the library.
|
"""Auto-fill an incomplete album by finding missing tracks in the library.
|
||||||
|
|
||||||
|
|
@ -1964,14 +2012,24 @@ class RepairWorker:
|
||||||
download_folder = self._config_manager.get('soulseek.download_path', '')
|
download_folder = self._config_manager.get('soulseek.download_path', '')
|
||||||
|
|
||||||
album_folder = None
|
album_folder = None
|
||||||
|
last_attempt = None
|
||||||
|
sample_db_path = None
|
||||||
for t in existing_tracks:
|
for t in existing_tracks:
|
||||||
resolved = _resolve_file_path(t.file_path, self.transfer_folder, download_folder, config_manager=self._config_manager)
|
from core.library.path_resolver import resolve_library_file_path_with_diagnostic
|
||||||
|
resolved, attempt = resolve_library_file_path_with_diagnostic(
|
||||||
|
t.file_path, transfer_folder=self.transfer_folder,
|
||||||
|
download_folder=download_folder, config_manager=self._config_manager,
|
||||||
|
)
|
||||||
|
last_attempt = attempt
|
||||||
|
if sample_db_path is None and isinstance(t.file_path, str) and t.file_path:
|
||||||
|
sample_db_path = t.file_path
|
||||||
if resolved and os.path.exists(resolved):
|
if resolved and os.path.exists(resolved):
|
||||||
album_folder = os.path.dirname(resolved)
|
album_folder = os.path.dirname(resolved)
|
||||||
break
|
break
|
||||||
|
|
||||||
if not album_folder:
|
if not album_folder:
|
||||||
return {'success': False, 'error': 'Could not determine album folder from existing tracks'}
|
return {'success': False,
|
||||||
|
'error': self._build_unresolvable_album_folder_error(last_attempt, sample_db_path)}
|
||||||
|
|
||||||
# Detect filename pattern
|
# Detect filename pattern
|
||||||
resolved_paths = []
|
resolved_paths = []
|
||||||
|
|
|
||||||
|
|
@ -403,3 +403,139 @@ def test_docker_resolve_path_pass_through_outside_docker(monkeypatch) -> None:
|
||||||
monkeypatch.setattr(os.path, "exists",
|
monkeypatch.setattr(os.path, "exists",
|
||||||
lambda p: False if p == "/.dockerenv" else real_exists(p))
|
lambda p: False if p == "/.dockerenv" else real_exists(p))
|
||||||
assert path_resolver._docker_resolve_path("H:\\Music\\track.flac") == "H:\\Music\\track.flac"
|
assert path_resolver._docker_resolve_path("H:\\Music\\track.flac") == "H:\\Music\\track.flac"
|
||||||
|
|
||||||
|
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
# Diagnostic helper — issue #558 (gabistek, Navidrome on Docker)
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
#
|
||||||
|
# `resolve_library_file_path_with_diagnostic` returns
|
||||||
|
# `(resolved, ResolveAttempt)` so callers can render a useful error
|
||||||
|
# instead of a silent None. Pre-fix the Album Completeness "Auto-Fill"
|
||||||
|
# button surfaced "Could not determine album folder from existing
|
||||||
|
# tracks" with no diagnostic, leaving Navidrome users (whose Subsonic
|
||||||
|
# API doesn't expose library paths the way Plex's does) with no signal
|
||||||
|
# about what to configure.
|
||||||
|
|
||||||
|
|
||||||
|
from core.library.path_resolver import ( # noqa: E402
|
||||||
|
ResolveAttempt,
|
||||||
|
resolve_library_file_path_with_diagnostic,
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
class TestResolveAttemptShape:
|
||||||
|
def test_returns_tuple_of_path_and_attempt(self, tmp_path: Path) -> None:
|
||||||
|
real = tmp_path / "track.flac"
|
||||||
|
real.write_bytes(b"a")
|
||||||
|
result = resolve_library_file_path_with_diagnostic(str(real))
|
||||||
|
assert isinstance(result, tuple)
|
||||||
|
assert len(result) == 2
|
||||||
|
path, attempt = result
|
||||||
|
assert path == str(real)
|
||||||
|
assert isinstance(attempt, ResolveAttempt)
|
||||||
|
|
||||||
|
def test_raw_path_existed_true_when_short_circuit(self, tmp_path: Path) -> None:
|
||||||
|
"""Happy path → resolver short-circuits at the first
|
||||||
|
`os.path.exists` check; `base_dirs_tried` stays empty."""
|
||||||
|
real = tmp_path / "track.flac"
|
||||||
|
real.write_bytes(b"a")
|
||||||
|
_, attempt = resolve_library_file_path_with_diagnostic(str(real))
|
||||||
|
assert attempt.raw_path_existed is True
|
||||||
|
assert attempt.base_dirs_tried == []
|
||||||
|
|
||||||
|
def test_raw_path_existed_false_when_walking(self, tmp_path: Path) -> None:
|
||||||
|
"""When the raw path doesn't exist but the suffix-walk finds it,
|
||||||
|
the attempt should report `raw_path_existed=False` and list the
|
||||||
|
base dir that succeeded among `base_dirs_tried`."""
|
||||||
|
# Create the file under a real base dir at a different parent
|
||||||
|
base = tmp_path / "library"
|
||||||
|
target = base / "Artist" / "Album" / "track.flac"
|
||||||
|
target.parent.mkdir(parents=True)
|
||||||
|
target.write_bytes(b"a")
|
||||||
|
|
||||||
|
# DB stores it as if scanned at /music/Artist/Album/track.flac
|
||||||
|
db_path = "/music/Artist/Album/track.flac"
|
||||||
|
path, attempt = resolve_library_file_path_with_diagnostic(
|
||||||
|
db_path, transfer_folder=str(base),
|
||||||
|
)
|
||||||
|
assert path == str(target), f"suffix-walk should have found the file under {base}"
|
||||||
|
assert attempt.raw_path_existed is False
|
||||||
|
assert str(base) in attempt.base_dirs_tried
|
||||||
|
|
||||||
|
|
||||||
|
class TestDiagnosticForFailedResolves:
|
||||||
|
def test_no_base_dirs_returns_none_with_empty_attempt(self) -> None:
|
||||||
|
"""No transfer/download/config/plex → resolver can't probe.
|
||||||
|
Diagnostic must report empty `base_dirs_tried` so the caller can
|
||||||
|
render a "no probe sources configured" hint."""
|
||||||
|
path, attempt = resolve_library_file_path_with_diagnostic(
|
||||||
|
"/music/Artist/Album/track.flac",
|
||||||
|
)
|
||||||
|
assert path is None
|
||||||
|
assert attempt.raw_path_existed is False
|
||||||
|
assert attempt.base_dirs_tried == []
|
||||||
|
assert attempt.had_config_manager is False
|
||||||
|
assert attempt.had_plex_client is False
|
||||||
|
|
||||||
|
def test_base_dirs_listed_even_when_walk_fails(self, tmp_path: Path) -> None:
|
||||||
|
"""When base dirs exist but the suffix-walk doesn't find the
|
||||||
|
file, `base_dirs_tried` must still report what was probed.
|
||||||
|
Lets the caller surface "we tried X, Y, Z" in the error."""
|
||||||
|
base = tmp_path / "transfer"
|
||||||
|
base.mkdir()
|
||||||
|
# Don't create the target file — the walk will fail
|
||||||
|
path, attempt = resolve_library_file_path_with_diagnostic(
|
||||||
|
"/music/Artist/Album/missing.flac",
|
||||||
|
transfer_folder=str(base),
|
||||||
|
)
|
||||||
|
assert path is None
|
||||||
|
assert str(base) in attempt.base_dirs_tried
|
||||||
|
|
||||||
|
def test_had_flags_track_caller_inputs(self, tmp_path: Path) -> None:
|
||||||
|
"""`had_config_manager` / `had_plex_client` reflect what the
|
||||||
|
caller passed in — useful for distinguishing 'caller didn't
|
||||||
|
wire up the optional input' from 'optional input was wired up
|
||||||
|
but produced no usable base dirs'."""
|
||||||
|
config = MagicMock()
|
||||||
|
config.get.return_value = "" # no config-driven paths
|
||||||
|
plex = SimpleNamespace(server=None, music_library=None)
|
||||||
|
|
||||||
|
path, attempt = resolve_library_file_path_with_diagnostic(
|
||||||
|
"/music/Artist/track.flac",
|
||||||
|
config_manager=config,
|
||||||
|
plex_client=plex,
|
||||||
|
)
|
||||||
|
assert path is None
|
||||||
|
assert attempt.had_config_manager is True
|
||||||
|
assert attempt.had_plex_client is True
|
||||||
|
|
||||||
|
|
||||||
|
class TestBackwardsCompat:
|
||||||
|
def test_existing_resolve_function_delegates_to_diagnostic(self, tmp_path: Path) -> None:
|
||||||
|
"""The non-diagnostic `resolve_library_file_path` is now a thin
|
||||||
|
wrapper that drops the attempt. Pin that the legacy signature
|
||||||
|
still returns the same path values across all the cases the
|
||||||
|
old function covered, so existing callers don't see drift."""
|
||||||
|
real = tmp_path / "track.flac"
|
||||||
|
real.write_bytes(b"a")
|
||||||
|
|
||||||
|
# Happy path
|
||||||
|
assert resolve_library_file_path(str(real)) == str(real)
|
||||||
|
|
||||||
|
# Suffix-walk path
|
||||||
|
base = tmp_path / "lib"
|
||||||
|
target = base / "Artist" / "Album" / "track.flac"
|
||||||
|
target.parent.mkdir(parents=True)
|
||||||
|
target.write_bytes(b"a")
|
||||||
|
result = resolve_library_file_path(
|
||||||
|
"/music/Artist/Album/track.flac",
|
||||||
|
transfer_folder=str(base),
|
||||||
|
)
|
||||||
|
assert result == str(target)
|
||||||
|
|
||||||
|
# Failure path
|
||||||
|
assert resolve_library_file_path(
|
||||||
|
"/music/Artist/missing.flac",
|
||||||
|
transfer_folder=str(base),
|
||||||
|
) is None
|
||||||
|
|
|
||||||
184
tests/test_repair_worker_unresolvable_folder_error.py
Normal file
184
tests/test_repair_worker_unresolvable_folder_error.py
Normal file
|
|
@ -0,0 +1,184 @@
|
||||||
|
"""Pin the diagnostic error string from
|
||||||
|
``RepairWorker._build_unresolvable_album_folder_error``.
|
||||||
|
|
||||||
|
GitHub issue #558 (gabistek, Navidrome on Docker / Arch host): the
|
||||||
|
Album Completeness Auto-Fill button surfaced a flat "Could not
|
||||||
|
determine album folder from existing tracks" error with no diagnostic.
|
||||||
|
Reporter is on Navidrome, which (unlike Plex) has no API that exposes
|
||||||
|
filesystem library paths — so the resolver returns None whenever the
|
||||||
|
DB-recorded path doesn't already exist as-is in SoulSync's container
|
||||||
|
view AND the user hasn't manually configured Settings → Library →
|
||||||
|
Music Paths.
|
||||||
|
|
||||||
|
The fix replaces the flat string with a multi-part diagnostic naming
|
||||||
|
the active media server, showing one sample DB path, listing the base
|
||||||
|
directories the resolver actually probed, and pointing the user at the
|
||||||
|
config that would unblock them. These tests pin each part so future
|
||||||
|
copy edits don't accidentally drop the actionable hint or the sample
|
||||||
|
path.
|
||||||
|
"""
|
||||||
|
|
||||||
|
from __future__ import annotations
|
||||||
|
|
||||||
|
import sys
|
||||||
|
import types
|
||||||
|
from types import SimpleNamespace
|
||||||
|
|
||||||
|
|
||||||
|
# ── Stub modules that the import of core.repair_worker pulls in ──
|
||||||
|
if "spotipy" not in sys.modules:
|
||||||
|
spotipy = types.ModuleType("spotipy")
|
||||||
|
|
||||||
|
class _DummySpotify:
|
||||||
|
def __init__(self, *args, **kwargs):
|
||||||
|
pass
|
||||||
|
|
||||||
|
oauth2 = types.ModuleType("spotipy.oauth2")
|
||||||
|
|
||||||
|
class _DummyOAuth:
|
||||||
|
def __init__(self, *args, **kwargs):
|
||||||
|
pass
|
||||||
|
|
||||||
|
spotipy.Spotify = _DummySpotify
|
||||||
|
oauth2.SpotifyOAuth = _DummyOAuth
|
||||||
|
oauth2.SpotifyClientCredentials = _DummyOAuth
|
||||||
|
spotipy.oauth2 = oauth2
|
||||||
|
sys.modules["spotipy"] = spotipy
|
||||||
|
sys.modules["spotipy.oauth2"] = oauth2
|
||||||
|
|
||||||
|
if "config.settings" not in sys.modules:
|
||||||
|
config_pkg = types.ModuleType("config")
|
||||||
|
settings_mod = types.ModuleType("config.settings")
|
||||||
|
|
||||||
|
class _DummyConfigManager:
|
||||||
|
def get(self, key, default=None):
|
||||||
|
return default
|
||||||
|
|
||||||
|
def get_active_media_server(self):
|
||||||
|
return "plex"
|
||||||
|
|
||||||
|
settings_mod.config_manager = _DummyConfigManager()
|
||||||
|
config_pkg.settings = settings_mod
|
||||||
|
sys.modules["config"] = config_pkg
|
||||||
|
sys.modules["config.settings"] = settings_mod
|
||||||
|
|
||||||
|
|
||||||
|
from core.library.path_resolver import ResolveAttempt
|
||||||
|
from core.repair_worker import RepairWorker
|
||||||
|
|
||||||
|
|
||||||
|
def _make_worker(active_server="plex"):
|
||||||
|
"""Bare RepairWorker with a config_manager that reports the given
|
||||||
|
active media server. We never run the full job — just exercise the
|
||||||
|
diagnostic builder."""
|
||||||
|
worker = RepairWorker(database=SimpleNamespace())
|
||||||
|
cfg = SimpleNamespace()
|
||||||
|
cfg.get_active_media_server = lambda: active_server
|
||||||
|
cfg.get = lambda key, default=None: default
|
||||||
|
worker._config_manager = cfg
|
||||||
|
return worker
|
||||||
|
|
||||||
|
|
||||||
|
def test_error_names_active_media_server():
|
||||||
|
"""User needs to know which server's path conventions are at play
|
||||||
|
so they can set the right mount in Settings."""
|
||||||
|
worker = _make_worker(active_server="navidrome")
|
||||||
|
msg = worker._build_unresolvable_album_folder_error(
|
||||||
|
ResolveAttempt(base_dirs_tried=["/app/Transfer"]),
|
||||||
|
"/music/Artist/Album/track.flac",
|
||||||
|
)
|
||||||
|
assert "navidrome" in msg.lower(), (
|
||||||
|
f"Active server name must appear in error; got: {msg}"
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def test_error_includes_sample_db_path():
|
||||||
|
"""One concrete path lets the user see what their media server
|
||||||
|
is reporting — usually enough to reverse-engineer the right mount."""
|
||||||
|
worker = _make_worker()
|
||||||
|
sample = "/music/Kendrick Lamar/Mr. Morale/01 - United in Grief.flac"
|
||||||
|
msg = worker._build_unresolvable_album_folder_error(
|
||||||
|
ResolveAttempt(base_dirs_tried=["/app/Transfer"]),
|
||||||
|
sample,
|
||||||
|
)
|
||||||
|
assert sample in msg, (
|
||||||
|
f"Sample DB path must appear verbatim in error; got: {msg}"
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def test_error_lists_base_dirs_tried():
|
||||||
|
"""User needs to know what the resolver probed — otherwise they
|
||||||
|
can't tell whether to add a new mount or whether the existing one
|
||||||
|
just doesn't match the recorded path."""
|
||||||
|
worker = _make_worker()
|
||||||
|
attempt = ResolveAttempt(
|
||||||
|
base_dirs_tried=["/app/Transfer", "/downloads", "/library"],
|
||||||
|
)
|
||||||
|
msg = worker._build_unresolvable_album_folder_error(attempt, "/music/x.flac")
|
||||||
|
for base in attempt.base_dirs_tried:
|
||||||
|
assert base in msg, f"Probed base dir {base!r} missing from error: {msg}"
|
||||||
|
|
||||||
|
|
||||||
|
def test_error_calls_out_no_base_dirs_when_empty():
|
||||||
|
"""When the resolver had nothing to probe, that's a different
|
||||||
|
failure mode than "tried 3 dirs and failed" — the user needs
|
||||||
|
different action. Pin that the message distinguishes them."""
|
||||||
|
worker = _make_worker()
|
||||||
|
msg = worker._build_unresolvable_album_folder_error(
|
||||||
|
ResolveAttempt(base_dirs_tried=[]),
|
||||||
|
"/music/x.flac",
|
||||||
|
)
|
||||||
|
assert "no base director" in msg.lower(), (
|
||||||
|
f"Empty-base-dirs case must surface 'no base directories'; got: {msg}"
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def test_error_always_includes_settings_hint():
|
||||||
|
"""The actionable fix line must always appear regardless of which
|
||||||
|
failure mode fired. This is the part the user needs to act on."""
|
||||||
|
worker = _make_worker()
|
||||||
|
for attempt in (
|
||||||
|
ResolveAttempt(base_dirs_tried=[]),
|
||||||
|
ResolveAttempt(base_dirs_tried=["/app/Transfer"]),
|
||||||
|
None,
|
||||||
|
):
|
||||||
|
msg = worker._build_unresolvable_album_folder_error(attempt, "/music/x.flac")
|
||||||
|
assert "Settings" in msg, f"Settings hint missing for attempt={attempt}; got: {msg}"
|
||||||
|
assert "Music Paths" in msg, f"Music Paths hint missing for attempt={attempt}; got: {msg}"
|
||||||
|
|
||||||
|
|
||||||
|
def test_error_handles_none_attempt_defensively():
|
||||||
|
"""If for some reason no ResolveAttempt is collected (e.g. zero
|
||||||
|
existing tracks loop never ran), the helper must not crash. It
|
||||||
|
can omit the probe-detail line but must still emit the actionable
|
||||||
|
Settings hint."""
|
||||||
|
worker = _make_worker()
|
||||||
|
msg = worker._build_unresolvable_album_folder_error(None, "/music/x.flac")
|
||||||
|
assert "Settings" in msg, f"None attempt must still emit Settings hint; got: {msg}"
|
||||||
|
assert "/music/x.flac" in msg
|
||||||
|
|
||||||
|
|
||||||
|
def test_error_handles_missing_sample_path():
|
||||||
|
"""If we couldn't sample a DB path (e.g. all entries had None
|
||||||
|
file_path), the path line is omitted but the rest of the message
|
||||||
|
still renders."""
|
||||||
|
worker = _make_worker()
|
||||||
|
msg = worker._build_unresolvable_album_folder_error(
|
||||||
|
ResolveAttempt(base_dirs_tried=["/app/Transfer"]),
|
||||||
|
None,
|
||||||
|
)
|
||||||
|
assert "Settings" in msg
|
||||||
|
# No sample-path line means no "Example DB-recorded path" prefix
|
||||||
|
assert "Example DB-recorded path:" not in msg
|
||||||
|
|
||||||
|
|
||||||
|
def test_error_handles_missing_config_manager():
|
||||||
|
"""RepairWorker may be constructed without a config_manager; the
|
||||||
|
builder shouldn't crash and should fall back to 'unknown' for the
|
||||||
|
server name rather than blowing up."""
|
||||||
|
worker = RepairWorker(database=SimpleNamespace())
|
||||||
|
worker._config_manager = None
|
||||||
|
msg = worker._build_unresolvable_album_folder_error(
|
||||||
|
ResolveAttempt(base_dirs_tried=[]), "/music/x.flac",
|
||||||
|
)
|
||||||
|
assert "unknown" in msg.lower()
|
||||||
|
|
@ -3416,6 +3416,7 @@ const WHATS_NEW = {
|
||||||
'2.5.1': [
|
'2.5.1': [
|
||||||
// --- post-release patch work on the 2.5.1 line — entries hidden by _getLatestWhatsNewVersion until the build version bumps ---
|
// --- post-release patch work on the 2.5.1 line — entries hidden by _getLatestWhatsNewVersion until the build version bumps ---
|
||||||
{ date: 'Unreleased — 2.5.1 patch work' },
|
{ date: 'Unreleased — 2.5.1 patch work' },
|
||||||
|
{ title: 'Album Completeness: "Could Not Determine Album Folder" Error Now Tells You What To Fix', desc: 'github issue #558 (gabistek, navidrome on docker / arch host): clicking auto-fill or fix selected on the album completeness findings page returned a flat "could not determine album folder from existing tracks" error with no diagnostic. trace: the path resolver in `core/library/path_resolver.py` probes transfer + download + `library.music_paths` config + plex api library locations to map db-recorded paths to actual files on disk. for plex users the api auto-discovers the mount paths (per #476). navidrome\'s subsonic api doesn\'t expose filesystem paths at all (only folder names via `getMusicFolders`), and navidrome\'s native rest api on top of that doesn\'t expose them either — there is no api signal we can probe. so for navidrome users in docker, if the path navidrome reports (`/music/artist/album/track.flac`) doesn\'t exist as-is in the soulsync container view AND the user hasn\'t manually configured settings → library → music paths, the resolver returns none and the fix workflow bailed silently. fix: lifted the resolver into a diagnostic-aware variant (`resolve_library_file_path_with_diagnostic` returning a `(resolved, ResolveAttempt)` tuple) that records what was tried — raw-path-existed, base-dirs-probed, whether config_manager / plex_client were wired up. repair_worker uses the diagnostic to render a multi-part error: names the active media server, shows one sample db-recorded path the album\'s tracks have, lists every base directory the resolver actually probed, and points at settings → library → music paths as the actionable fix. user can now read the error and know exactly what to mount or configure. no auto-probing of common docker conventions — too speculative, could resolve to wrong dirs on the suffix-walk if conventional paths happen to contain a partial collision. backwards compatible: legacy `resolve_library_file_path` kept as a thin wrapper that drops the attempt, every existing call site unchanged. 12 new tests pin: tuple shape, raw-path short-circuit attempt fields, base-dirs listed even on walk failure, had-flags reflect caller inputs, error renders active server name + sample path + base dirs, distinguishes empty-base-dirs vs tried-and-failed cases, settings hint always present, defensive against none attempt + missing sample + missing config_manager.', page: 'tools' },
|
||||||
{ title: 'Import History: Clear History Button Now Clears Stuck "Processing" Rows', desc: 'noticed on the import page: clear history left zombie rows behind that all showed "⧗ processing" status from 2-9 days ago. trace: `_record_in_progress` inserts a `status=\'processing\'` row up-front so the ui can render the in-flight import while it runs, then `_finalize_result` updates it to `completed`/`failed` when the import finishes. when the server is restarted mid-import (or the worker crashes), the row never gets finalized — stays at `processing` forever. the clear-history endpoint\'s sql `DELETE ... WHERE status IN (\'completed\', \'approved\', \'failed\', \'needs_identification\', \'rejected\')` didn\'t include `processing`, so those zombies survived every click. fix: add `processing` to the delete list, but guard against nuking actually-live imports by intersecting against `_snapshot_active()` — any folder hash currently registered in the worker\'s in-memory `_active_imports` map is excluded from the delete. `pending_review` deliberately left out so user still has to approve/reject those explicitly. one endpoint touched (`/api/auto-import/clear-completed` in web_server.py). no worker changes. zombie-row pile gets swept on next click, new imports still record + update normally.', page: 'import' },
|
{ title: 'Import History: Clear History Button Now Clears Stuck "Processing" Rows', desc: 'noticed on the import page: clear history left zombie rows behind that all showed "⧗ processing" status from 2-9 days ago. trace: `_record_in_progress` inserts a `status=\'processing\'` row up-front so the ui can render the in-flight import while it runs, then `_finalize_result` updates it to `completed`/`failed` when the import finishes. when the server is restarted mid-import (or the worker crashes), the row never gets finalized — stays at `processing` forever. the clear-history endpoint\'s sql `DELETE ... WHERE status IN (\'completed\', \'approved\', \'failed\', \'needs_identification\', \'rejected\')` didn\'t include `processing`, so those zombies survived every click. fix: add `processing` to the delete list, but guard against nuking actually-live imports by intersecting against `_snapshot_active()` — any folder hash currently registered in the worker\'s in-memory `_active_imports` map is excluded from the delete. `pending_review` deliberately left out so user still has to approve/reject those explicitly. one endpoint touched (`/api/auto-import/clear-completed` in web_server.py). no worker changes. zombie-row pile gets swept on next click, new imports still record + update normally.', page: 'import' },
|
||||||
{ title: 'Auto-Import: Falls Through To Other Metadata Sources When Primary Has No Match', desc: 'discord report (mushy): 16 bandcamp indie albums sat in staging because auto-import couldn\'t identify them. manual search at the bottom of the import music tab found the same albums fine — they just weren\'t on the user\'s primary metadata source (spotify) but existed on tidal/deezer. trace: `_search_metadata_source` in `core/auto_import_worker.py` only queried `get_primary_source()` — single source, no fallback. meanwhile `search_import_albums` (the manual search bar at the bottom of the tab) already iterated the full `get_source_priority(get_primary_source())` chain and broke on first source with results. asymmetric behavior — manual search worked, auto-import didn\'t, same album. fix: lift auto-import to use the same source-chain pattern. try primary first; if it returns nothing OR scores below the 0.4 threshold, fall through to next source in priority order. first source that produces a strong-enough match wins. result dict carries the `source` that actually matched (not the primary name), so downstream `_match_tracks` calls the right client to fetch the album\'s tracklist. defensive per-source try/except so a rate-limited or auth-failed source doesn\'t abort the chain. unconfigured sources (client=None) silently skipped. scoring math lifted to pure helper `_score_album_search_result` so weight tweaks (album 50% / artist 20% / track-count 30%) are pinned at the function boundary independent of the orchestrator. weight constants exposed at module level (`_ALBUM_NAME_WEIGHT`, `_ARTIST_NAME_WEIGHT`, `_TRACK_COUNT_WEIGHT`) — greppable, bumpable in one place. 9 integration tests + 18 scoring-helper tests. integration tests pin: primary-success path unchanged (no fallback fires, only primary client called), primary-empty falls through to next source, primary-weak-score falls through, first fallback success stops the chain (no wasted api calls on remaining sources), all-sources-fail returns None, per-source exception contained, unconfigured-source skipped gracefully, result `source` field reflects winning source, `identification_confidence` from winning source. backwards compatible — single-source users see no change (chain just has one entry).', page: 'import' },
|
{ title: 'Auto-Import: Falls Through To Other Metadata Sources When Primary Has No Match', desc: 'discord report (mushy): 16 bandcamp indie albums sat in staging because auto-import couldn\'t identify them. manual search at the bottom of the import music tab found the same albums fine — they just weren\'t on the user\'s primary metadata source (spotify) but existed on tidal/deezer. trace: `_search_metadata_source` in `core/auto_import_worker.py` only queried `get_primary_source()` — single source, no fallback. meanwhile `search_import_albums` (the manual search bar at the bottom of the tab) already iterated the full `get_source_priority(get_primary_source())` chain and broke on first source with results. asymmetric behavior — manual search worked, auto-import didn\'t, same album. fix: lift auto-import to use the same source-chain pattern. try primary first; if it returns nothing OR scores below the 0.4 threshold, fall through to next source in priority order. first source that produces a strong-enough match wins. result dict carries the `source` that actually matched (not the primary name), so downstream `_match_tracks` calls the right client to fetch the album\'s tracklist. defensive per-source try/except so a rate-limited or auth-failed source doesn\'t abort the chain. unconfigured sources (client=None) silently skipped. scoring math lifted to pure helper `_score_album_search_result` so weight tweaks (album 50% / artist 20% / track-count 30%) are pinned at the function boundary independent of the orchestrator. weight constants exposed at module level (`_ALBUM_NAME_WEIGHT`, `_ARTIST_NAME_WEIGHT`, `_TRACK_COUNT_WEIGHT`) — greppable, bumpable in one place. 9 integration tests + 18 scoring-helper tests. integration tests pin: primary-success path unchanged (no fallback fires, only primary client called), primary-empty falls through to next source, primary-weak-score falls through, first fallback success stops the chain (no wasted api calls on remaining sources), all-sources-fail returns None, per-source exception contained, unconfigured-source skipped gracefully, result `source` field reflects winning source, `identification_confidence` from winning source. backwards compatible — single-source users see no change (chain just has one entry).', page: 'import' },
|
||||||
{ title: 'Multi-Artist Tag Settings Now Actually Work (artist_separator + feat_in_title + write_multi_artist)', desc: 'three settings on settings → metadata → tags were partially or completely unimplemented. (1) `write_multi_artist` only worked because of a never-populated `_artists_list` field — `core/metadata/source.py` built `metadata["artist"]` as a hardcoded ", "-joined string but never assigned `metadata["_artists_list"]`, so `core/metadata/enrichment.py:114` always saw an empty list and silently no-op\'d the multi-value tag write. (2) `artist_separator` (default ", ") was referenced in the UI + settings.js save path but ZERO python code read the value — every multi-artist track ended up with hardcoded ", " regardless of what the user picked. (3) `feat_in_title` (when true: pull featured artists into the title as " (feat. X, Y)" and leave only primary in the ARTIST tag — picard convention) had no implementation at all. fix in source.py: populate `_artists_list` from the search response\'s artists array, then build the ARTIST string per the user\'s settings — primary-only when feat_in_title is on (with featured names appended to title; double-append guarded for source titles that already include "feat."), else joined with the configured separator. fix in enrichment.py id3 path: writing TPE1 twice (single-string then list) was overwriting the configured separator. now keeps TPE1 as the display string and writes a separate `TXXX:Artists` frame for the multi-value list (picard convention). vorbis path was already correct (separate "artist" + "artists" keys). deezer-specific upgrade path: deezer\'s `/search` endpoint only returns the primary artist — full contributors live on `/track/<id>`. when source==deezer AND the search response had a single artist AND a track_id is available, enrichment now fetches the per-track endpoint and upgrades the artists list before tag-write. one extra API call per affected deezer track (skipped when search already returned multiple). spotify, tidal, itunes search responses already include all artists so they\'re unaffected. 29 new tests pin: `_artists_list` populated for multi/single/no-artist cases, separator drives ARTIST string (default + custom), single-artist case unaffected by either setting, feat_in_title pulls featured to title + leaves primary in ARTIST, feat_in_title no-op for single artist, double-append guard recognizes 9 source-title variants ("(feat. X)", "(Feat. X)", "(FEAT X)", "(feat X)", "(Featuring X)", "[feat. X]", "ft. X", "(ft X)", "FT. X"), word-boundary regex doesn\'t false-match substrings ("Aftermath" still gets the append), combined-settings precedence (feat_in_title wins over separator for ARTIST string but `_artists_list` carries everyone for the multi-value tag), deezer upgrade fires only when search returned single artist + track_id available, no upgrade for non-deezer sources, upgrade failure falls through to search-result list, no false-positive when /track/<id> confirms single artist.', page: 'settings' },
|
{ title: 'Multi-Artist Tag Settings Now Actually Work (artist_separator + feat_in_title + write_multi_artist)', desc: 'three settings on settings → metadata → tags were partially or completely unimplemented. (1) `write_multi_artist` only worked because of a never-populated `_artists_list` field — `core/metadata/source.py` built `metadata["artist"]` as a hardcoded ", "-joined string but never assigned `metadata["_artists_list"]`, so `core/metadata/enrichment.py:114` always saw an empty list and silently no-op\'d the multi-value tag write. (2) `artist_separator` (default ", ") was referenced in the UI + settings.js save path but ZERO python code read the value — every multi-artist track ended up with hardcoded ", " regardless of what the user picked. (3) `feat_in_title` (when true: pull featured artists into the title as " (feat. X, Y)" and leave only primary in the ARTIST tag — picard convention) had no implementation at all. fix in source.py: populate `_artists_list` from the search response\'s artists array, then build the ARTIST string per the user\'s settings — primary-only when feat_in_title is on (with featured names appended to title; double-append guarded for source titles that already include "feat."), else joined with the configured separator. fix in enrichment.py id3 path: writing TPE1 twice (single-string then list) was overwriting the configured separator. now keeps TPE1 as the display string and writes a separate `TXXX:Artists` frame for the multi-value list (picard convention). vorbis path was already correct (separate "artist" + "artists" keys). deezer-specific upgrade path: deezer\'s `/search` endpoint only returns the primary artist — full contributors live on `/track/<id>`. when source==deezer AND the search response had a single artist AND a track_id is available, enrichment now fetches the per-track endpoint and upgrades the artists list before tag-write. one extra API call per affected deezer track (skipped when search already returned multiple). spotify, tidal, itunes search responses already include all artists so they\'re unaffected. 29 new tests pin: `_artists_list` populated for multi/single/no-artist cases, separator drives ARTIST string (default + custom), single-artist case unaffected by either setting, feat_in_title pulls featured to title + leaves primary in ARTIST, feat_in_title no-op for single artist, double-append guard recognizes 9 source-title variants ("(feat. X)", "(Feat. X)", "(FEAT X)", "(feat X)", "(Featuring X)", "[feat. X]", "ft. X", "(ft X)", "FT. X"), word-boundary regex doesn\'t false-match substrings ("Aftermath" still gets the append), combined-settings precedence (feat_in_title wins over separator for ARTIST string but `_artists_list` carries everyone for the multi-value tag), deezer upgrade fires only when search returned single artist + track_id available, no upgrade for non-deezer sources, upgrade failure falls through to search-result list, no false-positive when /track/<id> confirms single artist.', page: 'settings' },
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue