Skip to content

Commit

Permalink
[Discover] Navigating from Field statistics tab to text based languag…
Browse files Browse the repository at this point in the history
…es should return to the documents view (#152572)

Closes #152485
~Would be better to override this somewhere in app state management
logic but I could not find a right place for it.~ Done

Current changes make sure that for text-based queries only grid view is
possible (both on Discover and as embeddable) and the app state will be
updated accordingly.
  • Loading branch information
jughosta authored Apr 27, 2023
1 parent e9197ad commit f528b34
Show file tree
Hide file tree
Showing 10 changed files with 207 additions and 15 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -14,21 +14,24 @@ import { useTextBasedQueryLanguage } from './use_text_based_query_language';
import { BehaviorSubject } from 'rxjs';
import { FetchStatus } from '../../types';
import { DataDocuments$, RecordRawType } from '../services/discover_data_state_container';
import { DiscoverAppState } from '../services/discover_app_state_container';
import { DataTableRecord } from '../../../types';
import { AggregateQuery, Query } from '@kbn/es-query';
import { dataViewMock } from '../../../__mocks__/data_view';
import { DataViewListItem } from '@kbn/data-views-plugin/common';
import { savedSearchMock } from '../../../__mocks__/saved_search';
import { getDiscoverStateMock } from '../../../__mocks__/discover_state.mock';
import { VIEW_MODE } from '@kbn/saved-search-plugin/common';

function getHookProps(
query: AggregateQuery | Query | undefined,
dataViewsService?: DataViewsContract
dataViewsService?: DataViewsContract,
appState?: Partial<DiscoverAppState>
) {
const replaceUrlState = jest.fn();
const stateContainer = getDiscoverStateMock({ isTimeBased: true });
stateContainer.appState.replaceUrlState = replaceUrlState;
stateContainer.appState.update({ columns: [] });
stateContainer.appState.update({ columns: [], ...appState });
stateContainer.internalState.transitions.setSavedDataViews([dataViewMock as DataViewListItem]);

const msgLoading = {
Expand Down Expand Up @@ -83,6 +86,20 @@ describe('useTextBasedQueryLanguage', () => {
});
});
});
test('should change viewMode to DOCUMENT_LEVEL if it was AGGREGATED_LEVEL', async () => {
const props = getHookProps(query, undefined, {
viewMode: VIEW_MODE.AGGREGATED_LEVEL,
});
const { replaceUrlState } = props;

renderHook(() => useTextBasedQueryLanguage(props));

await waitFor(() => expect(replaceUrlState).toHaveBeenCalledTimes(1));
expect(replaceUrlState).toHaveBeenCalledWith({
index: 'the-data-view-id',
viewMode: VIEW_MODE.DOCUMENT_LEVEL,
});
});
test('changing a text based query with different result columns should change state when loading and finished', async () => {
const props = getHookProps(query);
const { documents$, replaceUrlState } = props;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,10 @@ import {
} from '@kbn/es-query';
import { useCallback, useEffect, useRef } from 'react';
import type { DataViewsContract } from '@kbn/data-views-plugin/public';
import { SavedSearch } from '@kbn/saved-search-plugin/public';
import { SavedSearch, VIEW_MODE } from '@kbn/saved-search-plugin/public';
import type { DiscoverStateContainer } from '../services/discover_state';
import type { DataDocuments$ } from '../services/discover_data_state_container';
import { getValidViewMode } from '../utils/get_valid_view_mode';
import { FetchStatus } from '../../types';

const MAX_NUM_OF_COLUMNS = 50;
Expand Down Expand Up @@ -57,7 +58,7 @@ export function useTextBasedQueryLanguage({
if (!query || next.fetchStatus === FetchStatus.ERROR) {
return;
}
const { columns: stateColumns, index } = stateContainer.appState.getState();
const { columns: stateColumns, index, viewMode } = stateContainer.appState.getState();
let nextColumns: string[] = [];
const isTextBasedQueryLang =
recordRawType === 'plain' && isOfAggregateQueryType(query) && 'sql' in query;
Expand Down Expand Up @@ -115,6 +116,9 @@ export function useTextBasedQueryLanguage({
const nextState = {
...(addDataViewToState && { index: dataViewObj.id }),
...(addColumnsToState && { columns: nextColumns }),
...(viewMode === VIEW_MODE.AGGREGATED_LEVEL && {
viewMode: getValidViewMode({ viewMode, isTextBasedQueryMode: true }),
}),
};
stateContainer.appState.replaceUrlState(nextState);
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,3 @@ export function getRawRecordType(query?: Query | AggregateQuery) {

return RecordRawType.DOCUMENT;
}

export function isPlainRecord(query?: Query | AggregateQuery): query is AggregateQuery {
return getRawRecordType(query) === RecordRawType.PLAIN;
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,9 @@

import { getStateDefaults } from './get_state_defaults';
import { createSearchSourceMock } from '@kbn/data-plugin/public/mocks';
import { VIEW_MODE } from '@kbn/saved-search-plugin/common';
import { dataViewWithTimefieldMock } from '../../../__mocks__/data_view_with_timefield';
import { savedSearchMock } from '../../../__mocks__/saved_search';
import { savedSearchMock, savedSearchMockWithSQL } from '../../../__mocks__/saved_search';
import { dataViewMock } from '../../../__mocks__/data_view';
import { discoverServiceMock } from '../../../__mocks__/services';

Expand Down Expand Up @@ -75,4 +76,42 @@ describe('getStateDefaults', () => {
}
`);
});

test('should set view mode correctly', () => {
const actualForUndefinedViewMode = getStateDefaults({
services: discoverServiceMock,
savedSearch: {
...savedSearchMockWithSQL,
viewMode: undefined,
},
});
expect(actualForUndefinedViewMode.viewMode).toBeUndefined();

const actualForTextBasedWithInvalidViewMode = getStateDefaults({
services: discoverServiceMock,
savedSearch: {
...savedSearchMockWithSQL,
viewMode: VIEW_MODE.AGGREGATED_LEVEL,
},
});
expect(actualForTextBasedWithInvalidViewMode.viewMode).toBe(VIEW_MODE.DOCUMENT_LEVEL);

const actualForTextBasedWithValidViewMode = getStateDefaults({
services: discoverServiceMock,
savedSearch: {
...savedSearchMockWithSQL,
viewMode: VIEW_MODE.DOCUMENT_LEVEL,
},
});
expect(actualForTextBasedWithValidViewMode.viewMode).toBe(VIEW_MODE.DOCUMENT_LEVEL);

const actualForWithValidViewMode = getStateDefaults({
services: discoverServiceMock,
savedSearch: {
...savedSearchMock,
viewMode: VIEW_MODE.AGGREGATED_LEVEL,
},
});
expect(actualForWithValidViewMode.viewMode).toBe(VIEW_MODE.AGGREGATED_LEVEL);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ import {
SEARCH_FIELDS_FROM_SOURCE,
SORT_DEFAULT_ORDER_SETTING,
} from '../../../../common';
import { isTextBasedQuery } from './is_text_based_query';
import { getValidViewMode } from './get_valid_view_mode';

function getDefaultColumns(savedSearch: SavedSearch, uiSettings: IUiSettingsClient) {
if (savedSearch.columns && savedSearch.columns.length > 0) {
Expand Down Expand Up @@ -81,7 +83,10 @@ export function getStateDefaults({
defaultState.rowHeight = savedSearch.rowHeight;
}
if (savedSearch.viewMode) {
defaultState.viewMode = savedSearch.viewMode;
defaultState.viewMode = getValidViewMode({
viewMode: savedSearch.viewMode,
isTextBasedQueryMode: isTextBasedQuery(query),
});
}
if (savedSearch.hideAggregatedPreview) {
defaultState.hideAggregatedPreview = savedSearch.hideAggregatedPreview;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
/*
* 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 { VIEW_MODE } from '@kbn/saved-search-plugin/common';
import { getValidViewMode } from './get_valid_view_mode';

describe('getValidViewMode', () => {
test('should work correctly for regular mode', () => {
expect(
getValidViewMode({
viewMode: undefined,
isTextBasedQueryMode: false,
})
).toBeUndefined();

expect(
getValidViewMode({
viewMode: VIEW_MODE.DOCUMENT_LEVEL,
isTextBasedQueryMode: false,
})
).toBe(VIEW_MODE.DOCUMENT_LEVEL);

expect(
getValidViewMode({
viewMode: VIEW_MODE.AGGREGATED_LEVEL,
isTextBasedQueryMode: false,
})
).toBe(VIEW_MODE.AGGREGATED_LEVEL);
});

test('should work correctly for text-based mode', () => {
expect(
getValidViewMode({
viewMode: undefined,
isTextBasedQueryMode: true,
})
).toBeUndefined();

expect(
getValidViewMode({
viewMode: VIEW_MODE.DOCUMENT_LEVEL,
isTextBasedQueryMode: true,
})
).toBe(VIEW_MODE.DOCUMENT_LEVEL);

expect(
getValidViewMode({
viewMode: VIEW_MODE.AGGREGATED_LEVEL,
isTextBasedQueryMode: true,
})
).toBe(VIEW_MODE.DOCUMENT_LEVEL);
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
/*
* 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 { VIEW_MODE } from '@kbn/saved-search-plugin/public';

/**
* Returns a valid view mode
* @param viewMode
* @param isTextBasedQueryMode
*/
export const getValidViewMode = ({
viewMode,
isTextBasedQueryMode,
}: {
viewMode?: VIEW_MODE;
isTextBasedQueryMode: boolean;
}): VIEW_MODE | undefined => {
if (viewMode === VIEW_MODE.AGGREGATED_LEVEL && isTextBasedQueryMode) {
// only this mode is supported for text-based languages
return VIEW_MODE.DOCUMENT_LEVEL;
}

return viewMode;
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/*
* 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 { isTextBasedQuery } from './is_text_based_query';

describe('isTextBasedQuery', () => {
it('should work correctly', () => {
expect(isTextBasedQuery({ query: '', language: 'lucene' })).toEqual(false);
expect(isTextBasedQuery({ sql: 'SELECT * from foo' })).toEqual(true);
expect(isTextBasedQuery()).toEqual(false);
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/*
* 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 type { AggregateQuery, Query } from '@kbn/es-query';
import { RecordRawType } from '../services/discover_data_state_container';
import { getRawRecordType } from './get_raw_record_type';

/**
* Checks if the query is of AggregateQuery type
* @param query
*/
export function isTextBasedQuery(query?: Query | AggregateQuery): query is AggregateQuery {
return getRawRecordType(query) === RecordRawType.PLAIN;
}
19 changes: 14 additions & 5 deletions src/plugins/discover/public/embeddable/saved_search_embeddable.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ import { SavedSearch } from '@kbn/saved-search-plugin/public';
import { METRIC_TYPE } from '@kbn/analytics';
import { VIEW_MODE } from '../../common/constants';
import { getSortForEmbeddable, SortPair } from '../utils/sorting';
import { RecordRawType } from '../application/main/services/discover_data_state_container';
import { buildDataTableRecord } from '../utils/build_data_record';
import { DataTableRecord, EsHitRecord } from '../types';
import { ISearchEmbeddable, SearchInput, SearchOutput } from './types';
Expand All @@ -59,7 +58,8 @@ import { DiscoverGridSettings } from '../components/discover_grid/types';
import { DocTableProps } from '../components/doc_table/doc_table_wrapper';
import { updateSearchSource } from './utils/update_search_source';
import { FieldStatisticsTable } from '../application/main/components/field_stats_table';
import { getRawRecordType } from '../application/main/utils/get_raw_record_type';
import { isTextBasedQuery } from '../application/main/utils/is_text_based_query';
import { getValidViewMode } from '../application/main/utils/get_valid_view_mode';
import { fetchSql } from '../application/main/utils/fetch_sql';
import { ADHOC_DATA_VIEW_RENDER_EVENT } from '../constants';

Expand Down Expand Up @@ -169,6 +169,11 @@ export class SavedSearchEmbeddable
return true;
}

private isTextBasedSearch = (savedSearch: SavedSearch): boolean => {
const query = savedSearch.searchSource.getField('query');
return isTextBasedQuery(query);
};

private fetch = async () => {
const searchSessionId = this.input.searchSessionId;
const useNewFieldsApi = !this.services.uiSettings.get(SEARCH_FIELDS_FROM_SOURCE, false);
Expand Down Expand Up @@ -229,8 +234,7 @@ export class SavedSearchEmbeddable

const query = this.savedSearch.searchSource.getField('query');
const dataView = this.savedSearch.searchSource.getField('index')!;
const recordRawType = getRawRecordType(query);
const useSql = recordRawType === RecordRawType.PLAIN;
const useSql = this.isTextBasedSearch(this.savedSearch);

try {
// Request SQL data
Expand Down Expand Up @@ -515,9 +519,14 @@ export class SavedSearchEmbeddable
return;
}

const viewMode = getValidViewMode({
viewMode: this.savedSearch.viewMode,
isTextBasedQueryMode: this.isTextBasedSearch(this.savedSearch),
});

if (
this.services.uiSettings.get(SHOW_FIELD_STATISTICS) === true &&
this.savedSearch.viewMode === VIEW_MODE.AGGREGATED_LEVEL &&
viewMode === VIEW_MODE.AGGREGATED_LEVEL &&
searchProps.services &&
searchProps.dataView &&
Array.isArray(searchProps.columns)
Expand Down

0 comments on commit f528b34

Please sign in to comment.