From 2e73aa5aa073a756c28a42c830b4a4983ec7df58 Mon Sep 17 00:00:00 2001 From: Ryland Herrick Date: Tue, 13 Oct 2020 11:22:11 -0500 Subject: [PATCH] [Security Solution][Detections] EQL UX Improvements (#80230) * Add loading spinner to EQL Query Bar This gives the user some indication that the validation is being performed in the background, and that their query's validity is currently undefined. * Preserve whitespace on display of rule queries This is useful for all rules, but particularly for EQL rules, where sequences will be multiline. * Fix tests around query description This content is now wrapped in a whitespace-preserving element, so these need to be updated ever so slightly. * Update cypress assertions around rule queries * Remove extra space from assertion * This extra space was removed in the presentation component. I didn't find an explicit reason why it was there in the first place. * Use have.value when asserting on form inputs --- .../alerts_detection_rules_custom.spec.ts | 8 ++++---- .../integration/alerts_detection_rules_eql.spec.ts | 2 +- .../alerts_detection_rules_override.spec.ts | 2 +- .../alerts_detection_rules_threshold.spec.ts | 2 +- .../cypress/tasks/create_new_rule.ts | 4 ++-- .../rules/description_step/helpers.test.tsx | 7 +++++-- .../components/rules/description_step/helpers.tsx | 8 ++++++-- .../components/rules/description_step/index.test.tsx | 6 ++++-- .../components/rules/eql_query_bar/eql_query_bar.tsx | 4 ++-- .../components/rules/eql_query_bar/footer.tsx | 12 +++++++++--- 10 files changed, 35 insertions(+), 20 deletions(-) diff --git a/x-pack/plugins/security_solution/cypress/integration/alerts_detection_rules_custom.spec.ts b/x-pack/plugins/security_solution/cypress/integration/alerts_detection_rules_custom.spec.ts index d8832dc4ee600..28889920e00e5 100644 --- a/x-pack/plugins/security_solution/cypress/integration/alerts_detection_rules_custom.spec.ts +++ b/x-pack/plugins/security_solution/cypress/integration/alerts_detection_rules_custom.spec.ts @@ -135,7 +135,7 @@ describe('Custom detection rules creation', () => { // expect define step to repopulate cy.get(DEFINE_EDIT_BUTTON).click(); - cy.get(CUSTOM_QUERY_INPUT).should('have.text', newRule.customQuery); + cy.get(CUSTOM_QUERY_INPUT).should('have.value', newRule.customQuery); cy.get(DEFINE_CONTINUE_BUTTON).should('exist').click({ force: true }); cy.get(DEFINE_CONTINUE_BUTTON).should('not.exist'); @@ -182,7 +182,7 @@ describe('Custom detection rules creation', () => { cy.get(ABOUT_INVESTIGATION_NOTES).should('have.text', INVESTIGATION_NOTES_MARKDOWN); cy.get(DEFINITION_DETAILS).within(() => { getDetails(INDEX_PATTERNS_DETAILS).should('have.text', indexPatterns.join('')); - getDetails(CUSTOM_QUERY_DETAILS).should('have.text', `${newRule.customQuery} `); + getDetails(CUSTOM_QUERY_DETAILS).should('have.text', newRule.customQuery); getDetails(RULE_TYPE_DETAILS).should('have.text', 'Query'); getDetails(TIMELINE_TEMPLATE_DETAILS).should('have.text', 'None'); }); @@ -293,7 +293,7 @@ describe('Custom detection rules deletion and edition', () => { waitForKibana(); // expect define step to populate - cy.get(CUSTOM_QUERY_INPUT).should('have.text', existingRule.customQuery); + cy.get(CUSTOM_QUERY_INPUT).should('have.value', existingRule.customQuery); if (existingRule.index && existingRule.index.length > 0) { cy.get(DEFINE_INDEX_INPUT).should('have.text', existingRule.index.join('')); } @@ -344,7 +344,7 @@ describe('Custom detection rules deletion and edition', () => { 'have.text', expectedEditedIndexPatterns.join('') ); - getDetails(CUSTOM_QUERY_DETAILS).should('have.text', `${editedRule.customQuery} `); + getDetails(CUSTOM_QUERY_DETAILS).should('have.text', editedRule.customQuery); getDetails(RULE_TYPE_DETAILS).should('have.text', 'Query'); getDetails(TIMELINE_TEMPLATE_DETAILS).should('have.text', 'None'); }); diff --git a/x-pack/plugins/security_solution/cypress/integration/alerts_detection_rules_eql.spec.ts b/x-pack/plugins/security_solution/cypress/integration/alerts_detection_rules_eql.spec.ts index ca7832603f13d..7914a1077aadc 100644 --- a/x-pack/plugins/security_solution/cypress/integration/alerts_detection_rules_eql.spec.ts +++ b/x-pack/plugins/security_solution/cypress/integration/alerts_detection_rules_eql.spec.ts @@ -143,7 +143,7 @@ describe('Detection rules, EQL', () => { cy.get(ABOUT_INVESTIGATION_NOTES).should('have.text', INVESTIGATION_NOTES_MARKDOWN); cy.get(DEFINITION_DETAILS).within(() => { getDetails(INDEX_PATTERNS_DETAILS).should('have.text', indexPatterns.join('')); - getDetails(CUSTOM_QUERY_DETAILS).should('have.text', `${eqlRule.customQuery} `); + getDetails(CUSTOM_QUERY_DETAILS).should('have.text', eqlRule.customQuery); getDetails(RULE_TYPE_DETAILS).should('have.text', 'Event Correlation'); getDetails(TIMELINE_TEMPLATE_DETAILS).should('have.text', 'None'); }); diff --git a/x-pack/plugins/security_solution/cypress/integration/alerts_detection_rules_override.spec.ts b/x-pack/plugins/security_solution/cypress/integration/alerts_detection_rules_override.spec.ts index 090012de72534..abc873f2df0ee 100644 --- a/x-pack/plugins/security_solution/cypress/integration/alerts_detection_rules_override.spec.ts +++ b/x-pack/plugins/security_solution/cypress/integration/alerts_detection_rules_override.spec.ts @@ -164,7 +164,7 @@ describe('Detection rules, override', () => { cy.get(ABOUT_INVESTIGATION_NOTES).should('have.text', INVESTIGATION_NOTES_MARKDOWN); cy.get(DEFINITION_DETAILS).within(() => { getDetails(INDEX_PATTERNS_DETAILS).should('have.text', indexPatterns.join('')); - getDetails(CUSTOM_QUERY_DETAILS).should('have.text', `${newOverrideRule.customQuery} `); + getDetails(CUSTOM_QUERY_DETAILS).should('have.text', newOverrideRule.customQuery); getDetails(RULE_TYPE_DETAILS).should('have.text', 'Query'); getDetails(TIMELINE_TEMPLATE_DETAILS).should('have.text', 'None'); }); diff --git a/x-pack/plugins/security_solution/cypress/integration/alerts_detection_rules_threshold.spec.ts b/x-pack/plugins/security_solution/cypress/integration/alerts_detection_rules_threshold.spec.ts index 5ee7e69e877e3..9d988a46662fa 100644 --- a/x-pack/plugins/security_solution/cypress/integration/alerts_detection_rules_threshold.spec.ts +++ b/x-pack/plugins/security_solution/cypress/integration/alerts_detection_rules_threshold.spec.ts @@ -143,7 +143,7 @@ describe('Detection rules, threshold', () => { cy.get(ABOUT_INVESTIGATION_NOTES).should('have.text', INVESTIGATION_NOTES_MARKDOWN); cy.get(DEFINITION_DETAILS).within(() => { getDetails(INDEX_PATTERNS_DETAILS).should('have.text', indexPatterns.join('')); - getDetails(CUSTOM_QUERY_DETAILS).should('have.text', `${newThresholdRule.customQuery} `); + getDetails(CUSTOM_QUERY_DETAILS).should('have.text', newThresholdRule.customQuery); getDetails(RULE_TYPE_DETAILS).should('have.text', 'Threshold'); getDetails(TIMELINE_TEMPLATE_DETAILS).should('have.text', 'None'); getDetails(THRESHOLD_DETAILS).should( diff --git a/x-pack/plugins/security_solution/cypress/tasks/create_new_rule.ts b/x-pack/plugins/security_solution/cypress/tasks/create_new_rule.ts index 9036d3747807f..8a6f764a8cc5e 100644 --- a/x-pack/plugins/security_solution/cypress/tasks/create_new_rule.ts +++ b/x-pack/plugins/security_solution/cypress/tasks/create_new_rule.ts @@ -190,7 +190,7 @@ export const fillDefineCustomRuleWithImportedQueryAndContinue = ( ) => { cy.get(IMPORT_QUERY_FROM_SAVED_TIMELINE_LINK).click(); cy.get(TIMELINE(rule.timelineId)).click(); - cy.get(CUSTOM_QUERY_INPUT).should('have.text', rule.customQuery); + cy.get(CUSTOM_QUERY_INPUT).should('have.value', rule.customQuery); cy.get(DEFINE_CONTINUE_BUTTON).should('exist').click({ force: true }); cy.get(CUSTOM_QUERY_INPUT).should('not.exist'); @@ -208,7 +208,7 @@ export const fillDefineThresholdRuleAndContinue = (rule: ThresholdRule) => { const threshold = 1; cy.get(CUSTOM_QUERY_INPUT).type(rule.customQuery); - cy.get(CUSTOM_QUERY_INPUT).invoke('text').should('eq', rule.customQuery); + cy.get(CUSTOM_QUERY_INPUT).should('have.value', rule.customQuery); cy.get(THRESHOLD_INPUT_AREA) .find(INPUT) .then((inputs) => { diff --git a/x-pack/plugins/security_solution/public/detections/components/rules/description_step/helpers.test.tsx b/x-pack/plugins/security_solution/public/detections/components/rules/description_step/helpers.test.tsx index 2ce9d1ea68b3c..ebdfdcc262b34 100644 --- a/x-pack/plugins/security_solution/public/detections/components/rules/description_step/helpers.test.tsx +++ b/x-pack/plugins/security_solution/public/detections/components/rules/description_step/helpers.test.tsx @@ -168,8 +168,11 @@ describe('helpers', () => { query: mockQueryBarWithQuery.query, savedId: mockQueryBarWithQuery.saved_id, }); - expect(result[0].title).toEqual(<>{i18n.QUERY_LABEL} ); - expect(result[0].description).toEqual(<>{mockQueryBarWithQuery.query} ); + + expect(result[0].title).toEqual(<>{i18n.QUERY_LABEL}); + expect(shallow(result[0].description as React.ReactElement).text()).toEqual( + mockQueryBarWithQuery.query + ); }); test('returns expected array of ListItems when "savedId" exists', () => { diff --git a/x-pack/plugins/security_solution/public/detections/components/rules/description_step/helpers.tsx b/x-pack/plugins/security_solution/public/detections/components/rules/description_step/helpers.tsx index 9ef1dd2bcb204..83413496c609d 100644 --- a/x-pack/plugins/security_solution/public/detections/components/rules/description_step/helpers.tsx +++ b/x-pack/plugins/security_solution/public/detections/components/rules/description_step/helpers.tsx @@ -51,6 +51,10 @@ const EuiBadgeWrap = (styled(EuiBadge)` } ` as unknown) as typeof EuiBadge; +const Query = styled.div` + white-space: pre-wrap; +`; + export const buildQueryBarDescription = ({ field, filters, @@ -92,8 +96,8 @@ export const buildQueryBarDescription = ({ items = [ ...items, { - title: <>{queryLabel ?? i18n.QUERY_LABEL} , - description: <>{query} , + title: <>{queryLabel ?? i18n.QUERY_LABEL}, + description: {query}, }, ]; } diff --git a/x-pack/plugins/security_solution/public/detections/components/rules/description_step/index.test.tsx b/x-pack/plugins/security_solution/public/detections/components/rules/description_step/index.test.tsx index 8179e5865e4ef..d881d05edbb0c 100644 --- a/x-pack/plugins/security_solution/public/detections/components/rules/description_step/index.test.tsx +++ b/x-pack/plugins/security_solution/public/detections/components/rules/description_step/index.test.tsx @@ -315,8 +315,10 @@ describe('description_step', () => { mockFilterManager ); - expect(result[0].title).toEqual(<>{i18n.QUERY_LABEL} ); - expect(result[0].description).toEqual(<>{mockQueryBar.queryBar.query.query} ); + expect(result[0].title).toEqual(<>{i18n.QUERY_LABEL}); + expect(shallow(result[0].description as React.ReactElement).text()).toEqual( + mockQueryBar.queryBar.query.query + ); }); }); diff --git a/x-pack/plugins/security_solution/public/detections/components/rules/eql_query_bar/eql_query_bar.tsx b/x-pack/plugins/security_solution/public/detections/components/rules/eql_query_bar/eql_query_bar.tsx index f7ee5be18154c..a04c179e6d162 100644 --- a/x-pack/plugins/security_solution/public/detections/components/rules/eql_query_bar/eql_query_bar.tsx +++ b/x-pack/plugins/security_solution/public/detections/components/rules/eql_query_bar/eql_query_bar.tsx @@ -32,7 +32,7 @@ export interface EqlQueryBarProps { export const EqlQueryBar: FC = ({ dataTestSubj, field, idAria }) => { const { addError } = useAppToasts(); const [errorMessages, setErrorMessages] = useState([]); - const { setValue } = field; + const { isValidating, setValue } = field; const { isValid, message, messages, error } = getValidationResults(field); const fieldValue = field.value.query.query as string; @@ -81,7 +81,7 @@ export const EqlQueryBar: FC = ({ dataTestSubj, field, idAria value={fieldValue} onChange={handleChange} /> - + ); diff --git a/x-pack/plugins/security_solution/public/detections/components/rules/eql_query_bar/footer.tsx b/x-pack/plugins/security_solution/public/detections/components/rules/eql_query_bar/footer.tsx index 19bab26f8aa58..7c0ddd6d8b3cc 100644 --- a/x-pack/plugins/security_solution/public/detections/components/rules/eql_query_bar/footer.tsx +++ b/x-pack/plugins/security_solution/public/detections/components/rules/eql_query_bar/footer.tsx @@ -6,7 +6,7 @@ import React, { FC } from 'react'; import styled from 'styled-components'; -import { EuiFlexGroup, EuiFlexItem, EuiPanel } from '@elastic/eui'; +import { EuiFlexGroup, EuiFlexItem, EuiLoadingSpinner, EuiPanel } from '@elastic/eui'; import * as i18n from './translations'; import { ErrorsPopover } from './errors_popover'; @@ -14,25 +14,31 @@ import { EqlOverviewLink } from './eql_overview_link'; export interface Props { errors: string[]; + isLoading?: boolean; } const Container = styled(EuiPanel)` border-radius: 0; background: ${({ theme }) => theme.eui.euiPageBackgroundColor}; - padding: ${({ theme }) => theme.eui.euiSizeXS}; + padding: ${({ theme }) => theme.eui.euiSizeXS} ${({ theme }) => theme.eui.euiSizeS}; `; const FlexGroup = styled(EuiFlexGroup)` min-height: ${({ theme }) => theme.eui.euiSizeXL}; `; -export const EqlQueryBarFooter: FC = ({ errors }) => ( +const Spinner = styled(EuiLoadingSpinner)` + margin: 0 ${({ theme }) => theme.eui.euiSizeS}; +`; + +export const EqlQueryBarFooter: FC = ({ errors, isLoading }) => ( {errors.length > 0 && ( )} + {isLoading && }