Skip to content

Commit

Permalink
[Discover] View mode changes should trigger Unsaved changes badge in …
Browse files Browse the repository at this point in the history
…ES|QL (elastic#187244)

- Closes elastic#184624

## Summary

This PR updates the logic to show Unsaved changes badge when view mode
changes in ES|QL mode.

### 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

---------

Co-authored-by: Matthias Wilhelm <[email protected]>
  • Loading branch information
jughosta and kertal authored Jul 5, 2024
1 parent 68d5370 commit 25d7346
Show file tree
Hide file tree
Showing 4 changed files with 87 additions and 13 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ import { dataViewMock } from '@kbn/discover-utils/src/__mocks__';
import { dataViewComplexMock } from '../../../__mocks__/data_view_complex';
import { getDiscoverGlobalStateContainer } from './discover_global_state_container';
import { createKbnUrlStateStorage } from '@kbn/kibana-utils-plugin/public';
import { VIEW_MODE } from '../../../../common/constants';
import { createSearchSourceMock } from '@kbn/data-plugin/common/search/search_source/mocks';

describe('DiscoverSavedSearchContainer', () => {
const savedSearch = savedSearchMock;
Expand Down Expand Up @@ -232,4 +234,57 @@ describe('DiscoverSavedSearchContainer', () => {
expect(savedSearchContainer.getHasChanged$().getValue()).toBe(false);
});
});

describe('isEqualSavedSearch', () => {
it('should return true for equal saved searches', () => {
const savedSearch1 = savedSearchMock;
const savedSearch2 = { ...savedSearchMock };
expect(isEqualSavedSearch(savedSearch1, savedSearch2)).toBe(true);
});

it('should return false for different saved searches', () => {
const savedSearch1 = savedSearchMock;
const savedSearch2 = { ...savedSearchMock, title: 'New title' };
expect(isEqualSavedSearch(savedSearch1, savedSearch2)).toBe(false);
});

it('should return true for saved searches with equal default values of viewMode and false otherwise', () => {
const savedSearch1 = { ...savedSearchMock, viewMode: undefined };
const savedSearch2 = { ...savedSearchMock, viewMode: VIEW_MODE.DOCUMENT_LEVEL };
const savedSearch3 = { ...savedSearchMock, viewMode: VIEW_MODE.AGGREGATED_LEVEL };
expect(isEqualSavedSearch(savedSearch1, savedSearch2)).toBe(true);
expect(isEqualSavedSearch(savedSearch2, savedSearch1)).toBe(true);
expect(isEqualSavedSearch(savedSearch1, savedSearch3)).toBe(false);
expect(isEqualSavedSearch(savedSearch2, savedSearch3)).toBe(false);
});

it('should return true for saved searches with equal default values of breakdownField and false otherwise', () => {
const savedSearch1 = { ...savedSearchMock, breakdownField: undefined };
const savedSearch2 = { ...savedSearchMock, breakdownField: '' };
const savedSearch3 = { ...savedSearchMock, breakdownField: 'test' };
expect(isEqualSavedSearch(savedSearch1, savedSearch2)).toBe(true);
expect(isEqualSavedSearch(savedSearch2, savedSearch1)).toBe(true);
expect(isEqualSavedSearch(savedSearch1, savedSearch3)).toBe(false);
expect(isEqualSavedSearch(savedSearch2, savedSearch3)).toBe(false);
});

it('should check searchSource fields', () => {
const savedSearch1 = {
...savedSearchMock,
searchSource: createSearchSourceMock({
index: savedSearchMock.searchSource.getField('index'),
}),
};
const savedSearch2 = {
...savedSearchMock,
searchSource: createSearchSourceMock({
index: savedSearchMock.searchSource.getField('index'),
}),
};
expect(isEqualSavedSearch(savedSearch1, savedSearch1)).toBe(true);
expect(isEqualSavedSearch(savedSearch1, savedSearch2)).toBe(true);
savedSearch2.searchSource.setField('query', { language: 'lucene', query: 'test' });
expect(isEqualSavedSearch(savedSearch1, savedSearch2)).toBe(false);
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -341,12 +341,7 @@ export function isEqualSavedSearch(savedSearchPrev: SavedSearch, savedSearchNext
const prevValue = getSavedSearchFieldForComparison(prevSavedSearch, key);
const nextValue = getSavedSearchFieldForComparison(nextSavedSearchWithoutSearchSource, key);

const isSame =
isEqual(prevValue, nextValue) ||
// By default, viewMode: undefined is equivalent to documents view
// So they should be treated as same
(key === 'viewMode' &&
(prevValue ?? VIEW_MODE.DOCUMENT_LEVEL) === (nextValue ?? VIEW_MODE.DOCUMENT_LEVEL));
const isSame = isEqual(prevValue, nextValue);

if (!isSame) {
addLog('[savedSearch] difference between initial and changed version', {
Expand Down Expand Up @@ -412,6 +407,12 @@ function getSavedSearchFieldForComparison(
return savedSearch.breakdownField || ''; // ignore the difference between an empty string and undefined
}

if (fieldName === 'viewMode') {
// By default, viewMode: undefined is equivalent to documents view
// So they should be treated as same
return savedSearch.viewMode ?? VIEW_MODE.DOCUMENT_LEVEL;
}

return savedSearch[fieldName];
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,16 @@ import { FetchStatus } from '../../../types';
import { dataViewComplexMock } from '../../../../__mocks__/data_view_complex';
import { getDiscoverStateMock } from '../../../../__mocks__/discover_state.mock';
import { discoverServiceMock } from '../../../../__mocks__/services';
import { createDataViewDataSource } from '../../../../../common/data_sources';
import { createDataViewDataSource, DataSourceType } from '../../../../../common/data_sources';
import { VIEW_MODE } from '@kbn/saved-search-plugin/common';

describe('buildStateSubscribe', () => {
const savedSearch = savedSearchMock;
const stateContainer = getDiscoverStateMock({ savedSearch });
stateContainer.dataState.refetch$.next = jest.fn();
stateContainer.dataState.reset = jest.fn();
stateContainer.actions.setDataView = jest.fn();
stateContainer.savedSearchState.update = jest.fn();

const getSubscribeFn = () => {
return buildStateSubscribe({
Expand Down Expand Up @@ -51,6 +53,20 @@ describe('buildStateSubscribe', () => {
expect(stateContainer.dataState.refetch$.next).not.toHaveBeenCalled();
});

it('should not call refetch$ if viewMode changes', async () => {
await getSubscribeFn()({
...stateContainer.appState.getState(),
dataSource: {
type: DataSourceType.Esql,
},
viewMode: VIEW_MODE.AGGREGATED_LEVEL,
});

expect(stateContainer.dataState.refetch$.next).not.toHaveBeenCalled();
expect(stateContainer.dataState.reset).not.toHaveBeenCalled();
expect(stateContainer.savedSearchState.update).toHaveBeenCalled();
});

it('should call refetch$ if the chart is hidden', async () => {
await getSubscribeFn()({ hideChart: true });

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,12 +55,14 @@ export const buildStateSubscribe =
const isEsqlMode = isDataSourceType(nextState.dataSource, DataSourceType.Esql);
const queryChanged = !isEqual(nextQuery, prevQuery) || !isEqual(nextQuery, prevState.query);

if (
isEsqlMode &&
isEqualState(prevState, nextState, ['dataSource', 'viewMode']) &&
!queryChanged
) {
// When there's a switch from data view to es|ql, this just leads to a cleanup of index and viewMode
if (isEsqlMode && prevState.viewMode !== nextState.viewMode && !queryChanged) {
savedSearchState.update({ nextState });
addLog('[appstate] subscribe $fetch ignored for es|ql', { prevState, nextState });
return;
}

if (isEsqlMode && isEqualState(prevState, nextState, ['dataSource']) && !queryChanged) {
// When there's a switch from data view to es|ql, this just leads to a cleanup of index
// And there's no subsequent action in this function required
addLog('[appstate] subscribe update ignored for es|ql', { prevState, nextState });
return;
Expand Down

0 comments on commit 25d7346

Please sign in to comment.