diff --git a/.github/workflows/docker-publish.yml b/.github/workflows/docker-publish.yml index 9ffbb51a..a216db03 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.2)' + description: 'Version tag (e.g. 2.7.3)' required: true - default: '2.7.2' + default: '2.7.3' jobs: build-and-push: diff --git a/core/imports/context.py b/core/imports/context.py index f718f014..024c3f36 100644 --- a/core/imports/context.py +++ b/core/imports/context.py @@ -319,7 +319,11 @@ def build_import_album_info( (album_info or {}).get("track_number") or track_info.get("track_number") or original_search.get("track_number") - or 1 + # "Track 01" bug: default to 0 (the codebase's "unknown" sentinel, + # same as total_tracks below), NOT 1. A fabricated 1 looks + # authoritative and blocks the pipeline's downstream recovery + # (embedded file tag / resolve chain); 0 lets it fall through. + or 0 ) disc_number = ( (album_info or {}).get("disc_number") diff --git a/core/imports/pipeline.py b/core/imports/pipeline.py index b6a077a9..ad6a0801 100644 --- a/core/imports/pipeline.py +++ b/core/imports/pipeline.py @@ -629,9 +629,17 @@ def post_process_matched_download(context_key, context, file_path, runtime, meta # See ``core/imports/track_number.py`` for the resolution # chain — pure function, unit-tested in isolation, single # place to fix the rule. - from core.imports.track_number import resolve_track_number + from core.imports.track_number import resolve_track_number, read_embedded_track_number track_info_for_resolve = context.get('track_info') if isinstance(context, dict) else None - track_number = resolve_track_number(album_info, track_info_for_resolve, file_path) + # "Track 01" bug: a single Deezer track is matched via an endpoint + # that omits track_position, so the context never carried the real + # number. The downloaded file itself does (deemix/source wrote it), + # so read it as a source between metadata and the filename guess. + embedded_track_number = read_embedded_track_number(file_path) + track_number = resolve_track_number( + album_info, track_info_for_resolve, file_path, + embedded_track_number=embedded_track_number, + ) logger.debug( "Final track_number processing: source=%s album_info=%s resolved=%s", album_info.get('source', 'unknown'), diff --git a/core/imports/quarantine.py b/core/imports/quarantine.py index 0e9af594..7dd43be6 100644 --- a/core/imports/quarantine.py +++ b/core/imports/quarantine.py @@ -153,6 +153,83 @@ def get_quarantined_source_keys(quarantine_dir: str) -> set: return keys +def quarantine_group_key( + expected_artist: Any, expected_track: Any, context: Any = None +) -> Optional[str]: + """Grouping key for "the same intended download target". + + #876: when several sources are downloaded for one wishlist/queue + track they each fail verification and land in quarantine as separate + entries. They are *alternatives for the same song*, so they should + group together — and once the user accepts one, the rest are + redundant failed attempts at a song they now own. + + The key identifies the *intended* target — what SoulSync was trying to + fetch — NOT the downloaded file's own tags. That matters: the file's + metadata is frequently *wrong* (that's why it failed acoustid / + integrity), whereas the target is fixed and identical across every + alternative for one song (they're all Soulseek uploads of the *same* + source track), so grouping by it is an exact relationship, not a fuzzy + metadata guess. + + Prefers a stable target-track id from the sidecar `context.track_info` + when present — isrc, then source id, then uri — since those are exact + and constant across siblings. Falls back to the normalized + artist|track name only for legacy/thin sidecars that carry no context. + Keys are kind-prefixed so an id-based key never collides with a + name-based one. + + Returns ``None`` when nothing identifies the target (no usable id and + both name fields empty). Callers treat a ``None`` key as "its own + singleton group" — ungroupable entries must never collapse together. + """ + ti = {} + if isinstance(context, dict): + maybe_ti = context.get("track_info") + if isinstance(maybe_ti, dict): + ti = maybe_ti + isrc = str(ti.get("isrc") or "").strip().lower() + if isrc: + return f"isrc:{isrc}" + tid = str(ti.get("id") or "").strip() + if tid: + return f"id:{tid}" + uri = str(ti.get("uri") or "").strip() + if uri: + return f"uri:{uri}" + artist = " ".join(str(expected_artist or "").split()).lower() + track = " ".join(str(expected_track or "").split()).lower() + if not artist and not track: + return None + return f"nm:{artist}|{track}" + + +def find_quarantine_siblings(quarantine_dir: str, entry_id: str) -> List[str]: + """Other entry ids that share ``entry_id``'s intended-target group key. + + Returns the ids of every *other* quarantine entry whose + `expected_artist`/`expected_track` normalize to the same key as + ``entry_id`` (see :func:`quarantine_group_key`). Excludes ``entry_id`` + itself. Returns ``[]`` when the entry is missing, has an ungroupable + (``None``) key, or has no siblings. Never raises. + """ + if not entry_id: + return [] + entries = list_quarantine_entries(quarantine_dir) + target_key = None + for e in entries: + if e.get("id") == entry_id: + target_key = e.get("group_key") + break + if target_key is None: + return [] + return [ + e["id"] + for e in entries + if e.get("id") != entry_id and e.get("group_key") == target_key + ] + + def list_quarantine_entries(quarantine_dir: str) -> List[Dict[str, Any]]: """Enumerate quarantined files paired with their sidecars. @@ -213,6 +290,11 @@ def list_quarantine_entries(quarantine_dir: str) -> List[Dict[str, Any]]: "reason": sidecar.get("quarantine_reason", "Unknown reason"), "expected_track": sidecar.get("expected_track", ""), "expected_artist": sidecar.get("expected_artist", ""), + "group_key": quarantine_group_key( + sidecar.get("expected_artist", ""), + sidecar.get("expected_track", ""), + ctx, + ), "timestamp": sidecar.get("timestamp", ""), "size_bytes": size_bytes, "has_full_context": isinstance(sidecar.get("context"), dict), diff --git a/core/imports/track_number.py b/core/imports/track_number.py index 43c98029..63b53b03 100644 --- a/core/imports/track_number.py +++ b/core/imports/track_number.py @@ -65,17 +65,64 @@ def _coerce_spotify_data(track_info: Any) -> dict: return {} +def read_embedded_track_number(file_path: str) -> Optional[int]: + """Read the track position from a downloaded audio file's own tags. + + Streaming sources (Deezer/deemix, Qobuz, Tidal) and most Soulseek + uploads write the correct album position into the file itself. That + tag is authoritative for the *source's* idea of the track's place on + its album — more reliable than a filename guess — so the resolver + consults it before falling back to the filename / default-1 floor. + + Issue #874-adjacent / "Track 01" bug: a single Deezer track is matched + via Deezer's ``/search/track`` endpoint, which omits ``track_position`` + (core/deezer_client.py), so the metadata context never carried the + real number — but the downloaded file *does* (deemix wrote it). This + recovers it with no network call. + + Returns a positive int, or None when the file has no usable + tracknumber tag / can't be read. Never raises. Handles the common + ``"2/15"`` (number/total) form by taking the leading number. + """ + if not file_path: + return None + try: + from mutagen import File as MutagenFile + audio = MutagenFile(file_path, easy=True) + if audio is None: + return None + raw = audio.get('tracknumber') + if isinstance(raw, list): + raw = raw[0] if raw else None + if raw is None: + return None + # "2/15" -> "2"; bare "2" -> "2". + text = str(raw).split('/', 1)[0].strip() + return _coerce_positive(text) + except Exception: + return None + + def resolve_track_number( album_info: Any, track_info: Any, file_path: str, + embedded_track_number: Any = None, ) -> Optional[int]: """Walk the resolution chain and return the first valid positive int found, or None when every source is missing / unusable. - Caller is responsible for the final default-1 floor — leaving - that out of this function so tests can pin "everything missing + Order: album_info -> track_info -> nested spotify_data -> filename -> + ``embedded_track_number`` (the source-written file tag, when the caller + supplies it). Caller is responsible for the final default-1 floor — + leaving that out of this function so tests can pin "everything missing returns None" separate from the floor behaviour. + + ``embedded_track_number`` is passed in (not read here) so this stays a + pure function — the file I/O lives in :func:`read_embedded_track_number`. + It is consulted **last**, only when every other source came up empty, so + it can never override a value the pre-fix resolver already produced — it + only fills the gap that would otherwise hit the default-1 floor. """ album_info = album_info if isinstance(album_info, dict) else {} track_info = track_info if isinstance(track_info, dict) else {} @@ -96,10 +143,19 @@ def resolve_track_number( # default-1 floor is the single source of that fallback — # otherwise this resolver would silently fill 1 and the # downstream floor logic would have no effect. - if not file_path: - return None - try: - from_filename = extract_explicit_track_number(file_path) - except Exception: - from_filename = None - return _coerce_positive(from_filename) + if file_path: + try: + from_filename = extract_explicit_track_number(file_path) + except Exception: + from_filename = None + ff = _coerce_positive(from_filename) + if ff is not None: + return ff + + # Embedded source-written file tag is consulted LAST — only when every + # other source (metadata + the ripped-album "NN - Title" filename) came + # up empty. This is deliberate: it can ONLY fill the gap that would + # otherwise hit the caller's default-1 floor, so it never overrides a + # value the pre-fix resolver would have used. A correctly-named file + # with a stale/wrong embedded tag is therefore never regressed. + return _coerce_positive(embedded_track_number) diff --git a/core/text/title_match.py b/core/text/title_match.py index 85e1c10b..36818fcd 100644 --- a/core/text/title_match.py +++ b/core/text/title_match.py @@ -201,9 +201,27 @@ def numeric_tokens_differ(title_a: str, title_b: str) -> bool: return _digit_tokens(title_a) != _digit_tokens(title_b) +def base_title_before_dash(title: str) -> str: + """The base title before Spotify's ' - ' version separator. + + Spotify renders versions as 'Calma - Remix' / 'Song - Radio Edit' / + 'Track - Remastered 2019'. Libraries (and the files people actually have) + very often store just the base — 'Calma' — so a literal search for + 'Calma - Remix' finds nothing and the OR-fuzzy fallback then floods on the + common qualifier word ('remix' matches every remix). This returns the base + ('Calma') for a base-title search fallback. Splits on the FIRST ' - ' (the + spaced hyphen is Spotify's separator; a bare hyphen inside a word is left + alone). Returns the title unchanged when there's no separator.""" + if not title: + return title + idx = title.find(' - ') + return title[:idx].strip() if idx > 0 else title + + __all__ = [ "titles_plausibly_same", "strip_redundant_context_qualifiers", "strip_subtitle_qualifiers", "numeric_tokens_differ", + "base_title_before_dash", ] diff --git a/core/tidal_client.py b/core/tidal_client.py index a18ab04f..4b542d3d 100644 --- a/core/tidal_client.py +++ b/core/tidal_client.py @@ -1655,6 +1655,8 @@ class TidalClient: ids: List[str] = [] next_path: Optional[str] = None + consecutive_429 = 0 + MAX_PAGE_RETRIES = 4 while True: if next_path: @@ -1687,6 +1689,28 @@ class TidalClient: logger.warning(f"Tidal collection page request failed: {e}") break + # Rate limited mid-walk → retry the SAME cursor page with backoff + # rather than silently truncating the collection. Without this a 429 + # on page ~5 capped a 513-track favorites list at ~98 (issue #880); + # the regular-playlist paginator already retries 429 the same way. + if resp.status_code == 429: + consecutive_429 += 1 + if consecutive_429 <= MAX_PAGE_RETRIES: + backoff = 5.0 * consecutive_429 # 5s, 10s, 15s, 20s + logger.warning( + f"Tidal collection {expected_type} rate limited (429) — " + f"retry {consecutive_429}/{MAX_PAGE_RETRIES} in {backoff}s " + f"({len(ids)} fetched so far)" + ) + time.sleep(backoff) + continue # next_path/cursor unchanged → re-request same page + logger.error( + f"Tidal collection {expected_type} still rate limited after " + f"{MAX_PAGE_RETRIES} retries — returning {len(ids)} IDs " + f"(PARTIAL: the collection may be larger)" + ) + break + if resp.status_code != 200: # 401/403 = scope/permission issue. Token predates the # `collection.read` scope expansion or the user revoked @@ -1704,6 +1728,8 @@ class TidalClient: ) break + consecutive_429 = 0 # a good page → reset the retry budget + try: data = resp.json() except ValueError as e: diff --git a/core/wishlist/ignore.py b/core/wishlist/ignore.py new file mode 100644 index 00000000..beced2c4 --- /dev/null +++ b/core/wishlist/ignore.py @@ -0,0 +1,162 @@ +"""Wishlist ignore-list — a TTL'd skip-gate for the wishlist (#874). + +When a user removes a track from the wishlist or cancels an in-flight +wishlist download, SoulSync would otherwise re-add it on the next +automatic cycle (watchlist scan, failed-track capture, or the cancel +handler's own re-add), so the same release downloads → fails/cancels → +re-queues forever. The ignore list records the user's "stop +auto-grabbing this" intent, and the wishlist *add* path checks it, +skipping automatic re-adds until the entry ages out. + +It is deliberately softer than the blocklist: + - it **expires** after ``IGNORE_TTL_DAYS`` so the track is re-attempted + again later rather than banned forever, and + - it **never blocks a manual force-download** — only the automatic + re-queue. (Manual downloads don't go through ``add_to_wishlist`` at + all, and an explicit manual *add* both bypasses the gate and clears + any existing ignore for the track.) + +This module is pure decision logic — no database handle and no clock of +its own; the caller passes ``now``. The SQL lives in ``MusicDatabase`` +as a thin wrapper around these helpers, which keeps the TTL / id-matching +rules unit-testable without a database. +""" + +from __future__ import annotations + +from datetime import datetime, timedelta +from typing import Any, Dict, Iterable, List, Optional, Set, Tuple + +IGNORE_TTL_DAYS = 30 + +# Recognised reasons (free-text tolerated; these are the canonical two). +REASON_REMOVED = "removed" +REASON_CANCELLED = "cancelled" + + +def normalize_ignore_id(track_id: Any) -> str: + """Canonical key for a wishlist track id. + + The wishlist stores some ids as a composite ``::`` + (when ``wishlist.allow_duplicate_tracks`` is on). The add-path gate + keys on the bare track id (``spotify_track_data['id']``), so we strip + the ``::album`` suffix here so an ignore recorded from a composite-id + wishlist row still matches the bare-id add attempt, and vice-versa. + Returns ``''`` for falsy/blank input. + """ + s = str(track_id or "").strip() + if not s: + return "" + return s.split("::", 1)[0] + + +def _parse_ts(value: Any) -> Optional[datetime]: + """Best-effort parse of a sqlite TIMESTAMP / ISO string / datetime.""" + if isinstance(value, datetime): + return value + if not value: + return None + s = str(value).strip() + for fmt in ( + "%Y-%m-%d %H:%M:%S", + "%Y-%m-%dT%H:%M:%S", + "%Y-%m-%d %H:%M:%S.%f", + "%Y-%m-%dT%H:%M:%S.%f", + ): + try: + return datetime.strptime(s, fmt) + except ValueError: + continue + try: + return datetime.fromisoformat(s) + except ValueError: + return None + + +def is_expired(created_at: Any, now: datetime, ttl_days: int = IGNORE_TTL_DAYS) -> bool: + """True when an ignore entry created at ``created_at`` has aged past TTL. + + Fail-SAFE in the gate's favour: an unparseable/blank timestamp is + treated as **expired** (returns True) so a corrupt row can never wedge + a track out of the wishlist permanently — the worst case is the + ignore silently lapses and the track becomes eligible again. + """ + created = _parse_ts(created_at) + if created is None: + return True + return now >= created + timedelta(days=ttl_days) + + +def active_ignored_ids( + rows: Iterable[Dict[str, Any]], now: datetime, ttl_days: int = IGNORE_TTL_DAYS +) -> Set[str]: + """Set of normalized track ids whose ignore entry is still within TTL.""" + out: Set[str] = set() + for row in rows or []: + tid = normalize_ignore_id(row.get("track_id")) + if tid and not is_expired(row.get("created_at"), now, ttl_days): + out.add(tid) + return out + + +def is_ignored( + rows: Iterable[Dict[str, Any]], track_id: Any, now: datetime, ttl_days: int = IGNORE_TTL_DAYS +) -> bool: + """Whether ``track_id`` matches an in-TTL entry among ``rows``.""" + key = normalize_ignore_id(track_id) + if not key: + return False + return key in active_ignored_ids(rows, now, ttl_days) + + +def extract_display(data: Any) -> Tuple[str, str]: + """Pull a (track_name, artist_name) pair from a Spotify-shaped dict. + + Tolerates ``artists`` as a list of dicts or bare strings, and missing + fields. Used to give ignore-list rows a human label for the UI. + Returns ``('', '')`` when nothing usable is present. + """ + if not isinstance(data, dict): + return "", "" + name = str(data.get("name") or "").strip() + artist = "" + artists = data.get("artists") or [] + if isinstance(artists, list) and artists: + first = artists[0] + if isinstance(first, dict): + artist = str(first.get("name") or "").strip() + else: + artist = str(first or "").strip() + return name, artist + + +def ignore_wishlist_track( + database: Any, + profile_id: int, + track_id: Any, + reason: str, + spotify_data: Any = None, +) -> bool: + """Record an ignore entry for a wishlist track. Best-effort; never raises. + + Copies the track's display name/artist from ``spotify_data`` when + provided (callers should capture it BEFORE removing the wishlist row, + since the row may be gone afterwards); otherwise leaves them blank. + Returns True when an entry was written. + """ + key = normalize_ignore_id(track_id) + if not key or database is None: + return False + name, artist = extract_display(spotify_data or {}) + try: + return bool( + database.add_to_wishlist_ignore( + key, + track_name=name, + artist_name=artist, + reason=reason or REASON_REMOVED, + profile_id=profile_id, + ) + ) + except Exception: + return False diff --git a/core/wishlist/routes.py b/core/wishlist/routes.py index c05a626c..a467676c 100644 --- a/core/wishlist/routes.py +++ b/core/wishlist/routes.py @@ -310,12 +310,30 @@ def remove_track_from_wishlist( if not spotify_track_id: return {"success": False, "error": "No spotify_track_id provided"}, 400 - success = get_wishlist_service().remove_track_from_wishlist( + service = get_wishlist_service() + _db = getattr(service, "database", None) + # #874: capture the track's display info BEFORE removal (the row is + # gone afterwards) so the ignore-list entry carries a human label. + _ignore_data = None + try: + if _db is not None: + _ignore_data = _db.get_wishlist_spotify_data( + spotify_track_id, profile_id=runtime.profile_id) + except Exception: + _ignore_data = None + + success = service.remove_track_from_wishlist( spotify_track_id, profile_id=runtime.profile_id, ) if success: + # #874: a user-initiated remove means "stop auto-requeuing this". + # Record a TTL'd ignore so the watchlist/auto-processor doesn't + # re-add it. Best-effort — never fails the remove. + from core.wishlist.ignore import ignore_wishlist_track, REASON_REMOVED + ignore_wishlist_track(_db, runtime.profile_id, + spotify_track_id, REASON_REMOVED, spotify_data=_ignore_data) runtime.logger.info("Successfully removed track from wishlist: %s", spotify_track_id) return {"success": True, "message": "Track removed from wishlist"}, 200 @@ -358,13 +376,20 @@ def remove_album_from_wishlist( if matched: spotify_track_id = track.get("track_id") or track.get("spotify_track_id") or track.get("id") if spotify_track_id: - tracks_to_remove.append(spotify_track_id) + # Keep the loaded spotify_data alongside the id so the #874 + # ignore entry can be labelled without a second DB read. + tracks_to_remove.append((spotify_track_id, spotify_data)) + from core.wishlist.ignore import ignore_wishlist_track, REASON_REMOVED + _db = getattr(wishlist_service, "database", None) removed_count = 0 album_remove_pid = runtime.profile_id - for spotify_track_id in tracks_to_remove: + for spotify_track_id, track_spotify_data in tracks_to_remove: if wishlist_service.remove_track_from_wishlist(spotify_track_id, profile_id=album_remove_pid): removed_count += 1 + # #874: user removed the whole album → ignore each track. + ignore_wishlist_track(_db, album_remove_pid, + spotify_track_id, REASON_REMOVED, spotify_data=track_spotify_data) if removed_count > 0: runtime.logger.info("Successfully removed %s tracks from album %s", removed_count, album_id) @@ -390,11 +415,22 @@ def remove_batch_from_wishlist( if not spotify_track_ids or not isinstance(spotify_track_ids, list): return {"success": False, "error": "Missing or invalid spotify_track_ids"}, 400 + from core.wishlist.ignore import ignore_wishlist_track, REASON_REMOVED + service = get_wishlist_service() + _db = getattr(service, "database", None) removed = 0 pid = runtime.profile_id for track_id in spotify_track_ids: - if get_wishlist_service().remove_track_from_wishlist(track_id, profile_id=pid): + # Capture label before the row is deleted (#874). + _data = None + try: + if _db is not None: + _data = _db.get_wishlist_spotify_data(track_id, profile_id=pid) + except Exception: + _data = None + if service.remove_track_from_wishlist(track_id, profile_id=pid): removed += 1 + ignore_wishlist_track(_db, pid, track_id, REASON_REMOVED, spotify_data=_data) runtime.logger.info("Batch removed %s track(s) from wishlist", removed) return { diff --git a/core/wishlist/service.py b/core/wishlist/service.py index 6dc8c859..df3be19a 100644 --- a/core/wishlist/service.py +++ b/core/wishlist/service.py @@ -214,7 +214,11 @@ class WishlistService: "preview_url": track_data.get("preview_url") if isinstance(track_data, dict) else None, "external_urls": track_data.get("external_urls", {}) if isinstance(track_data, dict) else {}, "popularity": track_data.get("popularity", 0) if isinstance(track_data, dict) else 0, - "track_number": track_data.get("track_number", 1) if isinstance(track_data, dict) else 1, + # "Track 01" bug: 0 = "unknown position", NOT a fabricated 1. + # A fake 1 looks authoritative and blocks the import + # pipeline's track-number recovery; 0 lets it recover the + # real position (file tag / source lookup) before the floor. + "track_number": track_data.get("track_number", 0) if isinstance(track_data, dict) else 0, "disc_number": track_data.get("disc_number", 1) if isinstance(track_data, dict) else 1, } diff --git a/database/music_database.py b/database/music_database.py index 0d06a6db..30d2c2b3 100644 --- a/database/music_database.py +++ b/database/music_database.py @@ -344,7 +344,29 @@ class MusicDatabase: source_info TEXT -- JSON of source context (playlist name, album info, etc.) ) """) - + + # Wishlist ignore-list (#874): a TTL'd skip-gate. When a user + # removes a track from the wishlist or cancels an in-flight + # wishlist download, the track is recorded here so the automatic + # re-add paths (watchlist scan, failed-track capture, cancel + # re-add) skip it until the entry ages out (see core.wishlist. + # ignore.IGNORE_TTL_DAYS). Softer than `blocklist`: it expires + # and never blocks a manual force-download. Keyed on the bare + # track id; unique per (profile, track) so re-ignoring refreshes. + cursor.execute(""" + CREATE TABLE IF NOT EXISTS wishlist_ignore ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + profile_id INTEGER NOT NULL DEFAULT 1, + track_id TEXT NOT NULL, + track_name TEXT DEFAULT '', + artist_name TEXT DEFAULT '', + reason TEXT DEFAULT 'removed', -- 'removed' | 'cancelled' + created_at TIMESTAMP DEFAULT CURRENT_TIMESTAMP, + UNIQUE(profile_id, track_id) + ) + """) + cursor.execute("CREATE INDEX IF NOT EXISTS idx_wishlist_ignore_profile ON wishlist_ignore (profile_id, track_id)") + # Watchlist table for storing artists to monitor for new releases cursor.execute(""" CREATE TABLE IF NOT EXISTS watchlist_artists ( @@ -6887,11 +6909,26 @@ class MusicDatabase: # STRATEGY 1: Try basic SQL LIKE search first (fastest) basic_results = self._search_tracks_basic(cursor, title, artist, limit, server_source, rank_artist) - + if basic_results: logger.debug(f"Basic search found {len(basic_results)} results") return basic_results + # STRATEGY 1b: Spotify renders versions as "Title - Qualifier" + # ("Calma - Remix") but libraries usually store just the base + # ("Calma"), so the literal search misses. Retry on the base title + # BEFORE the OR-fuzzy fallback (which would flood on the common + # qualifier word — every "... remix" matches "remix"). #: Calma - Remix + if title: + from core.text.title_match import base_title_before_dash + base_title = base_title_before_dash(title) + if base_title and base_title != title: + base_results = self._search_tracks_basic( + cursor, base_title, artist, limit, server_source, rank_artist) + if base_results: + logger.debug("Base-title search matched '%s' via '%s'", title, base_title) + return base_results + # STRATEGY 2: Broader fuzzy search - splits into individual words with OR matching fuzzy_results = self._search_tracks_fuzzy_fallback(cursor, title, artist, limit, server_source) if fuzzy_results: @@ -6920,6 +6957,17 @@ class MusicDatabase: if basic_rows: return [dict(r) for r in basic_rows] + # Base-title fallback for Spotify "Title - Qualifier" forms (see + # search_tracks STRATEGY 1b) before the OR-fuzzy flood. + if title: + from core.text.title_match import base_title_before_dash + base_title = base_title_before_dash(title) + if base_title and base_title != title: + base_rows = self._search_tracks_basic_rows( + cursor, base_title, artist, limit, server_source) + if base_rows: + return [dict(r) for r in base_rows] + fuzzy_rows = self._search_tracks_fuzzy_rows(cursor, title, artist, limit, server_source) return [dict(r) for r in fuzzy_rows] except Exception as e: @@ -8825,6 +8873,21 @@ class MusicDatabase: _blocked[0], _blocked[1]) return False + # Ignore-list guard (#874): a user who removed or cancelled this + # track asked us to stop AUTO-requeuing it — every automatic + # re-add funnels through here, so one check covers them all. + # A *manual* add is explicit user intent → bypass the gate AND + # clear any stale ignore so it sticks. Fail-open: any error here + # must never block a legitimate wishlist add. + try: + if source_type == 'manual': + self.remove_from_wishlist_ignore(track_id, profile_id=profile_id) + elif self.is_track_ignored(track_id, profile_id=profile_id): + logger.info("Skipping wishlist add — track is on the ignore-list (#874): %s", track_id) + return False + except Exception as _ignore_exc: + logger.debug("Wishlist ignore-list check skipped (fail-open): %s", _ignore_exc) + from core.library import manual_library_match as _mlm if _mlm.get_match_for_track(self, profile_id, spotify_track_data): logger.info( @@ -8969,7 +9032,147 @@ class MusicDatabase: except Exception as e: logger.error(f"Error removing track from wishlist: {e}") return False - + + # ── Wishlist ignore-list (#874) ────────────────────────────────────── + # A TTL'd skip-gate consulted by add_to_wishlist so user-removed / + # user-cancelled tracks are not auto-re-queued. All methods fail-open + # (an error here must never block a legitimate wishlist add). + + def add_to_wishlist_ignore(self, track_id: str, track_name: str = "", + artist_name: str = "", reason: str = "removed", + profile_id: int = 1) -> bool: + """Record (or refresh) an ignore entry for a wishlist track id. + + Keyed on the bare track id; UNIQUE(profile_id, track_id) means a + repeat ignore replaces the row and so refreshes its TTL clock. + """ + from core.wishlist.ignore import normalize_ignore_id + key = normalize_ignore_id(track_id) + if not key: + return False + try: + with self._get_connection() as conn: + cursor = conn.cursor() + cursor.execute(""" + INSERT OR REPLACE INTO wishlist_ignore + (profile_id, track_id, track_name, artist_name, reason, created_at) + VALUES (?, ?, ?, ?, ?, CURRENT_TIMESTAMP) + """, (profile_id, key, track_name or "", artist_name or "", reason or "removed")) + conn.commit() + logger.info("Added track to wishlist ignore-list (%s): '%s' [%s]", + reason or "removed", track_name or key, key) + return True + except Exception as e: + logger.error("Error adding to wishlist ignore-list: %s", e) + return False + + def is_track_ignored(self, track_id: str, profile_id: int = 1, + ttl_days: Optional[int] = None) -> bool: + """Whether ``track_id`` has a non-expired ignore entry. Fail-open False.""" + from core.wishlist.ignore import normalize_ignore_id, is_expired, IGNORE_TTL_DAYS + ttl = IGNORE_TTL_DAYS if ttl_days is None else ttl_days + key = normalize_ignore_id(track_id) + if not key: + return False + try: + with self._get_connection() as conn: + cursor = conn.cursor() + cursor.execute( + "SELECT created_at FROM wishlist_ignore WHERE profile_id = ? AND track_id = ?", + (profile_id, key)) + row = cursor.fetchone() + if not row: + return False + return not is_expired(row["created_at"], datetime.now(), ttl) + except Exception as e: + logger.debug("is_track_ignored failed open: %s", e) + return False + + def remove_from_wishlist_ignore(self, track_id: str, profile_id: int = 1) -> bool: + """Un-ignore a track (manual override / UI action). Returns True if a row went.""" + from core.wishlist.ignore import normalize_ignore_id + key = normalize_ignore_id(track_id) + if not key: + return False + try: + with self._get_connection() as conn: + cursor = conn.cursor() + cursor.execute( + "DELETE FROM wishlist_ignore WHERE profile_id = ? AND track_id = ?", + (profile_id, key)) + conn.commit() + return cursor.rowcount > 0 + except Exception as e: + logger.error("Error removing from wishlist ignore-list: %s", e) + return False + + def get_wishlist_ignore(self, profile_id: int = 1, + ttl_days: Optional[int] = None) -> List[Dict[str, Any]]: + """Active (non-expired) ignore entries, newest first; purges lapsed rows.""" + from core.wishlist.ignore import is_expired, IGNORE_TTL_DAYS + ttl = IGNORE_TTL_DAYS if ttl_days is None else ttl_days + try: + with self._get_connection() as conn: + cursor = conn.cursor() + cursor.execute( + "SELECT track_id, track_name, artist_name, reason, created_at " + "FROM wishlist_ignore WHERE profile_id = ? ORDER BY created_at DESC", + (profile_id,)) + rows = cursor.fetchall() + now = datetime.now() + active, expired_ids = [], [] + for r in rows: + if is_expired(r["created_at"], now, ttl): + expired_ids.append(r["track_id"]) + else: + active.append({ + "track_id": r["track_id"], + "track_name": r["track_name"] or "", + "artist_name": r["artist_name"] or "", + "reason": r["reason"] or "removed", + "created_at": r["created_at"], + }) + # Opportunistic housekeeping so the table can't grow unbounded. + if expired_ids: + cursor.executemany( + "DELETE FROM wishlist_ignore WHERE profile_id = ? AND track_id = ?", + [(profile_id, tid) for tid in expired_ids]) + conn.commit() + return active + except Exception as e: + logger.error("Error reading wishlist ignore-list: %s", e) + return [] + + def clear_wishlist_ignore(self, profile_id: int = 1) -> int: + """Drop every ignore entry for a profile. Returns rows removed.""" + try: + with self._get_connection() as conn: + cursor = conn.cursor() + cursor.execute("DELETE FROM wishlist_ignore WHERE profile_id = ?", (profile_id,)) + conn.commit() + return cursor.rowcount + except Exception as e: + logger.error("Error clearing wishlist ignore-list: %s", e) + return 0 + + def get_wishlist_spotify_data(self, track_id: str, profile_id: int = 1) -> Dict[str, Any]: + """Parsed ``spotify_data`` for a wishlist row, or {}. Used to label an + ignore entry with the track's name/artist before the row is removed.""" + try: + with self._get_connection() as conn: + cursor = conn.cursor() + cursor.execute( + "SELECT spotify_data FROM wishlist_tracks WHERE spotify_track_id = ? AND profile_id = ?", + (track_id, profile_id)) + row = cursor.fetchone() + if not row or not row["spotify_data"]: + return {} + data = json.loads(row["spotify_data"]) + return data if isinstance(data, dict) else {} + except Exception as e: + logger.debug("get_wishlist_spotify_data failed: %s", e) + return {} + def get_wishlist_tracks(self, limit: Optional[int] = None, profile_id: int = 1, offset: int = 0, category: Optional[str] = None) -> List[Dict[str, Any]]: """Get tracks in the wishlist for the given profile, ordered by date added diff --git a/tests/imports/test_quarantine_management.py b/tests/imports/test_quarantine_management.py index 3615ca6a..7234c497 100644 --- a/tests/imports/test_quarantine_management.py +++ b/tests/imports/test_quarantine_management.py @@ -5,9 +5,11 @@ from core.imports.quarantine import ( approve_quarantine_entry, delete_quarantine_entry, entry_id_from_quarantined_filename, + find_quarantine_siblings, get_quarantine_entry_stream_info, get_quarantined_source_keys, list_quarantine_entries, + quarantine_group_key, recover_to_staging, serialize_quarantine_context, ) @@ -71,18 +73,20 @@ def test_serialize_round_trips_through_json(): # list_quarantine_entries # ────────────────────────────────────────────────────────────────────── -def _write_entry(quarantine_dir, entry_id, original_name, *, with_context=False, trigger="integrity", reason="boom", file_bytes=b"X" * 100): +def _write_entry(quarantine_dir, entry_id, original_name, *, with_context=False, trigger="integrity", reason="boom", file_bytes=b"X" * 100, expected_track="Track", expected_artist="Artist", context=None): qfile = quarantine_dir / f"{entry_id}_{original_name}.quarantined" qfile.write_bytes(file_bytes) sidecar = { "original_filename": original_name, "quarantine_reason": reason, - "expected_track": "Track", - "expected_artist": "Artist", + "expected_track": expected_track, + "expected_artist": expected_artist, "timestamp": "2026-05-14T12:00:00", "trigger": trigger, } - if with_context: + if context is not None: + sidecar["context"] = context + elif with_context: sidecar["context"] = {"track_info": {"name": "Track"}, "context_key": entry_id} sidecar_path = quarantine_dir / f"{entry_id}_{os.path.splitext(original_name)[0]}.json" sidecar_path.write_text(json.dumps(sidecar)) @@ -454,3 +458,95 @@ def test_move_with_retry_returns_false_on_missing_source(tmp_path): # attempts=1 keeps the test fast (no retry sleeps) assert _move_with_retry(str(tmp_path / "nope.flac"), str(tmp_path / "dst.flac"), attempts=1, delay=0) is False + + +# ────────────────────────────────────────────────────────────────────── +# #876: grouping alternatives for one song — quarantine_group_key / +# find_quarantine_siblings, and the group_key field on list entries. +# ────────────────────────────────────────────────────────────────────── + +def test_group_key_prefers_isrc_over_everything(): + ctx = {"track_info": {"isrc": "USRC12345678", "id": "spid", "uri": "spotify:track:x"}} + assert quarantine_group_key("Artist", "Track", ctx) == "isrc:usrc12345678" + + +def test_group_key_falls_back_to_source_id_then_uri(): + assert quarantine_group_key("A", "T", {"track_info": {"id": "abc123"}}) == "id:abc123" + assert quarantine_group_key("A", "T", {"track_info": {"uri": "spotify:track:z"}}) == "uri:spotify:track:z" + + +def test_group_key_falls_back_to_normalized_name_without_context(): + # Trivial case/whitespace differences still collapse to one key. + k1 = quarantine_group_key("Kendrick Lamar", "DNA.") + k2 = quarantine_group_key("kendrick lamar", "dna.") + assert k1 == k2 == "nm:kendrick lamar|dna." + + +def test_group_key_none_when_nothing_identifies_target(): + assert quarantine_group_key("", "", {}) is None + assert quarantine_group_key("", "", None) is None + + +def test_list_entries_carry_group_key(tmp_path): + _write_entry(tmp_path, "20260514_120000", "a.flac", + context={"track_info": {"isrc": "USABC1234567"}}) + entries = list_quarantine_entries(str(tmp_path)) + assert entries[0]["group_key"] == "isrc:usabc1234567" + + +def test_find_siblings_returns_same_target_attempts(tmp_path): + # Two failed source attempts at the SAME target track (same isrc) + an + # unrelated entry. Siblings of #2 = {#1}, never the unrelated one. + same = {"track_info": {"isrc": "USAAA0000001"}} + other = {"track_info": {"isrc": "USZZZ9999999"}} + q1, _ = _write_entry(tmp_path, "20260514_120000", "src1.flac", context=same) + q2, _ = _write_entry(tmp_path, "20260514_120001", "src2.flac", context=same) + _write_entry(tmp_path, "20260514_120002", "diff.flac", context=other) + + id1 = entry_id_from_quarantined_filename(q1.name) + id2 = entry_id_from_quarantined_filename(q2.name) + assert find_quarantine_siblings(str(tmp_path), id2) == [id1] + + +def test_find_siblings_groups_by_intended_target_not_file_tags(tmp_path): + # Same intended target (isrc) even though the bad files differ — that's + # the whole point: the file metadata is wrong, the target is constant. + same = {"track_info": {"isrc": "USAAA0000001", "name": "Whatever"}} + q1, _ = _write_entry(tmp_path, "20260514_120000", "garbage_wrong_song.flac", context=same) + q2, _ = _write_entry(tmp_path, "20260514_120001", "another_bad_rip.flac", context=same) + id1 = entry_id_from_quarantined_filename(q1.name) + id2 = entry_id_from_quarantined_filename(q2.name) + assert find_quarantine_siblings(str(tmp_path), id1) == [id2] + + +def test_find_siblings_empty_for_ungroupable_entry(tmp_path): + # No id and blank expected fields -> None key -> never grouped. + q1, _ = _write_entry(tmp_path, "20260514_120000", "orphan.flac", + expected_track="", expected_artist="") + id1 = entry_id_from_quarantined_filename(q1.name) + assert find_quarantine_siblings(str(tmp_path), id1) == [] + + +def test_find_siblings_empty_for_missing_entry(tmp_path): + _write_entry(tmp_path, "20260514_120000", "a.flac") + assert find_quarantine_siblings(str(tmp_path), "does_not_exist") == [] + + +def test_siblings_must_be_captured_before_accepted_entry_leaves_quarantine(tmp_path): + # Regression for the approve-endpoint ordering: approving RESTORES (moves) + # the accepted entry out of quarantine, after which an id-based sibling + # lookup for that id can't resolve its group_key and returns []. The + # endpoint therefore captures siblings BEFORE approving. This pins that + # invariant: lookup-before == sibling found, lookup-after == empty. + same = {"track_info": {"isrc": "USAAA0000001"}} + q1, _ = _write_entry(tmp_path, "20260514_120000", "a.flac", context=same) + q2, _ = _write_entry(tmp_path, "20260514_120001", "b.flac", context=same) + id1 = entry_id_from_quarantined_filename(q1.name) + id2 = entry_id_from_quarantined_filename(q2.name) + + captured = find_quarantine_siblings(str(tmp_path), id1) # while id1 present + assert captured == [id2] + + delete_quarantine_entry(str(tmp_path), id1) # simulate approve restoring it + + assert find_quarantine_siblings(str(tmp_path), id1) == [] # too late now diff --git a/tests/imports/test_track_number_resolver.py b/tests/imports/test_track_number_resolver.py index 088a4bcb..381d5f86 100644 --- a/tests/imports/test_track_number_resolver.py +++ b/tests/imports/test_track_number_resolver.py @@ -14,7 +14,7 @@ from __future__ import annotations from unittest.mock import patch -from core.imports.track_number import resolve_track_number +from core.imports.track_number import read_embedded_track_number, resolve_track_number # --------------------------------------------------------------------------- @@ -226,3 +226,113 @@ def test_non_dict_track_info_treated_as_empty(): """Defensive — non-dict track_info won't crash the resolver.""" result = resolve_track_number({}, 'not a dict', '/dir/file.flac') assert result is None + + +# --------------------------------------------------------------------------- +# "Track 01" bug (Deezer single tracks): embedded file-tag source. +# --------------------------------------------------------------------------- + + +def test_embedded_tag_used_when_metadata_missing(): + """The Deezer single-track case: context carried no position (search + endpoint omits track_position), but the downloaded file did. With the + metadata-context de-poisoned to 0/None, the embedded tag is consulted + BEFORE the filename guess.""" + result = resolve_track_number( + album_info={'track_number': 0}, # de-poisoned "unknown" + track_info={'track_number': 0}, + file_path='/dl/Lenny Kravitz - Fly Away.flac', # no number prefix + embedded_track_number=2, + ) + assert result == 2 + + +def test_real_metadata_still_beats_embedded_tag(): + """No regression for album downloads: an authoritative album_info + position wins over the file tag (they normally agree, but the + context is the source of truth when present).""" + result = resolve_track_number( + album_info={'track_number': 5}, + track_info={}, + file_path='/dl/file.flac', + embedded_track_number=2, + ) + assert result == 5 + + +def test_filename_beats_embedded_tag_no_regression(): + """SAFETY: embedded tag is consulted LAST, so a correctly-named ripped + file ('05 - Song') with a stale/wrong embedded tag is NOT regressed — + the filename still wins, exactly as before the fix.""" + result = resolve_track_number( + album_info={}, + track_info={}, + file_path='/dl/09 - Mislabelled.flac', + embedded_track_number=2, # wrong/stale tag must NOT win + ) + assert result == 9 + + +def test_embedded_only_fills_the_floor_gap(): + """When metadata AND filename are both empty (the Deezer case), the + embedded tag fills what would otherwise be the blind default-1 floor.""" + result = resolve_track_number( + album_info={}, track_info={}, + file_path='/dl/Lenny Kravitz - Fly Away.flac', # no number prefix + embedded_track_number=2, + ) + assert result == 2 + + +def test_resolver_without_embedded_arg_is_backwards_compatible(): + """The new parameter is optional — existing 3-arg callers unaffected.""" + assert resolve_track_number({}, {'track_number': 4}, '/x.flac') == 4 + + +# read_embedded_track_number — mutagen-backed file tag reader. + +def _fake_mutagen(tag_value): + """Patch target factory: returns a callable standing in for + ``mutagen.File`` that yields an object whose .get('tracknumber') + returns tag_value (mimicking easy=True list-valued tags).""" + class _Audio(dict): + pass + def _factory(path, easy=False): + a = _Audio() + if tag_value is not None: + a['tracknumber'] = tag_value + return a + return _factory + + +def test_read_embedded_handles_number_slash_total(): + with patch('mutagen.File', _fake_mutagen(['2/15'])): + assert read_embedded_track_number('/x.flac') == 2 + + +def test_read_embedded_handles_bare_number(): + with patch('mutagen.File', _fake_mutagen(['7'])): + assert read_embedded_track_number('/x.flac') == 7 + + +def test_read_embedded_none_when_tag_absent(): + with patch('mutagen.File', _fake_mutagen(None)): + assert read_embedded_track_number('/x.flac') is None + + +def test_read_embedded_none_when_file_unreadable(): + def _factory(path, easy=False): + return None + with patch('mutagen.File', _factory): + assert read_embedded_track_number('/x.flac') is None + + +def test_read_embedded_never_raises(): + def _boom(path, easy=False): + raise RuntimeError('corrupt') + with patch('mutagen.File', _boom): + assert read_embedded_track_number('/x.flac') is None + + +def test_read_embedded_empty_path_returns_none(): + assert read_embedded_track_number('') is None diff --git a/tests/metadata/test_metadata_discography.py b/tests/metadata/test_metadata_discography.py index 65ac0624..a5109dbb 100644 --- a/tests/metadata/test_metadata_discography.py +++ b/tests/metadata/test_metadata_discography.py @@ -731,3 +731,26 @@ def test_get_artist_discography_prefers_source_specific_artist_ids(monkeypatch): ) ] assert deezer.album_calls == [] + + +def test_get_artist_detail_discography_splits_eps_from_singles(monkeypatch): + """#877: get_artist_detail_discography must put EPs in their OWN bucket (not + lumped into singles). The Download Discography modal now reads this split, so + its EPs filter has cards to act on and stays in sync with Artist Detail.""" + spotify = _FakeSourceClient( + album_results=[ + _album("a1", "An Album", "2024-01-01", album_type="album"), + _album("e1", "An EP", "2024-02-01", album_type="ep"), + _album("s1", "A Single", "2024-03-01", album_type="single"), + ] + ) + clients = {"deezer": _FakeSourceClient(), "spotify": spotify, "itunes": _FakeSourceClient()} + monkeypatch.setattr(metadata_registry, "get_primary_source", lambda spotify_client_factory=None: "deezer") + monkeypatch.setattr(metadata_registry, "get_source_priority", lambda primary: [primary, "spotify", "itunes"]) + monkeypatch.setattr(metadata_registry, "get_client_for_source", lambda source, **kwargs: clients.get(source)) + + result = metadata_discography.get_artist_detail_discography("artist-1", "Artist One", MetadataLookupOptions()) + + assert [r["id"] for r in result["albums"]] == ["a1"] + assert [r["id"] for r in result["eps"]] == ["e1"] + assert [r["id"] for r in result["singles"]] == ["s1"] diff --git a/tests/test_base_title_search_fallback.py b/tests/test_base_title_search_fallback.py new file mode 100644 index 00000000..f9c2b936 --- /dev/null +++ b/tests/test_base_title_search_fallback.py @@ -0,0 +1,88 @@ +"""Find & Add: Spotify "Title - Qualifier" must find the base-titled library track. + +wolf's report: Spotify shows "Calma - Remix", Find & Add searches that literal +string, the library stores the track as just "Calma" (only the duration marks it +as the remix) → the literal search misses and the OR-fuzzy fallback floods 20 +unrelated "... remix" hits. Dropping "- Remix" (searching "Calma") finds it. + +Fix: search_tracks retries on the base title (before Spotify's " - " separator) +before the OR-fuzzy flood. +""" + +from __future__ import annotations + +import pytest + +from core.text.title_match import base_title_before_dash +from database.music_database import MusicDatabase + + +# --- pure helper ----------------------------------------------------------- + +def test_base_title_before_dash_strips_spotify_version_suffix(): + assert base_title_before_dash('Calma - Remix') == 'Calma' + assert base_title_before_dash('Closer - Radio Edit') == 'Closer' + assert base_title_before_dash('Crocodile Rock - Remastered 2014') == 'Crocodile Rock' + + +def test_base_title_before_dash_leaves_plain_titles_alone(): + assert base_title_before_dash('Tom Sawyer') == 'Tom Sawyer' + assert base_title_before_dash('21st Century Schizoid Man') == '21st Century Schizoid Man' + assert base_title_before_dash('Up-Tight') == 'Up-Tight' # bare hyphen, not a separator + assert base_title_before_dash('') == '' + + +def test_base_title_before_dash_splits_first_separator_only(): + assert base_title_before_dash('A - B - C') == 'A' + + +# --- integration: the real search path ------------------------------------- + +@pytest.fixture +def db(tmp_path): + return MusicDatabase(str(tmp_path / "music.db")) + + +def _insert(db, tid, title, artist_id, artist_name): + with db._get_connection() as conn: + conn.execute("INSERT OR IGNORE INTO artists (id, name) VALUES (?, ?)", (artist_id, artist_name)) + conn.execute("INSERT OR IGNORE INTO albums (id, title, artist_id) VALUES (?, ?, ?)", + (artist_id, "Alb", artist_id)) + conn.execute( + "INSERT INTO tracks (id, album_id, artist_id, title, track_number, duration, file_path) " + "VALUES (?, ?, ?, ?, 1, 238, ?)", + (tid, artist_id, artist_id, title, f"/m/{tid}.flac"), + ) + conn.commit() + + +def test_spotify_dash_remix_finds_base_titled_track(db): + # Library stores the remix as just "Calma" (the wolf case). + _insert(db, 1, "Calma", 1, "Pedro Capó") + results = db.search_tracks(title="Calma - Remix", rank_artist="Pedro Capó") + titles = [t.title for t in results] + assert "Calma" in titles, "base-title fallback should find 'Calma' for 'Calma - Remix'" + + +def test_spotify_dash_remix_finds_parenthesized_remix(db): + # …and still matches when the library DID label it "(Remix)". + _insert(db, 1, "Calma (Remix)", 1, "Pedro Capó") + results = db.search_tracks(title="Calma - Remix", rank_artist="Pedro Capó") + assert any("Calma" in t.title for t in results) + + +def test_plain_title_unaffected_uses_basic_search(db): + _insert(db, 1, "Tom Sawyer", 1, "Rush") + results = db.search_tracks(title="Tom Sawyer") + assert [t.title for t in results] == ["Tom Sawyer"] + + +def test_dash_query_does_not_flood_when_base_matches(db): + # The base-title retry must short-circuit BEFORE the OR-fuzzy flood, so an + # unrelated "... Remix" track doesn't drown the real one. + _insert(db, 1, "Calma", 1, "Pedro Capó") + _insert(db, 2, "Some Other Song (KAIZ Remix)", 2, "Someone Else") + results = db.search_tracks(title="Calma - Remix", rank_artist="Pedro Capó") + titles = [t.title for t in results] + assert "Calma" in titles + assert "Some Other Song (KAIZ Remix)" not in titles diff --git a/tests/test_settings_redaction.py b/tests/test_settings_redaction.py index 3aea7a2a..a27e4d25 100644 --- a/tests/test_settings_redaction.py +++ b/tests/test_settings_redaction.py @@ -24,6 +24,23 @@ def _cm(config_data): return cm +# ── #879: the GET /api/settings handler calls config_manager.redacted_config(). +# If that method is ever renamed/removed the endpoint 500s, the web UI treats the +# error body as settings, blanks the form to defaults, and autosaves over the +# user's real config. Pin the method's presence on the class so that can't ship. + +def test_redacted_config_is_a_method_on_the_class(): + assert callable(getattr(ConfigManager, 'redacted_config', None)), ( + "GET /api/settings depends on ConfigManager.redacted_config(); removing or " + "renaming it 500s the settings endpoint and the UI then wipes the config (#879)") + + +def test_redacted_config_callable_on_instance_returns_dict(): + cm = _cm({'spotify': {'client_secret': 'REAL'}}) + out = cm.redacted_config() + assert isinstance(out, dict) and out.get('spotify', {}).get('client_secret') == S + + # ── redacted_config: secrets out, everything else intact ──────────────────── def test_configured_secrets_are_masked(): diff --git a/tests/test_tidal_collection_tracks.py b/tests/test_tidal_collection_tracks.py index b821a93c..88695e99 100644 --- a/tests/test_tidal_collection_tracks.py +++ b/tests/test_tidal_collection_tracks.py @@ -252,6 +252,55 @@ class TestIterCollectionTrackIds: assert ids == [] + def test_429_mid_walk_retries_and_completes(self): + """Regression for #880: a 429 mid-pagination must RETRY the same + cursor page (with backoff), not truncate the collection. Before the + fix a transient 429 on page ~5 capped a 513-track favorites list at + ~98 (the log showed `status=429` then `Retrieved 98/100`).""" + client = _make_authed_client() + # page1 (200) → 429 (transient) → page2 (200, end of chain) + responses = iter([ + _FakeResp(200, _PAGE_ONE), + _FakeResp(429, text=""), # rate limited — retry, don't truncate + _FakeResp(200, _PAGE_TWO), + ]) + with patch.object(client, '_ensure_valid_token', return_value=True), \ + patch('core.tidal_client.requests.get', side_effect=lambda *a, **kw: next(responses)), \ + patch('core.tidal_client.time.sleep'): + ids = client._iter_collection_track_ids() + + assert ids == ['1001', '1002', '1003', '1004', '1005'] # full chain, nothing dropped + + def test_429_does_not_set_reconnect_flag(self): + """A rate-limit is transient, NOT a scope problem — must not tell the + user to reconnect.""" + client = _make_authed_client() + responses = iter([_FakeResp(200, _PAGE_ONE), _FakeResp(429, text=""), _FakeResp(200, _PAGE_TWO)]) + with patch.object(client, '_ensure_valid_token', return_value=True), \ + patch('core.tidal_client.requests.get', side_effect=lambda *a, **kw: next(responses)), \ + patch('core.tidal_client.time.sleep'): + client._iter_collection_track_ids() + + assert client.collection_needs_reconnect() is False + + def test_429_exhausts_retries_returns_partial(self): + """If the 429s never clear, give up after the retry budget and return + what we have (PARTIAL) rather than looping forever.""" + client = _make_authed_client() + + def gen(): + yield _FakeResp(200, _PAGE_ONE) + while True: + yield _FakeResp(429, text="") + + responses = gen() + with patch.object(client, '_ensure_valid_token', return_value=True), \ + patch('core.tidal_client.requests.get', side_effect=lambda *a, **kw: next(responses)), \ + patch('core.tidal_client.time.sleep'): + ids = client._iter_collection_track_ids() + + assert ids == ['1001', '1002', '1003'] # page 1 survives; no hang + # --------------------------------------------------------------------------- # get_collection_tracks_count diff --git a/tests/wishlist/test_wishlist_ignore.py b/tests/wishlist/test_wishlist_ignore.py new file mode 100644 index 00000000..783511f2 --- /dev/null +++ b/tests/wishlist/test_wishlist_ignore.py @@ -0,0 +1,190 @@ +"""#874 — wishlist ignore-list: TTL skip-gate for user-removed/cancelled tracks. + +Two layers: + * pure logic (core.wishlist.ignore) — TTL, id-normalization, display extract + * DB seam (MusicDatabase on a temp db) — add/check/remove/list/clear, the + add_to_wishlist gate, the manual-add bypass, and the regression that the + success-cleanup removal path does NOT ignore. +""" + +from datetime import datetime, timedelta + +import pytest + +from core.wishlist.ignore import ( + IGNORE_TTL_DAYS, + REASON_CANCELLED, + REASON_REMOVED, + active_ignored_ids, + extract_display, + is_expired, + is_ignored, + normalize_ignore_id, +) +from database.music_database import MusicDatabase + + +# ── pure logic ────────────────────────────────────────────────────────── + +def test_normalize_strips_composite_album_suffix(): + assert normalize_ignore_id("track123::album456") == "track123" + assert normalize_ignore_id(" track123 ") == "track123" + assert normalize_ignore_id("") == "" + assert normalize_ignore_id(None) == "" + + +def test_is_expired_true_past_ttl_false_within(): + now = datetime(2026, 6, 15, 12, 0, 0) + fresh = (now - timedelta(days=5)).strftime("%Y-%m-%d %H:%M:%S") + stale = (now - timedelta(days=40)).strftime("%Y-%m-%d %H:%M:%S") + assert is_expired(fresh, now) is False + assert is_expired(stale, now) is True + + +def test_is_expired_unparseable_is_treated_expired_fail_open(): + # Corrupt timestamp must lapse (never wedge a track out of the wishlist). + assert is_expired("not-a-date", datetime(2026, 6, 15)) is True + assert is_expired("", datetime(2026, 6, 15)) is True + assert is_expired(None, datetime(2026, 6, 15)) is True + + +def test_is_ignored_matches_composite_and_bare_ids(): + now = datetime(2026, 6, 15, 12, 0, 0) + created = (now - timedelta(days=1)).strftime("%Y-%m-%d %H:%M:%S") + rows = [{"track_id": "abc", "created_at": created}] + # Stored bare, queried with composite (and vice-versa) — both match. + assert is_ignored(rows, "abc::album9", now) is True + rows2 = [{"track_id": "abc", "created_at": created}] + assert is_ignored(rows2, "abc", now) is True + assert is_ignored(rows2, "different", now) is False + + +def test_active_ignored_ids_drops_expired(): + now = datetime(2026, 6, 15, 12, 0, 0) + fresh = (now - timedelta(days=2)).strftime("%Y-%m-%d %H:%M:%S") + stale = (now - timedelta(days=99)).strftime("%Y-%m-%d %H:%M:%S") + rows = [ + {"track_id": "keep", "created_at": fresh}, + {"track_id": "drop", "created_at": stale}, + ] + assert active_ignored_ids(rows, now) == {"keep"} + + +def test_extract_display_handles_dict_and_string_artists(): + assert extract_display({"name": "Song", "artists": [{"name": "A"}]}) == ("Song", "A") + assert extract_display({"name": "Song", "artists": ["B"]}) == ("Song", "B") + assert extract_display({}) == ("", "") + assert extract_display(None) == ("", "") + + +# ── DB seam (temp database — never the live db) ───────────────────────── + +@pytest.fixture +def db(tmp_path): + return MusicDatabase(str(tmp_path / "m.db")) + + +def _track(track_id="t1", name="Some Song", artist="Some Artist", album_id="alb1"): + return { + "id": track_id, + "name": name, + "artists": [{"name": artist}], + "album": {"id": album_id, "name": "Some Album", "images": []}, + } + + +def test_add_check_remove_roundtrip(db): + assert db.is_track_ignored("t1") is False + assert db.add_to_wishlist_ignore("t1", "Song", "Artist", REASON_REMOVED) is True + assert db.is_track_ignored("t1") is True + # Composite id of the same base track is also considered ignored. + assert db.is_track_ignored("t1::albX") is True + assert db.remove_from_wishlist_ignore("t1") is True + assert db.is_track_ignored("t1") is False + + +def test_get_and_clear_ignore_list(db): + db.add_to_wishlist_ignore("a", "SongA", "ArtA", REASON_REMOVED) + db.add_to_wishlist_ignore("b", "SongB", "ArtB", REASON_CANCELLED) + entries = db.get_wishlist_ignore() + assert {e["track_id"] for e in entries} == {"a", "b"} + assert any(e["reason"] == "cancelled" for e in entries) + assert db.clear_wishlist_ignore() == 2 + assert db.get_wishlist_ignore() == [] + + +def test_expired_entry_is_not_active_and_gets_purged(db): + db.add_to_wishlist_ignore("old", "Old", "Art", REASON_REMOVED) + # Backdate it well past the TTL. + with db._get_connection() as conn: + conn.execute( + "UPDATE wishlist_ignore SET created_at = ? WHERE track_id = ?", + ((datetime.now() - timedelta(days=IGNORE_TTL_DAYS + 5)).strftime("%Y-%m-%d %H:%M:%S"), "old"), + ) + conn.commit() + assert db.is_track_ignored("old") is False # lapsed + assert db.get_wishlist_ignore() == [] # and purged on read + with db._get_connection() as conn: + remaining = conn.execute("SELECT COUNT(*) c FROM wishlist_ignore").fetchone()["c"] + assert remaining == 0 + + +# ── the gate: add_to_wishlist honours the ignore-list ─────────────────── + +def test_gate_blocks_auto_readd_but_manual_bypasses_and_clears(db): + track = _track("t1") + # Auto add works first time. + assert db.add_to_wishlist(track, source_type="playlist") is True + # User removes + ignores it. + db.remove_from_wishlist("t1") + db.add_to_wishlist_ignore("t1", "Some Song", "Some Artist", REASON_REMOVED) + # Auto re-add (watchlist / failed-capture / cancel) is now blocked. + assert db.add_to_wishlist(track, source_type="playlist") is False + assert db.is_track_ignored("t1") is True + # A MANUAL add bypasses the gate AND clears the ignore so it sticks. + assert db.add_to_wishlist(track, source_type="manual") is True + assert db.is_track_ignored("t1") is False + + +def test_gate_failopen_when_ignore_table_errors(db, monkeypatch): + # If the ignore check raises, the add must still succeed (never block). + monkeypatch.setattr(db, "is_track_ignored", lambda *a, **k: (_ for _ in ()).throw(RuntimeError("boom"))) + assert db.add_to_wishlist(_track("t9"), source_type="playlist") is True + + +def test_route_remove_track_records_ignore(db, monkeypatch): + # Pin the route → ignore wiring (not just the DB layer): a user-initiated + # remove via the route function must drop the row AND ignore the track. + import types + from core.wishlist import routes as routes_module + + db.add_to_wishlist(_track("rt1", name="Route Song", artist="Route Artist"), + source_type="playlist") + + class _Svc: + database = db + + def remove_track_from_wishlist(self, tid, profile_id=1): + return db.remove_from_wishlist(tid, profile_id=profile_id) + + monkeypatch.setattr(routes_module, "get_wishlist_service", lambda: _Svc()) + runtime = types.SimpleNamespace(profile_id=1, logger=routes_module.module_logger) + + payload, status = routes_module.remove_track_from_wishlist(runtime, "rt1") + assert status == 200 + assert db.is_track_ignored("rt1") is True + # The ignore carries the captured label. + entry = db.get_wishlist_ignore()[0] + assert entry["track_name"] == "Route Song" + assert entry["reason"] == REASON_REMOVED + + +def test_regression_success_cleanup_does_not_ignore(db): + # The post-download success path calls remove_from_wishlist directly — it + # must NOT add anything to the ignore-list (only user remove/cancel do). + track = _track("t5") + assert db.add_to_wishlist(track, source_type="playlist") is True + db.remove_from_wishlist("t5") # simulate success cleanup + assert db.is_track_ignored("t5") is False # NOT ignored + # And so a later legitimate auto-add still works. + assert db.add_to_wishlist(track, source_type="playlist") is True diff --git a/web_server.py b/web_server.py index fd53c71a..6d9cdb3d 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.2" +_SOULSYNC_BASE_VERSION = "2.7.3" def _build_version_string(): """Append short commit hash to version when available (e.g. 2.35+abc1234).""" @@ -7556,6 +7556,18 @@ def approve_quarantine_item(entry_id): docker_resolve_path(config_manager.get('soulseek.download_path', './downloads')), 'Transfer', ) + _req = request.get_json(silent=True) or {} + # #876: capture the sibling alternatives BEFORE approving — the approve + # restores (moves) this entry's file out of quarantine, after which its + # own group_key can no longer be looked up by id. Read-only here; the + # actual deletion happens only after the re-import is safely kicked off. + _sibling_ids = [] + if _req.get('remove_siblings'): + from core.imports.quarantine import find_quarantine_siblings + try: + _sibling_ids = find_quarantine_siblings(_get_quarantine_dir(), entry_id) + except Exception as sib_exc: + logger.warning(f"[Quarantine] Sibling lookup for {entry_id} failed: {sib_exc}") result = approve_quarantine_entry(_get_quarantine_dir(), entry_id, restore_dir) if result is None: return jsonify({ @@ -7574,7 +7586,6 @@ def approve_quarantine_item(entry_id): # context lost task_id/batch_id (the wrapper pops them before quarantine), # so we re-supply them here. Manager-tab approvals (no task_id) keep the # original inner-pipeline path. - _req = request.get_json(silent=True) or {} _task_id = (_req.get('task_id') or '').strip() or None _batch_id = None if _task_id: @@ -7594,7 +7605,28 @@ def approve_quarantine_item(entry_id): _reprocess = lambda: _post_process_matched_download(context_key, context, restored_path) threading.Thread(target=_reprocess, daemon=True).start() logger.info(f"[Quarantine] Approved {entry_id} (original_trigger={trigger}, bypass=all, task={_task_id}) → re-running pipeline") - return jsonify({"success": True, "trigger_bypassed": "all", "original_trigger": trigger}) + # #876: once one alternative for a song is accepted, the other + # quarantined attempts at the SAME intended target are redundant + # failed downloads of a track the user now owns. Delete the siblings + # captured above (scoped to the quarantine manager via `remove_siblings` + # — the download-modal chooser passes no flag and is unaffected). + removed_siblings = [] + if _sibling_ids: + from core.imports.quarantine import delete_quarantine_entry + try: + for sib_id in _sibling_ids: + if delete_quarantine_entry(_get_quarantine_dir(), sib_id): + removed_siblings.append(sib_id) + if removed_siblings: + logger.info(f"[Quarantine] Auto-removed {len(removed_siblings)} sibling alternative(s) of {entry_id}: {removed_siblings}") + except Exception as sib_exc: + logger.warning(f"[Quarantine] Sibling cleanup for {entry_id} failed: {sib_exc}") + return jsonify({ + "success": True, + "trigger_bypassed": "all", + "original_trigger": trigger, + "removed_siblings": removed_siblings, + }) except Exception as e: logger.error(f"[Quarantine] Error approving {entry_id}: {e}") return jsonify({"success": False, "error": str(e)}), 500 @@ -9114,7 +9146,11 @@ def get_artist_discography(artist_id): effective_override_source = 'spotify' from core.metadata.lookup import MetadataLookupOptions - from core.metadata_service import get_artist_discography as _get_artist_discography + # #877: use the artist-DETAIL discography so the Download Discography modal + # gets the SAME release-type split (albums / eps / singles) the Artist + # Detail view shows — EPs were being lumped into singles before, leaving + # the modal's EPs toggle dead. + from core.metadata.discography import get_artist_detail_discography as _get_artist_discography # Server-side per-source ID resolution. Look up the library row # by ANY of the IDs the frontend might send: library DB id, @@ -9192,6 +9228,7 @@ def get_artist_discography(artist_id): ) album_list = discography['albums'] + eps_list = discography.get('eps', []) singles_list = discography['singles'] active_source = discography['source'] source_priority = discography['source_priority'] @@ -9303,6 +9340,7 @@ def get_artist_discography(artist_id): return jsonify({ "albums": album_list, + "eps": eps_list, "singles": singles_list, "source": active_source or (source_priority[0] if source_priority else "unknown"), "artist_info": artist_info, @@ -16781,6 +16819,48 @@ def remove_batch_from_wishlist(): logger.error(f"Error batch removing from wishlist: {e}") return jsonify({"success": False, "error": str(e)}), 500 +@app.route('/api/wishlist/ignore-list', methods=['GET']) +def get_wishlist_ignore_list(): + """#874: active (non-expired) wishlist ignore entries for this profile.""" + try: + from core.wishlist_service import get_wishlist_service + runtime = _build_wishlist_route_runtime() + entries = get_wishlist_service().database.get_wishlist_ignore(profile_id=runtime.profile_id) + from core.wishlist.ignore import IGNORE_TTL_DAYS + return jsonify({"success": True, "entries": entries, "ttl_days": IGNORE_TTL_DAYS}) + except Exception as e: + logger.error(f"Error reading wishlist ignore-list: {e}") + return jsonify({"success": False, "error": str(e), "entries": []}), 500 + +@app.route('/api/wishlist/ignore-list/remove', methods=['POST']) +def remove_from_wishlist_ignore_list(): + """#874: un-ignore a track so it can be auto-acquired again.""" + try: + data = request.get_json() or {} + track_id = data.get('track_id') or data.get('spotify_track_id') + if not track_id: + return jsonify({"success": False, "error": "No track_id provided"}), 400 + from core.wishlist_service import get_wishlist_service + runtime = _build_wishlist_route_runtime() + ok = get_wishlist_service().database.remove_from_wishlist_ignore( + track_id, profile_id=runtime.profile_id) + return jsonify({"success": True, "removed": ok}) + except Exception as e: + logger.error(f"Error removing from wishlist ignore-list: {e}") + return jsonify({"success": False, "error": str(e)}), 500 + +@app.route('/api/wishlist/ignore-list/clear', methods=['POST']) +def clear_wishlist_ignore_list(): + """#874: clear the entire wishlist ignore-list for this profile.""" + try: + from core.wishlist_service import get_wishlist_service + runtime = _build_wishlist_route_runtime() + count = get_wishlist_service().database.clear_wishlist_ignore(profile_id=runtime.profile_id) + return jsonify({"success": True, "cleared": count}) + except Exception as e: + logger.error(f"Error clearing wishlist ignore-list: {e}") + return jsonify({"success": False, "error": str(e)}), 500 + @app.route('/api/add-album-to-wishlist', methods=['POST']) def add_album_track_to_wishlist(): """Endpoint to add a single track from an album to the wishlist.""" @@ -18918,26 +18998,44 @@ def _check_batch_completion_v2(batch_id): def _add_cancelled_task_to_wishlist(task): - """ - Helper function to add cancelled task to wishlist. - Separated for clarity and error isolation. + """Handle a user-cancelled download's wishlist state. + + #874: a manual cancel means "stop auto-retrying this release". The + previous behaviour re-added the cancelled track to the wishlist, which + the auto-processor then re-downloaded → re-cancelled → re-added, + forever. Instead we record a TTL'd ignore entry (so the watchlist / + auto-processor skip it) and drop it from the active wishlist. The user + can still force-download it manually (manual paths bypass the gate), + and the ignore lapses after core.wishlist.ignore.IGNORE_TTL_DAYS so it + is reconsidered later. Error-isolated; never raises into the caller. """ if not task: return - + try: from core.wishlist_service import get_wishlist_service + from core.wishlist.ignore import extract_display, REASON_CANCELLED wishlist_service = get_wishlist_service() - payload = _build_cancelled_task_wishlist_payload(task, profile_id=get_current_profile_id()) - success = wishlist_service.add_spotify_track_to_wishlist(**payload) - - if success: - logger.info(f"[Atomic Cancel] Added '{task.get('track_info', {}).get('name')}' to wishlist") - else: - logger.error(f"[Atomic Cancel] Failed to add '{task.get('track_info', {}).get('name')}' to wishlist") - + profile_id = get_current_profile_id() + track_info = task.get('track_info', {}) or {} + track_id = track_info.get('id') + name = track_info.get('name') + + if not track_id: + logger.warning("[Atomic Cancel] No track id on cancelled task — cannot ignore '%s'", name) + return + + disp_name, disp_artist = extract_display(track_info) + wishlist_service.database.add_to_wishlist_ignore( + track_id, track_name=disp_name, artist_name=disp_artist, + reason=REASON_CANCELLED, profile_id=profile_id) + # Drop it from the active wishlist so the auto-processor stops + # re-attempting it; the gate then blocks any automatic re-add. + wishlist_service.remove_track_from_wishlist(track_id, profile_id=profile_id) + logger.info("[Atomic Cancel] Ignored (TTL) + removed from wishlist: '%s'", name or track_id) + except Exception as e: - logger.error(f"[Atomic Cancel] Critical error adding to wishlist: {e}") + logger.error(f"[Atomic Cancel] Critical error handling cancelled task: {e}") @app.route('/api/playlists//cancel_batch', methods=['POST']) def cancel_batch(batch_id): diff --git a/webui/static/downloads.js b/webui/static/downloads.js index 26303e3b..fe322a78 100644 --- a/webui/static/downloads.js +++ b/webui/static/downloads.js @@ -1186,6 +1186,9 @@ async function openWishlistOverviewModal() { +
@@ -1312,6 +1315,117 @@ function closeWishlistOverviewModal() { console.log('✅ Modal closed'); } +// ── #874: Wishlist ignore-list ("Ignored") modal ──────────────────────── +// Tracks the user removed from the wishlist or cancelled mid-download are +// auto-skipped (not re-queued) until they expire. This modal lets the user +// see what's currently ignored and lift the skip (un-ignore / clear all). + +async function openWishlistIgnoreModal() { + let modal = document.getElementById('wishlist-ignore-modal'); + if (modal) modal.remove(); + modal = document.createElement('div'); + modal.id = 'wishlist-ignore-modal'; + modal.className = 'modal-overlay'; + modal.style.cssText = 'display:flex;position:fixed;inset:0;z-index:10050;align-items:center;justify-content:center;background:rgba(0,0,0,0.6);'; + modal.innerHTML = ` +
+
+

