Datasource: Overhaul plugin error handling and action buttons (#67014)

* - initial work on data source config page

* - add links to test status box
- add tracking function

* - add test for the DataSourceConfigAlert component

* - fix flicker of the alert box

* - fix the build

* - small improvements

* - fix failing build

* - fix failing unit tests

* - prettier and betterer fixes

* - fix failing e2e tests

* - fix build again

* - rewrite solution according to the PR comments

* - cleanup

* - fix failing e2e

* - use absolute path in link

* Minor fixes

---------

Co-authored-by: Torkel Ödegaard <torkel@grafana.com>

public/app/features/datasources/api.ts

@@ -68,7 +68,13 @@ export const createDataSource = (dataSource: Partial<DataSourceSettings>) =>
 
 export const getDataSourcePlugins = () => getBackendSrv().get('/api/plugins', { enabled: 1, type: 'datasource' });
 
-export const updateDataSource = (dataSource: DataSourceSettings) =>
-  getBackendSrv().put(`/api/datasources/uid/${dataSource.uid}`, dataSource);
+export const updateDataSource = (dataSource: DataSourceSettings) => {
+  // we're setting showErrorAlert and showSuccessAlert to false to suppress the popover notifications. Request result will now be
+  // handled by the data source config page
+  return getBackendSrv().put(`/api/datasources/uid/${dataSource.uid}`, dataSource, {
+    showErrorAlert: false,
+    showSuccessAlert: false,
+  });
+};
 
 export const deleteDataSource = (uid: string) => getBackendSrv().delete(`/api/datasources/uid/${uid}`);

public/app/features/datasources/components/ButtonRow.test.tsx

@@ -10,7 +10,6 @@ const setup = (propOverrides?: object) => {
     canSave: false,
     onSubmit: jest.fn(),
     onTest: jest.fn(),
-    exploreUrl: '/explore',
   };
 
   Object.assign(props, propOverrides);
@@ -22,7 +21,7 @@ describe('<ButtonRow>', () => {
   it('should render component', () => {
     setup();
 
-    expect(screen.getByRole('link', { name: 'Explore' })).toBeInTheDocument();
+    expect(screen.getByText('Test')).toBeInTheDocument();
   });
   it('should render save & test', () => {
     setup({ canSave: true });

public/app/features/datasources/components/ButtonRow.tsx

@@ -1,31 +1,23 @@
 import React from 'react';
 
 import { selectors } from '@grafana/e2e-selectors';
-import { Button, LinkButton } from '@grafana/ui';
-import { contextSrv } from 'app/core/core';
-import { AccessControlAction } from 'app/types';
+import { Button } from '@grafana/ui';
 
 export interface Props {
-  exploreUrl: string;
   canSave: boolean;
   onSubmit: (event: React.MouseEvent<HTMLButtonElement>) => void;
   onTest: (event: React.MouseEvent<HTMLButtonElement, MouseEvent>) => void;
 }
 
-export function ButtonRow({ canSave, onSubmit, onTest, exploreUrl }: Props) {
-  const canExploreDataSources = contextSrv.hasPermission(AccessControlAction.DataSourcesExplore);
-
+export function ButtonRow({ canSave, onSubmit, onTest }: Props) {
   return (
     <div className="gf-form-button-row">
-      <LinkButton variant="secondary" fill="solid" href={exploreUrl} disabled={!canExploreDataSources}>
-        Explore
-      </LinkButton>
       {canSave && (
         <Button
           type="submit"
           variant="primary"
           disabled={!canSave}
-          onClick={(event) => onSubmit(event)}
+          onClick={onSubmit}
           aria-label={selectors.pages.DataSource.saveAndTest}
         >
           Save &amp; test

public/app/features/datasources/components/DataSourceTestingStatus.tsx

@@ -1,30 +1,108 @@
-import React from 'react';
+import { css, cx } from '@emotion/css';
+import React, { HTMLAttributes } from 'react';
 
+import { DataSourceSettings as DataSourceSettingsType, GrafanaTheme2 } from '@grafana/data';
 import { selectors as e2eSelectors } from '@grafana/e2e-selectors';
-import { TestingStatus } from '@grafana/runtime';
-import { Alert } from '@grafana/ui';
+import { TestingStatus, config } from '@grafana/runtime';
+import { AlertVariant, Alert, useTheme2, Link } from '@grafana/ui';
+
+import { contextSrv } from '../../../core/core';
+import { AccessControlAction } from '../../../types';
+import { trackCreateDashboardClicked } from '../tracking';
 
 export type Props = {
   testingStatus?: TestingStatus;
+  exploreUrl: string;
+  dataSource: DataSourceSettingsType;
 };
 
-export function DataSourceTestingStatus({ testingStatus }: Props) {
-  const isError = testingStatus?.status === 'error';
+interface AlertMessageProps extends HTMLAttributes<HTMLDivElement> {
+  title: string;
+  severity?: AlertVariant;
+  exploreUrl: string;
+  dataSourceId: string;
+  onDashboardLinkClicked: () => void;
+}
+
+const getStyles = (theme: GrafanaTheme2, hasTitle: boolean) => {
+  return {
+    content: css`
+      color: ${theme.colors.text.secondary};
+      padding-top: ${hasTitle ? theme.spacing(1) : 0};
+      max-height: 50vh;
+      overflow-y: auto;
+    `,
+    disabled: css`
+      pointer-events: none;
+      color: ${theme.colors.text.secondary};
+    `,
+  };
+};
+
+const AlertSuccessMessage = ({ title, exploreUrl, dataSourceId, onDashboardLinkClicked }: AlertMessageProps) => {
+  const theme = useTheme2();
+  const hasTitle = Boolean(title);
+  const styles = getStyles(theme, hasTitle);
+  const canExploreDataSources = contextSrv.hasPermission(AccessControlAction.DataSourcesExplore);
+
+  return (
+    <div className={styles.content}>
+      Next, you can start to visualize data by{' '}
+      <Link
+        aria-label={`Create a dashboard`}
+        href={`/dashboard/new-with-ds/${dataSourceId}`}
+        className="external-link"
+        onClick={onDashboardLinkClicked}
+      >
+        building a dashboard
+      </Link>
+      , or by querying data in the{' '}
+      <Link
+        aria-label={`Explore data`}
+        className={cx('external-link', {
+          [`${styles.disabled}`]: !canExploreDataSources,
+          'test-disabled': !canExploreDataSources,
+        })}
+        href={exploreUrl}
+      >
+        Explore view
+      </Link>
+      .
+    </div>
+  );
+};
+
+AlertSuccessMessage.displayName = 'AlertSuccessMessage';
+
+export function DataSourceTestingStatus({ testingStatus, exploreUrl, dataSource }: Props) {
+  const severity = testingStatus?.status ? (testingStatus?.status as AlertVariant) : 'error';
   const message = testingStatus?.message;
   const detailsMessage = testingStatus?.details?.message;
   const detailsVerboseMessage = testingStatus?.details?.verboseMessage;
+  const onDashboardLinkClicked = () => {
+    trackCreateDashboardClicked({
+      grafana_version: config.buildInfo.version,
+      datasource_uid: dataSource.uid,
+      plugin_name: dataSource.typeName,
+      path: location.pathname,
+    });
+  };
 
   if (message) {
     return (
       <div className="gf-form-group p-t-2">
-        <Alert
-          severity={isError ? 'error' : 'success'}
-          title={message}
-          aria-label={e2eSelectors.pages.DataSource.alert}
-        >
+        <Alert severity={severity} title={message} aria-label={e2eSelectors.pages.DataSource.alert}>
           {testingStatus?.details && (
             <>
               {detailsMessage}
+              {severity === 'success' ? (
+                <AlertSuccessMessage
+                  title={message}
+                  exploreUrl={exploreUrl}
+                  dataSourceId={dataSource.uid}
+                  onDashboardLinkClicked={onDashboardLinkClicked}
+                />
+              ) : null}
               {detailsVerboseMessage ? (
                 <details style={{ whiteSpace: 'pre-wrap' }}>{String(detailsVerboseMessage)}</details>
               ) : null}

public/app/features/datasources/components/EditDataSource.tsx

@@ -119,7 +119,11 @@ export function EditDataSourceView({
 
   const onSubmit = async (e: React.MouseEvent<HTMLButtonElement> | React.FormEvent<HTMLFormElement>) => {
     e.preventDefault();
-    await onUpdate({ ...dataSource });
+    try {
+      await onUpdate({ ...dataSource });
+    } catch (err) {
+      return;
+    }
 
     onTest();
   };
@@ -173,9 +177,9 @@ export function EditDataSourceView({
         </DataSourcePluginContextProvider>
       )}
 
-      <DataSourceTestingStatus testingStatus={testingStatus} />
+      <DataSourceTestingStatus testingStatus={testingStatus} exploreUrl={exploreUrl} dataSource={dataSource} />
 
-      <ButtonRow onSubmit={onSubmit} onTest={onTest} exploreUrl={exploreUrl} canSave={!readOnly && hasWriteRights} />
+      <ButtonRow onSubmit={onSubmit} onTest={onTest} canSave={!readOnly && hasWriteRights} />
     </form>
   );
 }

public/app/features/datasources/pages/EditDataSourcePage.test.tsx

@@ -4,7 +4,7 @@ import { Store } from 'redux';
 import { TestProvider } from 'test/helpers/TestProvider';
 
 import { LayoutModes } from '@grafana/data';
-import { setAngularLoader } from '@grafana/runtime';
+import { setAngularLoader, config } from '@grafana/runtime';
 import { getRouteComponentProps } from 'app/core/navigation/__mocks__/routeProps';
 import { configureStore } from 'app/store/configureStore';
 
@@ -102,12 +102,23 @@ describe('<EditDataSourcePage>', () => {
     // Title
     expect(screen.queryByText(name)).toBeVisible();
 
+    // Buttons
+    expect(screen.queryByRole('button', { name: /Save (.*) test/i })).toBeVisible();
+
+    // wait for the rest of the async processes to finish
+    expect(await screen.findByText(name)).toBeVisible();
+  });
+
+  it('should show updated action buttons when topnav is on', async () => {
+    config.featureToggles.topnav = true;
+    setup(uid, store);
+
     await waitFor(() => {
       // Buttons
       expect(screen.queryByRole('button', { name: /Delete/i })).toBeVisible();
       expect(screen.queryByRole('button', { name: /Save (.*) test/i })).toBeVisible();
       expect(screen.queryByRole('link', { name: /Build a dashboard/i })).toBeVisible();
-      expect(screen.queryAllByRole('link', { name: /Explore/i })).toHaveLength(2);
+      expect(screen.queryAllByRole('link', { name: /Explore/i })).toHaveLength(1);
     });
   });
 });

public/app/features/datasources/state/actions.test.ts

@@ -217,7 +217,7 @@ describe('testDataSource', () => {
           ({
             get: jest.fn().mockReturnValue({
               testDatasource: jest.fn().mockReturnValue({
-                status: '',
+                status: 'success',
                 message: '',
               }),
               type: 'cloudwatch',
@@ -228,8 +228,9 @@ describe('testDataSource', () => {
       };
       const state = {
         testingStatus: {
-          status: '',
+          status: 'success',
           message: '',
+          details: {},
         },
       };
       const dispatchedActions = await thunkTester(state)

public/app/features/datasources/state/actions.ts

@@ -57,6 +57,40 @@ export interface TestDataSourceDependencies {
   getBackendSrv: typeof getBackendSrv;
 }
 
+type parseDataSourceSaveResponse = {
+  message?: string | undefined;
+  status?: string;
+  details?: HealthCheckResultDetails | { message?: string; verboseMessage?: string };
+};
+
+const parseHealthCheckError = (errorResponse: any): parseDataSourceSaveResponse => {
+  let message: string | undefined;
+  let details: HealthCheckResultDetails;
+
+  if (errorResponse.error && errorResponse.error instanceof HealthCheckError) {
+    message = errorResponse.error.message;
+    details = errorResponse.error.details;
+  } else if (isFetchError(errorResponse)) {
+    message = errorResponse.data.message ?? `HTTP error ${errorResponse.statusText}`;
+  } else if (errorResponse instanceof Error) {
+    message = errorResponse.message;
+  }
+
+  return { message, details };
+};
+
+const parseHealthCheckSuccess = (response: any): parseDataSourceSaveResponse => {
+  let message: string | undefined;
+  let status: string;
+  let details: { message?: string; verboseMessage?: string };
+
+  status = response.status;
+  message = response.message;
+  details = response.details;
+
+  return { status, message, details };
+};
+
 export const initDataSourceSettings = (
   uid: string,
   dependencies: InitDataSourceSettingDependencies = {
@@ -112,7 +146,9 @@ export const testDataSource = (
       try {
         const result = await dsApi.testDatasource();
 
-        dispatch(testDataSourceSucceeded(result));
+        const parsedResult = parseHealthCheckSuccess({ ...result, details: { ...result.details } });
+        dispatch(testDataSourceSucceeded(parsedResult));
+
         trackDataSourceTested({
           grafana_version: config.buildInfo.version,
           plugin_id: dsApi.type,
@@ -121,19 +157,9 @@ export const testDataSource = (
           path: editLink,
         });
       } catch (err) {
-        let message: string | undefined;
-        let details: HealthCheckResultDetails;
+        const formattedError = parseHealthCheckError(err);
 
-        if (err instanceof HealthCheckError) {
-          message = err.message;
-          details = err.details;
-        } else if (isFetchError(err)) {
-          message = err.data.message ?? `HTTP error ${err.statusText}`;
-        } else if (err instanceof Error) {
-          message = err.message;
-        }
-
-        dispatch(testDataSourceFailed({ message, details }));
+        dispatch(testDataSourceFailed({ ...formattedError }));
         trackDataSourceTested({
           grafana_version: config.buildInfo.version,
           plugin_id: dsApi.type,
@@ -216,6 +242,7 @@ export function addDataSource(
       isDefault: isFirstDataSource,
     };
 
+    // TODO: typo in name
     if (nameExits(dataSources, newInstance.name)) {
       newInstance.name = findNewName(dataSources, newInstance.name);
     }
@@ -248,9 +275,23 @@ export function loadDataSourcePlugins(): ThunkResult<void> {
 }
 
 export function updateDataSource(dataSource: DataSourceSettings) {
-  return async (dispatch: (dataSourceSettings: ThunkResult<Promise<DataSourceSettings>>) => DataSourceSettings) => {
-    await api.updateDataSource(dataSource);
+  return async (
+    dispatch: (
+      dataSourceSettings: ThunkResult<Promise<DataSourceSettings>> | { payload: unknown; type: string }
+    ) => DataSourceSettings
+  ) => {
+    try {
+      await api.updateDataSource(dataSource);
+    } catch (err: any) {
+      const formattedError = parseHealthCheckError(err);
+
+      dispatch(testDataSourceFailed(formattedError));
+
+      return Promise.reject(dataSource);
+    }
+
     await getDatasourceSrv().reload();
+
     return dispatch(loadDataSource(dataSource.uid));
   };
 }
@@ -259,13 +300,18 @@ export function deleteLoadedDataSource(): ThunkResult<void> {
   return async (dispatch, getStore) => {
     const { uid } = getStore().dataSources.dataSource;
 
-    await api.deleteDataSource(uid);
-    await getDatasourceSrv().reload();
+    try {
+      await api.deleteDataSource(uid);
+      await getDatasourceSrv().reload();
 
-    const datasourcesUrl = config.featureToggles.dataConnectionsConsole
-      ? CONNECTIONS_ROUTES.DataSources
-      : '/datasources';
+      const datasourcesUrl = config.featureToggles.dataConnectionsConsole
+        ? CONNECTIONS_ROUTES.DataSources
+        : '/datasources';
 
-    locationService.push(datasourcesUrl);
+      locationService.push(datasourcesUrl);
+    } catch (err) {
+      const formattedError = parseHealthCheckError(err);
+      dispatch(testDataSourceFailed(formattedError));
+    }
   };
 }

public/app/features/datasources/state/reducers.ts

@@ -146,7 +146,7 @@ export const dataSourceSettingsReducer = (
     return {
       ...state,
       testingStatus: {
-        message: 'Testing...',
+        message: 'Testing... this could take up to a couple of minutes',
         status: 'info',
       },
     };

public/app/features/datasources/types.ts

@@ -14,3 +14,5 @@ export type DataSourcesRoutes = {
   List: string;
   Dashboards: string;
 };
+
+export type DataSourceTestStatus = 'success' | 'warning' | 'error';