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.

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

[Mappings editor] Fix app crash when selecting "other" field type #79434

Merged
merged 2 commits into from
Oct 5, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -464,24 +464,58 @@ export const useField = <T>(
[errors]
);

/**
* Handler to update the state and make sure the component is still mounted.
* When resetting the form, some field might get unmounted (e.g. a toggle on "true" becomes "false" and now certain fields should not be in the DOM).
* In that scenario there is a race condition in the "reset" method below, because the useState() hook is not synchronous.
*
* A better approach would be to have the state in a reducer and being able to update all values in a single dispatch action.
*/
const updateStateIfMounted = useCallback(
(
state: 'isPristine' | 'isValidating' | 'isChangingValue' | 'isValidated' | 'errors' | 'value',
nextValue: any
) => {
if (isMounted.current === false) {
return;
}

switch (state) {
case 'value':
return setValue(nextValue);
case 'errors':
return setErrors(nextValue);
case 'isChangingValue':
return setIsChangingValue(nextValue);
case 'isPristine':
return setPristine(nextValue);
case 'isValidated':
return setIsValidated(nextValue);
case 'isValidating':
return setValidating(nextValue);
}
},
[setValue]
);

const reset: FieldHook<T>['reset'] = useCallback(
(resetOptions = { resetValue: true }) => {
const { resetValue = true, defaultValue: updatedDefaultValue } = resetOptions;

setPristine(true);
setValidating(false);
setIsChangingValue(false);
setIsValidated(false);
setErrors([]);
updateStateIfMounted('isPristine', true);
updateStateIfMounted('isValidating', false);
updateStateIfMounted('isChangingValue', false);
updateStateIfMounted('isValidated', false);
updateStateIfMounted('errors', []);

if (resetValue) {
hasBeenReset.current = true;
const newValue = deserializeValue(updatedDefaultValue ?? defaultValue);
setValue(newValue);
updateStateIfMounted('value', newValue);
return newValue;
}
},
[setValue, deserializeValue, defaultValue]
[updateStateIfMounted, deserializeValue, defaultValue]
);

// Don't take into account non blocker validation. Some are just warning (like trying to add a wrong ComboBox item)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/
import { act } from 'react-dom/test-utils';

import { componentHelpers, MappingsEditorTestBed } from '../helpers';

const { setup, getMappingsEditorDataFactory } = componentHelpers.mappingsEditor;

