Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[Security Solution][Detection Alerts] Alert tagging #157786
[Security Solution][Detection Alerts] Alert tagging #157786
Changes from 11 commits
3ad0adf
233586d
c5029b6
96658d8
c77aa15
d391a76
a06d3b5
1a5c13c
bbd7a89
938e805
3bcd05a
3b9291b
d3fda33
488293a
aa26c0a
09cae98
49c39b8
f9f91f9
b24c6d0
10eaf14
7d3d600
a9cc608
8698128
5f291df
f877402
2df309a
982b81f
a352269
8339d9f
64b5077
69cbf20
4a6a735
db9bc72
0119e69
9d8ae0c
baf4fde
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Q: should this be
string[]
?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 was thinking that in the previous commit here, yeah, but I ran into a test failing with an error message about this type having to be a keyword and not an array, like the other
string[]
types in that file. This is consistent with the other string arrays in the file, but if that type works better, I can revert to that other commitThere 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.
What do you think of following the rules bulk edit tags schema? It may make it easier to expand the feature by following a more reusable pattern like:
That way if we decide theres a new action type we want to introduce for tags, the schema updates are minimal.
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 be open to more discussion on this pertaining to more reusable patterns and expandability, it's definitely a bit unusual, but the main reason we created it like this in the first place was because we have the intermediate state in the bulk actions tags dropdown, so we couldn't just handle setting all queried alerts to the same tags array. With the two different buckets of add and remove we can cover each of the possible use cases for each specific alert with one api call.
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.
Can we group all these props under a single
onUpdateSuccess
prop? And let the caller do whatever it needs when the update succeeded, outside this component.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.
Here are you checking if the
checked
property exists or if it'snull
orundefined
?