Skip to content

Commit

Permalink
[Discover] Address issue with defaultColumns when changing data views (
Browse files Browse the repository at this point in the history
…#168994)

- Closes #168581

## Summary

This PR re-adds default columns when changing data views. Only columns
which are present in the data view will be re-added.

For testing: configure some fields for `defaultColumns` on Advanced
Settings page and test switching data views on Discover page.


### Checklist

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [x] This was checked for [cross-browser
compatibility](https://www.elastic.co/support/matrix#matrix_browsers)
  • Loading branch information
jughosta authored Nov 16, 2023
1 parent 96ff9a9 commit 57b5546
Show file tree
Hide file tree
Showing 9 changed files with 270 additions and 19 deletions.
2 changes: 1 addition & 1 deletion packages/kbn-test/src/kbn_client/kbn_client_ui_settings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import { ToolingLog } from '@kbn/tooling-log';

import { KbnClientRequester, pathWithSpace } from './kbn_client_requester';

export type UiSettingValues = Record<string, string | number | boolean>;
export type UiSettingValues = Record<string, string | number | boolean | string[]>;
interface UiSettingsApiResponse {
settings: {
[key: string]: {
Expand Down
15 changes: 15 additions & 0 deletions src/plugins/discover/public/__mocks__/data_view_complex.ts
Original file line number Diff line number Diff line change
Expand Up @@ -426,3 +426,18 @@ export const dataViewAdHoc = {
}),
isPersisted: () => false,
} as DataView;

export const dataViewWithDefaultColumnMock = buildDataViewMock({
name: 'data-view-with-user-default-column',
fields: [
...fields,
{
name: 'default_column',
type: 'date',
scripted: false,
searchable: true,
aggregatable: true,
},
] as DataView['fields'],
timeFieldName: '@timestamp',
});
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,14 @@
* Side Public License, v 1.
*/

import {
dataViewComplexMock,
dataViewWithDefaultColumnMock,
} from '../../../../__mocks__/data_view_complex';
import { changeDataView } from './change_data_view';
import { savedSearchMock } from '../../../../__mocks__/saved_search';
import { discoverServiceMock } from '../../../../__mocks__/services';
import type { DataView } from '@kbn/data-views-plugin/common';
import { dataViewComplexMock } from '../../../../__mocks__/data_view_complex';
import { getDiscoverStateMock } from '../../../../__mocks__/discover_state.mock';

const setupTestParams = (dataView: DataView | undefined) => {
Expand All @@ -27,11 +30,21 @@ const setupTestParams = (dataView: DataView | undefined) => {
};

describe('changeDataView', () => {
it('should set the right app state when a valid data view (which includes the preconfigured default column) to switch to is given', async () => {
const params = setupTestParams(dataViewWithDefaultColumnMock);
await changeDataView(dataViewWithDefaultColumnMock.id!, params);
expect(params.appState.update).toHaveBeenCalledWith({
columns: ['default_column'], // default_column would be added as dataViewWithDefaultColumn has it as a mapped field
index: 'data-view-with-user-default-column-id',
sort: [['@timestamp', 'desc']],
});
});

it('should set the right app state when a valid data view to switch to is given', async () => {
const params = setupTestParams(dataViewComplexMock as DataView);
await changeDataView('data-view-with-various-field-types', params);
const params = setupTestParams(dataViewComplexMock);
await changeDataView(dataViewComplexMock.id!, params);
expect(params.appState.update).toHaveBeenCalledWith({
columns: ['default_column'],
columns: [], // default_column would not be added as dataViewComplexMock does not have it as a mapped field
index: 'data-view-with-various-field-types-id',
sort: [['data', 'desc']],
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,11 @@

import { SortOrder } from '@kbn/saved-search-plugin/public';
import { DataView } from '@kbn/data-views-plugin/common';
import { MODIFY_COLUMNS_ON_SWITCH, SORT_DEFAULT_ORDER_SETTING } from '@kbn/discover-utils';
import {
MODIFY_COLUMNS_ON_SWITCH,
SORT_DEFAULT_ORDER_SETTING,
DEFAULT_COLUMNS_SETTING,
} from '@kbn/discover-utils';
import { DiscoverInternalStateContainer } from '../../services/discover_internal_state_container';
import { DiscoverAppStateContainer } from '../../services/discover_app_state_container';
import { addLog } from '../../../../utils/add_log';
Expand Down Expand Up @@ -46,6 +50,7 @@ export async function changeDataView(
const nextAppState = getDataViewAppState(
dataView,
nextDataView,
uiSettings.get(DEFAULT_COLUMNS_SETTING, []),
state.columns || [],
(state.sort || []) as SortOrder[],
uiSettings.get(MODIFY_COLUMNS_ON_SWITCH),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -704,7 +704,7 @@ describe('Test discover state actions', () => {
await state.actions.onChangeDataView(dataViewComplexMock.id!);
await new Promise(process.nextTick);
expect(getCurrentUrl()).toMatchInlineSnapshot(
`"/#?_g=(refreshInterval:(pause:!t,value:1000),time:(from:now-15d,to:now))&_a=(columns:!(default_column),index:data-view-with-various-field-types-id,interval:auto,sort:!(!(data,desc)))"`
`"/#?_g=(refreshInterval:(pause:!t,value:1000),time:(from:now-15d,to:now))&_a=(columns:!(),index:data-view-with-various-field-types-id,interval:auto,sort:!(!(data,desc)))"`
);
await waitFor(() => {
expect(state.dataState.fetch).toHaveBeenCalledTimes(1);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
import { getDataViewAppState } from './get_switch_data_view_app_state';
import { DataView } from '@kbn/data-views-plugin/public';

const emptyDefaultColumns: string[] = [];

/**
* Helper function returning an data view
*/
Expand Down Expand Up @@ -36,23 +38,36 @@ const getDataView = (id: string, timeFieldName: string, fields: string[]) => {
};

const currentDataView = getDataView('curr', '', ['category', 'name']);
const nextDataView = getDataView('next', '', ['category']);
const nextDataView = getDataView('next', '', ['category', 'user_default_column']);

describe('Discover getDataViewAppState', () => {
test('removing fields that are not part of the next data view, keeping unknown fields ', async () => {
const result = getDataViewAppState(
currentDataView,
nextDataView,
emptyDefaultColumns,
['category', 'name', 'unknown'],
[['category', 'desc']]
);
expect(result.columns).toEqual(['category', 'unknown']);
expect(result.sort).toEqual([['category', 'desc']]);
});
test('removing fields that are not part of the next data view and adding default columns', async () => {
const result = getDataViewAppState(
currentDataView,
nextDataView,
['user_default_column', 'user_unknown_default_column'],
['category', 'name', 'user_default_column'],
[['category', 'desc']]
);
expect(result.columns).toEqual(['category', 'user_default_column']);
expect(result.sort).toEqual([['category', 'desc']]);
});
test('removing sorted by fields that are not part of the next data view', async () => {
const result = getDataViewAppState(
currentDataView,
nextDataView,
emptyDefaultColumns,
['name'],
[
['category', 'desc'],
Expand All @@ -66,6 +81,7 @@ describe('Discover getDataViewAppState', () => {
const result = getDataViewAppState(
currentDataView,
nextDataView,
emptyDefaultColumns,
['name'],
[
['category', 'desc'],
Expand All @@ -80,7 +96,13 @@ describe('Discover getDataViewAppState', () => {
const current = getDataView('a', 'timeFieldA', ['timeFieldA']);
const next = getDataView('b', 'timeFieldB', ['timeFieldB']);

const result = getDataViewAppState(current, next, [], [['timeFieldA', 'desc']]);
const result = getDataViewAppState(
current,
next,
emptyDefaultColumns,
[],
[['timeFieldA', 'desc']]
);
expect(result.columns).toEqual([]);
expect(result.sort).toEqual([['timeFieldB', 'desc']]);
});
Expand All @@ -89,24 +111,38 @@ describe('Discover getDataViewAppState', () => {
const current = getDataView('a', 'timeFieldA', ['timeFieldA']);
const next = getDataView('b', '', ['timeFieldA']);

const result = getDataViewAppState(current, next, [], [['timeFieldA', 'desc']]);
const result = getDataViewAppState(
current,
next,
emptyDefaultColumns,
[],
[['timeFieldA', 'desc']]
);
expect(result.columns).toEqual([]);
expect(result.sort).toEqual([]);
});
test('add sorting by timefield when switching from an data view without timefield to an dataView with timefield', async () => {
const current = getDataView('b', '', ['timeFieldA']);
const next = getDataView('a', 'timeFieldA', ['timeFieldA']);

const result = getDataViewAppState(current, next, [], []);
const result = getDataViewAppState(current, next, emptyDefaultColumns, [], []);
expect(result.columns).toEqual([]);
expect(result.sort).toEqual([['timeFieldA', 'desc']]);
});
test('should change sorting for similar data views', async () => {
const current = getDataView('timebased', 'timefield', ['timefield']);
const next = getDataView('timebased2', 'timefield2', ['timefield', 'timefield2']);

const result = getDataViewAppState(current, next, [], [['timefield', 'desc']]);
const result = getDataViewAppState(
current,
next,
emptyDefaultColumns,
[],
[['timefield', 'desc']]
);
expect(result.columns).toEqual([]);
expect(result.sort).toEqual([['timefield2', 'desc']]);
});

// TODO: add tests
});
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/
import { uniq } from 'lodash';
import { isOfAggregateQueryType, Query, AggregateQuery } from '@kbn/es-query';
import type { DataView } from '@kbn/data-views-plugin/public';
import type { SortOrder } from '@kbn/saved-search-plugin/public';
Expand All @@ -17,20 +18,25 @@ import { getSortArray } from '../../../utils/sorting';
export function getDataViewAppState(
currentDataView: DataView,
nextDataView: DataView,
defaultColumns: string[],
currentColumns: string[],
currentSort: SortOrder[],
modifyColumns: boolean = true,
sortDirection: string = 'desc',
query?: Query | AggregateQuery
) {
const nextColumns = modifyColumns
? currentColumns.filter(
(column) =>
nextDataView.fields.getByName(column) || !currentDataView.fields.getByName(column)
)
: currentColumns;
let columns = currentColumns || [];

if (modifyColumns) {
const currentUnknownColumns = columns.filter(
(column) => !currentDataView.fields.getByName(column) && !defaultColumns.includes(column)
);
const currentColumnsRefreshed = uniq([...columns, ...defaultColumns]);
columns = currentColumnsRefreshed.filter(
(column) => nextDataView.fields.getByName(column) || currentUnknownColumns.includes(column)
);
}

let columns = nextColumns.length ? nextColumns : [];
if (query && isOfAggregateQueryType(query)) {
columns = [];
}
Expand Down
Loading

0 comments on commit 57b5546

Please sign in to comment.