Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Security Solution] Default Risk score slide bar values are overlaid on Rule Creation page #161793

Merged
merged 6 commits into from
Jul 18, 2023
Merged

[Security Solution] Default Risk score slide bar values are overlaid on Rule Creation page #161793

merged 6 commits into from
Jul 18, 2023

Conversation

e40pud
Copy link
Contributor

@e40pud e40pud commented Jul 12, 2023 •
edited by kibanamachine
Loading

Summary

Original ticket: #161456

These changes do workaround to display EuiRange component which is broken due to memoization (see more details in #160561 and elastic/eui#6846)

Before the fix:

Screenshot 2023-07-12 at 19 06 01

After the fix:

Screenshot 2023-07-12 at 19 05 46

@e40pud e40pud added release_note:skip Skip the PR/issue when compiling release notes ci:cloud-deploy Create or update a Cloud deployment v8.9.0 Team:Detection Engine Security Solution Detection Engine Area labels Jul 12, 2023
@e40pud e40pud requested a review from a team as a code owner July 12, 2023 17:09
@e40pud e40pud self-assigned this Jul 12, 2023
marshallmain
marshallmain reviewed Jul 12, 2023
View reviewed changes
...ins/security_solution/public/detection_engine/rule_creation_ui/pages/rule_creation/index.tsx Outdated
// EUI team suggested not to memoize EuiRange/EuiDualRange: https://github.com/elastic/eui/issues/6846
// Related ticket: https://github.com/elastic/kibana/issues/160561
// NOTE: We should remove this workaround once EUI team fixed EuiRange.
activeStep !== RuleStep.aboutRule ? null : (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need the StepAboutRule component to remain in the DOM even when it's not the active step, otherwise the formData gets cleared. Perhaps triggering an extra re-render at some point while the component is setting up could be a workaround for the risk score track issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok, that's why probably so many tests are broken :-) Will check what we can do to re-render action step after it appeared.

@e40pud
Review feedback
2a5acb4 
Instead of removing about page form from the DOM we should do extra render
@e40pud e40pud requested a review from a team as a code owner July 13, 2023 09:21
@e40pud e40pud requested a review from spong July 13, 2023 09:21
@e40pud e40pud requested a review from marshallmain July 13, 2023 09:28
@e40pud
Copy link
Contributor Author

e40pud commented Jul 13, 2023

@elasticmachine merge upstream

@spong
Copy link
Member

spong commented Jul 14, 2023

@e40pud, I'm going to be out next week and wasn't able to get to reviewing this today so am going to remove myself as a reviewer and let someone from the @elastic/security-detection-rule-management team take over if that's ok 👍

@spong spong removed their request for review July 14, 2023 22:08
vitaliidm
vitaliidm approved these changes Jul 18, 2023
View reviewed changes
Copy link
Contributor

@vitaliidm vitaliidm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @e40pud. LGTM

x-pack/plugins/security_solution/public/detections/components/rules/step_about_rule/index.tsx Outdated Show resolved Hide resolved
jpdjere
jpdjere approved these changes Jul 18, 2023
View reviewed changes
Copy link
Contributor

@jpdjere jpdjere left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the fix 👍

@kibana-ci
Copy link
Collaborator

kibana-ci commented Jul 18, 2023 •
edited
Loading

💚 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 9.8MB 9.8MB +70.0B

History

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

cc @e40pud

@e40pud e40pud added v8.9.1 and removed v8.9.0 labels Jul 18, 2023
@e40pud e40pud merged commit 731f587 into elastic:main Jul 18, 2023
@e40pud e40pud added release_note:fix and removed release_note:skip Skip the PR/issue when compiling release notes v8.10.0 labels Jul 18, 2023
@kibanamachine kibanamachine mentioned this pull request Jul 18, 2023
Merged
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Jul 18, 2023
@e40pud
[Security Solution] Default Risk score slide bar values are overlaid …
1ff34e2 
…on Rule Creation page (elastic#161793)

## Summary

Original ticket: elastic#161456

These changes do workaround to display EuiRange component which is
broken due to memoization (see more details in
elastic#160561 and
elastic/eui#6846)

Before the fix:

<img width="435" alt="Screenshot 2023-07-12 at 19 06 01"
src="https://github.com/elastic/kibana/assets/2700761/8deeb59a-712e-4990-9bda-cba31da8f23f">

After the fix:

<img width="426" alt="Screenshot 2023-07-12 at 19 05 46"
src="https://github.com/elastic/kibana/assets/2700761/7556cca0-d78b-453a-ac47-8a2cd3ef73d7">

---------

Co-authored-by: Kibana Machine <[email protected]>
Co-authored-by: Vitalii Dmyterko <[email protected]>
(cherry picked from commit 731f587)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
✅ 8.9

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Jul 18, 2023
@kibanamachine @e40pud
[8.9] [Security Solution] Default Risk score slide bar values are ove…
0fa5afd 
…rlaid on Rule Creation page (#161793) (#162138)

# Backport

This will backport the following commits from `main` to `8.9`:
- [[Security Solution] Default Risk score slide bar values are overlaid
on Rule Creation page
(#161793)](#161793)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Ievgen
Sorokopud","email":"[email protected]"},"sourceCommit":{"committedDate":"2023-07-18T12:36:07Z","message":"[Security
Solution] Default Risk score slide bar values are overlaid on Rule
Creation page (#161793)\n\n## Summary\r\n\r\nOriginal ticket:
#161456\r\n\r\nThese changes do workaround to display EuiRange component
which is\r\nbroken due to memoization (see more details
in\r\nhttps://github.com//issues/160561
and\r\nhttps://github.com/elastic/eui/issues/6846)\r\n\r\nBefore the
fix:\r\n\r\n<img width=\"435\" alt=\"Screenshot 2023-07-12 at 19 06
01\"\r\nsrc=\"https://github.com/elastic/kibana/assets/2700761/8deeb59a-712e-4990-9bda-cba31da8f23f\">\r\n\r\nAfter
the fix:\r\n\r\n<img width=\"426\" alt=\"Screenshot 2023-07-12 at 19 05
46\"\r\nsrc=\"https://github.com/elastic/kibana/assets/2700761/7556cca0-d78b-453a-ac47-8a2cd3ef73d7\">\r\n\r\n---------\r\n\r\nCo-authored-by:
Kibana Machine
<[email protected]>\r\nCo-authored-by:
Vitalii Dmyterko
<[email protected]>","sha":"731f5879799b6d2e1e35a4c24cf6440fc60a631a","branchLabelMapping":{"^v8.10.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","ci:cloud-deploy","Team:Detection
Engine","v8.10.0","v8.9.1"],"number":161793,"url":"https://github.com/elastic/kibana/pull/161793","mergeCommit":{"message":"[Security
Solution] Default Risk score slide bar values are overlaid on Rule
Creation page (#161793)\n\n## Summary\r\n\r\nOriginal ticket:
#161456\r\n\r\nThese changes do workaround to display EuiRange component
which is\r\nbroken due to memoization (see more details
in\r\nhttps://github.com//issues/160561
and\r\nhttps://github.com/elastic/eui/issues/6846)\r\n\r\nBefore the
fix:\r\n\r\n<img width=\"435\" alt=\"Screenshot 2023-07-12 at 19 06
01\"\r\nsrc=\"https://github.com/elastic/kibana/assets/2700761/8deeb59a-712e-4990-9bda-cba31da8f23f\">\r\n\r\nAfter
the fix:\r\n\r\n<img width=\"426\" alt=\"Screenshot 2023-07-12 at 19 05
46\"\r\nsrc=\"https://github.com/elastic/kibana/assets/2700761/7556cca0-d78b-453a-ac47-8a2cd3ef73d7\">\r\n\r\n---------\r\n\r\nCo-authored-by:
Kibana Machine
<[email protected]>\r\nCo-authored-by:
Vitalii Dmyterko
<[email protected]>","sha":"731f5879799b6d2e1e35a4c24cf6440fc60a631a"}},"sourceBranch":"main","suggestedTargetBranches":["8.9"],"targetPullRequestStates":[{"branch":"main","label":"v8.10.0","labelRegex":"^v8.10.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/161793","number":161793,"mergeCommit":{"message":"[Security
Solution] Default Risk score slide bar values are overlaid on Rule
Creation page (#161793)\n\n## Summary\r\n\r\nOriginal ticket:
#161456\r\n\r\nThese changes do workaround to display EuiRange component
which is\r\nbroken due to memoization (see more details
in\r\nhttps://github.com//issues/160561
and\r\nhttps://github.com/elastic/eui/issues/6846)\r\n\r\nBefore the
fix:\r\n\r\n<img width=\"435\" alt=\"Screenshot 2023-07-12 at 19 06
01\"\r\nsrc=\"https://github.com/elastic/kibana/assets/2700761/8deeb59a-712e-4990-9bda-cba31da8f23f\">\r\n\r\nAfter
the fix:\r\n\r\n<img width=\"426\" alt=\"Screenshot 2023-07-12 at 19 05
46\"\r\nsrc=\"https://github.com/elastic/kibana/assets/2700761/7556cca0-d78b-453a-ac47-8a2cd3ef73d7\">\r\n\r\n---------\r\n\r\nCo-authored-by:
Kibana Machine
<[email protected]>\r\nCo-authored-by:
Vitalii Dmyterko
<[email protected]>","sha":"731f5879799b6d2e1e35a4c24cf6440fc60a631a"}},{"branch":"8.9","label":"v8.9.1","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Ievgen Sorokopud <[email protected]>
@nastasha-solomon nastasha-solomon mentioned this pull request Jul 23, 2023
Merged
ThomThomson pushed a commit to ThomThomson/kibana that referenced this pull request Aug 1, 2023
@e40pud @kibanamachine @vitaliidm
[Security Solution] Default Risk score slide bar values are overlaid …
ccac531 
…on Rule Creation page (elastic#161793)

## Summary

Original ticket: elastic#161456

These changes do workaround to display EuiRange component which is
broken due to memoization (see more details in
elastic#160561 and
elastic/eui#6846)

Before the fix:

<img width="435" alt="Screenshot 2023-07-12 at 19 06 01"
src="https://github.com/elastic/kibana/assets/2700761/8deeb59a-712e-4990-9bda-cba31da8f23f">

After the fix:

<img width="426" alt="Screenshot 2023-07-12 at 19 05 46"
src="https://github.com/elastic/kibana/assets/2700761/7556cca0-d78b-453a-ac47-8a2cd3ef73d7">

---------

Co-authored-by: Kibana Machine <[email protected]>
Co-authored-by: Vitalii Dmyterko <[email protected]>
@e40pud e40pud mentioned this pull request Aug 28, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vitaliidm vitaliidm vitaliidm approved these changes

@jpdjere jpdjere jpdjere approved these changes

@marshallmain marshallmain Awaiting requested review from marshallmain

Assignees

@e40pud e40pud

Labels
ci:cloud-deploy Create or update a Cloud deployment release_note:fix Team:Detection Engine Security Solution Detection Engine Area v8.9.0 v8.9.1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@e40pud @spong @kibana-ci @kibanamachine @jpdjere @marshallmain @vitaliidm