From 6b7739d8b6317081675024557d6b6c91ac6e1d60 Mon Sep 17 00:00:00 2001 From: Joe Li <56132941+jlyaoyuli@users.noreply.github.com> Date: Fri, 1 Sep 2023 10:39:35 -0700 Subject: [PATCH] chore(frontend): Refactor NewExperiment to functional component (#9948) * Add an alternative component for new experiment page. * Add unit tests. Assign default values for useState(). * Move the files to functional_components folder. Update the imported files from absolute path. * Move handing experiemnt creation error to useEffect rename the create helper to createExperiment. * Move redirection logic after createExperiment() is succeed to useEffect(). * Fix dependency array for handling createExperiment() succeed case. * Remove pipeline id in dependency array. --- frontend/src/pages/NewExperiment.tsx | 28 +- .../NewExperimentFC.test.tsx | 299 ++++++++++++++++++ .../functional_components/NewExperimentFC.tsx | 194 ++++++++++++ 3 files changed, 510 insertions(+), 11 deletions(-) create mode 100644 frontend/src/pages/functional_components/NewExperimentFC.test.tsx create mode 100644 frontend/src/pages/functional_components/NewExperimentFC.tsx diff --git a/frontend/src/pages/NewExperiment.tsx b/frontend/src/pages/NewExperiment.tsx index b3bd1546968..7f1dc8fb130 100644 --- a/frontend/src/pages/NewExperiment.tsx +++ b/frontend/src/pages/NewExperiment.tsx @@ -15,21 +15,23 @@ */ import * as React from 'react'; -import BusyButton from '../atoms/BusyButton'; +import BusyButton from 'src/atoms/BusyButton'; import Button from '@material-ui/core/Button'; -import Input from '../atoms/Input'; +import Input from 'src/atoms/Input'; import { V2beta1Experiment } from 'src/apisv2beta1/experiment'; -import { Apis } from '../lib/Apis'; -import { Page, PageProps } from './Page'; -import { RoutePage, QUERY_PARAMS } from '../components/Router'; +import { Apis } from 'src/lib/Apis'; +import { Page, PageProps } from 'src/pages/Page'; +import { RoutePage, QUERY_PARAMS } from 'src/components/Router'; import { TextFieldProps } from '@material-ui/core/TextField'; -import { ToolbarProps } from '../components/Toolbar'; -import { URLParser } from '../lib/URLParser'; +import { ToolbarProps } from 'src/components/Toolbar'; +import { URLParser } from 'src/lib/URLParser'; import { classes, stylesheet } from 'typestyle'; -import { commonCss, padding, fontsize } from '../Css'; -import { logger, errorToMessage } from '../lib/Utils'; +import { commonCss, padding, fontsize } from 'src/Css'; +import { logger, errorToMessage } from 'src/lib/Utils'; import { NamespaceContext } from 'src/lib/KubeflowClient'; -import { getLatestVersion } from './NewRunV2'; +import { getLatestVersion } from 'src/pages/NewRunV2'; +import { NewExperimentFC } from 'src/pages/functional_components/NewExperimentFC'; +import { FeatureKey, isFeatureEnabled } from 'src/features'; interface NewExperimentState { description: string; @@ -201,7 +203,11 @@ export class NewExperiment extends Page<{ namespace?: string }, NewExperimentSta const EnhancedNewExperiment: React.FC = props => { const namespace = React.useContext(NamespaceContext); - return ; + return isFeatureEnabled(FeatureKey.FUNCTIONAL_COMPONENT) ? ( + + ) : ( + + ); }; export default EnhancedNewExperiment; diff --git a/frontend/src/pages/functional_components/NewExperimentFC.test.tsx b/frontend/src/pages/functional_components/NewExperimentFC.test.tsx new file mode 100644 index 00000000000..7ab083a6c5a --- /dev/null +++ b/frontend/src/pages/functional_components/NewExperimentFC.test.tsx @@ -0,0 +1,299 @@ +/* + * Copyright 2023 The Kubeflow Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { fireEvent, render, screen, waitFor } from '@testing-library/react'; +import * as React from 'react'; +import { CommonTestWrapper } from 'src/TestWrapper'; +import TestUtils from 'src/TestUtils'; +import { NewExperimentFC } from './NewExperimentFC'; +import { Apis } from 'src/lib/Apis'; +import { PageProps } from 'src/pages/Page'; +import * as features from 'src/features'; +import { RoutePage, QUERY_PARAMS } from 'src/components/Router'; + +describe('NewExperiment', () => { + const TEST_EXPERIMENT_ID = 'new-experiment-id'; + const createExperimentSpy = jest.spyOn(Apis.experimentServiceApiV2, 'createExperiment'); + const historyPushSpy = jest.fn(); + const updateDialogSpy = jest.fn(); + const updateSnackbarSpy = jest.fn(); + const updateToolbarSpy = jest.fn(); + + function generateProps(): PageProps { + return { + history: { push: historyPushSpy } as any, + location: { pathname: RoutePage.NEW_EXPERIMENT } as any, + match: '' as any, + toolbarProps: { actions: {}, breadcrumbs: [], pageTitle: TEST_EXPERIMENT_ID }, + updateBanner: () => null, + updateDialog: updateDialogSpy, + updateSnackbar: updateSnackbarSpy, + updateToolbar: updateToolbarSpy, + }; + } + + beforeEach(() => { + jest.clearAllMocks(); + // mock both v2_alpha and functional feature keys are enable. + jest.spyOn(features, 'isFeatureEnabled').mockReturnValue(true); + + createExperimentSpy.mockImplementation(() => ({ + experiment_id: 'new-experiment-id', + display_name: 'new-experiment-name', + })); + }); + + it('does not include any action buttons in the toolbar', () => { + render( + + + , + ); + + expect(updateToolbarSpy).toHaveBeenCalledWith({ + actions: {}, + breadcrumbs: [{ displayName: 'Experiments', href: RoutePage.EXPERIMENTS }], + pageTitle: 'New experiment', + }); + }); + + it("enables the 'Next' button when an experiment name is entered", () => { + render( + + + , + ); + + const experimentNameInput = screen.getByLabelText(/Experiment name/); + fireEvent.change(experimentNameInput, { target: { value: 'new-experiment-name' } }); + const nextButton = screen.getByText('Next'); + expect(nextButton.closest('button')?.disabled).toEqual(false); + }); + + it("re-disables the 'Next' button when an experiment name is cleared after having been entered", () => { + render( + + + , + ); + + const experimentNameInput = screen.getByLabelText(/Experiment name/); + fireEvent.change(experimentNameInput, { target: { value: 'new-experiment-name' } }); + const nextButton = screen.getByText('Next'); + expect(nextButton.closest('button')?.disabled).toEqual(false); + + // Remove experiment name + fireEvent.change(experimentNameInput, { target: { value: '' } }); + expect(nextButton.closest('button')?.disabled).toEqual(true); + }); + + it('updates the experiment name', () => { + render( + + + , + ); + + const experimentNameInput = screen.getByLabelText(/Experiment name/); + fireEvent.change(experimentNameInput, { target: { value: 'new-experiment-name' } }); + expect(experimentNameInput.closest('input')?.value).toBe('new-experiment-name'); + }); + + it('create new experiment', async () => { + render( + + + , + ); + + const experimentNameInput = screen.getByLabelText(/Experiment name/); + fireEvent.change(experimentNameInput, { target: { value: 'new-experiment-name' } }); + const experimentDescriptionInput = screen.getByLabelText('Description'); + fireEvent.change(experimentDescriptionInput, { + target: { value: 'new-experiment-description' }, + }); + const nextButton = screen.getByText('Next'); + expect(nextButton.closest('button')?.disabled).toEqual(false); + + fireEvent.click(nextButton); + await waitFor(() => { + expect(createExperimentSpy).toHaveBeenCalledWith( + expect.objectContaining({ + description: 'new-experiment-description', + display_name: 'new-experiment-name', + }), + ); + }); + }); + + it('create new experiment with namespace provided', async () => { + render( + + + , + ); + + const experimentNameInput = screen.getByLabelText(/Experiment name/); + fireEvent.change(experimentNameInput, { target: { value: 'new-experiment-name' } }); + const nextButton = screen.getByText('Next'); + expect(nextButton.closest('button')?.disabled).toEqual(false); + + fireEvent.click(nextButton); + await waitFor(() => { + expect(createExperimentSpy).toHaveBeenCalledWith( + expect.objectContaining({ + description: '', + display_name: 'new-experiment-name', + namespace: 'test-ns', + }), + ); + }); + }); + + it('navigates to NewRun page upon successful creation', async () => { + render( + + + , + ); + + const experimentNameInput = screen.getByLabelText(/Experiment name/); + fireEvent.change(experimentNameInput, { target: { value: 'new-experiment-name' } }); + const nextButton = screen.getByText('Next'); + expect(nextButton.closest('button')?.disabled).toEqual(false); + + fireEvent.click(nextButton); + await waitFor(() => { + expect(createExperimentSpy).toHaveBeenCalledWith( + expect.objectContaining({ + description: '', + display_name: 'new-experiment-name', + }), + ); + }); + expect(historyPushSpy).toHaveBeenCalledWith( + RoutePage.NEW_RUN + `?experimentId=${TEST_EXPERIMENT_ID}` + `&firstRunInExperiment=1`, + ); + }); + + it('includes pipeline ID and version ID in NewRun page query params if present', async () => { + const pipelineId = 'some-pipeline-id'; + const pipelineVersionId = 'version-id'; + const listPipelineVersionsSpy = jest.spyOn(Apis.pipelineServiceApiV2, 'listPipelineVersions'); + listPipelineVersionsSpy.mockImplementation(() => ({ + pipeline_versions: [{ pipeline_version_id: pipelineVersionId }], + })); + + const props = generateProps(); + props.location.search = `?${QUERY_PARAMS.pipelineId}=${pipelineId}`; + + render( + + + , + ); + + const experimentNameInput = screen.getByLabelText(/Experiment name/); + fireEvent.change(experimentNameInput, { target: { value: 'new-experiment-name' } }); + const nextButton = screen.getByText('Next'); + expect(nextButton.closest('button')?.disabled).toEqual(false); + + fireEvent.click(nextButton); + await waitFor(() => { + expect(createExperimentSpy).toHaveBeenCalledWith( + expect.objectContaining({ + description: '', + display_name: 'new-experiment-name', + }), + ); + }); + + expect(historyPushSpy).toHaveBeenCalledWith( + RoutePage.NEW_RUN + + `?experimentId=${TEST_EXPERIMENT_ID}` + + `&pipelineId=${pipelineId}` + + `&pipelineVersionId=${pipelineVersionId}` + + `&firstRunInExperiment=1`, + ); + }); + + it('shows snackbar confirmation after experiment is created', async () => { + render( + + + , + ); + + const experimentNameInput = screen.getByLabelText(/Experiment name/); + fireEvent.change(experimentNameInput, { target: { value: 'new-experiment-name' } }); + const nextButton = screen.getByText('Next'); + expect(nextButton.closest('button')?.disabled).toEqual(false); + + fireEvent.click(nextButton); + await waitFor(() => { + expect(createExperimentSpy).toHaveBeenCalledWith( + expect.objectContaining({ + description: '', + display_name: 'new-experiment-name', + }), + ); + }); + expect(updateSnackbarSpy).toHaveBeenLastCalledWith({ + autoHideDuration: 10000, + message: 'Successfully created new Experiment: new-experiment-name', + open: true, + }); + }); + + it('shows error dialog when experiment creation fails', async () => { + TestUtils.makeErrorResponseOnce(createExperimentSpy, 'There was something wrong!'); + render( + + + , + ); + + const experimentNameInput = screen.getByLabelText(/Experiment name/); + fireEvent.change(experimentNameInput, { target: { value: 'new-experiment-name' } }); + const nextButton = screen.getByText('Next'); + expect(nextButton.closest('button')?.disabled).toEqual(false); + + fireEvent.click(nextButton); + await waitFor(() => { + expect(createExperimentSpy).toHaveBeenCalled(); + }); + + expect(updateDialogSpy).toHaveBeenCalledWith( + expect.objectContaining({ + content: 'There was something wrong!', + title: 'Experiment creation failed', + }), + ); + }); + + it('navigates to experiment list page upon cancellation', () => { + render( + + + , + ); + + const cancelButton = screen.getByText('Cancel'); + fireEvent.click(cancelButton); + + expect(historyPushSpy).toHaveBeenCalledWith(RoutePage.EXPERIMENTS); + }); +}); diff --git a/frontend/src/pages/functional_components/NewExperimentFC.tsx b/frontend/src/pages/functional_components/NewExperimentFC.tsx new file mode 100644 index 00000000000..4cc53e8caf3 --- /dev/null +++ b/frontend/src/pages/functional_components/NewExperimentFC.tsx @@ -0,0 +1,194 @@ +/* + * Copyright 2023 The Kubeflow Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import React, { useEffect, useState } from 'react'; +import BusyButton from 'src/atoms/BusyButton'; +import Button from '@material-ui/core/Button'; +import Input from 'src/atoms/Input'; +import { V2beta1Experiment } from 'src/apisv2beta1/experiment'; +import { Apis } from 'src/lib/Apis'; +import { PageProps } from 'src/pages/Page'; +import { RoutePage, QUERY_PARAMS } from 'src/components/Router'; +import { URLParser } from 'src/lib/URLParser'; +import { classes, stylesheet } from 'typestyle'; +import { commonCss, padding, fontsize } from 'src/Css'; +import { errorToMessage } from 'src/lib/Utils'; +import { getLatestVersion } from 'src/pages/NewRunV2'; +import { useMutation } from 'react-query'; +import { V2beta1PipelineVersion } from 'src/apisv2beta1/pipeline'; + +const css = stylesheet({ + errorMessage: { + color: 'red', + }, + // TODO: move to Css.tsx and probably rename. + explanation: { + fontSize: fontsize.small, + }, +}); + +interface ExperimentProps { + namespace?: string; +} + +type NewExperimentFCProps = ExperimentProps & PageProps; + +export function NewExperimentFC(props: NewExperimentFCProps) { + const urlParser = new URLParser(props); + const { namespace, updateDialog, updateSnackbar, updateToolbar } = props; + const [description, setDescription] = useState(''); + const [experimentName, setExperimentName] = useState(''); + const [isbeingCreated, setIsBeingCreated] = useState(false); + const [errorMessage, setErrorMessage] = useState(''); + const [latestVersion, setLatestVersion] = useState(); + const [experimentResponse, setExperimentResponse] = useState(); + const [errMsgFromApi, setErrMsgFromApi] = useState(); + const pipelineId = urlParser.get(QUERY_PARAMS.pipelineId); + + useEffect(() => { + updateToolbar({ + actions: {}, + breadcrumbs: [{ displayName: 'Experiments', href: RoutePage.EXPERIMENTS }], + pageTitle: 'New experiment', + }); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, []); + + useEffect(() => { + if (pipelineId) { + (async () => { + setLatestVersion(await getLatestVersion(pipelineId)); + })(); + } + }, [pipelineId]); + + // Handle the redirection work when createExperiment is succeed + useEffect(() => { + if (experimentResponse) { + const searchString = pipelineId + ? new URLParser(props).build({ + [QUERY_PARAMS.experimentId]: experimentResponse.experiment_id || '', + [QUERY_PARAMS.pipelineId]: pipelineId, + [QUERY_PARAMS.pipelineVersionId]: latestVersion?.pipeline_version_id || '', + [QUERY_PARAMS.firstRunInExperiment]: '1', + }) + : new URLParser(props).build({ + [QUERY_PARAMS.experimentId]: experimentResponse.experiment_id || '', + [QUERY_PARAMS.firstRunInExperiment]: '1', + }); + props.history.push(RoutePage.NEW_RUN + searchString); + + updateSnackbar({ + autoHideDuration: 10000, + message: `Successfully created new Experiment: ${experimentResponse.display_name}`, + open: true, + }); + } + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [experimentResponse]); + + // Handle the error when createExperiment() is failed + useEffect(() => { + if (!experimentName) { + setErrorMessage('Experiment name is required'); + } else { + setErrorMessage(''); + } + }, [experimentName]); + + useEffect(() => { + if (errMsgFromApi) { + updateDialog({ + buttons: [{ text: 'Dismiss' }], + onClose: () => setIsBeingCreated(false), + content: errMsgFromApi, + title: 'Experiment creation failed', + }); + } + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [errMsgFromApi]); + + const newExperimentMutation = useMutation((experiment: V2beta1Experiment) => { + return Apis.experimentServiceApiV2.createExperiment(experiment); + }); + + const createExperiment = () => { + let newExperiment: V2beta1Experiment = { + display_name: experimentName, + description: description, + namespace: namespace, + }; + setIsBeingCreated(true); + + newExperimentMutation.mutate(newExperiment, { + onSuccess: response => { + setExperimentResponse(response); + }, + onError: async err => { + setErrMsgFromApi(await errorToMessage(err)); + }, + }); + }; + + return ( +
+
+
Experiment details
+
+ Think of an Experiment as a space that contains the history of all pipelines and their + associated runs +
+ + setExperimentName(event.target.value)} + value={experimentName} + autoFocus={true} + variant='outlined' + /> + setDescription(event.target.value)} + required={false} + value={description} + variant='outlined' + /> + +
+ + +
{errorMessage}
+
+
+
+ ); +}