[SIEM][Detections] Value Lists Management Modal #67068

rylnd · 2020-05-20T00:12:26Z

Summary

Addresses #65929.

Highlights:

Adds Modal, Form, and Table components
Adds a general useCursor hook to optimize pagination of queries

Outstanding tasks:

display feedback to user on success/error
styling
table pagination
- includes use of new cursor tech
move lists functions to lists plugin
tests

Outstanding issues:

Cancelling an import request does not prevent the backend from processing whatever's already been sent, it merely frees up the frontend to ignore the response. This may lead to unexpected behavior/inconsistent state.
FIXED: Import request takes ~30-40s for a 100,000 value list (100kB file)
- Should be addressed by [SIEM][Detection Engine] Speeds up value list imports by enabling streaming of files. #70685

Potential UI/UX improvements:

disable row during export/delete
FIXED: synchronize import/delete with a wait_for, or: add a Refresh button for the table
- addressed in [SIEM][Detection Engine][Lists] Adds "wait_for" to all the create, update, patch, delete endpoints #71262
FIXED: update layout so that bottom of table fits within modal window (i.e. no scrollbar)
- addressed by removing the "per page" option and fixing to five rows
FIXED: Pagination context menu's styles are incorrect:

Current view:

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
Documentation was added for features that require explanation or tutorials
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

elasticmachine · 2020-05-20T00:12:29Z

Pinging @elastic/siem (Team:SIEM)

spong · 2020-05-20T19:48:47Z

For the two UX items:

[UX]: with displaying feedback in a modal

I think it might be worthwhile to see if EUI is okay with us increasing the z-index of the toasts so they display above the modal mask. While the EuiCallout would be sufficient for upload errors, it'd be unwieldy to display success/error feedback from the export/delete actions. E.g. if you click to delete two lists one right after the next, both error out, do we just append the errors? Clear previous import errors? Maybe just append and have an X next to each so they can be dismissed individually? This approach also results in UI bounce unless we reserve a min height of empty space for the callout -- which is okay, but I think keeping things simple with our global toast pattern is best so long as EUI is 👍

[UX]: issues with auto-upload

Perhaps we can use the EuiFilePicker in its small format, and then have an Upload list button to the right of it? This also leaves room for helper text like in the mocks (i.e. "Upload a .csv file up to 400MB"). On upload the button (and radios?) disable, and the FilePicker switches to its loading state, and on success the FilePicker clears its previous file ref and the button re-enables -- similar to the Rule Import modal (which had the same bug 😅 )

cc @marrasherrier

rylnd · 2020-05-28T23:59:22Z

Regarding the above UX issues: We ended up going with toast notifications, and manual uploads. We reset the form (file picker and radio group) on a successful upload/import.

Imports and uses the hooks provided by the lists plugin. Tests coming next.

* uses useEffect on a task's state instead of promise chaining * handles the fact that API calls can be rejected with strings * uses exportList function instead of hook

For e.g. findLists, we can send along a cursor to optimize our query. On the backend, this cursor is used as part of a search_after query.

* Does not require args for setCursor as they're already passed to the hook * Finds nearest cursor for the same page size Eventually this logic will also include sortField as part of the hash/lookup, but we do not currently use that on the frontend.

We were previously storing the cursor on the _current_ page, when it's only truly valid for the _next_ page (and beyond). This was causing a few issues, but now that it's fixed everything works great.

This allows us to search_after a previous page's search, if available.

This is just a blob, so we have nothing to validate.

After uploading a list, the modal was being shown twice. Declaring the constituent state dependencies separately fixed the issue.

These hooks no longer care about/expose an abort function. In this one case where we need that functionality, we can do it ourselves relatively simply.

Dates were wrapping (and raw), and so were wrapped in a FormattedDate component. However, since this component didn't wrap, we needed to shrink/truncate the uploaded_by field as well as allow the fileName to truncate.

enzymejs/enzyme#2073 seems to be an ongoing issue, and causes components with useEffect to update after the test is completed. waitForUpdates ensures that updates have completed within an act() before continuing on.

