[Security Solution] [Platform] Adds state to remember what was in data view or index pattern selection when switching between the two #136448

dhurley14 · 2022-07-14T20:02:19Z

Summary

Also addressed: #135516

In collaboration with @nkhristinin

When switching between data views and index patterns on the rule creation form, the user could lose previously selected index patterns or data views. Depending on which was originally saved in the rule form.

Keep both fields index and dataView on the form
Made a DataViewSelector more controlled component and moved logic about fetch and state to a parent (step_define_rule)
Depends on chosen data type I omit some fields from the form

The small problem which I found, is that validation on DataViewSelector is triggered right away we click on the Data View button, which can be a bit confusing because users don’t touch this field.

…on when switching between the two

nkhristinin · 2022-07-19T18:26:08Z

@elasticmachine merge upstream

…xpected to be nullish but the form state has it set to an empty string, added that extra check into the EQL query bar validator

…-ref HEAD~1..HEAD --fix'

nkhristinin · 2022-07-21T08:17:18Z

@elasticmachine merge upstream

vitaliidm

thanks @dhurley14. It looks good overall, I've also did some testing: selected values are kept when switching between index and data views.

Also left few minor comments

...ck/plugins/security_solution/public/detections/components/rules/data_view_selector/index.tsx

x-pack/plugins/security_solution/public/detections/components/rules/step_define_rule/index.tsx

...ck/plugins/security_solution/public/detections/components/rules/data_view_selector/index.tsx

x-pack/plugins/security_solution/public/detections/components/rules/step_define_rule/index.tsx

...k/plugins/security_solution/public/detections/pages/detection_engine/rules/create/helpers.ts

…rules/step_define_rule/index.tsx Co-authored-by: Vitalii Dmyterko <[email protected]>

…rules/data_view_selector/index.tsx Co-authored-by: Vitalii Dmyterko <[email protected]>

kibana-ci · 2022-07-25T18:04:06Z

💚 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.2MB	5.2MB	+1.2KB

History

💔 Build #60364 failed 2d2afcb
💚 Build #59618 succeeded a33441e
💚 Build #59365 succeeded aea2f33
💚 Build #59129 succeeded 26440e2
💚 Build #58974 succeeded f73fa29

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

cc @dhurley14 @nkhristinin

banderror

I did a high-level review of the changes and I didn't check them thoroughly and didn't test the PR locally.

Please check my few comments.

Also, I feel like we need to simplify the implementation of the rule creation/editing forms because it looks hairy and every fix and feature adds complexity on top of what is already there. But that's a separate story 🙂 I'll make a note to open a ticket for refactoring.

banderror · 2022-07-25T18:30:17Z

x-pack/plugins/security_solution/public/detections/components/rules/step_define_rule/index.tsx

+    if (dataSourceType === DataSourceType.DataView) {
+      const fetchDataView = async () => {
+        if (dataView != null) {
+          const dv = await data.dataViews.get(dataView);
+          setDataViewTitle(dv.title);
+          setIndexPattern(dv);
+        }
+      };
+
+      fetchDataView();
+    }


