From 7cf3c84c92296267ea0a7c3d06b6ed0069b1ba5c Mon Sep 17 00:00:00 2001 From: Jack Westbrook Date: Fri, 8 Oct 2021 12:31:43 +0200 Subject: [PATCH] Datasources: Fix deletion of datasource if plugin cannot be found (#40095) * fix(pluginsettings): reject with error so datasource plugin loading failures still render ui * feat(pluginpage): handle plugin loading error * refactor(datasources): separate out datasource and meta loading so store has info for deletion * fix(datasourcesettings): introduce loading flag to wait for datasource and meta loading * test(datasourcesettings): fix failing test * test(datasources): assert loading status of datasource settings * test(datasources): update action tests for latest changes --- .../settings/DataSourceSettingsPage.test.tsx | 2 + .../settings/DataSourceSettingsPage.tsx | 50 ++++++++----------- .../datasources/state/actions.test.ts | 16 ++++-- .../app/features/datasources/state/actions.ts | 18 +++++-- .../datasources/state/reducers.test.ts | 10 ++-- .../features/datasources/state/reducers.ts | 5 +- public/app/features/plugins/PluginPage.tsx | 40 +++++++-------- .../features/plugins/PluginSettingsCache.ts | 3 +- public/app/types/datasources.ts | 1 + 9 files changed, 79 insertions(+), 66 deletions(-) diff --git a/public/app/features/datasources/settings/DataSourceSettingsPage.test.tsx b/public/app/features/datasources/settings/DataSourceSettingsPage.test.tsx index d4c846589d5..c1ab359fdaf 100644 --- a/public/app/features/datasources/settings/DataSourceSettingsPage.test.tsx +++ b/public/app/features/datasources/settings/DataSourceSettingsPage.test.tsx @@ -44,6 +44,7 @@ const getProps = (): Props => ({ page: null, plugin: null, loadError: null, + loading: false, testingStatus: {}, }); @@ -57,6 +58,7 @@ describe('Render', () => { it('should render loading if datasource is not ready', () => { const mockProps = getProps(); mockProps.dataSource.id = 0; + mockProps.loading = true; render(); diff --git a/public/app/features/datasources/settings/DataSourceSettingsPage.tsx b/public/app/features/datasources/settings/DataSourceSettingsPage.tsx index 0d182eed22c..8626f8f512d 100644 --- a/public/app/features/datasources/settings/DataSourceSettingsPage.tsx +++ b/public/app/features/datasources/settings/DataSourceSettingsPage.tsx @@ -1,5 +1,4 @@ import React, { PureComponent } from 'react'; -import { isString } from 'lodash'; // Components import Page from 'app/core/components/Page/Page'; import { PluginSettings } from './PluginSettings'; @@ -40,7 +39,7 @@ function mapStateToProps(state: StoreState, props: OwnProps) { const dataSourceId = props.match.params.uid; const params = new URLSearchParams(props.location.search); const dataSource = getDataSource(state.dataSources, dataSourceId); - const { plugin, loadError, testingStatus } = state.dataSourceSettings; + const { plugin, loadError, loading, testingStatus } = state.dataSourceSettings; const page = params.get('page'); const nav = plugin @@ -60,6 +59,7 @@ function mapStateToProps(state: StoreState, props: OwnProps) { page, plugin, loadError, + loading, testingStatus, navModel, }; @@ -111,7 +111,7 @@ export class DataSourceSettingsPage extends PureComponent { appEvents.publish( new ShowConfirmModalEvent({ title: 'Delete', - text: 'Are you sure you want to delete this data source?', + text: `Are you sure you want to delete the "${this.props.dataSource.name}" data source?`, yesText: 'Delete', icon: 'trash-alt', onConfirm: () => { @@ -166,19 +166,12 @@ export class DataSourceSettingsPage extends PureComponent { return url; } - renderLoadError(loadError: any) { - let showDelete = false; - let msg = loadError.toString(); - if (loadError.data) { - if (loadError.data.message) { - msg = loadError.data.message; - } - } else if (isString(loadError)) { - showDelete = true; - } + renderLoadError() { + const { loadError } = this.props; + const canDeleteDataSources = !this.isReadOnly() && contextSrv.hasPermission(AccessControlAction.DataSourcesDelete); const node = { - text: msg, + text: loadError!, subTitle: 'Data Source Error', icon: 'exclamation-triangle', }; @@ -189,18 +182,17 @@ export class DataSourceSettingsPage extends PureComponent { return ( - -
-
- {showDelete && ( - - )} - - Back - -
+ + {this.isReadOnly() && this.renderIsReadOnlyMessage()} +
+ {canDeleteDataSources && ( + + )} + + Back +
@@ -295,15 +287,15 @@ export class DataSourceSettingsPage extends PureComponent { } render() { - const { navModel, page, loadError } = this.props; + const { navModel, page, loadError, loading } = this.props; if (loadError) { - return this.renderLoadError(loadError); + return this.renderLoadError(); } return ( - + {this.hasDataSource ?
{page ? this.renderConfigPageBody(page) : this.renderSettings()}
: null} diff --git a/public/app/features/datasources/state/actions.test.ts b/public/app/features/datasources/state/actions.test.ts index 594a1eab6fd..dc17083bfb1 100644 --- a/public/app/features/datasources/state/actions.test.ts +++ b/public/app/features/datasources/state/actions.test.ts @@ -114,7 +114,7 @@ describe('getDataSourceUsingUidOrId', () => { it('should return empty response data', async () => { // @ts-ignore delete window.location; - window.location = {} as any; + window.location = {} as Location; const uidResponse = { ok: false, @@ -190,11 +190,11 @@ describe('initDataSourceSettings', () => { describe('when pageId is a valid', () => { it('then initDataSourceSettingsSucceeded should be dispatched', async () => { - const thunkMock = (): ThunkResult => (dispatch: ThunkDispatch, getState) => {}; const dataSource = { type: 'app' }; const dataSourceMeta = { id: 'some id' }; const dependencies: InitDataSourceSettingDependencies = { - loadDataSource: jest.fn(thunkMock) as any, + loadDataSource: jest.fn((): ThunkResult => (dispatch: ThunkDispatch, getState) => dataSource) as any, + loadDataSourceMeta: jest.fn((): ThunkResult => (dispatch: ThunkDispatch, getState) => {}), getDataSource: jest.fn().mockReturnValue(dataSource), getDataSourceMeta: jest.fn().mockReturnValue(dataSourceMeta), importDataSourcePlugin: jest.fn().mockReturnValue({} as GenericDataSourcePlugin), @@ -211,6 +211,9 @@ describe('initDataSourceSettings', () => { expect(dependencies.loadDataSource).toHaveBeenCalledTimes(1); expect(dependencies.loadDataSource).toHaveBeenCalledWith(256); + expect(dependencies.loadDataSourceMeta).toHaveBeenCalledTimes(1); + expect(dependencies.loadDataSourceMeta).toHaveBeenCalledWith(dataSource); + expect(dependencies.getDataSource).toHaveBeenCalledTimes(1); expect(dependencies.getDataSource).toHaveBeenCalledWith({}, 256); @@ -224,8 +227,10 @@ describe('initDataSourceSettings', () => { describe('when plugin loading fails', () => { it('then initDataSourceSettingsFailed should be dispatched', async () => { + const dataSource = { type: 'app' }; const dependencies: InitDataSourceSettingDependencies = { - loadDataSource: jest.fn().mockImplementation(() => { + loadDataSource: jest.fn((): ThunkResult => (dispatch: ThunkDispatch, getState) => dataSource) as any, + loadDataSourceMeta: jest.fn().mockImplementation(() => { throw new Error('Error loading plugin'); }), getDataSource: jest.fn(), @@ -243,6 +248,9 @@ describe('initDataSourceSettings', () => { expect(dispatchedActions).toEqual([initDataSourceSettingsFailed(new Error('Error loading plugin'))]); expect(dependencies.loadDataSource).toHaveBeenCalledTimes(1); expect(dependencies.loadDataSource).toHaveBeenCalledWith(301); + + expect(dependencies.loadDataSourceMeta).toHaveBeenCalledTimes(1); + expect(dependencies.loadDataSourceMeta).toHaveBeenCalledWith(dataSource); }); }); }); diff --git a/public/app/features/datasources/state/actions.ts b/public/app/features/datasources/state/actions.ts index 9b773f042f9..b4c5639198a 100644 --- a/public/app/features/datasources/state/actions.ts +++ b/public/app/features/datasources/state/actions.ts @@ -33,6 +33,7 @@ export interface DataSourceTypesLoadedPayload { export interface InitDataSourceSettingDependencies { loadDataSource: typeof loadDataSource; + loadDataSourceMeta: typeof loadDataSourceMeta; getDataSource: typeof getDataSource; getDataSourceMeta: typeof getDataSourceMeta; importDataSourcePlugin: typeof importDataSourcePlugin; @@ -47,6 +48,7 @@ export const initDataSourceSettings = ( pageId: string, dependencies: InitDataSourceSettingDependencies = { loadDataSource, + loadDataSourceMeta, getDataSource, getDataSourceMeta, importDataSourcePlugin, @@ -59,7 +61,8 @@ export const initDataSourceSettings = ( } try { - await dispatch(dependencies.loadDataSource(pageId)); + const loadedDataSource = await dispatch(dependencies.loadDataSource(pageId)); + await dispatch(dependencies.loadDataSourceMeta(loadedDataSource)); // have we already loaded the plugin then we can skip the steps below? if (getState().dataSourceSettings.plugin) { @@ -72,7 +75,6 @@ export const initDataSourceSettings = ( dispatch(initDataSourceSettingsSucceeded(importedPlugin)); } catch (err) { - console.error('Failed to import plugin module', err); dispatch(initDataSourceSettingsFailed(err)); } }; @@ -117,9 +119,17 @@ export function loadDataSources(): ThunkResult { }; } -export function loadDataSource(uid: string): ThunkResult { +export function loadDataSource(uid: string): ThunkResult> { return async (dispatch) => { const dataSource = await getDataSourceUsingUidOrId(uid); + + dispatch(dataSourceLoaded(dataSource)); + return dataSource; + }; +} + +export function loadDataSourceMeta(dataSource: DataSourceSettings): ThunkResult { + return async (dispatch) => { const pluginInfo = (await getPluginSettings(dataSource.type)) as DataSourcePluginMeta; const plugin = await importDataSourcePlugin(pluginInfo); const isBackend = plugin.DataSourceClass.prototype instanceof DataSourceWithBackend; @@ -127,7 +137,7 @@ export function loadDataSource(uid: string): ThunkResult { ...pluginInfo, isBackend: isBackend, }; - dispatch(dataSourceLoaded(dataSource)); + dispatch(dataSourceMetaLoaded(meta)); plugin.meta = meta; diff --git a/public/app/features/datasources/state/reducers.test.ts b/public/app/features/datasources/state/reducers.test.ts index 8cc844d86e6..cb5e6afed54 100644 --- a/public/app/features/datasources/state/reducers.test.ts +++ b/public/app/features/datasources/state/reducers.test.ts @@ -150,7 +150,7 @@ describe('dataSourceSettingsReducer', () => { .thenStateShouldEqual({ ...initialDataSourceSettingsState, plugin: {} as GenericDataSourcePlugin, - loadError: null, + loading: false, }); }); }); @@ -164,8 +164,12 @@ describe('dataSourceSettingsReducer', () => { }) .whenActionIsDispatched(initDataSourceSettingsFailed(new Error('Some error'))) .thenStatePredicateShouldEqual((resultingState) => { - expect(resultingState.plugin).toEqual(null); - expect(resultingState.loadError).toEqual('Some error'); + expect(resultingState).toEqual({ + testingStatus: {}, + loadError: 'Some error', + loading: false, + plugin: null, + }); return true; }); }); diff --git a/public/app/features/datasources/state/reducers.ts b/public/app/features/datasources/state/reducers.ts index 44fe7998bee..1af1f762d6b 100644 --- a/public/app/features/datasources/state/reducers.ts +++ b/public/app/features/datasources/state/reducers.ts @@ -97,6 +97,7 @@ export const dataSourcesReducer = (state: DataSourcesState = initialState, actio export const initialDataSourceSettingsState: DataSourceSettingsState = { testingStatus: {}, loadError: null, + loading: true, plugin: null, }; @@ -117,11 +118,11 @@ export const dataSourceSettingsReducer = ( action: AnyAction ): DataSourceSettingsState => { if (initDataSourceSettingsSucceeded.match(action)) { - return { ...state, plugin: action.payload, loadError: null }; + return { ...state, plugin: action.payload, loadError: null, loading: false }; } if (initDataSourceSettingsFailed.match(action)) { - return { ...state, plugin: null, loadError: action.payload.message }; + return { ...state, plugin: null, loadError: action.payload.message, loading: false }; } if (testDataSourceStarting.match(action)) { diff --git a/public/app/features/plugins/PluginPage.tsx b/public/app/features/plugins/PluginPage.tsx index 46e0f44466b..87e635681b0 100644 --- a/public/app/features/plugins/PluginPage.tsx +++ b/public/app/features/plugins/PluginPage.tsx @@ -62,33 +62,29 @@ class PluginPage extends PureComponent { } async componentDidMount() { - const { location, queryParams } = this.props; - const { appSubUrl } = config; - - const plugin = await loadPlugin(this.props.match.params.pluginId); - - if (!plugin) { + try { + const { location, queryParams } = this.props; + const { appSubUrl } = config; + const plugin = await loadPlugin(this.props.match.params.pluginId); + const { defaultPage, nav } = getPluginTabsNav( + plugin, + appSubUrl, + location.pathname, + queryParams, + contextSrv.hasRole('Admin') + ); + this.setState({ + loading: false, + plugin, + defaultPage, + nav, + }); + } catch { this.setState({ loading: false, nav: getNotFoundNav(), }); - return; // 404 } - - const { defaultPage, nav } = getPluginTabsNav( - plugin, - appSubUrl, - location.pathname, - queryParams, - contextSrv.hasRole('Admin') - ); - - this.setState({ - loading: false, - plugin, - defaultPage, - nav, - }); } componentDidUpdate(prevProps: Props) { diff --git a/public/app/features/plugins/PluginSettingsCache.ts b/public/app/features/plugins/PluginSettingsCache.ts index 1d285f1cde3..0061919769e 100644 --- a/public/app/features/plugins/PluginSettingsCache.ts +++ b/public/app/features/plugins/PluginSettingsCache.ts @@ -19,7 +19,6 @@ export function getPluginSettings(pluginId: string): Promise { return settings; }) .catch((err: any) => { - // err.isHandled = true; - return Promise.reject('Unknown Plugin'); + return Promise.reject(new Error('Unknown Plugin')); }); } diff --git a/public/app/types/datasources.ts b/public/app/types/datasources.ts index 589624a1f7d..03eb07a1652 100644 --- a/public/app/types/datasources.ts +++ b/public/app/types/datasources.ts @@ -26,6 +26,7 @@ export interface DataSourceSettingsState { plugin?: GenericDataSourcePlugin | null; testingStatus?: TestingStatus; loadError?: string | null; + loading: boolean; } export interface DataSourcePluginCategory {