Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Discover] Address reverting unsaved changes issue to unpin filters #172063

Merged
merged 13 commits into from
Dec 8, 2023
Merged

[Discover] Address reverting unsaved changes issue to unpin filters #172063

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
faf475a
[Discover] Address reverting unsaved changes issue
jughosta Nov 28, 2023
8d4cda5
Merge branch 'main' into 172025-unpin-filters-when-reverting-changes
jughosta Nov 29, 2023
f657e2c
[Discover] Improve filters comparison
jughosta Nov 29, 2023
5e7b528
[Discover] Simplify the logic
jughosta Nov 29, 2023
2d0f382
[Discover] Revert some changes
jughosta Nov 30, 2023
1d9daa4
Merge branch 'main' into 172025-unpin-filters-when-reverting-changes
jughosta Nov 30, 2023
6df1795
[Discover] Add a test and fix how global filters are applied
jughosta Nov 30, 2023
ab11287
[Discover] Fix deps
jughosta Nov 30, 2023
587f1d8
[Discover] Update tests
jughosta Nov 30, 2023
5ed4a64
Update src/plugins/discover/public/application/main/services/discover…
jughosta Nov 30, 2023
2d40a72
Update src/plugins/discover/public/application/main/services/discover…
jughosta Nov 30, 2023
e4b70f0
Merge branch 'main' into 172025-unpin-filters-when-reverting-changes
jughosta Dec 4, 2023
b2231b1
Merge branch 'main' into 172025-unpin-filters-when-reverting-changes
jughosta Dec 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions src/plugins/discover/public/application/main/services/discover_app_state_container.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import {
COMPARE_ALL_OPTIONS,
compareFilters,
Filter,
FilterCompareOptions,
FilterStateStore,
Query,
} from '@kbn/es-query';
Expand Down Expand Up @@ -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);
}

/**
Expand Down
38 changes: 31 additions & 7 deletions src/plugins/discover/public/application/main/services/discover_saved_search_container.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -279,11 +285,13 @@ export function isEqualSavedSearch(savedSearchPrev: SavedSearch, savedSearchNext
const hasChangesInSearchSource = (
['filter', 'query', 'index'] as Array<keyof SearchSourceFields>
).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, we don't show the unsaved changes badge
jughosta marked this conversation as resolved.
Show resolved Hide resolved
: isEqual(prevValue, nextValue);

if (!isSame) {
addLog('[savedSearch] difference between initial and changed version', {
Expand All @@ -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);
}
10 changes: 10 additions & 0 deletions src/plugins/discover/public/application/main/services/discover_state.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -457,6 +457,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;
};
Expand Down
2 changes: 1 addition & 1 deletion src/plugins/discover/public/application/main/utils/update_saved_search.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand Down
2 changes: 1 addition & 1 deletion src/plugins/discover/public/application/main/utils/update_saved_search.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ export function updateSavedSearch({

savedSearch.searchSource
.setField('query', state.query ?? undefined)
.setField('filter', [...appFilters, ...globalFilters]);
.setField('filter', [...globalFilters, ...appFilters]);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In UI pinned filters are rendered first, then app filters. So we better pass them to Elasticsearch in the same order.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following up about this topic, I just wanted to add some information, because it seems the order of query / filters doesn't matter from ES side, according to https://www.elastic.co/de/blog/elasticsearch-query-execution-order

Q: Does the order in which I put my queries/filters in the query DSL matter?
A: No, because they will be automatically reordered anyway based on their respective costs and match costs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @kertal! Then it's just helpful for the diffing logic.

}
if (state) {
savedSearch.columns = state.columns || [];
Expand Down
31 changes: 31 additions & 0 deletions test/functional/apps/discover/group3/_unsaved_changes_badge.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down Expand Up @@ -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');
});
});
}
5 changes: 5 additions & 0 deletions test/functional/services/filter_bar.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<boolean> {
const filter = await this.testSubjects.find(`~filter & ~filter-key-${key}`);
return (await filter.getAttribute('data-test-subj')).includes('filter-negated');
}

public async getFilterCount(): Promise<number> {
const filters = await this.testSubjects.findAll('~filter');
return filters.length;
Expand Down
31 changes: 31 additions & 0 deletions ...k/test_serverless/functional/test_suites/common/discover/group3/_unsaved_changes_badge.ts
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding to serverless too!

Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down Expand Up @@ -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');
});
});
}