Skip to content

[ML] Adding basic job validation#41459

Merged
jgowdyelastic merged 9 commits intoelastic:feature-new-ml-job-wizards-newfrom
jgowdyelastic:adding-basic-validation
Jul 19, 2019
Merged

[ML] Adding basic job validation#41459
jgowdyelastic merged 9 commits intoelastic:feature-new-ml-job-wizards-newfrom
jgowdyelastic:adding-basic-validation

Conversation

@jgowdyelastic
Copy link
Member

@jgowdyelastic jgowdyelastic commented Jul 18, 2019 •
edited
Loading

Adds basic job validation to all job wizards. This reproduces the same form validation present in the current job wizards and uses the same core basic validation checks.
This works by running the validation every time the jobCreator is updated.
Once validation is complete, jobValidatorUpdated is triggered and so any components listening to that variable can pull what they need from the results and display the errors if they exist.

Checks covered:

jobId
Does it contain bad characters. The ID isn't already being used by a job or group.

groupIds
Do any groups contain bad characters. Newly added groups aren't already being used by a job or group

modelMemoryLimit
Is it a valid format and is it below the max model memory limit.

bucketSpan
Is it a valid format.

duplicateDetectors
Are any two detectors the same. Only needed for population jobs.

Errors are displayed inline:
image

image

image

@elasticmachine
Copy link
Contributor

elasticmachine commented Jul 18, 2019

💔 Build Failed

@elasticmachine
Copy link
Contributor

elasticmachine commented Jul 18, 2019

💚 Build Succeeded

@jgowdyelastic jgowdyelastic marked this pull request as ready for review July 19, 2019 10:05
@jgowdyelastic jgowdyelastic requested a review from a team as a code owner July 19, 2019 10:05
@jgowdyelastic jgowdyelastic self-assigned this Jul 19, 2019
@jgowdyelastic jgowdyelastic added :ml non-issue Indicates to automation that a pull request should not appear in the release notes release_note:skip Skip the PR/issue when compiling release notes review v7.4.0 v8.0.0 labels Jul 19, 2019
@elasticmachine
Copy link
Contributor

elasticmachine commented Jul 19, 2019

Pinging @elastic/ml-ui

peteharverson
peteharverson reviewed Jul 19, 2019
View reviewed changes
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.

Overall LGTM. Just some minor comments.

I notice you aren't disabling the Next button if one of the validation checks fail. If you stick with that behaviour, I think you should probably add some indication on the final step as to why the Create button is disabled as all the validation messages are then hidden:

image

...s/job_details_step/components/advanced_section/components/model_memory_limit/description.tsx Outdated
Copy link
Contributor

@peteharverson peteharverson Jul 19, 2019

Choose a reason for hiding this comment

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

Don't forget to give this a meaningful ID when you replace the placeholder texts!

Copy link
Member Author

@jgowdyelastic jgowdyelastic Jul 19, 2019

Choose a reason for hiding this comment

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

https://github.com/elastic/kibana/projects/30#card-24169890

.../public/jobs/new_job_new/pages/components/job_details_step/components/groups/description.tsx Outdated
Copy link
Contributor

@peteharverson peteharverson Jul 19, 2019

Choose a reason for hiding this comment

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

As above, this will need a better ID when the placeholder texts are updated.

Copy link
Member Author

@jgowdyelastic jgowdyelastic Jul 19, 2019

Choose a reason for hiding this comment

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

https://github.com/elastic/kibana/projects/30#card-24169890

@elasticmachine
Copy link
Contributor

elasticmachine commented Jul 19, 2019

💔 Build Failed

peteharverson
peteharverson approved these changes Jul 19, 2019
View reviewed changes
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.

Latest changes LGTM

@elasticmachine
Copy link
Contributor

elasticmachine commented Jul 19, 2019

💔 Build Failed

@jgowdyelastic jgowdyelastic force-pushed the adding-basic-validation branch from 285f130 to e1db016 Compare July 19, 2019 13:41
@elasticmachine
Copy link
Contributor

elasticmachine commented Jul 19, 2019

💚 Build Succeeded

alvarezmelissa87
alvarezmelissa87 approved these changes Jul 19, 2019
View reviewed changes
Copy link
Contributor

