UN-3217 [FEAT] Show specific pipeline/API names in workflow deletion error message #1784
+67
−30
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pk-zipstack
requested review from
hari-kuriakose,
harini-venkataraman and
muhammad-ali-e
Contributor
Summary by CodeRabbit
WalkthroughBackend Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend as "Frontend\n(Workflows.jsx)"
participant Backend as "Backend\n(workflow_helper.py)"
participant DB as "Database"
User->>Frontend: request delete workflow
Frontend->>Backend: checkWorkflowUsage(workflowId)
Backend->>DB: query pipelines by workflowId
Backend->>DB: query api_deployments by workflowId
DB-->>Backend: pipeline_names, api_names
Backend-->>Frontend: {can_update, pipeline_names, api_names}
alt can_update == true
Frontend->>Backend: delete workflow
Backend->>DB: delete records
Backend-->>Frontend: success
Frontend-->>User: success message
else can_update == false
Frontend->>Frontend: getUsageMessage(pipeline_names, api_names)
Frontend-->>User: show contextual error message
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/src/components/workflows/workflow/Workflows.jsx`:
- Around line 149-159: checkWorkflowUsage currently mixes await with .then() and
swallows API errors by returning a default result; change checkWorkflowUsage to
use await/try and rethrow on failure (or return a rejected promise) so callers
can detect errors, and update deleteProject to wrap the call to
checkWorkflowUsage in a try/catch that on error invokes handleException and sets
setAlertDetails with the returned error (including a contextual message like
"Unable to delete workflow ${project.id}"); reference checkWorkflowUsage for the
API call change and deleteProject for adding the try/catch and
setAlertDetails(handleException(...)) error path.
🧹 Nitpick comments (1)
backend/workflow_manager/workflow_v2/workflow_helper.py (1)
998-1002: Pre-existing: redundant guard and wrong variable in error log.Two minor pre-existing issues in this method:
- Line 1001:
if not workflow or workflow is None:is dead code —Workflow.objects.get()raisesDoesNotExiston missing rows and never returnsNone/falsy. This branch is unreachable.- Line 1022: The error log references
id(Python built-in) instead ofworkflow_id.Neither is introduced by this PR, but since you're touching this method, it's a good opportunity to clean them up.
Suggested cleanup
def can_update_workflow(workflow_id: str) -> dict[str, Any]: try: workflow: Workflow = Workflow.objects.get(pk=workflow_id) - if not workflow or workflow is None: - raise WorkflowDoesNotExistError() pipeline_names = list(except Workflow.DoesNotExist: - logger.error(f"Error getting workflow: {id}") + logger.error(f"Error getting workflow: {workflow_id}") raise WorkflowDoesNotExistError()
Contributor
Test ResultsSummary
Runner Tests - Full Report
SDK1 Tests - Full Report
|
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@frontend/src/components/workflows/workflow/Workflows.jsx`:
- Around line 158-169: getUsageMessage currently returns an empty string when
can_update is false but backend returns no pipeline_names/api_names, causing a
blank error toast; update the getUsageMessage function to return a sensible
fallback message (e.g., `Cannot delete "<workflowName>" because it is in use.`
or similar) when total === 0 so the UI always displays a readable error; ensure
you update the return for the total === 0 branch in getUsageMessage and keep
existing firstName/remaining logic intact.
🧹 Nitpick comments (1)
frontend/src/components/workflows/workflow/Workflows.jsx (1)
171-204: Mixingawaitwith.then()/.catch()insidedeleteProject.
checkWorkflowUsageis properlyawaited, but thedeleteProjectcall on Line 175 uses.then()/.catch()withoutawait. This works but is inconsistent. If you ever need to act after deletion completes (e.g., analytics), the missingawaitwill bite. Considerawaiting it for consistency.Suggested refactor
if (usage.can_update) { - projectApiService - .deleteProject(project.id) - .then(() => { - getProjectList(); - setAlertDetails({ - type: "success", - content: "Workflow deleted successfully", - }); - }) - .catch((err) => { - setAlertDetails( - handleException(err, `Unable to delete workflow ${project.id}`) - ); + try { + await projectApiService.deleteProject(project.id); + getProjectList(); + setAlertDetails({ + type: "success", + content: "Workflow deleted successfully", }); + } catch (err) { + setAlertDetails( + handleException(err, `Unable to delete workflow ${project.id}`) + ); + } } else {
Comment on lines
+158
to
169
| const getUsageMessage = (workflowName, pipelineNames, apiNames) => { | ||
| const allNames = [...apiNames, ...pipelineNames]; | ||
| const total = allNames.length; | ||
| if (total === 0) return ""; | ||
| const firstName = `"${allNames[0]}"`; | ||
| if (total === 1) { | ||
| return `Cannot delete "${workflowName}" as it is used in ${firstName}.`; | ||
| } | ||
| const remaining = total - 1; | ||
| const pipelineLabel = remaining === 1 ? "pipeline" : "pipelines"; | ||
| return `Cannot delete "${workflowName}" as it is used in ${firstName} and ${remaining} other API/ETL/Task ${pipelineLabel}.`; | ||
| }; |
Contributor
There was a problem hiding this comment.
Empty error toast if can_update is false but backend returns no names.
If the backend returns can_update: false with empty pipeline_names and api_names (e.g., a race condition or backend bug), getUsageMessage returns "", resulting in a blank error notification at Line 192. Add a fallback message.
Suggested fix
const total = allNames.length;
- if (total === 0) return "";
+ if (total === 0)
+ return `Cannot delete "${workflowName}" as it is currently in use.`;
const firstName = `"${allNames[0]}"`;🤖 Prompt for AI Agents
In `@frontend/src/components/workflows/workflow/Workflows.jsx` around lines 158 -
169, getUsageMessage currently returns an empty string when can_update is false
but backend returns no pipeline_names/api_names, causing a blank error toast;
update the getUsageMessage function to return a sensible fallback message (e.g.,
`Cannot delete "<workflowName>" because it is in use.` or similar) when total
=== 0 so the UI always displays a readable error; ensure you update the return
for the total === 0 branch in getUsageMessage and keep existing
firstName/remaining logic intact.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What
Why
How
workflow_helper.py): Modifiedcan_update_workflowto returnpipeline_namesandapi_nameslists alongside the existingcan_updateboolean, by queryingPipelineandAPIDeploymentmodels for their display names.Workflows.jsx): AddedgetUsageMessagehelper that constructs a contextual message showing the first pipeline/API name and a count of remaining ones. Examples:Cannot delete "A" as it is used in "B".Cannot delete "A" as it is used in "B" and 2 other API/ETL/Task pipelines.Can this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)
can_updateboolean in the API response is unchanged in behavior. The two new fields (pipeline_names,api_names) are additive. The frontend gracefully defaults to empty arrays if the new fields are absent, so backward compatibility is preserved.Database Migrations
Env Config
Relevant Docs
Related Issues or PRs
Dependencies Versions
Notes on Testing
Screenshots
Checklist
I have read and understood the Contribution Guidelines.