Skip to content

Commit 8c764ed

Browse files
deadlycoconutsdeadlycoconuts
authored
Fix ensembler deletion UI bugs (#352)
* Add modalClosed to onConfirm function * Reset delete confirmation in delete job modal * Add additional condition to ensure active router versions are displayed before current ones * Make modalClosed unset all ensembler status * Make modal display tables for both active and current router versions * Refactor messages to make them reflect how router versions are displayed * Refactor deletion messages for ensembling jobs to reflect singularity/plurality * Add additional conditional handling to display ensembler deletion messages * Fix inconsistent pop up message * Reinstate terminate/delete button but disable it when ensembling job is terminating
1 parent 3347f34 commit 8c764ed

File tree

5 files changed

+64
-49
lines changed

5 files changed

+64
-49
lines changed

‎ui/src/ensembler/components/modal/DeleteEnsemblerModal.js

+14-11
Original file line numberDiff line numberDiff line change
@@ -54,40 +54,43 @@ export const DeleteEnsemblerModal = ({
5454
const modalClosed = () => {
5555
setCanDeleteEnsembler(true)
5656
setEnsemblerUsedByCurrentRouterVersion(false)
57+
setEnsemblerUsedByActiveRouterVersion(false)
58+
setEnsemblerUsedByActiveEnsemblingJob(false)
5759
setDeleteConfirmation("")
5860
}
5961

6062
return (
6163
<ConfirmationModal
6264
title="Delete Ensembler"
6365
onCancel={() => modalClosed()}
64-
onConfirm={submitForm}
66+
onConfirm={(arg) => {submitForm(arg); modalClosed()}}
6567
isLoading={isLoading}
6668
content={
6769
<div>
6870
{canDeleteEnsembler ? (
6971
<div>
7072
<p>
71-
You are about to delete Ensembler <b>{ensembler.name}</b>. This action cannot be undone.
73+
You are about to delete the Ensembler <b>{ensembler.name}</b>. This action <b>cannot</b> be undone.
7274
</p>
73-
To confirm, please type "<b>{ensembler.name}</b>" in the box below
75+
To confirm, please type "<b>{ensembler.name}</b>" in the box below:
7476
<EuiFieldText
7577
fullWidth
7678
placeholder={ensembler.name}
7779
value={deleteConfirmation}
7880
onChange={(e) => setDeleteConfirmation(e.target.value)}
7981
isInvalid={deleteConfirmation !== ensembler.name} />
8082
</div>
81-
) : ensemblerUsedByCurrentRouterVersion ? (
82-
<div>
83-
You cannot delete this ensembler because it is associated with a router version that is currently being used by a router
84-
<br/> <br/> If you still wish to delete this ensembler, please <b>Deploy</b> another version on this router.
85-
</div>
8683
) : (
8784
<div>
88-
You cannot delete this ensembler because there are <b>Active Router Versions</b> or <b>Ensembling Jobs</b> that use this ensembler.
89-
<br/> <br/> If you still wish to delete this ensembler, please <b>Undeploy</b> router versions and <b>Terminate</b> ensembling jobs that use this ensembler.
90-
</div>
85+
You cannot delete this ensembler because it is:
86+
<ul>
87+
{ensemblerUsedByCurrentRouterVersion &&
88+
<li>associated with one or more router versions <b>currently</b> used by one or more routers
89+
{ensemblerUsedByActiveRouterVersion && ", and"}</li>}
90+
{ensemblerUsedByActiveRouterVersion &&
91+
<li>used by one or more <b>Active Router Versions</b> or <b>Ensembling Jobs</b></li>}
92+
</ul>
93+
</div>
9194
)}
9295
{/* Only show The Ensembling Table if ensembler is not used by current router version */}
9396
{!ensemblerUsedByCurrentRouterVersion && (

‎ui/src/ensembler/components/table/ListEnsemblingJobsForEnsemblerTable.js

+5-3
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,8 @@ export const ListEnsemblingJobsForEnsemblerTable = ({
9393
{canDeleteEnsembler ? ( results.totalInactiveCount > 0 && (
9494
<div>
9595
<br/>
96-
<p>Deleting this Ensembler will also delete {results.totalInactiveCount} <b>Failed</b> or <b>Completed</b> Ensembling Jobs that use this Ensembler </p>
96+
Deleting this Ensembler will also delete {results.totalInactiveCount} <b>Failed</b> or <b>Completed</b>
97+
&nbsp;Ensembling Job{results.totalInactiveCount > 1 && "s"} that use{results.totalInactiveCount === 1 && "s"} this Ensembler:
9798
<EuiBasicTable
9899
items={results.inactiveItems}
99100
loading={!isLoaded}
@@ -105,15 +106,16 @@ export const ListEnsemblingJobsForEnsemblerTable = ({
105106
)) : ( results.totalActiveCount > 0 && (
106107
<div>
107108
<br/>
108-
<p>This Ensembler is being used by {results.totalActiveCount} <b>Active Ensembling Jobs</b>.
109-
If the ensembling jobs are in terminating state, please wait until the process complete to delete this ensembler</p>
109+
This Ensembler is used by {results.totalActiveCount} <b>Active Ensembling Job{results.totalActiveCount > 1 && "s"}</b>.
110+
If any ensembling jobs are in the terminating state, please wait until they are terminated to delete this ensembler.
110111
<EuiBasicTable
111112
items={results.activeItems}
112113
loading={!isLoaded}
113114
columns={columns}
114115
responsive={true}
115116
tableLayout="auto"
116117
/>
118+
If you wish to delete this ensembler, please <b>terminate</b> {results.totalActiveCount > 1 ? "these ensembling jobs" : "this ensembling job"}.
117119
</div>
118120
))}
119121
</Fragment>

‎ui/src/ensembler/components/table/ListRouterVersionsForEnsemblerTable.js

+32-22
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ export const ListRouterVersionsForEnsemblerTable = ({
1818
ensemblerUsedByCurrentRouterVersion
1919
}) => {
2020
const [results, setResults] = useState({ inactiveItems: [], activeItems:[], totalInactiveCount: 0, totalActiveCount:0 });
21+
const [filteredActiveRouterVersions, setFilteredActiveRouterVersions] = useState([])
2122

2223
const {
2324
appConfig: {
@@ -50,16 +51,15 @@ export const ListRouterVersionsForEnsemblerTable = ({
5051

5152
useEffect(() => {
5253
if (results.activeItems.length > 0){
53-
setEnsemblerUsedByActiveRouterVersion(true)
54-
} else {
55-
if (currentRouterVersion.isLoaded && !currentRouterVersion.error) {
56-
if (currentRouterVersion.data.length > 0){
57-
setEnsemblerUsedByCurrentRouterVersion(true)
58-
setEnsemblerUsedByActiveRouterVersion(false)
59-
} else {
60-
setEnsemblerUsedByCurrentRouterVersion(false)
61-
setEnsemblerUsedByActiveRouterVersion(false)
62-
}
54+
setEnsemblerUsedByActiveRouterVersion(true);
55+
}
56+
57+
if (currentRouterVersion.isLoaded && !currentRouterVersion.error) {
58+
if (currentRouterVersion.data.length > 0){
59+
setEnsemblerUsedByCurrentRouterVersion(true);
60+
setFilteredActiveRouterVersions(results.activeItems.filter(item => currentRouterVersion.data.every((e => e.id !== item.id))));
61+
} else {
62+
setEnsemblerUsedByCurrentRouterVersion(false)
6363
}
6464
}
6565
}, [results, currentRouterVersion, setEnsemblerUsedByActiveRouterVersion, setEnsemblerUsedByCurrentRouterVersion])
@@ -115,44 +115,54 @@ export const ListRouterVersionsForEnsemblerTable = ({
115115
</EuiCallOut>
116116
) : (
117117
<Fragment>
118-
{ensemblerUsedByCurrentRouterVersion ? (
119-
currentRouterVersion.data.length > 0 && (
118+
{canDeleteEnsembler && results.totalInactiveCount > 0 && (
120119
<div>
121120
<br/>
122-
<p>The router version with the related ensembler is being used by {currentRouterVersion.data.length} <b>Routers</b></p>
121+
Deleting this Ensembler will also delete {results.totalInactiveCount} <b>Failed</b> or <b>Undeployed</b>
122+
&nbsp;Router Version{results.totalInactiveCount > 1 && "s"} that
123+
use{results.totalInactiveCount === 1 && "s"} this Ensembler:
123124
<EuiBasicTable
124-
items={currentRouterVersion.data}
125+
items={results.inactiveItems}
125126
loading={!allRouterVersion.isLoaded && !currentRouterVersion.isLoaded}
126127
columns={columns}
127128
responsive={true}
128129
tableLayout="auto"
129130
/>
130131
</div>
131-
)) : canDeleteEnsembler ? ( results.totalInactiveCount > 0 && (
132+
)}
133+
{ensemblerUsedByCurrentRouterVersion && currentRouterVersion.data.length > 0 && (
132134
<div>
133-
<br/>
134-
<p>Deleting this Ensembler will also delete {results.totalInactiveCount} <b>Failed</b> or <b>Undeployed</b> Router Versions that use this Ensembler </p>
135+
This Ensembler is used by {currentRouterVersion.data.length}
136+
&nbsp;<b>Router{currentRouterVersion.data.length > 1 && "s"}</b>
137+
&nbsp;in {currentRouterVersion.data.length > 1 ? "their" : "its"} <b>current</b> configuration:
135138
<EuiBasicTable
136-
items={results.inactiveItems}
139+
items={currentRouterVersion.data}
137140
loading={!allRouterVersion.isLoaded && !currentRouterVersion.isLoaded}
138141
columns={columns}
139142
responsive={true}
140143
tableLayout="auto"
141144
/>
145+
If you wish to delete this ensembler, please <b>deploy</b> another router version
146+
for {currentRouterVersion.data.length > 1 ? "these routers" : "this router"}.
142147
</div>
143-
)) : ( results.totalActiveCount > 0 && (
148+
)}
149+
{filteredActiveRouterVersions.length > 0 && (
144150
<div>
145151
<br/>
146-
<p>This Ensembler is being used by {results.totalActiveCount} <b>Active Router Versions</b></p>
152+
This Ensembler is {ensemblerUsedByCurrentRouterVersion && currentRouterVersion.data.length > 0 && "also"}
153+
&nbsp;used by {filteredActiveRouterVersions.length}
154+
&nbsp;<b>Active Router Version{filteredActiveRouterVersions.length > 1 && "s"}</b>:
147155
<EuiBasicTable
148-
items={results.activeItems}
156+
items={filteredActiveRouterVersions}
149157
loading={!allRouterVersion.isLoaded && !currentRouterVersion.isLoaded}
150158
columns={columns}
151159
responsive={true}
152160
tableLayout="auto"
153161
/>
162+
If you wish to delete this ensembler, please <b>undeploy</b>
163+
&nbsp;{filteredActiveRouterVersions.length > 1 ? "these router versions" : "this router version"}.
154164
</div>
155-
))}
165+
)}
156166
</Fragment>
157167
);
158168
};

‎ui/src/jobs/components/modal/DeleteJobModal.js

+2-1
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,8 @@ export const DeleteJobModal = ({
4545
return (
4646
<ConfirmationModal
4747
title={isActiveJobStatus(job.status) ? 'Delete Ensembling Jobs' : 'Terminate Ensembling Jobs'}
48-
onConfirm={submitForm}
48+
onCancel={() => setDeleteConfirmation("")}
49+
onConfirm={(arg) => {submitForm(arg); setDeleteConfirmation("")}}
4950
isLoading={isLoading}
5051
disabled={deleteConfirmation !== job.name}
5152
content={

‎ui/src/jobs/list/ListEnsemblingJobsTable.js

+11-12
Original file line numberDiff line numberDiff line change
@@ -158,18 +158,17 @@ export const ListEnsemblingJobsTable = ({
158158
<EuiText size="xs">Monitoring</EuiText>
159159
</EuiButtonEmpty>
160160
</EuiFlexItem>
161-
{item.status !== "terminating" && (
162-
<EuiFlexItem grow={false} >
163-
<EuiButtonEmpty
164-
onClick={() => onDeleteJob(item)}
165-
color={"danger"}
166-
iconType={isActiveJobStatus(item.status) ? "trash" : "minusInCircle" }
167-
iconSide="left"
168-
size="xs">
169-
<EuiText size="xs"> {isActiveJobStatus(item.status) ? "Delete" : "Terminate" } </EuiText>
170-
</EuiButtonEmpty>
171-
</EuiFlexItem>
172-
)}
161+
<EuiFlexItem grow={false} >
162+
<EuiButtonEmpty
163+
onClick={() => onDeleteJob(item)}
164+
color={"danger"}
165+
iconType={isActiveJobStatus(item.status) ? "trash" : "minusInCircle" }
166+
iconSide="left"
167+
size="xs"
168+
isDisabled={item.status === "terminating"}>
169+
<EuiText size="xs"> {isActiveJobStatus(item.status) ? "Delete" : "Terminate" } </EuiText>
170+
</EuiButtonEmpty>
171+
</EuiFlexItem>
173172
</EuiFlexItem>
174173
</EuiFlexGroup>
175174
),

0 commit comments

Comments
 (0)