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] Persist hide chart option to local storage #114534

Merged
merged 14 commits into from
Oct 14, 2021
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import { DiscoverServices } from '../../../../../build_services';
import { useChartPanels } from './use_chart_panels';

const DiscoverHistogramMemoized = memo(DiscoverHistogram);
export const CHART_HIDDEN_KEY = 'discover:chartHidden';

export function DiscoverChart({
resetSavedSearch,
Expand All @@ -47,7 +48,8 @@ export function DiscoverChart({
}) {
const [showChartOptionsPopover, setShowChartOptionsPopover] = useState(false);

const { data } = services;
const { data, storage } = services;

const chartRef = useRef<{ element: HTMLElement | null; moveFocus: boolean }>({
element: null,
moveFocus: false,
Expand All @@ -71,7 +73,8 @@ export function DiscoverChart({
const newHideChart = !state.hideChart;
stateContainer.setAppState({ hideChart: newHideChart });
chartRef.current.moveFocus = !newHideChart;
}, [state, stateContainer]);
storage.set(CHART_HIDDEN_KEY, newHideChart);
}, [state.hideChart, stateContainer, storage]);

const timefilterUpdateHandler = useCallback(
(ranges: { from: number; to: number }) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ export function useDiscoverState({
savedSearch: SavedSearch;
history: History;
}) {
const { uiSettings: config, data, filterManager, indexPatterns } = services;
const { uiSettings: config, data, filterManager, indexPatterns, storage } = services;
const useNewFieldsApi = useMemo(() => !config.get(SEARCH_FIELDS_FROM_SOURCE), [config]);
const { timefilter } = data.query.timefilter;

Expand All @@ -53,13 +53,14 @@ export function useDiscoverState({
config,
data,
savedSearch,
storage,
}),
storeInSessionStorage: config.get('state:storeInSessionStorage'),
history,
toasts: services.core.notifications.toasts,
uiSettings: config,
}),
[config, data, history, savedSearch, services.core.notifications.toasts]
[config, data, history, savedSearch, services.core.notifications.toasts, storage]
);

const { appStateContainer } = stateContainer;
Expand Down Expand Up @@ -155,11 +156,12 @@ export function useDiscoverState({
config,
data,
savedSearch: newSavedSearch,
storage,
});
await stateContainer.replaceUrlAppState(newAppState);
setState(newAppState);
},
[indexPattern, services, config, data, stateContainer]
[services, indexPattern, config, data, storage, stateContainer]
);

