Wishlist: fix three regressions causing all imports to land as track 01 with no year
Real-world regression triggered by the album-bundle work earlier in
2.6.3. Tracks with full Spotify metadata were importing as
``01 - <title>`` under ``Artist - Album/`` (no year), even when the
source filename carried the correct track number and Spotify's
release_date was available.
Investigation via DB inspection of stored wishlist rows:
```
"Never Gonna Give You Up" → track_number=None, release_date=""
"idfc" → track_number=1, release_date=""
"No Sleep Till Brooklyn" → track_number=1, release_date=""
```
Source-of-truth Spotify metadata had release_date AND real track
positions, but the wishlist row was poisoned. Three regressions
compounded the loss:
**Fix A — ``track_object_to_dict`` (``core/wishlist/payloads.py:295``)
preserved only album.name during Track→dict conversion.**
Pre-fix:
```python
album_name = "Unknown Album"
if hasattr(track_object, "album") and track_object.album:
if hasattr(track_object.album, "name"):
album_name = track_object.album.name
else:
album_name = str(track_object.album)
result = {
...
"album": {"name": album_name}, # ← release_date / images / etc. all dropped
...
}
```
When a wishlist payload arrived as a Track dataclass instead of a
raw spotify_data dict, the Track→dict conversion stripped
release_date, images, album_type, total_tracks, id, and album-level
artists. Every wishlist row added through this path landed in the
DB with ``album={'name': X}`` only.
Post-fix: three branches handle the three album shapes
- ``album_attr`` is a dict → ``dict(album_attr)`` preserves every key
- ``album_attr`` is a sub-object → pull all common Album-dataclass
attrs (id, release_date, album_type, total_tracks, images, ...)
- ``album_attr`` is a bare string → build a dict from the track
object's adjacent attrs (release_date, album_id, album_type, ...)
and surface ``image_url`` as ``album.images``
**Fix B — ``core/discovery/playlist.py:309`` only added
``track_number`` / ``disc_number`` keys when truthy.**
Pre-fix:
```python
matched_data = { 'id': ..., 'name': ..., ... } # no track_number / disc_number
if track_number:
matched_data['track_number'] = track_number
if disc_number:
matched_data['disc_number'] = disc_number
```
Deezer-sourced matches always hit this branch with ``track_number=None``
because the cache enrichment at line 304 reads ``_raw.get('track_number')``
literally, but Deezer's raw shape uses ``track_position``. So the key
was omitted from ``matched_data``, downstream consumers couldn't
distinguish "missing key" from "value is 1", and the chain silently
filled 1.
Post-fix: keys are ALWAYS present (None when unknown). Also adds a
``best_match.track_number`` fallback so the Track-dataclass-mapped
value (which DOES include ``track_position``→``track_number``
mapping) gets used when the cache lookup misses.
**Fix C — Pipeline only consulted ``album_info.track_number`` before
falling to the filename (``core/imports/pipeline.py:645``).**
VA-collection source files like ``417 Fountains of Wayne - Stacys
Mom.flac`` have a leading playlist-position number that isn't the
album track number. The previous chain (album_info → filename →
floor-1) couldn't recover the real position because the filename
extractor either returned 417 (wrong) or None (caught by the floor).
But the wishlist payload's ``track_info.spotify_data.track_number``
HAD the right answer all along — Spotify says Stacy's Mom is track
3 on Welcome Interstate Managers.
Post-fix: resolution chain extracted into ``core/imports/track_number.py:resolve_track_number``
as a pure function:
1. ``album_info.track_number`` (album-bundle dispatch authoritative)
2. ``track_info.track_number`` (per-track flow payload)
3. ``track_info.spotify_data.track_number`` (nested fallback)
4. ``extract_explicit_track_number(file_path)`` (filename, returns
0 when no numeric prefix — vs the default helper that returns 1)
5. Caller (pipeline) applies the final >=1 floor
Each step coerces to a positive int or falls through to the next.
Pure function = unit-testable in isolation = single place to fix
the rule.
**Test coverage (37 new tests):**
- ``tests/wishlist/test_payloads.py`` (+4) — Track→dict conversion
preserves full album dict (dict / object / string album shapes) +
None-track-number stays None.
- ``tests/discovery/test_discovery_playlist.py`` (+2) — matched_data
always includes track_number/disc_number keys (None when unknown)
+ falls back to best_match attrs when cache misses.
- ``tests/imports/test_track_number_resolver.py`` (+16) — every
resolution-chain branch pinned: album_info-wins, track_info
fallback, spotify_data nested, JSON-string parsing, garbage-string
fall-through, zero / negative / non-numeric / string-numeric
coercion, filename fallback, explicit extractor vs default
extractor semantics, defensive None inputs, VA-collection
filename behaviour, all-sources-missing → None.
1571 wider-suite tests pass (wishlist + imports + discovery +
downloads + metadata). Ruff clean.
**Migration note:** existing wishlist rows that were saved under
the OLD ``track_object_to_dict`` (with stripped album metadata) still
have ``release_date=''`` in the DB blob. Those won't self-heal — the
next attempt loads from the poisoned blob. Users can remove + re-add
those tracks to refresh, or wait for the next sync run that
re-discovers them with full metadata. No automatic migration shipped
in this PR (scope creep — the forward path is fixed, backfill is a
separate concern).
This commit is contained in:
Broque Thomas
parent
945d086ec4
commit
997732ee63
8 changed files with 586 additions and 28 deletions
|
|
@ -306,6 +306,18 @@ def run_playlist_discovery_worker(playlists, automation_id=None, deps: PlaylistD
|
|||
except Exception as e:
|
||||
logger.debug("metadata cache lookup for album enrichment failed: %s", e)
|
||||
|
||||
# Always include ``track_number`` / ``disc_number``
|
||||
# in the matched payload — None when unknown rather
|
||||
# than omitting the key. Downstream consumers
|
||||
# (``ensure_wishlist_track_format``, post-process
|
||||
# pipeline) check for None to know "look this up
|
||||
# somewhere else"; an absent key was indistinguishable
|
||||
# from "value is 1" after older payload helpers
|
||||
# silently filled the default. Pre-fix Deezer-sourced
|
||||
# matches always omitted the key (Deezer's track shape
|
||||
# uses ``track_position`` and the cache lookup at
|
||||
# line 304 reads ``track_number`` literally so it
|
||||
# returns None for Deezer rows).
|
||||
matched_data = {
|
||||
'id': best_match.id if hasattr(best_match, 'id') else '',
|
||||
'name': best_match.name if hasattr(best_match, 'name') else '',
|
||||
|
|
@ -314,11 +326,13 @@ def run_playlist_discovery_worker(playlists, automation_id=None, deps: PlaylistD
|
|||
'duration_ms': best_match.duration_ms if hasattr(best_match, 'duration_ms') else 0,
|
||||
'image_url': match_image,
|
||||
'source': discovery_source,
|
||||
'track_number': track_number if track_number else (
|
||||
getattr(best_match, 'track_number', None)
|
||||
),
|
||||
'disc_number': disc_number if disc_number else (
|
||||
getattr(best_match, 'disc_number', None)
|
||||
),
|
||||
}
|
||||
if track_number:
|
||||
matched_data['track_number'] = track_number
|
||||
if disc_number:
|
||||
matched_data['disc_number'] = disc_number
|
||||
|
||||
extra_data = {
|
||||
'discovered': True,
|
||||
|
|
|
|||
|
|
@ -642,28 +642,19 @@ def post_process_matched_download(context_key, context, file_path, runtime, meta
|
|||
album_info=album_info,
|
||||
default=original_search.get('title', 'Unknown Track'),
|
||||
)
|
||||
# Read with explicit None default — the upstream payload helpers
|
||||
# (``core/wishlist/payloads.py``) now preserve missing track
|
||||
# numbers as None instead of pre-filling 1. That lets the
|
||||
# filename-extract fallback below actually fire on wishlist
|
||||
# re-attempts whose source payload lost the position. Pre-fix,
|
||||
# ``.get('track_number', 1)`` filled 1, the None-check below
|
||||
# never matched, and every wishlist re-try imported as ``01 -``
|
||||
# regardless of the source file's real track number.
|
||||
track_number = album_info.get('track_number')
|
||||
# Resolve track_number from the richest available source.
|
||||
# See ``core/imports/track_number.py`` for the resolution
|
||||
# chain — pure function, unit-tested in isolation, single
|
||||
# place to fix the rule.
|
||||
from core.imports.track_number import resolve_track_number
|
||||
track_info_for_resolve = context.get('track_info') if isinstance(context, dict) else None
|
||||
track_number = resolve_track_number(album_info, track_info_for_resolve, file_path)
|
||||
logger.debug(
|
||||
"Final track_number processing: source=%s album_info_track_number=%s track_number=%s",
|
||||
"Final track_number processing: source=%s album_info=%s resolved=%s",
|
||||
album_info.get('source', 'unknown'),
|
||||
album_info.get('track_number', 'NOT_FOUND'),
|
||||
track_number,
|
||||
)
|
||||
if track_number is None:
|
||||
track_number = extract_track_number_from_filename(file_path)
|
||||
logger.info(
|
||||
"Track number was None; extracted from filename=%r -> %s",
|
||||
os.path.basename(file_path),
|
||||
track_number,
|
||||
)
|
||||
if not isinstance(track_number, int) or track_number < 1:
|
||||
logger.error(f"Invalid track number ({track_number}), defaulting to 1")
|
||||
track_number = 1
|
||||
|
|
|
|||
105
core/imports/track_number.py
Normal file
105
core/imports/track_number.py
Normal file
|
|
@ -0,0 +1,105 @@
|
|||
"""Pure-function resolver for the import pipeline's track_number lookup.
|
||||
|
||||
Lifted from ``core/imports/pipeline.py`` so the multi-source fallback
|
||||
chain can be unit-tested in isolation. The pipeline integration is
|
||||
one call site that delegates to ``resolve_track_number`` and then
|
||||
applies the >=1 floor as the last-resort default.
|
||||
|
||||
Resolution order (first valid positive int wins):
|
||||
|
||||
1. ``album_info.track_number`` — set by upstream album-info builders
|
||||
when they have authoritative track position data (e.g. the
|
||||
album-bundle dispatch from ``core/downloads/master.py``).
|
||||
2. ``track_info.track_number`` — Spotify-shaped track dict carried
|
||||
on the per-task download context. Populated by the per-track
|
||||
flow when the wishlist payload still has Spotify's position.
|
||||
3. ``track_info.spotify_data.track_number`` — nested spotify_data
|
||||
dict inside track_info; common for wishlist-loop payloads that
|
||||
wrapped the source spotify dict under an outer envelope.
|
||||
4. ``extract_track_number_from_filename(file_path)`` — last resort
|
||||
when none of the metadata sources carried the value.
|
||||
|
||||
Pre-fix, the pipeline only consulted ``album_info`` and fell straight
|
||||
to the filename when it was None. That broke for VA-collection
|
||||
source files like ``417 Fountains of Wayne - Stacys Mom.flac`` where
|
||||
the leading number isn't the album track position — extract returned
|
||||
None or the wrong number, post-process defaulted to 1, and every
|
||||
such wishlist import landed as ``01 - <title>`` regardless of the
|
||||
real source position.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import json
|
||||
from typing import Any, Optional
|
||||
|
||||
from core.imports.filename import extract_explicit_track_number
|
||||
|
||||
|
||||
def _coerce_positive(value: Any) -> Optional[int]:
|
||||
"""Coerce ``value`` to a positive int, or return None when the
|
||||
value is missing / non-numeric / non-positive. Centralised so
|
||||
every check in ``resolve_track_number`` applies the same rules."""
|
||||
try:
|
||||
v = int(value)
|
||||
return v if v >= 1 else None
|
||||
except (TypeError, ValueError):
|
||||
return None
|
||||
|
||||
|
||||
def _coerce_spotify_data(track_info: Any) -> dict:
|
||||
"""Extract the nested ``spotify_data`` dict from a track_info
|
||||
payload, coercing string-JSON shapes and bad inputs to an empty
|
||||
dict so the caller can use ``.get`` safely."""
|
||||
if not isinstance(track_info, dict):
|
||||
return {}
|
||||
raw = track_info.get('spotify_data')
|
||||
if isinstance(raw, dict):
|
||||
return raw
|
||||
if isinstance(raw, str):
|
||||
try:
|
||||
parsed = json.loads(raw)
|
||||
return parsed if isinstance(parsed, dict) else {}
|
||||
except (ValueError, TypeError):
|
||||
return {}
|
||||
return {}
|
||||
|
||||
|
||||
def resolve_track_number(
|
||||
album_info: Any,
|
||||
track_info: Any,
|
||||
file_path: str,
|
||||
) -> Optional[int]:
|
||||
"""Walk the resolution chain and return the first valid positive
|
||||
int found, or None when every source is missing / unusable.
|
||||
|
||||
Caller is responsible for the final default-1 floor — leaving
|
||||
that out of this function so tests can pin "everything missing
|
||||
returns None" separate from the floor behaviour.
|
||||
"""
|
||||
album_info = album_info if isinstance(album_info, dict) else {}
|
||||
track_info = track_info if isinstance(track_info, dict) else {}
|
||||
spotify_data = _coerce_spotify_data(track_info)
|
||||
|
||||
resolved = (
|
||||
_coerce_positive(album_info.get('track_number'))
|
||||
or _coerce_positive(track_info.get('track_number'))
|
||||
or _coerce_positive(spotify_data.get('track_number'))
|
||||
)
|
||||
if resolved is not None:
|
||||
return resolved
|
||||
|
||||
# Filename fallback — use the EXPLICIT extractor variant which
|
||||
# returns 0 when no numeric prefix is recognised (vs. the default
|
||||
# variant that silently returns 1 for the unknown case). We want
|
||||
# "unknown" to stay unknown here so the pipeline's final
|
||||
# default-1 floor is the single source of that fallback —
|
||||
# otherwise this resolver would silently fill 1 and the
|
||||
# downstream floor logic would have no effect.
|
||||
if not file_path:
|
||||
return None
|
||||
try:
|
||||
from_filename = extract_explicit_track_number(file_path)
|
||||
except Exception:
|
||||
from_filename = None
|
||||
return _coerce_positive(from_filename)
|
||||
|
|
@ -292,18 +292,67 @@ def track_object_to_dict(track_object) -> Dict[str, Any]:
|
|||
else:
|
||||
artists_list.append({"name": str(artist)})
|
||||
|
||||
album_name = "Unknown Album"
|
||||
if hasattr(track_object, "album") and track_object.album:
|
||||
if hasattr(track_object.album, "name"):
|
||||
album_name = track_object.album.name
|
||||
else:
|
||||
album_name = str(track_object.album)
|
||||
# Build the album dict by preserving every field the track
|
||||
# object carries about its album. Pre-fix only ``name`` was
|
||||
# surfaced — release_date / images / album_type / total_tracks
|
||||
# / id / artists were all silently dropped during the
|
||||
# Track→dict conversion that runs whenever the wishlist payload
|
||||
# arrives as a Track dataclass (vs. a raw spotify_data dict).
|
||||
# That regression poisoned every wishlist row added from a
|
||||
# Track object: stored album={'name': X} only, so downstream
|
||||
# path-template rendering had no year and post-process had no
|
||||
# track count for relative-position math.
|
||||
album_attr = getattr(track_object, "album", None) if hasattr(track_object, "album") else None
|
||||
if isinstance(album_attr, dict):
|
||||
# Album was already a dict on the track object — preserve
|
||||
# every key the source put there.
|
||||
album_dict = dict(album_attr)
|
||||
album_dict.setdefault("name", "Unknown Album")
|
||||
elif album_attr is not None and hasattr(album_attr, "name"):
|
||||
# Album was a sub-object — pull every common field across
|
||||
# the Spotify / Deezer / iTunes Album dataclass shapes.
|
||||
album_dict = {
|
||||
"name": getattr(album_attr, "name", "") or "Unknown Album",
|
||||
}
|
||||
for src_attr, dest_key in (
|
||||
("id", "id"),
|
||||
("release_date", "release_date"),
|
||||
("album_type", "album_type"),
|
||||
("total_tracks", "total_tracks"),
|
||||
("total_discs", "total_discs"),
|
||||
("artists", "artists"),
|
||||
("images", "images"),
|
||||
("image_url", "image_url"),
|
||||
):
|
||||
val = getattr(album_attr, src_attr, None)
|
||||
if val not in (None, "", [], 0):
|
||||
album_dict[dest_key] = val
|
||||
else:
|
||||
# Album was a bare string OR truthy-but-untyped — pull
|
||||
# adjacent track-object attrs to flesh it out.
|
||||
album_name = str(album_attr) if album_attr else "Unknown Album"
|
||||
album_dict = {"name": album_name}
|
||||
for src_attr, dest_key in (
|
||||
("release_date", "release_date"),
|
||||
("album_id", "id"),
|
||||
("album_type", "album_type"),
|
||||
("total_tracks", "total_tracks"),
|
||||
):
|
||||
val = getattr(track_object, src_attr, None)
|
||||
if val not in (None, "", [], 0):
|
||||
album_dict[dest_key] = val
|
||||
# ``image_url`` lives on the track object on the Deezer /
|
||||
# iTunes Track dataclasses — surface as album.images so the
|
||||
# path template's artwork hook can find it.
|
||||
img = getattr(track_object, "image_url", None)
|
||||
if img and "images" not in album_dict:
|
||||
album_dict["images"] = [{"url": img}]
|
||||
|
||||
result = {
|
||||
"id": getattr(track_object, "id", None),
|
||||
"name": getattr(track_object, "name", "Unknown Track"),
|
||||
"artists": artists_list,
|
||||
"album": {"name": album_name},
|
||||
"album": album_dict,
|
||||
"duration_ms": getattr(track_object, "duration_ms", 0),
|
||||
"preview_url": getattr(track_object, "preview_url", None),
|
||||
"external_urls": getattr(track_object, "external_urls", {}),
|
||||
|
|
|
|||
|
|
@ -305,6 +305,64 @@ def test_match_above_threshold_writes_extra_data():
|
|||
assert deps._db.cache_saves # saved to cache
|
||||
|
||||
|
||||
def test_matched_data_always_includes_track_and_disc_number_keys():
|
||||
"""Discovery's matched_data must ALWAYS include ``track_number``
|
||||
and ``disc_number`` keys — None when unknown, not omitted. Pre-fix
|
||||
the keys were only added when truthy, so Deezer-sourced matches
|
||||
(where the cache stores ``track_position`` not ``track_number``)
|
||||
saved payloads without the key entirely. Downstream consumers
|
||||
couldn't distinguish "value is 1" from "key is missing" and the
|
||||
chain silently filled 1 every time. Pin the consistent-shape
|
||||
contract here."""
|
||||
match = _FakeMatch()
|
||||
match.track_number = None # simulate Deezer-sourced sparse match
|
||||
match.disc_number = None
|
||||
tracks = [_track(track_id=1)]
|
||||
deps = _build_deps(
|
||||
tracks_by_playlist={'p1': tracks},
|
||||
spotify_results=[match],
|
||||
score_result=(match, 0.95, 0),
|
||||
)
|
||||
|
||||
dp.run_playlist_discovery_worker([_playlist('p1')], deps=deps)
|
||||
|
||||
assert len(deps._db.extra_data_writes) == 1
|
||||
_, extra = deps._db.extra_data_writes[0]
|
||||
matched = extra['matched_data']
|
||||
# Keys MUST be present even when value is None — downstream relies
|
||||
# on explicit None to know "look this up elsewhere".
|
||||
assert 'track_number' in matched
|
||||
assert 'disc_number' in matched
|
||||
assert matched['track_number'] is None
|
||||
assert matched['disc_number'] is None
|
||||
|
||||
|
||||
def test_matched_data_pulls_track_number_from_best_match_when_cache_misses():
|
||||
"""Cache enrichment may return None (Deezer key-mismatch case),
|
||||
but the Track dataclass best_match itself often carries the
|
||||
track_number from the source-shape mapping. matched_data must
|
||||
fall back to ``best_match.track_number`` instead of silently
|
||||
dropping the field."""
|
||||
match = _FakeMatch()
|
||||
match.track_number = 8 # populated by Track.from_deezer_track
|
||||
match.disc_number = 2
|
||||
tracks = [_track(track_id=1)]
|
||||
deps = _build_deps(
|
||||
tracks_by_playlist={'p1': tracks},
|
||||
spotify_results=[match],
|
||||
score_result=(match, 0.95, 0),
|
||||
)
|
||||
|
||||
dp.run_playlist_discovery_worker([_playlist('p1')], deps=deps)
|
||||
|
||||
matched = deps._db.extra_data_writes[0][1]['matched_data']
|
||||
# When the cache lookup returns None for track_number, fall back
|
||||
# to best_match.track_number (populated by the Track dataclass'
|
||||
# from_<source>_track classmethod).
|
||||
assert matched['track_number'] == 8
|
||||
assert matched['disc_number'] == 2
|
||||
|
||||
|
||||
def test_match_below_threshold_falls_back_to_wing_it():
|
||||
"""No high-confidence match → Wing It stub written."""
|
||||
match = _FakeMatch()
|
||||
|
|
|
|||
228
tests/imports/test_track_number_resolver.py
Normal file
228
tests/imports/test_track_number_resolver.py
Normal file
|
|
@ -0,0 +1,228 @@
|
|||
"""Tests for ``core/imports/track_number.py:resolve_track_number``.
|
||||
|
||||
Pure-function resolver lifted out of the import pipeline so the
|
||||
multi-source fallback chain can be pinned in isolation. Real-world
|
||||
bug it addresses: wishlist-loop tracks were importing as ``01 -
|
||||
<title>`` because the pipeline only consulted ``album_info.track_number``
|
||||
and fell straight to the filename. When the filename was VA-collection
|
||||
shaped (``417 Fountains of Wayne - Stacys Mom.flac``), the extractor
|
||||
either returned the wrong number or None, the pipeline floored to 1,
|
||||
and the wishlist track's actual Spotify track_number was discarded.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
from unittest.mock import patch
|
||||
|
||||
from core.imports.track_number import resolve_track_number
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Resolution chain — album_info wins when populated.
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_album_info_track_number_wins_over_track_info():
|
||||
"""When album_info has a real track_number, the resolver returns
|
||||
it without consulting track_info / spotify_data / filename. This
|
||||
is the album-bundle dispatch case where master.py has already
|
||||
resolved authoritative position data."""
|
||||
result = resolve_track_number(
|
||||
album_info={'track_number': 8},
|
||||
track_info={'track_number': 3}, # stale wishlist data
|
||||
file_path='/some/path/08 No Sleep Till Brooklyn.flac',
|
||||
)
|
||||
assert result == 8
|
||||
|
||||
|
||||
def test_track_info_used_when_album_info_missing():
|
||||
"""Per-track flow lands here — wishlist payload had track_number
|
||||
8 from Spotify, album_info wasn't populated by an album-bundle
|
||||
dispatch."""
|
||||
result = resolve_track_number(
|
||||
album_info={},
|
||||
track_info={'track_number': 8},
|
||||
file_path='/some/path/417 Stacy.flac',
|
||||
)
|
||||
assert result == 8
|
||||
|
||||
|
||||
def test_spotify_data_used_when_track_info_top_level_missing():
|
||||
"""Some wishlist payloads carry the full Spotify track dict nested
|
||||
under ``spotify_data`` rather than at the top level. The resolver
|
||||
must dig into the nested shape when the top-level key is absent."""
|
||||
result = resolve_track_number(
|
||||
album_info={},
|
||||
track_info={'spotify_data': {'track_number': 5}},
|
||||
file_path='/some/path/file.flac',
|
||||
)
|
||||
assert result == 5
|
||||
|
||||
|
||||
def test_spotify_data_string_json_parsed_then_read():
|
||||
"""Some legacy payloads stored spotify_data as a JSON string
|
||||
instead of a dict (round-tripped through DB blob fields).
|
||||
Resolver must parse and read it — same data, different shape."""
|
||||
result = resolve_track_number(
|
||||
album_info={},
|
||||
track_info={'spotify_data': '{"track_number": 12}'},
|
||||
file_path='/some/path/file.flac',
|
||||
)
|
||||
assert result == 12
|
||||
|
||||
|
||||
def test_spotify_data_garbage_string_falls_through():
|
||||
"""Non-JSON string in spotify_data must NOT crash — fall through
|
||||
to the next source (filename) as if it weren't there."""
|
||||
result = resolve_track_number(
|
||||
album_info={},
|
||||
track_info={'spotify_data': 'not json at all'},
|
||||
file_path='/dir/03 - Song.flac',
|
||||
)
|
||||
# Filename has '03 - ' prefix → extract returns 3.
|
||||
assert result == 3
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Filename fallback.
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_filename_fallback_when_all_metadata_sources_missing():
|
||||
"""No album_info, no track_info → resolver tries the filename."""
|
||||
result = resolve_track_number(
|
||||
album_info={},
|
||||
track_info={},
|
||||
file_path='/dl/12 - Track Title.flac',
|
||||
)
|
||||
assert result == 12
|
||||
|
||||
|
||||
def test_filename_fallback_handles_zero_padded_prefixes():
|
||||
"""Standard ripped-album naming ``NN - Title.flac`` produces the
|
||||
correct track position from the filename extractor."""
|
||||
result = resolve_track_number(
|
||||
album_info={},
|
||||
track_info={},
|
||||
file_path='/dl/05 - Whatever.flac',
|
||||
)
|
||||
assert result == 5
|
||||
|
||||
|
||||
def test_filename_extractor_exception_silenced_to_none():
|
||||
"""If the filename extractor raises (defensive — shouldn't in
|
||||
practice), resolver returns None rather than blowing up the
|
||||
whole post-process chain."""
|
||||
with patch('core.imports.track_number.extract_explicit_track_number',
|
||||
side_effect=RuntimeError('boom')):
|
||||
result = resolve_track_number({}, {}, '/path/05 - Track.flac')
|
||||
assert result is None
|
||||
|
||||
|
||||
def test_no_file_path_returns_none_for_filename_step():
|
||||
"""Empty file_path skips the filename extractor — resolver
|
||||
returns None instead of crashing on the next-step coercion."""
|
||||
result = resolve_track_number({}, {}, '')
|
||||
assert result is None
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Defensive: invalid / zero / non-numeric inputs.
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_album_info_zero_track_number_falls_through():
|
||||
"""``track_number=0`` is invalid (album positions are 1-indexed),
|
||||
so the resolver treats it as missing and tries the next source."""
|
||||
result = resolve_track_number(
|
||||
album_info={'track_number': 0},
|
||||
track_info={'track_number': 7},
|
||||
file_path='/dir/file.flac',
|
||||
)
|
||||
assert result == 7
|
||||
|
||||
|
||||
def test_negative_track_number_treated_as_missing():
|
||||
"""Defensive — a hand-edited row carrying -3 falls through."""
|
||||
result = resolve_track_number(
|
||||
album_info={'track_number': -3},
|
||||
track_info={'track_number': 7},
|
||||
file_path='/dir/file.flac',
|
||||
)
|
||||
assert result == 7
|
||||
|
||||
|
||||
def test_non_numeric_track_number_treated_as_missing():
|
||||
"""Garbage string falls through to the next source."""
|
||||
result = resolve_track_number(
|
||||
album_info={'track_number': 'oops'},
|
||||
track_info={'track_number': 7},
|
||||
file_path='/dir/file.flac',
|
||||
)
|
||||
assert result == 7
|
||||
|
||||
|
||||
def test_string_numeric_track_number_coerced_to_int():
|
||||
"""Some payloads store track_number as ``'8'`` (string) instead
|
||||
of ``8`` (int) — particularly from older DB serialisation paths.
|
||||
Resolver must coerce, not reject."""
|
||||
result = resolve_track_number(
|
||||
album_info={'track_number': '8'},
|
||||
track_info={},
|
||||
file_path='/dir/file.flac',
|
||||
)
|
||||
assert result == 8
|
||||
|
||||
|
||||
def test_all_sources_missing_returns_none():
|
||||
"""When every source is missing AND the filename doesn't carry
|
||||
a positional prefix, resolver returns None. Caller (the pipeline)
|
||||
then applies the final default-1 floor."""
|
||||
result = resolve_track_number({}, {}, '/no-prefix-here.flac')
|
||||
assert result is None
|
||||
|
||||
|
||||
def test_va_collection_filename_returns_bogus_number_not_one():
|
||||
"""Real-world regression case: ``417 Fountains of Wayne - Stacys Mom.flac``
|
||||
is a VA-collection file where the leading ``417`` is a playlist
|
||||
position, not the album track number. The filename extractor
|
||||
returns whatever it returns (currently None because the regex
|
||||
requires NN- prefix with the dash); the resolver's job is to
|
||||
let that flow through faithfully so the caller's default-1
|
||||
floor catches it. Pin the bug-trigger filename shape so a
|
||||
future "smart" extractor that returns 417 here still produces
|
||||
a behaviour the pipeline floor can correct."""
|
||||
# Empty album_info + track_info + nothing else → resolver
|
||||
# delegates to the filename extractor. Whatever it returns for
|
||||
# this VA-shape file, the pipeline applies the >=1 floor.
|
||||
result = resolve_track_number(
|
||||
album_info={},
|
||||
track_info={},
|
||||
file_path='/dl/417 Fountains of Wayne - Stacys Mom.flac',
|
||||
)
|
||||
# We don't pin the exact value here because the underlying
|
||||
# extractor's contract for non-canonical filenames is fuzzy.
|
||||
# What we DO pin: the resolver doesn't crash, returns either
|
||||
# None or a positive int. Pipeline's floor handles the rest.
|
||||
assert result is None or (isinstance(result, int) and result >= 1)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Non-dict inputs (defensive).
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_none_album_info_treated_as_empty_dict():
|
||||
"""Defensive — caller might pass None when album_info wasn't built."""
|
||||
result = resolve_track_number(
|
||||
None,
|
||||
{'track_number': 3},
|
||||
'/dir/file.flac',
|
||||
)
|
||||
assert result == 3
|
||||
|
||||
|
||||
def test_non_dict_track_info_treated_as_empty():
|
||||
"""Defensive — non-dict track_info won't crash the resolver."""
|
||||
result = resolve_track_number({}, 'not a dict', '/dir/file.flac')
|
||||
assert result is None
|
||||
|
|
@ -173,6 +173,118 @@ def test_build_cancelled_task_wishlist_payload_preserves_track_number():
|
|||
assert td["album"]["release_date"] == "1986-11-15"
|
||||
|
||||
|
||||
def test_track_object_to_dict_preserves_full_album_dict():
|
||||
"""When the input track has an album as a DICT (e.g. raw Spotify
|
||||
spotify_track_data), every album field must survive the
|
||||
Track→dict conversion. Pre-fix the conversion built
|
||||
``album = {'name': X}`` only, silently dropping release_date /
|
||||
images / album_type / total_tracks. Result: every wishlist row
|
||||
added from a Track-object path had empty release_date in the DB
|
||||
→ import path-template rendered without year → user's main
|
||||
complaint."""
|
||||
track_obj = SimpleNamespace(
|
||||
id="track-1",
|
||||
name="Never Gonna Give You Up",
|
||||
artists=[{"name": "Rick Astley"}],
|
||||
album={
|
||||
"id": "alb-1",
|
||||
"name": "Whenever You Need Somebody",
|
||||
"release_date": "1987-11-12",
|
||||
"album_type": "album",
|
||||
"total_tracks": 10,
|
||||
"images": [{"url": "https://cdn.example/cover.jpg"}],
|
||||
"artists": [{"name": "Rick Astley"}],
|
||||
},
|
||||
duration_ms=213000,
|
||||
track_number=1,
|
||||
disc_number=1,
|
||||
)
|
||||
out = payloads.track_object_to_dict(track_obj)
|
||||
assert out["album"]["name"] == "Whenever You Need Somebody"
|
||||
assert out["album"]["release_date"] == "1987-11-12"
|
||||
assert out["album"]["album_type"] == "album"
|
||||
assert out["album"]["total_tracks"] == 10
|
||||
assert out["album"]["id"] == "alb-1"
|
||||
assert out["album"]["images"] == [{"url": "https://cdn.example/cover.jpg"}]
|
||||
assert out["album"]["artists"] == [{"name": "Rick Astley"}]
|
||||
|
||||
|
||||
def test_track_object_to_dict_extracts_release_date_from_album_object():
|
||||
"""When the album is a sub-OBJECT (e.g. Spotify/Deezer Album
|
||||
dataclass), the conversion must pull release_date / album_type /
|
||||
total_tracks from its attributes — not assume dict-only access.
|
||||
Pre-fix the only attr read was ``.name``, dropping everything
|
||||
else even when the object had it."""
|
||||
|
||||
class _AlbumLike:
|
||||
name = "Licensed to Ill"
|
||||
id = "alb-li"
|
||||
release_date = "1986-11-15"
|
||||
album_type = "album"
|
||||
total_tracks = 13
|
||||
artists = ["Beastie Boys"]
|
||||
images = [{"url": "https://cover.example/li.jpg"}]
|
||||
|
||||
track_obj = SimpleNamespace(
|
||||
id="track-2",
|
||||
name="No Sleep Till Brooklyn",
|
||||
artists=[{"name": "Beastie Boys"}],
|
||||
album=_AlbumLike(),
|
||||
duration_ms=242000,
|
||||
track_number=8,
|
||||
disc_number=1,
|
||||
)
|
||||
out = payloads.track_object_to_dict(track_obj)
|
||||
assert out["album"]["name"] == "Licensed to Ill"
|
||||
assert out["album"]["release_date"] == "1986-11-15"
|
||||
assert out["album"]["total_tracks"] == 13
|
||||
assert out["album"]["id"] == "alb-li"
|
||||
assert out["track_number"] == 8
|
||||
|
||||
|
||||
def test_track_object_to_dict_string_album_pulls_release_date_from_track_attrs():
|
||||
"""When the album is a bare STRING (the lean Track dataclass
|
||||
shape used by some metadata sources), the album dict has to be
|
||||
built from scratch. Pull release_date + album_type from adjacent
|
||||
track-object attrs and image_url from the track itself so we
|
||||
don't lose the path-template inputs entirely."""
|
||||
track_obj = SimpleNamespace(
|
||||
id="track-3",
|
||||
name="Stacy's Mom",
|
||||
artists=[{"name": "Fountains of Wayne"}],
|
||||
album="Welcome Interstate Managers",
|
||||
release_date="2003-06-10",
|
||||
album_type="album",
|
||||
total_tracks=15,
|
||||
image_url="https://cover.example/wim.jpg",
|
||||
track_number=3,
|
||||
disc_number=1,
|
||||
duration_ms=200000,
|
||||
)
|
||||
out = payloads.track_object_to_dict(track_obj)
|
||||
assert out["album"]["name"] == "Welcome Interstate Managers"
|
||||
assert out["album"]["release_date"] == "2003-06-10"
|
||||
assert out["album"]["album_type"] == "album"
|
||||
assert out["album"]["total_tracks"] == 15
|
||||
assert out["album"]["images"] == [{"url": "https://cover.example/wim.jpg"}]
|
||||
assert out["track_number"] == 3
|
||||
|
||||
|
||||
def test_track_object_to_dict_missing_track_number_stays_none():
|
||||
"""Track-object-style sources that genuinely don't know the track
|
||||
position must surface as None (not pre-filled 1), so the import
|
||||
pipeline's filename-extract fallback can fire."""
|
||||
track_obj = SimpleNamespace(
|
||||
id="track-4",
|
||||
name="Unknown Track",
|
||||
artists=[{"name": "Artist"}],
|
||||
album="Album",
|
||||
)
|
||||
out = payloads.track_object_to_dict(track_obj)
|
||||
assert out["track_number"] is None
|
||||
assert out["disc_number"] is None
|
||||
|
||||
|
||||
def test_build_cancelled_task_wishlist_payload_string_album_pulls_release_date_from_track_info():
|
||||
"""When the source ``album`` field is a bare string, the payload
|
||||
builder constructs an album dict from scratch — it must pull
|
||||
|
|
|
|||
|
|
@ -3415,6 +3415,7 @@ function closeHelperSearch() {
|
|||
const WHATS_NEW = {
|
||||
'2.6.3': [
|
||||
{ unreleased: true },
|
||||
{ title: 'Wishlist: fix imports landing as track 01 with no year in folder name', desc: 'follow-up to the earlier wishlist album-bundle work. tracks with rich Spotify metadata were still importing as `01 - <title>` with no year in the folder path because three regressions stacked: the Track→dict conversion in payload helpers dropped everything except `album.name` (silently throwing away release_date / images / album_type / total_tracks), Deezer-sourced discovery matches saved their payloads without `track_number` / `disc_number` keys at all (Deezer uses `track_position` while the cache lookup read `track_number` literally), and the import pipeline only consulted `album_info.track_number` before falling to the filename — which fails for VA-collection source files like `417 Fountains of Wayne - Stacys Mom.flac` where the leading number is a playlist position not the album track. all three patched, with the track_number resolution chain lifted into `core/imports/track_number.py` as a pure function with 18 unit tests pinning every branch.' },
|
||||
{ title: 'Wishlist: distinguish Queued from Analyzing batches in the UI', desc: 'wishlist runs with more than 3 sub-batches used to render every batch as "Analyzing..." simultaneously — even though the download worker pool only runs 3 at a time. so a 26-album wishlist scan looked like all 26 were working in parallel when really 23 were just sitting in the executor queue waiting their turn. now batches show a distinct "Queued ⏳" state while parked in the executor queue; they flip to "Analyzing..." when a worker actually picks them up. zero behavior change — just less misleading UI.' },
|
||||
{ title: 'Wishlist: only engage album-bundle when several tracks from the same album are missing', desc: 'auto-wishlist + manual-wishlist runs were promoting every single-track wishlist item to a per-album bundle search — so a wishlist of "26 single tracks from 26 different albums" downloaded full albums (5-42 files each, ~85% wasted bandwidth) and hammered slskd with concurrent searches. now the bundle path only engages when an album has 2 or more missing tracks in the wishlist; single-track items take the cheaper per-track path that already works fine. fewer slskd searches per cycle, less wasted bandwidth, no more downloading the same album three times in a row when staging-match misses a single track. configurable via `wishlist.album_bundle_min_tracks` if you want the old behaviour (set to 1) or stricter (set to 3+).' },
|
||||
{ title: 'Auto-Sync: schedule playlists by weekday + time, not just by hour interval', desc: 'new Weekly Board tab on the Auto-Sync manager. drag a playlist onto a day column (Mon-Sun) to schedule it for that weekday at the default time, then click the card to open an editor for multi-day picks, custom time, and timezone. existing hourly schedules stay on the Hourly Board tab — one playlist gets exactly one schedule so swapping between weekly and hourly auto-replaces the old one. timezone defaults to whatever your browser reports (e.g. America/Los_Angeles); the editor accepts any IANA tz string. backend already had the plumbing from earlier 2.6.3 commits — this lights up the UI.', page: 'automations' },
|
||||
|
|
|
|||
Loading…
Reference in a new issue