Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Security Solution] Add rule snooze settings on the rule details page #155407

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import { EuiButtonIcon, EuiToolTip } from '@elastic/eui';
import React, { useMemo } from 'react';
import { useUserData } from '../../detections/components/user_info';
import { hasUserCRUDPermission } from '../../common/utils/privileges';
import { useKibana } from '../../common/lib/kibana';
import type { RuleSnoozeSettings } from '../rule_management/logic';
import { useInvalidateFetchRulesSnoozeSettingsQuery } from '../rule_management/api/hooks/use_fetch_rules_snooze_settings';

interface RuleSnoozeBadgeProps {
/**
* Rule's snooze settings, when set to `undefined` considered as a loading state
*/
snoozeSettings: RuleSnoozeSettings | undefined;
/**
* It should represent a user readable error message happened during data snooze settings fetching
*/
error?: string;
showTooltipInline?: boolean;
}

export function RuleSnoozeBadge({
snoozeSettings,
error,
showTooltipInline = false,
}: RuleSnoozeBadgeProps): JSX.Element {
const RulesListNotifyBadge = useKibana().services.triggersActionsUi.getRulesListNotifyBadge;
const [{ canUserCRUD }] = useUserData();
const hasCRUDPermissions = hasUserCRUDPermission(canUserCRUD);
const invalidateFetchRuleSnoozeSettings = useInvalidateFetchRulesSnoozeSettingsQuery();
const isLoading = !snoozeSettings;
const rule = useMemo(() => {
return {
id: snoozeSettings?.id ?? '',
muteAll: snoozeSettings?.mute_all ?? false,
activeSnoozes: snoozeSettings?.active_snoozes ?? [],
isSnoozedUntil: snoozeSettings?.is_snoozed_until
? new Date(snoozeSettings.is_snoozed_until)
: undefined,
snoozeSchedule: snoozeSettings?.snooze_schedule,
isEditable: hasCRUDPermissions,
};
}, [snoozeSettings, hasCRUDPermissions]);

if (error) {
return (
<EuiToolTip content={error}>
<EuiButtonIcon size="s" iconType="bellSlash" disabled />
</EuiToolTip>
);
}

return (
<RulesListNotifyBadge
rule={rule}
isLoading={isLoading}
showTooltipInline={showTooltipInline}
onRuleChanged={invalidateFetchRuleSnoozeSettings}
/>
);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/

import React from 'react';
import { useFetchRulesSnoozeSettings } from '../../../../../rule_management/api/hooks/use_fetch_rules_snooze_settings';
import { RuleSnoozeBadge } from '../../../../../components/rule_snooze_badge';
import * as i18n from './translations';

interface RuleDetailsSnoozeBadge {
/**
* Rule's SO id (not ruleId)
*/
id: string;
}

export function RuleDetailsSnoozeSettings({ id }: RuleDetailsSnoozeBadge): JSX.Element {
const { data: rulesSnoozeSettings, isFetching, isError } = useFetchRulesSnoozeSettings([id]);
const snoozeSettings = rulesSnoozeSettings?.[0];

return (
<RuleSnoozeBadge
snoozeSettings={snoozeSettings}
error={
isError || (!snoozeSettings && !isFetching)
? i18n.UNABLE_TO_FETCH_RULE_SNOOZE_SETTINGS
: undefined
}
showTooltipInline={true}
/>
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
import { i18n } from '@kbn/i18n';

export const UNABLE_TO_FETCH_RULE_SNOOZE_SETTINGS = i18n.translate(
'xpack.securitySolution.detectionEngine.ruleManagement.ruleSnoozeBadge.error.unableToFetch',
'xpack.securitySolution.detectionEngine.ruleDetails.rulesSnoozeSettings.error.unableToFetch',
{
defaultMessage: 'Unable to fetch snooze settings',
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,11 @@ jest.mock('react-router-dom', () => {
};
});

// RuleDetailsSnoozeSettings is an isolated component and not essential for existing tests
jest.mock('./components/rule_details_snooze_settings', () => ({
RuleDetailsSnoozeSettings: () => <></>,
}));
Comment on lines +90 to +92
Copy link
Contributor

Choose a reason for hiding this comment

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

Wow, it's the 14th mock added to this test file 😅.
Honestly, it might be better just to delete this messy test altogether.

Relying heavily on mocks in a single test isn't great for several reasons:

  1. Bad readability: Too many mocks make reading and understanding the test difficult. In this case, there are ~170 LoC mock definitions, roughly half of the test file.
  2. Fragility: Mocks can cause tests to break easily when the code changes. This is because mocks assume certain things about how the system works, and if those things change, the test will fail even if everything else is still working fine.
  3. Violation of encapsulation: Mocks usually rely on implementation detail. This makes introducing code changes without breaking tests hard, slowing development.
  4. Fake test scenarios: When using too many mocks, you basically test mocks instead of actual code. This can cause tests to pass or fail regardless of the existing system behavior.

Copy link
Contributor Author

@maximpn maximpn Apr 21, 2023

Choose a reason for hiding this comment

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

I totally agree with the comment though I'd like to abstain from any other changes in the test file in this PR. It more or less outside of scope here so I'd rather address test file refactoring in a subsequent PR if you don't mind.


const mockRedirectLegacyUrl = jest.fn();
const mockGetLegacyUrlConflict = jest.fn();
jest.mock('../../../../common/lib/kibana', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,7 @@ import { EditRuleSettingButtonLink } from '../../../../detections/pages/detectio
import { useStartMlJobs } from '../../../rule_management/logic/use_start_ml_jobs';
import { useBulkDuplicateExceptionsConfirmation } from '../../../rule_management_ui/components/rules_table/bulk_actions/use_bulk_duplicate_confirmation';
import { BulkActionDuplicateExceptionsConfirmation } from '../../../rule_management_ui/components/rules_table/bulk_actions/bulk_duplicate_exceptions_confirmation';
import { RuleDetailsSnoozeSettings } from './components/rule_details_snooze_settings';

/**
* Need a 100% height here to account for the graph/analyze tool, which sets no explicit height parameters, but fills the available space.
Expand Down Expand Up @@ -539,23 +540,30 @@ const RuleDetailsPageComponent: React.FC<DetectionEngineComponentProps> = ({
const lastExecutionMessage = lastExecution?.message ?? '';

const ruleStatusInfo = useMemo(() => {
return ruleLoading ? (
<EuiFlexItem>
<EuiLoadingSpinner size="m" data-test-subj="rule-status-loader" />
</EuiFlexItem>
) : (
<RuleStatus status={lastExecutionStatus} date={lastExecutionDate}>
<EuiButtonIcon
data-test-subj="refreshButton"
color="primary"
onClick={refreshRule}
iconType="refresh"
aria-label={ruleI18n.REFRESH}
isDisabled={!isExistingRule}
/>
</RuleStatus>
return (
<>
{ruleLoading ? (
<EuiFlexItem>
<EuiLoadingSpinner size="m" data-test-subj="rule-status-loader" />
</EuiFlexItem>
) : (
<RuleStatus status={lastExecutionStatus} date={lastExecutionDate}>
<EuiButtonIcon
data-test-subj="refreshButton"
color="primary"
onClick={refreshRule}
iconType="refresh"
aria-label={ruleI18n.REFRESH}
isDisabled={!isExistingRule}
/>
</RuleStatus>
)}
<EuiFlexItem grow={false}>
<RuleDetailsSnoozeSettings id={ruleId} />
</EuiFlexItem>
</>
);
}, [lastExecutionStatus, lastExecutionDate, ruleLoading, isExistingRule, refreshRule]);
}, [ruleId, lastExecutionStatus, lastExecutionDate, ruleLoading, isExistingRule, refreshRule]);

const ruleError = useMemo(() => {
return ruleLoading ? (
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ export const useRulesTableContextMock = {
rulesSnoozeSettings: {
data: {},
isLoading: false,
isFetching: false,
isError: false,
},
pagination: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,18 @@ import { RuleSource } from './rules_table_saved_state';
import { useRulesTableSavedState } from './use_rules_table_saved_state';

interface RulesSnoozeSettings {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not related to this PR directly.

I was thinking about the snooze state inside the table context, and it seems a bit alien there. If I understood correctly, its purpose is to batch outgoing HTTP requests, i.e., send a single rule snooze request instead of 1 x N table rows. So I think it would be better to create an abstraction just for that. It could be a data hook that doesn't trigger a request immediately but waits some time and accumulates request params if other similar hooks are present on the page. Something similar to the GraphQL dataloader batching functionality but adapted for usage with react query.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, I also had some similar ideas already. My main concern is to make it simple, maintainable and reusable as creating such functionality only for one feature is time consuming. I think we can take the thoughts into account and analyze the possibilities when the snoozing epic is done.

data: Record<string, RuleSnoozeSettings>; // The key is a rule SO's id (not ruleId)
/**
* A map object using rule SO's id (not ruleId) as keys and snooze settings as values
*/
data: Record<string, RuleSnoozeSettings>;
/**
* Sets to true during the first data loading
*/
isLoading: boolean;
/**
* Sets to true during data loading
*/
isFetching: boolean;
isError: boolean;
}

Expand Down Expand Up @@ -290,6 +300,7 @@ export const RulesTableContextProvider = ({ children }: RulesTableContextProvide
const {
data: rulesSnoozeSettings,
isLoading: isSnoozeSettingsLoading,
isFetching: isSnoozeSettingsFetching,
isError: isSnoozeSettingsFetchError,
refetch: refetchSnoozeSettings,
} = useFetchRulesSnoozeSettings(
Expand Down Expand Up @@ -349,6 +360,7 @@ export const RulesTableContextProvider = ({ children }: RulesTableContextProvide
rulesSnoozeSettings: {
data: rulesSnoozeSettingsMap,
isLoading: isSnoozeSettingsLoading,
isFetching: isSnoozeSettingsFetching,
isError: isSnoozeSettingsFetchError,
},
pagination: {
Expand Down Expand Up @@ -382,6 +394,7 @@ export const RulesTableContextProvider = ({ children }: RulesTableContextProvide
rules,
rulesSnoozeSettings,
isSnoozeSettingsLoading,
isSnoozeSettingsFetching,
isSnoozeSettingsFetchError,
page,
perPage,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,3 +21,10 @@ export const ML_RULE_JOBS_WARNING_BUTTON_LABEL = i18n.translate(
defaultMessage: 'Visit rule details page to investigate',
}
);

export const UNABLE_TO_FETCH_RULES_SNOOZE_SETTINGS = i18n.translate(
'xpack.securitySolution.detectionEngine.ruleManagement.rulesSnoozeSettings.error.unableToFetch',
{
defaultMessage: 'Unable to fetch snooze settings',
}
);
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import type {
} from '../../../../../common/detection_engine/rule_monitoring';
import { isMlRule } from '../../../../../common/machine_learning/helpers';
import { getEmptyTagValue } from '../../../../common/components/empty_value';
import { RuleSnoozeBadge } from '../../../rule_management/components/rule_snooze_badge';
import { RuleSnoozeBadge } from '../../../components/rule_snooze_badge';
import { FormattedRelativePreferenceDate } from '../../../../common/components/formatted_date';
import { SecuritySolutionLinkAnchor } from '../../../../common/components/links';
import { getRuleDetailsTabUrl } from '../../../../common/components/link_to/redirect_to_detection_engine';
Expand All @@ -46,6 +46,7 @@ import { useHasActionsPrivileges } from './use_has_actions_privileges';
import { useHasMlPermissions } from './use_has_ml_permissions';
import { useRulesTableActions } from './use_rules_table_actions';
import { MlRuleWarningPopover } from './ml_rule_warning_popover';
import * as rulesTableI18n from './translations';

export type TableColumn = EuiBasicTableColumn<Rule> | EuiTableActionsColumnType<Rule>;

Expand Down Expand Up @@ -108,15 +109,33 @@ const useEnabledColumn = ({ hasCRUDPermissions, startMlJobs }: ColumnsProps): Ta
};

const useRuleSnoozeColumn = (): TableColumn => {
const {
state: { rulesSnoozeSettings },
} = useRulesTableContext();

return useMemo(
() => ({
field: 'snooze',
name: i18n.COLUMN_SNOOZE,
render: (_, rule: Rule) => <RuleSnoozeBadge id={rule.id} />,
render: (_, rule: Rule) => {
const snoozeSettings = rulesSnoozeSettings.data[rule.id];
const { isFetching, isError } = rulesSnoozeSettings;

return (
<RuleSnoozeBadge
snoozeSettings={snoozeSettings}
error={
isError || (!snoozeSettings && !isFetching)
? rulesTableI18n.UNABLE_TO_FETCH_RULES_SNOOZE_SETTINGS
: undefined
}
/>
);
},
Comment on lines +112 to +134
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to abstract the settings and error retrieval logic into a reusable hook. Something like:

const { snoozingSettings, error } = useRuleSnoozingSettings(rule.id)

Moreover, this logic is duplicated in the RuleDetailsSnoozeSettings component. It would be better to encapsulate it inside the RuleSnoozeBadge component instead of handling the data access part outside. The component might try to get the rule settings from the table context if it's available and fall back to data fetching in other cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I'm gonna address this in the next PR for adding support of rule snooze on the rule editing page.

width: '100px',
sortable: false,
}),
[]
[rulesSnoozeSettings]
);
};

Expand Down