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.

Sign up for GitHub

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] Clean up operations code #43784

Merged
merged 15 commits into from
Aug 28, 2019
Merged

[Lens] Clean up operations code #43784

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
b148492
simplify types and add documentation to operations framework
flash1293 Aug 22, 2019
8d380b7
make field mandatory in buildColumn of field based operation definitions
flash1293 Aug 22, 2019
694e845
handle state in app
flash1293 Aug 23, 2019
3cec779
Merge remote-tracking branch 'upstream/feature/lens' into lens/fix-qu…
flash1293 Aug 23, 2019
54ad77c
fix isMetric usages
flash1293 Aug 23, 2019
82a4fc8
fix integration tests
flash1293 Aug 23, 2019
a8ab4b5
Merge branch 'lens/fix-querybar-integration' into lens/cleanup-operat…
flash1293 Aug 23, 2019
544b4d7
eslint
flash1293 Aug 23, 2019
73cdade
improve typings
flash1293 Aug 26, 2019
3a6a23c
fix date handling on submit
flash1293 Aug 26, 2019
0ff7523
Merge branch 'lens/fix-querybar-integration' into lens/cleanup-operat…
flash1293 Aug 26, 2019
4729ae4
fix test
flash1293 Aug 26, 2019
822b41e
Merge branch 'lens/fix-querybar-integration' into lens/cleanup-operat…
flash1293 Aug 26, 2019
1625be3
Merge remote-tracking branch 'upstream/feature/lens' into lens/cleanu…
flash1293 Aug 27, 2019
2b91dfe
move to options object for changeParam and use top level imports inst…
flash1293 Aug 27, 2019
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
11 changes: 2 additions & 9 deletions x-pack/legacy/plugins/lens/public/indexpattern_plugin/dimension_panel/dimension_panel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,7 @@ import {
OperationType,
} from '../indexpattern';

