From 6390c528bf3f697ff3b36b42ed7b214e77220c0a Mon Sep 17 00:00:00 2001 From: Uchechukwu Orji Date: Thu, 7 May 2026 11:03:18 +0100 Subject: [PATCH] streamline shuttle move/copy operations --- backend/src/cms_backend/shuttle/move_files.py | 100 +++++++++--------- backend/tests/shuttle/test_move_files.py | 69 ++++++++++-- 2 files changed, 114 insertions(+), 55 deletions(-) diff --git a/backend/src/cms_backend/shuttle/move_files.py b/backend/src/cms_backend/shuttle/move_files.py index 50c3f594..992aba69 100644 --- a/backend/src/cms_backend/shuttle/move_files.py +++ b/backend/src/cms_backend/shuttle/move_files.py @@ -70,71 +70,73 @@ def move_book_files(session: OrmSession, book: Book): book.needs_file_operation = False return - # start with copies - while len(target_locations) > len(current_locations): - current_location = current_locations[0] - target_location = target_locations[0] + # Grab one valid source file to copy from since it's the same + # file spread across multiple locations with the same book + # data + source_location = current_locations[0] - current_path = current_location.full_local_path( - ShuttleContext.local_warehouse_paths - ) + current_locations_map = { + (loc.warehouse_id, loc.path): loc for loc in current_locations + } + + for target_location in target_locations: target_path = target_location.full_local_path( ShuttleContext.local_warehouse_paths ) - - target_path.parent.mkdir(parents=True, exist_ok=True) - shutil.copy(current_path, target_path) - logger.debug(f"Copied book {book.id} from {current_path} to {target_path}") - book.events.append( - f"{getnow()}: copied book from {current_location.full_str} to " - f"{target_location.full_str}" - ) - target_locations.remove(target_location) - target_location.status = "current" - - # continue with moves - while len(current_locations) > 0 and len(target_locations) > 0: - current_location = current_locations[0] - target_location = target_locations[0] - - current_path = current_location.full_local_path( - ShuttleContext.local_warehouse_paths + matching_current = current_locations_map.get( + (target_location.warehouse_id, target_location.path) ) - target_path = target_location.full_local_path( + if matching_current: + # This file is already here. Remove redundant target and remove the current + # location from the cleanup list + session.delete(target_location) + book.locations.remove(target_location) + current_locations.remove(matching_current) + logger.debug(f"Left book {book.id} at identical path {target_path}") + book.events.append( + f"{getnow()}: left book at identical location " + f"{target_location.full_str}" + ) + continue + + source_path = source_location.full_local_path( ShuttleContext.local_warehouse_paths ) - target_path.parent.mkdir(parents=True, exist_ok=True) - shutil.copy(current_path, target_path) - current_path.unlink() - logger.debug(f"Moved book {book.id} from {current_path} to {target_path}") + shutil.copy(source_path, target_path) + logger.debug(f"Copied book {book.id} from {source_path} to {target_path}") book.events.append( - f"{getnow()}: moved book from {current_location.full_str} to " + f"{getnow()}: copied book from {source_location.full_str} to " f"{target_location.full_str}" ) - current_locations.remove(current_location) - target_locations.remove(target_location) - book.locations.remove(current_location) - session.delete(current_location) - session.flush() target_location.status = "current" - # cleanup phase: delete extra current locations - while len(current_locations) > 0: - current_location = current_locations[0] - current_path = current_location.full_local_path( + # After making one copy, delete one of the current locations + # that is not the original to avoid filling disk up with copies + if len(current_locations) > 1: + loc_to_delete = current_locations.pop() + del_path = loc_to_delete.full_local_path( + ShuttleContext.local_warehouse_paths + ) + del_path.unlink(missing_ok=True) + logger.debug(f"Deleted book {book.id} from {del_path}") + book.events.append( + f"{getnow()}: deleted book from {loc_to_delete.full_str}" + ) + book.locations.remove(loc_to_delete) + session.delete(loc_to_delete) + + # Cleanup the original source location and any extra location (if we + # started with more currents than targets) + for current_location in current_locations: + del_path = current_location.full_local_path( ShuttleContext.local_warehouse_paths ) - - current_path.unlink(missing_ok=True) - logger.debug( - f"Deleted extra current location for book {book.id} at {current_path}" - ) - book.events.append( - f"{getnow()}: deleted old location {current_location.full_str}" - ) - current_locations.remove(current_location) + del_path.unlink(missing_ok=True) + logger.debug(f"Deleted book {book.id} from {del_path}") + book.events.append(f"{getnow()}: deleted book from {current_location.full_str}") book.locations.remove(current_location) session.delete(current_location) book.needs_file_operation = False + session.flush() diff --git a/backend/tests/shuttle/test_move_files.py b/backend/tests/shuttle/test_move_files.py index 5ab98113..0a52d170 100644 --- a/backend/tests/shuttle/test_move_files.py +++ b/backend/tests/shuttle/test_move_files.py @@ -141,9 +141,9 @@ def test_move_book_files_copy_operation( assert book.needs_processing is False assert book.has_error is False assert book.needs_file_operation is False + assert sum(1 for loc in book.locations if loc.status == "current") == 2 assert any("copied book from" in event for event in book.events) - # One target should now be current - assert sum(1 for loc in book.locations if loc.status == "current") >= 1 + assert any("deleted book" in event for event in book.events) def test_move_book_files_move_operation( @@ -180,14 +180,15 @@ def test_move_book_files_move_operation( mock_context.local_warehouse_paths = {warehouse.id: Path("/warehouse")} move_book_files(dbsession, book) - # Should have moved once + # Should have copied once and unlinked once assert mock_copy.call_count == 1 assert mock_unlink.call_count == 1 assert book.needs_processing is False assert book.has_error is False assert book.needs_file_operation is False - assert any("moved book from" in event for event in book.events) + assert any("copied book from" in event for event in book.events) + assert any("deleted book" in event for event in book.events) # Current location should be removed assert len([loc for loc in book.locations if loc.status == "current"]) == 1 @@ -229,14 +230,70 @@ def test_move_book_files_delete_operation( mock_context.local_warehouse_paths = {warehouse.id: Path("/warehouse")} move_book_files(dbsession, book) - # Should have deleted one extra location + # Should have copied once and deleted two extra locations assert mock_copy.call_count == 1 assert mock_unlink.call_count == 2 assert book.needs_processing is False assert book.has_error is False assert book.needs_file_operation is False - assert any("deleted old location" in event for event in book.events) + assert any("copied book from" in event for event in book.events) + assert any("deleted book" in event for event in book.events) + assert len([loc for loc in book.locations if loc.status == "current"]) == 1 + + +def test_move_book_files_identical_source_and_target_paths( + dbsession: OrmSession, + create_book: Callable[..., Book], + create_book_location: Callable[..., BookLocation], + create_warehouse: Callable[..., Warehouse], +): + """ + Test that move_book_files does not copy or delete if a target path matches a + current path + """ + warehouse = create_warehouse() + book = create_book() + book.needs_file_operation = True + dbsession.flush() + + # One current location + create_book_location( + book=book, warehouse_id=warehouse.id, path="same_path", status="current" + ) + + # One target location with exactly the same path + create_book_location( + book=book, warehouse_id=warehouse.id, path="same_path", status="target" + ) + # And another target location with a different path + create_book_location( + book=book, warehouse_id=warehouse.id, path="new_path", status="target" + ) + dbsession.flush() + + with ExitStack() as stack: + mock_context = stack.enter_context( + patch("cms_backend.shuttle.move_files.ShuttleContext") + ) + mock_copy = stack.enter_context(patch("shutil.copy")) + mock_unlink = stack.enter_context(patch("pathlib.Path.unlink")) + stack.enter_context(patch("pathlib.Path.mkdir")) + + mock_context.local_warehouse_paths = {warehouse.id: Path("/warehouse")} + move_book_files(dbsession, book) + + # Should only copy once (to "new_path") because "same_path" is identical + assert mock_copy.call_count == 1 + # Should NOT unlink "same_path" because it's in the target paths + assert mock_unlink.call_count == 0 + + assert book.needs_processing is False + assert book.has_error is False + assert book.needs_file_operation is False + assert sum(1 for loc in book.locations if loc.status == "current") == 2 + assert any("left book at" in event for event in book.events) + assert any("copied book from" in event for event in book.events) def test_move_book_files_updates_book_locations(