From f94e07f5a4d32f3b08960dcbab4070c449efaaaf Mon Sep 17 00:00:00 2001 From: Gilles De Mey Date: Thu, 8 Jun 2023 13:01:40 +0200 Subject: [PATCH] Alerting: Fix notification policies inheritance algorithm (#69304) --- .../alerting/unified/components/Strong.tsx | 2 +- .../EditNotificationPolicyForm.tsx | 41 ++++-- .../notification-policies/Policy.test.tsx | 6 +- .../notification-policies/Policy.tsx | 122 ++++++++--------- .../alerting/unified/types/amroutes.ts | 2 +- .../alerting/unified/utils/amroutes.test.ts | 19 ++- .../alerting/unified/utils/amroutes.ts | 15 ++- .../utils/notification-policies.test.ts | 126 +++++++++++++++++- .../unified/utils/notification-policies.ts | 53 +++++++- 9 files changed, 296 insertions(+), 90 deletions(-) diff --git a/public/app/features/alerting/unified/components/Strong.tsx b/public/app/features/alerting/unified/components/Strong.tsx index 99c6bcc42fc..f1adcebceb9 100644 --- a/public/app/features/alerting/unified/components/Strong.tsx +++ b/public/app/features/alerting/unified/components/Strong.tsx @@ -6,7 +6,7 @@ interface Props {} const Strong = ({ children }: React.PropsWithChildren) => { const theme = useTheme2(); - return {children}; + return {children}; }; export { Strong }; diff --git a/public/app/features/alerting/unified/components/notification-policies/EditNotificationPolicyForm.tsx b/public/app/features/alerting/unified/components/notification-policies/EditNotificationPolicyForm.tsx index 5e905fea564..718a47d5593 100644 --- a/public/app/features/alerting/unified/components/notification-policies/EditNotificationPolicyForm.tsx +++ b/public/app/features/alerting/unified/components/notification-policies/EditNotificationPolicyForm.tsx @@ -16,6 +16,7 @@ import { Switch, useStyles2, Badge, + FieldValidationMessage, } from '@grafana/ui'; import { MatcherOperator, RouteWithID } from 'app/plugins/datasource/alertmanager/types'; @@ -186,21 +187,33 @@ export const AmRoutesExpandedForm = ({ description="Group alerts when you receive a notification based on labels. If empty it will be inherited from the parent policy." > ( - { - setGroupByOptions((opts) => [...opts, stringToSelectableValue(opt)]); + rules={{ + validate: (value) => { + if (!value || value.length === 0) { + return 'At least one group by option is required.'; + } + return true; + }, + }} + render={({ field: { onChange, ref, ...field }, fieldState: { error } }) => ( + <> + { + setGroupByOptions((opts) => [...opts, stringToSelectableValue(opt)]); - // @ts-ignore-check: react-hook-form made me do this - setValue('groupBy', [...field.value, opt]); - }} - onChange={(value) => onChange(mapMultiSelectValueToStrings(value))} - options={[...commonGroupByOptions, ...groupByOptions]} - /> + // @ts-ignore-check: react-hook-form made me do this + setValue('groupBy', [...field.value, opt]); + }} + onChange={(value) => onChange(mapMultiSelectValueToStrings(value))} + options={[...commonGroupByOptions, ...groupByOptions]} + /> + {error && {error.message}} + )} control={control} name="groupBy" diff --git a/public/app/features/alerting/unified/components/notification-policies/Policy.test.tsx b/public/app/features/alerting/unified/components/notification-policies/Policy.test.tsx index 4ce4cb999d0..187640ab617 100644 --- a/public/app/features/alerting/unified/components/notification-policies/Policy.test.tsx +++ b/public/app/features/alerting/unified/components/notification-policies/Policy.test.tsx @@ -56,7 +56,7 @@ describe('Policy', () => { expect(within(defaultPolicy).getByText('Default policy')).toBeVisible(); // click "more actions" and check if we can edit and delete - expect(await within(defaultPolicy).getByTestId('more-actions')).toBeInTheDocument(); + expect(within(defaultPolicy).getByTestId('more-actions')).toBeInTheDocument(); await userEvent.click(within(defaultPolicy).getByTestId('more-actions')); // should be editable @@ -102,8 +102,8 @@ describe('Policy', () => { // click "more actions" and check if we can delete await userEvent.click(policy.getByTestId('more-actions')); - expect(await screen.queryByRole('menuitem', { name: 'Edit' })).not.toBeDisabled(); - expect(await screen.queryByRole('menuitem', { name: 'Delete' })).not.toBeDisabled(); + expect(screen.queryByRole('menuitem', { name: 'Edit' })).not.toBeDisabled(); + expect(screen.queryByRole('menuitem', { name: 'Delete' })).not.toBeDisabled(); await userEvent.click(screen.getByRole('menuitem', { name: 'Delete' })); expect(onDeletePolicy).toHaveBeenCalled(); diff --git a/public/app/features/alerting/unified/components/notification-policies/Policy.tsx b/public/app/features/alerting/unified/components/notification-policies/Policy.tsx index 67c3bfa71ab..07ea8cdbf97 100644 --- a/public/app/features/alerting/unified/components/notification-policies/Policy.tsx +++ b/public/app/features/alerting/unified/components/notification-policies/Policy.tsx @@ -1,5 +1,5 @@ import { css } from '@emotion/css'; -import { uniqueId, pick, groupBy, upperFirst, merge, reduce, sumBy } from 'lodash'; +import { uniqueId, groupBy, upperFirst, sumBy, isArray } from 'lodash'; import pluralize from 'pluralize'; import React, { FC, Fragment, ReactNode } from 'react'; import { Link } from 'react-router-dom'; @@ -7,19 +7,15 @@ import { Link } from 'react-router-dom'; import { GrafanaTheme2, IconName } from '@grafana/data'; import { Stack } from '@grafana/experimental'; import { Badge, Button, Dropdown, getTagColorsFromName, Icon, Menu, Tooltip, useStyles2 } from '@grafana/ui'; +import { Span } from '@grafana/ui/src/unstable'; import { contextSrv } from 'app/core/core'; -import { - RouteWithID, - Receiver, - ObjectMatcher, - Route, - AlertmanagerGroup, -} from 'app/plugins/datasource/alertmanager/types'; +import { RouteWithID, Receiver, ObjectMatcher, AlertmanagerGroup } from 'app/plugins/datasource/alertmanager/types'; import { ReceiversState } from 'app/types'; import { getNotificationsPermissions } from '../../utils/access-control'; import { normalizeMatchers } from '../../utils/matchers'; import { createContactPointLink, createMuteTimingLink } from '../../utils/misc'; +import { getInheritedProperties, InhertitableProperties } from '../../utils/notification-policies'; import { HoverCard } from '../HoverCard'; import { Label } from '../Label'; import { MetaText } from '../MetaText'; @@ -29,17 +25,12 @@ import { Strong } from '../Strong'; import { Matchers } from './Matchers'; import { TimingOptions, TIMING_OPTIONS_DEFAULTS } from './timingOptions'; -type InhertitableProperties = Pick< - Route, - 'receiver' | 'group_by' | 'group_wait' | 'group_interval' | 'repeat_interval' | 'mute_time_intervals' ->; - interface PolicyComponentProps { receivers?: Receiver[]; alertGroups?: AlertmanagerGroup[]; contactPointsState?: ReceiversState; readOnly?: boolean; - inheritedProperties?: InhertitableProperties; + inheritedProperties?: Partial; routesMatchingFilters?: RouteWithID[]; // routeAlertGroupsMap?: Map; @@ -79,7 +70,7 @@ const Policy: FC = ({ const contactPoint = currentRoute.receiver; const continueMatching = currentRoute.continue ?? false; - const groupBy = currentRoute.group_by ?? []; + const groupBy = currentRoute.group_by; const muteTimings = currentRoute.mute_time_intervals ?? []; const timingOptions: TimingOptions = { group_wait: currentRoute.group_wait, @@ -107,10 +98,15 @@ const Policy: FC = ({ errors.push(error); }); - const childPolicies = currentRoute.routes ?? []; - const isGrouping = Array.isArray(groupBy) && groupBy.length > 0; const hasInheritedProperties = inheritedProperties && Object.keys(inheritedProperties).length > 0; + const childPolicies = currentRoute.routes ?? []; + + const inheritedGrouping = hasInheritedProperties && inheritedProperties.group_by; + const noGrouping = isArray(groupBy) && groupBy[0] === '...'; + const customGrouping = !noGrouping && isArray(groupBy) && groupBy.length > 0; + const singleGroup = isDefaultPolicy && isArray(groupBy) && groupBy.length === 0; + const isEditable = canEditRoutes; const isDeletable = canDeleteRoutes && !isDefaultPolicy; @@ -218,17 +214,25 @@ const Policy: FC = ({ /> )} - {isGrouping && ( - - Grouped by - {groupBy.join(', ')} - - )} - {/* we only want to show "no grouping" on the root policy, children with empty groupBy will inherit from the parent policy */} - {!isGrouping && isDefaultPolicy && ( - - Not grouping - + {!inheritedGrouping && ( + <> + {customGrouping && ( + + Grouped by + {groupBy.join(', ')} + + )} + {singleGroup && ( + + Single group + + )} + {noGrouping && ( + + Not grouping + + )} + )} {hasMuteTimings && ( @@ -253,44 +257,18 @@ const Policy: FC = ({
{/* pass the "readOnly" prop from the parent, because if you can't edit the parent you can't edit children */} - {childPolicies.map((route) => { - // inherited properties are config properties that exist on the parent but not on currentRoute - const inheritableProperties: InhertitableProperties = pick(currentRoute, [ - 'receiver', - 'group_by', - 'group_wait', - 'group_interval', - 'repeat_interval', - 'mute_time_intervals', - ]); - - // TODO how to solve this TypeScript mystery - const inherited = merge( - reduce( - inheritableProperties, - (acc: Partial = {}, value, key) => { - // @ts-ignore - if (value !== undefined && route[key] === undefined) { - // @ts-ignore - acc[key] = value; - } - - return acc; - }, - {} - ), - inheritedProperties - ); + {childPolicies.map((child) => { + const childInheritedProperties = getInheritedProperties(currentRoute, child, inheritedProperties); return ( = ({ prope content={ {Object.entries(properties).map(([key, value]) => { - // no idea how to do this with TypeScript + // no idea how to do this with TypeScript without type casting... return (