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

[ML] DF Analytics creation wizard: ensure user can switch back to form from JSON editor #73752

Merged
merged 8 commits into from
Jul 31, 2020
Merged

[ML] DF Analytics creation wizard: ensure user can switch back to form from JSON editor #73752

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
44390dd
wip: add reducer action to switch to form
alvarezmelissa87 Jul 22, 2020
b84b287
rename getFormStateFromJobConfig
alvarezmelissa87 Jul 22, 2020
8c1ad24
wip: types fix
alvarezmelissa87 Jul 23, 2020
9900223
show destIndex input when switching back from editor
alvarezmelissa87 Jul 28, 2020
9f14ab8
ensure validation up to date when switching to form
alvarezmelissa87 Jul 29, 2020
23b3c2e
cannot switch back to form if advanced config
alvarezmelissa87 Jul 29, 2020
369ad8e
update types
alvarezmelissa87 Jul 29, 2020
715d0d4
localization fix
alvarezmelissa87 Jul 30, 2020
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
14 changes: 10 additions & 4 deletions ...ta_frame_analytics/pages/analytics_creation/components/details_step/details_step_form.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ export const DetailsStepForm: FC<CreateAnalyticsStepProps> = ({
const { ELASTIC_WEBSITE_URL, DOC_LINK_VERSION } = docLinks;

const { setFormState } = actions;
const { form, cloneJob, isJobCreated } = state;
const { form, cloneJob, hasSwitchedToEditor, isJobCreated } = state;
const {
createIndexPattern,
description,
Expand All @@ -61,7 +61,9 @@ export const DetailsStepForm: FC<CreateAnalyticsStepProps> = ({
resultsField,
} = form;

const [destIndexSameAsId, setDestIndexSameAsId] = useState<boolean>(cloneJob === undefined);
const [destIndexSameAsId, setDestIndexSameAsId] = useState<boolean>(
cloneJob === undefined && hasSwitchedToEditor === false
);

const forceInput = useRef<HTMLInputElement | null>(null);

Expand Down Expand Up @@ -90,7 +92,11 @@ export const DetailsStepForm: FC<CreateAnalyticsStepProps> = ({
useEffect(() => {
if (destinationIndexNameValid === true) {
debouncedIndexCheck();
} else if (destinationIndex.trim() === '' && destinationIndexNameExists === true) {
} else if (
typeof destinationIndex === 'string' &&
destinationIndex.trim() === '' &&
destinationIndexNameExists === true
) {
setFormState({ destinationIndexNameExists: false });
}

Expand All @@ -102,7 +108,7 @@ export const DetailsStepForm: FC<CreateAnalyticsStepProps> = ({
useEffect(() => {
if (destIndexSameAsId === true && !jobIdEmpty && jobIdValid) {
setFormState({ destinationIndex: jobId });
} else if (destIndexSameAsId === false) {
} else if (destIndexSameAsId === false && hasSwitchedToEditor === false) {
setFormState({ destinationIndex: '' });
}
}, [destIndexSameAsId, jobId]);
Expand Down
71 changes: 41 additions & 30 deletions x-pack/plugins/ml/public/application/data_frame_analytics/pages/analytics_creation/page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@

import React, { FC, useEffect, useState } from 'react';
import {
EuiButtonEmpty,
EuiFlexGroup,
EuiFlexItem,
EuiFormRow,
Expand All @@ -16,7 +15,7 @@ import {
EuiSpacer,
EuiSteps,
EuiStepStatus,
EuiText,
EuiSwitch,
EuiTitle,
} from '@elastic/eui';
import { i18n } from '@kbn/i18n';
Expand Down Expand Up @@ -48,9 +47,15 @@ export const Page: FC<Props> = ({ jobId }) => {
const { currentIndexPattern } = mlContext;

const createAnalyticsForm = useCreateAnalyticsForm();
const { isAdvancedEditorEnabled } = createAnalyticsForm.state;
const { jobType } = createAnalyticsForm.state.form;
const { initiateWizard, setJobClone, switchToAdvancedEditor } = createAnalyticsForm.actions;
const { state } = createAnalyticsForm;
const { isAdvancedEditorEnabled, disableSwitchToForm } = state;
const { jobType } = state.form;
const {
initiateWizard,
setJobClone,
switchToAdvancedEditor,
switchToForm,
} = createAnalyticsForm.actions;

useEffect(() => {
initiateWizard();
Expand Down Expand Up @@ -170,34 +175,40 @@ export const Page: FC<Props> = ({ jobId }) => {
</EuiFlexItem>
</EuiFlexGroup>
</EuiFlexItem>
{isAdvancedEditorEnabled === false && (
<EuiFlexItem grow={false}>
<EuiFormRow
helpText={i18n.translate(
'xpack.ml.dataframe.analytics.create.enableJsonEditorHelpText',

<EuiFlexItem grow={false}>
<EuiFormRow
helpText={
disableSwitchToForm &&
i18n.translate(
'xpack.ml.dataframe.analytics.create.jsonEditorDisabledSwitchText',
{
defaultMessage:
'Configuration contains advanced fields not supported by the form. You cannot switch back to the form.',
}
)
}
>
<EuiSwitch
disabled={jobType === undefined || disableSwitchToForm}
label={i18n.translate(
'xpack.ml.dataframe.analytics.create.switchToJsonEditorSwitch',
{
defaultMessage: 'You cannot switch back to this form from the json editor.',
defaultMessage: 'Switch to json editor',
}
)}
>
<EuiButtonEmpty
isDisabled={jobType === undefined}
iconType="link"
onClick={switchToAdvancedEditor}
data-test-subj="mlAnalyticsCreateJobWizardAdvancedEditorSwitch"
>
<EuiText size="s" grow={false}>
{i18n.translate(
'xpack.ml.dataframe.analytics.create.switchToJsonEditorSwitch',
{
defaultMessage: 'Switch to json editor',
}
)}
</EuiText>
</EuiButtonEmpty>
</EuiFormRow>
</EuiFlexItem>
)}
checked={isAdvancedEditorEnabled}
onChange={(e) => {
if (e.target.checked === true) {
switchToAdvancedEditor();
} else {
switchToForm();
}
}}
data-test-subj="mlAnalyticsCreateJobWizardAdvancedEditorSwitch"
/>
</EuiFormRow>
</EuiFlexItem>
</EuiFlexGroup>
<EuiSpacer />
{isAdvancedEditorEnabled === true && (
Expand Down
5 changes: 4 additions & 1 deletion ...ata_frame_analytics/pages/analytics_management/hooks/use_create_analytics_form/actions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ export enum ACTION {
SET_JOB_CONFIG,
SET_JOB_IDS,
SWITCH_TO_ADVANCED_EDITOR,
SWITCH_TO_FORM,
SET_ESTIMATED_MODEL_MEMORY_LIMIT,
SET_JOB_CLONE,
}
Expand All @@ -38,7 +39,8 @@ export type Action =
| ACTION.OPEN_MODAL
| ACTION.RESET_ADVANCED_EDITOR_MESSAGES
| ACTION.RESET_FORM
| ACTION.SWITCH_TO_ADVANCED_EDITOR;
| ACTION.SWITCH_TO_ADVANCED_EDITOR
| ACTION.SWITCH_TO_FORM;
}
// Actions with custom payloads:
| { type: ACTION.ADD_REQUEST_MESSAGE; requestMessage: FormMessage }
Expand Down Expand Up @@ -71,6 +73,7 @@ export interface ActionDispatchers {
setJobConfig: (payload: State['jobConfig']) => void;
startAnalyticsJob: () => void;
switchToAdvancedEditor: () => void;
switchToForm: () => void;
setEstimatedModelMemoryLimit: (value: State['estimatedModelMemoryLimit']) => void;
setJobClone: (cloneJob: DeepReadonly<DataFrameAnalyticsConfig>) => Promise<void>;
}
57 changes: 51 additions & 6 deletions ...ata_frame_analytics/pages/analytics_management/hooks/use_create_analytics_form/reducer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,17 @@ import { i18n } from '@kbn/i18n';
import { memoize } from 'lodash';
// @ts-ignore
import numeral from '@elastic/numeral';
import { isEmpty } from 'lodash';
import { isValidIndexName } from '../../../../../../../common/util/es_utils';

import { collapseLiteralStrings } from '../../../../../../../../../../src/plugins/es_ui_shared/public';

import { Action, ACTION } from './actions';
import { getInitialState, getJobConfigFromFormState, State } from './state';
import {
getInitialState,
getFormStateFromJobConfig,
getJobConfigFromFormState,
State,
} from './state';
import {
isJobIdValid,
validateModelMemoryLimitUnits,
Expand All @@ -41,6 +45,7 @@ import {
TRAINING_PERCENT_MAX,
} from '../../../../common/analytics';
import { indexPatterns } from '../../../../../../../../../../src/plugins/data/public';
import { isAdvancedConfig } from '../../components/action_clone/clone_button';

const mmlAllowedUnitsStr = `${ALLOWED_DATA_UNITS.slice(0, ALLOWED_DATA_UNITS.length - 1).join(
', '
Expand Down Expand Up @@ -458,13 +463,16 @@ export function reducer(state: State, action: Action): State {

case ACTION.SET_ADVANCED_EDITOR_RAW_STRING:
let resultJobConfig;
let disableSwitchToForm = false;
try {
resultJobConfig = JSON.parse(collapseLiteralStrings(action.advancedEditorRawString));
disableSwitchToForm = isAdvancedConfig(resultJobConfig);
} catch (e) {
return {
...state,
advancedEditorRawString: action.advancedEditorRawString,
isAdvancedEditorValidJson: false,
disableSwitchToForm: true,
advancedEditorMessages: [],
};
}
Expand All @@ -473,6 +481,7 @@ export function reducer(state: State, action: Action): State {
...validateAdvancedEditor({ ...state, jobConfig: resultJobConfig }),
advancedEditorRawString: action.advancedEditorRawString,
isAdvancedEditorValidJson: true,
disableSwitchToForm,
};

case ACTION.SET_FORM_STATE:
Expand Down Expand Up @@ -538,17 +547,53 @@ export function reducer(state: State, action: Action): State {

case ACTION.SWITCH_TO_ADVANCED_EDITOR:
let { jobConfig } = state;
const isJobConfigEmpty = isEmpty(state.jobConfig);
if (isJobConfigEmpty) {
jobConfig = getJobConfigFromFormState(state.form);
}
jobConfig = getJobConfigFromFormState(state.form);
const shouldDisableSwitchToForm = isAdvancedConfig(jobConfig);

return validateAdvancedEditor({
...state,
advancedEditorRawString: JSON.stringify(jobConfig, null, 2),
isAdvancedEditorEnabled: true,
disableSwitchToForm: shouldDisableSwitchToForm,
hasSwitchedToEditor: true,
jobConfig,
});

case ACTION.SWITCH_TO_FORM:
const { jobConfig: config, jobIds } = state;
const { jobId } = state.form;
// @ts-ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain and add a comment why the ignore is necessary? Do you plan to solve it in a follow up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The ignore is necessary because of the differences in CloneDataFrameAnalyticsConfig vs DataFrameAnalyticsConfig - particularly the allowance of some of those values being undefined when just switching back from the JSON editor (as when it's a cloned job, all the values must be defined). I plan to solve it in a follow-up.

const formState = getFormStateFromJobConfig(config, false);

if (typeof jobId === 'string' && jobId.trim() !== '') {
formState.jobId = jobId;
}

formState.jobIdExists = jobIds.some((id) => formState.jobId === id);
formState.jobIdEmpty = jobId === '';
formState.jobIdValid = isJobIdValid(jobId);
formState.jobIdInvalidMaxLength = !!maxLengthValidator(JOB_ID_MAX_LENGTH)(jobId);

formState.destinationIndexNameEmpty = formState.destinationIndex === '';
formState.destinationIndexNameValid = isValidIndexName(formState.destinationIndex || '');
formState.destinationIndexPatternTitleExists =
state.indexPatternsMap[formState.destinationIndex || ''] !== undefined;

if (formState.numTopFeatureImportanceValues !== undefined) {
formState.numTopFeatureImportanceValuesValid = validateNumTopFeatureImportanceValues(
formState.numTopFeatureImportanceValues
);
}

return validateForm({
...state,
// @ts-ignore
form: formState,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain and add a comment why the ignore is necessary? Do you plan to solve it in a follow up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above.

isAdvancedEditorEnabled: false,
advancedEditorRawString: JSON.stringify(config, null, 2),
jobConfig: config,
});

case ACTION.SET_ESTIMATED_MODEL_MEMORY_LIMIT:
return {
...state,
Expand Down
14 changes: 5 additions & 9 deletions ..._frame_analytics/pages/analytics_management/hooks/use_create_analytics_form/state.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,7 @@
* you may not use this file except in compliance with the Elastic License.
*/

import {
getCloneFormStateFromJobConfig,
getInitialState,
getJobConfigFromFormState,
} from './state';
import { getFormStateFromJobConfig, getInitialState, getJobConfigFromFormState } from './state';

const regJobConfig = {
id: 'reg-test-01',
Expand Down Expand Up @@ -96,8 +92,8 @@ describe('useCreateAnalyticsForm', () => {
]);
});

test('state: getCloneFormStateFromJobConfig() regression', () => {
const clonedState = getCloneFormStateFromJobConfig(regJobConfig);
test('state: getFormStateFromJobConfig() regression', () => {
const clonedState = getFormStateFromJobConfig(regJobConfig);

expect(clonedState?.sourceIndex).toBe('reg-test-index');
expect(clonedState?.includes).toStrictEqual([]);
Expand All @@ -112,8 +108,8 @@ describe('useCreateAnalyticsForm', () => {
expect(clonedState?.jobId).toBe(undefined);
});

test('state: getCloneFormStateFromJobConfig() outlier detection', () => {
const clonedState = getCloneFormStateFromJobConfig(outlierJobConfig);
test('state: getFormStateFromJobConfig() outlier detection', () => {
const clonedState = getFormStateFromJobConfig(outlierJobConfig);

expect(clonedState?.sourceIndex).toBe('outlier-test-index');
expect(clonedState?.includes).toStrictEqual(['field', 'other_field']);
Expand Down
16 changes: 13 additions & 3 deletions .../data_frame_analytics/pages/analytics_management/hooks/use_create_analytics_form/state.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
DataFrameAnalyticsId,
DataFrameAnalyticsConfig,
ANALYSIS_CONFIG_TYPE,
defaultSearchQuery,
} from '../../../../common/analytics';
import { CloneDataFrameAnalyticsConfig } from '../../components/action_clone';

Expand Down Expand Up @@ -44,6 +45,7 @@ export interface FormMessage {
export interface State {
advancedEditorMessages: FormMessage[];
advancedEditorRawString: string;
disableSwitchToForm: boolean;
form: {
computeFeatureInfluence: string;
createIndexPattern: boolean;
Expand Down Expand Up @@ -97,6 +99,7 @@ export interface State {
indexPatternsMap: SourceIndexMap;
isAdvancedEditorEnabled: boolean;
isAdvancedEditorValidJson: boolean;
hasSwitchedToEditor: boolean;
isJobCreated: boolean;
isJobStarted: boolean;
isValid: boolean;
Expand All @@ -110,6 +113,7 @@ export interface State {
export const getInitialState = (): State => ({
advancedEditorMessages: [],
advancedEditorRawString: '',
disableSwitchToForm: false,
form: {
computeFeatureInfluence: 'true',
createIndexPattern: true,
Expand All @@ -131,7 +135,7 @@ export const getInitialState = (): State => ({
jobIdInvalidMaxLength: false,
jobIdValid: false,
jobType: undefined,
jobConfigQuery: { match_all: {} },
jobConfigQuery: defaultSearchQuery,
jobConfigQueryString: undefined,
lambda: undefined,
loadingFieldOptions: false,
Expand Down Expand Up @@ -167,6 +171,7 @@ export const getInitialState = (): State => ({
indexPatternsMap: {},
isAdvancedEditorEnabled: false,
isAdvancedEditorValidJson: true,
hasSwitchedToEditor: false,
isJobCreated: false,
isJobStarted: false,
isValid: false,
Expand Down Expand Up @@ -283,8 +288,9 @@ function toCamelCase(property: string): string {
* Extracts form state for a job clone from the analytics job configuration.
* For cloning we keep job id and destination index empty.
*/
export function getCloneFormStateFromJobConfig(
analyticsJobConfig: Readonly<CloneDataFrameAnalyticsConfig>
export function getFormStateFromJobConfig(
analyticsJobConfig: Readonly<CloneDataFrameAnalyticsConfig>,
isClone: boolean = true
): Partial<State['form']> {
const jobType = Object.keys(analyticsJobConfig.analysis)[0] as ANALYSIS_CONFIG_TYPE;

Expand All @@ -300,6 +306,10 @@ export function getCloneFormStateFromJobConfig(
includes: analyticsJobConfig.analyzed_fields.includes,
};

if (isClone === false) {
resultState.destinationIndex = analyticsJobConfig?.dest.index ?? '';
}

const analysisConfig = analyticsJobConfig.analysis[jobType];

for (const key in analysisConfig) {
Expand Down
Loading