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.

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][Detections] Removes the Tour for the new features on the Rule Management page introduced in 8.1 #128398

Merged

Conversation

banderror
Copy link
Contributor

@banderror banderror commented Mar 23, 2022

Ticket: #125504

Summary

This PR removes the Tour UI from components of the Rule Management page. We don't need to show 8.1 features in the 8.2.0 version. The Tour was previously introduced in #124343.

Details

  • The tour steps are removed from the components (<EuiTourStep>).
  • The tour provider is removed from the page. It has been changed a little bit.
  • I thought it could be useful to leave the implementation for now, in case we want to show new tours in the next versions. If we don't use it during the next few dev cycles given we will be shipping new features on the Rule Management page, we will need to completely remove it from the codebase.
  • A short README is added with some notes on the Tour UI.

@banderror banderror added backport:skip This commit does not require backporting Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Feature:Rule Management Security Solution Detection Rule Management Team:Detection Rule Management Security Detection Rule Management Team v8.2.0 labels Mar 23, 2022
@banderror banderror requested a review from a team as a code owner March 23, 2022 17:01
@banderror banderror self-assigned this Mar 23, 2022
@banderror banderror requested a review from a team March 23, 2022 17:01
@banderror banderror force-pushed the remove-tour-ui-on-rule-management-page branch from a0dbdd6 to adaa518 Compare March 23, 2022 17:05
@elasticmachine
Copy link
Contributor

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

@banderror banderror added the release_note:skip Skip the PR/issue when compiling release notes label Mar 23, 2022
@elasticmachine
Copy link
Contributor

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

Copy link
Member

@spong spong left a comment

Choose a reason for hiding this comment

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

Checked out, tested locally (with carryover upgrade from your other tour PR), and LGTM! No issues in testing and code changes LGTM. Thank you for the additional comments and README to explain this functionality (why it's not enabled), and how to leverage it/update it in the future, really helps with context here . In agreement with keeping this code around for a release or two and then removing it at a later date. 👍

A side question, do we want to proactively remove these keys from the user's localstorage as part of these updates/removals? Do you know if the storage kibana.service has any sort of expiration cooked in, or is that all on the implementer? I would imagine the latter, but just curious here about these keys lingering around release over release...

@banderror
Copy link
Contributor Author

banderror commented Mar 24, 2022

@spong many thanks for your review 🙏

A side question, do we want to proactively remove these keys from the user's localstorage as part of these updates/removals?

Ideally - probably yes. But if we talk only about this storage key, IMHO it would be a very minor tech improvement compared to all the other tech stuff we can work on.

However, I'd say having a custom storage API in Kibana that would allow us to define some expiration and cleanup policy, as well as (and especially) versioning and migration for the stored data, could be useful.

Do you know if the storage kibana.service has any sort of expiration cooked in, or is that all on the implementer? I would imagine the latter, but just curious here about these keys lingering around release over release...

I wouldn't expect the storage exposed from Kibana services to implement any TTL/expiration/cleanup logic unless it was available in its interface explicitly. Meaning if we didn't set up a TTL for our key, Kibana shouldn't delete it based on some cleanup policy, because otherwise, it could break our app.

@banderror
Copy link
Contributor Author

I'm going to update the added README based on the notes from #128405 (comment) and then will merge this guy.

@banderror banderror force-pushed the remove-tour-ui-on-rule-management-page branch from adaa518 to d50e93a Compare March 24, 2022 12:49
@banderror banderror force-pushed the remove-tour-ui-on-rule-management-page branch from d50e93a to 5c906b3 Compare March 24, 2022 18:40
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Test Failures

  • [job] [logs] Security Solution Tests / value lists user with restricted access role "before each" hook for "Does not allow a t1 analyst user to upload a value list"

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 2986 2984 -2

Async chunks

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

id before after diff
securitySolution 4.8MB 4.8MB -3.1KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
securitySolution 247.1KB 247.0KB -99.0B

History

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

cc @banderror

@banderror banderror merged commit 0498f69 into elastic:main Mar 24, 2022
@banderror banderror deleted the remove-tour-ui-on-rule-management-page branch March 24, 2022 21:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Rule Management Security Solution Detection Rule Management release_note:skip Skip the PR/issue when compiling release notes Team:Detection Rule Management Security Detection Rule Management Team Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v8.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Security Solution][Detections] Remove the Tour for the new 8.1 features
5 participants