diff --git a/core/repair_worker.py b/core/repair_worker.py index 84c67532..64f9864a 100644 --- a/core/repair_worker.py +++ b/core/repair_worker.py @@ -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: diff --git a/tests/repair_jobs/test_quality_upgrade.py b/tests/repair_jobs/test_quality_upgrade.py index 9764c2af..d265d4bb 100644 --- a/tests/repair_jobs/test_quality_upgrade.py +++ b/tests/repair_jobs/test_quality_upgrade.py @@ -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'))