[Alerting] Showing confirmation modal on Alert Add/Edit when flyout closed without saving and changes made. #86370

ymao1 · 2020-12-17T20:03:23Z

Resolves #62824

Summary

In Alert Add/Edit flyouts, added check for changes when flyout is closed without saving and showing confirmation modal if user has made changes to the alert form.

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
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)

…as changed

ymao1 · 2020-12-17T20:19:08Z

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

  }, [alertTypeId]);

-  const closeFlyout = useCallback(() => {
-    setAlert(initialAlert);


We no longer need to reset the alert to an initial alert on close because with this PR we are removing the flyout on close completely instead of just hiding it.

ymao1 · 2020-12-17T20:25:46Z

x-pack/plugins/triggers_actions_ui/public/application/sections/alert_form/alert_has_changed.ts

+    ...(!!(alertTypeId && alertTypeId.length > 0) ? { alertTypeId } : {}),
+    ...(!!(throttle && throttle.length > 0) ? { throttle } : {}),
+  };
+}


I added tests for the scenarios I saw when manually testing this, namely:

the name field grabs focus when the add alert flyout opens so it starts out as undefined but is set to an empty string onBlur if the user doesn't enter anything

the alertTypeId field starts out undefined in the stack alerts flyout, but if you select an alert type and then X it out, it is set to null

the throttle field starts out undefined but if you set a throttle and then unset it, it is set to null

This tries to normalize these scenarios so that they can be compared.

…setAlertProperty

ymao1 · 2020-12-18T18:48:07Z

x-pack/plugins/apm/public/components/alerting/alerting_flyout/index.tsx

+    tags,
+    ...(alertName ? { name: alertName } : {}),
+  };
+


Setting initial values for alert name and tags via the initialValues prop of the AddAlert component instead of using setAlertProperty in the ServiceAlertTrigger component. This allows us to correctly check whether the user changes the alert name/tags and show the confirm on discard modal accordingly. Otherwise the default alert name/tag will always register as a change to the alert

elasticmachine · 2020-12-21T13:00:28Z

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

ymao1 · 2020-12-21T13:00:55Z

@elasticmachine merge upstream

elasticmachine · 2020-12-21T13:01:07Z

Pinging @elastic/apm-ui (Team:apm)

ymao1 · 2021-01-04T12:49:33Z

@elasticmachine merge upstream

…ing/confirm-before-close

ymao1 · 2021-01-04T16:00:16Z

@elasticmachine merge upstream

ymao1 · 2021-01-04T20:25:49Z

I found the problem, when you edited alert with adding more actions and then click Cancel -> Discard changes, the added actions wasn't removed (discarded)

@YulNaumenko Great catch! I pushed a fix for that issue.

smith

APM changes look good and work for me.

YulNaumenko

LGTM!

sorenlouv · 2021-01-02T01:29:05Z

x-pack/plugins/apm/public/components/alerting/alerting_flyout/index.tsx

+  const alertTypeName = alertType
+    ? ALERT_TYPES_CONFIG[alertType].name
+    : undefined;
+  const alertName = alertTypeName
+    ? serviceName
+      ? `${alertTypeName} | ${serviceName}`
+      : alertTypeName
+    : undefined;
+  const tags = ['apm'];
+  if (serviceName) {
+    tags.push(`service.name:${serviceName}`.toLowerCase());
+  }
+  const initialValues = {
+    tags,
+    ...(alertName ? { name: alertName } : {}),
+  };
+


Minor nit: can you extract this to a function?

Suggested change

const alertTypeName = alertType

? ALERT_TYPES_CONFIG[alertType].name

: undefined;

const alertName = alertTypeName

? serviceName

? `${alertTypeName} | ${serviceName}`

: alertTypeName

: undefined;

const tags = ['apm'];

if (serviceName) {

tags.push(`service.name:${serviceName}`.toLowerCase());

}

const initialValues = {

tags,

...(alertName ? { name: alertName } : {}),

};

const initialValues = getInitialValues(alertType, serviceName)

@sqren Done! You can see the change here

…ing/confirm-before-close

ymao1 · 2021-01-05T16:19:31Z

@elasticmachine merge upstream

…ao1/kibana into alerting/confirm-before-close

mikecote

