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

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(<DataSourceSettingsPage {...mockProps} />);
 

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<Props> {
     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<Props> {
     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<Props> {
 
     return (
       <Page navModel={nav}>
-        <Page.Contents>
-          <div>
-            <div className="gf-form-button-row">
-              {showDelete && (
-                <Button type="submit" variant="destructive" onClick={this.onDelete}>
-                  Delete
-                </Button>
-              )}
-              <LinkButton variant="secondary" href="datasources" fill="outline">
-                Back
-              </LinkButton>
-            </div>
+        <Page.Contents isLoading={this.props.loading}>
+          {this.isReadOnly() && this.renderIsReadOnlyMessage()}
+          <div className="gf-form-button-row">
+            {canDeleteDataSources && (
+              <Button type="submit" variant="destructive" onClick={this.onDelete}>
+                Delete
+              </Button>
+            )}
+            <LinkButton variant="secondary" href="datasources" fill="outline">
+              Back
+            </LinkButton>
           </div>
         </Page.Contents>
       </Page>
@@ -295,15 +287,15 @@ export class DataSourceSettingsPage extends PureComponent<Props> {
   }
 
   render() {
-    const { navModel, page, loadError } = this.props;
+    const { navModel, page, loadError, loading } = this.props;
 
     if (loadError) {
-      return this.renderLoadError(loadError);
+      return this.renderLoadError();
     }
 
     return (
       <Page navModel={navModel}>
-        <Page.Contents isLoading={!this.hasDataSource}>
+        <Page.Contents isLoading={loading}>
           {this.hasDataSource ? <div>{page ? this.renderConfigPageBody(page) : this.renderSettings()}</div> : null}
         </Page.Contents>
       </Page>

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<void> => (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<void> => (dispatch: ThunkDispatch, getState) => dataSource) as any,
+        loadDataSourceMeta: jest.fn((): ThunkResult<void> => (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<void> => (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);
     });
   });
 });

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<void> {
   };
 }
 
-export function loadDataSource(uid: string): ThunkResult<void> {
+export function loadDataSource(uid: string): ThunkResult<Promise<DataSourceSettings>> {
   return async (dispatch) => {
     const dataSource = await getDataSourceUsingUidOrId(uid);
+
+    dispatch(dataSourceLoaded(dataSource));
+    return dataSource;
+  };
+}
+
+export function loadDataSourceMeta(dataSource: DataSourceSettings): ThunkResult<void> {
+  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<void> {
       ...pluginInfo,
       isBackend: isBackend,
     };
-    dispatch(dataSourceLoaded(dataSource));
+
     dispatch(dataSourceMetaLoaded(meta));
 
     plugin.meta = meta;

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;
         });
     });

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)) {

public/app/features/plugins/PluginPage.tsx

@@ -62,33 +62,29 @@ class PluginPage extends PureComponent<Props, State> {
   }
 
   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) {

public/app/features/plugins/PluginSettingsCache.ts

@@ -19,7 +19,6 @@ export function getPluginSettings(pluginId: string): Promise<PluginMeta> {
       return settings;
     })
     .catch((err: any) => {
-      // err.isHandled = true;
-      return Promise.reject('Unknown Plugin');
+      return Promise.reject(new Error('Unknown Plugin'));
     });
 }

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 {