From e301877e647a1666eb08d5b1aba808642ce996d8 Mon Sep 17 00:00:00 2001 From: BoulderBadgeDad Date: Tue, 23 Jun 2026 11:26:00 -0700 Subject: [PATCH] #912: Empty Folder Cleaner reads its opt-in from the right config key MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The #891 'also remove image/sidecar-only folders' toggle never worked. Job settings are persisted as a nested dict under repair.jobs..settings (RepairWorker.set_job_settings), but the scan read flat keys — repair.jobs.empty_folder_cleaner.remove_residual_files — which never matched, so it always fell back to the False default and skipped every image/.lrc-only folder. (remove_junk_files had the same mismatch but its default is True, which is why only the truly-empty 'deleted' folder kept showing up.) Now reads from .settings like get_job_config / lossy_converter do. The pure dir_is_removable logic was already correct + tested; the bug was purely the config read in scan(), which had no test. Added two scan-level regression tests driving the real JobContext + a config that stores the toggle the way the UI does. --- core/repair_jobs/empty_folder_cleaner.py | 20 +++++---- tests/test_empty_folder_cleaner.py | 53 +++++++++++++++++++++++- 2 files changed, 65 insertions(+), 8 deletions(-) diff --git a/core/repair_jobs/empty_folder_cleaner.py b/core/repair_jobs/empty_folder_cleaner.py index fe1f5d02..e660a52c 100644 --- a/core/repair_jobs/empty_folder_cleaner.py +++ b/core/repair_jobs/empty_folder_cleaner.py @@ -91,13 +91,19 @@ class EmptyFolderCleanerJob(RepairJob): ignore_disposable = False try: if context.config_manager: - ignore_junk = bool(context.config_manager.get( - 'repair.jobs.empty_folder_cleaner.remove_junk_files', True)) - # #891: also clear folders left holding only images / .lrc / sidecars - # (what a reorganize leaves behind). Opt-in — default off. - ignore_disposable = bool(context.config_manager.get( - 'repair.jobs.empty_folder_cleaner.remove_residual_files', False)) - except Exception: # noqa: S110 — setting read is best-effort; defaults to True + # #912: job settings are persisted as a nested dict under + # `repair.jobs..settings` (see RepairWorker.set_job_settings / get_job_config). + # The old flat-key reads ('repair.jobs.empty_folder_cleaner.remove_residual_files') + # never matched what the UI saves, so the #891 opt-in toggle silently did nothing — + # the scan always fell back to the False default and skipped every image/.lrc folder. + job_settings = context.config_manager.get( + 'repair.jobs.empty_folder_cleaner.settings', {}) or {} + if isinstance(job_settings, dict): + ignore_junk = bool(job_settings.get('remove_junk_files', True)) + # #891: also clear folders left holding only images / .lrc / sidecars + # (what a reorganize leaves behind). Opt-in — default off. + ignore_disposable = bool(job_settings.get('remove_residual_files', False)) + except Exception: # noqa: S110 — setting read is best-effort; defaults to junk-only pass flagged = set() # dir paths we'd remove → a parent sees them as "gone" diff --git a/tests/test_empty_folder_cleaner.py b/tests/test_empty_folder_cleaner.py index 776a8e5a..3070efcf 100644 --- a/tests/test_empty_folder_cleaner.py +++ b/tests/test_empty_folder_cleaner.py @@ -6,7 +6,10 @@ from __future__ import annotations import os -from core.repair_jobs.empty_folder_cleaner import dir_is_removable, remove_empty_folder, is_junk +from core.repair_jobs.base import JobContext +from core.repair_jobs.empty_folder_cleaner import ( + EmptyFolderCleanerJob, dir_is_removable, is_junk, remove_empty_folder, +) # ── pure decision ─────────────────────────────────────────────────────────── @@ -120,3 +123,51 @@ def test_apply_residual_opt_still_refuses_real_content(tmp_path): remove_disposable=True, root=str(root), **_fx()) assert res['removed'] is False and d.exists() assert (d / 'booklet.pdf').exists() and (d / 'cover.jpg').exists() # nothing deleted + + +# ── #912: scan() must read the opt-in from where the UI SAVES it ───────────── +class _Cfg: + """Mimics ConfigManager: job settings live as a nested dict under + `repair.jobs..settings` (RepairWorker.set_job_settings writes there).""" + + def __init__(self, settings): + self._settings = settings + + def get(self, key, default=None): + if key == 'repair.jobs.empty_folder_cleaner.settings': + return self._settings + return default + + +def _run_scan(tmp_path, settings): + root = tmp_path / 'lib'; root.mkdir() + res_dir = root / 'Artist' / 'Old Album'; res_dir.mkdir(parents=True) + (res_dir / 'cover.jpg').write_text('img') # image + lyric only — the #912 case + (res_dir / 'lyrics.lrc').write_text('la') + keep = root / 'Artist2' / 'Real Album'; keep.mkdir(parents=True) + (keep / 'song.flac').write_text('audio') # has audio — must never be flagged + + flagged = [] + ctx = JobContext( + db=None, transfer_folder=str(root), config_manager=_Cfg(settings), + create_finding=lambda **kw: (flagged.append(kw.get('file_path')), True)[1], + ) + EmptyFolderCleanerJob().scan(ctx) + return str(res_dir), str(keep), set(flagged) + + +def test_scan_flags_residual_folder_when_opt_in_saved_under_settings(tmp_path): + # The toggle is stored at repair.jobs..settings.remove_residual_files. The scan must + # read it from THERE — the old flat-key read missed it, so the option did nothing (#912). + res_dir, keep, flagged = _run_scan( + tmp_path, {'remove_junk_files': True, 'remove_residual_files': True}) + assert res_dir in flagged # the cover.jpg + .lrc folder is now found + assert keep not in flagged # the audio folder is never touched + + +def test_scan_keeps_residual_folder_when_opt_off(tmp_path): + # Opt-off preserves the conservative default: an image/.lrc folder is left alone. + res_dir, keep, flagged = _run_scan( + tmp_path, {'remove_junk_files': True, 'remove_residual_files': False}) + assert res_dir not in flagged + assert keep not in flagged