From 32fff03c2091b956743661667e8ed56fa7687163 Mon Sep 17 00:00:00 2001 From: "Yuan (Bob) Gong" Date: Fri, 10 Apr 2020 10:17:47 +0800 Subject: [PATCH] [UI] Redirect to experiment list page when namespace changes in RunDetails or Compare page (#3483) * Redirect to experiment list page when namespace changes * Fix namespace initialize case * Update KubeflowClient.tsx --- frontend/src/lib/KubeflowClient.tsx | 24 ++++++- frontend/src/pages/Compare.test.tsx | 79 ++++++++++++++++++++- frontend/src/pages/Compare.tsx | 48 ++++++++----- frontend/src/pages/ExperimentDetails.tsx | 8 +-- frontend/src/pages/RunDetails.test.tsx | 88 ++++++++++++++++++++++-- frontend/src/pages/RunDetails.tsx | 34 +++++---- 6 files changed, 239 insertions(+), 42 deletions(-) diff --git a/frontend/src/lib/KubeflowClient.tsx b/frontend/src/lib/KubeflowClient.tsx index 3268c6ece10..f4bd5e3f4e8 100644 --- a/frontend/src/lib/KubeflowClient.tsx +++ b/frontend/src/lib/KubeflowClient.tsx @@ -1,4 +1,4 @@ -import React from 'react'; +import React, { useLayoutEffect, useContext, useRef } from 'react'; import { logger } from './Utils'; declare global { @@ -47,3 +47,25 @@ export class NamespaceContextProvider extends React.Component { return ; } } + +function usePrevious(value: T) { + const ref = useRef(value); + useLayoutEffect(() => { + ref.current = value; + }); + return ref.current; +} + +export function useNamespaceChangeEvent(): boolean { + const currentNamespace = useContext(NamespaceContext); + const previousNamespace = usePrevious(currentNamespace); + + if (!previousNamespace) { + // Previous namespace hasn't been initialized, this does not count as a change. + // When the webapp inits, the first render will have namespace=undefined, so + // this situation happens often. + return false; + } + + return previousNamespace !== currentNamespace; +} diff --git a/frontend/src/pages/Compare.test.tsx b/frontend/src/pages/Compare.test.tsx index 15856ff0ed7..c3ac25428ec 100644 --- a/frontend/src/pages/Compare.test.tsx +++ b/frontend/src/pages/Compare.test.tsx @@ -15,7 +15,8 @@ */ import * as React from 'react'; -import Compare, { TaggedViewerConfig } from './Compare'; +import { createMemoryHistory } from 'history'; +import EnhancedCompare, { TEST_ONLY, TaggedViewerConfig } from './Compare'; import TestUtils from '../TestUtils'; import { ReactWrapper, ShallowWrapper, shallow } from 'enzyme'; import { Apis } from '../lib/Apis'; @@ -26,7 +27,11 @@ import { PlotType } from '../components/viewers/Viewer'; import { OutputArtifactLoader } from '../lib/OutputArtifactLoader'; import { Workflow } from '../../third_party/argo-ui/argo_template'; import { ButtonKeys } from '../lib/Buttons'; +import { render } from '@testing-library/react'; +import { Router } from 'react-router-dom'; +import { NamespaceContext } from 'src/lib/KubeflowClient'; +const Compare = TEST_ONLY.Compare; class TestCompare extends Compare { public _selectionChanged(selectedIds: string[]): void { return super._selectionChanged(selectedIds); @@ -143,7 +148,9 @@ describe('Compare', () => { afterEach(async () => { // unmount() should be called before resetAllMocks() in case any part of the unmount life cycle // depends on mocks/spies - await tree.unmount(); + if (tree && tree.exists()) { + await tree.unmount(); + } }); it('clears banner upon initial load', () => { @@ -582,4 +589,72 @@ describe('Compare', () => { expect(tree).toMatchSnapshot(); }); + + describe('EnhancedCompare', () => { + it('redirects to experiments page when namespace changes', () => { + const history = createMemoryHistory({ + initialEntries: ['/does-not-matter'], + }); + const { rerender } = render( + + + + + , + ); + expect(history.location.pathname).not.toEqual('/experiments'); + rerender( + + + + + , + ); + expect(history.location.pathname).toEqual('/experiments'); + }); + + it('does not redirect when namespace stays the same', () => { + const history = createMemoryHistory({ + initialEntries: ['/initial-path'], + }); + const { rerender } = render( + + + + + , + ); + expect(history.location.pathname).toEqual('/initial-path'); + rerender( + + + + + , + ); + expect(history.location.pathname).toEqual('/initial-path'); + }); + + it('does not redirect when namespace initializes', () => { + const history = createMemoryHistory({ + initialEntries: ['/initial-path'], + }); + const { rerender } = render( + + + + + , + ); + expect(history.location.pathname).toEqual('/initial-path'); + rerender( + + + + + , + ); + expect(history.location.pathname).toEqual('/initial-path'); + }); + }); }); diff --git a/frontend/src/pages/Compare.tsx b/frontend/src/pages/Compare.tsx index fd76cd516bf..a3aa35c98e0 100644 --- a/frontend/src/pages/Compare.tsx +++ b/frontend/src/pages/Compare.tsx @@ -15,28 +15,30 @@ */ import * as React from 'react'; -import Buttons from '../lib/Buttons'; +import { Redirect } from 'react-router-dom'; +import { useNamespaceChangeEvent } from 'src/lib/KubeflowClient'; +import { classes, stylesheet } from 'typestyle'; +import { Workflow } from '../../third_party/argo-ui/argo_template'; +import { ApiRunDetail } from '../apis/run'; +import Hr from '../atoms/Hr'; +import Separator from '../atoms/Separator'; import CollapseButton from '../components/CollapseButton'; import CompareTable, { CompareTableProps } from '../components/CompareTable'; -import CompareUtils from '../lib/CompareUtils'; -import Hr from '../atoms/Hr'; import PlotCard, { PlotCardProps } from '../components/PlotCard'; -import RunList from './RunList'; -import Separator from '../atoms/Separator'; -import WorkflowParser from '../lib/WorkflowParser'; -import { ApiRunDetail } from '../apis/run'; +import { QUERY_PARAMS, RoutePage } from '../components/Router'; +import { ToolbarProps } from '../components/Toolbar'; +import { PlotType, ViewerConfig } from '../components/viewers/Viewer'; +import { componentMap } from '../components/viewers/ViewerContainer'; +import { commonCss, padding } from '../Css'; import { Apis } from '../lib/Apis'; +import Buttons from '../lib/Buttons'; +import CompareUtils from '../lib/CompareUtils'; import { OutputArtifactLoader } from '../lib/OutputArtifactLoader'; -import { Page } from './Page'; -import { RoutePage, QUERY_PARAMS } from '../components/Router'; -import { ToolbarProps } from '../components/Toolbar'; import { URLParser } from '../lib/URLParser'; -import { ViewerConfig, PlotType } from '../components/viewers/Viewer'; -import { Workflow } from '../../third_party/argo-ui/argo_template'; -import { classes, stylesheet } from 'typestyle'; -import { commonCss, padding } from '../Css'; -import { componentMap } from '../components/viewers/ViewerContainer'; import { logger } from '../lib/Utils'; +import WorkflowParser from '../lib/WorkflowParser'; +import { Page, PageProps } from './Page'; +import RunList from './RunList'; const css = stylesheet({ outputsRow: { @@ -328,4 +330,18 @@ class Compare extends Page<{}, CompareState> { } } -export default Compare; +const EnhancedCompare: React.FC = props => { + const namespaceChanged = useNamespaceChangeEvent(); + if (namespaceChanged) { + // Compare page compares two runs, when namespace changes, the runs don't + // exist in the new namespace, so we should redirect to experiment list page. + return ; + } + return ; +}; + +export default EnhancedCompare; + +export const TEST_ONLY = { + Compare, +}; diff --git a/frontend/src/pages/ExperimentDetails.tsx b/frontend/src/pages/ExperimentDetails.tsx index 976af75fd32..e581b665511 100644 --- a/frontend/src/pages/ExperimentDetails.tsx +++ b/frontend/src/pages/ExperimentDetails.tsx @@ -34,7 +34,7 @@ import { RunStorageState } from '../apis/run'; import { classes, stylesheet } from 'typestyle'; import { color, commonCss, padding } from '../Css'; import { logger } from '../lib/Utils'; -import { NamespaceContext } from 'src/lib/KubeflowClient'; +import { useNamespaceChangeEvent } from 'src/lib/KubeflowClient'; import { Redirect } from 'react-router-dom'; const css = stylesheet({ @@ -346,12 +346,10 @@ export class ExperimentDetails extends Page<{}, ExperimentDetailsState> { } const EnhancedExperimentDetails: React.FC = props => { - const namespace = React.useContext(NamespaceContext); - const [initialNamespace] = React.useState(namespace); - // When namespace changes, this experiment no longer belongs to new namespace. // So we redirect to experiment list page instead. - if (namespace !== initialNamespace) { + const namespaceChanged = useNamespaceChangeEvent(); + if (namespaceChanged) { return ; } diff --git a/frontend/src/pages/RunDetails.test.tsx b/frontend/src/pages/RunDetails.test.tsx index 51119a0f53d..08e9c44befc 100644 --- a/frontend/src/pages/RunDetails.test.tsx +++ b/frontend/src/pages/RunDetails.test.tsx @@ -17,8 +17,10 @@ import { Api, GetArtifactTypesResponse } from '@kubeflow/frontend'; import { mount, ReactWrapper, shallow, ShallowWrapper } from 'enzyme'; import * as React from 'react'; +import { render } from '@testing-library/react'; +import { createMemoryHistory } from 'history'; import { Workflow } from 'third_party/argo-ui/argo_template'; -import { ApiRelationship, ApiResourceType, ApiRunDetail, RunStorageState } from '../apis/run'; +import { ApiResourceType, ApiRunDetail, RunStorageState } from '../apis/run'; import { QUERY_PARAMS, RoutePage, RouteParams } from '../components/Router'; import { PlotType } from '../components/viewers/Viewer'; import { Apis } from '../lib/Apis'; @@ -27,9 +29,13 @@ import { OutputArtifactLoader } from '../lib/OutputArtifactLoader'; import { NodePhase } from '../lib/StatusUtils'; import * as Utils from '../lib/Utils'; import WorkflowParser from '../lib/WorkflowParser'; -import TestUtils, { diff } from '../TestUtils'; +import TestUtils from '../TestUtils'; import { PageProps } from './Page'; -import { RunDetails, RunDetailsInternalProps } from './RunDetails'; +import EnhancedRunDetails, { TEST_ONLY, RunDetailsInternalProps } from './RunDetails'; +import { Router } from 'react-router-dom'; +import { NamespaceContext } from 'src/lib/KubeflowClient'; + +const RunDetails = TEST_ONLY.RunDetails; const STEP_TABS = { ARTIFACTS: 0, @@ -142,9 +148,11 @@ describe('RunDetails', () => { }); afterEach(async () => { - // unmount() should be called before resetAllMocks() in case any part of the unmount life cycle - // depends on mocks/spies - await tree.unmount(); + if (tree && tree.exists()) { + // unmount() should be called before resetAllMocks() in case any part of the unmount life cycle + // depends on mocks/spies + await tree.unmount(); + } jest.resetAllMocks(); jest.restoreAllMocks(); }); @@ -1318,6 +1326,74 @@ describe('RunDetails', () => { expect(setInterval).toHaveBeenCalledTimes(0); }); }); + + describe('EnhancedRunDetails', () => { + it('redirects to experiments page when namespace changes', () => { + const history = createMemoryHistory({ + initialEntries: ['/does-not-matter'], + }); + const { rerender } = render( + + + + + , + ); + expect(history.location.pathname).not.toEqual('/experiments'); + rerender( + + + + + , + ); + expect(history.location.pathname).toEqual('/experiments'); + }); + + it('does not redirect when namespace stays the same', () => { + const history = createMemoryHistory({ + initialEntries: ['/initial-path'], + }); + const { rerender } = render( + + + + + , + ); + expect(history.location.pathname).toEqual('/initial-path'); + rerender( + + + + + , + ); + expect(history.location.pathname).toEqual('/initial-path'); + }); + + it('does not redirect when namespace initializes', () => { + const history = createMemoryHistory({ + initialEntries: ['/initial-path'], + }); + const { rerender } = render( + + + + + , + ); + expect(history.location.pathname).toEqual('/initial-path'); + rerender( + + + + + , + ); + expect(history.location.pathname).toEqual('/initial-path'); + }); + }); }); function clickGraphNode(wrapper: ShallowWrapper, nodeId: string) { diff --git a/frontend/src/pages/RunDetails.tsx b/frontend/src/pages/RunDetails.tsx index 4c6667737f1..8762984aa46 100644 --- a/frontend/src/pages/RunDetails.tsx +++ b/frontend/src/pages/RunDetails.tsx @@ -14,11 +14,20 @@ * limitations under the License. */ +import { Context, Execution, getResourceProperty } from '@kubeflow/frontend'; import CircularProgress from '@material-ui/core/CircularProgress'; import InfoIcon from '@material-ui/icons/InfoOutlined'; import { flatten } from 'lodash'; import * as React from 'react'; +import { Link, Redirect } from 'react-router-dom'; import { GkeMetadata, GkeMetadataContext } from 'src/lib/GkeMetadata'; +import { useNamespaceChangeEvent } from 'src/lib/KubeflowClient'; +import { + ExecutionHelpers, + getExecutionsFromContext, + getTfxRunContext, + KfpExecutionProperties, +} from 'src/lib/MlmdUtils'; import { classes, stylesheet } from 'typestyle'; import { NodePhase as ArgoNodePhase, @@ -38,7 +47,8 @@ import Graph from '../components/Graph'; import LogViewer from '../components/LogViewer'; import MinioArtifactLink from '../components/MinioArtifactLink'; import PlotCard from '../components/PlotCard'; -import { RoutePage, RouteParams, RoutePageFactory } from '../components/Router'; +import { PodEvents, PodInfo } from '../components/PodYaml'; +import { RoutePage, RoutePageFactory, RouteParams } from '../components/Router'; import SidePanel from '../components/SidePanel'; import { ToolbarProps } from '../components/Toolbar'; import { HTMLViewerConfig } from '../components/viewers/HTMLViewer'; @@ -63,18 +73,9 @@ import { serviceErrorToString, } from '../lib/Utils'; import WorkflowParser from '../lib/WorkflowParser'; +import { ExecutionDetailsContent } from './ExecutionDetails'; import { Page, PageProps } from './Page'; import { statusToIcon } from './Status'; -import { PodEvents, PodInfo } from '../components/PodYaml'; -import { Link } from 'react-router-dom'; -import { - getTfxRunContext, - getExecutionsFromContext, - KfpExecutionProperties, - ExecutionHelpers, -} from 'src/lib/MlmdUtils'; -import { Context, Execution, getResourceProperty } from '@kubeflow/frontend'; -import { ExecutionDetailsContent } from './ExecutionDetails'; enum SidePaneTab { ARTIFACTS, @@ -162,7 +163,7 @@ export const css = stylesheet({ }, }); -export class RunDetails extends Page { +class RunDetails extends Page { public state: RunDetailsState = { allArtifactConfigs: [], allowCustomVisualizations: false, @@ -1100,8 +1101,17 @@ const ArtifactsTabContent: React.FC<{ }; const EnhancedRunDetails: React.FC = props => { + const namespaceChanged = useNamespaceChangeEvent(); const gkeMetadata = React.useContext(GkeMetadataContext); + if (namespaceChanged) { + // Run details page shows info about a run, when namespace changes, the run + // doesn't exist in the new namespace, so we should redirect to experiment + // list page. + return ; + } return ; }; export default EnhancedRunDetails; + +export const TEST_ONLY = { RunDetails };