[Security Solution] Expandable flyout - Rule preview contents

[christineweng]

Summary

This PR is part 2 of adding a rule preview panel to the expandable flyout. PR (#161999) adds the preview skeleton, and this PR populates the actual content related to rule details:

Expandable flyout:

• Updated title to include created by and updated by timestamps, and rule switch button
• Added contents for about, define, schedule and actions (if any)
• Added a hook to fetch data for rule switch button - logic mimics rule details page (~/security_solution/public/detection_engine/rule_details_ui/pages/rule_details/index.tsx)

Rule & detections:

• Added isPanelView option allow rendering rule details in smaller font, so that it can fit in panel view
• Minor UI updates to gutter sizes and spacing to accommodate long text
• Extracted createdBy and updatedBy to ~/security_solution/public/detections/components/rules/rule_info to be shared between rule details page and flyout

How to test

• add xpack.securitySolution.enableExperimental: ['securityFlyoutEnabled'] to the kibana.dev.json file
• go to the Alerts page, and click on the expand detail button on any row of the table
• click on Overview, About, view Rule Summary, the rule preview panel should pop up

Checklist

• Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
• Unit or functional tests were updated or added to match the most common scenarios

[PhilippeOberti]

Left a few comments, the major one being the isInPanelView prop approach.

I would be fine keeping it, but I'm not a fan of props that are only there to drive the UI, especially if they are specific to a single use. In our case, it is extremely unlikely anybody else would ever need this specific behavior when reusing this component... Doing this a recipe to have later way too many props... (I understand that the columns prop is already doing that so we're not breaking a pattern here, but also to my point that's already 2 props to drive the UI...)

A few options I can see:

• have some sort of prop that would include all css changes
• like I mentioned in the comment, maybe try to see what we can do with the titleProps prop?
• could we pass in css changes and use them (I know it's feasible in Angular but have never done it with React)

But I believe my favorite approach would be to have our own component. If we look at the StepRuleDescriptionComponent, it's really not doing much... We don't care about the if (column==='multi') or the singleSplit sections.
Could we extract the listItems logic and build our own component?
The code duplication would be pretty small, and we wouldn't introduce a new usage-specific prop?

If you think it's too much work for not much value (which I don't deny) and if the @elastic/security-detection-rule-management team is OK with the current change, I can be convinced to merge the code as it is (after fixing the few nits)

[maximpn]

@christineweng these are nice changes for the rule preview 👍

Basically I agree with @PhilippeOberti regarding isInPanelView prop. We should avoid parametrisation as it adds too much complexity. React's philosophy is composition so we need to focus on this. Obviously it will require more effort but we can address some tech debt here. Rule details page wasn't designed to share its components in a wider environment and has suboptimal structure. It's possible to refactor it a bit to make the components you need shared. Specifically we need to address the following

• Have an ability to reuse rule details page's content with customizations at some other place (refactor for that StepRuleDescriptionComponent to split it or have a render prop)
• Make RuleSwitch shared without necessity to pull all the data and permission (avoid custom useRuleSwitch appearing in different places)
• Share the text (it looks like updated by, created by and enable/disabled rule texts are the same and it worth to find a way to share it via ready to use components)

I understand that it inflates the scope but I think we can find a way to address it. Feel free to reach me to discuss the above by voice if necessary.

[maximpn]

@christineweng fyi, Rule Management team has been working on a similar task having a flyout with rule details in this PR. There is an idea to build rule details lightweight components everyone can reuse to display rule details information. The feature is targeted 8.10. Taking into account my comments above it makes sense to have isInPanelView for now.

[nikitaindik]

@christineweng @maximpn Hey folks! Just wanted to clarify that we (the Rules Mgmt team) can't guarantee that we will be able to properly implement the reusable components mentioned above by FF date. We are doing our best to squeeze this refactoring in, but please don't count on us to have it fully ready by FF.

[christineweng]

@maximpn thank you so much for the feedback!

• Have an ability to reuse rule details page's content with customizations at some other place (refactor for that StepRuleDescriptionComponent to split it or have a render prop)

• Based on the current work from Nikita, keep current implementation as is, maybe plus some documentation to explain what it does

• Make RuleSwitch shared without necessity to pull all the data and permission (avoid custom useRuleSwitch appearing in different places)

• i think this would be the biggest effort to refactor, and I agree that the current hook should not be necessary (it essentially duplicate all the CRUD/rule checking). Since the rule switch was not part of the MVP of this preview, I talked with product and they are ok to have it in a later release. I removed all the switch related code.

• Share the text (it looks like updated by, created by and enable/disabled rule texts are the same and it worth to find a way to share it via ready to use components)

• I'm open to sharing components, will ping you about file location etc.

[PhilippeOberti]

just an idea, but it the CreatedBy and UpdatedBy components were taking a single prop rule: Rule | null it would simplify the way devs use it, and they wouldn't have to check if the rule is null when using the component, like you do now (for example rule?.updated_by). That logic would be embedded in your component, which is a slightly nicer developer experience...

I'm not strongly attached to this though, and for the sake of time (mainly updating tests) I'm ok keeping what you have!

[PhilippeOberti]

LGTM, thanks for making all the changes! I left a few more small nits, I'll leave it to you to implement them or not!

One last request: would you mind writing a ticket on our expandable flyout board to log the tech debt (to better share hooks and components)?

[christineweng]

@PhilippeOberti thanks! ticket created here

[maximpn]

@christineweng thank you for addressing my comments 🙏 special thanks for CreatedBy and UpdatedBy components 😉

[kibana-ci]

💚 Build Succeeded

• Buildkite Build
• Commit: 166e2f6

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
securitySolution	4393	4396	+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	15.6MB	15.6MB	+11.0KB

Unknown metric groups

References to deprecated APIs

id	before	after	diff
securitySolution	625	630	+5

History

• 💛 Build #149010 was flaky ae63e2b
• 💔 Build #148973 failed ab8fa09f114dffdeec30d8b9ab1fe2717befd28f
• 💔 Build #148954 failed dd2105c
• 💔 Build #148551 failed 712ae7eaac52eca59320fb0562ebdf27b51289cd
• 💔 Build #147382 failed 0ce6eaa
• 💔 Build #147286 failed f1da4a1

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

cc @christineweng