Conflicts: x-pack/plugins/security_solution/public/lists_plugin_deps.ts x-pack/plugins/security_solution/public/overview/pages/overview.test.tsx

Each of these logs a console.error without them.

marrasherrier · 2020-07-07T19:22:20Z

after uploading a list, will it show up immediately in the table below? not showing up now

FrankHassanabad · 2020-07-07T23:38:07Z

...ck/plugins/security_solution/public/alerts/components/value_lists_management_modal/modal.tsx

+      });
+      fetchLists();
+    },
+    // eslint-disable-next-line react-hooks/exhaustive-deps


Is it possible or easy to remove the disabling of the linter rule by passing in fetchLists?

The story here is in the commit that added it. I agree that this is a smell and I'll try it again from a different angle. If you have suggestions, though, I'd appreciate them! Ditto on useCursor.

FrankHassanabad · 2020-07-07T23:42:56Z

...ck/plugins/security_solution/public/alerts/components/value_lists_management_modal/table.tsx

+  {
+    field: 'created_at',
+    name: i18n.COLUMN_UPLOAD_DATE,
+    /* eslint-disable-next-line react/display-name */


Can you add a displayName: createdAt to avoid disabling the linter here or no?

Ref:
https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/display-name.md

The complaint is because this inline function returning markup is itself technically a component. I think a naive render: (value) => <NamedComponent value={value} /> would suffer the same issue, so you'd need to write a component that takes the full set of arguments to allow you to do render: NamedComponent.

I'll give it a shot.

rylnd · 2020-07-07T23:48:02Z

after uploading a list, will it show up immediately in the table below? not showing up now

@marrasherrier correct, this is the second bullet under "Potential UI/UX Improvements." I don't know what the best solution is here... @FrankHassanabad @spong ?

FrankHassanabad · 2020-07-07T23:48:12Z

x-pack/plugins/security_solution/public/common/utils/test_utils.ts

+    await new Promise((resolve) => setTimeout(resolve, 0));
+    wrapper.update();
+  });
+};


You probably need this extra utility but I usually use a waitFor() from here:
https://testing-library.com/docs/dom-testing-library/api-async

When I can before adding a workaround and then I do the workaround when I can't use one of those. Might help you out, might not. Ignore this if it doesn't, just mentioning it in case its helpful.

FrankHassanabad · 2020-07-07T23:50:20Z

after uploading a list, will it show up immediately in the table below? not showing up now

@marrasherrier correct, this is the second bullet under "Potential UI/UX Improvements." I don't know what the best solution is here... @FrankHassanabad @spong ?

I have on my list wait_for tasks for lists to add to most of the endpoints which will fix this before we ship.

FrankHassanabad

Looked it over and its really clean code and I always learn interesting things from PR's like this that have good links to other issues from Github across libraries.

A lot of good attention to details, I only had some minor questions but am going to give this a 👍 and LGTM! Our code base is better with it than without it.

Conflicts: x-pack/plugins/security_solution/public/lists_plugin_deps.ts x-pack/plugins/security_solution/public/overview/pages/overview.test.tsx

Conflicts: x-pack/plugins/lists/public/lists/api.test.ts x-pack/plugins/lists/public/lists/api.ts x-pack/plugins/lists/public/shared_exports.ts x-pack/plugins/security_solution/public/common/lib/kibana/hooks.ts x-pack/plugins/security_solution/public/detections/components/value_lists_management_modal/form.test.tsx x-pack/plugins/security_solution/public/detections/components/value_lists_management_modal/form.tsx x-pack/plugins/security_solution/public/detections/components/value_lists_management_modal/index.tsx x-pack/plugins/security_solution/public/detections/components/value_lists_management_modal/modal.test.tsx x-pack/plugins/security_solution/public/detections/components/value_lists_management_modal/modal.tsx x-pack/plugins/security_solution/public/detections/components/value_lists_management_modal/table.test.tsx x-pack/plugins/security_solution/public/detections/components/value_lists_management_modal/table.tsx x-pack/plugins/security_solution/public/detections/components/value_lists_management_modal/translations.ts x-pack/plugins/security_solution/public/lists_plugin_deps.ts

