From 1bed7c08644320025d63cba95993f4fb69c60798 Mon Sep 17 00:00:00 2001 From: dej611 Date: Tue, 20 Oct 2020 18:47:22 +0200 Subject: [PATCH 01/47] :sparkles: First pass with visualization validation + error messages --- .../datatable_visualization/visualization.tsx | 74 ++++++++++++++----- .../editor_frame/config_panel/layer_panel.tsx | 1 + .../workspace_panel/workspace_panel.tsx | 34 ++++++--- .../indexpattern_datasource/indexpattern.tsx | 5 ++ .../metric_visualization/visualization.tsx | 5 ++ .../pie_visualization/visualization.tsx | 5 ++ x-pack/plugins/lens/public/types.ts | 14 ++++ .../public/xy_visualization/visualization.tsx | 64 ++++++++++++++++ 8 files changed, 176 insertions(+), 26 deletions(-) diff --git a/x-pack/plugins/lens/public/datatable_visualization/visualization.tsx b/x-pack/plugins/lens/public/datatable_visualization/visualization.tsx index 1464ae6988a2d..67822fce7e979 100644 --- a/x-pack/plugins/lens/public/datatable_visualization/visualization.tsx +++ b/x-pack/plugins/lens/public/datatable_visualization/visualization.tsx @@ -6,7 +6,13 @@ import { Ast } from '@kbn/interpreter/common'; import { i18n } from '@kbn/i18n'; -import { SuggestionRequest, Visualization, VisualizationSuggestion, Operation } from '../types'; +import { + SuggestionRequest, + Visualization, + VisualizationSuggestion, + Operation, + DatasourcePublicAPI, +} from '../types'; import { LensIconChartDatatable } from '../assets/chart_datatable'; export interface LayerState { @@ -128,16 +134,13 @@ export const datatableVisualization: Visualization }, getConfiguration({ state, frame, layerId }) { - const layer = state.layers.find((l) => l.layerId === layerId); - if (!layer) { + const { sortedColumns, datasource } = + getDataSourceAndSortedColumns(state, frame.datasourceLayers, layerId) || {}; + + if (!sortedColumns) { return { groups: [] }; } - const datasource = frame.datasourceLayers[layer.layerId]; - const originalOrder = datasource.getTableSpec().map(({ columnId }) => columnId); - // When we add a column it could be empty, and therefore have no order - const sortedColumns = Array.from(new Set(originalOrder.concat(layer.columns))); - return { groups: [ { @@ -146,7 +149,9 @@ export const datatableVisualization: Visualization defaultMessage: 'Break down by', }), layerId: state.layers[0].layerId, - accessors: sortedColumns.filter((c) => datasource.getOperationForColumnId(c)?.isBucketed), + accessors: sortedColumns.filter( + (c) => datasource!.getOperationForColumnId(c)?.isBucketed + ), supportsMoreColumns: true, filterOperations: (op) => op.isBucketed, dataTestSubj: 'lnsDatatable_column', @@ -158,7 +163,7 @@ export const datatableVisualization: Visualization }), layerId: state.layers[0].layerId, accessors: sortedColumns.filter( - (c) => !datasource.getOperationForColumnId(c)?.isBucketed + (c) => !datasource!.getOperationForColumnId(c)?.isBucketed ), supportsMoreColumns: true, filterOperations: (op) => !op.isBucketed, @@ -195,13 +200,11 @@ export const datatableVisualization: Visualization }, toExpression(state, datasourceLayers, { title, description } = {}): Ast { - const layer = state.layers[0]; - const datasource = datasourceLayers[layer.layerId]; - const originalOrder = datasource.getTableSpec().map(({ columnId }) => columnId); - // When we add a column it could be empty, and therefore have no order - const sortedColumns = Array.from(new Set(originalOrder.concat(layer.columns))); - const operations = sortedColumns - .map((columnId) => ({ columnId, operation: datasource.getOperationForColumnId(columnId) })) + const { sortedColumns, datasource } = + getDataSourceAndSortedColumns(state, datasourceLayers, state.layers[0].layerId) || {}; + + const operations = sortedColumns! + .map((columnId) => ({ columnId, operation: datasource!.getOperationForColumnId(columnId) })) .filter((o): o is { columnId: string; operation: Operation } => !!o.operation); return { @@ -232,4 +235,41 @@ export const datatableVisualization: Visualization ], }; }, + + getErrorMessage(state, frame) { + const { sortedColumns, datasource } = + getDataSourceAndSortedColumns(state, frame.datasourceLayers, state.layers[0].layerId) || {}; + + // Data validation below + if ( + sortedColumns && + sortedColumns.filter((c) => !datasource!.getOperationForColumnId(c)?.isBucketed).length === 0 + ) { + return { + shortMessage: i18n.translate(`xpack.lens.datatable.dataFailureShort`, { + defaultMessage: `No metric dimension configured`, + }), + longMessage: i18n.translate(`xpack.lens.datatable.dataFailureLong`, { + defaultMessage: `Add a field to the metric dimension panel`, + }), + }; + } + return undefined; + }, }; + +function getDataSourceAndSortedColumns( + state: DatatableVisualizationState, + datasourceLayers: Record, + layerId: string +) { + const layer = state.layers.find((l: LayerState) => l.layerId === layerId); + if (!layer) { + return undefined; + } + const datasource = datasourceLayers[layer.layerId]; + const originalOrder = datasource.getTableSpec().map(({ columnId }) => columnId); + // When we add a column it could be empty, and therefore have no order + const sortedColumns = Array.from(new Set(originalOrder.concat(layer.columns))); + return { datasource, sortedColumns }; +} diff --git a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/layer_panel.tsx b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/layer_panel.tsx index e72bf75b010c3..b968aa4756454 100644 --- a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/layer_panel.tsx +++ b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/layer_panel.tsx @@ -409,6 +409,7 @@ export function LayerPanel( isNew: false, }); }, + dimensionGroups: [activeGroup], }} /> )} 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 2bbf183b7ae11..54858667b4b36 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 @@ -66,7 +66,7 @@ export interface WorkspacePanelProps { } interface WorkspaceState { - expressionBuildError: string | undefined; + expressionBuildError?: { shortMessage: string; longMessage: string }; expandError: boolean; } @@ -117,7 +117,7 @@ export function WorkspacePanel({ ); const [localState, setLocalState] = useState({ - expressionBuildError: undefined as string | undefined, + expressionBuildError: undefined, expandError: false, }); @@ -135,8 +135,13 @@ export function WorkspacePanel({ datasourceLayers: framePublicAPI.datasourceLayers, }); } catch (e) { + const message = activeVisualization?.getErrorMessage(visualizationState, framePublicAPI); + const defaultMessage = { + shortMessage: 'An error occurred in the expression', + longMessage: e.toString(), + }; // Most likely an error in the expression provided by a datasource or visualization - setLocalState((s) => ({ ...s, expressionBuildError: e.toString() })); + setLocalState((s) => ({ ...s, expressionBuildError: message || defaultMessage })); } }, // eslint-disable-next-line react-hooks/exhaustive-deps @@ -250,6 +255,8 @@ export function WorkspacePanel({ onEvent={onEvent} setLocalState={setLocalState} localState={localState} + visualizationState={visualizationState} + visualization={activeVisualization} ExpressionRendererComponent={ExpressionRendererComponent} /> ); @@ -291,6 +298,8 @@ export const InnerVisualizationWrapper = ({ setLocalState, localState, ExpressionRendererComponent, + visualizationState, + visualization, }: { expression: Ast | null | undefined; framePublicAPI: FramePublicAPI; @@ -299,6 +308,8 @@ export const InnerVisualizationWrapper = ({ setLocalState: (dispatch: (prevState: WorkspaceState) => WorkspaceState) => void; localState: WorkspaceState; ExpressionRendererComponent: ReactExpressionRendererType; + visualizationState: unknown; + visualization: Visualization | null; }) => { const autoRefreshFetch$ = useMemo(() => timefilter.getAutoRefreshFetch$(), [timefilter]); @@ -331,7 +342,7 @@ export const InnerVisualizationWrapper = ({ defaultMessage="An error occurred in the expression" /> - {localState.expressionBuildError} + {localState.expressionBuildError.longMessage} ); } @@ -346,16 +357,21 @@ export const InnerVisualizationWrapper = ({ onEvent={onEvent} renderError={(errorMessage?: string | null, error?: ExpressionRenderError | null) => { const visibleErrorMessage = getOriginalRequestErrorMessage(error) || errorMessage; + + const { shortMessage, longMessage } = + visualization?.getErrorMessage(visualizationState, framePublicAPI) || {}; return ( - + {shortMessage ?? ( + + )} {visibleErrorMessage ? ( @@ -372,7 +388,7 @@ export const InnerVisualizationWrapper = ({ })} - {localState.expandError ? visibleErrorMessage : null} + {localState.expandError ? longMessage || visibleErrorMessage : null} ) : null} diff --git a/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.tsx b/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.tsx index 28aeac223e4a6..3e3bdb5c3c1cd 100644 --- a/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.tsx +++ b/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.tsx @@ -340,6 +340,11 @@ export function getIndexPatternDatasource({ }, getDatasourceSuggestionsFromCurrentState, getDatasourceSuggestionsForVisualizeField, + + getErrorMessage(state, dimensionGroups) { + // console.log('datasource.getErrorMessage', { state, dimensionGroups }); + return undefined; + }, }; return indexPatternDatasource; diff --git a/x-pack/plugins/lens/public/metric_visualization/visualization.tsx b/x-pack/plugins/lens/public/metric_visualization/visualization.tsx index 77d189ce53d01..bd0c796d1d01d 100644 --- a/x-pack/plugins/lens/public/metric_visualization/visualization.tsx +++ b/x-pack/plugins/lens/public/metric_visualization/visualization.tsx @@ -115,4 +115,9 @@ export const metricVisualization: Visualization = { removeDimension({ prevState }) { return { ...prevState, accessor: undefined }; }, + + getErrorMessage(state, frame) { + // Is it possible to break it? + return undefined; + }, }; diff --git a/x-pack/plugins/lens/public/pie_visualization/visualization.tsx b/x-pack/plugins/lens/public/pie_visualization/visualization.tsx index dd1b36e00ebb9..c72e3ae733c02 100644 --- a/x-pack/plugins/lens/public/pie_visualization/visualization.tsx +++ b/x-pack/plugins/lens/public/pie_visualization/visualization.tsx @@ -214,4 +214,9 @@ export const pieVisualization: Visualization = { domElement ); }, + + getErrorMessage(state, frame) { + // 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 2b9ca5a2425f8..b4a7c0ee408a6 100644 --- a/x-pack/plugins/lens/public/types.ts +++ b/x-pack/plugins/lens/public/types.ts @@ -180,6 +180,11 @@ export interface Datasource { getDatasourceSuggestionsFromCurrentState: (state: T) => Array>; getPublicAPI: (props: PublicAPIProps) => DatasourcePublicAPI; + + getErrorMessage: ( + state: T, + dimensionGroups: VisualizationDimensionGroupConfig[] + ) => { shortMessage: string; longMessage: string } | undefined; } /** @@ -232,6 +237,7 @@ export type DatasourceDimensionEditorProps = DatasourceDimensionPro setState: StateSetter; core: Pick; dateRange: DateRange; + dimensionGroups: VisualizationDimensionGroupConfig[]; }; export type DatasourceDimensionTriggerProps = DatasourceDimensionProps & { @@ -557,6 +563,14 @@ export interface Visualization { state: T, datasourceLayers: Record ) => Ast | string | null; + /** + * The frame will call this function on all visualizations at error time in order + * to provide more context to the error and show it to the user + */ + getErrorMessage: ( + state: T, + frame: FramePublicAPI + ) => { shortMessage: string; longMessage: string } | undefined; } export interface LensFilterEvent { diff --git a/x-pack/plugins/lens/public/xy_visualization/visualization.tsx b/x-pack/plugins/lens/public/xy_visualization/visualization.tsx index 76c5a51cb7168..e891bdc25a13e 100644 --- a/x-pack/plugins/lens/public/xy_visualization/visualization.tsx +++ b/x-pack/plugins/lens/public/xy_visualization/visualization.tsx @@ -278,8 +278,72 @@ export const xyVisualization: Visualization = { toExpression, toPreviewExpression, + + getErrorMessage(state, frame) { + // Data error handling below here + const hasNoXAccessor = ({ xAccessor }: LayerConfig) => xAccessor == null; + const hasNoAccessors = ({ accessors }: LayerConfig) => + accessors == null || accessors.length === 0; + + // check if the layes in the state are compatible with this type of chart + if (state.layers.length > 1) { + const checks: Array<[string, (layer: LayerConfig) => boolean]> = [ + ['X', hasNoXAccessor], + ['Y', hasNoAccessors], + ]; + + // filter those layers with no accessors at all + const filteredLayers = state.layers.filter( + (layer) => !checks.every(([dimension, missingCriteria]) => missingCriteria(layer)) + ); + + for (const [dimension, criteria] of checks) { + const result = validateLayersForDimension(dimension, filteredLayers, criteria); + if (!result.valid) { + return result.payload; + } + } + } + return undefined; + }, }; +function validateLayersForDimension( + dimension: string, + layers: LayerConfig[], + missingCriteria: (layer: LayerConfig) => boolean +): { valid: true } | { valid: false; payload: { shortMessage: string; longMessage: string } } { + // Multiple layers must be consistent: + // * either a dimension is missing in ALL of them + // * or should not miss on any + if (layers.every(missingCriteria) || !layers.some(missingCriteria)) { + return { valid: true }; + } + // otherwise it's an error and it has to be reported + const layerMissingAccessors = layers.reduce((missing: number[], layer, i) => { + if (missingCriteria(layer)) { + missing.push(i); + } + return missing; + }, []); + return { + valid: false, + payload: { + shortMessage: i18n.translate(`xpack.lens.xyVisualization.dataFailure${dimension}Short`, { + defaultMessage: `Some layers are missing the ${dimension} dimension`, + }), + longMessage: i18n.translate(`xpack.lens.xyVisualization.dataFailure${dimension}Long`, { + defaultMessage: `{layers, plural, one {Layer} other {Layers}} ${layerMissingAccessors + .map((i: number) => i + 1) + .join( + ', ' + )} {layers, plural, one {has} other {have}} no dimension field set for the ${dimension} axis`, + values: { layers: layerMissingAccessors.length }, + }), + }, + }; +} + function newLayerState(seriesType: SeriesType, layerId: string): LayerConfig { return { layerId, From bb937013e6c7d0dd06bbf2d3347c3bdae4fab4ef Mon Sep 17 00:00:00 2001 From: dej611 Date: Wed, 21 Oct 2020 10:04:20 +0200 Subject: [PATCH 02/47] :fire: Remove indexpattern error handling for now --- .../editor_frame/config_panel/layer_panel.tsx | 1 - .../lens/public/indexpattern_datasource/indexpattern.tsx | 5 ----- x-pack/plugins/lens/public/types.ts | 6 ------ 3 files changed, 12 deletions(-) diff --git a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/layer_panel.tsx b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/layer_panel.tsx index b968aa4756454..e72bf75b010c3 100644 --- a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/layer_panel.tsx +++ b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/layer_panel.tsx @@ -409,7 +409,6 @@ export function LayerPanel( isNew: false, }); }, - dimensionGroups: [activeGroup], }} /> )} diff --git a/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.tsx b/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.tsx index 3e3bdb5c3c1cd..28aeac223e4a6 100644 --- a/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.tsx +++ b/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.tsx @@ -340,11 +340,6 @@ export function getIndexPatternDatasource({ }, getDatasourceSuggestionsFromCurrentState, getDatasourceSuggestionsForVisualizeField, - - getErrorMessage(state, dimensionGroups) { - // console.log('datasource.getErrorMessage', { state, dimensionGroups }); - return undefined; - }, }; return indexPatternDatasource; diff --git a/x-pack/plugins/lens/public/types.ts b/x-pack/plugins/lens/public/types.ts index b4a7c0ee408a6..1371efd1579ef 100644 --- a/x-pack/plugins/lens/public/types.ts +++ b/x-pack/plugins/lens/public/types.ts @@ -180,11 +180,6 @@ export interface Datasource { getDatasourceSuggestionsFromCurrentState: (state: T) => Array>; getPublicAPI: (props: PublicAPIProps) => DatasourcePublicAPI; - - getErrorMessage: ( - state: T, - dimensionGroups: VisualizationDimensionGroupConfig[] - ) => { shortMessage: string; longMessage: string } | undefined; } /** @@ -237,7 +232,6 @@ export type DatasourceDimensionEditorProps = DatasourceDimensionPro setState: StateSetter; core: Pick; dateRange: DateRange; - dimensionGroups: VisualizationDimensionGroupConfig[]; }; export type DatasourceDimensionTriggerProps = DatasourceDimensionProps & { From 0aea6c5b5f595df10da316d0e8e42300275a68c3 Mon Sep 17 00:00:00 2001 From: dej611 Date: Wed, 21 Oct 2020 10:32:30 +0200 Subject: [PATCH 03/47] :label: Fix type issues --- x-pack/plugins/lens/public/editor_frame_service/mocks.tsx | 1 + 1 file changed, 1 insertion(+) diff --git a/x-pack/plugins/lens/public/editor_frame_service/mocks.tsx b/x-pack/plugins/lens/public/editor_frame_service/mocks.tsx index 93898ef1d43a8..71d7b982c6fad 100644 --- a/x-pack/plugins/lens/public/editor_frame_service/mocks.tsx +++ b/x-pack/plugins/lens/public/editor_frame_service/mocks.tsx @@ -51,6 +51,7 @@ export function createMockVisualization(): jest.Mocked { setDimension: jest.fn(), removeDimension: jest.fn(), + getErrorMessage: jest.fn((_state, _frame) => undefined), }; } From d389d859af6fb58d60ec87f0a563766b84e78f53 Mon Sep 17 00:00:00 2001 From: dej611 Date: Wed, 21 Oct 2020 11:04:25 +0200 Subject: [PATCH 04/47] :white_check_mark: Add getErrorMessage test for data table --- .../visualization.test.tsx | 38 +++++++++++++++++++ 1 file changed, 38 insertions(+) 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 0db456e0760ec..45aaa758efad5 100644 --- a/x-pack/plugins/lens/public/datatable_visualization/visualization.test.tsx +++ b/x-pack/plugins/lens/public/datatable_visualization/visualization.test.tsx @@ -372,4 +372,42 @@ describe('Datatable Visualization', () => { }); }); }); + + describe('#getErrorMessage', () => { + it('returns an error explanation if the datasource is missing a metric dimension', () => { + const datasource = createMockDatasource('test'); + const layer = { layerId: 'a', columns: ['b', 'c'] }; + const frame = mockFrame(); + frame.datasourceLayers = { a: datasource.publicAPIMock }; + datasource.publicAPIMock.getTableSpec.mockReturnValue([{ columnId: 'c' }, { columnId: 'b' }]); + datasource.publicAPIMock.getOperationForColumnId.mockReturnValue({ + dataType: 'string', + isBucketed: true, // move it from the metric to the break down by side + label: 'label', + }); + + const error = datatableVisualization.getErrorMessage({ layers: [layer] }, frame); + + expect(error).toBeDefined(); + expect(error!.shortMessage).toMatch('No metric dimension configured'); + expect(error!.longMessage).toMatch('Add a field to the metric dimension panel'); + }); + + it('returns undefined if the metric dimension is defined', () => { + const datasource = createMockDatasource('test'); + const layer = { layerId: 'a', columns: ['b', 'c'] }; + const frame = mockFrame(); + frame.datasourceLayers = { a: datasource.publicAPIMock }; + datasource.publicAPIMock.getTableSpec.mockReturnValue([{ columnId: 'c' }, { columnId: 'b' }]); + datasource.publicAPIMock.getOperationForColumnId.mockReturnValue({ + dataType: 'string', + isBucketed: false, // keep it a metric + label: 'label', + }); + + const error = datatableVisualization.getErrorMessage({ layers: [layer] }, frame); + + expect(error).not.toBeDefined(); + }); + }); }); From 4c48b23fee6fe11f31e3b3b93d51f8309f30858f Mon Sep 17 00:00:00 2001 From: dej611 Date: Wed, 21 Oct 2020 18:10:40 +0200 Subject: [PATCH 05/47] :white_check_mark: Add tests for pie and metric error messages --- .../visualization.test.ts | 24 +++++++ .../pie_visualization/visualization.test.ts | 68 +++++++++++++++++++ 2 files changed, 92 insertions(+) create mode 100644 x-pack/plugins/lens/public/pie_visualization/visualization.test.ts 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 80c7a174b3264..d83f1c56d5ad3 100644 --- a/x-pack/plugins/lens/public/metric_visualization/visualization.test.ts +++ b/x-pack/plugins/lens/public/metric_visualization/visualization.test.ts @@ -193,4 +193,28 @@ describe('metric_visualization', () => { `); }); }); + + describe('#getErrorMessage', () => { + 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.getErrorMessage(exampleState(), frame); + + expect(error).not.toBeDefined(); + }); + }); }); diff --git a/x-pack/plugins/lens/public/pie_visualization/visualization.test.ts b/x-pack/plugins/lens/public/pie_visualization/visualization.test.ts new file mode 100644 index 0000000000000..a6e0df6e12e42 --- /dev/null +++ b/x-pack/plugins/lens/public/pie_visualization/visualization.test.ts @@ -0,0 +1,68 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +import { pieVisualization } from './visualization'; +import { PieVisualizationState } from './types'; +import { createMockDatasource, createMockFramePublicAPI } from '../editor_frame_service/mocks'; +import { DatasourcePublicAPI, FramePublicAPI } from '../types'; + +jest.mock('../id_generator'); + +const LAYER_ID = 'l1'; + +function exampleState(): PieVisualizationState { + return { + shape: 'pie', + layers: [ + { + layerId: LAYER_ID, + groups: [], + metric: undefined, + numberDisplay: 'percent', + categoryDisplay: 'default', + legendDisplay: 'default', + nestedLegend: false, + }, + ], + }; +} + +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('#getErrorMessage', () => { + 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.getErrorMessage(exampleState(), frame); + + expect(error).not.toBeDefined(); + }); + }); +}); From 91a700ab48916c95702140c3fa89891d97352ec1 Mon Sep 17 00:00:00 2001 From: dej611 Date: Thu, 22 Oct 2020 12:36:29 +0200 Subject: [PATCH 06/47] :globe_with_meridians: Fix i18n checks issues --- .../datatable_visualization/visualization.tsx | 4 +- .../workspace_panel/workspace_panel.tsx | 2 +- .../public/xy_visualization/visualization.tsx | 42 +++++++++++++------ 3 files changed, 32 insertions(+), 16 deletions(-) diff --git a/x-pack/plugins/lens/public/datatable_visualization/visualization.tsx b/x-pack/plugins/lens/public/datatable_visualization/visualization.tsx index 67822fce7e979..834be9823c131 100644 --- a/x-pack/plugins/lens/public/datatable_visualization/visualization.tsx +++ b/x-pack/plugins/lens/public/datatable_visualization/visualization.tsx @@ -246,10 +246,10 @@ export const datatableVisualization: Visualization sortedColumns.filter((c) => !datasource!.getOperationForColumnId(c)?.isBucketed).length === 0 ) { return { - shortMessage: i18n.translate(`xpack.lens.datatable.dataFailureShort`, { + shortMessage: i18n.translate('xpack.lens.datatable.dataFailureShort', { defaultMessage: `No metric dimension configured`, }), - longMessage: i18n.translate(`xpack.lens.datatable.dataFailureLong`, { + longMessage: i18n.translate('xpack.lens.datatable.dataFailureLong', { defaultMessage: `Add a field to the metric dimension panel`, }), }; 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 54858667b4b36..fe6573159506a 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 @@ -369,7 +369,7 @@ export const InnerVisualizationWrapper = ({ {shortMessage ?? ( )} diff --git a/x-pack/plugins/lens/public/xy_visualization/visualization.tsx b/x-pack/plugins/lens/public/xy_visualization/visualization.tsx index e891bdc25a13e..4927a24e4ff21 100644 --- a/x-pack/plugins/lens/public/xy_visualization/visualization.tsx +++ b/x-pack/plugins/lens/public/xy_visualization/visualization.tsx @@ -328,22 +328,38 @@ function validateLayersForDimension( }, []); return { valid: false, - payload: { - shortMessage: i18n.translate(`xpack.lens.xyVisualization.dataFailure${dimension}Short`, { - defaultMessage: `Some layers are missing the ${dimension} dimension`, - }), - longMessage: i18n.translate(`xpack.lens.xyVisualization.dataFailure${dimension}Long`, { - defaultMessage: `{layers, plural, one {Layer} other {Layers}} ${layerMissingAccessors - .map((i: number) => i + 1) - .join( - ', ' - )} {layers, plural, one {has} other {have}} no dimension field set for the ${dimension} axis`, - values: { layers: layerMissingAccessors.length }, - }), - }, + payload: getMessageIdsForDimension(dimension, layerMissingAccessors), }; } +// i18n ids cannot be dynamically generated, hence the function below +function getMessageIdsForDimension(dimension: string, layers: number[]) { + const layersList = layerMissingAccessors.map((i: number) => i + 1).join(', '); + switch (dimension) { + case 'X': + return { + shortMessage: i18n.translate('xpack.lens.xyVisualization.dataFailureXShort', { + defaultMessage: `Some layers are missing the X dimension`, + }), + longMessage: i18n.translate('xpack.lens.xyVisualization.dataFailureXLong', { + defaultMessage: `{layers, plural, one {Layer} other {Layers}} {layersList} {layers, plural, one {has} other {have}} no dimension field set for the X axis`, + values: { layers: layers.length, layersList }, + }), + }; + case 'Y': + return { + shortMessage: i18n.translate('xpack.lens.xyVisualization.dataFailureYShort', { + defaultMessage: `Some layers are missing the Y dimension`, + }), + longMessage: i18n.translate('xpack.lens.xyVisualization.dataFailureYLong', { + defaultMessage: `{layers, plural, one {Layer} other {Layers}} {layersList} {layers, plural, one {has} other {have}} no dimension field set for the Y axis`, + values: { layers: layers.length, layersList }, + }), + }; + } + return { shortMessage: '', longMessage: '' }; +} + function newLayerState(seriesType: SeriesType, layerId: string): LayerConfig { return { layerId, From b1d71dd9d05bd1b1bc9015f734e5ad6fcaad53b3 Mon Sep 17 00:00:00 2001 From: dej611 Date: Thu, 22 Oct 2020 14:13:43 +0200 Subject: [PATCH 07/47] :bug: Fix last issue --- x-pack/plugins/lens/public/xy_visualization/visualization.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugins/lens/public/xy_visualization/visualization.tsx b/x-pack/plugins/lens/public/xy_visualization/visualization.tsx index 4927a24e4ff21..2678a7e8781f7 100644 --- a/x-pack/plugins/lens/public/xy_visualization/visualization.tsx +++ b/x-pack/plugins/lens/public/xy_visualization/visualization.tsx @@ -334,7 +334,7 @@ function validateLayersForDimension( // i18n ids cannot be dynamically generated, hence the function below function getMessageIdsForDimension(dimension: string, layers: number[]) { - const layersList = layerMissingAccessors.map((i: number) => i + 1).join(', '); + const layersList = layers.map((i: number) => i + 1).join(', '); switch (dimension) { case 'X': return { From be2312d2e4b7a55c453da6cbb7b81c828e878d0f Mon Sep 17 00:00:00 2001 From: dej611 Date: Thu, 22 Oct 2020 18:53:10 +0200 Subject: [PATCH 08/47] :white_check_mark: Add more tests for the XY visualization validation code --- .../xy_visualization/visualization.test.ts | 138 ++++++++++++++++++ 1 file changed, 138 insertions(+) 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 d51b8c195c92c..ce289b43987ac 100644 --- a/x-pack/plugins/lens/public/xy_visualization/visualization.test.ts +++ b/x-pack/plugins/lens/public/xy_visualization/visualization.test.ts @@ -371,4 +371,142 @@ describe('xy_visualization', () => { expect(ops.filter(filterOperations).map((x) => x.dataType)).toEqual(['number']); }); }); + + describe('#getErrorMessage', () => { + it("should not return an error when there's only one dimension", () => { + expect( + xyVisualization.getErrorMessage( + { + ...exampleState(), + layers: [ + { + layerId: 'first', + seriesType: 'area', + xAccessor: 'a', + accessors: [], + }, + ], + }, + createMockFramePublicAPI() + ) + ).not.toBeDefined(); + }); + it("should not return an error when there's only one dimension on multiple layers (same axis everywhere)", () => { + expect( + xyVisualization.getErrorMessage( + { + ...exampleState(), + layers: [ + { + layerId: 'first', + seriesType: 'area', + xAccessor: 'a', + accessors: [], + }, + { + layerId: 'second', + seriesType: 'area', + xAccessor: 'a', + accessors: [], + }, + ], + }, + createMockFramePublicAPI() + ) + ).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.getErrorMessage( + { + ...exampleState(), + layers: [ + { + layerId: 'first', + seriesType: 'area', + xAccessor: 'a', + accessors: [], + }, + { + layerId: 'second', + seriesType: 'area', + xAccessor: undefined, + accessors: ['a'], + }, + ], + }, + createMockFramePublicAPI() + ) + ).toEqual({ + shortMessage: 'Some layers are missing the X dimension', + longMessage: 'Layer 2 has no dimension field set for the X axis', + }); + }); + it('should return an error with batched messages for the same error with multiple layers', () => { + expect( + xyVisualization.getErrorMessage( + { + ...exampleState(), + layers: [ + { + layerId: 'first', + seriesType: 'area', + xAccessor: 'a', + accessors: [], + }, + { + layerId: 'second', + seriesType: 'area', + xAccessor: undefined, + accessors: ['a'], + }, + { + layerId: 'third', + seriesType: 'area', + xAccessor: undefined, + accessors: ['a'], + }, + ], + }, + createMockFramePublicAPI() + ) + ).toEqual({ + shortMessage: 'Some layers are missing the X dimension', + longMessage: 'Layers 2, 3 have no dimension field set for the X axis', + }); + }); + it("should return an error when some layers are complete but other layers aren't", () => { + expect( + xyVisualization.getErrorMessage( + { + ...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() + ) + ).toEqual({ + shortMessage: 'Some layers are missing the Y dimension', + longMessage: 'Layer 1 has no dimension field set for the Y axis', + }); + }); + }); }); From c64603aa817fa292ae863592c0625bc152141a6f Mon Sep 17 00:00:00 2001 From: dej611 Date: Tue, 27 Oct 2020 18:49:44 +0100 Subject: [PATCH 09/47] :ok_hand: Included all feedback from first review --- .../visualization.test.tsx | 12 +- .../datatable_visualization/visualization.tsx | 41 ++++--- .../workspace_panel/workspace_panel.tsx | 103 ++++++++++++++---- .../public/editor_frame_service/mocks.tsx | 3 +- .../indexpattern_datasource/indexpattern.tsx | 28 ++++- .../public/indexpattern_datasource/utils.ts | 6 +- .../visualization.test.ts | 4 +- .../metric_visualization/visualization.tsx | 2 +- .../pie_visualization/visualization.test.ts | 4 +- .../pie_visualization/visualization.tsx | 2 +- x-pack/plugins/lens/public/types.ts | 5 +- .../xy_visualization/visualization.test.ts | 48 ++++---- .../public/xy_visualization/visualization.tsx | 28 +++-- 13 files changed, 202 insertions(+), 84 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 45aaa758efad5..c704e17b7ee54 100644 --- a/x-pack/plugins/lens/public/datatable_visualization/visualization.test.tsx +++ b/x-pack/plugins/lens/public/datatable_visualization/visualization.test.tsx @@ -373,7 +373,7 @@ describe('Datatable Visualization', () => { }); }); - describe('#getErrorMessage', () => { + describe('#getErrorMessages', () => { it('returns an error explanation if the datasource is missing a metric dimension', () => { const datasource = createMockDatasource('test'); const layer = { layerId: 'a', columns: ['b', 'c'] }; @@ -386,11 +386,11 @@ describe('Datatable Visualization', () => { label: 'label', }); - const error = datatableVisualization.getErrorMessage({ layers: [layer] }, frame); + const error = datatableVisualization.getErrorMessages({ layers: [layer] }, frame); - expect(error).toBeDefined(); - expect(error!.shortMessage).toMatch('No metric dimension configured'); - expect(error!.longMessage).toMatch('Add a field to the metric dimension panel'); + expect(error).toHaveLength(1); + expect(error![0].shortMessage).toMatch('Missing metric'); + expect(error![0].longMessage).toMatch('Add a field in the Metrics panel'); }); it('returns undefined if the metric dimension is defined', () => { @@ -405,7 +405,7 @@ describe('Datatable Visualization', () => { label: 'label', }); - const error = datatableVisualization.getErrorMessage({ layers: [layer] }, frame); + const error = datatableVisualization.getErrorMessages({ layers: [layer] }, frame); expect(error).not.toBeDefined(); }); diff --git a/x-pack/plugins/lens/public/datatable_visualization/visualization.tsx b/x-pack/plugins/lens/public/datatable_visualization/visualization.tsx index 834be9823c131..13229ce281859 100644 --- a/x-pack/plugins/lens/public/datatable_visualization/visualization.tsx +++ b/x-pack/plugins/lens/public/datatable_visualization/visualization.tsx @@ -236,25 +236,32 @@ export const datatableVisualization: Visualization }; }, - getErrorMessage(state, frame) { - const { sortedColumns, datasource } = - getDataSourceAndSortedColumns(state, frame.datasourceLayers, state.layers[0].layerId) || {}; + getErrorMessages(state, frame) { + const errors: Array<{ + shortMessage: string; + longMessage: string; + }> = []; + if (state) { + const { sortedColumns, datasource } = + getDataSourceAndSortedColumns(state, frame.datasourceLayers, state.layers[0].layerId) || {}; - // Data validation below - if ( - sortedColumns && - sortedColumns.filter((c) => !datasource!.getOperationForColumnId(c)?.isBucketed).length === 0 - ) { - return { - shortMessage: i18n.translate('xpack.lens.datatable.dataFailureShort', { - defaultMessage: `No metric dimension configured`, - }), - longMessage: i18n.translate('xpack.lens.datatable.dataFailureLong', { - defaultMessage: `Add a field to the metric dimension panel`, - }), - }; + // Data validation below + if ( + sortedColumns && + sortedColumns.filter((c) => !datasource!.getOperationForColumnId(c)?.isBucketed).length === + 0 + ) { + errors.push({ + shortMessage: i18n.translate('xpack.lens.datatable.dataFailureShort', { + defaultMessage: `Missing metric`, + }), + longMessage: i18n.translate('xpack.lens.datatable.dataFailureLong', { + defaultMessage: `Add a field in the Metrics panel`, + }), + }); + } } - return undefined; + return errors.length ? errors : undefined; }, }; 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 fe6573159506a..fe839deb7b832 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 @@ -9,7 +9,16 @@ import classNames from 'classnames'; import { FormattedMessage } from '@kbn/i18n/react'; import { Ast } from '@kbn/interpreter/common'; import { i18n } from '@kbn/i18n'; -import { EuiFlexGroup, EuiFlexItem, EuiIcon, EuiText, EuiButtonEmpty, EuiLink } from '@elastic/eui'; +import { + EuiFlexGroup, + EuiFlexItem, + EuiIcon, + EuiText, + EuiTextColor, + EuiButtonEmpty, + EuiLink, + EuiTitle, +} from '@elastic/eui'; import { CoreStart, CoreSetup } from 'kibana/public'; import { ExecutionContextSearch } from 'src/plugins/expressions'; import { @@ -66,7 +75,8 @@ export interface WorkspacePanelProps { } interface WorkspaceState { - expressionBuildError?: { shortMessage: string; longMessage: string }; + configurationValidationError?: Array<{ shortMessage: string; longMessage: string }>; + expressionBuildError?: Array<{ shortMessage: string; longMessage: string }>; expandError: boolean; } @@ -117,6 +127,7 @@ export function WorkspacePanel({ ); const [localState, setLocalState] = useState({ + configurationValidationError: undefined, expressionBuildError: undefined, expandError: false, }); @@ -124,8 +135,32 @@ export function WorkspacePanel({ const activeVisualization = activeVisualizationId ? visualizationMap[activeVisualizationId] : null; + const expression = useMemo( () => { + if (activeDatasourceId) { + const activeDatasource = activeDatasourceId ? datasourceMap[activeDatasourceId] : null; + const dataMessages = activeDatasource?.getErrorMessages( + datasourceStates[activeDatasourceId]?.state + ); + if (dataMessages) { + setLocalState((s) => ({ + ...s, + configurationValidationError: dataMessages, + })); + // TS will infer "void" if this is omitted + return; + } + } + const vizMessages = activeVisualization?.getErrorMessages(visualizationState, framePublicAPI); + if (vizMessages || localState.configurationValidationError) { + setLocalState((s) => ({ + ...s, + configurationValidationError: vizMessages, + })); + // TS will infer "void" if this is omitted + return; + } try { return buildExpression({ visualization: activeVisualization, @@ -135,13 +170,19 @@ export function WorkspacePanel({ datasourceLayers: framePublicAPI.datasourceLayers, }); } catch (e) { - const message = activeVisualization?.getErrorMessage(visualizationState, framePublicAPI); + const buildMessages = activeVisualization?.getErrorMessages( + visualizationState, + framePublicAPI + ); const defaultMessage = { - shortMessage: 'An error occurred in the expression', + shortMessage: 'One error occurred in the expression', longMessage: e.toString(), }; // Most likely an error in the expression provided by a datasource or visualization - setLocalState((s) => ({ ...s, expressionBuildError: message || defaultMessage })); + setLocalState((s) => ({ + ...s, + expressionBuildError: buildMessages ?? [defaultMessage], + })); } }, // eslint-disable-next-line react-hooks/exhaustive-deps @@ -255,8 +296,6 @@ export function WorkspacePanel({ onEvent={onEvent} setLocalState={setLocalState} localState={localState} - visualizationState={visualizationState} - visualization={activeVisualization} ExpressionRendererComponent={ExpressionRendererComponent} /> ); @@ -298,8 +337,6 @@ export const InnerVisualizationWrapper = ({ setLocalState, localState, ExpressionRendererComponent, - visualizationState, - visualization, }: { expression: Ast | null | undefined; framePublicAPI: FramePublicAPI; @@ -308,8 +345,6 @@ export const InnerVisualizationWrapper = ({ setLocalState: (dispatch: (prevState: WorkspaceState) => WorkspaceState) => void; localState: WorkspaceState; ExpressionRendererComponent: ReactExpressionRendererType; - visualizationState: unknown; - visualization: Visualization | null; }) => { const autoRefreshFetch$ = useMemo(() => timefilter.getAutoRefreshFetch$(), [timefilter]); @@ -330,6 +365,36 @@ export const InnerVisualizationWrapper = ({ ] ); + if (localState.configurationValidationError) { + return ( + + + + + + + + + + + + {localState.configurationValidationError[0].shortMessage} + + {localState.configurationValidationError[0].longMessage} + {localState.configurationValidationError.length > 1 + ? i18n.translate('xpack.lens.xyVisualization.dataFailureYLong', { + defaultMessage: ` + {errors} {errors, plural, one {error} other {errors}}`, + values: { errors: localState.configurationValidationError.length }, + }) + : null} + + + ); + } + if (localState.expressionBuildError) { return ( @@ -342,7 +407,7 @@ export const InnerVisualizationWrapper = ({ defaultMessage="An error occurred in the expression" /> - {localState.expressionBuildError.longMessage} + {localState.expressionBuildError[0].longMessage} ); } @@ -358,20 +423,16 @@ export const InnerVisualizationWrapper = ({ renderError={(errorMessage?: string | null, error?: ExpressionRenderError | null) => { const visibleErrorMessage = getOriginalRequestErrorMessage(error) || errorMessage; - const { shortMessage, longMessage } = - visualization?.getErrorMessage(visualizationState, framePublicAPI) || {}; return ( - {shortMessage ?? ( - - )} + {visibleErrorMessage ? ( @@ -388,7 +449,7 @@ export const InnerVisualizationWrapper = ({ })} - {localState.expandError ? longMessage || visibleErrorMessage : null} + {localState.expandError ? visibleErrorMessage : null} ) : null} diff --git a/x-pack/plugins/lens/public/editor_frame_service/mocks.tsx b/x-pack/plugins/lens/public/editor_frame_service/mocks.tsx index 71d7b982c6fad..f9631684aac4b 100644 --- a/x-pack/plugins/lens/public/editor_frame_service/mocks.tsx +++ b/x-pack/plugins/lens/public/editor_frame_service/mocks.tsx @@ -51,7 +51,7 @@ export function createMockVisualization(): jest.Mocked { setDimension: jest.fn(), removeDimension: jest.fn(), - getErrorMessage: jest.fn((_state, _frame) => undefined), + getErrorMessages: jest.fn((_state, _frame) => undefined), }; } @@ -91,6 +91,7 @@ export function createMockDatasource(id: string): DatasourceMock { // this is an additional property which doesn't exist on real datasources // but can be used to validate whether specific API mock functions are called publicAPIMock, + getErrorMessages: jest.fn((_state) => undefined), }; } diff --git a/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.tsx b/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.tsx index 28aeac223e4a6..cc5761df68285 100644 --- a/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.tsx +++ b/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.tsx @@ -39,7 +39,7 @@ import { getDatasourceSuggestionsForVisualizeField, } from './indexpattern_suggestions'; -import { isDraggedField, normalizeOperationDataType } from './utils'; +import { getInvalidReferences, isDraggedField, normalizeOperationDataType } from './utils'; import { LayerPanel } from './layerpanel'; import { IndexPatternColumn } from './operations'; import { @@ -340,6 +340,32 @@ export function getIndexPatternDatasource({ }, getDatasourceSuggestionsFromCurrentState, getDatasourceSuggestionsForVisualizeField, + + getErrorMessages(state) { + if (state) { + const invalidLayers = getInvalidReferences(state); + if (invalidLayers.length > 0) { + const realIndex = Object.values(state.layers) + .map((layer, i) => { + if (invalidLayers.includes(layer)) { + return i + 1; + } + }) + .filter(Boolean) as number[]; + return [ + { + shortMessage: i18n.translate('xpack.lens.indexPattern.dataReferenceFailureShort', { + defaultMessage: 'Invalid references', + }), + longMessage: i18n.translate('xpack.lens.indexPattern.dataReferenceFailureShort', { + defaultMessage: `{layers, plural, one {Layer} other {Layers}} {layersList} {layers, plural, one {has} other {have}} invalid reference`, + values: { layers: realIndex.length, layersList: realIndex.join(', ') }, + }), + }, + ]; + } + } + }, }; return indexPatternDatasource; diff --git a/x-pack/plugins/lens/public/indexpattern_datasource/utils.ts b/x-pack/plugins/lens/public/indexpattern_datasource/utils.ts index f1d2e7765d99f..a57be389c773e 100644 --- a/x-pack/plugins/lens/public/indexpattern_datasource/utils.ts +++ b/x-pack/plugins/lens/public/indexpattern_datasource/utils.ts @@ -44,7 +44,11 @@ export function isDraggedField(fieldCandidate: unknown): fieldCandidate is Dragg } export function hasInvalidReference(state: IndexPatternPrivateState) { - return Object.values(state.layers).some((layer) => { + return getInvalidReferences(state).length > 0; +} + +export function getInvalidReferences(state: IndexPatternPrivateState) { + return Object.values(state.layers || {}).filter((layer) => { return layer.columnOrder.some((columnId) => { const column = layer.columns[columnId]; return ( 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 d83f1c56d5ad3..5ee33f9b4b3dd 100644 --- a/x-pack/plugins/lens/public/metric_visualization/visualization.test.ts +++ b/x-pack/plugins/lens/public/metric_visualization/visualization.test.ts @@ -194,7 +194,7 @@ describe('metric_visualization', () => { }); }); - describe('#getErrorMessage', () => { + describe('#getErrorMessages', () => { it('returns undefined if no error is raised', () => { const datasource: DatasourcePublicAPI = { ...createMockDatasource('l1').publicAPIMock, @@ -212,7 +212,7 @@ describe('metric_visualization', () => { datasourceLayers: { l1: datasource }, }; - const error = metricVisualization.getErrorMessage(exampleState(), frame); + const error = metricVisualization.getErrorMessages(exampleState(), frame); 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 bd0c796d1d01d..b75ac89d7e4d8 100644 --- a/x-pack/plugins/lens/public/metric_visualization/visualization.tsx +++ b/x-pack/plugins/lens/public/metric_visualization/visualization.tsx @@ -116,7 +116,7 @@ export const metricVisualization: Visualization = { return { ...prevState, accessor: undefined }; }, - getErrorMessage(state, frame) { + getErrorMessages(state, frame) { // Is it possible to break it? return undefined; }, 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 a6e0df6e12e42..1d111b903f42d 100644 --- a/x-pack/plugins/lens/public/pie_visualization/visualization.test.ts +++ b/x-pack/plugins/lens/public/pie_visualization/visualization.test.ts @@ -42,7 +42,7 @@ function mockFrame(): FramePublicAPI { // Just a basic bootstrap here to kickstart the tests describe('pie_visualization', () => { - describe('#getErrorMessage', () => { + describe('#getErrorMessages', () => { it('returns undefined if no error is raised', () => { const datasource: DatasourcePublicAPI = { ...createMockDatasource('l1').publicAPIMock, @@ -60,7 +60,7 @@ describe('pie_visualization', () => { datasourceLayers: { l1: datasource }, }; - const error = pieVisualization.getErrorMessage(exampleState(), frame); + const error = pieVisualization.getErrorMessages(exampleState(), frame); 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 c72e3ae733c02..a38ff870fb1ba 100644 --- a/x-pack/plugins/lens/public/pie_visualization/visualization.tsx +++ b/x-pack/plugins/lens/public/pie_visualization/visualization.tsx @@ -215,7 +215,7 @@ export const pieVisualization: Visualization = { ); }, - getErrorMessage(state, frame) { + getErrorMessages(state, frame) { // 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 1371efd1579ef..eeb6549aaa679 100644 --- a/x-pack/plugins/lens/public/types.ts +++ b/x-pack/plugins/lens/public/types.ts @@ -180,6 +180,7 @@ export interface Datasource { getDatasourceSuggestionsFromCurrentState: (state: T) => Array>; getPublicAPI: (props: PublicAPIProps) => DatasourcePublicAPI; + getErrorMessages: (state: T) => Array<{ shortMessage: string; longMessage: string }> | undefined; } /** @@ -561,10 +562,10 @@ export interface Visualization { * The frame will call this function on all visualizations at error time in order * to provide more context to the error and show it to the user */ - getErrorMessage: ( + getErrorMessages: ( state: T, frame: FramePublicAPI - ) => { shortMessage: string; longMessage: string } | undefined; + ) => Array<{ shortMessage: string; longMessage: string }> | undefined; } export interface LensFilterEvent { 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 ce289b43987ac..594a423a67382 100644 --- a/x-pack/plugins/lens/public/xy_visualization/visualization.test.ts +++ b/x-pack/plugins/lens/public/xy_visualization/visualization.test.ts @@ -372,10 +372,10 @@ describe('xy_visualization', () => { }); }); - describe('#getErrorMessage', () => { + describe('#getErrorMessages', () => { it("should not return an error when there's only one dimension", () => { expect( - xyVisualization.getErrorMessage( + xyVisualization.getErrorMessages( { ...exampleState(), layers: [ @@ -393,7 +393,7 @@ describe('xy_visualization', () => { }); it("should not return an error when there's only one dimension on multiple layers (same axis everywhere)", () => { expect( - xyVisualization.getErrorMessage( + xyVisualization.getErrorMessages( { ...exampleState(), layers: [ @@ -417,7 +417,7 @@ describe('xy_visualization', () => { }); it('should return an error when there are multiple layers, one axis configured for each layer (but different axis from each other)', () => { expect( - xyVisualization.getErrorMessage( + xyVisualization.getErrorMessages( { ...exampleState(), layers: [ @@ -437,14 +437,20 @@ describe('xy_visualization', () => { }, createMockFramePublicAPI() ) - ).toEqual({ - shortMessage: 'Some layers are missing the X dimension', - longMessage: 'Layer 2 has no dimension field set for the X axis', - }); + ).toEqual([ + { + shortMessage: 'Missing X-axis', + longMessage: 'Layer 2 requires a field for the X-axis', + }, + { + shortMessage: 'Missing Y-axis', + longMessage: 'Layer 1 requires a field for the Y-axis', + }, + ]); }); it('should return an error with batched messages for the same error with multiple layers', () => { expect( - xyVisualization.getErrorMessage( + xyVisualization.getErrorMessages( { ...exampleState(), layers: [ @@ -452,7 +458,7 @@ describe('xy_visualization', () => { layerId: 'first', seriesType: 'area', xAccessor: 'a', - accessors: [], + accessors: ['a'], }, { layerId: 'second', @@ -470,14 +476,16 @@ describe('xy_visualization', () => { }, createMockFramePublicAPI() ) - ).toEqual({ - shortMessage: 'Some layers are missing the X dimension', - longMessage: 'Layers 2, 3 have no dimension field set for the X axis', - }); + ).toEqual([ + { + shortMessage: 'Missing X-axis', + longMessage: 'Layers 2, 3 require a field for the X-axis', + }, + ]); }); it("should return an error when some layers are complete but other layers aren't", () => { expect( - xyVisualization.getErrorMessage( + xyVisualization.getErrorMessages( { ...exampleState(), layers: [ @@ -503,10 +511,12 @@ describe('xy_visualization', () => { }, createMockFramePublicAPI() ) - ).toEqual({ - shortMessage: 'Some layers are missing the Y dimension', - longMessage: 'Layer 1 has no dimension field set for the Y axis', - }); + ).toEqual([ + { + shortMessage: 'Missing Y-axis', + longMessage: 'Layer 1 requires a field for the Y-axis', + }, + ]); }); }); }); diff --git a/x-pack/plugins/lens/public/xy_visualization/visualization.tsx b/x-pack/plugins/lens/public/xy_visualization/visualization.tsx index 2678a7e8781f7..1a79d7af4b97c 100644 --- a/x-pack/plugins/lens/public/xy_visualization/visualization.tsx +++ b/x-pack/plugins/lens/public/xy_visualization/visualization.tsx @@ -156,6 +156,8 @@ export const xyVisualization: Visualization = { getConfiguration(props) { const layer = props.state.layers.find((l) => l.layerId === props.layerId)!; + // Make it required only if there's a multi-layer inconsistency + const someLayersHaveXConfigured = props.state.layers.some((l) => l.xAccessor); return { groups: [ { @@ -167,7 +169,7 @@ export const xyVisualization: Visualization = { filterOperations: isBucketed, suggestedPriority: 1, supportsMoreColumns: !layer.xAccessor, - required: !layer.seriesType.includes('percentage'), + required: !layer.seriesType.includes('percentage') && someLayersHaveXConfigured, dataTestSubj: 'lnsXY_xDimensionPanel', }, { @@ -279,14 +281,19 @@ export const xyVisualization: Visualization = { toExpression, toPreviewExpression, - getErrorMessage(state, frame) { + getErrorMessages(state, frame) { // Data error handling below here const hasNoXAccessor = ({ xAccessor }: LayerConfig) => xAccessor == null; const hasNoAccessors = ({ accessors }: LayerConfig) => accessors == null || accessors.length === 0; - // check if the layes in the state are compatible with this type of chart - if (state.layers.length > 1) { + const errors: Array<{ + shortMessage: string; + longMessage: string; + }> = []; + + // check if the layers in the state are compatible with this type of chart + if (state && state.layers.length > 1) { const checks: Array<[string, (layer: LayerConfig) => boolean]> = [ ['X', hasNoXAccessor], ['Y', hasNoAccessors], @@ -300,11 +307,11 @@ export const xyVisualization: Visualization = { for (const [dimension, criteria] of checks) { const result = validateLayersForDimension(dimension, filteredLayers, criteria); if (!result.valid) { - return result.payload; + errors.push(result.payload); } } } - return undefined; + return errors.length ? errors : undefined; }, }; @@ -326,6 +333,7 @@ function validateLayersForDimension( } return missing; }, []); + return { valid: false, payload: getMessageIdsForDimension(dimension, layerMissingAccessors), @@ -339,20 +347,20 @@ function getMessageIdsForDimension(dimension: string, layers: number[]) { case 'X': return { shortMessage: i18n.translate('xpack.lens.xyVisualization.dataFailureXShort', { - defaultMessage: `Some layers are missing the X dimension`, + defaultMessage: `Missing X-axis`, }), longMessage: i18n.translate('xpack.lens.xyVisualization.dataFailureXLong', { - defaultMessage: `{layers, plural, one {Layer} other {Layers}} {layersList} {layers, plural, one {has} other {have}} no dimension field set for the X axis`, + defaultMessage: `{layers, plural, one {Layer} other {Layers}} {layersList} {layers, plural, one {requires} other {require}} a field for the X-axis`, values: { layers: layers.length, layersList }, }), }; case 'Y': return { shortMessage: i18n.translate('xpack.lens.xyVisualization.dataFailureYShort', { - defaultMessage: `Some layers are missing the Y dimension`, + defaultMessage: `Missing Y-axis`, }), longMessage: i18n.translate('xpack.lens.xyVisualization.dataFailureYLong', { - defaultMessage: `{layers, plural, one {Layer} other {Layers}} {layersList} {layers, plural, one {has} other {have}} no dimension field set for the Y axis`, + defaultMessage: `{layers, plural, one {Layer} other {Layers}} {layersList} {layers, plural, one {requires} other {require}} a field for the Y-axis`, values: { layers: layers.length, layersList }, }), }; From 8e7740104e5d1730a41fd55a5edd91bd5e462f78 Mon Sep 17 00:00:00 2001 From: dej611 Date: Tue, 27 Oct 2020 18:56:08 +0100 Subject: [PATCH 10/47] :pencil2: Off by one message --- .../editor_frame/workspace_panel/workspace_panel.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 fe839deb7b832..7c4b08546746c 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 @@ -387,7 +387,7 @@ export const InnerVisualizationWrapper = ({ {localState.configurationValidationError.length > 1 ? i18n.translate('xpack.lens.xyVisualization.dataFailureYLong', { defaultMessage: ` + {errors} {errors, plural, one {error} other {errors}}`, - values: { errors: localState.configurationValidationError.length }, + values: { errors: localState.configurationValidationError.length - 1 }, }) : null} From 7089b2f1c59002bbe74d471175cfdde2baaa135c Mon Sep 17 00:00:00 2001 From: dej611 Date: Tue, 27 Oct 2020 19:15:07 +0100 Subject: [PATCH 11/47] :globe_with_meridians: Fix i18n duplicate id --- .../lens/public/indexpattern_datasource/indexpattern.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.tsx b/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.tsx index 4a55bb173e5f4..c96eebd4af009 100644 --- a/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.tsx +++ b/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.tsx @@ -358,7 +358,7 @@ export function getIndexPatternDatasource({ shortMessage: i18n.translate('xpack.lens.indexPattern.dataReferenceFailureShort', { defaultMessage: 'Invalid references', }), - longMessage: i18n.translate('xpack.lens.indexPattern.dataReferenceFailureShort', { + longMessage: i18n.translate('xpack.lens.indexPattern.dataReferenceFailureLong', { defaultMessage: `{layers, plural, one {Layer} other {Layers}} {layersList} {layers, plural, one {has} other {have}} invalid reference`, values: { layers: realIndex.length, layersList: realIndex.join(', ') }, }), From 86cc058e76abf266f0c67b4c8e3608473afd809d Mon Sep 17 00:00:00 2001 From: dej611 Date: Wed, 28 Oct 2020 10:31:02 +0100 Subject: [PATCH 12/47] :globe_with_meridians: Fix last i18n issue --- .../editor_frame/workspace_panel/workspace_panel.tsx | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) 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 7c4b08546746c..557cc9a95370d 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 @@ -18,6 +18,7 @@ import { EuiButtonEmpty, EuiLink, EuiTitle, + EuiSpacer, } from '@elastic/eui'; import { CoreStart, CoreSetup } from 'kibana/public'; import { ExecutionContextSearch } from 'src/plugins/expressions'; @@ -384,8 +385,9 @@ export const InnerVisualizationWrapper = ({ {localState.configurationValidationError[0].shortMessage} {localState.configurationValidationError[0].longMessage} + {localState.configurationValidationError.length > 1 - ? i18n.translate('xpack.lens.xyVisualization.dataFailureYLong', { + ? i18n.translate('xpack.lens.editorFrame.configurationFailureMoreErrors', { defaultMessage: ` + {errors} {errors, plural, one {error} other {errors}}`, values: { errors: localState.configurationValidationError.length - 1 }, }) From 6672302da07fef53919647b45270ff09da63ba3e Mon Sep 17 00:00:00 2001 From: dej611 Date: Wed, 28 Oct 2020 12:50:32 +0100 Subject: [PATCH 13/47] :bug: Fixed a hook reflow issue --- .../workspace_panel/workspace_panel.tsx | 33 +++++++++---------- 1 file changed, 15 insertions(+), 18 deletions(-) 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 557cc9a95370d..41a3576abe94b 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 @@ -139,29 +139,24 @@ export function WorkspacePanel({ const expression = useMemo( () => { - if (activeDatasourceId) { - const activeDatasource = activeDatasourceId ? datasourceMap[activeDatasourceId] : null; - const dataMessages = activeDatasource?.getErrorMessages( - datasourceStates[activeDatasourceId]?.state - ); - if (dataMessages) { - setLocalState((s) => ({ - ...s, - configurationValidationError: dataMessages, - })); - // TS will infer "void" if this is omitted - return; - } - } + const activeDatasource = activeDatasourceId ? datasourceMap[activeDatasourceId] : null; + const dataMessages = activeDatasourceId + ? activeDatasource?.getErrorMessages(datasourceStates[activeDatasourceId]?.state) + : undefined; const vizMessages = activeVisualization?.getErrorMessages(visualizationState, framePublicAPI); - if (vizMessages || localState.configurationValidationError) { + + if (vizMessages || dataMessages) { setLocalState((s) => ({ ...s, - configurationValidationError: vizMessages, + configurationValidationError: [...(vizMessages || []), ...(dataMessages || [])], + })); + } else if (localState.configurationValidationError) { + setLocalState((s) => ({ + ...s, + configurationValidationError: undefined, })); - // TS will infer "void" if this is omitted - return; } + try { return buildExpression({ visualization: activeVisualization, @@ -195,6 +190,7 @@ export function WorkspacePanel({ framePublicAPI.dateRange, framePublicAPI.query, framePublicAPI.filters, + activeDatasourceId, ] ); @@ -413,6 +409,7 @@ export const InnerVisualizationWrapper = ({ ); } + return (
Date: Wed, 28 Oct 2020 15:49:58 +0100 Subject: [PATCH 14/47] :recycle:+:white_check_mark: Reworked validation flow + tests --- .../workspace_panel/workspace_panel.test.tsx | 131 +++++++++++++++++- .../workspace_panel/workspace_panel.tsx | 109 ++++++++------- 2 files changed, 191 insertions(+), 49 deletions(-) diff --git a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.test.tsx b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.test.tsx index 82205e93085c3..ac27ed2436160 100644 --- a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.test.tsx +++ b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.test.tsx @@ -454,6 +454,135 @@ describe('workspace_panel', () => { expect(expressionRendererMock).toHaveBeenCalledTimes(2); }); + it('should show an error message if validation on datasource does not pass', () => { + mockDatasource.getErrorMessages.mockReturnValue([ + { shortMessage: 'An error occurred', longMessage: 'An long description here' }, + ]); + mockDatasource.getLayers.mockReturnValue(['first']); + const framePublicAPI = createMockFramePublicAPI(); + framePublicAPI.datasourceLayers = { + first: mockDatasource.publicAPIMock, + }; + + instance = mount( + 'vis' }, + }} + visualizationState={{}} + dispatch={() => {}} + ExpressionRenderer={expressionRendererMock} + core={coreMock.createSetup()} + plugins={{ uiActions: uiActionsMock, data: dataMock }} + /> + ); + + expect(instance.find('[data-test-subj="configuration-failure"]').exists()).toBeTruthy(); + expect(instance.find(expressionRendererMock)).toHaveLength(0); + }); + + it('should show an error message if validation on visualization does not pass', () => { + mockDatasource.getErrorMessages.mockReturnValue(undefined); + mockDatasource.getLayers.mockReturnValue(['first']); + mockVisualization.getErrorMessages.mockReturnValue([ + { shortMessage: 'Some error happened', longMessage: 'Some long description happened' }, + ]); + mockVisualization.toExpression.mockReturnValue('vis'); + const framePublicAPI = createMockFramePublicAPI(); + framePublicAPI.datasourceLayers = { + first: mockDatasource.publicAPIMock, + }; + + instance = mount( + {}} + ExpressionRenderer={expressionRendererMock} + core={coreMock.createSetup()} + plugins={{ uiActions: uiActionsMock, data: dataMock }} + /> + ); + + expect(instance.find('[data-test-subj="configuration-failure"]').exists()).toBeTruthy(); + expect(instance.find(expressionRendererMock)).toHaveLength(0); + }); + + it('should show an error message if validation on both datasource and visualization do not pass', () => { + mockDatasource.getErrorMessages.mockReturnValue([ + { shortMessage: 'An error occurred', longMessage: 'An long description here' }, + ]); + mockDatasource.getLayers.mockReturnValue(['first']); + mockVisualization.getErrorMessages.mockReturnValue([ + { shortMessage: 'Some error happened', longMessage: 'Some long description happened' }, + ]); + mockVisualization.toExpression.mockReturnValue('vis'); + const framePublicAPI = createMockFramePublicAPI(); + framePublicAPI.datasourceLayers = { + first: mockDatasource.publicAPIMock, + }; + + instance = mount( + {}} + ExpressionRenderer={expressionRendererMock} + core={coreMock.createSetup()} + plugins={{ uiActions: uiActionsMock, data: dataMock }} + /> + ); + + // EuiFlexItem duplicates internally the attribute, so we need to filter only the most inner one here + expect( + instance.find('[data-test-subj="configuration-failure-short-message"]').last().text() + ).toEqual('An error occurred'); + expect( + instance.find('[data-test-subj="configuration-failure-more-errors"]').last().text() + ).toEqual(' +1 error'); + expect(instance.find(expressionRendererMock)).toHaveLength(0); + }); + it('should show an error message if the expression fails to parse', () => { mockDatasource.toExpression.mockReturnValue('|||'); mockDatasource.getLayers.mockReturnValue(['first']); @@ -487,7 +616,7 @@ describe('workspace_panel', () => { /> ); - expect(instance.find('[data-test-subj="expression-failure"]').first()).toBeTruthy(); + expect(instance.find('[data-test-subj="expression-failure"]').exists()).toBeTruthy(); expect(instance.find(expressionRendererMock)).toHaveLength(0); }); 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 41a3576abe94b..5ace39e068c2b 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 @@ -128,7 +128,6 @@ export function WorkspacePanel({ ); const [localState, setLocalState] = useState({ - configurationValidationError: undefined, expressionBuildError: undefined, expandError: false, }); @@ -137,48 +136,52 @@ export function WorkspacePanel({ ? visualizationMap[activeVisualizationId] : null; - const expression = useMemo( - () => { - const activeDatasource = activeDatasourceId ? datasourceMap[activeDatasourceId] : null; - const dataMessages = activeDatasourceId - ? activeDatasource?.getErrorMessages(datasourceStates[activeDatasourceId]?.state) - : undefined; - const vizMessages = activeVisualization?.getErrorMessages(visualizationState, framePublicAPI); + const configurationValidationError = useMemo(() => { + const activeDatasource = activeDatasourceId ? datasourceMap[activeDatasourceId] : null; + const dataMessages = activeDatasourceId + ? activeDatasource?.getErrorMessages(datasourceStates[activeDatasourceId]?.state) + : undefined; + const vizMessages = activeVisualization?.getErrorMessages(visualizationState, framePublicAPI); - if (vizMessages || dataMessages) { - setLocalState((s) => ({ - ...s, - configurationValidationError: [...(vizMessages || []), ...(dataMessages || [])], - })); - } else if (localState.configurationValidationError) { - setLocalState((s) => ({ - ...s, - configurationValidationError: undefined, - })); - } + if (vizMessages || dataMessages) { + // Data first, visualization next + return [...(dataMessages || []), ...(vizMessages || [])]; + } + }, [ + activeVisualization?.getErrorMessages, + visualizationState, + activeDatasourceId, + datasourceMap, + datasourceStates, + framePublicAPI, + ]); - try { - return buildExpression({ - visualization: activeVisualization, - visualizationState, - datasourceMap, - datasourceStates, - datasourceLayers: framePublicAPI.datasourceLayers, - }); - } catch (e) { - const buildMessages = activeVisualization?.getErrorMessages( - visualizationState, - framePublicAPI - ); - const defaultMessage = { - shortMessage: 'One error occurred in the expression', - longMessage: e.toString(), - }; - // Most likely an error in the expression provided by a datasource or visualization - setLocalState((s) => ({ - ...s, - expressionBuildError: buildMessages ?? [defaultMessage], - })); + const expression = useMemo( + () => { + if (!configurationValidationError) { + try { + return buildExpression({ + visualization: activeVisualization, + visualizationState, + datasourceMap, + datasourceStates, + datasourceLayers: framePublicAPI.datasourceLayers, + }); + } catch (e) { + const buildMessages = activeVisualization?.getErrorMessages( + visualizationState, + framePublicAPI + ); + const defaultMessage = { + shortMessage: 'One error occurred in the expression', + longMessage: e.toString(), + }; + // Most likely an error in the expression provided by a datasource or visualization + setLocalState((s) => ({ + ...s, + expressionBuildError: buildMessages ?? [defaultMessage], + })); + } } }, // eslint-disable-next-line react-hooks/exhaustive-deps @@ -190,7 +193,6 @@ export function WorkspacePanel({ framePublicAPI.dateRange, framePublicAPI.query, framePublicAPI.filters, - activeDatasourceId, ] ); @@ -292,7 +294,7 @@ export function WorkspacePanel({ timefilter={plugins.data.query.timefilter.timefilter} onEvent={onEvent} setLocalState={setLocalState} - localState={localState} + localState={{ ...localState, configurationValidationError }} ExpressionRendererComponent={ExpressionRendererComponent} /> ); @@ -364,11 +366,16 @@ export const InnerVisualizationWrapper = ({ if (localState.configurationValidationError) { return ( - + - + - {localState.configurationValidationError[0].shortMessage} + + {localState.configurationValidationError[0].shortMessage} + {localState.configurationValidationError[0].longMessage} - + + {localState.configurationValidationError.length > 1 ? i18n.translate('xpack.lens.editorFrame.configurationFailureMoreErrors', { - defaultMessage: ` + {errors} {errors, plural, one {error} other {errors}}`, + defaultMessage: ` +{errors} {errors, plural, one {error} other {errors}}`, values: { errors: localState.configurationValidationError.length - 1 }, }) : null} From f1127f42b881fd1738fdc48a41e724a71b1ee632 Mon Sep 17 00:00:00 2001 From: dej611 Date: Wed, 28 Oct 2020 16:11:29 +0100 Subject: [PATCH 15/47] :label: Fix type issue --- .../workspace_panel/workspace_panel.tsx | 20 +++++++++---------- 1 file changed, 9 insertions(+), 11 deletions(-) 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 5ace39e068c2b..32334ac35de15 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 @@ -18,7 +18,6 @@ import { EuiButtonEmpty, EuiLink, EuiTitle, - EuiSpacer, } from '@elastic/eui'; import { CoreStart, CoreSetup } from 'kibana/public'; import { ExecutionContextSearch } from 'src/plugins/expressions'; @@ -391,16 +390,15 @@ export const InnerVisualizationWrapper = ({ {localState.configurationValidationError[0].longMessage} - - {localState.configurationValidationError.length > 1 - ? i18n.translate('xpack.lens.editorFrame.configurationFailureMoreErrors', { - defaultMessage: ` +{errors} {errors, plural, one {error} other {errors}}`, - values: { errors: localState.configurationValidationError.length - 1 }, - }) - : null} + + + {localState.configurationValidationError.length > 1 + ? i18n.translate('xpack.lens.editorFrame.configurationFailureMoreErrors', { + defaultMessage: ` +{errors} {errors, plural, one {error} other {errors}}`, + values: { errors: localState.configurationValidationError.length - 1 }, + }) + : null} + ); From 95f9daf8cbd2e6f6f08b903be639831e2e89525b Mon Sep 17 00:00:00 2001 From: dej611 Date: Thu, 29 Oct 2020 12:01:59 +0100 Subject: [PATCH 16/47] :bug: Improved XY corner cases validation logic --- .../xy_visualization/visualization.test.ts | 97 ++++++++++++++++--- .../public/xy_visualization/visualization.tsx | 30 ++++-- 2 files changed, 104 insertions(+), 23 deletions(-) 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 7a18bdd679f53..ffb278c09e12e 100644 --- a/x-pack/plugins/lens/public/xy_visualization/visualization.test.ts +++ b/x-pack/plugins/lens/public/xy_visualization/visualization.test.ts @@ -404,7 +404,7 @@ describe('xy_visualization', () => { }); describe('#getErrorMessages', () => { - it("should not return an error when there's only one dimension", () => { + it("should not return an error when there's only one dimension (X or Y)", () => { expect( xyVisualization.getErrorMessages( { @@ -446,6 +446,75 @@ describe('xy_visualization', () => { ) ).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() + ) + ).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() + ) + ).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() + ) + ).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( @@ -470,12 +539,8 @@ describe('xy_visualization', () => { ) ).toEqual([ { - shortMessage: 'Missing X-axis', - longMessage: 'Layer 2 requires a field for the X-axis', - }, - { - shortMessage: 'Missing Y-axis', - longMessage: 'Layer 1 requires a field for the Y-axis', + shortMessage: 'Missing Vertical axis', + longMessage: 'Layer 1 requires a field for the Vertical axis', }, ]); }); @@ -495,13 +560,15 @@ describe('xy_visualization', () => { layerId: 'second', seriesType: 'area', xAccessor: undefined, - accessors: ['a'], + accessors: [], + splitAccessor: 'a', }, { layerId: 'third', seriesType: 'area', xAccessor: undefined, - accessors: ['a'], + accessors: [], + splitAccessor: 'a', }, ], }, @@ -509,8 +576,12 @@ describe('xy_visualization', () => { ) ).toEqual([ { - shortMessage: 'Missing X-axis', - longMessage: 'Layers 2, 3 require a field for the X-axis', + shortMessage: 'Missing Vertical axis', + longMessage: 'Layers 2, 3 require a field for the Vertical axis', + }, + { + shortMessage: 'Missing Horizontal axis', + longMessage: 'Layers 2, 3 require a field for the Horizontal axis', }, ]); }); @@ -544,8 +615,8 @@ describe('xy_visualization', () => { ) ).toEqual([ { - shortMessage: 'Missing Y-axis', - longMessage: 'Layer 1 requires a field for the Y-axis', + shortMessage: 'Missing Vertical axis', + longMessage: 'Layer 1 requires a field for the 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 fd1d9641595cc..110a16417c740 100644 --- a/x-pack/plugins/lens/public/xy_visualization/visualization.tsx +++ b/x-pack/plugins/lens/public/xy_visualization/visualization.tsx @@ -156,8 +156,11 @@ export const xyVisualization: Visualization = { getConfiguration(props) { const layer = props.state.layers.find((l) => l.layerId === props.layerId)!; - // Make it required only if there's a multi-layer inconsistency - const someLayersHaveXConfigured = props.state.layers.some((l) => l.xAccessor); + // Make it required only if there's a multi-layer inconsistency: + // Note that X-Axis is only required if there's an yAxis defined && there's no splitAccessor defined + const someLayersAreIncomplete = props.state.layers.every( + (l) => l.accessors.length === 0 && l.xAccessor == null && l.splitAccessor == null + ); const isHorizontal = isHorizontalChart(props.state.layers); return { groups: [ @@ -174,7 +177,7 @@ export const xyVisualization: Visualization = { filterOperations: isBucketed, suggestedPriority: 1, supportsMoreColumns: !layer.xAccessor, - required: !layer.seriesType.includes('percentage') && someLayersHaveXConfigured, + required: !layer.seriesType.includes('percentage') && someLayersAreIncomplete, dataTestSubj: 'lnsXY_xDimensionPanel', }, { @@ -292,9 +295,12 @@ export const xyVisualization: Visualization = { getErrorMessages(state, frame) { // Data error handling below here - const hasNoXAccessor = ({ xAccessor }: LayerConfig) => xAccessor == null; + const hasNoXAccessor = ({ xAccessor, accessors, splitAccessor }: LayerConfig) => + // Notify about missing X Axis only if yAxis is missing as well + xAccessor == null && accessors.length === 0; const hasNoAccessors = ({ accessors }: LayerConfig) => accessors == null || accessors.length === 0; + const hasNoSplitAccessor = ({ splitAccessor }: LayerConfig) => splitAccessor == null; const errors: Array<{ shortMessage: string; @@ -303,14 +309,18 @@ export const xyVisualization: Visualization = { // check if the layers in the state are compatible with this type of chart if (state && state.layers.length > 1) { + // Order is important here: Y Axis is fundamental to exist to make it valid const checks: Array<[string, (layer: LayerConfig) => boolean]> = [ - ['X', hasNoXAccessor], ['Y', hasNoAccessors], + ['X', hasNoXAccessor], ]; // filter those layers with no accessors at all const filteredLayers = state.layers.filter( - (layer) => !checks.every(([dimension, missingCriteria]) => missingCriteria(layer)) + (layer) => + !checks.every(([dimension, missingCriteria]) => missingCriteria(layer)) || + // Do not discard layers with only breakdown/split Accessor + !hasNoSplitAccessor(layer) ); for (const [dimension, criteria] of checks) { @@ -356,20 +366,20 @@ function getMessageIdsForDimension(dimension: string, layers: number[]) { case 'X': return { shortMessage: i18n.translate('xpack.lens.xyVisualization.dataFailureXShort', { - defaultMessage: `Missing X-axis`, + defaultMessage: `Missing Horizontal axis`, }), longMessage: i18n.translate('xpack.lens.xyVisualization.dataFailureXLong', { - defaultMessage: `{layers, plural, one {Layer} other {Layers}} {layersList} {layers, plural, one {requires} other {require}} a field for the X-axis`, + defaultMessage: `{layers, plural, one {Layer} other {Layers}} {layersList} {layers, plural, one {requires} other {require}} a field for the Horizontal axis`, values: { layers: layers.length, layersList }, }), }; case 'Y': return { shortMessage: i18n.translate('xpack.lens.xyVisualization.dataFailureYShort', { - defaultMessage: `Missing Y-axis`, + defaultMessage: `Missing Vertical axis`, }), longMessage: i18n.translate('xpack.lens.xyVisualization.dataFailureYLong', { - defaultMessage: `{layers, plural, one {Layer} other {Layers}} {layersList} {layers, plural, one {requires} other {require}} a field for the Y-axis`, + defaultMessage: `{layers, plural, one {Layer} other {Layers}} {layersList} {layers, plural, one {requires} other {require}} a field for the Vertical axis`, values: { layers: layers.length, layersList }, }), }; From ce7f857ccc121a909f43bbbb77afbb939c68e60e Mon Sep 17 00:00:00 2001 From: dej611 Date: Thu, 29 Oct 2020 12:04:21 +0100 Subject: [PATCH 17/47] :bug: Fix empty datatable scenario --- .../lens/public/datatable_visualization/visualization.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugins/lens/public/datatable_visualization/visualization.tsx b/x-pack/plugins/lens/public/datatable_visualization/visualization.tsx index 13229ce281859..08b2be2cd57e5 100644 --- a/x-pack/plugins/lens/public/datatable_visualization/visualization.tsx +++ b/x-pack/plugins/lens/public/datatable_visualization/visualization.tsx @@ -247,7 +247,7 @@ export const datatableVisualization: Visualization // Data validation below if ( - sortedColumns && + sortedColumns?.length && sortedColumns.filter((c) => !datasource!.getOperationForColumnId(c)?.isBucketed).length === 0 ) { From d88d55db33503998b19140896049332cbc0ce205 Mon Sep 17 00:00:00 2001 From: dej611 Date: Thu, 29 Oct 2020 17:32:48 +0100 Subject: [PATCH 18/47] :sparkles: + :white_check_mark: Improved error messages for invalid datasources + tests --- .../indexpattern.test.ts | 172 ++++++++++++++++++ .../indexpattern_datasource/indexpattern.tsx | 55 ++++-- .../public/indexpattern_datasource/utils.ts | 26 ++- 3 files changed, 239 insertions(+), 14 deletions(-) diff --git a/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.test.ts b/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.test.ts index 900cd02622aaf..bea25fd7e587c 100644 --- a/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.test.ts +++ b/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.test.ts @@ -560,4 +560,176 @@ describe('IndexPattern Data Source', () => { }); }); }); + + describe('#getErrorMessages', () => { + it('should detect a missing reference in a layer', () => { + const state = { + indexPatternRefs: [], + existingFields: {}, + isFirstExistenceFetch: false, + indexPatterns: expectedIndexPatterns, + layers: { + first: { + indexPatternId: '1', + columnOrder: ['col1'], + columns: { + col1: { + dataType: 'number', + isBucketed: false, + label: 'Foo', + operationType: 'count', // <= invalid + sourceField: 'bytes', + }, + }, + }, + }, + currentIndexPatternId: '1', + }; + const messages = indexPatternDatasource.getErrorMessages(state); + expect(messages).toHaveLength(1); + expect(messages![0]).toEqual({ + shortMessage: 'Invalid reference ', + longMessage: 'Field "bytes" has invalid reference', + }); + }); + + it('should detect and batch missing references in a layer', () => { + const state = { + indexPatternRefs: [], + existingFields: {}, + isFirstExistenceFetch: false, + indexPatterns: expectedIndexPatterns, + layers: { + first: { + indexPatternId: '1', + columnOrder: ['col1', 'col2'], + columns: { + col1: { + dataType: 'number', + isBucketed: false, + label: 'Foo', + operationType: 'count', // <= invalid + sourceField: 'bytes', + }, + col2: { + dataType: 'number', + isBucketed: false, + label: 'Foo', + operationType: 'count', // <= invalid + sourceField: 'memory', + }, + }, + }, + }, + currentIndexPatternId: '1', + }; + const messages = indexPatternDatasource.getErrorMessages(state); + expect(messages).toHaveLength(1); + expect(messages![0]).toEqual({ + shortMessage: 'Invalid references ', + longMessage: 'Fields "bytes", "memory" have invalid reference', + }); + }); + + it('should detect and batch missing references in multiple layers', () => { + const state = { + indexPatternRefs: [], + existingFields: {}, + isFirstExistenceFetch: false, + indexPatterns: expectedIndexPatterns, + layers: { + first: { + indexPatternId: '1', + columnOrder: ['col1', 'col2'], + columns: { + col1: { + dataType: 'number', + isBucketed: false, + label: 'Foo', + operationType: 'count', // <= invalid + sourceField: 'bytes', + }, + col2: { + dataType: 'number', + isBucketed: false, + label: 'Foo', + operationType: 'count', // <= invalid + sourceField: 'memory', + }, + }, + }, + second: { + indexPatternId: '1', + columnOrder: ['col1'], + columns: { + col1: { + dataType: 'string', + isBucketed: false, + label: 'Foo', + operationType: 'count', // <= invalid + sourceField: 'source', + }, + }, + }, + }, + currentIndexPatternId: '1', + }; + const messages = indexPatternDatasource.getErrorMessages(state); + expect(messages).toHaveLength(2); + expect(messages).toEqual([ + { + shortMessage: 'Invalid references on Layer 1', + longMessage: 'Fields "bytes", "memory" have invalid reference', + }, + { + shortMessage: 'Invalid reference on Layer 2', + longMessage: 'Field "source" has invalid reference', + }, + ]); + }); + + it('should return no errors if all references are satified', () => { + const state = { + indexPatternRefs: [], + existingFields: {}, + isFirstExistenceFetch: false, + indexPatterns: expectedIndexPatterns, + layers: { + first: { + indexPatternId: '1', + columnOrder: ['col1'], + columns: { + col1: { + dataType: 'number', + isBucketed: false, + label: 'Foo', + operationType: 'document', + sourceField: 'bytes', + }, + }, + }, + }, + currentIndexPatternId: '1', + }; + expect(indexPatternDatasource.getErrorMessages(state)).not.toBeDefined(); + }); + + it('should return no errors with layers with no columns', () => { + const state = { + indexPatternRefs: [], + existingFields: {}, + isFirstExistenceFetch: false, + indexPatterns: expectedIndexPatterns, + layers: { + first: { + indexPatternId: '1', + columnOrder: [], + columns: {}, + }, + }, + currentIndexPatternId: '1', + }; + expect(indexPatternDatasource.getErrorMessages(state)).not.toBeDefined(); + }); + }); }); diff --git a/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.tsx b/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.tsx index c96eebd4af009..298ff4d2e8b88 100644 --- a/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.tsx +++ b/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.tsx @@ -39,7 +39,12 @@ import { getDatasourceSuggestionsForVisualizeField, } from './indexpattern_suggestions'; -import { getInvalidReferences, isDraggedField, normalizeOperationDataType } from './utils'; +import { + getInvalidFieldReferencesForLayer, + getInvalidReferences, + isDraggedField, + normalizeOperationDataType, +} from './utils'; import { LayerPanel } from './layerpanel'; import { IndexPatternColumn } from './operations'; import { @@ -54,6 +59,7 @@ import { VisualizeFieldContext } from '../../../../../src/plugins/ui_actions/pub import { deleteColumn } from './state_helpers'; import { Datasource, StateSetter } from '../index'; import { ChartsPluginSetup } from '../../../../../src/plugins/charts/public'; +import { FieldBasedIndexPatternColumn } from './operations/definitions/column_types'; export { OperationType, IndexPatternColumn } from './operations'; @@ -345,25 +351,52 @@ export function getIndexPatternDatasource({ getErrorMessages(state) { if (state) { const invalidLayers = getInvalidReferences(state); + if (invalidLayers.length > 0) { const realIndex = Object.values(state.layers) .map((layer, i) => { - if (invalidLayers.includes(layer)) { - return i + 1; + const filteredIndex = invalidLayers.indexOf(layer); + if (filteredIndex > -1) { + return [filteredIndex, i + 1]; } }) - .filter(Boolean) as number[]; - return [ - { + .filter(Boolean) as Array<[number, number]>; + const invalidFieldsPerLayer: string[][] = getInvalidFieldReferencesForLayer( + invalidLayers, + state.indexPatterns + ); + const originalLayersList = Object.keys(state.layers); + + return realIndex.map(([filteredIndex, layerIndex]) => { + const fieldsWithBrokenReferences: string[] = invalidFieldsPerLayer[filteredIndex].map( + (columnId) => { + const column = invalidLayers[filteredIndex].columns[ + columnId + ] as FieldBasedIndexPatternColumn; + return column.sourceField; + } + ); + return { shortMessage: i18n.translate('xpack.lens.indexPattern.dataReferenceFailureShort', { - defaultMessage: 'Invalid references', + defaultMessage: + 'Invalid {fields, plural, one {reference} other {references}} {hasMultipleLayers, plural, one {} other {on Layer {layer}}}', + values: { + layer: layerIndex, + fields: fieldsWithBrokenReferences.length, + hasMultipleLayers: originalLayersList.length, + }, }), longMessage: i18n.translate('xpack.lens.indexPattern.dataReferenceFailureLong', { - defaultMessage: `{layers, plural, one {Layer} other {Layers}} {layersList} {layers, plural, one {has} other {have}} invalid reference`, - values: { layers: realIndex.length, layersList: realIndex.join(', ') }, + defaultMessage: `{fieldsLength, plural, one {Field} other {Fields}} "{fields}" {fieldsLength, plural, one {has} other {have}} invalid reference`, + values: { + layers: realIndex.length, + fields: fieldsWithBrokenReferences.join('", "'), + fieldsLength: fieldsWithBrokenReferences.length, + hasMultipleLayers: originalLayersList.length, + }, }), - }, - ]; + }; + }); } } }, diff --git a/x-pack/plugins/lens/public/indexpattern_datasource/utils.ts b/x-pack/plugins/lens/public/indexpattern_datasource/utils.ts index a57be389c773e..6783498e23233 100644 --- a/x-pack/plugins/lens/public/indexpattern_datasource/utils.ts +++ b/x-pack/plugins/lens/public/indexpattern_datasource/utils.ts @@ -5,7 +5,7 @@ */ import { DataType } from '../types'; -import { IndexPatternPrivateState, IndexPattern } from './types'; +import { IndexPatternPrivateState, IndexPattern, IndexPatternLayer } from './types'; import { DraggedField } from './indexpattern'; import { BaseIndexPatternColumn, @@ -48,7 +48,7 @@ export function hasInvalidReference(state: IndexPatternPrivateState) { } export function getInvalidReferences(state: IndexPatternPrivateState) { - return Object.values(state.layers || {}).filter((layer) => { + return Object.values(state.layers).filter((layer) => { return layer.columnOrder.some((columnId) => { const column = layer.columns[columnId]; return ( @@ -63,19 +63,39 @@ export function getInvalidReferences(state: IndexPatternPrivateState) { }); } +export function getInvalidFieldReferencesForLayer( + layers: IndexPatternLayer[], + indexPatternMap: Record +) { + return layers.map((layer) => { + return layer.columnOrder.filter((columnId) => { + const column = layer.columns[columnId]; + return ( + hasField(column) && + fieldIsInvalid( + column.sourceField, + column.operationType, + indexPatternMap[layer.indexPatternId] + ) + ); + }); + }); +} + export function fieldIsInvalid( sourceField: string | undefined, operationType: OperationType | undefined, indexPattern: IndexPattern ) { const operationDefinition = operationType && operationDefinitionMap[operationType]; + return Boolean( sourceField && operationDefinition && !indexPattern.fields.some( (field) => field.name === sourceField && - operationDefinition.input === 'field' && + operationDefinition?.input === 'field' && operationDefinition.getPossibleOperationForField(field) !== undefined ) ); From 694757beddb3e4000155dad7262835118aa51e6b Mon Sep 17 00:00:00 2001 From: dej611 Date: Thu, 29 Oct 2020 17:35:14 +0100 Subject: [PATCH 19/47] :globe_with_meridians: Add missing i18n translation --- .../editor_frame/workspace_panel/workspace_panel.tsx | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) 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 32334ac35de15..c23628b3494c6 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 @@ -172,7 +172,9 @@ export function WorkspacePanel({ framePublicAPI ); const defaultMessage = { - shortMessage: 'One error occurred in the expression', + shortMessage: i18n.translate('xpack.lens.editorFrame.buildExpressionError', { + defaultMessage: 'An unexpected error occurred while preparing the chart', + }), longMessage: e.toString(), }; // Most likely an error in the expression provided by a datasource or visualization From 0277aa5635f0befe794142025cd34ad0d6b981e0 Mon Sep 17 00:00:00 2001 From: dej611 Date: Thu, 29 Oct 2020 18:32:30 +0100 Subject: [PATCH 20/47] :label: Fix type issues --- .../indexpattern_datasource/indexpattern.test.ts | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.test.ts b/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.test.ts index bea25fd7e587c..299d7a6a13a37 100644 --- a/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.test.ts +++ b/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.test.ts @@ -585,7 +585,7 @@ describe('IndexPattern Data Source', () => { }, currentIndexPatternId: '1', }; - const messages = indexPatternDatasource.getErrorMessages(state); + const messages = indexPatternDatasource.getErrorMessages(state as IndexPatternPrivateState); expect(messages).toHaveLength(1); expect(messages![0]).toEqual({ shortMessage: 'Invalid reference ', @@ -623,7 +623,7 @@ describe('IndexPattern Data Source', () => { }, currentIndexPatternId: '1', }; - const messages = indexPatternDatasource.getErrorMessages(state); + const messages = indexPatternDatasource.getErrorMessages(state as IndexPatternPrivateState); expect(messages).toHaveLength(1); expect(messages![0]).toEqual({ shortMessage: 'Invalid references ', @@ -674,7 +674,7 @@ describe('IndexPattern Data Source', () => { }, currentIndexPatternId: '1', }; - const messages = indexPatternDatasource.getErrorMessages(state); + const messages = indexPatternDatasource.getErrorMessages(state as IndexPatternPrivateState); expect(messages).toHaveLength(2); expect(messages).toEqual([ { @@ -711,11 +711,13 @@ describe('IndexPattern Data Source', () => { }, currentIndexPatternId: '1', }; - expect(indexPatternDatasource.getErrorMessages(state)).not.toBeDefined(); + expect( + indexPatternDatasource.getErrorMessages(state as IndexPatternPrivateState) + ).not.toBeDefined(); }); it('should return no errors with layers with no columns', () => { - const state = { + const state: IndexPatternPrivateState = { indexPatternRefs: [], existingFields: {}, isFirstExistenceFetch: false, From 822a23e3b15936f4b706d72e3a811210fe228c15 Mon Sep 17 00:00:00 2001 From: dej611 Date: Thu, 29 Oct 2020 18:37:28 +0100 Subject: [PATCH 21/47] :globe_with_meridians: Fix i18n issues --- .../lens/public/indexpattern_datasource/indexpattern.tsx | 2 -- 1 file changed, 2 deletions(-) diff --git a/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.tsx b/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.tsx index 298ff4d2e8b88..a01757a548038 100644 --- a/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.tsx +++ b/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.tsx @@ -389,10 +389,8 @@ export function getIndexPatternDatasource({ longMessage: i18n.translate('xpack.lens.indexPattern.dataReferenceFailureLong', { defaultMessage: `{fieldsLength, plural, one {Field} other {Fields}} "{fields}" {fieldsLength, plural, one {has} other {have}} invalid reference`, values: { - layers: realIndex.length, fields: fieldsWithBrokenReferences.join('", "'), fieldsLength: fieldsWithBrokenReferences.length, - hasMultipleLayers: originalLayersList.length, }, }), }; From 957bd98ec9c71bdeee152e12c46a4fef11e0e505 Mon Sep 17 00:00:00 2001 From: dej611 Date: Thu, 29 Oct 2020 19:15:27 +0100 Subject: [PATCH 22/47] :sparkles: Filter out suggestions which fail to build --- .../editor_frame_service/editor_frame/suggestion_panel.tsx | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/suggestion_panel.tsx b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/suggestion_panel.tsx index 5e5e9cda954ee..8661a9b2c1fd2 100644 --- a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/suggestion_panel.tsx +++ b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/suggestion_panel.tsx @@ -184,6 +184,13 @@ export function SuggestionPanel({ activeVisualizationId: currentVisualizationId, visualizationState: currentVisualizationState, }) + .filter(({ visualizationId, visualizationState: suggestionVisualizationState }) => { + const suggestionVisualization = visualizationMap[visualizationId]; + // filter out visualizations with errors + return ( + suggestionVisualization.getErrorMessages(suggestionVisualizationState, frame) == null + ); + }) .map((suggestion) => ({ ...suggestion, previewExpression: preparePreviewExpression( From 2fd7a90aa6a6e7cecd60bf0114f185cf8500883d Mon Sep 17 00:00:00 2001 From: dej611 Date: Fri, 30 Oct 2020 12:02:12 +0100 Subject: [PATCH 23/47] :truck: Migrate datatable validation logic to the building phase, handling it as building state --- .../visualization.test.tsx | 29 +++++++++++++--- .../datatable_visualization/visualization.tsx | 33 +++++-------------- 2 files changed, 32 insertions(+), 30 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 c704e17b7ee54..909790c74e0b5 100644 --- a/x-pack/plugins/lens/public/datatable_visualization/visualization.test.tsx +++ b/x-pack/plugins/lens/public/datatable_visualization/visualization.test.tsx @@ -356,7 +356,7 @@ describe('Datatable Visualization', () => { datasource.publicAPIMock.getTableSpec.mockReturnValue([{ columnId: 'c' }, { columnId: 'b' }]); datasource.publicAPIMock.getOperationForColumnId.mockReturnValue({ dataType: 'string', - isBucketed: true, + isBucketed: false, // <= make them metrics label: 'label', }); @@ -364,6 +364,7 @@ describe('Datatable Visualization', () => { { layers: [layer] }, frame.datasourceLayers ) as Ast; + const tableArgs = buildExpression(expression).findFunction('lens_datatable_columns'); expect(tableArgs).toHaveLength(1); @@ -371,10 +372,30 @@ describe('Datatable Visualization', () => { columnIds: ['c', 'b'], }); }); + + it('returns no expression if the metric dimension is not defined', () => { + const datasource = createMockDatasource('test'); + const layer = { layerId: 'a', columns: ['b', 'c'] }; + const frame = mockFrame(); + frame.datasourceLayers = { a: datasource.publicAPIMock }; + datasource.publicAPIMock.getTableSpec.mockReturnValue([{ columnId: 'c' }, { columnId: 'b' }]); + datasource.publicAPIMock.getOperationForColumnId.mockReturnValue({ + dataType: 'string', + isBucketed: true, // move it from the metric to the break down by side + label: 'label', + }); + + const expression = datatableVisualization.toExpression( + { layers: [layer] }, + frame.datasourceLayers + ); + + expect(expression).toEqual(null); + }); }); describe('#getErrorMessages', () => { - it('returns an error explanation if the datasource is missing a metric dimension', () => { + it('returns undefined if the datasource is missing a metric dimension', () => { const datasource = createMockDatasource('test'); const layer = { layerId: 'a', columns: ['b', 'c'] }; const frame = mockFrame(); @@ -388,9 +409,7 @@ describe('Datatable Visualization', () => { const error = datatableVisualization.getErrorMessages({ layers: [layer] }, frame); - expect(error).toHaveLength(1); - expect(error![0].shortMessage).toMatch('Missing metric'); - expect(error![0].longMessage).toMatch('Add a field in the Metrics panel'); + expect(error).not.toBeDefined(); }); it('returns undefined if the metric dimension is defined', () => { diff --git a/x-pack/plugins/lens/public/datatable_visualization/visualization.tsx b/x-pack/plugins/lens/public/datatable_visualization/visualization.tsx index 08b2be2cd57e5..eef7df5f26001 100644 --- a/x-pack/plugins/lens/public/datatable_visualization/visualization.tsx +++ b/x-pack/plugins/lens/public/datatable_visualization/visualization.tsx @@ -203,6 +203,13 @@ export const datatableVisualization: Visualization const { sortedColumns, datasource } = getDataSourceAndSortedColumns(state, datasourceLayers, state.layers[0].layerId) || {}; + if ( + sortedColumns?.length && + sortedColumns.filter((c) => !datasource!.getOperationForColumnId(c)?.isBucketed).length === 0 + ) { + return null; + } + const operations = sortedColumns! .map((columnId) => ({ columnId, operation: datasource!.getOperationForColumnId(columnId) })) .filter((o): o is { columnId: string; operation: Operation } => !!o.operation); @@ -237,31 +244,7 @@ export const datatableVisualization: Visualization }, getErrorMessages(state, frame) { - const errors: Array<{ - shortMessage: string; - longMessage: string; - }> = []; - if (state) { - const { sortedColumns, datasource } = - getDataSourceAndSortedColumns(state, frame.datasourceLayers, state.layers[0].layerId) || {}; - - // Data validation below - if ( - sortedColumns?.length && - sortedColumns.filter((c) => !datasource!.getOperationForColumnId(c)?.isBucketed).length === - 0 - ) { - errors.push({ - shortMessage: i18n.translate('xpack.lens.datatable.dataFailureShort', { - defaultMessage: `Missing metric`, - }), - longMessage: i18n.translate('xpack.lens.datatable.dataFailureLong', { - defaultMessage: `Add a field in the Metrics panel`, - }), - }); - } - } - return errors.length ? errors : undefined; + return undefined; }, }; From 44642852a2d4ac4944590a0053c6c2e4ad28db83 Mon Sep 17 00:00:00 2001 From: dej611 Date: Fri, 30 Oct 2020 12:03:35 +0100 Subject: [PATCH 24/47] :label: Fix type issue --- .../lens/public/datatable_visualization/visualization.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugins/lens/public/datatable_visualization/visualization.tsx b/x-pack/plugins/lens/public/datatable_visualization/visualization.tsx index eef7df5f26001..e0f6ae31719ca 100644 --- a/x-pack/plugins/lens/public/datatable_visualization/visualization.tsx +++ b/x-pack/plugins/lens/public/datatable_visualization/visualization.tsx @@ -199,7 +199,7 @@ export const datatableVisualization: Visualization }; }, - toExpression(state, datasourceLayers, { title, description } = {}): Ast { + toExpression(state, datasourceLayers, { title, description } = {}): Ast | null { const { sortedColumns, datasource } = getDataSourceAndSortedColumns(state, datasourceLayers, state.layers[0].layerId) || {}; From e039c4f9c9cb17ca9d2990a1c5e0fb120fa464db Mon Sep 17 00:00:00 2001 From: dej611 Date: Fri, 30 Oct 2020 12:17:28 +0100 Subject: [PATCH 25/47] :pencil2: Add comment for future enhancements --- x-pack/plugins/lens/public/xy_visualization/visualization.tsx | 3 +++ 1 file changed, 3 insertions(+) diff --git a/x-pack/plugins/lens/public/xy_visualization/visualization.tsx b/x-pack/plugins/lens/public/xy_visualization/visualization.tsx index 110a16417c740..752b186e26d30 100644 --- a/x-pack/plugins/lens/public/xy_visualization/visualization.tsx +++ b/x-pack/plugins/lens/public/xy_visualization/visualization.tsx @@ -329,6 +329,9 @@ export const xyVisualization: Visualization = { errors.push(result.payload); } } + // Note: this mechanism may produce additional errors (i.e. Missing Y and X Axis on the same layers) + // Right now this is not a big concern as Y Axis will be shown/solved first and the redundant error will be wiped out + // TODO: maybe better filter out errors in the future? } return errors.length ? errors : undefined; }, From a84fad2869b4be6387c1912d69ac93d9d97ec1b8 Mon Sep 17 00:00:00 2001 From: dej611 Date: Fri, 30 Oct 2020 12:19:11 +0100 Subject: [PATCH 26/47] :pencil2: Updated comment --- x-pack/plugins/lens/public/types.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugins/lens/public/types.ts b/x-pack/plugins/lens/public/types.ts index 6f682631c131b..0d3b2ad7d1f0b 100644 --- a/x-pack/plugins/lens/public/types.ts +++ b/x-pack/plugins/lens/public/types.ts @@ -559,7 +559,7 @@ export interface Visualization { datasourceLayers: Record ) => Ast | string | null; /** - * The frame will call this function on all visualizations at error time in order + * 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: ( From 135c40d15829b456ca33453be7dabd4e94bfad1e Mon Sep 17 00:00:00 2001 From: dej611 Date: Fri, 30 Oct 2020 16:00:32 +0100 Subject: [PATCH 27/47] :world_with_meridians: Refactor axis labels --- .../public/xy_visualization/visualization.tsx | 47 ++++++++++--------- 1 file changed, 25 insertions(+), 22 deletions(-) diff --git a/x-pack/plugins/lens/public/xy_visualization/visualization.tsx b/x-pack/plugins/lens/public/xy_visualization/visualization.tsx index 752b186e26d30..75cdaa28bb876 100644 --- a/x-pack/plugins/lens/public/xy_visualization/visualization.tsx +++ b/x-pack/plugins/lens/public/xy_visualization/visualization.tsx @@ -166,13 +166,7 @@ export const xyVisualization: Visualization = { groups: [ { groupId: 'x', - groupLabel: isHorizontal - ? i18n.translate('xpack.lens.xyChart.verticalAxisLabel', { - defaultMessage: 'Vertical axis', - }) - : i18n.translate('xpack.lens.xyChart.horizontalAxisLabel', { - defaultMessage: 'Horizontal axis', - }), + groupLabel: getAxisName('x', { isHorizontal }), accessors: layer.xAccessor ? [layer.xAccessor] : [], filterOperations: isBucketed, suggestedPriority: 1, @@ -182,13 +176,7 @@ export const xyVisualization: Visualization = { }, { groupId: 'y', - groupLabel: isHorizontal - ? i18n.translate('xpack.lens.xyChart.horizontalAxisLabel', { - defaultMessage: 'Horizontal axis', - }) - : i18n.translate('xpack.lens.xyChart.verticalAxisLabel', { - defaultMessage: 'Vertical axis', - }), + groupLabel: getAxisName('y', { isHorizontal }), accessors: layer.accessors, filterOperations: isNumericMetric, supportsMoreColumns: true, @@ -358,32 +346,47 @@ function validateLayersForDimension( return { valid: false, - payload: getMessageIdsForDimension(dimension, layerMissingAccessors), + payload: getMessageIdsForDimension(dimension, layerMissingAccessors, isHorizontalChart(layers)), }; } +function getAxisName(axis: 'x' | 'y', { isHorizontal }: { isHorizontal: boolean }) { + const vertical = i18n.translate('xpack.lens.xyChart.verticalAxisLabel', { + defaultMessage: 'Vertical axis', + }); + const horizontal = i18n.translate('xpack.lens.xyChart.horizontalAxisLabel', { + defaultMessage: 'Horizontal axis', + }); + if (axis === 'x') { + return isHorizontal ? vertical : horizontal; + } + return isHorizontal ? horizontal : vertical; +} + // i18n ids cannot be dynamically generated, hence the function below -function getMessageIdsForDimension(dimension: string, layers: number[]) { +function getMessageIdsForDimension(dimension: string, layers: number[], isHorizontal: boolean) { const layersList = layers.map((i: number) => i + 1).join(', '); switch (dimension) { case 'X': return { shortMessage: i18n.translate('xpack.lens.xyVisualization.dataFailureXShort', { - defaultMessage: `Missing Horizontal axis`, + defaultMessage: `Missing {axis}`, + values: { axis: getAxisName('x', { isHorizontal }) }, }), longMessage: i18n.translate('xpack.lens.xyVisualization.dataFailureXLong', { - defaultMessage: `{layers, plural, one {Layer} other {Layers}} {layersList} {layers, plural, one {requires} other {require}} a field for the Horizontal axis`, - values: { layers: layers.length, layersList }, + defaultMessage: `{layers, plural, one {Layer} other {Layers}} {layersList} {layers, plural, one {requires} other {require}} a field for the {axis}`, + values: { layers: layers.length, layersList, axis: getAxisName('x', { isHorizontal }) }, }), }; case 'Y': return { shortMessage: i18n.translate('xpack.lens.xyVisualization.dataFailureYShort', { - defaultMessage: `Missing Vertical axis`, + defaultMessage: `Missing {axis}`, + values: { axis: getAxisName('y', { isHorizontal }) }, }), longMessage: i18n.translate('xpack.lens.xyVisualization.dataFailureYLong', { - defaultMessage: `{layers, plural, one {Layer} other {Layers}} {layersList} {layers, plural, one {requires} other {require}} a field for the Vertical axis`, - values: { layers: layers.length, layersList }, + defaultMessage: `{layers, plural, one {Layer} other {Layers}} {layersList} {layers, plural, one {requires} other {require}} a field for the {axis}`, + values: { layers: layers.length, layersList, axis: getAxisName('y', { isHorizontal }) }, }), }; } From b12a3200214e06816524e242f3a560b643b92458 Mon Sep 17 00:00:00 2001 From: dej611 Date: Fri, 30 Oct 2020 16:20:58 +0100 Subject: [PATCH 28/47] :globe_with_meridians: Reworked few validation messages --- .../workspace_panel/workspace_panel.tsx | 3 --- .../indexpattern_datasource/indexpattern.tsx | 26 ++++++++++++++++--- 2 files changed, 23 insertions(+), 6 deletions(-) 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 c23628b3494c6..4206f35f666a6 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 @@ -386,9 +386,6 @@ export const InnerVisualizationWrapper = ({ - - {localState.configurationValidationError[0].shortMessage} - {localState.configurationValidationError[0].longMessage} diff --git a/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.tsx b/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.tsx index a01757a548038..f0bd268b27b44 100644 --- a/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.tsx +++ b/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.tsx @@ -376,19 +376,39 @@ export function getIndexPatternDatasource({ return column.sourceField; } ); + + if (originalLayersList.length === 1) { + return { + shortMessage: i18n.translate('xpack.lens.indexPattern.dataReferenceFailureShort', { + defaultMessage: 'Invalid {fields, plural, one {reference} other {references}}', + values: { + layer: layerIndex, + fields: fieldsWithBrokenReferences.length, + }, + }), + longMessage: i18n.translate('xpack.lens.indexPattern.dataReferenceFailureLong', { + defaultMessage: `{fieldsLength, plural, one {Field} other {Fields}} "{fields}" {fieldsLength, plural, one {has} other {have}} invalid reference`, + values: { + fields: fieldsWithBrokenReferences.join('", "'), + fieldsLength: fieldsWithBrokenReferences.length, + }, + }), + }; + } return { shortMessage: i18n.translate('xpack.lens.indexPattern.dataReferenceFailureShort', { defaultMessage: - 'Invalid {fields, plural, one {reference} other {references}} {hasMultipleLayers, plural, one {} other {on Layer {layer}}}', + 'Invalid {fieldsLength, plural, one {reference} other {references}} on Layer {layer}', values: { layer: layerIndex, - fields: fieldsWithBrokenReferences.length, + fieldsLength: fieldsWithBrokenReferences.length, hasMultipleLayers: originalLayersList.length, }, }), longMessage: i18n.translate('xpack.lens.indexPattern.dataReferenceFailureLong', { - defaultMessage: `{fieldsLength, plural, one {Field} other {Fields}} "{fields}" {fieldsLength, plural, one {has} other {have}} invalid reference`, + defaultMessage: `Layer {layer} has invalid {fieldsLength, plural, one {reference} other {references}} in {fieldsLength, plural, one {field} other {fields}} "{fields}"`, values: { + layer: layerIndex, fields: fieldsWithBrokenReferences.join('", "'), fieldsLength: fieldsWithBrokenReferences.length, }, From 9f5d95163788dff2a07420a381065e5a4f71a641 Mon Sep 17 00:00:00 2001 From: dej611 Date: Fri, 30 Oct 2020 17:26:24 +0100 Subject: [PATCH 29/47] :bug: Fix break down validation + percentage charts --- .../workspace_panel/workspace_panel.tsx | 40 ++++++++++++----- .../public/xy_visualization/visualization.tsx | 45 ++++++++----------- 2 files changed, 49 insertions(+), 36 deletions(-) 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 4206f35f666a6..df87f2fb3c62a 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 @@ -366,6 +366,35 @@ export const InnerVisualizationWrapper = ({ ); if (localState.configurationValidationError) { + let showExtraErrors = null; + if (localState.configurationValidationError.length > 1) { + if (localState.expandError) { + showExtraErrors = localState.configurationValidationError + .slice(1) + .map(({ longMessage }, i) => ( + {longMessage} + )); + } else { + showExtraErrors = ( + + { + setLocalState((prevState: WorkspaceState) => ({ + ...prevState, + expandError: !prevState.expandError, + })); + }} + > + {i18n.translate('xpack.lens.editorFrame.configurationFailureMoreErrors', { + defaultMessage: ` +{errors} {errors, plural, one {error} other {errors}}`, + values: { errors: localState.configurationValidationError.length - 1 }, + })} + + + ); + } + } + return ( {localState.configurationValidationError[0].longMessage} - - - {localState.configurationValidationError.length > 1 - ? i18n.translate('xpack.lens.editorFrame.configurationFailureMoreErrors', { - defaultMessage: ` +{errors} {errors, plural, one {error} other {errors}}`, - values: { errors: localState.configurationValidationError.length - 1 }, - }) - : null} - - + {showExtraErrors} ); } diff --git a/x-pack/plugins/lens/public/xy_visualization/visualization.tsx b/x-pack/plugins/lens/public/xy_visualization/visualization.tsx index 75cdaa28bb876..92f05b79f2dc0 100644 --- a/x-pack/plugins/lens/public/xy_visualization/visualization.tsx +++ b/x-pack/plugins/lens/public/xy_visualization/visualization.tsx @@ -156,11 +156,7 @@ export const xyVisualization: Visualization = { getConfiguration(props) { const layer = props.state.layers.find((l) => l.layerId === props.layerId)!; - // Make it required only if there's a multi-layer inconsistency: - // Note that X-Axis is only required if there's an yAxis defined && there's no splitAccessor defined - const someLayersAreIncomplete = props.state.layers.every( - (l) => l.accessors.length === 0 && l.xAccessor == null && l.splitAccessor == null - ); + const isHorizontal = isHorizontalChart(props.state.layers); return { groups: [ @@ -171,7 +167,6 @@ export const xyVisualization: Visualization = { filterOperations: isBucketed, suggestedPriority: 1, supportsMoreColumns: !layer.xAccessor, - required: !layer.seriesType.includes('percentage') && someLayersAreIncomplete, dataTestSubj: 'lnsXY_xDimensionPanel', }, { @@ -283,12 +278,10 @@ export const xyVisualization: Visualization = { getErrorMessages(state, frame) { // Data error handling below here - const hasNoXAccessor = ({ xAccessor, accessors, splitAccessor }: LayerConfig) => - // Notify about missing X Axis only if yAxis is missing as well - xAccessor == null && accessors.length === 0; const hasNoAccessors = ({ accessors }: LayerConfig) => accessors == null || accessors.length === 0; - const hasNoSplitAccessor = ({ splitAccessor }: LayerConfig) => splitAccessor == null; + const hasNoSplitAccessor = ({ splitAccessor, seriesType }: LayerConfig) => + seriesType.includes('percentage') && splitAccessor == null; const errors: Array<{ shortMessage: string; @@ -300,27 +293,22 @@ export const xyVisualization: Visualization = { // Order is important here: Y Axis is fundamental to exist to make it valid const checks: Array<[string, (layer: LayerConfig) => boolean]> = [ ['Y', hasNoAccessors], - ['X', hasNoXAccessor], + ['Break down', hasNoSplitAccessor], ]; - // filter those layers with no accessors at all + // filter out those layers with no accessors at all const filteredLayers = state.layers.filter( - (layer) => - !checks.every(([dimension, missingCriteria]) => missingCriteria(layer)) || - // Do not discard layers with only breakdown/split Accessor - !hasNoSplitAccessor(layer) + ({ accessors, xAccessor, splitAccessor }: LayerConfig) => + accessors.length > 0 || xAccessor != null || splitAccessor != null ); - for (const [dimension, criteria] of checks) { const result = validateLayersForDimension(dimension, filteredLayers, criteria); if (!result.valid) { errors.push(result.payload); } } - // Note: this mechanism may produce additional errors (i.e. Missing Y and X Axis on the same layers) - // Right now this is not a big concern as Y Axis will be shown/solved first and the redundant error will be wiped out - // TODO: maybe better filter out errors in the future? } + return errors.length ? errors : undefined; }, }; @@ -329,7 +317,12 @@ function validateLayersForDimension( dimension: string, layers: LayerConfig[], missingCriteria: (layer: LayerConfig) => boolean -): { valid: true } | { valid: false; payload: { shortMessage: string; longMessage: string } } { +): + | { valid: true } + | { + valid: false; + payload: { shortMessage: string; longMessage: string }; + } { // Multiple layers must be consistent: // * either a dimension is missing in ALL of them // * or should not miss on any @@ -367,15 +360,15 @@ function getAxisName(axis: 'x' | 'y', { isHorizontal }: { isHorizontal: boolean function getMessageIdsForDimension(dimension: string, layers: number[], isHorizontal: boolean) { const layersList = layers.map((i: number) => i + 1).join(', '); switch (dimension) { - case 'X': + case 'Break down': return { - shortMessage: i18n.translate('xpack.lens.xyVisualization.dataFailureXShort', { + shortMessage: i18n.translate('xpack.lens.xyVisualization.dataFailureSplitShort', { defaultMessage: `Missing {axis}`, - values: { axis: getAxisName('x', { isHorizontal }) }, + values: { axis: 'Break down by axis' }, }), - longMessage: i18n.translate('xpack.lens.xyVisualization.dataFailureXLong', { + longMessage: i18n.translate('xpack.lens.xyVisualization.dataFailureSplitLong', { defaultMessage: `{layers, plural, one {Layer} other {Layers}} {layersList} {layers, plural, one {requires} other {require}} a field for the {axis}`, - values: { layers: layers.length, layersList, axis: getAxisName('x', { isHorizontal }) }, + values: { layers: layers.length, layersList, axis: 'Break down by axis' }, }), }; case 'Y': From f984d5de2f039cc22cea8b4c1abb0d96a003e29f Mon Sep 17 00:00:00 2001 From: dej611 Date: Fri, 30 Oct 2020 17:34:24 +0100 Subject: [PATCH 30/47] :white_check_mark: Align tests with new validation logic --- .../editor_frame/workspace_panel/workspace_panel.test.tsx | 3 --- .../public/indexpattern_datasource/indexpattern.test.ts | 8 ++++---- .../lens/public/xy_visualization/visualization.test.ts | 4 ---- 3 files changed, 4 insertions(+), 11 deletions(-) diff --git a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.test.tsx b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.test.tsx index ac27ed2436160..fcfc756f7c446 100644 --- a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.test.tsx +++ b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.test.tsx @@ -574,9 +574,6 @@ describe('workspace_panel', () => { ); // EuiFlexItem duplicates internally the attribute, so we need to filter only the most inner one here - expect( - instance.find('[data-test-subj="configuration-failure-short-message"]').last().text() - ).toEqual('An error occurred'); expect( instance.find('[data-test-subj="configuration-failure-more-errors"]').last().text() ).toEqual(' +1 error'); diff --git a/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.test.ts b/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.test.ts index fc8c6bc3879cf..1f02b00c04f04 100644 --- a/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.test.ts +++ b/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.test.ts @@ -640,7 +640,7 @@ describe('IndexPattern Data Source', () => { const messages = indexPatternDatasource.getErrorMessages(state as IndexPatternPrivateState); expect(messages).toHaveLength(1); expect(messages![0]).toEqual({ - shortMessage: 'Invalid reference ', + shortMessage: 'Invalid reference', longMessage: 'Field "bytes" has invalid reference', }); }); @@ -678,7 +678,7 @@ describe('IndexPattern Data Source', () => { const messages = indexPatternDatasource.getErrorMessages(state as IndexPatternPrivateState); expect(messages).toHaveLength(1); expect(messages![0]).toEqual({ - shortMessage: 'Invalid references ', + shortMessage: 'Invalid references', longMessage: 'Fields "bytes", "memory" have invalid reference', }); }); @@ -731,11 +731,11 @@ describe('IndexPattern Data Source', () => { expect(messages).toEqual([ { shortMessage: 'Invalid references on Layer 1', - longMessage: 'Fields "bytes", "memory" have invalid reference', + longMessage: 'Layer 1 has invalid references in fields "bytes", "memory"', }, { shortMessage: 'Invalid reference on Layer 2', - longMessage: 'Field "source" has invalid reference', + longMessage: 'Layer 2 has invalid reference in field "source"', }, ]); }); 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 ffb278c09e12e..8994b3c35e0d3 100644 --- a/x-pack/plugins/lens/public/xy_visualization/visualization.test.ts +++ b/x-pack/plugins/lens/public/xy_visualization/visualization.test.ts @@ -579,10 +579,6 @@ describe('xy_visualization', () => { shortMessage: 'Missing Vertical axis', longMessage: 'Layers 2, 3 require a field for the Vertical axis', }, - { - shortMessage: 'Missing Horizontal axis', - longMessage: 'Layers 2, 3 require a field for the Horizontal axis', - }, ]); }); it("should return an error when some layers are complete but other layers aren't", () => { From c7fdd61495e4530c98625beed137a05da3c24b6a Mon Sep 17 00:00:00 2001 From: dej611 Date: Mon, 2 Nov 2020 11:05:23 +0100 Subject: [PATCH 31/47] :recycle: Fix suggestion panel validation to match main panel --- .../editor_frame/suggestion_panel.tsx | 87 ++++++++++++------- .../workspace_panel/workspace_panel.tsx | 5 +- 2 files changed, 61 insertions(+), 31 deletions(-) diff --git a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/suggestion_panel.tsx b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/suggestion_panel.tsx index 9cc86dda91bea..8f5aa1008c564 100644 --- a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/suggestion_panel.tsx +++ b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/suggestion_panel.tsx @@ -61,11 +61,28 @@ const PreviewRenderer = ({ withLabel, ExpressionRendererComponent, expression, + hasError, }: { withLabel: boolean; - expression: string; + expression: string | null | undefined; ExpressionRendererComponent: ReactExpressionRendererType; + hasError: boolean; }) => { + const onErrorMessage = ( +
+ +
+ ); return (
- { - return ( -
- -
- ); - }} - /> + {!expression || hasError ? ( + onErrorMessage + ) : ( + { + return onErrorMessage; + }} + /> + )}
); }; @@ -112,6 +119,7 @@ const SuggestionPreview = ({ expression?: Ast | null; icon: IconType; title: string; + error?: boolean; }; ExpressionRenderer: ReactExpressionRendererType; selected: boolean; @@ -129,11 +137,12 @@ const SuggestionPreview = ({ data-test-subj="lnsSuggestion" onClick={onSelect} > - {preview.expression ? ( + {preview.expression || preview.error ? ( ) : ( @@ -170,7 +179,7 @@ export function SuggestionPanel({ ? stagedPreview.visualization.activeId : activeVisualizationId; - const { suggestions, currentStateExpression } = useMemo(() => { + const { suggestions, currentStateExpression, currentStateError } = useMemo(() => { const newSuggestions = getSuggestions({ datasourceMap, datasourceStates: currentDatasourceStates, @@ -198,8 +207,22 @@ export function SuggestionPanel({ .filter((suggestion) => !suggestion.hide) .slice(0, MAX_SUGGESTIONS_DISPLAYED); + const activeDatasource = activeDatasourceId ? datasourceMap[activeDatasourceId] : null; + const datasourceValidationErrors = activeDatasourceId + ? activeDatasource?.getErrorMessages(datasourceStates[activeDatasourceId]?.state) + : undefined; + + const visualizationValidationErrors = currentVisualizationId + ? visualizationMap[currentVisualizationId]?.getErrorMessages(currentVisualizationState, frame) + : undefined; + + const validationErrors = + datasourceValidationErrors || visualizationValidationErrors + ? [...(datasourceValidationErrors || []), ...(visualizationValidationErrors || [])] + : undefined; + const newStateExpression = - currentVisualizationState && currentVisualizationId + currentVisualizationState && currentVisualizationId && !validationErrors ? preparePreviewExpression( { visualizationState: currentVisualizationState }, visualizationMap[currentVisualizationId], @@ -209,12 +232,17 @@ export function SuggestionPanel({ ) : undefined; - return { suggestions: newSuggestions, currentStateExpression: newStateExpression }; + return { + suggestions: newSuggestions, + currentStateExpression: newStateExpression, + currentStateError: validationErrors, + }; // eslint-disable-next-line react-hooks/exhaustive-deps }, [ currentDatasourceStates, currentVisualizationState, currentVisualizationId, + activeDatasourceId, datasourceMap, visualizationMap, ]); @@ -312,6 +340,7 @@ export function SuggestionPanel({ {currentVisualizationId && ( ; expressionBuildError?: Array<{ shortMessage: string; longMessage: string }>; expandError: boolean; } @@ -343,7 +342,9 @@ export const InnerVisualizationWrapper = ({ timefilter: TimefilterContract; onEvent: (event: ExpressionRendererEvent) => void; setLocalState: (dispatch: (prevState: WorkspaceState) => WorkspaceState) => void; - localState: WorkspaceState; + localState: WorkspaceState & { + configurationValidationError?: Array<{ shortMessage: string; longMessage: string }>; + }; ExpressionRendererComponent: ReactExpressionRendererType; }) => { const autoRefreshFetch$ = useMemo(() => timefilter.getAutoRefreshFetch$(), [timefilter]); From d0bce6bf99b83d97b901e94a148c09ed2c94ac9d Mon Sep 17 00:00:00 2001 From: dej611 Date: Mon, 2 Nov 2020 11:21:44 +0100 Subject: [PATCH 32/47] :globe_with_meridians: Fix i18n issues --- .../indexpattern_datasource/indexpattern.tsx | 34 +++++++++++-------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.tsx b/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.tsx index f0bd268b27b44..bb3155af6930a 100644 --- a/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.tsx +++ b/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.tsx @@ -379,20 +379,25 @@ export function getIndexPatternDatasource({ if (originalLayersList.length === 1) { return { - shortMessage: i18n.translate('xpack.lens.indexPattern.dataReferenceFailureShort', { - defaultMessage: 'Invalid {fields, plural, one {reference} other {references}}', - values: { - layer: layerIndex, - fields: fieldsWithBrokenReferences.length, - }, - }), - longMessage: i18n.translate('xpack.lens.indexPattern.dataReferenceFailureLong', { - defaultMessage: `{fieldsLength, plural, one {Field} other {Fields}} "{fields}" {fieldsLength, plural, one {has} other {have}} invalid reference`, - values: { - fields: fieldsWithBrokenReferences.join('", "'), - fieldsLength: fieldsWithBrokenReferences.length, - }, - }), + shortMessage: i18n.translate( + 'xpack.lens.indexPattern.dataReferenceFailureShortSingleLayer', + { + defaultMessage: 'Invalid {fields, plural, one {reference} other {references}}', + values: { + fields: fieldsWithBrokenReferences.length, + }, + } + ), + longMessage: i18n.translate( + 'xpack.lens.indexPattern.dataReferenceFailureLongSingleLayer', + { + defaultMessage: `{fieldsLength, plural, one {Field} other {Fields}} "{fields}" {fieldsLength, plural, one {has} other {have}} invalid reference`, + values: { + fields: fieldsWithBrokenReferences.join('", "'), + fieldsLength: fieldsWithBrokenReferences.length, + }, + } + ), }; } return { @@ -402,7 +407,6 @@ export function getIndexPatternDatasource({ values: { layer: layerIndex, fieldsLength: fieldsWithBrokenReferences.length, - hasMultipleLayers: originalLayersList.length, }, }), longMessage: i18n.translate('xpack.lens.indexPattern.dataReferenceFailureLong', { From cb0214ebf11596e71a7d7718f9e2a62e8512e6ea Mon Sep 17 00:00:00 2001 From: dej611 Date: Mon, 2 Nov 2020 11:41:50 +0100 Subject: [PATCH 33/47] :wrench: Fix some refs for validation checks in suggestions --- .../editor_frame_service/editor_frame/suggestion_panel.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/suggestion_panel.tsx b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/suggestion_panel.tsx index 8f5aa1008c564..1911d9e5b2b09 100644 --- a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/suggestion_panel.tsx +++ b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/suggestion_panel.tsx @@ -209,7 +209,7 @@ export function SuggestionPanel({ const activeDatasource = activeDatasourceId ? datasourceMap[activeDatasourceId] : null; const datasourceValidationErrors = activeDatasourceId - ? activeDatasource?.getErrorMessages(datasourceStates[activeDatasourceId]?.state) + ? activeDatasource?.getErrorMessages(currentDatasourceStates[activeDatasourceId]?.state) : undefined; const visualizationValidationErrors = currentVisualizationId @@ -237,7 +237,6 @@ export function SuggestionPanel({ currentStateExpression: newStateExpression, currentStateError: validationErrors, }; - // eslint-disable-next-line react-hooks/exhaustive-deps }, [ currentDatasourceStates, currentVisualizationState, @@ -245,6 +244,7 @@ export function SuggestionPanel({ activeDatasourceId, datasourceMap, visualizationMap, + frame, ]); const context: ExecutionContextSearch = useMemo( From 6c2772336466173ccf30fe6e2dda86082dc957a7 Mon Sep 17 00:00:00 2001 From: dej611 Date: Mon, 2 Nov 2020 12:07:27 +0100 Subject: [PATCH 34/47] :bug: Fix missing key prop in multiple errors scenario --- .../editor_frame/workspace_panel/workspace_panel.tsx | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) 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 7fae48dee4595..edd84f060e1e6 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 @@ -372,8 +372,10 @@ export const InnerVisualizationWrapper = ({ if (localState.expandError) { showExtraErrors = localState.configurationValidationError .slice(1) - .map(({ longMessage }, i) => ( - {longMessage} + .map(({ longMessage }) => ( + + {longMessage} + )); } else { showExtraErrors = ( From 8d314359e441d619a3c2ef909a93b042907c0348 Mon Sep 17 00:00:00 2001 From: dej611 Date: Mon, 2 Nov 2020 18:08:20 +0100 Subject: [PATCH 35/47] :bug: Fix swtich issue from XY to partition --- .../plugins/lens/public/xy_visualization/to_expression.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/x-pack/plugins/lens/public/xy_visualization/to_expression.ts b/x-pack/plugins/lens/public/xy_visualization/to_expression.ts index 5a3c8faa2d476..319718b22c473 100644 --- a/x-pack/plugins/lens/public/xy_visualization/to_expression.ts +++ b/x-pack/plugins/lens/public/xy_visualization/to_expression.ts @@ -26,8 +26,8 @@ export const toExpression = ( state.layers.forEach((layer) => { metadata[layer.layerId] = {}; const datasource = datasourceLayers[layer.layerId]; - datasource.getTableSpec().forEach((column) => { - const operation = datasourceLayers[layer.layerId].getOperationForColumnId(column.columnId); + datasource?.getTableSpec().forEach((column) => { + const operation = datasourceLayers[layer.layerId]?.getOperationForColumnId(column.columnId); metadata[layer.layerId][column.columnId] = operation; }); }); @@ -179,7 +179,7 @@ export const buildExpression = ( layer.accessors .concat(layer.splitAccessor ? [layer.splitAccessor] : []) .forEach((accessor) => { - const operation = datasource.getOperationForColumnId(accessor); + const operation = datasource?.getOperationForColumnId(accessor); if (operation?.label) { columnToLabel[accessor] = operation.label; } @@ -188,7 +188,7 @@ export const buildExpression = ( const xAxisOperation = datasourceLayers && - datasourceLayers[layer.layerId].getOperationForColumnId(layer.xAccessor); + datasourceLayers[layer.layerId]?.getOperationForColumnId(layer.xAccessor); const isHistogramDimension = Boolean( xAxisOperation && From 97dc1aeb98bf69fa5215dfce15b345d5fc099172 Mon Sep 17 00:00:00 2001 From: dej611 Date: Mon, 2 Nov 2020 18:21:10 +0100 Subject: [PATCH 36/47] :globe_with_meridians: Fix i18n messages and aligned tests --- .../indexpattern_datasource/indexpattern.test.ts | 16 ++++++++-------- .../indexpattern_datasource/indexpattern.tsx | 8 ++++---- .../xy_visualization/visualization.test.ts | 12 ++++++------ .../public/xy_visualization/visualization.tsx | 8 ++++---- 4 files changed, 22 insertions(+), 22 deletions(-) diff --git a/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.test.ts b/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.test.ts index 1f02b00c04f04..6e04ecb7bc543 100644 --- a/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.test.ts +++ b/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.test.ts @@ -640,8 +640,8 @@ describe('IndexPattern Data Source', () => { const messages = indexPatternDatasource.getErrorMessages(state as IndexPatternPrivateState); expect(messages).toHaveLength(1); expect(messages![0]).toEqual({ - shortMessage: 'Invalid reference', - longMessage: 'Field "bytes" has invalid reference', + shortMessage: 'Invalid reference.', + longMessage: 'Field "bytes" has an invalid reference.', }); }); @@ -678,8 +678,8 @@ describe('IndexPattern Data Source', () => { const messages = indexPatternDatasource.getErrorMessages(state as IndexPatternPrivateState); expect(messages).toHaveLength(1); expect(messages![0]).toEqual({ - shortMessage: 'Invalid references', - longMessage: 'Fields "bytes", "memory" have invalid reference', + shortMessage: 'Invalid references.', + longMessage: 'Fields "bytes", "memory" have invalid reference.', }); }); @@ -730,12 +730,12 @@ describe('IndexPattern Data Source', () => { expect(messages).toHaveLength(2); expect(messages).toEqual([ { - shortMessage: 'Invalid references on Layer 1', - longMessage: 'Layer 1 has invalid references in fields "bytes", "memory"', + shortMessage: 'Invalid references on Layer 1.', + longMessage: 'Layer 1 has invalid references in fields "bytes", "memory".', }, { - shortMessage: 'Invalid reference on Layer 2', - longMessage: 'Layer 2 has invalid reference in field "source"', + shortMessage: 'Invalid reference on Layer 2.', + longMessage: 'Layer 2 has an invalid reference in field "source".', }, ]); }); diff --git a/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.tsx b/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.tsx index bb3155af6930a..9e37b1f2d3884 100644 --- a/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.tsx +++ b/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.tsx @@ -382,7 +382,7 @@ export function getIndexPatternDatasource({ shortMessage: i18n.translate( 'xpack.lens.indexPattern.dataReferenceFailureShortSingleLayer', { - defaultMessage: 'Invalid {fields, plural, one {reference} other {references}}', + defaultMessage: 'Invalid {fields, plural, one {reference} other {references}}.', values: { fields: fieldsWithBrokenReferences.length, }, @@ -391,7 +391,7 @@ export function getIndexPatternDatasource({ longMessage: i18n.translate( 'xpack.lens.indexPattern.dataReferenceFailureLongSingleLayer', { - defaultMessage: `{fieldsLength, plural, one {Field} other {Fields}} "{fields}" {fieldsLength, plural, one {has} other {have}} invalid reference`, + defaultMessage: `{fieldsLength, plural, one {Field} other {Fields}} "{fields}" {fieldsLength, plural, one {has an} other {have}} invalid reference.`, values: { fields: fieldsWithBrokenReferences.join('", "'), fieldsLength: fieldsWithBrokenReferences.length, @@ -403,14 +403,14 @@ export function getIndexPatternDatasource({ return { shortMessage: i18n.translate('xpack.lens.indexPattern.dataReferenceFailureShort', { defaultMessage: - 'Invalid {fieldsLength, plural, one {reference} other {references}} on Layer {layer}', + 'Invalid {fieldsLength, plural, one {reference} other {references}} on Layer {layer}.', values: { layer: layerIndex, fieldsLength: fieldsWithBrokenReferences.length, }, }), longMessage: i18n.translate('xpack.lens.indexPattern.dataReferenceFailureLong', { - defaultMessage: `Layer {layer} has invalid {fieldsLength, plural, one {reference} other {references}} in {fieldsLength, plural, one {field} other {fields}} "{fields}"`, + defaultMessage: `Layer {layer} has {fieldsLength, plural, one {an invalid} other {invalid}} {fieldsLength, plural, one {reference} other {references}} in {fieldsLength, plural, one {field} other {fields}} "{fields}".`, values: { layer: layerIndex, fields: fieldsWithBrokenReferences.join('", "'), 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 8994b3c35e0d3..e23541cc2216f 100644 --- a/x-pack/plugins/lens/public/xy_visualization/visualization.test.ts +++ b/x-pack/plugins/lens/public/xy_visualization/visualization.test.ts @@ -539,8 +539,8 @@ describe('xy_visualization', () => { ) ).toEqual([ { - shortMessage: 'Missing Vertical axis', - longMessage: 'Layer 1 requires a field for the Vertical axis', + shortMessage: 'Missing Vertical axis.', + longMessage: 'Layer 1 requires a field for the Vertical axis.', }, ]); }); @@ -576,8 +576,8 @@ describe('xy_visualization', () => { ) ).toEqual([ { - shortMessage: 'Missing Vertical axis', - longMessage: 'Layers 2, 3 require a field for the Vertical axis', + shortMessage: 'Missing Vertical axis.', + longMessage: 'Layers 2, 3 require a field for the Vertical axis.', }, ]); }); @@ -611,8 +611,8 @@ describe('xy_visualization', () => { ) ).toEqual([ { - shortMessage: 'Missing Vertical axis', - longMessage: 'Layer 1 requires a field for the Vertical axis', + shortMessage: 'Missing Vertical axis.', + longMessage: 'Layer 1 requires a field for the 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 92f05b79f2dc0..544fc68029f22 100644 --- a/x-pack/plugins/lens/public/xy_visualization/visualization.tsx +++ b/x-pack/plugins/lens/public/xy_visualization/visualization.tsx @@ -363,22 +363,22 @@ function getMessageIdsForDimension(dimension: string, layers: number[], isHorizo case 'Break down': return { shortMessage: i18n.translate('xpack.lens.xyVisualization.dataFailureSplitShort', { - defaultMessage: `Missing {axis}`, + defaultMessage: `Missing {axis}.`, values: { axis: 'Break down by axis' }, }), longMessage: i18n.translate('xpack.lens.xyVisualization.dataFailureSplitLong', { - defaultMessage: `{layers, plural, one {Layer} other {Layers}} {layersList} {layers, plural, one {requires} other {require}} a field for the {axis}`, + defaultMessage: `{layers, plural, one {Layer} other {Layers}} {layersList} {layers, plural, one {requires} other {require}} a field for the {axis}.`, values: { layers: layers.length, layersList, axis: 'Break down by axis' }, }), }; case 'Y': return { shortMessage: i18n.translate('xpack.lens.xyVisualization.dataFailureYShort', { - defaultMessage: `Missing {axis}`, + defaultMessage: `Missing {axis}.`, values: { axis: getAxisName('y', { isHorizontal }) }, }), longMessage: i18n.translate('xpack.lens.xyVisualization.dataFailureYLong', { - defaultMessage: `{layers, plural, one {Layer} other {Layers}} {layersList} {layers, plural, one {requires} other {require}} a field for the {axis}`, + defaultMessage: `{layers, plural, one {Layer} other {Layers}} {layersList} {layers, plural, one {requires} other {require}} a field for the {axis}.`, values: { layers: layers.length, layersList, axis: getAxisName('y', { isHorizontal }) }, }), }; From 0a0f5d35edb2eb44db00db303c4e883c175713b2 Mon Sep 17 00:00:00 2001 From: dej611 Date: Tue, 3 Nov 2020 10:12:34 +0100 Subject: [PATCH 37/47] :bug: Fix suggestions switching bug --- .../editor_frame/suggestion_panel.tsx | 126 +++++++++--------- .../public/xy_visualization/to_expression.ts | 8 +- 2 files changed, 70 insertions(+), 64 deletions(-) diff --git a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/suggestion_panel.tsx b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/suggestion_panel.tsx index 1911d9e5b2b09..c3712eaa3abb5 100644 --- a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/suggestion_panel.tsx +++ b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/suggestion_panel.tsx @@ -179,73 +179,79 @@ export function SuggestionPanel({ ? stagedPreview.visualization.activeId : activeVisualizationId; - const { suggestions, currentStateExpression, currentStateError } = useMemo(() => { - const newSuggestions = getSuggestions({ - datasourceMap, - datasourceStates: currentDatasourceStates, - visualizationMap, - activeVisualizationId: currentVisualizationId, - visualizationState: currentVisualizationState, - }) - .filter(({ visualizationId, visualizationState: suggestionVisualizationState }) => { - const suggestionVisualization = visualizationMap[visualizationId]; - // filter out visualizations with errors - return ( - suggestionVisualization.getErrorMessages(suggestionVisualizationState, frame) == null - ); + const { suggestions, currentStateExpression, currentStateError } = useMemo( + () => { + const newSuggestions = getSuggestions({ + datasourceMap, + datasourceStates: currentDatasourceStates, + visualizationMap, + activeVisualizationId: currentVisualizationId, + visualizationState: currentVisualizationState, }) - .map((suggestion) => ({ - ...suggestion, - previewExpression: preparePreviewExpression( - suggestion, - visualizationMap[suggestion.visualizationId], - datasourceMap, - currentDatasourceStates, - frame - ), - })) - .filter((suggestion) => !suggestion.hide) - .slice(0, MAX_SUGGESTIONS_DISPLAYED); - - const activeDatasource = activeDatasourceId ? datasourceMap[activeDatasourceId] : null; - const datasourceValidationErrors = activeDatasourceId - ? activeDatasource?.getErrorMessages(currentDatasourceStates[activeDatasourceId]?.state) - : undefined; - - const visualizationValidationErrors = currentVisualizationId - ? visualizationMap[currentVisualizationId]?.getErrorMessages(currentVisualizationState, frame) - : undefined; - - const validationErrors = - datasourceValidationErrors || visualizationValidationErrors - ? [...(datasourceValidationErrors || []), ...(visualizationValidationErrors || [])] - : undefined; - - const newStateExpression = - currentVisualizationState && currentVisualizationId && !validationErrors - ? preparePreviewExpression( - { visualizationState: currentVisualizationState }, - visualizationMap[currentVisualizationId], + .filter(({ visualizationId, visualizationState: suggestionVisualizationState }) => { + const suggestionVisualization = visualizationMap[visualizationId]; + // filter out visualizations with errors + return ( + suggestionVisualization.getErrorMessages(suggestionVisualizationState, frame) == null + ); + }) + .map((suggestion) => ({ + ...suggestion, + previewExpression: preparePreviewExpression( + suggestion, + visualizationMap[suggestion.visualizationId], datasourceMap, currentDatasourceStates, frame + ), + })) + .filter((suggestion) => !suggestion.hide) + .slice(0, MAX_SUGGESTIONS_DISPLAYED); + + const activeDatasource = activeDatasourceId ? datasourceMap[activeDatasourceId] : null; + const datasourceValidationErrors = activeDatasourceId + ? activeDatasource?.getErrorMessages(currentDatasourceStates[activeDatasourceId]?.state) + : undefined; + + const visualizationValidationErrors = currentVisualizationId + ? visualizationMap[currentVisualizationId]?.getErrorMessages( + currentVisualizationState, + frame ) : undefined; - return { - suggestions: newSuggestions, - currentStateExpression: newStateExpression, - currentStateError: validationErrors, - }; - }, [ - currentDatasourceStates, - currentVisualizationState, - currentVisualizationId, - activeDatasourceId, - datasourceMap, - visualizationMap, - frame, - ]); + const validationErrors = + datasourceValidationErrors || visualizationValidationErrors + ? [...(datasourceValidationErrors || []), ...(visualizationValidationErrors || [])] + : undefined; + + const newStateExpression = + currentVisualizationState && currentVisualizationId && !validationErrors + ? preparePreviewExpression( + { visualizationState: currentVisualizationState }, + visualizationMap[currentVisualizationId], + datasourceMap, + currentDatasourceStates, + frame + ) + : undefined; + + return { + suggestions: newSuggestions, + currentStateExpression: newStateExpression, + currentStateError: validationErrors, + }; + }, + // eslint-disable-next-line react-hooks/exhaustive-deps + [ + currentDatasourceStates, + currentVisualizationState, + currentVisualizationId, + activeDatasourceId, + datasourceMap, + visualizationMap, + ] + ); const context: ExecutionContextSearch = useMemo( () => ({ diff --git a/x-pack/plugins/lens/public/xy_visualization/to_expression.ts b/x-pack/plugins/lens/public/xy_visualization/to_expression.ts index 319718b22c473..5a3c8faa2d476 100644 --- a/x-pack/plugins/lens/public/xy_visualization/to_expression.ts +++ b/x-pack/plugins/lens/public/xy_visualization/to_expression.ts @@ -26,8 +26,8 @@ export const toExpression = ( state.layers.forEach((layer) => { metadata[layer.layerId] = {}; const datasource = datasourceLayers[layer.layerId]; - datasource?.getTableSpec().forEach((column) => { - const operation = datasourceLayers[layer.layerId]?.getOperationForColumnId(column.columnId); + datasource.getTableSpec().forEach((column) => { + const operation = datasourceLayers[layer.layerId].getOperationForColumnId(column.columnId); metadata[layer.layerId][column.columnId] = operation; }); }); @@ -179,7 +179,7 @@ export const buildExpression = ( layer.accessors .concat(layer.splitAccessor ? [layer.splitAccessor] : []) .forEach((accessor) => { - const operation = datasource?.getOperationForColumnId(accessor); + const operation = datasource.getOperationForColumnId(accessor); if (operation?.label) { columnToLabel[accessor] = operation.label; } @@ -188,7 +188,7 @@ export const buildExpression = ( const xAxisOperation = datasourceLayers && - datasourceLayers[layer.layerId]?.getOperationForColumnId(layer.xAccessor); + datasourceLayers[layer.layerId].getOperationForColumnId(layer.xAccessor); const isHistogramDimension = Boolean( xAxisOperation && From 37dca5a8d88e03776cf6ecad5cc558b11b216a9d Mon Sep 17 00:00:00 2001 From: dej611 Date: Tue, 3 Nov 2020 11:52:56 +0100 Subject: [PATCH 38/47] :refactor: Add more validation + refactored validation logic in a single place --- .../editor_frame/state_helpers.ts | 28 ++++++++- .../editor_frame/suggestion_panel.tsx | 57 ++++++++++--------- .../workspace_panel/workspace_panel.tsx | 32 +++++------ 3 files changed, 70 insertions(+), 47 deletions(-) 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 8b0334ab98c14..28ad6c531e255 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 @@ -6,7 +6,7 @@ import { SavedObjectReference } from 'kibana/public'; import { Ast } from '@kbn/interpreter/common'; -import { Datasource, DatasourcePublicAPI, Visualization } from '../../types'; +import { Datasource, DatasourcePublicAPI, FramePublicAPI, Visualization } from '../../types'; import { buildExpression } from './expression_helpers'; import { Document } from '../../persistence/saved_object_store'; import { VisualizeFieldContext } from '../../../../../../src/plugins/ui_actions/public'; @@ -91,3 +91,29 @@ export async function persistedStateToExpression( datasourceLayers, }); } + +export const validateDatasourceAndVisualization = ( + currentDataSource: Datasource | null, + currentDatasourceState: unknown | null, + currentVisualization: Visualization | null, + currentVisualizationState: unknown | undefined, + frameAPI: FramePublicAPI +): + | Array<{ + shortMessage: string; + longMessage: string; + }> + | undefined => { + const datasourceValidationErrors = currentDatasourceState + ? currentDataSource?.getErrorMessages(currentDatasourceState) + : undefined; + + const visualizationValidationErrors = currentVisualizationState + ? currentVisualization?.getErrorMessages(currentVisualizationState, frameAPI) + : undefined; + + if (datasourceValidationErrors || visualizationValidationErrors) { + return [...(datasourceValidationErrors || []), ...(visualizationValidationErrors || [])]; + } + return undefined; +}; diff --git a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/suggestion_panel.tsx b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/suggestion_panel.tsx index c3712eaa3abb5..201c91ee91676 100644 --- a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/suggestion_panel.tsx +++ b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/suggestion_panel.tsx @@ -34,6 +34,7 @@ import { import { prependDatasourceExpression } from './expression_helpers'; import { trackUiEvent, trackSuggestionEvent } from '../../lens_ui_telemetry'; import { DataPublicPluginStart } from '../../../../../../src/plugins/data/public'; +import { validateDatasourceAndVisualization } from './state_helpers'; const MAX_SUGGESTIONS_DISPLAYED = 5; @@ -188,13 +189,26 @@ export function SuggestionPanel({ activeVisualizationId: currentVisualizationId, visualizationState: currentVisualizationState, }) - .filter(({ visualizationId, visualizationState: suggestionVisualizationState }) => { - const suggestionVisualization = visualizationMap[visualizationId]; - // filter out visualizations with errors - return ( - suggestionVisualization.getErrorMessages(suggestionVisualizationState, frame) == null - ); - }) + .filter((suggestion) => !suggestion.hide) + .filter( + ({ + visualizationId, + visualizationState: suggestionVisualizationState, + datasourceState: suggestionDatasourceState, + datasourceId: suggetionDatasourceId, + }) => { + return ( + validateDatasourceAndVisualization( + suggetionDatasourceId ? datasourceMap[suggetionDatasourceId] : null, + suggestionDatasourceState, + visualizationMap[visualizationId], + suggestionVisualizationState, + frame + ) == null + ); + } + ) + .slice(0, MAX_SUGGESTIONS_DISPLAYED) .map((suggestion) => ({ ...suggestion, previewExpression: preparePreviewExpression( @@ -204,26 +218,15 @@ export function SuggestionPanel({ currentDatasourceStates, frame ), - })) - .filter((suggestion) => !suggestion.hide) - .slice(0, MAX_SUGGESTIONS_DISPLAYED); - - const activeDatasource = activeDatasourceId ? datasourceMap[activeDatasourceId] : null; - const datasourceValidationErrors = activeDatasourceId - ? activeDatasource?.getErrorMessages(currentDatasourceStates[activeDatasourceId]?.state) - : undefined; - - const visualizationValidationErrors = currentVisualizationId - ? visualizationMap[currentVisualizationId]?.getErrorMessages( - currentVisualizationState, - frame - ) - : undefined; - - const validationErrors = - datasourceValidationErrors || visualizationValidationErrors - ? [...(datasourceValidationErrors || []), ...(visualizationValidationErrors || [])] - : undefined; + })); + + const validationErrors = validateDatasourceAndVisualization( + activeDatasourceId ? datasourceMap[activeDatasourceId] : null, + activeDatasourceId && currentDatasourceStates[activeDatasourceId]?.state, + currentVisualizationId ? visualizationMap[currentVisualizationId] : null, + currentVisualizationState, + frame + ); const newStateExpression = currentVisualizationState && currentVisualizationId && !validationErrors 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 edd84f060e1e6..4605a7fefe138 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 @@ -51,6 +51,7 @@ import { import { WorkspacePanelWrapper } from './workspace_panel_wrapper'; import { DropIllustration } from '../../../assets/drop_illustration'; import { getOriginalRequestErrorMessage } from '../../error_helper'; +import { validateDatasourceAndVisualization } from '../state_helpers'; export interface WorkspacePanelProps { activeVisualizationId: string | null; @@ -134,25 +135,18 @@ export function WorkspacePanel({ ? visualizationMap[activeVisualizationId] : null; - const configurationValidationError = useMemo(() => { - const activeDatasource = activeDatasourceId ? datasourceMap[activeDatasourceId] : null; - const dataMessages = activeDatasourceId - ? activeDatasource?.getErrorMessages(datasourceStates[activeDatasourceId]?.state) - : undefined; - const vizMessages = activeVisualization?.getErrorMessages(visualizationState, framePublicAPI); - - if (vizMessages || dataMessages) { - // Data first, visualization next - return [...(dataMessages || []), ...(vizMessages || [])]; - } - }, [ - activeVisualization?.getErrorMessages, - visualizationState, - activeDatasourceId, - datasourceMap, - datasourceStates, - framePublicAPI, - ]); + const configurationValidationError = useMemo( + () => + validateDatasourceAndVisualization( + activeDatasourceId ? datasourceMap[activeDatasourceId] : null, + activeDatasourceId && datasourceStates[activeDatasourceId]?.state, + activeVisualization, + visualizationState, + framePublicAPI + ), + // eslint-disable-next-line react-hooks/exhaustive-deps + [activeVisualization, visualizationState, activeDatasourceId, datasourceMap, datasourceStates] + ); const expression = useMemo( () => { From fa32b19f763c1fb3fff9df47951f4e3f87d2c4c7 Mon Sep 17 00:00:00 2001 From: dej611 Date: Tue, 3 Nov 2020 12:02:14 +0100 Subject: [PATCH 39/47] :pencil2: Add note about lint hooks disable rule --- .../editor_frame/workspace_panel/workspace_panel.tsx | 3 +++ 1 file changed, 3 insertions(+) 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 4605a7fefe138..6ed532fe2017f 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 @@ -135,6 +135,9 @@ export function WorkspacePanel({ ? visualizationMap[activeVisualizationId] : null; + // Note: mind to all these eslint disable lines: the frameAPI will change too frequently + // and to prevent race conditions it is ok to leave them there. + const configurationValidationError = useMemo( () => validateDatasourceAndVisualization( From cdb7054b2ca43e4dd54c14d93bd56fdfea31b519 Mon Sep 17 00:00:00 2001 From: dej611 Date: Wed, 4 Nov 2020 12:28:18 +0100 Subject: [PATCH 40/47] :rotating_light: Fix linting issue --- .../plugins/lens/public/xy_visualization/visualization.tsx | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/x-pack/plugins/lens/public/xy_visualization/visualization.tsx b/x-pack/plugins/lens/public/xy_visualization/visualization.tsx index d0376ae1ac58d..c7f775586ca0d 100644 --- a/x-pack/plugins/lens/public/xy_visualization/visualization.tsx +++ b/x-pack/plugins/lens/public/xy_visualization/visualization.tsx @@ -293,8 +293,9 @@ export const getXyVisualization = ({ ); }, -toExpression: (state, layers, attributes) => toExpression(state, layers, paletteService, attributes), -toPreviewExpression: (state, layers) => toPreviewExpression(state, layers, paletteService), + toExpression: (state, layers, attributes) => + toExpression(state, layers, paletteService, attributes), + toPreviewExpression: (state, layers) => toPreviewExpression(state, layers, paletteService), getErrorMessages(state, frame) { // Data error handling below here @@ -331,7 +332,7 @@ toPreviewExpression: (state, layers) => toPreviewExpression(state, layers, palet return errors.length ? errors : undefined; }, -}; +}); function validateLayersForDimension( dimension: string, From 8618b3fe6acdb272298cb8ebb7ea276047377bf6 Mon Sep 17 00:00:00 2001 From: dej611 Date: Wed, 4 Nov 2020 12:51:55 +0100 Subject: [PATCH 41/47] :building_construction: Add infra API for datasource advanced validation --- .../editor_frame/config_panel/layer_panel.tsx | 1 + x-pack/plugins/lens/public/types.ts | 6 +++++- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/layer_panel.tsx b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/layer_panel.tsx index 82f753e3520b0..ea25226059d84 100644 --- a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/layer_panel.tsx +++ b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/layer_panel.tsx @@ -448,6 +448,7 @@ export function LayerPanel( columnId: activeId, filterOperations: activeGroup.filterOperations, suggestedPriority: activeGroup?.suggestedPriority, + dimensionGroups: groups, setState: (newState: unknown) => { props.updateAll( datasourceId, diff --git a/x-pack/plugins/lens/public/types.ts b/x-pack/plugins/lens/public/types.ts index 27ab8f258bba8..0ce63860d0610 100644 --- a/x-pack/plugins/lens/public/types.ts +++ b/x-pack/plugins/lens/public/types.ts @@ -181,7 +181,10 @@ export interface Datasource { getDatasourceSuggestionsFromCurrentState: (state: T) => Array>; getPublicAPI: (props: PublicAPIProps) => DatasourcePublicAPI; - getErrorMessages: (state: T) => Array<{ shortMessage: string; longMessage: string }> | undefined; + getErrorMessages: ( + state: T, + groups?: VisualizationDimensionGroupConfig[] + ) => Array<{ shortMessage: string; longMessage: string }> | undefined; /** * uniqueLabels of dimensions exposed for aria-labels of dragged dimensions */ @@ -238,6 +241,7 @@ export type DatasourceDimensionEditorProps = DatasourceDimensionPro setState: StateSetter; core: Pick; dateRange: DateRange; + dimensionGroups: VisualizationDimensionGroupConfig[]; }; export type DatasourceDimensionTriggerProps = DatasourceDimensionProps & { From 11caf4a71b290768cce002254473050869cf8425 Mon Sep 17 00:00:00 2001 From: dej611 Date: Wed, 4 Nov 2020 13:12:20 +0100 Subject: [PATCH 42/47] :white_check_mark: Align tests with new API --- .../lens/public/pie_visualization/visualization.test.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) 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 1d111b903f42d..628d42d3de667 100644 --- a/x-pack/plugins/lens/public/pie_visualization/visualization.test.ts +++ b/x-pack/plugins/lens/public/pie_visualization/visualization.test.ts @@ -4,15 +4,20 @@ * you may not use this file except in compliance with the Elastic License. */ -import { pieVisualization } from './visualization'; +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'); const LAYER_ID = 'l1'; +const pieVisualization = getPieVisualization({ + paletteService: chartPluginMock.createPaletteRegistry(), +}); + function exampleState(): PieVisualizationState { return { shape: 'pie', From 9de02ff9f0977897b3e9ce3e9c9eb340ea23148b Mon Sep 17 00:00:00 2001 From: dej611 Date: Wed, 4 Nov 2020 15:13:49 +0100 Subject: [PATCH 43/47] :white_check_mark: Fix type issues in tests --- .../dimension_panel/dimension_panel.test.tsx | 1 + .../indexpattern_datasource/dimension_panel/droppable.test.ts | 1 + 2 files changed, 2 insertions(+) diff --git a/x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/dimension_panel.test.tsx b/x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/dimension_panel.test.tsx index 829bd333ce2cc..92a4dad14dd25 100644 --- a/x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/dimension_panel.test.tsx +++ b/x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/dimension_panel.test.tsx @@ -174,6 +174,7 @@ describe('IndexPatternDimensionEditorPanel', () => { } as unknown) as DataPublicPluginStart['fieldFormats'], } as unknown) as DataPublicPluginStart, core: {} as CoreSetup, + dimensionGroups: [], }; jest.clearAllMocks(); diff --git a/x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/droppable.test.ts b/x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/droppable.test.ts index bbd1d4e0ae3ab..dd696f8be357f 100644 --- a/x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/droppable.test.ts +++ b/x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/droppable.test.ts @@ -146,6 +146,7 @@ describe('IndexPatternDimensionEditorPanel', () => { } as unknown) as DataPublicPluginStart['fieldFormats'], } as unknown) as DataPublicPluginStart, core: {} as CoreSetup, + dimensionGroups: [], }; jest.clearAllMocks(); From 8ccd1d47e9417ed557bcf267c1934f519aed50ab Mon Sep 17 00:00:00 2001 From: dej611 Date: Wed, 4 Nov 2020 15:15:52 +0100 Subject: [PATCH 44/47] :ok_hand: Early exists added --- .../indexpattern_datasource/indexpattern.tsx | 133 +++++++++--------- 1 file changed, 68 insertions(+), 65 deletions(-) diff --git a/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.tsx b/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.tsx index 13f61cde2a7c3..0d82292780808 100644 --- a/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.tsx +++ b/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.tsx @@ -343,78 +343,81 @@ export function getIndexPatternDatasource({ getDatasourceSuggestionsForVisualizeField, getErrorMessages(state) { - if (state) { - const invalidLayers = getInvalidReferences(state); - - if (invalidLayers.length > 0) { - const realIndex = Object.values(state.layers) - .map((layer, i) => { - const filteredIndex = invalidLayers.indexOf(layer); - if (filteredIndex > -1) { - return [filteredIndex, i + 1]; - } - }) - .filter(Boolean) as Array<[number, number]>; - const invalidFieldsPerLayer: string[][] = getInvalidFieldReferencesForLayer( - invalidLayers, - state.indexPatterns - ); - const originalLayersList = Object.keys(state.layers); - - return realIndex.map(([filteredIndex, layerIndex]) => { - const fieldsWithBrokenReferences: string[] = invalidFieldsPerLayer[filteredIndex].map( - (columnId) => { - const column = invalidLayers[filteredIndex].columns[ - columnId - ] as FieldBasedIndexPatternColumn; - return column.sourceField; - } - ); - - if (originalLayersList.length === 1) { - return { - shortMessage: i18n.translate( - 'xpack.lens.indexPattern.dataReferenceFailureShortSingleLayer', - { - defaultMessage: 'Invalid {fields, plural, one {reference} other {references}}.', - values: { - fields: fieldsWithBrokenReferences.length, - }, - } - ), - longMessage: i18n.translate( - 'xpack.lens.indexPattern.dataReferenceFailureLongSingleLayer', - { - defaultMessage: `{fieldsLength, plural, one {Field} other {Fields}} "{fields}" {fieldsLength, plural, one {has an} other {have}} invalid reference.`, - values: { - fields: fieldsWithBrokenReferences.join('", "'), - fieldsLength: fieldsWithBrokenReferences.length, - }, - } - ), - }; - } - return { - shortMessage: i18n.translate('xpack.lens.indexPattern.dataReferenceFailureShort', { - defaultMessage: - 'Invalid {fieldsLength, plural, one {reference} other {references}} on Layer {layer}.', + if (!state) { + return; + } + const invalidLayers = getInvalidReferences(state); + + if (invalidLayers.length === 0) { + return; + } + + const realIndex = Object.values(state.layers) + .map((layer, i) => { + const filteredIndex = invalidLayers.indexOf(layer); + if (filteredIndex > -1) { + return [filteredIndex, i + 1]; + } + }) + .filter(Boolean) as Array<[number, number]>; + const invalidFieldsPerLayer: string[][] = getInvalidFieldReferencesForLayer( + invalidLayers, + state.indexPatterns + ); + const originalLayersList = Object.keys(state.layers); + + return realIndex.map(([filteredIndex, layerIndex]) => { + const fieldsWithBrokenReferences: string[] = invalidFieldsPerLayer[filteredIndex].map( + (columnId) => { + const column = invalidLayers[filteredIndex].columns[ + columnId + ] as FieldBasedIndexPatternColumn; + return column.sourceField; + } + ); + + if (originalLayersList.length === 1) { + return { + shortMessage: i18n.translate( + 'xpack.lens.indexPattern.dataReferenceFailureShortSingleLayer', + { + defaultMessage: 'Invalid {fields, plural, one {reference} other {references}}.', values: { - layer: layerIndex, - fieldsLength: fieldsWithBrokenReferences.length, + fields: fieldsWithBrokenReferences.length, }, - }), - longMessage: i18n.translate('xpack.lens.indexPattern.dataReferenceFailureLong', { - defaultMessage: `Layer {layer} has {fieldsLength, plural, one {an invalid} other {invalid}} {fieldsLength, plural, one {reference} other {references}} in {fieldsLength, plural, one {field} other {fields}} "{fields}".`, + } + ), + longMessage: i18n.translate( + 'xpack.lens.indexPattern.dataReferenceFailureLongSingleLayer', + { + defaultMessage: `{fieldsLength, plural, one {Field} other {Fields}} "{fields}" {fieldsLength, plural, one {has an} other {have}} invalid reference.`, values: { - layer: layerIndex, fields: fieldsWithBrokenReferences.join('", "'), fieldsLength: fieldsWithBrokenReferences.length, }, - }), - }; - }); + } + ), + }; } - } + return { + shortMessage: i18n.translate('xpack.lens.indexPattern.dataReferenceFailureShort', { + defaultMessage: + 'Invalid {fieldsLength, plural, one {reference} other {references}} on Layer {layer}.', + values: { + layer: layerIndex, + fieldsLength: fieldsWithBrokenReferences.length, + }, + }), + longMessage: i18n.translate('xpack.lens.indexPattern.dataReferenceFailureLong', { + defaultMessage: `Layer {layer} has {fieldsLength, plural, one {an invalid} other {invalid}} {fieldsLength, plural, one {reference} other {references}} in {fieldsLength, plural, one {field} other {fields}} "{fields}".`, + values: { + layer: layerIndex, + fields: fieldsWithBrokenReferences.join('", "'), + fieldsLength: fieldsWithBrokenReferences.length, + }, + }), + }; + }); }, }; From c4949ef2e8c11b91e37e7e64dedcd190878d043f Mon Sep 17 00:00:00 2001 From: dej611 Date: Fri, 6 Nov 2020 10:59:49 +0100 Subject: [PATCH 45/47] :sparkles: Add layers groups to the API --- .../editor_frame/state_helpers.ts | 12 +++++++++++- .../public/indexpattern_datasource/indexpattern.tsx | 2 +- x-pack/plugins/lens/public/types.ts | 2 +- 3 files changed, 13 insertions(+), 3 deletions(-) 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 28ad6c531e255..6fe0980b076bf 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 @@ -104,8 +104,18 @@ export const validateDatasourceAndVisualization = ( longMessage: string; }> | undefined => { + const layersGroups = + currentVisualizationState && + currentVisualization?.getLayerIds(currentVisualizationState).map((layerId) => { + return currentVisualization?.getConfiguration({ + frame: frameAPI, + layerId, + state: currentVisualizationState, + }).groups; + }); + const datasourceValidationErrors = currentDatasourceState - ? currentDataSource?.getErrorMessages(currentDatasourceState) + ? currentDataSource?.getErrorMessages(currentDatasourceState, layersGroups) : undefined; const visualizationValidationErrors = currentVisualizationState diff --git a/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.tsx b/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.tsx index 0d82292780808..7540859eb9a64 100644 --- a/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.tsx +++ b/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern.tsx @@ -342,7 +342,7 @@ export function getIndexPatternDatasource({ getDatasourceSuggestionsFromCurrentState, getDatasourceSuggestionsForVisualizeField, - getErrorMessages(state) { + getErrorMessages(state, layersGroups) { if (!state) { return; } diff --git a/x-pack/plugins/lens/public/types.ts b/x-pack/plugins/lens/public/types.ts index 0ce63860d0610..def9b90c99df1 100644 --- a/x-pack/plugins/lens/public/types.ts +++ b/x-pack/plugins/lens/public/types.ts @@ -183,7 +183,7 @@ export interface Datasource { getPublicAPI: (props: PublicAPIProps) => DatasourcePublicAPI; getErrorMessages: ( state: T, - groups?: VisualizationDimensionGroupConfig[] + layersGroups?: VisualizationDimensionGroupConfig[][] ) => Array<{ shortMessage: string; longMessage: string }> | undefined; /** * uniqueLabels of dimensions exposed for aria-labels of dragged dimensions From f6a4e4d37f2696af7bbd54453781a68940239b64 Mon Sep 17 00:00:00 2001 From: dej611 Date: Fri, 6 Nov 2020 11:07:10 +0100 Subject: [PATCH 46/47] :white_check_mark: Fix some broken test after the validation change --- .../editor_frame/editor_frame.test.tsx | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/editor_frame.test.tsx b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/editor_frame.test.tsx index f7a6f0597bf9c..b3ea14efbae80 100644 --- a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/editor_frame.test.tsx +++ b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/editor_frame.test.tsx @@ -601,7 +601,8 @@ describe('editor_frame', () => { setDatasourceState(updatedState); }); - expect(mockVisualization.getConfiguration).toHaveBeenCalledTimes(2); + // validation requires to calls this getConfiguration API + expect(mockVisualization.getConfiguration).toHaveBeenCalledTimes(6); expect(mockVisualization.getConfiguration).toHaveBeenLastCalledWith( expect.objectContaining({ state: updatedState, @@ -680,7 +681,8 @@ describe('editor_frame', () => { setDatasourceState({}); }); - expect(mockVisualization.getConfiguration).toHaveBeenCalledTimes(2); + // validation requires to calls this getConfiguration API + expect(mockVisualization.getConfiguration).toHaveBeenCalledTimes(6); expect(mockVisualization.getConfiguration).toHaveBeenLastCalledWith( expect.objectContaining({ frame: expect.objectContaining({ @@ -1193,7 +1195,8 @@ describe('editor_frame', () => { instance.find('[data-test-subj="lnsSuggestion"]').at(2).simulate('click'); }); - expect(mockVisualization.getConfiguration).toHaveBeenCalledTimes(1); + // validation requires to calls this getConfiguration API + expect(mockVisualization.getConfiguration).toHaveBeenCalledTimes(4); expect(mockVisualization.getConfiguration).toHaveBeenCalledWith( expect.objectContaining({ state: suggestionVisState, From c374b24f53a47a180ddbcbd93bcfb23d42ccf621 Mon Sep 17 00:00:00 2001 From: dej611 Date: Mon, 9 Nov 2020 10:48:58 +0100 Subject: [PATCH 47/47] :ok_hand: Move to disctionary shape --- .../editor_frame/state_helpers.ts | 28 +++++++++++++------ x-pack/plugins/lens/public/types.ts | 2 +- 2 files changed, 21 insertions(+), 9 deletions(-) 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 6fe0980b076bf..647c0f3ac9cca 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 @@ -6,7 +6,13 @@ import { SavedObjectReference } from 'kibana/public'; import { Ast } from '@kbn/interpreter/common'; -import { Datasource, DatasourcePublicAPI, FramePublicAPI, Visualization } from '../../types'; +import { + Datasource, + DatasourcePublicAPI, + FramePublicAPI, + Visualization, + VisualizationDimensionGroupConfig, +} from '../../types'; import { buildExpression } from './expression_helpers'; import { Document } from '../../persistence/saved_object_store'; import { VisualizeFieldContext } from '../../../../../../src/plugins/ui_actions/public'; @@ -106,13 +112,19 @@ export const validateDatasourceAndVisualization = ( | undefined => { const layersGroups = currentVisualizationState && - currentVisualization?.getLayerIds(currentVisualizationState).map((layerId) => { - return currentVisualization?.getConfiguration({ - frame: frameAPI, - layerId, - state: currentVisualizationState, - }).groups; - }); + currentVisualization + ?.getLayerIds(currentVisualizationState) + .reduce>((memo, layerId) => { + const groups = currentVisualization?.getConfiguration({ + frame: frameAPI, + layerId, + state: currentVisualizationState, + }).groups; + if (groups) { + memo[layerId] = groups; + } + return memo; + }, {}); const datasourceValidationErrors = currentDatasourceState ? currentDataSource?.getErrorMessages(currentDatasourceState, layersGroups) diff --git a/x-pack/plugins/lens/public/types.ts b/x-pack/plugins/lens/public/types.ts index def9b90c99df1..53e513488705e 100644 --- a/x-pack/plugins/lens/public/types.ts +++ b/x-pack/plugins/lens/public/types.ts @@ -183,7 +183,7 @@ export interface Datasource { getPublicAPI: (props: PublicAPIProps) => DatasourcePublicAPI; getErrorMessages: ( state: T, - layersGroups?: VisualizationDimensionGroupConfig[][] + layersGroups?: Record ) => Array<{ shortMessage: string; longMessage: string }> | undefined; /** * uniqueLabels of dimensions exposed for aria-labels of dragged dimensions