From 89558f20bd5c827b6ceb17c730499ad077a57de0 Mon Sep 17 00:00:00 2001
From: Domas <domasx2@gmail.com>
Date: Wed, 19 May 2021 10:12:44 +0300
Subject: [PATCH] Alerting: improve error presentation in forms (#34309)

---
 .../features/alerting/unified/AmRoutes.tsx    |   5 -
 .../alerting/unified/api/alertmanager.ts      |  13 +-
 .../features/alerting/unified/api/ruler.ts    |  15 +-
 .../receivers/form/ReceiverForm.tsx           |  16 +-
 .../components/rule-editor/AlertRuleForm.tsx  |  34 ++--
 .../unified/components/rules/RulesFilter.tsx  |   1 +
 .../components/silences/SilencesEditor.tsx    |   9 +-
 .../alerting/unified/state/actions.ts         | 146 ++++++++++--------
 .../features/alerting/unified/utils/redux.ts  |  28 +++-
 9 files changed, 148 insertions(+), 119 deletions(-)

diff --git a/public/app/features/alerting/unified/AmRoutes.tsx b/public/app/features/alerting/unified/AmRoutes.tsx
index 1a5e2fa7198..55b6f3ef906 100644
--- a/public/app/features/alerting/unified/AmRoutes.tsx
+++ b/public/app/features/alerting/unified/AmRoutes.tsx
@@ -97,11 +97,6 @@ const AmRoutes: FC = () => {
   return (
     <AlertingPageWrapper pageId="am-routes">
       <AlertManagerPicker current={alertManagerSourceName} onChange={setAlertManagerSourceName} />
-      {savingError && !saving && (
-        <Alert severity="error" title="Error saving alert manager config">
-          {savingError.message || 'Unknown error.'}
-        </Alert>
-      )}
       {resultError && !resultLoading && (
         <Alert severity="error" title="Error loading alert manager config">
           {resultError.message || 'Unknown error.'}
diff --git a/public/app/features/alerting/unified/api/alertmanager.ts b/public/app/features/alerting/unified/api/alertmanager.ts
index 35be36a3e7e..dff5f43f1e5 100644
--- a/public/app/features/alerting/unified/api/alertmanager.ts
+++ b/public/app/features/alerting/unified/api/alertmanager.ts
@@ -71,10 +71,15 @@ export async function createOrUpdateSilence(
   alertmanagerSourceName: string,
   payload: SilenceCreatePayload
 ): Promise<Silence> {
-  const result = await getBackendSrv().post(
-    `/api/alertmanager/${getDatasourceAPIId(alertmanagerSourceName)}/api/v2/silences`,
-    payload
-  );
+  const result = await getBackendSrv()
+    .fetch<Silence>({
+      url: `/api/alertmanager/${getDatasourceAPIId(alertmanagerSourceName)}/api/v2/silences`,
+      data: payload,
+      showErrorAlert: false,
+      showSuccessAlert: false,
+      method: 'POST',
+    })
+    .toPromise();
   return result.data;
 }
 
diff --git a/public/app/features/alerting/unified/api/ruler.ts b/public/app/features/alerting/unified/api/ruler.ts
index c3a515df9dd..f73440025ff 100644
--- a/public/app/features/alerting/unified/api/ruler.ts
+++ b/public/app/features/alerting/unified/api/ruler.ts
@@ -51,11 +51,16 @@ export async function fetchRulerRulesGroup(
 }
 
 export async function deleteRulerRulesGroup(dataSourceName: string, namespace: string, groupName: string) {
-  return getBackendSrv().delete(
-    `/api/ruler/${getDatasourceAPIId(dataSourceName)}/api/v1/rules/${encodeURIComponent(
-      namespace
-    )}/${encodeURIComponent(groupName)}`
-  );
+  return getBackendSrv()
+    .fetch({
+      url: `/api/ruler/${getDatasourceAPIId(dataSourceName)}/api/v1/rules/${encodeURIComponent(
+        namespace
+      )}/${encodeURIComponent(groupName)}`,
+      method: 'DELETE',
+      showSuccessAlert: false,
+      showErrorAlert: false,
+    })
+    .toPromise();
 }
 
 // false in case ruler is not supported. this is weird, but we'll work on it
diff --git a/public/app/features/alerting/unified/components/receivers/form/ReceiverForm.tsx b/public/app/features/alerting/unified/components/receivers/form/ReceiverForm.tsx
index ac2d3854105..d7dc4a7606a 100644
--- a/public/app/features/alerting/unified/components/receivers/form/ReceiverForm.tsx
+++ b/public/app/features/alerting/unified/components/receivers/form/ReceiverForm.tsx
@@ -1,5 +1,5 @@
 import { css } from '@emotion/css';
-import { GrafanaTheme2 } from '@grafana/data';
+import { GrafanaTheme2, AppEvents } from '@grafana/data';
 import { Alert, Button, Field, Input, LinkButton, useStyles2 } from '@grafana/ui';
 import { useCleanup } from 'app/core/hooks/useCleanup';
 import { AlertManagerCortexConfig } from 'app/plugins/datasource/alertmanager/types';
@@ -12,6 +12,7 @@ import { ChannelValues, CommonSettingsComponentType, ReceiverFormValues } from '
 import { makeAMLink } from '../../../utils/misc';
 import { ChannelSubForm } from './ChannelSubForm';
 import { DeletedSubForm } from './fields/DeletedSubform';
+import { appEvents } from 'app/core/core';
 
 interface Props<R extends ChannelValues> {
   config: AlertManagerCortexConfig;
@@ -53,7 +54,7 @@ export function ReceiverForm<R extends ChannelValues>({
 
   useCleanup((state) => state.unifiedAlerting.saveAMConfig);
 
-  const { loading, error } = useUnifiedAlertingSelector((state) => state.saveAMConfig);
+  const { loading } = useUnifiedAlertingSelector((state) => state.saveAMConfig);
 
   const {
     handleSubmit,
@@ -79,6 +80,10 @@ export function ReceiverForm<R extends ChannelValues>({
     });
   };
 
+  const onInvalid = () => {
+    appEvents.emit(AppEvents.alertError, ['There are errors in the form. Please correct them and try again!']);
+  };
+
   return (
     <FormProvider {...formAPI}>
       {!config.alertmanager_config.route && (
@@ -86,13 +91,8 @@ export function ReceiverForm<R extends ChannelValues>({
           Because there is no default policy configured yet, this contact point will automatically be set as default.
         </Alert>
       )}
-      <form onSubmit={handleSubmit(submitCallback)}>
+      <form onSubmit={handleSubmit(submitCallback, onInvalid)}>
         <h4 className={styles.heading}>{initialValues ? 'Update contact point' : 'Create contact point'}</h4>
-        {error && (
-          <Alert severity="error" title="Error saving receiver">
-            {error.message || String(error)}
-          </Alert>
-        )}
         <Field label="Name" invalid={!!errors.name} error={errors.name && errors.name.message}>
           <Input
             id="name"
diff --git a/public/app/features/alerting/unified/components/rule-editor/AlertRuleForm.tsx b/public/app/features/alerting/unified/components/rule-editor/AlertRuleForm.tsx
index fd636ce013c..4aa39a82d1e 100644
--- a/public/app/features/alerting/unified/components/rule-editor/AlertRuleForm.tsx
+++ b/public/app/features/alerting/unified/components/rule-editor/AlertRuleForm.tsx
@@ -1,6 +1,6 @@
 import React, { FC, useMemo } from 'react';
-import { GrafanaTheme2 } from '@grafana/data';
-import { PageToolbar, Button, useStyles2, CustomScrollbar, Spinner, Alert } from '@grafana/ui';
+import { GrafanaTheme2, AppEvents } from '@grafana/data';
+import { PageToolbar, Button, useStyles2, CustomScrollbar, Spinner } from '@grafana/ui';
 import { css } from '@emotion/css';
 
 import { AlertTypeStep } from './AlertTypeStep';
@@ -20,6 +20,8 @@ import { rulerRuleToFormValues, getDefaultFormValues, getDefaultQueries } from '
 import { Link } from 'react-router-dom';
 import { useQueryParams } from 'app/core/hooks/useQueryParams';
 
+import { appEvents } from 'app/core/core';
+
 type Props = {
   existing?: RuleWithLocation;
 };
@@ -45,15 +47,10 @@ export const AlertRuleForm: FC<Props> = ({ existing }) => {
   const formAPI = useForm<RuleFormValues>({
     mode: 'onSubmit',
     defaultValues,
+    shouldFocusError: true,
   });
 
-  const {
-    handleSubmit,
-    watch,
-    formState: { errors },
-  } = formAPI;
-
-  const hasErrors = !!Object.values(errors).filter((x) => !!x).length;
+  const { handleSubmit, watch } = formAPI;
 
   const type = watch('type');
   const dataSourceName = watch('dataSourceName');
@@ -78,6 +75,10 @@ export const AlertRuleForm: FC<Props> = ({ existing }) => {
     );
   };
 
+  const onInvalid = () => {
+    appEvents.emit(AppEvents.alertError, ['There are errors in the form. Please correct them and try again!']);
+  };
+
   return (
     <FormProvider {...formAPI}>
       <form onSubmit={(e) => e.preventDefault()} className={styles.form}>
@@ -90,7 +91,7 @@ export const AlertRuleForm: FC<Props> = ({ existing }) => {
           <Button
             variant="secondary"
             type="button"
-            onClick={handleSubmit((values) => submit(values, false))}
+            onClick={handleSubmit((values) => submit(values, false), onInvalid)}
             disabled={submitState.loading}
           >
             {submitState.loading && <Spinner className={styles.buttonSpinner} inline={true} />}
@@ -99,7 +100,7 @@ export const AlertRuleForm: FC<Props> = ({ existing }) => {
           <Button
             variant="primary"
             type="button"
-            onClick={handleSubmit((values) => submit(values, true))}
+            onClick={handleSubmit((values) => submit(values, true), onInvalid)}
             disabled={submitState.loading}
           >
             {submitState.loading && <Spinner className={styles.buttonSpinner} inline={true} />}
@@ -109,17 +110,6 @@ export const AlertRuleForm: FC<Props> = ({ existing }) => {
         <div className={styles.contentOuter}>
           <CustomScrollbar autoHeightMin="100%" hideHorizontalTrack={true}>
             <div className={styles.contentInner}>
-              {hasErrors && (
-                <Alert
-                  severity="error"
-                  title="There are errors in the form below. Please fix them and try saving again"
-                />
-              )}
-              {submitState.error && (
-                <Alert severity="error" title="Error saving rule">
-                  {submitState.error.message || (submitState.error as any)?.data?.message || String(submitState.error)}
-                </Alert>
-              )}
               <AlertTypeStep editingExistingRule={!!existing} />
               {showStep2 && (
                 <>
diff --git a/public/app/features/alerting/unified/components/rules/RulesFilter.tsx b/public/app/features/alerting/unified/components/rules/RulesFilter.tsx
index 2c8939b217c..450b955a977 100644
--- a/public/app/features/alerting/unified/components/rules/RulesFilter.tsx
+++ b/public/app/features/alerting/unified/components/rules/RulesFilter.tsx
@@ -70,6 +70,7 @@ const RulesFilter = () => {
               prefix={searchIcon}
               onChange={handleQueryStringChange}
               defaultValue={queryString}
+              placeholder="Search"
             />
           </div>
           <div className={styles.rowChild}>
diff --git a/public/app/features/alerting/unified/components/silences/SilencesEditor.tsx b/public/app/features/alerting/unified/components/silences/SilencesEditor.tsx
index 796227bca85..f747f9bccc2 100644
--- a/public/app/features/alerting/unified/components/silences/SilencesEditor.tsx
+++ b/public/app/features/alerting/unified/components/silences/SilencesEditor.tsx
@@ -1,6 +1,6 @@
 import { Silence, SilenceCreatePayload } from 'app/plugins/datasource/alertmanager/types';
 import React, { FC, useState } from 'react';
-import { Alert, Button, Field, FieldSet, Input, LinkButton, TextArea, useStyles } from '@grafana/ui';
+import { Button, Field, FieldSet, Input, LinkButton, TextArea, useStyles } from '@grafana/ui';
 import {
   DefaultTimeZone,
   GrafanaTheme,
@@ -75,7 +75,7 @@ export const SilencesEditor: FC<Props> = ({ silence, alertManagerSourceName }) =
   const dispatch = useDispatch();
   const styles = useStyles(getStyles);
 
-  const { loading, error } = useUnifiedAlertingSelector((state) => state.updateSilence);
+  const { loading } = useUnifiedAlertingSelector((state) => state.updateSilence);
 
   useCleanup((state) => state.unifiedAlerting.updateSilence);
 
@@ -138,11 +138,6 @@ export const SilencesEditor: FC<Props> = ({ silence, alertManagerSourceName }) =
     <FormProvider {...formAPI}>
       <form onSubmit={handleSubmit(onSubmit)}>
         <FieldSet label={`${silence ? 'Recreate silence' : 'Create silence'}`}>
-          {error && (
-            <Alert severity="error" title="Error saving silence">
-              {error.message || (error as any)?.data?.message || String(error)}
-            </Alert>
-          )}
           <div className={styles.flexRow}>
             <SilencePeriod />
             <Field
diff --git a/public/app/features/alerting/unified/state/actions.ts b/public/app/features/alerting/unified/state/actions.ts
index 0533a304a18..00f958963ac 100644
--- a/public/app/features/alerting/unified/state/actions.ts
+++ b/public/app/features/alerting/unified/state/actions.ts
@@ -1,7 +1,5 @@
-import { AppEvents } from '@grafana/data';
 import { locationService } from '@grafana/runtime';
 import { createAsyncThunk } from '@reduxjs/toolkit';
-import { appEvents } from 'app/core/core';
 import {
   AlertmanagerAlert,
   AlertManagerCortexConfig,
@@ -36,7 +34,7 @@ import {
 import { RuleFormType, RuleFormValues } from '../types/rule-form';
 import { getAllRulesSourceNames, GRAFANA_RULES_SOURCE_NAME, isGrafanaRulesSource } from '../utils/datasource';
 import { makeAMLink } from '../utils/misc';
-import { withSerializedError } from '../utils/redux';
+import { withAppEvents, withSerializedError } from '../utils/redux';
 import { formValuesToRulerAlertingRuleDTO, formValuesToRulerGrafanaRuleDTO } from '../utils/rule-form';
 import {
   getRuleIdentifier,
@@ -169,14 +167,21 @@ export function deleteRuleAction(ruleIdentifier: RuleIdentifier): ThunkResult<vo
    * reload ruler rules
    */
   return async (dispatch) => {
-    const ruleWithLocation = await findExistingRule(ruleIdentifier);
-    if (!ruleWithLocation) {
-      throw new Error('Rule not found.');
-    }
-    await deleteRule(ruleWithLocation);
-    // refetch rules for this rules source
-    dispatch(fetchRulerRulesAction(ruleWithLocation.ruleSourceName));
-    dispatch(fetchPromRulesAction(ruleWithLocation.ruleSourceName));
+    withAppEvents(
+      (async () => {
+        const ruleWithLocation = await findExistingRule(ruleIdentifier);
+        if (!ruleWithLocation) {
+          throw new Error('Rule not found.');
+        }
+        await deleteRule(ruleWithLocation);
+        // refetch rules for this rules source
+        dispatch(fetchRulerRulesAction(ruleWithLocation.ruleSourceName));
+        dispatch(fetchPromRulesAction(ruleWithLocation.ruleSourceName));
+      })(),
+      {
+        successMessage: 'Rule deleted.',
+      }
+    );
   };
 }
 
@@ -298,32 +303,35 @@ export const saveRuleFormAction = createAsyncThunk(
     existing?: RuleWithLocation;
     redirectOnSave?: string;
   }): Promise<void> =>
-    withSerializedError(
-      (async () => {
-        const { type } = values;
-        // in case of cloud (cortex/loki)
-        let identifier: RuleIdentifier;
-        if (type === RuleFormType.cloud) {
-          identifier = await saveLotexRule(values, existing);
-          // in case of grafana managed
-        } else if (type === RuleFormType.grafana) {
-          identifier = await saveGrafanaRule(values, existing);
-        } else {
-          throw new Error('Unexpected rule form type');
-        }
-        if (redirectOnSave) {
-          locationService.push(redirectOnSave);
-        } else {
-          // redirect to edit page
-          const newLocation = `/alerting/${encodeURIComponent(stringifyRuleIdentifier(identifier))}/edit`;
-          if (locationService.getLocation().pathname !== newLocation) {
-            locationService.replace(newLocation);
+    withAppEvents(
+      withSerializedError(
+        (async () => {
+          const { type } = values;
+          // in case of system (cortex/loki)
+          let identifier: RuleIdentifier;
+          if (type === RuleFormType.cloud) {
+            identifier = await saveLotexRule(values, existing);
+            // in case of grafana managed
+          } else if (type === RuleFormType.grafana) {
+            identifier = await saveGrafanaRule(values, existing);
+          } else {
+            throw new Error('Unexpected rule form type');
           }
-        }
-        appEvents.emit(AppEvents.alertSuccess, [
-          existing ? `Rule "${values.name}" updated.` : `Rule "${values.name}" saved.`,
-        ]);
-      })()
+          if (redirectOnSave) {
+            locationService.push(redirectOnSave);
+          } else {
+            // redirect to edit page
+            const newLocation = `/alerting/${encodeURIComponent(stringifyRuleIdentifier(identifier))}/edit`;
+            if (locationService.getLocation().pathname !== newLocation) {
+              locationService.replace(newLocation);
+            }
+          }
+        })()
+      ),
+      {
+        successMessage: existing ? `Rule "${values.name}" updated.` : `Rule "${values.name}" saved.`,
+        errorMessage: 'Failed to save rule',
+      }
     )
 );
 
@@ -344,25 +352,27 @@ interface UpdateAlertManagerConfigActionOptions {
 export const updateAlertManagerConfigAction = createAsyncThunk<void, UpdateAlertManagerConfigActionOptions, {}>(
   'unifiedalerting/updateAMConfig',
   ({ alertManagerSourceName, oldConfig, newConfig, successMessage, redirectPath, refetch }, thunkAPI): Promise<void> =>
-    withSerializedError(
-      (async () => {
-        const latestConfig = await fetchAlertManagerConfig(alertManagerSourceName);
-        if (JSON.stringify(latestConfig) !== JSON.stringify(oldConfig)) {
-          throw new Error(
-            'It seems configuration has been recently updated. Please reload page and try again to make sure that recent changes are not overwritten.'
-          );
-        }
-        await updateAlertManagerConfig(alertManagerSourceName, addDefaultsToAlertmanagerConfig(newConfig));
-        if (successMessage) {
-          appEvents?.emit(AppEvents.alertSuccess, [successMessage]);
-        }
-        if (refetch) {
-          await thunkAPI.dispatch(fetchAlertManagerConfigAction(alertManagerSourceName));
-        }
-        if (redirectPath) {
-          locationService.push(makeAMLink(redirectPath, alertManagerSourceName));
-        }
-      })()
+    withAppEvents(
+      withSerializedError(
+        (async () => {
+          const latestConfig = await fetchAlertManagerConfig(alertManagerSourceName);
+          if (JSON.stringify(latestConfig) !== JSON.stringify(oldConfig)) {
+            throw new Error(
+              'It seems configuration has been recently updated. Please reload page and try again to make sure that recent changes are not overwritten.'
+            );
+          }
+          await updateAlertManagerConfig(alertManagerSourceName, addDefaultsToAlertmanagerConfig(newConfig));
+          if (refetch) {
+            await thunkAPI.dispatch(fetchAlertManagerConfigAction(alertManagerSourceName));
+          }
+          if (redirectPath) {
+            locationService.push(makeAMLink(redirectPath, alertManagerSourceName));
+          }
+        })()
+      ),
+      {
+        successMessage,
+      }
     )
 );
 
@@ -374,7 +384,9 @@ export const fetchAmAlertsAction = createAsyncThunk(
 
 export const expireSilenceAction = (alertManagerSourceName: string, silenceId: string): ThunkResult<void> => {
   return async (dispatch) => {
-    await expireSilence(alertManagerSourceName, silenceId);
+    await withAppEvents(expireSilence(alertManagerSourceName, silenceId), {
+      successMessage: 'Silence expired.',
+    });
     dispatch(fetchSilencesAction(alertManagerSourceName));
     dispatch(fetchAmAlertsAction(alertManagerSourceName));
   };
@@ -390,16 +402,18 @@ type UpdateSilenceActionOptions = {
 export const createOrUpdateSilenceAction = createAsyncThunk<void, UpdateSilenceActionOptions, {}>(
   'unifiedalerting/updateSilence',
   ({ alertManagerSourceName, payload, exitOnSave, successMessage }): Promise<void> =>
-    withSerializedError(
-      (async () => {
-        await createOrUpdateSilence(alertManagerSourceName, payload);
-        if (successMessage) {
-          appEvents.emit(AppEvents.alertSuccess, [successMessage]);
-        }
-        if (exitOnSave) {
-          locationService.push('/alerting/silences');
-        }
-      })()
+    withAppEvents(
+      withSerializedError(
+        (async () => {
+          await createOrUpdateSilence(alertManagerSourceName, payload);
+          if (exitOnSave) {
+            locationService.push('/alerting/silences');
+          }
+        })()
+      ),
+      {
+        successMessage,
+      }
     )
 );
 
diff --git a/public/app/features/alerting/unified/utils/redux.ts b/public/app/features/alerting/unified/utils/redux.ts
index 0493f2bbc1e..7560b343a17 100644
--- a/public/app/features/alerting/unified/utils/redux.ts
+++ b/public/app/features/alerting/unified/utils/redux.ts
@@ -1,6 +1,8 @@
 import { AnyAction, AsyncThunk, createSlice, Draft, isAsyncThunkAction, SerializedError } from '@reduxjs/toolkit';
 import { FetchError } from '@grafana/runtime';
 import { isArray } from 'angular';
+import { appEvents } from 'app/core/core';
+import { AppEvents } from '@grafana/data';
 export interface AsyncRequestState<T> {
   result?: T;
   loading: boolean;
@@ -106,14 +108,36 @@ export function withSerializedError<T>(p: Promise<T>): Promise<T> {
   });
 }
 
+export function withAppEvents<T>(
+  p: Promise<T>,
+  options: { successMessage?: string; errorMessage?: string }
+): Promise<T> {
+  return p
+    .then((v) => {
+      if (options.successMessage) {
+        appEvents.emit(AppEvents.alertSuccess, [options.successMessage]);
+      }
+      return v;
+    })
+    .catch((e) => {
+      const msg = messageFromError(e);
+      appEvents.emit(AppEvents.alertError, [`${options.errorMessage ?? 'Error'}: ${msg}`]);
+      throw e;
+    });
+}
+
 function isFetchError(e: unknown): e is FetchError {
   return typeof e === 'object' && e !== null && 'status' in e && 'data' in e;
 }
 
-function messageFromError(e: Error | FetchError): string {
+function messageFromError(e: Error | FetchError | SerializedError): string {
   if (isFetchError(e)) {
     if (e.data?.message) {
-      return e.data?.message;
+      let msg = e.data?.message;
+      if (typeof e.data?.error === 'string') {
+        msg += `; ${e.data.error}`;
+      }
+      return msg;
     } else if (isArray(e.data) && e.data.length && e.data[0]?.message) {
       return e.data
         .map((d) => d?.message)