[release-12.0.3] Alerting: Fix group interval override when adding new rules (#107441)

* Alerting: Fix group interval override when adding new rules (#107324)

* Fix group interval override when adding new rules to existing groups

* Fix lint errors

* Update tests snapshots

* Update tests snapshots

* Fix GrafanaGroupLoader

(cherry picked from commit 3a38832ff6)

* Fix failing tests, fix ruler mock

---------

Co-authored-by: Konrad Lalik <konradlalik@gmail.com>

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": [
         {

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": [
         {

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 };
   };

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,
     };
 

public/app/features/alerting/unified/mocks/grafanaRulerApi.ts

@@ -70,7 +70,7 @@ export const grafanaRulerGroup: RulerRuleGroupDTO<RulerGrafanaRuleDTO> = {
 
 export const grafanaRulerGroup2: RulerRuleGroupDTO<RulerGrafanaRuleDTO> = {
   name: grafanaRulerGroupName2,
-  interval: '1m',
+  interval: '5m',
   rules: [grafanaRulerRule],
 };
 

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<RulerRuleGroupDTO>({
         name: groupName,
         interval: matchingGroup?.interval,

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<RulerRuleGroupDTO>({
         name: groupName,
         interval: matchingGroup?.interval,

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'
 );

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' });
+  });
 });

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',
+        }),
+      ]),
+    });
+  });
 });

public/app/features/alerting/unified/rule-list/GrafanaGroupLoader.tsx

@@ -69,7 +69,7 @@ export function GrafanaGroupLoader({
     );
   }
 
-  if (!rulerResponse || !promResponse) {
+  if (!rulerResponse && !promResponse) {
     return (
       <Alert
         title={t(
@@ -84,7 +84,7 @@ export function GrafanaGroupLoader({
 
   return (
     <>
-      {rulerResponse.rules.map((rulerRule) => {
+      {rulerResponse?.rules.map((rulerRule) => {
         const promRule = matches.get(rulerRule);
 
         if (!promRule) {

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}`),