Triggers and actions UI changes LGTM!

I'm curious if you connected with @gchaps to go over the copy?

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

x-pack/plugins/triggers_actions_ui/public/application/sections/alert_form/alert_has_changed.ts

…ing/confirm-before-close

ymao1 · 2021-01-07T12:59:23Z

@gchaps Can you review the Discard modal wording in the screenshot? Right now, it reads Discard changes to alert? and Once you discard your changes, there's no getting them back.. I took them from a similar Discard modal in the Kibana UI. Do they make sense in this context?

gchaps · 2021-01-07T21:00:47Z

I think we have to have the notion that these are "unsaved changes". Here is a suggestion:

Discard unsaved changes?

If you leave this form, your changes will be lost.

Cancel | Discard changes

Or if you want to keep the wording "alert"

Discard unsaved changes to alert?

If you leave this form, your changes will be lost.

Cancel | Discard changes

…ing/confirm-before-close

ymao1 · 2021-01-07T21:06:34Z

I think we have to have the notion that these are "unsaved changes". Here is a suggestion:

Discard unsaved changes?
If you leave this form, your changes will be lost.
Cancel | Discard changes

Or if you want to keep the wording "alert"

Discard unsaved changes to alert?
If you leave this form, your changes will be lost.
Cancel | Discard changes

@gchaps I think the title Discard unsaved changes to alert? sounds good! Since this is a flyout, does the text If you leave this form still make sense?

gchaps · 2021-01-11T18:51:41Z

@ymao1 Good call. How about

Discard unsaved changes to alert?

You can't recover unsaved changes.

Cancel | Discard changes

…ing/confirm-before-close

kibanamachine · 2021-01-11T21:07:21Z

💚 Build Succeeded

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

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`apm`	1734	1735	+1
`triggersActionsUi`	330	333	+3
total			+4

Async chunks

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

id	before	after	diff
`apm`	5.4MB	5.4MB	-404.0B
`triggersActionsUi`	1.5MB	1.6MB	+7.9KB
total			+7.5KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`triggersActionsUi`	165.1KB	165.1KB	+1.0B

History

💚 Build #97527 succeeded 8f74935
💛 Build #97101 was flaky 967277a
💚 Build #97007 succeeded 50b757c
💔 Build #96935 failed 05cac2b
💚 Build #96760 succeeded 6552c47

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

