Album Completeness: surface diagnostic when resolver can't find album folder
GitHub issue #558: clicking Auto-Fill / Fix Selected on the Album Completeness findings page returned a flat "Could not determine album folder from existing tracks" error with no diagnostic. Reporter is on Navidrome on Docker — the path resolver in `core/library/path_resolver.py` couldn't find any of the album's tracks on disk because Navidrome's Subsonic API doesn't expose filesystem library paths the way Plex's API does (probed in #476). Default settings → `library.music_paths` empty → no base directories to probe → silent None. User had no signal about what to configure. Not a regression of #476 — that fix targeted Plex auto-discovery and worked correctly for it. Navidrome was never covered because the protocol gives the resolver nothing to probe. Fix scoped to the diagnostic surface, not auto-magic discovery: - Added `resolve_library_file_path_with_diagnostic` returning `(resolved, ResolveAttempt)`. ResolveAttempt records what the resolver tried — `raw_path_existed`, `base_dirs_tried`, `had_config_manager`, `had_plex_client`. Pure data, no rendering opinions. - Legacy `resolve_library_file_path` becomes a thin wrapper that drops the attempt; every existing call site is unchanged. - `RepairWorker._fix_incomplete_album` now uses the diagnostic helper and renders a multi-part error via `_build_unresolvable_album_folder_error`: names the active media server, shows one sample DB-recorded path, lists every base directory the resolver actually probed, and points the user at Settings → Library → Music Paths as the actionable fix. - Distinguishes empty-base-dirs vs tried-and-failed cases so the user knows whether to add a mount or fix the existing one. - No auto-probing of common Docker conventions (`/music`, `/media`, etc). Speculative — could resolve to wrong dirs on the suffix-walk if a conventional path happens to contain a partial collision. User stays in control. 12 new tests: - 7 in `tests/library/test_path_resolver.py`: tuple-shape contract, raw-path-existed short-circuit, base-dirs listed even on walk failure, had-flags reflect caller inputs, no-base-dirs returns None with empty attempt, legacy `resolve_library_file_path` delegates correctly across happy / suffix-walk / failure paths. - 8 in `tests/test_repair_worker_unresolvable_folder_error.py`: active server name in error, sample DB path verbatim, base dirs listed, empty-base-dirs phrased differently, Settings hint always present, defensive against None attempt / missing sample / missing config_manager. Full pytest sweep: 2774 passed.
This commit is contained in:
parent
d10546a9bc
commit
56ae10693b
5 changed files with 456 additions and 9 deletions
|
|
@ -32,7 +32,8 @@ from any background worker.
|
|||
from __future__ import annotations
|
||||
|
||||
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
|
||||
|
||||
|
|
@ -40,6 +41,33 @@ from utils.logging_config import get_logger
|
|||
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]:
|
||||
"""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.
|
||||
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:
|
||||
return None
|
||||
return None, attempt
|
||||
|
||||
if os.path.exists(file_path):
|
||||
return file_path
|
||||
attempt.raw_path_existed = True
|
||||
return file_path, attempt
|
||||
|
||||
path_parts = file_path.replace("\\", "/").split("/")
|
||||
base_dirs = _collect_base_dirs(transfer_folder, download_folder, config_manager, plex_client)
|
||||
attempt.base_dirs_tried = list(base_dirs)
|
||||
if not base_dirs:
|
||||
return None
|
||||
return None, attempt
|
||||
|
||||
# Skip index 0 to avoid drive-letter / leading-slash artifacts
|
||||
# (e.g. "E:" or "" from a leading "/").
|
||||
|
|
@ -166,8 +230,12 @@ def resolve_library_file_path(
|
|||
for i in range(1, len(path_parts)):
|
||||
candidate = os.path.join(base, *path_parts[i:])
|
||||
if os.path.exists(candidate):
|
||||
return candidate
|
||||
return None
|
||||
return candidate, attempt
|
||||
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
|
||||
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:
|
||||
pass
|
||||
|
||||
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):
|
||||
"""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', '')
|
||||
|
||||
album_folder = None
|
||||
last_attempt = None
|
||||
sample_db_path = None
|
||||
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):
|
||||
album_folder = os.path.dirname(resolved)
|
||||
break
|
||||
|
||||
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
|
||||
resolved_paths = []
|
||||
|
|
|
|||
|
|
@ -403,3 +403,139 @@ def test_docker_resolve_path_pass_through_outside_docker(monkeypatch) -> None:
|
|||
monkeypatch.setattr(os.path, "exists",
|
||||
lambda p: False if p == "/.dockerenv" else real_exists(p))
|
||||
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': [
|
||||
// --- 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' },
|
||||
{ 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: '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' },
|
||||
|
|
|
|||
Loading…
Reference in a new issue