Skip to content

Commit

Permalink
[UI] Auto refresh/redirect experiment list/detail page when user sele…
Browse files Browse the repository at this point in the history 
…cts a different namespace (kubeflow#3297)

* [UI] Auto refresh/redirect experiment list/detail page when namespace changes

* Add comments

* Address CR comment
  • Loading branch information
@Bobgy @Jeffwan
Bobgy authored and Jeffwan committed Dec 9, 2020
1 parent 9be63f0 commit 7564166
Show file tree
Hide file tree
Showing 4 changed files with 119 additions and 36 deletions.
36 changes: 34 additions & 2 deletions frontend/src/pages/ExperimentDetails.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
*/

import * as React from 'react';
import ExperimentDetails from './ExperimentDetails';
import EnhancedExperimentDetails, { ExperimentDetails } from './ExperimentDetails';
import TestUtils from '../TestUtils';
import { ApiExperiment } from '../apis/experiment';
import { Apis } from '../lib/Apis';
Expand All @@ -26,6 +26,10 @@ import { RunStorageState } from '../apis/run';
import { ToolbarProps } from '../components/Toolbar';
import { range } from 'lodash';
import { ButtonKeys } from '../lib/Buttons';
import { render } from '@testing-library/react';
import { NamespaceContext } from 'src/lib/KubeflowClient';
import { Router } from 'react-router-dom';
import { createMemoryHistory } from 'history';

describe('ExperimentDetails', () => {
let tree: ReactWrapper | ShallowWrapper;
Expand Down Expand Up @@ -108,7 +112,9 @@ describe('ExperimentDetails', () => {
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.exists()) {
await tree.unmount();
}
});

it('renders a page with no runs or recurring runs', async () => {
Expand Down Expand Up @@ -500,4 +506,30 @@ describe('ExperimentDetails', () => {
.simulate('click');
}
});

describe('EnhancedExperimentDetails', () => {
it('renders ExperimentDetails initially', () => {
render(<EnhancedExperimentDetails {...generateProps()}></EnhancedExperimentDetails>);
expect(getExperimentSpy).toHaveBeenCalledTimes(1);
});

it('redirects to ExperimentList page if namespace changes', () => {
const history = createMemoryHistory();
const { rerender } = render(
<Router history={history}>
<NamespaceContext.Provider value='test-ns-1'>
<EnhancedExperimentDetails {...generateProps()} />
</NamespaceContext.Provider>
</Router>,
);
rerender(
<Router history={history}>
<NamespaceContext.Provider value='test-ns-2'>
<EnhancedExperimentDetails {...generateProps()} />
</NamespaceContext.Provider>
</Router>,
);
expect(history.location.pathname).toEqual(RoutePage.EXPERIMENTS);
});
});
});
21 changes: 18 additions & 3 deletions frontend/src/pages/ExperimentDetails.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,14 @@ import Toolbar, { ToolbarProps } from '../components/Toolbar';
import Tooltip from '@material-ui/core/Tooltip';
import { ApiExperiment } from '../apis/experiment';
import { Apis } from '../lib/Apis';
import { Page } from './Page';
import { Page, PageProps } from './Page';
import { RoutePage, RouteParams } from '../components/Router';
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 { Redirect } from 'react-router-dom';

const css = stylesheet({
card: {
Expand Down Expand Up @@ -106,7 +108,7 @@ interface ExperimentDetailsState {
runListToolbarProps: ToolbarProps;
}

class ExperimentDetails extends Page<{}, ExperimentDetailsState> {
export class ExperimentDetails extends Page<{}, ExperimentDetailsState> {
private _runlistRef = React.createRef<RunList>();

constructor(props: any) {
Expand Down Expand Up @@ -343,4 +345,17 @@ class ExperimentDetails extends Page<{}, ExperimentDetailsState> {
}
}

export default ExperimentDetails;
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) {
return <Redirect to={RoutePage.EXPERIMENTS} />;
}

return <ExperimentDetails {...props} />;
};

export default EnhancedExperimentDetails;
96 changes: 66 additions & 30 deletions frontend/src/pages/ExperimentList.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@

import * as React from 'react';
import * as Utils from '../lib/Utils';
import { ExperimentList } from './ExperimentList';
import EnhancedExperimentList, { ExperimentList } from './ExperimentList';
import TestUtils from '../TestUtils';
import { ApiFilter, PredicateOp } from '../apis/filter';
import { RunStorageState } from '../apis/run';
Expand All @@ -28,6 +28,19 @@ import { ReactWrapper, ShallowWrapper, shallow } from 'enzyme';
import { RoutePage, QUERY_PARAMS } from '../components/Router';
import { range } from 'lodash';
import { ButtonKeys } from '../lib/Buttons';
import { NamespaceContext } from 'src/lib/KubeflowClient';
import { render } from '@testing-library/react';

// Default arguments for Apis.experimentServiceApi.listExperiment.
const LIST_EXPERIMENT_DEFAULTS = [
'', // page token
10, // page size
'created_at desc', // sort by
'', // filter
undefined, // resource_reference_key_type
undefined, // resource_reference_key_id
];
const LIST_EXPERIMENT_DEFAULTS_WITHOUT_RESOURCE_REFERENCE = LIST_EXPERIMENT_DEFAULTS.slice(0, 4);

describe('ExperimentList', () => {
let tree: ShallowWrapper | ReactWrapper;
Expand Down Expand Up @@ -82,7 +95,9 @@ describe('ExperimentList', () => {
afterEach(() => {
jest.resetAllMocks();
jest.clearAllMocks();
tree.unmount();
if (tree.exists()) {
tree.unmount();
}
});

it('renders an empty list with empty state message', () => {
Expand Down Expand Up @@ -140,14 +155,7 @@ describe('ExperimentList', () => {

it('calls Apis to list experiments, sorted by creation time in descending order', async () => {
await mountWithNExperiments(1, 1);
expect(listExperimentsSpy).toHaveBeenLastCalledWith(
'',
10,
'created_at desc',
'',
undefined,
undefined,
);
expect(listExperimentsSpy).toHaveBeenLastCalledWith(...LIST_EXPERIMENT_DEFAULTS);
expect(listRunsSpy).toHaveBeenLastCalledWith(
undefined,
5,
Expand Down Expand Up @@ -179,10 +187,7 @@ describe('ExperimentList', () => {
it('calls Apis to list experiments with namespace when available', async () => {
await mountWithNExperiments(1, 1, { namespace: 'test-ns' });
expect(listExperimentsSpy).toHaveBeenLastCalledWith(
'',
10,
'created_at desc',
'',
...LIST_EXPERIMENT_DEFAULTS_WITHOUT_RESOURCE_REFERENCE,
'NAMESPACE',
'test-ns',
);
Expand All @@ -196,14 +201,7 @@ describe('ExperimentList', () => {
expect(refreshBtn).toBeDefined();
await refreshBtn!.action();
expect(listExperimentsSpy.mock.calls.length).toBe(2);
expect(listExperimentsSpy).toHaveBeenLastCalledWith(
'',
10,
'created_at desc',
'',
undefined,
undefined,
);
expect(listExperimentsSpy).toHaveBeenLastCalledWith(...LIST_EXPERIMENT_DEFAULTS);
expect(updateBannerSpy).toHaveBeenLastCalledWith({});
});

Expand Down Expand Up @@ -248,14 +246,7 @@ describe('ExperimentList', () => {
TestUtils.makeErrorResponseOnce(listExperimentsSpy, 'bad stuff happened');
await refreshBtn!.action();
expect(listExperimentsSpy.mock.calls.length).toBe(2);
expect(listExperimentsSpy).toHaveBeenLastCalledWith(
'',
10,
'created_at desc',
'',
undefined,
undefined,
);
expect(listExperimentsSpy).toHaveBeenLastCalledWith(...LIST_EXPERIMENT_DEFAULTS);
expect(updateBannerSpy).toHaveBeenLastCalledWith(
expect.objectContaining({
additionalInfo: 'bad stuff happened',
Expand Down Expand Up @@ -438,4 +429,49 @@ describe('ExperimentList', () => {
}),
).toMatchSnapshot();
});

describe('EnhancedExperimentList', () => {
it('defaults to no namespace', () => {
render(<EnhancedExperimentList {...generateProps()} />);
expect(listExperimentsSpy).toHaveBeenLastCalledWith(...LIST_EXPERIMENT_DEFAULTS);
});

it('gets namespace from context', () => {
render(
<NamespaceContext.Provider value='test-ns'>
<EnhancedExperimentList {...generateProps()} />
</NamespaceContext.Provider>,
);
expect(listExperimentsSpy).toHaveBeenLastCalledWith(
...LIST_EXPERIMENT_DEFAULTS_WITHOUT_RESOURCE_REFERENCE,
'NAMESPACE',
'test-ns',
);
});

it('auto refreshes list when namespace changes', () => {
const { rerender } = render(
<NamespaceContext.Provider value='test-ns-1'>
<EnhancedExperimentList {...generateProps()} />
</NamespaceContext.Provider>,
);
expect(listExperimentsSpy).toHaveBeenCalledTimes(1);
expect(listExperimentsSpy).toHaveBeenLastCalledWith(
...LIST_EXPERIMENT_DEFAULTS_WITHOUT_RESOURCE_REFERENCE,
'NAMESPACE',
'test-ns-1',
);
rerender(
<NamespaceContext.Provider value='test-ns-2'>
<EnhancedExperimentList {...generateProps()} />
</NamespaceContext.Provider>,
);
expect(listExperimentsSpy).toHaveBeenCalledTimes(2);
expect(listExperimentsSpy).toHaveBeenLastCalledWith(
...LIST_EXPERIMENT_DEFAULTS_WITHOUT_RESOURCE_REFERENCE,
'NAMESPACE',
'test-ns-2',
);
});
});
});
2 changes: 1 addition & 1 deletion frontend/src/pages/ExperimentList.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,7 @@ export class ExperimentList extends Page<{ namespace?: string }, ExperimentListS

const EnhancedExperimentList: React.FC<PageProps> = props => {
const namespace = React.useContext(NamespaceContext);
return <ExperimentList {...props} namespace={namespace} />;
return <ExperimentList key={namespace} {...props} namespace={namespace} />;
};

export default EnhancedExperimentList;

0 comments on commit 7564166

Please sign in to comment.