[Endpoint] Adds take action dropdown and tests to alert details flyout #59242

dplumlee · 2020-03-03T22:53:08Z

Summary

Adds a take action dropdown to the alert details flyout as a base for the close alert and whitlelisting actions to be created on. Also fleshes out the alert detail flyout jest and functional tests. It also recreates the es-archive files for endpoint management and alerts to match the new sample data generator and updates the functional/api integration tests accordingly

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser
This was checked for cross-browser compatibility, including a check against IE11

For maintainers

This was checked for breaking API changes and was labeled appropriately

peluja1012 · 2020-03-04T09:41:57Z

.../endpoint/public/applications/endpoint/view/alerts/details/overview/take_action_dropdown.tsx

+
+export const TakeActionDropdown = styled(
+  memo(({ className }: { className?: string }) => {
+    const [isDropdownOpen, setIsDropdownOpen] = useState(false);


@oatkiller @paul-tavares @dplumlee Wanted to get your thoughts on local component state. Here Davis is using local component state to keep track of whether the dropdown is open or closed. In the past, we have leaned towards keeping all application state, including local component state, in the redux store. It's a bit different now because we have been using EUI components heavily and they use local component state internally. So currently we don't store all application state in the redux store.

My main question for you is: For reusable components that we create, do we want to we want to store the state in the redux store or use local state?

If we wanted to store reusable component state in the redux store, we could create new versions of helpers like tacotruck activatable.

question. can the popover et al work without any external state?

👍 I agree that for re-usable components, local state (or props) should be used to drive the component's functionality and for them to not have state stored in Redux (especially if there is a chance of these reusable components to be present on a view multiple times).

💭 That being said - this is a fuzzy boundary (IMO). Normally (I think) redux/store is helpful when wanting to share state across the entire application, but the way we are using it is more as a global place to store state that applies only to the given view. One could also argue that we could use local state in that case as well (especially around Views, which act more like Container Components). I can tell you that as I work more with the Ingest code base, which is all built around local state and the use of hooks, the dev. mental model is allot simpler.

I think this is fine. if some other component needs to know about the state then we should reconsider at that point.
In this case, it's not much different than having a native <select /> maintain its own open/closed state.

.../endpoint/public/applications/endpoint/view/alerts/details/overview/take_action_dropdown.tsx

elasticmachine · 2020-03-04T09:51:28Z

Pinging @elastic/endpoint-app-team (Feature:Endpoint)

elasticmachine · 2020-03-04T09:51:42Z

Pinging @elastic/endpoint-response (Team:Endpoint Response)

.../endpoint/public/applications/endpoint/view/alerts/details/overview/take_action_dropdown.tsx

peluja1012 · 2020-03-16T09:49:53Z

x-pack/test/functional/apps/endpoint/alert.ts

      await pageObjects.endpointAlerts.submitSearchBarFilter();
      const currentUrl = await browser.getCurrentUrl();
      expect(currentUrl).to.contain('query=');
      expect(currentUrl).to.contain('date_range=');
+      await pageObjects.endpointAlerts.enterSearchBarQuery('');
+      await pageObjects.endpointAlerts.submitSearchBarFilter();


We should put these in an after block. I suggest doing something like:

describe('when submitting a new bar query', () => { before(async () => { await pageObjects.endpointAlerts.enterSearchBarQuery('test query'); await pageObjects.endpointAlerts.submitSearchBarFilter(); }); it('should update the url correctly', () => {}); after(async () => { await pageObjects.endpointAlerts.enterSearchBarQuery(''); await pageObjects.endpointAlerts.submitSearchBarFilter(); }); });

peluja1012 · 2020-03-16T09:50:36Z

x-pack/test/functional/apps/endpoint/index.ts

@@ -15,6 +15,6 @@ export default function({ loadTestFile }: FtrProviderContext) {
    loadTestFile(require.resolve('./management'));
    loadTestFile(require.resolve('./policy_list'));
    loadTestFile(require.resolve('./policy_details'));
-    loadTestFile(require.resolve('./alert_list'));
+    loadTestFile(require.resolve('./alert'));


We should rename this file to alerts.ts instead of alert.ts.

oatkiller · 2020-03-12T14:22:52Z

x-pack/test/functional/config.js

@@ -88,6 +88,7 @@ export default async function({ readConfigFile }) {
        '--telemetry.banner=false',
        '--timelion.ui.enabled=true',
        '--xpack.endpoint.enabled=true',
+        '--xpack.endpoint.alertResultListDefaultDateRange.from=2018-01-10T00:00:00.000Z',


oatkiller · 2020-03-16T18:56:05Z

x-pack/plugins/endpoint/public/applications/endpoint/view/alerts/alert_details.test.tsx

+import { mockAlertResultList } from '../../store/alerts/mock_alert_result_list';
+import { DepsStartMock, depsStartMock } from '../../mocks';
+
+describe('when the alert details flyout is open', () => {


This description doesn't match the setup logic. Also, it seems very close to what is here:
https://github.com/elastic/kibana/blob/master/x-pack/plugins/endpoint/public/applications/endpoint/view/alerts/index.test.tsx#L23

I think these specs should be parts of that. The setup logic can be reused that way. Also, the tests are about the same feature set.

abstracted the building render logic out into its own function

oatkiller · 2020-03-16T18:58:49Z

x-pack/plugins/endpoint/public/applications/endpoint/view/alerts/alert_details.test.tsx

+    beforeEach(() => {
+      reactTestingLibrary.act(() => {
+        history.push({
+          ...history.location,


is this line necessary?

oatkiller · 2020-03-16T19:14:01Z

.../endpoint/public/applications/endpoint/view/alerts/details/overview/take_action_dropdown.tsx

+export const TakeActionDropdown = memo(() => {
+  const [isDropdownOpen, setIsDropdownOpen] = useState(false);
+
+  const TakeActionButton = useMemo(() => {


Creating a new component in the render method of another component is a bit nonstandard. The component itself will be redefined when isDropdownOpen is changed. For consistency with the recommended react patterns, consider defining this component once.

Here's a commit with the changes I'd recommend: oatkiller@98bca4a

oatkiller · 2020-03-16T19:16:39Z

.../endpoint/public/applications/endpoint/view/alerts/details/overview/take_action_dropdown.tsx

+      button={TakeActionButton}
+      isOpen={isDropdownOpen}
+      anchorPosition="downRight"
+      closePopover={() => setIsDropdownOpen(false)}


please use useCallback here to prevent EuiPopover from rendering too often

oatkiller · 2020-03-16T19:17:28Z

.../endpoint/public/applications/endpoint/view/alerts/details/overview/take_action_dropdown.tsx

+        <EuiButtonEmpty
+          data-test-subj="alertDetailTakeActionCloseAlertButton"
+          color="text"
+          onClick={() => {}}


Can you remove this function? It shouldn't do anything, and having a new function reference on each render will force EuiButtonEmpty to rerender each time

oatkiller · 2020-03-16T19:17:37Z

.../endpoint/public/applications/endpoint/view/alerts/details/overview/take_action_dropdown.tsx

+        <EuiButtonEmpty
+          data-test-subj="alertDetailTakeActionWhitelistButton"
+          color="text"
+          onClick={() => {}}


Can you remove this?

oatkiller · 2020-03-16T19:19:09Z

x-pack/test/functional/apps/endpoint/alerts.ts

+        expect(currentUrl).to.contain('date_range=');
+      });
+      after(async () => {
+        await pageObjects.endpointAlerts.enterSearchBarQuery('');


Why do you need this? is the page not being reloaded after each test?

it's not being reloaded after each test, seemingly only for each file, so this just clears out the state so as to not impede further tests in this file with narrowed filter conditions

kibanamachine · 2020-03-16T23:24:16Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: bbfd954

History

💚 Build #33614 succeeded 4ecb496
💚 Build #33507 succeeded 840e94a
💚 Build #33328 succeeded 90a419d
💔 Build #33291 failed 41e05a8
💔 Build #32839 failed 8a06655

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

elastic#59242) * adds dropdown * changes i18n fields * switches to buttons * adds tests for alert details flyout * updates es archiver data * finishes functional and react tests * cleanup tests for alerts * updates alert esarchive data * replaces es archives and fixes tests * rebase * fixes functional tests * suggested changes to take action button * addresses comments Co-authored-by: oatkiller <robert.austin@elastic.co>

* master: [SIEM] Adds 'Closes one signal when more than one opened signals are selected' test again (elastic#60380) [SIEM][Detections Engine] - Add rule markdown field to rule create, detail, and edit flows (elastic#60108) [Fleet] Add config revision to fleet agents (elastic#60292) Allow kbn-config-schema to ignore unknown keys (elastic#59560) [ML] Functional tests - disable df analytics clone tests skip flaky suite (elastic#58643) (elastic#58991) [FTR] Add support for --include and --exclude files via tags (elastic#60123) [SIEM] Fix link on overview page (elastic#60348) skip flaky test (elastic#60369) [Endpoint] Adds take action dropdown and tests to alert details flyout (elastic#59242) [Lens] Simplify state management from visualization (elastic#58279) Changing default type to start and allowing it to be configured by the event category (elastic#60323) [ML] Adds the class_assignment_objective to classification (elastic#60358)

* master: (51 commits) do not update cell background if is label cell (elastic#60308) FTR configurable test users (elastic#52431) [Reporting] Wholesale moves client to newest-platform (elastic#58945) [Ingest] Support `show_user` package registry flag (elastic#60338) [SIEM] Adds 'Closes one signal when more than one opened signals are selected' test again (elastic#60380) [SIEM][Detections Engine] - Add rule markdown field to rule create, detail, and edit flows (elastic#60108) [Fleet] Add config revision to fleet agents (elastic#60292) Allow kbn-config-schema to ignore unknown keys (elastic#59560) [ML] Functional tests - disable df analytics clone tests skip flaky suite (elastic#58643) (elastic#58991) [FTR] Add support for --include and --exclude files via tags (elastic#60123) [SIEM] Fix link on overview page (elastic#60348) skip flaky test (elastic#60369) [Endpoint] Adds take action dropdown and tests to alert details flyout (elastic#59242) [Lens] Simplify state management from visualization (elastic#58279) Changing default type to start and allowing it to be configured by the event category (elastic#60323) [ML] Adds the class_assignment_objective to classification (elastic#60358) [TSVB] fix text color when using custom background color (elastic#60261) Fix import to timefilter from in TSVB (elastic#60296) [NP] Get rid of usage redirectWhenMissing service (elastic#59777) ...

* alerting/view-in-app: (53 commits) fixed typo handle optional alerting plugin do not update cell background if is label cell (elastic#60308) FTR configurable test users (elastic#52431) [Reporting] Wholesale moves client to newest-platform (elastic#58945) [Ingest] Support `show_user` package registry flag (elastic#60338) [SIEM] Adds 'Closes one signal when more than one opened signals are selected' test again (elastic#60380) [SIEM][Detections Engine] - Add rule markdown field to rule create, detail, and edit flows (elastic#60108) [Fleet] Add config revision to fleet agents (elastic#60292) Allow kbn-config-schema to ignore unknown keys (elastic#59560) [ML] Functional tests - disable df analytics clone tests skip flaky suite (elastic#58643) (elastic#58991) [FTR] Add support for --include and --exclude files via tags (elastic#60123) [SIEM] Fix link on overview page (elastic#60348) skip flaky test (elastic#60369) [Endpoint] Adds take action dropdown and tests to alert details flyout (elastic#59242) [Lens] Simplify state management from visualization (elastic#58279) Changing default type to start and allowing it to be configured by the event category (elastic#60323) [ML] Adds the class_assignment_objective to classification (elastic#60358) [TSVB] fix text color when using custom background color (elastic#60261) ...

kibanamachine · 2020-03-18T14:38:45Z

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 59242 or prevent reminders by adding the backport:skip label.

elastic#59242) * adds dropdown * changes i18n fields * switches to buttons * adds tests for alert details flyout * updates es archiver data * finishes functional and react tests * cleanup tests for alerts * updates alert esarchive data * replaces es archives and fixes tests * rebase * fixes functional tests * suggested changes to take action button * addresses comments Co-authored-by: oatkiller <robert.austin@elastic.co>

kibanamachine · 2020-03-19T14:48:43Z

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

#59242) (#60598) * adds dropdown * changes i18n fields * switches to buttons * adds tests for alert details flyout * updates es archiver data * finishes functional and react tests * cleanup tests for alerts * updates alert esarchive data * replaces es archives and fixes tests * rebase * fixes functional tests * suggested changes to take action button * addresses comments Co-authored-by: oatkiller <robert.austin@elastic.co> Co-authored-by: Davis Plumlee <56367316+dplumlee@users.noreply.github.com> Co-authored-by: oatkiller <robert.austin@elastic.co>

dplumlee changed the title ~~[Endpoint] Adds dropdown~~ [Endpoint] Adds take action dropdown to alert details flyout Mar 3, 2020

peluja1012 reviewed Mar 4, 2020

View reviewed changes

.../endpoint/public/applications/endpoint/view/alerts/details/overview/take_action_dropdown.tsx Outdated Show resolved Hide resolved

peluja1012 reviewed Mar 4, 2020

View reviewed changes

.../endpoint/public/applications/endpoint/view/alerts/details/overview/take_action_dropdown.tsx Outdated Show resolved Hide resolved

peluja1012 reviewed Mar 4, 2020

View reviewed changes

.../endpoint/public/applications/endpoint/view/alerts/details/overview/take_action_dropdown.tsx Outdated Show resolved Hide resolved

peluja1012 added the Feature:Endpoint Elastic Endpoint feature label Mar 4, 2020

peluja1012 added the Team:Endpoint Response Endpoint Response Team label Mar 4, 2020

peluja1012 reviewed Mar 4, 2020

View reviewed changes

.../endpoint/public/applications/endpoint/view/alerts/details/overview/take_action_dropdown.tsx Outdated Show resolved Hide resolved

dplumlee force-pushed the alert-take-action-dropdown branch 2 times, most recently from 02b4f58 to aae466a Compare March 11, 2020 04:04

dplumlee added release_note:skip Skip the PR/issue when compiling release notes v7.7.0 v8.0.0 labels Mar 11, 2020

dplumlee marked this pull request as ready for review March 11, 2020 21:46

dplumlee requested a review from a team as a code owner March 11, 2020 21:46

dplumlee force-pushed the alert-take-action-dropdown branch 3 times, most recently from 8a06655 to 41e05a8 Compare March 15, 2020 04:28

dplumlee changed the title ~~[Endpoint] Adds take action dropdown to alert details flyout~~ [Endpoint] Adds take action dropdown and tests to alert details flyout Mar 15, 2020

dplumlee force-pushed the alert-take-action-dropdown branch 4 times, most recently from 8718999 to 90a419d Compare March 16, 2020 03:35

peluja1012 reviewed Mar 16, 2020

View reviewed changes

dplumlee force-pushed the alert-take-action-dropdown branch from 90a419d to 840e94a Compare March 16, 2020 17:12

peluja1012 approved these changes Mar 16, 2020

View reviewed changes

oatkiller suggested changes Mar 16, 2020

View reviewed changes

dplumlee and others added 13 commits March 16, 2020 15:39

adds dropdown

c81affe

changes i18n fields

619e648

switches to buttons

4290212

adds tests for alert details flyout

7cbd0e3

updates es archiver data

bc74cd8

finishes functional and react tests

668f53d

cleanup tests for alerts

351abad

updates alert esarchive data

1dbd1d9

replaces es archives and fixes tests

9d0fc99

rebase

10b8a5a

fixes functional tests

9cc7b16

suggested changes to take action button

4e931e9

addresses comments

bbfd954

dplumlee force-pushed the alert-take-action-dropdown branch from 4ecb496 to bbfd954 Compare March 16, 2020 21:43

kqualters-elastic approved these changes Mar 17, 2020

View reviewed changes

oatkiller approved these changes Mar 17, 2020

View reviewed changes

peluja1012 merged commit 53d23fc into elastic:master Mar 17, 2020

dplumlee deleted the alert-take-action-dropdown branch March 17, 2020 15:47

kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 18, 2020

peluja1012 mentioned this pull request Mar 19, 2020

[7.x] [Endpoint] Adds take action dropdown and tests to alert details flyout (#59242) #60598

Merged

kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Endpoint] Adds take action dropdown and tests to alert details flyout #59242

[Endpoint] Adds take action dropdown and tests to alert details flyout #59242

dplumlee commented Mar 3, 2020 •

edited

Loading

peluja1012 Mar 4, 2020

oatkiller Mar 4, 2020

paul-tavares Mar 4, 2020

oatkiller Mar 4, 2020

elasticmachine commented Mar 4, 2020

elasticmachine commented Mar 4, 2020

peluja1012 Mar 16, 2020 •

edited

Loading

peluja1012 Mar 16, 2020

oatkiller Mar 12, 2020

oatkiller Mar 16, 2020

dplumlee Mar 16, 2020

oatkiller Mar 16, 2020

oatkiller Mar 16, 2020

oatkiller Mar 16, 2020

oatkiller Mar 16, 2020

oatkiller Mar 16, 2020

oatkiller Mar 16, 2020

oatkiller Mar 16, 2020

dplumlee Mar 16, 2020 •

edited

Loading

kibanamachine commented Mar 16, 2020

kibanamachine commented Mar 18, 2020

kibanamachine commented Mar 19, 2020

[Endpoint] Adds take action dropdown and tests to alert details flyout #59242

[Endpoint] Adds take action dropdown and tests to alert details flyout #59242

Conversation

dplumlee commented Mar 3, 2020 • edited Loading

Summary

Checklist

For maintainers

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Mar 4, 2020

elasticmachine commented Mar 4, 2020

peluja1012 Mar 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dplumlee Mar 16, 2020 • edited Loading

Choose a reason for hiding this comment

kibanamachine commented Mar 16, 2020

💚 Build Succeeded

History

kibanamachine commented Mar 18, 2020

kibanamachine commented Mar 19, 2020

dplumlee commented Mar 3, 2020 •

edited

Loading

peluja1012 Mar 16, 2020 •

edited

Loading

dplumlee Mar 16, 2020 •

edited

Loading