Convert notify.warning calls to use toastNotifications #20767

cjcenizal · 2018-07-13T13:37:24Z

This PR introduces the following changes:

Removes routeBasedNotifier which wasn't being used.
Replaces the courierNotifier with toasts, and removes the ShardFailure and SearchTimeout classes because they're not longer used.
Uses toasts in region map, tile map, Graph, and vega visualizations.
Uses toasts to show index pattern warnings.
Uses toasts to show redirectWhenMissing warning.
Uses a variety of toasts and banners to show warnings in ML.
Uses toasts to show warnings in Monitoring.
Removes custom and warning types from notifier.

Index pattern

Region map

Courier

Redirect when missing

Graph

Monitoring

…ndex_pattern, redirect_when_missing, and graph.

elasticmachine · 2018-07-13T14:53:43Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

cjcenizal · 2018-07-17T01:41:21Z

@spalger Could you take a look at the code relating to routeBasedNotifier, courierNotifier, the ShardFailure and SearchTimeout errors, index pattern, redirectWhenMissing, and the notifier in general? This PR will aid in EUIfying the chrome, btw.

@elastic/kibana-design Could you please review the changes to region map, tile map, Graph, and vega visualizations?

@elastic/ml-ui Could you please review the changes to ML?

@bmcconaghy Could you please take a look at the change to Monitoring?

elasticmachine · 2018-07-17T02:52:13Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-07-17T04:56:48Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

jgowdyelastic · 2018-07-17T06:12:32Z

x-pack/plugins/ml/public/license/check_license.js

@@ -38,8 +38,16 @@ export function checkLicense(Private, kbnBaseUrl) {
    const message = features.message;
    const exists = _.find(notify._notifs, (item) => item.content === message);


This duplication check no longer works.
when jumping between pages we now get multiple banners.

Thanks! I just pushed a fix could you try it again?

elasticmachine · 2018-07-17T07:36:33Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

snide · 2018-07-17T13:37:14Z

Hey @cjcenizal this is great. Thanks for doing this. Love seeing so much code deleted. Apologies if those screens are not up to date, but could you remove the icon usage in the toasts? The warning color alone should be enough to pass on the status of the alert.

cchaos · 2018-07-17T15:11:19Z

Just looking at the screenshots, there seems to be some inconsistencies with period usage. I think we decided to only use periods if there are more than 2 sentences. Also for the last example in the graph app, can you link the "index pattern" text to the index pattern management screen?

spalger

Checked out the parts you asked me to review, LGTM

Thanks for doing this!!

elasticmachine · 2018-07-18T01:28:10Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-07-18T04:28:40Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

timroes · 2018-07-18T08:01:09Z

src/ui/public/courier/fetch/call_response_handlers.js

-        courierNotifier.warning(new ShardFailure(response));
+        toastNotifications.addWarning({
+          title: 'Shard failure',
+          text: `${response._shards.failed} of ${response._shards.total} shards failed`,


One of the most annoying things with shard failures currently is, that you don't get any insights into the actual failures, and that's very often a painpoint in discuss questions, since users need to check the actual browser request, and dig through that JSON. In the long run we definitely should build this notification in a way, that you can get a list of actual errors from the individual charts. Since you were anyway touching this place, I thought I mention that right now, in case you feel like we should do it within that PR. But I am also totally fine having that in a different PR (but I think we should take care of that rather sooner than later).

Great point Tim. I think this is outside of the scope for this PR, but I've updated #5957 with more information. Hopefully we can move on this sometime soon.

Yes, it would be good to add more info about the shard failure. When you do, it might be good to put this information in something other than a toast because a toast times out after a few seconds.

In the meantime, the toast can simply be one line:

3 of 6 shards failed

timroes

Visualization code looks good to me!

jgowdyelastic

ML changes LGTM

peteharverson

LGTM

pickypg

Monitoring update LGTM! Looks so nice

gchaps · 2018-07-19T22:02:48Z

src/core_plugins/region_map/public/region_map_visualization.js

+            title: `Couldn't show ${event.mismatches.length} ${event.mismatches.length > 1 ? 'results' : 'result'} on the map`,
+            text: `You can fix this by ensuring that each of these terms can be matched to a corresponding shape `
+              + `on that shape's join field:${event.mismatches.join(',')}`
+          });


Unable to show 3 results on map

Ensure that each term matches a shape on that shape's join field.

gchaps · 2018-07-19T22:06:41Z

src/ui/public/courier/fetch/call_response_handlers.js

+        toastNotifications.addWarning({
+          title: 'The data shown may be incomplete',
+          text: 'All or part of your request has timed out',
+        });


Use a title only:

Data might be incomplete because your request timed out

gchaps · 2018-07-19T22:14:53Z

src/ui/public/courier/fetch/call_response_handlers.js

-        courierNotifier.warning(new ShardFailure(response));
+        toastNotifications.addWarning({
+          title: 'Shard failure',
+          text: `${response._shards.failed} of ${response._shards.total} shards failed`,


Yes, it would be good to add more info about the shard failure. When you do, it might be good to put this information in something other than a toast because a toast times out after a few seconds.

In the meantime, the toast can simply be one line:

3 of 6 shards failed

gchaps · 2018-07-19T22:17:37Z

src/ui/public/index_patterns/_index_pattern.js

+              {' '} for more information
+            </Fragment>
+          ),
+        });
      }