🚫 Ignored Tracks

+ +
+

Removed or cancelled tracks are skipped by auto-download until they expire. Un-ignore to allow auto-download again (you can always download manually).

+
+
Loading...
+
+
+
+ +
+
+
`; + document.body.appendChild(modal); + modal.addEventListener('click', (e) => { if (e.target === modal) closeWishlistIgnoreModal(); }); + await loadWishlistIgnoreList(); +} + +function closeWishlistIgnoreModal() { + const modal = document.getElementById('wishlist-ignore-modal'); + if (modal) modal.remove(); +} + +async function loadWishlistIgnoreList() { + const list = document.getElementById('wishlist-ignore-list'); + const clearBtn = document.getElementById('wishlist-ignore-clear-btn'); + if (!list) return; + try { + const resp = await fetch('/api/wishlist/ignore-list'); + const data = await resp.json(); + const entries = (data && data.entries) || []; + if (clearBtn) clearBtn.style.display = entries.length ? '' : 'none'; + if (!entries.length) { + list.innerHTML = '
🎉

Nothing ignored.
'; + return; + } + const ttl = (data && data.ttl_days) || 30; + list.innerHTML = entries.map(e => { + const title = escapeHtml(e.track_name || e.track_id || 'Unknown'); + const artist = escapeHtml(e.artist_name || ''); + const reason = e.reason === 'cancelled' ? 'Cancelled' : 'Removed'; + const tid = escapeHtml(String(e.track_id || '')); + return `
+
+
${title}
+
${artist}${artist ? ' · ' : ''}${reason} · skips ${ttl}d
+
+ +
`; + }).join(''); + } catch (err) { + list.innerHTML = '
Error loading ignored tracks
'; + } +} + +async function unignoreWishlistTrack(trackId) { + if (!trackId) return; + try { + const resp = await fetch('/api/wishlist/ignore-list/remove', { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ track_id: trackId }), + }); + const data = await resp.json(); + if (data && data.success) { + showToast('Track un-ignored — it can be auto-downloaded again.', 'success'); + await loadWishlistIgnoreList(); + } else { + showToast(`Un-ignore failed: ${(data && data.error) || 'unknown'}`, 'error'); + } + } catch (err) { + showToast(`Un-ignore failed: ${err.message}`, 'error'); + } +} + +async function clearWishlistIgnoreList() { + if (!await showConfirmDialog({ + title: 'Clear Ignored List', + message: 'Allow all currently-ignored tracks to be auto-downloaded again?', + confirmText: 'Clear All', + cancelText: 'Cancel', + })) return; + try { + const resp = await fetch('/api/wishlist/ignore-list/clear', { method: 'POST' }); + const data = await resp.json(); + if (data && data.success) { + showToast(`Cleared ${data.cleared || 0} ignored track(s).`, 'success'); + await loadWishlistIgnoreList(); + } else { + showToast(`Clear failed: ${(data && data.error) || 'unknown'}`, 'error'); + } + } catch (err) { + showToast(`Clear failed: ${err.message}`, 'error'); + } +} + async function cleanupWishlistOverview() { console.log('🧹 cleanupWishlistOverview() called'); diff --git a/webui/static/helper.js b/webui/static/helper.js index b1168b2d..7774a8ac 100644 --- a/webui/static/helper.js +++ b/webui/static/helper.js @@ -3404,21 +3404,19 @@ 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.2': [ - { date: 'June 2026 — 2.7.2 release' }, - { title: 'Organize playlists into folders', desc: 'optionally mirror each playlist into its own folder on disk — symlink (no extra space) or copy — so external players (plex / jellyfin / music assistant) see your soulsync playlists as real folders. rebuilds itself after every sync, prunes removed tracks, and there\'s a manual rebuild button in settings.', page: 'settings' }, - { title: 'Export server playlists as M3U', desc: 'one-click "Export M3U" button in the Server Playlists compare/editor toolbar — writes a standard .m3u of the playlist\'s tracks and downloads it to your browser. handy for music assistant and friends.', page: 'sync' }, - { title: 'Follow-only watchlist', desc: 'each watchlist artist now has an "auto-download" toggle (on by default). turn it off to just follow an artist — scans still discover and surface new releases, they just don\'t get auto-added to the wishlist.', page: 'artists' }, - { title: 'Download from a SoundCloud link (#865)', desc: 'paste a soundcloud track url — including unlisted / private share links — into manual search and it resolves + downloads directly.', page: 'downloads' }, - { title: 'YouTube playlists keep the artist (#863)', desc: 'youtube / youtube-music playlists that used to import as "Unknown Artist" now recover the real artist from the track\'s music metadata, the "Artist - Title" pattern, or the uploading channel — and fall back to the matched artist when youtube gives nothing.', page: 'sync' }, - { title: 'Rename mirrored playlists', desc: 'a mirrored playlist now has a rename (✏️) button — set a custom name that changes how it shows in soulsync and how it syncs, survives upstream refreshes, and still tracks the same server playlist.', page: 'sync' }, - { title: 'New maintenance jobs', desc: 'ReplayGain Filler (#437) computes + writes missing replaygain tags across your library, and Empty Folder Cleaner sweeps out leftover empty directories. both under Tools → library maintenance.', page: 'tools' }, - { title: 'Export your watchlist + library', desc: 'one Export button with a scope selector dumps your watchlist roster and/or whole library roster to JSON / CSV / text.', page: 'artists' }, - { title: 'Custom completed-download path for Torrent/Usenet (#857)', desc: 'point soulsync at the in-container folder your torrent/usenet client drops finished files into (e.g. a category subfolder), so completed grabs are found instead of going missing.', page: 'settings' }, - { title: 'HiFi instances: restore + new working instance', desc: 'a "Restore Defaults" button re-adds any built-in instances you removed (keeps your own), the ✔/✖ controls have bigger tap targets, and a freshly-confirmed working instance is auto-pushed to everyone (thanks Sokhi).', page: 'settings' }, - { title: 'Artist DB Record inspector', desc: 'an artist\'s detail page can now show the raw "DB Record" — everything the database knows about that artist — for debugging metadata.', page: 'library' }, - { title: 'Fixes', desc: 'a hung database update self-heals now instead of wedging on "Starting..." forever (#859); Library Reorganize finally works on media-server libraries by falling back to tag mode (#862); spotify (no-auth) shows as connected and the dashboard test reports it correctly instead of claiming deezer; navidrome reconnects itself instead of latching disconnected; the orphan detector hard-bails on a mass-orphan flood; plus more #852 lock-screen hardening and login-password management in Manage Profiles.', page: 'settings' }, - { title: 'Earlier versions', desc: '2.7.1 added download verification & a review queue (acoustid fingerprint-checks every download), closed the websocket login-bypass (#852), and the acoustid Relocate fix. 2.7.0 brought multi-user for real — per-profile streaming accounts, opt-in login, reverse-proxy support. before that the 2.6.x cycle brought the blocklist, the download-retry overhaul, Download Origins, Spotify-no-auth metadata, and Library Re-tag.' }, + '2.7.3': [ + { date: 'June 2026 — 2.7.3 release' }, + { title: 'Quality Upgrade Finder', desc: 'a new findings-based job that scans your library for tracks you own in worse quality than is available and lets you upgrade them. matches by ISRC first, then album→track, then artist+title, with a direct track-ID tier, a dedup-skip, and a duration guard so it never swaps in the wrong song. replaces the old auto-acting Quality Scanner.', page: 'tools' }, + { title: 'Tidal playlist discovery shows everything (#867)', desc: 'tidal playlist discovery used to cap at ~21 tracks — it now walks the whole playlist. and the discovery modal opens instantly in its "discovering" state instead of freezing the UI for ~10s on a pre-fetch.', page: 'sync' }, + { title: 'Wishlist ignore-list (#874)', desc: 'remove a track from the wishlist or cancel an in-flight wishlist download and soulsync stops auto-re-adding it (it used to re-grab the same thing forever). softer than a blocklist — it expires after 30 days and never blocks a manual download. new "Ignored" view to see/undo it.', page: 'downloads' }, + { title: 'Quarantine: group duplicates + auto-clear (#876)', desc: 'multiple failed attempts at the same song now collapse into one collapsible group; approve the good one and the other options auto-clear. plus the quarantine tab shows the correct count the moment you open it.', page: 'downloads' }, + { title: 'Download Discography filters match Artist Detail (#877)', desc: 'the bulk-download modal now has the same Albums / EPs / Singles + Live / Compilations / Featured filters Artist Detail does — and the EPs toggle actually works now (it was always empty before).', page: 'artists' }, + { title: 'Settings can no longer wipe your config (#879)', desc: 'if the settings page failed to load it used to fall back to blank defaults and then autosave the blanks over your real config. it now bails on a failed load and tells you to reload — your saved config is left untouched.', page: 'settings' }, + { title: 'Tidal Favorites mirror grabs everything (#880)', desc: 'a transient tidal rate-limit (429) mid-sync used to truncate your Favorite Tracks mirror to ~98 of 500+. it retries the page with backoff now instead of stopping short.', page: 'sync' }, + { title: 'Track numbers fixed (the "Track 01" bug)', desc: 'single tracks — especially deezer-sourced — imported as "01 - Title" regardless of their real album position, littering album folders with duplicate 01s. soulsync now recovers the real position from the downloaded file\'s own tag instead of guessing 1.', page: 'downloads' }, + { title: 'More fixes', desc: 'deezer ARL stops appearing to "reset itself" — the saved token is tested directly, not a redacted mask (#870); an artist sharing a name with another no longer gets the wrong discography (disambiguated by the catalog you actually own, #868); a "Title - Remix" search now matches the base-titled track in your library; and a colon in a title (T:T) matches an underscore variant (T_T).', page: 'library' }, + { title: 'Sidebar polish', desc: 'frosted-glass header blur, vertically-centered nav count badges, and the My-Accounts / Personal-Settings buttons are hidden for admins (who use the global app account anyway).', page: 'settings' }, + { title: 'Earlier versions', desc: '2.7.2 added playlist-folder mirroring, server-playlist M3U export, follow-only watchlist, soundcloud-link + better youtube imports, and ReplayGain / Empty-Folder maintenance jobs. 2.7.1 added download verification (acoustid fingerprint-checks every download) + a review queue and closed the websocket login-bypass (#852). 2.7.0 made multi-user real — per-profile streaming accounts, opt-in login, reverse-proxy support. before that the 2.6.x cycle brought the blocklist, the download-retry overhaul, Download Origins, and Library Re-tag.' }, ], }; @@ -3449,58 +3447,57 @@ const WHATS_NEW = { // usage_note?: 'optional hint shown at the bottom' } const VERSION_MODAL_SECTIONS = [ { - title: "Organize playlists into folders", - description: "soulsync can now mirror each of your playlists into its own folder on disk, so external players (plex / jellyfin / music assistant) see them as real folders instead of just living inside soulsync.", + title: "Quality Upgrade Finder", + description: "a new findings-based job that scans your library for tracks you own in worse quality than is available, and lets you upgrade them. replaces the old auto-acting Quality Scanner.", features: [ - "symlink (no extra disk space) or copy — your choice", - "rebuilds itself after every sync and prunes tracks you removed", - "separate output root + a manual rebuild button in settings", + "matches by ISRC first, then album→track, then artist+title — using the IDs enrichment already embedded", + "a direct track-ID tier for exact source matches", + "dedup-skip so it won't re-find the same upgrade, and a duration guard so it never swaps in the wrong track", ], - usage_note: "Settings → Playlists → organize into folders", + usage_note: "Tools → Quality Upgrade Finder", }, { - title: "Better YouTube & SoundCloud imports", - description: "the two weak spots in playlist/link importing got fixed.", + title: "Wishlist ignore-list (#874)", + description: "remove a track from the wishlist, or cancel an in-flight wishlist download, and soulsync stops auto-re-adding it — it used to re-download the same release forever.", features: [ - "#863 — youtube / youtube-music playlists that imported as \"Unknown Artist\" now recover the real artist from music metadata, the \"Artist - Title\" pattern, or the uploading channel (and fall back to the matched artist)", - "#865 — paste a soundcloud track link, including unlisted / private share urls, into manual search to download it directly", + "softer than a blocklist: it expires after 30 days and never blocks a manual download", + "a new \"Ignored\" view lets you see what's skipped and un-ignore anything", + ], + usage_note: "Wishlist → Ignored", + }, + { + title: "Quarantine: group duplicates + auto-clear (#876)", + description: "the quarantine workflow got a cleanup pass.", + features: [ + "multiple failed attempts at the same song collapse into one collapsible group", + "approve the good one and the other options auto-clear", + "the quarantine tab shows the correct count the moment you open it (no more stale 0)", ], }, { - title: "Export server playlists as M3U", - description: "a one-click \"Export M3U\" button now sits in the Server Playlists compare/editor toolbar — writes a standard .m3u of the playlist and downloads it to your browser. great for music assistant.", - features: [], - usage_note: "Sync → Server Playlists → Export M3U", - }, - { - title: "Follow-only watchlist + more", - description: "a grab-bag of requested features.", + title: "Tidal discovery & favorites (#867, #880)", + description: "two tidal sync fixes.", features: [ - "per-artist \"auto-download\" toggle: turn it off to just follow an artist — scans still surface new releases, they just don't auto-add to the wishlist", - "rename mirrored playlists (✏️): a custom name that changes the display + sync name, survives refreshes, still tracks the server playlist", - "export your watchlist and/or whole library roster to JSON / CSV / text", - "ReplayGain Filler (#437) and Empty Folder Cleaner library-maintenance jobs", - "custom in-container completed-download path for Torrent / Usenet sources (#857)", - "HiFi instances: Restore Defaults button, bigger tap targets, and a new confirmed-working instance pushed to everyone", - "Artist detail \"DB Record\" inspector for debugging metadata", + "#867 — playlist discovery now walks the whole playlist instead of capping at ~21 tracks, and the modal opens instantly instead of freezing the UI for ~10s", + "#880 — a transient rate-limit (429) mid-sync no longer truncates your Favorite Tracks mirror to ~98 of 500+; it retries with backoff", ], }, { title: "Fixes this release", - description: "a stack of issue fixes on top of 2.7.1.", + description: "a stack of issue fixes on top of 2.7.2.", features: [ - "#859 — a hung database update self-heals instead of wedging on \"Starting...\" forever", - "#862 — Library Reorganize now works on media-server libraries (falls back to tag mode when there are no source IDs)", - "spotify (no-auth) shows as connected and the dashboard test reports it correctly, instead of claiming a deezer fallback", - "navidrome reconnects itself instead of latching \"disconnected\"", - "the orphan detector hard-bails on a mass-orphan flood instead of plowing ahead", - "more #852 lock-screen hardening + login-password management in Manage Profiles", - "Aria2 added to the torrent client list", + "the \"Track 01\" bug — single tracks (especially deezer) imported as \"01\" regardless of real album position; now recovered from the file's own tag", + "#879 — a failed Settings load can no longer overwrite your saved config with blank defaults", + "#877 — Download Discography filters now match Artist Detail (working EPs toggle + Live / Compilations / Featured)", + "#870 — deezer ARL stops appearing to \"reset itself\" (the saved token is tested, not a redacted mask)", + "#868 — an artist sharing a name with another no longer gets the wrong discography", + "Find & Add matches a \"Title - Remix\" search to the base-titled library track; a colon (T:T) matches an underscore variant (T_T)", + "sidebar polish: frosted-glass header, centered nav badges, admin-only cleanup", ], }, { - title: "Earlier in 2.7.1 / 2.7.0", - description: "2.7.1 added download verification & an unverified review queue (acoustid fingerprint-checks every download against what you asked for), closed the websocket login-bypass (#852), and added the acoustid Relocate fix action. 2.7.0 made multi-user real: per-profile streaming accounts (My Accounts), auto-sync running as its owner, opt-in username/password login with recovery, and reverse-proxy support. before that, the 2.6.x cycle brought the blocklist, the download-retry overhaul, Download Origins, Spotify-no-auth metadata, and Library Re-tag.", + title: "Earlier in 2.7.2 / 2.7.1 / 2.7.0", + description: "2.7.2 added playlist-folder mirroring, server-playlist M3U export, follow-only watchlist, soundcloud-link + better youtube imports, and ReplayGain / Empty-Folder maintenance jobs. 2.7.1 added download verification (acoustid fingerprint-checks every download against what you asked for) + an unverified review queue, and closed the websocket login-bypass (#852). 2.7.0 made multi-user real: per-profile streaming accounts (My Accounts), opt-in username/password login with recovery, and reverse-proxy support. before that, the 2.6.x cycle brought the blocklist, the download-retry overhaul, Download Origins, and Library Re-tag.", features: [], }, ]; diff --git a/webui/static/init.js b/webui/static/init.js index d339cb18..a8a11b98 100644 --- a/webui/static/init.js +++ b/webui/static/init.js @@ -1116,6 +1116,15 @@ function updateProfileIndicator() { const statusSection = document.querySelector('.status-section--clickable'); if (statusSection) statusSection.classList.toggle('status-section--locked', !currentProfile.is_admin); + // My Accounts (per-profile streaming OAuth) and My Settings (per-profile + // server library) are inert for admin — admin uses the global app account + // for every service and the full Settings page. Hide both for admin; keep + // them for non-admins, who actually get a connect/library UI. + const myAccountsBtn = document.getElementById('my-accounts-btn'); + const personalSettingsBtn = document.getElementById('personal-settings-btn'); + if (myAccountsBtn) myAccountsBtn.style.display = currentProfile.is_admin ? 'none' : ''; + if (personalSettingsBtn) personalSettingsBtn.style.display = currentProfile.is_admin ? 'none' : ''; + indicator.onclick = async () => { const res = await fetch('/api/profiles'); const data = await res.json(); diff --git a/webui/static/library.js b/webui/static/library.js index cd88e264..48f347f8 100644 --- a/webui/static/library.js +++ b/webui/static/library.js @@ -1891,16 +1891,12 @@ function createReleaseCard(release) { // Store mutable reference so stream updates propagate to click handler card._releaseData = release; - // Tag card for content-type filtering - const livePattern = /\b(live)\b|\(live[^)]*\)|\[live[^]]*\]/i; - const compilationPattern = /\b(greatest hits|best of|collection|anthology|essential)\b/i; - const featuredPattern = /\(?\bfeat\.?\s|\bft\.?\s|\bfeaturing\b/i; - const isLive = livePattern.test(release.title || '') || (release.album_type === 'compilation' && livePattern.test(release.title || '')); - const isCompilation = (release.album_type === 'compilation') || compilationPattern.test(release.title || ''); - const isFeatured = featuredPattern.test(release.title || ''); - card.setAttribute("data-is-live", isLive ? "true" : "false"); - card.setAttribute("data-is-compilation", isCompilation ? "true" : "false"); - card.setAttribute("data-is-featured", isFeatured ? "true" : "false"); + // Tag card for content-type filtering (shared classifier — #877, so Artist + // Detail and the Download Discography modal never drift apart). + const cc = _classifyReleaseContent(release); + card.setAttribute("data-is-live", cc.isLive ? "true" : "false"); + card.setAttribute("data-is-compilation", cc.isCompilation ? "true" : "false"); + card.setAttribute("data-is-featured", cc.isFeatured ? "true" : "false"); // Background image — use data-bg-src for IntersectionObserver lazy loading // (observeLazyBackgrounds is called by the caller after appending the grid). @@ -2518,8 +2514,8 @@ async function openDiscographyModal() { const data = await res.json(); if (!data.error) { - discography = { albums: data.albums || [], singles: data.singles || [] }; - if (discography.albums.length > 0 || discography.singles.length > 0) { + discography = { albums: data.albums || [], eps: data.eps || [], singles: data.singles || [] }; + if (discography.albums.length > 0 || discography.eps.length > 0 || discography.singles.length > 0) { artistsPageState.artistDiscography = discography; artistsPageState.sourceOverride = data.source || artistsPageState.sourceOverride || null; // Use metadata source ID for the modal (needed for download API calls) @@ -2569,6 +2565,9 @@ async function openDiscographyModal() { + + +
@@ -2605,21 +2604,36 @@ async function openDiscographyModal() { function _esc(s) { const d = document.createElement('div'); d.textContent = s; return d.innerHTML; } +// #877: single source of truth for content-type classification, shared by the +// Artist Detail cards and the Download Discography modal so they can't drift. +function _classifyReleaseContent(release) { + const t = (release && (release.title || release.name)) || ''; + const livePattern = /\b(live)\b|\(live[^)]*\)|\[live[^\]]*\]/i; + const compilationPattern = /\b(greatest hits|best of|collection|anthology|essential)\b/i; + const featuredPattern = /\(?\bfeat\.?\s|\bft\.?\s|\bfeaturing\b/i; + return { + isLive: livePattern.test(t), + isCompilation: (release && release.album_type === 'compilation') || compilationPattern.test(t), + isFeatured: featuredPattern.test(t), + }; +} + function _renderDiscogCard(release, index, completionData) { const comp = completionData?.albums?.find(c => c.id === release.id) || completionData?.singles?.find(c => c.id === release.id); const status = comp?.status || 'unknown'; const isOwned = status === 'completed'; const isPartial = status === 'partial' || status === 'nearly_complete'; const year = release.release_date ? release.release_date.substring(0, 4) : ''; - const tracks = release.total_tracks || 0; + const tracks = release.total_tracks || release.track_count || 0; const img = release.image_url || ''; + const cc = _classifyReleaseContent(release); const checked = !isOwned; const statusClass = isOwned ? 'owned' : isPartial ? 'partial' : ''; const statusIcon = isOwned ? '✓' : isPartial ? '◐' : ''; const albumName = release.name || release.title || ''; return ` -