From 0a4c3d7dc88f07547f15466e093c652c05311ee1 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Thu, 4 Jun 2026 10:06:34 -0700 Subject: [PATCH] Library re-tag: standard dry-run pattern (shows the Dry Run tag, opt-in auto-apply) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The job was the odd one out — auto_fix=False, no dry_run setting, so it never showed the 'Dry Run' badge the other jobs do (the badge keys off settings.dry_run === true). Aligned it to the standard pattern: - auto_fix=True + dry_run setting defaulting True. Default behavior is unchanged (findings only, nothing written) AND it now shows the Dry Run badge. - Turning dry_run off makes the scan auto-apply in place (result.auto_fixed), no finding — the opt-in 'just retag it' mode. - Extracted a shared apply_track_plans() used by both the scan auto-apply and the repair_worker fix handler (handler now resolves Docker paths then delegates — one code path, no duplication). Tests: dry_run=False auto-applies + writes + no finding; existing dry-run finding/skip/apply tests still green. 410 passing. --- core/repair_jobs/library_retag.py | 69 +++++++++++++++++++++++++++++-- core/repair_worker.py | 68 +++++++----------------------- tests/test_library_retag_job.py | 20 +++++++++ 3 files changed, 101 insertions(+), 56 deletions(-) diff --git a/core/repair_jobs/library_retag.py b/core/repair_jobs/library_retag.py index c6f11353..6219e6c0 100644 --- a/core/repair_jobs/library_retag.py +++ b/core/repair_jobs/library_retag.py @@ -45,6 +45,55 @@ def _read_current_tags(file_path): return {} +def apply_track_plans(track_plans, cover_action=None, cover_url=None) -> dict: + """Write each plan's tags in place (+ optionally embed/refresh cover art), + reusing tag_writer.write_tags_to_file. ``file_path`` on each plan must be a + real, reachable path (caller resolves Docker paths). Shared by the dry-run= + False auto-apply and the repair_worker fix handler. Never raises. + """ + import os as _os + result = {'written': 0, 'failed': 0, 'skipped': 0, 'cover_written': False} + embed_cover = bool(cover_action and cover_url) + cover_data = None + if embed_cover: + try: + from core.tag_writer import download_cover_art + cover_data = download_cover_art(cover_url) + except Exception as e: + logger.debug("retag cover download failed: %s", e) + embed_cover = embed_cover and cover_data is not None + + from core.tag_writer import write_tags_to_file + last_dir = None + for tp in track_plans or []: + fp = tp.get('file_path') + db_data = tp.get('db_data') or {} + if not fp or not _os.path.isfile(fp) or (not db_data and not embed_cover): + result['skipped'] += 1 + continue + try: + res = write_tags_to_file(fp, db_data, embed_cover=embed_cover, cover_data=cover_data) + if res.get('success'): + result['written'] += 1 + last_dir = _os.path.dirname(fp) + else: + result['failed'] += 1 + except Exception as e: + logger.warning("retag write failed for %s: %s", fp, e) + result['failed'] += 1 + + if cover_action and cover_data and last_dir: + try: + cover_path = _os.path.join(last_dir, 'cover.jpg') + if cover_action == 'replace' or not _os.path.exists(cover_path): + with open(cover_path, 'wb') as fh: + fh.write(cover_data[0]) + result['cover_written'] = True + except Exception as e: + logger.debug("retag cover.jpg write failed: %s", e) + return result + + def _add_source_ids(db_data, source, album_source_id, source_track): """Stamp the album/track source IDs onto the write payload so the canonical writer embeds them too (Spotify / iTunes / MusicBrainz).""" @@ -95,6 +144,8 @@ class LibraryRetagJob(RepairJob): 'comes from. Each finding lists every tag that would change (old -> new) per ' 'track so you can review before applying — nothing is written until you do.\n\n' 'Settings:\n' + '- Dry run (default ON): only create findings to review; nothing is written. ' + 'Turn it off to auto-apply on scan.\n' '- Mode: "overwrite" rewrites every field the source provides; "fill_missing" ' 'only fills blank tags (keeps your existing values).\n' '- Cover art: replace / fill-missing / skip.\n' @@ -104,6 +155,7 @@ class LibraryRetagJob(RepairJob): default_enabled = False default_interval_hours = 168 default_settings = { + 'dry_run': True, 'mode': MODE_OVERWRITE, 'cover_art': 'replace', 'source': '', @@ -113,7 +165,7 @@ class LibraryRetagJob(RepairJob): 'cover_art': ['replace', 'fill_missing', 'skip'], 'source': ['', 'spotify', 'itunes', 'deezer', 'musicbrainz'], } - auto_fix = False + auto_fix = True def _get_settings(self, context: JobContext) -> dict: merged = dict(self.default_settings) @@ -133,6 +185,7 @@ class LibraryRetagJob(RepairJob): settings = self._get_settings(context) mode = settings.get('mode', MODE_OVERWRITE) cover_mode = settings.get('cover_art', 'replace') + dry_run = settings.get('dry_run', True) source_order = self._source_order(settings) if not source_order: logger.warning("Library re-tag: no usable metadata sources configured") @@ -180,7 +233,7 @@ class LibraryRetagJob(RepairJob): try: self._scan_album(context, result, album_id, album_title, artist_name, - source, album_source_id, mode, cover_mode) + source, album_source_id, mode, cover_mode, dry_run) except Exception as e: logger.debug("Library re-tag: album %s failed: %s", album_id, e) result.errors += 1 @@ -192,7 +245,7 @@ class LibraryRetagJob(RepairJob): return result def _scan_album(self, context, result, album_id, album_title, artist_name, - source, album_source_id, mode, cover_mode): + source, album_source_id, mode, cover_mode, dry_run=True): # Local tracks for this album. with context.db._get_connection() as conn: cur = conn.cursor() @@ -257,6 +310,16 @@ class LibraryRetagJob(RepairJob): result.skipped += 1 return + # Not dry-run: apply the tags in place now (the track paths were already + # isfile-checked above) and count it as an auto-fix — no finding. + if not dry_run: + res = apply_track_plans(track_plans, cover_action, cover_url) + if res['written'] or res['cover_written']: + result.auto_fixed += 1 + else: + result.errors += 1 + return + total_changes = sum(len(tp['changes']) for tp in track_plans) summary_bits = [] if tag_change_tracks: diff --git a/core/repair_worker.py b/core/repair_worker.py index ab5650f6..2ddca93d 100644 --- a/core/repair_worker.py +++ b/core/repair_worker.py @@ -1365,68 +1365,30 @@ class RepairWorker: if not tracks: return {'success': False, 'error': 'No tracks to re-tag in finding'} - cover_action = details.get('cover_action') - cover_url = details.get('cover_url') - embed_cover = bool(cover_action and cover_url) + # Resolve container/host path mismatches, then delegate to the shared + # apply path the job's auto-fix mode also uses. download_folder = self._config_manager.get('soulseek.download_path', '') if self._config_manager else None - - # Download the cover once for the whole album (batch embed). - cover_data = None - if embed_cover: - try: - from core.tag_writer import download_cover_art - cover_data = download_cover_art(cover_url) - except Exception as e: - logger.debug("library_retag: cover download failed: %s", e) - embed_cover = embed_cover and cover_data is not None - - from core.tag_writer import write_tags_to_file - written = failed = skipped = 0 - last_dir = None + resolved_plans = [] for t in tracks: raw = t.get('file_path') - db_data = t.get('db_data') or {} - if not raw or (not db_data and not embed_cover): - skipped += 1 + if not raw: continue - resolved = _resolve_file_path(raw, self.transfer_folder, download_folder, - config_manager=self._config_manager) or raw - if not os.path.isfile(resolved): - skipped += 1 - continue - try: - res = write_tags_to_file(resolved, db_data, embed_cover=embed_cover, cover_data=cover_data) - if res.get('success'): - written += 1 - last_dir = os.path.dirname(resolved) - else: - failed += 1 - except Exception as e: - logger.warning("library_retag write failed for %s: %s", resolved, e) - failed += 1 + rp = _resolve_file_path(raw, self.transfer_folder, download_folder, + config_manager=self._config_manager) or raw + resolved_plans.append({'file_path': rp, 'db_data': t.get('db_data') or {}}) - # Refresh the cover.jpg sidecar to match (replace, or fill when missing). - cover_written = False - if cover_action and cover_data and last_dir: - try: - cover_path = os.path.join(last_dir, 'cover.jpg') - if cover_action == 'replace' or not os.path.exists(cover_path): - with open(cover_path, 'wb') as fh: - fh.write(cover_data[0]) - cover_written = True - except Exception as e: - logger.debug("library_retag: cover.jpg write failed: %s", e) + from core.repair_jobs.library_retag import apply_track_plans + res = apply_track_plans(resolved_plans, details.get('cover_action'), details.get('cover_url')) - if written == 0 and not cover_written: + if res['written'] == 0 and not res['cover_written']: return {'success': False, 'error': 'Nothing could be written — files unreachable or read-only?'} - msg = f'Re-tagged {written} track(s)' - if failed: - msg += f' ({failed} failed)' - if cover_written: + msg = f"Re-tagged {res['written']} track(s)" + if res['failed']: + msg += f" ({res['failed']} failed)" + if res['cover_written']: msg += ' + refreshed cover.jpg' - return {'success': True, 'action': 'library_retag', 'message': msg, - 'written': written, 'failed': failed, 'skipped': skipped} + return {'success': True, 'action': 'library_retag', 'message': msg, **res} def _fix_metadata_gap(self, entity_type, entity_id, file_path, details): """Apply found metadata fields to the track.""" diff --git a/tests/test_library_retag_job.py b/tests/test_library_retag_job.py index 28ab0c22..58a88960 100644 --- a/tests/test_library_retag_job.py +++ b/tests/test_library_retag_job.py @@ -91,6 +91,26 @@ def test_scan_creates_detailed_finding(tmp_path, monkeypatch): assert tp['db_data']['title'] == 'Real Title' +def test_scan_dry_run_off_auto_applies_no_finding(tmp_path, monkeypatch): + """dry_run=False: scan applies in place (auto_fixed) and creates NO finding.""" + track = tmp_path / 'track.flac'; track.write_bytes(b'') + conn = _db_with_album(str(tmp_path / 'm.db'), str(track), current_title='Old Title') + ctx = _context(conn, {'mode': 'overwrite', 'cover_art': 'skip', 'source': 'spotify', 'dry_run': False}) + _patch_source(monkeypatch, { + 'title': 'Old Title', 'album_artist': 'Real Artist', 'album': 'Real Album', + 'year': '2021', 'genre': 'Rock', 'track_number': 1, 'disc_number': 1, + }) + writes = [] + monkeypatch.setattr('core.tag_writer.write_tags_to_file', + lambda fp, db_data, **k: writes.append(db_data) or {'success': True}) + + result = lr.LibraryRetagJob().scan(ctx) + + assert ctx.findings == [] # no finding in apply mode + assert result.auto_fixed == 1 + assert writes and writes[0]['title'] == 'Real Title' # actually wrote + + def test_scan_skips_album_already_correct(tmp_path, monkeypatch): track = tmp_path / 'track.flac'; track.write_bytes(b'') conn = _db_with_album(str(tmp_path / 'm.db'), str(track), current_title='Real Title')