From a2c93bb8bc23e94991c1327651eb9b9dded4f8e3 Mon Sep 17 00:00:00 2001 From: Gilles De Mey Date: Wed, 6 Sep 2023 13:24:48 +0200 Subject: [PATCH] Alerting: Alert creation UI changes (#73835) --- .github/CODEOWNERS | 1 + .../alerting/unified/CloneRuleEditor.test.tsx | 2 +- .../features/alerting/unified/RuleEditor.tsx | 29 ++- .../RuleEditorCloudOnlyAllowed.test.tsx | 2 +- .../unified/RuleEditorCloudRules.test.tsx | 13 +- .../unified/components/CollapseToggle.tsx | 14 +- .../components/rule-editor/AlertRuleForm.tsx | 59 ++--- .../rule-editor/AnnotationHeaderField.tsx | 62 +++--- .../rule-editor/AnnotationsStep.tsx | 21 +- .../rule-editor/CloudEvaluationBehavior.tsx | 2 +- .../components/rule-editor/FolderAndGroup.tsx | 209 ++++++++---------- .../rule-editor/GrafanaEvaluationBehavior.tsx | 16 +- .../components/rule-editor/LabelsField.tsx | 45 ++-- .../components/rule-editor/NeedHelpInfo.tsx | 32 +-- .../rule-editor/NotificationsStep.tsx | 122 +++------- .../rule-editor/RuleEditorSection.tsx | 56 ++--- .../NotificationPreview.test.tsx | 35 +-- .../NotificationPreview.tsx | 60 ++--- .../NotificationPreviewByAlertManager.tsx | 1 - .../QueryAndExpressionsStep.tsx | 46 ++-- .../SmartAlertTypeDetector.tsx | 158 ++++++------- .../alerting/unified/mocks/alertmanagerApi.ts | 11 + public/test/helpers/alertingRuleEditor.tsx | 2 +- 23 files changed, 452 insertions(+), 546 deletions(-) diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index efcd155207c..5bbf3ce2ab1 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -453,6 +453,7 @@ lerna.json @grafana/frontend-ops /public/robots.txt @grafana/frontend-ops /public/sass/ @grafana/grafana-frontend-platform /public/test/ @grafana/grafana-frontend-platform +/public/test/helpers/alertingRuleEditor.tsx @grafana/alerting-frontend /public/views/ @grafana/grafana-frontend-platform /public/app/features/explore/Logs/ @grafana/observability-logs diff --git a/public/app/features/alerting/unified/CloneRuleEditor.test.tsx b/public/app/features/alerting/unified/CloneRuleEditor.test.tsx index 65544ad56f6..9978d9b31f2 100644 --- a/public/app/features/alerting/unified/CloneRuleEditor.test.tsx +++ b/public/app/features/alerting/unified/CloneRuleEditor.test.tsx @@ -71,7 +71,7 @@ afterAll(() => { const ui = { inputs: { - name: byRole('textbox', { name: /rule name name for the alert rule\./i }), + name: byRole('textbox', { name: 'name' }), expr: byTestId('expr'), folderContainer: byTestId(selectors.components.FolderPicker.containerV2), namespace: byTestId('namespace-picker'), diff --git a/public/app/features/alerting/unified/RuleEditor.tsx b/public/app/features/alerting/unified/RuleEditor.tsx index 6f5c2014dbd..159b0c8b291 100644 --- a/public/app/features/alerting/unified/RuleEditor.tsx +++ b/public/app/features/alerting/unified/RuleEditor.tsx @@ -5,6 +5,7 @@ import { NavModelItem } from '@grafana/data'; import { withErrorBoundary } from '@grafana/ui'; import { GrafanaRouteComponentProps } from 'app/core/navigation/types'; import { useDispatch } from 'app/types'; +import { RuleIdentifier } from 'app/types/unified-alerting'; import { AlertWarning } from './AlertWarning'; import { CloneRuleEditor } from './CloneRuleEditor'; @@ -16,27 +17,37 @@ import { fetchRulesSourceBuildInfoAction } from './state/actions'; import { useRulesAccess } from './utils/accessControlHooks'; import * as ruleId from './utils/rule-id'; -type RuleEditorProps = GrafanaRouteComponentProps<{ id?: string }>; +type RuleEditorProps = GrafanaRouteComponentProps<{ id?: string; type?: 'recording' | 'alerting' }>; const defaultPageNav: Partial = { icon: 'bell', id: 'alert-rule-view', }; -const getPageNav = (state: 'edit' | 'add') => { - if (state === 'edit') { - return { ...defaultPageNav, id: 'alert-rule-edit', text: 'Edit rule' }; - } else if (state === 'add') { - return { ...defaultPageNav, id: 'alert-rule-add', text: 'Add rule' }; +// sadly we only get the "type" when a new rule is being created, when editing an existing recording rule we can't actually know it from the URL +const getPageNav = (identifier?: RuleIdentifier, type?: 'recording' | 'alerting') => { + if (type === 'recording') { + if (identifier) { + // this branch should never trigger actually, the type param isn't used when editing rules + return { ...defaultPageNav, id: 'alert-rule-edit', text: 'Edit recording rule' }; + } else { + return { ...defaultPageNav, id: 'alert-rule-add', text: 'New recording rule' }; + } + } + + if (identifier) { + // keep this one ambiguous, don't mentiond a specific alert type here + return { ...defaultPageNav, id: 'alert-rule-edit', text: 'Edit rule' }; + } else { + return { ...defaultPageNav, id: 'alert-rule-add', text: 'New alert rule' }; } - return undefined; }; const RuleEditor = ({ match }: RuleEditorProps) => { const dispatch = useDispatch(); const [searchParams] = useURLSearchParams(); - const { id } = match.params; + const { id, type } = match.params; const identifier = ruleId.tryParse(id, true); const copyFromId = searchParams.get('copyFrom') ?? undefined; @@ -75,7 +86,7 @@ const RuleEditor = ({ match }: RuleEditorProps) => { }, [canCreateCloudRules, canCreateGrafanaRules, canEditRules, copyFromIdentifier, id, identifier, loading]); return ( - + {getContent()} ); diff --git a/public/app/features/alerting/unified/RuleEditorCloudOnlyAllowed.test.tsx b/public/app/features/alerting/unified/RuleEditorCloudOnlyAllowed.test.tsx index dc52fb645ae..61e2e3d576e 100644 --- a/public/app/features/alerting/unified/RuleEditorCloudOnlyAllowed.test.tsx +++ b/public/app/features/alerting/unified/RuleEditorCloudOnlyAllowed.test.tsx @@ -186,7 +186,7 @@ describe('RuleEditor cloud: checking editable data sources', () => { await ui.inputs.name.find(); - const switchToCloudButton = screen.getByText('Switch to data source-managed alert rule'); + const switchToCloudButton = screen.getByText('Data source-managed'); expect(switchToCloudButton).toBeInTheDocument(); await userEvent.click(switchToCloudButton); diff --git a/public/app/features/alerting/unified/RuleEditorCloudRules.test.tsx b/public/app/features/alerting/unified/RuleEditorCloudRules.test.tsx index 4a6960d35a0..323322d06c3 100644 --- a/public/app/features/alerting/unified/RuleEditorCloudRules.test.tsx +++ b/public/app/features/alerting/unified/RuleEditorCloudRules.test.tsx @@ -13,6 +13,12 @@ import { fetchRulerRules, fetchRulerRulesGroup, fetchRulerRulesNamespace, setRul import { ExpressionEditorProps } from './components/rule-editor/ExpressionEditor'; import { mockApi, mockFeatureDiscoveryApi, setupMswServer } from './mockApi'; import { disableRBAC, mockDataSource } from './mocks'; +import { + defaultAlertmanagerChoiceResponse, + emptyExternalAlertmanagersResponse, + mockAlertmanagerChoiceResponse, + mockAlertmanagersResponse, +} from './mocks/alertmanagerApi'; import { fetchRulerRulesIfNotFetchedYet } from './state/actions'; import { setupDataSources } from './testSetup/datasources'; import { buildInfoResponse } from './testSetup/featureDiscovery'; @@ -48,6 +54,8 @@ setupDataSources(dataSources.default); const server = setupMswServer(); mockFeatureDiscoveryApi(server).discoverDsFeatures(dataSources.default, buildInfoResponse.mimir); +mockAlertmanagerChoiceResponse(server, defaultAlertmanagerChoiceResponse); +mockAlertmanagersResponse(server, emptyExternalAlertmanagersResponse); mockApi(server).eval({ results: {} }); // these tests are rather slow because we have to wait for various API calls and mocks to be called @@ -110,10 +118,11 @@ describe('RuleEditor cloud', () => { expect(removeExpressionsButtons).toHaveLength(2); // Needs to wait for featrue discovery API call to finish - Check if ruler enabled - await waitFor(() => expect(screen.getByText('Switch to data source-managed alert rule')).toBeInTheDocument()); + await waitFor(() => expect(screen.getByText('Data source-managed')).toBeInTheDocument()); - const switchToCloudButton = screen.getByText('Switch to data source-managed alert rule'); + const switchToCloudButton = screen.getByText('Data source-managed'); expect(switchToCloudButton).toBeInTheDocument(); + expect(switchToCloudButton).not.toBeDisabled(); await user.click(switchToCloudButton); diff --git a/public/app/features/alerting/unified/components/CollapseToggle.tsx b/public/app/features/alerting/unified/components/CollapseToggle.tsx index f3b15669694..b11e871afa4 100644 --- a/public/app/features/alerting/unified/components/CollapseToggle.tsx +++ b/public/app/features/alerting/unified/components/CollapseToggle.tsx @@ -1,8 +1,6 @@ -import { css, cx } from '@emotion/css'; import React, { HTMLAttributes } from 'react'; -import { GrafanaTheme2 } from '@grafana/data'; -import { IconSize, useStyles2, Button } from '@grafana/ui'; +import { IconSize, Button } from '@grafana/ui'; interface Props extends HTMLAttributes { isCollapsed: boolean; @@ -23,8 +21,6 @@ export const CollapseToggle = ({ size = 'xl', ...restOfProps }: Props) => { - const styles = useStyles2(getStyles); - return ( ); }; - -export const getStyles = (theme: GrafanaTheme2) => ({ - expandButton: css` - margin-right: ${theme.spacing(1)}; - `, -}); 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 524dd17bc0b..d9ec536ee66 100644 --- a/public/app/features/alerting/unified/components/rule-editor/AlertRuleForm.tsx +++ b/public/app/features/alerting/unified/components/rule-editor/AlertRuleForm.tsx @@ -5,8 +5,19 @@ import { DeepMap, FieldError, FormProvider, useForm, useFormContext, UseFormWatc import { Link, useParams } from 'react-router-dom'; import { GrafanaTheme2 } from '@grafana/data'; +import { Stack } from '@grafana/experimental'; import { config, logInfo } from '@grafana/runtime'; -import { Button, ConfirmModal, CustomScrollbar, Field, HorizontalGroup, Input, Spinner, useStyles2 } from '@grafana/ui'; +import { + Button, + ConfirmModal, + CustomScrollbar, + Field, + HorizontalGroup, + Input, + Spinner, + Text, + useStyles2, +} from '@grafana/ui'; import { AppChromeUpdate } from 'app/core/components/AppChrome/AppChromeUpdate'; import { useAppNotification } from 'app/core/copy/appNotification'; import { contextSrv } from 'app/core/core'; @@ -48,7 +59,6 @@ const recordingRuleNameValidationPattern = { }; const AlertRuleNameInput = () => { - const styles = useStyles2(getStyles); const { register, watch, @@ -56,22 +66,29 @@ const AlertRuleNameInput = () => { } = useFormContext(); const ruleFormType = watch('type'); + const entityName = ruleFormType === RuleFormType.cloudRecording ? 'recording rule' : 'alert rule'; + return ( - - + + {/* sigh language rules – we should use translations ideally but for now we deal with "a" and "an" */} + Enter {entityName === 'alert rule' ? 'an' : 'a'} {entityName} name to identify your alert. + + } + > + @@ -254,7 +271,7 @@ export const AlertRuleForm = ({ existing, prefill }: Props) => {
e.preventDefault()} className={styles.form}>
-
+ {/* Step 1 */} {/* Step 2 */} @@ -282,7 +299,7 @@ export const AlertRuleForm = ({ existing, prefill }: Props) => { )} -
+
@@ -369,29 +386,15 @@ const getStyles = (theme: GrafanaTheme2) => { display: flex; flex-direction: column; `, - contentInner: css` - flex: 1; - padding: ${theme.spacing(2)}; - `, contentOuter: css` background: ${theme.colors.background.primary}; - border: 1px solid ${theme.colors.border.weak}; - border-radius: ${theme.shape.radius.default}; overflow: hidden; flex: 1; - margin-top: ${theme.spacing(1)}; `, flexRow: css` display: flex; flex-direction: row; justify-content: flex-start; `, - formInput: css` - width: 275px; - - & + & { - margin-left: ${theme.spacing(3)}; - } - `, }; }; diff --git a/public/app/features/alerting/unified/components/rule-editor/AnnotationHeaderField.tsx b/public/app/features/alerting/unified/components/rule-editor/AnnotationHeaderField.tsx index 99d7b2fd71a..4c996df2261 100644 --- a/public/app/features/alerting/unified/components/rule-editor/AnnotationHeaderField.tsx +++ b/public/app/features/alerting/unified/components/rule-editor/AnnotationHeaderField.tsx @@ -1,9 +1,8 @@ -import { css } from '@emotion/css'; import React from 'react'; import { FieldArrayWithId, useFormContext } from 'react-hook-form'; -import { GrafanaTheme2 } from '@grafana/data'; -import { InputControl, useStyles2 } from '@grafana/ui'; +import { Stack } from '@grafana/experimental'; +import { InputControl, Text } from '@grafana/ui'; import { RuleFormValues } from '../../types/rule-form'; import { Annotation, annotationDescriptions, annotationLabels } from '../../utils/constants'; @@ -21,58 +20,49 @@ const AnnotationHeaderField = ({ annotation: Annotation; index: number; }) => { - const styles = useStyles2(getStyles); const { control } = useFormContext(); + return ( -
-
+ + {annotationDescriptions[annotation]} + + ); }; -const getStyles = (theme: GrafanaTheme2) => ({ - annotationTitle: css` - color: ${theme.colors.text.primary}; - margin-bottom: 3px; - `, - - annotationContainer: css` - margin-top: 5px; - `, - - annotationDescription: css` - color: ${theme.colors.text.secondary}; - `, -}); - export default AnnotationHeaderField; diff --git a/public/app/features/alerting/unified/components/rule-editor/AnnotationsStep.tsx b/public/app/features/alerting/unified/components/rule-editor/AnnotationsStep.tsx index c9fbc5b7281..67406197e6c 100644 --- a/public/app/features/alerting/unified/components/rule-editor/AnnotationsStep.tsx +++ b/public/app/features/alerting/unified/components/rule-editor/AnnotationsStep.tsx @@ -6,7 +6,7 @@ import { useToggle } from 'react-use'; import { GrafanaTheme2 } from '@grafana/data'; import { Stack } from '@grafana/experimental'; -import { Button, Field, Input, TextArea, useStyles2 } from '@grafana/ui'; +import { Button, Field, Input, Text, TextArea, useStyles2 } from '@grafana/ui'; import { DashboardDataDTO } from 'app/types'; import { dashboardApi } from '../../api/dashboardApi'; @@ -97,10 +97,12 @@ const AnnotationsStep = () => { 'https://grafana.com/docs/grafana/latest/alerting/fundamentals/annotation-label/variables-label-annotation'; return ( - - Add annotations to provide more context in your alert notifications. + + + Add annotations to provide more context in your alert notifications. + { } return ( - -
+ + {fields.map((annotationField, index: number) => { const isUrl = annotations[index]?.key?.toLocaleLowerCase().endsWith('url'); const ValueInputComponent = isUrl ? Input : TextArea; @@ -206,7 +208,7 @@ const AnnotationsStep = () => { onDismiss={() => setShowPanelSelector(false)} /> )} -
+ ); }; @@ -223,11 +225,6 @@ const getStyles = (theme: GrafanaTheme2) => ({ gap: ${theme.spacing(1)}; display: flex; `, - flexColumn: css` - display: flex; - flex-direction: column; - margin-top: ${theme.spacing(2)}; - `, field: css` margin-bottom: ${theme.spacing(0.5)}; `, diff --git a/public/app/features/alerting/unified/components/rule-editor/CloudEvaluationBehavior.tsx b/public/app/features/alerting/unified/components/rule-editor/CloudEvaluationBehavior.tsx index d7cb7278d62..467150f24ff 100644 --- a/public/app/features/alerting/unified/components/rule-editor/CloudEvaluationBehavior.tsx +++ b/public/app/features/alerting/unified/components/rule-editor/CloudEvaluationBehavior.tsx @@ -25,7 +25,7 @@ export const CloudEvaluationBehavior = () => { const dataSourceName = watch('dataSourceName'); return ( - + -
+
{ - {(!isCreatingFolder && ( - ( - { - field.onChange({ title, uid }); - resetGroup(); + + {(!isCreatingFolder && ( + <> + ( +
+ { + field.onChange({ title, uid }); + resetGroup(); + }} + /> +
+ )} + name="folder" + rules={{ + required: { value: true, message: 'Select a folder' }, + validate: { + pathSeparator: (folder: Folder) => checkForPathSeparator(folder.title), + }, }} /> - )} - name="folder" - rules={{ - required: { value: true, message: 'Select a folder' }, - validate: { - pathSeparator: (folder: Folder) => checkForPathSeparator(folder.title), - }, - }} - /> - )) ||
Creating new folder...
} + or + + + )) ||
Creating new folder...
} + } - -
- or - -
{isCreatingFolder && ( setIsCreatingFolder(false)} /> )}
-
+
- ( - { - field.onChange(group.label ?? ''); - }} - isLoading={loading} - invalid={Boolean(folder) && !group && Boolean(fieldState.error)} - loadOptions={debouncedSearch} - cacheOptions - loadingMessage={'Loading groups...'} - defaultValue={defaultGroupValue} - defaultOptions={groupOptions} - getOptionLabel={(option: SelectableValue) => ( -
- {option.label} - {/* making the assumption here that it's provisioned when it's disabled, should probably change this */} - {option.isDisabled && ( - <> - {' '} - - + + ( +
+ { + field.onChange(group.label ?? ''); + }} + isLoading={loading} + invalid={Boolean(folder) && !group && Boolean(fieldState.error)} + loadOptions={debouncedSearch} + cacheOptions + loadingMessage={'Loading groups...'} + defaultValue={defaultGroupValue} + defaultOptions={groupOptions} + getOptionLabel={(option: SelectableValue) => ( +
+ {option.label} + {/* making the assumption here that it's provisioned when it's disabled, should probably change this */} + {option.isDisabled && ( + <> + {' '} + + + )} +
)} -
- )} - placeholder={'Select an evaluation group...'} - /> - )} - name="group" - control={control} - rules={{ - required: { value: true, message: 'Must enter a group name' }, - validate: { - pathSeparator: (group_: string) => checkForPathSeparator(group_), - }, - }} - /> + placeholder={'Select an evaluation group...'} + /> +
+ )} + name="group" + control={control} + rules={{ + required: { value: true, message: 'Must enter a group name' }, + validate: { + pathSeparator: (group_: string) => checkForPathSeparator(group_), + }, + }} + /> + or + + - -
- or - -
{isCreatingEvaluationGroup && ( ({ container: css` - margin-top: ${theme.spacing(1)}; display: flex; flex-direction: column; align-items: baseline; max-width: ${theme.breakpoints.values.lg}px; justify-content: space-between; `, - evaluationGroupsContainer: css` - width: 100%; - display: flex; - flex-direction: row; - gap: ${theme.spacing(2)}; - `, - - addButton: css` - display: flex; - direction: row; - gap: ${theme.spacing(2)}; - line-height: 2; - margin-top: 35px; - `, formInput: css` - max-width: ${theme.breakpoints.values.sm}px; flex-grow: 1; - - label { - width: ${theme.breakpoints.values.sm}px; - } `, - modal: css` width: ${theme.breakpoints.values.sm}px; `, - modalTitle: css` color: ${theme.colors.text.secondary}; margin-bottom: ${theme.spacing(2)}; diff --git a/public/app/features/alerting/unified/components/rule-editor/GrafanaEvaluationBehavior.tsx b/public/app/features/alerting/unified/components/rule-editor/GrafanaEvaluationBehavior.tsx index 0d40e3de5b3..8039dcb468a 100644 --- a/public/app/features/alerting/unified/components/rule-editor/GrafanaEvaluationBehavior.tsx +++ b/public/app/features/alerting/unified/components/rule-editor/GrafanaEvaluationBehavior.tsx @@ -4,7 +4,7 @@ import { RegisterOptions, useFormContext } from 'react-hook-form'; import { GrafanaTheme2, SelectableValue } from '@grafana/data'; import { Stack } from '@grafana/experimental'; -import { Field, Icon, IconButton, Input, InputControl, Label, Switch, Tooltip, useStyles2 } from '@grafana/ui'; +import { Field, Icon, IconButton, Input, InputControl, Label, Switch, Text, Tooltip, useStyles2 } from '@grafana/ui'; import { CombinedRuleGroup, CombinedRuleNamespace } from '../../../../../types/unified-alerting'; import { logInfo, LogMessages } from '../../Analytics'; @@ -185,11 +185,13 @@ function ForInput({ evaluateEvery }: { evaluateEvery: string }) { } function getDescription() { - const textToRender = 'Define how the alert rule is evaluated.'; const docsLink = 'https://grafana.com/docs/grafana/latest/alerting/fundamentals/alert-rules/rule-evaluation/'; + return ( - - {`${textToRender}`} + + + Define how the alert rule is evaluated. + + @@ -252,7 +254,6 @@ export function GrafanaEvaluationBehavior({ isCollapsed={!showErrorHandling} onToggle={(collapsed) => setShowErrorHandling(!collapsed)} text="Configure no data and error handling" - className={styles.collapseToggle} /> {showErrorHandling && ( <> @@ -296,9 +297,6 @@ const getStyles = (theme: GrafanaTheme2) => ({ inlineField: css` margin-bottom: 0; `, - collapseToggle: css` - margin: ${theme.spacing(2, 0, 2, -1)}; - `, evaluateLabel: css` margin-right: ${theme.spacing(1)}; `, diff --git a/public/app/features/alerting/unified/components/rule-editor/LabelsField.tsx b/public/app/features/alerting/unified/components/rule-editor/LabelsField.tsx index 56e56d43053..034b1e69f96 100644 --- a/public/app/features/alerting/unified/components/rule-editor/LabelsField.tsx +++ b/public/app/features/alerting/unified/components/rule-editor/LabelsField.tsx @@ -5,7 +5,18 @@ import { FieldArrayMethodProps, useFieldArray, useFormContext } from 'react-hook import { GrafanaTheme2, SelectableValue } from '@grafana/data'; import { Stack } from '@grafana/experimental'; -import { Button, Field, InlineLabel, Label, useStyles2, Tooltip, Icon, Input, LoadingPlaceholder } from '@grafana/ui'; +import { + Button, + Field, + InlineLabel, + Label, + useStyles2, + Text, + Tooltip, + Icon, + Input, + LoadingPlaceholder, +} from '@grafana/ui'; import { useDispatch } from 'app/types'; import { RulerRuleGroupDTO } from 'app/types/unified-alerting-dto'; @@ -129,7 +140,7 @@ const LabelsWithSuggestions: FC<{ dataSourceName: string }> = ({ dataSourceName <> {loading && } {!loading && ( - <> + {fields.map((field, index) => { return (
@@ -182,7 +193,7 @@ const LabelsWithSuggestions: FC<{ dataSourceName: string }> = ({ dataSourceName ); })} - + )} ); @@ -245,14 +256,16 @@ const LabelsWithoutSuggestions: FC = () => { ); }; -const LabelsField: FC = ({ className, dataSourceName }) => { +const LabelsField: FC = ({ dataSourceName }) => { const styles = useStyles2(getStyles); return ( -
-