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.

Sign up for GitHub

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

[Enterprise Search] Fix bug in Add Schema modal #104024

Merged
merged 5 commits into from
Jul 2, 2021
Merged

[Enterprise Search] Fix bug in Add Schema modal #104024

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cbbf500
[Enterprise Search] Fix bug in Add Schema modal
scottybollinger Jun 30, 2021
50ed764
Use default message when error response for actions.setServerField ha…
byronhulcher Jul 1, 2021
3a6f3d0
Merge branch 'master' into scottybollinger/schema-bug
kibanamachine Jul 1, 2021
906c4b9
i18n-ize default error message
byronhulcher Jul 1, 2021
299c963
Remove TODO
byronhulcher Jul 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions .../plugins/enterprise_search/public/applications/shared/flash_messages/handle_api_errors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,13 @@ interface Options {
isQueued?: boolean;
}

// TODO I know our error messages from the BE are not i18n-ized but should this be?
Copy link
Contributor

Choose a reason for hiding this comment

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

@constancecchen

Copy link
Contributor

Choose a reason for hiding this comment

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

Currently none of our error messages from the BE are localized, but if this one is in public/ it has access to i18n, so we should likely i18n it just in case.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also want to add it wouldn't be my preference to export this const. I'd just check for the static 'An unexpected error occurred' string in the schema logic file. It's short, easy to read, and gives developers human-readable context over a variable that means nothing if they don't open another file to check it

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh wait, sorry, just saw we're using it in the actual logic file, not just the test. Hm, sec

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll i18n this

export const defaultErrorMessage = 'An unexpected error occurred';

/**
* Converts API/HTTP errors into user-facing Flash Messages
*/
export const flashAPIErrors = (error: HttpResponse<ErrorResponse>, { 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 }];
Expand Down
19 changes: 16 additions & 3 deletions ...pplications/workplace_search/views/content_sources/components/schema/schema_logic.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -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);
});
});

Expand Down
5 changes: 4 additions & 1 deletion ...lic/applications/workplace_search/views/content_sources/components/schema/schema_logic.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -349,7 +350,9 @@ export const SchemaLogic = kea<MakeLogicType<SchemaValues, SchemaActions>>({
} 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);
}
Comment on lines 352 to 358
Copy link
Contributor

@cee-chen cee-chen Jul 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Why not just add the e?.body?.message check into the conditional and let invalid error formats fall back to flashAPIErrors, instead having to manually fall back to defaultErrorMessage?

Suggested change
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);
}
const errorMessage: string[] = e?.body?.message;
if (isAdding && errorMessage) {
actions.onSchemaSetFormErrors(errorMessage);
} else {
flashAPIErrors(e);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this would be a slight functional difference, in that when the message is missing, it would appear as a Flash Message error. actions.onSchemaSetFormErrors sets addFormFieldErrors which is used inside a SchemaAddFieldModal. I think we'd still want the default error message to appear in the SchemaAddFieldModal.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have a strong opinion either way. I guess I'm leaning slightly towards showing the error in the modal, closer to the user action that lead to the error. But I see the point in showing it as a flash message: if the error is unexpected, it's probably not caused by user action.

I will merge the PR as-is because the code fixes the original bug and is already tested.

Expand Down