-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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] Single Metric Viewer: Fix job check. #55191
[ML] Single Metric Viewer: Fix job check. #55191
Conversation
Pinging @elastic/ml-ui (:ml) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested and LGTM ⚡️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested and LGTM
@elasticmachine merge upstream |
<> | ||
<NavigationMenu tabId="timeseriesexplorer" /> | ||
{/* Show animated progress bar while loading */} | ||
{loading === true && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
total nit, this could be a ternary
setGlobalState('ml', { jobIds: [validSelectedJobIds[0]] }); | ||
return true; | ||
} else { | ||
// if a group has been loaded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this else reachable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good spot that must have been some quite old code I copied over. When picking a group to display in Anomaly Explorer, the job ids will also be added to the URL. Using the UI and coming to Single Metric Viewer, you cannot have a URL that just has groups but not jobs populated. I cleaned up the code in 6017f59.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
Fixes switching via navigation to Single Metric Viewer from Anomaly Explorer for a job which isn't supported in the Single Metric Viewer.
Fixes switching via navigation to Single Metric Viewer from Anomaly Explorer for a job which isn't supported in the Single Metric Viewer.
* upstream/master: [ML] Single Metric Viewer: Fix job check. (elastic#55191)
* master: (108 commits) [ML] Single Metric Viewer: Fix job check. (elastic#55191) Show error page when accessing unavailable app (elastic#54656) [ML] Improving job wizards with datafeed aggregations (elastic#55180) remove flaly assetion. a license presence tested anyway (elastic#55289) fix commonly used ranges uptime (elastic#54930) [SIEM] Use proper icons on Detections view (elastic#55215) Fix: invalid translation referenced (elastic#54901) [State Management] Remove AppState from edit_index_pattern page (elastic#54104) Implements `getStartServices` on server-side (elastic#55156) Move vis_vega_type/data_model tests to jest (elastic#55186) [SIEM] [Detection Engine] Update status on rule details page (elastic#55201) Fix KQL value suggestions for nested fields (elastic#54820) Enforce camelCase format for a plugin id (elastic#53759) [SIEM] Detection engine cleanup for rule details/creation/edit page (elastic#55069) Remove nested root from index pattern (elastic#54978) [Reporting/Migration] ReportingSetup, LegacySetup (elastic#54198) [SIEM] [Detection Engine] Fixes duplicate rule action (elastic#55252) [SIEM] Detections add alert & signal tab (elastic#55127) Management API - redirect on disabled app path (elastic#55136) [SIEM][Detection Engine] Fixes critical regression on the backend with immutable and tags ...
Summary
Part of #52986.
Fixes switching via navigation to Single Metric Viewer from Anomaly Explorer for a job which isn't supported in the Single Metric Viewer.
checkJobSelection()
method fromtimeseriesexplorer.js
to a stand-alone function asvalidateJobSelection()
intimeseriesexplorer_utils
. This allows us to move the check to<TimeSeriesExplorerUrlStateManager />
further up to this hook based outer component. Based on that we no longer need to pass on some of the stillundefined
props to the inner components which should result in some less renders and stricter props with less optional attributes. With this in place we now pass on the current job asselectedJobId
totimeseriesexplorer.js
and not the raw array we get fromglobalState
anymore.TimeSeriesExplorerPage
component formtimeseriesexplorer.js
to its own file to make it reusable.Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n supportDocumentation was added for features that require explanation or tutorialsThis was checked for keyboard-only and screenreader accessibilityFor maintainers