Alerting: Fix notification policies inheritance algorithm (#69304) (#…

…69782)

* Alerting: Fix notification policies inheritance algorithm (#69304)

(cherry picked from commit f94e07f)

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';
 
@@ -190,21 +191,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."
             >
               <InputControl
-                render={({ field: { onChange, ref, ...field } }) => (
-                  <MultiSelect
-                    aria-label="Group by"
-                    {...field}
-                    allowCustomValue
-                    className={formStyles.input}
-                    onCreateOption={(opt: string) => {
-                      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 } }) => (
+                  <>
+                    <MultiSelect
+                      aria-label="Group by"
+                      {...field}
+                      invalid={Boolean(error)}
+                      allowCustomValue
+                      className={formStyles.input}
+                      onCreateOption={(opt: string) => {
+                        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 && <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(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();

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, useMemo } from 'react';
 import { useEnabled } from 'react-enable';
@@ -8,21 +8,20 @@ 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 { AlertingFeature } from '../../features';
 import { getNotificationsPermissions } from '../../utils/access-control';
 import { normalizeMatchers } from '../../utils/amroutes';
 import { createContactPointLink, createMuteTimingLink } from '../../utils/misc';
-import { findMatchingAlertGroups } from '../../utils/notification-policies';
+import {
+  findMatchingAlertGroups,
+  getInheritedProperties,
+  InhertitableProperties,
+} from '../../utils/notification-policies';
 import { HoverCard } from '../HoverCard';
 import { Label } from '../Label';
 import { MetaText } from '../MetaText';
@@ -32,17 +31,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[];
 
   routeTree: RouteWithID;
@@ -79,7 +73,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 +101,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,17 +217,25 @@ const Policy: FC<PolicyComponentProps> = ({
                   />
                 </MetaText>
               )}
-              {isGrouping && (
-                <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 */}
-              {!isGrouping && isDefaultPolicy && (
-                <MetaText icon="layer-group">
-                  <span>Not grouping</span>
-                </MetaText>
+              {!inheritedGrouping && (
+                <>
+                  {customGrouping && (
+                    <MetaText icon="layer-group" data-testid="grouping">
+                      <span>Grouped by</span>
+                      <Strong>{groupBy.join(', ')}</Strong>
+                    </MetaText>
+                  )}
+                  {singleGroup && (
+                    <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">
@@ -253,44 +260,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) => {
-          // 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<Route> = {}, 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 (
             <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}
@@ -365,13 +346,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>}
             />
           );
         })}
@@ -559,6 +541,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

@@ -154,8 +154,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,
-    groupBy: route.group_by ?? [],
+    overrideGrouping: Array.isArray(route.group_by) && route.group_by.length > 0,
+    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 ?? '',
@@ -183,16 +183,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);