From e9e7453e1dd6785cebf53fd344830f73dadab42c Mon Sep 17 00:00:00 2001 From: Marco Liberati Date: Mon, 15 Feb 2021 13:00:31 +0100 Subject: [PATCH] [Lens] Improves error messages when in Dashboard (#90668) Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> --- .../visualization.test.tsx | 16 +- .../datatable_visualization/visualization.tsx | 2 +- .../editor_frame/state_helpers.ts | 55 +++- .../workspace_panel/workspace_panel.tsx | 5 +- .../embeddable/embeddable.test.tsx | 173 ++++++---- .../embeddable/embeddable.tsx | 12 +- .../embeddable/embeddable_factory.ts | 5 +- .../embeddable/expression_wrapper.tsx | 61 +++- .../editor_frame_service/error_helper.ts | 12 + .../public/editor_frame_service/mocks.tsx | 2 +- .../public/editor_frame_service/service.tsx | 6 +- .../lens/public/editor_frame_service/types.ts | 5 + .../visualization.test.ts | 18 +- .../metric_visualization/visualization.tsx | 2 +- .../lens/public/pie_visualization/toolbar.tsx | 9 +- .../pie_visualization/visualization.test.ts | 30 +- .../pie_visualization/visualization.tsx | 4 +- x-pack/plugins/lens/public/types.ts | 7 +- .../lens/public/visualization_container.scss | 8 + .../xy_visualization/color_assignment.ts | 2 +- .../xy_visualization/visualization.test.ts | 308 ++++++++---------- .../public/xy_visualization/visualization.tsx | 2 +- .../xy_visualization/xy_config_panel.tsx | 2 +- x-pack/test/functional/apps/lens/dashboard.ts | 34 ++ 24 files changed, 447 insertions(+), 333 deletions(-) diff --git a/x-pack/plugins/lens/public/datatable_visualization/visualization.test.tsx b/x-pack/plugins/lens/public/datatable_visualization/visualization.test.tsx index 2a6228f16867d..ad5e1e552ccd2 100644 --- a/x-pack/plugins/lens/public/datatable_visualization/visualization.test.tsx +++ b/x-pack/plugins/lens/public/datatable_visualization/visualization.test.tsx @@ -459,10 +459,10 @@ describe('Datatable Visualization', () => { label: 'label', }); - const error = datatableVisualization.getErrorMessages( - { layerId: 'a', columns: [{ columnId: 'b' }, { columnId: 'c' }] }, - frame - ); + const error = datatableVisualization.getErrorMessages({ + layerId: 'a', + columns: [{ columnId: 'b' }, { columnId: 'c' }], + }); expect(error).toBeUndefined(); }); @@ -478,10 +478,10 @@ describe('Datatable Visualization', () => { label: 'label', }); - const error = datatableVisualization.getErrorMessages( - { layerId: 'a', columns: [{ columnId: 'b' }, { columnId: 'c' }] }, - frame - ); + const error = datatableVisualization.getErrorMessages({ + layerId: 'a', + columns: [{ columnId: 'b' }, { columnId: 'c' }], + }); expect(error).toBeUndefined(); }); diff --git a/x-pack/plugins/lens/public/datatable_visualization/visualization.tsx b/x-pack/plugins/lens/public/datatable_visualization/visualization.tsx index 9625a814c7958..47f8ce09aea68 100644 --- a/x-pack/plugins/lens/public/datatable_visualization/visualization.tsx +++ b/x-pack/plugins/lens/public/datatable_visualization/visualization.tsx @@ -276,7 +276,7 @@ export const datatableVisualization: Visualization }; }, - getErrorMessages(state, frame) { + getErrorMessages(state) { return undefined; }, diff --git a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/state_helpers.ts b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/state_helpers.ts index 559e773dbc167..9c7ef19132c46 100644 --- a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/state_helpers.ts +++ b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/state_helpers.ts @@ -18,6 +18,9 @@ import { import { buildExpression } from './expression_helpers'; import { Document } from '../../persistence/saved_object_store'; import { VisualizeFieldContext } from '../../../../../../src/plugins/ui_actions/public'; +import { getActiveDatasourceIdFromDoc } from './state_management'; +import { ErrorMessage } from '../types'; +import { getMissingCurrentDatasource, getMissingVisualizationTypeError } from '../error_helper'; export async function initializeDatasources( datasourceMap: Record, @@ -72,7 +75,7 @@ export async function persistedStateToExpression( datasources: Record, visualizations: Record, doc: Document -): Promise { +): Promise<{ ast: Ast | null; errors: ErrorMessage[] | undefined }> { const { state: { visualization: visualizationState, datasourceStates: persistedDatasourceStates }, visualizationType, @@ -80,7 +83,12 @@ export async function persistedStateToExpression( title, description, } = doc; - if (!visualizationType) return null; + if (!visualizationType) { + return { + ast: null, + errors: [{ shortMessage: '', longMessage: getMissingVisualizationTypeError() }], + }; + } const visualization = visualizations[visualizationType!]; const datasourceStates = await initializeDatasources( datasources, @@ -97,15 +105,33 @@ export async function persistedStateToExpression( const datasourceLayers = createDatasourceLayers(datasources, datasourceStates); - return buildExpression({ - title, - description, + const datasourceId = getActiveDatasourceIdFromDoc(doc); + if (datasourceId == null) { + return { + ast: null, + errors: [{ shortMessage: '', longMessage: getMissingCurrentDatasource() }], + }; + } + const validationResult = validateDatasourceAndVisualization( + datasources[datasourceId], + datasourceStates[datasourceId].state, visualization, visualizationState, - datasourceMap: datasources, - datasourceStates, - datasourceLayers, - }); + { datasourceLayers } + ); + + return { + ast: buildExpression({ + title, + description, + visualization, + visualizationState, + datasourceMap: datasources, + datasourceStates, + datasourceLayers, + }), + errors: validationResult, + }; } export const validateDatasourceAndVisualization = ( @@ -113,13 +139,8 @@ export const validateDatasourceAndVisualization = ( currentDatasourceState: unknown | null, currentVisualization: Visualization | null, currentVisualizationState: unknown | undefined, - frameAPI: FramePublicAPI -): - | Array<{ - shortMessage: string; - longMessage: string; - }> - | undefined => { + frameAPI: Pick +): ErrorMessage[] | undefined => { const layersGroups = currentVisualizationState ? currentVisualization ?.getLayerIds(currentVisualizationState) @@ -141,7 +162,7 @@ export const validateDatasourceAndVisualization = ( : undefined; const visualizationValidationErrors = currentVisualizationState - ? currentVisualization?.getErrorMessages(currentVisualizationState, frameAPI) + ? currentVisualization?.getErrorMessages(currentVisualizationState) : undefined; if (datasourceValidationErrors?.length || visualizationValidationErrors?.length) { diff --git a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx index 2c4cecd356ced..219c0638f9b56 100644 --- a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx +++ b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx @@ -155,10 +155,7 @@ export const WorkspacePanel = React.memo(function WorkspacePanel({ datasourceLayers: framePublicAPI.datasourceLayers, }); } catch (e) { - const buildMessages = activeVisualization?.getErrorMessages( - visualizationState, - framePublicAPI - ); + const buildMessages = activeVisualization?.getErrorMessages(visualizationState); const defaultMessage = { shortMessage: i18n.translate('xpack.lens.editorFrame.buildExpressionError', { defaultMessage: 'An unexpected error occurred while preparing the chart', diff --git a/x-pack/plugins/lens/public/editor_frame_service/embeddable/embeddable.test.tsx b/x-pack/plugins/lens/public/editor_frame_service/embeddable/embeddable.test.tsx index d2085a4cc8a8b..227c8b4741501 100644 --- a/x-pack/plugins/lens/public/editor_frame_service/embeddable/embeddable.test.tsx +++ b/x-pack/plugins/lens/public/editor_frame_service/embeddable/embeddable.test.tsx @@ -116,11 +116,14 @@ describe('embeddable', () => { getTrigger, documentToExpression: () => Promise.resolve({ - type: 'expression', - chain: [ - { type: 'function', function: 'my', arguments: {} }, - { type: 'function', function: 'expression', arguments: {} }, - ], + ast: { + type: 'expression', + chain: [ + { type: 'function', function: 'my', arguments: {} }, + { type: 'function', function: 'expression', arguments: {} }, + ], + }, + errors: undefined, }), }, { @@ -140,6 +143,36 @@ describe('embeddable', () => { | expression`); }); + it('should not render the visualization if any error arises', async () => { + const embeddable = new Embeddable( + { + timefilter: dataPluginMock.createSetupContract().query.timefilter.timefilter, + attributeService, + expressionRenderer, + basePath, + indexPatternService: {} as IndexPatternsContract, + editable: true, + getTrigger, + documentToExpression: () => + Promise.resolve({ + ast: { + type: 'expression', + chain: [ + { type: 'function', function: 'my', arguments: {} }, + { type: 'function', function: 'expression', arguments: {} }, + ], + }, + errors: [{ shortMessage: '', longMessage: 'my validation error' }], + }), + }, + {} as LensEmbeddableInput + ); + await embeddable.initializeSavedVis({} as LensEmbeddableInput); + embeddable.render(mountpoint); + + expect(expressionRenderer).toHaveBeenCalledTimes(0); + }); + it('should initialize output with deduped list of index patterns', async () => { attributeService = attributeServiceMockFromSavedVis({ ...savedVis, @@ -162,11 +195,14 @@ describe('embeddable', () => { getTrigger, documentToExpression: () => Promise.resolve({ - type: 'expression', - chain: [ - { type: 'function', function: 'my', arguments: {} }, - { type: 'function', function: 'expression', arguments: {} }, - ], + ast: { + type: 'expression', + chain: [ + { type: 'function', function: 'my', arguments: {} }, + { type: 'function', function: 'expression', arguments: {} }, + ], + }, + errors: undefined, }), }, {} as LensEmbeddableInput @@ -194,11 +230,14 @@ describe('embeddable', () => { getTrigger, documentToExpression: () => Promise.resolve({ - type: 'expression', - chain: [ - { type: 'function', function: 'my', arguments: {} }, - { type: 'function', function: 'expression', arguments: {} }, - ], + ast: { + type: 'expression', + chain: [ + { type: 'function', function: 'my', arguments: {} }, + { type: 'function', function: 'expression', arguments: {} }, + ], + }, + errors: undefined, }), }, { id: '123' } as LensEmbeddableInput @@ -232,11 +271,14 @@ describe('embeddable', () => { getTrigger, documentToExpression: () => Promise.resolve({ - type: 'expression', - chain: [ - { type: 'function', function: 'my', arguments: {} }, - { type: 'function', function: 'expression', arguments: {} }, - ], + ast: { + type: 'expression', + chain: [ + { type: 'function', function: 'my', arguments: {} }, + { type: 'function', function: 'expression', arguments: {} }, + ], + }, + errors: undefined, }), }, { id: '123' } as LensEmbeddableInput @@ -265,11 +307,14 @@ describe('embeddable', () => { getTrigger, documentToExpression: () => Promise.resolve({ - type: 'expression', - chain: [ - { type: 'function', function: 'my', arguments: {} }, - { type: 'function', function: 'expression', arguments: {} }, - ], + ast: { + type: 'expression', + chain: [ + { type: 'function', function: 'my', arguments: {} }, + { type: 'function', function: 'expression', arguments: {} }, + ], + }, + errors: undefined, }), }, { id: '123' } as LensEmbeddableInput @@ -312,11 +357,14 @@ describe('embeddable', () => { getTrigger, documentToExpression: () => Promise.resolve({ - type: 'expression', - chain: [ - { type: 'function', function: 'my', arguments: {} }, - { type: 'function', function: 'expression', arguments: {} }, - ], + ast: { + type: 'expression', + chain: [ + { type: 'function', function: 'my', arguments: {} }, + { type: 'function', function: 'expression', arguments: {} }, + ], + }, + errors: undefined, }), }, input @@ -359,11 +407,14 @@ describe('embeddable', () => { getTrigger, documentToExpression: () => Promise.resolve({ - type: 'expression', - chain: [ - { type: 'function', function: 'my', arguments: {} }, - { type: 'function', function: 'expression', arguments: {} }, - ], + ast: { + type: 'expression', + chain: [ + { type: 'function', function: 'my', arguments: {} }, + { type: 'function', function: 'expression', arguments: {} }, + ], + }, + errors: undefined, }), }, input @@ -405,11 +456,14 @@ describe('embeddable', () => { getTrigger, documentToExpression: () => Promise.resolve({ - type: 'expression', - chain: [ - { type: 'function', function: 'my', arguments: {} }, - { type: 'function', function: 'expression', arguments: {} }, - ], + ast: { + type: 'expression', + chain: [ + { type: 'function', function: 'my', arguments: {} }, + { type: 'function', function: 'expression', arguments: {} }, + ], + }, + errors: undefined, }), }, input @@ -440,11 +494,14 @@ describe('embeddable', () => { getTrigger, documentToExpression: () => Promise.resolve({ - type: 'expression', - chain: [ - { type: 'function', function: 'my', arguments: {} }, - { type: 'function', function: 'expression', arguments: {} }, - ], + ast: { + type: 'expression', + chain: [ + { type: 'function', function: 'my', arguments: {} }, + { type: 'function', function: 'expression', arguments: {} }, + ], + }, + errors: undefined, }), }, { id: '123' } as LensEmbeddableInput @@ -475,11 +532,14 @@ describe('embeddable', () => { getTrigger, documentToExpression: () => Promise.resolve({ - type: 'expression', - chain: [ - { type: 'function', function: 'my', arguments: {} }, - { type: 'function', function: 'expression', arguments: {} }, - ], + ast: { + type: 'expression', + chain: [ + { type: 'function', function: 'my', arguments: {} }, + { type: 'function', function: 'expression', arguments: {} }, + ], + }, + errors: undefined, }), }, { id: '123' } as LensEmbeddableInput @@ -510,11 +570,14 @@ describe('embeddable', () => { getTrigger, documentToExpression: () => Promise.resolve({ - type: 'expression', - chain: [ - { type: 'function', function: 'my', arguments: {} }, - { type: 'function', function: 'expression', arguments: {} }, - ], + ast: { + type: 'expression', + chain: [ + { type: 'function', function: 'my', arguments: {} }, + { type: 'function', function: 'expression', arguments: {} }, + ], + }, + errors: undefined, }), }, { id: '123', timeRange, query, filters } as LensEmbeddableInput diff --git a/x-pack/plugins/lens/public/editor_frame_service/embeddable/embeddable.tsx b/x-pack/plugins/lens/public/editor_frame_service/embeddable/embeddable.tsx index dc5f9b366e6b5..ef265881f6eb3 100644 --- a/x-pack/plugins/lens/public/editor_frame_service/embeddable/embeddable.tsx +++ b/x-pack/plugins/lens/public/editor_frame_service/embeddable/embeddable.tsx @@ -51,6 +51,7 @@ import { IndexPatternsContract } from '../../../../../../src/plugins/data/public import { getEditPath, DOC_TYPE } from '../../../common'; import { IBasePath } from '../../../../../../src/core/public'; import { LensAttributeService } from '../../lens_attribute_service'; +import type { ErrorMessage } from '../types'; export type LensSavedObjectAttributes = Omit; @@ -77,7 +78,9 @@ export interface LensEmbeddableOutput extends EmbeddableOutput { export interface LensEmbeddableDeps { attributeService: LensAttributeService; - documentToExpression: (doc: Document) => Promise; + documentToExpression: ( + doc: Document + ) => Promise<{ ast: Ast | null; errors: ErrorMessage[] | undefined }>; editable: boolean; indexPatternService: IndexPatternsContract; expressionRenderer: ReactExpressionRendererType; @@ -99,6 +102,7 @@ export class Embeddable private subscription: Subscription; private isInitialized = false; private activeData: Partial | undefined; + private errors: ErrorMessage[] | undefined; private externalSearchContext: { timeRange?: TimeRange; @@ -225,8 +229,9 @@ export class Embeddable type: this.type, savedObjectId: (input as LensByReferenceInput)?.savedObjectId, }; - const expression = await this.deps.documentToExpression(this.savedVis); - this.expression = expression ? toExpression(expression) : null; + const { ast, errors } = await this.deps.documentToExpression(this.savedVis); + this.errors = errors; + this.expression = ast ? toExpression(ast) : null; await this.initializeOutput(); this.isInitialized = true; } @@ -279,6 +284,7 @@ export class Embeddable Promise; + documentToExpression: ( + doc: Document + ) => Promise<{ ast: Ast | null; errors: ErrorMessage[] | undefined }>; } export class EmbeddableFactory implements EmbeddableFactoryDefinition { diff --git a/x-pack/plugins/lens/public/editor_frame_service/embeddable/expression_wrapper.tsx b/x-pack/plugins/lens/public/editor_frame_service/embeddable/expression_wrapper.tsx index 8873388633552..a559e6a02419d 100644 --- a/x-pack/plugins/lens/public/editor_frame_service/embeddable/expression_wrapper.tsx +++ b/x-pack/plugins/lens/public/editor_frame_service/embeddable/expression_wrapper.tsx @@ -8,7 +8,7 @@ import React from 'react'; import { I18nProvider } from '@kbn/i18n/react'; import { FormattedMessage } from '@kbn/i18n/react'; -import { EuiFlexGroup, EuiFlexItem, EuiText, EuiIcon } from '@elastic/eui'; +import { EuiFlexGroup, EuiFlexItem, EuiText, EuiIcon, EuiEmptyPrompt } from '@elastic/eui'; import { ExpressionRendererEvent, ReactExpressionRendererType, @@ -18,10 +18,12 @@ import { ExecutionContextSearch } from 'src/plugins/data/public'; import { DefaultInspectorAdapters, RenderMode } from 'src/plugins/expressions'; import classNames from 'classnames'; import { getOriginalRequestErrorMessage } from '../error_helper'; +import { ErrorMessage } from '../types'; export interface ExpressionWrapperProps { ExpressionRenderer: ReactExpressionRendererType; expression: string | null; + errors: ErrorMessage[] | undefined; variables?: Record; searchContext: ExecutionContextSearch; searchSessionId?: string; @@ -37,6 +39,46 @@ export interface ExpressionWrapperProps { className?: string; } +interface VisualizationErrorProps { + errors: ExpressionWrapperProps['errors']; +} + +export function VisualizationErrorPanel({ errors }: VisualizationErrorProps) { + return ( +
+ + {errors ? ( + <> +

{errors[0].longMessage}

+ {errors.length > 1 ? ( +

+ +

+ ) : null} + + ) : ( +

+ +

+ )} + + } + /> +
+ ); +} + export function ExpressionWrapper({ ExpressionRenderer: ExpressionRendererComponent, expression, @@ -50,23 +92,12 @@ export function ExpressionWrapper({ hasCompatibleActions, style, className, + errors, }: ExpressionWrapperProps) { return ( - {expression === null || expression === '' ? ( - - - - - - - - - - + {errors || expression === null || expression === '' ? ( + ) : (
{ setDimension: jest.fn(), removeDimension: jest.fn(), - getErrorMessages: jest.fn((_state, _frame) => undefined), + getErrorMessages: jest.fn((_state) => undefined), }; } diff --git a/x-pack/plugins/lens/public/editor_frame_service/service.tsx b/x-pack/plugins/lens/public/editor_frame_service/service.tsx index 9e54a4d630dc2..8769aceca3bfd 100644 --- a/x-pack/plugins/lens/public/editor_frame_service/service.tsx +++ b/x-pack/plugins/lens/public/editor_frame_service/service.tsx @@ -72,7 +72,7 @@ export class EditorFrameService { * This is an asynchronous process and should only be triggered once for a saved object. * @param doc parsed Lens saved object */ - private async documentToExpression(doc: Document) { + private documentToExpression = async (doc: Document) => { const [resolvedDatasources, resolvedVisualizations] = await Promise.all([ collectAsyncDefinitions(this.datasources), collectAsyncDefinitions(this.visualizations), @@ -81,7 +81,7 @@ export class EditorFrameService { const { persistedStateToExpression } = await import('../async_services'); return await persistedStateToExpression(resolvedDatasources, resolvedVisualizations, doc); - } + }; public setup( core: CoreSetup, @@ -98,7 +98,7 @@ export class EditorFrameService { coreHttp: coreStart.http, timefilter: deps.data.query.timefilter.timefilter, expressionRenderer: deps.expressions.ReactExpressionRenderer, - documentToExpression: this.documentToExpression.bind(this), + documentToExpression: this.documentToExpression, indexPatternService: deps.data.indexPatterns, uiActions: deps.uiActions, }; diff --git a/x-pack/plugins/lens/public/editor_frame_service/types.ts b/x-pack/plugins/lens/public/editor_frame_service/types.ts index dc5a4aa0e234b..6043e96343899 100644 --- a/x-pack/plugins/lens/public/editor_frame_service/types.ts +++ b/x-pack/plugins/lens/public/editor_frame_service/types.ts @@ -8,3 +8,8 @@ import { Datatable } from 'src/plugins/expressions'; export type TableInspectorAdapter = Record; + +export interface ErrorMessage { + shortMessage: string; + longMessage: string; +} diff --git a/x-pack/plugins/lens/public/metric_visualization/visualization.test.ts b/x-pack/plugins/lens/public/metric_visualization/visualization.test.ts index 84abc38bf4106..66e524435ebc8 100644 --- a/x-pack/plugins/lens/public/metric_visualization/visualization.test.ts +++ b/x-pack/plugins/lens/public/metric_visualization/visualization.test.ts @@ -197,23 +197,7 @@ describe('metric_visualization', () => { describe('#getErrorMessages', () => { it('returns undefined if no error is raised', () => { - const datasource: DatasourcePublicAPI = { - ...createMockDatasource('l1').publicAPIMock, - getOperationForColumnId(_: string) { - return { - id: 'a', - dataType: 'number', - isBucketed: false, - label: 'shazm', - }; - }, - }; - const frame = { - ...mockFrame(), - datasourceLayers: { l1: datasource }, - }; - - const error = metricVisualization.getErrorMessages(exampleState(), frame); + const error = metricVisualization.getErrorMessages(exampleState()); expect(error).not.toBeDefined(); }); diff --git a/x-pack/plugins/lens/public/metric_visualization/visualization.tsx b/x-pack/plugins/lens/public/metric_visualization/visualization.tsx index b86ba71083440..91516b7b7319b 100644 --- a/x-pack/plugins/lens/public/metric_visualization/visualization.tsx +++ b/x-pack/plugins/lens/public/metric_visualization/visualization.tsx @@ -117,7 +117,7 @@ export const metricVisualization: Visualization = { return { ...prevState, accessor: undefined }; }, - getErrorMessages(state, frame) { + getErrorMessages(state) { // Is it possible to break it? return undefined; }, diff --git a/x-pack/plugins/lens/public/pie_visualization/toolbar.tsx b/x-pack/plugins/lens/public/pie_visualization/toolbar.tsx index 5ec97e90e57d9..e3bd54032a93c 100644 --- a/x-pack/plugins/lens/public/pie_visualization/toolbar.tsx +++ b/x-pack/plugins/lens/public/pie_visualization/toolbar.tsx @@ -17,6 +17,7 @@ import { EuiHorizontalRule, } from '@elastic/eui'; import { Position } from '@elastic/charts'; +import { PaletteRegistry } from 'src/plugins/charts/public'; import { DEFAULT_PERCENT_DECIMALS } from './constants'; import { PieVisualizationState, SharedPieLayerState } from './types'; import { VisualizationDimensionEditorProps, VisualizationToolbarProps } from '../types'; @@ -250,11 +251,15 @@ const DecimalPlaceSlider = ({ ); }; -export function DimensionEditor(props: VisualizationDimensionEditorProps) { +export function DimensionEditor( + props: VisualizationDimensionEditorProps & { + paletteService: PaletteRegistry; + } +) { return ( <> { props.setState({ ...props.state, palette: newPalette }); diff --git a/x-pack/plugins/lens/public/pie_visualization/visualization.test.ts b/x-pack/plugins/lens/public/pie_visualization/visualization.test.ts index 52fd4daac63c5..0cdeaa8c043d8 100644 --- a/x-pack/plugins/lens/public/pie_visualization/visualization.test.ts +++ b/x-pack/plugins/lens/public/pie_visualization/visualization.test.ts @@ -7,8 +7,6 @@ import { getPieVisualization } from './visualization'; import { PieVisualizationState } from './types'; -import { createMockDatasource, createMockFramePublicAPI } from '../editor_frame_service/mocks'; -import { DatasourcePublicAPI, FramePublicAPI } from '../types'; import { chartPluginMock } from '../../../../../src/plugins/charts/public/mocks'; jest.mock('../id_generator'); @@ -36,37 +34,11 @@ function exampleState(): PieVisualizationState { }; } -function mockFrame(): FramePublicAPI { - return { - ...createMockFramePublicAPI(), - addNewLayer: () => LAYER_ID, - datasourceLayers: { - [LAYER_ID]: createMockDatasource(LAYER_ID).publicAPIMock, - }, - }; -} - // Just a basic bootstrap here to kickstart the tests describe('pie_visualization', () => { describe('#getErrorMessages', () => { it('returns undefined if no error is raised', () => { - const datasource: DatasourcePublicAPI = { - ...createMockDatasource('l1').publicAPIMock, - getOperationForColumnId(_: string) { - return { - id: 'a', - dataType: 'number', - isBucketed: false, - label: 'shazm', - }; - }, - }; - const frame = { - ...mockFrame(), - datasourceLayers: { l1: datasource }, - }; - - const error = pieVisualization.getErrorMessages(exampleState(), frame); + const error = pieVisualization.getErrorMessages(exampleState()); expect(error).not.toBeDefined(); }); diff --git a/x-pack/plugins/lens/public/pie_visualization/visualization.tsx b/x-pack/plugins/lens/public/pie_visualization/visualization.tsx index 6408d7496d332..683acc49859b6 100644 --- a/x-pack/plugins/lens/public/pie_visualization/visualization.tsx +++ b/x-pack/plugins/lens/public/pie_visualization/visualization.tsx @@ -227,7 +227,7 @@ export const getPieVisualization = ({ renderDimensionEditor(domElement, props) { render( - + , domElement ); @@ -274,7 +274,7 @@ export const getPieVisualization = ({ )); }, - getErrorMessages(state, frame) { + getErrorMessages(state) { // not possible to break it? return undefined; }, diff --git a/x-pack/plugins/lens/public/types.ts b/x-pack/plugins/lens/public/types.ts index cccc35acb3fca..ba02a3376bae7 100644 --- a/x-pack/plugins/lens/public/types.ts +++ b/x-pack/plugins/lens/public/types.ts @@ -358,7 +358,7 @@ export interface LensMultiTable { export interface VisualizationConfigProps { layerId: string; - frame: FramePublicAPI; + frame: Pick; state: T; } @@ -631,10 +631,7 @@ export interface Visualization { * The frame will call this function on all visualizations at few stages (pre-build/build error) in order * to provide more context to the error and show it to the user */ - getErrorMessages: ( - state: T, - frame: FramePublicAPI - ) => Array<{ shortMessage: string; longMessage: string }> | undefined; + getErrorMessages: (state: T) => Array<{ shortMessage: string; longMessage: string }> | undefined; /** * The frame calls this function to display warnings about visualization diff --git a/x-pack/plugins/lens/public/visualization_container.scss b/x-pack/plugins/lens/public/visualization_container.scss index a67aa50127c81..d40a0b48ab40e 100644 --- a/x-pack/plugins/lens/public/visualization_container.scss +++ b/x-pack/plugins/lens/public/visualization_container.scss @@ -15,3 +15,11 @@ position: static; // Let the progress indicator position itself against the outer parent } } + +.lnsEmbeddedError { + flex-grow: 1; + display: flex; + align-items: center; + justify-content: center; + overflow: auto; +} diff --git a/x-pack/plugins/lens/public/xy_visualization/color_assignment.ts b/x-pack/plugins/lens/public/xy_visualization/color_assignment.ts index 27cc16ebf862b..d2e87ece5b5ec 100644 --- a/x-pack/plugins/lens/public/xy_visualization/color_assignment.ts +++ b/x-pack/plugins/lens/public/xy_visualization/color_assignment.ts @@ -95,7 +95,7 @@ export function getColorAssignments( export function getAccessorColorConfig( colorAssignments: ColorAssignments, - frame: FramePublicAPI, + frame: Pick, layer: XYLayerConfig, paletteService: PaletteRegistry ): AccessorConfig[] { diff --git a/x-pack/plugins/lens/public/xy_visualization/visualization.test.ts b/x-pack/plugins/lens/public/xy_visualization/visualization.test.ts index cdb7f452cf7cf..c244fa7fdfc89 100644 --- a/x-pack/plugins/lens/public/xy_visualization/visualization.test.ts +++ b/x-pack/plugins/lens/public/xy_visualization/visualization.test.ts @@ -589,137 +589,119 @@ describe('xy_visualization', () => { describe('#getErrorMessages', () => { it("should not return an error when there's only one dimension (X or Y)", () => { expect( - xyVisualization.getErrorMessages( - { - ...exampleState(), - layers: [ - { - layerId: 'first', - seriesType: 'area', - xAccessor: 'a', - accessors: [], - }, - ], - }, - createMockFramePublicAPI() - ) + xyVisualization.getErrorMessages({ + ...exampleState(), + layers: [ + { + layerId: 'first', + seriesType: 'area', + xAccessor: 'a', + accessors: [], + }, + ], + }) ).not.toBeDefined(); }); it("should not return an error when there's only one dimension on multiple layers (same axis everywhere)", () => { expect( - xyVisualization.getErrorMessages( - { - ...exampleState(), - layers: [ - { - layerId: 'first', - seriesType: 'area', - xAccessor: 'a', - accessors: [], - }, - { - layerId: 'second', - seriesType: 'area', - xAccessor: 'a', - accessors: [], - }, - ], - }, - createMockFramePublicAPI() - ) + xyVisualization.getErrorMessages({ + ...exampleState(), + layers: [ + { + layerId: 'first', + seriesType: 'area', + xAccessor: 'a', + accessors: [], + }, + { + layerId: 'second', + seriesType: 'area', + xAccessor: 'a', + accessors: [], + }, + ], + }) ).not.toBeDefined(); }); it('should not return an error when mixing different valid configurations in multiple layers', () => { expect( - xyVisualization.getErrorMessages( - { - ...exampleState(), - layers: [ - { - layerId: 'first', - seriesType: 'area', - xAccessor: 'a', - accessors: ['a'], - }, - { - layerId: 'second', - seriesType: 'area', - xAccessor: undefined, - accessors: ['a'], - splitAccessor: 'a', - }, - ], - }, - createMockFramePublicAPI() - ) + xyVisualization.getErrorMessages({ + ...exampleState(), + layers: [ + { + layerId: 'first', + seriesType: 'area', + xAccessor: 'a', + accessors: ['a'], + }, + { + layerId: 'second', + seriesType: 'area', + xAccessor: undefined, + accessors: ['a'], + splitAccessor: 'a', + }, + ], + }) ).not.toBeDefined(); }); it("should not return an error when there's only one splitAccessor dimension configured", () => { expect( - xyVisualization.getErrorMessages( - { - ...exampleState(), - layers: [ - { - layerId: 'first', - seriesType: 'area', - xAccessor: undefined, - accessors: [], - splitAccessor: 'a', - }, - ], - }, - createMockFramePublicAPI() - ) + xyVisualization.getErrorMessages({ + ...exampleState(), + layers: [ + { + layerId: 'first', + seriesType: 'area', + xAccessor: undefined, + accessors: [], + splitAccessor: 'a', + }, + ], + }) ).not.toBeDefined(); expect( - xyVisualization.getErrorMessages( - { - ...exampleState(), - layers: [ - { - layerId: 'first', - seriesType: 'area', - xAccessor: undefined, - accessors: [], - splitAccessor: 'a', - }, - { - layerId: 'second', - seriesType: 'area', - xAccessor: undefined, - accessors: [], - splitAccessor: 'a', - }, - ], - }, - createMockFramePublicAPI() - ) + xyVisualization.getErrorMessages({ + ...exampleState(), + layers: [ + { + layerId: 'first', + seriesType: 'area', + xAccessor: undefined, + accessors: [], + splitAccessor: 'a', + }, + { + layerId: 'second', + seriesType: 'area', + xAccessor: undefined, + accessors: [], + splitAccessor: 'a', + }, + ], + }) ).not.toBeDefined(); }); it('should return an error when there are multiple layers, one axis configured for each layer (but different axis from each other)', () => { expect( - xyVisualization.getErrorMessages( - { - ...exampleState(), - layers: [ - { - layerId: 'first', - seriesType: 'area', - xAccessor: 'a', - accessors: [], - }, - { - layerId: 'second', - seriesType: 'area', - xAccessor: undefined, - accessors: ['a'], - }, - ], - }, - createMockFramePublicAPI() - ) + xyVisualization.getErrorMessages({ + ...exampleState(), + layers: [ + { + layerId: 'first', + seriesType: 'area', + xAccessor: 'a', + accessors: [], + }, + { + layerId: 'second', + seriesType: 'area', + xAccessor: undefined, + accessors: ['a'], + }, + ], + }) ).toEqual([ { shortMessage: 'Missing Vertical axis.', @@ -729,34 +711,31 @@ describe('xy_visualization', () => { }); it('should return an error with batched messages for the same error with multiple layers', () => { expect( - xyVisualization.getErrorMessages( - { - ...exampleState(), - layers: [ - { - layerId: 'first', - seriesType: 'area', - xAccessor: 'a', - accessors: ['a'], - }, - { - layerId: 'second', - seriesType: 'area', - xAccessor: undefined, - accessors: [], - splitAccessor: 'a', - }, - { - layerId: 'third', - seriesType: 'area', - xAccessor: undefined, - accessors: [], - splitAccessor: 'a', - }, - ], - }, - createMockFramePublicAPI() - ) + xyVisualization.getErrorMessages({ + ...exampleState(), + layers: [ + { + layerId: 'first', + seriesType: 'area', + xAccessor: 'a', + accessors: ['a'], + }, + { + layerId: 'second', + seriesType: 'area', + xAccessor: undefined, + accessors: [], + splitAccessor: 'a', + }, + { + layerId: 'third', + seriesType: 'area', + xAccessor: undefined, + accessors: [], + splitAccessor: 'a', + }, + ], + }) ).toEqual([ { shortMessage: 'Missing Vertical axis.', @@ -766,32 +745,29 @@ describe('xy_visualization', () => { }); it("should return an error when some layers are complete but other layers aren't", () => { expect( - xyVisualization.getErrorMessages( - { - ...exampleState(), - layers: [ - { - layerId: 'first', - seriesType: 'area', - xAccessor: 'a', - accessors: [], - }, - { - layerId: 'second', - seriesType: 'area', - xAccessor: 'a', - accessors: ['a'], - }, - { - layerId: 'third', - seriesType: 'area', - xAccessor: 'a', - accessors: ['a'], - }, - ], - }, - createMockFramePublicAPI() - ) + xyVisualization.getErrorMessages({ + ...exampleState(), + layers: [ + { + layerId: 'first', + seriesType: 'area', + xAccessor: 'a', + accessors: [], + }, + { + layerId: 'second', + seriesType: 'area', + xAccessor: 'a', + accessors: ['a'], + }, + { + layerId: 'third', + seriesType: 'area', + xAccessor: 'a', + accessors: ['a'], + }, + ], + }) ).toEqual([ { shortMessage: 'Missing Vertical axis.', diff --git a/x-pack/plugins/lens/public/xy_visualization/visualization.tsx b/x-pack/plugins/lens/public/xy_visualization/visualization.tsx index a4dc7a91822bd..1ee4b2e050f3e 100644 --- a/x-pack/plugins/lens/public/xy_visualization/visualization.tsx +++ b/x-pack/plugins/lens/public/xy_visualization/visualization.tsx @@ -340,7 +340,7 @@ export const getXyVisualization = ({ toExpression(state, layers, paletteService, attributes), toPreviewExpression: (state, layers) => toPreviewExpression(state, layers, paletteService), - getErrorMessages(state, frame) { + getErrorMessages(state) { // Data error handling below here const hasNoAccessors = ({ accessors }: XYLayerConfig) => accessors == null || accessors.length === 0; diff --git a/x-pack/plugins/lens/public/xy_visualization/xy_config_panel.tsx b/x-pack/plugins/lens/public/xy_visualization/xy_config_panel.tsx index dd5fff3c49f4f..14733ad0f613b 100644 --- a/x-pack/plugins/lens/public/xy_visualization/xy_config_panel.tsx +++ b/x-pack/plugins/lens/public/xy_visualization/xy_config_panel.tsx @@ -508,7 +508,7 @@ export function DimensionEditor( return ( <> { setState(updateLayer(state, { ...layer, palette: newPalette }, index)); diff --git a/x-pack/test/functional/apps/lens/dashboard.ts b/x-pack/test/functional/apps/lens/dashboard.ts index 5cbd5dff45e1e..0d2db53ba73af 100644 --- a/x-pack/test/functional/apps/lens/dashboard.ts +++ b/x-pack/test/functional/apps/lens/dashboard.ts @@ -194,5 +194,39 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) { await dashboardAddPanel.filterEmbeddableNames('lnsPieVis'); await find.existsByLinkText('lnsPieVis'); }); + + it('should show validation messages if any error appears', async () => { + await PageObjects.common.navigateToApp('dashboard'); + await PageObjects.dashboard.clickNewDashboard(); + + await dashboardAddPanel.clickCreateNewLink(); + await dashboardAddPanel.clickVisType('lens'); + await PageObjects.header.waitUntilLoadingHasFinished(); + await PageObjects.lens.goToTimeRange(); + + await PageObjects.lens.configureDimension({ + dimension: 'lnsXY_xDimensionPanel > lns-empty-dimension', + operation: 'date_histogram', + field: '@timestamp', + }); + + await PageObjects.lens.configureDimension({ + dimension: 'lnsXY_yDimensionPanel > lns-empty-dimension', + operation: 'moving_average', + keepOpen: true, + }); + await PageObjects.lens.configureReference({ + operation: 'sum', + field: 'bytes', + }); + await PageObjects.lens.closeDimensionEditor(); + + // remove the x dimension to trigger the validation error + await PageObjects.lens.removeDimension('lnsXY_xDimensionPanel'); + await PageObjects.lens.saveAndReturn(); + + await PageObjects.header.waitUntilLoadingHasFinished(); + await testSubjects.existOrFail('embeddable-lens-failure'); + }); }); }