feat: feedback verbetering 01

2026-03-15 13:44:38 +01:00
parent 66abf991d8
commit 9a7ca4e2db
8 changed files with 101 additions and 121 deletions
@@ -1,6 +1,5 @@
 from __future__ import annotations

-import os
 from pathlib import Path

 import uuid
@@ -136,8 +135,11 @@ class MoveTaskService:
                    "source": item["source_absolute"],
                    "destination": item["destination_absolute"],
                    "kind": item["kind"],
+                    "same_root": item["same_root"],
                    "files": item["files"],
                    "directories": item["directories"],
+                    "progress_total_items": item["progress_total_items"],
+                    "progress_label": item["progress_label"],
                }
                for item in items
            ],
@@ -226,12 +228,12 @@ class MoveTaskService:
                    details={"path": source, "destination": destination_relative},
                )

+        progress_label = resolved_source.absolute.name
        if source_is_directory:
-            directories, files = self._build_directory_plan(
-                resolved_source=resolved_source,
-                destination_root=destination_absolute,
-                include_root_prefix=include_root_prefix,
-            )
+            files = []
+            directories = []
+            if include_root_prefix:
+                progress_label = resolved_source.absolute.name
        else:
            files = [
                {
@@ -241,6 +243,7 @@ class MoveTaskService:
                }
            ]
            directories = []
+            progress_label = files[0]["label"]

        return {
            "source_relative": resolved_source.relative,
@@ -252,6 +255,8 @@ class MoveTaskService:
            "total_bytes": int(resolved_source.absolute.stat().st_size) if source_is_file else None,
            "files": files,
            "directories": directories,
+            "progress_total_items": 1,
+            "progress_label": progress_label,
        }

    def _map_directory_validation(self, relative_path: str) -> None:
@@ -271,70 +276,6 @@ class MoveTaskService:
    def _join_destination_base(destination_base: str, name: str) -> str:
        return f"{destination_base.rstrip('/')}/{name}" if destination_base.rstrip("/") else f"/{name}"

-    def _build_directory_plan(
-        self,
-        *,
-        resolved_source: ResolvedPath,
-        destination_root: Path,
-        include_root_prefix: bool,
-    ) -> tuple[list[dict[str, str]], list[dict[str, str]]]:
-        directories: list[dict[str, str]] = [
-            {
-                "source": str(resolved_source.absolute),
-                "destination": str(destination_root),
-            }
-        ]
-        files: list[dict[str, str]] = []
-        for root, dirnames, filenames in os.walk(resolved_source.absolute, followlinks=False):
-            root_path = Path(root)
-            dirnames.sort(key=str.lower)
-            filenames.sort(key=str.lower)
-            for name in dirnames:
-                entry = root_path / name
-                if entry.is_symlink():
-                    raise AppError(
-                        code="type_conflict",
-                        message="Source directory must not contain symlinks",
-                        status_code=409,
-                        details={"path": resolved_source.relative},
-                    )
-                relative = entry.relative_to(resolved_source.absolute)
-                directories.append(
-                    {
-                        "source": str(entry),
-                        "destination": str(destination_root / relative),
-                    }
-                )
-            for name in filenames:
-                entry = root_path / name
-                if entry.is_symlink():
-                    raise AppError(
-                        code="type_conflict",
-                        message="Source directory must not contain symlinks",
-                        status_code=409,
-                        details={"path": resolved_source.relative},
-                    )
-                relative = entry.relative_to(resolved_source.absolute)
-                files.append(
-                    {
-                        "source": str(entry),
-                        "destination": str(destination_root / relative),
-                        "label": self._progress_label(
-                            top_level_name=resolved_source.absolute.name,
-                            relative_path=relative,
-                            include_root_prefix=include_root_prefix,
-                        ),
-                    }
-                )
-        return directories, files
-
-    @staticmethod
-    def _progress_label(*, top_level_name: str, relative_path: Path, include_root_prefix: bool) -> str:
-        relative_value = relative_path.as_posix()
-        if not relative_value:
-            return top_level_name
-        return f"{top_level_name}/{relative_value}" if include_root_prefix else relative_value
-
    @staticmethod
    def _is_nested_destination(source: Path, destination: Path) -> bool:
        try:
@@ -298,26 +298,22 @@ class TaskRunner:
            self._update_history_failed(task_id, str(exc))

    def _run_move_directory(self, task_id: str, item: dict[str, object]) -> None:
-        files = self._file_entries(item)
-        directories = self._directory_entries(item)
-        total_items = len(files)
+        total_items = int(item.get("progress_total_items", 1))
+        source_path = self._item_source_path(item)
+        destination_path = self._item_destination_path(item)
+        current_item = str(item.get("progress_label") or Path(source_path).name)
        if not self._repository.mark_running(
            task_id=task_id,
            done_items=0,
            total_items=total_items,
-            current_item=files[0]["label"] if files else None,
+            current_item=current_item,
        ):
            self._finalize_if_already_cancelled(task_id, done_items=0, total_items=total_items)
            return

        try:
-            completed_items = self._move_directory_files(
-                directories,
-                files,
-                task_id=task_id,
-                completed_items=0,
-                total_items=total_items,
-            )
+            self._filesystem.move_directory(source=source_path, destination=destination_path)
+            completed_items = total_items
            if self._is_cancel_requested(task_id):
                self._finalize_cancelled(task_id, done_items=completed_items, total_items=total_items)
                return
@@ -338,8 +334,8 @@ class TaskRunner:
            self._update_history_failed(task_id, str(exc))

    def _run_move_batch(self, task_id: str, items: list[dict[str, str]]) -> None:
-        total_items = self._total_file_count(items)
-        current_item = self._first_file_label(items)
+        total_items = self._total_move_work_count(items)
+        current_item = self._first_move_item_label(items)
        if not self._repository.mark_running(
            task_id=task_id,
            done_items=0,
@@ -501,6 +497,40 @@ class TaskRunner:
            return 0
        return int(task["done_items"])

+    @staticmethod
+    def _item_source_path(item: dict[str, object]) -> str:
+        value = item.get("source")
+        if isinstance(value, str):
+            return value
+        return str(item["source_absolute"])
+
+    @staticmethod
+    def _item_destination_path(item: dict[str, object]) -> str:
+        value = item.get("destination")
+        if isinstance(value, str):
+            return value
+        return str(item["destination_absolute"])
+
+    def _total_move_work_count(self, items: list[dict[str, object]]) -> int:
+        total = 0
+        for item in items:
+            progress_total_items = item.get("progress_total_items")
+            if progress_total_items is not None:
+                total += int(progress_total_items)
+                continue
+            total += len(self._file_entries(item))
+        return total
+
+    def _first_move_item_label(self, items: list[dict[str, object]]) -> str | None:
+        for item in items:
+            progress_label = item.get("progress_label")
+            if isinstance(progress_label, str) and progress_label:
+                return progress_label
+            files = self._file_entries(item)
+            if files:
+                return files[0]["label"]
+        return None
+
    def _copy_single_planned_file(
        self,
        task_id: str,
@@ -607,44 +637,24 @@ class TaskRunner:
        completed_items: int,
        total_items: int,
    ) -> int:
-        return self._move_directory_files(self._directory_entries(item), self._file_entries(item), task_id=task_id, completed_items=completed_items, total_items=total_items)
-
-    def _move_directory_files(
-        self,
-        directories: list[dict[str, str]],
-        files: list[dict[str, str]],
-        *,
-        task_id: str | None = None,
-        completed_items: int = 0,
-        total_items: int = 0,
-    ) -> int:
-        for directory in directories:
-            Path(directory["destination"]).mkdir(parents=True, exist_ok=True)
-        for file_entry in files:
-            if task_id is not None and self._is_cancel_requested(task_id):
-                return completed_items
-            if task_id is not None:
-                self._repository.update_progress(
-                    task_id=task_id,
-                    done_items=completed_items,
-                    total_items=total_items,
-                    current_item=file_entry["label"],
-                )
-            self._filesystem.move_file(source=file_entry["source"], destination=file_entry["destination"])
-            completed_items += 1
-            if task_id is not None:
-                self._repository.update_progress(
-                    task_id=task_id,
-                    done_items=completed_items,
-                    total_items=total_items,
-                    current_item=self._next_item_label_after_completion(completed_items, total_items, file_entry["label"]),
-                )
-            if task_id is not None and self._is_cancel_requested(task_id):
-                return completed_items
-        for directory in reversed(directories):
-            shutil.copystat(Path(directory["source"]), Path(directory["destination"]), follow_symlinks=False)
-        for directory in reversed(directories):
-            self._filesystem.delete_empty_directory(Path(directory["source"]))
+        progress_total_items = int(item.get("progress_total_items", 1))
+        source_path = self._item_source_path(item)
+        destination_path = self._item_destination_path(item)
+        progress_label = str(item.get("progress_label") or Path(source_path).name)
+        self._repository.update_progress(
+            task_id=task_id,
+            done_items=completed_items,
+            total_items=total_items,
+            current_item=progress_label,
+        )
+        self._filesystem.move_directory(source=source_path, destination=destination_path)
+        completed_items += progress_total_items
+        self._repository.update_progress(
+            task_id=task_id,
+            done_items=completed_items,
+            total_items=total_items,
+            current_item=self._next_item_label_after_completion(completed_items, total_items, progress_label),
+        )
        return completed_items

    @staticmethod
@@ -161,6 +161,31 @@ class MoveApiGoldenTest(unittest.TestCase):
        self.assertTrue((self.root1 / "target-parent" / "moved-dir" / "nested.txt").exists())
        self.assertFalse(src_dir.exists())

+    def test_move_directory_success_same_root_with_nested_symlink_keeps_direct_move_semantics(self) -> None:
+        src_dir = self.root1 / "source-dir"
+        src_dir.mkdir()
+        real_dir = self.root1 / "real-dir"
+        real_dir.mkdir()
+        (real_dir / "nested.txt").write_text("hello", encoding="utf-8")
+        (src_dir / "link-dir").symlink_to(real_dir, target_is_directory=True)
+        target_parent = self.root1 / "target-parent"
+        target_parent.mkdir()
+
+        response = self._request(
+            "POST",
+            "/api/files/move",
+            {"source": "storage1/source-dir", "destination": "storage1/target-parent/moved-dir"},
+        )
+
+        self.assertEqual(response.status_code, 202)
+        detail = self._wait_task(response.json()["task_id"])
+        self.assertEqual(detail["status"], "completed")
+        self.assertEqual(detail["done_items"], 1)
+        self.assertEqual(detail["total_items"], 1)
+        self.assertTrue((self.root1 / "target-parent" / "moved-dir").is_dir())
+        self.assertTrue((self.root1 / "target-parent" / "moved-dir" / "link-dir").is_symlink())
+        self.assertFalse(src_dir.exists())
+
    def test_move_success_cross_root_create_task_shape_and_completed(self) -> None:
        src = self.root1 / "source.txt"
        src.write_text("hello", encoding="utf-8")