Allow for the deletion of workflow reports #8156
Conversation
WalkthroughThe pull request introduces a new feature for deleting workflows in the Voxelytics section of the application, including both backend and frontend components. Key changes involve the addition of methods for deleting workflows in the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
app/controllers/VoxelyticsController.scala (2)
169-169: Enhance audit logging
The current logging only captures the workflow hash. Consider adding more details for better audit trails.
-_ = logger.info(s"Deleting workflow with hash $workflowHash")
+_ = logger.info(s"User ${request.identity.email} deleted workflow with hash $workflowHash")163-172: Consider implementing soft delete
Given that this feature is intended to handle sensitive information, consider implementing a soft delete mechanism instead of permanent deletion. This would:
- Allow for recovery in case of accidental deletions
- Maintain audit trails for compliance
- Provide flexibility in handling running workflows
Consider:
- Adding a
deletedflag to the workflow model - Implementing a separate cleanup job for permanent deletion
- Adding restoration capability for super users
Would you like me to provide a detailed implementation proposal?
frontend/javascripts/admin/admin_rest_api.ts (1)
2504-2508: Add JSDoc documentation for the deleteWorkflow function.
The implementation looks good, but adding JSDoc documentation would improve code maintainability and IDE support.
Add documentation like this:
+/**
+ * Deletes a voxelytics workflow and its associated data.
+ * @param workflowHash - The unique identifier of the workflow to delete
+ * @returns A promise that resolves when the workflow is deleted
+ */
export function deleteWorkflow(workflowHash: string): Promise<void> {
return Request.triggerRequest(`/api/voxelytics/workflows/${workflowHash}`, {
method: "DELETE",
});
}📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- CHANGELOG.unreleased.md (1 hunks)
- app/controllers/VoxelyticsController.scala (3 hunks)
- app/models/voxelytics/VoxelyticsDAO.scala (1 hunks)
- conf/webknossos.latest.routes (1 hunks)
- frontend/javascripts/admin/admin_rest_api.ts (1 hunks)
- frontend/javascripts/admin/voxelytics/task_list_view.tsx (3 hunks)
🔇 Additional comments (7)
CHANGELOG.unreleased.md (1)
22-22:
Enhance the changelog entry for workflow report deletion.
The current entry "Workflow reports may be deleted" should be expanded to provide more comprehensive information about this security-sensitive feature. Consider adding details about:
- The purpose (protecting sensitive information)
- Any restrictions or permissions
- Potential risks when deleting running workflows
Here's a suggested expansion:
-Workflow reports may be deleted. [#8156](https://github.com/scalableminds/webknossos/pull/8156)
+Added the ability to delete workflow reports to prevent sharing of sensitive information. Note: Deleting reports of running workflows may cause failures. This feature may be restricted to users with appropriate permissions. [#8156](https://github.com/scalableminds/webknossos/pull/8156)Additionally, consider adding entries under "Breaking Changes" if there are permission changes, and document any required configuration in the migration guide.
Let's check if there are any migration notes:
app/controllers/VoxelyticsController.scala (2)
23-23: LGTM: Clean dependency injection of UserService
The UserService dependency is properly injected following best practices.
166-167: LGTM: Proper security checks implemented
The implementation correctly validates both feature flag and super user status before proceeding with deletion.
frontend/javascripts/admin/voxelytics/task_list_view.tsx (1)
53-53: LGTM!
The import statement is correctly added.
conf/webknossos.latest.routes (1)
312-312: LGTM! Route follows RESTful conventions.
The new DELETE route for workflow deletion is well-structured and appropriately placed in the Voxelytics section.
Please ensure the controller implementation:
- Restricts access to authorized users (potentially super users only)
- Handles the case of deleting running workflows gracefully
✅ Verification successful
These scripts will help us locate the controller implementation and verify the access control mechanisms. The previous ast-grep pattern might have failed due to Scala-specific syntax, so we're using alternative approaches to find the relevant code.
Route implementation properly handles security and workflow deletion
The controller implementation satisfies the security requirements:
- Access is restricted to super users via
userService.assertIsSuperUser - Feature flag check is implemented with
wkConf.Features.voxelyticsEnabled - Workflow existence is verified before deletion
- Clean database deletion through VoxelyticsDAO
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the deleteWorkflow implementation to verify access control
ast-grep --pattern 'def deleteWorkflow(workflowHash: String)'
Length of output: 63
Script:
#!/bin/bash
# Let's try a broader search for the controller class and method
rg -A 5 "class VoxelyticsController"
Length of output: 506
Script:
#!/bin/bash
# Search for any Scala files containing VoxelyticsController
fd "VoxelyticsController.scala"
Length of output: 74
Script:
#!/bin/bash
# Search for any mentions of deleteWorkflow in Scala files
rg "deleteWorkflow" --type scala -A 5
Length of output: 1660
app/models/voxelytics/VoxelyticsDAO.scala (1)
1138-1150: Consider wrapping deletion operations in a transaction.
The implementation correctly handles both the workflow deletion and job reference cleanup. However, there are a few improvements to consider:
- Wrap both operations in a transaction to ensure atomicity
- Add validation to check if the workflow exists before deletion
- Consider handling orphaned records in related tables
Here's a suggested implementation with transactions and validation:
def deleteWorkflow(hash: String): Fox[Unit] =
for {
+ _ <- findWorkflowByHash(hash).map(_ => ()).orElse(Fox.fail("Workflow not found"))
+ _ <- sqlClient.withTransaction { implicit c =>
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 ()Let's verify potential orphaned records:
frontend/javascripts/admin/admin_rest_api.ts (1)
2504-2508: Verify the workflow deletion implementation.
The function implementation looks correct. Let's verify its usage and error handling in the codebase.
✅ Verification successful
The workflow deletion implementation is properly handled
The implementation is well-structured with appropriate error handling:
- The
deleteWorkflowfunction is correctly implemented usingRequest.triggerRequest - The usage in
task_list_view.tsxshows proper error handling:- Confirmation modal before deletion
- Try-catch block around the deletion
- Success message on completion
- Proper navigation after deletion
- The
Requestclass provides robust error handling:- HTTP status handling via
handleStatus - Error response parsing and formatting
- Toast notifications for user feedback
- Timeout handling
- Comprehensive error logging
- HTTP status handling via
The implementation follows good practices for both the API call and its usage in the UI.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for usage of deleteWorkflow function and related error handling
# Test: Search for imports and usage of deleteWorkflow
echo "Searching for deleteWorkflow usage..."
rg "deleteWorkflow" --type ts
# Test: Search for related error handling
echo "Searching for error handling..."
rg "deleteWorkflow.*catch" --type ts
Length of output: 823
Script:
#!/bin/bash
# Let's check the error handling implementation in task_list_view.tsx
echo "Checking error handling context in task_list_view.tsx..."
rg -B 5 -A 10 "deleteWorkflowReport" frontend/javascripts/admin/voxelytics/task_list_view.tsx
# Let's also verify if there's any global error handling for API requests
echo "Checking Request.triggerRequest implementation..."
rg -B 5 -A 10 "triggerRequest" frontend/javascripts/admin/admin_rest_api.ts
Length of output: 5897
Script:
#!/bin/bash
# Let's check the Request class implementation for global error handling
echo "Checking Request class implementation..."
rg -l "class Request" --type ts
# Once we find the file, let's examine its content
fd -e ts -x cat {} \; -E "node_modules" -E "dist" -E "build" | rg -A 20 "class Request"
Length of output: 815
Script:
#!/bin/bash
# Let's get the complete implementation of the Request class
echo "Getting complete Request class implementation..."
cat frontend/javascripts/libs/request.ts
Length of output: 11316
| 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") | ||
| _ <- voxelyticsDAO.deleteWorkflow(workflowHash) | ||
| } yield Ok | ||
| } |
There was a problem hiding this comment.
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.
| 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."); | ||
| } | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Add validation for running workflows and user permissions.
The deletion functionality should include additional checks:
- Prevent deletion of running workflows or handle potential failures.
- 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.
| @@ -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" }, | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
CHANGELOG.unreleased.md (1)
22-22: Expand the changelog entry to better reflect the feature's scope.
The current entry "Workflow reports may be deleted" is too brief and doesn't capture important implementation details. Consider expanding it to clarify:
- The restriction to super users
- Any limitations (e.g., handling of running workflows)
- Data cleanup implications
Consider this expanded entry:
-Workflow reports may be deleted. [#8156](https://github.com/scalableminds/webknossos/pull/8156)
+Added capability for super users to delete workflow reports, including cleanup of associated database records. [#8156](https://github.com/scalableminds/webknossos/pull/8156)frontend/javascripts/admin/voxelytics/task_list_view.tsx (1)
Line range hint 424-462: Consider implementing a more robust deletion strategy.
The current implementation could benefit from additional considerations:
-
Workflow State Management:
- Consider implementing a "soft delete" feature that marks workflows as deleted but retains the data for a period
- Add ability to archive workflows instead of permanent deletion
-
Edge Cases:
- Handle partial deletions (e.g., if database deletion succeeds but file cleanup fails)
- Consider implementing a recovery mechanism for accidentally deleted workflows
-
Audit Trail:
- Add logging of deletion events (who deleted what and when)
- Consider keeping a record of deleted workflows for compliance purposes
Would you like me to help implement any of these suggestions?
frontend/javascripts/admin/admin_rest_api.ts (1)
2504-2508: Basic implementation looks good but could use some improvements.
The function correctly implements the workflow deletion endpoint, but consider these enhancements:
- Add input validation for workflowHash:
export function deleteWorkflow(workflowHash: string): Promise<void> {
+ if (!workflowHash || typeof workflowHash !== 'string') {
+ return Promise.reject(new Error('Invalid workflow hash'));
+ }
return Request.triggerRequest(`/api/voxelytics/workflows/${workflowHash}`, {
method: "DELETE",
});
}- Add error handling wrapper:
export function deleteWorkflow(workflowHash: string): Promise<void> {
- return Request.triggerRequest(`/api/voxelytics/workflows/${workflowHash}`, {
- method: "DELETE",
- });
+ try {
+ return Request.triggerRequest(`/api/voxelytics/workflows/${workflowHash}`, {
+ method: "DELETE",
+ });
+ } catch (error) {
+ console.error('Failed to delete workflow:', error);
+ throw error;
+ }
}📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- CHANGELOG.unreleased.md (1 hunks)
- app/controllers/VoxelyticsController.scala (3 hunks)
- app/models/voxelytics/VoxelyticsDAO.scala (1 hunks)
- conf/webknossos.latest.routes (1 hunks)
- frontend/javascripts/admin/admin_rest_api.ts (1 hunks)
- frontend/javascripts/admin/voxelytics/task_list_view.tsx (3 hunks)
🔇 Additional comments (8)
CHANGELOG.unreleased.md (1)
22-22: Verify migration guide updates.
Since this feature introduces new functionality that may require configuration changes (e.g., super user permissions), consider whether updates to the migration guide are needed. If required, add a note in the changelog referencing the migration guide.
Let's check if the migration guide exists and if it needs updates:
app/controllers/VoxelyticsController.scala (2)
6-6: LGTM: Clean dependency injection implementation
The UserService is properly injected following dependency injection best practices.
Also applies to: 23-23
163-172: Several critical considerations for workflow deletion
While the basic implementation is functional, there are important considerations to address:
- The method should check if the workflow is currently running before deletion to prevent potential issues.
- Consider wrapping the deletion operation in a transaction to ensure atomicity.
- Consider enhancing the audit trail beyond basic logging.
- Add validation for the workflowHash parameter format.
Let's verify if workflows can be in a running state:
Suggested improvements:
def deleteWorkflow(workflowHash: String): Action[AnyContent] =
sil.SecuredAction.async { implicit request =>
for {
_ <- bool2Fox(wkConf.Features.voxelyticsEnabled) ?~> "voxelytics.disabled"
_ <- userService.assertIsSuperUser(request.identity)
+ _ <- ObjectId.validateFormat(workflowHash) ?~> "voxelytics.invalidWorkflowHash" ~> BAD_REQUEST
workflow <- voxelyticsDAO.findWorkflowByHash(workflowHash) ?~> "voxelytics.workflowNotFound" ~> NOT_FOUND
+ _ <- voxelyticsDAO.assertWorkflowNotRunning(workflowHash) ?~> "voxelytics.cannotDeleteRunningWorkflow" ~> CONFLICT
_ = logger.info(s"Deleting workflow with hash $workflowHash")
+ _ = logger.info(s"Deletion initiated by user ${request.identity._id} for workflow $workflowHash")
_ <- voxelyticsDAO.deleteWorkflow(workflowHash)
} yield Ok
}Consider adding ScalaDoc documentation:
/**
* Deletes a workflow and its associated data.
*
* @param workflowHash The unique identifier of the workflow
* @return 200 OK if deletion successful
* 400 BAD_REQUEST if workflow hash is invalid
* 401 UNAUTHORIZED if user is not authenticated
* 403 FORBIDDEN if user is not a super user
* 404 NOT_FOUND if workflow doesn't exist
* 409 CONFLICT if workflow is currently running
*/frontend/javascripts/admin/voxelytics/task_list_view.tsx (1)
53-53: LGTM!
The import statement is correctly placed and follows the established naming convention.
conf/webknossos.latest.routes (1)
312-312: LGTM! The route follows RESTful conventions.
The new DELETE route for workflow deletion is well-structured and consistently placed with other Voxelytics endpoints.
Let's verify the authorization checks in the controller:
✅ Verification successful
Authorization check is properly implemented
The verification confirms that the deleteWorkflow method in VoxelyticsController includes the required super user authorization check through userService.assertIsSuperUser(request.identity). The implementation also includes proper error handling for:
- Feature flag check:
wkConf.Features.voxelyticsEnabled - Workflow existence validation:
voxelyticsDAO.findWorkflowByHash - HTTP 404 response for non-existent workflows
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if super user authorization is properly implemented
# Expected: Find authorization check in the deleteWorkflow method
# Search for the controller implementation
ast-grep --pattern 'def deleteWorkflow(workflowHash: String)$$$'
# Search for authorization checks
rg -A 5 'deleteWorkflow.*workflowHash.*String'
Length of output: 2149
app/models/voxelytics/VoxelyticsDAO.scala (2)
1138-1150: 🛠️ Refactor suggestion
Clean up related data for complete deletion.
The current implementation only deletes from voxelytics_workflows and updates jobs, but there might be related data in other tables (e.g., runs, tasks, artifacts) that should be cleaned up for a complete deletion.
Let's verify the related tables that might need cleanup:
#!/bin/bash
# Description: Search for related Voxelytics tables in the codebase
# Test: Look for table definitions or queries. Expect: SQL table names and relationships.
rg "webknossos\.voxelytics_" | grep -i "from\|join\|into"Consider adding cleanup for related tables:
def deleteWorkflow(hash: String): Fox[Unit] =
sqlClient.withTransaction { implicit client =>
for {
_ <- run(q"""
DELETE FROM webknossos.voxelytics_artifacts
WHERE _task IN (
SELECT t._id
FROM webknossos.voxelytics_tasks t
JOIN webknossos.voxelytics_runs r ON r._id = t._run
WHERE r.workflow_hash = $hash
);
DELETE FROM webknossos.voxelytics_tasks
WHERE _run IN (
SELECT _id
FROM webknossos.voxelytics_runs
WHERE workflow_hash = $hash
);
DELETE FROM webknossos.voxelytics_runs
WHERE workflow_hash = $hash;
DELETE FROM webknossos.voxelytics_workflows
WHERE hash = $hash;
UPDATE webknossos.jobs
SET _voxelytics_workflowhash = NULL
WHERE _voxelytics_workflowhash = $hash;
""".asUpdate)
} yield ()
}1138-1150: Verify workflow state before deletion.
The current implementation doesn't check if the workflow is running before deletion. As per the PR objectives, there's a concern about deleting running workflows. Consider adding a check to prevent deletion of active workflows.
Let's verify if there are any running workflows in the codebase:
Consider adding a check for the workflow state before deletion:
- Query the workflow's current state
- If the workflow is running, either:
- Prevent deletion and return an error
- Or implement a force flag to allow deletion of running workflows
frontend/javascripts/admin/admin_rest_api.ts (1)
2504-2508: Verify deletion permission checks.
Since this implements a destructive operation, ensure that proper permission checks are in place.
| @@ -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" }, | |||
There was a problem hiding this comment.
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.
| 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."); | ||
| } | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding safeguards and improving error handling.
The deletion implementation could be enhanced in several ways:
- Add a check to prevent deletion of running workflows
- Add a loading state during deletion
- 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.
| 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(); | |
| } | |
| }, | |
| }); | |
| } |
| 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) | ||
| } yield () | ||
|
|
There was a problem hiding this comment.
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.
I’d say that’s ok. Maybe include a warning in the deletion confirmation modal along the lines of “Note that if the workflow is still running, this may cause it to fail”. Can be a standard text, no need to figure out whether it is, in fact, still running. Thing is, I’m not sure if webknossos can reliably tell that a workflow was cancelled/killed. Sometimes it may appear to be still running in wk, while the vx process is already killed.
That’s right, it should not be shown for non-superusers. Maybe dieknolle3333 can give you some pointers as to how to achieve this. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
frontend/javascripts/admin/voxelytics/task_list_view.tsx (2)
428-446: Add loading state and improve error handling.While the delete functionality works, it could be enhanced for better user experience:
- Add a loading state during deletion
- Provide more specific error messages
Consider this implementation:
async function deleteWorkflowReport() { await modal.confirm({ title: "Delete Workflow Report", content: "Are you sure you want to delete this workflow report?\nNote that if the workflow is still running, this may cause it to fail.", 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(); } }, }); }
469-474: Add tooltip for delete button.Consider adding a tooltip to explain why the delete option is not available for non-super users.
if (isCurrentUserSuperUser) overflowMenu.items?.push({ key: "6", onClick: deleteWorkflowReport, - label: "Delete Workflow Report", + label: ( + <Tooltip title="Only super users can delete workflow reports"> + <div>Delete Workflow Report</div> + </Tooltip> + ), });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
CHANGELOG.unreleased.md(1 hunks)frontend/javascripts/admin/voxelytics/task_list_view.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.unreleased.md
🔇 Additional comments (3)
frontend/javascripts/admin/voxelytics/task_list_view.tsx (3)
53-53: LGTM: Import statements are correctly placed.
The new imports for deleteWorkflow and Redux dependencies are appropriately added.
Also applies to: 57-58
277-278: LGTM: Super user check is properly implemented.
The Redux selector is correctly used to determine the user's super user status.
431-432: LGTM: Warning message about running workflows.
The confirmation modal appropriately warns users about the potential consequences of deleting a running workflow, as suggested in the PR comments by fm3.
| _ <- 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") |
There was a problem hiding this comment.
| _ = logger.info(s"Deleting workflow with hash $workflowHash") | |
| _ = logger.info(s"Deleting workflow with hash $workflowHash in organization ${request.identity._organization}") |
| @@ -1135,4 +1135,17 @@ class VoxelyticsDAO @Inject()(sqlClient: SqlClient)(implicit ec: ExecutionContex | |||
| """.asUpdate) | |||
| } yield () | |||
|
|
|||
| def deleteWorkflow(hash: String): Fox[Unit] = | |||
There was a problem hiding this comment.
| 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; |
There was a problem hiding this comment.
| WHERE hash = $hash; | |
| WHERE hash = $hash; | |
| AND _organization = $organizationId |
| SET _voxelytics_workflowhash = NULL | ||
| WHERE _voxelytics_workflowhash = $hash; |
There was a problem hiding this comment.
| SET _voxelytics_workflowhash = NULL | |
| WHERE _voxelytics_workflowhash = $hash; | |
| SET _voxelytics_workflowHash = NULL | |
| WHERE _voxelytics_workflowHash = $hash; |
| await modal.confirm({ | ||
| title: "Delete Workflow Report", | ||
| content: | ||
| "Are you sure you want to delete this workflow report?\nNote that if the workflow is still running, this may cause it to fail.", |
There was a problem hiding this comment.
| "Are you sure you want to delete this workflow report?\nNote that if the workflow is still running, this may cause it to fail.", | |
| "Are you sure you want to delete this workflow report? This can not be undone.\nNote that if the workflow is still running, this may cause it to fail.", |
| _ <- run(q""" | ||
| UPDATE webknossos.jobs | ||
| SET _voxelytics_workflowHash = NULL | ||
| WHERE _voxelytics_workflowHash = $hash; |
There was a problem hiding this comment.
Now that I think about it, we will have to check here too that this happens only for jobs of this orga (as identified by the _owner reference in the jobs table). Sorry that I missed this in the first round
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
app/controllers/VoxelyticsController.scala (1)
171-171: Enhance response with deletion metadataConsider returning metadata about the deleted workflow to help clients track the operation.
- } yield Ok + } yield JsonOk(Json.obj( + "workflowHash" -> workflowHash, + "organization" -> request.identity._organization, + "deletedAt" -> System.currentTimeMillis() + ))frontend/javascripts/admin/voxelytics/task_list_view.tsx (2)
428-446: Enhance error handling and user feedback.While the basic implementation is good, the error handling could be improved to provide more specific feedback to users.
Consider this enhanced implementation:
async function deleteWorkflowReport() { await modal.confirm({ title: "Delete Workflow Report", content: "Are you sure you want to delete this workflow report? This can not be undone. Note that if the workflow is still running, this may cause it to fail.", okText: "Delete", okButtonProps: { danger: true }, onOk: async () => { + const hide = message.loading("Deleting workflow report...", 0); try { + if (!isCurrentUserSuperUser) { + 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 instanceof Error && error.message === "Insufficient permissions" + ? "You don't have permission to delete workflow reports." + : "Could not delete workflow report. Please try again later." + ); + } finally { + hide(); } }, }); }
469-474: Consider adding a tooltip for better UX.The delete menu item could benefit from a tooltip explaining the super user requirement.
Consider this enhancement:
if (isCurrentUserSuperUser) overflowMenu.items?.push({ key: "6", onClick: deleteWorkflowReport, - label: "Delete Workflow Report", + label: ( + <Tooltip title="This action permanently removes the workflow report"> + <div>Delete Workflow Report</div> + </Tooltip> + ), });app/models/voxelytics/VoxelyticsDAO.scala (1)
1138-1151: Add validation for workflow existence.Consider adding validation to check if the workflow exists before attempting deletion. This would provide better error handling and user feedback.
Apply this diff to add validation:
def deleteWorkflow(hash: String, organizationId: String): Fox[Unit] = for { + // Verify workflow exists before deletion + workflow <- findWorkflowByHashAndOrganization(organizationId, hash) _ <- sqlClient.withTransaction { implicit client => for { _ <- run(q""" DELETE FROM webknossos.voxelytics_workflows WHERE hash = $hash AND _organization = $organizationId; """.asUpdate) _ <- run(q""" UPDATE webknossos.jobs SET _voxelytics_workflowHash = NULL WHERE _voxelytics_workflowHash = $hash; """.asUpdate) } yield () } } yield ()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
CHANGELOG.unreleased.md(1 hunks)app/controllers/VoxelyticsController.scala(3 hunks)app/models/voxelytics/VoxelyticsDAO.scala(1 hunks)frontend/javascripts/admin/voxelytics/task_list_view.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.unreleased.md
🔇 Additional comments (5)
app/controllers/VoxelyticsController.scala (2)
23-23: LGTM: UserService dependency properly injected
The addition of UserService follows the standard dependency injection pattern and is correctly placed in the constructor.
163-172: Verify complete cleanup of workflow data
Let's verify that all references to the workflow are properly cleaned up across the codebase.
✅ Verification successful
The initial results show multiple references to workflows across the codebase. Let's verify the deletion implementation and check if there are any related tables or job references that need cleanup during deletion.
Based on the verification results, I can now provide the final response:
Workflow deletion is properly implemented with complete cleanup
The deletion process is correctly implemented with proper cleanup:
- Deletes the workflow from
voxelytics_workflowstable - Nullifies workflow references in the jobs table by setting
_voxelytics_workflowHash = NULL - Properly scoped to organization context for security
- Requires super user permissions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for potential references to workflows that might need cleanup
echo "Searching for workflow-related database operations..."
rg -l "workflow.*delete|delete.*workflow" --type scala
echo "Searching for workflow references in queries..."
rg "workflowHash|workflow.hash" --type scala
echo "Checking for related collections or tables..."
rg "collection\(.*workflow.*\)" --type scala
Length of output: 10078
Script:
#!/bin/bash
# Let's check for workflow deletion implementation and related cleanup
echo "Checking workflow deletion implementation..."
rg -A 10 "def deleteWorkflow" --type scala
echo "Checking for job-workflow relationships..."
rg -A 5 "_voxelytics_workflowHash" --type scala
echo "Checking for workflow-related tables and their structure..."
rg "CREATE TABLE.*voxelytics" --type scala
Length of output: 3300
frontend/javascripts/admin/voxelytics/task_list_view.tsx (2)
53-53: LGTM!
The imports are correctly organized, bringing in the necessary dependencies for the new deletion functionality and user state management.
Also applies to: 57-58
277-278: LGTM!
The super user check is correctly implemented using Redux selector.
app/models/voxelytics/VoxelyticsDAO.scala (1)
1138-1151: Verify workflow state before deletion.
Per PR objectives, there are concerns about deleting running workflows. Consider adding a check to verify the workflow state before deletion.
Let's verify if there are any running 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 | ||
| _ = logger.info(s"Deleting workflow with hash $workflowHash in organization ${request.identity._organization}") | ||
| _ <- voxelyticsDAO.deleteWorkflow(workflowHash, request.identity._organization) | ||
| } yield Ok | ||
| } |
There was a problem hiding this comment.
Add validation for running workflows and improve error handling
The implementation needs several improvements for robustness:
- As discussed in the PR objectives, we should prevent deletion of running workflows or at least warn users
- The deletion operation should be wrapped in error handling
- Consider using a transaction to ensure atomicity of the deletion operation
Here's a suggested implementation:
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 in organization ${request.identity._organization}")
- _ <- voxelyticsDAO.deleteWorkflow(workflowHash, request.identity._organization)
+ result <- Fox.runAndHandleErrors {
+ voxelyticsDAO.inTransaction { implicit session =>
+ voxelyticsDAO.deleteWorkflow(workflowHash, request.identity._organization)
+ }
+ }
+ _ = if (result.isLeft) {
+ logger.error(s"Failed to delete workflow $workflowHash: ${result.left.get}")
+ }
+ _ <- bool2Fox(result.isRight) ?~> "voxelytics.deletionFailed" ~> INTERNAL_SERVER_ERROR
} yield Ok
}Committable suggestion skipped: line range outside the PR's diff.
| def deleteWorkflow(hash: String, organizationId: String): Fox[Unit] = | ||
| for { | ||
| _ <- run(q""" | ||
| DELETE FROM webknossos.voxelytics_workflows | ||
| WHERE hash = $hash | ||
| AND _organization = $organizationId; | ||
| """.asUpdate) | ||
| _ <- run(q""" | ||
| UPDATE webknossos.jobs | ||
| SET _voxelytics_workflowHash = NULL | ||
| WHERE _voxelytics_workflowHash = $hash; | ||
| """.asUpdate) | ||
| } yield () | ||
|
|
There was a problem hiding this comment.
Wrap database operations in a transaction for data consistency.
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.
Apply this diff to wrap operations in a transaction:
def deleteWorkflow(hash: String, organizationId: String): Fox[Unit] =
for {
- _ <- run(q"""
- DELETE FROM webknossos.voxelytics_workflows
- WHERE hash = $hash
- AND _organization = $organizationId;
- """.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
+ AND _organization = $organizationId;
+ """.asUpdate)
+ _ <- run(q"""
+ UPDATE webknossos.jobs
+ SET _voxelytics_workflowHash = NULL
+ WHERE _voxelytics_workflowHash = $hash;
+ """.asUpdate)
+ } yield ()
+ }
} yield ()📝 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.
| def deleteWorkflow(hash: String, organizationId: String): Fox[Unit] = | |
| for { | |
| _ <- run(q""" | |
| DELETE FROM webknossos.voxelytics_workflows | |
| WHERE hash = $hash | |
| AND _organization = $organizationId; | |
| """.asUpdate) | |
| _ <- run(q""" | |
| UPDATE webknossos.jobs | |
| SET _voxelytics_workflowHash = NULL | |
| WHERE _voxelytics_workflowHash = $hash; | |
| """.asUpdate) | |
| } yield () | |
| def deleteWorkflow(hash: String, organizationId: String): Fox[Unit] = | |
| for { | |
| _ <- sqlClient.withTransaction { implicit client => | |
| for { | |
| _ <- run(q""" | |
| DELETE FROM webknossos.voxelytics_workflows | |
| WHERE hash = $hash | |
| AND _organization = $organizationId; | |
| """.asUpdate) | |
| _ <- run(q""" | |
| UPDATE webknossos.jobs | |
| SET _voxelytics_workflowHash = NULL | |
| WHERE _voxelytics_workflowHash = $hash; | |
| """.asUpdate) | |
| } yield () | |
| } | |
| } yield () |
🛠️ Refactor suggestion
Consider implementing cascade deletes for associated data.
The current implementation only handles the workflow record and job references. Consider adding cascade deletes for:
- Associated runs and their data
- Related artifacts
- Other dependent data
This will prevent orphaned data in the database.
Apply this diff to add cascade deletes:
def deleteWorkflow(hash: String, organizationId: String): Fox[Unit] =
for {
_ <- sqlClient.withTransaction { implicit client =>
for {
+ // Delete associated runs and their data first
+ _ <- run(q"""
+ DELETE FROM webknossos.voxelytics_artifacts a
+ USING webknossos.voxelytics_tasks t,
+ webknossos.voxelytics_runs r
+ WHERE a._task = t._id
+ AND t._run = r._id
+ AND r.workflow_hash = $hash
+ AND r._organization = $organizationId;
+ """.asUpdate)
+ _ <- run(q"""
+ DELETE FROM webknossos.voxelytics_tasks t
+ USING webknossos.voxelytics_runs r
+ WHERE t._run = r._id
+ AND r.workflow_hash = $hash
+ AND r._organization = $organizationId;
+ """.asUpdate)
+ _ <- run(q"""
+ DELETE FROM webknossos.voxelytics_runs
+ WHERE workflow_hash = $hash
+ AND _organization = $organizationId;
+ """.asUpdate)
_ <- run(q"""
DELETE FROM webknossos.voxelytics_workflows
WHERE hash = $hash
AND _organization = $organizationId;
""".asUpdate)
_ <- run(q"""
UPDATE webknossos.jobs
SET _voxelytics_workflowHash = NULL
WHERE _voxelytics_workflowHash = $hash;
""".asUpdate)
} yield ()
}
} yield ()📝 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.
| def deleteWorkflow(hash: String, organizationId: String): Fox[Unit] = | |
| for { | |
| _ <- run(q""" | |
| DELETE FROM webknossos.voxelytics_workflows | |
| WHERE hash = $hash | |
| AND _organization = $organizationId; | |
| """.asUpdate) | |
| _ <- run(q""" | |
| UPDATE webknossos.jobs | |
| SET _voxelytics_workflowHash = NULL | |
| WHERE _voxelytics_workflowHash = $hash; | |
| """.asUpdate) | |
| } yield () | |
| def deleteWorkflow(hash: String, organizationId: String): Fox[Unit] = | |
| for { | |
| _ <- sqlClient.withTransaction { implicit client => | |
| for { | |
| // Delete associated runs and their data first | |
| _ <- run(q""" | |
| DELETE FROM webknossos.voxelytics_artifacts a | |
| USING webknossos.voxelytics_tasks t, | |
| webknossos.voxelytics_runs r | |
| WHERE a._task = t._id | |
| AND t._run = r._id | |
| AND r.workflow_hash = $hash | |
| AND r._organization = $organizationId; | |
| """.asUpdate) | |
| _ <- run(q""" | |
| DELETE FROM webknossos.voxelytics_tasks t | |
| USING webknossos.voxelytics_runs r | |
| WHERE t._run = r._id | |
| AND r.workflow_hash = $hash | |
| AND r._organization = $organizationId; | |
| """.asUpdate) | |
| _ <- run(q""" | |
| DELETE FROM webknossos.voxelytics_runs | |
| WHERE workflow_hash = $hash | |
| AND _organization = $organizationId; | |
| """.asUpdate) | |
| _ <- run(q""" | |
| DELETE FROM webknossos.voxelytics_workflows | |
| WHERE hash = $hash | |
| AND _organization = $organizationId; | |
| """.asUpdate) | |
| _ <- run(q""" | |
| UPDATE webknossos.jobs | |
| SET _voxelytics_workflowHash = NULL | |
| WHERE _voxelytics_workflowHash = $hash; | |
| """.asUpdate) | |
| } yield () | |
| } | |
| } yield () |
(though please wait until at least tomorrow with deploying)
URL of deployed dev instance (used for testing):
Steps to test:
Questions
TODOs:
Issues:
(Please delete unneeded items, merge only when none are left open)
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores