From fb052cfe3578b80ae72c93d2edef64668098d603 Mon Sep 17 00:00:00 2001 From: Matthias Wilhelm Date: Wed, 5 Aug 2020 10:39:31 +0200 Subject: [PATCH 1/7] Modify columns and sort when switching index pattern --- .../public/application/angular/discover.js | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/src/plugins/discover/public/application/angular/discover.js b/src/plugins/discover/public/application/angular/discover.js index 4a27f261a6220..3930315b3beca 100644 --- a/src/plugins/discover/public/application/angular/discover.js +++ b/src/plugins/discover/public/application/angular/discover.js @@ -276,8 +276,20 @@ function discoverController($element, $route, $scope, $timeout, $window, Promise }); $scope.setIndexPattern = async (id) => { - await replaceUrlAppState({ index: id }); - $route.reload(); + const nextIndexPattern = await indexPatterns.get(id); + if (nextIndexPattern) { + const nextColumns = $scope.state.columns.filter((column) => + nextIndexPattern.fields.getByName(column) + ); + const nextSort = getSortArray($scope.state.sort, nextIndexPattern); + const nextState = { + index: id, + columns: nextColumns.length ? nextColumns : ['_source'], + sort: nextSort, + }; + await replaceUrlAppState(nextState); + $route.reload(); + } }; // update data source when filters update From 4602ea2f335e5f74146d136e29f8d259c1595fb1 Mon Sep 17 00:00:00 2001 From: Matthias Wilhelm Date: Wed, 5 Aug 2020 12:02:47 +0200 Subject: [PATCH 2/7] Omit setting local state on index pattern switch --- src/plugins/discover/public/application/angular/discover.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/plugins/discover/public/application/angular/discover.js b/src/plugins/discover/public/application/angular/discover.js index 3930315b3beca..7da9b54671f33 100644 --- a/src/plugins/discover/public/application/angular/discover.js +++ b/src/plugins/discover/public/application/angular/discover.js @@ -252,6 +252,11 @@ function discoverController($element, $route, $scope, $timeout, $window, Promise if (!_.isEqual(newStatePartial, oldStatePartial)) { $scope.$evalAsync(async () => { + if (oldStatePartial.index !== newStatePartial.index) { + //in case of index switch the route has currently to be reloaded, legacy + return; + } + $scope.state = { ...newState }; // detect changes that should trigger fetching of new data From 92f87731ccafba89d82a34a5da692c4bff47c457 Mon Sep 17 00:00:00 2001 From: Matthias Wilhelm Date: Wed, 30 Sep 2020 12:52:15 +0200 Subject: [PATCH 3/7] Add unit tests --- .../public/application/angular/discover.js | 16 ++- ...get_switch_index_pattern_app_state.test.ts | 101 ++++++++++++++++++ .../get_switch_index_pattern_app_state.ts | 43 ++++++++ 3 files changed, 151 insertions(+), 9 deletions(-) create mode 100644 src/plugins/discover/public/application/helpers/get_switch_index_pattern_app_state.test.ts create mode 100644 src/plugins/discover/public/application/helpers/get_switch_index_pattern_app_state.ts diff --git a/src/plugins/discover/public/application/angular/discover.js b/src/plugins/discover/public/application/angular/discover.js index bf0885262006d..25e11a4928afb 100644 --- a/src/plugins/discover/public/application/angular/discover.js +++ b/src/plugins/discover/public/application/angular/discover.js @@ -65,6 +65,7 @@ const { import { getRootBreadcrumbs, getSavedSearchBreadcrumbs } from '../helpers/breadcrumbs'; import { validateTimeRange } from '../helpers/validate_time_range'; +import { getSwitchIndexPatternAppState } from '../helpers/get_switch_index_pattern_app_state'; import { esFilters, indexPatterns as indexPatternsUtils, @@ -283,16 +284,13 @@ function discoverController($element, $route, $scope, $timeout, $window, Promise $scope.setIndexPattern = async (id) => { const nextIndexPattern = await indexPatterns.get(id); if (nextIndexPattern) { - const nextColumns = $scope.state.columns.filter((column) => - nextIndexPattern.fields.getByName(column) + const nextAppState = getSwitchIndexPatternAppState( + nextIndexPattern, + $scope.indexPattern, + $scope.state.columns, + $scope.state.sort ); - const nextSort = getSortArray($scope.state.sort, nextIndexPattern); - const nextState = { - index: id, - columns: nextColumns.length ? nextColumns : ['_source'], - sort: nextSort, - }; - await replaceUrlAppState(nextState); + await replaceUrlAppState(nextAppState); $route.reload(); } }; diff --git a/src/plugins/discover/public/application/helpers/get_switch_index_pattern_app_state.test.ts b/src/plugins/discover/public/application/helpers/get_switch_index_pattern_app_state.test.ts new file mode 100644 index 0000000000000..d57607c11f063 --- /dev/null +++ b/src/plugins/discover/public/application/helpers/get_switch_index_pattern_app_state.test.ts @@ -0,0 +1,101 @@ +/* + * Licensed to Elasticsearch B.V. under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch B.V. licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { getSwitchIndexPatternAppState } from './get_switch_index_pattern_app_state'; +import { IIndexPatternFieldList, IndexPattern } from '../../../../data/common/index_patterns'; + +const indexPatternPrev: IndexPattern = { + id: 'prev', + getFieldByName(name) { + return this.fields.getByName(name); + }, + fields: { + getByName: (name: string) => { + const fields = [ + { name: 'category', sortable: true }, + { name: 'name', sortable: true }, + ] as IIndexPatternFieldList; + return fields.find((field) => field.name === name); + }, + }, +} as IndexPattern; + +const indexPatternNext = { + id: 'next', + getFieldByName(name) { + return this.fields.getByName(name); + }, + fields: { + getByName: (name: string) => { + const fields = [{ name: 'category', sortable: true }] as IIndexPatternFieldList; + return fields.find((field) => field.name === name); + }, + }, +} as IndexPattern; + +describe('Discover getSwitchIndexPatternAppState', () => { + test('removing fields that are not part of the next index pattern, keeping unknown fields ', async () => { + const result = getSwitchIndexPatternAppState( + indexPatternNext, + indexPatternPrev, + ['category', 'name', 'unknown'], + [['category', 'desc']] + ); + expect(result).toMatchInlineSnapshot(` + Object { + "columns": Array [ + "category", + "unknown", + ], + "index": "next", + "sort": Array [ + Array [ + "category", + "desc", + ], + ], + } + `); + }); + test('removing sorted by fields that are not part of the next index pattern', async () => { + const result = getSwitchIndexPatternAppState( + indexPatternNext, + indexPatternPrev, + ['name'], + [ + ['category', 'desc'], + ['name', 'asc'], + ] + ); + expect(result).toMatchInlineSnapshot(` + Object { + "columns": Array [ + "_source", + ], + "index": "next", + "sort": Array [ + Array [ + "category", + "desc", + ], + ], + } + `); + }); +}); diff --git a/src/plugins/discover/public/application/helpers/get_switch_index_pattern_app_state.ts b/src/plugins/discover/public/application/helpers/get_switch_index_pattern_app_state.ts new file mode 100644 index 0000000000000..e39d998b90355 --- /dev/null +++ b/src/plugins/discover/public/application/helpers/get_switch_index_pattern_app_state.ts @@ -0,0 +1,43 @@ +/* + * Licensed to Elasticsearch B.V. under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch B.V. licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { getSortArray } from '../angular/doc_table'; +import { SortPairArr } from '../angular/doc_table/lib/get_sort'; +import { IndexPattern } from '../../kibana_services'; + +/** + * Helper function to remove or adapt the currently selected columns/sort to be valid with the next + * index pattern, returns a new state object + */ +export function getSwitchIndexPatternAppState( + nextIndexPattern: IndexPattern, + prevIndexPattern: IndexPattern, + currentColumns: string[], + currentSort: SortPairArr[] +) { + const nextColumns = currentColumns.filter( + (column) => + nextIndexPattern.fields.getByName(column) || !prevIndexPattern.fields.getByName(column) + ); + const nextSort = getSortArray(currentSort, nextIndexPattern); + return { + index: nextIndexPattern.id, + columns: nextColumns.length ? nextColumns : ['_source'], + sort: nextSort, + }; +} From 36d9b382f748ba0c182968f280cf6abaac1bd3cc Mon Sep 17 00:00:00 2001 From: Matthias Wilhelm Date: Thu, 1 Oct 2020 10:44:22 +0200 Subject: [PATCH 4/7] Add uiSetting to allow switch to legacy behavior --- src/plugins/discover/common/index.ts | 1 + .../public/application/angular/discover.js | 6 +- ...get_switch_index_pattern_app_state.test.ts | 59 ++++++++----------- .../get_switch_index_pattern_app_state.ts | 15 +++-- src/plugins/discover/server/ui_settings.ts | 14 +++++ 5 files changed, 52 insertions(+), 43 deletions(-) diff --git a/src/plugins/discover/common/index.ts b/src/plugins/discover/common/index.ts index 72030d91220b5..4334af63539e3 100644 --- a/src/plugins/discover/common/index.ts +++ b/src/plugins/discover/common/index.ts @@ -27,3 +27,4 @@ export const FIELDS_LIMIT_SETTING = 'fields:popularLimit'; export const CONTEXT_DEFAULT_SIZE_SETTING = 'context:defaultSize'; export const CONTEXT_STEP_SETTING = 'context:step'; export const CONTEXT_TIE_BREAKER_FIELDS_SETTING = 'context:tieBreakerFields'; +export const MODIFY_COLUMNS_ON_SWITCH = 'discover:modifyColumnsOnSwitch'; diff --git a/src/plugins/discover/public/application/angular/discover.js b/src/plugins/discover/public/application/angular/discover.js index 30be31b2eda5d..078a047324113 100644 --- a/src/plugins/discover/public/application/angular/discover.js +++ b/src/plugins/discover/public/application/angular/discover.js @@ -80,6 +80,7 @@ import { SORT_DEFAULT_ORDER_SETTING, SEARCH_ON_PAGE_LOAD_SETTING, DOC_HIDE_TIME_COLUMN_SETTING, + MODIFY_COLUMNS_ON_SWITCH, } from '../../../common'; const fetchStatuses = { @@ -285,10 +286,11 @@ function discoverController($element, $route, $scope, $timeout, $window, Promise const nextIndexPattern = await indexPatterns.get(id); if (nextIndexPattern) { const nextAppState = getSwitchIndexPatternAppState( - nextIndexPattern, $scope.indexPattern, + nextIndexPattern, $scope.state.columns, - $scope.state.sort + $scope.state.sort, + config.get(MODIFY_COLUMNS_ON_SWITCH) ); await replaceUrlAppState(nextAppState); $route.reload(); diff --git a/src/plugins/discover/public/application/helpers/get_switch_index_pattern_app_state.test.ts b/src/plugins/discover/public/application/helpers/get_switch_index_pattern_app_state.test.ts index d57607c11f063..d35346ed24737 100644 --- a/src/plugins/discover/public/application/helpers/get_switch_index_pattern_app_state.test.ts +++ b/src/plugins/discover/public/application/helpers/get_switch_index_pattern_app_state.test.ts @@ -20,7 +20,7 @@ import { getSwitchIndexPatternAppState } from './get_switch_index_pattern_app_state'; import { IIndexPatternFieldList, IndexPattern } from '../../../../data/common/index_patterns'; -const indexPatternPrev: IndexPattern = { +const currentIndexPattern: IndexPattern = { id: 'prev', getFieldByName(name) { return this.fields.getByName(name); @@ -36,7 +36,7 @@ const indexPatternPrev: IndexPattern = { }, } as IndexPattern; -const indexPatternNext = { +const nextIndexPattern = { id: 'next', getFieldByName(name) { return this.fields.getByName(name); @@ -52,50 +52,39 @@ const indexPatternNext = { describe('Discover getSwitchIndexPatternAppState', () => { test('removing fields that are not part of the next index pattern, keeping unknown fields ', async () => { const result = getSwitchIndexPatternAppState( - indexPatternNext, - indexPatternPrev, + currentIndexPattern, + nextIndexPattern, ['category', 'name', 'unknown'], [['category', 'desc']] ); - expect(result).toMatchInlineSnapshot(` - Object { - "columns": Array [ - "category", - "unknown", - ], - "index": "next", - "sort": Array [ - Array [ - "category", - "desc", - ], - ], - } - `); + expect(result.columns).toEqual(['category', 'unknown']); + expect(result.sort).toEqual([['category', 'desc']]); }); test('removing sorted by fields that are not part of the next index pattern', async () => { const result = getSwitchIndexPatternAppState( - indexPatternNext, - indexPatternPrev, + currentIndexPattern, + nextIndexPattern, ['name'], [ ['category', 'desc'], ['name', 'asc'], ] ); - expect(result).toMatchInlineSnapshot(` - Object { - "columns": Array [ - "_source", - ], - "index": "next", - "sort": Array [ - Array [ - "category", - "desc", - ], - ], - } - `); + expect(result.columns).toEqual(['_source']); + expect(result.sort).toEqual([['category', 'desc']]); + }); + test('removing sorted by fields that without modifying columns', async () => { + const result = getSwitchIndexPatternAppState( + currentIndexPattern, + nextIndexPattern, + ['name'], + [ + ['category', 'desc'], + ['name', 'asc'], + ], + false + ); + expect(result.columns).toEqual(['name']); + expect(result.sort).toEqual([['category', 'desc']]); }); }); diff --git a/src/plugins/discover/public/application/helpers/get_switch_index_pattern_app_state.ts b/src/plugins/discover/public/application/helpers/get_switch_index_pattern_app_state.ts index e39d998b90355..458b9b7e066fd 100644 --- a/src/plugins/discover/public/application/helpers/get_switch_index_pattern_app_state.ts +++ b/src/plugins/discover/public/application/helpers/get_switch_index_pattern_app_state.ts @@ -25,15 +25,18 @@ import { IndexPattern } from '../../kibana_services'; * index pattern, returns a new state object */ export function getSwitchIndexPatternAppState( + currentIndexPattern: IndexPattern, nextIndexPattern: IndexPattern, - prevIndexPattern: IndexPattern, currentColumns: string[], - currentSort: SortPairArr[] + currentSort: SortPairArr[], + modifyColumns: boolean = true ) { - const nextColumns = currentColumns.filter( - (column) => - nextIndexPattern.fields.getByName(column) || !prevIndexPattern.fields.getByName(column) - ); + const nextColumns = modifyColumns + ? currentColumns.filter( + (column) => + nextIndexPattern.fields.getByName(column) || !currentIndexPattern.fields.getByName(column) + ) + : currentColumns; const nextSort = getSortArray(currentSort, nextIndexPattern); return { index: nextIndexPattern.id, diff --git a/src/plugins/discover/server/ui_settings.ts b/src/plugins/discover/server/ui_settings.ts index 3eca11cc584a9..a685df07ab9fb 100644 --- a/src/plugins/discover/server/ui_settings.ts +++ b/src/plugins/discover/server/ui_settings.ts @@ -32,6 +32,7 @@ import { CONTEXT_DEFAULT_SIZE_SETTING, CONTEXT_STEP_SETTING, CONTEXT_TIE_BREAKER_FIELDS_SETTING, + MODIFY_COLUMNS_ON_SWITCH, } from '../common'; export const uiSettings: Record = { @@ -163,4 +164,17 @@ export const uiSettings: Record = { category: ['discover'], schema: schema.arrayOf(schema.string()), }, + [MODIFY_COLUMNS_ON_SWITCH]: { + name: i18n.translate('discover.advancedSettings.discover.modifyColumnsOnSwitchTitle', { + defaultMessage: 'Switch index pattern behavior', + }), + value: true, + description: i18n.translate('discover.advancedSettings.discover.modifyColumnsOnSwitchText', { + defaultMessage: + 'When switching index patterns, selected columns, that are not available in the ' + + 'next index pattern, are removed.', + }), + category: ['discover'], + schema: schema.boolean(), + }, }; From df08d57ad604d7270e23e8012e1201cebe87ddb4 Mon Sep 17 00:00:00 2001 From: Matthias Wilhelm Date: Thu, 1 Oct 2020 10:49:40 +0200 Subject: [PATCH 5/7] Change title of ui settings --- src/plugins/discover/server/ui_settings.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/plugins/discover/server/ui_settings.ts b/src/plugins/discover/server/ui_settings.ts index a685df07ab9fb..1f8407f14638b 100644 --- a/src/plugins/discover/server/ui_settings.ts +++ b/src/plugins/discover/server/ui_settings.ts @@ -166,7 +166,7 @@ export const uiSettings: Record = { }, [MODIFY_COLUMNS_ON_SWITCH]: { name: i18n.translate('discover.advancedSettings.discover.modifyColumnsOnSwitchTitle', { - defaultMessage: 'Switch index pattern behavior', + defaultMessage: 'Modify columns when index pattern is switched', }), value: true, description: i18n.translate('discover.advancedSettings.discover.modifyColumnsOnSwitchText', { From 445f050633811004e790cb26f895d3de586c4f71 Mon Sep 17 00:00:00 2001 From: Matthias Wilhelm Date: Fri, 2 Oct 2020 08:43:45 +0200 Subject: [PATCH 6/7] Update ui_settings.ts --- src/plugins/discover/server/ui_settings.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/plugins/discover/server/ui_settings.ts b/src/plugins/discover/server/ui_settings.ts index 1f8407f14638b..822a0457643eb 100644 --- a/src/plugins/discover/server/ui_settings.ts +++ b/src/plugins/discover/server/ui_settings.ts @@ -166,13 +166,12 @@ export const uiSettings: Record = { }, [MODIFY_COLUMNS_ON_SWITCH]: { name: i18n.translate('discover.advancedSettings.discover.modifyColumnsOnSwitchTitle', { - defaultMessage: 'Modify columns when index pattern is switched', + defaultMessage: 'Modify columns when changing index patterns', }), value: true, description: i18n.translate('discover.advancedSettings.discover.modifyColumnsOnSwitchText', { defaultMessage: - 'When switching index patterns, selected columns, that are not available in the ' + - 'next index pattern, are removed.', + 'Remove columns that not available in the new index pattern.' }), category: ['discover'], schema: schema.boolean(), From fbc3f9e3a97c5e2f9dedb41797d00ad2e96f3a98 Mon Sep 17 00:00:00 2001 From: Matthias Wilhelm Date: Fri, 2 Oct 2020 11:28:54 +0200 Subject: [PATCH 7/7] Change linting error --- src/plugins/discover/server/ui_settings.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/plugins/discover/server/ui_settings.ts b/src/plugins/discover/server/ui_settings.ts index 822a0457643eb..5447b982eef14 100644 --- a/src/plugins/discover/server/ui_settings.ts +++ b/src/plugins/discover/server/ui_settings.ts @@ -170,8 +170,7 @@ export const uiSettings: Record = { }), value: true, description: i18n.translate('discover.advancedSettings.discover.modifyColumnsOnSwitchText', { - defaultMessage: - 'Remove columns that not available in the new index pattern.' + defaultMessage: 'Remove columns that not available in the new index pattern.', }), category: ['discover'], schema: schema.boolean(),