[Security Solution][Detections] - Fix loading indicators in the rules management table by banderror · Pull Request #91925 · elastic/kibana

banderror · 2021-02-18T19:47:01Z

Base PR: #91342
Fixes: #91336

Summary

This PR fixes loading indicators used in the rules management table.

[Added] Blocking indicator. We show a spinner and "freeze" (fade out) the table when any of these changes: filters, sorting, pagination, manual click on Refresh button.
[Adjusted] Non-blocking indicator. We show a non-blocking "ribbon" (progress bar) only when auto-refresh is in progress.
Initial loading indicator. We show it only on the first table load.

Code and tests are slightly adjusted. Things to note are marked below in additional GH comments.

Screenshots

Initial load:

Manual refresh:

Auto refresh:

Pagination:

Backports

-> 7.13 #92270
-> 7.12 #92266

Checklist

Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

banderror · 2021-02-19T18:01:57Z

...lugins/security_solution/public/detections/pages/detection_engine/rules/all/rules_tables.tsx

Cyclomatic complexity of the RulesTables component exceeded the limit. I disabled it for now. I think a decent refactoring is needed for the whole table (maybe the whole page).

yikes, we might want to file that in a tech debt ticket

@dplumlee Agree, I'll do that. I'll add a few tickets and update my plan for 7.13+. I'm planning to focus on optimizing our rules-related endpoints now (#91265 is in fact about that). Once I'm done with the endpoints, I hope to switch to the client-side table implementation and this tech debt.

Thanks for reminding about being explicit and transparent 👍

Here we go: #92160 and #92169

banderror · 2021-02-19T18:04:44Z

...lugins/security_solution/public/detections/pages/detection_engine/rules/all/rules_tables.tsx

Loader doesn't pass data-test-subj="loadingPanelAllRulesTable" to its children and renders data-test-subj="loading-spinner" internally.

Current Cypress tests use data-test-subj="loading-spinner" to identify this loading indicator. It works, but it's somewhat brittle.

banderror · 2021-02-19T18:15:16Z

x-pack/plugins/security_solution/cypress/tasks/alerts_detection_rules.ts

What do you think about using this click-and-wait approach by default? Think it's safer and the tests would probably be less flaky.

Some of the tests here were not working because of the lack of proper waiting for certain indicators on the page to show up and then disappear. I added this waiting where needed: either like here, inside the tasks, or outside of tasks (which seems to be a more common pattern in our codebase).

banderror · 2021-02-19T18:17:57Z

x-pack/plugins/security_solution/cypress/screens/alerts_detection_rules.ts

Not sure if constants like FIVE_ROWS are needed. Unless a constant is a kind of default for the majority of tests, I'd keep the function and get rid of the constants.

FYI cleaned this up in a separate commit.

banderror · 2021-02-19T18:18:20Z

x-pack/plugins/security_solution/cypress/screens/alerts_detection_rules.ts

Here's this generic loading-spinner

banderror · 2021-02-19T18:19:01Z

x-pack/plugins/security_solution/cypress/integration/detection_rules/sorting.spec.ts

Fixed and unskipped

elasticmachine · 2021-02-19T18:22:54Z

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

elasticmachine · 2021-02-19T18:24:38Z

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

dplumlee

This all looks good! If we're not too far over the complexity max for the rules table file it might be worth a small refactor to not have to disable it for this pr, otherwise we should open a ticket for it to be addressed in tech debt next release. Actually either way we should open a ticket being so close to the max, but optimally we obv don't want to disable lint rules for entire files. Maybe just around a certain function if possible?

banderror · 2021-02-22T13:27:16Z

I added new tickets for the tech debt and refactoring:

[Security Solution][Detections] Address cyclomatic complexity issue in RulesTables component #92160
[Security Solution] Extract frontend rule management logic from React components #92169

