Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow for the deletion of workflow reports #8156

Merged
merged 11 commits into from
Nov 6, 2024
1 change: 1 addition & 0 deletions CHANGELOG.unreleased.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ For upgrade instructions, please check the [migration guide](MIGRATIONS.released
- Added a button to the search popover in the skeleton and segment tab to select all matching non-group results. [#8123](https://github.com/scalableminds/webknossos/pull/8123)
- Unified wording in UI and code: “Magnification”/“mag” is now used in place of “Resolution“ most of the time, compare [https://docs.webknossos.org/webknossos/terminology.html](terminology document). [#8111](https://github.com/scalableminds/webknossos/pull/8111)
- Added support for adding remote OME-Zarr NGFF version 0.5 datasets. [#8122](https://github.com/scalableminds/webknossos/pull/8122)
- Workflow reports may be deleted. [#8156](https://github.com/scalableminds/webknossos/pull/8156)

### Changed
- Some mesh-related actions were disabled in proofreading-mode when using meshfiles that were created for a mapping rather than an oversegmentation. [#8091](https://github.com/scalableminds/webknossos/pull/8091)
Expand Down
13 changes: 13 additions & 0 deletions app/controllers/VoxelyticsController.scala
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package controllers
import com.scalableminds.util.time.Instant
import com.scalableminds.util.tools.{Fox, FoxImplicits}
import models.organization.OrganizationDAO
import models.user.UserService
import models.voxelytics._
import play.api.libs.json._
import play.api.mvc._
Expand All @@ -19,6 +20,7 @@ class VoxelyticsController @Inject()(
organizationDAO: OrganizationDAO,
voxelyticsDAO: VoxelyticsDAO,
voxelyticsService: VoxelyticsService,
userService: UserService,
lokiClient: LokiClient,
wkConf: WkConf,
sil: Silhouette[WkEnv])(implicit ec: ExecutionContext, bodyParsers: PlayBodyParsers)
Expand Down Expand Up @@ -158,6 +160,17 @@ class VoxelyticsController @Inject()(
} yield JsonOk(result)
}

def deleteWorkflow(workflowHash: String): Action[AnyContent] =
sil.SecuredAction.async { implicit request =>
for {
_ <- bool2Fox(wkConf.Features.voxelyticsEnabled) ?~> "voxelytics.disabled"
_ <- userService.assertIsSuperUser(request.identity)
_ <- voxelyticsDAO.findWorkflowByHash(workflowHash) ?~> "voxelytics.workflowNotFound" ~> NOT_FOUND
_ = logger.info(s"Deleting workflow with hash $workflowHash")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
_ = logger.info(s"Deleting workflow with hash $workflowHash")
_ = logger.info(s"Deleting workflow with hash $workflowHash in organization ${request.identity._organization}")

_ <- voxelyticsDAO.deleteWorkflow(workflowHash)
} yield Ok
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add validation for running workflows

The PR objectives mention concerns about deleting running workflows. Consider adding a check to prevent deletion of active workflows.

 def deleteWorkflow(workflowHash: String): Action[AnyContent] =
   sil.SecuredAction.async { implicit request =>
     for {
       _ <- bool2Fox(wkConf.Features.voxelyticsEnabled) ?~> "voxelytics.disabled"
       _ <- userService.assertIsSuperUser(request.identity)
-      _ <- voxelyticsDAO.findWorkflowByHash(workflowHash) ?~> "voxelytics.workflowNotFound" ~> NOT_FOUND
+      workflow <- voxelyticsDAO.findWorkflowByHash(workflowHash) ?~> "voxelytics.workflowNotFound" ~> NOT_FOUND
+      isRunning <- voxelyticsDAO.isWorkflowRunning(workflowHash)
+      _ <- bool2Fox(!isRunning) ?~> "voxelytics.cannotDeleteRunningWorkflow" ~> CONFLICT
       _ = logger.info(s"Deleting workflow with hash $workflowHash")
       _ <- voxelyticsDAO.deleteWorkflow(workflowHash)
     } yield Ok
   }

Committable suggestion was skipped due to low confidence.


def storeWorkflowEvents(workflowHash: String, runName: String): Action[List[WorkflowEvent]] =
sil.SecuredAction.async(validateJson[List[WorkflowEvent]]) { implicit request =>
def createWorkflowEvent(runId: ObjectId, events: List[WorkflowEvent]): Fox[Unit] =
Expand Down
13 changes: 13 additions & 0 deletions app/models/voxelytics/VoxelyticsDAO.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1135,4 +1135,17 @@ class VoxelyticsDAO @Inject()(sqlClient: SqlClient)(implicit ec: ExecutionContex
""".asUpdate)
} yield ()

def deleteWorkflow(hash: String): Fox[Unit] =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def deleteWorkflow(hash: String): Fox[Unit] =
def deleteWorkflow(hash: String, organizationId: String): Fox[Unit] =

for {
_ <- run(q"""
DELETE FROM webknossos.voxelytics_workflows
WHERE hash = $hash;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
WHERE hash = $hash;
WHERE hash = $hash;
AND _organization = $organizationId

""".asUpdate)
_ <- run(q"""
UPDATE webknossos.jobs
SET _voxelytics_workflowhash = NULL
WHERE _voxelytics_workflowhash = $hash;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
SET _voxelytics_workflowhash = NULL
WHERE _voxelytics_workflowhash = $hash;
SET _voxelytics_workflowHash = NULL
WHERE _voxelytics_workflowHash = $hash;

""".asUpdate)
} yield ()

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Consider wrapping the delete operations in a transaction.

The workflow deletion involves multiple database operations that should be atomic to maintain data consistency. If the first operation succeeds but the second fails, it could leave the database in an inconsistent state.

Consider wrapping both operations in a transaction:

def deleteWorkflow(hash: String): Fox[Unit] =
  for {
-   _ <- run(q"""
-                 DELETE FROM webknossos.voxelytics_workflows
-                 WHERE hash = $hash;
-                 """.asUpdate)
-   _ <- run(q"""
-                 UPDATE webknossos.jobs
-                 SET _voxelytics_workflowhash = NULL
-                 WHERE _voxelytics_workflowhash = $hash;
-       """.asUpdate)
+   _ <- sqlClient.withTransaction { implicit client =>
+     for {
+       _ <- run(q"""
+         DELETE FROM webknossos.voxelytics_workflows
+         WHERE hash = $hash;
+         """.asUpdate)
+       _ <- run(q"""
+         UPDATE webknossos.jobs
+         SET _voxelytics_workflowhash = NULL
+         WHERE _voxelytics_workflowhash = $hash;
+         """.asUpdate)
+     } yield ()
+   }
  } yield ()

Committable suggestion was skipped due to low confidence.

}
1 change: 1 addition & 0 deletions conf/webknossos.latest.routes
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,7 @@ POST /verifyEmail
POST /voxelytics/workflows controllers.VoxelyticsController.storeWorkflow()
GET /voxelytics/workflows controllers.VoxelyticsController.listWorkflows()
GET /voxelytics/workflows/:workflowHash controllers.VoxelyticsController.getWorkflow(workflowHash: String, runId: Option[String])
DELETE /voxelytics/workflows/:workflowHash controllers.VoxelyticsController.deleteWorkflow(workflowHash: String)
POST /voxelytics/workflows/:workflowHash/events controllers.VoxelyticsController.storeWorkflowEvents(workflowHash: String, runName: String)
GET /voxelytics/workflows/:workflowHash/chunkStatistics controllers.VoxelyticsController.getChunkStatistics(workflowHash: String, runId: Option[String], taskName: String)
GET /voxelytics/workflows/:workflowHash/artifactChecksums controllers.VoxelyticsController.getArtifactChecksums(workflowHash: String, runId: Option[String], taskName: String, artifactName: Option[String])
Expand Down
6 changes: 6 additions & 0 deletions frontend/javascripts/admin/admin_rest_api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2501,6 +2501,12 @@ export function getVoxelyticsArtifactChecksums(
);
}

export function deleteWorkflow(workflowHash: string): Promise<void> {
return Request.triggerRequest(`/api/voxelytics/workflows/${workflowHash}`, {
method: "DELETE",
});
}

// ### Help / Feedback userEmail
export function sendHelpEmail(message: string) {
return Request.receiveJSON(
Expand Down
22 changes: 21 additions & 1 deletion frontend/javascripts/admin/voxelytics/task_list_view.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ import TaskView from "./task_view";
import { formatLog } from "./log_tab";
import { addAfterPadding, addBeforePadding } from "./utils";
import { LOG_LEVELS } from "oxalis/constants";
import { getVoxelyticsLogs } from "admin/admin_rest_api";
import { getVoxelyticsLogs, deleteWorkflow } from "admin/admin_rest_api";
import ArtifactsDiskUsageList from "./artifacts_disk_usage_list";
import { notEmpty } from "libs/utils";
import type { ArrayElement } from "types/globals";
Expand Down Expand Up @@ -421,6 +421,25 @@ export default function TaskListView({
}
}

async function deleteWorkflowReport() {
await modal.confirm({
title: "Delete Workflow Report",
content: "Are you sure you want to delete this workflow report?",
okText: "Delete",
okButtonProps: { danger: true },
onOk: async () => {
try {
await deleteWorkflow(report.workflow.hash);
history.push("/workflows");
message.success("Workflow report deleted.");
} catch (error) {
console.error(error);
message.error("Could not delete workflow report.");
}
},
});
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add validation for running workflows and user permissions.

The deletion functionality should include additional checks:

  1. Prevent deletion of running workflows or handle potential failures.
  2. Restrict the deletion capability to super users only.

Consider updating the implementation:

 async function deleteWorkflowReport() {
+  // Check if any task is running
+  const hasRunningTasks = report.tasks.some(
+    (task) => task.state === VoxelyticsRunState.RUNNING
+  );
+
+  if (hasRunningTasks) {
+    return message.error(
+      "Cannot delete workflow report while tasks are running."
+    );
+  }
+
   await modal.confirm({
     title: "Delete Workflow Report",
     content: "Are you sure you want to delete this workflow report?",
     okText: "Delete",
     okButtonProps: { danger: true },
     onOk: async () => {
       try {
+        // Add permission check before deletion
+        if (!user.isSuperUser) {
+          throw new Error("Insufficient permissions");
+        }
         await deleteWorkflow(report.workflow.hash);
         history.push("/workflows");
         message.success("Workflow report deleted.");
       } catch (error) {
         console.error(error);
-        message.error("Could not delete workflow report.");
+        message.error(
+          error.message === "Insufficient permissions"
+            ? "You don't have permission to delete workflow reports."
+            : "Could not delete workflow report."
+        );
       }
     },
   });
 }

Committable suggestion was skipped due to low confidence.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider adding safeguards and improving error handling.

The deletion implementation could be enhanced in several ways:

  1. Add a check to prevent deletion of running workflows
  2. Add a loading state during deletion
  3. Provide more specific error messages

Consider this improved implementation:

 async function deleteWorkflowReport() {
+  const isRunning = report.tasks.some(task => task.state === VoxelyticsRunState.RUNNING);
+  if (isRunning) {
+    message.error("Cannot delete a running workflow.");
+    return;
+  }
   await modal.confirm({
     title: "Delete Workflow Report",
     content: "Are you sure you want to delete this workflow report?",
     okText: "Delete",
     okButtonProps: { danger: true },
     onOk: async () => {
+      const hide = message.loading("Deleting workflow report...", 0);
       try {
         await deleteWorkflow(report.workflow.hash);
         history.push("/workflows");
         message.success("Workflow report deleted.");
       } catch (error) {
         console.error(error);
-        message.error("Could not delete workflow report.");
+        message.error(`Failed to delete workflow report: ${error instanceof Error ? error.message : 'Unknown error'}`);
+      } finally {
+        hide();
       }
     },
   });
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
async function deleteWorkflowReport() {
await modal.confirm({
title: "Delete Workflow Report",
content: "Are you sure you want to delete this workflow report?",
okText: "Delete",
okButtonProps: { danger: true },
onOk: async () => {
try {
await deleteWorkflow(report.workflow.hash);
history.push("/workflows");
message.success("Workflow report deleted.");
} catch (error) {
console.error(error);
message.error("Could not delete workflow report.");
}
},
});
}
async function deleteWorkflowReport() {
const isRunning = report.tasks.some(task => task.state === VoxelyticsRunState.RUNNING);
if (isRunning) {
message.error("Cannot delete a running workflow.");
return;
}
await modal.confirm({
title: "Delete Workflow Report",
content: "Are you sure you want to delete this workflow report?",
okText: "Delete",
okButtonProps: { danger: true },
onOk: async () => {
const hide = message.loading("Deleting workflow report...", 0);
try {
await deleteWorkflow(report.workflow.hash);
history.push("/workflows");
message.success("Workflow report deleted.");
} catch (error) {
console.error(error);
message.error(`Failed to delete workflow report: ${error instanceof Error ? error.message : 'Unknown error'}`);
} finally {
hide();
}
},
});
}


const overflowMenu: MenuProps = {
items: [
{ key: "1", onClick: copyAllArtifactPaths, label: "Copy All Artifact Paths" },
Expand All @@ -440,6 +459,7 @@ export default function TaskListView({
),
},
{ key: "5", onClick: showArtifactsDiskUsageList, label: "Show Disk Usage of Artifacts" },
{ key: "6", onClick: deleteWorkflowReport, label: "Delete Workflow Report" },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Conditionally render or disable the delete option.

The delete menu item should be conditionally rendered or disabled based on user permissions and workflow state.

Update the menu item to include these conditions:

-      { key: "6", onClick: deleteWorkflowReport, label: "Delete Workflow Report" },
+      {
+        key: "6",
+        onClick: deleteWorkflowReport,
+        label: "Delete Workflow Report",
+        disabled: !user.isSuperUser || report.tasks.some(
+          (task) => task.state === VoxelyticsRunState.RUNNING
+        ),
+        tooltip: !user.isSuperUser
+          ? "Only super users can delete workflow reports"
+          : report.tasks.some((task) => task.state === VoxelyticsRunState.RUNNING)
+          ? "Cannot delete workflow report while tasks are running"
+          : undefined,
+      },

Committable suggestion was skipped due to low confidence.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add access control for the delete option.

According to the PR objectives, the delete functionality should be restricted to super users. The menu item should be conditionally rendered or disabled based on user permissions.

Consider this implementation:

-      { key: "6", onClick: deleteWorkflowReport, label: "Delete Workflow Report" },
+      {
+        key: "6",
+        onClick: deleteWorkflowReport,
+        label: "Delete Workflow Report",
+        disabled: !isSuperUser,
+        title: !isSuperUser ? "Only super users can delete workflow reports" : undefined
+      },

You'll need to add the isSuperUser prop to the component and pass it from the parent component.

Committable suggestion was skipped due to low confidence.

],
};

Expand Down