Fix #767: Library Organizer dry run no longer creates folders
The reorganize preview (dry run) was physically creating destination album folders, littering the library with empty dirs and making "changes" before the user ever hit Apply. Cause: preview_album_reorganize calls build_final_path_for_track purely to COMPUTE the destination path string — but that shared helper has 9 os.makedirs side effects (it's also the live download/import path builder, where creating the dir is correct). So computing the preview path created "Lenka (Expanded Edition)/" on disk. Fix: build_final_path_for_track gains create_dirs=True; all 9 makedirs now route through a gated helper. The reorganize PREVIEW passes create_dirs=False, so a dry run computes the exact destination path with zero filesystem side effects. Everything else keeps the default True: - the download/import post-process flow (still writes files into the dir), - retag, - the reorganize APPLY path — verified it goes through post_process_fn (the real pipeline → build_final_path_for_track with create_dirs=True), so live moves still create their destination dirs. The gate only silences the dry run. Tests: tests/imports/test_import_paths.py — create_dirs=False computes the correct path (matching the reported "01 - The Show.flac") but writes NOTHING to disk (not even the Transfer root); create_dirs=True still creates folders; both yield an identical path. Updated two reorganize-orchestrator test doubles to accept the new kwarg. 148 reorganize/paths/retag/pipeline tests pass. Does NOT fix the second half of #767 (Expanded Edition picked over the standard album). That is NOT a reorganizer bug: the library album row was linked to the deluxe release at enrichment time (its stored spotify_album_id/itunes_album_id/ deezer_id points at "Lenka (Expanded Edition)"), and the reorganizer faithfully reorganizes to whatever the album is linked to. The real fix is in album enrichment's edition preference — tracked separately.
This commit is contained in:
BoulderBadgeDad
parent
bba0836324
commit
3b49ac8280
4 changed files with 111 additions and 15 deletions
|
|
@ -418,8 +418,20 @@ def _coerce_int(value: Any, default: int = 1) -> int:
|
|||
return coerced if coerced > 0 else default
|
||||
|
||||
|
||||
def build_final_path_for_track(context, artist_context, album_info, file_ext):
|
||||
"""Shared path builder used by both post-processing and verification."""
|
||||
def build_final_path_for_track(context, artist_context, album_info, file_ext, create_dirs: bool = True):
|
||||
"""Shared path builder used by both post-processing and verification.
|
||||
|
||||
``create_dirs`` gates the directory-creation side effects. The download
|
||||
import flow leaves it True (it's about to write the file there). The
|
||||
library-reorganize PREVIEW passes False so a dry run can compute the exact
|
||||
destination path WITHOUT physically creating the folder — fixes #767 (dry
|
||||
run was leaving empty destination folders behind)."""
|
||||
_real_makedirs = os.makedirs
|
||||
|
||||
def _ensure_dir(path, **_kw):
|
||||
if create_dirs:
|
||||
_real_makedirs(path, exist_ok=True)
|
||||
|
||||
transfer_dir = docker_resolve_path(_get_config_manager().get("soulseek.transfer_path", "./Transfer"))
|
||||
context = normalize_import_context(context)
|
||||
track_info = get_import_track_info(context)
|
||||
|
|
@ -440,7 +452,7 @@ def build_final_path_for_track(context, artist_context, album_info, file_ext):
|
|||
original_dir = os.path.dirname(original_path)
|
||||
original_stem = os.path.splitext(os.path.basename(original_path))[0]
|
||||
final_path = os.path.join(original_dir, original_stem + file_ext)
|
||||
os.makedirs(original_dir, exist_ok=True)
|
||||
_ensure_dir(original_dir, exist_ok=True)
|
||||
logger.info("[Enhance] Using original file location: %s", final_path)
|
||||
return final_path, True
|
||||
|
||||
|
|
@ -477,12 +489,12 @@ def build_final_path_for_track(context, artist_context, album_info, file_ext):
|
|||
folder_path, filename_base = get_file_path_from_template(template_context, "playlist_path")
|
||||
if folder_path and filename_base:
|
||||
final_path = os.path.join(transfer_dir, folder_path, filename_base + file_ext)
|
||||
os.makedirs(os.path.join(transfer_dir, folder_path), exist_ok=True)
|
||||
_ensure_dir(os.path.join(transfer_dir, folder_path), exist_ok=True)
|
||||
return final_path, True
|
||||
|
||||
playlist_name_sanitized = sanitize_filename(playlist_name)
|
||||
playlist_dir = os.path.join(transfer_dir, playlist_name_sanitized)
|
||||
os.makedirs(playlist_dir, exist_ok=True)
|
||||
_ensure_dir(playlist_dir, exist_ok=True)
|
||||
artist_name_sanitized = sanitize_filename(template_context["artist"])
|
||||
track_name_sanitized = sanitize_filename(track_name)
|
||||
new_filename = f"{artist_name_sanitized} - {track_name_sanitized}{file_ext}"
|
||||
|
|
@ -579,10 +591,10 @@ def build_final_path_for_track(context, artist_context, album_info, file_ext):
|
|||
if total_discs > 1 and not user_controls_disc:
|
||||
disc_folder = f"{disc_label} {disc_number}"
|
||||
final_path = os.path.join(transfer_dir, folder_path, disc_folder, filename_base + file_ext)
|
||||
os.makedirs(os.path.join(transfer_dir, folder_path, disc_folder), exist_ok=True)
|
||||
_ensure_dir(os.path.join(transfer_dir, folder_path, disc_folder), exist_ok=True)
|
||||
else:
|
||||
final_path = os.path.join(transfer_dir, folder_path, filename_base + file_ext)
|
||||
os.makedirs(os.path.join(transfer_dir, folder_path), exist_ok=True)
|
||||
_ensure_dir(os.path.join(transfer_dir, folder_path), exist_ok=True)
|
||||
return final_path, True
|
||||
|
||||
artist_name_sanitized = sanitize_filename(template_context["albumartist"])
|
||||
|
|
@ -592,7 +604,7 @@ def build_final_path_for_track(context, artist_context, album_info, file_ext):
|
|||
album_dir = os.path.join(artist_dir, album_folder_name)
|
||||
if total_discs > 1:
|
||||
album_dir = os.path.join(album_dir, f"{disc_label} {disc_number}")
|
||||
os.makedirs(album_dir, exist_ok=True)
|
||||
_ensure_dir(album_dir, exist_ok=True)
|
||||
final_track_name_sanitized = sanitize_filename(clean_track_name)
|
||||
new_filename = f"{track_number:02d} - {final_track_name_sanitized}{file_ext}"
|
||||
return os.path.join(album_dir, new_filename), True
|
||||
|
|
@ -629,10 +641,10 @@ def build_final_path_for_track(context, artist_context, album_info, file_ext):
|
|||
if filename_base:
|
||||
if folder_path:
|
||||
final_path = os.path.join(transfer_dir, folder_path, filename_base + file_ext)
|
||||
os.makedirs(os.path.join(transfer_dir, folder_path), exist_ok=True)
|
||||
_ensure_dir(os.path.join(transfer_dir, folder_path), exist_ok=True)
|
||||
else:
|
||||
final_path = os.path.join(transfer_dir, filename_base + file_ext)
|
||||
os.makedirs(transfer_dir, exist_ok=True)
|
||||
_ensure_dir(transfer_dir, exist_ok=True)
|
||||
return final_path, True
|
||||
|
||||
artist_name_sanitized = sanitize_filename(template_context["artist"])
|
||||
|
|
@ -640,6 +652,6 @@ def build_final_path_for_track(context, artist_context, album_info, file_ext):
|
|||
artist_dir = os.path.join(transfer_dir, artist_name_sanitized)
|
||||
single_folder_name = f"{artist_name_sanitized} - {final_track_name_sanitized}"
|
||||
single_dir = os.path.join(artist_dir, single_folder_name)
|
||||
os.makedirs(single_dir, exist_ok=True)
|
||||
_ensure_dir(single_dir, exist_ok=True)
|
||||
new_filename = f"{final_track_name_sanitized}{file_ext}"
|
||||
return os.path.join(single_dir, new_filename), True
|
||||
|
|
|
|||
|
|
@ -989,7 +989,12 @@ def preview_album_reorganize(
|
|||
album_info = _build_album_info(context)
|
||||
try:
|
||||
spotify_artist = context['spotify_artist']
|
||||
new_full, _ok = build_final_path_fn(context, spotify_artist, album_info, file_ext)
|
||||
# Dry run: compute the destination path WITHOUT creating the folder.
|
||||
# Previously this physically created the album dir during preview,
|
||||
# leaving empty folders all over the library (#767).
|
||||
new_full, _ok = build_final_path_fn(
|
||||
context, spotify_artist, album_info, file_ext, create_dirs=False
|
||||
)
|
||||
item['new_path'] = (
|
||||
os.path.relpath(new_full, transfer_dir)
|
||||
if transfer_dir and new_full and new_full.startswith(transfer_dir)
|
||||
|
|
|
|||
|
|
@ -17,6 +17,84 @@ def test_sanitize_filename_replaces_illegal_characters():
|
|||
assert import_paths.sanitize_filename("AUX.txt").startswith("_")
|
||||
|
||||
|
||||
# ── #767: dry-run path build must not create folders ──────────────────────
|
||||
|
||||
def _album_path_config(tmp_path):
|
||||
return _Config({
|
||||
"soulseek.transfer_path": str(tmp_path / "Transfer"),
|
||||
"file_organization.enabled": True,
|
||||
"file_organization.templates": {
|
||||
"album_path": "$albumartist/$albumartist - $album/$track - $title",
|
||||
"single_path": "$artist/$artist - $title",
|
||||
},
|
||||
"file_organization.collab_artist_mode": "first",
|
||||
"file_organization.disc_label": "Disc",
|
||||
})
|
||||
|
||||
|
||||
def _album_context():
|
||||
return {
|
||||
"artist": {"name": "Lenka"},
|
||||
"album": {"name": "Lenka", "id": "album-1", "release_date": "2008-01-01",
|
||||
"total_tracks": 12, "album_type": "album", "artists": [{"name": "Lenka"}]},
|
||||
"track_info": {"name": "The Show", "id": "t1", "track_number": 1,
|
||||
"disc_number": 1, "artists": [{"name": "Lenka"}]},
|
||||
"original_search_result": {"title": "The Show", "clean_title": "The Show",
|
||||
"clean_album": "Lenka", "clean_artist": "Lenka",
|
||||
"artists": [{"name": "Lenka"}]},
|
||||
"source": "deezer", "is_album_download": False,
|
||||
}
|
||||
|
||||
|
||||
def test_create_dirs_false_does_not_create_folders(monkeypatch, tmp_path):
|
||||
monkeypatch.setattr(import_paths, "_get_config_manager", lambda: _album_path_config(tmp_path))
|
||||
monkeypatch.setattr(import_paths, "_get_album_tracks_for_source", lambda *a: None)
|
||||
|
||||
final_path, created = import_paths.build_final_path_for_track(
|
||||
_album_context(), {"name": "Lenka"},
|
||||
{"is_album": True, "album_name": "Lenka", "track_number": 1, "disc_number": 1},
|
||||
".flac", create_dirs=False,
|
||||
)
|
||||
|
||||
# Path is still computed correctly...
|
||||
assert created is True
|
||||
assert final_path == str(tmp_path / "Transfer" / "Lenka" / "Lenka - Lenka" / "01 - The Show.flac")
|
||||
# ...but NOTHING was written to disk — not even the Transfer root.
|
||||
assert not (tmp_path / "Transfer").exists()
|
||||
|
||||
|
||||
def test_create_dirs_true_still_creates_folders(monkeypatch, tmp_path):
|
||||
# The download/import flow must keep working (default behavior unchanged).
|
||||
monkeypatch.setattr(import_paths, "_get_config_manager", lambda: _album_path_config(tmp_path))
|
||||
monkeypatch.setattr(import_paths, "_get_album_tracks_for_source", lambda *a: None)
|
||||
|
||||
final_path, created = import_paths.build_final_path_for_track(
|
||||
_album_context(), {"name": "Lenka"},
|
||||
{"is_album": True, "album_name": "Lenka", "track_number": 1, "disc_number": 1},
|
||||
".flac", # create_dirs defaults True
|
||||
)
|
||||
|
||||
assert created is True
|
||||
assert (tmp_path / "Transfer" / "Lenka" / "Lenka - Lenka").is_dir()
|
||||
|
||||
|
||||
def test_create_dirs_false_and_true_yield_identical_path(monkeypatch, tmp_path):
|
||||
monkeypatch.setattr(import_paths, "_get_config_manager", lambda: _album_path_config(tmp_path))
|
||||
monkeypatch.setattr(import_paths, "_get_album_tracks_for_source", lambda *a: None)
|
||||
|
||||
dry, _ = import_paths.build_final_path_for_track(
|
||||
_album_context(), {"name": "Lenka"},
|
||||
{"is_album": True, "album_name": "Lenka", "track_number": 1, "disc_number": 1},
|
||||
".flac", create_dirs=False,
|
||||
)
|
||||
live, _ = import_paths.build_final_path_for_track(
|
||||
_album_context(), {"name": "Lenka"},
|
||||
{"is_album": True, "album_name": "Lenka", "track_number": 1, "disc_number": 1},
|
||||
".flac", create_dirs=True,
|
||||
)
|
||||
assert dry == live
|
||||
|
||||
|
||||
def test_build_simple_download_destination_uses_album_folder(monkeypatch, tmp_path):
|
||||
config = _Config({"soulseek.transfer_path": str(tmp_path / "Transfer")})
|
||||
monkeypatch.setattr(import_paths, "_get_config_manager", lambda: config)
|
||||
|
|
|
|||
|
|
@ -1163,9 +1163,10 @@ def test_keeps_album_sidecars_when_a_track_failed_to_move(monkeypatch, tmpdirs):
|
|||
|
||||
# --- preview function (shared planning with the orchestrator) -----------
|
||||
|
||||
def _fake_path_builder(context, spotify_artist, _album_info, file_ext):
|
||||
def _fake_path_builder(context, spotify_artist, _album_info, file_ext, **_kw):
|
||||
"""Stand-in for `_build_final_path_for_track`. Inserts Disc N/ when
|
||||
total_discs > 1 — same convention the real builder uses."""
|
||||
total_discs > 1 — same convention the real builder uses. Accepts
|
||||
**_kw so the preview's create_dirs=False kwarg (#767) passes through."""
|
||||
album = context['spotify_album']['name']
|
||||
artist = spotify_artist['name']
|
||||
track_info = context['track_info']
|
||||
|
|
@ -1180,7 +1181,7 @@ def _fake_path_builder(context, spotify_artist, _album_info, file_ext):
|
|||
return '/'.join(parts), True
|
||||
|
||||
|
||||
def _path_builder_album_vs_single(context, spotify_artist, album_info, file_ext):
|
||||
def _path_builder_album_vs_single(context, spotify_artist, album_info, file_ext, **_kw):
|
||||
"""Stand-in that emulates the real `_build_final_path_for_track`
|
||||
branch on `album_info.get('is_album')`. ALBUM mode produces an
|
||||
album folder with disc subfolder + numbered file; SINGLE mode
|
||||
|
|
|
|||
Loading…
Reference in a new issue