diff --git a/frontend/src/TestUtils.tsx b/frontend/src/TestUtils.tsx index 6e9a3ee756d..bb5d9942b5e 100644 --- a/frontend/src/TestUtils.tsx +++ b/frontend/src/TestUtils.tsx @@ -17,8 +17,10 @@ import * as React from 'react'; // @ts-ignore import createRouterContext from 'react-router-test-context'; +import { PageProps, Page } from './pages/Page'; import { mount, ReactWrapper } from 'enzyme'; import { object } from 'prop-types'; +import { match } from 'react-router'; export default class TestUtils { /** @@ -54,4 +56,27 @@ export default class TestUtils { }; }); } + + // tslint:disable-next-line:variable-name + public static generatePageProps(PageElement: new (x: PageProps) => Page, + location: Location, matchValue: match, + historyPushSpy: jest.SpyInstance | null, updateBannerSpy: jest.SpyInstance, + updateDialogSpy: jest.SpyInstance, updateToolbarSpy: jest.SpyInstance, + updateSnackbarSpy: jest.SpyInstance): PageProps { + const pageProps = { + history: { push: historyPushSpy } as any, + location: location as any, + match: matchValue, + toolbarProps: { actions: [], breadcrumbs: [], pageTitle: '' }, + updateBanner: updateBannerSpy as any, + updateDialog: updateDialogSpy as any, + updateSnackbar: updateSnackbarSpy as any, + updateToolbar: updateToolbarSpy as any, + } as PageProps; + pageProps.toolbarProps = new PageElement(pageProps).getInitialToolbarState(); + // The toolbar spy gets called in the getInitialToolbarState method, reset it + // in order to simplify tests + updateToolbarSpy.mockReset(); + return pageProps; + } } diff --git a/frontend/src/lib/Buttons.ts b/frontend/src/lib/Buttons.ts index 90e2cd498ee..b136309037e 100644 --- a/frontend/src/lib/Buttons.ts +++ b/frontend/src/lib/Buttons.ts @@ -18,112 +18,306 @@ import AddIcon from '@material-ui/icons/Add'; import CollapseIcon from '@material-ui/icons/UnfoldLess'; import ExpandIcon from '@material-ui/icons/UnfoldMore'; import { ToolbarActionConfig } from '../components/Toolbar'; +import { PageProps } from '../pages/Page'; +import { URLParser } from './URLParser'; +import { RoutePage, QUERY_PARAMS } from '../components/Router'; +import { Apis } from './Apis'; +import { errorToMessage, s } from './Utils'; -interface ButtonsMap { [key: string]: (action: () => void) => ToolbarActionConfig; } - -// tslint:disable-next-line:variable-name -const Buttons: ButtonsMap = { - archive: action => ({ - action, - disabled: true, - disabledTitle: 'Select at least one resource to archive', - id: 'archiveBtn', - title: 'Archive', - tooltip: 'Archive', - }), - cloneRun: action => ({ - action, - disabled: true, - disabledTitle: 'Select a run to clone', - id: 'cloneBtn', - title: 'Clone run', - tooltip: 'Create a copy from this run\s initial state', - }), - collapseSections: action => ({ - action, - icon: CollapseIcon, - id: 'collapseBtn', - title: 'Collapse all', - tooltip: 'Collapse all sections', - }), - compareRuns: action => ({ - action, - disabled: true, - disabledTitle: 'Select multiple runs to compare', - id: 'compareBtn', - title: 'Compare runs', - tooltip: 'Compare up to 10 selected runs', - }), - delete: action => ({ - action, - disabled: true, - disabledTitle: 'Select at least one resource to delete', - id: 'deleteBtn', - title: 'Delete', - tooltip: 'Delete', - }), - disableRun: action => ({ - action, - disabled: true, - disabledTitle: 'Run schedule already disabled', - id: 'disableBtn', - title: 'Disable', - tooltip: 'Disable the run\'s trigger', - }), - enableRun: action => ({ - action, - disabled: true, - disabledTitle: 'Run schedule already enabled', - id: 'enableBtn', - title: 'Enable', - tooltip: 'Enable the run\'s trigger', - }), - expandSections: action => ({ - action, - icon: ExpandIcon, - id: 'expandBtn', - title: 'Expand all', - tooltip: 'Expand all sections', - }), - newExperiment: action => ({ - action, - icon: AddIcon, - id: 'newExperimentBtn', - outlined: true, - title: 'Create an experiment', - tooltip: 'Create a new experiment', - }), - newRun: action => ({ - action, - icon: AddIcon, - id: 'createNewRunBtn', - outlined: true, - primary: true, - title: 'Create run', - tooltip: 'Create a new run within this pipeline', - }), - refresh: action => ({ - action, - id: 'refreshBtn', - title: 'Refresh', - tooltip: 'Refresh the list', - }), - restore: action => ({ - action, - disabled: true, - disabledTitle: 'Select at least one resource to restore', - id: 'archiveBtn', - title: 'Archive', - tooltip: 'Archive', - }), - upload: action => ({ - action, - icon: AddIcon, - id: 'uploadBtn', - outlined: true, - title: 'Upload pipeline', - tooltip: 'Upload pipeline', - }), -}; - -export default Buttons; +export default class Buttons { + private _props: PageProps; + private _refresh: () => void; + private _urlParser: URLParser; + + constructor(pageProps: PageProps, refresh: () => void) { + this._props = pageProps; + this._refresh = refresh; + this._urlParser = new URLParser(pageProps); + } + + public cloneRun(getSelectedIds: () => string[], useCurrentResource: boolean): ToolbarActionConfig { + return { + action: () => this._cloneRun(getSelectedIds()), + disabled: !useCurrentResource, + disabledTitle: useCurrentResource ? undefined : 'Select a run to clone', + id: 'cloneBtn', + title: 'Clone run', + tooltip: 'Create a copy from this run\s initial state', + }; + } + + public collapseSections(action: () => void): ToolbarActionConfig { + return { + action, + icon: CollapseIcon, + id: 'collapseBtn', + title: 'Collapse all', + tooltip: 'Collapse all sections', + }; + } + + public compareRuns(getSelectedIds: () => string[]): ToolbarActionConfig { + return { + action: () => this._compareRuns(getSelectedIds()), + disabled: true, + disabledTitle: 'Select multiple runs to compare', + id: 'compareBtn', + title: 'Compare runs', + tooltip: 'Compare up to 10 selected runs', + }; + } + + public delete(getSelectedIds: () => string[], resourceName: 'pipeline' | 'recurring run config', + callback: (selectedIds: string[], success: boolean) => void, useCurrentResource: boolean): ToolbarActionConfig { + return { + action: () => resourceName === 'pipeline' ? + this._deletePipeline(getSelectedIds(), callback, useCurrentResource) : + this._deleteRecurringRun(getSelectedIds()[0], useCurrentResource, callback), + disabled: !useCurrentResource, + disabledTitle: useCurrentResource ? undefined : `Select at least one ${resourceName} to delete`, + id: 'deleteBtn', + title: 'Delete', + tooltip: 'Delete', + }; + } + + public disableRecurringRun(getId: () => string): ToolbarActionConfig { + return { + action: () => this._setRecurringRunEnabledState(getId(), false), + disabled: true, + disabledTitle: 'Run schedule already disabled', + id: 'disableBtn', + title: 'Disable', + tooltip: 'Disable the run\'s trigger', + }; + } + + public enableRecurringRun(getId: () => string): ToolbarActionConfig { + return { + action: () => this._setRecurringRunEnabledState(getId(), true), + disabled: true, + disabledTitle: 'Run schedule already enabled', + id: 'enableBtn', + title: 'Enable', + tooltip: 'Enable the run\'s trigger', + }; + } + + public expandSections(action: () => void): ToolbarActionConfig { + return { + action, + icon: ExpandIcon, + id: 'expandBtn', + title: 'Expand all', + tooltip: 'Expand all sections', + }; + } + + public newExperiment(getPipelineId?: () => string): ToolbarActionConfig { + return { + action: () => this._createNewExperiment(getPipelineId ? getPipelineId() : ''), + icon: AddIcon, + id: 'newExperimentBtn', + outlined: true, + title: 'Create an experiment', + tooltip: 'Create a new experiment', + }; + } + + public newRun(getExperimentId?: () => string): ToolbarActionConfig { + return { + action: () => this._createNewRun(false, getExperimentId ? getExperimentId() : undefined), + icon: AddIcon, + id: 'createNewRunBtn', + outlined: true, + primary: true, + title: 'Create run', + tooltip: 'Create a new run', + }; + } + + public newRunFromPipeline(getPipelineId: () => string): ToolbarActionConfig { + return { + action: () => this._createNewRunFromPipeline(getPipelineId()), + icon: AddIcon, + id: 'createNewRunBtn', + outlined: true, + primary: true, + title: 'Create run', + tooltip: 'Create a new run', + }; + } + + public newRecurringRun(experimentId: string): ToolbarActionConfig { + return { + action: () => this._createNewRun(true, experimentId), + icon: AddIcon, + id: 'createNewRecurringRunBtn', + outlined: true, + title: 'Create recurring run', + tooltip: 'Create a new recurring run', + }; + } + + public refresh(action: () => void): ToolbarActionConfig { + return { + action, + id: 'refreshBtn', + title: 'Refresh', + tooltip: 'Refresh the list', + }; + } + + public upload(action: () => void): ToolbarActionConfig { + return { + action, + icon: AddIcon, + id: 'uploadBtn', + outlined: true, + title: 'Upload pipeline', + tooltip: 'Upload pipeline', + }; + } + + private _cloneRun(selectedIds: string[]): void { + if (selectedIds.length === 1) { + const runId = selectedIds[0]; + const searchString = this._urlParser.build({ [QUERY_PARAMS.cloneFromRun]: runId || '' }); + this._props.history.push(RoutePage.NEW_RUN + searchString); + } + } + + private _deletePipeline(selectedIds: string[], callback: (selectedIds: string[], success: boolean) => void, + useCurrentResource: boolean): void { + this._dialogActionHandler(selectedIds, useCurrentResource, + id => Apis.pipelineServiceApi.deletePipeline(id), callback, 'Delete', 'pipeline'); + } + + private _deleteRecurringRun(id: string, useCurrentResource: boolean, + callback: (_: string[], success: boolean) => void): void { + this._dialogActionHandler([id], useCurrentResource, Apis.jobServiceApi.deleteJob, callback, 'Delete', + 'recurring run config'); + } + + private _dialogActionHandler(selectedIds: string[], useCurrentResource: boolean, + api: (id: string) => Promise, callback: (selectedIds: string[], success: boolean) => void, + actionName: string, resourceName: string): void { + + const dialogClosedHandler = (confirmed: boolean) => + this._dialogClosed(confirmed, selectedIds, actionName, resourceName, useCurrentResource, api, callback); + + this._props.updateDialog({ + buttons: [{ + onClick: async () => await dialogClosedHandler(true), + text: actionName, + }, { + onClick: async () => await dialogClosedHandler(false), + text: 'Cancel', + }], + onClose: async () => await dialogClosedHandler(false), + title: `${actionName} ${useCurrentResource ? 'this' : selectedIds.length} ${resourceName}${useCurrentResource ? '' : s(selectedIds.length)}?`, + }); + } + + private async _dialogClosed(confirmed: boolean, selectedIds: string[], actionName: string, + resourceName: string, useCurrentResource: boolean, api: (id: string) => Promise, + callback: (selectedIds: string[], success: boolean) => void): Promise { + if (confirmed) { + const unsuccessfulIds: string[] = []; + const errorMessages: string[] = []; + await Promise.all(selectedIds.map(async (id) => { + try { + await api(id); + } catch (err) { + unsuccessfulIds.push(id); + const errorMessage = await errorToMessage(err); + errorMessages.push(`Failed to ${actionName.toLowerCase()} ${resourceName}: ${id} with error: "${errorMessage}"`); + } + })); + + const successfulOps = selectedIds.length - unsuccessfulIds.length; + if (useCurrentResource || successfulOps > 0) { + this._props.updateSnackbar({ + message: `${actionName} succeeded for ${useCurrentResource ? 'this' : successfulOps} ${resourceName}${useCurrentResource ? '' : s(successfulOps)}`, + open: true, + }); + this._refresh(); + } + + if (unsuccessfulIds.length > 0) { + this._props.updateDialog({ + buttons: [{ text: 'Dismiss' }], + content: errorMessages.join('\n\n'), + title: `Failed to ${actionName.toLowerCase()} ${useCurrentResource ? '' : unsuccessfulIds.length + ' '}${resourceName}${useCurrentResource ? '' : s(unsuccessfulIds)}`, + }); + } + + callback(unsuccessfulIds, !unsuccessfulIds.length); + } + } + private _compareRuns(selectedIds: string[]): void { + const indices = selectedIds; + if (indices.length > 1 && indices.length <= 10) { + const runIds = selectedIds.join(','); + const searchString = this._urlParser.build({ [QUERY_PARAMS.runlist]: runIds }); + this._props.history.push(RoutePage.COMPARE + searchString); + } + } + + private _createNewExperiment(pipelineId: string): void { + const searchString = pipelineId ? this._urlParser.build({ + [QUERY_PARAMS.pipelineId]: pipelineId + }) : ''; + this._props.history.push(RoutePage.NEW_EXPERIMENT + searchString); + } + + private _createNewRun(isRecurring: boolean, experimentId?: string): void { + const searchString = this._urlParser.build(Object.assign( + { [QUERY_PARAMS.experimentId]: experimentId || '' }, + isRecurring ? { [QUERY_PARAMS.isRecurring]: '1' } : {})); + this._props.history.push(RoutePage.NEW_RUN + searchString); + } + + private _createNewRunFromPipeline(pipelineId?: string): void { + let searchString = ''; + const fromRunId = this._urlParser.get(QUERY_PARAMS.fromRunId); + + if (fromRunId) { + searchString = this._urlParser.build(Object.assign( + { [QUERY_PARAMS.fromRunId]: fromRunId } + )); + } else { + searchString = this._urlParser.build(Object.assign( + { [QUERY_PARAMS.pipelineId]: pipelineId || '' } + )); + } + + this._props.history.push(RoutePage.NEW_RUN + searchString); + } + + private async _setRecurringRunEnabledState(id: string, enabled: boolean): Promise { + if (id) { + const toolbarActions = [...this._props.toolbarProps.actions]; + + const buttonIndex = enabled ? 1 : 2; + + toolbarActions[buttonIndex].busy = true; + this._props.updateToolbar({ actions: toolbarActions }); + try { + await (enabled ? Apis.jobServiceApi.enableJob(id) : Apis.jobServiceApi.disableJob(id)); + this._refresh(); + } catch (err) { + const errorMessage = await errorToMessage(err); + this._props.updateDialog({ + buttons: [{ text: 'Dismiss' }], + content: errorMessage, + title: `Failed to ${enabled ? 'enable' : 'disable'} recurring run`, + }); + } finally { + toolbarActions[buttonIndex].busy = false; + this._props.updateToolbar({ actions: toolbarActions }); + } + } + } + +} diff --git a/frontend/src/pages/AllRunsList.tsx b/frontend/src/pages/AllRunsList.tsx index f42fa6e74e0..1db2d2074f6 100644 --- a/frontend/src/pages/AllRunsList.tsx +++ b/frontend/src/pages/AllRunsList.tsx @@ -18,9 +18,7 @@ import * as React from 'react'; import Buttons from '../lib/Buttons'; import RunList from './RunList'; import { Page } from './Page'; -import { RoutePage, QUERY_PARAMS } from '../components/Router'; import { ToolbarProps } from '../components/Toolbar'; -import { URLParser } from '../lib/URLParser'; import { classes } from 'typestyle'; import { commonCss, padding } from '../Css'; @@ -41,12 +39,13 @@ class AllRunsList extends Page<{}, AllRunsListState> { } public getInitialToolbarState(): ToolbarProps { + const buttons = new Buttons(this.props, this.refresh.bind(this)); return { actions: [ - Buttons.newExperiment(this._newExperimentClicked.bind(this)), - Buttons.compareRuns(this._compareRuns.bind(this)), - Buttons.cloneRun(this._cloneRun.bind(this)), - Buttons.refresh(this.refresh.bind(this)), + buttons.newExperiment(), + buttons.compareRuns(() => this.state.selectedIds), + buttons.cloneRun(() => this.state.selectedIds, false), + buttons.refresh(this.refresh.bind(this)), ], breadcrumbs: [], pageTitle: 'Experiments', @@ -69,21 +68,6 @@ class AllRunsList extends Page<{}, AllRunsListState> { } } - private _newExperimentClicked(): void { - this.props.history.push(RoutePage.NEW_EXPERIMENT); - } - - private _compareRuns(): void { - const indices = this.state.selectedIds; - if (indices.length > 1 && indices.length <= 10) { - const runIds = this.state.selectedIds.join(','); - const searchString = new URLParser(this.props).build({ - [QUERY_PARAMS.runlist]: runIds, - }); - this.props.history.push(RoutePage.COMPARE + searchString); - } - } - private _selectionChanged(selectedIds: string[]): void { const toolbarActions = [...this.props.toolbarProps.actions]; // Compare runs button @@ -93,16 +77,6 @@ class AllRunsList extends Page<{}, AllRunsListState> { this.props.updateToolbar({ breadcrumbs: this.props.toolbarProps.breadcrumbs, actions: toolbarActions }); this.setState({ selectedIds }); } - - private _cloneRun(): void { - if (this.state.selectedIds.length === 1) { - const runId = this.state.selectedIds[0]; - const searchString = new URLParser(this.props).build({ - [QUERY_PARAMS.cloneFromRun]: runId || '' - }); - this.props.history.push(RoutePage.NEW_RUN + searchString); - } - } } export default AllRunsList; diff --git a/frontend/src/pages/Compare.test.tsx b/frontend/src/pages/Compare.test.tsx index be9f965f28c..58058cab157 100644 --- a/frontend/src/pages/Compare.test.tsx +++ b/frontend/src/pages/Compare.test.tsx @@ -46,20 +46,14 @@ describe('Compare', () => { const getRunSpy = jest.spyOn(Apis.runServiceApi, 'getRun'); const outputArtifactLoaderSpy = jest.spyOn(OutputArtifactLoader, 'load'); + function generateProps(): PageProps { - return { - history: { push: historyPushSpy } as any, - location: { - pathname: RoutePage.COMPARE, - search: `?${QUERY_PARAMS.runlist}=${MOCK_RUN_1_ID},${MOCK_RUN_2_ID},${MOCK_RUN_3_ID}` - } as any, - match: { params: {} } as any, - toolbarProps: Compare.prototype.getInitialToolbarState(), - updateBanner: updateBannerSpy, - updateDialog: updateDialogSpy, - updateSnackbar: updateSnackbarSpy, - updateToolbar: updateToolbarSpy, - }; + const location = { + pathname: RoutePage.COMPARE, + search: `?${QUERY_PARAMS.runlist}=${MOCK_RUN_1_ID},${MOCK_RUN_2_ID},${MOCK_RUN_3_ID}` + } as any; + return TestUtils.generatePageProps(Compare, location, {} as any, historyPushSpy, + updateBannerSpy, updateDialogSpy, updateToolbarSpy, updateSnackbarSpy); } const MOCK_RUN_1_ID = 'mock-run-1-id'; diff --git a/frontend/src/pages/Compare.tsx b/frontend/src/pages/Compare.tsx index 0e0386d4310..efbc7425f30 100644 --- a/frontend/src/pages/Compare.tsx +++ b/frontend/src/pages/Compare.tsx @@ -81,10 +81,11 @@ class Compare extends Page<{}, CompareState> { } public getInitialToolbarState(): ToolbarProps { + const buttons = new Buttons(this.props, this.refresh.bind(this)); return { actions: [ - Buttons.expandSections(() => this.setState({ collapseSections: {} })), - Buttons.collapseSections(this._collapseAllSections.bind(this)), + buttons.expandSections(() => this.setState({ collapseSections: {} })), + buttons.collapseSections(this._collapseAllSections.bind(this)), ], breadcrumbs: [{ displayName: 'Experiments', href: RoutePage.EXPERIMENTS }], pageTitle: 'Compare runs', diff --git a/frontend/src/pages/ExperimentDetails.test.tsx b/frontend/src/pages/ExperimentDetails.test.tsx index 570595a85f2..23fd4a9b688 100644 --- a/frontend/src/pages/ExperimentDetails.test.tsx +++ b/frontend/src/pages/ExperimentDetails.test.tsx @@ -53,16 +53,9 @@ describe('ExperimentDetails', () => { } function generateProps(): PageProps { - return { - history: { push: historyPushSpy } as any, - location: '' as any, - match: { params: { [RouteParams.experimentId]: MOCK_EXPERIMENT.id } } as any, - toolbarProps: ExperimentDetails.prototype.getInitialToolbarState(), - updateBanner: updateBannerSpy, - updateDialog: updateDialogSpy, - updateSnackbar: updateSnackbarSpy, - updateToolbar: updateToolbarSpy, - }; + const match = { params: { [RouteParams.experimentId]: MOCK_EXPERIMENT.id } } as any; + return TestUtils.generatePageProps(ExperimentDetails, {} as any, match, historyPushSpy, + updateBannerSpy, updateDialogSpy, updateToolbarSpy, updateSnackbarSpy); } async function mockNJobs(n: number): Promise { @@ -392,7 +385,7 @@ describe('ExperimentDetails', () => { tree.find('.tableRow').simulate('click'); const cloneBtn = (tree.state('runListToolbarProps') as ToolbarProps) - .actions.find(b => b.title === 'Clone'); + .actions.find(b => b.title === 'Clone run'); await cloneBtn!.action(); expect(historyPushSpy).toHaveBeenCalledWith( @@ -427,7 +420,7 @@ describe('ExperimentDetails', () => { tree.update(); const cloneBtn = (tree.state('runListToolbarProps') as ToolbarProps) - .actions.find(b => b.title === 'Clone'); + .actions.find(b => b.title === 'Clone run'); for (let i = 0; i < 4; i++) { if (i === 1) { diff --git a/frontend/src/pages/ExperimentDetails.tsx b/frontend/src/pages/ExperimentDetails.tsx index be3e4c5894d..568a881a5d5 100644 --- a/frontend/src/pages/ExperimentDetails.tsx +++ b/frontend/src/pages/ExperimentDetails.tsx @@ -15,7 +15,6 @@ */ import * as React from 'react'; -import AddIcon from '@material-ui/icons/Add'; import Button from '@material-ui/core/Button'; import Buttons from '../lib/Buttons'; import Dialog from '@material-ui/core/Dialog'; @@ -25,14 +24,13 @@ import Paper from '@material-ui/core/Paper'; import PopOutIcon from '@material-ui/icons/Launch'; import RecurringRunsManager from './RecurringRunsManager'; import RunList from '../pages/RunList'; -import Toolbar, { ToolbarActionConfig, ToolbarProps } from '../components/Toolbar'; +import Toolbar, { ToolbarProps } from '../components/Toolbar'; import Tooltip from '@material-ui/core/Tooltip'; import { ApiExperiment } from '../apis/experiment'; import { ApiResourceType } from '../apis/job'; import { Apis } from '../lib/Apis'; import { Page } from './Page'; -import { RoutePage, RouteParams, QUERY_PARAMS } from '../components/Router'; -import { URLParser } from '../lib/URLParser'; +import { RoutePage, RouteParams } from '../components/Router'; import { classes, stylesheet } from 'typestyle'; import { color, commonCss, padding } from '../Css'; import { logger } from '../lib/Utils'; @@ -103,7 +101,7 @@ interface ExperimentDetailsState { activeRecurringRunsCount: number; experiment: ApiExperiment | null; recurringRunsManagerOpen: boolean; - selectedRunIds: string[]; + selectedIds: string[]; selectedTab: number; runListToolbarProps: ToolbarProps; } @@ -112,59 +110,35 @@ class ExperimentDetails extends Page<{}, ExperimentDetailsState> { private _runlistRef = React.createRef(); - private _runListToolbarActions: ToolbarActionConfig[] = [{ - action: () => this._createNewRun(false), - icon: AddIcon, - id: 'createNewRunBtn', - outlined: true, - primary: true, - title: 'Create run', - tooltip: 'Create a new run within this experiment', - }, { - action: () => this._createNewRun(true), - icon: AddIcon, - id: 'createNewRecurringRunBtn', - outlined: true, - title: 'Create recurring run', - tooltip: 'Create a new recurring run in this experiment', - }, { - action: this._compareRuns.bind(this), - disabled: true, - disabledTitle: 'Select multiple runs to compare', - id: 'compareBtn', - title: 'Compare runs', - tooltip: 'Compare up to 10 selected runs', - }, { - action: this._cloneRun.bind(this), - disabled: true, - disabledTitle: 'Select a run to clone', - id: 'cloneBtn', - title: 'Clone', - tooltip: 'Create a copy from this run\s initial state', - }]; - constructor(props: any) { super(props); + const buttons = new Buttons(this.props, this.refresh.bind(this)); this.state = { activeRecurringRunsCount: 0, experiment: null, recurringRunsManagerOpen: false, runListToolbarProps: { - actions: this._runListToolbarActions, + actions: [ + buttons.newRun(() => this.props.match.params[RouteParams.experimentId]), + buttons.newRecurringRun(this.props.match.params[RouteParams.experimentId]), + buttons.compareRuns(() => this.state.selectedIds), + buttons.cloneRun(() => this.state.selectedIds, false), + ], breadcrumbs: [], pageTitle: 'Runs', topLevelToolbar: false, }, // TODO: remove - selectedRunIds: [], + selectedIds: [], selectedTab: 0, }; } public getInitialToolbarState(): ToolbarProps { + const buttons = new Buttons(this.props, this.refresh.bind(this)); return { - actions: [Buttons.refresh(this.refresh.bind(this))], + actions: [buttons.refresh(this.refresh.bind(this))], breadcrumbs: [{ displayName: 'Experiments', href: RoutePage.EXPERIMENTS }], // TODO: determine what to show if no props. pageTitle: this.props ? this.props.match.params[RouteParams.experimentId] : '', @@ -218,7 +192,7 @@ class ExperimentDetails extends Page<{}, ExperimentDetailsState> { { } } - private _compareRuns(): void { - const indices = this.state.selectedRunIds; - if (indices.length > 1 && indices.length <= 10) { - const runIds = this.state.selectedRunIds.join(','); - const searchString = new URLParser(this.props).build({ - [QUERY_PARAMS.runlist]: runIds, - }); - this.props.history.push(RoutePage.COMPARE + searchString); - } - } - - private _createNewRun(isRecurring: boolean): void { - const searchString = new URLParser(this.props).build(Object.assign( - { [QUERY_PARAMS.experimentId]: this.state.experiment!.id || '', }, - isRecurring ? { [QUERY_PARAMS.isRecurring]: '1' } : {})); - this.props.history.push(RoutePage.NEW_RUN + searchString); - } - - private _cloneRun(): void { - if (this.state.selectedRunIds.length === 1) { - const runId = this.state.selectedRunIds[0]; - const searchString = new URLParser(this.props).build({ - [QUERY_PARAMS.cloneFromRun]: runId || '' - }); - this.props.history.push(RoutePage.NEW_RUN + searchString); - } - } - - private _selectionChanged(selectedRunIds: string[]): void { + private _selectionChanged(selectedIds: string[]): void { const toolbarActions = [...this.state.runListToolbarProps.actions]; // Compare runs button - toolbarActions[2].disabled = selectedRunIds.length <= 1 || selectedRunIds.length > 10; + toolbarActions[2].disabled = selectedIds.length <= 1 || selectedIds.length > 10; // Clone run button - toolbarActions[3].disabled = selectedRunIds.length !== 1; + toolbarActions[3].disabled = selectedIds.length !== 1; this.setState({ runListToolbarProps: { actions: toolbarActions, @@ -339,7 +285,7 @@ class ExperimentDetails extends Page<{}, ExperimentDetailsState> { pageTitle: this.state.runListToolbarProps.pageTitle, topLevelToolbar: this.state.runListToolbarProps.topLevelToolbar, }, - selectedRunIds + selectedIds }); } diff --git a/frontend/src/pages/ExperimentList.test.tsx b/frontend/src/pages/ExperimentList.test.tsx index 06c553f8fb9..f716a127f77 100644 --- a/frontend/src/pages/ExperimentList.test.tsx +++ b/frontend/src/pages/ExperimentList.test.tsx @@ -40,16 +40,8 @@ describe('ExperimentList', () => { const formatDateStringSpy = jest.spyOn(Utils, 'formatDateString'); function generateProps(): PageProps { - return { - history: { push: historyPushSpy } as any, - location: '' as any, - match: '' as any, - toolbarProps: ExperimentList.prototype.getInitialToolbarState(), - updateBanner: updateBannerSpy, - updateDialog: updateDialogSpy, - updateSnackbar: updateSnackbarSpy, - updateToolbar: updateToolbarSpy, - }; + return TestUtils.generatePageProps(ExperimentList, { pathname: RoutePage.EXPERIMENTS } as any, + '' as any, historyPushSpy, updateBannerSpy, updateDialogSpy, updateToolbarSpy, updateSnackbarSpy); } async function mountWithNExperiments(n: number, nRuns: number): Promise { @@ -261,7 +253,7 @@ describe('ExperimentList', () => { it('enables clone button when one run is selected', async () => { const tree = await mountWithNExperiments(1, 1); - (tree.instance() as any)._runSelectionChanged(['run1']); + (tree.instance() as any)._selectionChanged(['run1']); expect(updateToolbarSpy).toHaveBeenCalledTimes(2); expect(updateToolbarSpy.mock.calls[0][0].actions.find((b: any) => b.title === 'Clone run')) .toHaveProperty('disabled', true); @@ -272,7 +264,7 @@ describe('ExperimentList', () => { it('disables clone button when more than one run is selected', async () => { const tree = await mountWithNExperiments(1, 1); - (tree.instance() as any)._runSelectionChanged(['run1', 'run2']); + (tree.instance() as any)._selectionChanged(['run1', 'run2']); expect(updateToolbarSpy).toHaveBeenCalledTimes(2); expect(updateToolbarSpy.mock.calls[0][0].actions.find((b: any) => b.title === 'Clone run')) .toHaveProperty('disabled', true); @@ -283,9 +275,9 @@ describe('ExperimentList', () => { it('enables compare runs button only when more than one is selected', async () => { const tree = await mountWithNExperiments(1, 1); - (tree.instance() as any)._runSelectionChanged(['run1']); - (tree.instance() as any)._runSelectionChanged(['run1', 'run2']); - (tree.instance() as any)._runSelectionChanged(['run1', 'run2', 'run3']); + (tree.instance() as any)._selectionChanged(['run1']); + (tree.instance() as any)._selectionChanged(['run1', 'run2']); + (tree.instance() as any)._selectionChanged(['run1', 'run2', 'run3']); expect(updateToolbarSpy).toHaveBeenCalledTimes(4); expect(updateToolbarSpy.mock.calls[0][0].actions.find((b: any) => b.title === 'Compare runs')) .toHaveProperty('disabled', true); @@ -300,7 +292,7 @@ describe('ExperimentList', () => { it('navigates to compare page with the selected run ids', async () => { const tree = await mountWithNExperiments(1, 1); - (tree.instance() as any)._runSelectionChanged(['run1', 'run2', 'run3']); + (tree.instance() as any)._selectionChanged(['run1', 'run2', 'run3']); const compareBtn = (tree.instance() as ExperimentList) .getInitialToolbarState().actions.find(b => b.title === 'Compare runs'); await compareBtn!.action(); @@ -310,7 +302,7 @@ describe('ExperimentList', () => { it('navigates to new run page with the selected run id for cloning', async () => { const tree = await mountWithNExperiments(1, 1); - (tree.instance() as any)._runSelectionChanged(['run1']); + (tree.instance() as any)._selectionChanged(['run1']); const cloneBtn = (tree.instance() as ExperimentList) .getInitialToolbarState().actions.find(b => b.title === 'Clone run'); await cloneBtn!.action(); diff --git a/frontend/src/pages/ExperimentList.tsx b/frontend/src/pages/ExperimentList.tsx index ee7c74736c8..f5efcbc1c4e 100644 --- a/frontend/src/pages/ExperimentList.tsx +++ b/frontend/src/pages/ExperimentList.tsx @@ -24,9 +24,8 @@ import { ApiResourceType, ApiRun } from '../apis/run'; import { Apis, ExperimentSortKeys, ListRequest, RunSortKeys } from '../lib/Apis'; import { Link } from 'react-router-dom'; import { Page } from './Page'; -import { RoutePage, RouteParams, QUERY_PARAMS } from '../components/Router'; +import { RoutePage, RouteParams } from '../components/Router'; import { ToolbarProps } from '../components/Toolbar'; -import { URLParser } from '../lib/URLParser'; import { classes } from 'typestyle'; import { commonCss, padding } from '../Css'; import { logger } from '../lib/Utils'; @@ -40,7 +39,7 @@ interface DisplayExperiment extends ApiExperiment { interface ExperimentListState { displayExperiments: DisplayExperiment[]; - selectedRunIds: string[]; + selectedIds: string[]; selectedTab: number; } @@ -52,18 +51,19 @@ class ExperimentList extends Page<{}, ExperimentListState> { this.state = { displayExperiments: [], - selectedRunIds: [], + selectedIds: [], selectedTab: 0, }; } public getInitialToolbarState(): ToolbarProps { + const buttons = new Buttons(this.props, this.refresh.bind(this)); return { actions: [ - Buttons.newExperiment(this._newExperimentClicked.bind(this)), - Buttons.compareRuns(this._compareRuns.bind(this)), - Buttons.cloneRun(this._cloneRun.bind(this)), - Buttons.refresh(this.refresh.bind(this)), + buttons.newExperiment(), + buttons.compareRuns(() => this.state.selectedIds), + buttons.cloneRun(() => this.state.selectedIds, false), + buttons.refresh(this.refresh.bind(this)), ], breadcrumbs: [], pageTitle: 'Experiments', @@ -156,15 +156,6 @@ class ExperimentList extends Page<{}, ExperimentListState> { return response.next_page_token || ''; } - private _cloneRun(): void { - if (this.state.selectedRunIds.length === 1) { - const searchString = new URLParser(this.props).build({ - [QUERY_PARAMS.cloneFromRun]: this.state.selectedRunIds[0] || '' - }); - this.props.history.push(RoutePage.NEW_RUN + searchString); - } - } - private _nameCustomRenderer(value: string, id: string): JSX.Element { return e.stopPropagation()} to={RoutePage.EXPERIMENT_DETAILS.replace(':' + RouteParams.experimentId, id)}>{value}; @@ -180,30 +171,15 @@ class ExperimentList extends Page<{}, ExperimentListState> { ; } - private _runSelectionChanged(selectedRunIds: string[]): void { + private _selectionChanged(selectedIds: string[]): void { const actions = produce(this.props.toolbarProps.actions, draft => { // Enable/Disable Run compare button - draft[1].disabled = selectedRunIds.length <= 1 || selectedRunIds.length > 10; + draft[1].disabled = selectedIds.length <= 1 || selectedIds.length > 10; // Enable/Disable Clone button - draft[2].disabled = selectedRunIds.length !== 1; + draft[2].disabled = selectedIds.length !== 1; }); this.props.updateToolbar({ actions }); - this.setState({ selectedRunIds }); - } - - private _compareRuns(): void { - const indices = this.state.selectedRunIds; - if (indices.length > 1 && indices.length <= 10) { - const runIds = this.state.selectedRunIds.join(','); - const searchString = new URLParser(this.props).build({ - [QUERY_PARAMS.runlist]: runIds, - }); - this.props.history.push(RoutePage.COMPARE + searchString); - } - } - - private _newExperimentClicked(): void { - this.props.history.push(RoutePage.NEW_EXPERIMENT); + this.setState({ selectedIds }); } private _toggleRowExpand(rowIndex: number): void { @@ -221,8 +197,8 @@ class ExperimentList extends Page<{}, ExperimentListState> { const experiment = this.state.displayExperiments[experimentIndex]; const runIds = (experiment.last5Runs || []).map((r) => r.id!); return null} {...this.props} - disablePaging={true} selectedIds={this.state.selectedRunIds} noFilterBox={true} - onSelectionChange={this._runSelectionChanged.bind(this)} disableSorting={true} />; + disablePaging={true} selectedIds={this.state.selectedIds} noFilterBox={true} + onSelectionChange={this._selectionChanged.bind(this)} disableSorting={true} />; } } diff --git a/frontend/src/pages/PipelineDetails.test.tsx b/frontend/src/pages/PipelineDetails.test.tsx index 654a1e9749a..45883d16811 100644 --- a/frontend/src/pages/PipelineDetails.test.tsx +++ b/frontend/src/pages/PipelineDetails.test.tsx @@ -45,25 +45,16 @@ describe('PipelineDetails', () => { let testRun: ApiRunDetail = {}; function generateProps(fromRunSpec = false): PageProps { - // getInitialToolbarState relies on page props having been populated, so fill those first - const pageProps: PageProps = { - history: { push: historyPushSpy } as any, - location: { search: fromRunSpec ? `?${QUERY_PARAMS.fromRunId}=test-run-id` : '' } as any, - match: { - isExact: true, - params: fromRunSpec ? {} : { [RouteParams.pipelineId]: testPipeline.id }, - path: '', - url: '', - }, - toolbarProps: { actions: [], breadcrumbs: [], pageTitle: '' }, - updateBanner: updateBannerSpy, - updateDialog: updateDialogSpy, - updateSnackbar: updateSnackbarSpy, - updateToolbar: updateToolbarSpy, + const match = { + isExact: true, + params: fromRunSpec ? {} : { [RouteParams.pipelineId]: testPipeline.id }, + path: '', + url: '', }; - return Object.assign(pageProps, { - toolbarProps: new PipelineDetails(pageProps).getInitialToolbarState(), - }); + const location = { search: fromRunSpec ? `?${QUERY_PARAMS.fromRunId}=test-run-id` : '' } as any; + const pageProps = TestUtils.generatePageProps(PipelineDetails, location, match, historyPushSpy, + updateBannerSpy, updateDialogSpy, updateToolbarSpy, updateSnackbarSpy); + return pageProps; } beforeAll(() => jest.spyOn(console, 'error').mockImplementation()); @@ -454,7 +445,7 @@ describe('PipelineDetails', () => { await confirmBtn.onClick(); expect(updateDialogSpy).toHaveBeenCalledTimes(2); // Delete dialog + error dialog expect(updateDialogSpy).toHaveBeenLastCalledWith(expect.objectContaining({ - content: 'woops', + content: 'Failed to delete pipeline: test-pipeline-id with error: "woops"', title: 'Failed to delete pipeline', })); }); @@ -471,7 +462,7 @@ describe('PipelineDetails', () => { await confirmBtn.onClick(); expect(updateSnackbarSpy).toHaveBeenCalledTimes(1); expect(updateSnackbarSpy).toHaveBeenLastCalledWith(expect.objectContaining({ - message: 'Successfully deleted pipeline: ' + testPipeline.name, + message: 'Delete succeeded for this pipeline', open: true, })); }); diff --git a/frontend/src/pages/PipelineDetails.tsx b/frontend/src/pages/PipelineDetails.tsx index 9e22e0cd6ca..68a6ec1cacb 100644 --- a/frontend/src/pages/PipelineDetails.tsx +++ b/frontend/src/pages/PipelineDetails.tsx @@ -39,7 +39,7 @@ import { UnControlled as CodeMirror } from 'react-codemirror2'; import { Workflow } from '../../third_party/argo-ui/argo_template'; import { classes, stylesheet } from 'typestyle'; import { color, commonCss, padding, fontsize, fonts } from '../Css'; -import { logger, errorToMessage, formatDateString } from '../lib/Utils'; +import { logger, formatDateString } from '../lib/Utils'; interface PipelineDetailsState { graph?: dagre.graphlib.Graph; @@ -113,8 +113,11 @@ class PipelineDetails extends Page<{}, PipelineDetailsState> { } public getInitialToolbarState(): ToolbarProps { + const buttons = new Buttons(this.props, this.refresh.bind(this)); const fromRunId = new URLParser(this.props).get(QUERY_PARAMS.fromRunId); - let actions: ToolbarActionConfig[] = [Buttons.newRun(this._createNewRun.bind(this))]; + let actions: ToolbarActionConfig[] = [ + buttons.newRunFromPipeline(() => this.state.pipeline ? this.state.pipeline.id! : ''), + ]; if (fromRunId) { return { @@ -127,15 +130,9 @@ class PipelineDetails extends Page<{}, PipelineDetailsState> { } else { // Add buttons for creating experiment and deleting pipeline actions = actions.concat([ - Buttons.newExperiment(this._createNewExperiment.bind(this)), - Buttons.delete(() => this.props.updateDialog({ - buttons: [ - { onClick: () => this._deleteDialogClosed(true), text: 'Delete' }, - { onClick: () => this._deleteDialogClosed(false), text: 'Cancel' }, - ], - onClose: () => this._deleteDialogClosed(false), - title: 'Delete this pipeline?', - })) + buttons.newExperiment(() => this.state.pipeline ? this.state.pipeline.id! : ''), + buttons.delete(() => this.state.pipeline ? [this.state.pipeline.id!] : [], + 'pipeline', () => null, true), ]); return { actions, @@ -348,52 +345,6 @@ class PipelineDetails extends Page<{}, PipelineDetailsState> { templateString, }); } - - private _createNewRun(): void { - let searchString = ''; - const fromRunId = new URLParser(this.props).get(QUERY_PARAMS.fromRunId); - - if (fromRunId) { - searchString = new URLParser(this.props).build(Object.assign( - { [QUERY_PARAMS.fromRunId]: fromRunId } - )); - } else { - searchString = new URLParser(this.props).build(Object.assign( - { [QUERY_PARAMS.pipelineId]: this.state.pipeline!.id || '' } - )); - } - - this.props.history.push(RoutePage.NEW_RUN + searchString); - } - - private _createNewExperiment(): void { - const searchString = new URLParser(this.props).build({ - [QUERY_PARAMS.pipelineId]: this.state.pipeline!.id || '' - }); - this.props.history.push(RoutePage.NEW_EXPERIMENT + searchString); - } - - private async _deleteDialogClosed(deleteConfirmed: boolean): Promise { - if (deleteConfirmed) { - try { - await Apis.pipelineServiceApi.deletePipeline(this.state.pipeline!.id!); - this.props.updateSnackbar({ - autoHideDuration: 10000, - message: `Successfully deleted pipeline: ${this.state.pipeline!.name}`, - open: true, - }); - this.props.history.push(RoutePage.PIPELINES); - } catch (err) { - const errorMessage = await errorToMessage(err); - this.props.updateDialog({ - buttons: [{ text: 'Dismiss' }], - content: errorMessage, - title: 'Failed to delete pipeline', - }); - logger.error('Deleting pipeline failed with error:', err); - } - } - } } export default PipelineDetails; diff --git a/frontend/src/pages/PipelineList.test.tsx b/frontend/src/pages/PipelineList.test.tsx index 0db992402d9..6098ad65cd7 100644 --- a/frontend/src/pages/PipelineList.test.tsx +++ b/frontend/src/pages/PipelineList.test.tsx @@ -39,16 +39,8 @@ describe('PipelineList', () => { const uploadPipelineSpy = jest.spyOn(Apis, 'uploadPipeline'); function generateProps(): PageProps { - return { - history: {} as any, - location: '' as any, - match: '' as any, - toolbarProps: PipelineList.prototype.getInitialToolbarState(), - updateBanner: updateBannerSpy, - updateDialog: updateDialogSpy, - updateSnackbar: updateSnackbarSpy, - updateToolbar: updateToolbarSpy, - }; + return TestUtils.generatePageProps(PipelineList, '' as any, '' as any, null, updateBannerSpy, + updateDialogSpy, updateToolbarSpy, updateSnackbarSpy); } async function mountWithNPipelines(n: number): Promise { @@ -328,7 +320,7 @@ describe('PipelineList', () => { const confirmBtn = call.buttons.find((b: any) => b.text === 'Delete'); await confirmBtn.onClick(); expect(updateSnackbarSpy).toHaveBeenLastCalledWith({ - message: 'Successfully deleted 1 pipeline!', + message: 'Delete succeeded for 1 pipeline', open: true, }); }); @@ -345,7 +337,7 @@ describe('PipelineList', () => { await confirmBtn.onClick(); const lastCall = updateDialogSpy.mock.calls[1][0]; expect(lastCall).toMatchObject({ - content: 'Deleting pipeline: test pipeline name0 failed with error: "woops, failed"', + content: 'Failed to delete pipeline: test-pipeline-id0 with error: "woops, failed"', title: 'Failed to delete 1 pipeline', }); }); @@ -373,14 +365,14 @@ describe('PipelineList', () => { expect(updateDialogSpy).toHaveBeenCalledTimes(2); const lastCall = updateDialogSpy.mock.calls[1][0]; expect(lastCall).toMatchObject({ - content: 'Deleting pipeline: test pipeline name0 failed with error: "woops, failed!"\n\n' + - 'Deleting pipeline: test pipeline name1 failed with error: "woops, failed!"', + content: 'Failed to delete pipeline: test-pipeline-id0 with error: "woops, failed!"\n\n' + + 'Failed to delete pipeline: test-pipeline-id1 with error: "woops, failed!"', title: 'Failed to delete 2 pipelines', }); // Should show snackbar for the one successful deletion expect(updateSnackbarSpy).toHaveBeenLastCalledWith({ - message: 'Successfully deleted 2 pipelines!', + message: 'Delete succeeded for 2 pipelines', open: true, }); }); diff --git a/frontend/src/pages/PipelineList.tsx b/frontend/src/pages/PipelineList.tsx index 2f7bf17bff6..5204ae58ff8 100644 --- a/frontend/src/pages/PipelineList.tsx +++ b/frontend/src/pages/PipelineList.tsx @@ -27,7 +27,7 @@ import { RoutePage, RouteParams } from '../components/Router'; import { ToolbarProps } from '../components/Toolbar'; import { classes } from 'typestyle'; import { commonCss, padding } from '../Css'; -import { formatDateString, errorToMessage, s } from '../lib/Utils'; +import { formatDateString, errorToMessage } from '../lib/Utils'; interface PipelineListState { pipelines: ApiPipeline[]; @@ -49,18 +49,17 @@ class PipelineList extends Page<{}, PipelineListState> { } public getInitialToolbarState(): ToolbarProps { + const buttons = new Buttons(this.props, this.refresh.bind(this)); return { actions: [ - Buttons.upload(() => this.setStateSafe({ uploadDialogOpen: true })), - Buttons.refresh(this.refresh.bind(this)), - Buttons.delete(() => this.props.updateDialog({ - buttons: [ - { onClick: async () => await this._deleteDialogClosed(true), text: 'Delete' }, - { onClick: async () => await this._deleteDialogClosed(false), text: 'Cancel' }, - ], - onClose: async () => await this._deleteDialogClosed(false), - title: `Delete ${this.state.selectedIds.length} pipeline${s(this.state.selectedIds)}?`, - })), + buttons.upload(() => this.setStateSafe({ uploadDialogOpen: true })), + buttons.refresh(this.refresh.bind(this)), + buttons.delete( + () => this.state.selectedIds, + 'pipeline', + ids => this._selectionChanged(ids), + false, + ), ], breadcrumbs: [], pageTitle: 'Pipelines', @@ -138,42 +137,6 @@ class PipelineList extends Page<{}, PipelineListState> { this.setStateSafe({ selectedIds }); } - private async _deleteDialogClosed(deleteConfirmed: boolean): Promise { - if (deleteConfirmed) { - const unsuccessfulDeleteIds: string[] = []; - const errorMessages: string[] = []; - // TODO: Show spinner during wait. - await Promise.all(this.state.selectedIds.map(async (id) => { - try { - await Apis.pipelineServiceApi.deletePipeline(id); - } catch (err) { - unsuccessfulDeleteIds.push(id); - const pipeline = this.state.pipelines.find((p) => p.id === id); - const errorMessage = await errorToMessage(err); - errorMessages.push( - `Deleting pipeline${pipeline ? ': ' + pipeline.name : ''} failed with error: "${errorMessage}"`); - } - })); - - const successfulDeletes = this.state.selectedIds.length - unsuccessfulDeleteIds.length; - if (successfulDeletes > 0) { - this.props.updateSnackbar({ - message: `Successfully deleted ${successfulDeletes} pipeline${successfulDeletes === 1 ? '' : 's'}!`, - open: true, - }); - this.refresh(); - } - - if (unsuccessfulDeleteIds.length > 0) { - this.showErrorDialog( - `Failed to delete ${unsuccessfulDeleteIds.length} pipeline${unsuccessfulDeleteIds.length === 1 ? '' : 's'}`, - errorMessages.join('\n\n')); - } - - this._selectionChanged(unsuccessfulDeleteIds); - } - } - private async _uploadDialogClosed(confirmed: boolean, name: string, file: File | null, url: string, method: ImportMethod, description?: string): Promise { diff --git a/frontend/src/pages/RecurringRunDetails.test.tsx b/frontend/src/pages/RecurringRunDetails.test.tsx index e0752bcacce..2dfb3879bbf 100644 --- a/frontend/src/pages/RecurringRunDetails.test.tsx +++ b/frontend/src/pages/RecurringRunDetails.test.tsx @@ -24,12 +24,6 @@ import { RouteParams, RoutePage } from '../components/Router'; import { ToolbarActionConfig } from '../components/Toolbar'; import { shallow } from 'enzyme'; -class TestRecurringRunDetails extends RecurringRunDetails { - public async _deleteDialogClosed(confirmed: boolean): Promise { - super._deleteDialogClosed(confirmed); - } -} - describe('RecurringRunDetails', () => { const updateBannerSpy = jest.fn(); const updateDialogSpy = jest.fn(); @@ -45,16 +39,9 @@ describe('RecurringRunDetails', () => { let fullTestJob: ApiJob = {}; function generateProps(): PageProps { - return { - history: { push: historyPushSpy } as any, - location: '' as any, - match: { params: { [RouteParams.runId]: fullTestJob.id }, isExact: true, path: '', url: '' }, - toolbarProps: RecurringRunDetails.prototype.getInitialToolbarState(), - updateBanner: updateBannerSpy, - updateDialog: updateDialogSpy, - updateSnackbar: updateSnackbarSpy, - updateToolbar: updateToolbarSpy, - }; + const match = { params: { [RouteParams.runId]: fullTestJob.id }, isExact: true, path: '', url: '' }; + return TestUtils.generatePageProps(RecurringRunDetails, '' as any, match, historyPushSpy, + updateBannerSpy, updateDialogSpy, updateToolbarSpy, updateSnackbarSpy); } beforeEach(() => { @@ -321,7 +308,7 @@ describe('RecurringRunDetails', () => { const deleteBtn = instance.getInitialToolbarState().actions.find(b => b.title === 'Delete'); await deleteBtn!.action(); expect(updateDialogSpy).toHaveBeenLastCalledWith(expect.objectContaining({ - title: 'Delete this recurring run?', + title: 'Delete this recurring run config?', })); tree.unmount(); }); @@ -359,23 +346,32 @@ describe('RecurringRunDetails', () => { // or clicking outside it, it should be treated the same way as clicking Cancel. it('redirects back to parent experiment after delete', async () => { - const tree = shallow(); + const tree = shallow(); await TestUtils.flushPromises(); - const instance = tree.instance() as TestRecurringRunDetails; - await instance._deleteDialogClosed(true); + const deleteBtn = (tree.instance() as RecurringRunDetails) + .getInitialToolbarState().actions.find(b => b.title === 'Delete'); + await deleteBtn!.action(); + const call = updateDialogSpy.mock.calls[0][0]; + const confirmBtn = call.buttons.find((b: any) => b.text === 'Delete'); + await confirmBtn.onClick(); + expect(deleteJobSpy).toHaveBeenLastCalledWith('test-job-id'); expect(historyPushSpy).toHaveBeenCalledTimes(1); expect(historyPushSpy).toHaveBeenLastCalledWith(RoutePage.EXPERIMENTS); tree.unmount(); }); it('shows snackbar after successful deletion', async () => { - const tree = shallow(); + const tree = shallow(); await TestUtils.flushPromises(); - const instance = tree.instance() as TestRecurringRunDetails; - await instance._deleteDialogClosed(true); + const deleteBtn = (tree.instance() as RecurringRunDetails) + .getInitialToolbarState().actions.find(b => b.title === 'Delete'); + await deleteBtn!.action(); + const call = updateDialogSpy.mock.calls[0][0]; + const confirmBtn = call.buttons.find((b: any) => b.text === 'Delete'); + await confirmBtn.onClick(); expect(updateSnackbarSpy).toHaveBeenCalledTimes(1); expect(updateSnackbarSpy).toHaveBeenLastCalledWith({ - message: 'Successfully deleted recurring run: ' + fullTestJob.name, + message: 'Delete succeeded for this recurring run config', open: true, }); tree.unmount(); @@ -383,15 +379,19 @@ describe('RecurringRunDetails', () => { it('shows error dialog after failing deletion', async () => { TestUtils.makeErrorResponseOnce(deleteJobSpy, 'could not delete'); - const tree = shallow(); + const tree = shallow(); await TestUtils.flushPromises(); - const instance = tree.instance() as TestRecurringRunDetails; - await instance._deleteDialogClosed(true); + const deleteBtn = (tree.instance() as RecurringRunDetails) + .getInitialToolbarState().actions.find(b => b.title === 'Delete'); + await deleteBtn!.action(); + const call = updateDialogSpy.mock.calls[0][0]; + const confirmBtn = call.buttons.find((b: any) => b.text === 'Delete'); + await confirmBtn.onClick(); await TestUtils.flushPromises(); - expect(updateDialogSpy).toHaveBeenCalledTimes(1); + expect(updateDialogSpy).toHaveBeenCalledTimes(2); expect(updateDialogSpy).toHaveBeenLastCalledWith(expect.objectContaining({ - content: 'could not delete', - title: 'Failed to delete recurring run', + content: 'Failed to delete recurring run config: test-job-id with error: "could not delete"', + title: 'Failed to delete recurring run config', })); // Should not reroute expect(historyPushSpy).not.toHaveBeenCalled(); diff --git a/frontend/src/pages/RecurringRunDetails.tsx b/frontend/src/pages/RecurringRunDetails.tsx index bc2f50d68c4..b3de4c58cce 100644 --- a/frontend/src/pages/RecurringRunDetails.tsx +++ b/frontend/src/pages/RecurringRunDetails.tsx @@ -23,7 +23,7 @@ import { ApiJob } from '../apis/job'; import { Apis } from '../lib/Apis'; import { Page } from './Page'; import { RoutePage, RouteParams } from '../components/Router'; -import { ToolbarActionConfig, Breadcrumb, ToolbarProps } from '../components/Toolbar'; +import { Breadcrumb, ToolbarProps } from '../components/Toolbar'; import { classes } from 'typestyle'; import { commonCss, padding } from '../Css'; import { formatDateString, enabledDisplayString, errorToMessage } from '../lib/Utils'; @@ -44,19 +44,18 @@ class RecurringRunDetails extends Page<{}, RecurringRunConfigState> { } public getInitialToolbarState(): ToolbarProps { + const buttons = new Buttons(this.props, this.refresh.bind(this)); return { actions: [ - Buttons.refresh(this.refresh.bind(this)), - Buttons.enableRun(() => this._setEnabledState(true)), - Buttons.disableRun(() => this._setEnabledState(false)), - Buttons.delete(() => this.props.updateDialog({ - buttons: [ - { onClick: () => this._deleteDialogClosed(true), text: 'Delete' }, - { onClick: () => this._deleteDialogClosed(false), text: 'Cancel' }, - ], - onClose: () => this._deleteDialogClosed(false), - title: 'Delete this recurring run?', - })), + buttons.refresh(this.refresh.bind(this)), + buttons.enableRecurringRun(() => this.state.run ? this.state.run.id! : ''), + buttons.disableRecurringRun(() => this.state.run ? this.state.run.id! : ''), + buttons.delete( + () => this.state.run ? [this.state.run!.id!] : [], + 'recurring run config', + this._deleteCallback.bind(this), + true, + ), ], breadcrumbs: [], pageTitle: '', @@ -179,50 +178,12 @@ class RecurringRunDetails extends Page<{}, RecurringRunConfigState> { this.setState({ run }); } - protected async _setEnabledState(enabled: boolean): Promise { - if (this.state.run) { - const toolbarActions = [...this.props.toolbarProps.actions]; - - const buttonIndex = enabled ? 1 : 2; - const id = this.state.run.id!; - - toolbarActions[buttonIndex].busy = true; - this._updateToolbar(toolbarActions); - try { - await (enabled ? Apis.jobServiceApi.enableJob(id) : Apis.jobServiceApi.disableJob(id)); - this.refresh(); - } catch (err) { - const errorMessage = await errorToMessage(err); - this.showErrorDialog( - `Failed to ${enabled ? 'enable' : 'disable'} recurring run`, errorMessage); - } finally { - toolbarActions[buttonIndex].busy = false; - this._updateToolbar(toolbarActions); - } - } - } - - protected _updateToolbar(actions: ToolbarActionConfig[]): void { - this.props.updateToolbar({ actions }); - } - - protected async _deleteDialogClosed(deleteConfirmed: boolean): Promise { - if (deleteConfirmed) { - // TODO: Show spinner during wait. - try { - await Apis.jobServiceApi.deleteJob(this.state.run!.id!); - const breadcrumbs = this.props.toolbarProps.breadcrumbs; - const previousPage = breadcrumbs.length ? - breadcrumbs[breadcrumbs.length - 1].href : RoutePage.EXPERIMENTS; - this.props.history.push(previousPage); - this.props.updateSnackbar({ - message: `Successfully deleted recurring run: ${this.state.run!.name}`, - open: true, - }); - } catch (err) { - const errorMessage = await errorToMessage(err); - this.showErrorDialog('Failed to delete recurring run', errorMessage); - } + private _deleteCallback(_: string[], success: boolean): void { + if (success) { + const breadcrumbs = this.props.toolbarProps.breadcrumbs; + const previousPage = breadcrumbs.length ? + breadcrumbs[breadcrumbs.length - 1].href : RoutePage.EXPERIMENTS; + this.props.history.push(previousPage); } } } diff --git a/frontend/src/pages/RunDetails.tsx b/frontend/src/pages/RunDetails.tsx index 3efdde0476d..19e5ec2e501 100644 --- a/frontend/src/pages/RunDetails.tsx +++ b/frontend/src/pages/RunDetails.tsx @@ -35,9 +35,8 @@ import { Apis } from '../lib/Apis'; import { NodePhase, statusToIcon, hasFinished } from './Status'; import { OutputArtifactLoader } from '../lib/OutputArtifactLoader'; import { Page } from './Page'; -import { RoutePage, RouteParams, QUERY_PARAMS } from '../components/Router'; +import { RoutePage, RouteParams } from '../components/Router'; import { ToolbarProps } from '../components/Toolbar'; -import { URLParser } from '../lib/URLParser'; import { ViewerConfig } from '../components/viewers/Viewer'; import { Workflow } from '../../third_party/argo-ui/argo_template'; import { classes, stylesheet } from 'typestyle'; @@ -132,10 +131,11 @@ class RunDetails extends Page { } public getInitialToolbarState(): ToolbarProps { + const buttons = new Buttons(this.props, this.refresh.bind(this)); return { actions: [ - Buttons.cloneRun(this._cloneRun.bind(this)), - Buttons.refresh(this.refresh.bind(this)), + buttons.cloneRun(() => this.state.runMetadata ? [this.state.runMetadata!.id!] : [], true), + buttons.refresh(this.refresh.bind(this)), ], breadcrumbs: [{ displayName: 'Experiments', href: RoutePage.EXPERIMENTS }], pageTitle: this.props.runId!, @@ -225,13 +225,13 @@ class RunDetails extends Page {
-
- - - Runtime execution graph. Only steps that are currently running or have already completed are shown. +
+ + + Runtime execution graph. Only steps that are currently running or have already completed are shown. +
-
} {!graph && (
@@ -515,15 +515,6 @@ class RunDetails extends Page { this.setStateSafe({ sidepanelBusy: false }); } } - - private _cloneRun(): void { - if (this.state.runMetadata) { - const searchString = new URLParser(this.props).build({ - [QUERY_PARAMS.cloneFromRun]: this.state.runMetadata.id || '' - }); - this.props.history.push(RoutePage.NEW_RUN + searchString); - } - } } export default RunDetails; diff --git a/frontend/src/pages/__snapshots__/Compare.test.tsx.snap b/frontend/src/pages/__snapshots__/Compare.test.tsx.snap index 0a0556ff288..f907fbf36ba 100644 --- a/frontend/src/pages/__snapshots__/Compare.test.tsx.snap +++ b/frontend/src/pages/__snapshots__/Compare.test.tsx.snap @@ -100,11 +100,7 @@ exports[`Compare creates a map of viewers 1`] = ` "search": "?runlist=run-with-workflow", } } - match={ - Object { - "params": Object {}, - } - } + match={Object {}} onError={[Function]} onSelectionChange={[Function]} runIdListMask={ @@ -315,11 +311,7 @@ exports[`Compare creates an extra aggregation plot for compatible viewers 1`] = "search": "?runlist=run1-id,run2-id", } } - match={ - Object { - "params": Object {}, - } - } + match={Object {}} onError={[Function]} onSelectionChange={[Function]} runIdListMask={ @@ -584,11 +576,7 @@ exports[`Compare displays parameters from multiple runs 1`] = ` "search": "?runlist=run1,run2", } } - match={ - Object { - "params": Object {}, - } - } + match={Object {}} onError={[Function]} onSelectionChange={[Function]} runIdListMask={ @@ -754,11 +742,7 @@ exports[`Compare displays run's parameters if the run has any 1`] = ` "search": "?runlist=run-with-parameters", } } - match={ - Object { - "params": Object {}, - } - } + match={Object {}} onError={[Function]} onSelectionChange={[Function]} runIdListMask={ @@ -914,11 +898,7 @@ exports[`Compare does not show viewers for deselected runs 1`] = ` "search": "?runlist=run-with-workflow-1,run-with-workflow-2", } } - match={ - Object { - "params": Object {}, - } - } + match={Object {}} onError={[Function]} onSelectionChange={[Function]} runIdListMask={ @@ -1053,11 +1033,7 @@ exports[`Compare expands all sections if they were collapsed 1`] = ` "search": "?runlist=run-with-workflow-1,run-with-workflow-2", } } - match={ - Object { - "params": Object {}, - } - } + match={Object {}} onError={[Function]} onSelectionChange={[Function]} runIdListMask={ @@ -1316,11 +1292,7 @@ exports[`Compare renders a page with multiple runs 1`] = ` "search": "?runlist=mock-run-1-id,mock-run-2-id,mock-run-3-id", } } - match={ - Object { - "params": Object {}, - } - } + match={Object {}} onError={[Function]} onSelectionChange={[Function]} runIdListMask={ @@ -1468,11 +1440,7 @@ exports[`Compare renders a page with no runs 1`] = ` "search": "", } } - match={ - Object { - "params": Object {}, - } - } + match={Object {}} onError={[Function]} onSelectionChange={[Function]} runIdListMask={Array []} diff --git a/frontend/src/pages/__snapshots__/ExperimentDetails.test.tsx.snap b/frontend/src/pages/__snapshots__/ExperimentDetails.test.tsx.snap index 54e48c81993..67c50f37959 100644 --- a/frontend/src/pages/__snapshots__/ExperimentDetails.test.tsx.snap +++ b/frontend/src/pages/__snapshots__/ExperimentDetails.test.tsx.snap @@ -89,7 +89,7 @@ exports[`ExperimentDetails fetches this experiment's recurring runs 1`] = ` "outlined": true, "primary": true, "title": "Create run", - "tooltip": "Create a new run within this experiment", + "tooltip": "Create a new run", }, Object { "action": [Function], @@ -97,7 +97,7 @@ exports[`ExperimentDetails fetches this experiment's recurring runs 1`] = ` "id": "createNewRecurringRunBtn", "outlined": true, "title": "Create recurring run", - "tooltip": "Create a new recurring run in this experiment", + "tooltip": "Create a new recurring run", }, Object { "action": [Function], @@ -112,7 +112,7 @@ exports[`ExperimentDetails fetches this experiment's recurring runs 1`] = ` "disabled": true, "disabledTitle": "Select a run to clone", "id": "cloneBtn", - "title": "Clone", + "title": "Clone run", "tooltip": "Create a copy from this runs initial state", }, ] @@ -128,7 +128,7 @@ exports[`ExperimentDetails fetches this experiment's recurring runs 1`] = ` "push": [MockFunction], } } - location="" + location={Object {}} match={ Object { "params": Object { @@ -155,7 +155,7 @@ exports[`ExperimentDetails fetches this experiment's recurring runs 1`] = ` "href": "/experiments", }, ], - "pageTitle": "", + "pageTitle": "some-mock-experiment-id", } } updateBanner={ @@ -232,7 +232,7 @@ exports[`ExperimentDetails fetches this experiment's recurring runs 1`] = ` "push": [MockFunction], } } - location="" + location={Object {}} match={ Object { "params": Object { @@ -256,7 +256,7 @@ exports[`ExperimentDetails fetches this experiment's recurring runs 1`] = ` "href": "/experiments", }, ], - "pageTitle": "", + "pageTitle": "some-mock-experiment-id", } } updateBanner={ @@ -433,7 +433,7 @@ exports[`ExperimentDetails removes all description text after second newline and "outlined": true, "primary": true, "title": "Create run", - "tooltip": "Create a new run within this experiment", + "tooltip": "Create a new run", }, Object { "action": [Function], @@ -441,7 +441,7 @@ exports[`ExperimentDetails removes all description text after second newline and "id": "createNewRecurringRunBtn", "outlined": true, "title": "Create recurring run", - "tooltip": "Create a new recurring run in this experiment", + "tooltip": "Create a new recurring run", }, Object { "action": [Function], @@ -456,7 +456,7 @@ exports[`ExperimentDetails removes all description text after second newline and "disabled": true, "disabledTitle": "Select a run to clone", "id": "cloneBtn", - "title": "Clone", + "title": "Clone run", "tooltip": "Create a copy from this runs initial state", }, ] @@ -472,7 +472,7 @@ exports[`ExperimentDetails removes all description text after second newline and "push": [MockFunction], } } - location="" + location={Object {}} match={ Object { "params": Object { @@ -499,7 +499,7 @@ exports[`ExperimentDetails removes all description text after second newline and "href": "/experiments", }, ], - "pageTitle": "", + "pageTitle": "some-mock-experiment-id", } } updateBanner={ @@ -576,7 +576,7 @@ exports[`ExperimentDetails removes all description text after second newline and "push": [MockFunction], } } - location="" + location={Object {}} match={ Object { "params": Object { @@ -600,7 +600,7 @@ exports[`ExperimentDetails removes all description text after second newline and "href": "/experiments", }, ], - "pageTitle": "", + "pageTitle": "some-mock-experiment-id", } } updateBanner={ @@ -764,7 +764,7 @@ exports[`ExperimentDetails renders a page with no runs or recurring runs 1`] = ` "outlined": true, "primary": true, "title": "Create run", - "tooltip": "Create a new run within this experiment", + "tooltip": "Create a new run", }, Object { "action": [Function], @@ -772,7 +772,7 @@ exports[`ExperimentDetails renders a page with no runs or recurring runs 1`] = ` "id": "createNewRecurringRunBtn", "outlined": true, "title": "Create recurring run", - "tooltip": "Create a new recurring run in this experiment", + "tooltip": "Create a new recurring run", }, Object { "action": [Function], @@ -787,7 +787,7 @@ exports[`ExperimentDetails renders a page with no runs or recurring runs 1`] = ` "disabled": true, "disabledTitle": "Select a run to clone", "id": "cloneBtn", - "title": "Clone", + "title": "Clone run", "tooltip": "Create a copy from this runs initial state", }, ] @@ -803,7 +803,7 @@ exports[`ExperimentDetails renders a page with no runs or recurring runs 1`] = ` "push": [MockFunction], } } - location="" + location={Object {}} match={ Object { "params": Object { @@ -830,7 +830,7 @@ exports[`ExperimentDetails renders a page with no runs or recurring runs 1`] = ` "href": "/experiments", }, ], - "pageTitle": "", + "pageTitle": "some-mock-experiment-id", } } updateBanner={ @@ -907,7 +907,7 @@ exports[`ExperimentDetails renders a page with no runs or recurring runs 1`] = ` "push": [MockFunction], } } - location="" + location={Object {}} match={ Object { "params": Object { @@ -931,7 +931,7 @@ exports[`ExperimentDetails renders a page with no runs or recurring runs 1`] = ` "href": "/experiments", }, ], - "pageTitle": "", + "pageTitle": "some-mock-experiment-id", } } updateBanner={ @@ -1093,7 +1093,7 @@ exports[`ExperimentDetails uses an empty string if the experiment has no descrip "outlined": true, "primary": true, "title": "Create run", - "tooltip": "Create a new run within this experiment", + "tooltip": "Create a new run", }, Object { "action": [Function], @@ -1101,7 +1101,7 @@ exports[`ExperimentDetails uses an empty string if the experiment has no descrip "id": "createNewRecurringRunBtn", "outlined": true, "title": "Create recurring run", - "tooltip": "Create a new recurring run in this experiment", + "tooltip": "Create a new recurring run", }, Object { "action": [Function], @@ -1116,7 +1116,7 @@ exports[`ExperimentDetails uses an empty string if the experiment has no descrip "disabled": true, "disabledTitle": "Select a run to clone", "id": "cloneBtn", - "title": "Clone", + "title": "Clone run", "tooltip": "Create a copy from this runs initial state", }, ] @@ -1132,7 +1132,7 @@ exports[`ExperimentDetails uses an empty string if the experiment has no descrip "push": [MockFunction], } } - location="" + location={Object {}} match={ Object { "params": Object { @@ -1159,7 +1159,7 @@ exports[`ExperimentDetails uses an empty string if the experiment has no descrip "href": "/experiments", }, ], - "pageTitle": "", + "pageTitle": "some-mock-experiment-id", } } updateBanner={ @@ -1236,7 +1236,7 @@ exports[`ExperimentDetails uses an empty string if the experiment has no descrip "push": [MockFunction], } } - location="" + location={Object {}} match={ Object { "params": Object { @@ -1260,7 +1260,7 @@ exports[`ExperimentDetails uses an empty string if the experiment has no descrip "href": "/experiments", }, ], - "pageTitle": "", + "pageTitle": "some-mock-experiment-id", } } updateBanner={