danvics/soulsync
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 1

fix: substitute \$cdnum in download paths and skip auto disc folder when template uses it

Browse source 
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.
This commit is contained in:
Broque Thomas 2026-04-28 21:32:24 -07:00
parent 278f30e336
commit d97d105b97
2 changed files with 178 additions and 2 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

38
core/imports/paths.py
View file

@ -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")

142
tests/imports/test_import_paths.py
View file

@ -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()