/**
Expand Down Expand Up @@ -204,10 +206,11 @@ export function useDiscoverState({
config,
data,
savedSearch,
storage,
});
stateContainer.replaceUrlAppState(newAppState);
setState(newAppState);
}, [config, data, savedSearch, reset, stateContainer]);
}, [config, data, savedSearch, reset, stateContainer, storage]);

/**
* Trigger data fetching on indexPattern or savedSearch changes
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,18 @@ import { uiSettingsMock } from '../../../../__mocks__/ui_settings';
import { indexPatternWithTimefieldMock } from '../../../../__mocks__/index_pattern_with_timefield';
import { savedSearchMock } from '../../../../__mocks__/saved_search';
import { indexPatternMock } from '../../../../__mocks__/index_pattern';
import { discoverServiceMock } from '../../../../__mocks__/services';

describe('getStateDefaults', () => {
const storage = discoverServiceMock.storage;

test('index pattern with timefield', () => {
savedSearchMock.searchSource = createSearchSourceMock({ index: indexPatternWithTimefieldMock });
const actual = getStateDefaults({
config: uiSettingsMock,
data: dataPluginMock.createStartContract(),
savedSearch: savedSearchMock,
storage,
});
expect(actual).toMatchInlineSnapshot(`
Object {
Expand Down Expand Up @@ -49,6 +53,7 @@ describe('getStateDefaults', () => {
config: uiSettingsMock,
data: dataPluginMock.createStartContract(),
savedSearch: savedSearchMock,
storage,
});
expect(actual).toMatchInlineSnapshot(`
Object {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ import { DataPublicPluginStart } from '../../../../../../data/public';

import { AppState } from '../services/discover_state';
import { getDefaultSort, getSortArray } from '../components/doc_table';
import { CHART_HIDDEN_KEY } from '../components/chart/discover_chart';
import { Storage } from '../../../../../../kibana_utils/public';

function getDefaultColumns(savedSearch: SavedSearch, config: IUiSettingsClient) {
if (savedSearch.columns && savedSearch.columns.length > 0) {
Expand All @@ -26,16 +28,19 @@ export function getStateDefaults({
config,
data,
savedSearch,
storage,
}: {
config: IUiSettingsClient;
data: DataPublicPluginStart;
savedSearch: SavedSearch;
storage: Storage;
}) {
const searchSource = savedSearch.searchSource;
const indexPattern = savedSearch.searchSource.getField('index');
const query = searchSource.getField('query') || data.query.queryString.getDefaultQuery();
const sort = getSortArray(savedSearch.sort, indexPattern!);
const columns = getDefaultColumns(savedSearch, config);
const chartHidden = Boolean(storage.get(CHART_HIDDEN_KEY));

const defaultState = {
query,
Expand All @@ -46,13 +51,13 @@ export function getStateDefaults({
index: indexPattern!.id,
interval: 'auto',
filters: cloneDeep(searchSource.getOwnField('filter')),
hideChart: undefined,
hideChart: chartHidden ? chartHidden : undefined,
savedQuery: undefined,
} as AppState;
if (savedSearch.grid) {
defaultState.grid = savedSearch.grid;
}
if (savedSearch.hideChart) {
if (savedSearch.hideChart !== undefined) {
defaultState.hideChart = savedSearch.hideChart;
}

Expand Down
72 changes: 72 additions & 0 deletions test/functional/apps/discover/_chart_hidden.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

import expect from '@kbn/expect';
import { FtrProviderContext } from '../../ftr_provider_context';

export default function ({ getService, getPageObjects }: FtrProviderContext) {
const log = getService('log');
const esArchiver = getService('esArchiver');
const kibanaServer = getService('kibanaServer');
const PageObjects = getPageObjects(['common', 'discover', 'header', 'timePicker']);

const defaultSettings = {
defaultIndex: 'logstash-*',
};

describe('discover show/hide chart test', function () {
before(async function () {
log.debug('load kibana index with default index pattern');

await kibanaServer.importExport.load('test/functional/fixtures/kbn_archiver/discover.json');

// and load a set of makelogs data
await esArchiver.loadIfNeeded('test/functional/fixtures/es_archiver/logstash_functional');
await kibanaServer.uiSettings.replace(defaultSettings);
await PageObjects.common.navigateToApp('discover');
await PageObjects.timePicker.setDefaultAbsoluteRange();
});

after(async () => {
await kibanaServer.uiSettings.unset('timepicker:timeDefaults');
Copy link
Member

Choose a reason for hiding this comment

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

'timepicker:timeDefaults' do we need to reset this? since it has not been set?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I thought this was unsetting the default time in Discover 🤦‍♀️

await kibanaServer.uiSettings.unset('defaultIndex');
});

it('shows chart by default', async function () {
expect(await PageObjects.discover.isChartVisible()).to.be(true);
});

it('hiding the chart persists the setting', async function () {
await PageObjects.discover.toggleChartVisibility();
expect(await PageObjects.discover.isChartVisible()).to.be(false);

await PageObjects.common.navigateToApp('dashboard');
await PageObjects.common.navigateToApp('discover');
await PageObjects.timePicker.setDefaultAbsoluteRange();
await PageObjects.header.waitUntilLoadingHasFinished();

expect(await PageObjects.discover.isChartVisible()).to.be(false);
});

it('persists hidden chart option on the saved search ', async function () {
const savedSearchTitle = 'chart hidden';
await PageObjects.discover.saveSearch(savedSearchTitle);

await PageObjects.discover.toggleChartVisibility();
expect(await PageObjects.discover.isChartVisible()).to.be(true);

await PageObjects.common.navigateToApp('discover');
await PageObjects.timePicker.setDefaultAbsoluteRange();
await PageObjects.header.waitUntilLoadingHasFinished();
expect(await PageObjects.discover.isChartVisible()).to.be(true);

await PageObjects.discover.loadSavedSearch(savedSearchTitle);
expect(await PageObjects.discover.isChartVisible()).to.be(false);
});
});
}
1 change: 1 addition & 0 deletions test/functional/apps/discover/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,5 +52,6 @@ export default function ({ getService, loadTestFile }: FtrProviderContext) {
loadTestFile(require.resolve('./_huge_fields'));
loadTestFile(require.resolve('./_date_nested'));
loadTestFile(require.resolve('./_search_on_page_load'));
loadTestFile(require.resolve('./_chart_hidden'));
});
}
11 changes: 11 additions & 0 deletions test/functional/page_objects/discover_page.ts
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,17 @@ export class DiscoverPageObject extends FtrService {
return await this.globalNav.getLastBreadcrumb();
}

public async isChartVisible() {
return await this.testSubjects.exists('discoverChart');
}

public async toggleChartVisibility() {
await this.testSubjects.click('discoverChartOptionsToggle');
await this.testSubjects.exists('discoverChartToggle');
await this.testSubjects.click('discoverChartToggle');
await this.header.waitUntilLoadingHasFinished();
}

public async getChartInterval() {
await this.testSubjects.click('discoverChartOptionsToggle');
await this.testSubjects.click('discoverTimeIntervalPanel');
Expand Down