diff --git a/public/app/features/alerting/unified/hooks/ruleGroup/__snapshots__/useMoveRuleFromRuleGroup.test.tsx.snap b/public/app/features/alerting/unified/hooks/ruleGroup/__snapshots__/useMoveRuleFromRuleGroup.test.tsx.snap index d5ff767a76f..f88931de9b6 100644 --- a/public/app/features/alerting/unified/hooks/ruleGroup/__snapshots__/useMoveRuleFromRuleGroup.test.tsx.snap +++ b/public/app/features/alerting/unified/hooks/ruleGroup/__snapshots__/useMoveRuleFromRuleGroup.test.tsx.snap @@ -4,6 +4,7 @@ exports[`Moving a Data source managed rule should move a rule in a namespace to [ { "body": { + "interval": "1m", "name": "group-1", "rules": [ { @@ -49,6 +50,7 @@ exports[`Moving a Data source managed rule should move a rule in an existing gro [ { "body": { + "interval": "1m", "name": "entirely new group name", "rules": [ { @@ -190,6 +192,7 @@ exports[`Moving a Grafana managed rule should move a rule from an existing group [ { "body": { + "interval": "1m", "name": "empty-group", "rules": [ { diff --git a/public/app/features/alerting/unified/hooks/ruleGroup/__snapshots__/useUpdateRuleInRuleGroup.test.tsx.snap b/public/app/features/alerting/unified/hooks/ruleGroup/__snapshots__/useUpdateRuleInRuleGroup.test.tsx.snap index 1b0143c619e..6afe36981f9 100644 --- a/public/app/features/alerting/unified/hooks/ruleGroup/__snapshots__/useUpdateRuleInRuleGroup.test.tsx.snap +++ b/public/app/features/alerting/unified/hooks/ruleGroup/__snapshots__/useUpdateRuleInRuleGroup.test.tsx.snap @@ -4,6 +4,7 @@ exports[`Updating a Data source managed rule should be able to move a rule if ta [ { "body": { + "interval": "1m", "name": "a new group", "rules": [ { @@ -144,7 +145,7 @@ exports[`Updating a Grafana managed rule should move a rule in to another group [ { "body": { - "interval": "1m", + "interval": "5m", "name": "grafana-group-2", "rules": [ { diff --git a/public/app/features/alerting/unified/hooks/ruleGroup/useProduceNewRuleGroup.ts b/public/app/features/alerting/unified/hooks/ruleGroup/useProduceNewRuleGroup.ts index ddbbc0e2401..c5e0eb9faf7 100644 --- a/public/app/features/alerting/unified/hooks/ruleGroup/useProduceNewRuleGroup.ts +++ b/public/app/features/alerting/unified/hooks/ruleGroup/useProduceNewRuleGroup.ts @@ -6,7 +6,7 @@ import { PostableRulerRuleGroupDTO } from 'app/types/unified-alerting-dto'; import { alertRuleApi } from '../../api/alertRuleApi'; import { featureDiscoveryApi } from '../../api/featureDiscoveryApi'; import { notFoundToNullOrThrow } from '../../api/util'; -import { ruleGroupReducer } from '../../reducers/ruler/ruleGroups'; +import { addRuleAction, ruleGroupReducer } from '../../reducers/ruler/ruleGroups'; import { DEFAULT_GROUP_EVALUATION_INTERVAL } from '../../rule-editor/formDefaults'; import { getDatasourceAPIUid } from '../../utils/datasource'; @@ -62,10 +62,15 @@ export function useProduceNewRuleGroup() { .catch(notFoundToNullOrThrow); const initialRuleGroupDefinition = latestRuleGroupDefinition ?? createBlankRuleGroup(groupName); - const newRuleGroupDefinition = actions.reduce( - (ruleGroup, action) => ruleGroupReducer(ruleGroup, action), - initialRuleGroupDefinition - ); + const newRuleGroupDefinition = actions.reduce((ruleGroup, action) => { + // This is a workaround to ensure that the interval is set correctly when adding a rule to an existing rule group. + // The interval is set to default for DMA rules even for existing rule groups with a non-default interval. + // We no longer allow setting the interval for existing groups, but still allow that when you create a new rule group. + if (latestRuleGroupDefinition && addRuleAction.match(action)) { + action.payload.interval = latestRuleGroupDefinition.interval; + } + return ruleGroupReducer(ruleGroup, action); + }, initialRuleGroupDefinition); return { newRuleGroupDefinition, rulerConfig }; }; diff --git a/public/app/features/alerting/unified/hooks/ruleGroup/useUpdateRuleInRuleGroup.test.tsx b/public/app/features/alerting/unified/hooks/ruleGroup/useUpdateRuleInRuleGroup.test.tsx index 2324ed363db..739e57a86f6 100644 --- a/public/app/features/alerting/unified/hooks/ruleGroup/useUpdateRuleInRuleGroup.test.tsx +++ b/public/app/features/alerting/unified/hooks/ruleGroup/useUpdateRuleInRuleGroup.test.tsx @@ -9,8 +9,8 @@ import { PostableRuleDTO } from 'app/types/unified-alerting-dto'; import { setupMswServer } from '../../mockApi'; import { grantUserPermissions } from '../../mocks'; import { - grafanaRulerGroupName, - grafanaRulerGroupName2, + grafanaRulerGroup, + grafanaRulerGroup2, grafanaRulerNamespace, grafanaRulerRule, } from '../../mocks/grafanaRulerApi'; @@ -41,7 +41,7 @@ describe('Updating a Grafana managed rule', () => { const ruleGroupID: RuleGroupIdentifier = { dataSourceName: GRAFANA_RULES_SOURCE_NAME, - groupName: grafanaRulerGroupName, + groupName: grafanaRulerGroup.name, namespaceName: grafanaRulerNamespace.uid, }; @@ -71,13 +71,13 @@ describe('Updating a Grafana managed rule', () => { const ruleGroupID: RuleGroupIdentifier = { dataSourceName: GRAFANA_RULES_SOURCE_NAME, - groupName: grafanaRulerGroupName, + groupName: grafanaRulerGroup.name, namespaceName: grafanaRulerNamespace.uid, }; const targetRuleGroupID: RuleGroupIdentifier = { dataSourceName: GRAFANA_RULES_SOURCE_NAME, - groupName: grafanaRulerGroupName2, + groupName: grafanaRulerGroup2.name, namespaceName: grafanaRulerNamespace.uid, }; @@ -110,7 +110,7 @@ describe('Updating a Grafana managed rule', () => { it('should fail if the rule does not exist in the group', async () => { const ruleGroupID: RuleGroupIdentifier = { dataSourceName: GRAFANA_RULES_SOURCE_NAME, - groupName: grafanaRulerGroupName, + groupName: grafanaRulerGroup.name, namespaceName: grafanaRulerNamespace.uid, }; diff --git a/public/app/features/alerting/unified/mocks/grafanaRulerApi.ts b/public/app/features/alerting/unified/mocks/grafanaRulerApi.ts index 13eb4185823..18c690e7678 100644 --- a/public/app/features/alerting/unified/mocks/grafanaRulerApi.ts +++ b/public/app/features/alerting/unified/mocks/grafanaRulerApi.ts @@ -70,7 +70,7 @@ export const grafanaRulerGroup: RulerRuleGroupDTO = { export const grafanaRulerGroup2: RulerRuleGroupDTO = { name: grafanaRulerGroupName2, - interval: '1m', + interval: '5m', rules: [grafanaRulerRule], }; diff --git a/public/app/features/alerting/unified/mocks/server/handlers/grafanaRuler.ts b/public/app/features/alerting/unified/mocks/server/handlers/grafanaRuler.ts index 36fb9b1318d..414afc1f07b 100644 --- a/public/app/features/alerting/unified/mocks/server/handlers/grafanaRuler.ts +++ b/public/app/features/alerting/unified/mocks/server/handlers/grafanaRuler.ts @@ -71,15 +71,17 @@ export const rulerRuleGroupHandler = (options?: HandlerOptions) => { return options.response; } - // This mimic API response as closely as possible. - // Invalid folderUid returns 403 but invalid group will return 202 with empty list of rules - // This should be fixed soon to return 404 instead of 202 const namespace = rulerTestDb.getNamespace(folderUid); if (!namespace) { return new HttpResponse(null, { status: 403 }); } const matchingGroup = rulerTestDb.getGroup(folderUid, groupName); + + if (!matchingGroup) { + return HttpResponse.json({ message: 'group does not exist' }, { status: 404 }); + } + return HttpResponse.json({ name: groupName, interval: matchingGroup?.interval, diff --git a/public/app/features/alerting/unified/mocks/server/handlers/mimirRuler.ts b/public/app/features/alerting/unified/mocks/server/handlers/mimirRuler.ts index 6dbdff31031..a0bee5efaf3 100644 --- a/public/app/features/alerting/unified/mocks/server/handlers/mimirRuler.ts +++ b/public/app/features/alerting/unified/mocks/server/handlers/mimirRuler.ts @@ -53,6 +53,11 @@ export const rulerRuleGroupHandler = (options?: HandlerOptions) => { } const matchingGroup = namespace.find((group) => group.name === groupName); + + if (!matchingGroup) { + return HttpResponse.json({ message: 'group does not exist' }, { status: 404 }); + } + return HttpResponse.json({ name: groupName, interval: matchingGroup?.interval, diff --git a/public/app/features/alerting/unified/reducers/ruler/ruleGroups.ts b/public/app/features/alerting/unified/reducers/ruler/ruleGroups.ts index b8d91042b2c..82224e02d65 100644 --- a/public/app/features/alerting/unified/reducers/ruler/ruleGroups.ts +++ b/public/app/features/alerting/unified/reducers/ruler/ruleGroups.ts @@ -9,6 +9,8 @@ import { hashRulerRule } from '../../utils/rule-id'; import { isCloudRuleIdentifier, isGrafanaRuleIdentifier, rulerRuleType } from '../../utils/rules'; // rule-scoped actions +// TOOD The interval field only make sense when adding a rule to a new rule group. +// We need to split these into distinct actions and introduce a separete addNewRuleGroupAction. export const addRuleAction = createAction<{ rule: PostableRuleDTO; groupName?: string; interval?: string }>( 'ruleGroup/rules/add' ); diff --git a/public/app/features/alerting/unified/rule-editor/RuleEditorCloudRules.test.tsx b/public/app/features/alerting/unified/rule-editor/RuleEditorCloudRules.test.tsx index d70177fc7a9..b245733670f 100644 --- a/public/app/features/alerting/unified/rule-editor/RuleEditorCloudRules.test.tsx +++ b/public/app/features/alerting/unified/rule-editor/RuleEditorCloudRules.test.tsx @@ -8,7 +8,7 @@ import { AccessControlAction } from 'app/types'; import { ExpressionEditorProps } from '../components/rule-editor/ExpressionEditor'; import { setupMswServer } from '../mockApi'; import { grantUserPermissions } from '../mocks'; -import { GROUP_3, NAMESPACE_2 } from '../mocks/mimirRulerApi'; +import { GROUP_3, GROUP_4, NAMESPACE_2 } from '../mocks/mimirRulerApi'; import { mimirDataSource } from '../mocks/server/configure'; import { MIMIR_DATASOURCE_UID } from '../mocks/server/constants'; import { captureRequests, serializeRequests } from '../mocks/server/events'; @@ -86,4 +86,52 @@ describe('RuleEditor cloud', () => { const serializedRequests = await serializeRequests(requests); expect(serializedRequests).toMatchSnapshot(); }); + + it('should keep existing rule interval duration when attaching new rules', async () => { + const { user } = renderRuleEditor(); + + const removeExpressionsButtons = await screen.findAllByLabelText(/Remove expression/); + expect(removeExpressionsButtons).toHaveLength(2); + + // Needs to wait for feature discovery API call to finish - Check if ruler enabled + expect(await screen.findByText('Data source-managed')).toBeInTheDocument(); + + const switchToCloudButton = screen.getByText('Data source-managed'); + expect(switchToCloudButton).toBeInTheDocument(); + expect(switchToCloudButton).toBeEnabled(); + + await user.click(switchToCloudButton); + + //expressions are removed after switching to data-source managed + expect(screen.queryAllByLabelText(/Remove expression/)).toHaveLength(0); + + expect(screen.getByTestId(selectors.components.DataSourcePicker.inputV2)).toBeInTheDocument(); + + const dataSourceSelect = await ui.inputs.dataSource.find(); + await user.click(dataSourceSelect); + await user.click(screen.getByText(MIMIR_DATASOURCE_UID)); + + await user.type(await ui.inputs.expr.find(), 'up == 1'); + + await user.type(ui.inputs.name.get(), 'my great new rule with 3m interval'); + await clickSelectOption(ui.inputs.namespace.get(), NAMESPACE_2); + await clickSelectOption(ui.inputs.group.get(), GROUP_4); + + await user.type(ui.inputs.annotationValue(0).get(), 'some summary'); + await user.type(ui.inputs.annotationValue(1).get(), 'some description'); + + // TODO remove skipPointerEventsCheck once https://github.com/jsdom/jsdom/issues/3232 is fixed + await user.click(ui.buttons.addLabel.get()); + + // save and check what was sent to backend + const capture = captureRequests(); + await user.click(ui.buttons.saveAndExit.get()); + const requests = await capture; + + const serializedRequests = await serializeRequests(requests); + const saveRequest = serializedRequests.find((req) => req.method === 'POST'); + + expect(saveRequest).toBeDefined(); + expect(saveRequest?.body).toMatchObject({ interval: '3m' }); + }); }); diff --git a/public/app/features/alerting/unified/rule-editor/RuleEditorGrafanaRules.test.tsx b/public/app/features/alerting/unified/rule-editor/RuleEditorGrafanaRules.test.tsx index 9505c96fd23..8182794e0d0 100644 --- a/public/app/features/alerting/unified/rule-editor/RuleEditorGrafanaRules.test.tsx +++ b/public/app/features/alerting/unified/rule-editor/RuleEditorGrafanaRules.test.tsx @@ -11,7 +11,7 @@ import { DashboardSearchItemType } from 'app/features/search/types'; import { AccessControlAction } from 'app/types'; import { grantUserPermissions, mockDataSource, mockFolder } from '../mocks'; -import { grafanaRulerGroup, grafanaRulerRule } from '../mocks/grafanaRulerApi'; +import { grafanaRulerGroup, grafanaRulerGroup2, grafanaRulerRule } from '../mocks/grafanaRulerApi'; import { setFolderResponse } from '../mocks/server/configure'; import { captureRequests, serializeRequests } from '../mocks/server/events'; import { setupDataSources } from '../testSetup/datasources'; @@ -140,4 +140,47 @@ describe('RuleEditor grafana managed rules', () => { const serializedRequests = await serializeRequests(requests); expect(serializedRequests).toMatchSnapshot(); }); + + it('should keep existing group interval when creating new rule in existing group', async () => { + const capture = captureRequests((r) => r.method === 'POST' && r.url.includes('/api/ruler/')); + + const { user } = renderRuleEditor(); + + await user.type(await ui.inputs.name.find(), 'my great new rule'); + await user.click(await screen.findByRole('button', { name: /select folder/i })); + await user.click(await screen.findByLabelText(/folder a/i)); + + // Select the existing group with 5m interval + const groupInput = await ui.inputs.group.find(); + await user.click(await byRole('combobox').find(groupInput)); + await clickSelectOption(groupInput, grafanaRulerGroup2.name); + await user.type(ui.inputs.annotationValue(1).get(), 'some description'); + + // Set pending period to none (0s) to avoid validation errors + const pendingPeriodInput = await ui.inputs.pendingPeriod.find(); + await user.clear(pendingPeriodInput); + await user.type(pendingPeriodInput, '0s'); + + await user.click(ui.buttons.saveAndExit.get()); + + expect(await screen.findByRole('status')).toHaveTextContent('Rule added successfully'); + const requests = await capture; + const serializedRequests = await serializeRequests(requests); + + // Verify that the existing group's 5m interval is preserved + const saveRequest = serializedRequests.find((req) => req.method === 'POST'); + expect(saveRequest).toBeDefined(); + expect(saveRequest?.body).toMatchObject({ + name: grafanaRulerGroup2.name, + interval: '5m', // The existing group's interval should be preserved + rules: expect.arrayContaining([ + expect.objectContaining({ + annotations: expect.objectContaining({ + description: 'some description', + }), + for: '0s', + }), + ]), + }); + }); }); diff --git a/public/app/features/alerting/unified/rule-list/GrafanaGroupLoader.tsx b/public/app/features/alerting/unified/rule-list/GrafanaGroupLoader.tsx index df87f6fcd15..aaed8e3e370 100644 --- a/public/app/features/alerting/unified/rule-list/GrafanaGroupLoader.tsx +++ b/public/app/features/alerting/unified/rule-list/GrafanaGroupLoader.tsx @@ -69,7 +69,7 @@ export function GrafanaGroupLoader({ ); } - if (!rulerResponse || !promResponse) { + if (!rulerResponse && !promResponse) { return ( - {rulerResponse.rules.map((rulerRule) => { + {rulerResponse?.rules.map((rulerRule) => { const promRule = matches.get(rulerRule); if (!promRule) { diff --git a/public/test/helpers/alertingRuleEditor.tsx b/public/test/helpers/alertingRuleEditor.tsx index d15eee0a08b..bf0df213cd8 100644 --- a/public/test/helpers/alertingRuleEditor.tsx +++ b/public/test/helpers/alertingRuleEditor.tsx @@ -23,6 +23,7 @@ export const ui = { folderContainer: byTestId(selectors.components.FolderPicker.containerV2), namespace: byTestId('namespace-picker'), group: byTestId('group-picker'), + pendingPeriod: byRole('textbox', { name: /^pending period/i }), annotationKey: (idx: number) => byTestId(`annotation-key-${idx}`), annotationValue: (idx: number) => byTestId(`annotation-value-${idx}`), labelKey: (idx: number) => byTestId(`label-key-${idx}`),