diff --git a/core/discovery/manual_match.py b/core/discovery/manual_match.py index 8c56bbdf..02179ca0 100644 --- a/core/discovery/manual_match.py +++ b/core/discovery/manual_match.py @@ -16,6 +16,10 @@ to test in isolation: overwrites the user's deliberate pick with whatever the auto-search ranks first, so manual matches are exempt regardless of provider drift. `is_drifted_for_redo` encapsulates the decision. + +3. *Should the Playlist Pipeline pre-scan (re)discover this track at all?* + — `should_rediscover` encapsulates that gate, with the manual match + checked FIRST so a leftover Wing It flag can't override the user's pick. """ from __future__ import annotations @@ -68,3 +72,49 @@ def is_drifted_for_redo( return False cached_provider = extra_data.get('provider', 'spotify') return cached_provider != active_provider + + +def should_rediscover(extra_data: Optional[Dict[str, Any]]) -> bool: + """Return True when a mirrored track needs (re)discovery, False to skip it. + + This is the gate the Playlist Pipeline pre-scan runs over every mirrored + track before discovering. The **ordering is the fix**: a manual match is + authoritative and is checked FIRST. + + ``extra_data`` is *merged* on save (see ``update_mirrored_track_extra_data``), + so a track that was a Wing It stub and is then manually fixed still carries + ``wing_it_fallback: True`` alongside the new ``manual_match: True``. The old + pre-scan tested ``wing_it_fallback`` before ``manual_match``, so the stale + flag won and the pipeline re-discovered the track — silently reverting the + user's pick to Wing It. Checking ``manual_match`` first makes the fix stick. + + Decision order: + * manual_match -> skip (authoritative; never re-discover) + * wing_it_fallback -> redo (stub — keep trying for a real match) + * discovered + complete -> skip (full metadata already stored) + * discovered + incomplete -> redo (backfill track_number / album fields) + * unmatched_by_user -> skip (user deliberately removed the match) + * never discovered -> redo (first-time discovery) + """ + extra = extra_data if isinstance(extra_data, dict) else {} + + if extra.get('discovered'): + if extra.get('manual_match'): + return False + if extra.get('wing_it_fallback'): + return True + # Otherwise re-discover only when the stored match is missing the + # enriched fields (track_number + release_date/album.id) that older + # discoveries dropped via the Track dataclass. + matched = extra.get('matched_data') + matched = matched if isinstance(matched, dict) else {} + album = matched.get('album') + album = album if isinstance(album, dict) else {} + has_track_num = matched.get('track_number') + has_release = album.get('release_date') + has_album_id = album.get('id') + return not (has_track_num and (has_release or has_album_id)) + + if extra.get('unmatched_by_user'): + return False + return True diff --git a/core/discovery/playlist.py b/core/discovery/playlist.py index 14af5c29..0e71ec85 100644 --- a/core/discovery/playlist.py +++ b/core/discovery/playlist.py @@ -37,6 +37,8 @@ import time from dataclasses import dataclass from typing import Any, Callable +from core.discovery.manual_match import should_rediscover + logger = logging.getLogger(__name__) @@ -145,44 +147,14 @@ def run_playlist_discovery_worker(playlists, automation_id=None, deps: PlaylistD existing_extra = json.loads(track['extra_data']) if isinstance(track['extra_data'], str) else track['extra_data'] except (json.JSONDecodeError, TypeError): pass - if existing_extra.get('discovered'): - if existing_extra.get('wing_it_fallback'): - # Wing It stub — always re-attempt to find a real match - undiscovered_tracks.append(track) - elif existing_extra.get('manual_match'): - # User explicitly picked this match via the Fix popup. - # Manual fixes are authoritative: they may lack - # track_number / album.id / release_date (the Fix-popup - # save shape is intentionally lean — search-result rows - # don't include track_number, and the MBID-lookup flat - # shape doesn't carry album.id), but re-running discovery - # against the active source would overwrite the user's - # deliberate pick with whatever the auto-search ranks - # first. Skip — pipeline only re-discovers when the user - # has cleared the match. - pl_skipped += 1 - total_skipped += 1 - else: - # Check if matched_data is complete — old discoveries may be missing - # track_number/release_date due to the Track dataclass stripping them. - # Re-discover these so the enriched pipeline fills in the gaps. - md = existing_extra.get('matched_data', {}) - album = md.get('album', {}) - has_track_num = md.get('track_number') - has_release = album.get('release_date') if isinstance(album, dict) else None - has_album_id = album.get('id') if isinstance(album, dict) else None - if has_track_num and (has_release or has_album_id): - pl_skipped += 1 - total_skipped += 1 - else: - # Incomplete discovery — re-discover to get full metadata - undiscovered_tracks.append(track) - elif existing_extra.get('unmatched_by_user'): - # User explicitly removed this match — respect their choice + # `should_rediscover` is the single source of truth for this + # gate (manual match checked FIRST so a stale Wing It flag can't + # revert a user's deliberate fix — see its docstring). + if should_rediscover(existing_extra): + undiscovered_tracks.append(track) + else: pl_skipped += 1 total_skipped += 1 - else: - undiscovered_tracks.append(track) if pl_skipped > 0: deps.update_automation_progress(automation_id, diff --git a/tests/discovery/test_manual_match.py b/tests/discovery/test_manual_match.py index 8bc34b09..0ad21f36 100644 --- a/tests/discovery/test_manual_match.py +++ b/tests/discovery/test_manual_match.py @@ -10,6 +10,7 @@ AST-parsing the route file). from core.discovery.manual_match import ( derive_manual_match_provider, is_drifted_for_redo, + should_rediscover, ) @@ -104,3 +105,73 @@ def test_drift_default_provider_is_spotify_when_absent(): extra = {'discovered': True} # no provider field assert is_drifted_for_redo(extra, 'spotify') is False assert is_drifted_for_redo(extra, 'musicbrainz') is True + + +# --------------------------------------------------------------------------- +# should_rediscover — the Playlist Pipeline pre-scan gate +# --------------------------------------------------------------------------- + + +def test_rediscovers_never_discovered_track(): + assert should_rediscover({}) is True + assert should_rediscover(None) is True + + +def test_skips_complete_discovery(): + extra = { + 'discovered': True, + 'matched_data': {'track_number': 3, 'album': {'release_date': '2020'}}, + } + assert should_rediscover(extra) is False + + +def test_rediscovers_incomplete_discovery(): + # Missing track_number / release_date / album.id — re-discover to backfill. + extra = {'discovered': True, 'matched_data': {'name': 'X'}} + assert should_rediscover(extra) is True + + +def test_album_id_satisfies_completeness(): + extra = { + 'discovered': True, + 'matched_data': {'track_number': 1, 'album': {'id': 'al-1'}}, + } + assert should_rediscover(extra) is False + + +def test_rediscovers_wing_it_stub(): + extra = {'discovered': True, 'wing_it_fallback': True} + assert should_rediscover(extra) is True + + +def test_skips_manual_match(): + extra = {'discovered': True, 'manual_match': True} + assert should_rediscover(extra) is False + + +def test_skips_unmatched_by_user(): + extra = {'unmatched_by_user': True} + assert should_rediscover(extra) is False + + +def test_regression_manual_match_wins_over_stale_wing_it_flag(): + """The #799 revert bug: extra_data is MERGED on save, so a track fixed + after being a Wing It stub still carries wing_it_fallback=True alongside + the new manual_match=True. The manual match MUST win — otherwise the + pipeline re-discovers and silently reverts the user's pick to Wing It. + + Before the fix the pre-scan checked wing_it_fallback first and returned + True (re-discover). It must now skip.""" + extra = { + 'discovered': True, + 'wing_it_fallback': True, # stale flag left by the merge + 'manual_match': True, # the user's authoritative fix + 'matched_data': {'name': 'The Real Match'}, + } + assert should_rediscover(extra) is False + + +def test_manual_match_wins_even_without_other_fields(): + # Lean Fix-popup save shape (no track_number/album) must still be honored. + extra = {'discovered': True, 'manual_match': True, 'wing_it_fallback': True} + assert should_rediscover(extra) is False diff --git a/web_server.py b/web_server.py index 39fa71dc..77da23ad 100644 --- a/web_server.py +++ b/web_server.py @@ -23222,6 +23222,12 @@ def update_youtube_discovery_match(): 'confidence': 1.0, 'matched_data': matched_data, 'manual_match': True, + # extra_data is MERGED on save, so explicitly clear + # any stale stub/removal flags from before this fix — + # otherwise a leftover wing_it_fallback would make the + # pipeline re-discover and revert this manual pick. + 'wing_it_fallback': False, + 'unmatched_by_user': False, } db.update_mirrored_track_extra_data(db_track_id, extra_data) result['matched_data'] = matched_data