@alvarezmelissa87 alvarezmelissa87 left a comment

Choose a reason for hiding this comment

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

LGTM ⚡️

@jgowdyelastic jgowdyelastic merged commit 8538437 into elastic:feature-new-ml-job-wizards-new Jul 19, 2019
jgowdyelastic added a commit that referenced this pull request Jul 31, 2019
@jgowdyelastic
[ML] Merging job wizards feature branch to master (#42339)
653fc09 
* [ML] Adding new job wizards services (#40718)

* [ML] Adding new job wizards services

* adding ts-ignore for incomplte kibana timefilter type

* [ML] New job wizards UI (#40748)

* [ML] New job wizards UI

* small refactor

* fixing type problems

* updating snapshots

* updating translations ids for temporary new job links

* updating translation ids

* fixing population by field selection

* small refactor to by field select

* moving component descriptions to separate files

* moving chart grid component into own file

* fixing event rate count detector

* fixing decimal place rounding

* changes based on review

* more changes based on review

* fixing test

* [ML] Adding created_by to job (#41015)

* [ML] Adding json preview (#41078)

* [ML] Adding job json preview

* fix typo

* [ML] Fixing view results button (#41077)

* [ML] Adding generic custom settings functions to job creator (#41046)

* [ML] Adding generic custom settings functions to job creator

* fixing typo

* [ML] Adding advanced section to job details step (#41157)

* [ML] Adding advanced section to job details step

* disabling model plot for population

* changes based on review

* updating include paths

* [ML] Adding additional settings section to job details step (#41339)

* [ML] Adding additional settings section to job details step

* changes based on review

* [ML] Adding basic job validation (#41459)

* [ML] Adding basic job validation

* removing observable

* small refactor

* comments and refactors

* disabling create job button

* adding duplicate job and group id checks

* disabling next on invalid wizard step

* changes based on review

* removing unused includes

* [ML] Adding advanced job validation (#41634)

* [ML] Adding advanced job validation

* clean up

* function rename

* updating tests

* show some previous step info

* removing . from validation titles

* fixes advanced validation duration check

* removing include

* [ML] Changing wizard layout (#41802)

* [ML] Changing wizard layout

* adding a comment

* fixing wizard step link issue

* removing greyed out controls in summary

* filtering out basic validation results

* adding wizards steps back into summary

* moving idFilterList

* removing unused includes

* [ML] Adding date picker to time range step (#41978)

* [ML] Adding date picker to time range step

* adding comments

* sorting influencers

* removing euiSize for width

* [ML] Displaying job creation errors (#42165)

* [ML] Displaying job creation errors

* removing unnecessary try/catch

* [ML] Updating index pattern type (#42269)

* [ML] Updating index pattern type

* changes based on review
jgowdyelastic added a commit to jgowdyelastic/kibana that referenced this pull request Aug 5, 2019
@jgowdyelastic
[ML] Merging job wizards feature branch to master (elastic#42339)
53c530c 
* [ML] Adding new job wizards services (elastic#40718)

* [ML] Adding new job wizards services

* adding ts-ignore for incomplte kibana timefilter type

* [ML] New job wizards UI (elastic#40748)

* [ML] New job wizards UI

* small refactor

* fixing type problems

* updating snapshots

* updating translations ids for temporary new job links

* updating translation ids

* fixing population by field selection

* small refactor to by field select

* moving component descriptions to separate files

* moving chart grid component into own file

* fixing event rate count detector

* fixing decimal place rounding

* changes based on review

* more changes based on review

* fixing test

* [ML] Adding created_by to job (elastic#41015)

* [ML] Adding json preview (elastic#41078)

* [ML] Adding job json preview

* fix typo

* [ML] Fixing view results button (elastic#41077)

* [ML] Adding generic custom settings functions to job creator (elastic#41046)

* [ML] Adding generic custom settings functions to job creator

* fixing typo

* [ML] Adding advanced section to job details step (elastic#41157)

* [ML] Adding advanced section to job details step

* disabling model plot for population

* changes based on review

* updating include paths

* [ML] Adding additional settings section to job details step (elastic#41339)

* [ML] Adding additional settings section to job details step

* changes based on review

* [ML] Adding basic job validation (elastic#41459)

* [ML] Adding basic job validation

* removing observable

* small refactor

* comments and refactors

* disabling create job button

* adding duplicate job and group id checks

* disabling next on invalid wizard step

* changes based on review

* removing unused includes

* [ML] Adding advanced job validation (elastic#41634)

* [ML] Adding advanced job validation

* clean up

* function rename

* updating tests

* show some previous step info

* removing . from validation titles

* fixes advanced validation duration check

* removing include

* [ML] Changing wizard layout (elastic#41802)

* [ML] Changing wizard layout

* adding a comment

* fixing wizard step link issue

* removing greyed out controls in summary

* filtering out basic validation results

* adding wizards steps back into summary

* moving idFilterList

* removing unused includes

* [ML] Adding date picker to time range step (elastic#41978)

* [ML] Adding date picker to time range step

* adding comments

* sorting influencers

* removing euiSize for width

* [ML] Displaying job creation errors (elastic#42165)

* [ML] Displaying job creation errors

* removing unnecessary try/catch

* [ML] Updating index pattern type (elastic#42269)

* [ML] Updating index pattern type

* changes based on review
jgowdyelastic added a commit that referenced this pull request Aug 5, 2019
@jgowdyelastic
[ML] Merging job wizards feature branch to master (#42339) (#42586)
139afe3 
* [ML] Adding new job wizards services (#40718)

* [ML] Adding new job wizards services

* adding ts-ignore for incomplte kibana timefilter type

* [ML] New job wizards UI (#40748)

* [ML] New job wizards UI

* small refactor

* fixing type problems

* updating snapshots

* updating translations ids for temporary new job links

* updating translation ids

* fixing population by field selection

* small refactor to by field select

* moving component descriptions to separate files

* moving chart grid component into own file

* fixing event rate count detector

* fixing decimal place rounding

* changes based on review

* more changes based on review

* fixing test

* [ML] Adding created_by to job (#41015)

* [ML] Adding json preview (#41078)

* [ML] Adding job json preview

* fix typo

* [ML] Fixing view results button (#41077)

* [ML] Adding generic custom settings functions to job creator (#41046)

* [ML] Adding generic custom settings functions to job creator

* fixing typo

* [ML] Adding advanced section to job details step (#41157)

* [ML] Adding advanced section to job details step

* disabling model plot for population

* changes based on review

* updating include paths

* [ML] Adding additional settings section to job details step (#41339)

* [ML] Adding additional settings section to job details step

* changes based on review

* [ML] Adding basic job validation (#41459)

* [ML] Adding basic job validation

* removing observable

* small refactor

* comments and refactors

* disabling create job button

* adding duplicate job and group id checks

* disabling next on invalid wizard step

* changes based on review

* removing unused includes

* [ML] Adding advanced job validation (#41634)

* [ML] Adding advanced job validation

* clean up

* function rename

* updating tests

* show some previous step info

* removing . from validation titles

* fixes advanced validation duration check

* removing include

* [ML] Changing wizard layout (#41802)

* [ML] Changing wizard layout

* adding a comment

* fixing wizard step link issue

* removing greyed out controls in summary

* filtering out basic validation results

* adding wizards steps back into summary

* moving idFilterList

* removing unused includes

* [ML] Adding date picker to time range step (#41978)

* [ML] Adding date picker to time range step

* adding comments

* sorting influencers

* removing euiSize for width

* [ML] Displaying job creation errors (#42165)

* [ML] Displaying job creation errors

* removing unnecessary try/catch

* [ML] Updating index pattern type (#42269)

* [ML] Updating index pattern type

* changes based on review
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alvarezmelissa87 alvarezmelissa87 alvarezmelissa87 approved these changes

@peteharverson peteharverson peteharverson approved these changes

@walterra walterra Awaiting requested review from walterra

Assignees

@jgowdyelastic jgowdyelastic

Labels

:ml non-issue Indicates to automation that a pull request should not appear in the release notes release_note:skip Skip the PR/issue when compiling release notes review v7.4.0 v8.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jgowdyelastic @elasticmachine @alvarezmelissa87 @peteharverson