From 528aedbdfea947c8c991ec1c7b36752071e6773c Mon Sep 17 00:00:00 2001 From: ragnarlotus Date: Thu, 25 Jun 2026 22:54:43 +0200 Subject: [PATCH 1/7] fix(reorganize): include MusicBrainz release IDs --- core/library_reorganize.py | 1 + tests/test_reorganize_canonical_source.py | 24 +++++++++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/core/library_reorganize.py b/core/library_reorganize.py index 574d5ee7..a7e4fa37 100644 --- a/core/library_reorganize.py +++ b/core/library_reorganize.py @@ -100,6 +100,7 @@ _ALBUM_ID_COLUMNS = { 'deezer': 'deezer_id', 'discogs': 'discogs_id', 'hydrabase': 'soul_id', + 'musicbrainz': 'musicbrainz_release_id', } # Human-facing label for each source. diff --git a/tests/test_reorganize_canonical_source.py b/tests/test_reorganize_canonical_source.py index 711638ed..bb39c436 100644 --- a/tests/test_reorganize_canonical_source.py +++ b/tests/test_reorganize_canonical_source.py @@ -73,3 +73,27 @@ def test_no_canonical_unchanged(monkeypatch): album_data = {"spotify_album_id": "sp1"} source, _, _ = lr._resolve_source(album_data, "spotify") assert source == "spotify" + + +def test_musicbrainz_release_id_is_used_by_priority_walk(monkeypatch): + _patch_fetch(monkeypatch, { + ('musicbrainz', 'mb-release-1'): [{'name': 'x'}], + }) + monkeypatch.setattr( + lr, + 'get_source_priority', + lambda primary: ['musicbrainz', 'spotify'], + ) + + album_data = { + 'musicbrainz_release_id': 'mb-release-1', + } + + source, api_album, items = lr._resolve_source( + album_data, + 'musicbrainz', + ) + + assert source == 'musicbrainz' + assert api_album == {'name': 'musicbrainz:mb-release-1'} + assert items == [{'name': 'x'}] From 8a6b02b3fdeeefaad2b665479d2571e0bcabe0da Mon Sep 17 00:00:00 2001 From: ragnarlotus Date: Fri, 26 Jun 2026 03:10:07 +0200 Subject: [PATCH 2/7] test(reorganize): verify MusicBrainz integration --- tests/test_reorganize_canonical_source.py | 79 ++++++++++++++++++++--- 1 file changed, 69 insertions(+), 10 deletions(-) diff --git a/tests/test_reorganize_canonical_source.py b/tests/test_reorganize_canonical_source.py index bb39c436..f7a2939b 100644 --- a/tests/test_reorganize_canonical_source.py +++ b/tests/test_reorganize_canonical_source.py @@ -7,7 +7,11 @@ canonical_source/canonical_album_id, and an explicit user source pick from __future__ import annotations +from unittest.mock import MagicMock + import core.library_reorganize as lr +import core.metadata.registry as metadata_registry +from core.musicbrainz_search import MusicBrainzSearchClient def _patch_fetch(monkeypatch, tracklists): @@ -76,24 +80,79 @@ def test_no_canonical_unchanged(monkeypatch): def test_musicbrainz_release_id_is_used_by_priority_walk(monkeypatch): - _patch_fetch(monkeypatch, { - ('musicbrainz', 'mb-release-1'): [{'name': 'x'}], - }) + client = MusicBrainzSearchClient() + client._client = MagicMock() + client._client.get_release_group.return_value = None + client._client.get_release.return_value = { + "id": "mb-release-1", + "title": "Test Album", + "date": "2024-01-01", + "artist-credit": [{"name": "Test Artist"}], + "release-group": { + "id": "mb-group-1", + "primary-type": "Album", + "secondary-types": [], + }, + "media": [ + { + "position": 1, + "tracks": [ + { + "id": "track-1", + "number": "1", + "position": 1, + "length": 180000, + "recording": { + "id": "recording-1", + "title": "Test Track", + "artist-credit": [{"name": "Test Artist"}], + "length": 180000, + }, + }, + ], + }, + ], + } + + monkeypatch.setattr( + metadata_registry, + "get_musicbrainz_client", + lambda *args, **kwargs: client, + ) monkeypatch.setattr( lr, - 'get_source_priority', - lambda primary: ['musicbrainz', 'spotify'], + "get_source_priority", + lambda primary: ["musicbrainz", "spotify"], ) album_data = { - 'musicbrainz_release_id': 'mb-release-1', + "musicbrainz_release_id": "mb-release-1", } source, api_album, items = lr._resolve_source( album_data, - 'musicbrainz', + "musicbrainz", ) - assert source == 'musicbrainz' - assert api_album == {'name': 'musicbrainz:mb-release-1'} - assert items == [{'name': 'x'}] + assert source == "musicbrainz" + assert api_album["id"] == "mb-release-1" + assert api_album["name"] == "Test Album" + assert items == api_album["tracks"] + assert items == [ + { + "id": "recording-1", + "name": "Test Track", + "artists": [{"name": "Test Artist"}], + "duration_ms": 180000, + "track_number": 1, + "disc_number": 1, + }, + ] + client._client.get_release_group.assert_any_call( + "mb-release-1", + includes=["releases", "artist-credits"], + ) + client._client.get_release.assert_any_call( + "mb-release-1", + includes=["recordings", "artist-credits", "release-groups"], + ) From 31637e00962a5d75ecbb16126a4fee99a88d2532 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Fri, 26 Jun 2026 19:04:24 -0700 Subject: [PATCH 3/7] canonical: recognize musicbrainz as a readable album source (PR #929 follow-up) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #929 added 'musicbrainz' to library_reorganize._ALBUM_ID_COLUMNS but not to the canonical layer, breaking the equality invariant test (canonical reads exactly what reorganize reads). add musicbrainz to CANONICAL_ALBUM_SOURCES, and move it from the 'can't pin' param group to the 'pins' group in the manual-lock tests — now consistent, and forward-compatible with pinning a deliberately-matched MB edition. inert at runtime today (mb isn't in the manual source selector, so should_pin is never called with it). 540 canonical/reorganize tests green. --- core/metadata/canonical_version.py | 2 +- tests/test_canonical_manual_lock.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/core/metadata/canonical_version.py b/core/metadata/canonical_version.py index 9871d89e..1f54a1e8 100644 --- a/core/metadata/canonical_version.py +++ b/core/metadata/canonical_version.py @@ -211,7 +211,7 @@ def pick_canonical_release( # core.library_reorganize._ALBUM_ID_COLUMNS — a test pins them in sync). A manual # match on any of these should pin/lock the canonical version (#758); a match on # a source the canonical tools don't read (e.g. lastfm) has no version to pin. -CANONICAL_ALBUM_SOURCES = frozenset({'spotify', 'itunes', 'deezer', 'discogs', 'hydrabase'}) +CANONICAL_ALBUM_SOURCES = frozenset({'spotify', 'itunes', 'deezer', 'discogs', 'hydrabase', 'musicbrainz'}) def should_pin_manual_canonical(entity_type: str, source: str) -> bool: diff --git a/tests/test_canonical_manual_lock.py b/tests/test_canonical_manual_lock.py index ad9fd99e..54f9bcea 100644 --- a/tests/test_canonical_manual_lock.py +++ b/tests/test_canonical_manual_lock.py @@ -24,7 +24,7 @@ from database.music_database import MusicDatabase # should_pin_manual_canonical — pure # --------------------------------------------------------------------------- -@pytest.mark.parametrize('source', ['spotify', 'itunes', 'deezer', 'discogs', 'hydrabase']) +@pytest.mark.parametrize('source', ['spotify', 'itunes', 'deezer', 'discogs', 'hydrabase', 'musicbrainz']) def test_pins_album_on_recognised_source(source): assert should_pin_manual_canonical('album', source) is True @@ -34,7 +34,7 @@ def test_does_not_pin_non_album(entity): assert should_pin_manual_canonical(entity, 'spotify') is False -@pytest.mark.parametrize('source', ['lastfm', 'genius', 'musicbrainz', 'audiodb', 'tidal']) +@pytest.mark.parametrize('source', ['lastfm', 'genius', 'audiodb', 'tidal']) def test_does_not_pin_source_canonical_cant_read(source): # No album-version data the canonical tools read → nothing to pin. assert should_pin_manual_canonical('album', source) is False From e96d62432f9372a2494d21e3b9b58b7c2058da5f Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Fri, 26 Jun 2026 19:56:11 -0700 Subject: [PATCH 4/7] test(album-completeness): stop polluting sys.modules (fix flaky suite failure) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit the file faked spotipy + config.settings in sys.modules at import time with no teardown. the fake config.settings had no ConfigManager, so depending on collection order it leaked into tests/test_config_save_retry and intermittently failed the full suite. the real modules import fine in the test env (spotipy is installed, config.settings has both ConfigManager + config_manager), so the stubs were pure liability — removed them. album tests still pass (10), the album+config combo that errored now passes (17), 573 repair/ config/canonical tests green. --- tests/test_album_completeness_job.py | 34 ++++------------------------ 1 file changed, 4 insertions(+), 30 deletions(-) diff --git a/tests/test_album_completeness_job.py b/tests/test_album_completeness_job.py index bd0885cd..4cca27fc 100644 --- a/tests/test_album_completeness_job.py +++ b/tests/test_album_completeness_job.py @@ -1,4 +1,3 @@ -import sys import types @@ -10,35 +9,10 @@ class _DummyConfigManager: return "plex" -if "spotipy" not in sys.modules: - spotipy = types.ModuleType("spotipy") - - class _DummySpotify: - def __init__(self, *args, **kwargs): - pass - - oauth2 = types.ModuleType("spotipy.oauth2") - - class _DummyOAuth: - def __init__(self, *args, **kwargs): - pass - - spotipy.Spotify = _DummySpotify - oauth2.SpotifyOAuth = _DummyOAuth - oauth2.SpotifyClientCredentials = _DummyOAuth - spotipy.oauth2 = oauth2 - sys.modules["spotipy"] = spotipy - sys.modules["spotipy.oauth2"] = oauth2 - -if "config.settings" not in sys.modules: - config_pkg = types.ModuleType("config") - settings_mod = types.ModuleType("config.settings") - - settings_mod.config_manager = _DummyConfigManager() - config_pkg.settings = settings_mod - sys.modules["config"] = config_pkg - sys.modules["config.settings"] = settings_mod - +# NOTE: deliberately no sys.modules stubbing of spotipy / config.settings here. Both import +# fine in the test env, and faking them globally (with no teardown) leaked into other files — +# it left a config.settings with no ConfigManager, intermittently breaking +# tests/test_config_save_retry depending on collection order. from core.repair_jobs.album_completeness import AlbumCompletenessJob import core.repair_jobs.album_completeness as album_completeness_module From 7e2d2db08df067870dcdda866839119a1d26dc43 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Fri, 26 Jun 2026 20:30:20 -0700 Subject: [PATCH 5/7] watchlist: don't fuse different editions as the same album (Sokhi: Expedition 33) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _normalize_album_for_match stripped ANY trailing '- clause', so a real distinguishing subtitle ('Clair Obscur: Expedition 33 - Nos vies en Lumière (Bonus Edition)') collapsed to the same name as the OST → _albums_likely_match treated them as one album → the watchlist marked unowned tracks of one edition as owned via the other and under-wishlisted. - strip a trailing '- ...' clause ONLY when every token in it is an edition/format qualifier (+ connectors / year-ordinal): '- Single', '- Acoustic Version', '- 2011 Remaster' still collapse, but real subtitles ('- Nos vies en Lumière', '- Volume 2', '- Live in Berlin') are kept. Avoids the inverse regression (a same-album pair splitting into a redownload loop), which a naive narrow strip would have caused. - drop the loose substring shortcut + raise the fuzzy floor 0.6→0.85; genuine drift already collapses to an EXACT match, so the looseness only ever produced false fuses. blast radius: _albums_likely_match has exactly one caller (the allow-duplicates skip). 48 album-match tests pass (qualifier-suffix merges + edition-subtitle splits) + 219 watchlist. --- core/watchlist_scanner.py | 33 ++++++++++++++++++++++------- tests/test_watchlist_album_match.py | 24 +++++++++++++++++++++ 2 files changed, 49 insertions(+), 8 deletions(-) diff --git a/core/watchlist_scanner.py b/core/watchlist_scanner.py index 55d03105..f8eda5e3 100644 --- a/core/watchlist_scanner.py +++ b/core/watchlist_scanner.py @@ -328,6 +328,22 @@ _ALBUM_QUALIFIER_RE = re.compile( re.IGNORECASE, ) +# A trailing "- ..." clause is stripped ONLY when EVERY token in it is an edition/format +# qualifier (+ connectors / a year-ordinal). So "- Single", "- Acoustic Version", "- 2011 +# Remaster" collapse to the base name, but a real distinguishing subtitle ("- Nos vies en +# Lumière", "- Live in Berlin") is kept — the bug was a blanket "- anything$" strip that +# erased subtitles and fused different editions (Sokhi: Expedition 33 OST vs Bonus Edition). +_DASH_QUALIFIER_WORD = ( + r'live|acoustic|electric|instrumental|unplugged|mono|stereo|demos?|reissue|' + r'remix(?:es)?|edit(?:ed)?|radio|single|ep|lp|version|mix(?:es)?|sessions?|bootleg|' + r'covers?|original|redux|deluxe|expanded|remaster(?:ed)?|anniversary|special|' + r'edition|bonus|extended|explicit|clean|soundtrack|ost|score' +) +_TRAILING_DASH_QUALIFIER_RE = re.compile( + r'\s*-\s*(?:(?:' + _DASH_QUALIFIER_WORD + r'|the|a|and|&|\+|\d+(?:st|nd|rd|th)?)\b[\s\-]*)+$', + re.IGNORECASE, +) + def _normalize_album_for_match(name: str) -> str: """Return a canonical form of an album name suitable for fuzzy comparison. @@ -347,8 +363,9 @@ def _normalize_album_for_match(name: str) -> str: # they're almost always edition or commentary noise, not part of the # album's identifying name. cleaned = re.sub(r'\s*[\(\[][^\)\]]*[\)\]]\s*', ' ', cleaned) - # Trailing dash-clauses ("Album - Remastered", "Album - Live") - cleaned = re.sub(r'\s*-\s*[^-]+$', '', cleaned) + # Trailing dash-clause, but ONLY when it's entirely edition/format qualifiers — a real + # subtitle is preserved (see _TRAILING_DASH_QUALIFIER_RE). + cleaned = _TRAILING_DASH_QUALIFIER_RE.sub(' ', cleaned) cleaned = re.sub(r'[^a-z0-9 ]+', ' ', cleaned.lower()) cleaned = re.sub(r'\s+', ' ', cleaned).strip() return cleaned @@ -382,7 +399,7 @@ def _extract_volume_marker(normalized_name: str): return last.group(1) or last.group(2) -def _albums_likely_match(spotify_album: str, lib_album: str, threshold: float = 0.6) -> bool: +def _albums_likely_match(spotify_album: str, lib_album: str, threshold: float = 0.85) -> bool: """Return True when two album names plausibly identify the same release. Designed to swallow naming drift between metadata sources and the @@ -406,11 +423,11 @@ def _albums_likely_match(spotify_album: str, lib_album: str, threshold: float = return False if norm_a == norm_b: return True - # After normalization the shorter name often becomes a prefix / - # substring of the longer one ("napoleon dynamite" ⊂ "napoleon - # dynamite music from the motion picture" before stripping). - if norm_a in norm_b or norm_b in norm_a: - return True + # No loose substring shortcut: after qualifier-stripping, a short name being a + # prefix of a longer one is usually a DIFFERENT edition carrying a real subtitle + # ("clair obscur expedition 33" ⊂ "clair obscur expedition 33 nos vies en lumiere"), + # not naming drift. Genuine drift collapses to an EXACT match above; everything else + # must clear a high overall-similarity bar. return SequenceMatcher(None, norm_a, norm_b).ratio() >= threshold diff --git a/tests/test_watchlist_album_match.py b/tests/test_watchlist_album_match.py index cbe9fbf2..2ededda9 100644 --- a/tests/test_watchlist_album_match.py +++ b/tests/test_watchlist_album_match.py @@ -156,6 +156,13 @@ def test_compilation_score_explanation() -> None: ("Inception (Music From The Motion Picture)", "Inception Soundtrack"), # Substring containment ("Random Access Memories", "Random Access Memories (Bonus Edition)"), + # Dash-suffixed qualifiers must still collapse — these are the SAME album, so + # treating them as different would re-wishlist/redownload forever (the failure the + # original blanket strip guarded against; the narrowed strip must keep covering it). + ("Album Name", "Album Name - Single"), + ("Album Name", "Album Name - Acoustic Version"), + ("Hotel California", "Hotel California - 2013 Remaster"), + ("Album", "Album - The Remixes"), ], ) def test_likely_match_positive(spotify_name, lib_name) -> None: @@ -176,12 +183,29 @@ def test_likely_match_positive(spotify_name, lib_name) -> None: ("Abbey Road", "Sgt. Pepper's Lonely Hearts Club Band"), # Same word in title but different album ("Greatest Hits Volume 1", "Greatest Hits Volume 2"), + # Sokhi: distinct editions of the same franchise — the OST vs a bonus edition + # with a real subtitle. USED to collapse to the same normalized name (the blanket + # trailing-dash strip removed "- Nos vies en Lumière"), so the watchlist marked + # unowned OST tracks as owned via the bonus edition. Must be DIFFERENT albums. + ("Clair Obscur: Expedition 33: Original Soundtrack", + "Clair Obscur: Expedition 33 - Nos vies en Lumière (Bonus Edition)"), + # A real subtitle after a dash must not be stripped down to the base name. + ("The Album", "The Album - A Whole Different Subtitle"), ], ) def test_likely_match_negative(spotify_name, lib_name) -> None: assert not _albums_likely_match(spotify_name, lib_name) +def test_real_subtitle_after_dash_is_preserved() -> None: + # the regression's root cause: a meaningful subtitle must survive normalization, + # while a recognized qualifier after a dash ("- Live", "- 2011") still collapses. + assert _normalize_album_for_match("Clair Obscur: Expedition 33 - Nos vies en Lumière") \ + != _normalize_album_for_match("Clair Obscur: Expedition 33") + assert _normalize_album_for_match("Some Album - Live") == _normalize_album_for_match("Some Album") + assert _normalize_album_for_match("Some Album - 2011") == _normalize_album_for_match("Some Album") + + # --------------------------------------------------------------------------- # _albums_likely_match — defensive cases # --------------------------------------------------------------------------- From b1f061a2a8e61377ba732d5ad25e7c3f42c25dbb Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Fri, 26 Jun 2026 21:45:41 -0700 Subject: [PATCH 6/7] manual search: float the pasted Qobuz/Tidal track to the top (#932) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit a pasted track link IS resolved + searched, but the 'bubble the exact track to the top' step read getattr(t,'id') — and TrackResult has no top-level id (the source id lives in _source_metadata['track_id']). so the bubble was a silent no-op: the linked track sat buried among fuzzy text-search lookalikes and the user saw unrelated tracks. qobuz made it worse — _qobuz_to_track_result never stamped _source_metadata at all, so the track had no id to match. - stamp _source_metadata={'source':'qobuz','track_id':...} on qobuz TrackResults (mirrors tidal) - extract the bubble into pure, tested helpers (linked_track_id / bubble_linked_track_first) that read _source_metadata['track_id'] — fixes it for tidal too, str/int-safe, stable no-op 19 track-link tests (+6 new) + 87 qobuz/download tests green. --- core/downloads/track_link.py | 24 +++++++++++++++- core/qobuz_client.py | 4 +++ tests/downloads/test_track_link.py | 44 ++++++++++++++++++++++++++++++ web_server.py | 12 ++++---- 4 files changed, 77 insertions(+), 7 deletions(-) diff --git a/core/downloads/track_link.py b/core/downloads/track_link.py index e6dec8bd..3565a5ff 100644 --- a/core/downloads/track_link.py +++ b/core/downloads/track_link.py @@ -13,9 +13,31 @@ Pure + import-safe: parsing only, no network. from __future__ import annotations import re -from typing import Any, Optional, Tuple +from typing import Any, List, Optional, Tuple from urllib.parse import urlparse + +def linked_track_id(track: Any) -> str: + """The source track id stamped on a search result, read from + ``_source_metadata['track_id']`` — the field every ID-downloadable source + (Tidal, Qobuz) records. Empty string when absent. ``TrackResult`` has no + top-level ``id``, so callers must NOT use ``getattr(t, 'id')`` (that always + missed and left the pasted-link bubble a silent no-op — #932).""" + meta = getattr(track, '_source_metadata', None) + if not isinstance(meta, dict): + return '' + return str(meta.get('track_id') or '') + + +def bubble_linked_track_first(tracks: List[Any], link_track_id: str) -> List[Any]: + """Float the result whose source id matches a pasted link to the top so the + user sees the EXACT track they linked, not a fuzzy text-search lookalike + (#813/#932). Stable + a graceful no-op when no result carries the id.""" + if not link_track_id or not tracks: + return tracks + target = str(link_track_id) + return sorted(tracks, key=lambda t: linked_track_id(t) != target) + # host substring → download source id. Only ID-downloadable streaming sources. _HOSTS = ( ('tidal.com', 'tidal'), diff --git a/core/qobuz_client.py b/core/qobuz_client.py index 1c7c3b6f..e90e934a 100644 --- a/core/qobuz_client.py +++ b/core/qobuz_client.py @@ -1074,6 +1074,10 @@ class QobuzClient(DownloadSourcePlugin): title=title, album=album_name, track_number=track.get('track_number'), + # Stamp the Qobuz track id so a pasted-link manual search can float the + # exact track to the top (#932). Without this the bubble never matched + # and the linked track stayed buried among fuzzy lookalikes. + _source_metadata={'source': 'qobuz', 'track_id': str(track.get('id') or '')}, ) # Stamp real API quality so the global ranker sees actual diff --git a/tests/downloads/test_track_link.py b/tests/downloads/test_track_link.py index 39dfe8a1..06cbc1bb 100644 --- a/tests/downloads/test_track_link.py +++ b/tests/downloads/test_track_link.py @@ -78,3 +78,47 @@ def test_payload_non_dict_or_empty(): assert q('tidal', None) is None assert q('tidal', {}) is None assert q('qobuz', 'garbage') is None + + +# ── bubble the pasted-link track to the top (#932) ── + +from types import SimpleNamespace + +from core.downloads.track_link import linked_track_id, bubble_linked_track_first + + +def _result(track_id=None): + """Mimic a TrackResult: NO top-level `id`, the source id lives in + _source_metadata['track_id'] (or absent entirely).""" + meta = {'source': 'qobuz', 'track_id': track_id} if track_id is not None else None + return SimpleNamespace(_source_metadata=meta, title='t') + + +def test_linked_track_id_reads_source_metadata(): + assert linked_track_id(_result('296427754')) == '296427754' + + +def test_linked_track_id_empty_when_absent(): + # the exact #932 trap: there is no top-level `id`, so getattr(t,'id') would miss. + r = _result() + assert not hasattr(r, 'id') + assert linked_track_id(r) == '' + + +def test_bubble_floats_exact_track_to_top(): + fuzzy_a, exact, fuzzy_b = _result('111'), _result('296427754'), _result('222') + out = bubble_linked_track_first([fuzzy_a, exact, fuzzy_b], '296427754') + assert out[0] is exact # exact track surfaced first + assert out[1:] == [fuzzy_a, fuzzy_b] # stable order for the rest + + +def test_bubble_handles_int_vs_str_id(): + out = bubble_linked_track_first([_result('999'), _result('296427754')], 296427754) + assert linked_track_id(out[0]) == '296427754' + + +def test_bubble_noop_when_nothing_matches_or_empty(): + a, b = _result('1'), _result('2') + assert bubble_linked_track_first([a, b], '296427754') == [a, b] # unchanged + assert bubble_linked_track_first([], '296427754') == [] + assert bubble_linked_track_first([a, b], '') == [a, b] diff --git a/web_server.py b/web_server.py index b87f2ba7..19f9bc52 100644 --- a/web_server.py +++ b/web_server.py @@ -7533,13 +7533,13 @@ def manual_search_for_task(task_id): "error": error, }) + "\n" continue - # Pasted-link exact match: bubble the track whose id matches - # the link to the top so the user sees the exact version - # first (graceful no-op if ids don't line up). + # Pasted-link exact match: bubble the track whose source id + # matches the link to the top so the user sees the exact version + # first. Reads _source_metadata['track_id'] (TrackResult has no + # top-level id) — the old getattr(t,'id') always missed (#932). if src_name == link_source and link_track_id and tracks: - tracks = sorted( - tracks, - key=lambda t: str(getattr(t, 'id', '')) != str(link_track_id)) + from core.downloads.track_link import bubble_linked_track_first + tracks = bubble_linked_track_first(tracks, link_track_id) serialized = [] for t in tracks: s = _serialize_candidate(t, source_override=src_name) From eddaea2f93a22384f66cc6b66ff3e4f96daca5a8 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Sat, 27 Jun 2026 00:37:35 -0700 Subject: [PATCH 7/7] watchlist history: record automatic scans too (#933) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit save_watchlist_scan_run had a single caller — the manual scan endpoint. the automatic/ scheduled path (process_watchlist_scan_automatically) ran the full scan but never wrote a history row, so nightly scans never showed up in the History modal — only manual ones. - new shared helper persist_scan_run(database, state, ...) extracts the run from the finished watchlist_scan_state and writes one history row - the automatic path now stamps scan_run_id/scan_track_events and calls it - the manual path is refactored onto the same helper so the two can't drift apart again - history is global (no profile filter), so the all-profiles nightly scan records one aggregate row (profile_id None → 1, never NULL) tests: 4 new persist_scan_run seam tests (real DB) + 2 new auto-scan integration tests proving the auto path actually records (completed + cancelled, exactly once). 420 watchlist/automation tests green. --- core/watchlist/auto_scan.py | 17 ++++++++- core/watchlist/scan_history.py | 51 +++++++++++++++++++++++++ tests/test_watchlist_scan_history.py | 56 ++++++++++++++++++++++++++++ tests/watchlist/test_auto_scan.py | 38 +++++++++++++++++++ web_server.py | 20 ++-------- 5 files changed, 165 insertions(+), 17 deletions(-) create mode 100644 core/watchlist/scan_history.py diff --git a/core/watchlist/auto_scan.py b/core/watchlist/auto_scan.py index 759d43f7..e26ab998 100644 --- a/core/watchlist/auto_scan.py +++ b/core/watchlist/auto_scan.py @@ -185,7 +185,11 @@ def process_watchlist_scan_automatically(automation_id=None, profile_id=None, de 'results': [], 'summary': {}, 'error': None, - 'cancel_requested': False + 'cancel_requested': False, + # #933: stamp these so this scan lands in the History modal too — + # the scanner fills scan_track_events; persist_scan_run reads both. + 'scan_run_id': datetime.now().strftime('%Y%m%d-%H%M%S'), + 'scan_track_events': [], } scan_results = [] @@ -294,6 +298,17 @@ def process_watchlist_scan_automatically(automation_id=None, profile_id=None, de total_added_to_wishlist = deps.watchlist_scan_state.get('summary', {}).get('tracks_added_to_wishlist', 0) logger.warning("Automatic watchlist scan cancelled — skipping post-scan steps") + # #933: record this run in the History ledger — same helper the manual + # scan uses, so scheduled scans show up alongside manual ones. + try: + from core.watchlist.scan_history import persist_scan_run + persist_scan_run( + database, deps.watchlist_scan_state, + profile_id=profile_id, was_cancelled=was_cancelled, + ) + except Exception as _hist_err: + logger.error(f"Failed to persist watchlist scan run: {_hist_err}") + # Post-scan steps — skip if cancelled if not was_cancelled: # Populate discovery pool from similar artists (per-profile) diff --git a/core/watchlist/scan_history.py b/core/watchlist/scan_history.py new file mode 100644 index 00000000..a87011ca --- /dev/null +++ b/core/watchlist/scan_history.py @@ -0,0 +1,51 @@ +"""Persist a finished watchlist scan to the History ledger (#831 / #933). + +Both the manual scan (``web_server.start_watchlist_scan``) and the automatic +scan (``core.watchlist.auto_scan.process_watchlist_scan_automatically``) finish +with the same ``watchlist_scan_state`` shape, but only the manual path used to +record a history row — so scheduled/nightly scans never showed up in the +History modal (#933). This single helper is the shared seam: both paths call it, +so they can't drift apart again. + +Pure except for the one ``database.save_watchlist_scan_run`` call — the field +extraction is unit-testable with a fake database. +""" + +from __future__ import annotations + +from datetime import datetime +from typing import Any, Dict, Optional + + +def _iso(value: Any) -> Optional[str]: + """ISO-format a datetime; pass through an already-stringified timestamp.""" + if value is None: + return None + return value.isoformat() if hasattr(value, 'isoformat') else str(value) + + +def persist_scan_run(database: Any, state: Dict[str, Any], *, + profile_id: Any, was_cancelled: bool) -> bool: + """Record one watchlist scan run + its track ledger from ``state``. + + Reads the counts/timestamps/ledger off the live ``watchlist_scan_state`` the + scanner just finished writing, and writes a single history row. ``run_id`` + comes from ``state['scan_run_id']`` (both paths stamp it); a timestamp + fallback keeps it from ever colliding if that's somehow missing. Returns the + DB call's truthiness; callers wrap in their own try/except so a history-write + failure never breaks the scan. + """ + summary = state.get('summary') or {} + run_id = state.get('scan_run_id') or datetime.now().strftime('%Y%m%d-%H%M%S') + return database.save_watchlist_scan_run( + run_id=run_id, + profile_id=profile_id if profile_id else 1, + status='cancelled' if was_cancelled else 'completed', + started_at=_iso(state.get('started_at')), + completed_at=_iso(state.get('completed_at')) or datetime.now().isoformat(), + total_artists=summary.get('total_artists', state.get('total_artists', 0)), + artists_scanned=summary.get('successful_scans', 0), + tracks_found=state.get('tracks_found_this_scan', 0), + tracks_added=state.get('tracks_added_this_scan', 0), + track_events=state.get('scan_track_events') or [], + ) diff --git a/tests/test_watchlist_scan_history.py b/tests/test_watchlist_scan_history.py index 3b6f4924..7281e230 100644 --- a/tests/test_watchlist_scan_history.py +++ b/tests/test_watchlist_scan_history.py @@ -47,6 +47,62 @@ def test_resave_is_idempotent_on_run_id(db): assert len(runs) == 1 and runs[0]['tracks_added'] == 11 +# ── persist_scan_run: the shared seam both scan paths use (#933) ── + +from datetime import datetime + +from core.watchlist.scan_history import persist_scan_run + + +def _state(**over): + """A watchlist_scan_state as the scanner leaves it when a scan finishes.""" + s = { + 'scan_run_id': 'auto-run-1', + 'started_at': datetime(2026, 6, 26, 2, 0, 0), + 'completed_at': datetime(2026, 6, 26, 2, 4, 0), + 'total_artists': 40, + 'tracks_found_this_scan': 7, + 'tracks_added_this_scan': 3, + 'scan_track_events': _events(n_added=3, n_skipped=1), + 'summary': {'total_artists': 40, 'successful_scans': 40, + 'new_tracks_found': 7, 'tracks_added_to_wishlist': 3}, + } + s.update(over) + return s + + +def test_persist_scan_run_records_a_history_row(db): + # the #933 fix: an automatic (all-profiles → profile_id=None) scan must land in History. + assert persist_scan_run(db, _state(), profile_id=None, was_cancelled=False) is True + runs = db.get_watchlist_scan_runs() + assert len(runs) == 1 + r = runs[0] + assert (r['run_id'], r['status'], r['tracks_found'], r['tracks_added']) == \ + ('auto-run-1', 'completed', 7, 3) + assert r['profile_id'] == 1 # None coerced to a concrete profile, never NULL + # the per-run ledger came through too + assert [e['status'] for e in db.get_watchlist_scan_run_events('auto-run-1')] == \ + ['added', 'added', 'added', 'skipped'] + + +def test_persist_scan_run_cancelled_status(db): + persist_scan_run(db, _state(scan_run_id='c1'), profile_id=2, was_cancelled=True) + assert db.get_watchlist_scan_runs()[0]['status'] == 'cancelled' + + +def test_persist_scan_run_accepts_datetime_or_iso_string(db): + # state timestamps may be datetime (auto path) or already-iso strings — both must persist. + persist_scan_run(db, _state(scan_run_id='dt', completed_at='2026-06-26T02:09:00'), + profile_id=1, was_cancelled=False) + assert db.get_watchlist_scan_runs()[0]['run_id'] == 'dt' + + +def test_persist_scan_run_tolerates_sparse_state(db): + # a bare/early-finished state must not raise — history-write must never break a scan. + assert persist_scan_run(db, {'scan_run_id': 'sparse'}, profile_id=None, was_cancelled=False) + assert db.get_watchlist_scan_runs()[0]['tracks_added'] == 0 + + def test_prune_keeps_most_recent(db): for i in range(1, 8): db.save_watchlist_scan_run( diff --git a/tests/watchlist/test_auto_scan.py b/tests/watchlist/test_auto_scan.py index 4ad70258..6ac83413 100644 --- a/tests/watchlist/test_auto_scan.py +++ b/tests/watchlist/test_auto_scan.py @@ -81,6 +81,11 @@ class _FakeDB: self._watchlist_artists = watchlist_artists or [] self._lb_profiles = lb_profiles or [] self.database_path = '/tmp/test.db' + self.scan_runs_saved = [] + + def save_watchlist_scan_run(self, **kwargs): + self.scan_runs_saved.append(kwargs) + return True def get_all_profiles(self): return self._profiles @@ -237,6 +242,39 @@ def test_successful_scan_runs_post_steps(patched_modules): assert deps._state_ref[0]['summary']['new_tracks_found'] == 2 +def test_successful_scan_records_history_run(patched_modules): + """#933: the automatic scan must persist a History row (it used to skip this, + so only manual scans appeared in History).""" + scanner, db = patched_modules + deps = _build_deps() + + autosc.process_watchlist_scan_automatically(automation_id='a1', deps=deps) + + assert len(db.scan_runs_saved) == 1 # exactly one row, no double-record + saved = db.scan_runs_saved[0] + assert saved['status'] == 'completed' + assert saved['artists_scanned'] == 1 # one successful _ScanResult + assert saved['run_id'] # a run id was stamped + + +def test_cancelled_scan_still_records_history_run(patched_modules, monkeypatch): + """A cancelled scan is recorded too, with status='cancelled'.""" + scanner, db = patched_modules + deps = _build_deps() + # Make the scan observe a cancel request mid-run. + orig = scanner.scan_watchlist_artists + def _cancel_during(artists, *, scan_state, progress_callback, cancel_check): + scan_state['cancel_requested'] = True + return orig(artists, scan_state=scan_state, progress_callback=progress_callback, + cancel_check=cancel_check) + monkeypatch.setattr(scanner, 'scan_watchlist_artists', _cancel_during) + + autosc.process_watchlist_scan_automatically(automation_id='a1', deps=deps) + + assert len(db.scan_runs_saved) == 1 + assert db.scan_runs_saved[0]['status'] == 'cancelled' + + def test_completion_emits_automation_event(patched_modules): """Successful scan emits 'watchlist_scan_completed' on automation_engine.""" scanner, _ = patched_modules diff --git a/web_server.py b/web_server.py index 19f9bc52..09614540 100644 --- a/web_server.py +++ b/web_server.py @@ -28223,22 +28223,10 @@ def start_watchlist_scan(): # #831 round 2: persist this run + its track ledger so the # Watchlist History modal can show what every past scan did. try: - _state = watchlist_scan_state - get_database().save_watchlist_scan_run( - run_id=_state.get('scan_run_id') or datetime.now().strftime('%Y%m%d-%H%M%S'), - profile_id=scan_profile_id, - status='cancelled' if was_cancelled else 'completed', - started_at=(_state.get('started_at').isoformat() - if _state.get('started_at') else None), - completed_at=(_state.get('completed_at') or datetime.now()).isoformat() - if not isinstance(_state.get('completed_at'), str) - else _state.get('completed_at'), - total_artists=(_state.get('summary') or {}).get('total_artists', - _state.get('total_artists', 0)), - artists_scanned=(_state.get('summary') or {}).get('successful_scans', 0), - tracks_found=_state.get('tracks_found_this_scan', 0), - tracks_added=_state.get('tracks_added_this_scan', 0), - track_events=_state.get('scan_track_events') or [], + from core.watchlist.scan_history import persist_scan_run + persist_scan_run( + get_database(), watchlist_scan_state, + profile_id=scan_profile_id, was_cancelled=was_cancelled, ) except Exception as _hist_err: logger.error(f"Failed to persist watchlist scan run: {_hist_err}")