#799: stop mirrored discovery from reverting a manual fix to Wing It
A manually-fixed mirrored track silently reverted to 'Wing It' after re-running discovery. Two compounding causes: - extra_data is MERGED on save (update_mirrored_track_extra_data), and the manual-fix DB write (web_server.py) didn't clear the prior wing_it_fallback flag — so a track fixed after being a Wing It stub kept wing_it_fallback=True. - the Playlist Pipeline pre-scan checked wing_it_fallback BEFORE manual_match (if/elif), so the stale flag won: the track was re-discovered and, on a miss, fell back to Wing It — discarding the user's pick. Fix: extracted the pre-scan gate into core.discovery.manual_match.should_rediscover (manual_match checked FIRST = authoritative, regardless of leftover flags), and the manual-fix write now also clears wing_it_fallback/unmatched_by_user. Behavior is identical for every other branch — only the manual-vs-wing-it ordering changes. Tested at the seam incl. the exact regression (wing_it_fallback + manual_match both set -> skip). 227 discovery tests green.
This commit is contained in:
parent
55c9b52aee
commit
c20150e370
4 changed files with 135 additions and 36 deletions
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Reference in a new issue