Datasource: Overhaul plugin error handling and action buttons

.betterer.results

@@ -380,8 +380,7 @@ exports[`better eslint`] = {
       [0, 0, 0, "Unexpected any. Specify a different type.", "25"],
       [0, 0, 0, "Unexpected any. Specify a different type.", "26"],
       [0, 0, 0, "Unexpected any. Specify a different type.", "27"],
-      [0, 0, 0, "Unexpected any. Specify a different type.", "28"],
-      [0, 0, 0, "Unexpected any. Specify a different type.", "29"]
+      [0, 0, 0, "Unexpected any. Specify a different type.", "28"]
     ],
     "packages/grafana-data/src/types/explore.ts:5381": [
       [0, 0, 0, "Unexpected any. Specify a different type.", "0"],
@@ -841,8 +840,7 @@ exports[`better eslint`] = {
       [0, 0, 0, "Do not use any type assertions.", "2"],
       [0, 0, 0, "Unexpected any. Specify a different type.", "3"],
       [0, 0, 0, "Unexpected any. Specify a different type.", "4"],
-      [0, 0, 0, "Unexpected any. Specify a different type.", "5"],
-      [0, 0, 0, "Unexpected any. Specify a different type.", "6"]
+      [0, 0, 0, "Unexpected any. Specify a different type.", "5"]
     ],
     "packages/grafana-runtime/src/utils/plugin.ts:5381": [
       [0, 0, 0, "Unexpected any. Specify a different type.", "0"]
@@ -2638,7 +2636,8 @@ exports[`better eslint`] = {
       [0, 0, 0, "Use data-testid for E2E selectors instead of aria-label", "0"]
     ],
     "public/app/features/datasources/components/DataSourceTestingStatus.tsx:5381": [
-      [0, 0, 0, "Use data-testid for E2E selectors instead of aria-label", "0"]
+      [0, 0, 0, "Do not use any type assertions.", "0"],
+      [0, 0, 0, "Use data-testid for E2E selectors instead of aria-label", "1"]
     ],
     "public/app/features/datasources/components/DataSourceTypeCard.tsx:5381": [
       [0, 0, 0, "Use data-testid for E2E selectors instead of aria-label", "0"]
@@ -2652,8 +2651,11 @@ exports[`better eslint`] = {
       [0, 0, 0, "Unexpected any. Specify a different type.", "5"]
     ],
     "public/app/features/datasources/state/actions.ts:5381": [
-      [0, 0, 0, "Do not use any type assertions.", "0"],
-      [0, 0, 0, "Do not use any type assertions.", "1"]
+      [0, 0, 0, "Unexpected any. Specify a different type.", "0"],
+      [0, 0, 0, "Unexpected any. Specify a different type.", "1"],
+      [0, 0, 0, "Do not use any type assertions.", "2"],
+      [0, 0, 0, "Do not use any type assertions.", "3"],
+      [0, 0, 0, "Unexpected any. Specify a different type.", "4"]
     ],
     "public/app/features/datasources/state/navModel.ts:5381": [
       [0, 0, 0, "Do not use any type assertions.", "0"],

packages/grafana-data/src/types/datasource.ts

@@ -260,7 +260,7 @@ abstract class DataSourceApi<
    * a TestingStatus object. Unknown errors and HTTP errors can be re-thrown and will be handled here:
    * public/app/features/datasources/state/actions.ts
    */
-  abstract testDatasource(): Promise<any>;
+  abstract testDatasource(): Promise<TestDataSourceResponse>;
 
   /**
    * Override to skip executing a query
@@ -473,6 +473,13 @@ export interface DataQueryResponse {
   traceIds?: string[];
 }
 
+export interface TestDataSourceResponse {
+  status: string;
+  message: string;
+  error?: Error;
+  details?: { message?: string; verboseMessage?: string };
+}
+
 export enum DataQueryErrorType {
   Cancelled = 'cancelled',
   Timeout = 'timeout',

packages/grafana-runtime/src/utils/DataSourceWithBackend.ts

@@ -7,6 +7,7 @@ import {
   DataQuery,
   DataQueryRequest,
   DataQueryResponse,
+  TestDataSourceResponse,
   DataSourceApi,
   DataSourceInstanceSettings,
   DataSourceJsonData,
@@ -342,7 +343,7 @@ class DataSourceWithBackend<
    * Checks the plugin health
    * see public/app/features/datasources/state/actions.ts for what needs to be returned here
    */
-  async testDatasource(): Promise<any> {
+  async testDatasource(): Promise<TestDataSourceResponse> {
     return this.callHealthCheck().then((res) => {
       if (res.status === HealthStatus.OK) {
         return {
@@ -351,7 +352,11 @@ class DataSourceWithBackend<
         };
       }
 
-      throw new HealthCheckError(res.message, res.details);
+      return Promise.reject({
+        status: 'error',
+        message: res.message,
+        error: new HealthCheckError(res.message, res.details),
+      });
     });
   }
 }

plugins-bundled/internal/input-datasource/src/InputDatasource.ts

@@ -2,6 +2,7 @@
 import {
   DataQueryRequest,
   DataQueryResponse,
+  TestDataSourceResponse,
   DataSourceApi,
   DataSourceInstanceSettings,
   MetricFindValue,
@@ -68,7 +69,7 @@ export class InputDatasource extends DataSourceApi<InputQuery, InputOptions> {
     return Promise.resolve({ data: results });
   }
 
-  testDatasource() {
+  testDatasource(): Promise<TestDataSourceResponse> {
     return new Promise((resolve, reject) => {
       let rowCount = 0;
       let info = `${this.data.length} Series:`;

public/app/core/services/backend_srv.ts

@@ -301,7 +301,7 @@ export class BackendSrv implements BackendService {
       return;
     }
 
-    // is showSuccessAlert is undefined we only show alerts non GET request, non data query and local api requests
+    // if showSuccessAlert is undefined we only show alerts non GET request, non data query and local api requests
     if (
       config.showSuccessAlert === undefined &&
       (config.method === 'GET' || isDataQuery(config.url) || !isLocalUrl(config.url))

public/app/core/specs/backend_srv.test.ts

@@ -125,15 +125,18 @@ describe('backendSrv', () => {
   });
 
   describe('request', () => {
+    const testMessage = 'Datasource updated';
+    const errorMessage = 'UnAuthorized';
+
     describe('when making a successful call and conditions for showSuccessAlert are not favorable', () => {
       it('then it should return correct result and not emit anything', async () => {
         const { backendSrv, appEventsMock, expectRequestCallChain } = getTestContext({
-          data: { message: 'A message' },
+          data: { message: testMessage },
         });
         const url = '/api/dashboard/';
         const result = await backendSrv.request({ url, method: 'DELETE', showSuccessAlert: false });
 
-        expect(result).toEqual({ message: 'A message' });
+        expect(result).toEqual({ message: testMessage });
         expect(appEventsMock.emit).not.toHaveBeenCalled();
         expectRequestCallChain({ url, method: 'DELETE', showSuccessAlert: false });
       });
@@ -142,14 +145,14 @@ describe('backendSrv', () => {
     describe('when making a successful call and conditions for showSuccessAlert are favorable', () => {
       it('then it should emit correct message', async () => {
         const { backendSrv, appEventsMock, expectRequestCallChain } = getTestContext({
-          data: { message: 'A message' },
+          data: { message: testMessage },
         });
         const url = '/api/dashboard/';
         const result = await backendSrv.request({ url, method: 'DELETE', showSuccessAlert: true });
 
-        expect(result).toEqual({ message: 'A message' });
+        expect(result).toEqual({ message: testMessage });
         expect(appEventsMock.emit).toHaveBeenCalledTimes(1);
-        expect(appEventsMock.emit).toHaveBeenCalledWith(AppEvents.alertSuccess, ['A message']);
+        expect(appEventsMock.emit).toHaveBeenCalledWith(AppEvents.alertSuccess, [testMessage]);
         expectRequestCallChain({ url, method: 'DELETE', showSuccessAlert: true });
       });
     });
@@ -161,8 +164,8 @@ describe('backendSrv', () => {
         const { backendSrv, appEventsMock, logoutMock, expectRequestCallChain } = getTestContext({
           ok: false,
           status: 401,
-          statusText: 'UnAuthorized',
-          data: { message: 'UnAuthorized' },
+          statusText: errorMessage,
+          data: { message: errorMessage },
           url,
         });
 
@@ -174,18 +177,18 @@ describe('backendSrv', () => {
           .request({ url, method: 'GET', retry: 0 })
           .catch((error) => {
             expect(error.status).toBe(401);
-            expect(error.statusText).toBe('UnAuthorized');
-            expect(error.data).toEqual({ message: 'UnAuthorized' });
+            expect(error.statusText).toBe(errorMessage);
+            expect(error.data).toEqual({ message: errorMessage });
             expect(appEventsMock.emit).not.toHaveBeenCalled();
             expect(logoutMock).not.toHaveBeenCalled();
             expect(backendSrv.loginPing).toHaveBeenCalledTimes(1);
             expectRequestCallChain({ url, method: 'GET', retry: 0 });
             jest.advanceTimersByTime(50);
           })
           .catch((error) => {
-            expect(error).toEqual({ message: 'UnAuthorized' });
+            expect(error).toEqual({ message: errorMessage });
             expect(appEventsMock.emit).toHaveBeenCalledTimes(1);
-            expect(appEventsMock.emit).toHaveBeenCalledWith(AppEvents.alertWarning, ['UnAuthorized', '']);
+            expect(appEventsMock.emit).toHaveBeenCalledWith(AppEvents.alertWarning, [errorMessage, '']);
           });
       });
     });
@@ -196,7 +199,7 @@ describe('backendSrv', () => {
         const { backendSrv, appEventsMock, logoutMock, expectRequestCallChain } = getTestContext({
           ok: false,
           status: 401,
-          statusText: 'UnAuthorized',
+          statusText: errorMessage,
           data: { message: 'Token revoked', error: { id: 'ERR_TOKEN_REVOKED', maxConcurrentSessions: 3 } },
           url,
         });
@@ -226,8 +229,8 @@ describe('backendSrv', () => {
         const { backendSrv, appEventsMock, logoutMock, expectRequestCallChain } = getTestContext({
           ok: false,
           status: 401,
-          statusText: 'UnAuthorized',
-          data: { message: 'UnAuthorized' },
+          statusText: errorMessage,
+          data: { message: errorMessage },
         });
 
         backendSrv.loginPing = jest

public/app/features/dashboard/services/PublicDashboardDataSource.ts

@@ -5,6 +5,7 @@ import {
   DataQuery,
   DataQueryRequest,
   DataQueryResponse,
+  TestDataSourceResponse,
   DataSourceApi,
   DataSourceJsonData,
   DataSourcePluginMeta,
@@ -151,7 +152,7 @@ export class PublicDashboardDataSource extends DataSourceApi<DataQuery, DataSour
     return { data: [toDataFrame(annotations)] };
   }
 
-  testDatasource(): Promise<null> {
-    return Promise.resolve(null);
+  testDatasource(): Promise<TestDataSourceResponse> {
+    return Promise.resolve({ message: '', status: '' });
   }
 }

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