Alerting: Fix notification policies inheritance algorithm (#69304)

public/app/features/alerting/unified/components/Strong.tsx

@@ -6,7 +6,7 @@ interface Props {}
 
 const Strong = ({ children }: React.PropsWithChildren<Props>) => {
   const theme = useTheme2();
-  return <strong style={{ color: theme.colors.text.maxContrast }}>{children}</strong>;
+  return <strong style={{ color: theme.colors.text.primary }}>{children}</strong>;
 };
 
 export { Strong };

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,10 +187,20 @@ export const AmRoutesExpandedForm = ({
               description="Group alerts when you receive a notification based on labels. If empty it will be inherited from the parent policy."
             >
               <InputControl
-                render={({ field: { onChange, ref, ...field } }) => (
+                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 } }) => (
+                  <>
                     <MultiSelect
                       aria-label="Group by"
                       {...field}
+                      invalid={Boolean(error)}
                       allowCustomValue
                       className={formStyles.input}
                       onCreateOption={(opt: string) => {
@@ -201,6 +212,8 @@ export const AmRoutesExpandedForm = ({
                       onChange={(value) => onChange(mapMultiSelectValueToStrings(value))}
                       options={[...commonGroupByOptions, ...groupByOptions]}
                     />
+                    {error && <FieldValidationMessage>{error.message}</FieldValidationMessage>}
+                  </>
                 )}
                 control={control}
                 name="groupBy"

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(screen.queryByRole('menuitem', { name: 'Edit' })).not.toBeDisabled();
-      expect(await screen.queryByRole('menuitem', { name: 'Delete' })).not.toBeDisabled();
+      expect(screen.queryByRole('menuitem', { name: 'Delete' })).not.toBeDisabled();
 
       await userEvent.click(screen.getByRole('menuitem', { name: 'Delete' }));
       expect(onDeletePolicy).toHaveBeenCalled();

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 {
+import { RouteWithID, Receiver, ObjectMatcher, AlertmanagerGroup } from 'app/plugins/datasource/alertmanager/types';
-  RouteWithID,
-  Receiver,
-  ObjectMatcher,
-  Route,
-  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<InhertitableProperties>;
   routesMatchingFilters?: RouteWithID[];
   // routeAlertGroupsMap?: Map<string, AlertmanagerGroup[]>;
 
@@ -79,7 +70,7 @@ const Policy: FC<PolicyComponentProps> = ({
 
   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<PolicyComponentProps> = ({
     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,18 +214,26 @@ const Policy: FC<PolicyComponentProps> = ({
                   />
                 </MetaText>
               )}
-              {isGrouping && (
+              {!inheritedGrouping && (
+                <>
+                  {customGrouping && (
                     <MetaText icon="layer-group" data-testid="grouping">
                       <span>Grouped by</span>
                       <Strong>{groupBy.join(', ')}</Strong>
                     </MetaText>
                   )}
-              {/* we only want to show "no grouping" on the root policy, children with empty groupBy will inherit from the parent policy */}
+                  {singleGroup && (
-              {!isGrouping && isDefaultPolicy && (
+                    <MetaText icon="layer-group">
+                      <span>Single group</span>
+                    </MetaText>
+                  )}
+                  {noGrouping && (
                     <MetaText icon="layer-group">
                       <span>Not grouping</span>
                     </MetaText>
                   )}
+                </>
+              )}
               {hasMuteTimings && (
                 <MetaText icon="calendar-slash" data-testid="mute-timings">
                   <span>Muted when</span>
@@ -253,44 +257,18 @@ const Policy: FC<PolicyComponentProps> = ({
       </div>
       <div className={styles.childPolicies}>
         {/* pass the "readOnly" prop from the parent, because if you can't edit the parent you can't edit children */}
-        {childPolicies.map((route) => {
+        {childPolicies.map((child) => {
-          // inherited properties are config properties that exist on the parent but not on currentRoute
+          const childInheritedProperties = getInheritedProperties(currentRoute, child, inheritedProperties);
-          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<Route> = {}, value, key) => {
-                // @ts-ignore
-                if (value !== undefined && route[key] === undefined) {
-                  // @ts-ignore
-                  acc[key] = value;
-                }
-
-                return acc;
-              },
-              {}
-            ),
-            inheritedProperties
-          );
 
           return (
             <Policy
               key={uniqueId()}
               routeTree={routeTree}
-              currentRoute={route}
+              currentRoute={child}
               receivers={receivers}
               contactPointsState={contactPointsState}
               readOnly={readOnly}
-              inheritedProperties={inherited}
+              inheritedProperties={childInheritedProperties}
               onAddPolicy={onAddPolicy}
               onEditPolicy={onEditPolicy}
               onDeletePolicy={onDeletePolicy}
@@ -366,13 +344,14 @@ const InheritedProperties: FC<{ properties: InhertitableProperties }> = ({ prope
     content={
       <Stack direction="row" gap={0.5}>
         {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 (
             <Label
               key={key}
               // @ts-ignore
               label={routePropertyToLabel(key)}
-              value={<Strong>{Array.isArray(value) ? value.join(', ') : value}</Strong>}
+              // @ts-ignore
+              value={<Strong>{routePropertyToValue(key, value)}</Strong>}
             />
           );
         })}
@@ -560,6 +539,29 @@ const routePropertyToLabel = (key: keyof InhertitableProperties): string => {
   }
 };
 
+const routePropertyToValue = (key: keyof InhertitableProperties, value: string | string[]): React.ReactNode => {
+  const isNotGrouping = key === 'group_by' && Array.isArray(value) && value[0] === '...';
+  const isSingleGroup = key === 'group_by' && Array.isArray(value) && value.length === 0;
+
+  if (isNotGrouping) {
+    return (
+      <Span variant="bodySmall" color="secondary">
+        Not grouping
+      </Span>
+    );
+  }
+
+  if (isSingleGroup) {
+    return (
+      <Span variant="bodySmall" color="secondary">
+        Single group
+      </Span>
+    );
+  }
+
+  return Array.isArray(value) ? value.join(', ') : value;
+};
+
 const getStyles = (theme: GrafanaTheme2) => ({
   matcher: (label: string) => {
     const { color, borderColor } = getTagColorsFromName(label);

public/app/features/alerting/unified/types/amroutes.ts

@@ -6,7 +6,7 @@ export interface FormAmRoute {
   continue: boolean;
   receiver: string;
   overrideGrouping: boolean;
-  groupBy: string[];
+  groupBy?: string[];
   overrideTimings: boolean;
   groupWaitValue: string;
   groupIntervalValue: string;

public/app/features/alerting/unified/utils/amroutes.test.ts

@@ -37,7 +37,7 @@ describe('formAmRouteToAmRoute', () => {
       const amRoute = formAmRouteToAmRoute('test', route, { id: 'root' });
 
       // Assert
-      expect(amRoute.group_by).toStrictEqual([]);
+      expect(amRoute.group_by).toStrictEqual(undefined);
     });
   });
 
@@ -56,10 +56,23 @@ describe('formAmRouteToAmRoute', () => {
 });
 
 describe('amRouteToFormAmRoute', () => {
+  describe('when called with empty group_by array', () => {
+    it('should set overrideGrouping true and groupBy empty', () => {
+      // Arrange
+      const amRoute = buildAmRoute({ group_by: [] });
+
+      // Act
+      const formRoute = amRouteToFormAmRoute(amRoute);
+
+      // Assert
+      expect(formRoute.groupBy).toStrictEqual([]);
+      expect(formRoute.overrideGrouping).toBe(false);
+    });
+  });
+
   describe('when called with empty group_by', () => {
     it.each`
       group_by
-      ${[]}
       ${null}
       ${undefined}
     `("when group_by is '$group_by', should set overrideGrouping false", ({ group_by }) => {
@@ -70,7 +83,7 @@ describe('amRouteToFormAmRoute', () => {
       const formRoute = amRouteToFormAmRoute(amRoute);
 
       // Assert
-      expect(formRoute.groupBy).toStrictEqual([]);
+      expect(formRoute.groupBy).toStrictEqual(undefined);
       expect(formRoute.overrideGrouping).toBe(false);
     });
   });

public/app/features/alerting/unified/utils/amroutes.ts

@@ -108,8 +108,8 @@ export const amRouteToFormAmRoute = (route: RouteWithID | Route | undefined): Fo
     ],
     continue: route.continue ?? false,
     receiver: route.receiver ?? '',
-    overrideGrouping: Array.isArray(route.group_by) && route.group_by.length !== 0,
+    overrideGrouping: Array.isArray(route.group_by) && route.group_by.length > 0,
-    groupBy: route.group_by ?? [],
+    groupBy: route.group_by ?? undefined,
     overrideTimings: [route.group_wait, route.group_interval, route.repeat_interval].some(Boolean),
     groupWaitValue: route.group_wait ?? '',
     groupIntervalValue: route.group_interval ?? '',
@@ -137,16 +137,19 @@ export const formAmRouteToAmRoute = (
     receiver,
   } = formAmRoute;
 
-  const group_by = overrideGrouping && groupBy ? groupBy : [];
+  // "undefined" means "inherit from the parent policy", currently supported by group_by, group_wait, group_interval, and repeat_interval
+  const INHERIT_FROM_PARENT = undefined;
+
+  const group_by = overrideGrouping ? groupBy : INHERIT_FROM_PARENT;
 
   const overrideGroupWait = overrideTimings && groupWaitValue;
-  const group_wait = overrideGroupWait ? groupWaitValue : undefined;
+  const group_wait = overrideGroupWait ? groupWaitValue : INHERIT_FROM_PARENT;
 
   const overrideGroupInterval = overrideTimings && groupIntervalValue;
-  const group_interval = overrideGroupInterval ? groupIntervalValue : undefined;
+  const group_interval = overrideGroupInterval ? groupIntervalValue : INHERIT_FROM_PARENT;
 
   const overrideRepeatInterval = overrideTimings && repeatIntervalValue;
-  const repeat_interval = overrideRepeatInterval ? repeatIntervalValue : undefined;
+  const repeat_interval = overrideRepeatInterval ? repeatIntervalValue : INHERIT_FROM_PARENT;
   const object_matchers = formAmRoute.object_matchers
     ?.filter((route) => route.name && route.value && route.operator)
     .map(({ name, operator, value }) => [name, operator, value] as ObjectMatcher);

public/app/features/alerting/unified/utils/notification-policies.test.ts

@@ -1,6 +1,6 @@
 import { MatcherOperator, Route, RouteWithID } from 'app/plugins/datasource/alertmanager/types';
 
-import { findMatchingRoutes, normalizeRoute } from './notification-policies';
+import { findMatchingRoutes, normalizeRoute, getInheritedProperties } from './notification-policies';
 
 import 'core-js/stable/structured-clone';
 
@@ -121,6 +121,130 @@ describe('findMatchingRoutes', () => {
   });
 });
 
+describe('getInheritedProperties()', () => {
+  describe('group_by: []', () => {
+    it('should get group_by: [] from parent', () => {
+      const parent: Route = {
+        receiver: 'PARENT',
+        group_by: ['label'],
+      };
+
+      const child: Route = {
+        receiver: 'CHILD',
+        group_by: [],
+      };
+
+      const childInherited = getInheritedProperties(parent, child);
+      expect(childInherited).toHaveProperty('group_by', ['label']);
+    });
+
+    it('should get group_by: [] from parent inherited properties', () => {
+      const parent: Route = {
+        receiver: 'PARENT',
+        group_by: [],
+      };
+
+      const child: Route = {
+        receiver: 'CHILD',
+        group_by: [],
+      };
+
+      const parentInherited = { group_by: ['label'] };
+
+      const childInherited = getInheritedProperties(parent, child, parentInherited);
+      expect(childInherited).toHaveProperty('group_by', ['label']);
+    });
+
+    it('should not inherit if the child overrides an inheritable value (group_by)', () => {
+      const parent: Route = {
+        receiver: 'PARENT',
+        group_by: ['parentLabel'],
+      };
+
+      const child: Route = {
+        receiver: 'CHILD',
+        group_by: ['childLabel'],
+      };
+
+      const childInherited = getInheritedProperties(parent, child);
+      expect(childInherited).not.toHaveProperty('group_by');
+    });
+
+    it('should inherit if group_by is undefined', () => {
+      const parent: Route = {
+        receiver: 'PARENT',
+        group_by: ['label'],
+      };
+
+      const child: Route = {
+        receiver: 'CHILD',
+        group_by: undefined,
+      };
+
+      const childInherited = getInheritedProperties(parent, child);
+      expect(childInherited).toHaveProperty('group_by', ['label']);
+    });
+  });
+
+  describe('regular "undefined" values', () => {
+    it('should compute inherited properties being undefined', () => {
+      const parent: Route = {
+        receiver: 'PARENT',
+        group_wait: '10s',
+      };
+
+      const child: Route = {
+        receiver: 'CHILD',
+      };
+
+      const childInherited = getInheritedProperties(parent, child);
+      expect(childInherited).toHaveProperty('group_wait', '10s');
+    });
+
+    it('should compute inherited properties being undefined from parent inherited properties', () => {
+      const parent: Route = {
+        receiver: 'PARENT',
+      };
+
+      const child: Route = {
+        receiver: 'CHILD',
+      };
+
+      const childInherited = getInheritedProperties(parent, child, { group_wait: '10s' });
+      expect(childInherited).toHaveProperty('group_wait', '10s');
+    });
+
+    it('should not inherit if the child overrides an inheritable value', () => {
+      const parent: Route = {
+        receiver: 'PARENT',
+        group_wait: '10s',
+      };
+
+      const child: Route = {
+        receiver: 'CHILD',
+        group_wait: '30s',
+      };
+
+      const childInherited = getInheritedProperties(parent, child);
+      expect(childInherited).not.toHaveProperty('group_wait');
+    });
+
+    it('should not inherit if the child overrides an inheritable value and the parent inherits', () => {
+      const parent: Route = {
+        receiver: 'PARENT',
+      };
+
+      const child: Route = {
+        receiver: 'CHILD',
+        group_wait: '30s',
+      };
+
+      const childInherited = getInheritedProperties(parent, child, { group_wait: '60s' });
+      expect(childInherited).not.toHaveProperty('group_wait');
+    });
+  });
+});
+
 describe('normalizeRoute', () => {
   it('should map matchers property to object_matchers', function () {
     const route: RouteWithID = {

public/app/features/alerting/unified/utils/notification-policies.ts

@@ -1,3 +1,5 @@
+import { isArray, merge, pick, reduce } from 'lodash';
+
 import { AlertmanagerGroup, Route, RouteWithID } from 'app/plugins/datasource/alertmanager/types';
 
 import { Label, normalizeMatchers, labelsMatchObjectMatchers } from './matchers';
@@ -82,4 +84,53 @@ function findMatchingAlertGroups(
   }, matchingGroups);
 }
 
-export { findMatchingAlertGroups, findMatchingRoutes };
+export type InhertitableProperties = Pick<
+  Route,
+  'receiver' | 'group_by' | 'group_wait' | 'group_interval' | 'repeat_interval' | 'mute_time_intervals'
+>;
+
+// inherited properties are config properties that exist on the parent route (or its inherited properties) but not on the child route
+function getInheritedProperties(
+  parentRoute: Route,
+  childRoute: Route,
+  propertiesParentInherited?: Partial<InhertitableProperties>
+) {
+  const fullParentProperties = merge({}, parentRoute, propertiesParentInherited);
+
+  const inheritableProperties: InhertitableProperties = pick(fullParentProperties, [
+    'receiver',
+    'group_by',
+    'group_wait',
+    'group_interval',
+    'repeat_interval',
+    'mute_time_intervals',
+  ]);
+
+  // TODO how to solve this TypeScript mystery?
+  const inherited = reduce(
+    inheritableProperties,
+    (inheritedProperties: Partial<Route> = {}, parentValue, property) => {
+      // @ts-ignore
+      const inheritFromParent = parentValue !== undefined && childRoute[property] === undefined;
+      const inheritEmptyGroupByFromParent =
+        property === 'group_by' && isArray(childRoute[property]) && childRoute[property]?.length === 0;
+
+      if (inheritFromParent) {
+        // @ts-ignore
+        inheritedProperties[property] = parentValue;
+      }
+
+      if (inheritEmptyGroupByFromParent) {
+        // @ts-ignore
+        inheritedProperties[property] = parentValue;
+      }
+
+      return inheritedProperties;
+    },
+    {}
+  );
+
+  return inherited;
+}
+
+export { findMatchingAlertGroups, findMatchingRoutes, getInheritedProperties };