[Alerting UI][License] Disable alert types in UI when the license doesn't support it. #85496

YulNaumenko · 2020-12-10T00:02:06Z

Current PR covers the licensing UI for the alert types.

In the Alert flyouts (Create and Edit), alert types disabled by the license restriction is displayed similarly as unavailable action types in the Connector flyout:

The sort order is a bit more complicated then for action types, because we have groups of alert types by the solution.
Current PR implements sort order by keeping the solutions alphabet order if the solution contains at least one available alert type and if all alert types under solution is disabled by the license restriction it will be sorted after the solutions with available alert types.
Then inside the solution group we sort solution items inside by the license availability and then alphabetically by alert type name.
Alerts, which has alert type belongs to the higher order license displayed disabled with explanation icon in the alerts list in Managment UI, similarly as it designed for connectors list.

User still have a possibility to delete alerts with the higher order alert type license (same as for the action types).
There is a possibility to navigate to the alert details page. Should we support this?

All messages for licensing tooltips are corresponding to the action types design.

@arisonl, @alexfrancoeur please let me know if you have any objections on the current licensing implementation.

Resolve #85496

…sn't support it.

elasticmachine · 2020-12-10T00:03:17Z

Pinging @elastic/kibana-alerting-services (Team:Alerting Services)

gmmorris · 2020-12-10T10:30:25Z

x-pack/plugins/triggers_actions_ui/public/application/lib/alert_type_compare.ts

+  // .sort(([a], [b]) =>
+  // solutions ? solutions.get(a)!.localeCompare(solutions.get(b)!) : a.localeCompare(b)
+  // )


gmmorris

Works well, but I'm a little worried about the maintainability of alert_type_compare.ts.

I'd appreciate if we can rethink some of the naming in there and add unit tests around it.

gmmorris · 2020-12-10T10:59:13Z

x-pack/plugins/triggers_actions_ui/public/application/lib/alert_type_compare.ts

+      alertTypeItem: AlertTypeModel;
+    }>
+  ],
+  solutions: Map<string, string> | undefined


It isn't quite clear what a solution is.
Is there a more descriptive name we can use here?

Additionally, can we add some unit tests around this please?

gmmorris · 2020-12-10T11:05:50Z

x-pack/plugins/triggers_actions_ui/public/application/lib/alert_type_compare.ts

+  const alertTypeItemsA = a[1].find((alertTypeItem) => alertTypeItem.checkEnabledResult.isEnabled);
+  const alertTypeItemsB = b[1].find((alertTypeItem) => alertTypeItem.checkEnabledResult.isEnabled);
+
+  if (!!alertTypeItemsA && !alertTypeItemsB) {
+    return -1;
+  }
+  if (!alertTypeItemsA && !!alertTypeItemsB) {
+    return 1;
+  }


The mix of !! and ! makes this a little hard to follow... can we make this a little clearer?
A couple of suggestions that might help:

Can we rename A and B to something more descriptive? Perhaps AlertTypesListA? It's common to use Left and Right rather than A and B. So perhaps LeftAlertTypesList and RightAlertTypesList?

change alertTypeItemsA to a more descriptive name perhaps? Maybe enabledAlertTypeInList?

perhaps, to avoid the mix of !! with !, we do something like:

const hasEnabledAlertTypeInListA = a[1].find((alertTypeItem) => alertTypeItem.checkEnabledResult.isEnabled) != undefined;

Something like that?

ymao1

LGTM after adding the tests that @gmmorris suggested. Works as expected.

mdefazio

Looks good. A few minor things:

We should have a way for the user to solve the error if they are reaching the detail view. I think if the phrase 'Please upgrade your license' is linked to the appropriate place then that solves it.
Ideally, I think we would show a modal warning when reaching the detail view that provides the reason for the error with the option to upgrade or delete the alert. Alternatively, we provide the context / fix for the error on the list view and not allow them to get to the detail view. As discussed, we can handle in a follow-up PR.

mdefazio

