Skip to content

Commit

Permalink
Add VisLayer error toasts when rendering vis (#3649)
Browse files Browse the repository at this point in the history
* Add VisLayer error toasts when rendering vis
* Finalize formatting of err stack details
* Add one test case
* Make message of VisLayerError required
* Move test_helpers; clean up test; add comment
* Add id to error toast
* Add context to helper fns
* Change back to toStrictEqual() for tests

Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>

---------

Signed-off-by: Tyler Ohlsen <ohltyler@amazon.com>
  • Loading branch information
ohltyler committed Apr 26, 2023
1 parent 3a719c5 commit 03b393f
Show file tree
Hide file tree
Showing 12 changed files with 211 additions and 23 deletions.
4 changes: 4 additions & 0 deletions src/core/public/notifications/toasts/toasts_api.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,10 @@ export interface ErrorToastOptions extends ToastOptions {
* message will still be shown in the detailed error modal.
*/
toastMessage?: string;
/**
* Unique ID for the toast. Can be used to prevent duplicate toasts on re-renders.
*/
id?: string;
}

const normalizeToast = (toastOrTitle: ToastInput): ToastInputFields => {
Expand Down
1 change: 1 addition & 0 deletions src/plugins/vis_augmenter/public/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ export {
PointInTimeEvent,
PointInTimeEventsVisLayer,
isPointInTimeEventsVisLayer,
isVisLayerWithError,
} from './types';

export * from './expressions';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
*/

import { cloneDeep } from 'lodash';
import { VisLayerExpressionFn, ISavedAugmentVis } from '../../types';
import { VisLayerExpressionFn, ISavedAugmentVis } from '../../';
import { VIS_REFERENCE_NAME } from '../saved_augment_vis_references';

const pluginResourceId = 'test-plugin-resource-id';
Expand Down
33 changes: 19 additions & 14 deletions src/plugins/vis_augmenter/public/types.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,20 +3,13 @@
* SPDX-License-Identifier: Apache-2.0
*/

import { VisLayerTypes, VisLayer, isPointInTimeEventsVisLayer, isValidVisLayer } from './types';

const generateVisLayer = (type: any): VisLayer => {
return {
type,
originPlugin: 'test-plugin',
pluginResource: {
type: 'test-resource-type',
id: 'test-resource-id',
name: 'test-resource-name',
urlPath: 'test-resource-url-path',
},
};
};
import {
VisLayerTypes,
isPointInTimeEventsVisLayer,
isValidVisLayer,
isVisLayerWithError,
} from './types';
import { generateVisLayer } from './utils';

describe('isPointInTimeEventsVisLayer()', function () {
it('should return false if type does not match', function () {
Expand All @@ -41,3 +34,15 @@ describe('isValidVisLayer()', function () {
expect(isValidVisLayer(visLayer)).toBe(true);
});
});

describe('isVisLayerWithError()', function () {
it('should return false if no error', function () {
const visLayer = generateVisLayer('unknown-vis-layer-type', false);
expect(isVisLayerWithError(visLayer)).toBe(false);
});

it('should return true if error', function () {
const visLayer = generateVisLayer(VisLayerTypes.PointInTimeEvents, true);
expect(isVisLayerWithError(visLayer)).toBe(true);
});
});
11 changes: 10 additions & 1 deletion src/plugins/vis_augmenter/public/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ export enum VisLayerErrorTypes {

export interface VisLayerError {
type: keyof typeof VisLayerErrorTypes;
message?: string;
message: string;
}

export type PluginResourceType = string;
Expand Down Expand Up @@ -53,6 +53,15 @@ export const isPointInTimeEventsVisLayer = (obj: any) => {
return obj?.type === VisLayerTypes.PointInTimeEvents;
};

/**
* Used to determine if a given saved obj has a valid type and can
* be converted into a VisLayer
*/
export const isValidVisLayer = (obj: any) => {
return obj?.type in VisLayerTypes;
};

/**
* Used for checking if an existing VisLayer has a populated error field or not
*/
export const isVisLayerWithError = (visLayer: VisLayer): boolean => visLayer.error !== undefined;
1 change: 1 addition & 0 deletions src/plugins/vis_augmenter/public/utils/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,4 @@
*/

export * from './utils';
export * from './test_helpers';
36 changes: 36 additions & 0 deletions src/plugins/vis_augmenter/public/utils/test_helpers.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

import { get } from 'lodash';
import { VisLayer, VisLayerErrorTypes } from '../types';

export const generateVisLayer = (
type: any,
error: boolean = false,
errorMessage: string = 'some-error-message',
resource?: {
type?: string;
id?: string;
name?: string;
urlPath?: string;
}
): VisLayer => {
return {
type,
originPlugin: 'test-plugin',
pluginResource: {
type: get(resource, 'type', 'test-resource-type'),
id: get(resource, 'id', 'test-resource-id'),
name: get(resource, 'name', 'test-resource-name'),
urlPath: get(resource, 'urlPath', 'test-resource-url-path'),
},
error: error
? {
type: VisLayerErrorTypes.FETCH_FAILURE,
message: errorMessage,
}
: undefined,
};
};
71 changes: 69 additions & 2 deletions src/plugins/vis_augmenter/public/utils/utils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,19 @@ import { Vis } from '../../../visualizations/public';
import {
buildPipelineFromAugmentVisSavedObjs,
getAugmentVisSavedObjs,
getAnyErrors,
isEligibleForVisLayers,
} from './utils';
import { VisLayerTypes, ISavedAugmentVis, VisLayerExpressionFn } from '../types';
import {
createSavedAugmentVisLoader,
SavedObjectOpenSearchDashboardsServicesWithAugmentVis,
getMockAugmentVisSavedObjectClient,
generateAugmentVisSavedObject,
} from '../saved_augment_vis';
ISavedAugmentVis,
VisLayerExpressionFn,
VisLayerTypes,
} from '../';
import { generateVisLayer } from './';

describe('utils', () => {
// TODO: redo / update this test suite when eligibility is finalized.
Expand Down Expand Up @@ -129,4 +133,67 @@ describe('utils', () => {
expect(str).toEqual(`fn-1 arg1="value-1"\n| fn-2 arg2="value-2"`);
});
});

describe('getAnyErrors', () => {
const noErrorLayer1 = generateVisLayer(VisLayerTypes.PointInTimeEvents, false);
const noErrorLayer2 = generateVisLayer(VisLayerTypes.PointInTimeEvents, false);
const errorLayer1 = generateVisLayer(VisLayerTypes.PointInTimeEvents, true, 'uh-oh!', {
type: 'resource-type-1',
id: '1234',
name: 'resource-1',
});
const errorLayer2 = generateVisLayer(
VisLayerTypes.PointInTimeEvents,
true,
'oh no something terrible has happened :(',
{
type: 'resource-type-2',
id: '5678',
name: 'resource-2',
}
);
const errorLayer3 = generateVisLayer(VisLayerTypes.PointInTimeEvents, true, 'oops!', {
type: 'resource-type-1',
id: 'abcd',
name: 'resource-3',
});

it('empty array - returns undefined', async () => {
const err = getAnyErrors([], 'title-vis-title');
expect(err).toEqual(undefined);
});
it('single VisLayer no errors - returns undefined', async () => {
const err = getAnyErrors([noErrorLayer1], 'test-vis-title');
expect(err).toEqual(undefined);
});
it('multiple VisLayers no errors - returns undefined', async () => {
const err = getAnyErrors([noErrorLayer1, noErrorLayer2], 'test-vis-title');
expect(err).toEqual(undefined);
});
it('single VisLayer with error - returns formatted error', async () => {
const err = getAnyErrors([errorLayer1], 'test-vis-title');
expect(err).not.toEqual(undefined);
expect(err?.stack).toStrictEqual(`-----resource-type-1-----\nID: 1234\nMessage: "uh-oh!"`);
});
it('multiple VisLayers with errors - returns formatted error', async () => {
const err = getAnyErrors([errorLayer1, errorLayer2], 'test-vis-title');
expect(err).not.toEqual(undefined);
expect(err?.stack).toStrictEqual(
`-----resource-type-1-----\nID: 1234\nMessage: "uh-oh!"\n\n\n` +
`-----resource-type-2-----\nID: 5678\nMessage: "oh no something terrible has happened :("`
);
});
it('multiple VisLayers with errors of same type - returns formatted error', async () => {
const err = getAnyErrors([errorLayer1, errorLayer3], 'test-vis-title');
expect(err).not.toEqual(undefined);
expect(err?.stack).toStrictEqual(
`-----resource-type-1-----\nID: 1234\nMessage: "uh-oh!"\n\n` + `ID: abcd\nMessage: "oops!"`
);
});
it('VisLayers with and without error - returns formatted error', async () => {
const err = getAnyErrors([noErrorLayer1, errorLayer1], 'test-vis-title');
expect(err).not.toEqual(undefined);
expect(err?.stack).toStrictEqual(`-----resource-type-1-----\nID: 1234\nMessage: "uh-oh!"`);
});
});
});
45 changes: 43 additions & 2 deletions src/plugins/vis_augmenter/public/utils/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,21 @@
* SPDX-License-Identifier: Apache-2.0
*/

import { get } from 'lodash';
import { get, isEmpty } from 'lodash';
import { Vis } from '../../../../plugins/visualizations/public';
import {
formatExpression,
buildExpressionFunction,
buildExpression,
ExpressionAstFunctionBuilder,
} from '../../../../plugins/expressions/public';
import { ISavedAugmentVis, SavedAugmentVisLoader, VisLayerFunctionDefinition } from '../';
import {
ISavedAugmentVis,
SavedAugmentVisLoader,
VisLayerFunctionDefinition,
VisLayer,
isVisLayerWithError,
} from '../';

// TODO: provide a deeper eligibility check.
// Tracked in https://github.com/opensearch-project/OpenSearch-Dashboards/issues/3268
Expand Down Expand Up @@ -58,3 +64,38 @@ export const buildPipelineFromAugmentVisSavedObjs = (objs: ISavedAugmentVis[]):
throw new Error('Expression function from augment-vis saved objects could not be generated');
}
};

