From 694600ed0472fb0c5acb1ffb408e36e1e2d091fa Mon Sep 17 00:00:00 2001 From: Jack Westbrook Date: Tue, 23 Nov 2021 15:38:08 +0100 Subject: [PATCH] Catalog: Provide default tab fallback in Plugin Details page (#41979) * feat(catalog): introduce defaultTab to usePluginDetailsTabs hook * feat(catalog): use defaultTab as fallback tab for PluginDetails * chore(catalog): remove hardcoded page query param in list items * refactor(catalog): prefer let over react ref when setting default tab in PluginDetails * refactor(catalog): pass pageId to plugin details body rather than duplicate logic * test(catalog): remove query param from List item test hrefs * test(catalog): introduce a test for default app config page for installed app plugins --- .../admin/components/PluginDetailsBody.tsx | 4 +- .../admin/components/PluginListItem.test.tsx | 4 +- .../admin/components/PluginListItem.tsx | 7 +--- .../admin/hooks/usePluginDetailsTabs.tsx | 19 +++++++-- .../admin/pages/PluginDetails.test.tsx | 42 +++++++++++++++++-- .../plugins/admin/pages/PluginDetails.tsx | 6 +-- 6 files changed, 63 insertions(+), 19 deletions(-) diff --git a/public/app/features/plugins/admin/components/PluginDetailsBody.tsx b/public/app/features/plugins/admin/components/PluginDetailsBody.tsx index e925b8afc97..5608e976b42 100644 --- a/public/app/features/plugins/admin/components/PluginDetailsBody.tsx +++ b/public/app/features/plugins/admin/components/PluginDetailsBody.tsx @@ -13,12 +13,12 @@ import { PluginDashboards } from './PluginDashboards'; type Props = { plugin: CatalogPlugin; queryParams: UrlQueryMap; + pageId: string; }; -export function PluginDetailsBody({ plugin, queryParams }: Props): JSX.Element { +export function PluginDetailsBody({ plugin, queryParams, pageId }: Props): JSX.Element { const styles = useStyles2(getStyles); const { value: pluginConfig } = usePluginConfig(plugin); - const pageId = queryParams.page; if (pageId === PluginTabIds.OVERVIEW) { return ( diff --git a/public/app/features/plugins/admin/components/PluginListItem.test.tsx b/public/app/features/plugins/admin/components/PluginListItem.test.tsx index 658cc047ade..fb2ed8558b0 100644 --- a/public/app/features/plugins/admin/components/PluginListItem.test.tsx +++ b/public/app/features/plugins/admin/components/PluginListItem.test.tsx @@ -59,7 +59,7 @@ describe('PluginListItem', () => { it('renders a card with link, image, name, orgName and badges', () => { render(); - expect(screen.getByRole('link')).toHaveAttribute('href', '/plugins/test-plugin?page=overview'); + expect(screen.getByRole('link')).toHaveAttribute('href', '/plugins/test-plugin'); const logo = screen.getByRole('img'); expect(logo).toHaveAttribute('src', plugin.info.logos.small); @@ -102,7 +102,7 @@ describe('PluginListItem', () => { it('renders a row with link, image, name, orgName and badges', () => { render(); - expect(screen.getByRole('link')).toHaveAttribute('href', '/plugins/test-plugin?page=overview'); + expect(screen.getByRole('link')).toHaveAttribute('href', '/plugins/test-plugin'); const logo = screen.getByRole('img'); expect(logo).toHaveAttribute('src', plugin.info.logos.small); diff --git a/public/app/features/plugins/admin/components/PluginListItem.tsx b/public/app/features/plugins/admin/components/PluginListItem.tsx index 0928d689b66..772f5f9a0b8 100644 --- a/public/app/features/plugins/admin/components/PluginListItem.tsx +++ b/public/app/features/plugins/admin/components/PluginListItem.tsx @@ -1,7 +1,7 @@ import React from 'react'; import { css, cx } from '@emotion/css'; import { GrafanaTheme2 } from '@grafana/data'; -import { CatalogPlugin, PluginIconName, PluginListDisplayMode, PluginTabIds } from '../types'; +import { CatalogPlugin, PluginIconName, PluginListDisplayMode } from '../types'; import { PluginListItemBadges } from './PluginListItemBadges'; import { PluginLogo } from './PluginLogo'; import { Icon, useStyles2 } from '@grafana/ui'; @@ -19,10 +19,7 @@ export function PluginListItem({ plugin, pathName, displayMode = PluginListDispl const isList = displayMode === PluginListDisplayMode.List; return ( - +

{plugin.name}

diff --git a/public/app/features/plugins/admin/hooks/usePluginDetailsTabs.tsx b/public/app/features/plugins/admin/hooks/usePluginDetailsTabs.tsx index ffcb3059789..6a045fca1a4 100644 --- a/public/app/features/plugins/admin/hooks/usePluginDetailsTabs.tsx +++ b/public/app/features/plugins/admin/hooks/usePluginDetailsTabs.tsx @@ -9,15 +9,18 @@ type ReturnType = { error: Error | undefined; loading: boolean; tabs: PluginDetailsTab[]; + defaultTab: string; }; export const usePluginDetailsTabs = (plugin?: CatalogPlugin, defaultTabs: PluginDetailsTab[] = []): ReturnType => { const { loading, error, value: pluginConfig } = usePluginConfig(plugin); const isPublished = Boolean(plugin?.isPublished); const { pathname } = useLocation(); - const tabs = useMemo(() => { + + const [tabs, defaultTab] = useMemo(() => { const canConfigurePlugins = isOrgAdmin(); const tabs: PluginDetailsTab[] = [...defaultTabs]; + let defaultTab; if (isPublished) { tabs.push({ @@ -30,7 +33,8 @@ export const usePluginDetailsTabs = (plugin?: CatalogPlugin, defaultTabs: Plugin // Not extending the tabs with the config pages if the plugin is not installed if (!pluginConfig) { - return tabs; + defaultTab = PluginTabIds.OVERVIEW; + return [tabs, defaultTab]; } if (canConfigurePlugins) { @@ -42,6 +46,7 @@ export const usePluginDetailsTabs = (plugin?: CatalogPlugin, defaultTabs: Plugin id: PluginTabIds.CONFIG, href: `${pathname}?page=${PluginTabIds.CONFIG}`, }); + defaultTab = PluginTabIds.CONFIG; } if (pluginConfig.configPages) { @@ -52,6 +57,9 @@ export const usePluginDetailsTabs = (plugin?: CatalogPlugin, defaultTabs: Plugin id: page.id, href: `${pathname}?page=${page.id}`, }); + if (!defaultTab) { + defaultTab = page.id; + } } } @@ -66,12 +74,17 @@ export const usePluginDetailsTabs = (plugin?: CatalogPlugin, defaultTabs: Plugin } } - return tabs; + if (!defaultTab) { + defaultTab = PluginTabIds.OVERVIEW; + } + + return [tabs, defaultTab]; }, [pluginConfig, defaultTabs, pathname, isPublished]); return { error, loading, tabs, + defaultTab, }; }; diff --git a/public/app/features/plugins/admin/pages/PluginDetails.test.tsx b/public/app/features/plugins/admin/pages/PluginDetails.test.tsx index e9fe5e85f4d..2057a28ca97 100644 --- a/public/app/features/plugins/admin/pages/PluginDetails.test.tsx +++ b/public/app/features/plugins/admin/pages/PluginDetails.test.tsx @@ -40,7 +40,7 @@ jest.mock('../hooks/usePluginConfig.tsx', () => ({ const renderPluginDetails = ( pluginOverride: Partial, { - pageId = PluginTabIds.OVERVIEW, + pageId, pluginsStateOverride, }: { pageId?: PluginTabIds; @@ -55,7 +55,7 @@ const renderPluginDetails = ( location: { hash: '', pathname: `/plugins/${id}`, - search: `?page=${pageId}`, + search: pageId ? `?page=${pageId}` : '', state: undefined, }, }); @@ -118,11 +118,11 @@ describe('Plugin details page', () => { const props = getRouteComponentProps({ match: { params: { pluginId: id }, isExact: true, url: '', path: '' }, - queryParams: { page: PluginTabIds.OVERVIEW }, + queryParams: {}, location: { hash: '', pathname: `/plugins/${id}`, - search: `?page=${PluginTabIds.OVERVIEW}`, + search: '', state: undefined, }, }); @@ -145,6 +145,40 @@ describe('Plugin details page', () => { await waitFor(() => expect(queryByText(/licensed under the apache 2.0 license/i)).toBeInTheDocument()); }); + it('should display an app config page by default for installed app plugins', async () => { + const name = 'Akumuli'; + + // @ts-ignore + usePluginConfig.mockReturnValue({ + value: { + meta: { + type: PluginType.app, + enabled: false, + pinned: false, + jsonData: {}, + }, + configPages: [ + { + title: 'Config', + icon: 'cog', + id: 'configPage', + body: function ConfigPage() { + return
Custom Config Page!
; + }, + }, + ], + }, + }); + + const { queryByText } = renderPluginDetails({ + name, + isInstalled: true, + type: PluginType.app, + }); + + await waitFor(() => expect(queryByText(/custom config page/i)).toBeInTheDocument()); + }); + it('should display the number of downloads in the header', async () => { // depending on what locale you have the Intl.NumberFormat will return a format that contains // whitespaces. In that case we don't want testing library to remove whitespaces. diff --git a/public/app/features/plugins/admin/pages/PluginDetails.tsx b/public/app/features/plugins/admin/pages/PluginDetails.tsx index e8bd18aee35..8674ca21f0d 100644 --- a/public/app/features/plugins/admin/pages/PluginDetails.tsx +++ b/public/app/features/plugins/admin/pages/PluginDetails.tsx @@ -25,7 +25,6 @@ export default function PluginDetails({ match, queryParams }: Props): JSX.Elemen params: { pluginId = '' }, url, } = match; - const pageId = (queryParams.page as PluginTabIds) || PluginTabIds.OVERVIEW; const parentUrl = url.substring(0, url.lastIndexOf('/')); const defaultTabs: PluginDetailsTab[] = [ { @@ -36,11 +35,12 @@ export default function PluginDetails({ match, queryParams }: Props): JSX.Elemen }, ]; const plugin = useGetSingle(pluginId); // fetches the localplugin settings - const { tabs } = usePluginDetailsTabs(plugin, defaultTabs); + const { tabs, defaultTab } = usePluginDetailsTabs(plugin, defaultTabs); const { isLoading: isFetchLoading } = useFetchStatus(); const { isLoading: isFetchDetailsLoading } = useFetchDetailsStatus(); const styles = useStyles2(getStyles); const prevTabs = usePrevious(tabs); + const pageId = (queryParams.page as PluginTabIds) || defaultTab; // If an app plugin is uninstalled we need to reset the active tab when the config / dashboards tabs are removed. useEffect(() => { @@ -95,7 +95,7 @@ export default function PluginDetails({ match, queryParams }: Props): JSX.Elemen - +