feat(ui): Add ability to select multiple workflows from list and perform actions on them. Fixes #3185 #3234
Conversation
…abel in the new labels column
… to take up less real estate.
…ait from labels column
…mponent to keep track of show/hide state. Reformat column widths for readability and change format of labels
…ely cosmetic at this point)
…ork done on page load
…-functional delete selected button, must revise to deal with async correctly later
alexec
changed the title
| @@ -31,7 +31,8 @@ export class WorkflowsService { | |||
| 'items.metadata.labels', | |||
| 'items.status.phase', | |||
| 'items.status.finishedAt', | |||
| 'items.status.startedAt' | |||
| 'items.status.startedAt', | |||
| 'items.spec' | |||
There was a problem hiding this comment.
this is a lot of data - can we avoid it?
There was a problem hiding this comment.
I'll check if it can be further specified. I believe I added that for the Util functions for checking to see if an action can be performed on a wf. I'll also see if that can be determined without using spec at all.
There was a problem hiding this comment.
Only needed item.spec.suspended for the check. Just fixed
|
|
||
| export interface WorkflowAction { | ||
| title: string; | ||
| action: () => Promise<any>; |
There was a problem hiding this comment.
What does this Promise return? Does it have to be any?
There was a problem hiding this comment.
I made this any because of the WorkflowDeleteResponse type used here in workflows-service.ts:
public delete(name: string, namespace: string): Promise<WorkflowDeleteResponse> {...For some reason, the delete function is the only one with its own Response type. I'll look into making a generic WorkflowActionResponse type to get rid of that any
There was a problem hiding this comment.
I see. If we don't use the result of the promise anywhere then this should be fine
| private confirmAction(title: string): void { | ||
| if (!confirm(`Are you sure you want to ${title} this workflow?`)) { | ||
| return; | ||
| } | ||
| return; | ||
| } |
There was a problem hiding this comment.
Is this function correct? It doesn't seem the if statement has any effect here
There was a problem hiding this comment.
I think this is correct, I tried it without the if and it didn't work (the confirm dialogue didn't pop up)
There was a problem hiding this comment.
I meant more that it doesn't matter if confirm returns true or false, the result doesn't chagne
| } | ||
| ]; | ||
| const actions: any = Actions.WorkflowActions; | ||
| const items = Object.keys(actions).map(actionName => { |
There was a problem hiding this comment.
Are we sure that this returns the items in the same order every time? We don't want to have the buttons shuffle around.
There was a problem hiding this comment.
I'm fairly confident that as of ES2015 the order of object props can be guaranteed, and as of ES6 Object.keys relies on the underlying method that they use to guarantee the order. Here's the relevant portion of the spec:
https://tc39.es/ecma262/#sec-ordinaryownpropertykeys
| const items = Object.keys(actions).map(actionName => { | ||
| const action = actions[actionName]; | ||
| return { | ||
| title: action.title.charAt(0).toUpperCase() + action.title.slice(1), |
There was a problem hiding this comment.
This seems a tad dirty. Is there a reason as to why action.action is lower case? Could it be capital case instead and if it needs to be lower case then we can do action.action.lower() or whatever the TS equivalent is
There was a problem hiding this comment.
yeah I wasn't wild about that either. Capital case is a good solution and *.lower() is certainly cleaner, I'll do that.
There was a problem hiding this comment.
It doesn't seem like this was changed?
| private getHandleErrorFunction(title: string): () => void { | ||
| return () => { | ||
| this.appContext.apis.notifications.show({ | ||
| content: `Unable to ${title} workflow`, | ||
| type: NotificationType.Error | ||
| }); | ||
| }; | ||
| } |
There was a problem hiding this comment.
This can probably just be inlined
| private noneSelected(): boolean { | ||
| return Object.keys(this.props.selectedWorkflows).length < 1; | ||
| } |
There was a problem hiding this comment.
Why is this necessary? Couldn't we just do getNumberSelected() == 0?
There was a problem hiding this comment.
Yeah, good point. Changed
| } | ||
|
|
||
| private getNumberSelected(): number { | ||
| return Object.keys(this.props.selectedWorkflows).length; |
There was a problem hiding this comment.
I could be wrong, but doesn't
| return Object.keys(this.props.selectedWorkflows).length; | |
| return this.props.selectedWorkflows.length; |
work?
There was a problem hiding this comment.
I tried it, it doesn't - since selectedWorkflows is an object rather than an array it doesn't have a length property
| private confirmAction(title: string): void { | ||
| if (!confirm(`Are you sure you want to ${title} all selected workflows?`)) { | ||
| return; | ||
| } | ||
| return; | ||
| } |
There was a problem hiding this comment.
Same here regarding the if statement
| private getHandleErrorFunction(title: string): () => void { | ||
| return () => { | ||
| this.props.loadWorkflows(); | ||
| this.appContext.apis.notifications.show({ | ||
| content: `Unable to ${title} workflow`, | ||
| type: NotificationType.Error | ||
| }); | ||
| }; | ||
| } |
|
|
||
| private renderActions(ctx: any): JSX.Element[] { | ||
| const actionButtons = []; | ||
| for (const action of this.getActions(ctx)) { |
There was a problem hiding this comment.
I feel like we don't need getActions and can do the whole thing in one loop
There was a problem hiding this comment.
If I click the checkbox on a Workflow, it treats it as a link: https://cl.ly/13ecf2ed9f9c I'm on Firefox if that matters
On another note, can we hide the toolbar unless there is one or more Workflows selected?
Also, I don't think we need the "Selected" in $ACTION Selected on the buttons. If we only show the toolbar when there is >= 1 Workflows selected, then it should be clear what we mean
|
Ah interesting, will test on Firefox and fix. Might be a z-index issue. I was thinking along the same lines but was undecided, that makes the decision easy... will change both |
…on button labels more succinct
…d slight reformatting of wf drawer.
|
Kudos, SonarCloud Quality Gate passed!
|
| [action in WorkflowActionName]: boolean; | ||
| }; | ||
|
|
||
| export type WorkflowActionName = 'RETRY' | 'RESUBMIT' | 'SUSPEND' | 'RESUME' | 'STOP' | 'TERMINATE' | 'DELETE'; |
There was a problem hiding this comment.
the actions map to API which use lowercase names?
There was a problem hiding this comment.
also - I think you should use the term operation rather than action
There was a problem hiding this comment.
I had them as lowercase but changed to upper for labeling purposes. Simon pointed out that the way I was initially converting lowercase to a label (string.charAt(0).toUpperCase() + string.slice(1);) was clunky and just having them as uppercase would be easier. Thinking about it now though, it wouldn't be clunky to have them lowercase and do string.toUpperCase() to convert. The union type is never used to actually query the API here, so would the change to lowercase be for consistency?
Operation sounds good, I'll change that.
| disabled: (wf: Workflow) => boolean; | ||
| } | ||
|
|
||
| export const WorkflowActions = { |
| disabled: (wf: Workflow) => !Utils.isWorkflowSuspended(wf), | ||
| action: services.workflows.terminate | ||
| }, | ||
| DELETE: { |
There was a problem hiding this comment.
can we make sure that we get an "are you sure" confirmation for delete?
There was a problem hiding this comment.
In the workflows toolbar and the workflows details, there's a confirmation before deleting - in fact for the workflows toolbar, there's a confirmation before performing any batch action. Would it be better to have the confirmation happen in workflow-actions.ts?
| @@ -69,7 +70,8 @@ export class WorkflowsService { | |||
| 'result.object.status.phase', | |||
| 'result.object.status.startedAt', | |||
| 'result.type', | |||
| 'result.object.metadata.labels' | |||
| 'result.object.metadata.labels', | |||
| 'result.object.spec' | |||
There was a problem hiding this comment.
do you want the whole spec here? could be a lot of data
|
|
||
| export type WorkflowOperationName = 'RETRY' | 'RESUBMIT' | 'SUSPEND' | 'RESUME' | 'STOP' | 'TERMINATE' | 'DELETE'; | ||
|
|
||
| export interface WorkflowAction { |
| @@ -14,7 +14,7 @@ import {CostOptimisationNudge} from '../../../shared/components/cost-optimisatio | |||
| import {Loading} from '../../../shared/components/loading'; | |||
| import {hasWarningConditionBadge} from '../../../shared/conditions-panel'; | |||
| import {Consumer, ContextApis} from '../../../shared/context'; | |||
| import {Utils} from '../../../shared/utils'; | |||
| import * as Actions from '../../../shared/workflow-operations'; | |||
|
of course, here are a couple: |
|
I can't see this feature in 2.9.1. Is that planned for 2.10 or do we need to do something to enable it? |
Checklist:
"fix(controller): Updates such and such. Fixes #1234".