Using react-query might help for keeping dataView parameter (it's a name or an id, right?) in sync with the data view saved object that needs to be fetched. We used this library for the Rules Table and for fetching rule execution results and events in #126063. It simplifies code in such cases and makes it clean and robust.

banderror · 2022-07-25T18:34:41Z

x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/types.ts

+export enum DataSourceType {
+  IndexPatterns = 'indexPatterns',
+  DataView = 'dataView',
+}


Is this defined on the FE side only, or at the API and/or BE level as well?

We might benefit from defining it under the common folder because we need to distinguish rules with these two types of data sources in tests as well, for example: #136822 (comment)

dhurley14 added 2 commits July 14, 2022 11:08

adds state to remember what was in data view or index pattern selecti…

2d2221b

…on when switching between the two

cleanup

865e231

dhurley14 changed the title ~~adds state to remember what was in data view or index pattern selecti…~~ adds state to remember what was in data view or index pattern selection when switching between the two Jul 14, 2022

remove console log

b2be33b

dhurley14 self-assigned this Jul 14, 2022

dhurley14 requested review from nkhristinin and yctercero July 14, 2022 20:20

dhurley14 added review release_note:skip Skip the PR/issue when compiling release notes Feature:Detection Rules Security Solution rules and Detection Engine Team:Security Solution Platform Security Solution Platform Team v8.4.0 labels Jul 14, 2022

dhurley14 marked this pull request as ready for review July 14, 2022 20:20

dhurley14 requested a review from a team as a code owner July 14, 2022 20:20

dhurley14 requested a review from vitaliidm July 14, 2022 20:20

dhurley14 changed the title ~~adds state to remember what was in data view or index pattern selection when switching between the two~~ [Security Solution] [Platform] Adds state to remember what was in data view or index pattern selection when switching between the two Jul 14, 2022

vitaliidm and others added 2 commits July 18, 2022 14:34

Merge branch 'main' into dv-bug-fixes

53b9115

Keep index and dataVew on form

a63f9a1

nkhristinin added the ci:deploy-cloud label Jul 19, 2022

kibanamachine and others added 3 commits July 19, 2022 14:26

Merge branch 'main' into dv-bug-fixes

93b2c7b

fix type errors and fixes failure in validation when dataViewId was e…

5e216aa

…xpected to be nullish but the form state has it set to an empty string, added that extra check into the EQL query bar validator

update jest test

8ac0f45

dhurley14 enabled auto-merge (squash) July 20, 2022 01:38

nkhristinin and others added 5 commits July 20, 2022 06:14

Fix for eql

ee98001

More check for eql

f73fa29

Fix validation

b785fc9

Remove unused code and fix defaultValue

38be325

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

26440e2

…-ref HEAD~1..HEAD --fix'

dhurley14 assigned nkhristinin Jul 20, 2022

Show dataViewLabel

6a55019

Merge branch 'main' into dv-bug-fixes

aea2f33

vitaliidm reviewed Jul 21, 2022

View reviewed changes

nkhristinin and others added 5 commits July 21, 2022 18:35

Update x-pack/plugins/security_solution/public/detections/components/…

6aeb934

…rules/step_define_rule/index.tsx Co-authored-by: Vitalii Dmyterko <[email protected]>

Update x-pack/plugins/security_solution/public/detections/components/…

354066d

…rules/step_define_rule/index.tsx Co-authored-by: Vitalii Dmyterko <[email protected]>

Update x-pack/plugins/security_solution/public/detections/components/…

2bdc67f

…rules/data_view_selector/index.tsx Co-authored-by: Vitalii Dmyterko <[email protected]>

Update x-pack/plugins/security_solution/public/detections/components/…

0ae898f

…rules/data_view_selector/index.tsx Co-authored-by: Vitalii Dmyterko <[email protected]>

add new helper function for writing data source to rule form

a33441e

dhurley14 removed the request for review from nkhristinin July 22, 2022 15:58

dhurley14 added 2 commits July 25, 2022 09:39

Merge branch 'main' into dv-bug-fixes

af27f0d

merge with main

2d2afcb

banderror self-requested a review July 25, 2022 13:55

merge with main 2

8fda3cf

banderror approved these changes Jul 25, 2022

View reviewed changes

dhurley14 merged commit c2985c4 into elastic:main Jul 25, 2022

kibanamachine added the backport:skip This commit does not require backporting label Jul 25, 2022

dhurley14 deleted the dv-bug-fixes branch July 25, 2022 18:42

marshallmain mentioned this pull request Jul 25, 2022

[Security Solution][Platform] Field options are not populated in edit rule page with data views #137113

Closed

tylersmalley added ci:cloud-deploy Create or update a Cloud deployment and removed ci:deploy-cloud labels Aug 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution] [Platform] Adds state to remember what was in data view or index pattern selection when switching between the two #136448

[Security Solution] [Platform] Adds state to remember what was in data view or index pattern selection when switching between the two #136448

dhurley14 commented Jul 14, 2022 •

edited by nkhristinin

Loading

nkhristinin commented Jul 19, 2022

nkhristinin commented Jul 21, 2022

vitaliidm left a comment

kibana-ci commented Jul 25, 2022

banderror left a comment

banderror Jul 25, 2022 •

edited

Loading

banderror Jul 25, 2022

[Security Solution] [Platform] Adds state to remember what was in data view or index pattern selection when switching between the two #136448

[Security Solution] [Platform] Adds state to remember what was in data view or index pattern selection when switching between the two #136448

Conversation

dhurley14 commented Jul 14, 2022 • edited by nkhristinin Loading

Summary

nkhristinin commented Jul 19, 2022

nkhristinin commented Jul 21, 2022

vitaliidm left a comment

Choose a reason for hiding this comment

kibana-ci commented Jul 25, 2022

💚 Build Succeeded

Metrics [docs]

Async chunks

History

banderror left a comment

Choose a reason for hiding this comment

banderror Jul 25, 2022 • edited Loading

Choose a reason for hiding this comment

banderror Jul 25, 2022

Choose a reason for hiding this comment

dhurley14 commented Jul 14, 2022 •

edited by nkhristinin

Loading

banderror Jul 25, 2022 •

edited

Loading