[Security Solution][Detections] Pre-refactoring for the rule management table by banderror · Pull Request #91302 · elastic/kibana

banderror · 2021-02-12T17:26:00Z

Related to: #89877

Summary

This is based on #89877 and the kind of pre-refactoring that has been done there.

Mainly this:

consolidates application logic in a single place (moves the reducer and the side effects close to each other, etc)
removes some of the redundant state, leverages the reducer as the source of truth for state
makes it easier to dispatch events, removes some of the noise

While this refactoring is a totally unfinished work, and might look not good enough (or at all), still I'd like to merge it because of the logic consolidation. I'm going to finalize the refactoring later when I start implementing new filters and other table UX improvements. So the code is going to become better and maybe even quite different from what it is right now. (Btw because of that, I'm not adding or removing any tests here because this is an intermediate kind of state of the code).

Checklist

Unit or functional tests were updated or added to match the most common scenarios

elasticmachine · 2021-02-12T17:26:03Z

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

elasticmachine · 2021-02-12T17:26:03Z

Pinging @elastic/security-detections-response (Team:Detections and Resp)

…ment in one place

kibanamachine · 2021-02-17T16:28:49Z

💛 Build succeeded, but was flaky

continuous-integration/kibana-ci/pull-request
Commit: f3e4aba
Documentation Changes
Flaky suites:
- xpack-securitySolutionCypress

Test Failures

Kibana Pipeline / general / "before all" hook for "should contain notes".Timeline notes tab "before all" hook for "should contain notes"

Link to Jenkins

Stack Trace

Failed Tests Reporter:
  - Test has not failed recently on tracked branches

AssertionError: Timed out retrying after 60000ms: Expected to find element: `[data-test-subj="title-19d7f1c0-7135-11eb-ae22-8be1da9a1bee"]`, but never found it.

Because this error occurred during a `before all` hook we are skipping the remaining tests in the current suite: `Timeline notes tab`

Although you have test retries enabled, we do not retry tests when `before all` or `after all` hooks fail
    at Object.openTimelineById (http://localhost:6111/__cypress/tests?p=cypress/integration/timelines/notes_tab.spec.ts:16007:15)
    at Context.eval (http://localhost:6111/__cypress/tests?p=cypress/integration/timelines/notes_tab.spec.ts:15041:24)

Kibana Pipeline / general / "after all" hook for "should contain notes".Timeline notes tab "after all" hook for "should contain notes"

Link to Jenkins

Stack Trace

Failed Tests Reporter:
  - Test has not failed recently on tracked branches

CypressError: `cy.filter()` failed because it requires a DOM element.

The subject received was:

  > `undefined`

The previous command that ran was:

  > `cy.get()`

All 2 subject validations failed on this subject.

Because this error occurred during a `after all` hook we are skipping the remaining tests in the current suite: `Timeline notes tab`

Although you have test retries enabled, we do not retry tests when `before all` or `after all` hooks fail
    at ensureElement (http://elastic:changeme@localhost:6111/__cypress/runner/cypress_runner.js:161322:24)
    at validateType (http://elastic:changeme@localhost:6111/__cypress/runner/cypress_runner.js:161159:16)
    at Object.ensureSubjectByType (http://elastic:changeme@localhost:6111/__cypress/runner/cypress_runner.js:161195:9)
    at pushSubjectAndValidate (http://elastic:changeme@localhost:6111/__cypress/runner/cypress_runner.js:169888:15)
    at Context.<anonymous> (http://elastic:changeme@localhost:6111/__cypress/runner/cypress_runner.js:170225:18)
From Your Spec Code:
    at Object.closeTimeline (http://localhost:6111/__cypress/tests?p=cypress/integration/timelines/notes_tab.spec.ts:15960:43)
    at Context.eval (http://localhost:6111/__cypress/tests?p=cypress/integration/timelines/notes_tab.spec.ts:15051:20)

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`securitySolution`	2205	2208	+3

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`securitySolution`	7.7MB	7.7MB	+2.0KB

History

💔 Build #107241 failed 691cca65cefca8802a0528452147d79c08bb5cf9
💛 Build #106533 was flaky 1a1d115bda74cdb451ddb178ddc87d5d8433a95e
💔 Build #106384 failed 0243c5c720e9339b011499164648497e7085fee8

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

dhurley14 · 2021-02-17T22:06:40Z

...ution/public/detections/containers/detection_engine/rules/rules_table/rules_table_reducer.ts

+        if (
+          tableRef != null &&
+          tableRef.current != null &&
+          tableRef.current.changeSelection != null
+        ) {


Nit: can we consolidate this into just

if(tableRef?.current?.changeSelection != null){...

I'll do it in #91925, thank you.

dhurley14

LGTM! Nice small cleanup here.

…nt table (elastic#91302) **Related to:** elastic#89877 ## Summary This is based on elastic#89877 and the kind of pre-refactoring that has been done there. Mainly this: - consolidates application logic in a single place (moves the reducer and the side effects close to each other, etc) - removes some of the redundant state, leverages the reducer as the source of truth for state - makes it easier to dispatch events, removes some of the noise While this refactoring is a totally unfinished work, and might look not good enough (or at all), still I'd like to merge it because of the logic consolidation. I'm going to finalize the refactoring later when I start implementing new filters and other table UX improvements. So the code is going to become better and maybe even quite different from what it is right now. (Btw because of that, I'm not adding or removing any tests here because this is an intermediate kind of state of the code). ### Checklist - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

kibanamachine · 2021-02-18T00:40:37Z

💔 Backport failed

✅ 7.x / #91777
❌ 7.13: The branch "7.13" is invalid or doesn't exist

Successful backport PRs will be merged automatically after passing CI.

To backport manually, check out the target branch and run:
node scripts/backport --pr 91302

…nt table (#91302) (#91777) **Related to:** #89877 ## Summary This is based on #89877 and the kind of pre-refactoring that has been done there. Mainly this: - consolidates application logic in a single place (moves the reducer and the side effects close to each other, etc) - removes some of the redundant state, leverages the reducer as the source of truth for state - makes it easier to dispatch events, removes some of the noise While this refactoring is a totally unfinished work, and might look not good enough (or at all), still I'd like to merge it because of the logic consolidation. I'm going to finalize the refactoring later when I start implementing new filters and other table UX improvements. So the code is going to become better and maybe even quite different from what it is right now. (Btw because of that, I'm not adding or removing any tests here because this is an intermediate kind of state of the code). ### Checklist - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios Co-authored-by: Georgii Gorbachev <georgii.gorbachev@elastic.co>

…nt table (elastic#91302) **Related to:** elastic#89877 ## Summary This is based on elastic#89877 and the kind of pre-refactoring that has been done there. Mainly this: - consolidates application logic in a single place (moves the reducer and the side effects close to each other, etc) - removes some of the redundant state, leverages the reducer as the source of truth for state - makes it easier to dispatch events, removes some of the noise While this refactoring is a totally unfinished work, and might look not good enough (or at all), still I'd like to merge it because of the logic consolidation. I'm going to finalize the refactoring later when I start implementing new filters and other table UX improvements. So the code is going to become better and maybe even quite different from what it is right now. (Btw because of that, I'm not adding or removing any tests here because this is an intermediate kind of state of the code). ### Checklist - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios # Conflicts: # x-pack/plugins/security_solution/public/detections/pages/detection_engine/rules/all/rules_tables.tsx

banderror self-assigned this Feb 12, 2021

banderror requested a review from a team as a code owner February 12, 2021 17:26

banderror requested a review from a team February 12, 2021 17:26

banderror force-pushed the rule-management-table-refactoring branch 2 times, most recently from 1a1d115 to 691cca6 Compare February 17, 2021 11:54

banderror added 5 commits February 17, 2021 15:14

Pre-refactoring: consolidate some of the logic related to rule manage…

6999612

…ment in one place

Implement in-memory pagination, filtering, sorting and searching

06b692f

Fix tests and clean up

e2bd19d

Update license comments

fa63097

Switch back to the server-side implementation

f3e4aba

banderror force-pushed the rule-management-table-refactoring branch from 691cca6 to f3e4aba Compare February 17, 2021 14:14

dhurley14 reviewed Feb 17, 2021

View reviewed changes

dhurley14 approved these changes Feb 17, 2021

View reviewed changes

spong merged commit 2c111c6 into elastic:master Feb 18, 2021

spong added the auto-backport Deprecated - use backport:version if exact versions are needed label Feb 18, 2021

kibanamachine mentioned this pull request Feb 18, 2021

[7.x] [Security Solution][Detections] Pre-refactoring for the rule management table (#91302) #91777

Merged

spong removed the v7.13.0 label Feb 18, 2021

banderror deleted the rule-management-table-refactoring branch February 18, 2021 16:51

This was referenced Feb 24, 2021

[7.11] [Security Solution][Detections] Pre-refactoring for the rule management table (#91302) #92585

Closed

[Security Solutions][Detection Engine] We don't frost on page clicks of manage detection rules #91336

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution][Detections] Pre-refactoring for the rule management table#91302

[Security Solution][Detections] Pre-refactoring for the rule management table#91302
spong merged 5 commits intoelastic:masterfrom
banderror:rule-management-table-refactoring

banderror commented Feb 12, 2021

Uh oh!

elasticmachine commented Feb 12, 2021

Uh oh!

elasticmachine commented Feb 12, 2021

Uh oh!

kibanamachine commented Feb 17, 2021

Stack Trace

Stack Trace

Uh oh!

dhurley14 Feb 17, 2021 •

edited

Loading

Uh oh!

banderror Feb 18, 2021

Uh oh!

dhurley14 left a comment

Uh oh!

kibanamachine commented Feb 18, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

banderror commented Feb 12, 2021

Summary

Checklist

Uh oh!

elasticmachine commented Feb 12, 2021

Uh oh!

elasticmachine commented Feb 12, 2021

Uh oh!

kibanamachine commented Feb 17, 2021

💛 Build succeeded, but was flaky

Test Failures

Stack Trace

Stack Trace

Metrics [docs]

Module Count

Async chunks

History

Uh oh!

dhurley14 Feb 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

banderror Feb 18, 2021

Choose a reason for hiding this comment

Uh oh!

dhurley14 left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Feb 18, 2021

💔 Backport failed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dhurley14 Feb 17, 2021 •

edited

Loading