-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[TIP] Investigate in timeline #140496
[TIP] Investigate in timeline #140496
Conversation
- add investigate in timeline component using basic redux actions duplicated from Security Solution plugin - investigates in timeline adds threat indicator as well as source entries using mapping
- remove first option as well as actions, selectors and types - add new investigate in timeline hook in Security Solution plugin and pass via context to TI plugin - replace UrlOriginal by UrlFull in the threat.indicator.name mapping
# Conflicts: # x-pack/plugins/threat_intelligence/public/modules/timeline/components/add_to_timeline/add_to_timeline.tsx # x-pack/plugins/threat_intelligence/public/modules/timeline/components/add_to_timeline/styles.ts
- split InvestigateInTimeline into 2 components (one for Button display the other for ButtonIcon) - extract most of the logic into useInvestigateInTimeline hook - add unit and e2e tests - remove threat.enrichments entries in the map
ran command: node scripts/build_kibana_platform_plugins --update-limits
@elasticmachine merge upstream |
@elasticmachine merge upstream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
limits.yml
@elasticmachine merge upstream |
const securitySolutionContext: SecuritySolutionPluginContext = { | ||
getFiltersGlobalComponent: () => FiltersGlobal, | ||
getPageWrapper: () => SecuritySolutionPageWrapper, | ||
licenseService, | ||
sourcererDataView: sourcererDataView as unknown as SourcererDataView, | ||
getSecuritySolutionStore: securitySolutionStore, | ||
getUseInvestigateInTimeline: useInvestigateInTimeline, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
LGTM! |
@elasticmachine merge upstream |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall code looks good and the feature works as expect, good job! There are some small comments and suggestions
export const ACTION_INVESTIGATE_IN_TIMELINE = i18n.translate( | ||
'xpack.securitySolution.threatIntelligence.investigateInTimelineTitle', | ||
{ | ||
defaultMessage: 'Investigate in timeline', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember talking with UX writer they mentioned that they always use Timeline capitalized and it's the only feature that behaves this way, let's change to Investigate in Timeline
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though I don't see where this defaultMeassage appears, I'd expect it to show up as a tooltip on the action icon, but it's not there. Btw about tooltips, I see that our expand icon has a tooltip "View Details", but Investigate in Timeline icon doesn't have a tooltip. I think we should add it, but it can be in a separate PR if it's not a one-lines somewhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point. I've added the tooltip here as well as in every other places in the plugin I could find
const { filterManager: activeFilterManager } = useDeepEqualSelector((state) => | ||
getManageTimeline(state, TimelineId.active ?? '') | ||
); | ||
const filterManager = useMemo( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
out of curiosity what this filterManager
does and what is the logic behind filterManagerBackup
?
onOpen={setExpanded} | ||
isOpen={Boolean(expanded && expanded._id === indicator._id)} | ||
/> | ||
<div css={styles.rowActionsDiv}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can EUI flex components be used here, eg EuiFlexGroup
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup done, good cleanup I could delete the styles.ts
file entirely
|
||
return ( | ||
<EuiButton onClick={onClick} fill {...props}> | ||
Investigate in Timeline |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be translated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup thanks, I went through the plugin and added a few more as well
} | ||
|
||
return ( | ||
<div css={styles.inlineFlex}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same question about using flex components from EUI https://elastic.github.io/eui/#/layout/flex
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yup, style.ts
deleted!
|
||
const { key, value } = getIndicatorFieldAndValue(indicator, RawIndicatorFieldId.Name); | ||
|
||
if (!value || value === EMPTY_VALUE || !key) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we repeat this peace of logic in many places, I think we should at least move it to a utility function, ideally think about a model of our Indicator and about methods of this model. The best illustration of why it's bad to repeat business logic even if it's super simple is in indicator_value_actions.tsx
where we have !key || value === EMPTY_VALUE || !key
which is incorrect
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I extracted it, looks much better to me that way, thanks!
- add EuiTooltip for all EuiButtonIcon - add missing translations - replace css with EuiFlexGroup where possible - extract fieldValueValid logic
ab8e9e9
to
eb5538a
Compare
* @returns true if correct, false if not | ||
*/ | ||
export const fieldAndValueValid = (field: string | null, value: string | null): boolean => | ||
value != null && value !== '' && value !== EMPTY_VALUE && field != null && field !== ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any partucular reason to do value != null && value !== ''
instead of !value
. Also not sure why non-strict comparison with null. I'd vote for a simpler version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok I was confused for a sec... !value
will not work with empty strings, but !!value will
. It's cleaned up now! :)
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
History
To update your PR or re-run it, just comment with: |
Pinging @elastic/security-solution (Team: SecuritySolution) |
Summary
This PR adds the Investigate in Timeline functionality to the Indicators table.
Behavior
When the user clicks on the Investigate in Timeline button in the actions column of the Indicators table or the flyout footer, the timeline view is presented with multiple entries already populated:
threat.indicator.file.hash.sha256: 'abc'
)file.hash.sha256: 'abc'
)If a saved timeline was open, or the unsaved default timeline already had some values, a new unsaved timeline is created with the above pairs.
As the investigate in timeline feature is a lot more complex than the more basic add to timeline (see some of the discussions here), and since there isn't anything available from the Security Solution or the Timeline plugin to make it work out of the box (like there was for the Add to Timeline feature), the current state of this PR presents 2 approaches:
Notes:
url
mapping toRawIndicatorFieldId.UrlFull
instead ofRawIndicatorFieldId.UrlOriginal
(see here)node scripts/build_kibana_platform_plugins --update-limits
Screen.Recording.2022-09-12.at.2.18.55.PM.mov
https://github.com/elastic/security-team/issues/4558
Checklist
Delete any items that are not applicable to this PR.