feat: feedback verbetering 03

2026-03-15 14:16:17 +01:00
parent 492082c2b7
commit 3d82699535
10 changed files with 197 additions and 49 deletions
@@ -2,6 +2,7 @@ from __future__ import annotations

 import uuid
 from datetime import datetime, timezone
+from pathlib import Path

 from backend.app.api.errors import AppError
 from backend.app.api.schemas import TaskCreateResponse
@@ -26,31 +27,12 @@ class DeleteTaskService:

    def create_delete_task(self, path: str, recursive: bool = False) -> TaskCreateResponse:
        try:
-            resolved_target = self._path_guard.resolve_existing_path(path)
-
-            if resolved_target.absolute.is_file():
-                kind = "file"
-            elif resolved_target.absolute.is_dir():
-                kind = "directory"
-                if not recursive and any(resolved_target.absolute.iterdir()):
-                    raise AppError(
-                        code="directory_not_empty",
-                        message="Directory is not empty",
-                        status_code=409,
-                        details={"path": resolved_target.relative},
-                    )
-            else:
-                raise AppError(
-                    code="type_conflict",
-                    message="Unsupported path type for delete",
-                    status_code=409,
-                    details={"path": resolved_target.relative},
-                )
+            item = self._build_delete_item(path=path, recursive=recursive)

            task_id = str(uuid.uuid4())
            task = self._repository.create_task(
                operation="delete",
-                source=resolved_target.relative,
+                source=item["relative_path"],
                destination="",
                task_id=task_id,
            )
@@ -58,14 +40,9 @@ class DeleteTaskService:
                entry_id=task_id,
                operation="delete",
                status="queued",
-                path=resolved_target.relative,
-            )
-            self._runner.enqueue_delete_path(
-                task_id=task["id"],
-                target=str(resolved_target.absolute),
-                kind=kind,
-                recursive=recursive,
+                path=item["relative_path"],
            )
+            self._runner.enqueue_delete_path(task_id=task["id"], item=item)
            return TaskCreateResponse(task_id=task["id"], status=task["status"])
        except AppError as exc:
            self._record_history(
@@ -94,6 +71,66 @@ class DeleteTaskService:
            )
            raise error

+    def _build_delete_item(self, path: str, recursive: bool) -> dict:
+        resolved_target = self._path_guard.resolve_existing_path(path)
+
+        if resolved_target.absolute.is_file():
+            files = [{"path": str(resolved_target.absolute), "label": resolved_target.absolute.name}]
+            directories: list[str] = []
+            kind = "file"
+        elif resolved_target.absolute.is_dir():
+            kind = "directory"
+            if not recursive and any(resolved_target.absolute.iterdir()):
+                raise AppError(
+                    code="directory_not_empty",
+                    message="Directory is not empty",
+                    status_code=409,
+                    details={"path": resolved_target.relative},
+                )
+            if recursive:
+                files, directories = self._build_recursive_delete_plan(resolved_target.absolute)
+            else:
+                files = []
+                directories = [str(resolved_target.absolute)]
+        else:
+            raise AppError(
+                code="type_conflict",
+                message="Unsupported path type for delete",
+                status_code=409,
+                details={"path": resolved_target.relative},
+            )
+
+        return {
+            "target": str(resolved_target.absolute),
+            "relative_path": resolved_target.relative,
+            "kind": kind,
+            "recursive": recursive,
+            "files": files,
+            "directories": directories,
+            "progress_total_items": len(files),
+            "progress_label": files[0]["label"] if files else None,
+        }
+
+    def _build_recursive_delete_plan(self, root: Path) -> tuple[list[dict[str, str]], list[str]]:
+        files: list[dict[str, str]] = []
+        directories: list[str] = []
+
+        def walk(path: Path, relative_prefix: Path) -> None:
+            for entry in sorted(path.iterdir(), key=lambda child: child.name.lower()):
+                relative_path = relative_prefix / entry.name
+                if entry.is_symlink():
+                    files.append({"path": str(entry), "label": relative_path.as_posix()})
+                    continue
+                if entry.is_dir():
+                    walk(entry, relative_path)
+                    directories.append(str(entry))
+                    continue
+                files.append({"path": str(entry), "label": relative_path.as_posix()})
+
+        walk(root, Path())
+        directories.append(str(root))
+        return files, directories
+
    def _record_history(self, **kwargs) -> None:
        if self._history_repository:
            self._history_repository.create_entry(**kwargs)
@@ -80,10 +80,10 @@ class TaskRunner:
        )
        thread.start()

-    def enqueue_delete_path(self, task_id: str, target: str, kind: str, recursive: bool) -> None:
+    def enqueue_delete_path(self, task_id: str, item: dict[str, object]) -> None:
        thread = threading.Thread(
            target=self._run_delete_path,
-            args=(task_id, target, kind, recursive),
+            args=(task_id, item),
            daemon=True,
        )
        thread.start()
