From d97d105b9738b979a6183db9ccc695626b1e2743 Mon Sep 17 00:00:00 2001 From: Broque Thomas <26755000+Nezreka@users.noreply.github.com> Date: Tue, 28 Apr 2026 21:32:24 -0700 Subject: [PATCH] fix: substitute \$cdnum in download paths and skip auto disc folder when template uses it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit User report: multi-disc albums on the latest dev had literal "\$cdnum" in their filenames instead of the expected "CDxx" label, plus a redundant "Disc N" folder on top of the in-filename label. Two bugs in core/imports/paths.py: 1. _replace_template_variables (the substitution helper used by every download path builder) had no handling for \$cdnum or \${cdnum}. The matching helper in web_server.py and core/repair_jobs/library_reorganize.py did the substitution; this one didn't, so production downloads passed the placeholder through unchanged. Added a cdnum_value computation (CD%02d when total_discs > 1, empty otherwise) plus the corresponding bracket_map entry and \$cdnum replace before \$track (matches the ordering in the other path builders). 2. The album-path branch of build_final_path_for_track auto-injected a "Disc N" folder whenever total_discs > 1, suppressed only when the template contained \$disc. Templates using \$cdnum (or \${disc} / \${discnum} / \${cdnum}) got both a "CDxx" label in the filename and the auto folder. Widened the user_controls_disc check to cover all the disc-bearing placeholders. Bonus cleanup along the way: - Folder-part stripping now drops a leading \$cdnum token (mirrors the existing \$disc / \$discnum / \$quality strip — defensive against an empty cdnum landing alone in a folder segment). - Filename cleanup now strips a leading " - " left behind when \$cdnum expands to empty on a single-disc album (mirrors the same regex in library_reorganize.py). - album_template config access switched from the dotted-path key to the nested-dict access pattern used by the rest of the function — handles both production config_manager and the flat _Config used in tests. Tests: 4 new under tests/imports/test_import_paths.py - multi-disc cdnum substitution produces "CD02" - single-disc cdnum collapses to empty - folder-part containing only \$cdnum is dropped - build_final_path_for_track with \$cdnum template produces no auto "Disc N" folder Full suite: 1276 passing (was 1272). Ruff clean. --- core/imports/paths.py | 38 +++++++- tests/imports/test_import_paths.py | 142 +++++++++++++++++++++++++++++ 2 files changed, 178 insertions(+), 2 deletions(-) diff --git a/core/imports/paths.py b/core/imports/paths.py index 5ac357d1..9fd9dea7 100644 --- a/core/imports/paths.py +++ b/core/imports/paths.py @@ -191,6 +191,15 @@ def _replace_template_variables(template: str, context: dict) -> str: except Exception: pass + # $cdnum — smart CD label for multi-disc filenames. Produces "CD01" / + # "CD02" etc. when the album has 2+ discs, empty string otherwise. + # Empty output collapses gracefully via the trailing dash cleanup + # regex below, so single-disc albums don't end up with "CD01" literal + # in every name. + _total_discs = _coerce_int(clean_context.get("total_discs", 1), 1) + _disc_number = _coerce_int(clean_context.get("disc_number", 1), 1) + cdnum_value = f"CD{_disc_number:02d}" if _total_discs > 1 else "" + bracket_map = { "albumartist": album_artist_value, "albumtype": clean_context.get("albumtype", "Album"), @@ -200,6 +209,7 @@ def _replace_template_variables(template: str, context: dict) -> str: "album": clean_context.get("album", "Unknown Album"), "title": clean_context.get("title", "Unknown Track"), "track": f"{_coerce_int(clean_context.get('track_number', 1), 1):02d}", + "cdnum": cdnum_value, "disc": str(_coerce_int(clean_context.get("disc_number", 1), 1)), "discnum": str(_coerce_int(clean_context.get("disc_number", 1), 1)), "year": str(clean_context.get("year", "")), @@ -215,6 +225,10 @@ def _replace_template_variables(template: str, context: dict) -> str: result = result.replace("$artist", clean_context.get("artist", "Unknown Artist")) result = result.replace("$album", clean_context.get("album", "Unknown Album")) result = result.replace("$title", clean_context.get("title", "Unknown Track")) + # $cdnum must replace before $track to follow the longest-prefix-first + # rule used throughout this function (no current $c* var collides, but + # ordering matches the web_server.py path-builder for parity). + result = result.replace("$cdnum", cdnum_value) result = result.replace("$track", f"{clean_context.get('track_number', 1):02d}") result = result.replace("$year", str(clean_context.get("year", ""))) @@ -248,6 +262,7 @@ def get_file_path_from_template_raw(template: str, context: dict) -> tuple[str, part = part.replace("$quality", "") part = part.replace("$discnum", "") part = part.replace("$disc", "") + part = part.replace("$cdnum", "") part = re.sub(r"\s*\[\s*\]", "", part) part = re.sub(r"\s*\(\s*\)", "", part) part = re.sub(r"\s*\{\s*\}", "", part) @@ -264,6 +279,9 @@ def get_file_path_from_template_raw(template: str, context: dict) -> tuple[str, filename_base = re.sub(r"\s*\(\s*\)", "", filename_base) filename_base = re.sub(r"\s*\{\s*\}", "", filename_base) filename_base = re.sub(r"\s*-\s*$", "", filename_base) + # Leading dash cleanup — lets $cdnum at the start of a filename + # cleanly disappear on single-disc albums (empty-value case). + filename_base = re.sub(r"^\s*-\s*", "", filename_base) filename_base = re.sub(r"\s+", " ", filename_base).strip() sanitized_folders = [sanitize_filename(part) for part in cleaned_folders] @@ -314,6 +332,7 @@ def get_file_path_from_template(context: dict, template_type: str = "album_path" part = part.replace("$quality", "") part = part.replace("$discnum", "") part = part.replace("$disc", "") + part = part.replace("$cdnum", "") part = re.sub(r"\s*\[\s*\]", "", part) part = re.sub(r"\s*\(\s*\)", "", part) part = re.sub(r"\s*\{\s*\}", "", part) @@ -330,6 +349,9 @@ def get_file_path_from_template(context: dict, template_type: str = "album_path" filename_base = re.sub(r"\s*\(\s*\)", "", filename_base) filename_base = re.sub(r"\s*\{\s*\}", "", filename_base) filename_base = re.sub(r"\s*-\s*$", "", filename_base) + # Leading dash cleanup — lets $cdnum at the start of a filename + # cleanly disappear on single-disc albums (empty-value case). + filename_base = re.sub(r"^\s*-\s*", "", filename_base) filename_base = re.sub(r"\s+", " ", filename_base).strip() sanitized_folders = [sanitize_filename(part) for part in cleaned_folders] @@ -518,8 +540,20 @@ def build_final_path_for_track(context, artist_context, album_info, file_ext): except Exception as _disc_err: logger.warning("[Multi-Disc] Could not resolve total_discs: %s", _disc_err) - album_template = _get_config_manager().get("file_organization.templates.album_path", "") - user_controls_disc = "$disc" in album_template + # Now that total_discs is fully resolved, expose it to the template + # so $cdnum can decide between "CDxx" and an empty string. + template_context["total_discs"] = total_discs + + album_template = _get_config_manager().get("file_organization.templates", {}).get("album_path", "") or "" + # Suppress the auto-injected disc folder when the user already + # encodes the disc in the filename via $disc, $discnum, or $cdnum. + user_controls_disc = ( + "$disc" in album_template + or "$cdnum" in album_template + or "${disc}" in album_template + or "${discnum}" in album_template + or "${cdnum}" in album_template + ) disc_label = _get_config_manager().get("file_organization.disc_label", "Disc") folder_path, filename_base = get_file_path_from_template(template_context, "album_path") diff --git a/tests/imports/test_import_paths.py b/tests/imports/test_import_paths.py index e7194089..899bfec6 100644 --- a/tests/imports/test_import_paths.py +++ b/tests/imports/test_import_paths.py @@ -75,6 +75,72 @@ def test_get_file_path_from_template_raw_handles_quality_and_disc_placeholders(m assert filename == "3 - Song One [FLAC 16bit]" +def test_get_file_path_from_template_raw_substitutes_cdnum_for_multi_disc(monkeypatch): + """$cdnum should expand to 'CDxx' on multi-disc albums (regression). + + Reported by user: filenames had literal '$cdnum' instead of 'CD02' + because `_replace_template_variables` did not handle the placeholder. + """ + monkeypatch.setattr(import_paths, "_get_config_manager", lambda: _Config({})) + + folder_path, filename = import_paths.get_file_path_from_template_raw( + "$artist/$album/$cdnum - $track - $title", + { + "artist": "Artist", + "album": "Album", + "title": "Song", + "track_number": 5, + "disc_number": 2, + "total_discs": 2, + }, + ) + + assert folder_path == os.path.join("Artist", "Album") + assert filename == "CD02 - 05 - Song" + + +def test_get_file_path_from_template_raw_collapses_cdnum_for_single_disc(monkeypatch): + """$cdnum should expand to '' on single-disc albums so it disappears cleanly.""" + monkeypatch.setattr(import_paths, "_get_config_manager", lambda: _Config({})) + + folder_path, filename = import_paths.get_file_path_from_template_raw( + "$artist/$album/$cdnum - $track - $title", + { + "artist": "Artist", + "album": "Album", + "title": "Song", + "track_number": 5, + "disc_number": 1, + "total_discs": 1, + }, + ) + + # No "CD01" prefix; trailing-dash regex collapses the empty placeholder. + assert folder_path == os.path.join("Artist", "Album") + assert filename == "05 - Song" + + +def test_get_file_path_from_template_raw_strips_cdnum_from_folders(monkeypatch): + """Even if user puts $cdnum inside a folder segment, it gets stripped (defensive).""" + monkeypatch.setattr(import_paths, "_get_config_manager", lambda: _Config({})) + + folder_path, filename = import_paths.get_file_path_from_template_raw( + "$artist/$album/$cdnum/$track - $title", + { + "artist": "Artist", + "album": "Album", + "title": "Song", + "track_number": 5, + "disc_number": 1, + "total_discs": 1, + }, + ) + + # Folder containing only $cdnum (which expands to empty for single-disc) gets dropped. + assert folder_path == os.path.join("Artist", "Album") + assert filename == "05 - Song" + + def test_resolve_album_group_upgrades_standard_to_deluxe(): album_naming.clear_album_grouping_cache() @@ -230,3 +296,79 @@ def test_build_final_path_for_track_uses_track_disc_number_without_provider_look assert final_path == str( tmp_path / "Transfer" / "Artist One" / "Artist One - Album One" / "Disc 2" / "04 - Song Two.flac" ) + + +def test_build_final_path_for_track_with_cdnum_template_skips_disc_folder(monkeypatch, tmp_path): + """When the user template encodes the disc via $cdnum, the auto disc folder must not be added. + + Reported by user: multi-disc albums got both a "CDxx" label in the + filename AND a redundant "Disc N" folder. The auto-folder should + suppress when the template already encodes the disc. + """ + config = _Config( + { + "soulseek.transfer_path": str(tmp_path / "Transfer"), + "file_organization.enabled": True, + "file_organization.templates": { + "album_path": "$albumartist/$albumartist - $album/$cdnum - $track - $title", + "single_path": "$artist/$artist - $title", + }, + "file_organization.collab_artist_mode": "first", + "file_organization.disc_label": "Disc", + } + ) + monkeypatch.setattr(import_paths, "_get_config_manager", lambda: config) + monkeypatch.setattr( + import_paths, + "_get_album_tracks_for_source", + lambda source, album_id: None, + ) + + context = { + "artist": {"name": "Artist One"}, + "album": { + "name": "Album One", + "id": "album-1", + "release_date": "2026-01-01", + "total_tracks": 24, + "total_discs": 2, + "album_type": "album", + "artists": [{"name": "Artist One"}], + }, + "track_info": { + "name": "Song Two", + "id": "track-2", + "track_number": 4, + "disc_number": 2, + "artists": [{"name": "Artist One"}], + }, + "original_search_result": { + "title": "Song Two", + "clean_title": "Song Two", + "clean_album": "Album One", + "clean_artist": "Artist One", + "artists": [{"name": "Artist One"}], + }, + "source": "deezer", + "is_album_download": False, + } + + final_path, created = import_paths.build_final_path_for_track( + context, + {"name": "Artist One"}, + { + "is_album": True, + "album_name": "Album One", + "track_number": 4, + "disc_number": 2, + }, + ".flac", + ) + + assert created is True + # Filename has the CD02 label; NO "Disc 2" folder injected. + assert final_path == str( + tmp_path / "Transfer" / "Artist One" / "Artist One - Album One" / "CD02 - 04 - Song Two.flac" + ) + # Verify the disc folder was not created either. + assert not (tmp_path / "Transfer" / "Artist One" / "Artist One - Album One" / "Disc 2").exists()