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.

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] Show "Unsaved changes" badge on time filter changes in case time range is stored along with saved search #178659

Merged
merged 8 commits into from
Mar 20, 2024
2 changes: 2 additions & 0 deletions src/plugins/discover/public/__mocks__/saved_search.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ export const savedSearchMock = {
id: 'the-saved-search-id',
title: 'A saved search',
searchSource: createSearchSourceMock({ index: dataViewMock }),
columns: ['default_column'],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ℹ️ Making the mocked saved search more realistic, as these are filled in automatically when discover loads the saved search

sort: [],
} as unknown as SavedSearch;

export const savedSearchMockWithTimeField = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import {
createSearchSessionRestorationDataProvider,
} from './discover_state';
import { createBrowserHistory, createMemoryHistory, History } from 'history';
import { dataPluginMock } from '@kbn/data-plugin/public/mocks';
import { createSearchSourceMock, dataPluginMock } from '@kbn/data-plugin/public/mocks';
import type { SavedSearch, SortOrder } from '@kbn/saved-search-plugin/public';
import {
savedSearchAdHoc,
Expand Down Expand Up @@ -524,6 +524,43 @@ describe('Test discover state actions', () => {
unsubscribe();
});

test('loadSavedSearch given a URL with different time range than the stored one showing as changed', async () => {
const url = '/#_g=(time:(from:now-24h%2Fh,to:now))';
const savedSearch = {
...savedSearchMock,
searchSource: createSearchSourceMock({ index: dataViewMock, filter: [] }),
timeRestore: true,
timeRange: { from: 'now-666m', to: 'now' },
};
const { state } = await getState(url, {
savedSearch,
isEmptyUrl: false,
});
await state.actions.loadSavedSearch({ savedSearchId: savedSearchMock.id });
const unsubscribe = state.actions.initializeAndSync();
await new Promise(process.nextTick);
expect(state.savedSearchState.getHasChanged$().getValue()).toBe(true);
unsubscribe();
});

test('loadSavedSearch given a URL with different time range than without timeRestore not showing as changed', async () => {
const url = '/#?_g=(time:(from:now-24h%2Fh,to:now))';
const savedSearch = {
...savedSearchMock,
searchSource: createSearchSourceMock({ index: dataViewMock, filter: [] }),
timeRestore: false,
};
const { state } = await getState(url, {
savedSearch,
isEmptyUrl: false,
});
await state.actions.loadSavedSearch({ savedSearchId: savedSearchMock.id });
const unsubscribe = state.actions.initializeAndSync();
await new Promise(process.nextTick);
expect(state.savedSearchState.getHasChanged$().getValue()).toBe(false);
unsubscribe();
});

test('loadSavedSearch ignoring hideChart in URL', async () => {
const url = '/#?_a=(hideChart:true,columns:!(message))&_g=()';
const { state } = await getState(url, { savedSearch: savedSearchMock });
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -375,6 +375,16 @@ export function getDiscoverStateContainer({
* state containers initializing and subscribing to changes triggering e.g. data fetching
*/
const initializeAndSync = () => {
// This needs to be the first thing that's wired up because initAndSync is pulling the current state from the URL which
// might change the time filter and thus needs to re-check whether the saved search has changed.
const timefilerUnsubscribe = services.timefilter.getTimeUpdate$().subscribe(() => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ℹ️ This needs to be the first thing that's wired up because the next line is pulling the current state from the URL which might change the time filter and thus needs to re-check whether the saved search has changed

savedSearchContainer.update({
Copy link
Contributor

Choose a reason for hiding this comment

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

This method contains a lot of logic. What if we apply only timeRange and refreshInterval changes here instead of the whole update?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made an update, is this what you had in mind?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, thanks! I think we also need to update refreshInterval if it got changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, could you take another look?

nextDataView: internalStateContainer.getState().dataView,
nextState: appStateContainer.getState(),
useFilterAndQueryServices: true,
});
});

// initialize app state container, syncing with _g and _a part of the URL
const appStateInitAndSyncUnsubscribe = appStateContainer.initAndSync(
savedSearchContainer.getState()
Expand Down Expand Up @@ -426,6 +436,7 @@ export function getDiscoverStateContainer({
appStateUnsubscribe();
appStateInitAndSyncUnsubscribe();
filterUnsubscribe.unsubscribe();
timefilerUnsubscribe.unsubscribe();
};
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { savedSearchMock } from '../../../__mocks__/saved_search';
import { discoverServiceMock } from '../../../__mocks__/services';
import { Filter, FilterStateStore, Query } from '@kbn/es-query';
import { updateSavedSearch } from './update_saved_search';
import { SavedSearch } from '@kbn/saved-search-plugin/public';

describe('updateSavedSearch', () => {
const query: Query = {
Expand Down Expand Up @@ -73,6 +74,56 @@ describe('updateSavedSearch', () => {
expect(savedSearch.searchSource.getField('filter')).toEqual([globalFilter, appFilter]);
});

it('should set time range is timeRestore is enabled', async () => {
const savedSearch: SavedSearch = {
...savedSearchMock,
searchSource: savedSearchMock.searchSource.createCopy(),
timeRestore: true,
};
(discoverServiceMock.timefilter.getTime as jest.Mock).mockReturnValue({
from: 'now-666m',
to: 'now',
});
updateSavedSearch({
savedSearch,
globalStateContainer: createGlobalStateContainer(),
services: discoverServiceMock,
state: {
query,
filters: [appFilter],
},
});
expect(savedSearch.timeRange).toEqual({
from: 'now-666m',
to: 'now',
});
});

it('should not set time range is timeRestore is enabled', async () => {
const savedSearch: SavedSearch = {
...savedSearchMock,
searchSource: savedSearchMock.searchSource.createCopy(),
timeRestore: false,
};
(discoverServiceMock.timefilter.getTime as jest.Mock).mockReturnValue({
from: 'now-666m',
to: 'now',
});
updateSavedSearch({
savedSearch,
globalStateContainer: createGlobalStateContainer(),
services: discoverServiceMock,
state: {
query,
filters: [appFilter],
},
});
expect(savedSearch.timeRange).not.toEqual({
from: 'now-666m',
to: 'now',
});
});

it('should set query and filters from services', async () => {
const savedSearch = {
...savedSearchMock,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,9 @@ export function updateSavedSearch({
.setField('query', state.query ?? undefined)
.setField('filter', [...globalFilters, ...appFilters]);
}
if (savedSearch.timeRestore) {
savedSearch.timeRange = services.timefilter.getTime();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (savedSearch.timeRestore) {
savedSearch.timeRange = services.timefilter.getTime();
}

It should already work based on the code at the end of the function. Doesn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, missed that, thanks!

if (state) {
savedSearch.columns = state.columns || [];
savedSearch.sort = (state.sort as SortOrder[]) || [];
Expand Down