@@ -429,39 +429,57 @@ class TaskRunner:
            total_items=total_items,
        )

-    def _run_delete_path(self, task_id: str, target: str, kind: str, recursive: bool) -> None:
+    def _run_delete_path(self, task_id: str, item: dict[str, object]) -> None:
+        target = str(item["target"])
+        kind = str(item["kind"])
+        recursive = bool(item["recursive"])
+        files = list(item.get("files", []))  # type: ignore[arg-type]
+        directories = list(item.get("directories", []))  # type: ignore[arg-type]
+        total_items = int(item.get("progress_total_items", len(files)))
+        current_item = str(item.get("progress_label")) if item.get("progress_label") else None
        if not self._repository.mark_running(
            task_id=task_id,
            done_items=0,
-            total_items=1,
-            current_item=target,
+            total_items=total_items,
+            current_item=current_item,
        ):
-            self._finalize_if_already_cancelled(task_id, done_items=0, total_items=1)
+            self._finalize_if_already_cancelled(task_id, done_items=0, total_items=total_items)
            return

+        completed_items = 0
        try:
-            path = Path(target)
            if kind == "file":
-                self._filesystem.delete_file(path)
+                file_entry = files[0]
+                completed_items = self._delete_planned_file(task_id, file_entry, completed_items, total_items)
            elif recursive:
-                self._filesystem.delete_directory_recursive(path)
+                for file_entry in files:
+                    if self._is_cancel_requested(task_id):
+                        self._finalize_cancelled(task_id, done_items=completed_items, total_items=total_items)
+                        return
+                    completed_items = self._delete_planned_file(task_id, file_entry, 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
+                for directory in directories:
+                    self._filesystem.delete_empty_directory(Path(directory))
            else:
-                self._filesystem.delete_empty_directory(path)
+                self._filesystem.delete_empty_directory(Path(target))
            self._complete_or_cancel_item_task(
                task_id=task_id,
-                done_items=1,
-                total_items=1,
+                done_items=completed_items,
+                total_items=total_items,
            )
        except OSError as exc:
+            task = self._repository.get_task(task_id)
            self._repository.mark_failed(
                task_id=task_id,
                error_code="io_error",
                error_message=str(exc),
-                failed_item=target,
+                failed_item=(task.get("current_item") if task else None) or target,
                done_bytes=None,
                total_bytes=None,
-                done_items=0,
-                total_items=1,
+                done_items=completed_items,
+                total_items=total_items,
            )
            self._update_history_failed(task_id, str(exc))

@@ -630,6 +648,29 @@ class TaskRunner:
        )
        return completed_items

