Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert notify.warning calls to use toastNotifications #20767

Merged
merged 17 commits into from
Jul 20, 2018
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 3 additions & 5 deletions src/core_plugins/region_map/public/region_map_vis_params.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,13 @@
*/

import { uiModules } from 'ui/modules';
import { toastNotifications } from 'ui/notify';
import regionMapVisParamsTemplate from './region_map_vis_params.html';
import { mapToLayerWithId } from './util';
import '../../tile_map/public/editors/wms_options';

uiModules.get('kibana/region_map')
.directive('regionMapVisParams', function (serviceSettings, regionmapsConfig, Notifier) {

const notify = new Notifier({ location: 'Region map' });

.directive('regionMapVisParams', function (serviceSettings, regionmapsConfig) {
return {
restrict: 'E',
template: regionMapVisParamsTemplate,
Expand Down Expand Up @@ -84,7 +82,7 @@ uiModules.get('kibana/region_map')

})
.catch(function (error) {
notify.warning(error.message);
toastNotifications.addWarning(error.message);
});
}

Expand Down
17 changes: 8 additions & 9 deletions src/core_plugins/region_map/public/region_map_visualization.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,9 @@ import ChoroplethLayer from './choropleth_layer';
import { truncatedColorMaps } from 'ui/vislib/components/color/truncated_colormaps';
import AggResponsePointSeriesTooltipFormatterProvider from './tooltip_formatter';
import 'ui/vis/map/service_settings';
import { toastNotifications } from 'ui/notify';

export function RegionMapsVisualizationProvider(Private, Notifier, config) {
export function RegionMapsVisualizationProvider(Private, config) {

const tooltipFormatter = Private(AggResponsePointSeriesTooltipFormatterProvider);
const BaseMapsVisualization = Private(BaseMapsVisualizationProvider);
Expand All @@ -36,18 +37,15 @@ export function RegionMapsVisualizationProvider(Private, Notifier, config) {
super(container, vis);
this._vis = this.vis;
this._choroplethLayer = null;
this._notify = new Notifier({ location: 'Region map' });
}


async render(esResponse, status) {
await super.render(esResponse, status);
if (this._choroplethLayer) {
await this._choroplethLayer.whenDataLoaded();
}
}


async _updateData(tableGroup) {
this._chartData = tableGroup;
let results;
Expand Down Expand Up @@ -81,7 +79,6 @@ export function RegionMapsVisualizationProvider(Private, Notifier, config) {
this._kibanaMap.useUiStateFromVisualization(this._vis);
}


async _updateParams() {

await super._updateParams();
Expand Down Expand Up @@ -156,13 +153,15 @@ export function RegionMapsVisualizationProvider(Private, Notifier, config) {
const rowIndex = this._chartData.tables[0].rows.findIndex(row => row[0] === event);
this._vis.API.events.addFilter(this._chartData.tables[0], 0, rowIndex, event);
});

this._choroplethLayer.on('styleChanged', (event) => {
const shouldShowWarning = this._vis.params.isDisplayWarning && config.get('visualization:regionmap:showWarnings');
if (event.mismatches.length > 0 && shouldShowWarning) {
this._notify.warning(`Could not show ${event.mismatches.length} ${event.mismatches.length > 1 ? 'results' : 'result'} on the map.`
+ ` To avoid this, ensure that each term can be matched to a corresponding shape on that shape's join field.`
+ ` Could not match following terms: ${event.mismatches.join(',')}`
);
toastNotifications.addWarning({
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(',')}`
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unable to show 3 results on map

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

}
});

Expand Down
6 changes: 3 additions & 3 deletions src/core_plugins/tile_map/public/base_maps_visualization.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import { KibanaMap } from 'ui/vis/map/kibana_map';
import * as Rx from 'rxjs';
import { filter, first } from 'rxjs/operators';
import 'ui/vis/map/service_settings';

import { toastNotifications } from 'ui/notify';

const MINZOOM = 0;
const MAXZOOM = 22;//increase this to 22. Better for WMS
Expand Down Expand Up @@ -142,7 +142,7 @@ export function BaseMapsVisualizationProvider(serviceSettings) {
this._setTmsLayer(firstRoadMapLayer);
}
} catch (e) {
this._notify.warning(e.message);
toastNotifications.addWarning(e.message);
return;
}
return;
Expand Down Expand Up @@ -174,7 +174,7 @@ export function BaseMapsVisualizationProvider(serviceSettings) {

}
} catch (tmsLoadingError) {
this._notify.warning(tmsLoadingError.message);
toastNotifications.addWarning(tmsLoadingError.message);
}


Expand Down
4 changes: 2 additions & 2 deletions src/core_plugins/vega/public/vega_visualization.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
* under the License.
*/

import { Notifier } from 'ui/notify';
import { toastNotifications, Notifier } from 'ui/notify';
import { VegaView } from './vega_view/vega_view';
import { VegaMapView } from './vega_view/vega_map_view';
import { SavedObjectsClientProvider, findObjectByTitle } from 'ui/saved_objects';
Expand Down Expand Up @@ -59,7 +59,7 @@ export function VegaVisualizationProvider(Private, vegaConfig, serviceSettings,
*/
async render(visData, status) {
if (!visData && !this._vegaView) {
notify.warning('Unable to render without data');
toastNotifications.addWarning('Unable to render without data');
return;
}

Expand Down
4 changes: 0 additions & 4 deletions src/ui/public/__tests__/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,8 @@

import expect from 'expect.js';
import {
SearchTimeout,
RequestFailure,
FetchFailure,
ShardFailure,
VersionConflict,
MappingConflict,
RestrictedMapping,
Expand All @@ -46,10 +44,8 @@ import {

describe('ui/errors', () => {
const errors = [
new SearchTimeout(),
new RequestFailure('an error', { }),
new FetchFailure({ }),
new ShardFailure({ '_shards': 5 }),
new VersionConflict({ }),
new MappingConflict({ }),
new RestrictedMapping('field', 'indexPattern'),
Expand Down
15 changes: 10 additions & 5 deletions src/ui/public/courier/fetch/call_response_handlers.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,9 @@
* under the License.
*/

import { RequestFailure, SearchTimeout, ShardFailure } from '../../errors';

import { toastNotifications } from '../../notify';
import { RequestFailure } from '../../errors';
import { RequestStatus } from './req_status';
import { courierNotifier } from './notifier';

export function CallResponseHandlersProvider(Private, Promise) {
const ABORTED = RequestStatus.ABORTED;
Expand All @@ -35,11 +34,17 @@ export function CallResponseHandlersProvider(Private, Promise) {
const response = responses[index];

if (response.timed_out) {
courierNotifier.warning(new SearchTimeout());
toastNotifications.addWarning({
title: 'The data shown may be incomplete',
text: 'All or part of your request has timed out',
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use a title only:

Data might be incomplete because your request timed out

}

if (response._shards && response._shards.failed) {
courierNotifier.warning(new ShardFailure(response));
toastNotifications.addWarning({
title: 'Shard failure',
text: `${response._shards.failed} of ${response._shards.total} shards failed`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

});
}

function progress() {
Expand Down
3 changes: 1 addition & 2 deletions src/ui/public/courier/fetch/fetch_now.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import { CallClientProvider } from './call_client';
import { CallResponseHandlersProvider } from './call_response_handlers';
import { ContinueIncompleteProvider } from './continue_incomplete';
import { RequestStatus } from './req_status';
import { location } from './notifier';

/**
* Fetch now provider should be used if you want the results searched and returned immediately.
Expand Down Expand Up @@ -53,7 +52,7 @@ export function FetchNowProvider(Private, Promise) {

return searchRequest.retry();
}))
.catch(error => fatalError(error, location));
.catch(error => fatalError(error, 'Courier fetch'));
}

function fetchSearchResults(searchRequests) {
Expand Down
26 changes: 0 additions & 26 deletions src/ui/public/courier/fetch/notifier.js

This file was deleted.

24 changes: 0 additions & 24 deletions src/ui/public/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,16 +51,6 @@ export class KbnError {
// instanceof checks.
createLegacyClass(KbnError).inherits(Error);

/**
* SearchTimeout error class
*/
export class SearchTimeout extends KbnError {
constructor() {
super('All or part of your request has timed out. The data shown may be incomplete.',
SearchTimeout);
}
}

/**
* Request Failure - When an entire multi request fails
* @param {Error} err - the Error that came back
Expand Down Expand Up @@ -92,20 +82,6 @@ export class FetchFailure extends KbnError {
}
}

/**
* ShardFailure Error - when one or more shards fail
* @param {Object} resp - The response from es.
*/
export class ShardFailure extends KbnError {
constructor(resp) {
super(
`${resp._shards.failed} of ${resp._shards.total} shards failed.`,
ShardFailure);

this.resp = resp;
}
}

/**
* A doc was re-indexed but it was out of date.
* @param {Object} resp - The response from es (one of the multi-response responses).
Expand Down
27 changes: 12 additions & 15 deletions src/ui/public/index_patterns/__tests__/_index_pattern.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,16 +29,14 @@ import { FixturesStubbedSavedObjectIndexPatternProvider } from 'fixtures/stubbed
import { IndexPatternsIntervalsProvider } from '../_intervals';
import { IndexPatternProvider } from '../_index_pattern';
import NoDigestPromises from 'test_utils/no_digest_promises';
import { Notifier } from '../../notify';
import { toastNotifications } from '../../notify';

import { FieldsFetcherProvider } from '../fields_fetcher_provider';
import { StubIndexPatternsApiClientModule } from './stub_index_patterns_api_client';
import { IndexPatternsApiClientProvider } from '../index_patterns_api_client_provider';
import { IsUserAwareOfUnsupportedTimePatternProvider } from '../unsupported_time_patterns';
import { SavedObjectsClientProvider } from '../../saved_objects';

const MARKDOWN_LINK_RE = /\[(.+?)\]\((.+?)\)/;

describe('index pattern', function () {
NoDigestPromises.activateForSuite();

Expand Down Expand Up @@ -468,23 +466,22 @@ describe('index pattern', function () {
}

it('logs a warning when the index pattern source includes `intervalName`', async () => {
const indexPattern = await createUnsupportedTimePattern();
expect(Notifier.prototype._notifs).to.have.length(1);

const notif = Notifier.prototype._notifs.shift();
expect(notif).to.have.property('type', 'warning');
expect(notif.content).to.match(MARKDOWN_LINK_RE);

const [, text, url] = notif.content.match(MARKDOWN_LINK_RE);
expect(text).to.contain(indexPattern.title);
expect(url).to.contain(indexPattern.id);
expect(url).to.contain('management/kibana/indices');
await createUnsupportedTimePattern();
expect(toastNotifications.list).to.have.length(1);
});

it('does not notify if isUserAwareOfUnsupportedTimePattern() returns true', async () => {
// Ideally, _index_pattern.js shouldn't be tightly coupled to toastNotifications. Instead, it
// should notify its consumer of this state and the consumer should be responsible for
// notifying the user. This test verifies the side effect of the state until we can remove
// this coupling.

// Clear existing toasts.
toastNotifications.list.splice(0);

isUserAwareOfUnsupportedTimePattern.returns(true);
await createUnsupportedTimePattern();
expect(Notifier.prototype._notifs).to.have.length(0);
expect(toastNotifications.list).to.have.length(0);
});
});
});
18 changes: 11 additions & 7 deletions src/ui/public/index_patterns/_index_pattern.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
* under the License.
*/

import React, { Fragment } from 'react';
import _ from 'lodash';
import { SavedObjectNotFound, DuplicateField, IndexPatternMissingIndices } from '../errors';
import angular from 'angular';
Expand Down Expand Up @@ -120,13 +121,16 @@ export function IndexPatternProvider(Private, config, Promise, confirmModalPromi

if (indexPattern.isUnsupportedTimePattern()) {
if (!isUserAwareOfUnsupportedTimePattern(indexPattern)) {
const warning = (
'Support for time-intervals has been removed. ' +
`View the ["${indexPattern.title}" index pattern in management](` +
kbnUrl.getRouteHref(indexPattern, 'edit') +
') for more information.'
);
notify.warning(warning, { lifetime: Infinity });
toastNotifications.addWarning({
title: 'Support for time-intervals has been removed',
text: (
<Fragment>
View the {' '}
<a href={kbnUrl.getRouteHref(indexPattern, 'edit')}>{indexPattern.title} index pattern in management</a>
{' '} for more information
</Fragment>
),
});
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Support for time intervals was removed

For more information, view the log* index pattern.

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

}

Expand Down
Loading