… management table (elastic#91925) **Base PR:** elastic#91342 **Fixes:** elastic#91336 ## Summary This PR fixes loading indicators used in the rules management table. - [Added] Blocking indicator. We show a spinner and "freeze" (fade out) the table when any of these changes: filters, sorting, pagination, manual click on Refresh button. - [Adjusted] Non-blocking indicator. We show a non-blocking "ribbon" (progress bar) only when auto-refresh is in progress. - Initial loading indicator. We show it only on the first table load. Code and tests are slightly adjusted. Things to note are marked below in additional GH comments. Co-authored-by: Yara Tercero <yara.tercero@elastic.co>

kibanamachine · 2021-02-22T19:10:59Z

💚 Backport successful

✅ 7.12 / #92266

Successful backport PRs will be merged automatically after passing CI.

… management table (#91925) (#92266) **Base PR:** #91342 **Fixes:** #91336 ## Summary This PR fixes loading indicators used in the rules management table. - [Added] Blocking indicator. We show a spinner and "freeze" (fade out) the table when any of these changes: filters, sorting, pagination, manual click on Refresh button. - [Adjusted] Non-blocking indicator. We show a non-blocking "ribbon" (progress bar) only when auto-refresh is in progress. - Initial loading indicator. We show it only on the first table load. Code and tests are slightly adjusted. Things to note are marked below in additional GH comments. Co-authored-by: Yara Tercero <yara.tercero@elastic.co> Co-authored-by: Georgii Gorbachev <georgii.gorbachev@elastic.co> Co-authored-by: Yara Tercero <yara.tercero@elastic.co>

kibanamachine · 2021-02-22T21:36:56Z

💚 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`	7.7MB	7.7MB	+878.0B

History

💛 Build #108444 was flaky 9a41944
💛 Build #108359 was flaky 9c45c63da7f13269ae0bddd7196852408cf5f64e
💔 Build #108309 failed 97d5cb5
💔 Build #108102 failed 0da851ac6ad942f8b418e898f0a36372cf07f27b
💔 Build #107967 failed 2b31075c642ca52af880237b5aa06a5cd0a1eba8

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

cc @banderror @yctercero

kibanamachine · 2021-02-23T19:21:20Z

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

… management table (elastic#91925) **Base PR:** elastic#91342 **Fixes:** elastic#91336 ## Summary This PR fixes loading indicators used in the rules management table. - [Added] Blocking indicator. We show a spinner and "freeze" (fade out) the table when any of these changes: filters, sorting, pagination, manual click on Refresh button. - [Adjusted] Non-blocking indicator. We show a non-blocking "ribbon" (progress bar) only when auto-refresh is in progress. - Initial loading indicator. We show it only on the first table load. Code and tests are slightly adjusted. Things to note are marked below in additional GH comments. Co-authored-by: Yara Tercero <yara.tercero@elastic.co>

banderror · 2021-02-24T10:23:48Z

On it ^^^

… management table (#91925) (#92270) **Base PR:** #91342 **Fixes:** #91336 ## Summary This PR fixes loading indicators used in the rules management table. - [Added] Blocking indicator. We show a spinner and "freeze" (fade out) the table when any of these changes: filters, sorting, pagination, manual click on Refresh button. - [Adjusted] Non-blocking indicator. We show a non-blocking "ribbon" (progress bar) only when auto-refresh is in progress. - Initial loading indicator. We show it only on the first table load. Code and tests are slightly adjusted. Things to note are marked below in additional GH comments. Co-authored-by: Yara Tercero <yara.tercero@elastic.co> Co-authored-by: Yara Tercero <yara.tercero@elastic.co> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

banderror mentioned this pull request Feb 18, 2021

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

Merged

1 task

banderror commented Feb 19, 2021

View reviewed changes

banderror self-assigned this Feb 19, 2021

banderror added bug Fixes for quality problems that affect the customer experience release_note:fix Team:Detections and Resp Security Detection Response Team v7.12.0 v8.0.0 labels Feb 19, 2021

banderror requested review from a team and yctercero February 19, 2021 18:22

banderror marked this pull request as ready for review February 19, 2021 18:22

banderror requested a review from a team as a code owner February 19, 2021 18:22

banderror added the Feature:Detection Rules Security Solution rules and Detection Engine label Feb 19, 2021

banderror assigned yctercero Feb 19, 2021

banderror added the Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. label Feb 19, 2021

dplumlee approved these changes Feb 19, 2021

View reviewed changes

yctercero and others added 7 commits February 22, 2021 11:27

wip - proper loading ui

f48fec9

trying to get cypress tests to work

86c1b71

removing flakey test

c3632d7

cleanup

579e1bc

Fix unit tests

89c99bb

Clean up code a bit

bf88a39

Fix Cypress tests

97d5cb5

banderror mentioned this pull request Feb 22, 2021

[Security Solution][Detections] Address cyclomatic complexity issue in RulesTables component #92160

Open

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

Fix and clean up Cypress tests

9a41944

banderror removed the auto-backport Deprecated - use backport:version if exact versions are needed label Feb 22, 2021

banderror force-pushed the rules-table-loading-indicators branch from 9c45c63 to 9a41944 Compare February 22, 2021 15:47

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

banderror merged commit 4511fe5 into elastic:master Feb 22, 2021

banderror mentioned this pull request Feb 22, 2021

[Security Solution][Detections] - Fix all rules page loading states for different actions #91342

Closed

8 tasks

kibanamachine mentioned this pull request Feb 22, 2021

[7.12] [Security Solution][Detections] - Fix loading indicators in the rules management table (#91925) #92266

Merged

banderror deleted the rules-table-loading-indicators branch February 22, 2021 19:11

banderror added the v7.13.0 label Feb 22, 2021

banderror restored the rules-table-loading-indicators branch February 22, 2021 19:24

banderror mentioned this pull request Feb 22, 2021

[7.x] [Security Solution][Detections] - Fix loading indicators in the rules management table (#91925) #92270

Merged

banderror deleted the rules-table-loading-indicators branch February 22, 2021 19:29

kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Feb 23, 2021

banderror added the v7.11.2 label Feb 24, 2021

banderror mentioned this pull request Feb 24, 2021

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

Closed

banderror removed the v7.11.2 label Feb 24, 2021

kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Feb 24, 2021

Conversation

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

Uh oh!

Summary

Screenshots

Backports

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Feb 19, 2021

Uh oh!

elasticmachine commented Feb 19, 2021

Uh oh!

dplumlee left a comment

Choose a reason for hiding this comment

Uh oh!

banderror commented Feb 22, 2021

Uh oh!

kibanamachine commented Feb 22, 2021

💚 Backport successful

Uh oh!

kibanamachine commented Feb 22, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

History

Uh oh!

kibanamachine commented Feb 23, 2021

Uh oh!

banderror commented Feb 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

banderror commented Feb 18, 2021 •

edited

Loading