[ML] Enhance job id error message by darnautov · Pull Request #45349 · elastic/kibana

darnautov · 2019-09-11T10:17:05Z

Summary

Extend job id validation messages specifying the max length restriction.

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~[ ] This was checked for cross-browser compatibility, including a check against IE11~~
Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
~~[ ] Documentation was added for features that require explanation or tutorials~~
Unit or functional tests were updated or added to match the most common scenarios
~~This was checked for keyboard-only and screenreader accessibility~~

For maintainers

~~[ ] This was checked for breaking API changes and was labeled appropriately~~
~~[ ] This includes a feature addition or change that requires a release note and was labeled appropriately~~

elasticmachine · 2019-09-11T10:17:08Z

Pinging @elastic/ml-ui

elasticmachine · 2019-09-11T11:52:19Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

…or-message

elasticmachine · 2019-09-13T18:18:57Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

…or-message

walterra

Code overall looks good, I just added some questions. One more question I have: The naming JOB_ID_MAX_LENGTH = 64 somehow implies to me that 64 would be a valid max length, but then the checks are done using lesser than and also the error messages say less than. I couldn't find a reference how this is handled in the backend so can you double check if less than 64 is the correct validation?

walterra · 2019-09-16T09:42:13Z

x-pack/legacy/plugins/ml/common/util/validators.ts