It looks like this broke because EuiTable's pagination changed from a button to an anchor tag.

These have style issues, and anything above 5 rows causes the modal to scroll, so we're going to disable it for now.

We don't display the nice errors in the case of an ApiError right now, but this is better than it was.

Our start() no longer resolves in a meaningful way, so we instead need to perform the refetch in an effect watching the result of our delete.

useAsync generally does not know how what its tasks are going to be rejected with, hence the unknown. For these API calls we know that it will be an Error, but I don't currently have a way to type that generally. For now, we'll cast it where we use it.

kibanamachine · 2020-07-14T01:56:39Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 59e3b02

Build metrics

✅ unchanged

History

💚 Build #59640 succeeded 18459c1
💚 Build #59016 succeeded e0cb6a5
💚 Build #58595 succeeded 0abbfc4
💚 Build #58540 succeeded 728b272
💚 Build #58516 succeeded 0e4fee7

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

* Add Frontend components for Value Lists Management Modal Imports and uses the hooks provided by the lists plugin. Tests coming next. * Update value list components to use newest Lists API * uses useEffect on a task's state instead of promise chaining * handles the fact that API calls can be rejected with strings * uses exportList function instead of hook * Close modal on outside click * Add hook for using a cursor with paged API calls. For e.g. findLists, we can send along a cursor to optimize our query. On the backend, this cursor is used as part of a search_after query. * Better implementation of useCursor * Does not require args for setCursor as they're already passed to the hook * Finds nearest cursor for the same page size Eventually this logic will also include sortField as part of the hash/lookup, but we do not currently use that on the frontend. * Fixes useCursor hook functionality We were previously storing the cursor on the _current_ page, when it's only truly valid for the _next_ page (and beyond). This was causing a few issues, but now that it's fixed everything works great. * Add cursor to lists query This allows us to search_after a previous page's search, if available. * Do not validate response of export This is just a blob, so we have nothing to validate. * Fix double callback post-import After uploading a list, the modal was being shown twice. Declaring the constituent state dependencies separately fixed the issue. * Update ValueListsForm to manually abort import request These hooks no longer care about/expose an abort function. In this one case where we need that functionality, we can do it ourselves relatively simply. * Default modal table to five rows * Update translation keys following plugin rename * Try to fit table contents on a single row Dates were wrapping (and raw), and so were wrapped in a FormattedDate component. However, since this component didn't wrap, we needed to shrink/truncate the uploaded_by field as well as allow the fileName to truncate. * Add helper function to prevent tests from logging errors enzymejs/enzyme#2073 seems to be an ongoing issue, and causes components with useEffect to update after the test is completed. waitForUpdates ensures that updates have completed within an act() before continuing on. * Add jest tests for our form, table, and modal components * Fix translation conflict * Add more waitForUpdates to new overview page tests Each of these logs a console.error without them. * Fix bad merge resolution That resulted in duplicate exports. * Make cursor an optional parameter to findLists This param is an optimization and not required for basic functionality. * Tweaking Table column sizes Makes actions column smaller, leaving more room for everything else. * Fix bug where onSuccess is called upon pagination change Because fetchLists changes when pagination does, and handleUploadSuccess changes with fetchLists, our useEffect in Form was being fired on every pagination change due to its onSuccess changing. The solution in this instance is to remove fetchLists from handleUploadSuccess's dependencies, as we merely want to invoke fetchLists from it, not change our reference. * Fix failing test It looks like this broke because EuiTable's pagination changed from a button to an anchor tag. * Hide page size options on ValueLists modal table These have style issues, and anything above 5 rows causes the modal to scroll, so we're going to disable it for now. * Update error callbacks now that we have Errors We don't display the nice errors in the case of an ApiError right now, but this is better than it was. * Synchronize delete with the subsequent fetch Our start() no longer resolves in a meaningful way, so we instead need to perform the refetch in an effect watching the result of our delete. * Cast our unknown error to an Error useAsync generally does not know how what its tasks are going to be rejected with, hence the unknown. For these API calls we know that it will be an Error, but I don't currently have a way to type that generally. For now, we'll cast it where we use it. * Import lists code from our new, standardized modules Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

