library reorganize: wire rename-only mode through the queue (#875)
Threads the rename_only flag from the apply endpoint to the executor, additively (default False everywhere → existing full-flow behaviour byte-for-byte unchanged): - /api/library/album/<id>/reorganize-files reads `rename_only` from the body → enqueue. - QueueItem gains rename_only (+ surfaced in to_dict for the status panel). - reorganize_runner.build_runner takes build_final_path_fn and branches: a rename_only item routes to reorganize_album_rename_only (no staging dir, no copy, no post-process); everything else falls through to the full reorganize_album. Staging is only created for the full path now. - web_server injects build_final_path_fn (= _build_final_path_for_track, the same builder the preview uses) so apply matches the preview exactly. Fixed a test landmine: _make_item returns a MagicMock, whose .rename_only is a truthy mock that wrongly took the new branch — set it to False to match the real QueueItem default. +2 runner tests (rename_only routes to the rename executor + creates no staging; missing path-builder → clean setup_failed). 209 reorganize tests green, ruff clean. Left: the modal (Full vs Rename-only) + optional post-rename server scan + the issue reply.
This commit is contained in:
parent
92c101e300
commit
5df873655f
4 changed files with 107 additions and 12 deletions
|
|
@ -70,6 +70,9 @@ class QueueItem:
|
||||||
# 'tags' = read each file's embedded tags as the source
|
# 'tags' = read each file's embedded tags as the source
|
||||||
# of truth (issue #592). Zero API calls.
|
# of truth (issue #592). Zero API calls.
|
||||||
metadata_source: str = 'api'
|
metadata_source: str = 'api'
|
||||||
|
# Rename-only mode (#875): move files to the current naming scheme WITHOUT the
|
||||||
|
# copy + post-processing (re-tag / quality / AcoustID) the full flow runs.
|
||||||
|
rename_only: bool = False
|
||||||
status: str = 'queued' # queued | running | done | failed | cancelled
|
status: str = 'queued' # queued | running | done | failed | cancelled
|
||||||
started_at: Optional[float] = None
|
started_at: Optional[float] = None
|
||||||
finished_at: Optional[float] = None
|
finished_at: Optional[float] = None
|
||||||
|
|
@ -96,6 +99,7 @@ class QueueItem:
|
||||||
'artist_name': self.artist_name,
|
'artist_name': self.artist_name,
|
||||||
'source': self.source,
|
'source': self.source,
|
||||||
'metadata_source': self.metadata_source,
|
'metadata_source': self.metadata_source,
|
||||||
|
'rename_only': self.rename_only,
|
||||||
'enqueued_at': self.enqueued_at,
|
'enqueued_at': self.enqueued_at,
|
||||||
'started_at': self.started_at,
|
'started_at': self.started_at,
|
||||||
'finished_at': self.finished_at,
|
'finished_at': self.finished_at,
|
||||||
|
|
@ -161,6 +165,7 @@ class ReorganizeQueue:
|
||||||
artist_name: str,
|
artist_name: str,
|
||||||
source: Optional[str] = None,
|
source: Optional[str] = None,
|
||||||
metadata_source: str = 'api',
|
metadata_source: str = 'api',
|
||||||
|
rename_only: bool = False,
|
||||||
) -> dict:
|
) -> dict:
|
||||||
"""Add an album to the queue. Returns a result dict:
|
"""Add an album to the queue. Returns a result dict:
|
||||||
|
|
||||||
|
|
@ -190,6 +195,7 @@ class ReorganizeQueue:
|
||||||
source=source,
|
source=source,
|
||||||
enqueued_at=time.time(),
|
enqueued_at=time.time(),
|
||||||
metadata_source=metadata_source or 'api',
|
metadata_source=metadata_source or 'api',
|
||||||
|
rename_only=bool(rename_only),
|
||||||
)
|
)
|
||||||
self._items.append(item)
|
self._items.append(item)
|
||||||
position = sum(1 for i in self._items if i.status == 'queued')
|
position = sum(1 for i in self._items if i.status == 'queued')
|
||||||
|
|
|
||||||
|
|
@ -37,6 +37,7 @@ def build_runner(
|
||||||
is_shutting_down_fn: Callable[[], bool],
|
is_shutting_down_fn: Callable[[], bool],
|
||||||
get_download_path: Callable[[], str],
|
get_download_path: Callable[[], str],
|
||||||
get_transfer_path: Callable[[], str],
|
get_transfer_path: Callable[[], str],
|
||||||
|
build_final_path_fn: Optional[Callable] = None,
|
||||||
) -> Callable[[object], dict]:
|
) -> Callable[[object], dict]:
|
||||||
"""Return the closure the queue worker invokes per item.
|
"""Return the closure the queue worker invokes per item.
|
||||||
|
|
||||||
|
|
@ -59,7 +60,7 @@ def build_runner(
|
||||||
A callable ``runner(item)`` suitable for
|
A callable ``runner(item)`` suitable for
|
||||||
:meth:`core.reorganize_queue.ReorganizeQueue.set_runner`.
|
:meth:`core.reorganize_queue.ReorganizeQueue.set_runner`.
|
||||||
"""
|
"""
|
||||||
from core.library_reorganize import reorganize_album
|
from core.library_reorganize import reorganize_album, reorganize_album_rename_only
|
||||||
from core.reorganize_queue import get_queue
|
from core.reorganize_queue import get_queue
|
||||||
|
|
||||||
def _update_track_path(track_id, new_path):
|
def _update_track_path(track_id, new_path):
|
||||||
|
|
@ -80,17 +81,6 @@ def build_runner(
|
||||||
# server restart.
|
# server restart.
|
||||||
download_dir = get_download_path()
|
download_dir = get_download_path()
|
||||||
transfer_dir = get_transfer_path()
|
transfer_dir = get_transfer_path()
|
||||||
staging_root = os.path.join(download_dir, 'ssync_staging')
|
|
||||||
try:
|
|
||||||
os.makedirs(staging_root, exist_ok=True)
|
|
||||||
except OSError as mk_err:
|
|
||||||
logger.error(f"[Reorganize] Cannot create staging dir {staging_root}: {mk_err}")
|
|
||||||
return {
|
|
||||||
'status': 'setup_failed',
|
|
||||||
'source': None,
|
|
||||||
'total': 0, 'moved': 0, 'skipped': 0, 'failed': 0,
|
|
||||||
'errors': [{'error': f'Could not create staging dir: {mk_err}'}],
|
|
||||||
}
|
|
||||||
|
|
||||||
def _cleanup_empty(src_dir):
|
def _cleanup_empty(src_dir):
|
||||||
try:
|
try:
|
||||||
|
|
@ -105,6 +95,42 @@ def build_runner(
|
||||||
# Progress fan-out failures must never break a run.
|
# Progress fan-out failures must never break a run.
|
||||||
logger.debug("reorganize progress fan-out: %s", e)
|
logger.debug("reorganize progress fan-out: %s", e)
|
||||||
|
|
||||||
|
# Rename-only mode (#875): just move files to the current scheme — no staging,
|
||||||
|
# no copy, no post-processing. Falls through to the full pipeline otherwise.
|
||||||
|
if getattr(item, 'rename_only', False):
|
||||||
|
if build_final_path_fn is None:
|
||||||
|
return {
|
||||||
|
'status': 'setup_failed', 'source': None,
|
||||||
|
'total': 0, 'moved': 0, 'skipped': 0, 'failed': 0,
|
||||||
|
'errors': [{'error': 'Rename-only mode unavailable (no path builder)'}],
|
||||||
|
}
|
||||||
|
return reorganize_album_rename_only(
|
||||||
|
album_id=item.album_id,
|
||||||
|
db=get_database(),
|
||||||
|
transfer_dir=transfer_dir,
|
||||||
|
resolve_file_path_fn=resolve_file_path_fn,
|
||||||
|
build_final_path_fn=build_final_path_fn,
|
||||||
|
update_track_path_fn=_update_track_path,
|
||||||
|
cleanup_empty_dir_fn=_cleanup_empty,
|
||||||
|
on_progress=_on_progress,
|
||||||
|
primary_source=item.source,
|
||||||
|
strict_source=bool(item.source),
|
||||||
|
metadata_source=getattr(item, 'metadata_source', 'api') or 'api',
|
||||||
|
stop_check=is_shutting_down_fn,
|
||||||
|
)
|
||||||
|
|
||||||
|
staging_root = os.path.join(download_dir, 'ssync_staging')
|
||||||
|
try:
|
||||||
|
os.makedirs(staging_root, exist_ok=True)
|
||||||
|
except OSError as mk_err:
|
||||||
|
logger.error(f"[Reorganize] Cannot create staging dir {staging_root}: {mk_err}")
|
||||||
|
return {
|
||||||
|
'status': 'setup_failed',
|
||||||
|
'source': None,
|
||||||
|
'total': 0, 'moved': 0, 'skipped': 0, 'failed': 0,
|
||||||
|
'errors': [{'error': f'Could not create staging dir: {mk_err}'}],
|
||||||
|
}
|
||||||
|
|
||||||
return reorganize_album(
|
return reorganize_album(
|
||||||
album_id=item.album_id,
|
album_id=item.album_id,
|
||||||
db=get_database(),
|
db=get_database(),
|
||||||
|
|
|
||||||
|
|
@ -73,6 +73,9 @@ def _make_item(*, queue_id='qid-1', album_id='alb-1', source=None):
|
||||||
item.queue_id = queue_id
|
item.queue_id = queue_id
|
||||||
item.album_id = album_id
|
item.album_id = album_id
|
||||||
item.source = source
|
item.source = source
|
||||||
|
# Match the real QueueItem default: a bare MagicMock would return a truthy
|
||||||
|
# mock for .rename_only and wrongly take the rename-only branch (#875).
|
||||||
|
item.rename_only = False
|
||||||
return item
|
return item
|
||||||
|
|
||||||
|
|
||||||
|
|
@ -233,3 +236,57 @@ def test_runner_progress_callback_forwards_to_queue(monkeypatch, tmp_path):
|
||||||
# The progress fan-out happened *while* the item was running. The
|
# The progress fan-out happened *while* the item was running. The
|
||||||
# final snapshot shows the worker-set values — what we're really
|
# final snapshot shows the worker-set values — what we're really
|
||||||
# asserting is that progress callbacks didn't raise.
|
# asserting is that progress callbacks didn't raise.
|
||||||
|
|
||||||
|
|
||||||
|
def test_rename_only_item_routes_to_rename_executor(monkeypatch, tmp_path):
|
||||||
|
"""#875: an item with rename_only=True invokes the rename-only executor (NOT the
|
||||||
|
full reorganize_album), and never creates a staging dir."""
|
||||||
|
captured = {}
|
||||||
|
|
||||||
|
def fake_rename_only(**kwargs):
|
||||||
|
captured.update(kwargs)
|
||||||
|
return {'status': 'completed', 'source': 'deezer',
|
||||||
|
'total': 1, 'moved': 1, 'skipped': 0, 'failed': 0, 'errors': []}
|
||||||
|
|
||||||
|
def fail_full(**kwargs):
|
||||||
|
raise AssertionError("full reorganize_album must NOT run for rename_only")
|
||||||
|
|
||||||
|
monkeypatch.setattr('core.library_reorganize.reorganize_album', fail_full, raising=True)
|
||||||
|
monkeypatch.setattr('core.library_reorganize.reorganize_album_rename_only',
|
||||||
|
fake_rename_only, raising=True)
|
||||||
|
|
||||||
|
runner = build_runner(
|
||||||
|
get_database=lambda: object(),
|
||||||
|
resolve_file_path_fn=lambda p: p,
|
||||||
|
post_process_fn=lambda *a, **k: None,
|
||||||
|
cleanup_empty_directories_fn=lambda *a, **k: None,
|
||||||
|
is_shutting_down_fn=lambda: False,
|
||||||
|
get_download_path=lambda: str(tmp_path),
|
||||||
|
get_transfer_path=lambda: str(tmp_path / 'transfer'),
|
||||||
|
build_final_path_fn=lambda *a, **k: (None, True),
|
||||||
|
)
|
||||||
|
item = _make_item(album_id='alb-R', source='deezer')
|
||||||
|
item.rename_only = True
|
||||||
|
summary = runner(item)
|
||||||
|
|
||||||
|
assert summary['status'] == 'completed' and summary['moved'] == 1
|
||||||
|
assert captured['album_id'] == 'alb-R'
|
||||||
|
assert callable(captured['build_final_path_fn'])
|
||||||
|
assert not (tmp_path / 'ssync_staging').exists() # no staging for rename-only
|
||||||
|
|
||||||
|
|
||||||
|
def test_rename_only_without_path_builder_fails_cleanly(monkeypatch, tmp_path):
|
||||||
|
# Defensive: build_final_path_fn omitted → rename-only can't run, returns setup_failed
|
||||||
|
# instead of crashing.
|
||||||
|
runner = build_runner(
|
||||||
|
get_database=lambda: object(),
|
||||||
|
resolve_file_path_fn=lambda p: p,
|
||||||
|
post_process_fn=lambda *a, **k: None,
|
||||||
|
cleanup_empty_directories_fn=lambda *a, **k: None,
|
||||||
|
is_shutting_down_fn=lambda: False,
|
||||||
|
get_download_path=lambda: str(tmp_path),
|
||||||
|
get_transfer_path=lambda: str(tmp_path / 'transfer'),
|
||||||
|
)
|
||||||
|
item = _make_item()
|
||||||
|
item.rename_only = True
|
||||||
|
assert runner(item)['status'] == 'setup_failed'
|
||||||
|
|
|
||||||
|
|
@ -11767,6 +11767,9 @@ def reorganize_album_files(album_id):
|
||||||
artist_name=meta['artist_name'],
|
artist_name=meta['artist_name'],
|
||||||
source=chosen_source,
|
source=chosen_source,
|
||||||
metadata_source=metadata_source,
|
metadata_source=metadata_source,
|
||||||
|
# Rename-only (#875): just move files to the current naming scheme — skip
|
||||||
|
# the copy + post-processing (re-tag / quality / AcoustID) of the full flow.
|
||||||
|
rename_only=bool(data.get('rename_only')),
|
||||||
)
|
)
|
||||||
return jsonify({"success": True, **result})
|
return jsonify({"success": True, **result})
|
||||||
except Exception as e:
|
except Exception as e:
|
||||||
|
|
@ -11883,6 +11886,9 @@ try:
|
||||||
get_transfer_path=lambda: docker_resolve_path(
|
get_transfer_path=lambda: docker_resolve_path(
|
||||||
config_manager.get('soulseek.transfer_path', './Transfer')
|
config_manager.get('soulseek.transfer_path', './Transfer')
|
||||||
),
|
),
|
||||||
|
# Rename-only mode (#875) computes destinations via the same path builder the
|
||||||
|
# preview uses, so apply matches exactly what the user saw.
|
||||||
|
build_final_path_fn=lambda *a, **kw: _build_final_path_for_track(*a, **kw),
|
||||||
))
|
))
|
||||||
except Exception as _runner_init_err:
|
except Exception as _runner_init_err:
|
||||||
logger.error(f"Failed to register reorganize queue runner: {_runner_init_err}")
|
logger.error(f"Failed to register reorganize queue runner: {_runner_init_err}")
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue