Skip to content
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
54 commits
Select commit Hold shift + click to select a range
1bed7c0
:sparkles: First pass with visualization validation + error messages
dej611 Oct 20, 2020
bb93701
:fire: Remove indexpattern error handling for now
dej611 Oct 21, 2020
0aea6c5
:label: Fix type issues
dej611 Oct 21, 2020
d389d85
:white_check_mark: Add getErrorMessage test for data table
dej611 Oct 21, 2020
4c48b23
:white_check_mark: Add tests for pie and metric error messages
dej611 Oct 21, 2020
91a700a
:globe_with_meridians: Fix i18n checks issues
dej611 Oct 22, 2020
b1d71dd
:bug: Fix last issue
dej611 Oct 22, 2020
be2312d
:white_check_mark: Add more tests for the XY visualization validation…
dej611 Oct 22, 2020
c64603a
:ok_hand: Included all feedback from first review
dej611 Oct 27, 2020
8e77401
:pencil2: Off by one message
dej611 Oct 27, 2020
64a22ef
Merge remote-tracking branch 'upstream/master' into feature/lens/disp…
dej611 Oct 27, 2020
7089b2f
:globe_with_meridians: Fix i18n duplicate id
dej611 Oct 27, 2020
86cc058
:globe_with_meridians: Fix last i18n issue
dej611 Oct 28, 2020
4e0c179
Merge remote-tracking branch 'upstream/master' into feature/lens/disp…
dej611 Oct 28, 2020
6672302
:bug: Fixed a hook reflow issue
dej611 Oct 28, 2020
ee80a6b
:recycle:+:white_check_mark: Reworked validation flow + tests
dej611 Oct 28, 2020
f1127f4
:label: Fix type issue
dej611 Oct 28, 2020
95f9daf
:bug: Improved XY corner cases validation logic
dej611 Oct 29, 2020
ce7f857
:bug: Fix empty datatable scenario
dej611 Oct 29, 2020
d88d55d
:sparkles: + :white_check_mark: Improved error messages for invalid d…
dej611 Oct 29, 2020
694757b
:globe_with_meridians: Add missing i18n translation
dej611 Oct 29, 2020
0277aa5
:label: Fix type issues
dej611 Oct 29, 2020
822a23e
:globe_with_meridians: Fix i18n issues
dej611 Oct 29, 2020
957bd98
:sparkles: Filter out suggestions which fail to build
dej611 Oct 29, 2020
7bfabd1
Merge branch 'master' into feature/lens/display-error-workspace
kibanamachine Oct 30, 2020
2fd7a90
:truck: Migrate datatable validation logic to the building phase, han…
dej611 Oct 30, 2020
4464285
:label: Fix type issue
dej611 Oct 30, 2020
e039c4f
:pencil2: Add comment for future enhancements
dej611 Oct 30, 2020
a84fad2
:pencil2: Updated comment
dej611 Oct 30, 2020
d06f099
Merge branch 'master' into feature/lens/display-error-workspace
kibanamachine Oct 30, 2020
135c40d
:world_with_meridians: Refactor axis labels
dej611 Oct 30, 2020
b12a320
:globe_with_meridians: Reworked few validation messages
dej611 Oct 30, 2020
9f5d951
:bug: Fix break down validation + percentage charts
dej611 Oct 30, 2020
f984d5d
:white_check_mark: Align tests with new validation logic
dej611 Oct 30, 2020
5e265ae
Merge branch 'feature/lens/display-error-workspace' of github.com:dej…
dej611 Oct 30, 2020
4b3c36a
Merge branch 'master' into feature/lens/display-error-workspace
kibanamachine Nov 2, 2020
c7fdd61
:recycle: Fix suggestion panel validation to match main panel
dej611 Nov 2, 2020
d0bce6b
:globe_with_meridians: Fix i18n issues
dej611 Nov 2, 2020
cb0214e
:wrench: Fix some refs for validation checks in suggestions
dej611 Nov 2, 2020
d3fabc2
Merge branch 'feature/lens/display-error-workspace' of github.com:dej…
dej611 Nov 2, 2020
6c27723
:bug: Fix missing key prop in multiple errors scenario
dej611 Nov 2, 2020
9437162
Merge branch 'master' into feature/lens/display-error-workspace
kibanamachine Nov 2, 2020
4ac3bd8
Merge branch 'master' into feature/lens/display-error-workspace
kibanamachine Nov 2, 2020
6798112
Merge remote-tracking branch 'upstream/master' into feature/lens/disp…
dej611 Nov 2, 2020
8d31435
:bug: Fix swtich issue from XY to partition
dej611 Nov 2, 2020
97dc1ae
:globe_with_meridians: Fix i18n messages and aligned tests
dej611 Nov 2, 2020
33b71fa
Merge branch 'master' into feature/lens/display-error-workspace
kibanamachine Nov 3, 2020
0a0f5d3
:bug: Fix suggestions switching bug
dej611 Nov 3, 2020
37dca5a
:refactor: Add more validation + refactored validation logic in a sin…
dej611 Nov 3, 2020
fa32b19
:pencil2: Add note about lint hooks disable rule
dej611 Nov 3, 2020
1a440a9
Merge remote-tracking branch 'upstream/master' into feature/lens/disp…
dej611 Nov 4, 2020
cdb7054
:rotating_light: Fix linting issue
dej611 Nov 4, 2020
11caf4a
:white_check_mark: Align tests with new API
dej611 Nov 4, 2020
8ccd1d4
:ok_hand: Early exists added
dej611 Nov 4, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -372,4 +372,42 @@ describe('Datatable Visualization', () => {
});
});
});

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'] };
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.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');
});

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.getErrorMessages({ layers: [layer] }, frame);

expect(error).not.toBeDefined();
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -128,16 +134,13 @@ export const datatableVisualization: Visualization<DatatableVisualizationState>
},

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: [
{
Expand All @@ -146,7 +149,9 @@ export const datatableVisualization: Visualization<DatatableVisualizationState>
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',
Expand All @@ -158,7 +163,7 @@ export const datatableVisualization: Visualization<DatatableVisualizationState>
}),
layerId: state.layers[0].layerId,
accessors: sortedColumns.filter(
(c) => !datasource.getOperationForColumnId(c)?.isBucketed
(c) => !datasource!.getOperationForColumnId(c)?.isBucketed
),
supportsMoreColumns: true,
filterOperations: (op) => !op.isBucketed,
Expand Down Expand Up @@ -195,13 +200,11 @@ export const datatableVisualization: Visualization<DatatableVisualizationState>
},

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 {
Expand Down Expand Up @@ -232,4 +235,48 @@ export const datatableVisualization: Visualization<DatatableVisualizationState>
],
};
},

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 (
Copy link
Contributor

@flash1293 flash1293 Oct 29, 2020

Choose a reason for hiding this comment

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

This kicks in on intial state with an empty datatable:

  • Go to Lens
  • Switch to table
  • "Invalid configuration" is shown

I'm on the fence whether this is a "building state" (showing the "please drop field illustration") or an error state (showing the error message). If the user drags fields to the dimensions instead of the workspace, they will see this even if they didn't do anything wrong.

Your call if we want to treat it as a config error.
If yes: Please add a condition to this so it's not showing right away: sortedColumns.length > 0
If no: Let's remove this and simply return null instead of an expression in toExpression so no chart is rendered

Thinking about it I'm leaning towards treating it as a building error because that's what we do for xy charts:
Screenshot 2020-10-29 at 10 55 08

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 errors.length ? errors : undefined;
},
};

function getDataSourceAndSortedColumns(
state: DatatableVisualizationState,
datasourceLayers: Record<string, DatasourcePublicAPI>,
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 };
}
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -66,7 +75,8 @@ export interface WorkspacePanelProps {
}

interface WorkspaceState {
expressionBuildError: string | undefined;
configurationValidationError?: Array<{ shortMessage: string; longMessage: string }>;
expressionBuildError?: Array<{ shortMessage: string; longMessage: string }>;
expandError: boolean;
}

