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

[ML] Transforms: Adds docs_per_second to transform edit form. #65365

Merged
merged 1 commit into from
May 6, 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
4 changes: 4 additions & 0 deletions x-pack/plugins/transform/public/app/common/transform.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,10 @@ export interface CreateRequestBody extends PreviewRequestBody {
index: IndexName;
};
frequency?: string;
settings?: {
max_page_search_size?: number;
docs_per_second?: number;
};
sync?: {
time: {
field: string;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,14 +32,21 @@ export const EditTransformFlyoutForm: FC<EditTransformFlyoutFormProps> = ({
onChange={value => dispatch({ field: 'description', value })}
value={formFields.description.value}
/>
{/*
<EditTransformFlyoutFormTextInput
defaultValue={config.dest.index}
label={i18n.translate('xpack.transform.transformList.editFlyoutFormDestinationIndexLabel', {
defaultMessage: 'Destination Index',
errorMessages={formFields.docsPerSecond.errorMessages}
helpText={i18n.translate(
'xpack.transform.transformList.editFlyoutFormDocsPerSecondHelptext',
{
defaultMessage:
'To enable throttling, set a limit of documents per second of input documents.',
Copy link
Contributor

Choose a reason for hiding this comment

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

One for a follow-up - is there any guidance we can give as to what is a sensible value here?

}
)}
label={i18n.translate('xpack.transform.transformList.editFlyoutFormdocsPerSecondLabel', {
defaultMessage: 'Documents per second',
})}
onChange={onChangeDestinationIndexHandler}
/>*/}
onChange={value => dispatch({ field: 'docsPerSecond', value })}
value={formFields.docsPerSecond.value}
/>
<EditTransformFlyoutFormTextInput
errorMessages={formFields.frequency.errorMessages}
helpText={i18n.translate('xpack.transform.transformList.editFlyoutFormFrequencyHelptext', {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ import {
formReducerFactory,
frequencyValidator,
getDefaultState,
numberAboveZeroValidator,
FormField,
} from './use_edit_transform_flyout';

const getTransformConfigMock = (): TransformPivotConfig => ({
Expand Down Expand Up @@ -43,14 +45,21 @@ const getTransformConfigMock = (): TransformPivotConfig => ({
description: 'the-description',
});

const getDescriptionFieldMock = (value = '') => ({
const getDescriptionFieldMock = (value = ''): FormField => ({
isOptional: true,
value,
errorMessages: [],
validator: 'string',
});

const getFrequencyFieldMock = (value = '') => ({
const getDocsPerSecondFieldMock = (value = ''): FormField => ({
isOptional: true,
value,
errorMessages: [],
validator: 'numberAboveZero',
});

const getFrequencyFieldMock = (value = ''): FormField => ({
isOptional: true,
value,
errorMessages: [],
Expand All @@ -63,13 +72,16 @@ describe('Transform: applyFormFieldsToTransformConfig()', () => {

const updateConfig = applyFormFieldsToTransformConfig(transformConfigMock, {
description: getDescriptionFieldMock(transformConfigMock.description),
docsPerSecond: getDocsPerSecondFieldMock(),
frequency: getFrequencyFieldMock(),
});

// This case will return an empty object. In the actual UI, this case should not happen
// because the Update-Button will be disabled when no form field was changed.
expect(Object.keys(updateConfig)).toHaveLength(0);
expect(updateConfig.description).toBe(undefined);
// `docs_per_second` is nested under `settings` so we're just checking against that.
expect(updateConfig.settings).toBe(undefined);
expect(updateConfig.frequency).toBe(undefined);
});

Expand All @@ -80,23 +92,28 @@ describe('Transform: applyFormFieldsToTransformConfig()', () => {

const updateConfig = applyFormFieldsToTransformConfig(transformConfigMock, {
description: getDescriptionFieldMock('the-new-description'),
frequency: getFrequencyFieldMock(undefined),
docsPerSecond: getDocsPerSecondFieldMock('10'),
frequency: getFrequencyFieldMock('1m'),
});

expect(Object.keys(updateConfig)).toHaveLength(1);
expect(Object.keys(updateConfig)).toHaveLength(3);
expect(updateConfig.description).toBe('the-new-description');
expect(updateConfig.frequency).toBe(undefined);
expect(updateConfig.settings?.docs_per_second).toBe(10);
expect(updateConfig.frequency).toBe('1m');
});

test('should only include changed form fields', () => {
const transformConfigMock = getTransformConfigMock();
const updateConfig = applyFormFieldsToTransformConfig(transformConfigMock, {
description: getDescriptionFieldMock('the-updated-description'),
docsPerSecond: getDocsPerSecondFieldMock(),
frequency: getFrequencyFieldMock(),
});

expect(Object.keys(updateConfig)).toHaveLength(1);
expect(updateConfig.description).toBe('the-updated-description');
// `docs_per_second` is nested under `settings` so we're just checking against that.
expect(updateConfig.settings).toBe(undefined);
expect(updateConfig.frequency).toBe(undefined);
});
});
Expand Down Expand Up @@ -166,3 +183,21 @@ describe('Transform: frequencyValidator()', () => {
expect(frequencyValidator('59m')).toHaveLength(0);
});
});

describe('Transform: numberValidator()', () => {
test('it should only allow numbers', () => {
// numberValidator() returns an array of error messages so
// an array with a length of 0 means a successful validation.

// invalid
expect(numberAboveZeroValidator('a-string')).toHaveLength(1);
expect(numberAboveZeroValidator('0s')).toHaveLength(1);
expect(numberAboveZeroValidator('1m')).toHaveLength(1);
expect(numberAboveZeroValidator(-1)).toHaveLength(1);
expect(numberAboveZeroValidator(0)).toHaveLength(1);

// valid
expect(numberAboveZeroValidator(1)).toHaveLength(0);
expect(numberAboveZeroValidator('1')).toHaveLength(0);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -11,29 +11,43 @@ import { i18n } from '@kbn/i18n';

import { TransformPivotConfig } from '../../../../common';

// A Validator function takes in a value to check and returns an array of error messages.
// If no messages (empty array) get returned, the value is valid.
type Validator = (arg: any) => string[];

// Note on the form validation and input components used:
// All inputs use `EuiFieldText` which means all form values will be treated as strings.
// This means we cast other formats like numbers coming from the transform config to strings,
// then revalidate them and cast them again to number before submitting a transform update.
// We do this so we have fine grained control over field validation and the option to
// cast to special values like `null` for disabling `docs_per_second`.
const numberAboveZeroNotValidErrorMessage = i18n.translate(
'xpack.transform.transformList.editFlyoutFormNumberNotValidErrorMessage',
{
defaultMessage: 'Value needs to be a number above zero.',
}
);
export const numberAboveZeroValidator: Validator = arg =>
!isNaN(arg) && parseInt(arg, 10) > 0 ? [] : [numberAboveZeroNotValidErrorMessage];

// The way the current form is set up, this validator is just a sanity check,
// it should never trigger an error, because `EuiFieldText` always returns a string.
const stringNotValidErrorMessage = i18n.translate(
'xpack.transform.transformList.editFlyoutFormStringNotValidErrorMessage',
{
defaultMessage: 'Value needs to be of type string.',
}
);

type Validator = (arg: any) => string[];

// The way the current form is set up,
// this validator is just a sanity check,
// it should never trigger an error.
const stringValidator: Validator = arg =>
typeof arg === 'string' ? [] : [stringNotValidErrorMessage];

// Only allow frequencies in the form of 1s/1h etc.
const frequencyNotValidErrorMessage = i18n.translate(
'xpack.transform.transformList.editFlyoutFormFrequencyNotValidErrorMessage',
{
defaultMessage: 'The frequency value is not valid.',
}
);

// Only allow frequencies in the form of 1s/1h etc.
export const frequencyValidator: Validator = arg => {
if (typeof arg !== 'string' || arg === null) {
return [stringNotValidErrorMessage];
Expand All @@ -58,33 +72,37 @@ export const frequencyValidator: Validator = arg => {
);
};

interface Validate {
[key: string]: Validator;
}
type Validators = 'string' | 'frequency' | 'numberAboveZero';

type Validate = {
[key in Validators]: Validator;
};

const validate: Validate = {
string: stringValidator,
frequency: frequencyValidator,
numberAboveZero: numberAboveZeroValidator,
};

interface Field {
export interface FormField {
errorMessages: string[];
isOptional: boolean;
validator: keyof typeof validate;
validator: keyof Validate;
value: string;
}

const defaultField: Field = {
const defaultField: FormField = {
errorMessages: [],
isOptional: true,
validator: 'string',
value: '',
};

interface EditTransformFlyoutFieldsState {
[key: string]: Field;
description: Field;
frequency: Field;
[key: string]: FormField;
description: FormField;
frequency: FormField;
docsPerSecond: FormField;
}

export interface EditTransformFlyoutState {
Expand All @@ -101,18 +119,22 @@ interface Action {
}

// Some attributes can have a value of `null` to trigger
// a reset to the default value.
// a reset to the default value, or in the case of `docs_per_second`
// `null` is used to disable throttling.
interface UpdateTransformPivotConfig {
description: string;
frequency: string;
settings: {
docs_per_second: number | null;
};
}

// Takes in the form configuration and returns a
// request object suitable to be sent to the
// transform update API endpoint.
export const applyFormFieldsToTransformConfig = (
config: TransformPivotConfig,
{ description, frequency }: EditTransformFlyoutFieldsState
{ description, docsPerSecond, frequency }: EditTransformFlyoutFieldsState
): Partial<UpdateTransformPivotConfig> => {
const updateConfig: Partial<UpdateTransformPivotConfig> = {};

Expand All @@ -132,6 +154,16 @@ export const applyFormFieldsToTransformConfig = (
updateConfig.description = description.value;
}

// if the input field was left empty,
// fall back to the default value of `null`
// which will disable throttling.
const docsPerSecondFormValue =
docsPerSecond.value !== '' ? parseInt(docsPerSecond.value, 10) : null;
const docsPerSecondConfigValue = config.settings?.docs_per_second ?? null;
if (docsPerSecondFormValue !== docsPerSecondConfigValue) {
updateConfig.settings = { docs_per_second: docsPerSecondFormValue };
}

return updateConfig;
};

Expand All @@ -141,6 +173,11 @@ export const getDefaultState = (config: TransformPivotConfig): EditTransformFlyo
formFields: {
description: { ...defaultField, value: config?.description ?? '' },
frequency: { ...defaultField, value: config?.frequency ?? '', validator: 'frequency' },
docsPerSecond: {
...defaultField,
value: config?.settings?.docs_per_second?.toString() ?? '',
validator: 'numberAboveZero',
},
},
isFormTouched: false,
isFormValid: true,
Expand All @@ -154,10 +191,13 @@ const isFormValid = (fieldsState: EditTransformFlyoutFieldsState) =>
// Updates a form field with its new value,
// runs validation and populates
// `errorMessages` if any errors occur.
const formFieldReducer = (state: Field, value: string): Field => {
const formFieldReducer = (state: FormField, value: string): FormField => {
return {
...state,
errorMessages: state.isOptional && value.length === 0 ? [] : validate[state.validator](value),
errorMessages:
Copy link
Contributor

Choose a reason for hiding this comment

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

If the update fails, a toast notification is correctly displayed. But when I dismiss the toast, the flyout closes, not allowing the user to correct any mistakes. The flyout should remain open after the toast error is dismissed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good find, will be solved in a follow-up.

state.isOptional && typeof value === 'string' && value.length === 0
? []
: validate[state.validator](value),
value,
};
};
Expand Down