Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

[Lens] Improves error messages when in Dashboard #90668

Merged
merged 18 commits into from
Feb 15, 2021
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
File filter

Filter by extension

Filter by extension

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

const error = datatableVisualization.getErrorMessages(
{ layerId: 'a', columns: [{ columnId: 'b' }, { columnId: 'c' }] },
frame
);
const error = datatableVisualization.getErrorMessages({
layerId: 'a',
columns: [{ columnId: 'b' }, { columnId: 'c' }],
});

expect(error).toBeUndefined();
});
Expand All @@ -478,10 +478,10 @@ describe('Datatable Visualization', () => {
label: 'label',
});

const error = datatableVisualization.getErrorMessages(
{ layerId: 'a', columns: [{ columnId: 'b' }, { columnId: 'c' }] },
frame
);
const error = datatableVisualization.getErrorMessages({
layerId: 'a',
columns: [{ columnId: 'b' }, { columnId: 'c' }],
});

expect(error).toBeUndefined();
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,7 @@ export const datatableVisualization: Visualization<DatatableVisualizationState>
};
},

getErrorMessages(state, frame) {
getErrorMessages(state) {
return undefined;
},

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ import {
import { buildExpression } from './expression_helpers';
import { Document } from '../../persistence/saved_object_store';
import { VisualizeFieldContext } from '../../../../../../src/plugins/ui_actions/public';
import { getActiveDatasourceIdFromDoc } from './state_management';
import { ErrorMessage } from '../types';
import { getMissingCurrentDatasource, getMissingVisualizationTypeError } from '../error_helper';

export async function initializeDatasources(
datasourceMap: Record<string, Datasource>,
Expand Down Expand Up @@ -72,15 +75,20 @@ export async function persistedStateToExpression(
datasources: Record<string, Datasource>,
visualizations: Record<string, Visualization>,
doc: Document
): Promise<Ast | null> {
): Promise<{ ast: Ast | null; errors: ErrorMessage[] | undefined }> {
const {
state: { visualization: visualizationState, datasourceStates: persistedDatasourceStates },
visualizationType,
references,
title,
description,
} = doc;
if (!visualizationType) return null;
if (!visualizationType) {
return {
ast: null,
errors: [{ shortMessage: '', longMessage: getMissingVisualizationTypeError() }],
};
}
const visualization = visualizations[visualizationType!];
const datasourceStates = await initializeDatasources(
datasources,
Expand All @@ -97,29 +105,42 @@ export async function persistedStateToExpression(

const datasourceLayers = createDatasourceLayers(datasources, datasourceStates);

return buildExpression({
title,
description,
const datasourceId = getActiveDatasourceIdFromDoc(doc);
if (datasourceId == null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Strict equality works here, I'm surprised the linter allows ==

Suggested change
if (datasourceId == null) {
if (datasourceId === null) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there any particular meaning for null and undefined here, or do they have the same meaning?

eslint has a special option for null or undefined case in the rule eqeqeq, which is usually enabled.
As in steps 2 and 3 of the ECMA262 Abstract Equality spec the == operator treat them the same exact way. So unless specific distinct meaning of the two there's no difference.

return {
ast: null,
errors: [{ shortMessage: '', longMessage: getMissingCurrentDatasource() }],
};
}
const validationResult = validateDatasourceAndVisualization(
datasources[datasourceId],
datasourceStates[datasourceId].state,
visualization,
visualizationState,
datasourceMap: datasources,
datasourceStates,
datasourceLayers,
});
{ datasourceLayers }
);

return {
ast: buildExpression({
title,
description,
visualization,
visualizationState,
datasourceMap: datasources,
datasourceStates,
datasourceLayers,
}),
errors: validationResult,
};
}

export const validateDatasourceAndVisualization = (
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm just realizing this function is only operating on a single datasource - I guess this is something we have to revisit once we introduce additional datasource that can be active at the same time (like SQL). Nothing for this PR though.

currentDataSource: Datasource | null,
currentDatasourceState: unknown | null,
currentVisualization: Visualization | null,
currentVisualizationState: unknown | undefined,
frameAPI: FramePublicAPI
):
| Array<{
shortMessage: string;
longMessage: string;
}>
| undefined => {
frameAPI: Pick<FramePublicAPI, 'datasourceLayers'>
): ErrorMessage[] | undefined => {
const layersGroups = currentVisualizationState
? currentVisualization
?.getLayerIds(currentVisualizationState)
Expand All @@ -141,7 +162,7 @@ export const validateDatasourceAndVisualization = (
: undefined;

const visualizationValidationErrors = currentVisualizationState
? currentVisualization?.getErrorMessages(currentVisualizationState, frameAPI)
? currentVisualization?.getErrorMessages(currentVisualizationState)
: undefined;

if (datasourceValidationErrors?.length || visualizationValidationErrors?.length) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -155,10 +155,7 @@ export const WorkspacePanel = React.memo(function WorkspacePanel({
datasourceLayers: framePublicAPI.datasourceLayers,
});
} catch (e) {
const buildMessages = activeVisualization?.getErrorMessages(
visualizationState,
framePublicAPI
);
const buildMessages = activeVisualization?.getErrorMessages(visualizationState);
const defaultMessage = {
shortMessage: i18n.translate('xpack.lens.editorFrame.buildExpressionError', {
defaultMessage: 'An unexpected error occurred while preparing the chart',
Expand Down
Loading