From 9a4fc7c67d1e7d9714df466ea8191ff2f1e4cf5e Mon Sep 17 00:00:00 2001 From: Scotty Bollinger Date: Fri, 2 Jul 2021 09:25:41 -0500 Subject: [PATCH 1/2] [Enterprise Search] Fix bug in Add Schema modal (#104024) * [Enterprise Search] Fix bug in Add Schema modal This PR fixes a bug where an error passed from the server was not rendering and was causing the UI to hang. The problem is that the `body` property was missed in the error object. * Use default message when error response for actions.setServerField has no message * i18n-ize default error message * Remove TODO Co-authored-by: Byron Hulcher Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> --- .../flash_messages/handle_api_errors.ts | 11 +++++++++-- .../components/schema/schema_logic.test.ts | 19 ++++++++++++++++--- .../components/schema/schema_logic.ts | 5 ++++- 3 files changed, 29 insertions(+), 6 deletions(-) diff --git a/x-pack/plugins/enterprise_search/public/applications/shared/flash_messages/handle_api_errors.ts b/x-pack/plugins/enterprise_search/public/applications/shared/flash_messages/handle_api_errors.ts index 11003d0fcc171..1b5dab0839663 100644 --- a/x-pack/plugins/enterprise_search/public/applications/shared/flash_messages/handle_api_errors.ts +++ b/x-pack/plugins/enterprise_search/public/applications/shared/flash_messages/handle_api_errors.ts @@ -5,6 +5,8 @@ * 2.0. */ +import { i18n } from '@kbn/i18n'; + import { HttpResponse } from 'src/core/public'; import { FlashMessagesLogic } from './flash_messages_logic'; @@ -31,12 +33,17 @@ interface Options { isQueued?: boolean; } +export const defaultErrorMessage = i18n.translate( + 'xpack.enterpriseSearch.shared.flashMessages.defaultErrorMessage', + { + defaultMessage: 'An unexpected error occurred', + } +); + /** * Converts API/HTTP errors into user-facing Flash Messages */ export const flashAPIErrors = (error: HttpResponse, { isQueued }: Options = {}) => { - const defaultErrorMessage = 'An unexpected error occurred'; - const errorFlashMessages: IFlashMessage[] = Array.isArray(error?.body?.attributes?.errors) ? error.body!.attributes.errors.map((message) => ({ type: 'error', message })) : [{ type: 'error', message: error?.body?.message || defaultErrorMessage }]; diff --git a/x-pack/plugins/enterprise_search/public/applications/workplace_search/views/content_sources/components/schema/schema_logic.test.ts b/x-pack/plugins/enterprise_search/public/applications/workplace_search/views/content_sources/components/schema/schema_logic.test.ts index fd1a574b3438f..3e8322145dad6 100644 --- a/x-pack/plugins/enterprise_search/public/applications/workplace_search/views/content_sources/components/schema/schema_logic.test.ts +++ b/x-pack/plugins/enterprise_search/public/applications/workplace_search/views/content_sources/components/schema/schema_logic.test.ts @@ -27,6 +27,7 @@ const spyScrollTo = jest.fn(); Object.defineProperty(global.window, 'scrollTo', { value: spyScrollTo }); import { ADD, UPDATE } from '../../../../../shared/constants/operations'; +import { defaultErrorMessage } from '../../../../../shared/flash_messages/handle_api_errors'; import { SchemaType } from '../../../../../shared/schema/types'; import { AppLogic } from '../../../../app_logic'; @@ -390,13 +391,25 @@ describe('SchemaLogic', () => { expect(onSchemaSetSuccessSpy).toHaveBeenCalledWith(serverResponse); }); - it('handles error', async () => { + it('handles error with message', async () => { + const onSchemaSetFormErrorsSpy = jest.spyOn(SchemaLogic.actions, 'onSchemaSetFormErrors'); + // We expect body.message to be a string[] when it is present + http.post.mockReturnValue(Promise.reject({ body: { message: ['this is an error'] } })); + SchemaLogic.actions.setServerField(schema, ADD); + await nextTick(); + + expect(onSchemaSetFormErrorsSpy).toHaveBeenCalledWith(['this is an error']); + expect(spyScrollTo).toHaveBeenCalledWith(0, 0); + }); + + it('handles error with no message', async () => { const onSchemaSetFormErrorsSpy = jest.spyOn(SchemaLogic.actions, 'onSchemaSetFormErrors'); - http.post.mockReturnValue(Promise.reject({ message: 'this is an error' })); + http.post.mockReturnValue(Promise.reject()); SchemaLogic.actions.setServerField(schema, ADD); await nextTick(); - expect(onSchemaSetFormErrorsSpy).toHaveBeenCalledWith('this is an error'); + expect(onSchemaSetFormErrorsSpy).toHaveBeenCalledWith([defaultErrorMessage]); + expect(spyScrollTo).toHaveBeenCalledWith(0, 0); }); }); diff --git a/x-pack/plugins/enterprise_search/public/applications/workplace_search/views/content_sources/components/schema/schema_logic.ts b/x-pack/plugins/enterprise_search/public/applications/workplace_search/views/content_sources/components/schema/schema_logic.ts index b2c329f0544fd..7af074d412a60 100644 --- a/x-pack/plugins/enterprise_search/public/applications/workplace_search/views/content_sources/components/schema/schema_logic.ts +++ b/x-pack/plugins/enterprise_search/public/applications/workplace_search/views/content_sources/components/schema/schema_logic.ts @@ -17,6 +17,7 @@ import { setErrorMessage, clearFlashMessages, } from '../../../../../shared/flash_messages'; +import { defaultErrorMessage } from '../../../../../shared/flash_messages/handle_api_errors'; import { HttpLogic } from '../../../../../shared/http'; import { IndexJob, @@ -349,7 +350,9 @@ export const SchemaLogic = kea>({ } catch (e) { window.scrollTo(0, 0); if (isAdding) { - actions.onSchemaSetFormErrors(e?.message); + // We expect body.message to be a string[] for actions.onSchemaSetFormErrors + const message: string[] = e?.body?.message || [defaultErrorMessage]; + actions.onSchemaSetFormErrors(message); } else { flashAPIErrors(e); } From 18834cf0adec616cb0a7fc55ad713b324690f9a5 Mon Sep 17 00:00:00 2001 From: Scotty Bollinger Date: Wed, 7 Jul 2021 07:42:21 -0500 Subject: [PATCH 2/2] [Workplace Search] Fix bug where error was behind modal stuck in loading state (#104360) * Fix an issue from previous PR In https://github.com/elastic/kibana/pull/104024, the error handling incorrectly used the `message` property on the response, when it should have been the attributes.errors array. * Use inline error for duplicate name --- .../components/schema/schema_logic.test.ts | 9 ++++++--- .../content_sources/components/schema/schema_logic.ts | 10 +++++----- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/x-pack/plugins/enterprise_search/public/applications/workplace_search/views/content_sources/components/schema/schema_logic.test.ts b/x-pack/plugins/enterprise_search/public/applications/workplace_search/views/content_sources/components/schema/schema_logic.test.ts index 3e8322145dad6..d642900aea169 100644 --- a/x-pack/plugins/enterprise_search/public/applications/workplace_search/views/content_sources/components/schema/schema_logic.test.ts +++ b/x-pack/plugins/enterprise_search/public/applications/workplace_search/views/content_sources/components/schema/schema_logic.test.ts @@ -325,10 +325,11 @@ describe('SchemaLogic', () => { }); it('handles duplicate', () => { + const onSchemaSetFormErrorsSpy = jest.spyOn(SchemaLogic.actions, 'onSchemaSetFormErrors'); SchemaLogic.actions.onInitializeSchema(serverResponse); SchemaLogic.actions.addNewField('foo', SchemaType.Number); - expect(setErrorMessage).toHaveBeenCalledWith('New field already exists: foo.'); + expect(onSchemaSetFormErrorsSpy).toHaveBeenCalledWith(['New field already exists: foo.']); }); }); @@ -393,8 +394,10 @@ describe('SchemaLogic', () => { it('handles error with message', async () => { const onSchemaSetFormErrorsSpy = jest.spyOn(SchemaLogic.actions, 'onSchemaSetFormErrors'); - // We expect body.message to be a string[] when it is present - http.post.mockReturnValue(Promise.reject({ body: { message: ['this is an error'] } })); + // We expect body.attributes.errors to be a string[] when it is present + http.post.mockReturnValue( + Promise.reject({ body: { attributes: { errors: ['this is an error'] } } }) + ); SchemaLogic.actions.setServerField(schema, ADD); await nextTick(); diff --git a/x-pack/plugins/enterprise_search/public/applications/workplace_search/views/content_sources/components/schema/schema_logic.ts b/x-pack/plugins/enterprise_search/public/applications/workplace_search/views/content_sources/components/schema/schema_logic.ts index 7af074d412a60..f43be974102b2 100644 --- a/x-pack/plugins/enterprise_search/public/applications/workplace_search/views/content_sources/components/schema/schema_logic.ts +++ b/x-pack/plugins/enterprise_search/public/applications/workplace_search/views/content_sources/components/schema/schema_logic.ts @@ -301,15 +301,15 @@ export const SchemaLogic = kea>({ addNewField: ({ fieldName, newFieldType }) => { if (fieldName in values.activeSchema) { window.scrollTo(0, 0); - setErrorMessage( + actions.onSchemaSetFormErrors([ i18n.translate( 'xpack.enterpriseSearch.workplaceSearch.contentSource.schema.newFieldExists.message', { defaultMessage: 'New field already exists: {fieldName}.', values: { fieldName }, } - ) - ); + ), + ]); } else { const schema = cloneDeep(values.activeSchema); schema[fieldName] = newFieldType; @@ -350,8 +350,8 @@ export const SchemaLogic = kea>({ } catch (e) { window.scrollTo(0, 0); if (isAdding) { - // We expect body.message to be a string[] for actions.onSchemaSetFormErrors - const message: string[] = e?.body?.message || [defaultErrorMessage]; + // We expect body.attributes.errors to be a string[] for actions.onSchemaSetFormErrors + const message: string[] = e?.body?.attributes?.errors || [defaultErrorMessage]; actions.onSchemaSetFormErrors(message); } else { flashAPIErrors(e);