[Uptime] Disable 'Create Rule' button when user doesn't have uptime write permissions [#118404] #120379

lucasfcosta · 2021-12-03T16:59:31Z

Summary

This PR closes #118404.

Before this PR, users would be able to open the flyout to create an alert and would end-up seeing an error toast when they tried to save it. This flyout could be opened both from within the the top menu, and the monitor view itself, in the popover for when users have enabled anomaly detection.

This PR adds commits to:

Disable the create alert button in the menu on the top when the user doesn't have permissions to write to Uptime. It will also display a helpful tooltip.
Disable the create alert button within the monitor view itself, in the popover for when users have enabled anomaly detection. This disabled button will contain a tooltip too.

Both changes above can be seen in the gifs below (in the same order):

Furthermore, after this PR users who don't have access to uptime will not see the flyover for creating alerts popup automatically because they can't use it anyway.

How to this PR:

Test Case 1: Top Menu - Alert Disabled

Create a role which has All permissions to use Machine Learning capabilities
Create a new user and assign it the role from step (1) and the viewer role
Login with the user from step (2) and ensure that you cannot use the top menu to create an alert (the button should be disabled) and that a tooltip appears.

Test Case 2: Top Menu - Alert Enabled

Create a monitor and make sure it has run
As a user with write access to Uptime, try use the button on the top right corner to create an alert. The button should be enabled and the alert creation should succeed.

Test Case 3: Monitor View - Alert Disabled

Create a monitor and make sure it has run
Create a role which has All permissions to use Machine Learning capabilities
Create a new user and assign it the role from step (2) and the viewer role
Login with the user from step (2) and access the details page for the monitor from step (1)
Try to create a new anomaly detection rule and ensure the alert flyout does not pop automatically.
Ensure that when clicking the anomaly detection button you cannot create an alert (the button should be disabled) and that a tooltip appears.

Test Case 4: Monitor View - Alert Enabled

Create a monitor and make sure it has run
As a user with write access to Uptime, try to create a new anomaly detection rule and ensure the alert flyout does pop automatically.
Ensure that the button to create an alert in the "Anomaly detection" is enabled and that you can create an alert through it

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
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)
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

Release note

Disables the button to create alerts in Uptime when users do not have permissions to do so.

elasticmachine · 2021-12-06T15:57:13Z

Pinging @elastic/uptime (Team:uptime)

EamonnTP

Looks good. Just a couple of minor copy suggestions.

EamonnTP · 2021-12-07T09:33:16Z

x-pack/plugins/uptime/public/components/monitor/ml/ml_manage_job.test.tsx

