streamline shuttle move/copy operations

backend/src/cms_backend/shuttle/move_files.py

@@ -70,71 +70,56 @@ 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(
+    for target_location in target_locations:
+        source_path = source_location.full_local_path(
             ShuttleContext.local_warehouse_paths
         )
         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}")
+        shutil.copy(source_path, target_path)
+        logger.debug(f"Copied book {book.id} from {source_path} to {target_path}")
         book.events.append(
-            f"{getnow()}: copied book from {current_location.full_str} to "
+            f"{getnow()}: copied book from {source_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
-        )
-        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)
-        current_path.unlink()
-        logger.debug(f"Moved book {book.id} from {current_path} to {target_path}")
-        book.events.append(
-            f"{getnow()}: moved book from {current_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 old location for book {book.id} at {del_path}")
+            book.events.append(
+                f"{getnow()}: deleted old location {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}"
-        )
+        del_path.unlink(missing_ok=True)
+        logger.debug(f"Deleted old location for book {book.id} at {del_path}")
         book.events.append(
             f"{getnow()}: deleted old location {current_location.full_str}"
         )
-        current_locations.remove(current_location)
         book.locations.remove(current_location)
         session.delete(current_location)
 
     book.needs_file_operation = False
+    session.flush()

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 old location" 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 old location" 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,16 @@ 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("copied book from" in event for event in book.events)
     assert any("deleted old location" 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_updates_book_locations(