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] DF Analytics creation wizard: ensure user can switch back to form from JSON editor #73752

Conversation

alvarezmelissa87
Copy link
Contributor

@alvarezmelissa87 alvarezmelissa87 commented Jul 29, 2020

Summary

Related meta issue: #66661

This PR allows the user to switch back and forth from the JSON editor and form.

  • Similar to cloning, don't allow switch back to form if it's an advanced job config

image

  • ensure destination index is shown in the form when coming back from the JSON editor

Running flaky test runner to ensure functional tests not affected: https://kibana-ci.elastic.co/job/kibana+flaky-test-suite-runner/711/

Checklist

Delete any items that are not applicable to this PR.

@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@peteharverson
Copy link
Contributor

peteharverson commented Jul 30, 2020

As discussed, is it possible to disable the switch back to the form editor if you add an unsupported field into the includes section?

image

An error is triggered once you are back in the form editor, and if you then switch again to the JSON editor the switch becomes disabled.


@peteharverson - thanks for taking a look! I think I'll address that in a follow up as it might require a bit of refactor so we don't do a full validation every single time the config string changes, as that would be a bit too much.

Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

Left a comment about unsupported includes fields, but otherwise LGTM.

@alvarezmelissa87 alvarezmelissa87 force-pushed the ml-dfanalytics-switch-back-to-form branch from 77762e2 to 369ad8e Compare July 30, 2020 17:20
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

page load bundle size

id value diff baseline
ml 4.3MB +4.2KB 4.3MB

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Copy link
Contributor

@walterra walterra left a comment

Choose a reason for hiding this comment

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

LGTM, just added a question about ts-ignores.

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.

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.

@alvarezmelissa87 alvarezmelissa87 merged commit 9c5bbe4 into elastic:master Jul 31, 2020
alvarezmelissa87 added a commit to alvarezmelissa87/kibana that referenced this pull request Jul 31, 2020
…m from JSON editor (elastic#73752)

* wip: add reducer action to switch to form

* rename getFormStateFromJobConfig

* wip: types fix

* show destIndex input when switching back from editor

* ensure validation up to date when switching to form

* cannot switch back to form if advanced config

* update types

* localization fix
@alvarezmelissa87 alvarezmelissa87 deleted the ml-dfanalytics-switch-back-to-form branch July 31, 2020 14:48
alvarezmelissa87 added a commit that referenced this pull request Jul 31, 2020
…m from JSON editor (#73752) (#73950)

* wip: add reducer action to switch to form

* rename getFormStateFromJobConfig

* wip: types fix

* show destIndex input when switching back from editor

* ensure validation up to date when switching to form

* cannot switch back to form if advanced config

* update types

* localization fix
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants