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

[SecuritySolution] Turn prevalence count into a button #133791

Conversation

janmonschke
Copy link
Contributor

Summary

After some feedback with design and product, we decided to remove the Investigate in timeline column and make the prevalence count a button that achieves the same thing. Now the interaction ties in better with other parts of the Kibana UI where we show counts to start a timeline investigation and it uses less space in the alert flyout.

Before After
Screenshot 2022-06-07 at 17 21 56 Screenshot 2022-06-07 at 17 21 19

Checklist

- Ties in with other parts of the UI where we show counts that are actionable and start a timeline investigation
- Uses less space in the flyout
@janmonschke janmonschke added release_note:skip Skip the PR/issue when compiling release notes auto-backport Deprecated - use backport:version if exact versions are needed Team:Threat Hunting:Investigations Security Solution Investigations Team v8.3.0 v8.3.1 labels Jun 7, 2022
@janmonschke janmonschke self-assigned this Jun 7, 2022
@janmonschke janmonschke requested a review from a team as a code owner June 7, 2022 15:33
@@ -113,22 +96,4 @@ describe('AddToTimelineCellRenderer', () => {
expect(screen.queryByLabelText(ACTION_INVESTIGATE_IN_TIMELINE)).not.toBeInTheDocument();
});
});

describe('When the field is the host status field', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test is not required anymore since no prevalence data is returned for this field and the button therefore is not rendered.

@@ -0,0 +1,88 @@
/*
Copy link
Contributor Author

@janmonschke janmonschke Jun 7, 2022

Choose a reason for hiding this comment

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

I renamed this file, so tracking changes in this file is a bit cumbersome. I annotated the changes for y'all's convenience.

Copy link
Contributor

Choose a reason for hiding this comment

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

💯

Comment on lines +24 to +25
React.PropsWithChildren<AlertSummaryRow['description']>
>(({ data, eventId, fieldFromBrowserField, linkValue, timelineId, values, children }) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added children to type and extracted it from props

}
}, [dispatch, clearTimeline, actionCellConfig]);

const showButton = values != null && !isEmpty(actionCellConfig?.dataProvider);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed const fieldHasActionsEnabled = hasHoverOrRowActions(data.field); since the logic is not required anymore.

Comment on lines +74 to +81
<EuiButtonEmpty
aria-label={ACTION_INVESTIGATE_IN_TIMELINE}
onClick={configureAndOpenTimeline}
flush="right"
size="xs"
>
{children}
</EuiButtonEmpty>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uses a different button than it did before

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
securitySolution 5.1MB 5.1MB +19.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @janmonschke

Copy link
Contributor

@michaelolo24 michaelolo24 left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for doing. LGTM!

Copy link
Contributor

@kqualters-elastic kqualters-elastic left a comment

Choose a reason for hiding this comment

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

desk tested locally, works as expected lgtm 👍

@kqualters-elastic kqualters-elastic merged commit dd95fa2 into elastic:main Jun 8, 2022
kibanamachine pushed a commit that referenced this pull request Jun 8, 2022
* feat: turn prevalence count into a button

- Ties in with other parts of the UI where we show counts that are actionable and start a timeline investigation
- Uses less space in the flyout

* fix: remove unused variable

* [CI] Auto-commit changed files from 'node scripts/precommit_hook.js --ref HEAD~1..HEAD --fix'

Co-authored-by: kibanamachine <[email protected]>
(cherry picked from commit dd95fa2)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.3

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Jun 8, 2022
…3957)

* feat: turn prevalence count into a button

- Ties in with other parts of the UI where we show counts that are actionable and start a timeline investigation
- Uses less space in the flyout

* fix: remove unused variable

* [CI] Auto-commit changed files from 'node scripts/precommit_hook.js --ref HEAD~1..HEAD --fix'

Co-authored-by: kibanamachine <[email protected]>
(cherry picked from commit dd95fa2)

Co-authored-by: Jan Monschke <[email protected]>
@ghost
Copy link

ghost commented Jun 14, 2022

@janmonschke tested pr on local kibana and targeted issue is now fixed . prevalence count into a button and got added in investigation✔️

10.0.6.219.-.Remote.Desktop.Connection.2022-06-14.18-21-40.mp4

@LeeDr LeeDr removed the v8.3.1 label Jun 29, 2022
Comment on lines +61 to +62
selectedDataViewId: 'security-solution-default',
selectedPatterns: ['.alerts-security.alerts-default'],
Copy link
Contributor

Choose a reason for hiding this comment

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

The data view ids are space sensitive. You should just get them off of redux, sourcerer.defaultDataView.id and sourcerer.signalIndexName

Above, call

import { sourcererSelectors } from '../../store';
 
const getDataViewsSelector = useMemo(
  () => sourcererSelectors.getSourcererDataViewsSelector(), []
);
const {
  defaultDataView,
  signalIndexName,
} = useDeepEqualSelector((state) => getDataViewsSelector(state));

and then here we would call

 selectedDataViewId: defaultDataView.id,
 selectedPatterns: [signalIndexName],

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed release_note:skip Skip the PR/issue when compiling release notes Team:Threat Hunting:Investigations Security Solution Investigations Team v8.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants