-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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.
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
[Security Solution] Address guided onboarding feedback for the rules area #145223
Conversation
55bf5dc
to
92c8273
Compare
Pinging @elastic/security-detections-response (Team:Detections and Resp) |
Pinging @elastic/security-solution (Team: SecuritySolution) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xcrzx The changes LGTM
There is a comment for improvement.
false | ||
); | ||
|
||
const { data: onboardingRules } = useFindRulesQuery( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea that only demoRule
is needed so it's possible to check if that rule is loaded. So the functionality can be moved to a separate hook like useDemoRule()
which returns demoRule
and isLoading
which should be enough to properly determine the tourStatus
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I like that idea 👍 We could improve that in a follow-up if you don't mind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good
}; | ||
|
||
export enum GuidedOnboardingRulesStatus { | ||
'inactive' = 'inactive', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just curious why enum keys are string literals while it can be started with capital case keys as in the TypeScript docs? So it could be
export enum GuidedOnboardingRulesStatus {
Inactive = 'inactive',
InstallRules = 'installRules',
...
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💚 Build Succeeded
Metrics [docs]Async chunks
Unknown metric groupsESLint disabled in files
ESLint disabled line counts
Total ESLint disabled count
To update your PR or re-run it, just comment with: cc @xcrzx |
…ression and new terms multi fields (#145775) ## Summary Adds new tour highlighting new rule capabilities in 8.6 - new terms multi fields (#143943) and alert suppression (#142686). I tried using the generic `RulesFeatureTour` again (main...marshallmain:kibana:failed-tour) but it still crashes the page. I also looked at integrating this tour with the Guided onboarding tour for rules management (#145223), but concluded that they should be separate since guided onboarding is experimental and this tour should be displayed to users even if they are not new users. This PR is essentially a copy of the new terms tour in 8.4 (#138469).
…ression and new terms multi fields (elastic#145775) ## Summary Adds new tour highlighting new rule capabilities in 8.6 - new terms multi fields (elastic#143943) and alert suppression (elastic#142686). I tried using the generic `RulesFeatureTour` again (elastic/kibana@main...marshallmain:kibana:failed-tour) but it still crashes the page. I also looked at integrating this tour with the Guided onboarding tour for rules management (elastic#145223), but concluded that they should be separate since guided onboarding is experimental and this tour should be displayed to users even if they are not new users. This PR is essentially a copy of the new terms tour in 8.4 (elastic#138469). (cherry picked from commit 13c1b0b)
…r suppression and new terms multi fields (#145775) (#146479) # Backport This will backport the following commits from `main` to `8.6`: - [[Security Solution][Alerts] Add tour to rule management page for suppression and new terms multi fields (#145775)](#145775) <!--- Backport version: 8.9.7 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Marshall Main","email":"55718608+marshallmain@users.noreply.github.com"},"sourceCommit":{"committedDate":"2022-11-28T21:35:02Z","message":"[Security Solution][Alerts] Add tour to rule management page for suppression and new terms multi fields (#145775)\n\n## Summary\r\n\r\nAdds new tour highlighting new rule capabilities in 8.6 - new terms\r\nmulti fields (#143943) and alert\r\nsuppression (https://github.com/elastic/kibana/pull/142686).\r\n\r\nI tried using the generic `RulesFeatureTour` again\r\n(https://github.com/elastic/kibana/compare/main...marshallmain:kibana:failed-tour)\r\nbut it still crashes the page.\r\n\r\nI also looked at integrating this tour with the Guided onboarding tour\r\nfor rules management (https://github.com/elastic/kibana/pull/145223),\r\nbut concluded that they should be separate since guided onboarding is\r\nexperimental and this tour should be displayed to users even if they are\r\nnot new users.\r\n\r\nThis PR is essentially a copy of the new terms tour in 8.4\r\n(https://github.com/elastic/kibana/pull/138469).","sha":"13c1b0b863b7d8b324d33f2aaf45d90d5c8c108e","branchLabelMapping":{"^v8.7.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","Team: SecuritySolution","Team:Detection Alerts","v8.6.0","v8.7.0"],"number":145775,"url":"https://github.com/elastic/kibana/pull/145775","mergeCommit":{"message":"[Security Solution][Alerts] Add tour to rule management page for suppression and new terms multi fields (#145775)\n\n## Summary\r\n\r\nAdds new tour highlighting new rule capabilities in 8.6 - new terms\r\nmulti fields (#143943) and alert\r\nsuppression (https://github.com/elastic/kibana/pull/142686).\r\n\r\nI tried using the generic `RulesFeatureTour` again\r\n(https://github.com/elastic/kibana/compare/main...marshallmain:kibana:failed-tour)\r\nbut it still crashes the page.\r\n\r\nI also looked at integrating this tour with the Guided onboarding tour\r\nfor rules management (https://github.com/elastic/kibana/pull/145223),\r\nbut concluded that they should be separate since guided onboarding is\r\nexperimental and this tour should be displayed to users even if they are\r\nnot new users.\r\n\r\nThis PR is essentially a copy of the new terms tour in 8.4\r\n(https://github.com/elastic/kibana/pull/138469).","sha":"13c1b0b863b7d8b324d33f2aaf45d90d5c8c108e"}},"sourceBranch":"main","suggestedTargetBranches":["8.6"],"targetPullRequestStates":[{"branch":"8.6","label":"v8.6.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.7.0","labelRegex":"^v8.7.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/145775","number":145775,"mergeCommit":{"message":"[Security Solution][Alerts] Add tour to rule management page for suppression and new terms multi fields (#145775)\n\n## Summary\r\n\r\nAdds new tour highlighting new rule capabilities in 8.6 - new terms\r\nmulti fields (#143943) and alert\r\nsuppression (https://github.com/elastic/kibana/pull/142686).\r\n\r\nI tried using the generic `RulesFeatureTour` again\r\n(https://github.com/elastic/kibana/compare/main...marshallmain:kibana:failed-tour)\r\nbut it still crashes the page.\r\n\r\nI also looked at integrating this tour with the Guided onboarding tour\r\nfor rules management (https://github.com/elastic/kibana/pull/145223),\r\nbut concluded that they should be separate since guided onboarding is\r\nexperimental and this tour should be displayed to users even if they are\r\nnot new users.\r\n\r\nThis PR is essentially a copy of the new terms tour in 8.4\r\n(https://github.com/elastic/kibana/pull/138469).","sha":"13c1b0b863b7d8b324d33f2aaf45d90d5c8c108e"}}]}] BACKPORT--> Co-authored-by: Marshall Main <55718608+marshallmain@users.noreply.github.com>
Related to: #144016
Summary
This follow-up PR addresses guided onboarding feedback mentioned here and here.
To summarize:
Find rule step
Enable rule step
Testing instructions
xpack.securitySolution.enableExperimental: ['guidedOnboarding']
xpack.cloud.id: "x"
yarn start --run-examples
/app/guidedOnboardingExample
security
and Step ID torules