+      userEvent.hover(getByText(labels.ENABLE_ANOMALY_ALERT));
+
+      expect(
+        await findByText('You need write access to Uptime to create anomaly alerts.')


Suggested change

await findByText('You need write access to Uptime to create anomaly alerts.')

await findByText('You need read-write access to Uptime to create anomaly rules.')

Hmmm, actually having thought again about this:

I was thinking that perhaps the change from alerts to rules would actually have a different meaning from what the button does.

In that PR we're disabling the ability to create alerts themselves, as the rule is already created when users get to see that tooltip.

I will keep "alerts" as we discussed on Slack, thank you @EamonnTP!

EamonnTP · 2021-12-07T09:33:36Z

x-pack/plugins/uptime/public/components/monitor/ml/translations.tsx

+export const ENABLE_ANOMALY_NO_PERMISSIONS_TOOLTIP = i18n.translate(
+  'xpack.uptime.ml.enableAnomalyDetectionPanel.noPermissionsTooltip',
+  {
+    defaultMessage: 'You need write access to Uptime to create anomaly alerts.',


Suggested change

defaultMessage: 'You need write access to Uptime to create anomaly alerts.',

defaultMessage: 'You need read-write access to Uptime to create anomaly rules.',

EamonnTP · 2021-12-07T09:34:18Z

x-pack/plugins/uptime/public/components/overview/alerts/toggle_alert_flyout_button.test.tsx

+      userEvent.click(getByText('Alerts and rules'));
+      userEvent.hover(getByText(ToggleFlyoutTranslations.openAlertContextPanelLabel));
+      await expect(() =>
+        findByText('Creating alerts in this application requires write access to Uptime')


Suggested change

findByText('Creating alerts in this application requires write access to Uptime')

findByText('You need read-write access to Uptime to create rules.')

I've updated this to:

You need read-write access to Uptime to create alerts in this app.

Otherwise users could be confused as to why they'd need write access to Uptime to create alerts in general. I thought it would be good to be specific and clear that this only applies for alerts in this application.

EamonnTP · 2021-12-07T09:34:44Z

x-pack/plugins/uptime/public/components/overview/alerts/toggle_alert_flyout_button.test.tsx

+      userEvent.click(getByText('Alerts and rules'));
+      userEvent.hover(getByText(ToggleFlyoutTranslations.openAlertContextPanelLabel));
+      expect(
+        await findByText('Creating alerts in this application requires write access to Uptime')


Suggested change

await findByText('Creating alerts in this application requires write access to Uptime')

await findByText('You need read-write access to Uptime to create rules.')

EamonnTP · 2021-12-07T09:35:09Z

x-pack/plugins/uptime/public/components/overview/alerts/toggle_alert_flyout_button.tsx

+const noWritePermissionsTooltipContent = i18n.translate(
+  'xpack.uptime.alertDropdown.noWritePermissions',
+  {
+    defaultMessage: 'Creating alerts in this application requires write access to Uptime',


Suggested change

defaultMessage: 'Creating alerts in this application requires write access to Uptime',

defaultMessage: 'You need read-write access to Uptime to create rules.',

shahzad31 · 2021-12-07T11:43:14Z

x-pack/plugins/uptime/public/components/monitor/ml/__snapshots__/ml_manage_job.test.tsx.snap

@@ -1,5 +1,128 @@
 // Jest Snapshot v1, https://goo.gl/fbAQLP

+exports[`Manage ML Job rendering renders without errors 1`] = `


Can we please avoid snapshot tests, we discourage these in uptime and prefer using RTL.

Great shout, these were actually not supposed to be here as I deleted the old snapshot tests and started using RTL exactly because I also wanted to avoid snapshot tests. I'll clean up.

…rite permissions [elastic#118404] Before this commit, users would be able to open the flyout to create an alert and would end-up seeing an error toast when they tried to save it. This commit will now disable the create alert button when the user doesn't have permissions to write to Uptime. It will also display a helpful tooltip.

…ime [elastic#118404] This commit causes users not to be able to use the "Enable Anomaly Alert" button within the popover in the monitors screen. That button will now be disabled and contain an informative tooltip whenever users don't have permissions to write to Uptime. We've chosen to take this approach so that we don't have to modify the component which deals with the alert creation, which belongs to another team and that we plan on eventually replacing. Furthermore, this pattern is already used in the logs app.

liciavale · 2021-12-07T15:53:30Z

Is there any doc we could link? Like a Read more or something to explain how to get access to Uptime or what to do in case I want to enable this? The tooltip is useful to give context but I wonder if we should link it to somewhere else to explain what to do next.

lucasfcosta · 2021-12-07T17:41:01Z

@liciavale That could be a great idea, and I have attempted to add a link to the tooltip themselves. That attempt led me to discover why we don't do this anywhere else:

You can't click on the links in them because as soon as you hover out of the element the tooltip disappear
The link contrast requires style and doesn't work out of the box, so we need to style it with a ghost colour.

You can see these problems on the GIF below.

@liciavale, considering the above (only number 1 actually, given 2 is easy to do), are you okay not to have the link there?

An alternative would be to use a popover instead and make it pop using an event listener for the hover event.

However, considering we'll also have to have hoverable tooltips in the recorder (and that tooltips should be hoverable anyway as per this link), IMO it would be much cleaner an DRYer to make tooltips support hovering rather than using popovers as tooltips.

Resolution

As discussed with @liciavale, I've opened an issue in the EUI repo here: elastic/eui#5453 and for now we won't add the link to the tooltip.

liciavale

lgtm!

shahzad31 · 2021-12-09T12:17:56Z

x-pack/plugins/uptime/public/components/monitor/ml/ml_flyout_container.tsx

-        dispatch(setAlertFlyoutType(CLIENT_ALERT_TYPES.DURATION_ANOMALY));
-        dispatch(setAlertFlyoutVisible(true));
+
+        const hasUptimeWrite = core.services.application?.capabilities.uptime?.save ?? false;


i am not sure why we are checking uptime.save capability, shouldn't we check if user has alerting save capability?

alerting:save

Not really, unfortunately, that's not the capability used to save alerts. I did discuss this with Justin and he mentioned that we actually rely on save, not on alerting:save, which is, by the way, always true, even when users have only read permissions.

Given save permission is what is actually used, I was thinking that investigating why we don't use alerting:save could be a separate issue, what do you think?

justinkambic

LGTM - I took the time to repro this on main as well, seems to be working as designed.

Note:

Not sure if we consider this within the scope of these changes, but seems related. I can still open up the "add connector" dialogue from the settings page:

I'm not able to create anything once I'm in there, but we should simply disable the link as we have done for the rest of the elements on the page.

lucasfcosta · 2021-12-13T15:32:16Z

@elasticmachine merge upstream

…e-write

lucasfcosta · 2021-12-13T15:52:27Z

Having spoken about this PR with @justinkambic on Slack we agreed that the "create connector" button within the "settings" page will be out of scope for this PR because:

It deals with a different permission (actions and connectors, as seen below), not with the write permission for Uptime.
It would require a different copy, and thus a whole new review from design/docs/product, which would elongate the cycle time for this PR for longer than we need it given it's an unrelated permission that has a different problem.

Considering the above, the separate issue #121098 has been created.

kibana-ci · 2021-12-13T16:46:40Z

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`uptime`	683.3KB	684.1KB	+839.0B

History

💚 Build #11871 succeeded d7272ff
💔 Build #11852 failed a0b4853bb539f89c18a2c9e239c2adfb7ed7f350
💚 Build #11780 succeeded d7272ff
💔 Build #11723 failed 1d9e159a82f0f587f3f98e376a5ae1d4200a2b9a
💔 Build #11461 failed 8eae631046faf54227d4beeda4dd755904520679
💔 Build #11391 failed 8e574b8a8312300e4debb941e006d2e3fa50ac45

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

…rite permissions [elastic#118404] (elastic#120379) * [Uptime] Disable 'Create Rule' button when user doesn't have uptime write permissions [elastic#118404] Before this commit, users would be able to open the flyout to create an alert and would end-up seeing an error toast when they tried to save it. This commit will now disable the create alert button when the user doesn't have permissions to write to Uptime. It will also display a helpful tooltip. * [Uptime] Disable "Enable Anomaly Alert" when users can't write to uptime [elastic#118404] This commit causes users not to be able to use the "Enable Anomaly Alert" button within the popover in the monitors screen. That button will now be disabled and contain an informative tooltip whenever users don't have permissions to write to Uptime. We've chosen to take this approach so that we don't have to modify the component which deals with the alert creation, which belongs to another team and that we plan on eventually replacing. Furthermore, this pattern is already used in the logs app. Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

kibanamachine · 2021-12-14T09:29:21Z

💚 Backport successful

Status	Branch	Result
✅	8.0

This backport PR will be merged automatically after passing CI.

kibanamachine · 2021-12-16T09:34:59Z

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

…time write permissions [#118404] (#120379) (#121163) * [Uptime] Disable 'Create Rule' button when user doesn't have uptime write permissions [#118404] (#120379) * [Uptime] Disable 'Create Rule' button when user doesn't have uptime write permissions [#118404] Before this commit, users would be able to open the flyout to create an alert and would end-up seeing an error toast when they tried to save it. This commit will now disable the create alert button when the user doesn't have permissions to write to Uptime. It will also display a helpful tooltip. * [Uptime] Disable "Enable Anomaly Alert" when users can't write to uptime [#118404] This commit causes users not to be able to use the "Enable Anomaly Alert" button within the popover in the monitors screen. That button will now be disabled and contain an informative tooltip whenever users don't have permissions to write to Uptime. We've chosen to take this approach so that we don't have to modify the component which deals with the alert creation, which belongs to another team and that we plan on eventually replacing. Furthermore, this pattern is already used in the logs app. Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> * add missing useKibana hook to ML Flyout Container for detecting uptime write permissions Co-authored-by: Lucas F. da Costa <lucas@lucasfcosta.com> Co-authored-by: Lucas Fernandes da Costa <lucas.costa@elastic.co>

…rite permissions [elastic#118404] (elastic#120379) * [Uptime] Disable 'Create Rule' button when user doesn't have uptime write permissions [elastic#118404] Before this commit, users would be able to open the flyout to create an alert and would end-up seeing an error toast when they tried to save it. This commit will now disable the create alert button when the user doesn't have permissions to write to Uptime. It will also display a helpful tooltip. * [Uptime] Disable "Enable Anomaly Alert" when users can't write to uptime [elastic#118404] This commit causes users not to be able to use the "Enable Anomaly Alert" button within the popover in the monitors screen. That button will now be disabled and contain an informative tooltip whenever users don't have permissions to write to Uptime. We've chosen to take this approach so that we don't have to modify the component which deals with the alert creation, which belongs to another team and that we plan on eventually replacing. Furthermore, this pattern is already used in the logs app. Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

lucasfcosta force-pushed the disable-alerts-creation-button-without-uptime-write branch from 5e5dd16 to 8e574b8 Compare December 6, 2021 15:37

lucasfcosta added auto-backport Deprecated - use backport:version if exact versions are needed release_note:fix Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v8.0.0 v8.1.0 labels Dec 6, 2021

lucasfcosta marked this pull request as ready for review December 6, 2021 15:57

lucasfcosta requested a review from a team as a code owner December 6, 2021 15:57

lucasfcosta requested review from liciavale and drewpost December 6, 2021 17:32

lucasfcosta force-pushed the disable-alerts-creation-button-without-uptime-write branch from 8e574b8 to 8eae631 Compare December 6, 2021 17:41

EamonnTP reviewed Dec 7, 2021

View reviewed changes

shahzad31 reviewed Dec 7, 2021

View reviewed changes

lucasfcosta force-pushed the disable-alerts-creation-button-without-uptime-write branch from a039275 to 1d9e159 Compare December 7, 2021 11:50

lucasfcosta added 2 commits December 7, 2021 14:24

lucasfcosta force-pushed the disable-alerts-creation-button-without-uptime-write branch from 1d9e159 to d7272ff Compare December 7, 2021 14:25

lucasfcosta force-pushed the disable-alerts-creation-button-without-uptime-write branch from abe5784 to a0b4853 Compare December 7, 2021 16:52

lucasfcosta force-pushed the disable-alerts-creation-button-without-uptime-write branch from a0b4853 to d7272ff Compare December 7, 2021 17:48

lucasfcosta mentioned this pull request Dec 7, 2021

[EuiTooltip] [EuiContextMenu] Allowing tooltips to be hoverable elastic/eui#5453

Closed

liciavale reviewed Dec 7, 2021

View reviewed changes

liciavale approved these changes Dec 7, 2021

View reviewed changes

shahzad31 reviewed Dec 9, 2021

View reviewed changes

justinkambic approved these changes Dec 9, 2021

View reviewed changes

Merge branch 'main' into disable-alerts-creation-button-without-uptim…

45e8422

…e-write

lucasfcosta mentioned this pull request Dec 13, 2021

[Uptime] Users without permissions to create actions and connectors can still open the actions and connectors flyout in settings, but can't use it #121098

Closed

lucasfcosta merged commit afefefa into elastic:main Dec 14, 2021

lucasfcosta deleted the disable-alerts-creation-button-without-uptime-write branch December 14, 2021 09:25

kibanamachine mentioned this pull request Dec 14, 2021

[8.0] [Uptime] Disable 'Create Rule' button when user doesn't have uptime write permissions [#118404] (#120379) #121163

Merged

kibanamachine added backport missing Added to PRs automatically when the are determined to be missing a backport. and removed backport missing Added to PRs automatically when the are determined to be missing a backport. labels Dec 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Uptime] Disable 'Create Rule' button when user doesn't have uptime write permissions [#118404] #120379

[Uptime] Disable 'Create Rule' button when user doesn't have uptime write permissions [#118404] #120379

lucasfcosta commented Dec 3, 2021 •

edited

Loading

elasticmachine commented Dec 6, 2021

EamonnTP left a comment

EamonnTP Dec 7, 2021

lucasfcosta Dec 7, 2021

EamonnTP Dec 7, 2021

EamonnTP Dec 7, 2021

lucasfcosta Dec 7, 2021

EamonnTP Dec 7, 2021

EamonnTP Dec 7, 2021

shahzad31 Dec 7, 2021

lucasfcosta Dec 7, 2021 •

edited

Loading

liciavale commented Dec 7, 2021 •

edited

Loading

lucasfcosta commented Dec 7, 2021 •

edited

Loading

liciavale left a comment

shahzad31 Dec 9, 2021

lucasfcosta Dec 9, 2021 •

edited

Loading

justinkambic left a comment

lucasfcosta commented Dec 13, 2021

lucasfcosta commented Dec 13, 2021

kibana-ci commented Dec 13, 2021

kibanamachine commented Dec 14, 2021

kibanamachine commented Dec 16, 2021

	await findByText('You need write access to Uptime to create anomaly alerts.')
	await findByText('You need read-write access to Uptime to create anomaly rules.')

	defaultMessage: 'You need write access to Uptime to create anomaly alerts.',
	defaultMessage: 'You need read-write access to Uptime to create anomaly rules.',

	findByText('Creating alerts in this application requires write access to Uptime')
	findByText('You need read-write access to Uptime to create rules.')

	await findByText('Creating alerts in this application requires write access to Uptime')
	await findByText('You need read-write access to Uptime to create rules.')

	defaultMessage: 'Creating alerts in this application requires write access to Uptime',
	defaultMessage: 'You need read-write access to Uptime to create rules.',

		@@ -1,5 +1,128 @@
		// Jest Snapshot v1, https://goo.gl/fbAQLP

		exports[`Manage ML Job rendering renders without errors 1`] = `

[Uptime] Disable 'Create Rule' button when user doesn't have uptime write permissions [#118404] #120379

[Uptime] Disable 'Create Rule' button when user doesn't have uptime write permissions [#118404] #120379

Conversation

lucasfcosta commented Dec 3, 2021 • edited Loading

Summary

How to this PR:

Test Case 1: Top Menu - Alert Disabled

Test Case 2: Top Menu - Alert Enabled

Test Case 3: Monitor View - Alert Disabled

Test Case 4: Monitor View - Alert Enabled

Checklist

For maintainers

Release note

elasticmachine commented Dec 6, 2021

EamonnTP left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucasfcosta Dec 7, 2021 • edited Loading

Choose a reason for hiding this comment

liciavale commented Dec 7, 2021 • edited Loading

lucasfcosta commented Dec 7, 2021 • edited Loading

Resolution

liciavale left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucasfcosta Dec 9, 2021 • edited Loading

Choose a reason for hiding this comment

justinkambic left a comment

Choose a reason for hiding this comment

lucasfcosta commented Dec 13, 2021

lucasfcosta commented Dec 13, 2021

kibana-ci commented Dec 13, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

History

kibanamachine commented Dec 14, 2021

💚 Backport successful

kibanamachine commented Dec 16, 2021

lucasfcosta commented Dec 3, 2021 •

edited

Loading

lucasfcosta Dec 7, 2021 •

edited

Loading

liciavale commented Dec 7, 2021 •

edited

Loading

lucasfcosta commented Dec 7, 2021 •

edited

Loading

lucasfcosta Dec 9, 2021 •

edited

Loading