From 6392542db4cb9d5dc0ab29e337af11bee69c5dcc Mon Sep 17 00:00:00 2001 From: Gilles De Mey Date: Thu, 30 Jan 2025 14:42:11 +0100 Subject: [PATCH] Alerting: Fix inheritance of the timing options for policy tree (#99398) --- .../unified/NotificationPoliciesPage.test.tsx | 11 ++-- .../NotificationPoliciesList.tsx | 55 +++++++++---------- .../notification-policies/Policy.test.tsx | 8 +-- .../notification-policies/Policy.tsx | 15 ++--- 4 files changed, 40 insertions(+), 49 deletions(-) diff --git a/public/app/features/alerting/unified/NotificationPoliciesPage.test.tsx b/public/app/features/alerting/unified/NotificationPoliciesPage.test.tsx index 2bbb102b58b..dd124c0f9a6 100644 --- a/public/app/features/alerting/unified/NotificationPoliciesPage.test.tsx +++ b/public/app/features/alerting/unified/NotificationPoliciesPage.test.tsx @@ -296,14 +296,14 @@ describe.each([ }); it('allows user to reload and update policies if its been changed by another user', async () => { - jest.retryTimes(2); const { user } = renderNotificationPolicies(); + const NEW_INTERVAL = '12h'; await getRootRoute(); const existingConfig = getAlertmanagerConfig(GRAFANA_RULES_SOURCE_NAME); const modifiedConfig = produce(existingConfig, (draft) => { - draft.alertmanager_config.route!.group_interval = '12h'; + draft.alertmanager_config.route!.group_interval = NEW_INTERVAL; }); setAlertmanagerConfig(GRAFANA_RULES_SOURCE_NAME, modifiedConfig); @@ -317,11 +317,8 @@ describe.each([ await user.click(screen.getByRole('button', { name: /cancel/i })); await user.click(screen.getByRole('button', { name: /reload policies/i })); - await openDefaultPolicyEditModal(); - await user.click(await screen.findByRole('button', { name: /update default policy/i })); - expect(await screen.findByText(/updated notification policies/i)).toBeInTheDocument(); - // TODO: Check if test flakiness/length can be improved - }, 60000); + expect((await screen.findAllByTestId('timing-options'))[0]).toHaveTextContent(NEW_INTERVAL); + }); it('Should be able to delete an empty route', async () => { const defaultConfig: AlertManagerCortexConfig = { diff --git a/public/app/features/alerting/unified/components/notification-policies/NotificationPoliciesList.tsx b/public/app/features/alerting/unified/components/notification-policies/NotificationPoliciesList.tsx index 2e3e8d3bebd..a65dd8daf36 100644 --- a/public/app/features/alerting/unified/components/notification-policies/NotificationPoliciesList.tsx +++ b/public/app/features/alerting/unified/components/notification-policies/NotificationPoliciesList.tsx @@ -1,3 +1,4 @@ +import { defaults } from 'lodash'; import { useEffect, useMemo, useState } from 'react'; import { useAsyncFn } from 'react-use'; @@ -23,6 +24,7 @@ import { InsertPosition } from './../../utils/routeTree'; import { NotificationPoliciesFilter, findRoutesByMatchers, findRoutesMatchingPredicate } from './Filters'; import { useAddPolicyModal, useAlertGroupsModal, useDeletePolicyModal, useEditPolicyModal } from './Modals'; import { Policy } from './Policy'; +import { TIMING_OPTIONS_DEFAULTS } from './timingOptions'; import { useAddNotificationPolicy, useDeleteNotificationPolicy, @@ -218,34 +220,31 @@ export const NotificationPoliciesList = () => { )} {hasPoliciesData && ( - {rootRoute && ( - - )} - {rootRoute && ( - - )} + + )} {addModal} 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 c2e43ab77b2..89272e58572 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 @@ -136,7 +136,7 @@ describe('Policy', () => { expect(within(firstPolicy).getByTestId('contact-point')).toHaveTextContent('provisioned-contact-point'); expect(within(firstPolicy).getByTestId('mute-timings')).toHaveTextContent('Muted when mt-1'); expect(within(firstPolicy).getByTestId('active-timings')).toHaveTextContent('Active when mt-2'); - expect(within(firstPolicy).getByTestId('inherited-properties')).toHaveTextContent('Inherited2 properties'); + expect(within(firstPolicy).getByTestId('inherited-properties')).toHaveTextContent('Inherited4 properties'); // second custom policy should be correct const secondPolicy = customPolicies[1]; @@ -144,7 +144,7 @@ describe('Policy', () => { expect(within(secondPolicy).queryByTestId('continue-matching')).not.toBeInTheDocument(); expect(within(secondPolicy).queryByTestId('mute-timings')).not.toBeInTheDocument(); expect(within(secondPolicy).queryByTestId('active-timings')).not.toBeInTheDocument(); - expect(within(secondPolicy).getByTestId('inherited-properties')).toHaveTextContent('Inherited3 properties'); + expect(within(secondPolicy).getByTestId('inherited-properties')).toHaveTextContent('Inherited5 properties'); // third custom policy should be correct const thirdPolicy = customPolicies[2]; @@ -381,8 +381,8 @@ const mockRoutes: RouteWithID = { }, ], group_wait: '30s', - group_interval: undefined, - repeat_interval: undefined, + group_interval: '5m', + repeat_interval: '4h', }; describe('isAutoGeneratedRootAndSimplifiedEnabled', () => { 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 7f2d800390d..befb2cfe27b 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 { defaults, isArray, sumBy, uniqueId } from 'lodash'; +import { isArray, sumBy, uniqueId } from 'lodash'; import pluralize from 'pluralize'; import * as React from 'react'; import { FC, Fragment, ReactNode, useState } from 'react'; @@ -51,7 +51,9 @@ import { GrafanaPoliciesExporter } from '../export/GrafanaPoliciesExporter'; import { Matchers } from './Matchers'; import { RoutesMatchingFilters } from './NotificationPoliciesList'; -import { TIMING_OPTIONS_DEFAULTS, TimingOptions } from './timingOptions'; +import { TimingOptions } from './timingOptions'; + +const POLICIES_PER_PAGE = 20; interface PolicyComponentProps { receivers?: Receiver[]; @@ -172,8 +174,6 @@ const Policy = (props: PolicyComponentProps) => { errors.push(error); }); - const POLICIES_PER_PAGE = 20; - const [visibleChildPolicies, setVisibleChildPolicies] = useState(POLICIES_PER_PAGE); // build the menu actions for our policy @@ -511,12 +511,7 @@ function MetadataRow({ )} - {timingOptions && ( - // for the default policy we will also merge the default timings, that way a user can observe what the timing options would be - - )} + {timingOptions && } {hasInheritedProperties && ( <>