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.

Sign up for GitHub

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

Rule flyout opening from Findings and Alerts page #219

Merged
merged 7 commits into from
Dec 16, 2022
Merged

Rule flyout opening from Findings and Alerts page #219

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
be9f14e
rule flyout opening from findings and alerts page
djindjic Dec 13, 2022
8ad1b1f
update cypress test for findings page
djindjic Dec 13, 2022
264c4ea
make code shorter
djindjic Dec 15, 2022
dae1c8f
toast error notifications for rule deletion
djindjic Dec 15, 2022
dbb89b2
cleanup component state
djindjic Dec 15, 2022
547b8ae
avoid as any in favor of RuleItemInfoBase
djindjic Dec 15, 2022
250a31e
fix cypress test for rules
djindjic Dec 16, 2022
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
21 changes: 14 additions & 7 deletions cypress/integration/4_findings.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
* SPDX-License-Identifier: Apache-2.0
*/

import { PLUGIN_NAME } from '../support/constants';
import { PLUGIN_NAME, TWENTY_SECONDS_TIMEOUT } from '../support/constants';
import sample_document from '../fixtures/sample_document.json';
import sample_index_settings from '../fixtures/sample_index_settings.json';
import sample_field_mappings from '../fixtures/sample_field_mappings.json';
Expand Down Expand Up @@ -106,14 +106,21 @@ describe('Findings', () => {
// TODO - upon reaching rules page, trigger appropriate rules detail flyout
// see github issue #124 at https://github.com/opensearch-project/security-analytics-dashboards-plugin/issues/124

it('takes user to rules page when rule name inside accordion drop down is clicked', () => {
it('opens rule details flyout when rule name inside accordion drop down is clicked', () => {
// Click rule link
cy.get(`[data-test-subj="finding-details-flyout-USB Device Plugged-details"]`)
.invoke('removeAttr', 'target')
.click({ force: true });
cy.get(`[data-test-subj="finding-details-flyout-USB Device Plugged-details"]`).click({
force: true,
});

// Confirm destination reached
cy.url().should('include', 'opensearch_security_analytics_dashboards#/rules');
// Validate flyout appearance
cy.get('[data-test-subj="rule_flyout_USB Device Plugged"]', TWENTY_SECONDS_TIMEOUT).within(
() => {
cy.get('[data-test-subj="rule_flyout_rule_name"]', TWENTY_SECONDS_TIMEOUT).contains(
'USB Device Plugged',
TWENTY_SECONDS_TIMEOUT
);
}
);
});

after(() => {
Expand Down
42 changes: 39 additions & 3 deletions public/pages/Findings/components/FindingDetailsFlyout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,16 +25,21 @@ import {
import { capitalizeFirstLetter, renderTime } from '../../../utils/helpers';
import { DEFAULT_EMPTY_DATA, ROUTES } from '../../../utils/constants';
import { Finding, Query } from '../models/interfaces';
import { RuleViewerFlyout } from '../../Rules/components/RuleViewerFlyout/RuleViewerFlyout';
import { RuleTableItem } from '../../Rules/utils/helpers';
import { RuleSource } from '../../../../server/models/interfaces';

interface FindingDetailsFlyoutProps {
finding: Finding;
closeFlyout: () => void;
backButton?: React.ReactNode;
allRules: object;
allRules: { [id: string]: RuleSource };
}

interface FindingDetailsFlyoutState {
loading: boolean;
ruleViewerFlyoutShown: boolean;
ruleViewerFlyoutData: RuleTableItem | null;
}

export default class FindingDetailsFlyout extends Component<
Expand All @@ -45,6 +50,8 @@ export default class FindingDetailsFlyout extends Component<
super(props);
this.state = {
loading: false,
ruleViewerFlyoutShown: false,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This boolean seems redundant since we are anyways rendering the flyout based on the data. I think just ruleViewerFlyoutData is enough for check. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totally right. I've cleaned it up

ruleViewerFlyoutData: null,
};
}

Expand All @@ -62,6 +69,28 @@ export default class FindingDetailsFlyout extends Component<
);
};

showRuleDetails = (fullRule, ruleId: string) => {
this.setState({
...this.state,
ruleViewerFlyoutShown: true,
ruleViewerFlyoutData: {
ruleId: ruleId,
title: fullRule.title,
level: fullRule.level,
category: fullRule.category,
description: fullRule.description,
source: fullRule.source,
ruleInfo: {
_source: fullRule,
} as any,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need to use as any here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed it to as RuleItemInfoBase which is more appropriate.

TS assertion is needed because we don't have all the data to construct whole object. Other properties are anyway not in use in the Rule flyout.

Let me know if you have some better idea.

},
});
};

hideRuleDetails = () => {
this.setState({ ...this.state, ruleViewerFlyoutShown: false, ruleViewerFlyoutData: null });
};

renderRuleDetails = (rules: Query[] = []) => {
const {
allRules,
Expand Down Expand Up @@ -94,8 +123,7 @@ export default class FindingDetailsFlyout extends Component<
{/*//TODO: Refactor EuiLink to filter rules table to the specific rule.*/}
<EuiFormRow label={'Rule name'}>
<EuiLink
href={`#${ROUTES.RULES}`}
target={'_blank'}
onClick={() => this.showRuleDetails(fullRule, rule.id)}
data-test-subj={`finding-details-flyout-${fullRule.title}-details`}
>
{fullRule.title || DEFAULT_EMPTY_DATA}
Expand Down Expand Up @@ -208,6 +236,13 @@ export default class FindingDetailsFlyout extends Component<
hideCloseButton
data-test-subj={'finding-details-flyout'}
>
{this.state.ruleViewerFlyoutShown && this.state.ruleViewerFlyoutData && (
<RuleViewerFlyout
hideFlyout={this.hideRuleDetails}
ruleTableItem={this.state.ruleViewerFlyoutData}
/>
)}

<EuiFlyoutHeader hasBorder={true}>
<EuiFlexGroup justifyContent="flexStart" alignItems="center">
<EuiFlexItem>
Expand All @@ -222,6 +257,7 @@ export default class FindingDetailsFlyout extends Component<
</EuiFlexItem>
<EuiFlexItem grow={false}>
<EuiButtonIcon
aria-label="close"
iconType="cross"
display="empty"
iconSize="m"
Expand Down
42 changes: 35 additions & 7 deletions public/pages/Rules/components/RuleViewerFlyout/RuleViewerFlyout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
EuiFlyoutBody,
EuiFlyoutHeader,
EuiTitle,
EuiButtonIcon,
} from '@elastic/eui';
import { ROUTES } from '../../../../utils/constants';
import React, { useMemo, useState } from 'react';
Expand All @@ -21,9 +22,9 @@ import { RuleViewerFlyoutHeaderActions } from './RuleViewFlyoutHeaderActions';
import { RuleService } from '../../../../services';

export interface RuleViewerFlyoutProps {
history: RouteComponentProps['history'];
history?: RouteComponentProps['history'];
ruleTableItem: RuleTableItem;
ruleService: RuleService;
ruleService?: RuleService;
hideFlyout: (refreshRules?: boolean) => void;
}

Expand All @@ -42,13 +43,19 @@ export const RuleViewerFlyout: React.FC<RuleViewerFlyoutProps> = ({
setActionsPopoverOpen(false);
};
const duplicateRule = () => {
if (!history) {
return;
}
history.push({
Copy link
Collaborator

Choose a reason for hiding this comment

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

Club together into

history?.push(...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

pathname: ROUTES.RULES_DUPLICATE,
state: { ruleItem: ruleTableItem.ruleInfo },
});
};

const editRule = () => {
if (!history) {
return;
}
history.push({
pathname: ROUTES.RULES_EDIT,
state: { ruleItem: ruleTableItem.ruleInfo },
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we clubbed into

history?.push(...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Expand All @@ -74,6 +81,9 @@ export const RuleViewerFlyout: React.FC<RuleViewerFlyoutProps> = ({
};

const onDeleteRuleConfirmed = async () => {
if (!ruleService) {
return;
}
const deleteRuleRes = await ruleService.deleteRule(ruleTableItem.ruleId);

if (!deleteRuleRes.ok) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's fix this Todo? we can show an error toast saying Failed to delete rule

Copy link
Contributor Author

Choose a reason for hiding this comment

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

check this commit, I think its a right error handling here now:
dae1c8f#diff-ddf4d433db9a97c023d49ad7aeb681cf5376c5248d42f51246485942e687863fR86

Expand All @@ -97,17 +107,35 @@ export const RuleViewerFlyout: React.FC<RuleViewerFlyoutProps> = ({
);

return (
<EuiFlyout onClose={hideFlyout} data-test-subj={`rule_flyout_${ruleTableItem.title}`}>
<EuiFlyout
onClose={hideFlyout}
hideCloseButton
ownFocus={true}
size={'m'}
data-test-subj={`rule_flyout_${ruleTableItem.title}`}
>
{isDeleteModalVisible && deleteModal ? deleteModal : null}
<EuiFlyoutHeader hasBorder>
<EuiFlexGroup>
<EuiFlyoutHeader hasBorder={true}>
<EuiFlexGroup alignItems="center">
<EuiFlexItem>
<EuiTitle size="m">
<h3>{ruleTableItem.title}</h3>
</EuiTitle>
</EuiFlexItem>
<EuiFlexItem grow={false} style={{ marginRight: '50px' }}>
{headerActions}
{ruleService && history && (
<EuiFlexItem grow={false} style={{ marginRight: '50px' }}>
{headerActions}
</EuiFlexItem>
)}
<EuiFlexItem grow={false}>
<EuiButtonIcon
aria-label="close"
iconType="cross"
display="empty"
iconSize="m"
onClick={() => hideFlyout()}
data-test-subj={`close-finding-details-flyout`}
/>
</EuiFlexItem>
</EuiFlexGroup>
</EuiFlyoutHeader>
Expand Down
4 changes: 2 additions & 2 deletions public/utils/helpers.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,10 +65,10 @@ export function createTextDetailsGroup(
) : (
<>
<EuiFlexGroup>
{data.map(({ label, content, url }) => {
{data.map(({ label, content, url }, index) => {
return (
<EuiFlexItem
key={label}
key={index}
amsiglan marked this conversation as resolved.
Show resolved Hide resolved
grow={false}
style={{ minWidth: `${100 / (columnNum || data.length)}%` }}
>
Expand Down