diff --git a/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern_suggestions.test.tsx b/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern_suggestions.test.tsx index c4ebcab85e722..ccae659934ba7 100644 --- a/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern_suggestions.test.tsx +++ b/x-pack/plugins/lens/public/indexpattern_datasource/indexpattern_suggestions.test.tsx @@ -965,7 +965,19 @@ describe('IndexPattern Data Source suggestions', () => { currentLayer: { ...initialState.layers.currentLayer, columns: { - cola: initialState.layers.currentLayer.columns.cola, + cola: { + dataType: 'string', + isBucketed: true, + sourceField: 'source', + label: 'values of source', + customLabel: true, + operationType: 'terms', + params: { + orderBy: { type: 'alphabetical', fallback: false }, + orderDirection: 'asc', + size: 5, + }, + }, }, columnOrder: ['cola'], }, diff --git a/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/index.ts b/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/index.ts index 0b63dc6ece974..37bd64251ed81 100644 --- a/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/index.ts +++ b/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/index.ts @@ -179,7 +179,7 @@ interface BaseOperationDefinitionProps { columns: Record ) => string; /** - * This function is called if another column in the same layer changed or got removed. + * This function is called if another column in the same layer changed or got added/removed. * Can be used to update references to other columns (e.g. for sorting). * Based on the current column and the other updated columns, this function has to * return an updated column. If not implemented, the `id` function is used instead. diff --git a/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/terms/index.tsx b/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/terms/index.tsx index 857e8b3605cfc..d226fe6f2a745 100644 --- a/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/terms/index.tsx +++ b/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/terms/index.tsx @@ -57,7 +57,9 @@ export interface TermsIndexPatternColumn extends FieldBasedIndexPatternColumn { operationType: 'terms'; params: { size: number; - orderBy: { type: 'alphabetical' } | { type: 'column'; columnId: string }; + // if order is alphabetical, the `fallback` flag indicates whether it became alphabetical because there wasn't + // another option or whether the user explicitly chose to make it alphabetical. + orderBy: { type: 'alphabetical'; fallback?: boolean } | { type: 'column'; columnId: string }; orderDirection: 'asc' | 'desc'; otherBucket?: boolean; missingBucket?: boolean; @@ -123,7 +125,7 @@ export const termsOperation: OperationDefinition { const columns = layer.columns; const currentColumn = columns[thisColumnId] as TermsIndexPatternColumn; - if (currentColumn.params.orderBy.type === 'column') { + if (currentColumn.params.orderBy.type === 'column' || currentColumn.params.orderBy.fallback) { // check whether the column is still there and still a metric - const columnSortedBy = columns[currentColumn.params.orderBy.columnId]; - if (!columnSortedBy || !isSortableByColumn(layer, changedColumnId)) { + const columnSortedBy = + currentColumn.params.orderBy.type === 'column' + ? columns[currentColumn.params.orderBy.columnId] + : undefined; + if ( + !columnSortedBy || + (currentColumn.params.orderBy.type === 'column' && + !isSortableByColumn(layer, currentColumn.params.orderBy.columnId)) + ) { + // check whether we can find another metric column to sort by + const existingMetricColumn = Object.entries(layer.columns) + .filter(([columnId]) => isSortableByColumn(layer, columnId)) + .map(([id]) => id)[0]; return { ...currentColumn, params: { ...currentColumn.params, - orderBy: { type: 'alphabetical' }, - orderDirection: 'asc', + orderBy: existingMetricColumn + ? { type: 'column', columnId: existingMetricColumn } + : { type: 'alphabetical', fallback: true }, + orderDirection: existingMetricColumn ? 'desc' : 'asc', }, }; } @@ -197,7 +212,7 @@ export const termsOperation: OperationDefinition) => + onChange={(e: React.ChangeEvent) => { + const newOrderByValue = fromValue(e.target.value); + const updatedLayer = updateColumnParam({ + layer, + columnId, + paramName: 'orderBy', + value: newOrderByValue, + }); updateLayer( updateColumnParam({ - layer, + layer: updatedLayer, columnId, - paramName: 'orderBy', - value: fromValue(e.target.value), + paramName: 'orderDirection', + value: newOrderByValue.type === 'alphabetical' ? 'asc' : 'desc', }) - ) - } + ); + }} aria-label={i18n.translate('xpack.lens.indexPattern.terms.orderBy', { defaultMessage: 'Rank by', })} diff --git a/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/terms/terms.test.tsx b/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/terms/terms.test.tsx index f75bec141ccae..2e7307f6a2ec4 100644 --- a/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/terms/terms.test.tsx +++ b/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/terms/terms.test.tsx @@ -358,7 +358,7 @@ describe('terms', () => { }, }); expect(termsColumn.params).toEqual( - expect.objectContaining({ orderBy: { type: 'alphabetical' } }) + expect.objectContaining({ orderBy: { type: 'alphabetical', fallback: true } }) ); }); @@ -469,7 +469,7 @@ describe('terms', () => { ); expect(updatedColumn.params).toEqual( expect.objectContaining({ - orderBy: { type: 'alphabetical' }, + orderBy: { type: 'alphabetical', fallback: true }, }) ); }); @@ -516,7 +516,7 @@ describe('terms', () => { ); expect(updatedColumn.params).toEqual( expect.objectContaining({ - orderBy: { type: 'alphabetical' }, + orderBy: { type: 'alphabetical', fallback: true }, }) ); }); @@ -548,7 +548,7 @@ describe('terms', () => { ); expect(termsColumn.params).toEqual( expect.objectContaining({ - orderBy: { type: 'alphabetical' }, + orderBy: { type: 'alphabetical', fallback: true }, }) ); }); @@ -592,7 +592,81 @@ describe('terms', () => { ); expect(termsColumn.params).toEqual( expect.objectContaining({ - orderBy: { type: 'alphabetical' }, + orderBy: { type: 'alphabetical', fallback: true }, + }) + ); + }); + + it('should set order to ascending if falling back to alphabetical', () => { + const termsColumn = termsOperation.onOtherColumnChanged!( + { + columns: { + col2: { + label: 'Top value of category', + dataType: 'string', + isBucketed: true, + + // Private + operationType: 'terms', + params: { + orderBy: { type: 'column', columnId: 'col1' }, + size: 3, + orderDirection: 'desc', + }, + sourceField: 'category', + }, + }, + columnOrder: [], + indexPatternId: '', + }, + 'col2', + 'col1' + ); + expect(termsColumn.params).toEqual( + expect.objectContaining({ + orderDirection: 'asc', + }) + ); + }); + + it('should switch back to descending metric sorting if alphabetical sorting was applied as fallback', () => { + const initialColumn: TermsIndexPatternColumn = { + label: 'Top value of category', + dataType: 'string', + isBucketed: true, + + // Private + operationType: 'terms', + params: { + orderBy: { type: 'alphabetical', fallback: true }, + size: 3, + orderDirection: 'asc', + }, + sourceField: 'category', + }; + const updatedColumn = termsOperation.onOtherColumnChanged!( + { + indexPatternId: '', + columnOrder: [], + columns: { + col2: initialColumn, + col1: { + label: 'Count', + dataType: 'number', + isBucketed: false, + sourceField: 'Records', + operationType: 'count', + }, + }, + }, + 'col2', + 'col1' + ); + + expect(updatedColumn.params).toEqual( + expect.objectContaining({ + orderBy: { type: 'column', columnId: 'col1' }, + orderDirection: 'desc', }) ); }); @@ -774,6 +848,7 @@ describe('terms', () => { type: 'column', columnId: 'col2', }, + orderDirection: 'desc', }, }, }, diff --git a/x-pack/plugins/lens/public/indexpattern_datasource/operations/layer_helpers.test.ts b/x-pack/plugins/lens/public/indexpattern_datasource/operations/layer_helpers.test.ts index 34e2eb2c90122..d3ca70c086cb5 100644 --- a/x-pack/plugins/lens/public/indexpattern_datasource/operations/layer_helpers.test.ts +++ b/x-pack/plugins/lens/public/indexpattern_datasource/operations/layer_helpers.test.ts @@ -352,6 +352,50 @@ describe('state_helpers', () => { ).toEqual(expect.objectContaining({ columnOrder: ['col1', 'col2'] })); }); + it('should call onOtherColumn changed on existing columns', () => { + expect( + insertNewColumn({ + layer: { + indexPatternId: '1', + columnOrder: ['col1'], + columns: { + col1: { + label: 'Top values of source', + dataType: 'string', + isBucketed: true, + + // Private + operationType: 'terms', + sourceField: 'source', + params: { + orderBy: { type: 'alphabetical', fallback: true }, + orderDirection: 'asc', + size: 5, + }, + }, + }, + }, + columnId: 'col2', + indexPattern, + op: 'sum', + field: indexPattern.fields[2], + visualizationGroups: [], + }) + ).toEqual( + expect.objectContaining({ + columns: expect.objectContaining({ + col1: expect.objectContaining({ + params: { + orderBy: { columnId: 'col2', type: 'column' }, + orderDirection: 'desc', + size: 5, + }, + }), + }), + }) + ); + }); + it('should allow multiple metrics', () => { expect( insertNewColumn({ @@ -908,7 +952,11 @@ describe('state_helpers', () => { columns: { col1: { ...termsColumn, - params: { orderBy: { type: 'alphabetical' }, orderDirection: 'asc', size: 5 }, + params: { + orderBy: { type: 'alphabetical', fallback: true }, + orderDirection: 'asc', + size: 5, + }, }, id1: expect.objectContaining({ dataType: 'number', @@ -1624,7 +1672,7 @@ describe('state_helpers', () => { ...termsColumn, params: { ...termsColumn.params, - orderBy: { type: 'alphabetical' }, + orderBy: { type: 'alphabetical', fallback: true }, orderDirection: 'asc', }, }, diff --git a/x-pack/plugins/lens/public/indexpattern_datasource/operations/layer_helpers.ts b/x-pack/plugins/lens/public/indexpattern_datasource/operations/layer_helpers.ts index 1661e5de8248e..bbe2ca4cd3d61 100644 --- a/x-pack/plugins/lens/public/indexpattern_datasource/operations/layer_helpers.ts +++ b/x-pack/plugins/lens/public/indexpattern_datasource/operations/layer_helpers.ts @@ -714,7 +714,11 @@ function addBucket( columns: { ...layer.columns, [addedColumnId]: column }, columnOrder: updatedColumnOrder, }; - return { ...tempLayer, columnOrder: getColumnOrder(tempLayer) }; + return { + ...tempLayer, + columns: adjustColumnReferencesForChangedColumn(tempLayer, addedColumnId), + columnOrder: getColumnOrder(tempLayer), + }; } export function reorderByGroups( @@ -766,7 +770,11 @@ function addMetric( [addedColumnId]: column, }, }; - return { ...tempLayer, columnOrder: getColumnOrder(tempLayer) }; + return { + ...tempLayer, + columnOrder: getColumnOrder(tempLayer), + columns: adjustColumnReferencesForChangedColumn(tempLayer, addedColumnId), + }; } export function getMetricOperationTypes(field: IndexPatternField) { diff --git a/x-pack/test/functional/apps/lens/chart_data.ts b/x-pack/test/functional/apps/lens/chart_data.ts index c4db59c020f13..887906ce2205b 100644 --- a/x-pack/test/functional/apps/lens/chart_data.ts +++ b/x-pack/test/functional/apps/lens/chart_data.ts @@ -37,19 +37,19 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) { }); const expectedData = [ - { x: '0.53.251.53', y: 4624.75 }, - { x: '0.108.3.2', y: 7359.41 }, - { x: '0.209.80.244', y: 6169.9 }, - { x: '0.228.1.71', y: 7092.8 }, - { x: '0.254.91.215', y: 3835.58 }, - { x: '__other__', y: 5727.24 }, + { x: '97.220.3.248', y: 19755 }, + { x: '169.228.188.120', y: 18994 }, + { x: '78.83.247.30', y: 17246 }, + { x: '226.82.228.233', y: 15687 }, + { x: '93.28.27.24', y: 15614.33 }, + { x: '__other__', y: 5722.77 }, ]; function assertMatchesExpectedData(state: DebugState) { expect( state.bars![0].bars.map((bar) => ({ x: bar.x, - y: Math.round(bar.y * 100) / 100, + y: Math.floor(bar.y * 100) / 100, })) ).to.eql(expectedData); } @@ -94,7 +94,7 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) { expect(terms.map((term) => (term === 'Other' ? '__other__' : term))).to.eql( expectedData.map(({ x }) => x) ); - expect(values.map((value) => Math.round(100 * Number(value.replace(',', ''))) / 100)).to.eql( + expect(values.map((value) => Math.floor(100 * Number(value.replace(',', ''))) / 100)).to.eql( expectedData.map(({ y }) => y) ); });