From 58bb9afd1b59fe0a8dd32ba46aee65857bfbbe96 Mon Sep 17 00:00:00 2001 From: Riley Bauer <34456002+rileyjbauer@users.noreply.github.com> Date: Tue, 18 Dec 2018 14:14:13 -0800 Subject: [PATCH] Allows uploading a pipeline via a URL (#554) Still needs verification on real cluster --- frontend/mock-backend/mock-api-middleware.ts | 29 +- .../components/UploadPipelineDialog.test.tsx | 116 +++- .../src/components/UploadPipelineDialog.tsx | 123 ++-- .../UploadPipelineDialog.test.tsx.snap | 606 ++++++++++++------ frontend/src/pages/PipelineList.test.tsx | 184 +++--- frontend/src/pages/PipelineList.tsx | 36 +- 6 files changed, 735 insertions(+), 359 deletions(-) diff --git a/frontend/mock-backend/mock-api-middleware.ts b/frontend/mock-backend/mock-api-middleware.ts index 5fad10f1e4e..ef58c0eb2b6 100644 --- a/frontend/mock-backend/mock-api-middleware.ts +++ b/frontend/mock-backend/mock-api-middleware.ts @@ -25,6 +25,7 @@ import { ApiListJobsResponse, ApiJob } from '../src/apis/job'; import { ApiRun, ApiListRunsResponse, ApiResourceType } from '../src/apis/run'; import { ApiListExperimentsResponse, ApiExperiment } from '../src/apis/experiment'; import RunUtils from '../src/lib/RunUtils'; +import { Response } from 'express-serve-static-core'; const rocMetadataJsonPath = './eval-output/metadata.json'; const rocMetadataJsonPath2 = './eval-output/metadata2.json'; @@ -497,20 +498,16 @@ export default (app: express.Application) => { res.send(JSON.stringify({ template: fs.readFileSync(filePath, 'utf-8') })); }); - app.options(v1beta1Prefix + '/pipelines/upload', (req, res) => { - res.send(); - }); - app.post(v1beta1Prefix + '/pipelines/upload', (req, res) => { + function mockCreatePipeline(res: Response, name: string, body?: any): void { res.header('Content-Type', 'application/json'); // Don't allow uploading multiple pipelines with the same name - const pipelineName = decodeURIComponent(req.query.name); - if (fixedData.pipelines.find((p) => p.name === pipelineName)) { + if (fixedData.pipelines.find((p) => p.name === name)) { res.status(502).send( - `A Pipeline named: "${pipelineName}" already exists. Please choose a different name.`); + `A Pipeline named: "${name}" already exists. Please choose a different name.`); } else { - const pipeline = req.body; + const pipeline = body || {}; pipeline.id = 'new-pipeline-' + (fixedData.pipelines.length + 1); - pipeline.name = pipelineName; + pipeline.name = name; pipeline.created_at = new Date(); pipeline.description = 'TODO: the mock middleware does not actually use the uploaded pipeline'; @@ -530,6 +527,20 @@ export default (app: express.Application) => { res.send(fixedData.pipelines[fixedData.pipelines.length - 1]); }, 1000); } + } + + app.options(v1beta1Prefix + '/pipelines', (req, res) => { + res.send(); + }); + app.post(v1beta1Prefix + '/pipelines', (req, res) => { + mockCreatePipeline(res, req.body.name); + }); + + app.options(v1beta1Prefix + '/pipelines/upload', (req, res) => { + res.send(); + }); + app.post(v1beta1Prefix + '/pipelines/upload', (req, res) => { + mockCreatePipeline(res, decodeURIComponent(req.query.name), req.body); }); app.get('/artifacts/get', (req, res) => { diff --git a/frontend/src/components/UploadPipelineDialog.test.tsx b/frontend/src/components/UploadPipelineDialog.test.tsx index c828e173d98..d87a143dace 100644 --- a/frontend/src/components/UploadPipelineDialog.test.tsx +++ b/frontend/src/components/UploadPipelineDialog.test.tsx @@ -15,69 +15,153 @@ */ import * as React from 'react'; -import { shallow } from 'enzyme'; -import UploadPipelineDialog from './UploadPipelineDialog'; +import { shallow, ReactWrapper, ShallowWrapper } from 'enzyme'; +import UploadPipelineDialog, { ImportMethod } from './UploadPipelineDialog'; +import TestUtils from '../TestUtils'; describe('UploadPipelineDialog', () => { + let tree: ReactWrapper | ShallowWrapper; + + afterEach(() => { + tree.unmount(); + }); + it('renders closed', () => { - const tree = shallow(); + tree = shallow(); expect(tree).toMatchSnapshot(); }); it('renders open', () => { - const tree = shallow(); + tree = shallow(); expect(tree).toMatchSnapshot(); }); it('renders an active dropzone', () => { - const tree = shallow(); + tree = shallow(); tree.setState({ dropzoneActive: true }); expect(tree).toMatchSnapshot(); }); it('renders with a selected file to upload', () => { - const tree = shallow(); + tree = shallow(); tree.setState({ fileToUpload: true }); expect(tree).toMatchSnapshot(); }); + it('renders alternate UI for uploading via URL', () => { + tree = shallow(); + tree.setState({ importMethod: ImportMethod.URL }); + expect(tree).toMatchSnapshot(); + }); + it('calls close callback with null and empty string when canceled', () => { const spy = jest.fn(); - const tree = shallow(); + tree = shallow(); tree.find('#cancelUploadBtn').simulate('click'); - expect(spy).toHaveBeenCalledWith('', null, ''); + expect(spy).toHaveBeenCalledWith(false, '', null, '', ImportMethod.LOCAL, ''); }); it('calls close callback with null and empty string when dialog is closed', () => { const spy = jest.fn(); - const tree = shallow(); + tree = shallow(); tree.find('WithStyles(Dialog)').simulate('close'); - expect(spy).toHaveBeenCalledWith('', null, ''); + expect(spy).toHaveBeenCalledWith(false, '', null, '', ImportMethod.LOCAL, ''); }); - it('calls close callback with file name, file object, and descriptio when confirmed', () => { + it('calls close callback with file name, file object, and description when confirmed', () => { const spy = jest.fn(); - const tree = shallow(); + tree = shallow(); (tree.instance() as any)._dropzoneRef = { current: { open: () => null } }; (tree.instance() as UploadPipelineDialog).handleChange('uploadPipelineName')({ target: { value: 'test name' } }); tree.find('#confirmUploadBtn').simulate('click'); - expect(spy).toHaveBeenLastCalledWith('test name', null, ''); + expect(spy).toHaveBeenLastCalledWith(true, 'test name', null, '', ImportMethod.LOCAL, ''); + }); + + it('sets the import method based on which radio button is toggled', () => { + tree = shallow(); + // Import method is LOCAL by default + expect(tree.state('importMethod')).toBe(ImportMethod.LOCAL); + + // Click 'Import by URL' + tree.find('#uploadFromUrlBtn').simulate('change'); + expect(tree.state('importMethod')).toBe(ImportMethod.URL); + + // Click back to default, 'Upload a file' + tree.find('#uploadLocalFileBtn').simulate('change'); + expect(tree.state('importMethod')).toBe(ImportMethod.LOCAL); + }); + + it('resets all state if the dialog is closed and the callback returns true', async () => { + const spy = jest.fn(() => true); + + tree = shallow(); + tree.setState({ + busy: true, + dropzoneActive: true, + file: {}, + fileName: 'test file name', + fileUrl: 'https://some.url.com', + importMethod: ImportMethod.URL, + uploadPipelineDescription: 'test description', + uploadPipelineName: 'test pipeline name', + }); + + tree.find('#confirmUploadBtn').simulate('click'); + await TestUtils.flushPromises(); + + expect(tree.state('busy')).toBe(false); + expect(tree.state('dropzoneActive')).toBe(false); + expect(tree.state('file')).toBeNull(); + expect(tree.state('fileName')).toBe(''); + expect(tree.state('fileUrl')).toBe(''); + expect(tree.state('importMethod')).toBe(ImportMethod.LOCAL); + expect(tree.state('uploadPipelineDescription')).toBe(''); + expect(tree.state('uploadPipelineName')).toBe(''); + }); + + it('does not reset the state if the dialog is closed and the callback returns false', async () => { + const spy = jest.fn(() => false); + + tree = shallow(); + tree.setState({ + busy: true, + dropzoneActive: true, + file: {}, + fileName: 'test file name', + fileUrl: 'https://some.url.com', + importMethod: ImportMethod.URL, + uploadPipelineDescription: 'test description', + uploadPipelineName: 'test pipeline name', + }); + + tree.find('#confirmUploadBtn').simulate('click'); + await TestUtils.flushPromises(); + + expect(tree.state('dropzoneActive')).toBe(true); + expect(tree.state('file')).toEqual({}); + expect(tree.state('fileName')).toBe('test file name'); + expect(tree.state('fileUrl')).toBe('https://some.url.com'); + expect(tree.state('importMethod')).toBe(ImportMethod.URL); + expect(tree.state('uploadPipelineDescription')).toBe('test description'); + expect(tree.state('uploadPipelineName')).toBe('test pipeline name'); + // 'busy' is set to false regardless upon the callback returning + expect(tree.state('busy')).toBe(false); }); it('sets an active dropzone on drag', () => { - const tree = shallow(); + tree = shallow(); tree.find('#dropZone').simulate('dragEnter'); expect(tree.state()).toHaveProperty('dropzoneActive', true); }); it('sets an inactive dropzone on drag leave', () => { - const tree = shallow(); + tree = shallow(); tree.find('#dropZone').simulate('dragLeave'); expect(tree.state()).toHaveProperty('dropzoneActive', false); }); it('sets a file object on drop', () => { - const tree = shallow(); + tree = shallow(); const file = { name: 'test upload file' }; tree.find('#dropZone').simulate('drop', [file]); expect(tree.state()).toHaveProperty('dropzoneActive', false); diff --git a/frontend/src/components/UploadPipelineDialog.tsx b/frontend/src/components/UploadPipelineDialog.tsx index 79625cd2fd1..da04ace5551 100644 --- a/frontend/src/components/UploadPipelineDialog.tsx +++ b/frontend/src/components/UploadPipelineDialog.tsx @@ -21,11 +21,13 @@ import Dialog from '@material-ui/core/Dialog'; import DialogActions from '@material-ui/core/DialogActions'; import DialogTitle from '@material-ui/core/DialogTitle'; import Dropzone from 'react-dropzone'; +import FormControlLabel from '@material-ui/core/FormControlLabel'; import Input from '../atoms/Input'; -import { stylesheet } from 'typestyle'; -import { padding } from '../Css'; import InputAdornment from '@material-ui/core/InputAdornment'; +import Radio from '@material-ui/core/Radio'; import { TextFieldProps } from '@material-ui/core/TextField'; +import { padding, commonCss } from '../Css'; +import { stylesheet, classes } from 'typestyle'; const css = stylesheet({ dropOverlay: { @@ -33,7 +35,6 @@ const css = stylesheet({ border: '2px dashed #aaa', bottom: 0, left: 0, - margin: 3, padding: '2.5em 0', position: 'absolute', right: 0, @@ -42,13 +43,18 @@ const css = stylesheet({ zIndex: 1, }, root: { - minWidth: 500, + width: 500, }, }); +export enum ImportMethod { + LOCAL = 'local', + URL = 'url', +} + interface UploadPipelineDialogProps { open: boolean; - onClose: (name: string, file: File | null, description?: string) => Promise; + onClose: (confirmed: boolean, name: string, file: File | null, url: string, method: ImportMethod, description?: string) => Promise; } interface UploadPipelineDialogState { @@ -56,6 +62,8 @@ interface UploadPipelineDialogState { dropzoneActive: boolean; file: File | null; fileName: string; + fileUrl: string; + importMethod: ImportMethod; uploadPipelineDescription: string; uploadPipelineName: string; } @@ -71,54 +79,87 @@ class UploadPipelineDialog extends React.Component this._uploadDialogClosed(false)} open={this.props.open} classes={{ paper: css.root }}> Upload and name your pipeline - - {dropzoneActive &&
Drop files..
} - -
- Choose a pipeline package file from your computer, and give the pipeline a unique name. -
- You can also drag and drop the file here. +
+
+ } onChange={() => this.setState({ importMethod: ImportMethod.LOCAL })} /> + } onChange={() => this.setState({ importMethod: ImportMethod.URL })} />
- - - - ), - readOnly: true, - }} /> + + {importMethod === ImportMethod.LOCAL && ( + + + + {dropzoneActive && ( +
Drop files..
+ )} + +
+ Choose a pipeline package file from your computer, and give the pipeline a unique name. +
+ You can also drag and drop the file here. +
+ + + + ), + readOnly: true, + }} /> +
+
+ )} + + {importMethod === ImportMethod.URL && ( + +
+ URL must be publicly accessible. +
+ +
+ )} - {/* */} - +
+ + {/* */} this._uploadDialogClosed.bind(this)(true)} - title='Upload' busy={busy} disabled={!file || !uploadPipelineName} /> + title='Upload' busy={busy} disabled={ + !uploadPipelineName || (importMethod === ImportMethod.LOCAL ? !file : !fileUrl)} /> @@ -151,22 +192,28 @@ class UploadPipelineDialog extends React.Component { const success = await this.props.onClose( - this.state.uploadPipelineName, file, this.state.uploadPipelineDescription); + confirmed, + this.state.uploadPipelineName, + this.state.file, + this.state.fileUrl, + this.state.importMethod, + this.state.uploadPipelineDescription, + ); if (success) { this.setState({ busy: false, dropzoneActive: false, file: null, + fileName: '', + fileUrl: '', + importMethod: ImportMethod.LOCAL, uploadPipelineDescription: '', uploadPipelineName: '', }); } else { - this.setState({ - busy: false, - }); + this.setState({ busy: false }); } }); } diff --git a/frontend/src/components/__snapshots__/UploadPipelineDialog.test.tsx.snap b/frontend/src/components/__snapshots__/UploadPipelineDialog.test.tsx.snap index c8d2353b2c7..4578bfc8903 100644 --- a/frontend/src/components/__snapshots__/UploadPipelineDialog.test.tsx.snap +++ b/frontend/src/components/__snapshots__/UploadPipelineDialog.test.tsx.snap @@ -1,6 +1,6 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP -exports[`UploadPipelineDialog renders an active dropzone 1`] = ` +exports[`UploadPipelineDialog renders alternate UI for uploading via URL 1`] = ` Upload and name your pipeline -
- Drop files.. + + } + id="uploadLocalFileBtn" + label="Upload a file" + onChange={[Function]} + /> + + } + id="uploadFromUrlBtn" + label="Import by URL" + onChange={[Function]} + />
- Choose a pipeline package file from your computer, and give the pipeline a unique name. -
- You can also drag and drop the file here. + URL must be publicly accessible.
- - Choose file - - , - "readOnly": true, - } - } - label="File" + label="URL" onChange={[Function]} required={true} value="" @@ -85,7 +61,7 @@ exports[`UploadPipelineDialog renders an active dropzone 1`] = ` required={true} value="" /> -
+
`; -exports[`UploadPipelineDialog renders closed 1`] = ` +exports[`UploadPipelineDialog renders an active dropzone 1`] = ` Upload and name your pipeline -
- Choose a pipeline package file from your computer, and give the pipeline a unique name. -
- You can also drag and drop the file here. + + } + id="uploadLocalFileBtn" + label="Upload a file" + onChange={[Function]} + /> + + } + id="uploadFromUrlBtn" + label="Import by URL" + onChange={[Function]} + />
- - - Choose file - - , - "readOnly": true, + "tabIndex": -1, } } - label="File" + maxSize={Infinity} + minSize={0} + multiple={true} + onDragEnter={[Function]} + onDragLeave={[Function]} + onDrop={[Function]} + preventDropOnDocument={true} + style={ + Object { + "position": "relative", + } + } + > +
+ Drop files.. +
+
+ Choose a pipeline package file from your computer, and give the pipeline a unique name. +
+ You can also drag and drop the file here. +
+ + + Choose file + + , + "readOnly": true, + } + } + label="File" + onChange={[Function]} + required={true} + value="" + /> +
+ + + + + + Cancel + + +
+`; + +exports[`UploadPipelineDialog renders closed 1`] = ` + + + Upload and name your pipeline + +
+
+ + } + id="uploadLocalFileBtn" + label="Upload a file" + onChange={[Function]} + /> + + } + id="uploadFromUrlBtn" + label="Import by URL" + onChange={[Function]} + /> +
+ +
+ Choose a pipeline package file from your computer, and give the pipeline a unique name. +
+ You can also drag and drop the file here. +
+ + + Choose file + + , + "readOnly": true, + } + } + label="File" + onChange={[Function]} + required={true} + value="" + /> +
- +
Upload and name your pipeline -
- Choose a pipeline package file from your computer, and give the pipeline a unique name. -
- You can also drag and drop the file here. + + } + id="uploadLocalFileBtn" + label="Upload a file" + onChange={[Function]} + /> + + } + id="uploadFromUrlBtn" + label="Import by URL" + onChange={[Function]} + />
- - - Choose file - - , - "readOnly": true, + "tabIndex": -1, } } - label="File" - onChange={[Function]} - required={true} - value="" - /> + maxSize={Infinity} + minSize={0} + multiple={true} + onDragEnter={[Function]} + onDragLeave={[Function]} + onDrop={[Function]} + preventDropOnDocument={true} + style={ + Object { + "position": "relative", + } + } + > +
+ Choose a pipeline package file from your computer, and give the pipeline a unique name. +
+ You can also drag and drop the file here. +
+ + + Choose file + + , + "readOnly": true, + } + } + label="File" + onChange={[Function]} + required={true} + value="" + /> +
- + Upload and name your pipeline -
- Choose a pipeline package file from your computer, and give the pipeline a unique name. -
- You can also drag and drop the file here. + + } + id="uploadLocalFileBtn" + label="Upload a file" + onChange={[Function]} + /> + + } + id="uploadFromUrlBtn" + label="Import by URL" + onChange={[Function]} + />
- - - Choose file - - , - "readOnly": true, + "tabIndex": -1, } } - label="File" - onChange={[Function]} - required={true} - value="" - /> + maxSize={Infinity} + minSize={0} + multiple={true} + onDragEnter={[Function]} + onDragLeave={[Function]} + onDrop={[Function]} + preventDropOnDocument={true} + style={ + Object { + "position": "relative", + } + } + > +
+ Choose a pipeline package file from your computer, and give the pipeline a unique name. +
+ You can also drag and drop the file here. +
+ + + Choose file + + , + "readOnly": true, + } + } + label="File" + onChange={[Function]} + required={true} + value="" + /> +
- + { + + let tree: ReactWrapper | ShallowWrapper; + const updateBannerSpy = jest.fn(); const updateDialogSpy = jest.fn(); const updateSnackbarSpy = jest.fn(); const updateToolbarSpy = jest.fn(); const listPipelinesSpy = jest.spyOn(Apis.pipelineServiceApi, 'listPipelines'); + const createPipelineSpy = jest.spyOn(Apis.pipelineServiceApi, 'createPipeline'); const deletePipelineSpy = jest.spyOn(Apis.pipelineServiceApi, 'deletePipeline'); const uploadPipelineSpy = jest.spyOn(Apis, 'uploadPipeline'); @@ -50,7 +55,7 @@ describe('PipelineList', () => { listPipelinesSpy.mockImplementationOnce(() => ({ pipelines: range(n).map(i => ({ id: 'test-pipeline-id' + i, name: 'test pipeline name' + i })), })); - const tree = TestUtils.mountWithRouter(); + tree = TestUtils.mountWithRouter(); await listPipelinesSpy; await TestUtils.flushPromises(); tree.update(); // Make sure the tree is updated before returning it @@ -58,24 +63,21 @@ describe('PipelineList', () => { } beforeEach(() => { - // Reset mocks - updateBannerSpy.mockReset(); - updateDialogSpy.mockReset(); - updateSnackbarSpy.mockReset(); - updateToolbarSpy.mockReset(); - listPipelinesSpy.mockReset(); - deletePipelineSpy.mockReset(); - uploadPipelineSpy.mockReset(); + jest.clearAllMocks(); + }); + + afterEach(() => { + jest.resetAllMocks(); + tree.unmount(); }); it('renders an empty list with empty state message', () => { - const tree = shallow(); + tree = shallow(); expect(tree).toMatchSnapshot(); - tree.unmount(); }); it('renders a list of one pipeline', async () => { - const tree = shallow(); + tree = shallow(); tree.setState({ pipelines: [{ created_at: new Date(2018, 8, 22, 11, 5, 48), @@ -86,11 +88,10 @@ describe('PipelineList', () => { }); await listPipelinesSpy; expect(tree).toMatchSnapshot(); - tree.unmount(); }); it('renders a list of one pipeline with no description or created date', async () => { - const tree = shallow(); + tree = shallow(); tree.setState({ pipelines: [{ name: 'pipeline1', @@ -99,11 +100,10 @@ describe('PipelineList', () => { }); await listPipelinesSpy; expect(tree).toMatchSnapshot(); - tree.unmount(); }); it('renders a list of one pipeline with error', async () => { - const tree = shallow(); + tree = shallow(); tree.setState({ pipelines: [{ created_at: new Date(2018, 8, 22, 11, 5, 48), @@ -115,20 +115,18 @@ describe('PipelineList', () => { }); await listPipelinesSpy; expect(tree).toMatchSnapshot(); - tree.unmount(); }); it('calls Apis to list pipelines, sorted by creation time in descending order', async () => { listPipelinesSpy.mockImplementationOnce(() => ({ pipelines: [{ name: 'pipeline1' }] })); - const tree = TestUtils.mountWithRouter(); + tree = TestUtils.mountWithRouter(); await listPipelinesSpy; expect(listPipelinesSpy).toHaveBeenLastCalledWith('', 10, 'created_at desc'); expect(tree.state()).toHaveProperty('pipelines', [{ name: 'pipeline1' }]); - tree.unmount(); }); it('has a Refresh button, clicking it refreshes the pipeline list', async () => { - const tree = await mountWithNPipelines(1); + tree = await mountWithNPipelines(1); const instance = tree.instance() as PipelineList; expect(listPipelinesSpy.mock.calls.length).toBe(1); const refreshBtn = instance.getInitialToolbarState().actions.find(b => b.title === 'Refresh'); @@ -137,12 +135,11 @@ describe('PipelineList', () => { expect(listPipelinesSpy.mock.calls.length).toBe(2); expect(listPipelinesSpy).toHaveBeenLastCalledWith('', 10, 'created_at desc'); expect(updateBannerSpy).toHaveBeenLastCalledWith({}); - tree.unmount(); }); it('shows error banner when listing pipelines fails', async () => { TestUtils.makeErrorResponseOnce(listPipelinesSpy, 'bad stuff happened'); - const tree = TestUtils.mountWithRouter(); + tree = TestUtils.mountWithRouter(); await listPipelinesSpy; await TestUtils.flushPromises(); expect(updateBannerSpy).toHaveBeenLastCalledWith(expect.objectContaining({ @@ -150,11 +147,10 @@ describe('PipelineList', () => { message: 'Error: failed to retrieve list of pipelines. Click Details for more information.', mode: 'error', })); - tree.unmount(); }); it('shows error banner when listing pipelines fails after refresh', async () => { - const tree = TestUtils.mountWithRouter(); + tree = TestUtils.mountWithRouter(); const instance = tree.instance() as PipelineList; const refreshBtn = instance.getInitialToolbarState().actions.find(b => b.title === 'Refresh'); expect(refreshBtn).toBeDefined(); @@ -167,12 +163,11 @@ describe('PipelineList', () => { message: 'Error: failed to retrieve list of pipelines. Click Details for more information.', mode: 'error', })); - tree.unmount(); }); it('hides error banner when listing pipelines fails then succeeds', async () => { TestUtils.makeErrorResponseOnce(listPipelinesSpy, 'bad stuff happened'); - const tree = TestUtils.mountWithRouter(); + tree = TestUtils.mountWithRouter(); const instance = tree.instance() as PipelineList; await listPipelinesSpy; await TestUtils.flushPromises(); @@ -188,68 +183,61 @@ describe('PipelineList', () => { await refreshBtn!.action(); expect(listPipelinesSpy.mock.calls.length).toBe(2); expect(updateBannerSpy).toHaveBeenLastCalledWith({}); - tree.unmount(); }); it('renders pipeline names as links to their details pages', async () => { - const tree = await mountWithNPipelines(1); + tree = await mountWithNPipelines(1); const link = tree.find('a[children="test pipeline name0"]'); expect(link).toHaveLength(1); expect(link.prop('href')).toBe(RoutePage.PIPELINE_DETAILS.replace( ':' + RouteParams.pipelineId + '?', 'test-pipeline-id0' )); - tree.unmount(); }); it('always has upload pipeline button enabled', async () => { - const tree = await mountWithNPipelines(1); + tree = await mountWithNPipelines(1); const calls = updateToolbarSpy.mock.calls[0]; expect(calls[0].actions.find((b: any) => b.title === 'Upload pipeline')).not.toHaveProperty('disabled'); - tree.unmount(); }); it('enables delete button when one pipeline is selected', async () => { - const tree = await mountWithNPipelines(1); + tree = await mountWithNPipelines(1); tree.find('.tableRow').simulate('click'); expect(updateToolbarSpy.mock.calls).toHaveLength(2); // Initial call, then selection update const calls = updateToolbarSpy.mock.calls[1]; expect(calls[0].actions.find((b: any) => b.title === 'Delete')).toHaveProperty('disabled', false); - tree.unmount(); }); it('enables delete button when two pipelines are selected', async () => { - const tree = await mountWithNPipelines(2); + tree = await mountWithNPipelines(2); tree.find('.tableRow').at(0).simulate('click'); tree.find('.tableRow').at(1).simulate('click'); expect(updateToolbarSpy.mock.calls).toHaveLength(3); // Initial call, then selection updates const calls = updateToolbarSpy.mock.calls[2]; expect(calls[0].actions.find((b: any) => b.title === 'Delete')).toHaveProperty('disabled', false); - tree.unmount(); }); it('re-disables delete button pipelines are unselected', async () => { - const tree = await mountWithNPipelines(1); + tree = await mountWithNPipelines(1); tree.find('.tableRow').at(0).simulate('click'); tree.find('.tableRow').at(0).simulate('click'); expect(updateToolbarSpy.mock.calls).toHaveLength(3); // Initial call, then selection updates const calls = updateToolbarSpy.mock.calls[2]; expect(calls[0].actions.find((b: any) => b.title === 'Delete')).toHaveProperty('disabled', true); - tree.unmount(); }); it('shows delete dialog when delete button is clicked', async () => { - const tree = await mountWithNPipelines(1); + tree = await mountWithNPipelines(1); tree.find('.tableRow').at(0).simulate('click'); const deleteBtn = (tree.instance() as PipelineList) .getInitialToolbarState().actions.find(b => b.title === 'Delete'); await deleteBtn!.action(); const call = updateDialogSpy.mock.calls[0][0]; expect(call).toHaveProperty('title', 'Delete 1 pipeline?'); - tree.unmount(); }); it('shows delete dialog when delete button is clicked, indicating several pipelines to delete', async () => { - const tree = await mountWithNPipelines(5); + tree = await mountWithNPipelines(5); tree.find('.tableRow').at(0).simulate('click'); tree.find('.tableRow').at(2).simulate('click'); tree.find('.tableRow').at(3).simulate('click'); @@ -258,11 +246,10 @@ describe('PipelineList', () => { await deleteBtn!.action(); const call = updateDialogSpy.mock.calls[0][0]; expect(call).toHaveProperty('title', 'Delete 3 pipelines?'); - tree.unmount(); }); it('does not call delete API for selected pipeline when delete dialog is canceled', async () => { - const tree = await mountWithNPipelines(1); + tree = await mountWithNPipelines(1); tree.find('.tableRow').at(0).simulate('click'); const deleteBtn = (tree.instance() as PipelineList) .getInitialToolbarState().actions.find(b => b.title === 'Delete'); @@ -271,11 +258,10 @@ describe('PipelineList', () => { const cancelBtn = call.buttons.find((b: any) => b.text === 'Cancel'); await cancelBtn.onClick(); expect(deletePipelineSpy).not.toHaveBeenCalled(); - tree.unmount(); }); it('calls delete API for selected pipeline after delete dialog is confirmed', async () => { - const tree = await mountWithNPipelines(1); + tree = await mountWithNPipelines(1); tree.find('.tableRow').at(0).simulate('click'); const deleteBtn = (tree.instance() as PipelineList) .getInitialToolbarState().actions.find(b => b.title === 'Delete'); @@ -284,11 +270,10 @@ describe('PipelineList', () => { const confirmBtn = call.buttons.find((b: any) => b.text === 'Delete'); await confirmBtn.onClick(); expect(deletePipelineSpy).toHaveBeenLastCalledWith('test-pipeline-id0'); - tree.unmount(); }); it('updates the selected indices after a pipeline is deleted', async () => { - const tree = await mountWithNPipelines(5); + tree = await mountWithNPipelines(5); tree.find('.tableRow').at(0).simulate('click'); expect(tree.state()).toHaveProperty('selectedIds', ['test-pipeline-id0']); const deleteBtn = (tree.instance() as PipelineList) @@ -298,11 +283,10 @@ describe('PipelineList', () => { const confirmBtn = call.buttons.find((b: any) => b.text === 'Delete'); await confirmBtn.onClick(); expect(tree.state()).toHaveProperty('selectedIds', []); - tree.unmount(); }); it('updates the selected indices after multiple pipelines are deleted', async () => { - const tree = await mountWithNPipelines(5); + tree = await mountWithNPipelines(5); tree.find('.tableRow').at(0).simulate('click'); tree.find('.tableRow').at(3).simulate('click'); expect(tree.state()).toHaveProperty('selectedIds', ['test-pipeline-id0', 'test-pipeline-id3']); @@ -313,11 +297,10 @@ describe('PipelineList', () => { const confirmBtn = call.buttons.find((b: any) => b.text === 'Delete'); await confirmBtn.onClick(); expect(tree.state()).toHaveProperty('selectedIds', []); - tree.unmount(); }); it('calls delete API for all selected pipelines after delete dialog is confirmed', async () => { - const tree = await mountWithNPipelines(5); + tree = await mountWithNPipelines(5); tree.find('.tableRow').at(0).simulate('click'); tree.find('.tableRow').at(1).simulate('click'); tree.find('.tableRow').at(4).simulate('click'); @@ -331,11 +314,10 @@ describe('PipelineList', () => { expect(deletePipelineSpy).toHaveBeenCalledWith('test-pipeline-id0'); expect(deletePipelineSpy).toHaveBeenCalledWith('test-pipeline-id1'); expect(deletePipelineSpy).toHaveBeenCalledWith('test-pipeline-id4'); - tree.unmount(); }); it('shows snackbar confirmation after pipeline is deleted', async () => { - const tree = await mountWithNPipelines(1); + tree = await mountWithNPipelines(1); tree.find('.tableRow').at(0).simulate('click'); const deleteBtn = (tree.instance() as PipelineList) .getInitialToolbarState().actions.find(b => b.title === 'Delete'); @@ -347,11 +329,10 @@ describe('PipelineList', () => { message: 'Successfully deleted 1 pipeline!', open: true, }); - tree.unmount(); }); it('shows error dialog when pipeline deletion fails', async () => { - const tree = await mountWithNPipelines(1); + tree = await mountWithNPipelines(1); tree.find('.tableRow').at(0).simulate('click'); TestUtils.makeErrorResponseOnce(deletePipelineSpy, 'woops, failed'); const deleteBtn = (tree.instance() as PipelineList) @@ -365,11 +346,10 @@ describe('PipelineList', () => { content: 'Deleting pipeline: test pipeline name0 failed with error: "woops, failed"', title: 'Failed to delete 1 pipeline', }); - tree.unmount(); }); it('shows error dialog when multiple pipeline deletions fail', async () => { - const tree = await mountWithNPipelines(5); + tree = await mountWithNPipelines(5); tree.find('.tableRow').at(0).simulate('click'); tree.find('.tableRow').at(2).simulate('click'); tree.find('.tableRow').at(1).simulate('click'); @@ -401,56 +381,71 @@ describe('PipelineList', () => { message: 'Successfully deleted 2 pipelines!', open: true, }); - tree.unmount(); }); it('shows upload dialog when upload button is clicked', async () => { - const tree = await mountWithNPipelines(0); + tree = await mountWithNPipelines(0); const instance = tree.instance() as PipelineList; const uploadBtn = instance.getInitialToolbarState().actions.find(b => b.title === 'Upload pipeline'); expect(uploadBtn).toBeDefined(); await uploadBtn!.action(); expect(instance.state).toHaveProperty('uploadDialogOpen', true); - tree.unmount(); }); - it('can dismiss upload dialog', async () => { - const tree = shallow(); + it('dismisses the upload dialog', async () => { + tree = shallow(); tree.setState({ uploadDialogOpen: true }); - tree.find('UploadPipelineDialog').simulate('close'); + tree.find('UploadPipelineDialog').simulate('close', false); tree.update(); expect(tree.state()).toHaveProperty('uploadDialogOpen', false); - tree.unmount(); }); - it('does not try to upload if no file is returned from upload dialog', async () => { - const tree = shallow(); + it('does not try to upload if the upload dialog dismissed', async () => { + tree = shallow(); const handlerSpy = jest.spyOn(tree.instance() as any, '_uploadDialogClosed'); tree.setState({ uploadDialogOpen: true }); - tree.find('UploadPipelineDialog').simulate('close', 'some name', null); - expect(handlerSpy).toHaveBeenLastCalledWith('some name', null); + tree.find('UploadPipelineDialog').simulate('close', false); + expect(handlerSpy).toHaveBeenLastCalledWith(false); + expect(uploadPipelineSpy).not.toHaveBeenCalled(); + }); + + it('does not try to upload if import method is local and no file is returned from upload dialog', async () => { + tree = shallow(); + const handlerSpy = jest.spyOn(tree.instance() as any, '_uploadDialogClosed'); + tree.setState({ uploadDialogOpen: true }); + tree.find('UploadPipelineDialog').simulate('close', true, 'some name', null, '', ImportMethod.LOCAL); + expect(handlerSpy).toHaveBeenLastCalledWith(true, 'some name', null, '', ImportMethod.LOCAL); + expect(uploadPipelineSpy).not.toHaveBeenCalled(); + }); + + it('does not try to upload if import method is url and no url is returned from upload dialog', async () => { + tree = shallow(); + const handlerSpy = jest.spyOn(tree.instance() as any, '_uploadDialogClosed'); + tree.setState({ uploadDialogOpen: true }); + tree.find('UploadPipelineDialog').simulate('close', true, 'some name', null, '', ImportMethod.URL); + expect(handlerSpy).toHaveBeenLastCalledWith(true, 'some name', null, '', ImportMethod.URL); expect(uploadPipelineSpy).not.toHaveBeenCalled(); - tree.unmount(); }); - it('tries to upload if a file is returned from upload dialog', async () => { - const tree = shallow(); + it('tries to upload if import method is local and a file is returned from upload dialog', async () => { + tree = shallow(); tree.setState({ uploadDialogOpen: true }); - tree.find('UploadPipelineDialog').simulate('close', 'some name', { body: 'something' }); + tree.find('UploadPipelineDialog').simulate('close', true, 'some name', { body: 'something' }, '', ImportMethod.LOCAL); tree.update(); + await createPipelineSpy; await uploadPipelineSpy; expect(uploadPipelineSpy).toHaveBeenLastCalledWith('some name', { body: 'something' }); + expect(createPipelineSpy).not.toHaveBeenCalled(); // Check the dialog is closed expect(tree.state()).toHaveProperty('uploadDialogOpen', false); - tree.unmount(); }); it('shows error dialog and does not dismiss upload dialog when upload fails', async () => { TestUtils.makeErrorResponseOnce(uploadPipelineSpy, 'woops, could not upload'); - const tree = shallow(); + tree = shallow(); tree.setState({ uploadDialogOpen: true }); - tree.find('UploadPipelineDialog').simulate('close', 'some name', { body: 'something' }); + tree.find('UploadPipelineDialog').simulate('close', true, 'some name', { body: 'something' }, '', ImportMethod.LOCAL); tree.update(); await uploadPipelineSpy; await TestUtils.flushPromises(); @@ -461,7 +456,44 @@ describe('PipelineList', () => { })); // Check the dialog is not closed expect(tree.state()).toHaveProperty('uploadDialogOpen', true); - tree.unmount(); + }); + + it('tries to create a pipeline if import method is url and a url is returned from upload dialog', async () => { + tree = shallow(); + tree.setState({ uploadDialogOpen: true }); + tree.find('UploadPipelineDialog').simulate('close', true, 'some name', null, 'https://some.url.com', ImportMethod.URL); + tree.update(); + await createPipelineSpy; + await uploadPipelineSpy; + expect(createPipelineSpy).toHaveBeenLastCalledWith({ + name: 'some name', + url: { pipeline_url: 'https://some.url.com' } + }); + expect(uploadPipelineSpy).not.toHaveBeenCalled(); + + // Check the dialog is closed + expect(tree.state()).toHaveProperty('uploadDialogOpen', false); + }); + + it('shows error dialog and does not dismiss upload dialog when create fails', async () => { + TestUtils.makeErrorResponseOnce(createPipelineSpy, 'woops, could not create'); + tree = shallow(); + tree.setState({ uploadDialogOpen: true }); + tree.find('UploadPipelineDialog').simulate('close', true, 'some name', null, 'https://some.url.com', ImportMethod.URL); + tree.update(); + await uploadPipelineSpy; + await TestUtils.flushPromises(); + expect(createPipelineSpy).toHaveBeenLastCalledWith({ + name: 'some name', + url: { pipeline_url: 'https://some.url.com' } + }); + expect(updateDialogSpy).toHaveBeenLastCalledWith(expect.objectContaining({ + content: 'woops, could not create', + title: 'Failed to upload pipeline', + })); + + // Check the dialog is not closed + expect(tree.state()).toHaveProperty('uploadDialogOpen', true); }); }); diff --git a/frontend/src/pages/PipelineList.tsx b/frontend/src/pages/PipelineList.tsx index 8916f1777ed..8ad2dc4be33 100644 --- a/frontend/src/pages/PipelineList.tsx +++ b/frontend/src/pages/PipelineList.tsx @@ -17,7 +17,7 @@ import * as React from 'react'; import AddIcon from '@material-ui/icons/Add'; import CustomTable, { Column, Row } from '../components/CustomTable'; -import UploadPipelineDialog from '../components/UploadPipelineDialog'; +import UploadPipelineDialog, { ImportMethod } from '../components/UploadPipelineDialog'; import produce from 'immer'; import { ApiPipeline, ApiListPipelinesResponse } from '../apis/pipeline'; import { Apis, PipelineSortKeys, ListRequest } from '../lib/Apis'; @@ -130,7 +130,7 @@ class PipelineList extends Page<{}, PipelineListState> { await this.showPageError('Error: failed to retrieve list of pipelines.', err); } - this.setStateSafe({ pipelines: response ? response.pipelines || [] : [] }); + this.setStateSafe({ pipelines: (response && response.pipelines) || [] }); return response ? response.next_page_token || '' : ''; } @@ -189,22 +189,28 @@ class PipelineList extends Page<{}, PipelineListState> { } } - private async _uploadDialogClosed(name: string, file: File | null, description?: string): Promise { - if (!!file) { - try { - await Apis.uploadPipeline(name, file); - this.setStateSafe({ uploadDialogOpen: false }); - this.refresh(); - return true; - } catch (err) { - const errorMessage = await errorToMessage(err); - this.showErrorDialog('Failed to upload pipeline', errorMessage); - return false; - } - } else { + private async _uploadDialogClosed(confirmed: boolean, name: string, file: File | null, url: string, + method: ImportMethod, description?: string): Promise { + + if (!confirmed + || (method === ImportMethod.LOCAL && !file) + || (method === ImportMethod.URL && !url)) { this.setStateSafe({ uploadDialogOpen: false }); return false; } + + try { + method === ImportMethod.LOCAL + ? await Apis.uploadPipeline(name, file!) + : await Apis.pipelineServiceApi.createPipeline({ name, url: { pipeline_url: url } }); + this.setStateSafe({ uploadDialogOpen: false }); + this.refresh(); + return true; + } catch (err) { + const errorMessage = await errorToMessage(err); + this.showErrorDialog('Failed to upload pipeline', errorMessage); + return false; + } } }