[Security Solution][Detections] Value Lists Modal supports multiple exports #73532
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Modifying columns has revealed that they should be exposed as props, at which point we have no real need for the table component.
I thought this was useful when I wrote it!
Instead of passing our export function to GenericDownloader, we now manage the multiple exports ourselves, and when successful we pass the blob to GenericDownloader. * tracks a list of exporting IDs instead of single ID * chains onto the export promise to set local state
These verify that we've wired up our table actions to our API calls. A little brittle/tied to implementation, but I'd rather have them than not.
rylnd
added
Team:SIEM
v8.0.0
release_note:skip
|
@elasticmachine merge upstream |
rylnd
changed the title
|
@elasticmachine merge upstream |
|
Pinging @elastic/siem (Team:SIEM) |
...s/security_solution/public/detections/components/value_lists_management_modal/modal.test.tsx
Outdated
Show resolved
Hide resolved
...lugins/security_solution/public/detections/components/value_lists_management_modal/modal.tsx
Outdated
Show resolved
Hide resolved
FrankHassanabad
approved these changes
Jul 28, 2020
This component takes a blob and downloads it in a cross-browser-compatible manner.
Converts to the try/catch/finally form as well.
We lost this test subj during our refactor, oops
Our component fails due to this method being undefined, so we mock it out for these tests. We do not need to reset the mock as it is assigned fresh on every test.
dhurley14
approved these changes
Jul 29, 2020
Defines a global static method in a more portable way, as the regular assignment was failing on CI as the property was readonly.
The less we assume about the UI, the more robust these'll be.
💚 Build SucceededBuild metricsasync chunks size
History
To update your PR or re-run it, just comment with: |
rylnd
added a commit
to rylnd/kibana
that referenced
this pull request
Jul 29, 2020
…xports (elastic#73532) * Remove need for ValueListsTable Modifying columns has revealed that they should be exposed as props, at which point we have no real need for the table component. * Unroll the ActionButton component I thought this was useful when I wrote it! * Handle multiple simultaneous exports on value lists modal Instead of passing our export function to GenericDownloader, we now manage the multiple exports ourselves, and when successful we pass the blob to GenericDownloader. * tracks a list of exporting IDs instead of single ID * chains onto the export promise to set local state * Port useful table tests over to modal tests These verify that we've wired up our table actions to our API calls. A little brittle/tied to implementation, but I'd rather have them than not. * WIP: Simpler version of GenericDownloader * Replace use of GenericDownloader with simpler AutoDownload This component takes a blob and downloads it in a cross-browser-compatible manner. * Handle error when uploading value lists Converts to the try/catch/finally form as well. * Fix failing cypress test We lost this test subj during our refactor, oops * More explicit setting of global DOM function Our component fails due to this method being undefined, so we mock it out for these tests. We do not need to reset the mock as it is assigned fresh on every test. * Fixes jest failures on CI Defines a global static method in a more portable way, as the regular assignment was failing on CI as the property was readonly. * Simplify our export/delete clicks in jest tests The less we assume about the UI, the more robust these'll be. Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
rylnd
added a commit
to rylnd/kibana
that referenced
this pull request
Jul 29, 2020
…xports (elastic#73532) * Remove need for ValueListsTable Modifying columns has revealed that they should be exposed as props, at which point we have no real need for the table component. * Unroll the ActionButton component I thought this was useful when I wrote it! * Handle multiple simultaneous exports on value lists modal Instead of passing our export function to GenericDownloader, we now manage the multiple exports ourselves, and when successful we pass the blob to GenericDownloader. * tracks a list of exporting IDs instead of single ID * chains onto the export promise to set local state * Port useful table tests over to modal tests These verify that we've wired up our table actions to our API calls. A little brittle/tied to implementation, but I'd rather have them than not. * WIP: Simpler version of GenericDownloader * Replace use of GenericDownloader with simpler AutoDownload This component takes a blob and downloads it in a cross-browser-compatible manner. * Handle error when uploading value lists Converts to the try/catch/finally form as well. * Fix failing cypress test We lost this test subj during our refactor, oops * More explicit setting of global DOM function Our component fails due to this method being undefined, so we mock it out for these tests. We do not need to reset the mock as it is assigned fresh on every test. * Fixes jest failures on CI Defines a global static method in a more portable way, as the regular assignment was failing on CI as the property was readonly. * Simplify our export/delete clicks in jest tests The less we assume about the UI, the more robust these'll be. Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
rylnd
added a commit
that referenced
this pull request
Jul 29, 2020
…xports (#73532) (#73621) * Remove need for ValueListsTable Modifying columns has revealed that they should be exposed as props, at which point we have no real need for the table component. * Unroll the ActionButton component I thought this was useful when I wrote it! * Handle multiple simultaneous exports on value lists modal Instead of passing our export function to GenericDownloader, we now manage the multiple exports ourselves, and when successful we pass the blob to GenericDownloader. * tracks a list of exporting IDs instead of single ID * chains onto the export promise to set local state * Port useful table tests over to modal tests These verify that we've wired up our table actions to our API calls. A little brittle/tied to implementation, but I'd rather have them than not. * WIP: Simpler version of GenericDownloader * Replace use of GenericDownloader with simpler AutoDownload This component takes a blob and downloads it in a cross-browser-compatible manner. * Handle error when uploading value lists Converts to the try/catch/finally form as well. * Fix failing cypress test We lost this test subj during our refactor, oops * More explicit setting of global DOM function Our component fails due to this method being undefined, so we mock it out for these tests. We do not need to reset the mock as it is assigned fresh on every test. * Fixes jest failures on CI Defines a global static method in a more portable way, as the regular assignment was failing on CI as the property was readonly. * Simplify our export/delete clicks in jest tests The less we assume about the UI, the more robust these'll be. Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
rylnd
added a commit
that referenced
this pull request
Jul 29, 2020
…iple exports (#73532) (#73622) * [Security Solution][Detections] Value Lists Modal supports multiple exports (#73532) * Remove need for ValueListsTable Modifying columns has revealed that they should be exposed as props, at which point we have no real need for the table component. * Unroll the ActionButton component I thought this was useful when I wrote it! * Handle multiple simultaneous exports on value lists modal Instead of passing our export function to GenericDownloader, we now manage the multiple exports ourselves, and when successful we pass the blob to GenericDownloader. * tracks a list of exporting IDs instead of single ID * chains onto the export promise to set local state * Port useful table tests over to modal tests These verify that we've wired up our table actions to our API calls. A little brittle/tied to implementation, but I'd rather have them than not. * WIP: Simpler version of GenericDownloader * Replace use of GenericDownloader with simpler AutoDownload This component takes a blob and downloads it in a cross-browser-compatible manner. * Handle error when uploading value lists Converts to the try/catch/finally form as well. * Fix failing cypress test We lost this test subj during our refactor, oops * More explicit setting of global DOM function Our component fails due to this method being undefined, so we mock it out for these tests. We do not need to reset the mock as it is assigned fresh on every test. * Fixes jest failures on CI Defines a global static method in a more portable way, as the regular assignment was failing on CI as the property was readonly. * Simplify our export/delete clicks in jest tests The less we assume about the UI, the more robust these'll be. Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> * Skip tests failing in CI environment These passed on master but are now failing on this backport branch. Skipping them for now. Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
gmmorris
added a commit
to gmmorris/kibana
that referenced
this pull request
Jul 29, 2020
* master: (126 commits) [ML] Disabling ML if license feature is disabled (elastic#73187) [ML] Fixing old _xpack style es endpoint paths (elastic#73667) [DOCS] [Lens] 7.9 docs refresh (elastic#72301) [ML] DF Analytics results: ensure `View` link is only enabled when job has successfully completed (elastic#73539) Set timeRange to default to trigger the error message (elastic#73629) [ML] Functional tests - stabilize DFA navigation and index pattern handling (elastic#73660) [ILM] Add links to "Snapshot and Restore" from ILM "wait for snapshot policy" (elastic#72473) [kbn-storybook] Update Storybook to 5.3.19 (elastic#73320) [Metrics UI] Fix hasData call to ensure it has data not just indices (elastic#72969) [Uptime] Use `service.name` to link from Uptime -> APM where available (elastic#73618) allow others to update `URL.revokeObjectURL` property if needed (elastic#73639) regen docs (elastic#73650) [Visualize] Fix inspector download filename issue when saving in-place (elastic#72605) [Data] Query Input String manager (elastic#72093) [Security Solutions] Add tooltips (elastic#73436) Do not render descriptionless actions within an EuiCard (elastic#73611) [Security Solution][Detections] Value Lists Modal supports multiple exports (elastic#73532) [Security Solution][Resolver] Handle disabled process collection (elastic#73592) [Security_Solution][Bug] Fix user name/domain to ECS structure (elastic#73530) [Security Solution][Exceptions] - Update rule.exceptions_list to include exception list list_id (elastic#73349) ...
MindyRS
added
the
Team: SecuritySolution
|
Pinging @elastic/security-solution (Team: SecuritySolution) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This allows multiple concurrent exports on the value lists modal, similar to how multiple deletions is working. Modifying the table actions uncovered some poorly-coupled components, and so I opted to remove ValueListsTable entirely in lieu of an EuiTable.
This also fixes an issue with the same export being triggered multiple times due to GenericDownloader rerendering on e.g. pagination; now the component is only rendered when it has a download to perform. I wrote a simpler
GenericDownloaderthat simply receives a blob and a callback function,AutoDownload, to accomplish this task.Checklist
Delete any items that are not applicable to this PR.
For maintainers