From 2fac3bd41e538335b61e0c9603e50d9675f23609 Mon Sep 17 00:00:00 2001 From: Levente Balogh Date: Tue, 12 Sep 2023 12:49:10 +0200 Subject: [PATCH] Plugins: Show deprecated plugins (#74598) * feat: add a `isDeprecated` field to `CatalogPlugin` * tests: update the tests for merging local & remote * feat: display a deprecated badge in the plugins list * feat: show a deprecated warning if the plugin is deprecated * Fix linting issues * Review notes Co-authored-by: Marcus Andersson * refactor: remove `isDeprecated` from the details (it's already in the main CatalogPlugin object) * refactor: use an enum for remote statuses --------- Co-authored-by: Marcus Andersson --- .../admin/__mocks__/catalogPlugin.mock.ts | 1 + public/app/features/plugins/admin/api.ts | 12 ++- .../Badges/PluginDeprecatedBadge.tsx | 14 ++++ .../plugins/admin/components/Badges/index.ts | 1 + .../InstallControlsButton.test.tsx | 1 + .../PluginDetailsDeprecatedWarning.tsx | 25 ++++++ .../admin/components/PluginDetailsPage.tsx | 3 + .../admin/components/PluginList.test.tsx | 1 + .../admin/components/PluginListItem.test.tsx | 1 + .../components/PluginListItemBadges.test.tsx | 1 + .../admin/components/PluginListItemBadges.tsx | 2 + .../features/plugins/admin/helpers.test.ts | 84 +++++++++++++++---- public/app/features/plugins/admin/helpers.ts | 36 ++++---- .../admin/pages/PluginDetails.test.tsx | 24 ++++++ public/app/features/plugins/admin/types.ts | 13 ++- 15 files changed, 185 insertions(+), 34 deletions(-) create mode 100644 public/app/features/plugins/admin/components/Badges/PluginDeprecatedBadge.tsx create mode 100644 public/app/features/plugins/admin/components/PluginDetailsDeprecatedWarning.tsx diff --git a/public/app/features/plugins/admin/__mocks__/catalogPlugin.mock.ts b/public/app/features/plugins/admin/__mocks__/catalogPlugin.mock.ts index 112ebbc09cc..e4c8ffb510f 100644 --- a/public/app/features/plugins/admin/__mocks__/catalogPlugin.mock.ts +++ b/public/app/features/plugins/admin/__mocks__/catalogPlugin.mock.ts @@ -17,6 +17,7 @@ export default { isEnterprise: false, isInstalled: false, isDisabled: false, + isDeprecated: false, isPublished: true, name: 'Zabbix', orgName: 'Alexander Zobnin', diff --git a/public/app/features/plugins/admin/api.ts b/public/app/features/plugins/admin/api.ts index 73768f9753b..abe86d3b8f7 100644 --- a/public/app/features/plugins/admin/api.ts +++ b/public/app/features/plugins/admin/api.ts @@ -3,7 +3,7 @@ import { getBackendSrv, isFetchError } from '@grafana/runtime'; import { accessControlQueryParam } from 'app/core/utils/accessControl'; import { API_ROOT, GCOM_API_ROOT } from './constants'; -import { isLocalPluginVisible, isRemotePluginVisible } from './helpers'; +import { isLocalPluginVisibleByConfig, isRemotePluginVisibleByConfig } from './helpers'; import { LocalPlugin, RemotePlugin, CatalogPluginDetails, Version, PluginVersion } from './types'; export async function getPluginDetails(id: string): Promise { @@ -29,9 +29,13 @@ export async function getPluginDetails(id: string): Promise { - const { items: remotePlugins }: { items: RemotePlugin[] } = await getBackendSrv().get(`${GCOM_API_ROOT}/plugins`); + // We are also fetching deprecated plugins, because we would like to be able to label plugins in the list that are both installed and deprecated. + // (We won't show not installed deprecated plugins in the list) + const { items: remotePlugins }: { items: RemotePlugin[] } = await getBackendSrv().get(`${GCOM_API_ROOT}/plugins`, { + includeDeprecated: true, + }); - return remotePlugins.filter(isRemotePluginVisible); + return remotePlugins.filter(isRemotePluginVisibleByConfig); } export async function getPluginErrors(): Promise { @@ -97,7 +101,7 @@ export async function getLocalPlugins(): Promise { accessControlQueryParam({ embedded: 0 }) ); - return localPlugins.filter(isLocalPluginVisible); + return localPlugins.filter(isLocalPluginVisibleByConfig); } export async function installPlugin(id: string) { diff --git a/public/app/features/plugins/admin/components/Badges/PluginDeprecatedBadge.tsx b/public/app/features/plugins/admin/components/Badges/PluginDeprecatedBadge.tsx new file mode 100644 index 00000000000..bd24780b5d4 --- /dev/null +++ b/public/app/features/plugins/admin/components/Badges/PluginDeprecatedBadge.tsx @@ -0,0 +1,14 @@ +import React from 'react'; + +import { Badge } from '@grafana/ui'; + +export function PluginDeprecatedBadge(): React.ReactElement { + return ( + + ); +} diff --git a/public/app/features/plugins/admin/components/Badges/index.ts b/public/app/features/plugins/admin/components/Badges/index.ts index a93033c0591..9f4f6c0a9c8 100644 --- a/public/app/features/plugins/admin/components/Badges/index.ts +++ b/public/app/features/plugins/admin/components/Badges/index.ts @@ -3,3 +3,4 @@ export { PluginInstalledBadge } from './PluginInstallBadge'; export { PluginEnterpriseBadge } from './PluginEnterpriseBadge'; export { PluginUpdateAvailableBadge } from './PluginUpdateAvailableBadge'; export { PluginAngularBadge } from './PluginAngularBadge'; +export { PluginDeprecatedBadge } from './PluginDeprecatedBadge'; diff --git a/public/app/features/plugins/admin/components/InstallControls/InstallControlsButton.test.tsx b/public/app/features/plugins/admin/components/InstallControls/InstallControlsButton.test.tsx index be65618a497..d06268248d7 100644 --- a/public/app/features/plugins/admin/components/InstallControls/InstallControlsButton.test.tsx +++ b/public/app/features/plugins/admin/components/InstallControls/InstallControlsButton.test.tsx @@ -28,6 +28,7 @@ const plugin: CatalogPlugin = { isDev: false, isEnterprise: false, isDisabled: false, + isDeprecated: false, isPublished: true, }; diff --git a/public/app/features/plugins/admin/components/PluginDetailsDeprecatedWarning.tsx b/public/app/features/plugins/admin/components/PluginDetailsDeprecatedWarning.tsx new file mode 100644 index 00000000000..8e05af174db --- /dev/null +++ b/public/app/features/plugins/admin/components/PluginDetailsDeprecatedWarning.tsx @@ -0,0 +1,25 @@ +import React, { useState } from 'react'; + +import { Alert } from '@grafana/ui'; + +import { CatalogPlugin } from '../types'; + +type Props = { + className?: string; + plugin: CatalogPlugin; +}; + +export function PluginDetailsDeprecatedWarning(props: Props): React.ReactElement | null { + const { className, plugin } = props; + const [dismissed, setDismissed] = useState(false); + const isWarningVisible = plugin.isDeprecated && !dismissed; + + return isWarningVisible ? ( + setDismissed(true)}> +

+ This {plugin.type} plugin is deprecated and removed from the catalog. No further updates will be made to the + plugin. +

+ + ) : null; +} diff --git a/public/app/features/plugins/admin/components/PluginDetailsPage.tsx b/public/app/features/plugins/admin/components/PluginDetailsPage.tsx index 146e12365f5..39f9b50659e 100644 --- a/public/app/features/plugins/admin/components/PluginDetailsPage.tsx +++ b/public/app/features/plugins/admin/components/PluginDetailsPage.tsx @@ -19,6 +19,8 @@ import { usePluginPageExtensions } from '../hooks/usePluginPageExtensions'; import { useGetSingle, useFetchStatus, useFetchDetailsStatus } from '../state/hooks'; import { PluginTabIds } from '../types'; +import { PluginDetailsDeprecatedWarning } from './PluginDetailsDeprecatedWarning'; + export type Props = { // The ID of the plugin pluginId: string; @@ -87,6 +89,7 @@ export function PluginDetailsPage({ )} + diff --git a/public/app/features/plugins/admin/components/PluginList.test.tsx b/public/app/features/plugins/admin/components/PluginList.test.tsx index 86c30ea2735..45f153a71a4 100644 --- a/public/app/features/plugins/admin/components/PluginList.test.tsx +++ b/public/app/features/plugins/admin/components/PluginList.test.tsx @@ -45,6 +45,7 @@ const getMockPlugin = (id: string): CatalogPlugin => { isDev: false, isEnterprise: false, isDisabled: false, + isDeprecated: false, isPublished: true, }; }; diff --git a/public/app/features/plugins/admin/components/PluginListItem.test.tsx b/public/app/features/plugins/admin/components/PluginListItem.test.tsx index 856709f4589..dbb5bbb50a8 100644 --- a/public/app/features/plugins/admin/components/PluginListItem.test.tsx +++ b/public/app/features/plugins/admin/components/PluginListItem.test.tsx @@ -55,6 +55,7 @@ describe('PluginListItem', () => { isDev: false, isEnterprise: false, isDisabled: false, + isDeprecated: false, isPublished: true, }; diff --git a/public/app/features/plugins/admin/components/PluginListItemBadges.test.tsx b/public/app/features/plugins/admin/components/PluginListItemBadges.test.tsx index 105ca09c50f..e6906d72874 100644 --- a/public/app/features/plugins/admin/components/PluginListItemBadges.test.tsx +++ b/public/app/features/plugins/admin/components/PluginListItemBadges.test.tsx @@ -31,6 +31,7 @@ describe('PluginListItemBadges', () => { isDev: false, isEnterprise: false, isDisabled: false, + isDeprecated: false, isPublished: true, }; diff --git a/public/app/features/plugins/admin/components/PluginListItemBadges.tsx b/public/app/features/plugins/admin/components/PluginListItemBadges.tsx index 8cd581f9ae9..1dcbbf99e07 100644 --- a/public/app/features/plugins/admin/components/PluginListItemBadges.tsx +++ b/public/app/features/plugins/admin/components/PluginListItemBadges.tsx @@ -11,6 +11,7 @@ import { PluginInstalledBadge, PluginUpdateAvailableBadge, PluginAngularBadge, + PluginDeprecatedBadge, } from './Badges'; type PluginBadgeType = { @@ -35,6 +36,7 @@ export function PluginListItemBadges({ plugin }: PluginBadgeType) { {plugin.isDisabled && } + {plugin.isDeprecated && } {plugin.isInstalled && } {hasUpdate && } {plugin.angularDetected && } diff --git a/public/app/features/plugins/admin/helpers.test.ts b/public/app/features/plugins/admin/helpers.test.ts index d3d3596bfa1..c2f36fd2754 100644 --- a/public/app/features/plugins/admin/helpers.test.ts +++ b/public/app/features/plugins/admin/helpers.test.ts @@ -10,10 +10,10 @@ import { mergeLocalsAndRemotes, sortPlugins, Sorters, - isLocalPluginVisible, - isRemotePluginVisible, + isLocalPluginVisibleByConfig, + isRemotePluginVisibleByConfig, } from './helpers'; -import { RemotePlugin, LocalPlugin } from './types'; +import { RemotePlugin, LocalPlugin, RemotePluginStatus } from './types'; describe('Plugins/Helpers', () => { let remotePlugin: RemotePlugin; @@ -65,6 +65,29 @@ describe('Plugins/Helpers', () => { // Only remote expect(findMerged('plugin-4')).toEqual(mergeLocalAndRemote(undefined, getRemotePluginMock({ slug: 'plugin-4' }))); }); + + test('skips deprecated plugins unless they have a local - installed - counterpart', () => { + const merged = mergeLocalsAndRemotes(localPlugins, [ + ...remotePlugins, + getRemotePluginMock({ slug: 'plugin-5', status: RemotePluginStatus.Deprecated }), + ]); + const findMerged = (mergedId: string) => merged.find(({ id }) => id === mergedId); + + expect(merged).toHaveLength(4); + expect(findMerged('plugin-5')).toBeUndefined(); + }); + + test('keeps deprecated plugins in case they have a local counterpart', () => { + const merged = mergeLocalsAndRemotes( + [...localPlugins, getLocalPluginMock({ id: 'plugin-5' })], + [...remotePlugins, getRemotePluginMock({ slug: 'plugin-5', status: RemotePluginStatus.Deprecated })] + ); + const findMerged = (mergedId: string) => merged.find(({ id }) => id === mergedId); + + expect(merged).toHaveLength(5); + expect(findMerged('plugin-5')).not.toBeUndefined(); + expect(findMerged('plugin-5')?.isDeprecated).toBe(true); + }); }); describe('mergeLocalAndRemote()', () => { @@ -100,6 +123,7 @@ describe('Plugins/Helpers', () => { isDisabled: false, isEnterprise: false, isInstalled: false, + isDeprecated: false, isPublished: true, name: 'Zabbix', orgName: 'Alexander Zobnin', @@ -139,8 +163,8 @@ describe('Plugins/Helpers', () => { }); test('adds an "isEnterprise" field', () => { - const enterprisePlugin = { ...remotePlugin, status: 'enterprise' } as RemotePlugin; - const notEnterprisePlugin = { ...remotePlugin, status: 'unknown' } as RemotePlugin; + const enterprisePlugin = { ...remotePlugin, status: RemotePluginStatus.Enterprise } as RemotePlugin; + const notEnterprisePlugin = { ...remotePlugin, status: RemotePluginStatus.Active } as RemotePlugin; expect(mapRemoteToCatalog(enterprisePlugin).isEnterprise).toBe(true); expect(mapRemoteToCatalog(notEnterprisePlugin).isEnterprise).toBe(false); @@ -175,6 +199,7 @@ describe('Plugins/Helpers', () => { isEnterprise: false, isInstalled: true, isPublished: false, + isDeprecated: false, name: 'Zabbix', orgName: 'Alexander Zobnin', popularity: 0, @@ -223,6 +248,7 @@ describe('Plugins/Helpers', () => { isEnterprise: false, isInstalled: true, isPublished: true, + isDeprecated: false, name: 'Zabbix', orgName: 'Alexander Zobnin', popularity: 0.2111, @@ -319,15 +345,17 @@ describe('Plugins/Helpers', () => { test('`.isEnterprise` - prefers the remote', () => { // Local & Remote - expect(mapToCatalogPlugin(localPlugin, { ...remotePlugin, status: 'enterprise' })).toMatchObject({ - isEnterprise: true, - }); - expect(mapToCatalogPlugin(localPlugin, { ...remotePlugin, status: 'unknown' })).toMatchObject({ + expect(mapToCatalogPlugin(localPlugin, { ...remotePlugin, status: RemotePluginStatus.Enterprise })).toMatchObject( + { + isEnterprise: true, + } + ); + expect(mapToCatalogPlugin(localPlugin, { ...remotePlugin, status: RemotePluginStatus.Active })).toMatchObject({ isEnterprise: false, }); // Remote only - expect(mapToCatalogPlugin(undefined, { ...remotePlugin, status: 'enterprise' })).toMatchObject({ + expect(mapToCatalogPlugin(undefined, { ...remotePlugin, status: RemotePluginStatus.Enterprise })).toMatchObject({ isEnterprise: true, }); @@ -338,6 +366,34 @@ describe('Plugins/Helpers', () => { expect(mapToCatalogPlugin()).toMatchObject({ isEnterprise: false }); }); + test('`.isDeprecated` - comes from the remote', () => { + // Local & Remote + expect(mapToCatalogPlugin(localPlugin, { ...remotePlugin, status: RemotePluginStatus.Deprecated })).toMatchObject( + { + isDeprecated: true, + } + ); + expect(mapToCatalogPlugin(localPlugin, { ...remotePlugin, status: RemotePluginStatus.Enterprise })).toMatchObject( + { + isDeprecated: false, + } + ); + + // Remote only + expect(mapToCatalogPlugin(undefined, { ...remotePlugin, status: RemotePluginStatus.Deprecated })).toMatchObject({ + isDeprecated: true, + }); + expect(mapToCatalogPlugin(undefined, { ...remotePlugin, status: RemotePluginStatus.Enterprise })).toMatchObject({ + isDeprecated: false, + }); + + // Local only + expect(mapToCatalogPlugin(localPlugin)).toMatchObject({ isDeprecated: false }); + + // No local or remote + expect(mapToCatalogPlugin()).toMatchObject({ isDeprecated: false }); + }); + test('`.isInstalled` - prefers the local', () => { // Local & Remote expect(mapToCatalogPlugin(localPlugin, remotePlugin)).toMatchObject({ isInstalled: true }); @@ -671,7 +727,7 @@ describe('Plugins/Helpers', () => { id: 'barchart', }); - expect(isLocalPluginVisible(plugin)).toBe(true); + expect(isLocalPluginVisibleByConfig(plugin)).toBe(true); }); test('should return FALSE if the plugin is listed as hidden in the main Grafana configuration', () => { @@ -680,7 +736,7 @@ describe('Plugins/Helpers', () => { id: 'akumuli-datasource', }); - expect(isLocalPluginVisible(plugin)).toBe(false); + expect(isLocalPluginVisibleByConfig(plugin)).toBe(false); }); }); @@ -691,7 +747,7 @@ describe('Plugins/Helpers', () => { slug: 'barchart', }); - expect(isRemotePluginVisible(plugin)).toBe(true); + expect(isRemotePluginVisibleByConfig(plugin)).toBe(true); }); test('should return FALSE if the plugin is listed as hidden in the main Grafana configuration', () => { @@ -700,7 +756,7 @@ describe('Plugins/Helpers', () => { slug: 'akumuli-datasource', }); - expect(isRemotePluginVisible(plugin)).toBe(false); + expect(isRemotePluginVisibleByConfig(plugin)).toBe(false); }); }); }); diff --git a/public/app/features/plugins/admin/helpers.ts b/public/app/features/plugins/admin/helpers.ts index 9ef6444c97f..8b695b3f9bb 100644 --- a/public/app/features/plugins/admin/helpers.ts +++ b/public/app/features/plugins/admin/helpers.ts @@ -5,7 +5,7 @@ import { contextSrv } from 'app/core/core'; import { getBackendSrv } from 'app/core/services/backend_srv'; import { AccessControlAction } from 'app/types'; -import { CatalogPlugin, LocalPlugin, RemotePlugin, Version } from './types'; +import { CatalogPlugin, LocalPlugin, RemotePlugin, RemotePluginStatus, Version } from './types'; export function mergeLocalsAndRemotes( local: LocalPlugin[] = [], @@ -16,21 +16,24 @@ export function mergeLocalsAndRemotes( const errorByPluginId = groupErrorsByPluginId(errors); // add locals - local.forEach((l) => { - const remotePlugin = remote.find((r) => r.slug === l.id); - const error = errorByPluginId[l.id]; + local.forEach((localPlugin) => { + const remoteCounterpart = remote.find((r) => r.slug === localPlugin.id); + const error = errorByPluginId[localPlugin.id]; - if (!remotePlugin) { - catalogPlugins.push(mergeLocalAndRemote(l, undefined, error)); + if (!remoteCounterpart) { + catalogPlugins.push(mergeLocalAndRemote(localPlugin, undefined, error)); } }); // add remote - remote.forEach((r) => { - const localPlugin = local.find((l) => l.id === r.slug); - const error = errorByPluginId[r.slug]; + remote.forEach((remotePlugin) => { + const localCounterpart = local.find((l) => l.id === remotePlugin.slug); + const error = errorByPluginId[remotePlugin.slug]; + const shouldSkip = remotePlugin.status === RemotePluginStatus.Deprecated && !localCounterpart; // We are only listing deprecated plugins in case they are installed. - catalogPlugins.push(mergeLocalAndRemote(localPlugin, r, error)); + if (!shouldSkip) { + catalogPlugins.push(mergeLocalAndRemote(localCounterpart, remotePlugin, error)); + } }); return catalogPlugins; @@ -85,9 +88,10 @@ export function mapRemoteToCatalog(plugin: RemotePlugin, error?: PluginError): C isPublished: true, isInstalled: isDisabled, isDisabled: isDisabled, + isDeprecated: status === RemotePluginStatus.Deprecated, isCore: plugin.internal, isDev: false, - isEnterprise: status === 'enterprise', + isEnterprise: status === RemotePluginStatus.Enterprise, type: typeCode, error: error?.errorCode, angularDetected, @@ -129,6 +133,7 @@ export function mapLocalToCatalog(plugin: LocalPlugin, error?: PluginError): Cat isDisabled: isDisabled, isCore: signature === 'internal', isPublished: false, + isDeprecated: false, isDev: Boolean(dev), isEnterprise: false, type, @@ -169,9 +174,10 @@ export function mapToCatalogPlugin(local?: LocalPlugin, remote?: RemotePlugin, e }, isCore: Boolean(remote?.internal || local?.signature === PluginSignatureStatus.internal), isDev: Boolean(local?.dev), - isEnterprise: remote?.status === 'enterprise', + isEnterprise: remote?.status === RemotePluginStatus.Enterprise, isInstalled: Boolean(local) || isDisabled, isDisabled: isDisabled, + isDeprecated: remote?.status === RemotePluginStatus.Deprecated, isPublished: true, // TODO name: remote?.name || local?.name || '', @@ -296,11 +302,11 @@ export const hasInstallControlWarning = ( ); }; -export const isLocalPluginVisible = (p: LocalPlugin) => isPluginVisible(p.id); +export const isLocalPluginVisibleByConfig = (p: LocalPlugin) => isNotHiddenByConfig(p.id); -export const isRemotePluginVisible = (p: RemotePlugin) => isPluginVisible(p.slug); +export const isRemotePluginVisibleByConfig = (p: RemotePlugin) => isNotHiddenByConfig(p.slug); -function isPluginVisible(id: string) { +function isNotHiddenByConfig(id: string) { const { pluginCatalogHiddenPlugins }: { pluginCatalogHiddenPlugins: string[] } = config; return !pluginCatalogHiddenPlugins.includes(id); diff --git a/public/app/features/plugins/admin/pages/PluginDetails.test.tsx b/public/app/features/plugins/admin/pages/PluginDetails.test.tsx index 4f7f12e0236..a8482bdb52b 100644 --- a/public/app/features/plugins/admin/pages/PluginDetails.test.tsx +++ b/public/app/features/plugins/admin/pages/PluginDetails.test.tsx @@ -743,6 +743,30 @@ describe('Plugin details page', () => { await waitFor(() => expect(queryByText(/angular plugin/i)).not.toBeInTheDocument); }); + + it('should display a deprecation warning if the plugin is deprecated', async () => { + const { queryByText } = renderPluginDetails({ + id, + isInstalled: true, + isDeprecated: true, + }); + + await waitFor(() => + expect(queryByText(/plugin is deprecated and removed from the catalog/i)).toBeInTheDocument() + ); + }); + + it('should not display a deprecation warning in the plugin is not deprecated', async () => { + const { queryByText } = renderPluginDetails({ + id, + isInstalled: true, + isDeprecated: false, + }); + + await waitFor(() => + expect(queryByText(/plugin is deprecated and removed from the catalog/i)).not.toBeInTheDocument() + ); + }); }); describe('viewed as user without grafana admin permissions', () => { diff --git a/public/app/features/plugins/admin/types.ts b/public/app/features/plugins/admin/types.ts index 5604170108b..3a9f49855e4 100644 --- a/public/app/features/plugins/admin/types.ts +++ b/public/app/features/plugins/admin/types.ts @@ -43,6 +43,7 @@ export interface CatalogPlugin extends WithAccessControlMetadata { isEnterprise: boolean; isInstalled: boolean; isDisabled: boolean; + isDeprecated: boolean; // `isPublished` is TRUE if the plugin is published to grafana.com isPublished: boolean; name: string; @@ -111,7 +112,7 @@ export type RemotePlugin = { readme?: string; signatureType: PluginSignatureType | ''; slug: string; - status: string; + status: RemotePluginStatus; typeCode: PluginType; typeId: number; typeName: string; @@ -127,6 +128,16 @@ export type RemotePlugin = { angularDetected?: boolean; }; +// The available status codes on GCOM are available here: +// https://github.com/grafana/grafana-com/blob/main/packages/grafana-com-plugins-api/src/plugins/plugin.model.js#L74 +export enum RemotePluginStatus { + Deleted = 'deleted', + Active = 'active', + Pending = 'pending', + Deprecated = 'deprecated', + Enterprise = 'enterprise', +} + export type LocalPlugin = WithAccessControlMetadata & { category: string; defaultNavUrl: string;