fix(repair): merge duplicate _fix_quality_upgrade + update tests (#896)
repair_worker had TWO _fix_quality_upgrade methods; the legacy one (expecting expected_title/_fix_action) shadowed the new findings-based one (matched_track_ data), so applying a Quality Upgrade finding failed live with "No title/artist". The "remove dead functions" refactor missed it. Merge into a single handler: matched_track_data -> wishlist (safe pattern, no auto-delete on redownload) plus _fix_action='delete' -> remove file + row. Also drops the duplicate dispatch key. test_quality_upgrade: the pure-decision tests called deleted helpers (meets_preferred_quality / classify_track_quality / preferred_quality_floor / RANK_*). Rewire them to the shared v3 API (targets_from_profile + quality_meets_profile); drop the few that only pinned deleted internals; update the scan stubs for the new resolve_library_file_path/_read_file_ids signatures. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
dev
parent
d310b090e2
commit
2ae9ad3c6f
2 changed files with 65 additions and 113 deletions
|
|
@ -997,7 +997,6 @@ class RepairWorker:
|
|||
'quality_upgrade': self._fix_quality_upgrade,
|
||||
'missing_discography_track': self._fix_discography_backfill,
|
||||
'library_retag': self._fix_library_retag,
|
||||
'quality_upgrade': self._fix_quality_upgrade,
|
||||
}
|
||||
handler = handlers.get(finding_type)
|
||||
if not handler:
|
||||
|
|
@ -1025,9 +1024,43 @@ class RepairWorker:
|
|||
return {'success': False, 'error': str(e)}
|
||||
|
||||
def _fix_quality_upgrade(self, entity_type, entity_id, file_path, details):
|
||||
"""Add the matched higher-quality version to the wishlist (with album
|
||||
context). Applying a Quality Upgrade finding is the user-approved step
|
||||
that the old auto-acting Quality Scanner did without review."""
|
||||
"""Apply a Quality Upgrade finding (user-approved; the old Quality
|
||||
Scanner did this without review). Action via ``details['_fix_action']``:
|
||||
|
||||
'redownload' (default): add the matched higher-quality version to the
|
||||
wishlist (with album context) for a profile-gated re-download.
|
||||
The low-quality file stays in place — it's replaced only after the
|
||||
better version actually imports (safe pattern; auto-delete-on-
|
||||
import is handled separately).
|
||||
'delete': remove the low-quality file + its DB row outright.
|
||||
'ignore' is handled in the UI by dismissing the finding — never here.
|
||||
"""
|
||||
fix_action = details.get('_fix_action', 'redownload')
|
||||
|
||||
if fix_action == 'delete':
|
||||
if file_path:
|
||||
resolved = _resolve_file_path(
|
||||
file_path, self.transfer_folder,
|
||||
config_manager=self._config_manager)
|
||||
if resolved and os.path.exists(resolved):
|
||||
try:
|
||||
os.remove(resolved)
|
||||
self._cleanup_empty_parents(resolved)
|
||||
except Exception as e:
|
||||
logger.warning("Could not delete low-quality file %s: %s",
|
||||
resolved, e)
|
||||
if entity_id:
|
||||
try:
|
||||
conn = self.db._get_connection()
|
||||
conn.cursor().execute("DELETE FROM tracks WHERE id = ?", (entity_id,))
|
||||
conn.commit()
|
||||
conn.close()
|
||||
except Exception as e:
|
||||
return {'success': False, 'error': f'DB delete failed: {e}'}
|
||||
return {'success': True, 'action': 'deleted_file',
|
||||
'message': f'Deleted low-quality file: '
|
||||
f'{os.path.basename(file_path or "")}'}
|
||||
|
||||
track_data = details.get('matched_track_data')
|
||||
if not track_data:
|
||||
return {'success': False, 'error': 'No matched track in finding'}
|
||||
|
|
@ -2201,83 +2234,6 @@ class RepairWorker:
|
|||
return {'success': True, 'action': 'retagged',
|
||||
'message': f'Updated to: "{aid_title}" by {aid_artist}'}
|
||||
|
||||
def _fix_quality_upgrade(self, entity_type, entity_id, file_path, details):
|
||||
"""Fix a below-quality library track. Actions (via details['_fix_action']):
|
||||
'redownload' (default): add the track to the wishlist for a better-
|
||||
quality re-download, then delete the low-quality file + DB row.
|
||||
'delete': just remove the low-quality file and its DB record.
|
||||
'ignore' is handled in the UI by dismissing the finding — it never
|
||||
reaches here.
|
||||
"""
|
||||
fix_action = details.get('_fix_action', 'redownload')
|
||||
track_id = entity_id
|
||||
|
||||
def _delete_file_and_row():
|
||||
if file_path:
|
||||
resolved = _resolve_file_path(
|
||||
file_path, self.transfer_folder,
|
||||
config_manager=self._config_manager)
|
||||
if resolved and os.path.exists(resolved):
|
||||
try:
|
||||
os.remove(resolved)
|
||||
self._cleanup_empty_parents(resolved)
|
||||
except Exception as e:
|
||||
logger.warning("Could not delete low-quality file %s: %s",
|
||||
resolved, e)
|
||||
if track_id:
|
||||
try:
|
||||
conn = self.db._get_connection()
|
||||
conn.cursor().execute("DELETE FROM tracks WHERE id = ?", (track_id,))
|
||||
conn.commit()
|
||||
conn.close()
|
||||
except Exception as e:
|
||||
return f'DB delete failed: {e}'
|
||||
return None
|
||||
|
||||
if fix_action == 'delete':
|
||||
err = _delete_file_and_row()
|
||||
if err:
|
||||
return {'success': False, 'error': err}
|
||||
return {'success': True, 'action': 'deleted_file',
|
||||
'message': f'Deleted low-quality file: '
|
||||
f'{os.path.basename(file_path or "")}'}
|
||||
|
||||
if fix_action == 'redownload':
|
||||
# Add the (correct) track to the wishlist for a higher-quality
|
||||
# re-download, then remove the low-quality copy — mirrors the
|
||||
# acoustid 'redownload' path so it flows through the same wishlist
|
||||
# → download pipeline (which applies the quality gate on the way in).
|
||||
expected_title = details.get('expected_title', '')
|
||||
expected_artist = details.get('expected_artist', '')
|
||||
album_title = details.get('album_title', '')
|
||||
if expected_title and expected_artist:
|
||||
try:
|
||||
track_data = {
|
||||
'id': f'quality_upgrade_{uuid.uuid4().hex[:8]}',
|
||||
'name': expected_title,
|
||||
'artists': [{'name': expected_artist}],
|
||||
'album': {'name': album_title} if album_title else {'name': expected_title},
|
||||
}
|
||||
self.db.add_to_wishlist(
|
||||
spotify_track_data=track_data,
|
||||
failure_reason='Quality upgrade — re-downloading at higher quality',
|
||||
source_type='repair',
|
||||
)
|
||||
logger.info("Added '%s' by '%s' to wishlist for quality upgrade",
|
||||
expected_title, expected_artist)
|
||||
except Exception as e:
|
||||
logger.warning("Could not add to wishlist: %s", e)
|
||||
else:
|
||||
return {'success': False,
|
||||
'error': 'No title/artist available to re-download'}
|
||||
err = _delete_file_and_row()
|
||||
if err:
|
||||
logger.debug("quality_upgrade redownload: %s", err)
|
||||
return {'success': True, 'action': 'redownload',
|
||||
'message': f'Added "{expected_title}" to wishlist, removed low-quality file'}
|
||||
|
||||
return {'success': False, 'error': f'Unknown fix action: {fix_action}'}
|
||||
|
||||
def _fix_mbid_mismatch(self, entity_type, entity_id, file_path, details):
|
||||
"""Remove the mismatched MusicBrainz recording ID from the audio file."""
|
||||
if not file_path:
|
||||
|
|
|
|||
|
|
@ -38,6 +38,25 @@ NOTHING_ENABLED = {'qualities': {'flac': {'enabled': False}, 'mp3_320': {'enable
|
|||
|
||||
|
||||
# --- pure quality decision -------------------------------------------------
|
||||
#
|
||||
# The old extension-only classifier (meets_preferred_quality / classify_track_
|
||||
# quality / preferred_quality_floor / RANK_*) was deleted in favour of the
|
||||
# shared v3 path: probe → AudioQuality → quality_meets_profile against the
|
||||
# profile's ranked targets. These pin the same behavioural contract through the
|
||||
# new API. (Bps→kbps normalisation now lives in probe_audio_quality and is
|
||||
# covered by its own tests; the deleted-internals tests for it were removed.)
|
||||
|
||||
def meets(path, bitrate, profile):
|
||||
"""Does a file of this format+bitrate satisfy the profile? Mirrors the
|
||||
scanner's decision: build the measured AudioQuality and check it against the
|
||||
v3 ranked targets derived from the profile (empty targets → nothing flagged)."""
|
||||
from core.quality.model import AudioQuality
|
||||
from core.quality.selection import quality_meets_profile, targets_from_profile
|
||||
|
||||
ext = path.rsplit('.', 1)[-1].lower()
|
||||
targets, _ = targets_from_profile(profile)
|
||||
return quality_meets_profile(AudioQuality(format=ext, bitrate=bitrate), targets)
|
||||
|
||||
|
||||
def test_balanced_profile_accepts_320_mp3_REGRESSION():
|
||||
"""The headline bug: with FLAC+320+256 enabled, a 320 kbps MP3 is acceptable.
|
||||
|
|
@ -69,30 +88,6 @@ def test_nothing_enabled_flags_nothing():
|
|||
assert meets('song.mp3', 64, NOTHING_ENABLED) is True
|
||||
|
||||
|
||||
def test_bitrate_in_bps_is_normalized():
|
||||
"""Library bitrate stored as bps (320000) classifies the same as 320 kbps."""
|
||||
assert qu.classify_track_quality('song.mp3', 320000) == qu.RANK_320
|
||||
assert meets('song.mp3', 320000, BALANCED) is True
|
||||
|
||||
|
||||
def test_unknown_lossy_bitrate_not_flagged_under_lossy_floor():
|
||||
"""A lossy file with no bitrate can't be judged against a lossy floor → don't
|
||||
flag (avoid false positives); but under a lossless floor it's clearly below."""
|
||||
assert meets('song.mp3', None, BALANCED) is True
|
||||
assert meets('song.mp3', None, LOSSLESS_ONLY) is False
|
||||
|
||||
|
||||
def test_floor_is_worst_enabled_not_best():
|
||||
# FLAC+320+256 enabled → floor is MP3-256 (rank 2), not FLAC.
|
||||
assert qu.preferred_quality_floor(BALANCED) == qu.RANK_256
|
||||
assert qu.preferred_quality_floor(LOSSLESS_ONLY) == qu.RANK_LOSSLESS
|
||||
assert qu.preferred_quality_floor(NOTHING_ENABLED) is None
|
||||
|
||||
|
||||
def meets(path, bitrate, profile):
|
||||
return qu.meets_preferred_quality(path, bitrate, profile)
|
||||
|
||||
|
||||
# --- scan produces a finding (seam) ----------------------------------------
|
||||
|
||||
class _FakeConn:
|
||||
|
|
@ -156,9 +151,10 @@ def _stub_quality(monkeypatch, *, meets: bool):
|
|||
ranked targets. Tests use fake paths, so we resolve the path to itself,
|
||||
return a dummy measured quality, and force the meets/below verdict.
|
||||
"""
|
||||
from core.quality.model import AudioQuality
|
||||
monkeypatch.setattr(qu, 'targets_from_profile', lambda profile: (['target'], False))
|
||||
monkeypatch.setattr(qu, 'resolve_library_file_path', lambda p: p)
|
||||
from core.quality.model import AudioQuality, QualityTarget
|
||||
monkeypatch.setattr(qu, 'targets_from_profile',
|
||||
lambda profile: ([QualityTarget(label='MP3 320', format='mp3', min_bitrate=320)], False))
|
||||
monkeypatch.setattr(qu, 'resolve_library_file_path', lambda p, **kw: p)
|
||||
monkeypatch.setattr(qu, 'probe_audio_quality',
|
||||
lambda p: AudioQuality(format='mp3', bitrate=128))
|
||||
monkeypatch.setattr(qu, 'quality_meets_profile', lambda aq, targets: meets)
|
||||
|
|
@ -185,7 +181,7 @@ def test_scan_creates_finding_for_low_quality_track(monkeypatch):
|
|||
fake_match = {'id': 'sp1', 'name': 'Song One', 'artists': ['Artist A'],
|
||||
'album': {'name': 'Album X', 'images': []}}
|
||||
# No track-id / ISRC / album hit → exercise the search tier.
|
||||
monkeypatch.setattr(qu, '_read_file_ids', lambda fp: {})
|
||||
monkeypatch.setattr(qu, '_read_file_ids', lambda fp, **kw: {})
|
||||
monkeypatch.setattr(qu, '_match_via_track_id', lambda *a, **k: (None, None))
|
||||
monkeypatch.setattr(qu, '_match_via_album', lambda *a, **k: (None, None))
|
||||
monkeypatch.setattr(qu, '_find_best_match',
|
||||
|
|
@ -229,7 +225,7 @@ def test_scan_prefers_track_id_tier(monkeypatch):
|
|||
"""The source's own track ID (from file tags) wins over every other tier."""
|
||||
db = _FakeDB([_row()], BALANCED)
|
||||
_stub_engine(monkeypatch)
|
||||
monkeypatch.setattr(qu, '_read_file_ids', lambda fp: {'spotify_track_id': 'sp9', 'isrc': 'X'})
|
||||
monkeypatch.setattr(qu, '_read_file_ids', lambda fp, **kw: {'spotify_track_id': 'sp9', 'isrc': 'X'})
|
||||
fake = {'id': 'sp9', 'name': 'Song One', 'album': {'name': 'Album X'}}
|
||||
monkeypatch.setattr(qu, '_match_via_track_id', lambda ids, sp: (fake, 'spotify'))
|
||||
monkeypatch.setattr(qu, '_normalize_track_match', lambda t, s: dict(fake))
|
||||
|
|
@ -292,7 +288,7 @@ def test_scan_prefers_isrc_exact_match_over_fuzzy(monkeypatch):
|
|||
and do NOT run the album/search tiers."""
|
||||
db = _FakeDB([_row()], BALANCED)
|
||||
_stub_engine(monkeypatch)
|
||||
monkeypatch.setattr(qu, '_read_file_ids', lambda fp: {'isrc': 'USRC17607839'})
|
||||
monkeypatch.setattr(qu, '_read_file_ids', lambda fp, **kw: {'isrc': 'USRC17607839'})
|
||||
monkeypatch.setattr(qu, '_match_via_track_id', lambda *a, **k: (None, None))
|
||||
fake = {'id': 'sp1', 'name': 'Song One', 'artists': ['Artist A'], 'album': {'name': 'Album X'}}
|
||||
monkeypatch.setattr(qu, '_match_via_isrc', lambda isrc, sp: (fake, 'spotify'))
|
||||
|
|
@ -314,7 +310,7 @@ def test_scan_falls_back_to_search_without_ids(monkeypatch):
|
|||
"""No track-ID / ISRC / album hit → fall back to fuzzy search."""
|
||||
db = _FakeDB([_row()], BALANCED)
|
||||
_stub_engine(monkeypatch)
|
||||
monkeypatch.setattr(qu, '_read_file_ids', lambda fp: {}) # un-enriched
|
||||
monkeypatch.setattr(qu, '_read_file_ids', lambda fp, **kw: {}) # un-enriched
|
||||
monkeypatch.setattr(qu, '_match_via_track_id', lambda *a, **k: (None, None))
|
||||
monkeypatch.setattr(qu, '_match_via_album', lambda *a, **k: (None, None))
|
||||
fake = {'id': 'sp1', 'name': 'Song One', 'artists': ['Artist A'], 'album': {'name': 'Album X'}}
|
||||
|
|
@ -333,7 +329,7 @@ def test_scan_uses_album_tier_when_no_ids(monkeypatch):
|
|||
'album', and the fuzzy search is never reached."""
|
||||
db = _FakeDB([_row()], BALANCED)
|
||||
_stub_engine(monkeypatch)
|
||||
monkeypatch.setattr(qu, '_read_file_ids', lambda fp: {})
|
||||
monkeypatch.setattr(qu, '_read_file_ids', lambda fp, **kw: {})
|
||||
monkeypatch.setattr(qu, '_match_via_track_id', lambda *a, **k: (None, None))
|
||||
fake = {'id': 'sp1', 'name': 'Song One', 'artists': ['Artist A'], 'album': {'name': 'Album X'}}
|
||||
monkeypatch.setattr(qu, '_match_via_album', lambda *a, **k: (fake, 'spotify'))
|
||||
|
|
|
|||
Loading…
Reference in a new issue