diff --git a/public/app/features/alerting/unified/AmRoutes.test.tsx b/public/app/features/alerting/unified/AmRoutes.test.tsx index c4a30208147..b70549af1b9 100644 --- a/public/app/features/alerting/unified/AmRoutes.test.tsx +++ b/public/app/features/alerting/unified/AmRoutes.test.tsx @@ -302,9 +302,9 @@ describe('AmRoutes', () => { // configure timing intervals await userEvent.click(byText('Timing options').get(rootRouteContainer)); - await updateTiming(ui.groupWaitContainer.get(), '1', 'Minutes'); - await updateTiming(ui.groupIntervalContainer.get(), '4', 'Minutes'); - await updateTiming(ui.groupRepeatContainer.get(), '5', 'Hours'); + await updateTiming(ui.groupWaitContainer.get(), '1m'); + await updateTiming(ui.groupIntervalContainer.get(), '4m'); + await updateTiming(ui.groupRepeatContainer.get(), '5h'); //save await userEvent.click(ui.saveButton.get(rootRouteContainer)); @@ -728,11 +728,9 @@ const clickSelectOption = async (selectElement: HTMLElement, optionText: string) await selectOptionInTest(selectElement, optionText); }; -const updateTiming = async (selectElement: HTMLElement, value: string, timeUnit: string): Promise => { - const input = byRole('textbox').get(selectElement); - const select = byRole('combobox').get(selectElement); +const updateTiming = async (timingInputContainer: HTMLElement, value: string): Promise => { + const input = byRole('textbox').get(timingInputContainer); + await userEvent.clear(input); await userEvent.type(input, value); - await userEvent.click(select); - await selectOptionInTest(selectElement, timeUnit); }; diff --git a/public/app/features/alerting/unified/components/amroutes/AmRootRouteForm.test.tsx b/public/app/features/alerting/unified/components/amroutes/AmRootRouteForm.test.tsx new file mode 100644 index 00000000000..22c93550371 --- /dev/null +++ b/public/app/features/alerting/unified/components/amroutes/AmRootRouteForm.test.tsx @@ -0,0 +1,137 @@ +import { render } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import { noop } from 'lodash'; +import React from 'react'; +import { Provider } from 'react-redux'; +import { MemoryRouter } from 'react-router-dom'; +import { byRole } from 'testing-library-selector'; + +import { Route } from 'app/plugins/datasource/alertmanager/types'; +import { configureStore } from 'app/store/configureStore'; + +import * as grafanaApp from '../../components/receivers/grafanaAppReceivers/grafanaApp'; +import { FormAmRoute } from '../../types/amroutes'; +import { amRouteToFormAmRoute } from '../../utils/amroutes'; +import { GRAFANA_RULES_SOURCE_NAME } from '../../utils/datasource'; +import { AmRouteReceiver } from '../receivers/grafanaAppReceivers/types'; + +import { AmRootRouteForm } from './AmRootRouteForm'; + +const ui = { + error: byRole('alert'), + timingOptionsBtn: byRole('button', { name: /Timing options/ }), + submitBtn: byRole('button', { name: /Save/ }), + groupWaitInput: byRole('textbox', { name: /Group wait/ }), + groupIntervalInput: byRole('textbox', { name: /Group interval/ }), + repeatIntervalInput: byRole('textbox', { name: /Repeat interval/ }), +}; + +const useGetGrafanaReceiverTypeCheckerMock = jest.spyOn(grafanaApp, 'useGetGrafanaReceiverTypeChecker'); +useGetGrafanaReceiverTypeCheckerMock.mockReturnValue(() => undefined); + +// TODO Default and Notification policy form should be unified so we don't need to maintain two almost identical forms +describe('AmRootRouteForm', function () { + describe('Timing options', function () { + it('should render prometheus duration strings in form inputs', async function () { + const user = userEvent.setup(); + + renderRouteForm({ + group_wait: '1m30s', + group_interval: '2d4h30m35s', + repeat_interval: '1w2d6h', + }); + + await user.click(ui.timingOptionsBtn.get()); + expect(ui.groupWaitInput.get()).toHaveValue('1m30s'); + expect(ui.groupIntervalInput.get()).toHaveValue('2d4h30m35s'); + expect(ui.repeatIntervalInput.get()).toHaveValue('1w2d6h'); + }); + it('should allow submitting valid prometheus duration strings', async function () { + const user = userEvent.setup(); + + const onSubmit = jest.fn(); + renderRouteForm( + { + receiver: 'default', + }, + [{ value: 'default', label: 'Default' }], + onSubmit + ); + + await user.click(ui.timingOptionsBtn.get()); + + await user.type(ui.groupWaitInput.get(), '5m25s'); + await user.type(ui.groupIntervalInput.get(), '35m40s'); + await user.type(ui.repeatIntervalInput.get(), '4h30m'); + + await user.click(ui.submitBtn.get()); + + expect(ui.error.queryAll()).toHaveLength(0); + expect(onSubmit).toHaveBeenCalledWith( + expect.objectContaining>({ + groupWaitValue: '5m25s', + groupIntervalValue: '35m40s', + repeatIntervalValue: '4h30m', + }), + expect.anything() + ); + }); + }); + + it('should allow resetting existing timing options', async function () { + const user = userEvent.setup(); + + const onSubmit = jest.fn(); + renderRouteForm( + { + receiver: 'default', + group_wait: '1m30s', + group_interval: '2d4h30m35s', + repeat_interval: '1w2d6h', + }, + [{ value: 'default', label: 'Default' }], + onSubmit + ); + + await user.click(ui.timingOptionsBtn.get()); + await user.clear(ui.groupWaitInput.get()); + await user.clear(ui.groupIntervalInput.get()); + await user.clear(ui.repeatIntervalInput.get()); + + await user.click(ui.submitBtn.get()); + + expect(ui.error.queryAll()).toHaveLength(0); + expect(onSubmit).toHaveBeenCalledWith( + expect.objectContaining>({ + groupWaitValue: '', + groupIntervalValue: '', + repeatIntervalValue: '', + }), + expect.anything() + ); + }); +}); + +function renderRouteForm( + route: Route, + receivers: AmRouteReceiver[] = [], + onSubmit: (route: Partial) => void = noop +) { + const Wrapper = ({ children }: { children: React.ReactNode }) => ( + + {children} + + ); + const [formAmRoute] = amRouteToFormAmRoute(route); + + render( + , + { wrapper: Wrapper } + ); +} diff --git a/public/app/features/alerting/unified/components/amroutes/AmRootRouteForm.tsx b/public/app/features/alerting/unified/components/amroutes/AmRootRouteForm.tsx index 6768061c0cb..ecc840b5886 100644 --- a/public/app/features/alerting/unified/components/amroutes/AmRootRouteForm.tsx +++ b/public/app/features/alerting/unified/components/amroutes/AmRootRouteForm.tsx @@ -1,22 +1,22 @@ -import { cx } from '@emotion/css'; import React, { FC, useState } from 'react'; -import { Button, Collapse, Field, Form, Input, InputControl, Link, MultiSelect, Select, useStyles2 } from '@grafana/ui'; +import { Button, Collapse, Field, Form, InputControl, Link, MultiSelect, Select, useStyles2 } from '@grafana/ui'; import { FormAmRoute } from '../../types/amroutes'; import { + commonGroupByOptions, mapMultiSelectValueToStrings, mapSelectValueToString, - optionalPositiveInteger, - stringToSelectableValue, + promDurationValidator, stringsToSelectableValues, - commonGroupByOptions, + stringToSelectableValue, } from '../../utils/amroutes'; import { makeAMLink } from '../../utils/misc'; -import { timeOptions } from '../../utils/time'; import { AmRouteReceiver } from '../receivers/grafanaAppReceivers/types'; +import { PromDurationInput } from './PromDurationInput'; import { getFormStyles } from './formStyles'; +import { TIMING_OPTIONS_DEFAULTS } from './timingOptions'; export interface AmRootRouteFormProps { alertManagerSourceName: string; @@ -39,7 +39,7 @@ export const AmRootRouteForm: FC = ({ return (
- {({ control, errors, setValue }) => ( + {({ register, control, errors, setValue }) => ( <> <> @@ -102,112 +102,50 @@ export const AmRootRouteForm: FC = ({ label="Timing options" onToggle={setIsTimingOptionsExpanded} > - - <> -
- ( - - )} - control={control} - name="groupWaitValue" - rules={{ - validate: optionalPositiveInteger, - }} - /> - ( - - )} - control={control} - name="groupIntervalValue" - rules={{ - validate: optionalPositiveInteger, - }} - /> - ( - - )} - control={control} - name="repeatIntervalValue" - rules={{ - validate: optionalPositiveInteger, - }} - /> - ( - @@ -108,7 +107,7 @@ export const AmRoutesExpandedForm: FC = ({ onCancel, )} defaultValue={field.operator} control={control} - name={`${localPath}.operator` as const} + name={`object_matchers.${index}.operator`} rules={{ required: { value: true, message: 'Required.' } }} /> @@ -118,7 +117,7 @@ export const AmRoutesExpandedForm: FC = ({ onCancel, error={errors.object_matchers?.[index]?.value?.message} > @@ -209,38 +208,11 @@ export const AmRoutesExpandedForm: FC = ({ onCancel, invalid={!!errors.groupWaitValue} error={errors.groupWaitValue?.message} > - <> -
- ( - - )} - control={control} - name="groupWaitValue" - rules={{ - validate: optionalPositiveInteger, - }} - /> - ( - - )} - control={control} - name="groupIntervalValue" - rules={{ - validate: optionalPositiveInteger, - }} - /> - ( - - )} - control={control} - name="repeatIntervalValue" - rules={{ - validate: optionalPositiveInteger, - }} - /> - ( - } disabled={false}> + + + } + {...props} + ref={ref} + /> + ); + } +); + +PromDurationInput.displayName = 'PromDurationInput'; diff --git a/public/app/features/alerting/unified/components/amroutes/formStyles.ts b/public/app/features/alerting/unified/components/amroutes/formStyles.ts index f61a525c080..7cb793af79d 100644 --- a/public/app/features/alerting/unified/components/amroutes/formStyles.ts +++ b/public/app/features/alerting/unified/components/amroutes/formStyles.ts @@ -16,11 +16,11 @@ export const getFormStyles = (theme: GrafanaTheme2) => { input: css` flex: 1; `, - timingContainer: css` - max-width: ${theme.spacing(33)}; + promDurationInput: css` + max-width: ${theme.spacing(32)}; `, - smallInput: css` - width: ${theme.spacing(6.5)}; + timingFormContainer: css` + padding: ${theme.spacing(1)}; `, linkText: css` text-decoration: underline; diff --git a/public/app/features/alerting/unified/components/amroutes/timingOptions.ts b/public/app/features/alerting/unified/components/amroutes/timingOptions.ts new file mode 100644 index 00000000000..62376d582ee --- /dev/null +++ b/public/app/features/alerting/unified/components/amroutes/timingOptions.ts @@ -0,0 +1,11 @@ +export type TimingOptions = { + group_wait?: string; + group_interval?: string; + repeat_interval?: string; +}; + +export const TIMING_OPTIONS_DEFAULTS: Required = { + group_wait: '30s', + group_interval: '5m', + repeat_interval: '4h', +}; diff --git a/public/app/features/alerting/unified/types/amroutes.ts b/public/app/features/alerting/unified/types/amroutes.ts index 59c70e68983..f412a0e3c7f 100644 --- a/public/app/features/alerting/unified/types/amroutes.ts +++ b/public/app/features/alerting/unified/types/amroutes.ts @@ -9,11 +9,8 @@ export interface FormAmRoute { groupBy: string[]; overrideTimings: boolean; groupWaitValue: string; - groupWaitValueType: string; groupIntervalValue: string; - groupIntervalValueType: string; repeatIntervalValue: string; - repeatIntervalValueType: string; muteTimeIntervals: string[]; routes: FormAmRoute[]; } diff --git a/public/app/features/alerting/unified/utils/amroutes.ts b/public/app/features/alerting/unified/utils/amroutes.ts index 57cde530c32..94e4548a74a 100644 --- a/public/app/features/alerting/unified/utils/amroutes.ts +++ b/public/app/features/alerting/unified/utils/amroutes.ts @@ -1,5 +1,4 @@ import { isUndefined, omitBy } from 'lodash'; -import { Validate } from 'react-hook-form'; import { SelectableValue } from '@grafana/data'; import { MatcherOperator, Route } from 'app/plugins/datasource/alertmanager/types'; @@ -9,9 +8,7 @@ import { MatcherFieldValue } from '../types/silence-form'; import { matcherToMatcherField, parseMatcher } from './alertmanager'; import { GRAFANA_RULES_SOURCE_NAME } from './datasource'; -import { parseInterval, timeOptions } from './time'; - -const defaultValueAndType: [string, string] = ['', '']; +import { isValidPrometheusDuration } from './time'; const matchersToArrayFieldMatchers = ( matchers: Record | undefined, @@ -29,25 +26,6 @@ const matchersToArrayFieldMatchers = ( [] as MatcherFieldValue[] ); -const intervalToValueAndType = ( - strValue: string | undefined, - defaultValue?: typeof defaultValueAndType -): [string, string] => { - if (!strValue) { - return defaultValue ?? defaultValueAndType; - } - - const [value, valueType] = strValue ? parseInterval(strValue) : [undefined, undefined]; - - const timeOption = timeOptions.find((opt) => opt.value === valueType); - - if (!value || !timeOption) { - return defaultValueAndType; - } - - return [String(value), timeOption.value]; -}; - const selectableValueToString = (selectableValue: SelectableValue): string => selectableValue.value!; const selectableValuesToStrings = (arr: Array> | undefined): string[] => @@ -79,11 +57,8 @@ export const emptyRoute: FormAmRoute = { receiver: '', overrideTimings: false, groupWaitValue: '', - groupWaitValueType: timeOptions[0].value, groupIntervalValue: '', - groupIntervalValueType: timeOptions[0].value, repeatIntervalValue: '', - repeatIntervalValueType: timeOptions[0].value, muteTimeIntervals: [], }; @@ -117,10 +92,6 @@ export const amRouteToFormAmRoute = (route: Route | undefined): [FormAmRoute, Re (matcher) => ({ name: matcher[0], operator: matcher[1], value: matcher[2] } as MatcherFieldValue) ) ?? []; - const [groupWaitValue, groupWaitValueType] = intervalToValueAndType(route.group_wait, ['', 's']); - const [groupIntervalValue, groupIntervalValueType] = intervalToValueAndType(route.group_interval, ['', 'm']); - const [repeatIntervalValue, repeatIntervalValueType] = intervalToValueAndType(route.repeat_interval, ['', 'h']); - return [ { id, @@ -133,13 +104,10 @@ export const amRouteToFormAmRoute = (route: Route | undefined): [FormAmRoute, Re receiver: route.receiver ?? '', overrideGrouping: Array.isArray(route.group_by) && route.group_by.length !== 0, groupBy: route.group_by ?? [], - overrideTimings: [groupWaitValue, groupIntervalValue, repeatIntervalValue].some(Boolean), - groupWaitValue, - groupWaitValueType, - groupIntervalValue, - groupIntervalValueType, - repeatIntervalValue, - repeatIntervalValueType, + overrideTimings: [route.group_wait, route.group_interval, route.repeat_interval].some(Boolean), + groupWaitValue: route.group_wait ?? '', + groupIntervalValue: route.group_interval ?? '', + repeatIntervalValue: route.repeat_interval ?? '', routes: formRoutes, muteTimeIntervals: route.mute_time_intervals ?? [], }, @@ -154,28 +122,19 @@ export const formAmRouteToAmRoute = ( ): Route => { const existing: Route | undefined = id2ExistingRoute[formAmRoute.id]; - const { - overrideGrouping, - groupBy, - overrideTimings, - groupWaitValue, - groupWaitValueType, - groupIntervalValue, - groupIntervalValueType, - repeatIntervalValue, - repeatIntervalValueType, - } = formAmRoute; + const { overrideGrouping, groupBy, overrideTimings, groupWaitValue, groupIntervalValue, repeatIntervalValue } = + formAmRoute; const group_by = overrideGrouping && groupBy ? groupBy : []; const overrideGroupWait = overrideTimings && groupWaitValue; - const group_wait = overrideGroupWait ? `${groupWaitValue}${groupWaitValueType}` : undefined; + const group_wait = overrideGroupWait ? groupWaitValue : undefined; const overrideGroupInterval = overrideTimings && groupIntervalValue; - const group_interval = overrideGroupInterval ? `${groupIntervalValue}${groupIntervalValueType}` : undefined; + const group_interval = overrideGroupInterval ? groupIntervalValue : undefined; const overrideRepeatInterval = overrideTimings && repeatIntervalValue; - const repeat_interval = overrideRepeatInterval ? `${repeatIntervalValue}${repeatIntervalValueType}` : undefined; + const repeat_interval = overrideRepeatInterval ? repeatIntervalValue : undefined; const amRoute: Route = { ...(existing ?? {}), @@ -235,10 +194,10 @@ export const mapMultiSelectValueToStrings = ( return selectableValuesToStrings(selectableValues); }; -export const optionalPositiveInteger: Validate = (value) => { - if (!value) { - return undefined; +export function promDurationValidator(duration: string) { + if (duration.length === 0) { + return true; } - return !/^\d+$/.test(value) ? 'Must be a positive integer.' : undefined; -}; + return isValidPrometheusDuration(duration) || 'Invalid duration format. Must be {number}{time_unit}'; +} diff --git a/public/app/features/alerting/unified/utils/time.ts b/public/app/features/alerting/unified/utils/time.ts index 491c5b790ee..9a96fa1f4aa 100644 --- a/public/app/features/alerting/unified/utils/time.ts +++ b/public/app/features/alerting/unified/utils/time.ts @@ -1,4 +1,3 @@ -import { durationToMilliseconds, parseDuration } from '@grafana/data'; import { describeInterval } from '@grafana/data/src/datetime/rangeutil'; import { TimeOptions } from '../types/time'; @@ -28,10 +27,6 @@ export const timeOptions = Object.entries(TimeOptions).map(([key, value]) => ({ value: value, })); -export function parseDurationToMilliseconds(duration: string) { - return durationToMilliseconds(parseDuration(duration)); -} - export function isValidPrometheusDuration(duration: string): boolean { try { parsePrometheusDuration(duration);