/**
* Returns an error with an aggregated message about all of the
* errors found in the set of VisLayers. If no errors, returns undefined.
*/
export const getAnyErrors = (visLayers: VisLayer[], visTitle: string): Error | undefined => {
const visLayersWithErrors = visLayers.filter((visLayer) => isVisLayerWithError(visLayer));
if (!isEmpty(visLayersWithErrors)) {
// Aggregate by unique plugin resource type
const resourceTypes = [
...new Set(visLayersWithErrors.map((visLayer) => visLayer.pluginResource.type)),
];

let msgDetails = '';
resourceTypes.forEach((type, index) => {
const matchingVisLayers = visLayersWithErrors.filter(
(visLayer) => visLayer.pluginResource.type === type
);
if (index !== 0) msgDetails += '\n\n\n';
msgDetails += `-----${type}-----`;
matchingVisLayers.forEach((visLayer, idx) => {
if (idx !== 0) msgDetails += '\n';
msgDetails += `\nID: ${visLayer.pluginResource.id}`;
msgDetails += `\nMessage: "${visLayer.error?.message}"`;
});
});

const err = new Error(`Certain plugin resources failed to load on the ${visTitle} chart`);
// We set as the stack here so it can be parsed and shown cleanly in the details modal coming from the error toast notification.
err.stack = msgDetails;
return err;
} else {
return undefined;
}
};
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ import {
} from '../../../expressions/public';
import { buildPipeline } from '../legacy/build_pipeline';
import { Vis, SerializedVis } from '../vis';
import { getExpressions, getUiActions } from '../services';
import { getExpressions, getNotifications, getUiActions } from '../services';
import { VIS_EVENT_TO_TRIGGER } from './events';
import { VisualizeEmbeddableFactoryDeps } from './visualize_embeddable_factory';
import { TriggerId } from '../../../ui_actions/public';
Expand All @@ -71,6 +71,7 @@ import {
isEligibleForVisLayers,
getAugmentVisSavedObjs,
buildPipelineFromAugmentVisSavedObjs,
getAnyErrors,
} from '../../../vis_augmenter/public';
import { VisSavedObject } from '../types';