import {
getAvailableOperationsByMetadata,
buildColumn,
operationDefinitionMap,
OperationDefinition,
} from '../operations';
import { getAvailableOperationsByMetadata, buildColumn, changeField } from '../operations';
import { PopoverEditor } from './popover_editor';
import { DragContextState, ChildDragDropProvider, DragDrop } from '../../drag_drop';
import { changeColumn, deleteColumn } from '../state_helpers';
Expand Down Expand Up @@ -130,9 +125,7 @@ export const IndexPatternDimensionPanel = memo(function IndexPatternDimensionPan
// If only the field has changed use the onFieldChange method on the operation to get the
// new column, otherwise use the regular buildColumn to get a new column.
const newColumn = hasFieldChanged
? (operationDefinitionMap[selectedColumn.operationType] as OperationDefinition<
IndexPatternColumn
>).onFieldChange(selectedColumn, currentIndexPattern, droppedItem.field)
? changeField(selectedColumn, currentIndexPattern, droppedItem.field)
: buildColumn({
columns: props.state.layers[props.layerId].columns,
indexPattern: currentIndexPattern,
Expand Down
12 changes: 3 additions & 9 deletions x-pack/legacy/plugins/lens/public/indexpattern_plugin/dimension_panel/popover_editor.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ import {
operationDefinitionMap,
getOperationDisplay,
buildColumn,
OperationDefinition,
changeField,
} from '../operations';
import { deleteColumn, changeColumn } from '../state_helpers';
import { FieldSelect } from './field_select';
Expand Down Expand Up @@ -284,14 +284,7 @@ export function PopoverEditor(props: PopoverEditorProps) {
) {
// If we just changed the field are not in an error state and the operation didn't change,
// we use the operations onFieldChange method to calculate the new column.
const operation = operationDefinitionMap[
choice.operationType
] as OperationDefinition<IndexPatternColumn>;
column = operation.onFieldChange(
selectedColumn,
currentIndexPattern,
fieldMap[choice.field]
);
column = changeField(selectedColumn, currentIndexPattern, fieldMap[choice.field]);
} else {
// Otherwise we'll use the buildColumn method to calculate a new column
column = buildColumn({
Expand Down Expand Up @@ -358,6 +351,7 @@ export function PopoverEditor(props: PopoverEditorProps) {
state={state}
setState={setState}
columnId={columnId}
currentColumn={state.layers[layerId].columns[columnId]}
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems trivial to compute from the other props (layerId, columnId, state), so why pass it in separately?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My comment in the description about this was probably not super clear:

Adds currentColumn to param editor props to avoid plugging it out of the state object and casting it in every param editor

It's because of types - columns is a Record<string, IndexPatternColumn>, but inside of the param editor we need the specific type, TermsIndexPatternColumn, DateHistogramIndexPatternColumn and so on. We can cast it in there (that's how it was done before), but I try to get rid of casts where possible because they are a type mismatch waiting to happen. Doing it here is also implicitly casting the type to right one, but it's only a single place compared to the n places within in the param editor components, so less surface to get it wrong in the future.

storage={props.storage}
uiSettings={props.uiSettings}
layerId={layerId}
Expand Down
66 changes: 2 additions & 64 deletions x-pack/legacy/plugins/lens/public/indexpattern_plugin/indexpattern.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,9 @@ import { Storage } from 'ui/storage';
import {
DatasourceDimensionPanelProps,
DatasourceDataPanelProps,
DimensionPriority,
Operation,
DatasourceLayerPanelProps,
} from '../types';
import { Query } from '../../../../../../src/legacy/core_plugins/data/public/query';
import { getIndexPatterns } from './loader';
import { toExpression } from './to_expression';
import { IndexPatternDimensionPanel } from './dimension_panel';
Expand All @@ -29,70 +27,10 @@ import {

import { isDraggedField } from './utils';
import { LayerPanel } from './layerpanel';
import { IndexPatternColumn } from './operations';
import { Datasource } from '..';

export type OperationType = IndexPatternColumn['operationType'];

export type IndexPatternColumn =
| DateHistogramIndexPatternColumn
| TermsIndexPatternColumn
| SumIndexPatternColumn
| AvgIndexPatternColumn
| MinIndexPatternColumn
| MaxIndexPatternColumn
| CountIndexPatternColumn
| FilterRatioIndexPatternColumn;

export interface BaseIndexPatternColumn extends Operation {
// Private
operationType: OperationType;
suggestedPriority?: DimensionPriority;
}

type Omit<T, K> = Pick<T, Exclude<keyof T, K>>;
type ParameterlessIndexPatternColumn<
TOperationType extends OperationType,
TBase extends BaseIndexPatternColumn = FieldBasedIndexPatternColumn
> = Omit<TBase, 'operationType'> & { operationType: TOperationType };

export interface FieldBasedIndexPatternColumn extends BaseIndexPatternColumn {
sourceField: string;
suggestedPriority?: DimensionPriority;
}

export interface DateHistogramIndexPatternColumn extends FieldBasedIndexPatternColumn {
operationType: 'date_histogram';
params: {
interval: string;
timeZone?: string;
};
}

export interface TermsIndexPatternColumn extends FieldBasedIndexPatternColumn {
operationType: 'terms';
params: {
size: number;
orderBy: { type: 'alphabetical' } | { type: 'column'; columnId: string };
orderDirection: 'asc' | 'desc';
};
}

export interface FilterRatioIndexPatternColumn extends BaseIndexPatternColumn {
operationType: 'filter_ratio';
params: {
numerator: Query;
denominator: Query;
};
}

export type CountIndexPatternColumn = ParameterlessIndexPatternColumn<
'count',
BaseIndexPatternColumn
>;
export type SumIndexPatternColumn = ParameterlessIndexPatternColumn<'sum'>;
export type AvgIndexPatternColumn = ParameterlessIndexPatternColumn<'avg'>;
export type MinIndexPatternColumn = ParameterlessIndexPatternColumn<'min'>;
export type MaxIndexPatternColumn = ParameterlessIndexPatternColumn<'max'>;
export { OperationType, IndexPatternColumn } from './operations';

export interface IndexPattern {
id: string;
Expand Down
219 changes: 0 additions & 219 deletions x-pack/legacy/plugins/lens/public/indexpattern_plugin/operations.ts
View file
Open in desktop

This file was deleted.

4 changes: 3 additions & 1 deletion ...dexpattern_plugin/__mocks__/operations.ts → ...tern_plugin/operations/__mocks__/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* you may not use this file except in compliance with the Elastic License.
*/

const actual = jest.requireActual('../operations');
const actual = jest.requireActual('../../operations');

jest.spyOn(actual.operationDefinitionMap.date_histogram, 'paramEditor');
jest.spyOn(actual.operationDefinitionMap.terms, 'onOtherColumnChanged');
Expand All @@ -17,5 +17,7 @@ export const {
getOperationTypesForField,
getOperationResultType,
operationDefinitionMap,
operationDefinitions,
isColumnTransferable,
changeField,
} = actual;
Loading