Added review comment about the tooltip/icon as discussed—Let's just wrap the name with the tooltip and remove the info icon.

...gins/triggers_actions_ui/public/application/sections/alerts_list/components/alerts_list.scss

YulNaumenko · 2020-12-10T19:02:58Z

Looks good. A few minor things:

We should have a way for the user to solve the error if they are reaching the detail view. I think if the phrase 'Please upgrade your license' is linked to the appropriate place then that solves it.

Ideally, I think we would show a modal warning when reaching the detail view that provides the reason for the error with the option to upgrade or delete the alert. Alternatively, we provide the context / fix for the error on the list view and not allow them to get to the detail view. As discussed, we can handle in a follow-up PR.

I opened follow up issue on this. Currently not able to add a link to the error message text 'Please upgrade your license'. But instead of this I've added button Manage license, for the licensing Error reason, similar as we have on the connector flyout.

mdefazio

Just a small tweak to the button style...

...s/triggers_actions_ui/public/application/sections/alert_details/components/alert_details.tsx

myasonik

In the PR checklist, we have a couple a11y items in there, could you please verify them?

Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)

myasonik · 2020-12-10T19:23:07Z

x-pack/plugins/triggers_actions_ui/public/application/sections/alert_form/alert_form.tsx

+            labelAppend={
+              hasDisabledByLicenseAlertTypes && (
+                <EuiTitle size="xxs">
+                  <h5>


Not sure how to get all the way to this screen in the UI but an h5 strikes me as an odd choice. Do we already have an h1 - h4 on this page?

mdefazio

LGTM!

kindsun

Geo alerts changes lgtm!

code review
tested locally in chrome

kibanamachine · 2020-12-10T21:38:28Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: e3555dc

Metrics [docs]

‼️ ERROR: no builds found for mergeBase sha [93614af]

History

💔 Build #93634 failed f204384
💚 Build #93626 succeeded 291a830
💚 Build #93369 succeeded 2b6082e
💔 Build #93331 failed 60dfef8

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

* [Alerts][License] Define minimum license required for each alert type (#84997) * Define minimum license required for each alert type * fixed typechecks * fixed tests * fixed tests * fixed due to comments * fixed due to comments * removed file * removed casting to LicenseType * [Alerts][License] Add license checks to alerts HTTP APIs and execution (#85223) * [Alerts][License] Add license checks to alerts HTTP APIs and execution * fixed typechecks * resolved conflicts * resolved conflicts * added router tests * fixed typechecks * added license check support for alert task running * fixed typechecks * added integration tests * fixed due to comments * fixed due to comments * fixed tests * fixed typechecks * [Alerting UI][License] Disable alert types in UI when the license doesn't support it. (#85496) * [Alerting UI][License] Disable alert types in UI when the license doesn't support it. * fixed typechecks * added licensing for alert list and details page * fixed multy select menu * fixed due to comments * fixed due to comments * fixed due to comments * fixed typechecks * fixed license error message * fixed license error message * fixed typechecks * fixed license error message Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

[Alerting UI][License] Disable alert types in UI when the license doe…

d03cf87

…sn't support it.

YulNaumenko requested a review from a team as a code owner December 10, 2020 00:02

YulNaumenko changed the title ~~[Alerting UI][License] Disable alert types in UI when the license doe…~~ [Alerting UI][License] Disable alert types in UI when the license doesn't support it. Dec 10, 2020

YulNaumenko self-assigned this Dec 10, 2020

YulNaumenko added Feature:Alerting Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) labels Dec 10, 2020

YulNaumenko added v7.11.0 v8.0.0 release_note:skip Skip the PR/issue when compiling release notes labels Dec 10, 2020

Merge remote-tracking branch upstream/master

60dfef8

YulNaumenko linked an issue Dec 10, 2020 that may be closed by this pull request

[Alerting UI][License] Disable alert types in UI when the license doesn't support it. #85282

Closed

YulNaumenko added 2 commits December 9, 2020 21:12

fixed typechecks

24dfe8b

added licensing for alert list and details page

1bb9310

YulNaumenko requested a review from a team as a code owner December 10, 2020 06:41

YulNaumenko requested a review from kindsun December 10, 2020 06:44

fixed multy select menu

2b6082e

gmmorris reviewed Dec 10, 2020

View reviewed changes

gmmorris approved these changes Dec 10, 2020

View reviewed changes

ymao1 approved these changes Dec 10, 2020

View reviewed changes

mdefazio requested changes Dec 10, 2020

View reviewed changes

...gins/triggers_actions_ui/public/application/sections/alerts_list/components/alerts_list.scss Outdated Show resolved Hide resolved

fixed due to comments

291a830

YulNaumenko requested a review from mdefazio December 10, 2020 18:42

fixed due to comments

f204384

mdefazio requested changes Dec 10, 2020

View reviewed changes

...s/triggers_actions_ui/public/application/sections/alert_details/components/alert_details.tsx Outdated Show resolved Hide resolved

myasonik reviewed Dec 10, 2020

View reviewed changes

mdefazio approved these changes Dec 10, 2020

View reviewed changes

fixed due to comments

e3555dc

kindsun approved these changes Dec 10, 2020

View reviewed changes

YulNaumenko merged commit 3921475 into elastic:alerts/license-checks Dec 10, 2020

YulNaumenko mentioned this pull request Dec 10, 2020

License checks for alerts plugin #85649

Merged

mikecote mentioned this pull request Dec 14, 2020

[Alerting UI][License] Disable alert types in UI when the license doesn't support it. #85282

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Alerting UI][License] Disable alert types in UI when the license doesn't support it. #85496

[Alerting UI][License] Disable alert types in UI when the license doesn't support it. #85496

YulNaumenko commented Dec 10, 2020 •

edited

Loading

elasticmachine commented Dec 10, 2020

gmmorris Dec 10, 2020

gmmorris left a comment

gmmorris Dec 10, 2020

gmmorris Dec 10, 2020

ymao1 left a comment

mdefazio left a comment

mdefazio left a comment

YulNaumenko commented Dec 10, 2020 •

edited

Loading

mdefazio left a comment

myasonik left a comment •

edited by YulNaumenko

Loading

myasonik Dec 10, 2020

YulNaumenko Dec 10, 2020

mdefazio left a comment

kindsun left a comment

kibanamachine commented Dec 10, 2020

[Alerting UI][License] Disable alert types in UI when the license doesn't support it. #85496

[Alerting UI][License] Disable alert types in UI when the license doesn't support it. #85496

Conversation

YulNaumenko commented Dec 10, 2020 • edited Loading

elasticmachine commented Dec 10, 2020

gmmorris Dec 10, 2020

Choose a reason for hiding this comment

gmmorris left a comment

Choose a reason for hiding this comment

gmmorris Dec 10, 2020

Choose a reason for hiding this comment

gmmorris Dec 10, 2020

Choose a reason for hiding this comment

ymao1 left a comment

Choose a reason for hiding this comment

mdefazio left a comment

Choose a reason for hiding this comment

mdefazio left a comment

Choose a reason for hiding this comment

YulNaumenko commented Dec 10, 2020 • edited Loading

mdefazio left a comment

Choose a reason for hiding this comment

myasonik left a comment • edited by YulNaumenko Loading

Choose a reason for hiding this comment

myasonik Dec 10, 2020

Choose a reason for hiding this comment

YulNaumenko Dec 10, 2020

Choose a reason for hiding this comment

mdefazio left a comment

Choose a reason for hiding this comment

kindsun left a comment

Choose a reason for hiding this comment

kibanamachine commented Dec 10, 2020

💚 Build Succeeded

Metrics [docs]

History

YulNaumenko commented Dec 10, 2020 •

edited

Loading

YulNaumenko commented Dec 10, 2020 •

edited

Loading

myasonik left a comment •

edited by YulNaumenko

Loading