+          actualLength: value.length,
+        },
+      };
+    } else {


Absolute nitpick: You could just skip the else and return without it. For reference, here's the section on "return early from function" https://github.com/elastic/kibana/blob/master/style_guides/js_style_guide.md#returnthrow-early-from-functions

walterra · 2019-09-16T09:43:32Z

x-pack/legacy/plugins/ml/server/models/job_validation/messages.js

      text: i18n.translate('xpack.ml.models.jobValidation.messages.jobIdValidMessage', {
        defaultMessage: 'Lowercase alphanumeric (a-z and 0-9) characters, hyphens or underscores, ' +
-          'starts and ends with an alphanumeric character.',
+          'starts and ends with an alphanumeric character, and is less than 64 characters.',


Could this also make use of JOB_ID_MAX_LENGTH?

walterra · 2019-09-16T09:43:42Z

x-pack/legacy/plugins/ml/server/models/job_validation/messages.js

      text: i18n.translate('xpack.ml.models.jobValidation.messages.jobGroupIdValidMessage', {
        defaultMessage: 'Lowercase alphanumeric (a-z and 0-9) characters, hyphens or underscores, ' +
-          'starts and ends with an alphanumeric character.',
+          'starts and ends with an alphanumeric character, and is less than 64 characters.',


Could this also make use of JOB_ID_MAX_LENGTH?

elasticmachine · 2019-09-16T10:04:35Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

x-pack/legacy/plugins/ml/server/models/job_validation/messages.js

elasticmachine · 2019-09-16T15:07:06Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

peteharverson

Some suggestions for slight rewording of the validation messages.
Checks works well for anomaly detection job IDs. Can we get the same check in for Analytics jobs too?

x-pack/legacy/plugins/ml/public/jobs/new_job_new/common/job_validator/util.ts

x-pack/legacy/plugins/ml/server/models/job_validation/messages.js

jgowdyelastic · 2019-09-16T15:40:55Z

x-pack/legacy/plugins/ml/common/util/validators.ts

+ * Provides a validator function for maximum allowed input length.
+ * @param maxLength Maximum length allowed.
+ */
+export function maxLengthValidator(


This function is now used as falsy check, the contents of the returned object are ignored.
Maybe it should be changed to just return a boolean.

I just realised that there is a string_utils.js file along side this file. IMO, this function is worth moving inside there, as it is validating the length of a string.

I use it as falsy check because of the current code structure. Validator function provides useful info to render error messages as well as error key which lets generate an object with all errors for specific field/model.

jgowdyelastic

LGTM

elasticmachine · 2019-09-16T17:51:17Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

alvarezmelissa87

.LGTM ⚡️

…essage # Conflicts: # x-pack/plugins/translations/translations/ja-JP.json # x-pack/plugins/translations/translations/zh-CN.json

peteharverson

Tested the new validations in the Anomaly Detection job wizards and the Analytics create job modal and works well. Just one typo needs fixing!

x-pack/legacy/plugins/ml/server/models/job_validation/messages.js

elasticmachine · 2019-09-17T09:33:48Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

peteharverson

Latest edits LGTM!

walterra · 2019-09-17T09:47:50Z

...lic/data_frame_analytics/pages/analytics_management/hooks/use_create_analytics_form/state.ts

    jobId: DataFrameAnalyticsId;
    jobIdExists: boolean;
    jobIdEmpty: boolean;
+    jobIdInvalidMaxLength?: boolean;


Would be good to have this in line with the other attributes which are not optional. To fix it I think you just need to add a default jobIdInvalidMaxLength: false; to getInitialState().

elasticmachine · 2019-09-17T11:19:08Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

[ML] Enhance job id error message

* master: (33 commits) [easy] Exclude __examples__ from coverage (elastic#45556) [DOCS] Update CCR links (elastic#44012) Use unique junit report filenames again (elastic#45897) [ftr/savedObjects] add simple saved object api client to ftr s… (elastic#45856) New visualization editor Lens (elastic#36437) Sort using unix timestamp value (elastic#43162) [APM] Use POST instead of implicit GET (elastic#45903) [Canvas] Converting workpad header components to typescript and adding i18n (elastic#45274) skip flaky test (elastic#45884) set IS_PIPELINE_JOB in intake jobs (elastic#45850) [Uptime] Fix/issue 48 integration popup closes after refresh (elastic#45759) [Logs UI] Support zoom by brushing in the log rate chart (elastic#45879) [DOCS] Changes name to host (elastic#45798) [ML] Add population job wizard test (elastic#45765) [skip-ci][Maps][File upload] Geojson indexing and styling docs (elastic#41394) remove setTimeoue for state change (elastic#45853) [Graph] Restructure folders and add readme (elastic#45782) [ML] Enhance job id error message (elastic#45349) [SIEM] Do not update state component when they did unmount (elastic#45847) [i18n] sync from 7.4 latest translations (elastic#45823) ...

* [ML] Enhance job id error message (#45349) [ML] Enhance job id error message * fix i18n

[ML] Enhance job id error message

6e9cc05

darnautov added release_note:enhancement :ml v8.0.0 v7.5.0 labels Sep 11, 2019

darnautov requested review from jgowdyelastic and peteharverson September 11, 2019 10:17

darnautov requested a review from a team as a code owner September 11, 2019 10:17

[ML] maxLengthValidator, job id validation messages

c479a44

darnautov requested review from alvarezmelissa87 and walterra September 13, 2019 16:37

Merge branch 'master' of github.com:elastic/kibana into ML-job-id-err…

a4f7543

…or-message

Merge branch 'master' of github.com:elastic/kibana into ML-job-id-err…

410b564

…or-message

walterra reviewed Sep 16, 2019

View reviewed changes

peteharverson reviewed Sep 16, 2019

View reviewed changes

x-pack/legacy/plugins/ml/server/models/job_validation/messages.js Outdated Show resolved Hide resolved

x-pack/legacy/plugins/ml/server/models/job_validation/messages.js Outdated Show resolved Hide resolved

[ML] job id and job group id validation messages

dbc81e1

peteharverson added the Feature:Anomaly Detection ML anomaly detection label Sep 16, 2019

[ML] fix i18n

5666aec

peteharverson reviewed Sep 16, 2019

View reviewed changes

[ML] rewording

0e3bc0e

jgowdyelastic reviewed Sep 16, 2019

View reviewed changes

[ML] job id length validation for analytics

025f91b

darnautov requested review from jgowdyelastic, peteharverson and walterra September 16, 2019 16:40

jgowdyelastic approved these changes Sep 16, 2019

View reviewed changes

alvarezmelissa87 approved these changes Sep 17, 2019

View reviewed changes

darnautov added 2 commits September 17, 2019 09:54

[ML] no valid message in case of empty job groups

628b8cb

Merge remote-tracking branch 'upstream/master' into ML-job-id-error-m…

30b745d

…essage # Conflicts: # x-pack/plugins/translations/translations/ja-JP.json # x-pack/plugins/translations/translations/zh-CN.json

peteharverson requested changes Sep 17, 2019

View reviewed changes

x-pack/legacy/plugins/ml/server/models/job_validation/messages.js Outdated Show resolved Hide resolved

[ML] fix typo

2bc9962

darnautov requested a review from peteharverson September 17, 2019 09:42

peteharverson approved these changes Sep 17, 2019

View reviewed changes

walterra reviewed Sep 17, 2019

View reviewed changes

darnautov merged commit 87f81e8 into elastic:master Sep 17, 2019

darnautov deleted the ML-job-id-error-message branch September 17, 2019 11:24

darnautov self-assigned this Sep 17, 2019

darnautov mentioned this pull request Sep 17, 2019

[7.x] [ML] Enhance job id error message (#45349) #45882

Merged

darnautov added a commit to darnautov/kibana that referenced this pull request Sep 17, 2019

[ML] Enhance job id error message (elastic#45349)

6a4c77c

[ML] Enhance job id error message

darnautov added a commit that referenced this pull request Sep 18, 2019

[7.x] [ML] Enhance job id error message (#45349) (#45882)

49fa492

* [ML] Enhance job id error message (#45349) [ML] Enhance job id error message * fix i18n

Conversation

darnautov commented Sep 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

elasticmachine commented Sep 11, 2019

Uh oh!

elasticmachine commented Sep 11, 2019

💚 Build Succeeded

Uh oh!

elasticmachine commented Sep 13, 2019

💚 Build Succeeded

Uh oh!

walterra left a comment

Choose a reason for hiding this comment

Uh oh!

walterra Sep 16, 2019

Choose a reason for hiding this comment

Uh oh!

walterra Sep 16, 2019

Choose a reason for hiding this comment

Uh oh!

walterra Sep 16, 2019

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Sep 16, 2019

💚 Build Succeeded

Uh oh!

Uh oh!

Uh oh!

elasticmachine commented Sep 16, 2019

💔 Build Failed

Uh oh!

peteharverson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jgowdyelastic Sep 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jgowdyelastic Sep 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

darnautov Sep 16, 2019

Choose a reason for hiding this comment

Uh oh!

jgowdyelastic left a comment

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Sep 16, 2019

💔 Build Failed

Uh oh!

alvarezmelissa87 left a comment

Choose a reason for hiding this comment

Uh oh!

peteharverson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

elasticmachine commented Sep 17, 2019

💚 Build Succeeded

Uh oh!

peteharverson left a comment

Choose a reason for hiding this comment

Uh oh!

walterra Sep 17, 2019

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Sep 17, 2019

💚 Build Succeeded

Uh oh!

darnautov commented Sep 11, 2019 •

edited

Loading

jgowdyelastic Sep 16, 2019 •

edited

Loading

jgowdyelastic Sep 16, 2019 •

edited

Loading