Merge pull request #717 from Nezreka/fix/wishlist-album-bundle-postprocess
Fix/wishlist album bundle postprocess
This commit is contained in:
BoulderBadgeDad
GitHub
commit
731d31d2f7
5 changed files with 196 additions and 10 deletions
|
|
@ -167,15 +167,32 @@ def try_staging_match(task_id, batch_id, track, deps: StagingDeps):
|
|||
|
||||
Returns True if a match was found and the file was moved to the transfer folder.
|
||||
Returns False to fall through to normal download.
|
||||
|
||||
Every silent-False exit point logs at INFO with the rejection reason
|
||||
so #706 / #708-class "track staged but never imported, ends up
|
||||
re-added to wishlist" loops can be diagnosed from app.log without
|
||||
a re-instrumentation round-trip. Per-candidate skips log at DEBUG
|
||||
so the noise stays out of INFO unless explicitly turned up.
|
||||
"""
|
||||
track_title = (track.name or '').strip() if hasattr(track, 'name') else ''
|
||||
track_artist = (track.artists[0] if (hasattr(track, 'artists') and track.artists) else '').strip()
|
||||
# Compact identifier for the log lines below so a multi-batch
|
||||
# wishlist run can be greppable per-track.
|
||||
_track_label = f"'{track_title}' by '{track_artist}'"
|
||||
|
||||
staging_files = deps.get_staging_file_cache(batch_id or task_id)
|
||||
if not staging_files:
|
||||
logger.info(
|
||||
"[Staging] No match attempted for %s — staging cache empty for batch %s",
|
||||
_track_label, batch_id or task_id,
|
||||
)
|
||||
return False
|
||||
|
||||
track_title = track.name or ''
|
||||
track_artist = track.artists[0] if track.artists else ''
|
||||
|
||||
if not track_title:
|
||||
logger.info(
|
||||
"[Staging] No match attempted for task %s — track has empty title",
|
||||
task_id,
|
||||
)
|
||||
return False
|
||||
|
||||
from difflib import SequenceMatcher
|
||||
|
|
@ -186,12 +203,20 @@ def try_staging_match(task_id, batch_id, track, deps: StagingDeps):
|
|||
|
||||
best_match = None
|
||||
best_score = 0.0
|
||||
# Track per-candidate scoring so the rejection log can show the
|
||||
# near-miss that DID exist (useful when title-sim is 0.79 — one
|
||||
# point below the threshold).
|
||||
candidate_scores: list = []
|
||||
|
||||
for sf in staging_files:
|
||||
sf_title_variants = _staging_title_variants(sf['title'], normalize)
|
||||
sf_norm_artist = normalize(sf['artist'])
|
||||
|
||||
if not sf_title_variants:
|
||||
logger.debug(
|
||||
"[Staging] Skip candidate %s — no usable title variants",
|
||||
os.path.basename(sf.get('full_path', '?')),
|
||||
)
|
||||
continue
|
||||
|
||||
# Title similarity (primary)
|
||||
|
|
@ -201,6 +226,12 @@ def try_staging_match(task_id, batch_id, track, deps: StagingDeps):
|
|||
for candidate in sf_title_variants
|
||||
)
|
||||
if title_sim < 0.80:
|
||||
logger.debug(
|
||||
"[Staging] Skip candidate %s — title_sim=%.2f below 0.80 threshold (%s vs %s)",
|
||||
os.path.basename(sf.get('full_path', '?')),
|
||||
title_sim, norm_title, '|'.join(sf_title_variants[:3]),
|
||||
)
|
||||
candidate_scores.append((sf, title_sim, None, 0.0))
|
||||
continue
|
||||
|
||||
# Artist similarity (secondary)
|
||||
|
|
@ -221,12 +252,37 @@ def try_staging_match(task_id, batch_id, track, deps: StagingDeps):
|
|||
else:
|
||||
combined = (title_sim * 0.80) + (artist_sim * 0.20)
|
||||
|
||||
candidate_scores.append((sf, title_sim, artist_sim, combined))
|
||||
|
||||
if combined > best_score:
|
||||
best_score = combined
|
||||
best_match = sf
|
||||
|
||||
# Require high confidence to avoid false positives
|
||||
if not best_match or best_score < 0.75:
|
||||
# Log the rejection with the best near-miss so we can see why
|
||||
# the staged files didn't claim this wishlist track. Pre-fix
|
||||
# this returned False silently and the loop "download album,
|
||||
# stage files, never claim them, re-add to wishlist" was
|
||||
# impossible to debug from logs alone.
|
||||
if candidate_scores:
|
||||
near_miss = max(candidate_scores, key=lambda c: c[3])
|
||||
sf, title_sim, artist_sim, combined = near_miss
|
||||
logger.info(
|
||||
"[Staging] No match for %s in batch %s — best candidate %s "
|
||||
"(title_sim=%.2f, artist_sim=%s, combined=%.2f) below 0.75 threshold",
|
||||
_track_label, batch_id or task_id,
|
||||
os.path.basename(sf.get('full_path', '?')),
|
||||
title_sim,
|
||||
f"{artist_sim:.2f}" if artist_sim is not None else 'n/a',
|
||||
combined,
|
||||
)
|
||||
else:
|
||||
logger.info(
|
||||
"[Staging] No match for %s in batch %s — %d staging files "
|
||||
"but none had usable title variants",
|
||||
_track_label, batch_id or task_id, len(staging_files),
|
||||
)
|
||||
return False
|
||||
|
||||
logger.info(f"[Staging] Match found for '{track_title}' by '{track_artist}': "
|
||||
|
|
|
|||
|
|
@ -418,8 +418,12 @@ def detect_album_info_web(context, artist_context=None):
|
|||
context,
|
||||
album_info={
|
||||
"album_name": album_name,
|
||||
"track_number": track_info.get("track_number", 1),
|
||||
"disc_number": track_info.get("disc_number", 1),
|
||||
# Preserve missing numbers as None so the import pipeline
|
||||
# can fall through to ``extract_track_number_from_filename``
|
||||
# at ``core/imports/pipeline.py:652`` instead of locking
|
||||
# to track/disc 01 for every wishlist re-attempt.
|
||||
"track_number": track_info.get("track_number"),
|
||||
"disc_number": track_info.get("disc_number"),
|
||||
"album_image_url": album_ctx.get("image_url", ""),
|
||||
"confidence": 0.5,
|
||||
},
|
||||
|
|
|
|||
|
|
@ -642,7 +642,15 @@ def post_process_matched_download(context_key, context, file_path, runtime, meta
|
|||
album_info=album_info,
|
||||
default=original_search.get('title', 'Unknown Track'),
|
||||
)
|
||||
track_number = album_info.get('track_number', 1)
|
||||
# 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')
|
||||
logger.debug(
|
||||
"Final track_number processing: source=%s album_info_track_number=%s track_number=%s",
|
||||
album_info.get('source', 'unknown'),
|
||||
|
|
|
|||
|
|
@ -118,8 +118,15 @@ def ensure_wishlist_track_format(track_info):
|
|||
'artists': artists_list,
|
||||
'album': album,
|
||||
'duration_ms': track_info.get('duration_ms', 0),
|
||||
'track_number': track_info.get('track_number', 1),
|
||||
'disc_number': track_info.get('disc_number', 1),
|
||||
# track/disc numbers preserved as-is (no default-to-1 fallback).
|
||||
# Defaulting to 1 here poisons the downstream chain — the import
|
||||
# pipeline at ``core/imports/pipeline.py:645`` only runs its
|
||||
# ``extract_track_number_from_filename`` fallback when this
|
||||
# value is None, so a pre-filled 1 would lock every wishlist
|
||||
# re-attempt to track 01 regardless of source filename. Leave
|
||||
# missing values explicit so the pipeline can detect-and-recover.
|
||||
'track_number': track_info.get('track_number'),
|
||||
'disc_number': track_info.get('disc_number'),
|
||||
'preview_url': track_info.get('preview_url'),
|
||||
'external_urls': track_info.get('external_urls', {}),
|
||||
'popularity': track_info.get('popularity', 0),
|
||||
|
|
@ -156,18 +163,33 @@ def build_cancelled_task_wishlist_payload(task, profile_id: int = 1):
|
|||
|
||||
album_raw = track_info.get('album', {})
|
||||
if isinstance(album_raw, dict):
|
||||
# Full-dict shape — copy as-is so release_date / id / total_tracks
|
||||
# / artists survive the round-trip. Earlier this only ``setdefault``ed
|
||||
# name/album_type/images, but ``dict(album_raw)`` already
|
||||
# preserves every other key the source carries; the setdefaults
|
||||
# only fill blanks. release_date in particular MUST survive so
|
||||
# the import path-template renders the year in the folder name.
|
||||
album_data = dict(album_raw)
|
||||
album_data.setdefault('name', 'Unknown Album')
|
||||
album_data.setdefault('album_type', track_info.get('album_type', 'album'))
|
||||
if 'images' not in album_data and track_info.get('album_image_url'):
|
||||
album_data['images'] = [{'url': track_info.get('album_image_url')}]
|
||||
else:
|
||||
# String-shape album — preserve every adjacent track_info field
|
||||
# we know about so the constructed dict is still usable
|
||||
# downstream. Pre-fix this only set name + album_type, dropping
|
||||
# release_date / total_tracks / id even when the caller had them.
|
||||
album_data = {
|
||||
'name': str(album_raw) if album_raw else 'Unknown Album',
|
||||
'album_type': track_info.get('album_type', 'album'),
|
||||
}
|
||||
if track_info.get('album_image_url'):
|
||||
album_data['images'] = [{'url': track_info.get('album_image_url')}]
|
||||
if track_info.get('album_release_date') or track_info.get('release_date'):
|
||||
album_data['release_date'] = (
|
||||
track_info.get('album_release_date')
|
||||
or track_info.get('release_date')
|
||||
)
|
||||
|
||||
track_data = {
|
||||
'id': track_info.get('id'),
|
||||
|
|
@ -175,6 +197,13 @@ def build_cancelled_task_wishlist_payload(task, profile_id: int = 1):
|
|||
'artists': formatted_artists,
|
||||
'album': album_data,
|
||||
'duration_ms': track_info.get('duration_ms'),
|
||||
# Preserve track / disc position so a cancellation→re-add doesn't
|
||||
# lock the next attempt to track 01. ``None`` is intentional —
|
||||
# downstream ``core/imports/pipeline.py:645`` reads None as
|
||||
# "extract from filename instead", which is what we want when
|
||||
# the position genuinely isn't known.
|
||||
'track_number': track_info.get('track_number'),
|
||||
'disc_number': track_info.get('disc_number'),
|
||||
}
|
||||
|
||||
source_context = {
|
||||
|
|
@ -279,8 +308,11 @@ def track_object_to_dict(track_object) -> Dict[str, Any]:
|
|||
"preview_url": getattr(track_object, "preview_url", None),
|
||||
"external_urls": getattr(track_object, "external_urls", {}),
|
||||
"popularity": getattr(track_object, "popularity", 0),
|
||||
"track_number": getattr(track_object, "track_number", 1),
|
||||
"disc_number": getattr(track_object, "disc_number", 1),
|
||||
# See ``ensure_wishlist_track_format`` — preserve missing
|
||||
# values as None so the import pipeline's filename fallback
|
||||
# can fire instead of locking to track/disc 01.
|
||||
"track_number": getattr(track_object, "track_number", None),
|
||||
"disc_number": getattr(track_object, "disc_number", None),
|
||||
}
|
||||
|
||||
logger.debug(
|
||||
|
|
|
|||
|
|
@ -106,6 +106,92 @@ def test_extract_spotify_track_from_modal_info_reconstructs_from_slskd_result():
|
|||
assert out["album"]["name"] == "Album Three"
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# track_number / disc_number preservation through the wishlist payload
|
||||
# helpers — pins the bug A fix from PR 2/4. Pre-fix the helpers
|
||||
# defaulted missing numbers to 1, which locked every wishlist retry
|
||||
# to track 01 because the import pipeline's filename-extract fallback
|
||||
# only fires when the value is None (not the pre-filled 1).
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
def test_ensure_wishlist_track_format_preserves_real_track_number():
|
||||
"""Real track positions must survive the format helper. Pre-fix
|
||||
the helper read ``track_info.get('track_number', 1)`` which always
|
||||
returned 1 if the upstream payload had dropped the key — the
|
||||
desired number was lost on every round-trip."""
|
||||
track = {
|
||||
"name": "No Sleep Till Brooklyn",
|
||||
"artist": "Beastie Boys",
|
||||
"album": {"name": "Licensed to Ill", "release_date": "1986-11-15"},
|
||||
"track_number": 8,
|
||||
"disc_number": 1,
|
||||
}
|
||||
out = payloads.ensure_wishlist_track_format(track)
|
||||
assert out["track_number"] == 8
|
||||
assert out["disc_number"] == 1
|
||||
|
||||
|
||||
def test_ensure_wishlist_track_format_keeps_missing_track_number_as_none():
|
||||
"""When the upstream payload doesn't carry a track number, the
|
||||
helper must NOT pre-fill 1 — that poisons the chain and locks the
|
||||
file to track 01. Leave None so the import pipeline's filename
|
||||
fallback at ``core/imports/pipeline.py:652`` can fire."""
|
||||
track = {
|
||||
"name": "Mystery Track",
|
||||
"artist": "Artist",
|
||||
"album": {"name": "Album"},
|
||||
}
|
||||
out = payloads.ensure_wishlist_track_format(track)
|
||||
assert out["track_number"] is None
|
||||
assert out["disc_number"] is None
|
||||
|
||||
|
||||
def test_build_cancelled_task_wishlist_payload_preserves_track_number():
|
||||
"""Cancellation→re-add path was the worst offender — the payload
|
||||
builder dropped track_number from the saved data entirely (didn't
|
||||
even include the key). Next wishlist cycle saw missing key →
|
||||
helper defaulted to 1 → file imported as 01. Now both the
|
||||
cancellation payload AND the helper preserve real positions."""
|
||||
task = {
|
||||
"track_info": {
|
||||
"id": "trk-1", "name": "Brass Monkey",
|
||||
"artists": [{"name": "Beastie Boys"}],
|
||||
"album": {"name": "Licensed to Ill", "release_date": "1986-11-15"},
|
||||
"track_number": 11,
|
||||
"disc_number": 1,
|
||||
},
|
||||
"playlist_name": "Wishlist",
|
||||
"playlist_id": "p1",
|
||||
}
|
||||
out = payloads.build_cancelled_task_wishlist_payload(task)
|
||||
td = out["track_data"]
|
||||
assert td["track_number"] == 11
|
||||
assert td["disc_number"] == 1
|
||||
# Album release_date survives the round-trip so the path template
|
||||
# renders the year in the folder name.
|
||||
assert td["album"]["release_date"] == "1986-11-15"
|
||||
|
||||
|
||||
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
|
||||
release_date / album_image_url / etc. from the adjacent
|
||||
track_info fields rather than dropping them silently."""
|
||||
task = {
|
||||
"track_info": {
|
||||
"id": "trk-2", "name": "Song",
|
||||
"artists": [{"name": "Artist"}],
|
||||
"album": "Bare String Album",
|
||||
"release_date": "2020-06-01",
|
||||
},
|
||||
}
|
||||
out = payloads.build_cancelled_task_wishlist_payload(task)
|
||||
album = out["track_data"]["album"]
|
||||
assert album["name"] == "Bare String Album"
|
||||
assert album["release_date"] == "2020-06-01"
|
||||
|
||||
|
||||
def test_ensure_wishlist_track_format_defaults_non_dict_album_to_album_type():
|
||||
"""When ``album`` arrives as a non-dict (legacy/reconstruction path) we
|
||||
must not stamp ``album_type='single'`` — that lies about the origin
|
||||
|
|
|
|||
Loading…
Reference in a new issue