Skip to content
Jump to bottom

Automated Test: notification-rule-enhancements #327

Closed
wants to merge 1 commit into from
+542 −418
Closed

Automated Test: notification-rule-enhancements #327

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
77 changes: 53 additions & 24 deletions public/app/features/alerting/unified/components/rule-viewer/AlertRuleMenu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,12 @@ import { useRulePluginLinkExtension } from 'app/features/alerting/unified/plugin
import { Rule, RuleGroupIdentifierV2, RuleIdentifier } from 'app/types/unified-alerting';
import { PromAlertingRuleState, RulerRuleDTO } from 'app/types/unified-alerting-dto';

import { AlertRuleAction, useRulerRuleAbility } from '../../hooks/useAbilities';
import {
AlertRuleAction,
skipToken,
useGrafanaPromRuleAbilities,
useRulerRuleAbilities,
} from '../../hooks/useAbilities';
import { createShareLink, isLocalDevEnv, isOpenSourceEdition } from '../../utils/misc';
import * as ruleId from '../../utils/rule-id';
import { prometheusRuleType, rulerRuleType } from '../../utils/rules';
Expand All @@ -33,6 +38,8 @@ interface Props {
/**
* Get a list of menu items + divider elements for rendering in an alert rule's
* dropdown menu
* If the consumer of this component comes from the alert list view, we need to use promRule to check abilities and permissions,
* as we have removed all requests to the ruler API in the list view.
*/
const AlertRuleMenu = ({
promRule,
Expand All @@ -46,29 +53,51 @@ const AlertRuleMenu = ({
buttonSize,
fill,
}: Props) => {
// check all abilities and permissions
const [pauseSupported, pauseAllowed] = useRulerRuleAbility(rulerRule, groupIdentifier, AlertRuleAction.Pause);
const canPause = pauseSupported && pauseAllowed;

const [deleteSupported, deleteAllowed] = useRulerRuleAbility(rulerRule, groupIdentifier, AlertRuleAction.Delete);
const canDelete = deleteSupported && deleteAllowed;

const [duplicateSupported, duplicateAllowed] = useRulerRuleAbility(
rulerRule,
groupIdentifier,
AlertRuleAction.Duplicate
);
const canDuplicate = duplicateSupported && duplicateAllowed;

const [silenceSupported, silenceAllowed] = useRulerRuleAbility(rulerRule, groupIdentifier, AlertRuleAction.Silence);
const canSilence = silenceSupported && silenceAllowed;

const [exportSupported, exportAllowed] = useRulerRuleAbility(
rulerRule,
groupIdentifier,
AlertRuleAction.ModifyExport
);
const canExport = exportSupported && exportAllowed;
// check all abilities and permissions using rulerRule
const [rulerPauseAbility, rulerDeleteAbility, rulerDuplicateAbility, rulerSilenceAbility, rulerExportAbility] =
useRulerRuleAbilities(rulerRule, groupIdentifier, [
AlertRuleAction.Pause,
AlertRuleAction.Delete,
AlertRuleAction.Duplicate,
AlertRuleAction.Silence,
AlertRuleAction.ModifyExport,
]);

// check all abilities and permissions using promRule
const [
grafanaPauseAbility,
grafanaDeleteAbility,
grafanaDuplicateAbility,
grafanaSilenceAbility,
grafanaExportAbility,
] = useGrafanaPromRuleAbilities(prometheusRuleType.grafana.rule(promRule) ? promRule : skipToken, [
AlertRuleAction.Pause,
AlertRuleAction.Delete,
AlertRuleAction.Duplicate,
AlertRuleAction.Silence,
AlertRuleAction.ModifyExport,
]);

const [pauseSupported, pauseAllowed] = rulerPauseAbility;
const [grafanaPauseSupported, grafanaPauseAllowed] = grafanaPauseAbility;
const canPause = (pauseSupported && pauseAllowed) || (grafanaPauseSupported && grafanaPauseAllowed);

const [deleteSupported, deleteAllowed] = rulerDeleteAbility;
const [grafanaDeleteSupported, grafanaDeleteAllowed] = grafanaDeleteAbility;
const canDelete = (deleteSupported && deleteAllowed) || (grafanaDeleteSupported && grafanaDeleteAllowed);

const [duplicateSupported, duplicateAllowed] = rulerDuplicateAbility;
const [grafanaDuplicateSupported, grafanaDuplicateAllowed] = grafanaDuplicateAbility;
const canDuplicate =
(duplicateSupported && duplicateAllowed) || (grafanaDuplicateSupported && grafanaDuplicateAllowed);

const [silenceSupported, silenceAllowed] = rulerSilenceAbility;
const [grafanaSilenceSupported, grafanaSilenceAllowed] = grafanaSilenceAbility;
const canSilence = (silenceSupported && silenceAllowed) || (grafanaSilenceSupported && grafanaSilenceAllowed);

const [exportSupported, exportAllowed] = rulerExportAbility;
const [grafanaExportSupported, grafanaExportAllowed] = grafanaExportAbility;
const canExport = (exportSupported && exportAllowed) || (grafanaExportSupported && grafanaExportAllowed);

const ruleExtensionLinks = useRulePluginLinkExtension(promRule, groupIdentifier);

Expand Down
5 changes: 4 additions & 1 deletion public/app/features/alerting/unified/components/rules/RuleDetails.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { setupMswServer } from 'app/features/alerting/unified/mockApi';

import { useIsRuleEditable } from '../../hooks/useIsRuleEditable';
import { getCloudRule, getGrafanaRule } from '../../mocks';
import { mimirDataSource } from '../../mocks/server/configure';

import { RuleDetails } from './RuleDetails';

Expand All @@ -32,6 +33,8 @@ const ui = {

setupMswServer();

const { dataSource: mimirDs } = mimirDataSource();

beforeAll(() => {
jest.clearAllMocks();
});
Expand Down Expand Up @@ -81,7 +84,7 @@ describe('RuleDetails RBAC', () => {
});

describe('Cloud rules action buttons', () => {
const cloudRule = getCloudRule({ name: 'Cloud' });
const cloudRule = getCloudRule({ name: 'Cloud' }, { rulesSource: mimirDs });

it('Should not render Edit button for users with the update permission', async () => {
// Arrange
Expand Down
108 changes: 89 additions & 19 deletions public/app/features/alerting/unified/components/rules/RulesTable.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,14 @@ import { byRole } from 'testing-library-selector';
import { setPluginLinksHook } from '@grafana/runtime';
import { setupMswServer } from 'app/features/alerting/unified/mockApi';

import { AlertRuleAction, useAlertRuleAbility, useRulerRuleAbility } from '../../hooks/useAbilities';
import {
AlertRuleAction,
useAlertRuleAbility,
useGrafanaPromRuleAbilities,
useGrafanaPromRuleAbility,
useRulerRuleAbilities,
useRulerRuleAbility,
} from '../../hooks/useAbilities';
import { getCloudRule, getGrafanaRule } from '../../mocks';
import { mimirDataSource } from '../../mocks/server/configure';

Expand All @@ -13,11 +20,15 @@ import { RulesTable } from './RulesTable';
jest.mock('../../hooks/useAbilities');

const mocks = {
// This is a bit unfortunate, but we need to mock both abilities
// RuleActionButtons still needs to use the useAlertRuleAbility hook
// whereas AlertRuleMenu has already been refactored to use useRulerRuleAbility
// Mock the hooks that are actually used by the components:
// RuleActionsButtons uses: useAlertRuleAbility (singular)
// AlertRuleMenu uses: useRulerRuleAbilities and useGrafanaPromRuleAbilities (plural)
// We can also use useGrafanaPromRuleAbility (singular) for simpler mocking
useRulerRuleAbility: jest.mocked(useRulerRuleAbility),
useAlertRuleAbility: jest.mocked(useAlertRuleAbility),
useGrafanaPromRuleAbility: jest.mocked(useGrafanaPromRuleAbility),
useRulerRuleAbilities: jest.mocked(useRulerRuleAbilities),
useGrafanaPromRuleAbilities: jest.mocked(useGrafanaPromRuleAbilities),
};

setPluginLinksHook(() => ({
Expand Down Expand Up @@ -46,30 +57,55 @@ describe('RulesTable RBAC', () => {
jest.clearAllMocks();
jest.restoreAllMocks();
jest.resetAllMocks();

// Set up default neutral mocks for all hooks
// Singular hooks (used by RuleActionsButtons and can simplify mocking)
mocks.useAlertRuleAbility.mockReturnValue([false, false]);
mocks.useRulerRuleAbility.mockReturnValue([false, false]);
mocks.useGrafanaPromRuleAbility.mockReturnValue([false, false]);

// Plural hooks (used by AlertRuleMenu) - need to return arrays based on input actions
mocks.useRulerRuleAbilities.mockImplementation((_rule, _groupIdentifier, actions) => {
return actions.map(() => [false, false]);
});
mocks.useGrafanaPromRuleAbilities.mockImplementation((_rule, actions) => {
return actions.map(() => [false, false]);
});
});

describe('Grafana rules action buttons', () => {
const grafanaRule = getGrafanaRule({ name: 'Grafana' });

it('Should not render Edit button for users without the update permission', async () => {
mocks.useRulerRuleAbility.mockImplementation((_rule, _groupIdentifier, action) => {
// Mock the specific hooks needed for Grafana rules
// Using singular hook for simpler mocking
mocks.useAlertRuleAbility.mockImplementation((rule, action) => {
return action === AlertRuleAction.Update ? [true, false] : [true, true];
});
mocks.useAlertRuleAbility.mockImplementation((_rule, action) => {
mocks.useGrafanaPromRuleAbility.mockImplementation((rule, action) => {
return action === AlertRuleAction.Update ? [true, false] : [true, true];
});
// Still need plural hook for AlertRuleMenu component
mocks.useGrafanaPromRuleAbilities.mockImplementation((rule, actions) => {
return actions.map((action) => {
return action === AlertRuleAction.Update ? [true, false] : [true, true];
});
});

render(<RulesTable rules={[grafanaRule]} />);

await waitFor(() => expect(ui.actionButtons.edit.query()).not.toBeInTheDocument());
});
Comment on lines 79 to 98
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# Find RulesTable component implementation
fd "RulesTable.tsx" --type=f

Repository: admin-coderabbit/coderabbit-grafana-mirror

Length of output: 318

🏁 Script executed:

# Check which RuleActionsButtons variant is imported/used in RulesTable
rg -n "RuleActionsButtons" public/app/features/alerting/unified/components/rules/ --type=ts -C 3

Repository: admin-coderabbit/coderabbit-grafana-mirror

Length of output: 14455

🏁 Script executed:

# Check what hooks are used in RuleActionsButtons variants
rg -n "useGrafanaPromRuleAbility\|useAlertRuleAbility" public/app/features/alerting/unified/components/rules/ --type=ts -B 2 -A 2

Repository: admin-coderabbit/coderabbit-grafana-mirror

Length of output: 68

🏁 Script executed:

# Check what hooks are actually imported/used in RuleActionsButtons.tsx
cat -n public/app/features/alerting/unified/components/rules/RuleActionsButtons.tsx | head -100

Repository: admin-coderabbit/coderabbit-grafana-mirror

Length of output: 4569

🏁 Script executed:

# Search for useGrafanaPromRuleAbility and useAlertRuleAbility in RuleActionsButtons
rg "useGrafanaPromRuleAbility|useAlertRuleAbility" public/app/features/alerting/unified/components/rules/RuleActionsButtons.tsx

Repository: admin-coderabbit/coderabbit-grafana-mirror

Length of output: 264

🏁 Script executed:

# Check what AlertRuleMenu uses
rg "useGrafanaPromRuleAbilities|useRulerRuleAbilities" public/app/features/alerting/unified/components/rules/ --type=ts -l

Repository: admin-coderabbit/coderabbit-grafana-mirror

Length of output: 159

🏁 Script executed:

# Check AlertRuleMenu to see what hooks it actually uses
rg -n "useGrafanaPromRuleAbilities|useRulerRuleAbilities" public/app/features/alerting/unified/components/rule-viewer/AlertRuleMenu.tsx -A 2

Repository: admin-coderabbit/coderabbit-grafana-mirror

Length of output: 572

🏁 Script executed:

# Let's also see what the test mocks are actually doing and if they're needed
cat -n public/app/features/alerting/unified/components/rules/RulesTable.test.tsx | sed -n '20,40p'

Repository: admin-coderabbit/coderabbit-grafana-mirror

Length of output: 997

🏁 Script executed:

# Check if useGrafanaPromRuleAbility is used anywhere in the test file
rg -n "useGrafanaPromRuleAbility" public/app/features/alerting/unified/components/rules/RulesTable.test.tsx

Repository: admin-coderabbit/coderabbit-grafana-mirror

Length of output: 422

Remove the unnecessary useGrafanaPromRuleAbility mock (line 85-87).

RuleActionsButtons only uses useAlertRuleAbility. AlertRuleMenu, which is rendered by RuleActionsButtons, uses the plural useGrafanaPromRuleAbilities, not the singular useGrafanaPromRuleAbility. The singular mock is unused and can be removed.

🤖 Prompt for AI Agents 
In `@public/app/features/alerting/unified/components/rules/RulesTable.test.tsx`
around lines 79 - 98, Remove the unused mock for useGrafanaPromRuleAbility in
the test: keep the mocks for useAlertRuleAbility (used by RuleActionsButtons)
and useGrafanaPromRuleAbilities (used by AlertRuleMenu) and delete the
mockImplementation block that references useGrafanaPromRuleAbility; ensure
RulesTable test still renders with the existing mocks and the assertion awaiting
ui.actionButtons.edit remains unchanged.


it('Should not render Delete button for users without the delete permission', async () => {
mocks.useRulerRuleAbility.mockImplementation((_rule, _groupIdentifier, action) => {
// Mock the specific hooks needed for Grafana rules
mocks.useAlertRuleAbility.mockImplementation((rule, action) => {
return action === AlertRuleAction.Delete ? [true, false] : [true, true];
});
mocks.useAlertRuleAbility.mockImplementation((_rule, action) => {
return action === AlertRuleAction.Delete ? [true, false] : [true, true];
mocks.useGrafanaPromRuleAbilities.mockImplementation((rule, actions) => {
return actions.map((action) => {
return action === AlertRuleAction.Delete ? [true, false] : [true, true];
});
});

render(<RulesTable rules={[grafanaRule]} />);
Expand All @@ -80,11 +116,14 @@ describe('RulesTable RBAC', () => {
});

it('Should render Edit button for users with the update permission', async () => {
mocks.useRulerRuleAbility.mockImplementation((_rule, _groupIdentifier, action) => {
// Mock the specific hooks needed for Grafana rules
mocks.useAlertRuleAbility.mockImplementation((rule, action) => {
return action === AlertRuleAction.Update ? [true, true] : [false, false];
});
mocks.useAlertRuleAbility.mockImplementation((_rule, action) => {
return action === AlertRuleAction.Update ? [true, true] : [false, false];
mocks.useGrafanaPromRuleAbilities.mockImplementation((rule, actions) => {
return actions.map((action) => {
return action === AlertRuleAction.Update ? [true, true] : [false, false];
});
});

render(<RulesTable rules={[grafanaRule]} />);
Expand All @@ -93,11 +132,14 @@ describe('RulesTable RBAC', () => {
});

it('Should render Delete button for users with the delete permission', async () => {
mocks.useRulerRuleAbility.mockImplementation((_rule, _groupIdentifier, action) => {
// Mock the specific hooks needed for Grafana rules
mocks.useAlertRuleAbility.mockImplementation((rule, action) => {
return action === AlertRuleAction.Delete ? [true, true] : [false, false];
});
mocks.useAlertRuleAbility.mockImplementation((_rule, action) => {
return action === AlertRuleAction.Delete ? [true, true] : [false, false];
mocks.useGrafanaPromRuleAbilities.mockImplementation((rule, actions) => {
return actions.map((action) => {
return action === AlertRuleAction.Delete ? [true, true] : [false, false];
});
});

render(<RulesTable rules={[grafanaRule]} />);
Expand All @@ -123,11 +165,15 @@ describe('RulesTable RBAC', () => {
};

beforeEach(() => {
mocks.useRulerRuleAbility.mockImplementation(() => {
return [true, true];
// Mock all hooks needed for the creating/deleting state tests
mocks.useRulerRuleAbility.mockImplementation(() => [true, true]);
mocks.useAlertRuleAbility.mockImplementation(() => [true, true]);
// Mock plural hooks for AlertRuleMenu
mocks.useRulerRuleAbilities.mockImplementation((_rule, _groupIdentifier, actions) => {
return actions.map(() => [true, true]);
});
mocks.useAlertRuleAbility.mockImplementation(() => {
return [true, true];
mocks.useGrafanaPromRuleAbilities.mockImplementation((_rule, actions) => {
return actions.map(() => [true, true]);
});
});

Expand Down Expand Up @@ -164,6 +210,12 @@ describe('RulesTable RBAC', () => {
mocks.useAlertRuleAbility.mockImplementation((_rule, action) => {
return action === AlertRuleAction.Update ? [true, false] : [true, true];
});
// Cloud rules only need useRulerRuleAbilities mock (useGrafanaPromRuleAbilities gets skipToken)
mocks.useRulerRuleAbilities.mockImplementation((_rule, _groupIdentifier, actions) => {
return actions.map((action) => {
return action === AlertRuleAction.Update ? [true, false] : [true, true];
});
});

render(<RulesTable rules={[cloudRule]} />);

Expand All @@ -177,6 +229,12 @@ describe('RulesTable RBAC', () => {
mocks.useAlertRuleAbility.mockImplementation((_rule, action) => {
return action === AlertRuleAction.Delete ? [true, false] : [true, true];
});
// Cloud rules only need useRulerRuleAbilities mock (useGrafanaPromRuleAbilities gets skipToken)
mocks.useRulerRuleAbilities.mockImplementation((_rule, _groupIdentifier, actions) => {
return actions.map((action) => {
return action === AlertRuleAction.Delete ? [true, false] : [true, true];
});
});

render(<RulesTable rules={[cloudRule]} />);

Expand All @@ -191,6 +249,12 @@ describe('RulesTable RBAC', () => {
mocks.useAlertRuleAbility.mockImplementation((_rule, action) => {
return action === AlertRuleAction.Update ? [true, true] : [false, false];
});
// Cloud rules only need useRulerRuleAbilities mock (useGrafanaPromRuleAbilities gets skipToken)
mocks.useRulerRuleAbilities.mockImplementation((_rule, _groupIdentifier, actions) => {
return actions.map((action) => {
return action === AlertRuleAction.Update ? [true, true] : [false, false];
});
});

render(<RulesTable rules={[cloudRule]} />);

Expand All @@ -204,6 +268,12 @@ describe('RulesTable RBAC', () => {
mocks.useAlertRuleAbility.mockImplementation((_rule, action) => {
return action === AlertRuleAction.Delete ? [true, true] : [false, false];
});
// Cloud rules only need useRulerRuleAbilities mock (useGrafanaPromRuleAbilities gets skipToken)
mocks.useRulerRuleAbilities.mockImplementation((_rule, _groupIdentifier, actions) => {
return actions.map((action) => {
return action === AlertRuleAction.Delete ? [true, true] : [false, false];
});
});

render(<RulesTable rules={[cloudRule]} />);

Expand Down