describe('Mappings editor: other datatype', () => {
/**
* Variable to store the mappings data forwarded to the consumer component
*/
let data: any;
let onChangeHandler: jest.Mock = jest.fn();
let getMappingsEditorData = getMappingsEditorDataFactory(onChangeHandler);
let testBed: MappingsEditorTestBed;

beforeAll(() => {
jest.useFakeTimers();
});

afterAll(() => {
jest.useRealTimers();
});

beforeEach(() => {
onChangeHandler = jest.fn();
getMappingsEditorData = getMappingsEditorDataFactory(onChangeHandler);
});

test('allow to add custom field type', async () => {
await act(async () => {
testBed = setup({ onChange: onChangeHandler });
});
testBed.component.update();

const {
component,
actions: { addField },
} = testBed;

await addField('myField', 'other', 'customType');

const mappings = {
properties: {
myField: {
type: 'customType',
},
},
};

({ data } = await getMappingsEditorData(component));
expect(data).toEqual(mappings);
});

test('allow to change a field type to a custom type', async () => {
const defaultMappings = {
properties: {
myField: {
type: 'text',
},
},
};

let updatedMappings = { ...defaultMappings };

await act(async () => {
testBed = setup({ value: defaultMappings, onChange: onChangeHandler });
});
testBed.component.update();

const {
component,
find,
form,
actions: { startEditField, updateFieldAndCloseFlyout },
} = testBed;

// Open the flyout to edit the field
await startEditField('myField');

// Change the field type
await act(async () => {
find('mappingsEditorFieldEdit.fieldType').simulate('change', [
{
label: 'other',
value: 'other',
},
]);
});
component.update();

form.setInputValue('mappingsEditorFieldEdit.fieldSubType', 'customType');

// Save the field and close the flyout
await updateFieldAndCloseFlyout();

updatedMappings = {
properties: {
myField: {
type: 'customType',
},
},
};

({ data } = await getMappingsEditorData(component));
expect(data).toEqual(updatedMappings);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ const createActions = (testBed: TestBed<TestSubjects>) => {
return { field: find(testSubject as TestSubjects), testSubject };
};

const addField = async (name: string, type: string) => {
const addField = async (name: string, type: string, subType?: string) => {
await act(async () => {
form.setInputValue('nameParameterInput', name);
find('createFieldForm.fieldType').simulate('change', [
Expand All @@ -160,6 +160,17 @@ const createActions = (testBed: TestBed<TestSubjects>) => {
]);
});

component.update();

if (subType !== undefined) {
await act(async () => {
if (type === 'other') {
// subType is a text input
form.setInputValue('createFieldForm.fieldSubType', subType);
}
});
}

await act(async () => {
find('createFieldForm.addButton').simulate('click');
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,39 +4,81 @@
* you may not use this file except in compliance with the Elastic License.
*/

import React from 'react';
import React, { useCallback } from 'react';

import { i18n } from '@kbn/i18n';
import { UseField, TextField, FieldConfig } from '../../../shared_imports';
import { fieldValidators } from '../../../shared_imports';

const { emptyField } = fieldValidators;
import { UseField, TextField, FieldConfig, FieldHook } from '../../../shared_imports';
import { getFieldConfig } from '../../../lib';

/**
* This is a special component that does not have an explicit entry in {@link PARAMETERS_DEFINITION}.
*
* We use it to store the name of types unknown to the mappings editor in the "subType" path.
*/

type FieldType = [{ value: string }];

const typeParameterConfig = getFieldConfig('type');

const fieldConfig: FieldConfig = {
label: i18n.translate('xpack.idxMgmt.mappingsEditor.otherTypeNameFieldLabel', {
defaultMessage: 'Type Name',
}),
defaultValue: '',
deserializer: typeParameterConfig.deserializer,
serializer: typeParameterConfig.serializer,
validations: [
{
validator: emptyField(
i18n.translate(
'xpack.idxMgmt.mappingsEditor.parameters.validations.otherTypeNameIsRequiredErrorMessage',
{
defaultMessage: 'The type name is required.',
}
)
),
validator: ({ value: fieldValue }) => {
if ((fieldValue as FieldType)[0].value.trim() === '') {
return {
message: i18n.translate(
'xpack.idxMgmt.mappingsEditor.parameters.validations.otherTypeNameIsRequiredErrorMessage',
{
defaultMessage: 'The type name is required.',
}
),
};
}
},
},
],
};

interface Props {
field: FieldHook<FieldType>;
}

/**
* The "subType" parameter can be configured either with a ComboBox (when the type is known)
* or with a TextField (when the type is unknown). This causes its value to have different type
* (either an array of object either a string). In order to align both value and let the consumer of
* the value worry about a single type, we will create a custom TextField component that works with the
* array of object that the ComboBox works with.
*/
const CustomTextField = ({ field }: Props) => {
const { setValue } = field;

const transformedField: FieldHook<any> = {
...field,
value: field.value[0]?.value ?? '',
};

const onChange = useCallback(
(e: React.ChangeEvent<HTMLInputElement>) => {
setValue([{ value: e.target.value }]);
},
[setValue]
);

return (
<TextField
field={transformedField}
euiFieldProps={{ onChange, 'data-test-subj': 'fieldSubType' }}
/>
);
};

export const OtherTypeNameParameter = () => (
<UseField path="subType" config={fieldConfig} component={TextField} />
<UseField path="subType" config={fieldConfig} component={CustomTextField} />
);
Original file line number Diff line number Diff line change
Expand Up @@ -91,8 +91,8 @@ export const EditField = React.memo(({ form, field, allFields, exitEdit, updateF
<FormDataProvider pathsToWatch={['type', 'subType']}>
{({ type, subType }) => {
const linkDocumentation =
documentationService.getTypeDocLink(subType?.[0].value) ||
documentationService.getTypeDocLink(type?.[0].value);
documentationService.getTypeDocLink(subType?.[0]?.value) ||
documentationService.getTypeDocLink(type?.[0]?.value);

if (!linkDocumentation) {
return null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ export const PARAMETERS_DEFINITION: { [key in ParameterName]: ParameterDefinitio
},
];
}
return [];
return [{ value: '' }];
},
serializer: (fieldType: ComboBoxOption[] | undefined) =>
fieldType && fieldType.length ? fieldType[0].value : fieldType,
Expand Down