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] Fix saving of custom URLs for job created in Advanced page #21207

Merged

Conversation

peteharverson
Copy link
Contributor

Fixes an issue where custom URLs added to a job created in the Advanced job page were not being saved. Bug introduced by edits in #21094.

Also removes the created_by property under custom_settings if the job has custom URLs defined, as these will not be editable if the job is cloned via one of the job wizards.

@peteharverson peteharverson added bug Fixes for quality problems that affect the customer experience review v7.0.0 v6.4.0 :ml Feature:Anomaly Detection ML anomaly detection labels Jul 25, 2018
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui

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

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Member

@jgowdyelastic jgowdyelastic left a comment

Choose a reason for hiding this comment

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

Generally LGTM, i suggested a simplification for the settings checks.


if (newJobData && newJobData.customUrls) {
if (settingsData.custom_settings === undefined) {
settingsData.custom_settings = {};
Copy link
Member

@jgowdyelastic jgowdyelastic Jul 25, 2018

Choose a reason for hiding this comment

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

rather than having the previous check for job.custom_setting and then this check for settingsData.custom_settings they could both be replaced with a single check here:

settingsData.custom_settings = job.custom_settings || {};

or

settingsData.custom_settings = (job.custom_settings !== undefined) ? job.custom_settings : {};

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@peteharverson peteharverson merged commit 124ec66 into elastic:master Jul 26, 2018
@peteharverson peteharverson deleted the ml-edit-customurl-settings branch July 26, 2018 08:23
peteharverson added a commit to peteharverson/kibana that referenced this pull request Jul 26, 2018
…tic#21207)

* [ML] Fix saving of custom URLs for job created in Advanced page

* [ML] Edit to custom URL save custom_settings following review
peteharverson added a commit to peteharverson/kibana that referenced this pull request Jul 26, 2018
…tic#21207)

* [ML] Fix saving of custom URLs for job created in Advanced page

* [ML] Edit to custom URL save custom_settings following review
sorenlouv pushed a commit to sorenlouv/kibana that referenced this pull request Jul 26, 2018
…tic#21207)

* [ML] Fix saving of custom URLs for job created in Advanced page

* [ML] Edit to custom URL save custom_settings following review
sorenlouv pushed a commit to sorenlouv/kibana that referenced this pull request Jul 26, 2018
…tic#21207)

* [ML] Fix saving of custom URLs for job created in Advanced page

* [ML] Edit to custom URL save custom_settings following review
peteharverson added a commit that referenced this pull request Jul 26, 2018
…) (#21259)

* [ML] Fix saving of custom URLs for job created in Advanced page

* [ML] Edit to custom URL save custom_settings following review
peteharverson pushed a commit that referenced this pull request Jul 26, 2018
…) (#21261)

* [ML] Fix saving of custom URLs for job created in Advanced page

* [ML] Edit to custom URL save custom_settings following review
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Feature:Anomaly Detection ML anomaly detection :ml review v6.4.0 v7.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants