Skip to content

Commit

Permalink
[discover] do not show old results on error (#170769)
Browse files Browse the repository at this point in the history 
Prerequisite for #167904

### Test
1. install sample web logs
2. Open discover
3. Select time range that displays results
4. Add filter
    ```
    {
      "error_query": {
        "indices": [
          {
            "error_type": "exception",
            "message": "local shard failure message 123",
            "name": "kibana_sample_data_logs"
          }
        ]
      }
    }
    ```
5. Verify discover should display `EmptyPrompt` showing error message
instead of callout with stale data.
<img width="500" alt="Screenshot 2023-11-07 at 9 23 43 AM"
src="https://github.com/elastic/kibana/assets/373691/60237f8c-dcf5-4b4a-991f-4a9f073048af">

---------

Co-authored-by: kibanamachine <[email protected]>
  • Loading branch information
@nreese @kibanamachine
nreese and kibanamachine authored Nov 10, 2023
1 parent bc2202d commit c078b91
Show file tree
Hide file tree
Showing 22 changed files with 78 additions and 257 deletions.
14 changes: 0 additions & 14 deletions src/plugins/discover/public/application/main/components/layout/discover_documents.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ import {
SHOW_MULTIFIELDS,
SORT_DEFAULT_ORDER_SETTING,
} from '@kbn/discover-utils';
import { i18n } from '@kbn/i18n';
import useObservable from 'react-use/lib/useObservable';
import type { DocViewFilterFn } from '@kbn/unified-doc-viewer/types';
import { DiscoverGrid } from '../../../../components/discover_grid';
Expand Down Expand Up @@ -65,7 +64,6 @@ import { DiscoverGridFlyout } from '../../../../components/discover_grid_flyout'
import { getRenderCustomToolbarWithElements } from '../../../../components/discover_grid/render_custom_toolbar';
import { useSavedSearchInitial } from '../../services/discover_state_provider';
import { useFetchMoreRecords } from './use_fetch_more_records';
import { ErrorCallout } from '../../../../components/common/error_callout';
import { SelectedVSAvailableCallout } from './selected_vs_available_callout';

const containerStyles = css`
Expand Down Expand Up @@ -256,22 +254,11 @@ function DiscoverDocumentsComponent({
[dataView, onAddColumn, onAddFilter, onRemoveColumn, query, savedSearch.id, setExpandedDoc]
);

const dataState = useDataState(stateContainer.dataState.data$.main$);
const documents = useObservable(stateContainer.dataState.data$.documents$);

const callouts = useMemo(
() => (
<>
{dataState.error && (
<ErrorCallout
title={i18n.translate('discover.documentsErrorTitle', {
defaultMessage: 'Search error',
})}
error={dataState.error}
inline
data-test-subj="discoverMainError"
/>
)}
<SelectedVSAvailableCallout
isPlainRecord={isTextBasedQuery}
textBasedQueryColumns={documents?.textBasedQueryColumns}
Expand All @@ -281,7 +268,6 @@ function DiscoverDocumentsComponent({
</>
),
[
dataState.error,
isTextBasedQuery,
currentColumns,
documents?.textBasedQueryColumns,
Expand Down
5 changes: 2 additions & 3 deletions src/plugins/discover/public/application/main/components/layout/discover_layout.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -114,8 +114,8 @@ export function DiscoverLayout({ stateContainer }: DiscoverLayoutProps) {

const isPlainRecord = useMemo(() => getRawRecordType(query) === RecordRawType.PLAIN, [query]);
const resultState = useMemo(
() => getResultState(dataState.fetchStatus, dataState.foundDocuments!, isPlainRecord),
[dataState.fetchStatus, dataState.foundDocuments, isPlainRecord]
() => getResultState(dataState.fetchStatus, dataState.foundDocuments ?? false),
[dataState.fetchStatus, dataState.foundDocuments]
);

const onOpenInspector = useInspector({
Expand Down Expand Up @@ -338,7 +338,6 @@ export function DiscoverLayout({ stateContainer }: DiscoverLayoutProps) {
}
)}
error={dataState.error}
data-test-subj="discoverNoResultsError"
/>
) : (
<DiscoverNoResults
Expand Down
23 changes: 14 additions & 9 deletions src/plugins/discover/public/application/main/utils/get_result_state.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,33 +9,38 @@ import { getResultState, resultStatuses } from './get_result_state';
import { FetchStatus } from '../../types';

describe('getResultState', () => {
test('fetching uninitialized', () => {
test(`should return 'uninitialized' when fetching uninitialized`, () => {
const actual = getResultState(FetchStatus.UNINITIALIZED, false);
expect(actual).toBe(resultStatuses.UNINITIALIZED);
});

test('fetching complete with no records', () => {
test(`should return 'loading' when fetching is loading`, () => {
const actual = getResultState(FetchStatus.LOADING, false);
expect(actual).toBe(resultStatuses.LOADING);
});

test(`should return 'none' when fetching is complete with no records`, () => {
const actual = getResultState(FetchStatus.COMPLETE, false);
expect(actual).toBe(resultStatuses.NO_RESULTS);
});

test('fetching ongoing aka loading', () => {
const actual = getResultState(FetchStatus.LOADING, false);
expect(actual).toBe(resultStatuses.LOADING);
test(`should return 'none' after a fetch error`, () => {
const actual = getResultState(FetchStatus.ERROR, false);
expect(actual).toBe(resultStatuses.NO_RESULTS);
});

test('fetching ready', () => {
test(`should return 'ready' when fetching completes with records`, () => {
const actual = getResultState(FetchStatus.COMPLETE, true);
expect(actual).toBe(resultStatuses.READY);
});

test('re-fetching after already data is available', () => {
test(`should reurn 'ready' when re-fetching after already data is available`, () => {
const actual = getResultState(FetchStatus.LOADING, true);
expect(actual).toBe(resultStatuses.READY);
});

test('after a fetch error when data was successfully fetched before ', () => {
test(`should return 'none' after a fetch error when data was successfully fetched before`, () => {
const actual = getResultState(FetchStatus.ERROR, true);
expect(actual).toBe(resultStatuses.READY);
expect(actual).toBe(resultStatuses.NO_RESULTS);
});
});
8 changes: 2 additions & 6 deletions src/plugins/discover/public/application/main/utils/get_result_state.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,11 @@ export const resultStatuses = {
* Returns the current state of the result, depends on fetchStatus and the given fetched rows
* Determines what is displayed in Discover main view (loading view, data view, empty data view, ...)
*/
export function getResultState(
fetchStatus: FetchStatus,
foundDocuments: boolean = false,
isPlainRecord?: boolean
) {
export function getResultState(fetchStatus: FetchStatus, foundDocuments: boolean = false) {
if (fetchStatus === FetchStatus.UNINITIALIZED) {
return resultStatuses.UNINITIALIZED;
}
if (isPlainRecord && fetchStatus === FetchStatus.ERROR) return resultStatuses.NO_RESULTS;
if (fetchStatus === FetchStatus.ERROR) return resultStatuses.NO_RESULTS;

if (!foundDocuments && fetchStatus === FetchStatus.LOADING) return resultStatuses.LOADING;
else if (foundDocuments) return resultStatuses.READY;
Expand Down
77 changes: 4 additions & 73 deletions src/plugins/discover/public/components/common/error_callout.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
* Side Public License, v 1.
*/

import { EuiButton, EuiCallOut, EuiEmptyPrompt, EuiLink, EuiModal } from '@elastic/eui';
import { EuiButton, EuiEmptyPrompt } from '@elastic/eui';
import { KibanaContextProvider } from '@kbn/kibana-react-plugin/public';
import { findTestSubject } from '@kbn/test-jest-helpers';
import { mount } from 'enzyme';
Expand Down Expand Up @@ -37,14 +37,11 @@ describe('ErrorCallout', () => {
it('should render', () => {
const title = 'Error title';
const error = new Error('My error');
const wrapper = mountWithServices(
<ErrorCallout title={title} error={error} data-test-subj="errorCallout" />
);
const wrapper = mountWithServices(<ErrorCallout title={title} error={error} />);
const prompt = wrapper.find(EuiEmptyPrompt);
expect(prompt).toHaveLength(1);
expect(prompt.prop('title')).toBeDefined();
expect(prompt.prop('title')).not.toBeInstanceOf(String);
expect(prompt.prop('data-test-subj')).toBe('errorCallout');
expect(prompt.prop('body')).toBeDefined();
expect(findTestSubject(prompt, 'discoverErrorCalloutTitle').contains(title)).toBe(true);
expect(findTestSubject(prompt, 'discoverErrorCalloutMessage').contains(error.message)).toBe(
Expand All @@ -53,97 +50,31 @@ describe('ErrorCallout', () => {
expect(prompt.find(EuiButton)).toHaveLength(1);
});

it('should render inline', () => {
const title = 'Error title';
const error = new Error('My error');
const wrapper = mountWithServices(
<ErrorCallout title={title} error={error} inline data-test-subj="errorCallout" />
);
const callout = wrapper.find(EuiCallOut);
expect(callout).toHaveLength(1);
expect(callout.prop('title')).toBeDefined();
expect(callout.prop('title')).not.toBeInstanceOf(String);
expect(callout.prop('size')).toBe('s');
expect(callout.prop('data-test-subj')).toBe('errorCallout');
expect(
findTestSubject(callout, 'discoverErrorCalloutMessage').contains(`${title}: ${error.message}`)
).toBe(true);
expect(callout.find(EuiLink)).toHaveLength(1);
});

it('should render with override display', () => {
const title = 'Override title';
const error = new Error('My error');
const overrideDisplay = <div>Override display</div>;
mockGetSearchErrorOverrideDisplay.mockReturnValue({ title, body: overrideDisplay });
const wrapper = mountWithServices(
<ErrorCallout title="Original title" error={error} data-test-subj="errorCallout" />
);
const wrapper = mountWithServices(<ErrorCallout title="Original title" error={error} />);
const prompt = wrapper.find(EuiEmptyPrompt);
expect(prompt).toHaveLength(1);
expect(prompt.prop('title')).toBeDefined();
expect(prompt.prop('title')).not.toBeInstanceOf(String);
expect(prompt.prop('data-test-subj')).toBe('errorCallout');
expect(prompt.prop('body')).toBeDefined();
expect(findTestSubject(prompt, 'discoverErrorCalloutTitle').contains(title)).toBe(true);
expect(prompt.contains(overrideDisplay)).toBe(true);
expect(prompt.find(EuiButton)).toHaveLength(0);
});

it('should render with override display and inline', () => {
const title = 'Override title';
const error = new Error('My error');
const overrideDisplay = <div>Override display</div>;
mockGetSearchErrorOverrideDisplay.mockReturnValue({ title, body: overrideDisplay });
const wrapper = mountWithServices(
<ErrorCallout title="Original title" error={error} inline data-test-subj="errorCallout" />
);
const callout = wrapper.find(EuiCallOut);
expect(callout).toHaveLength(1);
expect(callout.prop('title')).toBeDefined();
expect(callout.prop('title')).not.toBeInstanceOf(String);
expect(callout.prop('size')).toBe('s');
expect(callout.prop('data-test-subj')).toBe('errorCallout');
expect(callout.find(EuiLink)).toHaveLength(1);
expect(wrapper.find(EuiModal)).toHaveLength(0);
expect(wrapper.contains(title)).toBe(true);
expect(wrapper.contains(overrideDisplay)).toBe(false);
callout.find(EuiLink).simulate('click');
expect(wrapper.find(EuiModal)).toHaveLength(1);
expect(findTestSubject(wrapper, 'discoverErrorCalloutOverrideModalTitle').contains(title)).toBe(
true
);
expect(
findTestSubject(wrapper, 'discoverErrorCalloutOverrideModalBody').contains(overrideDisplay)
).toBe(true);
expect(wrapper.contains(overrideDisplay)).toBe(true);
});

it('should call showErrorDialog when the button is clicked', () => {
(discoverServiceMock.core.notifications.showErrorDialog as jest.Mock).mockClear();
const title = 'Error title';
const error = new Error('My error');
const wrapper = mountWithServices(
<ErrorCallout title={title} error={error} data-test-subj="errorCallout" />
);
const wrapper = mountWithServices(<ErrorCallout title={title} error={error} />);
wrapper.find(EuiButton).find('button').simulate('click');
expect(discoverServiceMock.core.notifications.showErrorDialog).toHaveBeenCalledWith({
title,
error,
});
});

it('should call showErrorDialog when the button is clicked inline', () => {
(discoverServiceMock.core.notifications.showErrorDialog as jest.Mock).mockClear();
const title = 'Error title';
const error = new Error('My error');
const wrapper = mountWithServices(
<ErrorCallout title={title} error={error} inline data-test-subj="errorCallout" />
);
wrapper.find(EuiLink).find('button').simulate('click');
expect(discoverServiceMock.core.notifications.showErrorDialog).toHaveBeenCalledWith({
title,
error,
});
});
});
Loading

0 comments on commit c078b91

Please sign in to comment.