Skip to content

Commit

Permalink
Fix: Routing bug on uninitialised plugin (#628)
Browse files Browse the repository at this point in the history
* feat: add Router to test suite customRender

* feat: add featureToggles option to customRender

* fix: import Router from react-router-dom

* fix: tidy up router and add routing tests

* feat: add useLayout effect to route back home

* feat: add 404 tests to routing

---------

Co-authored-by: Russ <8377044+rdubrock@users.noreply.github.com>
  • Loading branch information
ckbedwell and rdubrock authored Oct 17, 2023
1 parent ba03029 commit 0be20b5
Show file tree
Hide file tree
Showing 18 changed files with 311 additions and 175 deletions.
1 change: 1 addition & 0 deletions __mocks__/@grafana/runtime.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ export const getBackendSrv = () => ({
fetch: {
toPromise: () => jest.fn().mockResolvedValue({ ok: true }),
},
request: () => jest.fn().mockResolvedValue({ ok: true }),
});

export const getLocationSrv = () => ({
Expand Down
2 changes: 1 addition & 1 deletion src/components/ConfigActions.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ export const ConfigActions = ({ enabled, pluginId }: Props) => {
};

const handleSetup = () => {
navigate(ROUTES.Setup);
navigate(ROUTES.Home);
};

const getAction = () => {
Expand Down
4 changes: 1 addition & 3 deletions src/components/InstanceProvider.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -151,10 +151,8 @@ export const InstanceProvider = ({
throw new Error('There was an error finding datasources required for Synthetic Monitoring');
}

const provisioned = Boolean(meta.jsonData?.metrics?.grafanaName);

return (
<InstanceContext.Provider value={{ meta, instance: instances, loading: instancesLoading, provisioned }}>
<InstanceContext.Provider value={{ meta, instance: instances, loading: instancesLoading }}>
{children}
</InstanceContext.Provider>
);
Expand Down
84 changes: 84 additions & 0 deletions src/components/Routing.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
import React from 'react';
import { screen } from '@testing-library/react';

import { type CustomRenderOptions, createInstance, render } from 'test/render';
import { ROUTES } from 'types';
import { PLUGIN_URL_PATH } from './constants';
import { getRoute, Routing } from './Routing';

function renderRouting(options?: CustomRenderOptions) {
return render(<Routing onNavChanged={jest.fn} />, options);
}

const notaRoute = `${PLUGIN_URL_PATH}/404`;

describe('Only renders the unprovisioned setup page regardless of route when app is not provisioned', () => {
Object.entries(ROUTES).map(([key, route]) => {
test(`Route ${key}`, () => {
renderRouting({ path: getRoute(route), meta: { jsonData: undefined } });
screen.getByText('Provisioning is required for Synthetic Monitoring.');
});
});

test('Non-existent route (404)', () => {
renderRouting({ path: notaRoute, meta: { jsonData: undefined } });
screen.getByText('Provisioning is required for Synthetic Monitoring.');
});
});

describe('Only renders the welcome page regardless of route when app is not initializd', () => {
Object.entries(ROUTES).map(([key, route]) => {
test(`Route ${key}`, () => {
const instance = createInstance({ api: undefined });
renderRouting({ instance, path: getRoute(route) });
screen.getByText('Ready to start using synthetic monitoring?');
});
});

test('Non-existent route (404)', () => {
const instance = createInstance({ api: undefined });
renderRouting({ instance, path: notaRoute });
screen.getByText('Ready to start using synthetic monitoring?');
});
});

// Would like to have asserted on the h1s but rendering the Grafana pluginpage is tricky
describe('Routes to pages correctly', () => {
test('Home page renders', async () => {
renderRouting({ path: getRoute(ROUTES.Home) });
const homePageText = await screen.findByText('What you can do', { selector: 'h2' });
expect(homePageText).toBeInTheDocument();
});

test('Checks page renders', async () => {
renderRouting({ path: getRoute(ROUTES.Checks) });
const checksButton = await screen.findByText('Add new check');
expect(checksButton).toBeInTheDocument();
});

test('Probes page renders', async () => {
renderRouting({ path: getRoute(ROUTES.Probes) });
const probeReachabilityTexts = await screen.findAllByText('Reachability');
expect(probeReachabilityTexts.length).toBeGreaterThan(0);
});

test('Alert page renders', async () => {
renderRouting({ path: getRoute(ROUTES.Alerts) });
const alertsText = await screen.findByText('Learn more about alerting for Synthetic Monitoring.');
expect(alertsText).toBeInTheDocument();
});

test('Config page renders', async () => {
renderRouting({ path: getRoute(ROUTES.Config) });
const configText = await screen.findByText(
/Synthetic Monitoring is a blackbox monitoring solution provided as part of/i
);
expect(configText).toBeInTheDocument();
});

test('Non-existent route redirects to homepage', async () => {
renderRouting({ path: notaRoute });
const homePageText = await screen.findByText('What you can do', { selector: 'h2' });
expect(homePageText).toBeInTheDocument();
});
});
115 changes: 51 additions & 64 deletions src/components/Routing.tsx
Original file line number Diff line number Diff line change
@@ -1,58 +1,39 @@
import { CheckRouter } from 'page/CheckRouter';
import HomePage from 'page/HomePage';
import { ProbeRouter } from 'page/ProbeRouter';
import React, { useEffect, useContext } from 'react';
import React, { useLayoutEffect, useEffect, useContext } from 'react';
import { Redirect, Route, Switch, useLocation } from 'react-router-dom';
import { Alerting } from 'components/Alerting';
import { AppRootProps } from '@grafana/data';
import { getNavModel } from 'page/pageDefinitions';
import { config } from '@grafana/runtime';

import { PLUGIN_URL_PATH } from './constants';
import { InstanceContext } from 'contexts/InstanceContext';
import { WelcomePage } from 'page/WelcomePage';
import { UnprovisionedSetup } from './UnprovisionedSetup';
import { ROUTES } from 'types';
import { QueryParamMap, useNavigation } from 'hooks/useNavigation';
import { useQuery } from 'hooks/useQuery';
import { DashboardRedirecter } from './DashboardRedirecter';
import { ROUTES } from 'types';
import { config } from '@grafana/runtime';
import { PluginPage } from 'components/PluginPage';
import { AlertingPage } from 'page/AlertingPage';
import { CheckRouter } from 'page/CheckRouter';
import { ConfigPage } from 'page/ConfigPage';
import { DashboardPage } from 'page/DashboardPage';
import { ChecksContextProvider } from './ChecksContextProvider';
import { getNavModel } from 'page/pageDefinitions';
import { InstanceContext } from 'contexts/InstanceContext';
import { ProbeRouter } from 'page/ProbeRouter';
import { UnprovisionedSetup } from 'page/UnprovisionedSetup';
import { WelcomePage } from 'page/WelcomePage';
import { HomePage } from 'page/HomePage';
import { DashboardRedirecter } from './DashboardRedirecter';

export const Routing = ({ onNavChanged, meta, ...rest }: AppRootProps) => {
export const Routing = ({ onNavChanged }: Pick<AppRootProps, 'onNavChanged'>) => {
const queryParams = useQuery();
const navigate = useNavigation();
const location = useLocation();
const { instance, provisioned } = useContext(InstanceContext);
const initialized = meta.enabled && instance.api;
const { instance, meta } = useContext(InstanceContext);
const provisioned = Boolean(meta?.jsonData?.metrics?.grafanaName);
const initialized = meta?.enabled && instance.api;
const logo = meta?.info.logos.large || ``;

useEffect(() => {
const navModel = getNavModel(meta.info.logos.large, location.pathname);
const navModel = getNavModel(logo, location.pathname);
if (!config.featureToggles.topnav) {
onNavChanged(navModel);
}
}, [meta.info.logos.large, onNavChanged, location.pathname]);

useEffect(() => {
// not provisioned and not initialized
if (
meta.enabled &&
(!instance.metrics || !instance.logs) &&
!provisioned &&
!location.pathname.includes('unprovisioned')
) {
navigate(ROUTES.Unprovisioned);
}
// not provisioned and just initialized
if (meta.enabled && instance.metrics && instance.logs && location.pathname.includes('unprovisioned')) {
navigate(ROUTES.Home);
}
// Provisioned but not initialized
if (meta.enabled && !instance.api && provisioned && !location.pathname.includes('setup')) {
navigate(ROUTES.Setup);
}
}, [meta.enabled, instance.metrics, instance.logs, location.pathname, navigate, instance.api, provisioned]);
}, [logo, onNavChanged, location.pathname]);

const page = queryParams.get('page');
useEffect(() => {
Expand All @@ -66,46 +47,52 @@ export const Routing = ({ onNavChanged, meta, ...rest }: AppRootProps) => {
}
}, [page, navigate, queryParams]);

useLayoutEffect(() => {
if (!provisioned || (!initialized && location.pathname !== getRoute(ROUTES.Home))) {
navigate(ROUTES.Home);
}
}, [provisioned, initialized, location.pathname, navigate]);

if (!provisioned) {
return <UnprovisionedSetup />;
}

if (!initialized) {
return <WelcomePage />;
}

return (
<Switch>
<Route exact path={`${PLUGIN_URL_PATH}${ROUTES.Redirect}`}>
<Route exact path={getRoute(ROUTES.Redirect)}>
<DashboardRedirecter />
</Route>
<Route path={`${PLUGIN_URL_PATH}${ROUTES.Setup}`}>
{initialized ? <Redirect to={`${PLUGIN_URL_PATH}${ROUTES.Home}`} /> : <WelcomePage />}
</Route>
<Route path={`${PLUGIN_URL_PATH}${ROUTES.Unprovisioned}`}>
<UnprovisionedSetup />
</Route>
<Route exact path={`${PLUGIN_URL_PATH}${ROUTES.Home}`}>
<Route exact path={getRoute(ROUTES.Home)}>
<HomePage />
</Route>
<Route path={`${PLUGIN_URL_PATH}${ROUTES.Probes}`}>
<ProbeRouter />
</Route>
<Route exact path={`${PLUGIN_URL_PATH}${ROUTES.Alerts}`}>
<PluginPage>
<Alerting />
</PluginPage>
<Route path={getRoute(ROUTES.Scene)}>
<DashboardPage />
</Route>
<Route path={`${PLUGIN_URL_PATH}${ROUTES.Checks}`}>
<Route path={getRoute(ROUTES.Checks)}>
<CheckRouter />
</Route>
<Route path={`${PLUGIN_URL_PATH}${ROUTES.Scene}`}>
<ChecksContextProvider>
<DashboardPage />
</ChecksContextProvider>
<Route path={getRoute(ROUTES.Probes)}>
<ProbeRouter />
</Route>
<Route exact path={getRoute(ROUTES.Alerts)}>
<AlertingPage />
</Route>
<Route path={`${PLUGIN_URL_PATH}${ROUTES.Config}`}>
<PluginPage>
<ConfigPage />
</PluginPage>
<Route path={getRoute(ROUTES.Config)}>
<ConfigPage />
</Route>

{/* Default route (only redirect if the path matches the plugin's URL) */}
<Route path={PLUGIN_URL_PATH}>
<Redirect to={`${PLUGIN_URL_PATH}${ROUTES.Home}`} />
<Redirect to={getRoute(ROUTES.Home)} />
</Route>
</Switch>
);
};

export function getRoute(route: ROUTES) {
return `${PLUGIN_URL_PATH}${route}`;
}
2 changes: 0 additions & 2 deletions src/contexts/InstanceContext.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,10 @@ export interface InstanceContextValue {
loading: boolean;
instance: GrafanaInstances;
meta: AppPluginMeta<GlobalSettings> | undefined;
provisioned?: boolean;
}

export const InstanceContext = createContext<InstanceContextValue>({
instance: {},
loading: true,
meta: undefined,
provisioned: false,
});
32 changes: 19 additions & 13 deletions src/hooks/useNavigation.ts
Original file line number Diff line number Diff line change
@@ -1,24 +1,30 @@
import { useCallback } from 'react';
import { useHistory } from 'react-router-dom';
import { getLocationSrv } from '@grafana/runtime';

import { PLUGIN_URL_PATH } from 'components/constants';
import { useHistory } from 'react-router-dom';

export type QueryParamMap = {
[key: string]: string;
};

export function useNavigation() {
const history = useHistory();
const navigate = (url: string, queryParams?: QueryParamMap, external?: boolean, additionalState?: any) => {
const normalized = url.startsWith('/') ? url.slice(1) : url;
if (external) {
getLocationSrv().update({ partial: false, path: `/${normalized}`, query: queryParams });
} else {
const paramString = Object.entries(queryParams ?? {}).reduce(
(acc, [key, val]) => acc.concat(`&${key}=${val}`),
''
);
history.push(`${PLUGIN_URL_PATH}${normalized}${paramString ? '?' : ''}${paramString}`, additionalState);
}
};
const navigate = useCallback(
(url: string, queryParams?: QueryParamMap, external?: boolean, additionalState?: any) => {
const normalized = url.startsWith('/') ? url.slice(1) : url;
if (external) {
getLocationSrv().update({ partial: false, path: `/${normalized}`, query: queryParams });
} else {
const paramString = Object.entries(queryParams ?? {}).reduce(
(acc, [key, val]) => acc.concat(`&${key}=${val}`),
''
);
history.push(`${PLUGIN_URL_PATH}${normalized}${paramString ? '?' : ''}${paramString}`, additionalState);
}
},
[history]
);

return navigate;
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { screen, waitFor, within } from '@testing-library/react';
import { type UserEvent } from '@testing-library/user-event';

import { render } from 'test/render';
import { Alerting } from 'components/Alerting';
import { AlertingPage } from 'page/AlertingPage';
import {
ALERT_PROBE_SUCCESS_RECORDING_EXPR,
DEFAULT_ALERT_NAMES_BY_FAMILY_AND_SENSITIVITY,
Expand Down Expand Up @@ -38,7 +38,7 @@ const setDefaultRules = jest.fn();
const setRules = jest.fn().mockImplementation(() => Promise.resolve({ ok: true }));

const renderAlerting = ({ withAlerting = true } = {}) => {
return render(<Alerting />, {
return render(<AlertingPage />, {
instance: {
alertRuler: withAlerting ? ({ url: 'alertUrl' } as unknown as DataSourceSettings) : undefined,
},
Expand Down
26 changes: 18 additions & 8 deletions src/components/Alerting.tsx → src/page/AlertingPage.tsx
Original file line number Diff line number Diff line change
@@ -1,14 +1,16 @@
import { GrafanaTheme2 } from '@grafana/data';
import { Button, HorizontalGroup, Icon, Modal, Spinner, useStyles2, Alert } from '@grafana/ui';
import React, { FC, useState, useContext } from 'react';
import { css } from '@emotion/css';
import { useAlerts } from 'hooks/useAlerts';
import { AlertRuleForm } from './AlertRuleForm';
import { GrafanaTheme2 } from '@grafana/data';
import { config } from '@grafana/runtime';
import { Button, HorizontalGroup, Icon, Modal, Spinner, useStyles2, Alert } from '@grafana/ui';

import { AlertFormValues, AlertRule } from 'types';
import { InstanceContext } from 'contexts/InstanceContext';
import { transformAlertFormValues } from './alertingTransformations';
import { useAlerts } from 'hooks/useAlerts';
import useUnifiedAlertsEnabled from 'hooks/useUnifiedAlertsEnabled';
import { config } from '@grafana/runtime';
import { AlertRuleForm } from 'components/AlertRuleForm';
import { InstanceContext } from 'contexts/InstanceContext';
import { transformAlertFormValues } from 'components/alertingTransformations';
import { PluginPage } from 'components/PluginPage';

type SplitAlertRules = {
recordingRules: AlertRule[];
Expand Down Expand Up @@ -36,7 +38,15 @@ const getStyles = (theme: GrafanaTheme2) => ({
`,
});

export const Alerting: FC = () => {
export const AlertingPage = () => {
return (
<PluginPage>
<Alerting />
</PluginPage>
);
};

const Alerting: FC = () => {
const styles = useStyles2(getStyles);
const { alertRules, setDefaultRules, setRules, alertError } = useAlerts();
const [updatingDefaultRules, setUpdatingDefaultRules] = useState(false);
Expand Down
Loading

0 comments on commit 0be20b5

Please sign in to comment.