Support for time intervals was removed

For more information, view the log* index pattern.

where "view the log* index pattern" is the link

gchaps · 2018-07-19T22:23:19Z

x-pack/plugins/graph/public/app.js

-      notify.warning('Invalid URL - the url must contain a {{gquery}} string');
+      toastNotifications.addWarning({
+        title: 'Invalid URL',
+        text: 'The url must contain a {{gquery}} string',


In the description:

url -> URL
add ending period after string

The URL must contain a {{gquery}} string.

gchaps · 2018-07-19T22:30:14Z

x-pack/plugins/graph/public/app.js

+    toastNotifications.addWarning({
+      title: 'No data sources',
+      text: <p>Go to <a href={url}>Management &gt; Index Patterns</a> and create an index pattern</p>,
+    });
  }



No data source

Go to Management > Index Patterns and create an index pattern.

Notes:
sources -> source
add period after pattern

gchaps · 2018-07-19T22:33:02Z

x-pack/plugins/ml/public/timeseriesexplorer/timeseriesexplorer_controller.js

          }

          if (selectedJobIds.length > 1 || mlJobSelectService.groupIds.length) {
          // if more than one job or a group has been loaded from the URL
            if (selectedJobIds.length > 1) {
            // if more than one job, select the first job from the selection.
-              notify.warning('Only one job may be viewed at a time in this dashboard', { lifetime: 30000 });
+              toastNotifications.addWarning('Only one job may be viewed at a time in this dashboard');
              mlJobSelectService.setJobIds([selectedJobIds[0]]);


You can only view one job at a time in this dashboard

gchaps · 2018-07-19T22:33:28Z

x-pack/plugins/ml/public/timeseriesexplorer/timeseriesexplorer_controller.js

              mlJobSelectService.setJobIds([selectedJobIds[0]]);
            } else {
            // if a group has been loaded
              if (selectedJobIds.length > 0) {
              // if the group contains valid jobs, select the first
-                notify.warning('Only one job may be viewed at a time in this dashboard', { lifetime: 30000 });
+                toastNotifications.addWarning('Only one job may be viewed at a time in this dashboard');
                mlJobSelectService.setJobIds([selectedJobIds[0]]);


You can only view one job at a time in this dashboard

gchaps · 2018-07-19T22:42:15Z

x-pack/plugins/ml/public/util/index_utils.js

@@ -71,7 +71,7 @@ export function timeBasedIndexCheck(indexPattern, showNotification = false) {
    if (showNotification) {
      const message = `The index pattern ${indexPattern.title} is not time series based. \
        Anomaly detection can only be run over indices which are time based.`;
-      notify.warning(message, { lifetime: 0 });
+      toastNotifications.addWarning(message);
    }


The index pattern ${indexPattern.title} is not based on a time series.

Anomaly detection only runs over time-based indices.

elasticmachine · 2018-07-20T01:18:14Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

cjcenizal · 2018-07-20T01:25:04Z

Failure:

01:01:48    │ proc  [ftr]        └- ✖ fail: "management scripted fields creating and using Painless numeric scripted fields should see scripted field value in Discover"
01:01:48    │ proc  [ftr]        │        tryForTime timeout: [POST http://localhost:9515/session/500caaac3ccb3785de8657b0c9bd4388/element / {"using":"css selector","value":".visualization"}] no such element: Unable to locate element: {"method":"css selector","selector":".visualization"}

cjcenizal · 2018-07-20T01:25:08Z

Retest

elasticmachine · 2018-07-20T02:31:22Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

cjcenizal · 2018-07-20T03:39:05Z

Looks like this is a legit failure.

cjcenizal · 2018-07-20T18:42:43Z

Retest

elasticmachine · 2018-07-20T20:00:24Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…g-toasts

elasticmachine · 2018-07-20T22:01:57Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

* Replace notify.warning with toastNotifications in region map, vega, index_pattern, redirect_when_missing, graph, monitoring, and ML * Link to index patterns from Graph toast. * Delete RouteBasedNotifier. * Remove courierNotifier and SearchTimeout and ShardFailure errors. * Remove warning and custom notifier types.

cjcenizal added 4 commits July 12, 2018 19:33

Replace notify.warning with toastNotifications in region map, vega, i…

699cd0e

…ndex_pattern, redirect_when_missing, and graph.

Replace notify.warning with toastNotifications in monitoring.

32d6cf7

Replace notify.warning with toastNotifications in ML.

e08de1c

Delete RouteBasedNotifier.

d02d5ef

cjcenizal added 4 commits July 16, 2018 18:08

Remove courierNotifier and SearchTimeout and ShardFailure errors.

7bb60ce

Fix formatting of index pattern toast.

b94c0fb

Improve formatting of Graph tests.

0e70114

Remove warning and custom notifier types.

c37c0b9

cjcenizal changed the title ~~[WIP] Converts notify.warning calls to use toastNotifications~~ Converts notify.warning calls to use toastNotifications Jul 17, 2018

cjcenizal added chore Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v7.0.0 v6.4.0 labels Jul 17, 2018

cjcenizal requested review from spalger, thomasneirynck, bmcconaghy, peteharverson and ppisljar July 17, 2018 01:41

cjcenizal changed the title ~~Converts notify.warning calls to use toastNotifications~~ Convert notify.warning calls to use toastNotifications Jul 17, 2018

jgowdyelastic reviewed Jul 17, 2018

View reviewed changes

Fix index pattern test.

73e2399

cjcenizal force-pushed the notify-warning-toasts branch from 684e48f to 73e2399 Compare July 17, 2018 06:16

Fix duplicate banner bug in ML.

8afc6dc

spalger approved these changes Jul 18, 2018

View reviewed changes

Fix monitoring test.

28a0bd3

timroes reviewed Jul 18, 2018

View reviewed changes

timroes approved these changes Jul 18, 2018

View reviewed changes

jgowdyelastic approved these changes Jul 18, 2018

View reviewed changes

peteharverson approved these changes Jul 18, 2018

View reviewed changes

cjcenizal mentioned this pull request Jul 18, 2018

Surface more information for diagnosing shard failures #5957

Closed

cjcenizal requested a review from pickypg July 18, 2018 14:33

pickypg approved these changes Jul 18, 2018

View reviewed changes

cjcenizal removed request for thomasneirynck, bmcconaghy and ppisljar July 18, 2018 16:19

gchaps reviewed Jul 19, 2018

View reviewed changes

Update copy.

20070bb

Merge branch 'master' of github.com:elastic/kibana into notify-warnin…

cedeae0

…g-toasts

cjcenizal merged commit ab0e2ba into elastic:master Jul 20, 2018

cjcenizal deleted the notify-warning-toasts branch July 20, 2018 22:11

cjcenizal mentioned this pull request Jul 20, 2018

[6.x] Convert notify.warning calls to use toastNotifications (#20767) #21054

Merged

		@@ -38,8 +38,16 @@ export function checkLicense(Private, kbnBaseUrl) {
		const message = features.message;
		const exists = _.find(notify._notifs, (item) => item.content === message);

Convert notify.warning calls to use toastNotifications #20767

Convert notify.warning calls to use toastNotifications #20767

Conversation

cjcenizal commented Jul 13, 2018 • edited Loading

Index pattern

Region map

Courier

Redirect when missing

Graph

Monitoring

elasticmachine commented Jul 13, 2018

💔 Build Failed

cjcenizal commented Jul 17, 2018 • edited Loading

elasticmachine commented Jul 17, 2018

💔 Build Failed

elasticmachine commented Jul 17, 2018

💔 Build Failed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Jul 17, 2018

💔 Build Failed

snide commented Jul 17, 2018

cchaos commented Jul 17, 2018

spalger left a comment

Choose a reason for hiding this comment

elasticmachine commented Jul 18, 2018

💔 Build Failed

elasticmachine commented Jul 18, 2018

💚 Build Succeeded

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timroes left a comment

Choose a reason for hiding this comment

jgowdyelastic left a comment

Choose a reason for hiding this comment

peteharverson left a comment

Choose a reason for hiding this comment

pickypg 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

Choose a reason for hiding this comment

elasticmachine commented Jul 20, 2018

💔 Build Failed

cjcenizal commented Jul 20, 2018

cjcenizal commented Jul 20, 2018

elasticmachine commented Jul 20, 2018

💔 Build Failed

cjcenizal commented Jul 20, 2018

cjcenizal commented Jul 20, 2018

elasticmachine commented Jul 20, 2018

💔 Build Failed

elasticmachine commented Jul 20, 2018

💚 Build Succeeded

cjcenizal commented Jul 13, 2018 •

edited

Loading

cjcenizal commented Jul 17, 2018 •

edited

Loading