[SIEM] Add license check to ML Rule form by rylnd · Pull Request #60691 · elastic/kibana

rylnd · 2020-03-19T21:16:33Z

Summary

If the user doesn't have a platinum or trial license, we disable the ML card and change its text, with a link to the subscriptions marketing page
Adds aria-describedby and data-test-subj to new input fields

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
~~Documentation was added for features that require explanation or tutorials~~
Unit or functional tests were updated or added to match the most common scenarios
~~This was checked for keyboard-only and screenreader accessibility~~
~~This renders correctly on smaller devices using a responsive layout. (You can test this in your browser~~
~~This was checked for cross-browser compatibility, including a check against IE11~~

For maintainers

This was checked for breaking API changes and was labeled appropriately

If they don't have a Platinum or Trial license, then we disable the ML Card and provide them a link to the subscriptions marketing page.

This is already passed as isLoading

elasticmachine · 2020-03-19T21:16:35Z

Pinging @elastic/siem (Team:SIEM)

Conflicts: x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/components/select_rule_type/index.tsx x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/components/step_define_rule/index.tsx

FrankHassanabad · 2020-03-20T22:46:20Z

Optional, I would update the checklist for the PR.

FrankHassanabad · 2020-03-20T23:05:49Z

...egacy/plugins/siem/public/pages/detection_engine/rules/components/select_rule_type/index.tsx

-            description={license ? i18n.ML_TYPE_DESCRIPTION : i18n.ML_TYPE_DISABLED_DESCRIPTION}
-            isDisabled={!license}
+            description={<MlCardDescription hasValidLicense={hasValidLicense} />}
            icon={<EuiIcon size="l" type="machineLearningApp" />}


I played with adding:

isDisabled={isReadOnly || !hasValidLicense}

To see what it looked like if the whole thing was diasbled.

What you have now:

vs if the whole thing was disabled:

Up to you and design on which is better but I prefer the whole thing being disabled.

I ended up going with the full-disabling, as it is more visually striking and a better CTA

FrankHassanabad · 2020-03-20T23:12:22Z

...egacy/plugins/siem/public/pages/detection_engine/rules/components/select_rule_type/index.tsx

+export const SelectRuleType: React.FC<SelectRuleTypeProps> = ({
+  describedByIds = [],
+  field,
+  hasValidLicense = false,


You have these two taking values when they are defaulted by the SelectRuleTypeProps shows they cannot have defaults and are required?

If they are required I would just drop the defaulting or change the types to be ? variants where they are defaults.

...egacy/plugins/siem/public/pages/detection_engine/rules/components/select_rule_type/index.tsx

FrankHassanabad

Few questions about the optional parameters and one UI question but I checked this out and tested locally by flipping flags and this looks good as is.

LGTM, don't want to hold this up over small stuff.

If we're editing an existing rule, or if the user has an insufficient license, we disable both the card and its selectability. This is more visually striking, and a more obvious CTA.

kibanamachine · 2020-03-23T15:58:10Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 41217c7

History

💚 Build #35151 succeeded be56dc3
💚 Build #34975 succeeded 55df0d2
💚 Build #34711 succeeded 4621085

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

* master: [Uptime] Skip failing location test temporarily (elastic#60938) [ML] Disabling datafeed editing when job is running (elastic#60751) Adding `authc.invalidateAPIKeyAsInternalUser` (elastic#60717) [SIEM] Add license check to ML Rule form (elastic#60691) Adding `authc.grantAPIKeyAsInternalUser` (elastic#60423) Support Histogram Data Type (elastic#59387) [Upgrade Assistant] Fix edge case where reindex op can falsely be seen as stale (elastic#60770) [SIEM] [Cases] Update case icons (elastic#60812) [TSVB] Fix percentiles band mode (elastic#60741)

* Gate ML Rules behind a license check If they don't have a Platinum or Trial license, then we disable the ML Card and provide them a link to the subscriptions marketing page. * Add aria-describedby for new ML input fields * Add data-test-subj to new ML input fields * Remove unused prop This is already passed as isLoading * Fix capitalization on translation id * Declare defaulted props as optional * Gray out entire ML card when ML Rules are disabled If we're editing an existing rule, or if the user has an insufficient license, we disable both the card and its selectability. This is more visually striking, and a more obvious CTA.

* master: (26 commits) [Alerting] Fixes flaky test in Alert Instances Details page (elastic#60893) cleanup visualizations api (elastic#59958) Inline timezoneProvider function, remove ui/vis/lib/timezone (elastic#60475) [SIEM] Adds 'Open one signal' Cypress test (elastic#60484) [UA] Upgrade assistant migration meta data can become stale (elastic#60789) [Metrics Alerts] Remove metric field from doc count on backend (elastic#60679) [Uptime] Skip failing location test temporarily (elastic#60938) [ML] Disabling datafeed editing when job is running (elastic#60751) Adding `authc.invalidateAPIKeyAsInternalUser` (elastic#60717) [SIEM] Add license check to ML Rule form (elastic#60691) Adding `authc.grantAPIKeyAsInternalUser` (elastic#60423) Support Histogram Data Type (elastic#59387) [Upgrade Assistant] Fix edge case where reindex op can falsely be seen as stale (elastic#60770) [SIEM] [Cases] Update case icons (elastic#60812) [TSVB] Fix percentiles band mode (elastic#60741) Fix formatter on range aggregation (elastic#58651) Goodbye, legacy data plugin 👋 (elastic#60449) [Metrics UI] Alerting for metrics explorer and inventory (elastic#58779) [Remote clustersadopt changes to remote info API (elastic#60795) Only run xpack siem cypress in PRs when there are siem changes (elastic#60661) ...

rylnd added 4 commits March 19, 2020 15:25

Gate ML Rules behind a license check

02ff875

If they don't have a Platinum or Trial license, then we disable the ML Card and provide them a link to the subscriptions marketing page.

Add aria-describedby for new ML input fields

95ce642

Add data-test-subj to new ML input fields

42eaba7

Remove unused prop

004ae91

This is already passed as isLoading

rylnd added Team:SIEM v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.7.0 labels Mar 19, 2020

rylnd self-assigned this Mar 19, 2020

rylnd requested review from FrankHassanabad and patrykkopycinski March 19, 2020 21:16

Fix capitalization on translation id

4621085

rylnd mentioned this pull request Mar 19, 2020

[SIEM] Feature: ML Rules #60301

Closed

11 tasks

Merge branch 'master' into ml_rule_license_check

55df0d2

Conflicts: x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/components/select_rule_type/index.tsx x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/components/step_define_rule/index.tsx

rylnd requested a review from a team as a code owner March 20, 2020 16:32

FrankHassanabad reviewed Mar 20, 2020

View reviewed changes

...egacy/plugins/siem/public/pages/detection_engine/rules/components/select_rule_type/index.tsx Outdated Show resolved Hide resolved

FrankHassanabad approved these changes Mar 20, 2020

View reviewed changes

rylnd added 4 commits March 20, 2020 21:36

Declare defaulted props as optional

fac289c

Gray out entire ML card when ML Rules are disabled

1b6b460

If we're editing an existing rule, or if the user has an insufficient license, we disable both the card and its selectability. This is more visually striking, and a more obvious CTA.

Merge branch 'master' into ml_rule_license_check

be56dc3

Merge branch 'master' into ml_rule_license_check

41217c7

rylnd merged commit 21e8cea into elastic:master Mar 23, 2020

rylnd deleted the ml_rule_license_check branch March 23, 2020 16:10

rylnd mentioned this pull request Mar 23, 2020

[7.x] [SIEM] Add license check to ML Rule form (#60691) #60928

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SIEM] Add license check to ML Rule form#60691

[SIEM] Add license check to ML Rule form#60691
rylnd merged 10 commits intoelastic:masterfrom
rylnd:ml_rule_license_check

rylnd commented Mar 19, 2020 •

edited

Loading

Uh oh!

elasticmachine commented Mar 19, 2020

Uh oh!

FrankHassanabad commented Mar 20, 2020

Uh oh!

FrankHassanabad Mar 20, 2020

Uh oh!

rylnd Mar 23, 2020

Uh oh!

FrankHassanabad Mar 20, 2020

Uh oh!

Uh oh!

FrankHassanabad left a comment

Uh oh!

kibanamachine commented Mar 23, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

rylnd commented Mar 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

elasticmachine commented Mar 19, 2020

Uh oh!

FrankHassanabad commented Mar 20, 2020

Uh oh!

FrankHassanabad Mar 20, 2020

Choose a reason for hiding this comment

Uh oh!

rylnd Mar 23, 2020

Choose a reason for hiding this comment

Uh oh!

FrankHassanabad Mar 20, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

FrankHassanabad left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Mar 23, 2020

💚 Build Succeeded

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rylnd commented Mar 19, 2020 •

edited

Loading