…losed without saving and changes made. (elastic#86370) * Adding hasChanged check and showing confirmation modal if something has changed * Showing confirmation always on close * Adding functional test * Setting name and tags for APM alerts using initial values instead of setAlertProperty * Checking for alert param changes separately * Checking for alert param changes separately * Fixing functional test * Resetting initial alert params on alert type change * Fixing duplicate import * Cloning edited alert * PR fixes * PR fixes * Updating modal wording Co-authored-by: Kibana Machine <[email protected]>

…losed without saving and changes made. (#86370) (#87927) * Adding hasChanged check and showing confirmation modal if something has changed * Showing confirmation always on close * Adding functional test * Setting name and tags for APM alerts using initial values instead of setAlertProperty * Checking for alert param changes separately * Checking for alert param changes separately * Fixing functional test * Resetting initial alert params on alert type change * Fixing duplicate import * Cloning edited alert * PR fixes * PR fixes * Updating modal wording Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>

ymao1 added 3 commits December 17, 2020 14:54

Adding hasChanged check and showing confirmation modal if something h…

2b76e2f

…as changed

Showing confirmation always on close

770c655

Adding functional test

3beb155

ymao1 commented Dec 17, 2020

View reviewed changes

ymao1 self-assigned this Dec 17, 2020

ymao1 added 4 commits December 18, 2020 09:28

Setting name and tags for APM alerts using initial values instead of …

08d4cf7

…setAlertProperty

Checking for alert param changes separately

5fded13

Checking for alert param changes separately

bcf01cf

Fixing functional test

a0a2606

ymao1 commented Dec 18, 2020

View reviewed changes

Resetting initial alert params on alert type change

3d3fefc

ymao1 added Feature:Alerting release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Platform ResponseOps team (formerly the Cases and Alerting teams) t// v7.12.0 v8.0.0 labels Dec 21, 2020

ymao1 marked this pull request as ready for review December 21, 2020 13:00

ymao1 requested review from a team as code owners December 21, 2020 13:00

Merge branch 'master' into alerting/confirm-before-close

1358b28

botelastic bot added the Team:APM - DEPRECATED Use Team:obs-ux-infra_services. label Dec 21, 2020

kibanamachine and others added 3 commits January 4, 2021 07:49

Merge branch 'master' into alerting/confirm-before-close

95e1830

Fixing duplicate import

1acb11c

Merge branch 'master' of https://github.com/elastic/kibana into alert…

3f39a1c

…ing/confirm-before-close

Merge branch 'master' into alerting/confirm-before-close

5eb0c6a

smith approved these changes Jan 5, 2021

View reviewed changes

YulNaumenko approved these changes Jan 5, 2021

View reviewed changes

sorenlouv reviewed Jan 5, 2021

View reviewed changes

ymao1 added 2 commits January 5, 2021 08:06

Merge branch 'master' of https://github.com/elastic/kibana into alert…

33cfe2a

…ing/confirm-before-close

PR fixes

05cac2b

kibanamachine and others added 3 commits January 5, 2021 11:19

Merge branch 'master' into alerting/confirm-before-close

50b757c

Fixing merge conflict

7abdb53

Merge branch 'alerting/confirm-before-close' of https://github.com/ym…

967277a

…ao1/kibana into alerting/confirm-before-close

sorenlouv approved these changes Jan 6, 2021

View reviewed changes

mikecote self-requested a review January 7, 2021 12:20

mikecote approved these changes Jan 7, 2021

View reviewed changes

ymao1 added 2 commits January 7, 2021 07:49

Merge branch 'master' of https://github.com/elastic/kibana into alert…

9ad41bb

…ing/confirm-before-close

PR fixes

8f74935

Merge branch 'master' of https://github.com/elastic/kibana into alert…

a2b565e

…ing/confirm-before-close

ymao1 added 2 commits January 11, 2021 13:52

Merge branch 'master' of https://github.com/elastic/kibana into alert…

c695ddc

…ing/confirm-before-close

Updating modal wording

cee5274

ymao1 merged commit 666af32 into elastic:master Jan 11, 2021

ymao1 mentioned this pull request Jan 11, 2021

[7.x] [Alerting] Showing confirmation modal on Alert Add/Edit when flyout closed without saving and changes made. (#86370) #87927

Merged

mikecote added release_note:enhancement and removed release_note:skip Skip the PR/issue when compiling release notes labels Feb 2, 2021

ymao1 deleted the alerting/confirm-before-close branch February 4, 2021 15:23

[Alerting] Showing confirmation modal on Alert Add/Edit when flyout closed without saving and changes made. #86370

[Alerting] Showing confirmation modal on Alert Add/Edit when flyout closed without saving and changes made. #86370

Uh oh!

Conversation

ymao1 commented Dec 17, 2020

Summary

Checklist

Uh oh!

ymao1 Dec 17, 2020

Choose a reason for hiding this comment

Uh oh!

ymao1 Dec 17, 2020

Choose a reason for hiding this comment

Uh oh!

ymao1 Dec 18, 2020

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Dec 21, 2020

Uh oh!

ymao1 commented Dec 21, 2020

Uh oh!

elasticmachine commented Dec 21, 2020

Uh oh!

ymao1 commented Jan 4, 2021

Uh oh!

ymao1 commented Jan 4, 2021

Uh oh!

ymao1 commented Jan 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smith left a comment

Choose a reason for hiding this comment

Uh oh!

YulNaumenko left a comment

Choose a reason for hiding this comment

Uh oh!

sorenlouv Jan 2, 2021

Choose a reason for hiding this comment

Uh oh!

ymao1 Jan 5, 2021

Choose a reason for hiding this comment

Uh oh!

ymao1 commented Jan 5, 2021

Uh oh!

mikecote left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ymao1 commented Jan 7, 2021

Uh oh!

gchaps commented Jan 7, 2021

Uh oh!

ymao1 commented Jan 7, 2021

Uh oh!

gchaps commented Jan 11, 2021

Uh oh!

kibanamachine commented Jan 11, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Page load bundle

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

ymao1 commented Jan 4, 2021 •

edited

Loading