* master: (314 commits) [APM] Use status_code field to calculate error rate (elastic#71109) [Observability] Change appLink passing the date range (elastic#71259) [Security] Add Timeline improvements (elastic#71506) adjust vislib bar opacity (elastic#71421) Fix ScopedHistory mock and adapt usages (elastic#71404) [Security Solution] Add hook for reading/writing resolver query params (elastic#70809) [APM] Bug fixes from ML integration testing (elastic#71564) [Discover] Add caused_by.type and caused_by.reason to error toast modal (elastic#70404) [Security Solution] Add 3rd level breadcrumb to admin page (elastic#71275) [Security Solution][Exceptions] Exception modal bulk close alerts that match exception attributes (elastic#71321) Change signal.rule.risk score mapping from keyword to float (elastic#71126) Added help text where needed on connectors and alert actions UI (elastic#69601) [SIEM][Detections] Value Lists Management Modal (elastic#67068) [test] Skips test preventing promotion of ES snapshot elastic#71582 [test] Skips test preventing promotion of ES snapshot elastic#71555 [ILM] Fix alignment of the timing field (elastic#71273) [SIEM][Detection Engine][Lists] Adds the ability for exception lists to be multi-list queried. (elastic#71540) initial telemetry setup (elastic#69330) [Reporting] Formatting fixes for CSV export in Discover, CSV download from Dashboard panel (elastic#67027) Search across spaces (elastic#67644) ...

…t-apps-page-titles * 'master' of github.com:elastic/kibana: (88 commits) [ML] Functional tests - disable DFA creation and cloning tests [APM] Use status_code field to calculate error rate (elastic#71109) [Observability] Change appLink passing the date range (elastic#71259) [Security] Add Timeline improvements (elastic#71506) adjust vislib bar opacity (elastic#71421) Fix ScopedHistory mock and adapt usages (elastic#71404) [Security Solution] Add hook for reading/writing resolver query params (elastic#70809) [APM] Bug fixes from ML integration testing (elastic#71564) [Discover] Add caused_by.type and caused_by.reason to error toast modal (elastic#70404) [Security Solution] Add 3rd level breadcrumb to admin page (elastic#71275) [Security Solution][Exceptions] Exception modal bulk close alerts that match exception attributes (elastic#71321) Change signal.rule.risk score mapping from keyword to float (elastic#71126) Added help text where needed on connectors and alert actions UI (elastic#69601) [SIEM][Detections] Value Lists Management Modal (elastic#67068) [test] Skips test preventing promotion of ES snapshot elastic#71582 [test] Skips test preventing promotion of ES snapshot elastic#71555 [ILM] Fix alignment of the timing field (elastic#71273) [SIEM][Detection Engine][Lists] Adds the ability for exception lists to be multi-list queried. (elastic#71540) initial telemetry setup (elastic#69330) [Reporting] Formatting fixes for CSV export in Discover, CSV download from Dashboard panel (elastic#67027) ... # Conflicts: # x-pack/plugins/index_management/public/application/index.tsx

* master: (72 commits) [test] Skips test preventing promotion of ES snapshot elastic#71612 [Logs UI] Remove UUID from Alert Instances (elastic#71340) [Metrics UI] Remove UUID from Alert Instance IDs (elastic#71335) [ML] Functional tests - disable DFA creation and cloning tests [APM] Use status_code field to calculate error rate (elastic#71109) [Observability] Change appLink passing the date range (elastic#71259) [Security] Add Timeline improvements (elastic#71506) adjust vislib bar opacity (elastic#71421) Fix ScopedHistory mock and adapt usages (elastic#71404) [Security Solution] Add hook for reading/writing resolver query params (elastic#70809) [APM] Bug fixes from ML integration testing (elastic#71564) [Discover] Add caused_by.type and caused_by.reason to error toast modal (elastic#70404) [Security Solution] Add 3rd level breadcrumb to admin page (elastic#71275) [Security Solution][Exceptions] Exception modal bulk close alerts that match exception attributes (elastic#71321) Change signal.rule.risk score mapping from keyword to float (elastic#71126) Added help text where needed on connectors and alert actions UI (elastic#69601) [SIEM][Detections] Value Lists Management Modal (elastic#67068) [test] Skips test preventing promotion of ES snapshot elastic#71582 [test] Skips test preventing promotion of ES snapshot elastic#71555 [ILM] Fix alignment of the timing field (elastic#71273) ...

elasticmachine · 2021-09-23T14:34:27Z

Pinging @elastic/security-solution (Team: SecuritySolution)

rylnd added release_note:enhancement Team:SIEM v8.0.0 v7.9.0 labels May 20, 2020

rylnd self-assigned this May 20, 2020

rylnd changed the title ~~Value lists ui~~ [SIEM][Detections] Value Lists Management Modal May 20, 2020

rylnd mentioned this pull request Jun 19, 2020

[Security][Lists] Add API functions and react hooks for value list APIs #69603

Merged

4 tasks

rylnd force-pushed the value_lists_ui branch from 64450a5 to 8807f52 Compare June 30, 2020 01:37

rylnd added 10 commits July 1, 2020 11:45

Add Frontend components for Value Lists Management Modal

f807600

Imports and uses the hooks provided by the lists plugin. Tests coming next.

Update value list components to use newest Lists API

cc34d86

* uses useEffect on a task's state instead of promise chaining * handles the fact that API calls can be rejected with strings * uses exportList function instead of hook

Close modal on outside click

af6a86e

Add hook for using a cursor with paged API calls.

bf7fb72

For e.g. findLists, we can send along a cursor to optimize our query. On the backend, this cursor is used as part of a search_after query.

Better implementation of useCursor

38fa27c

* Does not require args for setCursor as they're already passed to the hook * Finds nearest cursor for the same page size Eventually this logic will also include sortField as part of the hash/lookup, but we do not currently use that on the frontend.

Fixes useCursor hook functionality

b606f57

We were previously storing the cursor on the _current_ page, when it's only truly valid for the _next_ page (and beyond). This was causing a few issues, but now that it's fixed everything works great.

Add cursor to lists query

ce1edb2

This allows us to search_after a previous page's search, if available.

Do not validate response of export

e39116c

This is just a blob, so we have nothing to validate.

Fix double callback post-import

df70e5c

After uploading a list, the modal was being shown twice. Declaring the constituent state dependencies separately fixed the issue.

Update ValueListsForm to manually abort import request

344b81c

These hooks no longer care about/expose an abort function. In this one case where we need that functionality, we can do it ourselves relatively simply.

rylnd force-pushed the value_lists_ui branch from 8807f52 to 344b81c Compare July 1, 2020 20:36

rylnd added 8 commits July 1, 2020 15:54

Default modal table to five rows

23fd1ee

Update translation keys following plugin rename

e3670ea

Try to fit table contents on a single row

c36e959

Dates were wrapping (and raw), and so were wrapped in a FormattedDate component. However, since this component didn't wrap, we needed to shrink/truncate the uploaded_by field as well as allow the fileName to truncate.

Add helper function to prevent tests from logging errors

312fbde

enzymejs/enzyme#2073 seems to be an ongoing issue, and causes components with useEffect to update after the test is completed. waitForUpdates ensures that updates have completed within an act() before continuing on.

Add jest tests for our form, table, and modal components

9587b7e

Fix translation conflict

ab66483

Merge branch 'master' into value_lists_ui

7789c91

Conflicts: x-pack/plugins/security_solution/public/lists_plugin_deps.ts x-pack/plugins/security_solution/public/overview/pages/overview.test.tsx

Add more waitForUpdates to new overview page tests

f16cf4e

Each of these logs a console.error without them.

rylnd marked this pull request as ready for review July 6, 2020 17:08

rylnd requested review from a team as code owners July 6, 2020 17:08

FrankHassanabad reviewed Jul 7, 2020

View reviewed changes

FrankHassanabad approved these changes Jul 7, 2020

View reviewed changes

Merge branch 'master' into value_lists_ui

18459c1

Conflicts: x-pack/plugins/security_solution/public/lists_plugin_deps.ts x-pack/plugins/security_solution/public/overview/pages/overview.test.tsx

marrasherrier approved these changes Jul 13, 2020

View reviewed changes

rylnd added 7 commits July 13, 2020 18:05

Fix failing test

8fb7e4c

It looks like this broke because EuiTable's pagination changed from a button to an anchor tag.

Hide page size options on ValueLists modal table

e496614

These have style issues, and anything above 5 rows causes the modal to scroll, so we're going to disable it for now.

Update error callbacks now that we have Errors

3f12007

We don't display the nice errors in the case of an ApiError right now, but this is better than it was.

Synchronize delete with the subsequent fetch

bc984c4

Our start() no longer resolves in a meaningful way, so we instead need to perform the refetch in an effect watching the result of our delete.

Import lists code from our new, standardized modules

59e3b02

rylnd merged commit 835c13d into elastic:master Jul 14, 2020

rylnd deleted the value_lists_ui branch July 14, 2020 02:11

rylnd mentioned this pull request Jul 14, 2020

[7.x] [SIEM][Detections] Value Lists Management Modal (#67068) #71585

Merged

rylnd mentioned this pull request Jul 29, 2020

[SIEM][Detections] Create Value List Manager UI #65929

Closed

4 tasks

MindyRS added the Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. label Sep 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SIEM][Detections] Value Lists Management Modal #67068

[SIEM][Detections] Value Lists Management Modal #67068

rylnd commented May 20, 2020 •

edited

Loading

elasticmachine commented May 20, 2020

spong commented May 20, 2020

rylnd commented May 28, 2020

marrasherrier commented Jul 7, 2020

FrankHassanabad Jul 7, 2020 •

edited

Loading

rylnd Jul 7, 2020

FrankHassanabad Jul 7, 2020

rylnd Jul 7, 2020

rylnd commented Jul 7, 2020

FrankHassanabad Jul 7, 2020

FrankHassanabad commented Jul 7, 2020

FrankHassanabad left a comment

kibanamachine commented Jul 14, 2020

elasticmachine commented Sep 23, 2021

[SIEM][Detections] Value Lists Management Modal #67068

[SIEM][Detections] Value Lists Management Modal #67068

Conversation

rylnd commented May 20, 2020 • edited Loading

Summary

Highlights:

Outstanding tasks:

Outstanding issues:

Potential UI/UX improvements:

Current view:

Checklist

For maintainers

elasticmachine commented May 20, 2020

spong commented May 20, 2020

[UX]: with displaying feedback in a modal

[UX]: issues with auto-upload

rylnd commented May 28, 2020

marrasherrier commented Jul 7, 2020

FrankHassanabad Jul 7, 2020 • edited Loading

Choose a reason for hiding this comment

rylnd Jul 7, 2020

Choose a reason for hiding this comment

FrankHassanabad Jul 7, 2020

Choose a reason for hiding this comment

rylnd Jul 7, 2020

Choose a reason for hiding this comment

rylnd commented Jul 7, 2020

FrankHassanabad Jul 7, 2020

Choose a reason for hiding this comment

FrankHassanabad commented Jul 7, 2020

FrankHassanabad left a comment

Choose a reason for hiding this comment

kibanamachine commented Jul 14, 2020

💚 Build Succeeded

Build metrics

History

elasticmachine commented Sep 23, 2021

rylnd commented May 20, 2020 •

edited

Loading

FrankHassanabad Jul 7, 2020 •

edited

Loading