Skip to content

Commit

Permalink
[Console] Fixes for console error handling and loading of autocomplete (
Browse files Browse the repository at this point in the history
#58587) (#59178)

* Fix console error handling when offline

In cases when the client cannot connect to server
the UI would get stuck in a loading state. We need to handle
that case explicitly to stop the progress spinner and report
the error correctly.

* Fix editor request cycle. Request should always complete

The bug was that the request could error in such a way that the
requestFail dispatch was not being called. Leaving the loading spinner
running and an unhelpful error message would appear.

Also partly fixed the loading of autocomplete data and cleaned up
a legacy import.

* Fixed loading of mappings in as they were updated from
settings modal.

* Fix the mappings update logic

TODO, this function needs to be revisited, but for now
it is convenient to have the Settings service passed in every
time so that the poller can be updated.

* Fix poll interval

* Address PR feedback

Rename variable (instance -> editorRegistry) and remove unused
file

Co-authored-by: Elastic Machine <[email protected]>

Co-authored-by: Elastic Machine <[email protected]>
  • Loading branch information
jloleysens and elasticmachine authored Mar 4, 2020
1 parent a181529 commit 8fd6f43
Show file tree
Hide file tree
Showing 14 changed files with 292 additions and 46 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import { I18nProvider } from '@kbn/i18n/react';
import { act } from 'react-dom/test-utils';
import * as sinon from 'sinon';

import { notificationServiceMock } from '../../../../../../../../core/public/mocks';
import { serviceContextMock } from '../../../../contexts/services_context.mock';

import { nextTick } from 'test_utils/enzyme_helpers';
import {
Expand Down Expand Up @@ -61,21 +61,7 @@ describe('Legacy (Ace) Console Editor Component Smoke Test', () => {

beforeEach(() => {
document.queryCommandSupported = sinon.fake(() => true);
mockedAppContextValue = {
elasticsearchUrl: 'test',
services: {
trackUiMetric: { count: () => {}, load: () => {} },
settings: {} as any,
storage: {} as any,
history: {
getSavedEditorState: () => ({} as any),
updateCurrentState: jest.fn(),
} as any,
notifications: notificationServiceMock.createSetupContract(),
objectStorageClient: {} as any,
},
docLinkVersion: 'NA',
};
mockedAppContextValue = serviceContextMock.create();
});

afterEach(() => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ const inputId = 'ConAppInputTextarea';

function EditorUI({ initialTextValue }: EditorProps) {
const {
services: { history, notifications },
services: { history, notifications, settings: settingsService },
docLinkVersion,
elasticsearchUrl,
} = useServicesContext();
Expand Down Expand Up @@ -172,7 +172,7 @@ function EditorUI({ initialTextValue }: EditorProps) {
setInputEditor(editor);
setTextArea(editorRef.current!.querySelector('textarea'));

mappings.retrieveAutoCompleteInfo();
mappings.retrieveAutoCompleteInfo(settingsService, settingsService.getAutocomplete());

const unsubscribeResizer = subscribeResizeChecker(editorRef.current!, editor);
setupAutosave();
Expand All @@ -182,7 +182,7 @@ function EditorUI({ initialTextValue }: EditorProps) {
mappings.clearSubscriptions();
window.removeEventListener('hashchange', onHashChange);
};
}, [saveCurrentTextObject, initialTextValue, history, setInputEditor]);
}, [saveCurrentTextObject, initialTextValue, history, setInputEditor, settingsService]);

useEffect(() => {
const { current: editor } = editorInstanceRef;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,10 @@ import { createReadOnlyAceEditor, CustomAceEditor } from '../../../../models/leg
import { subscribeResizeChecker } from '../subscribe_console_resize_checker';
import { applyCurrentSettings } from './apply_editor_settings';

function modeForContentType(contentType: string) {
function modeForContentType(contentType?: string) {
if (!contentType) {
return 'ace/mode/text';
}
if (contentType.indexOf('application/json') >= 0) {
return 'ace/mode/json';
} else if (contentType.indexOf('application/yaml') >= 0) {
Expand Down
19 changes: 11 additions & 8 deletions src/plugins/console/public/application/containers/settings.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import { AutocompleteOptions, DevToolsSettingsModal } from '../components';
// @ts-ignore
import mappings from '../../lib/mappings/mappings';
import { useServicesContext, useEditorActionContext } from '../contexts';
import { DevToolsSettings } from '../../services';
import { DevToolsSettings, Settings as SettingsService } from '../../services';

const getAutocompleteDiff = (newSettings: DevToolsSettings, prevSettings: DevToolsSettings) => {
return Object.keys(newSettings.autocomplete).filter(key => {
Expand All @@ -32,11 +32,12 @@ const getAutocompleteDiff = (newSettings: DevToolsSettings, prevSettings: DevToo
});
};

const refreshAutocompleteSettings = (selectedSettings: any) => {
mappings.retrieveAutoCompleteInfo(selectedSettings);
const refreshAutocompleteSettings = (settings: SettingsService, selectedSettings: any) => {
mappings.retrieveAutoCompleteInfo(settings, selectedSettings);
};

const fetchAutocompleteSettingsIfNeeded = (
settings: SettingsService,
newSettings: DevToolsSettings,
prevSettings: DevToolsSettings
) => {
Expand All @@ -60,10 +61,10 @@ const fetchAutocompleteSettingsIfNeeded = (
},
{}
);
mappings.retrieveAutoCompleteInfo(changedSettings.autocomplete);
} else if (isPollingChanged) {
mappings.retrieveAutoCompleteInfo(settings, changedSettings);
} else if (isPollingChanged && newSettings.polling) {
// If the user has turned polling on, then we'll fetch all selected autocomplete settings.
mappings.retrieveAutoCompleteInfo();
mappings.retrieveAutoCompleteInfo(settings, settings.getAutocomplete());
}
}
};
Expand All @@ -81,7 +82,7 @@ export function Settings({ onClose }: Props) {

const onSaveSettings = (newSettings: DevToolsSettings) => {
const prevSettings = settings.toJSON();
fetchAutocompleteSettingsIfNeeded(newSettings, prevSettings);
fetchAutocompleteSettingsIfNeeded(settings, newSettings, prevSettings);

// Update the new settings in localStorage
settings.updateSettings(newSettings);
Expand All @@ -98,7 +99,9 @@ export function Settings({ onClose }: Props) {
<DevToolsSettingsModal
onClose={onClose}
onSaveSettings={onSaveSettings}
refreshAutocompleteSettings={refreshAutocompleteSettings}
refreshAutocompleteSettings={(selectedSettings: any) =>
refreshAutocompleteSettings(settings, selectedSettings)
}
settings={settings.toJSON()}
/>
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
import { SenseEditor } from '../../models/sense_editor';

export class EditorRegistry {
inputEditor: SenseEditor | undefined;
private inputEditor: SenseEditor | undefined;

setInputEditor(inputEditor: SenseEditor) {
this.inputEditor = inputEditor;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
import { notificationServiceMock } from '../../../../../core/public/mocks';
import { HistoryMock } from '../../services/history.mock';
import { SettingsMock } from '../../services/settings.mock';
import { StorageMock } from '../../services/storage.mock';

import { ContextValue } from './services_context';

export const serviceContextMock = {
create: (): ContextValue => {
const storage = new StorageMock({} as any, 'test');
(storage.keys as jest.Mock).mockImplementation(() => []);
return {
elasticsearchUrl: 'test',
services: {
trackUiMetric: { count: () => {}, load: () => {} },
storage,
settings: new SettingsMock(storage),
history: new HistoryMock(storage),
notifications: notificationServiceMock.createSetupContract(),
objectStorageClient: {} as any,
},
docLinkVersion: 'NA',
};
},
};
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ export function ServicesContextProvider({ children, value }: ContextProps) {
export const useServicesContext = () => {
const context = useContext(ServicesContext);
if (context === undefined) {
throw new Error('useAppContext must be used inside the AppContextProvider.');
throw new Error('useServicesContext must be used inside the ServicesContextProvider.');
}
return context;
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

jest.mock('./send_request_to_es', () => ({ sendRequestToES: jest.fn() }));
jest.mock('../../contexts/editor_context/editor_registry', () => ({
instance: { getInputEditor: jest.fn() },
}));
jest.mock('./track', () => ({ track: jest.fn() }));
jest.mock('../../contexts/request_context', () => ({ useRequestActionContext: jest.fn() }));

import React from 'react';
import { renderHook, act } from '@testing-library/react-hooks';

import { ContextValue, ServicesContextProvider } from '../../contexts';
import { serviceContextMock } from '../../contexts/services_context.mock';
import { useRequestActionContext } from '../../contexts/request_context';
import { instance as editorRegistry } from '../../contexts/editor_context/editor_registry';

import { sendRequestToES } from './send_request_to_es';
import { useSendCurrentRequestToES } from './use_send_current_request_to_es';

describe('useSendCurrentRequestToES', () => {
let mockContextValue: ContextValue;
let dispatch: (...args: any[]) => void;
const contexts = ({ children }: { children?: any }) => (
<ServicesContextProvider value={mockContextValue}>{children}</ServicesContextProvider>
);

beforeEach(() => {
mockContextValue = serviceContextMock.create();
dispatch = jest.fn();
(useRequestActionContext as jest.Mock).mockReturnValue(dispatch);
});

afterEach(() => {
jest.resetAllMocks();
});

it('calls send request to ES', async () => {
// Set up mocks
(mockContextValue.services.settings.toJSON as jest.Mock).mockReturnValue({});
// This request should succeed
(sendRequestToES as jest.Mock).mockResolvedValue([]);
(editorRegistry.getInputEditor as jest.Mock).mockImplementation(() => ({
getRequestsInRange: () => ['test'],
}));

const { result } = renderHook(() => useSendCurrentRequestToES(), { wrapper: contexts });
await act(() => result.current());
expect(sendRequestToES).toHaveBeenCalledWith({ requests: ['test'] });

// Second call should be the request success
const [, [requestSucceededCall]] = (dispatch as jest.Mock).mock.calls;
expect(requestSucceededCall).toEqual({ type: 'requestSuccess', payload: { data: [] } });
});

it('handles known errors', async () => {
// Set up mocks
(sendRequestToES as jest.Mock).mockRejectedValue({ response: 'nada' });
(editorRegistry.getInputEditor as jest.Mock).mockImplementation(() => ({
getRequestsInRange: () => ['test'],
}));

const { result } = renderHook(() => useSendCurrentRequestToES(), { wrapper: contexts });
await act(() => result.current());
// Second call should be the request failure
const [, [requestFailedCall]] = (dispatch as jest.Mock).mock.calls;

// The request must have concluded
expect(requestFailedCall).toEqual({ type: 'requestFail', payload: { response: 'nada' } });
});

it('handles unknown errors', async () => {
// Set up mocks
(sendRequestToES as jest.Mock).mockRejectedValue(NaN /* unexpected error value */);
(editorRegistry.getInputEditor as jest.Mock).mockImplementation(() => ({
getRequestsInRange: () => ['test'],
}));

const { result } = renderHook(() => useSendCurrentRequestToES(), { wrapper: contexts });
await act(() => result.current());
// Second call should be the request failure
const [, [requestFailedCall]] = (dispatch as jest.Mock).mock.calls;

// The request must have concluded
expect(requestFailedCall).toEqual({ type: 'requestFail', payload: undefined });
// It also notified the user
expect(mockContextValue.services.notifications.toasts.addError).toHaveBeenCalledWith(NaN, {
title: 'Unknown Request Error',
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ export const useSendCurrentRequestToES = () => {
// or templates may have changed, so we'll need to update this data. Assume that if
// the user disables polling they're trying to optimize performance or otherwise
// preserve resources, so they won't want this request sent either.
mappings.retrieveAutoCompleteInfo();
mappings.retrieveAutoCompleteInfo(settings, settings.getAutocomplete());
}

dispatch({
Expand All @@ -74,12 +74,16 @@ export const useSendCurrentRequestToES = () => {
},
});
} catch (e) {
if (e.response?.contentType) {
if (e?.response) {
dispatch({
type: 'requestFail',
payload: e,
});
} else {
dispatch({
type: 'requestFail',
payload: undefined,
});
notifications.toasts.addError(e, {
title: i18n.translate('console.notification.unknownRequestErrorTitle', {
defaultMessage: 'Unknown Request Error',
Expand Down
2 changes: 1 addition & 1 deletion src/plugins/console/public/application/stores/request.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import { ESRequestResult } from '../hooks/use_send_current_request_to_es/send_re
export type Actions =
| { type: 'sendRequest'; payload: undefined }
| { type: 'requestSuccess'; payload: { data: ESRequestResult[] } }
| { type: 'requestFail'; payload: ESRequestResult<string> };
| { type: 'requestFail'; payload: ESRequestResult<string> | undefined };

export interface Store {
requestInFlight: boolean;
Expand Down
Loading

0 comments on commit 8fd6f43

Please sign in to comment.