From c89c990f30ea1e01bc29438aa48995fa2477119c Mon Sep 17 00:00:00 2001 From: Julia Rechkunova Date: Fri, 8 Dec 2023 22:45:31 +0100 Subject: [PATCH] [Discover] Address reverting unsaved changes issue to unpin filters (#172063) - Closes https://github.com/elastic/kibana/issues/172025 ## Summary This PR changes filters diffing logic for the "Unsaved changes" badge and fixes "Revert changes" action of it. What to expect when testing: - If user pins a filter and the order of filters does not change, then the badge should not appear. - If user has 2 filters for example and pins the second one, then this action would change filters order and the badge would appear. - If user presses "Revert changes" action, then newly pinned filters would become unpinned. ### 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 --- .../services/discover_app_state_container.ts | 9 ++++- .../discover_saved_search_container.ts | 38 +++++++++++++++---- .../main/services/discover_state.ts | 10 +++++ .../main/utils/update_saved_search.test.ts | 2 +- .../main/utils/update_saved_search.ts | 2 +- .../discover/group3/_unsaved_changes_badge.ts | 31 +++++++++++++++ test/functional/services/filter_bar.ts | 5 +++ .../discover/group3/_unsaved_changes_badge.ts | 31 +++++++++++++++ 8 files changed, 117 insertions(+), 11 deletions(-) diff --git a/src/plugins/discover/public/application/main/services/discover_app_state_container.ts b/src/plugins/discover/public/application/main/services/discover_app_state_container.ts index 124c83beda236..facf8e26fb851 100644 --- a/src/plugins/discover/public/application/main/services/discover_app_state_container.ts +++ b/src/plugins/discover/public/application/main/services/discover_app_state_container.ts @@ -16,6 +16,7 @@ import { COMPARE_ALL_OPTIONS, compareFilters, Filter, + FilterCompareOptions, FilterStateStore, Query, } from '@kbn/es-query'; @@ -328,13 +329,17 @@ export function setState( /** * Helper function to compare 2 different filter states */ -export function isEqualFilters(filtersA?: Filter[] | Filter, filtersB?: Filter[] | Filter) { +export function isEqualFilters( + filtersA?: Filter[] | Filter, + filtersB?: Filter[] | Filter, + comparatorOptions: FilterCompareOptions = COMPARE_ALL_OPTIONS +) { if (!filtersA && !filtersB) { return true; } else if (!filtersA || !filtersB) { return false; } - return compareFilters(filtersA, filtersB, COMPARE_ALL_OPTIONS); + return compareFilters(filtersA, filtersB, comparatorOptions); } /** diff --git a/src/plugins/discover/public/application/main/services/discover_saved_search_container.ts b/src/plugins/discover/public/application/main/services/discover_saved_search_container.ts index 8948c954d039c..422fa787da849 100644 --- a/src/plugins/discover/public/application/main/services/discover_saved_search_container.ts +++ b/src/plugins/discover/public/application/main/services/discover_saved_search_container.ts @@ -8,19 +8,25 @@ import { SavedSearch } from '@kbn/saved-search-plugin/public'; import { BehaviorSubject } from 'rxjs'; +import { COMPARE_ALL_OPTIONS, FilterCompareOptions } from '@kbn/es-query'; import type { SearchSourceFields } from '@kbn/data-plugin/common'; import type { DataView } from '@kbn/data-views-plugin/common'; import { SavedObjectSaveOpts } from '@kbn/saved-objects-plugin/public'; -import { isEqual } from 'lodash'; +import { isEqual, isFunction } from 'lodash'; import { restoreStateFromSavedSearch } from '../../../services/saved_searches/restore_from_saved_search'; import { updateSavedSearch } from '../utils/update_saved_search'; import { addLog } from '../../../utils/add_log'; import { handleSourceColumnState } from '../../../utils/state_helpers'; -import { DiscoverAppState } from './discover_app_state_container'; +import { DiscoverAppState, isEqualFilters } from './discover_app_state_container'; import { DiscoverServices } from '../../../build_services'; import { getStateDefaults } from '../utils/get_state_defaults'; import type { DiscoverGlobalStateContainer } from './discover_global_state_container'; +const FILTERS_COMPARE_OPTIONS: FilterCompareOptions = { + ...COMPARE_ALL_OPTIONS, + state: false, // We don't compare filter types (global vs appState). +}; + export interface UpdateParams { /** * The next data view to be used @@ -279,11 +285,13 @@ export function isEqualSavedSearch(savedSearchPrev: SavedSearch, savedSearchNext const hasChangesInSearchSource = ( ['filter', 'query', 'index'] as Array ).some((key) => { - const prevValue = - key === 'index' ? prevSearchSource.getField(key)?.id : prevSearchSource.getField(key); - const nextValue = - key === 'index' ? nextSearchSource.getField(key)?.id : nextSearchSource.getField(key); - const isSame = isEqual(prevValue, nextValue); + const prevValue = getSearchSourceFieldValueForComparison(prevSearchSource, key); + const nextValue = getSearchSourceFieldValueForComparison(nextSearchSource, key); + + const isSame = + key === 'filter' + ? isEqualFilters(prevValue, nextValue, FILTERS_COMPARE_OPTIONS) // if a filter gets pinned and the order of filters does not change, we don't show the unsaved changes badge + : isEqual(prevValue, nextValue); if (!isSame) { addLog('[savedSearch] difference between initial and changed version', { @@ -304,3 +312,19 @@ export function isEqualSavedSearch(savedSearchPrev: SavedSearch, savedSearchNext return true; } + +function getSearchSourceFieldValueForComparison( + searchSource: SavedSearch['searchSource'], + searchSourceFieldName: keyof SearchSourceFields +) { + if (searchSourceFieldName === 'index') { + return searchSource.getField('index')?.id; + } + + if (searchSourceFieldName === 'filter') { + const filterField = searchSource.getField('filter'); + return isFunction(filterField) ? filterField() : filterField; + } + + return searchSource.getField(searchSourceFieldName); +} diff --git a/src/plugins/discover/public/application/main/services/discover_state.ts b/src/plugins/discover/public/application/main/services/discover_state.ts index 507076391c384..5e4f8a30199ed 100644 --- a/src/plugins/discover/public/application/main/services/discover_state.ts +++ b/src/plugins/discover/public/application/main/services/discover_state.ts @@ -459,6 +459,16 @@ export function getDiscoverStateContainer({ timefilter: services.timefilter, }); const newAppState = getDefaultAppState(nextSavedSearch, services); + + // a saved search can't have global (pinned) filters so we can reset global filters state + const globalFilters = globalStateContainer.get()?.filters; + if (globalFilters) { + await globalStateContainer.set({ + ...globalStateContainer.get(), + filters: [], + }); + } + await appStateContainer.replaceUrlState(newAppState); return nextSavedSearch; }; diff --git a/src/plugins/discover/public/application/main/utils/update_saved_search.test.ts b/src/plugins/discover/public/application/main/utils/update_saved_search.test.ts index 8c8d48cd9105b..17fee93fc92af 100644 --- a/src/plugins/discover/public/application/main/utils/update_saved_search.test.ts +++ b/src/plugins/discover/public/application/main/utils/update_saved_search.test.ts @@ -70,7 +70,7 @@ describe('updateSavedSearch', () => { }, }); expect(savedSearch.searchSource.getField('query')).toEqual(query); - expect(savedSearch.searchSource.getField('filter')).toEqual([appFilter, globalFilter]); + expect(savedSearch.searchSource.getField('filter')).toEqual([globalFilter, appFilter]); }); it('should set query and filters from services', async () => { diff --git a/src/plugins/discover/public/application/main/utils/update_saved_search.ts b/src/plugins/discover/public/application/main/utils/update_saved_search.ts index fb6ffd6621825..ff9d5a70f39be 100644 --- a/src/plugins/discover/public/application/main/utils/update_saved_search.ts +++ b/src/plugins/discover/public/application/main/utils/update_saved_search.ts @@ -52,7 +52,7 @@ export function updateSavedSearch({ savedSearch.searchSource .setField('query', state.query ?? undefined) - .setField('filter', [...appFilters, ...globalFilters]); + .setField('filter', [...globalFilters, ...appFilters]); } if (state) { savedSearch.columns = state.columns || []; diff --git a/test/functional/apps/discover/group3/_unsaved_changes_badge.ts b/test/functional/apps/discover/group3/_unsaved_changes_badge.ts index 292835b37ef10..c931a11f4f5f4 100644 --- a/test/functional/apps/discover/group3/_unsaved_changes_badge.ts +++ b/test/functional/apps/discover/group3/_unsaved_changes_badge.ts @@ -10,12 +10,14 @@ import expect from '@kbn/expect'; import { FtrProviderContext } from '../ftr_provider_context'; const SAVED_SEARCH_NAME = 'test saved search'; +const SAVED_SEARCH_WITH_FILTERS_NAME = 'test saved search with filters'; export default function ({ getService, getPageObjects }: FtrProviderContext) { const esArchiver = getService('esArchiver'); const kibanaServer = getService('kibanaServer'); const testSubjects = getService('testSubjects'); const dataGrid = getService('dataGrid'); + const filterBar = getService('filterBar'); const PageObjects = getPageObjects([ 'settings', 'common', @@ -163,5 +165,34 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) { await PageObjects.discover.waitUntilSearchingHasFinished(); await testSubjects.missingOrFail('unsavedChangesBadge'); }); + + it('should not show the badge after pinning the first filter but after disabling a filter', async () => { + await filterBar.addFilter({ field: 'extension', operation: 'is', value: 'png' }); + await filterBar.addFilter({ field: 'bytes', operation: 'exists' }); + await PageObjects.discover.saveSearch(SAVED_SEARCH_WITH_FILTERS_NAME); + await PageObjects.discover.waitUntilSearchingHasFinished(); + + await testSubjects.missingOrFail('unsavedChangesBadge'); + + await filterBar.toggleFilterPinned('extension'); + await PageObjects.discover.waitUntilSearchingHasFinished(); + expect(await filterBar.isFilterPinned('extension')).to.be(true); + + await testSubjects.missingOrFail('unsavedChangesBadge'); + + await filterBar.toggleFilterNegated('bytes'); + await PageObjects.discover.waitUntilSearchingHasFinished(); + expect(await filterBar.isFilterNegated('bytes')).to.be(true); + + await testSubjects.existOrFail('unsavedChangesBadge'); + + await PageObjects.discover.revertUnsavedChanges(); + await testSubjects.missingOrFail('unsavedChangesBadge'); + + expect(await filterBar.getFilterCount()).to.be(2); + expect(await filterBar.isFilterPinned('extension')).to.be(false); + expect(await filterBar.isFilterNegated('bytes')).to.be(false); + expect(await PageObjects.discover.getHitCount()).to.be('1,373'); + }); }); } diff --git a/test/functional/services/filter_bar.ts b/test/functional/services/filter_bar.ts index eb9aae45621e1..cd927ee18fb83 100644 --- a/test/functional/services/filter_bar.ts +++ b/test/functional/services/filter_bar.ts @@ -178,6 +178,11 @@ export class FilterBarService extends FtrService { return (await filter.getAttribute('data-test-subj')).includes('filter-pinned'); } + public async isFilterNegated(key: string): Promise { + const filter = await this.testSubjects.find(`~filter & ~filter-key-${key}`); + return (await filter.getAttribute('data-test-subj')).includes('filter-negated'); + } + public async getFilterCount(): Promise { const filters = await this.testSubjects.findAll('~filter'); return filters.length; diff --git a/x-pack/test_serverless/functional/test_suites/common/discover/group3/_unsaved_changes_badge.ts b/x-pack/test_serverless/functional/test_suites/common/discover/group3/_unsaved_changes_badge.ts index f063a805635f7..1f6f89a7bb33b 100644 --- a/x-pack/test_serverless/functional/test_suites/common/discover/group3/_unsaved_changes_badge.ts +++ b/x-pack/test_serverless/functional/test_suites/common/discover/group3/_unsaved_changes_badge.ts @@ -9,12 +9,14 @@ import expect from '@kbn/expect'; import { FtrProviderContext } from '../../../../ftr_provider_context'; const SAVED_SEARCH_NAME = 'test saved search'; +const SAVED_SEARCH_WITH_FILTERS_NAME = 'test saved search with filters'; export default function ({ getService, getPageObjects }: FtrProviderContext) { const esArchiver = getService('esArchiver'); const kibanaServer = getService('kibanaServer'); const testSubjects = getService('testSubjects'); const dataGrid = getService('dataGrid'); + const filterBar = getService('filterBar'); const PageObjects = getPageObjects([ 'settings', 'common', @@ -162,5 +164,34 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) { await PageObjects.discover.waitUntilSearchingHasFinished(); await testSubjects.missingOrFail('unsavedChangesBadge'); }); + + it('should not show the badge after pinning the first filter but after disabling a filter', async () => { + await filterBar.addFilter({ field: 'extension', operation: 'is', value: 'png' }); + await filterBar.addFilter({ field: 'bytes', operation: 'exists' }); + await PageObjects.discover.saveSearch(SAVED_SEARCH_WITH_FILTERS_NAME); + await PageObjects.discover.waitUntilSearchingHasFinished(); + + await testSubjects.missingOrFail('unsavedChangesBadge'); + + await filterBar.toggleFilterPinned('extension'); + await PageObjects.discover.waitUntilSearchingHasFinished(); + expect(await filterBar.isFilterPinned('extension')).to.be(true); + + await testSubjects.missingOrFail('unsavedChangesBadge'); + + await filterBar.toggleFilterNegated('bytes'); + await PageObjects.discover.waitUntilSearchingHasFinished(); + expect(await filterBar.isFilterNegated('bytes')).to.be(true); + + await testSubjects.existOrFail('unsavedChangesBadge'); + + await PageObjects.discover.revertUnsavedChanges(); + await testSubjects.missingOrFail('unsavedChangesBadge'); + + expect(await filterBar.getFilterCount()).to.be(2); + expect(await filterBar.isFilterPinned('extension')).to.be(false); + expect(await filterBar.isFilterNegated('bytes')).to.be(false); + expect(await PageObjects.discover.getHitCount()).to.be('1,373'); + }); }); }