Skip to content

Commit

Permalink
Show error page when accessing unavailable app (#54656) (#55317)
Browse files Browse the repository at this point in the history
* display not found page instead of throwing an error when accessible unavailable app

* move types to public folder

* fix types import

* remove updater from start app

* remove unnecessary await
  • Loading branch information
pgayvallet authored Jan 20, 2020
1 parent 285a201 commit ac386b3
Show file tree
Hide file tree
Showing 12 changed files with 292 additions and 112 deletions.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 1 addition & 11 deletions src/core/public/application/application_service.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -525,17 +525,7 @@ describe('#start()', () => {
const { getComponent } = await service.start(startDeps);

expect(() => shallow(createElement(getComponent))).not.toThrow();
expect(getComponent()).toMatchInlineSnapshot(`
<AppRouter
history={
Object {
"push": [MockFunction],
}
}
mounters={Map {}}
setAppLeaveHandler={[Function]}
/>
`);
expect(getComponent()).toMatchSnapshot();
});

it('renders null when in legacy mode', async () => {
Expand Down
13 changes: 7 additions & 6 deletions src/core/public/application/application_service.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@

import React from 'react';
import { BehaviorSubject, Observable, Subject, Subscription } from 'rxjs';
import { map, takeUntil } from 'rxjs/operators';
import { map, shareReplay, takeUntil } from 'rxjs/operators';
import { createBrowserHistory, History } from 'history';

import { InjectedMetadataSetup } from '../injected_metadata';
Expand Down Expand Up @@ -256,6 +256,11 @@ export class ApplicationService {
)
.subscribe(apps => applications$.next(apps));

const applicationStatuses$ = applications$.pipe(
map(apps => new Map([...apps.entries()].map(([id, app]) => [id, app.status!]))),
shareReplay(1)
);

return {
applications$,
capabilities,
Expand All @@ -264,11 +269,6 @@ export class ApplicationService {
getUrlForApp: (appId, { path }: { path?: string } = {}) =>
getAppUrl(availableMounters, appId, path),
navigateToApp: async (appId, { path, state }: { path?: string; state?: any } = {}) => {
const app = applications$.value.get(appId);
if (app && app.status !== AppStatus.accessible) {
// should probably redirect to the error page instead
throw new Error(`Trying to navigate to an inaccessible application: ${appId}`);
}
if (await this.shouldNavigate(overlays)) {
this.appLeaveHandlers.delete(this.currentAppId$.value!);
this.navigate!(getAppUrl(availableMounters, appId, path), state);
Expand All @@ -283,6 +283,7 @@ export class ApplicationService {
<AppRouter
history={this.history}
mounters={availableMounters}
appStatuses$={applicationStatuses$}
setAppLeaveHandler={this.setAppLeaveHandler}
/>
);
Expand Down
33 changes: 33 additions & 0 deletions src/core/public/application/integration_tests/router.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,18 @@
*/

import React from 'react';
import { BehaviorSubject } from 'rxjs';
import { createMemoryHistory, History, createHashHistory } from 'history';

import { AppRouter, AppNotFound } from '../ui';
import { EitherApp, MockedMounterMap, MockedMounterTuple } from '../test_types';
import { createRenderer, createAppMounter, createLegacyAppMounter } from './utils';
import { AppStatus } from '../types';

describe('AppContainer', () => {
let mounters: MockedMounterMap<EitherApp>;
let history: History;
let appStatuses$: BehaviorSubject<Map<string, AppStatus>>;
let update: ReturnType<typeof createRenderer>;

const navigate = (path: string) => {
Expand All @@ -38,19 +41,34 @@ describe('AppContainer', () => {
new Map([...mounters].map(([appId, { mounter }]) => [appId, mounter]));
const setAppLeaveHandlerMock = () => undefined;

const mountersToAppStatus$ = () => {
return new BehaviorSubject(
new Map(
[...mounters.keys()].map(id => [
id,
id.startsWith('disabled') ? AppStatus.inaccessible : AppStatus.accessible,
])
)
);
};

beforeEach(() => {
mounters = new Map([
createAppMounter('app1', '<span>App 1</span>'),
createLegacyAppMounter('legacyApp1', jest.fn()),
createAppMounter('app2', '<div>App 2</div>'),
createLegacyAppMounter('baseApp:legacyApp2', jest.fn()),
createAppMounter('app3', '<div>App 3</div>', '/custom/path'),
createAppMounter('disabledApp', '<div>Disabled app</div>'),
createLegacyAppMounter('disabledLegacyApp', jest.fn()),
] as Array<MockedMounterTuple<EitherApp>>);
history = createMemoryHistory();
appStatuses$ = mountersToAppStatus$();
update = createRenderer(
<AppRouter
history={history}
mounters={mockMountersToMounters()}
appStatuses$={appStatuses$}
setAppLeaveHandler={setAppLeaveHandlerMock}
/>
);
Expand Down Expand Up @@ -89,6 +107,7 @@ describe('AppContainer', () => {
<AppRouter
history={history}
mounters={mockMountersToMounters()}
appStatuses$={mountersToAppStatus$()}
setAppLeaveHandler={setAppLeaveHandlerMock}
/>
);
Expand All @@ -107,6 +126,7 @@ describe('AppContainer', () => {
<AppRouter
history={history}
mounters={mockMountersToMounters()}
appStatuses$={mountersToAppStatus$()}
setAppLeaveHandler={setAppLeaveHandlerMock}
/>
);
Expand Down Expand Up @@ -147,6 +167,7 @@ describe('AppContainer', () => {
<AppRouter
history={history}
mounters={mockMountersToMounters()}
appStatuses$={mountersToAppStatus$()}
setAppLeaveHandler={setAppLeaveHandlerMock}
/>
);
Expand Down Expand Up @@ -202,4 +223,16 @@ describe('AppContainer', () => {

expect(dom?.exists(AppNotFound)).toBe(true);
});

it('displays error page if app is inaccessible', async () => {
const dom = await navigate('/app/disabledApp');

expect(dom?.exists(AppNotFound)).toBe(true);
});

it('displays error page if legacy app is inaccessible', async () => {
const dom = await navigate('/app/disabledLegacyApp');

expect(dom?.exists(AppNotFound)).toBe(true);
});
});
9 changes: 6 additions & 3 deletions src/core/public/application/ui/app_container.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -26,23 +26,26 @@ import React, {
MutableRefObject,
} from 'react';

import { AppUnmount, Mounter, AppLeaveHandler } from '../types';
import { AppLeaveHandler, AppStatus, AppUnmount, Mounter } from '../types';
import { AppNotFound } from './app_not_found_screen';

interface Props {
appId: string;
mounter?: Mounter;
appStatus: AppStatus;
setAppLeaveHandler: (appId: string, handler: AppLeaveHandler) => void;
}

export const AppContainer: FunctionComponent<Props> = ({
mounter,
appId,
setAppLeaveHandler,
appStatus,
}: Props) => {
const [appNotFound, setAppNotFound] = useState(false);
const elementRef = useRef<HTMLDivElement>(null);
const unmountRef: MutableRefObject<AppUnmount | null> = useRef<AppUnmount>(null);
// const appStatus = useObservable(appStatus$);

useLayoutEffect(() => {
const unmount = () => {
Expand All @@ -52,7 +55,7 @@ export const AppContainer: FunctionComponent<Props> = ({
}
};
const mount = async () => {
if (!mounter) {
if (!mounter || appStatus !== AppStatus.accessible) {
return setAppNotFound(true);
}

Expand All @@ -71,7 +74,7 @@ export const AppContainer: FunctionComponent<Props> = ({

mount();
return unmount;
}, [appId, mounter, setAppLeaveHandler]);
}, [appId, appStatus, mounter, setAppLeaveHandler]);

return (
<Fragment>
Expand Down
2 changes: 1 addition & 1 deletion src/core/public/application/ui/app_not_found_screen.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import React from 'react';
import { FormattedMessage } from '@kbn/i18n/react';

export const AppNotFound = () => (
<EuiPage style={{ minHeight: '100%' }}>
<EuiPage style={{ minHeight: '100%' }} data-test-subj="appNotFoundPageContent">
<EuiPageBody>
<EuiPageContent verticalPosition="center" horizontalPosition="center">
<EuiEmptyPrompt
Expand Down
Loading

0 comments on commit ac386b3

Please sign in to comment.