Expand Down Expand Up @@ -117,15 +127,40 @@ export function WorkspacePanel({
);

const [localState, setLocalState] = useState<WorkspaceState>({
expressionBuildError: undefined as string | undefined,
configurationValidationError: undefined,
expressionBuildError: undefined,
expandError: false,
});

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,
Expand All @@ -135,8 +170,19 @@ export function WorkspacePanel({
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: e.toString() }));
setLocalState((s) => ({
...s,
expressionBuildError: buildMessages ?? [defaultMessage],
}));
}
},
// eslint-disable-next-line react-hooks/exhaustive-deps
Expand Down Expand Up @@ -319,6 +365,36 @@ export const InnerVisualizationWrapper = ({
]
);

if (localState.configurationValidationError) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is also shown during dragging which looks weird (notice the drop visualization overlaying the error message):
Screenshot 2020-10-29 at 10 37 08

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was caused on the same issue as the "building phase" error above. Fixing that, fixed this too.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is still an issue if there's a dimension configured for the break down by, but no metrics and the user starts dragging a field

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, thinking about it again what should happen is to blur the error message like a rendered chart if the user hovers over the workspace with their field.

return (
<EuiFlexGroup style={{ maxWidth: '100%' }} direction="column" alignItems="center">
<EuiFlexItem>
<EuiIcon type="alert" size="xl" color="danger" />
</EuiFlexItem>
<EuiFlexItem data-test-subj="configuration-failure">
<EuiTitle size="s">
<EuiTextColor color="danger">
<FormattedMessage
id="xpack.lens.editorFrame.configurationFailure"
defaultMessage="Invalid configuration"
/>
</EuiTextColor>
</EuiTitle>
</EuiFlexItem>
<EuiFlexItem>{localState.configurationValidationError[0].shortMessage}</EuiFlexItem>
<EuiFlexItem className="eui-textBreakAll">
{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 - 1 },
})
: null}
</EuiFlexItem>
</EuiFlexGroup>
);
}

if (localState.expressionBuildError) {
return (
<EuiFlexGroup style={{ maxWidth: '100%' }} direction="column" alignItems="center">
Expand All @@ -331,7 +407,7 @@ export const InnerVisualizationWrapper = ({
defaultMessage="An error occurred in the expression"
/>
</EuiFlexItem>
<EuiFlexItem grow={false}>{localState.expressionBuildError}</EuiFlexItem>
<EuiFlexItem grow={false}>{localState.expressionBuildError[0].longMessage}</EuiFlexItem>
</EuiFlexGroup>
);
}
Expand All @@ -346,6 +422,7 @@ export const InnerVisualizationWrapper = ({
onEvent={onEvent}
renderError={(errorMessage?: string | null, error?: ExpressionRenderError | null) => {
const visibleErrorMessage = getOriginalRequestErrorMessage(error) || errorMessage;

return (
<EuiFlexGroup style={{ maxWidth: '100%' }} direction="column" alignItems="center">
<EuiFlexItem>
Expand Down
2 changes: 2 additions & 0 deletions x-pack/plugins/lens/public/editor_frame_service/mocks.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ export function createMockVisualization(): jest.Mocked<Visualization> {

setDimension: jest.fn(),
removeDimension: jest.fn(),
getErrorMessages: jest.fn((_state, _frame) => undefined),
};
}

Expand Down Expand Up @@ -90,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),
};
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -341,6 +341,32 @@ export function getIndexPatternDatasource({
},
getDatasourceSuggestionsFromCurrentState,
getDatasourceSuggestionsForVisualizeField,

getErrorMessages(state) {
if (state) {
Copy link
Contributor

@flash1293 flash1293 Nov 4, 2020

Choose a reason for hiding this comment

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

minor nit: This function can be unnested by returning early making it a little easier to follow:

if (!state) return;
const invalidLayers = getInvalidReferences(state);
if (invaludLayers.length === 0) return;
...

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 [
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Two things about this message:

  • Can we not mention layer as long as there's just a single one? It's confusing for things like pie charts
  • Can we mention the problematic field name?

{
shortMessage: i18n.translate('xpack.lens.indexPattern.dataReferenceFailureShort', {
defaultMessage: 'Invalid references',
}),
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(', ') },
}),
},
];
}
}
},
};

return indexPatternDatasource;
Expand Down
6 changes: 5 additions & 1 deletion x-pack/plugins/lens/public/indexpattern_datasource/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down
Loading