From 57b5546da6f47f285a0fe5ee3bfbd337f08e315e Mon Sep 17 00:00:00 2001 From: Julia Rechkunova Date: Thu, 16 Nov 2023 10:04:06 +0100 Subject: [PATCH] [Discover] Address issue with defaultColumns when changing data views (#168994) - Closes https://github.com/elastic/kibana/issues/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) --- .../src/kbn_client/kbn_client_ui_settings.ts | 2 +- .../public/__mocks__/data_view_complex.ts | 15 ++ .../main/hooks/utils/change_data_view.test.ts | 21 ++- .../main/hooks/utils/change_data_view.ts | 7 +- .../main/services/discover_state.test.ts | 2 +- .../get_switch_data_view_app_state.test.ts | 46 ++++- .../utils/get_switch_data_view_app_state.ts | 20 +- .../apps/discover/group3/_default_columns.ts | 175 ++++++++++++++++++ test/functional/apps/discover/group3/index.ts | 1 + 9 files changed, 270 insertions(+), 19 deletions(-) create mode 100644 test/functional/apps/discover/group3/_default_columns.ts diff --git a/packages/kbn-test/src/kbn_client/kbn_client_ui_settings.ts b/packages/kbn-test/src/kbn_client/kbn_client_ui_settings.ts index 8b9277fdab8b9..599184b30a461 100644 --- a/packages/kbn-test/src/kbn_client/kbn_client_ui_settings.ts +++ b/packages/kbn-test/src/kbn_client/kbn_client_ui_settings.ts @@ -10,7 +10,7 @@ import { ToolingLog } from '@kbn/tooling-log'; import { KbnClientRequester, pathWithSpace } from './kbn_client_requester'; -export type UiSettingValues = Record; +export type UiSettingValues = Record; interface UiSettingsApiResponse { settings: { [key: string]: { diff --git a/src/plugins/discover/public/__mocks__/data_view_complex.ts b/src/plugins/discover/public/__mocks__/data_view_complex.ts index 03161a0df4e3f..05051f48b155c 100644 --- a/src/plugins/discover/public/__mocks__/data_view_complex.ts +++ b/src/plugins/discover/public/__mocks__/data_view_complex.ts @@ -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', +}); diff --git a/src/plugins/discover/public/application/main/hooks/utils/change_data_view.test.ts b/src/plugins/discover/public/application/main/hooks/utils/change_data_view.test.ts index 37a503716af0e..a0266b65ed01d 100644 --- a/src/plugins/discover/public/application/main/hooks/utils/change_data_view.test.ts +++ b/src/plugins/discover/public/application/main/hooks/utils/change_data_view.test.ts @@ -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) => { @@ -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']], }); diff --git a/src/plugins/discover/public/application/main/hooks/utils/change_data_view.ts b/src/plugins/discover/public/application/main/hooks/utils/change_data_view.ts index db7a4b0c00bbe..7218ea76b2ac6 100644 --- a/src/plugins/discover/public/application/main/hooks/utils/change_data_view.ts +++ b/src/plugins/discover/public/application/main/hooks/utils/change_data_view.ts @@ -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'; @@ -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), diff --git a/src/plugins/discover/public/application/main/services/discover_state.test.ts b/src/plugins/discover/public/application/main/services/discover_state.test.ts index dd2bc17bd891a..c946ac70bbe62 100644 --- a/src/plugins/discover/public/application/main/services/discover_state.test.ts +++ b/src/plugins/discover/public/application/main/services/discover_state.test.ts @@ -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); diff --git a/src/plugins/discover/public/application/main/utils/get_switch_data_view_app_state.test.ts b/src/plugins/discover/public/application/main/utils/get_switch_data_view_app_state.test.ts index cf987c8c71090..e265a90ab291d 100644 --- a/src/plugins/discover/public/application/main/utils/get_switch_data_view_app_state.test.ts +++ b/src/plugins/discover/public/application/main/utils/get_switch_data_view_app_state.test.ts @@ -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 */ @@ -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'], @@ -66,6 +81,7 @@ describe('Discover getDataViewAppState', () => { const result = getDataViewAppState( currentDataView, nextDataView, + emptyDefaultColumns, ['name'], [ ['category', 'desc'], @@ -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']]); }); @@ -89,7 +111,13 @@ 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([]); }); @@ -97,7 +125,7 @@ describe('Discover getDataViewAppState', () => { 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']]); }); @@ -105,8 +133,16 @@ describe('Discover getDataViewAppState', () => { 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 }); diff --git a/src/plugins/discover/public/application/main/utils/get_switch_data_view_app_state.ts b/src/plugins/discover/public/application/main/utils/get_switch_data_view_app_state.ts index 064fe36d974ec..9597aa8537425 100644 --- a/src/plugins/discover/public/application/main/utils/get_switch_data_view_app_state.ts +++ b/src/plugins/discover/public/application/main/utils/get_switch_data_view_app_state.ts @@ -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'; @@ -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 = []; } diff --git a/test/functional/apps/discover/group3/_default_columns.ts b/test/functional/apps/discover/group3/_default_columns.ts new file mode 100644 index 0000000000000..64028c54a1b19 --- /dev/null +++ b/test/functional/apps/discover/group3/_default_columns.ts @@ -0,0 +1,175 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +import expect from '@kbn/expect'; +import { FtrProviderContext } from '../ftr_provider_context'; + +export default function ({ getService, getPageObjects }: FtrProviderContext) { + const dataGrid = getService('dataGrid'); + const PageObjects = getPageObjects([ + 'common', + 'discover', + 'timePicker', + 'unifiedFieldList', + 'unifiedSearch', + 'header', + ]); + const esArchiver = getService('esArchiver'); + const kibanaServer = getService('kibanaServer'); + const security = getService('security'); + const defaultSettings = { + defaultIndex: 'logstash-*', + defaultColumns: ['message', 'extension', 'DestCountry'], + hideAnnouncements: true, + }; + + describe('discover default columns', function () { + before(async () => { + await security.testUser.setRoles(['kibana_admin', 'test_logstash_reader']); + await esArchiver.loadIfNeeded('test/functional/fixtures/es_archiver/logstash_functional'); + await esArchiver.load('test/functional/fixtures/es_archiver/kibana_sample_data_logs_tsdb'); + await esArchiver.loadIfNeeded( + 'test/functional/fixtures/es_archiver/kibana_sample_data_flights' + ); + await kibanaServer.importExport.load('test/functional/fixtures/kbn_archiver/discover'); + await kibanaServer.importExport.load( + 'test/functional/fixtures/kbn_archiver/kibana_sample_data_logs_tsdb' + ); + await kibanaServer.importExport.load( + 'test/functional/fixtures/kbn_archiver/kibana_sample_data_flights_index_pattern' + ); + }); + + after(async () => { + await kibanaServer.importExport.unload('test/functional/fixtures/kbn_archiver/discover'); + await esArchiver.unload('test/functional/fixtures/es_archiver/logstash_functional'); + await esArchiver.unload('test/functional/fixtures/es_archiver/kibana_sample_data_logs_tsdb'); + await esArchiver.unload('test/functional/fixtures/es_archiver/kibana_sample_data_flights'); + await kibanaServer.savedObjects.cleanStandardList(); + }); + + beforeEach(async function () { + await PageObjects.timePicker.setDefaultAbsoluteRangeViaUiSettings(); + await kibanaServer.uiSettings.update(defaultSettings); + await PageObjects.common.navigateToApp('discover'); + await PageObjects.discover.waitUntilSearchingHasFinished(); + }); + + it('should render default columns', async function () { + expect(await dataGrid.getHeaderFields()).to.eql([ + '@timestamp', + 'message', + 'extension', + 'DestCountry', + ]); + }); + + it('should render only available default columns after switching data views', async function () { + expect(await dataGrid.getHeaderFields()).to.eql([ + '@timestamp', + 'message', + 'extension', + 'DestCountry', + ]); + + await PageObjects.unifiedSearch.switchDataView( + 'discover-dataView-switch-link', + 'Kibana Sample Data Logs (TSDB)', + false + ); + await PageObjects.discover.waitUntilSearchingHasFinished(); + await PageObjects.discover.expandTimeRangeAsSuggestedInNoResultsMessage(); + await PageObjects.discover.waitUntilSearchingHasFinished(); + expect(await dataGrid.getHeaderFields()).to.eql(['timestamp', 'message', 'extension']); + + await PageObjects.unifiedSearch.switchDataView( + 'discover-dataView-switch-link', + 'kibana_sample_data_flights', + false + ); + await PageObjects.discover.waitUntilSearchingHasFinished(); + await PageObjects.discover.expandTimeRangeAsSuggestedInNoResultsMessage(); + await PageObjects.discover.waitUntilSearchingHasFinished(); + expect(await dataGrid.getHeaderFields()).to.eql(['timestamp', 'DestCountry']); + + await PageObjects.unifiedSearch.switchDataView( + 'discover-dataView-switch-link', + 'logstash-*', + false + ); + await PageObjects.discover.waitUntilSearchingHasFinished(); + await PageObjects.discover.expandTimeRangeAsSuggestedInNoResultsMessage(); + await PageObjects.discover.waitUntilSearchingHasFinished(); + expect(await dataGrid.getHeaderFields()).to.eql(['@timestamp', 'extension']); + }); + + it('should combine selected columns and default columns after switching data views', async function () { + await PageObjects.unifiedFieldList.clickFieldListItemAdd('bytes'); + await PageObjects.unifiedFieldList.clickFieldListItemRemove('DestCountry'); + await PageObjects.unifiedFieldList.clickFieldListItemRemove('message'); + await PageObjects.discover.waitUntilSearchingHasFinished(); + + expect(await dataGrid.getHeaderFields()).to.eql(['@timestamp', 'extension', 'bytes']); + + await PageObjects.unifiedSearch.switchDataView( + 'discover-dataView-switch-link', + 'Kibana Sample Data Logs (TSDB)', + false + ); + await PageObjects.discover.waitUntilSearchingHasFinished(); + await PageObjects.discover.expandTimeRangeAsSuggestedInNoResultsMessage(); + await PageObjects.discover.waitUntilSearchingHasFinished(); + expect(await dataGrid.getHeaderFields()).to.eql([ + 'timestamp', + 'extension', + 'bytes', + 'message', + ]); + + await PageObjects.unifiedSearch.switchDataView( + 'discover-dataView-switch-link', + 'logstash-*', + false + ); + await PageObjects.discover.waitUntilSearchingHasFinished(); + await PageObjects.discover.expandTimeRangeAsSuggestedInNoResultsMessage(); + await PageObjects.discover.waitUntilSearchingHasFinished(); + expect(await dataGrid.getHeaderFields()).to.eql(['@timestamp', 'extension', 'bytes']); + }); + + it('should not change columns if discover:modifyColumnsOnSwitch is off', async function () { + await kibanaServer.uiSettings.update({ + 'discover:modifyColumnsOnSwitch': false, + }); + await PageObjects.common.navigateToApp('discover'); + await PageObjects.discover.waitUntilSearchingHasFinished(); + + expect(await dataGrid.getHeaderFields()).to.eql([ + '@timestamp', + 'message', + 'extension', + 'DestCountry', + ]); + + await PageObjects.unifiedSearch.switchDataView( + 'discover-dataView-switch-link', + 'kibana_sample_data_flights', + false + ); + await PageObjects.discover.waitUntilSearchingHasFinished(); + await PageObjects.discover.expandTimeRangeAsSuggestedInNoResultsMessage(); + await PageObjects.discover.waitUntilSearchingHasFinished(); + expect(await dataGrid.getHeaderFields()).to.eql([ + 'timestamp', + 'message', + 'extension', + 'DestCountry', + ]); + }); + }); +} diff --git a/test/functional/apps/discover/group3/index.ts b/test/functional/apps/discover/group3/index.ts index 4ac4572014a52..848bdc84def4d 100644 --- a/test/functional/apps/discover/group3/index.ts +++ b/test/functional/apps/discover/group3/index.ts @@ -20,6 +20,7 @@ export default function ({ getService, loadTestFile }: FtrProviderContext) { await esArchiver.unload('test/functional/fixtures/es_archiver/logstash_functional'); }); + loadTestFile(require.resolve('./_default_columns')); loadTestFile(require.resolve('./_drag_drop')); loadTestFile(require.resolve('./_sidebar')); loadTestFile(require.resolve('./_request_counts'));