diff --git a/.github/workflows/docker-publish.yml b/.github/workflows/docker-publish.yml index dc089154..48dac197 100644 --- a/.github/workflows/docker-publish.yml +++ b/.github/workflows/docker-publish.yml @@ -9,9 +9,9 @@ on: workflow_dispatch: inputs: version_tag: - description: 'Version tag (e.g. 2.7.6)' + description: 'Version tag (e.g. 2.7.7)' required: true - default: '2.7.6' + default: '2.7.7' jobs: build-and-push: diff --git a/core/discovery/listening_recommendations.py b/core/discovery/listening_recommendations.py new file mode 100644 index 00000000..9af7b417 --- /dev/null +++ b/core/discovery/listening_recommendations.py @@ -0,0 +1,205 @@ +"""Listening-driven recommendation core (#913). + +PURE, side-effect-free ranking that turns "the artists you listen to most" plus +"who's similar to each" into: + + 1. a consensus-ranked list of artists you'd probably love but don't own, and + 2. an aggregated candidate-track list for a generated playlist. + +No DB / network / config here. The caller (the watchlist scanner) supplies the +seeds (top-played artists), the ``similar_artists`` rows per seed, and the +owned-artist set, then fetches top tracks for the winners. Keeping the decision +logic in one pure place makes it fully unit-testable without the live stack and +keeps the scan wiring thin — and additive, so it can't disturb existing flows. + +Scoring rationale (the "best in class" bit): a recommended artist's score is +``Σ over the seeds that recommend it of (seed_weight × similarity)``. That single +sum rewards all three signals at once — **consensus** (an artist endorsed by many +of your seeds accumulates more terms), your **play weight** (heavier seeds push +harder), and **similarity strength** — instead of a flat "appears in N lists". +``seed_count`` is exposed separately for display ("because you like A, B, C") and +as the adventurousness dial's lever (``min_seed_count``). +""" + +from __future__ import annotations + +from dataclasses import dataclass, field +from typing import Dict, List, Optional, Sequence, Set + + +def _norm(name: object) -> str: + return str(name or "").strip().lower() + + +def _positive_float(value: object, default: float = 1.0) -> float: + try: + f = float(value) # type: ignore[arg-type] + except (TypeError, ValueError): + return default + return f if f > 0 else default + + +def _get(row: object, attr: str): + """Read a field from a dataclass row or a dict row.""" + if isinstance(row, dict): + return row.get(attr) + return getattr(row, attr, None) + + +def group_similars_by_seed( + seeds: Sequence[dict], + similar_rows: Sequence, + id_to_name: Dict[str, str], + *, + source_id_attr: str = "source_artist_id", + similar_name_attr: str = "similar_artist_name", +) -> Dict[str, List[dict]]: + """Reshape flat ``similar_artists`` rows into ``{seed_name_lower: [{'name': similar}]}``. + + The stored rows key the similar artist by the SEED's source id (``source_artist_id``), + not its name, so :func:`rank_recommended_artists` can't consume them directly. This + resolves each row's source id to a name via ``id_to_name`` (``{source_artist_id: + artist_name}`` for the library, built by the caller) and keeps only rows that resolve + to one of the ``seeds``. Rows may be dataclass objects or dicts. Pure — no I/O. + """ + seed_names = {_norm(s.get("name")) for s in seeds} + seed_names.discard("") + id_to_norm = {str(k): _norm(v) for k, v in (id_to_name or {}).items()} + + out: Dict[str, List[dict]] = {} + for row in similar_rows or (): + seed_name = id_to_norm.get(str(_get(row, source_id_attr) or ""), "") + if not seed_name or seed_name not in seed_names: + continue + sim_name = str(_get(row, similar_name_attr) or "").strip() + if sim_name: + out.setdefault(seed_name, []).append({"name": sim_name}) + return out + + +@dataclass +class RecommendedArtist: + """One artist recommended from your listening, with the why.""" + name: str # display name (first-seen casing) + score: float # Σ seed_weight × similarity + seed_count: int # distinct seeds endorsing it (consensus) + seeds: List[str] = field(default_factory=list) # display names of those seeds + + +def rank_recommended_artists( + seeds: Sequence[dict], + similars_by_seed: Dict[str, Sequence[dict]], + owned_artist_names: Optional[Set[str]] = None, + *, + limit: int = 30, + min_seed_count: int = 1, +) -> List[RecommendedArtist]: + """Rank artists similar to your most-played by consensus + play weight + similarity. + + Args: + seeds: ``[{'name': str, 'weight': float}]`` — your top-played artists. + ``weight`` (play count or any positive number) defaults to 1.0. + similars_by_seed: ``{seed_name_lower: [{'name': str, 'score': float}]}`` — the + similar-artist rows for each seed. ``score`` is optional (defaults 1.0). + owned_artist_names: lowercased names already in the library — excluded so the + result is artists you DON'T have. The seeds themselves are always excluded. + limit: max results. + min_seed_count: drop recommendations endorsed by fewer than N seeds — the + adventurousness dial's "Safer" end raises this for higher-confidence picks. + + Returns up to ``limit`` :class:`RecommendedArtist`, highest score first. + """ + owned = {_norm(a) for a in (owned_artist_names or set())} + seed_norms = {_norm(s.get("name")) for s in seeds} + seed_norms.discard("") + exclude = owned | seed_norms + + acc: Dict[str, dict] = {} + for seed in seeds: + s_name = _norm(seed.get("name")) + if not s_name: + continue + s_display = str(seed.get("name") or "").strip() + weight = _positive_float(seed.get("weight", 1.0)) + for sim in similars_by_seed.get(s_name, ()) or (): + a_norm = _norm(sim.get("name")) + if not a_norm or a_norm in exclude: + continue + sim_score = _positive_float(sim.get("score", 1.0)) + row = acc.setdefault( + a_norm, {"name": str(sim.get("name") or "").strip(), "score": 0.0, "seeds": {}} + ) + row["score"] += weight * sim_score + row["seeds"].setdefault(s_name, s_display) # one seed counts once + + out: List[RecommendedArtist] = [] + floor = max(1, int(min_seed_count)) + for row in acc.values(): + seed_count = len(row["seeds"]) + if seed_count < floor: + continue + out.append(RecommendedArtist( + name=row["name"], + score=round(row["score"], 6), + seed_count=seed_count, + seeds=list(row["seeds"].values()), + )) + out.sort(key=lambda r: (-r.score, -r.seed_count, r.name.lower())) + return out[:limit] + + +def aggregate_candidate_tracks( + recommended_artists: Sequence[RecommendedArtist], + top_tracks_by_artist: Dict[str, Sequence[dict]], + owned_track_keys: Optional[Set] = None, + *, + per_artist: int = 3, + limit: int = 50, + exclude_owned: bool = True, +) -> List[dict]: + """Build the candidate track list for the generated playlist. + + Takes the top ``per_artist`` tracks from each recommended artist **in artist-rank + order**, dedups by ``(artist, title)``, optionally drops owned tracks (the + "discovery" flavor) and caps at ``limit``. Each returned track dict is the source + track plus ``_seed_artist`` (which recommended artist it came from). + + Args: + recommended_artists: ranked output of :func:`rank_recommended_artists`. + top_tracks_by_artist: ``{artist_name_lower: [track_dict, ...]}`` — fetched by + the caller (Last.fm / source top tracks), NOT limited to a curated pool. + owned_track_keys: set of ``(artist_lower, title_lower)`` already in the library. + exclude_owned: drop tracks in ``owned_track_keys`` (discovery flavor). Set False + for a "replay" playlist of tracks you already own. + """ + owned = owned_track_keys or set() + seen: Set = set() + out: List[dict] = [] + for art in recommended_artists: + tracks = top_tracks_by_artist.get(_norm(art.name), ()) or () + taken = 0 + for t in tracks: + if taken >= per_artist: + break + title = str(t.get("name") or t.get("title") or "").strip() + if not title: + continue + key = (_norm(art.name), _norm(title)) + if key in seen: + continue + if exclude_owned and key in owned: + continue + seen.add(key) + out.append({**t, "_seed_artist": art.name}) + taken += 1 + if len(out) >= limit: + break + return out[:limit] + + +__all__ = [ + "RecommendedArtist", + "group_similars_by_seed", + "rank_recommended_artists", + "aggregate_candidate_tracks", +] diff --git a/core/discovery/youtube.py b/core/discovery/youtube.py index 97cf7fd4..7c4d05fe 100644 --- a/core/discovery/youtube.py +++ b/core/discovery/youtube.py @@ -32,6 +32,26 @@ from typing import Any, Callable logger = logging.getLogger(__name__) +_UNKNOWN_ARTIST = 'Unknown Artist' + + +def resolve_display_artist(yt_artist: str, matched_artist: str) -> str: + """The artist to show in the 'YT Artist' column (#909). + + YouTube's flat playlist data carries no artist, so a track starts as + "Unknown Artist" and only gains a real name if per-video recovery succeeds. + When recovery comes up empty but the track still matched confidently, show + the matched artist instead of a misleading "Unknown Artist". Returns the + original ``yt_artist`` whenever it's already a real name (recovery worked) or + when there's no matched artist to fall back to — purely a display choice, the + match itself is unaffected. + """ + current = (yt_artist or '').strip() + if current and current != _UNKNOWN_ARTIST: + return current # recovery already gave a real name — keep it + fallback = (matched_artist or '').strip() + return fallback or _UNKNOWN_ARTIST # backfill from the match, else honest Unknown + @dataclass class YoutubeDiscoveryDeps: @@ -131,14 +151,15 @@ def run_youtube_discovery_worker(url_hash, deps: YoutubeDiscoveryDeps): cached_match = cache_db.get_discovery_cache_match(cache_key[0], cache_key[1], discovery_source) if cached_match and deps.validate_discovery_cache_artist(cleaned_artist, cached_match): logger.debug(f"CACHE HIT [{i+1}/{len(tracks)}]: {cleaned_artist} - {cleaned_title}") + _match_artist = deps.extract_artist_name(cached_match.get('artists', [''])[0]) if cached_match.get('artists') else '' result = { 'index': i, 'yt_track': cleaned_title, - 'yt_artist': cleaned_artist, + 'yt_artist': resolve_display_artist(cleaned_artist, _match_artist), 'status': 'Found', 'status_class': 'found', 'spotify_track': cached_match.get('name', ''), - 'spotify_artist': deps.extract_artist_name(cached_match.get('artists', [''])[0]) if cached_match.get('artists') else '', + 'spotify_artist': _match_artist, 'spotify_album': cached_match.get('album', {}).get('name', '') if isinstance(cached_match.get('album'), dict) else cached_match.get('album', ''), 'duration': f"{int(track['duration_ms']) // 60000}:{(int(track['duration_ms']) % 60000) // 1000:02d}" if track['duration_ms'] else '0:00', 'discovery_source': discovery_source, @@ -265,15 +286,17 @@ def run_youtube_discovery_worker(url_hash, deps: YoutubeDiscoveryDeps): best_confidence = confidence logger.info(f"Strategy 4 YouTube match (extended): {match.artists[0]} - {match.name} (confidence: {confidence:.3f})") - # Create result entry + # Create result entry. yt_artist falls back to the matched artist when + # YouTube/recovery left it "Unknown Artist" but we matched confidently (#909). + _match_artist = deps.extract_artist_name(matched_track.artists[0]) if matched_track else '' result = { 'index': i, 'yt_track': cleaned_title, - 'yt_artist': cleaned_artist, + 'yt_artist': resolve_display_artist(cleaned_artist, _match_artist), 'status': 'Found' if matched_track else 'Not Found', 'status_class': 'found' if matched_track else 'not-found', 'spotify_track': matched_track.name if matched_track else '', - 'spotify_artist': deps.extract_artist_name(matched_track.artists[0]) if matched_track else '', + 'spotify_artist': _match_artist, 'spotify_album': matched_track.album if matched_track else '', 'duration': f"{int(track['duration_ms']) // 60000}:{(int(track['duration_ms']) % 60000) // 1000:02d}" if track['duration_ms'] else '0:00', 'discovery_source': discovery_source, diff --git a/core/downloads/candidates.py b/core/downloads/candidates.py index d9f20777..1edca30d 100644 --- a/core/downloads/candidates.py +++ b/core/downloads/candidates.py @@ -205,6 +205,21 @@ def attempt_download_with_candidates(task_id, candidates, track, batch_id=None, 'artists': _fallback_album_artists } + # #915: parity with Reorganize / manual Enrich. If the album context is lean + # (no release_date) and the user's PRIMARY metadata source isn't Spotify, hydrate + # it from that source — the same place a reorganize reads — so the download's + # $year folder, release_date and album_type match instead of dropping the year / + # defaulting to YYYY-01-01 and forcing a manual reorganize afterwards. + try: + from core.downloads.track_metadata_backfill import backfill_album_context_from_source + from core.metadata import registry as _meta_registry + from core.metadata.album_tracks import get_album_for_source as _get_album_for_source + backfill_album_context_from_source( + spotify_album_context, _meta_registry.get_primary_source(), _get_album_for_source, + ) + except Exception as _bf_err: # noqa: BLE001 — never let backfill break a download + logger.debug("[Context] primary-source album backfill skipped: %s", _bf_err) + download_payload = candidate.__dict__ username = download_payload.get('username') diff --git a/core/downloads/track_metadata_backfill.py b/core/downloads/track_metadata_backfill.py index da7fac0e..82474d65 100644 --- a/core/downloads/track_metadata_backfill.py +++ b/core/downloads/track_metadata_backfill.py @@ -93,6 +93,56 @@ def _backfill_album_context( album_context['image_url'] = first['url'] +# Placeholder album ids used when no real source album id is known — never queryable. +_SENTINEL_ALBUM_IDS = {'explicit_album', 'from_sync_modal', ''} + + +def backfill_album_context_from_source( + album_context: Dict[str, Any], + primary_source: Optional[str], + get_album_for_source_fn: Any, +) -> bool: + """Hydrate a lean album context from the user's PRIMARY metadata source (#915). + + Post-processing's only album backfill (:func:`hydrate_download_metadata`) goes through + ``spotify_client.get_track_details`` — Spotify-only. An iTunes/Deezer-primary user's + download therefore kept a lean context (no ``release_date``), so the path dropped the + ``$year`` and the date defaulted to ``YYYY-01-01`` — until they ran a Reorganize, which + reads the full album from the PRIMARY source. This closes that gap by doing the same: + fetch the full album from the primary source and backfill, so a download's pathing/tags + match what a later reorganize would produce. + + ``get_album_for_source_fn(source, album_id)`` is injected (the real one is + ``core.metadata.album_tracks.get_album_for_source``) so this stays pure + testable. + No-op when: the context is already complete; the primary source is spotify (the existing + track-details path covers it); or no real source album id is present. Returns True when + it filled anything. Never raises — a backfill failure must not break a download. + """ + if not isinstance(album_context, dict) or not _album_is_lean(album_context): + return False + if not primary_source or primary_source == 'spotify': + return False + album_id = album_context.get('id') + if not album_id or str(album_id) in _SENTINEL_ALBUM_IDS: + return False + try: + album = get_album_for_source_fn(primary_source, str(album_id)) + except Exception as e: # noqa: BLE001 — defensive: never let backfill break a download + logger.warning("[Context] primary-source (%s) album backfill failed: %s", primary_source, e) + return False + if not isinstance(album, dict): + return False + before = album_context.get('release_date') + _backfill_album_context(album_context, {'album': album}) + if album_context.get('release_date') and album_context.get('release_date') != before: + logger.info( + "[Context] Hydrated lean album context from primary source %s " + "(release_date=%r, total_tracks=%r)", + primary_source, album_context.get('release_date'), album_context.get('total_tracks'), + ) + return True + + def hydrate_download_metadata( track: Any, track_info: Any, @@ -172,4 +222,5 @@ def hydrate_download_metadata( __all__ = [ 'ResolvedTrackMetadata', 'hydrate_download_metadata', + 'backfill_album_context_from_source', ] diff --git a/core/imports/file_ops.py b/core/imports/file_ops.py index c3620d78..9923c39f 100644 --- a/core/imports/file_ops.py +++ b/core/imports/file_ops.py @@ -2,6 +2,7 @@ from __future__ import annotations +import errno import logging import os import re @@ -102,6 +103,41 @@ def cleanup_slskd_dedup_siblings(source_path) -> List[str]: return deleted +def _atomic_cross_device_move(src: Path, dst: Path) -> None: + """Move ``src`` to ``dst`` across filesystems WITHOUT ever exposing a partial file at + the final path. + + Copies into a hidden temp sibling of ``dst`` (same filesystem), fsyncs, then does an + atomic ``os.replace`` into place, then deletes ``src``. A media-server file watcher + (Jellyfin/Plex real-time monitoring) therefore only ever indexes the COMPLETE file — + an incremental in-place copy was what Jellyfin could catch mid-write and cache with + null/incomplete metadata (tracks landing with no disc). Cleans up the temp on failure. + """ + src, dst = Path(src), Path(dst) + tmp = dst.parent / f".{dst.name}.ssync-tmp" + try: + with open(src, "rb") as f_src, open(tmp, "wb") as f_dst: + shutil.copyfileobj(f_src, f_dst) + f_dst.flush() + os.fsync(f_dst.fileno()) + try: + shutil.copystat(str(src), str(tmp)) # preserve mtime/permissions (copy2-like) + except OSError: + pass + os.replace(str(tmp), str(dst)) # atomic within dst's filesystem + except Exception: + try: + if tmp.exists(): + tmp.unlink() + except OSError: + pass + raise + try: + src.unlink() + except OSError: + logger.info(f"Could not delete source after cross-device move (may be owned by another process): {src}") + + def safe_move_file(src, dst): """Move a file safely across filesystems.""" src = Path(src) @@ -129,7 +165,13 @@ def safe_move_file(src, dst): break try: - shutil.move(str(src), str(dst)) + # Same-filesystem move: an atomic rename that also overwrites dst. A media-server + # watcher (Jellyfin/Plex real-time monitoring) therefore never sees a partial file + # at the final name. Cross-filesystem raises EXDEV (some network mounts raise + # EPERM/EACCES) and we copy atomically below instead of letting the move write the + # destination incrementally — the partial-file-at-final-name is what caused tracks + # to land in Jellyfin with null/incomplete metadata (no disc). + os.replace(str(src), str(dst)) return except FileNotFoundError: if dst.exists(): @@ -137,8 +179,6 @@ def safe_move_file(src, dst): return raise except (OSError, PermissionError) as e: - error_msg = str(e).lower() - if dst.exists() and dst.stat().st_size > 0: logger.warning(f"Move raised {type(e).__name__} but destination exists, treating as success: {e}") try: @@ -147,23 +187,21 @@ def safe_move_file(src, dst): logger.info(f"Could not delete source file (may be owned by another process): {src}") return - if "cross-device" in error_msg or "operation not permitted" in error_msg or "permission denied" in error_msg: - logger.warning(f"Cross-device move detected, using fallback copy method: {e}") + error_msg = str(e).lower() + cross_device = ( + getattr(e, "errno", None) in (errno.EXDEV, errno.EPERM, errno.EACCES) + or "cross-device" in error_msg + or "operation not permitted" in error_msg + or "permission denied" in error_msg + ) + if cross_device: + logger.warning(f"Cross-device move, using atomic copy+rename: {e}") try: - with open(src, "rb") as f_src: - with open(dst, "wb") as f_dst: - shutil.copyfileobj(f_src, f_dst) - f_dst.flush() - os.fsync(f_dst.fileno()) - - try: - src.unlink() - except PermissionError: - logger.info(f"Could not delete source file (may be owned by another process): {src}") - logger.info(f"Successfully moved file using fallback method: {src} -> {dst}") + _atomic_cross_device_move(src, dst) + logger.info(f"Successfully moved file atomically across filesystems: {src} -> {dst}") return except Exception as fallback_error: - logger.error(f"Fallback copy also failed: {fallback_error}") + logger.error(f"Atomic cross-device move failed: {fallback_error}") raise raise diff --git a/core/itunes_client.py b/core/itunes_client.py index 25f9c73a..0d34b6db 100644 --- a/core/itunes_client.py +++ b/core/itunes_client.py @@ -741,7 +741,9 @@ class iTunesClient: if cached and cached.get('items'): return cached - results = self._lookup(id=album_id, entity='song') + # #918: the iTunes Lookup API returns only 50 related entities unless `limit` is + # passed (max 200), so albums >50 tracks were truncated in the download window. + results = self._lookup(id=album_id, entity='song', limit=200) if not results: return None @@ -763,7 +765,7 @@ class iTunesClient: try: fb_results = self.session.get( self.LOOKUP_URL, - params={'id': album_id, 'entity': 'song', 'country': fallback}, + params={'id': album_id, 'entity': 'song', 'country': fallback, 'limit': 200}, # #918 timeout=15 ) if fb_results.status_code == 200: diff --git a/core/library/missing_track_import.py b/core/library/missing_track_import.py index 7b322be4..ad59b5ae 100644 --- a/core/library/missing_track_import.py +++ b/core/library/missing_track_import.py @@ -10,6 +10,7 @@ from __future__ import annotations from dataclasses import dataclass import logging import os +import re import shutil import uuid from typing import Any, Callable, Dict, Optional @@ -121,6 +122,20 @@ def import_existing_track_for_album_slot(album_id: str, payload: dict, deps: Mis if album_data.get("server_source") and source_track.get("server_source") and album_data["server_source"] != source_track["server_source"]: raise MissingTrackImportError("Selected track belongs to a different library source", 400) + # #917: "I have this" rebuilds the destination path from album metadata. When the album row + # has no year, the rebuilt path drops the $year and the copied file lands in a NEW, yearless + # directory instead of the album's existing folder. Recover the year from a sibling track so + # the import reuses the same directory. + if not album_data.get("year"): + recovered_year = _existing_album_year_from_sibling( + database, album_id, deps.resolve_library_file_path_fn, + int(expected.get("disc_number") or 1), int(expected.get("track_number") or 1), + ) + if recovered_year: + album_data["year"] = recovered_year + logger.info("[I Have This] recovered album year %s from existing folder for album %s", + recovered_year, album_id) + source_path = deps.resolve_library_file_path_fn(source_track.get("file_path")) if not source_path: raise MissingTrackImportError(_file_not_found_message(source_track.get("file_path")), 404) @@ -426,6 +441,50 @@ def _sync_imported_track(deps: MissingTrackImportDeps, track_id, expected_title: logger.debug("Existing-track import server sync skipped/failed: %s", sync_err) +def _existing_album_year_from_sibling( + database, + album_id: str, + resolve_library_file_path_fn: Callable[[Optional[str]], Optional[str]], + target_disc: int, + target_track: int, +) -> Optional[str]: + """Find the release year already baked into this album's on-disk folder (#917). + + Read from a sibling track — its own ``year`` column first, else a ``(YYYY)`` / + ``[YYYY]`` in the album folder name — so an "I have this" import reuses the album's + existing directory instead of rebuilding a yearless one. Returns the 4-digit year + string, or None when no signal exists. + """ + try: + with database._get_connection() as conn: + cursor = conn.cursor() + cursor.execute( + """ + SELECT file_path, year FROM tracks + WHERE album_id = ? + AND file_path IS NOT NULL AND file_path != '' + AND NOT (COALESCE(disc_number, 1) = ? AND track_number = ?) + ORDER BY COALESCE(disc_number, 1), track_number + LIMIT 12 + """, + (album_id, target_disc, target_track), + ) + rows = cursor.fetchall() + for row in rows: + year = row["year"] + if year is not None and str(year).strip()[:4].isdigit(): + return str(year).strip()[:4] + resolved = resolve_library_file_path_fn(row["file_path"]) + if resolved: + folder = os.path.basename(os.path.dirname(resolved)) + match = re.search(r"[(\[](\d{4})[)\]]", folder) # "Album (2019)" / "Album [2019]" + if match: + return match.group(1) + except Exception as exc: + logger.debug("Could not recover album year from sibling for %s: %s", album_id, exc) + return None + + def copy_album_identity_from_target_sibling( database, album_id: str, diff --git a/core/library/redownload.py b/core/library/redownload.py index 08d5cd70..a2bc895f 100644 --- a/core/library/redownload.py +++ b/core/library/redownload.py @@ -16,6 +16,7 @@ from core.runtime_state import ( download_tasks, tasks_lock, ) +from core.metadata.album_tracks import get_album_for_source from core.metadata.registry import ( get_deezer_client, get_itunes_client, @@ -26,6 +27,27 @@ from database.music_database import get_database logger = logging.getLogger(__name__) +def _album_data_from_source(full: dict, album_id: str, fallback_name: str) -> dict: + """Build the redownload `album_data` from a primary-source get_album result (#915). + + Mirrors the Spotify branch's album_data shape so iTunes/Deezer redownloads carry the + real release_date / album_type / total_tracks — instead of a lean {'name': ...} that + drops the $year and forces a manual reorganize afterwards.""" + images = full.get('images') or [] + image_url = full.get('image_url') or '' + if not image_url and images and isinstance(images[0], dict): + image_url = images[0].get('url', '') + return { + 'id': str(full.get('id') or album_id), + 'name': full.get('name') or fallback_name, + 'release_date': full.get('release_date', ''), + 'album_type': full.get('album_type', 'album'), + 'total_tracks': full.get('total_tracks', 0), + 'images': images, + 'image_url': image_url, + } + + def _get_itunes_client(): """Mirror of web_server._get_itunes_client — delegates to registry.""" return get_itunes_client() @@ -141,12 +163,26 @@ def redownload_start(track_id): 'images': album_images, 'image_url': album_images[0]['url'] if album_images else '', } - elif meta_source == 'itunes': - track_number = full_track_details.get('trackNumber') - disc_number = full_track_details.get('discNumber', 1) - elif meta_source == 'deezer': - track_number = full_track_details.get('track_position') - disc_number = full_track_details.get('disk_number', 1) + elif meta_source in ('itunes', 'deezer'): + # #915: parity with the Spotify branch + Reorganize — pull the full album from + # the primary source so album_data carries release_date/album_type/total_tracks + # (was lean {'name': ...}, which dropped the $year on iTunes/Deezer redownloads). + if meta_source == 'itunes': + track_number = full_track_details.get('trackNumber') + disc_number = full_track_details.get('discNumber', 1) + _alb_id = full_track_details.get('collectionId') + else: + track_number = full_track_details.get('track_position') + disc_number = full_track_details.get('disk_number', 1) + _alb_id = (full_track_details.get('album') or {}).get('id') + if _alb_id: + try: + _full_album = get_album_for_source(meta_source, str(_alb_id)) + except Exception as _alb_err: # noqa: BLE001 — never let metadata break redownload + logger.debug("[Redownload] %s album fetch failed: %s", meta_source, _alb_err) + _full_album = None + if isinstance(_full_album, dict): + album_data = _album_data_from_source(_full_album, str(_alb_id), metadata.get('album', '')) track_data = { 'id': meta_id, diff --git a/core/library_reorganize.py b/core/library_reorganize.py index a5c9e4cd..574d5ee7 100644 --- a/core/library_reorganize.py +++ b/core/library_reorganize.py @@ -27,6 +27,7 @@ entirely. """ import os +import re import shutil import threading import time @@ -407,6 +408,18 @@ def _differentiators_in(norm_title: str) -> frozenset: return frozenset(t for t in norm_title.split() if t in _VERSION_DIFFERENTIATORS) +# Featured-artist credit: "(feat. X)" / "[ft X]" / a trailing "feat. X". The +# parenthesised form is stripped wherever it appears; the bare form only when +# something follows it (so a song literally named "The Feat" is left alone, and +# "Defeat"/"Lift" never trip the word-boundary). Case-insensitive. +_FEAT_RE = re.compile( + r"""\s*[\(\[]\s*(?:feat|ft|featuring)\b\.?[^)\]]*[\)\]] # (feat. X) / [ft. X] + | \s+(?:feat|ft|featuring)\b\.?\s+\S.*$ # trailing feat. X ... + """, + re.IGNORECASE | re.VERBOSE, +) + + def _normalize_title(value) -> str: """Lowercase + strip cosmetic punctuation and treat brackets / dashes / slashes as word separators so the same track named slightly @@ -418,10 +431,17 @@ def _normalize_title(value) -> str: - ``Don't Stop Believin'`` ↔ ``Don’t Stop Believin’`` - ``Swimming Pools (Drank) - Extended Version`` ↔ ``Swimming Pools (Drank) (Extended Version)`` + - ``The Chase (feat. Big Artist)`` ↔ ``The Chase`` (#914) """ if value is None: return '' - out = str(value).strip().lower() + out = str(value).strip() + # #914: drop featured-artist credits FIRST (while the parens are still here to + # bound the group). iTunes appends "(feat. X)" to track titles while a user's + # file is often just "The Chase" — the credit is metadata, not the song's + # identity, and leaving it in dropped the match ratio below the threshold so + # correctly-identified tracks reported as "not in the tracklist". + out = _FEAT_RE.sub('', out).lower() # Strip characters that don't carry meaning across providers. for ch in ('"', "'", '‘', '’', '“', '”', '.', ',', '!', '?', '(', ')', '[', ']', '{', '}'): diff --git a/core/navidrome_client.py b/core/navidrome_client.py index edfc7ffc..e042788a 100644 --- a/core/navidrome_client.py +++ b/core/navidrome_client.py @@ -1192,7 +1192,11 @@ class NavidromeClient(MediaServerClient): primary = existing_playlists[0] existing_tracks = self.get_playlist_tracks(primary.id) - current_ids = [str(t.id) for t in existing_tracks if getattr(t, 'id', None)] + # #905: NavidromeTrack exposes the Subsonic song id as `ratingKey` (NOT `.id`, + # which doesn't exist) — same as append_to_playlist reads it. Reading `t.id` here + # made current_ids ALWAYS empty, so reconcile thought the playlist was empty and + # re-added every track each sync (playlists doubling) while removing nothing. + current_ids = [str(t.ratingKey) for t in existing_tracks if getattr(t, 'ratingKey', None)] desired_ids = [] for t in tracks: tid = (str(t.ratingKey) if hasattr(t, 'ratingKey') diff --git a/core/repair_jobs/empty_folder_cleaner.py b/core/repair_jobs/empty_folder_cleaner.py index fe1f5d02..e660a52c 100644 --- a/core/repair_jobs/empty_folder_cleaner.py +++ b/core/repair_jobs/empty_folder_cleaner.py @@ -91,13 +91,19 @@ class EmptyFolderCleanerJob(RepairJob): ignore_disposable = False try: if context.config_manager: - ignore_junk = bool(context.config_manager.get( - 'repair.jobs.empty_folder_cleaner.remove_junk_files', True)) - # #891: also clear folders left holding only images / .lrc / sidecars - # (what a reorganize leaves behind). Opt-in — default off. - ignore_disposable = bool(context.config_manager.get( - 'repair.jobs.empty_folder_cleaner.remove_residual_files', False)) - except Exception: # noqa: S110 — setting read is best-effort; defaults to True + # #912: job settings are persisted as a nested dict under + # `repair.jobs..settings` (see RepairWorker.set_job_settings / get_job_config). + # The old flat-key reads ('repair.jobs.empty_folder_cleaner.remove_residual_files') + # never matched what the UI saves, so the #891 opt-in toggle silently did nothing — + # the scan always fell back to the False default and skipped every image/.lrc folder. + job_settings = context.config_manager.get( + 'repair.jobs.empty_folder_cleaner.settings', {}) or {} + if isinstance(job_settings, dict): + ignore_junk = bool(job_settings.get('remove_junk_files', True)) + # #891: also clear folders left holding only images / .lrc / sidecars + # (what a reorganize leaves behind). Opt-in — default off. + ignore_disposable = bool(job_settings.get('remove_residual_files', False)) + except Exception: # noqa: S110 — setting read is best-effort; defaults to junk-only pass flagged = set() # dir paths we'd remove → a parent sees them as "gone" diff --git a/core/watchlist_scanner.py b/core/watchlist_scanner.py index d4584519..33a74cf4 100644 --- a/core/watchlist_scanner.py +++ b/core/watchlist_scanner.py @@ -3971,6 +3971,14 @@ class WatchlistScanner: except Exception as e: logger.debug(f"Error building BYLT for {played_artist.get('name', '?')}: {e}") + # #913: listening-driven recommendations — consensus-ranked artists you'd love but + # don't own + candidate playlist tracks. Self-contained + double-guarded so it can + # never affect the scan. + try: + self._build_listening_recommendations(profile_id, sources_to_process) + except Exception as _lr_err: # noqa: BLE001 — never let recs break the scan + logger.debug("[Listening Recs] skipped: %s", _lr_err) + # Also save without suffix for backward compatibility (use first active source). active_source = sources_to_process[0] release_radar_key = f'release_radar_{active_source}' @@ -3989,6 +3997,80 @@ class WatchlistScanner: import traceback traceback.print_exc() + def _build_listening_recommendations(self, profile_id, sources_to_process): + """#913: consensus-ranked artists you'd love but don't own, plus candidate playlist + tracks, derived from your most-played artists + the similar_artists graph. + + The ranking lives in core.discovery.listening_recommendations (pure + tested); this only + gathers inputs — all already in the DB, NO new network — and stores the result under NEW + metadata/curated keys. Fully self-contained and guarded: any failure logs and returns, so + it can never disturb the scan. Phase-1 candidate tracks come from the discovery pool (like + BYLT); a later phase swaps in a direct top-tracks fetch for richer coverage. + """ + try: + import json as _json + from core.discovery.listening_recommendations import ( + aggregate_candidate_tracks, + group_similars_by_seed, + rank_recommended_artists, + ) + + seeds = [{'name': s['name'], 'weight': s.get('play_count', 1)} + for s in (self.database.get_top_artists('all', 30) or []) if s.get('name')] + if not seeds: + return + + # id -> name + owned-artist set for the WHOLE library (similar_artists rows key the + # similar artist by the seed artist's id). + id_to_name, owned = {}, set() + with self.database._get_connection() as conn: + cur = conn.cursor() + cur.execute("SELECT id, name FROM artists WHERE name IS NOT NULL AND name != ''") + for row in cur.fetchall(): + id_to_name[str(row[0])] = row[1] + owned.add((row[1] or '').lower()) + + similar_rows = self.database.get_top_similar_artists(limit=1000, profile_id=profile_id) + similars_by_seed = group_similars_by_seed(seeds, similar_rows, id_to_name) + recs = rank_recommended_artists(seeds, similars_by_seed, owned, limit=40) + if not recs: + logger.info("[Listening Recs] no recommendations yet (no similar-artist coverage)") + return + + self.database.set_metadata('listening_recs_artists', _json.dumps([ + {'name': r.name, 'seed_count': r.seed_count, 'seeds': r.seeds[:5], 'score': r.score} + for r in recs + ])) + + # Candidate tracks from the discovery pool grouped by artist (phase 1: no new network). + pool, active_source = [], None + for src in (sources_to_process or []): + pool = self.database.get_discovery_pool_tracks( + limit=5000, new_releases_only=False, source=src, profile_id=profile_id) + if pool: + active_source = src + break + + track_ids = [] + if pool: + by_artist = {} + for t in pool: + an = (getattr(t, 'artist_name', '') or '').lower() + tid = (getattr(t, 'spotify_track_id', None) if active_source == 'spotify' + else getattr(t, 'itunes_track_id', None) if active_source == 'itunes' + else getattr(t, 'deezer_track_id', None)) + if an and tid: + by_artist.setdefault(an, []).append({'name': getattr(t, 'track_name', ''), 'id': tid}) + candidates = aggregate_candidate_tracks(recs, by_artist, per_artist=3, limit=50) + track_ids = [c['id'] for c in candidates if c.get('id')] + if track_ids: + self.database.save_curated_playlist('listening_recs_tracks', track_ids, profile_id=profile_id) + + logger.info("[Listening Recs] %d recommended artists, %d candidate tracks", + len(recs), len(track_ids)) + except Exception as e: + logger.debug("[Listening Recs] generation skipped: %s", e) + def sync_spotify_library_cache(self, profile_id=1): """Sync user's saved Spotify albums into the local cache. diff --git a/database/music_database.py b/database/music_database.py index 1b140874..2eb1ed80 100644 --- a/database/music_database.py +++ b/database/music_database.py @@ -301,6 +301,7 @@ class MusicDatabase: file_path TEXT, bitrate INTEGER, file_size INTEGER, -- bytes; populated by deep scan from media-server API + year INTEGER, -- per-track release year from file tags (albums.year is canonical) created_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP, updated_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP, FOREIGN KEY (album_id) REFERENCES albums (id) ON DELETE CASCADE, @@ -1156,6 +1157,14 @@ class MusicDatabase: if track_cols and 'file_size' not in track_cols: cursor.execute("ALTER TABLE tracks ADD COLUMN file_size INTEGER") logger.info("Repaired missing file_size column on tracks table") + # #910 — Full Refresh writes a per-track `year` (from file tags), but the column + # was only ever in the live INSERT, never in CREATE TABLE or a migration. On any + # DB that predates this fix, every Full Refresh track insert hard-fails with + # "table tracks has no column named year". Additive + nullable; nothing reads it + # except the writer, so this is safe to backfill on every existing DB. + if track_cols and 'year' not in track_cols: + cursor.execute("ALTER TABLE tracks ADD COLUMN year INTEGER") + logger.info("Repaired missing year column on tracks table (#910)") cursor.execute("PRAGMA table_info(albums)") album_cols = {c[1] for c in cursor.fetchall()} diff --git a/pr_description.md b/pr_description.md index 501ebcc7..22a15ec1 100644 --- a/pr_description.md +++ b/pr_description.md @@ -1,41 +1,44 @@ -# soulsync 2.7.6 — `dev` → `main` +# soulsync 2.7.7 — `dev` → `main` -patch release on top of 2.7.5. the headline is going the *other* way with playlists — exporting them TO listenbrainz — plus youtube liked-music sync, a deep-scan data-loss guard, and a round of dashboard performance work. +a fix-heavy patch on top of 2.7.6 — a big sweep of reported issues, the start of listening-driven recommendations, and a metadata-parity fix that stops downloads from needing a manual reorganize afterward. --- ## what's new -### export playlists to listenbrainz (#903) -soulsync already pulls playlists IN from everywhere — now it can push one back OUT. every mirrored-playlist card gets a 📤 export button: pick **download .jspf** (a standard playlist file you can hand-upload anywhere) or **sync to listenbrainz** (creates the playlist straight on your LB account). each track is matched to its musicbrainz *recording* id via a cheapest-first waterfall — cache → your library (`musicbrainz_recording_id`) → the file's own tag → a live musicbrainz lookup — with the result cached so the same song never costs twice. live "matching N/M · X matched" status shows on the card, and re-syncing **updates the same LB playlist in place** instead of making duplicates. tracks that can't be resolved to an MBID are skipped (LB requires them) and counted so you see the coverage. +### downloads now tag + path like reorganize does (#915) +the headline fix. when you add or redownload music, post-processing used to backfill missing album data from **spotify only** — so an iTunes/deezer-primary user kept a "lean" context and the path **dropped the `$year`** while the release date defaulted to `YYYY-01-01`. you'd then run a reorganize to fix it every time. now post-processing (and redownload) pull the full album from your **primary metadata source** — the exact same place reorganize/enrich read — so the year, real release date, and album type are right the first time. covers the add/download flow and single-track redownload (iTunes + deezer). -### youtube liked-music sync (#902) -you can now sync your youtube music **Liked Music** playlist (`music.youtube.com/playlist?list=LM`). it's a private playlist, so it needs auth — and the existing "read a browser's cookies" option only works when the browser is on the same machine. added a **"paste cookies.txt"** option in Settings → YouTube so server/docker installs (and anyone on a browser like Zen that yt-dlp can't read) can supply their login from anywhere. +### listening-driven recommendations — foundation (#913) +the start of "discover based on what you actually listen to." during the watchlist scan, soulsync now ranks artists you'd love but don't own — seeded from your top-played artists, scored by **consensus** (who's similar to *many* of your favorites), play weight, and similarity strength — and builds a candidate track list from them. generated and stored now; the discover row + synced playlist come next. -### deep scan won't relocate your library (#904) -a standalone Deep Scan moves files it doesn't recognize into Staging for import. if the DB was empty/out of sync with disk (a volume swap, a DB reset, external tag edits), it treated your **entire** library as "unrecognized" and relocated all of it — one user lost ~1,500 tracks into Staging. now a guard refuses the move when the unrecognized share is implausibly large (the desync signature), leaves everything in place, and warns instead. plus a **"Transfer is my permanent library — never move files out"** toggle for people whose Transfer folder *is* their live library. - -### dashboard performance -a pass at the "soulsync makes my GPU work hard just sitting there" complaints. the sidebar sweep animates `transform` instead of `left` (no per-frame layout), particle glows are pre-rendered sprites instead of per-frame gradients, blur radii + redundant/invisible card shadows are trimmed, and low-power machines auto-drop to performance mode. all the visible effects stay; they're just cheaper to draw. +### jellyfin stops indexing half-written tracks +multi-disc tracks landing with "no disc" in jellyfin turned out to be a write race: a cross-filesystem move (downloads volume → library volume) wrote the file to its final path **incrementally**, and jellyfin's real-time watcher could catch it mid-write and cache incomplete metadata. now the final placement is **atomic** — copy to a hidden temp sibling, then an atomic rename — so a watcher only ever sees the complete file. ### fixes -- **file-import manual matches stick (#901)** — a manual match on a file-imported playlist track is no longer forgotten on re-sync (the tracks now carry a stable id; existing ones are backfilled once). -- **manual match heals a stale Plex key** — a Find & Add match whose stored Plex ratingKey went stale is now re-resolved against a live Plex search instead of silently breaking. -- **multi-disc albums** — a track now files into the Disc folder that matches its own disc tag (no more disc-2 tracks landing in Disc 1), and a track is never written disc-less. -- **auto-download track numbers** — a track auto-grabbed from the playlist pipeline / wishlist / watchlist now gets its real in-album position instead of being tagged `1/1`. +- **navidrome playlists doubling (#905)** — every resync re-added the whole playlist (a 4-song list grew to 12). reconcile read the server's current tracks via a missing attribute, so it always thought the playlist was empty. fixed; also pushes a deduped list. +- **youtube playlists capped at ~100 (#908)** — a yt-dlp/youtube regression truncated big playlists (Liked Music came back as 104). worked around to page past it (~200) until the upstream fix lands. +- **album redownload grabbed the wrong edition (#911)** — it did a fresh search instead of using the album's matched source id, so a 66-track OST could redownload as a 19-track single. now uses the canonical matched source. +- **iTunes albums over 50 tracks truncated (#918)** — the iTunes lookup defaulted to 50 entities; now requests the full album. +- **enhanced view showed multi-disc tracks as missing (#916)** — owned disc-2+ tracks (stored as disc 1) no longer flag as "missing"; matched by title like reorganize. +- **reorganize vs "(feat. X)" (#914)** — a bare local title now matches an iTunes track titled "Song (feat. Artist)" instead of reporting it not-in-tracklist. +- **"I have this" dropped the year (#917)** — it rebuilt a yearless path and copied into a new folder; now reuses the album's existing folder. +- **full refresh imported 0 tracks (#910)** — every track insert failed on a missing `year` column; added it + a migration so older DBs self-heal. +- **youtube discovery "Unknown Artist" (#909)** — when youtube hands back only a title, the matched artist now backfills the column instead of leaving "Unknown Artist". +- **empty folder cleaner toggle did nothing (#912)** — the "also remove image/sidecar-only folders" option read the wrong config key; now honored. --- ## a brief recap of what came before -2.7.5 was a fix-heavy cycle — matching & artwork accuracy, the HiFi preview mess (#895) — plus M3U import (#893), ignore-list management (#897), and per-playlist file naming. 2.7.4 was re-identify; 2.7.3 the Quality Upgrade Finder + ignore-list (#874); 2.7.2 playlist-folder mirroring + M3U export; 2.7.1 download verification + a review queue (#852); 2.7.0 made multi-user real. +2.7.6 went the *other* way with playlists — exporting them TO listenbrainz — plus youtube liked-music sync, a deep-scan data-loss guard (#904), and dashboard performance work. 2.7.5 was matching & artwork accuracy + M3U import; 2.7.4 re-identify; 2.7.3 the Quality Upgrade Finder; 2.7.2 playlist-folder mirroring; 2.7.1 download verification; 2.7.0 made multi-user real. --- ## tests -additive + fail-safe — new behavior is opt-in or guarded, nothing existing rewired. new seam/regression suites across the listenbrainz export (JSPF build, the MBID waterfall + dedup, the persistent cache, the LB create/update-in-place client), the youtube cookie precedence, and the #904 deep-scan guard (incl. the 1,500-file regression). the listenbrainz push + update-in-place were also validated live against a real account. relevant suites green; `ruff check` clean on touched modules. +additive + fail-safe — new behavior is guarded or scoped, nothing existing rewired. new seam/regression suites across the `year`-column migration (#910), the navidrome reconcile fix (#905 — reverting the one-char change flips the tests red), feat-matching (#914), the multi-disc not-missing logic (#916), the iTunes full-album limit (#918, proven live against the real API), the "I have this" year recovery (#917), the primary-source backfill (#915), the listening-recs core (#913), and atomic file placement. relevant suites green; `ruff check` clean repo-wide. ## post-merge -- [ ] tag `v2.7.6` on `main` -- [ ] docker-publish with `version_tag: 2.7.6` +- [ ] tag `v2.7.7` on `main` +- [ ] docker-publish with `version_tag: 2.7.7` - [ ] discord announce (auto-fired by the workflow) -- [ ] reply on #902 / #903 / #904 +- [ ] reply on the issue batch (#905 / #908 / #909 / #910 / #911 / #912 / #913 / #914 / #915 / #916 / #917 / #918) diff --git a/services/sync_service.py b/services/sync_service.py index c1213f1a..077af59a 100644 --- a/services/sync_service.py +++ b/services/sync_service.py @@ -37,6 +37,23 @@ def _plex_track_file(plex_track) -> str: return '' +def _dedupe_by_rating_key(tracks: list) -> list: + """Drop repeated media-server tracks (same ratingKey), preserving first-seen + order. The same library track can match more than one source entry (or appear + twice in the source), and pushing the dupes made reconcile/replace re-add the + track every sync — part of the #905 doubling. The sync dispatch MUST send this + deduped list, not the raw matched list.""" + seen = set() + out = [] + for t in tracks: + key = getattr(t, 'ratingKey', None) + if key is None or key in seen: + continue + seen.add(key) + out.append(t) + return out + + def reresolve_manual_match_live_plex(cache_db, media_client, m, *, profile_id, source_track_id, server_source): """Re-resolve a manual match whose stored Plex ratingKey went stale. @@ -438,21 +455,16 @@ class PlaylistSyncService: f"{server_type.title()} objects with ratingKeys" ) - # Deduplicate by ratingKey — media servers silently drop duplicates - seen_keys = set() - deduped_tracks = [] - for t in valid_tracks: - if t.ratingKey not in seen_keys: - seen_keys.add(t.ratingKey) - deduped_tracks.append(t) - if len(deduped_tracks) < len(valid_tracks): + # Deduplicate by ratingKey — media servers silently drop duplicates, + # and pushing dupes made every sync re-add the same track (#905). The + # dispatch below sends THIS deduped list, never the raw `valid_tracks`. + plex_tracks = _dedupe_by_rating_key(valid_tracks) + if len(plex_tracks) < len(valid_tracks): logger.info( - f"Deduplicated {len(valid_tracks) - len(deduped_tracks)} duplicate ratingKeys " - f"({len(valid_tracks)} → {len(deduped_tracks)} tracks)" + f"Deduplicated {len(valid_tracks) - len(plex_tracks)} duplicate ratingKeys " + f"({len(valid_tracks)} → {len(plex_tracks)} tracks)" ) - plex_tracks = deduped_tracks - if not media_client: logger.error("No active media client available for playlist sync") sync_success = False @@ -462,11 +474,11 @@ class PlaylistSyncService: f"(mode: {sync_mode})" ) if sync_mode == 'append': - sync_success = media_client.append_to_playlist(playlist.name, valid_tracks) + sync_success = media_client.append_to_playlist(playlist.name, plex_tracks) elif sync_mode == 'reconcile': - sync_success = self._reconcile_or_replace(media_client, playlist.name, valid_tracks) + sync_success = self._reconcile_or_replace(media_client, playlist.name, plex_tracks) else: - sync_success = media_client.update_playlist(playlist.name, valid_tracks) + sync_success = media_client.update_playlist(playlist.name, plex_tracks) synced_tracks = len(plex_tracks) if sync_success else 0 # Not in library (for wishlist), not "total minus playlist size". diff --git a/tests/discovery/test_discovery_youtube.py b/tests/discovery/test_discovery_youtube.py index 5114f159..e7f816a5 100644 --- a/tests/discovery/test_discovery_youtube.py +++ b/tests/discovery/test_discovery_youtube.py @@ -406,3 +406,31 @@ def test_results_sorted_by_index(): indices = [r['index'] for r in states['h11']['discovery_results']] assert indices == sorted(indices) + + +# --------------------------------------------------------------------------- +# resolve_display_artist (#909) — YT-artist column backfill +# --------------------------------------------------------------------------- + +def test_display_artist_keeps_recovered_name(): + # Recovery already produced a real artist → never overwritten by the match. + assert dy.resolve_display_artist('Goo Goo Dolls', 'Spotify Goo') == 'Goo Goo Dolls' + + +def test_display_artist_backfills_from_match_when_unknown(): + # YouTube/recovery gave nothing, but we matched → show the matched artist. + assert dy.resolve_display_artist('Unknown Artist', 'Don McLean') == 'Don McLean' + assert dy.resolve_display_artist('', 'Don McLean') == 'Don McLean' + assert dy.resolve_display_artist(None, 'Don McLean') == 'Don McLean' + + +def test_display_artist_stays_unknown_with_no_match(): + # No recovery AND no match → honest "Unknown Artist" (an unmatched/error row). + assert dy.resolve_display_artist('Unknown Artist', '') == 'Unknown Artist' + assert dy.resolve_display_artist('Unknown Artist', None) == 'Unknown Artist' + assert dy.resolve_display_artist('', '') == 'Unknown Artist' + + +def test_display_artist_trims_whitespace(): + assert dy.resolve_display_artist(' ', 'Matched') == 'Matched' + assert dy.resolve_display_artist('Real Artist ', '') == 'Real Artist' diff --git a/tests/discovery/test_listening_recommendations.py b/tests/discovery/test_listening_recommendations.py new file mode 100644 index 00000000..a4610c80 --- /dev/null +++ b/tests/discovery/test_listening_recommendations.py @@ -0,0 +1,169 @@ +"""Listening-driven recommendation core (#913) — pure ranking + candidate aggregation.""" + +from __future__ import annotations + +from core.discovery.listening_recommendations import ( + aggregate_candidate_tracks, + rank_recommended_artists, +) + + +def _seed(name, weight=1.0): + return {"name": name, "weight": weight} + + +# ── rank_recommended_artists ───────────────────────────────────────────────── +def test_consensus_outranks_single_endorsement(): + # 'Common' is similar to BOTH seeds; 'Solo' to one. Equal weights/scores. + seeds = [_seed("A"), _seed("B")] + sims = { + "a": [{"name": "Common"}, {"name": "Solo"}], + "b": [{"name": "Common"}], + } + out = rank_recommended_artists(seeds, sims) + assert [r.name for r in out] == ["Common", "Solo"] + assert out[0].seed_count == 2 + assert sorted(out[0].seeds) == ["A", "B"] + assert out[1].seed_count == 1 + + +def test_play_weight_boosts_a_seeds_similars(): + seeds = [_seed("Fav", weight=100), _seed("Minor", weight=1)] + sims = {"fav": [{"name": "FromFav"}], "minor": [{"name": "FromMinor"}]} + out = rank_recommended_artists(seeds, sims) + assert out[0].name == "FromFav" # heavier seed's similar wins + + +def test_similarity_score_weights_within_a_seed(): + seeds = [_seed("A")] + sims = {"a": [{"name": "Close", "score": 0.9}, {"name": "Far", "score": 0.1}]} + out = rank_recommended_artists(seeds, sims) + assert [r.name for r in out] == ["Close", "Far"] + + +def test_owned_and_seed_artists_are_excluded(): + seeds = [_seed("A"), _seed("B")] + sims = {"a": [{"name": "Owned"}, {"name": "B"}, {"name": "New"}]} # 'B' is a seed + out = rank_recommended_artists(seeds, sims, owned_artist_names={"owned"}) + assert [r.name for r in out] == ["New"] # Owned dropped, seed B dropped + + +def test_min_seed_count_filters_low_consensus(): + seeds = [_seed("A"), _seed("B")] + sims = {"a": [{"name": "Common"}, {"name": "Solo"}], "b": [{"name": "Common"}]} + out = rank_recommended_artists(seeds, sims, min_seed_count=2) + assert [r.name for r in out] == ["Common"] # 'Solo' (1 seed) dropped + + +def test_case_insensitive_dedup_and_matching(): + seeds = [_seed("Radiohead")] + sims = {"radiohead": [{"name": "Muse"}, {"name": "MUSE"}]} # same artist twice + out = rank_recommended_artists(seeds, sims) + assert len(out) == 1 and out[0].name in ("Muse", "MUSE") + assert out[0].score == 2.0 # accumulated (still one seed) + assert out[0].seed_count == 1 + + +def test_empty_and_limit(): + assert rank_recommended_artists([], {}) == [] + seeds = [_seed("A")] + sims = {"a": [{"name": f"S{i}"} for i in range(10)]} + assert len(rank_recommended_artists(seeds, sims, limit=3)) == 3 + + +# ── aggregate_candidate_tracks ─────────────────────────────────────────────── +def _recs(*names): + return rank_recommended_artists( + [_seed(n) for n in names], + {n.lower(): [{"name": f"sim-{n}"}] for n in names}, + ) + + +def test_aggregate_caps_per_artist_and_total_in_rank_order(): + recs = _recs("A", "B") # -> recommended sim-A, sim-B + tracks = { + "sim-a": [{"name": "a1"}, {"name": "a2"}, {"name": "a3"}], + "sim-b": [{"name": "b1"}, {"name": "b2"}], + } + out = aggregate_candidate_tracks(recs, tracks, per_artist=2, limit=10) + names = [t["name"] for t in out] + assert names == ["a1", "a2", "b1", "b2"] # per_artist=2, rank order + assert all(t["_seed_artist"].startswith("sim-") for t in out) + + +def test_aggregate_excludes_owned_when_requested(): + recs = _recs("A") + tracks = {"sim-a": [{"name": "Owned Song"}, {"name": "New Song"}]} + owned = {("sim-a", "owned song")} + out = aggregate_candidate_tracks(recs, tracks, owned, per_artist=5, exclude_owned=True) + assert [t["name"] for t in out] == ["New Song"] + # replay flavor keeps owned + keep = aggregate_candidate_tracks(recs, tracks, owned, per_artist=5, exclude_owned=False) + assert [t["name"] for t in keep] == ["Owned Song", "New Song"] + + +def test_aggregate_dedups_and_respects_total_limit(): + recs = _recs("A", "B") + tracks = {"sim-a": [{"name": "dup"}], "sim-b": [{"name": "dup"}, {"name": "x"}]} + # 'dup' under sim-a and sim-b are different (artist,title) keys -> both kept; + # within an artist a repeat would dedup. Here check the total limit instead. + out = aggregate_candidate_tracks(recs, tracks, per_artist=5, limit=2) + assert len(out) == 2 + + +def test_aggregate_skips_artist_with_no_tracks(): + recs = _recs("A", "B") + out = aggregate_candidate_tracks(recs, {"sim-a": [{"name": "only"}]}, per_artist=5) + assert [t["name"] for t in out] == ["only"] # sim-b had no tracks -> skipped + + +# ── group_similars_by_seed (id->name join) ─────────────────────────────────── +from dataclasses import dataclass as _dc # noqa: E402 + +from core.discovery.listening_recommendations import group_similars_by_seed # noqa: E402 + + +@_dc +class _Row: + source_artist_id: str + similar_artist_name: str + + +def test_group_resolves_source_id_to_seed_name(): + seeds = [_seed("Radiohead"), _seed("Bjork")] + rows = [ + _Row("id-rh", "Muse"), + _Row("id-rh", "Coldplay"), + _Row("id-bj", "Portishead"), + _Row("id-unknown", "Nobody"), # id not in map -> dropped + ] + id_to_name = {"id-rh": "Radiohead", "id-bj": "Bjork"} + out = group_similars_by_seed(seeds, rows, id_to_name) + assert {n["name"] for n in out["radiohead"]} == {"Muse", "Coldplay"} + assert [n["name"] for n in out["bjork"]] == ["Portishead"] + assert "id-unknown" not in out and "Nobody" not in str(out) + + +def test_group_keeps_only_rows_for_actual_seeds(): + # id resolves to a name, but that name isn't a seed -> dropped. + seeds = [_seed("A")] + rows = [_Row("id-a", "SimA"), _Row("id-x", "SimX")] + out = group_similars_by_seed(seeds, rows, {"id-a": "A", "id-x": "X"}) + assert list(out.keys()) == ["a"] + + +def test_group_accepts_dict_rows(): + seeds = [_seed("A")] + rows = [{"source_artist_id": "id-a", "similar_artist_name": "SimA"}] + out = group_similars_by_seed(seeds, rows, {"id-a": "A"}) + assert out["a"] == [{"name": "SimA"}] + + +def test_group_then_rank_end_to_end(): + # The two-step the scanner will run: group rows, then rank. + seeds = [_seed("A", weight=2), _seed("B", weight=1)] + rows = [_Row("ia", "Common"), _Row("ia", "Solo"), _Row("ib", "Common")] + grouped = group_similars_by_seed(seeds, rows, {"ia": "A", "ib": "B"}) + ranked = rank_recommended_artists(seeds, grouped, owned_artist_names={"solo"}) + assert ranked[0].name == "Common" and ranked[0].seed_count == 2 + assert all(r.name != "Solo" for r in ranked) # owned excluded diff --git a/tests/downloads/test_track_metadata_backfill.py b/tests/downloads/test_track_metadata_backfill.py index 99f77351..6eb74ed5 100644 --- a/tests/downloads/test_track_metadata_backfill.py +++ b/tests/downloads/test_track_metadata_backfill.py @@ -507,5 +507,73 @@ def test_api_called_at_most_once_per_invocation(): assert client.get_track_details.call_count == 1 +# ── #915: source-aware album-context backfill (parity with Reorganize/Enrich) ── +from core.downloads.track_metadata_backfill import backfill_album_context_from_source # noqa: E402 + + +def _lean_ctx(album_id="itunes-123"): + return {"id": album_id, "name": "Big OST", "release_date": "", "total_tracks": 0, "album_type": "album"} + + +def _itunes_album(**over): + base = {"id": "itunes-123", "name": "Big OST", "release_date": "2024-04-17", + "total_tracks": 70, "album_type": "album"} + base.update(over) + return base + + +def test_backfill_hydrates_lean_context_from_primary_source(): + ctx = _lean_ctx() + calls = [] + + def get_album(source, album_id): + calls.append((source, album_id)) + return _itunes_album() + + assert backfill_album_context_from_source(ctx, "itunes", get_album) is True + assert ctx["release_date"] == "2024-04-17" # real date, not YYYY-01-01 + assert ctx["total_tracks"] == 70 + assert calls == [("itunes", "itunes-123")] # queried the PRIMARY source with the album id + + +def test_backfill_noop_when_context_already_complete(): + ctx = {"id": "itunes-123", "release_date": "2024-04-17", "total_tracks": 70, "album_type": "album"} + called = [] + backfill_album_context_from_source(ctx, "itunes", lambda *a: called.append(a)) + assert called == [] # complete -> no fetch + assert ctx["release_date"] == "2024-04-17" + + +def test_backfill_noop_for_spotify_primary(): + # Spotify is covered by hydrate_download_metadata's get_track_details path. + called = [] + assert backfill_album_context_from_source(_lean_ctx(), "spotify", lambda *a: called.append(a)) is False + assert called == [] + + +def test_backfill_noop_for_sentinel_album_id(): + called = [] + for sentinel in ("explicit_album", "from_sync_modal", ""): + backfill_album_context_from_source(_lean_ctx(sentinel), "itunes", lambda *a: called.append(a)) + assert called == [] # no real id -> never queries + + +def test_backfill_leaves_context_lean_when_source_returns_nothing(): + ctx = _lean_ctx() + backfill_album_context_from_source(ctx, "itunes", lambda *a: None) + assert ctx["release_date"] == "" # still lean, but no crash + + +def test_backfill_swallows_source_errors(): + ctx = _lean_ctx() + + def boom(*_a): + raise RuntimeError("itunes down") + + # Must not raise — a backfill failure cannot break a download. + assert backfill_album_context_from_source(ctx, "itunes", boom) is False + assert ctx["release_date"] == "" + + if __name__ == '__main__': pytest.main([__file__, '-v']) diff --git a/tests/imports/test_import_file_ops.py b/tests/imports/test_import_file_ops.py index ce9ecd4a..a23df527 100644 --- a/tests/imports/test_import_file_ops.py +++ b/tests/imports/test_import_file_ops.py @@ -169,3 +169,78 @@ def test_read_staging_file_metadata_uses_filename_fallbacks_when_tags_are_invali "track_number": 2, "disc_number": 1, } + + +# ── atomic cross-filesystem move (Jellyfin null-disc mitigation) ────────────── +import errno # noqa: E402 +import os # noqa: E402 + +import pytest # noqa: E402 + +from core.imports import file_ops as _fo # noqa: E402 +from core.imports.file_ops import _atomic_cross_device_move # noqa: E402 + + +def test_same_fs_move_moves_and_removes_source(tmp_path): + src = tmp_path / "s.flac" + src.write_text("hello") + dst = tmp_path / "lib" / "t.flac" # parent created by safe_move_file + safe_move_file(src, dst) + assert dst.read_text() == "hello" + assert not src.exists() + + +def test_cross_device_move_routes_to_atomic_and_completes(tmp_path, monkeypatch): + # Simulate a cross-filesystem move: the same-fs os.replace raises EXDEV, and the + # atomic helper's temp->dst replace (same fs) succeeds. The file must complete and + # no partial temp file may be left at the final name's directory. + src = tmp_path / "s.flac" + src.write_text("payload") + dstdir = tmp_path / "lib" + dstdir.mkdir() + dst = dstdir / "t.flac" + + real_replace = os.replace + + def fake_replace(a, b): + if str(a) == str(src): # the cross-fs move attempt + raise OSError(errno.EXDEV, "Invalid cross-device link") + return real_replace(a, b) # the temp -> dst rename (same fs) + + monkeypatch.setattr(_fo.os, "replace", fake_replace) + safe_move_file(src, dst) + + assert dst.read_text() == "payload" + assert not src.exists() + assert not list(dstdir.glob(".*ssync-tmp")) # complete file only, no leftover temp + + +def test_atomic_helper_completes_and_cleans_temp(tmp_path): + src = tmp_path / "s.flac" + src.write_text("payload") + dstdir = tmp_path / "d" + dstdir.mkdir() + dst = dstdir / "t.flac" + _atomic_cross_device_move(src, dst) + assert dst.read_text() == "payload" + assert not src.exists() + assert not list(dstdir.glob(".*ssync-tmp")) + + +def test_atomic_helper_cleans_temp_and_keeps_source_on_failure(tmp_path, monkeypatch): + src = tmp_path / "s.flac" + src.write_text("payload") + dstdir = tmp_path / "d" + dstdir.mkdir() + dst = dstdir / "t.flac" + + def boom(_a, _b): + raise OSError("replace failed") + + monkeypatch.setattr(_fo.os, "replace", boom) + with pytest.raises(OSError): + _atomic_cross_device_move(src, dst) + + assert src.exists() # source preserved on failure + assert not dst.exists() # no partial final file + assert not list(dstdir.glob(".*ssync-tmp")) # temp cleaned up diff --git a/tests/library/test_missing_track_import.py b/tests/library/test_missing_track_import.py index 077810be..4850c381 100644 --- a/tests/library/test_missing_track_import.py +++ b/tests/library/test_missing_track_import.py @@ -258,3 +258,47 @@ def test_import_rejects_missing_expected_track_context(tmp_path): assert exc.value.status_code == 400 assert "expected_track" in str(exc.value) + + +# ── #917: recover album year from existing folder so "I have this" reuses the dir ── +def _year_db(rows): + conn = sqlite3.connect(":memory:") + conn.row_factory = sqlite3.Row + conn.execute("CREATE TABLE tracks (album_id TEXT, disc_number INTEGER, track_number INTEGER, file_path TEXT, year INTEGER)") + conn.executemany( + "INSERT INTO tracks (album_id, disc_number, track_number, file_path, year) VALUES (?,?,?,?,?)", + [(r.get("album_id", "A1"), r.get("disc_number", 1), r.get("track_number", 1), + r.get("file_path"), r.get("year")) for r in rows], + ) + conn.commit() + return _FakeDB(conn) + + +def _ident(p): + return p + + +def test_year_recovered_from_sibling_year_column(): + db = _year_db([{"track_number": 3, "file_path": "/m/Artist/Album/03.flac", "year": 2024}]) + assert mti._existing_album_year_from_sibling(db, "A1", _ident, 1, 5) == "2024" + + +def test_year_recovered_from_paren_folder_name(): + db = _year_db([{"track_number": 3, "file_path": "/music/Artist/Album (2019)/03 - Song.flac", "year": None}]) + assert mti._existing_album_year_from_sibling(db, "A1", _ident, 1, 5) == "2019" + + +def test_year_recovered_from_bracket_folder_name(): + db = _year_db([{"track_number": 3, "file_path": "/music/Artist/Album [2008]/03.flac", "year": None}]) + assert mti._existing_album_year_from_sibling(db, "A1", _ident, 1, 5) == "2008" + + +def test_year_none_when_no_signal(): + db = _year_db([{"track_number": 3, "file_path": "/music/Artist/Album/03.flac", "year": None}]) + assert mti._existing_album_year_from_sibling(db, "A1", _ident, 1, 5) is None + + +def test_year_ignores_the_target_slot_itself(): + # The only sibling row IS the slot being imported -> excluded -> no year. + db = _year_db([{"disc_number": 1, "track_number": 5, "file_path": "/m/Album (2030)/05.flac", "year": 2030}]) + assert mti._existing_album_year_from_sibling(db, "A1", _ident, 1, 5) is None diff --git a/tests/library/test_redownload_album_data.py b/tests/library/test_redownload_album_data.py new file mode 100644 index 00000000..8a648951 --- /dev/null +++ b/tests/library/test_redownload_album_data.py @@ -0,0 +1,38 @@ +"""Redownload builds full album_data from the primary source (#915). + +iTunes/Deezer single-track redownloads used to carry a lean album_data ({'name': ...}), +dropping the $year folder. _album_data_from_source mirrors the Spotify branch so the real +release_date / album_type / total_tracks come through. +""" + +from __future__ import annotations + +from core.library.redownload import _album_data_from_source + + +def test_builds_full_album_data_from_source(): + full = {'id': 'it-1', 'name': 'Big OST', 'release_date': '2024-04-17', + 'album_type': 'album', 'total_tracks': 70, 'image_url': 'http://x/cover.jpg'} + out = _album_data_from_source(full, 'it-1', 'fallback') + assert out['release_date'] == '2024-04-17' # real date, not YYYY-01-01 + assert out['album_type'] == 'album' + assert out['total_tracks'] == 70 + assert out['id'] == 'it-1' + assert out['name'] == 'Big OST' + assert out['image_url'] == 'http://x/cover.jpg' + + +def test_image_url_falls_back_to_images_array(): + full = {'name': 'A', 'release_date': '2020-01-01', 'images': [{'url': 'http://x/img.jpg'}]} + out = _album_data_from_source(full, 'a1', 'fb') + assert out['image_url'] == 'http://x/img.jpg' + + +def test_defaults_when_fields_missing(): + out = _album_data_from_source({}, 'a1', 'Fallback Album') + assert out['id'] == 'a1' # falls back to the queried id + assert out['name'] == 'Fallback Album' + assert out['album_type'] == 'album' # default + assert out['total_tracks'] == 0 + assert out['release_date'] == '' + assert out['image_url'] == '' diff --git a/tests/test_empty_folder_cleaner.py b/tests/test_empty_folder_cleaner.py index 776a8e5a..3070efcf 100644 --- a/tests/test_empty_folder_cleaner.py +++ b/tests/test_empty_folder_cleaner.py @@ -6,7 +6,10 @@ from __future__ import annotations import os -from core.repair_jobs.empty_folder_cleaner import dir_is_removable, remove_empty_folder, is_junk +from core.repair_jobs.base import JobContext +from core.repair_jobs.empty_folder_cleaner import ( + EmptyFolderCleanerJob, dir_is_removable, is_junk, remove_empty_folder, +) # ── pure decision ─────────────────────────────────────────────────────────── @@ -120,3 +123,51 @@ def test_apply_residual_opt_still_refuses_real_content(tmp_path): remove_disposable=True, root=str(root), **_fx()) assert res['removed'] is False and d.exists() assert (d / 'booklet.pdf').exists() and (d / 'cover.jpg').exists() # nothing deleted + + +# ── #912: scan() must read the opt-in from where the UI SAVES it ───────────── +class _Cfg: + """Mimics ConfigManager: job settings live as a nested dict under + `repair.jobs..settings` (RepairWorker.set_job_settings writes there).""" + + def __init__(self, settings): + self._settings = settings + + def get(self, key, default=None): + if key == 'repair.jobs.empty_folder_cleaner.settings': + return self._settings + return default + + +def _run_scan(tmp_path, settings): + root = tmp_path / 'lib'; root.mkdir() + res_dir = root / 'Artist' / 'Old Album'; res_dir.mkdir(parents=True) + (res_dir / 'cover.jpg').write_text('img') # image + lyric only — the #912 case + (res_dir / 'lyrics.lrc').write_text('la') + keep = root / 'Artist2' / 'Real Album'; keep.mkdir(parents=True) + (keep / 'song.flac').write_text('audio') # has audio — must never be flagged + + flagged = [] + ctx = JobContext( + db=None, transfer_folder=str(root), config_manager=_Cfg(settings), + create_finding=lambda **kw: (flagged.append(kw.get('file_path')), True)[1], + ) + EmptyFolderCleanerJob().scan(ctx) + return str(res_dir), str(keep), set(flagged) + + +def test_scan_flags_residual_folder_when_opt_in_saved_under_settings(tmp_path): + # The toggle is stored at repair.jobs..settings.remove_residual_files. The scan must + # read it from THERE — the old flat-key read missed it, so the option did nothing (#912). + res_dir, keep, flagged = _run_scan( + tmp_path, {'remove_junk_files': True, 'remove_residual_files': True}) + assert res_dir in flagged # the cover.jpg + .lrc folder is now found + assert keep not in flagged # the audio folder is never touched + + +def test_scan_keeps_residual_folder_when_opt_off(tmp_path): + # Opt-off preserves the conservative default: an image/.lrc folder is left alone. + res_dir, keep, flagged = _run_scan( + tmp_path, {'remove_junk_files': True, 'remove_residual_files': False}) + assert res_dir not in flagged + assert keep not in flagged diff --git a/tests/test_itunes_album_tracks_limit.py b/tests/test_itunes_album_tracks_limit.py new file mode 100644 index 00000000..1fa36e40 --- /dev/null +++ b/tests/test_itunes_album_tracks_limit.py @@ -0,0 +1,46 @@ +"""iTunes album-track fetch must request the full album, not the 50-entity default (#918). + +The iTunes Lookup API returns only 50 related entities unless `limit` is passed (max 200), +so albums >50 tracks were truncated to the first 50 in the download window. get_album_tracks +must pass limit=200. +""" + +from __future__ import annotations + +from core import itunes_client as ic + + +class _Cache: + """Cache stub: always a miss; any write method is a no-op.""" + def get_entity(self, *a, **k): + return None + + def __getattr__(self, _name): + return lambda *a, **k: None + + +_RESULTS = [ + {'wrapperType': 'collection', 'collectionId': 123, 'collectionName': 'Big OST', + 'artistName': 'Composer', 'trackCount': 70, 'artworkUrl100': 'http://x/100x100bb.jpg'}, + {'wrapperType': 'track', 'kind': 'song', 'trackId': 1, 'trackName': 'Track 1', + 'trackNumber': 1, 'discNumber': 1, 'artistName': 'Composer', 'trackTimeMillis': 1000}, +] + + +def test_get_album_tracks_requests_limit_200(monkeypatch): + client = ic.iTunesClient(country='US') + captured = {} + + def fake_lookup(**params): + captured.update(params) + return _RESULTS + + monkeypatch.setattr(client, '_lookup', fake_lookup) + monkeypatch.setattr(ic, 'get_metadata_cache', lambda: _Cache()) + + result = client.get_album_tracks('123') + + assert captured.get('limit') == 200 # #918: full album, not the iTunes 50 default + assert captured.get('entity') == 'song' + assert captured.get('id') == '123' + assert result is not None diff --git a/tests/test_navidrome_reconcile.py b/tests/test_navidrome_reconcile.py new file mode 100644 index 00000000..f1abb842 --- /dev/null +++ b/tests/test_navidrome_reconcile.py @@ -0,0 +1,74 @@ +"""Navidrome reconcile must read the CURRENT playlist via ratingKey (#905). + +NavidromeTrack exposes the Subsonic song id as ``ratingKey`` — it has no ``.id``. +reconcile_playlist used ``t.id``, so the current-track list came back empty, the +add/remove plan saw "nothing is here", and every sync re-added the whole matched +set (playlists doubling). These tests drive the real reconcile_playlist with a +stubbed client and assert the Subsonic params reflect the true delta. +""" + +from __future__ import annotations + +from types import SimpleNamespace + +from core.navidrome_client import NavidromeClient, NavidromeTrack + + +def _track(song_id): + """A NavidromeTrack as get_playlist_tracks / the matcher produce them.""" + return NavidromeTrack({'id': song_id, 'title': f'Song {song_id}'}, client=None) + + +def test_navidrome_track_exposes_ratingkey_not_id(): + # Root cause: the attribute is ratingKey, NOT id. + t = _track('42') + assert t.ratingKey == '42' + assert not hasattr(t, 'id') + + +def _client_with_existing(existing_ids): + """A NavidromeClient whose server playlist already holds `existing_ids`, + capturing the Subsonic params reconcile sends.""" + c = NavidromeClient.__new__(NavidromeClient) + c.ensure_connection = lambda: True + c.get_playlists_by_name = lambda name: [SimpleNamespace(id='PL1')] + c.get_playlist_tracks = lambda pid: [_track(i) for i in existing_ids] + captured = {} + + def _req(method, params): + captured['method'] = method + captured['params'] = params + return {'status': 'ok'} + + c._make_request = _req + return c, captured + + +def test_no_change_resync_is_a_noop(): + # Playlist already == desired → reconcile must add/remove NOTHING. + c, captured = _client_with_existing(['1', '2', '3', '4']) + desired = [_track('1'), _track('2'), _track('3'), _track('4')] + assert c.reconcile_playlist('My Playlist', desired) is True + # plan empty → early return, updatePlaylist never called (no re-add). + assert 'params' not in captured + + +def test_removed_source_track_is_removed_not_everything_readded(): + # warl0ck's exact case: server has 1..5, source now has 1..4 (5 removed). + c, captured = _client_with_existing(['1', '2', '3', '4', '5']) + desired = [_track('1'), _track('2'), _track('3'), _track('4')] + assert c.reconcile_playlist('My Playlist', desired) is True + params = captured['params'] + assert params['playlistId'] == 'PL1' + assert 'songIdToAdd' not in params # the bug: would re-add 1..4 → doubling + assert params['songIndexToRemove'] == [4] # only song '5' (index 4) is removed + + +def test_added_source_track_is_appended_once(): + # Server has 1..3, source now has 1..4 → add only '4'. + c, captured = _client_with_existing(['1', '2', '3']) + desired = [_track('1'), _track('2'), _track('3'), _track('4')] + assert c.reconcile_playlist('My Playlist', desired) is True + params = captured['params'] + assert params['songIdToAdd'] == ['4'] + assert 'songIndexToRemove' not in params diff --git a/tests/test_reorganize_feat_matching.py b/tests/test_reorganize_feat_matching.py new file mode 100644 index 00000000..8d784bbe --- /dev/null +++ b/tests/test_reorganize_feat_matching.py @@ -0,0 +1,66 @@ +"""Reorganize title matcher: featured-artist credits must not block a match (#914). + +iTunes appends "(feat. X)" to track titles while a user's file is often just the +bare title. Before the fix that extra credit dropped the substring ratio below the +match threshold, so a correctly-identified track was reported as "no matching track +in the iTunes tracklist". The credit is metadata, so it's stripped before scoring. +""" + +from __future__ import annotations + +from core.library_reorganize import _find_api_track, _normalize_title + + +# ── normalization ──────────────────────────────────────────────────────────── +def test_feat_paren_stripped_equals_bare(): + assert _normalize_title('The Chase (feat. Big Artist)') == _normalize_title('The Chase') + assert _normalize_title('The Chase (feat. Big Artist)') == 'the chase' + + +def test_feat_variants_all_stripped(): + for v in ('Song (feat. A)', 'Song (ft. A)', 'Song [ft A]', + 'Song (featuring A & B)', 'Song feat. A', 'Song ft. A & B'): + assert _normalize_title(v) == 'song', v + + +def test_feat_strip_preserves_version_differentiator(): + # The remix tag must survive so the hard-reject still distinguishes recordings. + assert _normalize_title('Song (feat. A) - Remix') == 'song remix' + + +def test_bare_feat_word_not_overstripped(): + # "The Feat" (nothing after) and words containing the letters are left alone. + assert _normalize_title('The Feat') == 'the feat' + assert _normalize_title('Defeat') == 'defeat' + assert _normalize_title('Lift Off') == 'lift off' + + +# ── matcher (the #914 failure) ─────────────────────────────────────────────── +def _api(name, tn): + return {'name': name, 'track_number': tn} + + +def test_bare_local_matches_feat_titled_api_track_without_tn(): + # The exact bug: long featured-artist name pushed the ratio below threshold and + # there was no track-number rescue. After stripping feat it's an EXACT match. + api = [_api('The Chase (feat. Somebody Very Famous)', 9)] + assert _find_api_track(api, 'The Chase', None) is api[0] + + +def test_bare_local_matches_feat_titled_api_track_with_tn(): + api = [_api('Money Trees (feat. Jay Rock)', 6), _api('Poetic Justice (feat. Drake)', 7)] + assert _find_api_track(api, 'Money Trees', 6) is api[0] + assert _find_api_track(api, 'Poetic Justice', 7) is api[1] + + +def test_feat_strip_does_not_cross_match_different_songs(): + # Stripping feat must not collapse two genuinely different titles together. + api = [_api('The Chase (feat. X)', 1), _api('The Race (feat. Y)', 2)] + assert _find_api_track(api, 'The Race', None) is api[1] + assert _find_api_track(api, 'Nonexistent Song', None) is None + + +def test_remix_still_hard_rejected_even_with_feat(): + # A bare "Song" must NOT match an API "Song (feat. X) [Remix]" — different recording. + api = [_api('Song (feat. X) - Remix', 1)] + assert _find_api_track(api, 'Song', 1) is None diff --git a/tests/test_sync_dedupe.py b/tests/test_sync_dedupe.py new file mode 100644 index 00000000..386322ea --- /dev/null +++ b/tests/test_sync_dedupe.py @@ -0,0 +1,40 @@ +"""Sync must push a ratingKey-deduped track list (#905). + +The dedup was computed but the dispatch sent the raw matched list, so a track +matched by more than one source entry got pushed multiple times — and on +reconcile/replace that re-added it every sync (playlists doubling). This guards +the pure dedup helper the dispatch now uses. +""" + +from __future__ import annotations + +from services.sync_service import _dedupe_by_rating_key + + +class _T: + def __init__(self, rk): + self.ratingKey = rk + + +def test_removes_duplicate_rating_keys_preserving_order(): + a, b, c = _T('1'), _T('2'), _T('3') + dup_b = _T('2') + out = _dedupe_by_rating_key([a, b, c, dup_b, a]) + assert [t.ratingKey for t in out] == ['1', '2', '3'] + assert out[1] is b # first-seen object kept, not the later duplicate + + +def test_no_duplicates_is_identity(): + items = [_T('1'), _T('2'), _T('3')] + assert _dedupe_by_rating_key(items) == items + + +def test_drops_tracks_without_rating_key(): + class _NoKey: + pass + out = _dedupe_by_rating_key([_T('1'), _NoKey(), _T('2')]) + assert [t.ratingKey for t in out] == ['1', '2'] + + +def test_empty(): + assert _dedupe_by_rating_key([]) == [] diff --git a/tests/test_tracks_year_migration.py b/tests/test_tracks_year_migration.py new file mode 100644 index 00000000..8aedff45 --- /dev/null +++ b/tests/test_tracks_year_migration.py @@ -0,0 +1,104 @@ +"""Migration + regression test for the per-track ``year`` column (#910). + +Full Refresh INSERTs a per-track ``year`` (read from file tags), but the column was +only ever present in that live INSERT — never in CREATE TABLE and never in a +migration. So on every DB (old *and* current) the Full Refresh track insert +hard-failed with ``table tracks has no column named year``, importing 0 tracks +while artists/albums imported fine. + +The repair backstop (``_ensure_core_media_schema_columns``) must ALTER ``year`` +onto any tracks table that lacks it. Additive + nullable; nothing reads it except +the writer, so backfilling it on every existing DB is safe. +""" + +from __future__ import annotations + +import sqlite3 + +import pytest + +from database.music_database import MusicDatabase + + +def _track_cols(cur): + cur.execute("PRAGMA table_info(tracks)") + return {c[1] for c in cur.fetchall()} + + +# An upgraded tracks table that has every Full Refresh insert column EXCEPT year +# (mirrors the #910 reporter exactly: "64 columns, only year absent"). Text ids +# match the live schema after the id->TEXT migration. +_OLD_TRACKS = ( + "CREATE TABLE tracks (id TEXT PRIMARY KEY, album_id TEXT, artist_id TEXT, " + "title TEXT, track_number INTEGER, disc_number INTEGER, duration INTEGER, " + "file_path TEXT, bitrate INTEGER, server_source TEXT, " + "created_at TIMESTAMP, updated_at TIMESTAMP)" +) + +# The exact Full Refresh insert from web_server.py (the statement that failed). +_FULL_REFRESH_INSERT = ( + "INSERT OR IGNORE INTO tracks (id, album_id, artist_id, title, track_number, disc_number, " + "duration, file_path, bitrate, year, server_source, created_at, updated_at) " + "VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, 'soulsync', CURRENT_TIMESTAMP, CURRENT_TIMESTAMP)" +) +_ROW = ('t1::soulsync', 'a1', 'ar1', 'Song', 1, 1, 200000, '/music/song.flac', 1000, 2009) + + +def test_fresh_db_has_year_column(tmp_path): + db = MusicDatabase(str(tmp_path / "m.db")) + cur = db._get_connection().cursor() + assert 'year' in _track_cols(cur) + + +def test_year_column_is_nullable(tmp_path): + db = MusicDatabase(str(tmp_path / "m.db")) + cur = db._get_connection().cursor() + cur.execute("PRAGMA table_info(tracks)") + info = {c[1]: c for c in cur.fetchall()} # name -> (cid, name, type, notnull, dflt, pk) + assert info['year'][3] == 0 # nullable -> safe to add to a populated table + + +def test_migration_is_idempotent(tmp_path): + db = MusicDatabase(str(tmp_path / "m.db")) + cur = db._get_connection().cursor() + before = _track_cols(cur) + # Re-running must not raise (the PRAGMA guard skips the existing column). + db._ensure_core_media_schema_columns(cur) + db._ensure_core_media_schema_columns(cur) + assert _track_cols(cur) == before + assert 'year' in _track_cols(cur) + + +def test_migration_adds_year_to_old_tracks_table(tmp_path): + conn = sqlite3.connect(str(tmp_path / "old.db")) + conn.execute(_OLD_TRACKS) + conn.commit() + cur = conn.cursor() + assert 'year' not in _track_cols(cur) + + db = MusicDatabase(str(tmp_path / "scratch.db")) + db._ensure_core_media_schema_columns(cur) + conn.commit() + + assert 'year' in _track_cols(cur) + + +def test_full_refresh_insert_fails_before_repair_and_succeeds_after(tmp_path): + """Regression for #910: the real Full Refresh track insert hard-fails on a + year-less tracks table, then succeeds once the repair has added the column.""" + conn = sqlite3.connect(str(tmp_path / "old.db")) + conn.execute(_OLD_TRACKS) + conn.commit() + cur = conn.cursor() + + # Before the fix: the live insert blows up exactly as the issue reports. + with pytest.raises(sqlite3.OperationalError, match="no column named year"): + cur.execute(_FULL_REFRESH_INSERT, _ROW) + + # Apply the repair backstop, then the same insert must succeed and persist year. + db = MusicDatabase(str(tmp_path / "scratch.db")) + db._ensure_core_media_schema_columns(cur) + cur.execute(_FULL_REFRESH_INSERT, _ROW) + conn.commit() + cur.execute("SELECT year FROM tracks WHERE id = 't1::soulsync'") + assert cur.fetchone()[0] == 2009 diff --git a/web_server.py b/web_server.py index eaa6bb47..a282de9a 100644 --- a/web_server.py +++ b/web_server.py @@ -40,7 +40,7 @@ logger = setup_logging(_log_level, _log_path) # App version — single source of truth for backup metadata, system-info, update check, etc. # Semver: MAJOR.MINOR.PATCH. Bump at each dev→main release. -_SOULSYNC_BASE_VERSION = "2.7.6" +_SOULSYNC_BASE_VERSION = "2.7.7" def _build_version_string(): """Append short commit hash to version when available (e.g. 2.35+abc1234).""" @@ -14993,7 +14993,12 @@ def parse_youtube_playlist(url): 'no_warnings': True, 'extract_flat': 'in_playlist', # Only extract basic info, no individual video metadata 'skip_download': True, # Don't download, just extract IDs and basic info - 'lazy_playlist': False, # Force full playlist resolution (prevents ~100 entry cap) + 'lazy_playlist': False, # Force full playlist resolution + # #908 / yt-dlp #16943: a YouTube-side regression caps the webpage-based playlist + # path at the first ~100-item page (Liked Music came back as only 104). Skipping the + # webpage and paging via the InnerTube API directly gets far more — verified live + # 100 -> 200+ on a large playlist. Remove once the upstream fix (PR #16948) ships. + 'extractor_args': {'youtubetab': {'skip': ['webpage']}}, } ydl_opts.update(_youtube_cookie_opts()) diff --git a/webui/static/helper.js b/webui/static/helper.js index 10173057..98f77c85 100644 --- a/webui/static/helper.js +++ b/webui/static/helper.js @@ -3404,17 +3404,22 @@ function closeHelperSearch() { const WHATS_NEW = { // Convention: keep only the CURRENT release here, plus a single brief // "Earlier versions" summary entry. Don't accumulate old per-version blocks. - '2.7.6': [ - { date: 'June 2026 — 2.7.6 release' }, - { title: 'Export playlists to ListenBrainz (#903)', desc: 'soulsync already pulls playlists IN — now it can push one OUT. every mirrored-playlist card gets a 📤 export button: download a standard .jspf file, or sync the playlist straight to your ListenBrainz account. each track is matched to its musicbrainz recording id (cache → your library → file tag → live musicbrainz), with live "matching N/M" status on the card. re-syncing updates the SAME LB playlist in place instead of duplicating it.', page: 'sync' }, - { title: 'YouTube Liked Music sync (#902)', desc: 'you can now sync your youtube music "Liked Music" playlist (music.youtube.com/playlist?list=LM). it\'s private, so it needs auth — added a "paste cookies.txt" option in Settings → YouTube so server/docker installs (or browsers yt-dlp can\'t read, like Zen) can supply their login from anywhere.', page: 'settings' }, - { title: 'Deep Scan won\'t relocate your library (#904)', desc: 'a standalone Deep Scan moves unrecognized files into Staging. if the DB was empty/out of sync with disk, it treated your ENTIRE library as unrecognized and relocated all of it. now a guard refuses the move when the unrecognized share is implausibly large, leaves files in place, and warns — plus a "Transfer is my permanent library — never move files out" toggle.', page: 'settings' }, - { title: 'Dashboard performance', desc: 'a pass at "soulsync works my GPU hard just sitting there". the sidebar sweep animates transform not left, particle glows are cached sprites, blur radii + redundant/invisible card shadows are trimmed, and low-power machines auto-drop to performance mode. the effects all stay — they\'re just cheaper to draw.', page: 'dashboard' }, - { title: 'File-import manual matches stick (#901)', desc: 'a manual match on a file-imported playlist track is no longer forgotten on re-sync — the tracks now carry a stable id (existing ones backfilled once), so your pick survives.', page: 'sync' }, - { title: 'Manual match heals a stale Plex key', desc: 'a Find & Add match whose stored Plex ratingKey went stale is now re-resolved against a live Plex search instead of silently breaking on the next sync.', page: 'sync' }, - { title: 'Multi-disc albums', desc: 'a track now files into the Disc folder that matches its own disc tag (no more disc-2 tracks landing in Disc 1), and a track is never written disc-less.', page: 'downloads' }, - { title: 'Auto-download track numbers', desc: 'a track auto-grabbed from the playlist pipeline / wishlist / watchlist now gets its real in-album position instead of being tagged 1/1.', page: 'downloads' }, - { title: 'Earlier versions', desc: '2.7.5 was a fix-heavy cycle — matching & artwork accuracy, the HiFi preview mess (#895), M3U import (#893), ignore-list management (#897), per-playlist file naming. 2.7.4 added re-identify; 2.7.3 the Quality Upgrade Finder + ignore-list (#874); 2.7.2 playlist-folder mirroring + M3U export; 2.7.1 download verification + a review queue (#852); 2.7.0 made multi-user real.' }, + '2.7.7': [ + { date: 'June 2026 — 2.7.7 release' }, + { title: 'Downloads tag + path like Reorganize (#915)', desc: 'adding/redownloading music used to backfill missing album data from Spotify ONLY — so an iTunes/Deezer-primary user kept a "lean" context, the path dropped the $year and the date defaulted to YYYY-01-01 until you ran a reorganize. now post-processing AND redownload pull the full album from your PRIMARY metadata source (the same place reorganize/enrich read), so the year, real release date and album type are right the first time.', page: 'downloads' }, + { title: 'Listening-driven recommendations — foundation (#913)', desc: 'the start of "discover based on what you actually listen to". during the watchlist scan, soulsync now ranks artists you\'d love but don\'t own — seeded from your top-played artists, scored by consensus (similar to MANY of your favorites), play weight and similarity — and builds a candidate track list. generated + stored now; the discover row + synced playlist come next.', page: 'discover' }, + { title: 'Jellyfin stops indexing half-written tracks', desc: 'multi-disc tracks landing with "no disc" in jellyfin was a write race — a cross-filesystem move wrote the file to its final path incrementally and jellyfin caught it mid-write. final placement is now atomic (temp sibling + atomic rename), so a watcher only ever sees the COMPLETE file.', page: 'downloads' }, + { title: 'Navidrome playlists doubling (#905)', desc: 'every resync re-added the whole playlist (a 4-song list grew to 12) — reconcile read the server\'s current tracks via a missing attribute, so it always thought the playlist was empty. fixed, plus a deduped push.', page: 'sync' }, + { title: 'YouTube playlists capped at ~100 (#908)', desc: 'a yt-dlp/youtube regression truncated big playlists (Liked Music came back as 104). worked around to page past it (~200) until the upstream fix lands.', page: 'sync' }, + { title: 'Album redownload grabbed the wrong edition (#911)', desc: 'redownload did a fresh search instead of using the album\'s matched source id, so a 66-track OST could come back as a 19-track single. now uses the canonical matched source.', page: 'library' }, + { title: 'iTunes albums over 50 tracks truncated (#918)', desc: 'the iTunes lookup defaulted to 50 entities, cutting big albums off in the download window. now requests the full album.', page: 'library' }, + { title: 'Enhanced view showed multi-disc tracks as missing (#916)', desc: 'owned disc-2+ tracks (stored as disc 1) no longer flag as "missing" — matched by title like reorganize.', page: 'library' }, + { title: 'Reorganize vs "(feat. X)" (#914)', desc: 'a bare local title now matches an iTunes track titled "Song (feat. Artist)" instead of being reported not-in-tracklist.', page: 'library' }, + { title: '"I have this" dropped the year (#917)', desc: 'it rebuilt a yearless path and copied into a NEW folder; now reuses the album\'s existing folder.', page: 'library' }, + { title: 'Full Refresh imported 0 tracks (#910)', desc: 'every track insert failed on a missing year column; added it + a migration so older DBs self-heal.', page: 'settings' }, + { title: 'YouTube discovery "Unknown Artist" (#909)', desc: 'when youtube hands back only a title, the matched artist now backfills the column instead of leaving "Unknown Artist".', page: 'sync' }, + { title: 'Empty Folder Cleaner toggle did nothing (#912)', desc: 'the "also remove image/sidecar-only folders" option read the wrong config key; now honored.', page: 'settings' }, + { title: 'Earlier versions', desc: '2.7.6 went the OTHER way with playlists — exporting them TO listenbrainz (#903) — plus youtube liked-music sync (#902), a deep-scan data-loss guard (#904), and dashboard perf. 2.7.5 was matching & artwork accuracy + M3U import; 2.7.4 re-identify; 2.7.3 the Quality Upgrade Finder; 2.7.2 playlist-folder mirroring; 2.7.1 download verification; 2.7.0 made multi-user real.' }, ], }; @@ -3445,45 +3450,39 @@ const WHATS_NEW = { // usage_note?: 'optional hint shown at the bottom' } const VERSION_MODAL_SECTIONS = [ { - title: "Export playlists to ListenBrainz (#903)", - description: "soulsync already pulls playlists IN from everywhere — now it can push one back OUT.", + title: "Downloads tag + path like Reorganize (#915)", + description: "the headline fix — adding or redownloading music now gets the year, release date and album type right the FIRST time, instead of needing a manual reorganize after.", features: [ - "📤 export button on every mirrored-playlist card — download a standard .jspf file, or sync the playlist straight onto your ListenBrainz account", - "each track is matched to its musicbrainz recording id via a cheapest-first waterfall — cache → your library → the file's own tag → a live musicbrainz lookup — and the result is cached so the same song never costs twice", - "live \"matching N/M · X matched\" status on the card, and re-syncing updates the SAME LB playlist in place instead of making duplicates", - ], - usage_note: "find it on the 📤 button when you hover a mirrored playlist card. tracks that can't be resolved to a musicbrainz id are skipped (LB requires them) and counted, so you see the coverage.", - }, - { - title: "YouTube Liked Music sync (#902)", - description: "sync your youtube music \"Liked Music\" playlist (music.youtube.com/playlist?list=LM).", - features: [ - "it's a private playlist, so it needs auth — the existing browser-cookie option only works when the browser is on the same machine as soulsync", - "added a \"paste cookies.txt\" option in Settings → YouTube so server/docker installs (or browsers yt-dlp can't read, like Zen) can supply their login from anywhere", + "post-processing used to backfill missing album data from Spotify ONLY — so an iTunes/Deezer-primary user kept a \"lean\" context, the path dropped the $year, and the release date defaulted to YYYY-01-01", + "now post-processing AND single-track redownload pull the full album from your PRIMARY metadata source — the exact same place reorganize / manual enrich read", + "so the $year folder, real release date and album type land correctly on the first pass (iTunes + Deezer)", ], }, { - title: "Deep Scan won't relocate your library (#904)", - description: "a standalone Deep Scan moves unrecognized files into Staging — which went very wrong when the DB was out of sync with disk.", + title: "Listening-driven recommendations — foundation (#913)", + description: "the start of \"discover based on what you actually listen to.\"", features: [ - "if the DB was empty/desynced, a scan treated your ENTIRE library as unrecognized and relocated all of it (one user lost ~1,500 tracks into Staging)", - "now a guard refuses the move when the unrecognized share is implausibly large (the desync signature), leaves everything in place, and warns instead", - "new \"Transfer is my permanent library — never move files out\" toggle for people whose Transfer folder IS their live library", + "during the watchlist scan, soulsync ranks artists you'd love but don't own — seeded from your top-played artists", + "scored by consensus (similar to MANY of your favorites), play weight, and similarity strength — not just \"appears in a list\"", + "generated + stored now; the discover row + a synced playlist come next", ], }, { - title: "Dashboard performance + fixes", - description: "lighter dashboard, and a handful of matching/import fixes.", + title: "A big batch of fixes", + description: "a fix-heavy cycle — playlists, downloads, multi-disc and the enhanced view.", features: [ - "dashboard — the sidebar sweep animates transform not left, particle glows are cached sprites, blur/shadow excess is trimmed, and low-power machines auto-drop to performance mode (the effects all stay, just cheaper to draw)", - "#901 — a manual match on a file-imported playlist track now survives re-sync (stable ids, existing ones backfilled)", - "a Find & Add match whose stored Plex key went stale is re-resolved against a live Plex search instead of breaking", - "multi-disc albums file each track in the Disc folder matching its tag (no disc-less / wrong-disc tracks); auto-grabbed tracks get their real in-album number instead of 1/1", + "jellyfin \"no disc\" tracks — a cross-filesystem move wrote the file to its final path incrementally and jellyfin caught it mid-write; final placement is now atomic, so a watcher only ever sees the complete file", + "#905 — navidrome playlists doubling every resync (reconcile thought the playlist was empty); fixed + a deduped push", + "#908 — youtube playlists capped at ~100 (a yt-dlp/youtube regression); worked around to ~200 until upstream lands", + "#911 — album redownload grabbed the wrong edition (fresh search vs the matched source id); now uses the canonical source", + "#918 — iTunes albums over 50 tracks were truncated in the download window; now requests the full album", + "#916 — the enhanced view flagged multi-disc tracks as missing; now matched by title like reorganize", + "#914 #917 #910 #909 #912 — reorganize vs \"(feat. X)\", \"I have this\" dropping the year, Full Refresh importing 0 tracks, youtube \"Unknown Artist\", and the Empty Folder Cleaner toggle that did nothing", ], }, { - title: "Earlier in 2.7.5", - description: "a fix-heavy cycle — matching & artwork accuracy plus a few quality-of-life features.", + title: "Earlier in 2.7.6 / 2.7.5", + description: "2.7.6 went the OTHER way with playlists — exporting them TO listenbrainz (#903) — plus youtube liked-music sync (#902), a deep-scan data-loss guard (#904), and dashboard perf. before that, 2.7.5 was a fix-heavy cycle — matching & artwork accuracy plus a few quality-of-life features.", features: [ "special-edition cover art (a \"Gustave Edition\" uses its OWN cover), deezer track numbers, and the \"The\" duplicate fix", "HiFi 30-second previews disguised as full songs are caught and rejected (#895)", diff --git a/webui/static/library.js b/webui/static/library.js index c654d211..ca9cfa35 100644 --- a/webui/static/library.js +++ b/webui/static/library.js @@ -3964,23 +3964,60 @@ async function ensureEnhancedAlbumCanonicalTracks(album) { } } +// Loose title key for owned<->canonical matching. Mirrors the Reorganize +// matcher (core.library_reorganize._normalize_title), which already maps these +// same multi-disc tracks correctly: drop only the featured-artist credit, then +// treat every other separator (brackets, dashes, slashes, punctuation) as +// whitespace — so "X (Main Theme)" and "X - Main Theme" collapse to the same key +// while "(feat. Y)" is removed. Keeping bracket CONTENT (not deleting it) is what +// makes editions line up. +function _normTitleForMatch(value) { + return String(value || '') + .toLowerCase() + .replace(/[([]\s*(?:feat|ft|featuring)\b[^)\]]*[)\]]/g, ' ') // (feat. Y) / [ft Y] + .replace(/\s+(?:feat|ft|featuring)\b\.?\s.*$/g, ' ') // trailing feat. Y … + .replace(/[^a-z0-9]+/g, ' ') // all other separators -> space (KEEP content) + .trim(); +} + function _deriveEnhancedMissingTracks(album, canonicalTracks) { - const occupiedSlots = new Set(); - (album.tracks || []).forEach(track => { + // #916: multi-disc albums store disc_number = 1 for EVERY track in the library + // (the scanner doesn't split discs), so a strict disc:track slot match flags every + // canonical disc-2+ track as missing. Match each canonical track to an owned track + // by slot FIRST, then fall back to title — consuming each owned track once so genuine + // missings and duplicate titles still count correctly. + const owned = (album.tracks || []).map(t => ({ + slot: _trackSlotKey(t), + title: _normTitleForMatch(t.title || t.name), + used: false, + })); + const slotIndex = new Map(); + owned.forEach((o, i) => { if (o.slot !== '1:0' && !slotIndex.has(o.slot)) slotIndex.set(o.slot, i); }); + + const missing = []; + (canonicalTracks || []).forEach(track => { const key = _trackSlotKey(track); - if (key !== '1:0') occupiedSlots.add(key); - }); - return (canonicalTracks || []) - .map(track => ({ + const normalized = _normalizeExpectedMissingTrack(track, album); + if (key === '1:0' || !normalized._hasActionableContext) return; + + // 1) exact disc:track slot + const si = slotIndex.get(key); + if (si != null && !owned[si].used) { owned[si].used = true; return; } + + // 2) fallback: title vs any UNUSED owned track (handles the disc_number=1 collapse) + const nt = _normTitleForMatch(track.name || track.title); + if (nt) { + const m = owned.find(o => !o.used && o.title === nt); + if (m) { m.used = true; return; } + } + + missing.push({ ...track, name: track.name || track.title, duration_ms: track.duration_ms || track.duration || 0, - })) - .filter(track => { - const key = _trackSlotKey(track); - const normalized = _normalizeExpectedMissingTrack(track, album); - return key !== '1:0' && normalized._hasActionableContext && !occupiedSlots.has(key); }); + }); + return missing; } function _getEnhancedAlbumTrackRows(album) { @@ -5416,43 +5453,80 @@ function _pollRedownloadProgress(taskId, overlay) { async function redownloadLibraryAlbum(album, artistName, btn) { const albumName = album.title || ''; const spotifyAlbumId = album.spotify_album_id || ''; + const itunesAlbumId = album.itunes_album_id || ''; + // #911 — the album's CANONICAL source (the same one the Enhanced view tags + displays it as) + // wins. Redownload must pull THAT exact edition, not a fresh search that can resolve to a + // different one (issue: matched the 66-track 'Original Soundtrack Collection', a search got + // the 19-track 'Volume 1'). _getEnhancedAlbumCanonicalSource is the single source of truth + // for which source identifies this album, across spotify/deezer/itunes/musicbrainz/… + const canonical = _getEnhancedAlbumCanonicalSource(album); - if (!spotifyAlbumId && !albumName) { + if (!canonical && !spotifyAlbumId && !itunesAlbumId && !albumName) { showToast('No album ID or name available for redownload', 'warning'); return; } + // Fetch a specific album edition by its source id (the Spotify/iTunes endpoints both return + // a Spotify-shaped payload, so downstream handling is identical). + const fetchAlbumBySource = (source, id, name, artist) => { + const params = new URLSearchParams({ name: name || albumName, artist: artist || artistName || '' }); + const base = source === 'itunes' ? '/api/itunes/album/' : '/api/spotify/album/'; + return fetch(`${base}${encodeURIComponent(id)}?${params}`); + }; + const origText = btn ? btn.innerHTML : ''; try { if (btn) { btn.disabled = true; btn.textContent = 'Loading...'; } - let response; - if (spotifyAlbumId) { - const params = new URLSearchParams({ name: albumName, artist: artistName || '' }); - response = await fetch(`/api/spotify/album/${encodeURIComponent(spotifyAlbumId)}?${params}`); - } + let albumData = null; - if (!response || !response.ok) { - const query = `${artistName || ''} ${albumName}`.trim(); - const searchResp = await fetch('/api/enhanced-search', { - method: 'POST', - headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ query }) - }); - if (!searchResp.ok) throw new Error('Album search failed'); - const searchData = await searchResp.json(); - const found = searchData.spotify_albums?.[0] || searchData.itunes_albums?.[0]; - if (!found || !found.id) { - showToast(`Could not find "${albumName}" by ${artistName || 'unknown'}`, 'warning'); - return; + // 1) Primary: the canonical tagged source (any source), via the SAME + // /api/album//tracks endpoint the Enhanced view uses for its canonical tracklist — + // so a redownload is always the album the user is actually looking at. + if (canonical) { + const params = new URLSearchParams({ name: albumName, artist: artistName || '', source: canonical.source }); + const r = await fetch(`/api/album/${encodeURIComponent(canonical.id)}/tracks?${params}`); + if (r.ok) { + const data = await r.json(); + if (data && data.success && Array.isArray(data.tracks) && data.tracks.length) { + albumData = { ...data.album, tracks: data.tracks }; // normalize to {…, tracks:[]} + } } - const params = new URLSearchParams({ name: found.name || albumName, artist: found.artist || artistName || '' }); - response = await fetch(`/api/spotify/album/${encodeURIComponent(found.id)}?${params}`); } - if (!response.ok) throw new Error(`Failed to load album: ${response.status}`); + // 2) Fallback: the stored spotify/iTunes id, then a last-resort search. + if (!albumData) { + let response; + if (spotifyAlbumId) { + response = await fetchAlbumBySource('spotify', spotifyAlbumId); + } else if (itunesAlbumId) { + response = await fetchAlbumBySource('itunes', itunesAlbumId); + } + + if (!response || !response.ok) { + const query = `${artistName || ''} ${albumName}`.trim(); + const searchResp = await fetch('/api/enhanced-search', { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ query }) + }); + if (!searchResp.ok) throw new Error('Album search failed'); + const searchData = await searchResp.json(); + const spotHit = searchData.spotify_albums?.[0]; + const found = spotHit || searchData.itunes_albums?.[0]; + if (!found || !found.id) { + showToast(`Could not find "${albumName}" by ${artistName || 'unknown'}`, 'warning'); + return; + } + // Fetch from the MATCHING source endpoint — the old fallback always hit Spotify, + // which is wrong for an iTunes search hit. + response = await fetchAlbumBySource(spotHit ? 'spotify' : 'itunes', found.id, found.name, found.artist); + } + + if (!response.ok) throw new Error(`Failed to load album: ${response.status}`); + albumData = await response.json(); + } - const albumData = await response.json(); if (!albumData || !albumData.tracks || albumData.tracks.length === 0) { showToast(`No tracks found for "${albumName}"`, 'warning'); return;