[Security Solution][Expandable flyout] - replace advanced settings with feature flag #184169
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
PhilippeOberti
force-pushed
the
expandable-flyout-ff
branch
4 times, most recently
from
0635f52 to
83f1baa
Compare
PhilippeOberti
force-pushed
the
expandable-flyout-ff
branch
from
83f1baa to
3dcb99e
Compare
PhilippeOberti
added
release_note:skip
|
Pinging @elastic/security-threat-hunting-investigations (Team:Threat Hunting:Investigations) |
Files by Code Ownerelastic/appex-sharedux
elastic/kibana-core
elastic/kibana-localization
elastic/kibana-management
elastic/kibana-telemetry
elastic/security-defend-workflows
elastic/security-detection-engine
elastic/security-engineering-productivity
elastic/security-entity-analytics
elastic/security-solution
elastic/security-threat-hunting-explore
elastic/security-threat-hunting-investigations
|
pgayvallet
approved these changes
May 30, 2024
sebelga
approved these changes
May 30, 2024
pzl
approved these changes
May 30, 2024
rylnd
approved these changes
May 30, 2024
| @@ -71,39 +69,37 @@ describe('Threat Match Enrichment', { tags: ['@ess', '@serverless', '@skipInServ | |||
| }); | |||
| }); | |||
|
|
|||
| it('Displays persisted enrichments on the JSON view', () => { | |||
There was a problem hiding this comment.
The addition of the Table tab test is good, but could you explain why the JSON view coverage was removed from cypress?
There was a problem hiding this comment.
hey @rylnd, I can explain for sure!
I tried for 2+ hours to fix the JSON test, but the issue is the new flyout uses the JsonCodeEditor from the unified_doc_viewer which is different from the old flyout that was using a simple EuiCodeBlock. The code block was allowing us to easily retrieve the content and test against it. The JsonCodeEditor does not allow to do that.
The only way I kinda found to get it to work was to Cypress scroll to the correct position and select the row, but this is extremely flaky and we should never do that.
My thought was, for this test we don't really care if the value is showing in the JSON tab. The purpose of this test is not to verify that the flyout JSON tab shows the correct value. We should trust that the JSON tab and its components are rendering the document correctly (and we already have tests for that on the expandable flyout section). What we care about if the fact that the document has the correct value. The easiest way I found to do this was by filtering the table and verifying that the expected value was there.
Is that a wrong assumption?
christineweng
approved these changes
May 30, 2024
| @@ -25,43 +24,34 @@ jest.mock('@kbn/expandable-flyout'); | |||
| const onFlyoutCloseMock = jest.fn(); | |||
|
|
|||
| describe('useUnifiedTimelineExpandableFlyout', () => { | |||
| beforeEach(() => { | |||
| it('should have expandable flyout disabled when expandable flyout is disabled in Experimental Features', () => { | |||
There was a problem hiding this comment.
nit for readability, consider rephrasing it to "should disable expandable flyout when expandableFlyoutDisabled flag is true"
There was a problem hiding this comment.
much better, fixed in 7f60145 thanks!
| @@ -30,12 +29,12 @@ import { visit } from '../../../tasks/navigation'; | |||
| import { startAlertsCasesTour } from '../../../tasks/api_calls/tour'; | |||
| import { deleteAlertsAndRules } from '../../../tasks/api_calls/common'; | |||
|
|
|||
| describe('Guided onboarding tour', { tags: ['@ess'] }, () => { | |||
| // re-enable this when https://github.com/elastic/kibana/pull/181442 is merged | |||
| ftrConfig: { | ||
| kbnServerArgs: [ | ||
| `--xpack.securitySolution.enableExperimental=${JSON.stringify([ | ||
| 'expandableFlyoutDisabled', |
There was a problem hiding this comment.
for backlog: we should have some cypress tests for flyout in timeline - an expandable flyout equivalent of this file
There was a problem hiding this comment.
yeah, I was being lazy... I added some basic tests (here 7f60145) that we should probably improve a bit when we remove this entire old flyout codebase!
PhilippeOberti
force-pushed
the
expandable-flyout-ff
branch
from
3dcb99e to
f4f61bc
Compare
PhilippeOberti
commented
May 30, 2024
| @@ -201,7 +197,8 @@ describe('Isolate command', { tags: ['@ess', '@serverless', '@brokenInServerless | |||
| } | |||
| ); | |||
|
|
|||
| describe('from Cases', () => { | |||
| // TODO re-enable when https://github.com/elastic/security-team/issues/9625 is merged | |||
| describe.skip('from Cases', () => { | |||
There was a problem hiding this comment.
this test highlighted a bug with how the expandable flyout is set up to work in Cases. As the expandable flyout has been enabled since 8.10, this PR doesn't introduce any new issues, but just reveals the fact that the Cypress tests were behind, by still using the old flyout.
We'll take a look at the ticket to fix the issue shortly, and will re-enable the tests!
yuliacech
approved these changes
May 31, 2024
| @@ -92,9 +92,6 @@ describe('Isolate command', { tags: ['@ess', '@serverless', '@brokenInServerless | |||
| describe.skip( | |||
There was a problem hiding this comment.
just fyi, created ticket for our team to re-enable this: https://github.com/elastic/security-team/issues/9626
gergoabraham
approved these changes
May 31, 2024
angorayc
reviewed
May 31, 2024
| @@ -30,12 +29,12 @@ import { visit } from '../../../tasks/navigation'; | |||
| import { startAlertsCasesTour } from '../../../tasks/api_calls/tour'; | |||
| import { deleteAlertsAndRules } from '../../../tasks/api_calls/common'; | |||
|
|
|||
| describe('Guided onboarding tour', { tags: ['@ess'] }, () => { | |||
| // re-enable this when https://github.com/elastic/kibana/pull/181442 is merged | |||
| describe.skip('Guided onboarding tour', { tags: ['@ess'] }, () => { | |||
There was a problem hiding this comment.
no, I had skipped it because my branch was opened at the time it was failing and you were working on the fix. It's unskipped in my local, and working!
I'm fixing a couple of other comments then will push a commit. No changes to this file anymore!
PhilippeOberti
force-pushed
the
expandable-flyout-ff
branch
from
f4f61bc to
7f60145
Compare
angorayc
approved these changes
May 31, 2024
PhilippeOberti
added
release_note:enhancement
and removed
release_note:skip
PhilippeOberti
force-pushed
the
expandable-flyout-ff
branch
from
7f60145 to
5276ab1
Compare
- unskip tour cypress test - update investigate in timeline cypress test - fix test title
PhilippeOberti
force-pushed
the
expandable-flyout-ff
branch
from
5276ab1 to
36ae0a0
Compare
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
1 task
machadoum
added a commit
that referenced
this pull request
Jun 12, 2024
## Summary MKI environment does not support feature flags. The test needs the old flyout to work, but the new flyout is now rendered by default. PR Athat introduced the issue: #184169 ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
The goal of this PR is to simplify the current expandable flyout advanced settings and feature flag setup. We currently have 1 advanced settings and 5 feature flags around the usage of the expandable flyout in Security Solution. The advanced settings and all the feature flags are
trueby default.This PR consolidates the 5 feature flags and the advanced settings into a single feature flag. The new flag is called
expandableFlyoutDisabledand is set tofalseby default. That way customers can turn the usage of the expandable flyout within Security Solution with one change in theirkibana.ymlfile, should the need arise.Here are the main changes:
securitySolution:enableExpandableFlyoutadvanced settings, so that users cannot revert to the old flyout through the UIexpandableEventFlyoutEnabled,expandableFlyoutInCreateRuleEnabled,expandableTimelineFlyoutEnabled,newUserDetailsFlyoutandnewHostDetailsFlyoutfeature flags. All these flags weretrueby default so we're not expecting customers to change these values.TO DO
https://github.com/elastic/security-team/issues/9360