Expand Down Expand Up @@ -511,13 +512,27 @@ export class VisualizeEmbeddable
layers: [] as VisLayers,
};
// We cannot use this.handler in this case, since it does not support the run() cmd
// we need here. So, we consume the expressions service to run this instead.
// we need here. So, we consume the expressions service to run this directly instead.
const exprVisLayers = (await getExpressions().run(
visLayersPipeline,
visLayersPipelineInput,
expressionParams as Record<string, unknown>
)) as ExprVisLayers;
return exprVisLayers.layers;
const visLayers = exprVisLayers.layers;
const err = getAnyErrors(visLayers, this.vis.title);
// This is only true when one or more VisLayers has an error
if (err !== undefined) {
const { toasts } = getNotifications();
toasts.addError(err, {
title: i18n.translate('visualizations.renderVisTitle', {
defaultMessage: `Error loading data on the ${this.vis.title} chart`,
}),
toastMessage: ' ',
id: this.id,
});
}

return visLayers;
}
return [] as VisLayers;
};
Expand Down
4 changes: 4 additions & 0 deletions src/plugins/visualizations/public/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import {
Plugin,
ApplicationStart,
SavedObjectsClientContract,
NotificationsStart,
} from '../../../core/public';
import { TypesService, TypesSetup, TypesStart } from './vis_types';
import {
Expand All @@ -61,6 +62,7 @@ import {
setOverlays,
setSavedSearchLoader,
setEmbeddable,
setNotifications,
} from './services';
import {
VISUALIZE_EMBEDDABLE_TYPE,
Expand Down Expand Up @@ -130,6 +132,7 @@ export interface VisualizationsStartDeps {
dashboard: DashboardStart;
getAttributeService: DashboardStart['getAttributeService'];
savedObjectsClient: SavedObjectsClientContract;
notifications: NotificationsStart;
}

/**
Expand Down Expand Up @@ -220,6 +223,7 @@ export class VisualizationsPlugin
});
setSavedAugmentVisLoader(savedAugmentVisLoader);
setSavedSearchLoader(savedSearchLoader);
setNotifications(core.notifications);
return {
...types,
showNewVisModal,
Expand Down
5 changes: 5 additions & 0 deletions src/plugins/visualizations/public/services.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import {
IUiSettingsClient,
OverlayStart,
SavedObjectsStart,
NotificationsStart,
} from '../../../core/public';
import { TypesStart } from './vis_types';
import { createGetterSetter } from '../../opensearch_dashboards_utils/common';
Expand Down Expand Up @@ -111,3 +112,7 @@ export const [getSavedSearchLoader, setSavedSearchLoader] = createGetterSetter<S
export const [getSavedAugmentVisLoader, setSavedAugmentVisLoader] = createGetterSetter<
SavedAugmentVisLoader
>('SavedAugmentVisLoader');

export const [getNotifications, setNotifications] = createGetterSetter<NotificationsStart>(
'Notifications'
);

0 comments on commit 03b393f

Please sign in to comment.