+    def _delete_planned_file(
+        self,
+        task_id: str,
+        file_entry: dict[str, str],
+        completed_items: int,
+        total_items: int,
+    ) -> int:
+        self._repository.update_progress(
+            task_id=task_id,
+            done_items=completed_items,
+            total_items=total_items,
+            current_item=file_entry["label"],
+        )
+        self._filesystem.delete_file(Path(file_entry["path"]))
+        completed_items += 1
+        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"]),
+        )
+        return completed_items
+
    def _move_directory_item(
        self,
        task_id: str,
@@ -293,6 +293,9 @@ class FileOpsApiGoldenTest(unittest.TestCase):
        detail = self._wait_task(body["task_id"])
        self.assertEqual(detail["operation"], "delete")
        self.assertEqual(detail["status"], "completed")
+        self.assertEqual(detail["done_items"], 1)
+        self.assertEqual(detail["total_items"], 1)
+        self.assertIsNone(detail["current_item"])
        self.assertFalse(target.exists())

    def test_delete_file_cancelled_after_current_delete_finishes(self) -> None:
@@ -330,7 +333,7 @@ class FileOpsApiGoldenTest(unittest.TestCase):
        task_id = response.json()["task_id"]
        self.assertTrue(blocking_fs.entered.wait(timeout=2.0))
        running = self._wait_for_status(task_id, {"running"})
-        self.assertEqual(running["current_item"], str(self.scope / "delete_later.txt"))
+        self.assertEqual(running["current_item"], "delete_later.txt")

        cancel_response = self._post(f"/api/tasks/{task_id}/cancel", {})
        self.assertEqual(cancel_response.status_code, 200)
@@ -358,6 +361,9 @@ class FileOpsApiGoldenTest(unittest.TestCase):
        detail = self._wait_task(body["task_id"])
        self.assertEqual(detail["operation"], "delete")
        self.assertEqual(detail["status"], "completed")
+        self.assertEqual(detail["done_items"], 0)
+        self.assertEqual(detail["total_items"], 0)
+        self.assertIsNone(detail["current_item"])
        self.assertFalse(target.exists())

    def test_delete_not_found(self) -> None:
@@ -424,6 +430,7 @@ class FileOpsApiGoldenTest(unittest.TestCase):
        nested = target / "nested"
        nested.mkdir()
        (nested / "a.txt").write_text("a", encoding="utf-8")
+        (target / "b.txt").write_text("b", encoding="utf-8")

        response = self._post(
            "/api/files/delete",
@@ -436,8 +443,67 @@ class FileOpsApiGoldenTest(unittest.TestCase):
        detail = self._wait_task(body["task_id"])
        self.assertEqual(detail["operation"], "delete")
        self.assertEqual(detail["status"], "completed")
+        self.assertEqual(detail["done_items"], 2)
+        self.assertEqual(detail["total_items"], 2)
+        self.assertIsNone(detail["current_item"])
        self.assertFalse(target.exists())

+    def test_delete_non_empty_directory_recursive_cancelled_after_current_file_finishes(self) -> None:
+        blocking_fs = BlockingDeleteFilesystemAdapter()
+        path_guard = PathGuard({"storage1": str(self.root)})
+        service = FileOpsService(path_guard=path_guard, filesystem=blocking_fs)
+        delete_service = DeleteTaskService(
+            path_guard=path_guard,
+            repository=self.repo,
+            runner=TaskRunner(repository=self.repo, filesystem=blocking_fs),
+        )
+        task_service = TaskService(repository=self.repo)
+
+        async def _override_file_ops_service() -> FileOpsService:
+            return service
+
+        async def _override_delete_task_service() -> DeleteTaskService:
+            return delete_service
+
+        async def _override_task_service() -> TaskService:
+            return task_service
+
+        app.dependency_overrides[get_file_ops_service] = _override_file_ops_service
+        app.dependency_overrides[get_delete_task_service] = _override_delete_task_service
+        app.dependency_overrides[get_task_service] = _override_task_service
+
+        target = self.scope / "delete_recursive_later"
+        target.mkdir()
+        nested = target / "nested"
+        nested.mkdir()
+        (target / "a.txt").write_text("a", encoding="utf-8")
+        (nested / "b.txt").write_text("b", encoding="utf-8")
+
+        response = self._post(
+            "/api/files/delete",
+            {"path": "storage1/scope/delete_recursive_later", "recursive": True},
+        )
+
+        task_id = response.json()["task_id"]
+        self.assertTrue(blocking_fs.entered.wait(timeout=2.0))
+        running = self._wait_for_status(task_id, {"running"})
+        self.assertEqual(running["current_item"], "a.txt")
+        self.assertEqual(running["done_items"], 0)
+        self.assertEqual(running["total_items"], 2)
+
+        cancel_response = self._post(f"/api/tasks/{task_id}/cancel", {})
+        self.assertEqual(cancel_response.status_code, 200)
+        self.assertEqual(cancel_response.json()["status"], "cancelling")
+
+        blocking_fs.release.set()
+        detail = self._wait_task(task_id)
+        self.assertEqual(detail["status"], "cancelled")
+        self.assertEqual(detail["done_items"], 1)
+        self.assertEqual(detail["total_items"], 2)
+        self.assertFalse(target.joinpath("a.txt").exists())
+        self.assertTrue(target.joinpath("nested", "b.txt").exists())
+        self.assertTrue(target.exists())
+
    def test_delete_invalid_path(self) -> None:
        response = self._post(
            "/api/files/delete",
@@ -260,7 +260,7 @@ class TasksApiGoldenTest(unittest.TestCase):
            started_at="2026-03-10T10:00:01Z",
            done_items=0,
            total_items=1,
-            current_item="storage1/trash.txt",
+            current_item="trash.txt",
        )

        response = self._get("/api/tasks/task-delete")
@@ -271,7 +271,7 @@ class TasksApiGoldenTest(unittest.TestCase):
        self.assertEqual(body["status"], "running")
        self.assertEqual(body["done_items"], 0)
        self.assertEqual(body["total_items"], 1)
-        self.assertEqual(body["current_item"], "storage1/trash.txt")
+        self.assertEqual(body["current_item"], "trash.txt")

    def test_cancel_running_delete_task_returns_cancelling(self) -> None:
        self._insert_task(
@@ -284,7 +284,7 @@ class TasksApiGoldenTest(unittest.TestCase):
            started_at="2026-03-10T10:00:01Z",
            done_items=0,
            total_items=1,
-            current_item="storage1/trash.txt",
+            current_item="trash.txt",
        )

        response = self._post("/api/tasks/task-delete/cancel")
@@ -293,7 +293,7 @@ class TasksApiGoldenTest(unittest.TestCase):
        body = response.json()
        self.assertEqual(body["operation"], "delete")
        self.assertEqual(body["status"], "cancelling")
-        self.assertEqual(body["current_item"], "storage1/trash.txt")
+        self.assertEqual(body["current_item"], "trash.txt")

    def test_cancel_completed_task_rejected(self) -> None:
        self._insert_task(