[UI] Redirect to experiment list page when namespace changes in RunDe…

…tails or Compare page (#3483) * Redirect to experiment list page when namespace changes * Fix namespace initialize case * Update KubeflowClient.tsx
kubeflow · Apr 10, 2020 · 32fff03 · 32fff03
1 parent e463728
commit 32fff03
Show file tree

Hide file tree

Showing 6 changed files with 239 additions and 42 deletions.
diff --git 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 <NamespaceContext.Provider value={this.state.namespace} {...this.props} />;
   }
 }
+
+function usePrevious<T>(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
@@ -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(
+        <Router history={history}>
+          <NamespaceContext.Provider value='ns1'>
+            <EnhancedCompare {...generateProps()} />
+          </NamespaceContext.Provider>
+        </Router>,
+      );
+      expect(history.location.pathname).not.toEqual('/experiments');
+      rerender(
+        <Router history={history}>
+          <NamespaceContext.Provider value='ns2'>
+            <EnhancedCompare {...generateProps()} />
+          </NamespaceContext.Provider>
+        </Router>,
+      );
+      expect(history.location.pathname).toEqual('/experiments');
+    });
+
+    it('does not redirect when namespace stays the same', () => {
+      const history = createMemoryHistory({
+        initialEntries: ['/initial-path'],
+      });
+      const { rerender } = render(
+        <Router history={history}>
+          <NamespaceContext.Provider value='ns1'>
+            <EnhancedCompare {...generateProps()} />
+          </NamespaceContext.Provider>
+        </Router>,
+      );
+      expect(history.location.pathname).toEqual('/initial-path');
+      rerender(
+        <Router history={history}>
+          <NamespaceContext.Provider value='ns1'>
+            <EnhancedCompare {...generateProps()} />
+          </NamespaceContext.Provider>
+        </Router>,
+      );
+      expect(history.location.pathname).toEqual('/initial-path');
+    });
+
+    it('does not redirect when namespace initializes', () => {
+      const history = createMemoryHistory({
+        initialEntries: ['/initial-path'],
+      });
+      const { rerender } = render(
+        <Router history={history}>
+          <NamespaceContext.Provider value={undefined}>
+            <EnhancedCompare {...generateProps()} />
+          </NamespaceContext.Provider>
+        </Router>,
+      );
+      expect(history.location.pathname).toEqual('/initial-path');
+      rerender(
+        <Router history={history}>
+          <NamespaceContext.Provider value='ns1'>
+            <EnhancedCompare {...generateProps()} />
+          </NamespaceContext.Provider>
+        </Router>,
+      );
+      expect(history.location.pathname).toEqual('/initial-path');
+    });
+  });
 });
diff --git 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<PageProps> = 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 <Redirect to={RoutePage.EXPERIMENTS} />;
+  }
+  return <Compare {...props} />;
+};
+
+export default EnhancedCompare;
+
+export const TEST_ONLY = {
+  Compare,
+};
diff --git 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<PageProps> = 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 <Redirect to={RoutePage.EXPERIMENTS} />;
   }
 

diff --git 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(
+        <Router history={history}>
+          <NamespaceContext.Provider value='ns1'>
+            <EnhancedRunDetails {...generateProps()} />
+          </NamespaceContext.Provider>
+        </Router>,
+      );
+      expect(history.location.pathname).not.toEqual('/experiments');
+      rerender(
+        <Router history={history}>
+          <NamespaceContext.Provider value='ns2'>
+            <EnhancedRunDetails {...generateProps()} />
+          </NamespaceContext.Provider>
+        </Router>,
+      );
+      expect(history.location.pathname).toEqual('/experiments');
+    });
+
+    it('does not redirect when namespace stays the same', () => {
+      const history = createMemoryHistory({
+        initialEntries: ['/initial-path'],
+      });
+      const { rerender } = render(
+        <Router history={history}>
+          <NamespaceContext.Provider value='ns1'>
+            <EnhancedRunDetails {...generateProps()} />
+          </NamespaceContext.Provider>
+        </Router>,
+      );
+      expect(history.location.pathname).toEqual('/initial-path');
+      rerender(
+        <Router history={history}>
+          <NamespaceContext.Provider value='ns1'>
+            <EnhancedRunDetails {...generateProps()} />
+          </NamespaceContext.Provider>
+        </Router>,
+      );
+      expect(history.location.pathname).toEqual('/initial-path');
+    });
+
+    it('does not redirect when namespace initializes', () => {
+      const history = createMemoryHistory({
+        initialEntries: ['/initial-path'],
+      });
+      const { rerender } = render(
+        <Router history={history}>
+          <NamespaceContext.Provider value={undefined}>
+            <EnhancedRunDetails {...generateProps()} />
+          </NamespaceContext.Provider>
+        </Router>,
+      );
+      expect(history.location.pathname).toEqual('/initial-path');
+      rerender(
+        <Router history={history}>
+          <NamespaceContext.Provider value='ns1'>
+            <EnhancedRunDetails {...generateProps()} />
+          </NamespaceContext.Provider>
+        </Router>,
+      );
+      expect(history.location.pathname).toEqual('/initial-path');
+    });
+  });
 });
 
 function clickGraphNode(wrapper: ShallowWrapper, nodeId: string) {