-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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] Anomaly Explorer: if filter includes wildcard ensure matching swimlanes are not masked #65384
[ML] Anomaly Explorer: if filter includes wildcard ensure matching swimlanes are not masked #65384
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.
Code LGTM.
0f8ea28
to
416dff9
Compare
const wildCardFields = | ||
filteredFields !== undefined | ||
? filteredFields.filter(field => /\@kuery-wildcard\@$/.test(field)) | ||
: []; | ||
const substring = | ||
wildCardFields.length > 0 ? wildCardFields[0].replace(/\@kuery-wildcard\@$/, '') : null; | ||
const hasMatchingPoints = | ||
substring !== null && | ||
swimlaneData.points.some(point => { | ||
return point.laneLabel.includes(substring); | ||
}); |
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.
I'd suggest creating a dedicated function hasMatchingPoints
which receives filteredFields
and swimlaneData
as an input and return a boolean. It'll easy to cover this logic with a unit test.
I also see you filter the entire collection of filteredFields
but eventually use only the first element to get a substring. Maybe worth to use find
instead of filter
?
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.
TBH I think it should not be part of this component. It makes more sense to resolve it on a parent level and just provide maskAll
prop.
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 call on using find
👌 I'll update that.
Makes sense to create a helper function and pass in the correct maskAll prop. 👍
Thanks! 😄
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.
Updated in d7d89c0
cc @darnautov
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.
thanks @alvarezmelissa87! would be great to have 2 unit tests for hasMatchingPoints
(for true and false)
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.
Added in d92bf5c 🙌 😄
cc @darnautov
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.
thanks a lot for adding tests @alvarezmelissa87!
nit: test description generally starts with should
, e.g. something like test('should return true when...')
, but it can be changed later 🙂
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!
retest |
2df3ac6
to
7d4f894
Compare
retest |
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 locally and LGTM
7d4f894
to
b0b5963
Compare
retest |
💛 Build succeeded, but was flaky
Test FailuresKibana Pipeline / kibana-xpack-agent / Chrome X-Pack UI Functional Tests.x-pack/test/functional_with_es_ssl/apps/uptime/alert_flyout·ts.Uptime app with real-world data uptime alerts tls alert can open alert flyoutStandard Out
Stack Trace
History
To update your PR or re-run it, just comment with: |
…imlanes are not masked (elastic#65384) * if wildcard search no mask for matching swimlanes * update swimlane proptypes * add helper function * add tests for helper function
…imlanes are not masked (elastic#65384) * if wildcard search no mask for matching swimlanes * update swimlane proptypes * add helper function * add tests for helper function
…imlanes are not masked (elastic#65384) * if wildcard search no mask for matching swimlanes * update swimlane proptypes * add helper function * add tests for helper function
Summary
Fixes #61270
Ensure mask is not set over view by swimlanes if swimlane data includes filter pattern.
Checklist
Delete any items that are not applicable to this PR.