[Security Solution] New Alert table may have broken building block highlighting. #152318
Assignees
Labels
bug
Fixes for quality problems that affect the customer experience
fixed
impact:high
Addressing this issue will have a high level of impact on the quality/strength of our product.
Team: SecuritySolution
Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc.
Team:Threat Hunting:Investigations
Security Solution Investigations Team
v8.8.0
Comments
logeekal
added
bug
|
Pinging @elastic/security-solution (Team: SecuritySolution) |
This was referenced Feb 28, 2023
|
@logeekal when reporting bugs, please follow this template. We should track the version where the issue was found, the steps to reproduce it, the current behaviour and the expected one, those are the most important fields. You can find here an example about how to report an issue. Please note that following all the above is helpful for colleagues to validate the fix when available. Thanks! |
|
Sure @MadameSheema, I will keep that in mind and will update this bug according to the template. |
michaelolo24
added
impact:high
|
@MadameSheema I updated the description. Would love to chat about how to automate testing for this functionality as well. Since the only visual change for the highlighting is a background color aka an added CSS class I'm not super sure how to test this best in Cypress. Is testing for the existence of the class enough? |
|
@janmonschke for this case, yes, checking that the element has the expected CSS could be enough. Can we cover this with a unit test? If not or is complex, instead of creating a new Cypress test I would take advantage of the one we already have |
|
@MadameSheema Ah yes, in that case, I will just add this as a unit test, thanks :) |
1 task
|
@MadameSheema Sadly it wasn't possible to do it in a unit test. So I added a quick extra assertion to the test you highlighted. 👍 #155497 |
janmonschke
added a commit
that referenced
this issue
Apr 28, 2023
## Summary As described in #152318, we noticed that building block alerts were not highlighted anymore after the migration to the new alerts table. A preferred implementation of building block alert highlighting would follow the [`EUIDataGrid` approach of row highlighting](https://eui.elastic.co/#/tabular-content/data-grid-style-display#grid-row-classes). The `DataGrid` allows you to pass custom CSS class names for each row (`gridStyle.rowClasses`). That would allow us to highlight table rows with building block alerts. However, without access to the underlying data, we would not be able generate the correct `rowClasses` for rows with building block alerts. So simply passing `gridStyle.rowClasses` to the `AlertsStateTable` was not an option. Therefore in this PR we're introducing a new prop on the `AlertsTable`, the `highlightedRowMapper`. It's a callback function that receives the alert data and when it returns true, that row will be highlighted. This allows for highlighting of rows from the outside without exposing too many details about the underlying data structures. **Screenshot of the alerts table with a highlightedRowMapper that highlights building block alerts** <img width="1259" alt="Screenshot 2023-04-21 at 13 03 54" src="https://user-images.githubusercontent.com/68591/233620704-a56204c0-e285-4289-897a-58481f440446.png"> ### Additional notes - Since the alerts table has default grid styles, it allows to pass `gridStyle` and it computes its own `rowClasses` for "active row" highlighting, the logic for merging all those styles looks intimidating. I tried my best to comment that part of code to make it clear why the merges are necessary and how they work. - While working on the issue, I noticed that active rows are not highlighted anymore (related bug: #155487). The changes in this PR fix that behaviour as well as you can see in the screenshot below: <img width="936" alt="Screenshot 2023-04-21 at 13 04 15" src="https://user-images.githubusercontent.com/68591/233620752-d752dada-9c97-4f00-933a-5425e19a5793.png"> ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios --------- Co-authored-by: Kibana Machine <[email protected]>
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this issue
Apr 28, 2023
) ## Summary As described in elastic#152318, we noticed that building block alerts were not highlighted anymore after the migration to the new alerts table. A preferred implementation of building block alert highlighting would follow the [`EUIDataGrid` approach of row highlighting](https://eui.elastic.co/#/tabular-content/data-grid-style-display#grid-row-classes). The `DataGrid` allows you to pass custom CSS class names for each row (`gridStyle.rowClasses`). That would allow us to highlight table rows with building block alerts. However, without access to the underlying data, we would not be able generate the correct `rowClasses` for rows with building block alerts. So simply passing `gridStyle.rowClasses` to the `AlertsStateTable` was not an option. Therefore in this PR we're introducing a new prop on the `AlertsTable`, the `highlightedRowMapper`. It's a callback function that receives the alert data and when it returns true, that row will be highlighted. This allows for highlighting of rows from the outside without exposing too many details about the underlying data structures. **Screenshot of the alerts table with a highlightedRowMapper that highlights building block alerts** <img width="1259" alt="Screenshot 2023-04-21 at 13 03 54" src="https://user-images.githubusercontent.com/68591/233620704-a56204c0-e285-4289-897a-58481f440446.png"> ### Additional notes - Since the alerts table has default grid styles, it allows to pass `gridStyle` and it computes its own `rowClasses` for "active row" highlighting, the logic for merging all those styles looks intimidating. I tried my best to comment that part of code to make it clear why the merges are necessary and how they work. - While working on the issue, I noticed that active rows are not highlighted anymore (related bug: elastic#155487). The changes in this PR fix that behaviour as well as you can see in the screenshot below: <img width="936" alt="Screenshot 2023-04-21 at 13 04 15" src="https://user-images.githubusercontent.com/68591/233620752-d752dada-9c97-4f00-933a-5425e19a5793.png"> ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios --------- Co-authored-by: Kibana Machine <[email protected]> (cherry picked from commit ce71264)
kibanamachine
added a commit
that referenced
this issue
May 2, 2023
…) (#156137) # Backport This will backport the following commits from `main` to `8.8`: - [[SecuritySolution] Fix building block alert highlighting (#155497)](#155497) <!--- Backport version: 8.9.7 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Jan Monschke","email":"[email protected]"},"sourceCommit":{"committedDate":"2023-04-28T07:50:21Z","message":"[SecuritySolution] Fix building block alert highlighting (#155497)\n\n## Summary\r\n\r\nAs described in #152318, we\r\nnoticed that building block alerts were not highlighted anymore after\r\nthe migration to the new alerts table.\r\n\r\nA preferred implementation of building block alert highlighting would\r\nfollow the [`EUIDataGrid` approach of row\r\nhighlighting](https://eui.elastic.co/#/tabular-content/data-grid-style-display#grid-row-classes).\r\nThe `DataGrid` allows you to pass custom CSS class names for each row\r\n(`gridStyle.rowClasses`). That would allow us to highlight table rows\r\nwith building block alerts.\r\n\r\nHowever, without access to the underlying data, we would not be able\r\ngenerate the correct `rowClasses` for rows with building block alerts.\r\nSo simply passing `gridStyle.rowClasses` to the `AlertsStateTable` was\r\nnot an option.\r\n\r\nTherefore in this PR we're introducing a new prop on the `AlertsTable`,\r\nthe `highlightedRowMapper`. It's a callback function that receives the\r\nalert data and when it returns true, that row will be highlighted.\r\n\r\nThis allows for highlighting of rows from the outside without exposing\r\ntoo many details about the underlying data structures.\r\n\r\n**Screenshot of the alerts table with a highlightedRowMapper that\r\nhighlights building block alerts**\r\n\r\n<img width=\"1259\" alt=\"Screenshot 2023-04-21 at 13 03 54\"\r\nsrc=\"https://user-images.githubusercontent.com/68591/233620704-a56204c0-e285-4289-897a-58481f440446.png\">\r\n\r\n### Additional notes\r\n\r\n- Since the alerts table has default grid styles, it allows to pass\r\n`gridStyle` and it computes its own `rowClasses` for \"active row\"\r\nhighlighting, the logic for merging all those styles looks intimidating.\r\nI tried my best to comment that part of code to make it clear why the\r\nmerges are necessary and how they work.\r\n- While working on the issue, I noticed that active rows are not\r\nhighlighted anymore (related bug:\r\nhttps://github.com//issues/155487). The changes in this PR\r\nfix that behaviour as well as you can see in the screenshot below:\r\n\r\n<img width=\"936\" alt=\"Screenshot 2023-04-21 at 13 04 15\"\r\nsrc=\"https://user-images.githubusercontent.com/68591/233620752-d752dada-9c97-4f00-933a-5425e19a5793.png\">\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n\r\n---------\r\n\r\nCo-authored-by: Kibana Machine <[email protected]>","sha":"ce71264d88c46c45ed8ac00dc6c74bcbd05809c7","branchLabelMapping":{"^v8.9.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","release_note:skip","Team:ResponseOps","Team:Threat Hunting:Investigations","backport:prev-minor","v8.8.0","v8.9.0"],"number":155497,"url":"https://github.com/elastic/kibana/pull/155497","mergeCommit":{"message":"[SecuritySolution] Fix building block alert highlighting (#155497)\n\n## Summary\r\n\r\nAs described in #152318, we\r\nnoticed that building block alerts were not highlighted anymore after\r\nthe migration to the new alerts table.\r\n\r\nA preferred implementation of building block alert highlighting would\r\nfollow the [`EUIDataGrid` approach of row\r\nhighlighting](https://eui.elastic.co/#/tabular-content/data-grid-style-display#grid-row-classes).\r\nThe `DataGrid` allows you to pass custom CSS class names for each row\r\n(`gridStyle.rowClasses`). That would allow us to highlight table rows\r\nwith building block alerts.\r\n\r\nHowever, without access to the underlying data, we would not be able\r\ngenerate the correct `rowClasses` for rows with building block alerts.\r\nSo simply passing `gridStyle.rowClasses` to the `AlertsStateTable` was\r\nnot an option.\r\n\r\nTherefore in this PR we're introducing a new prop on the `AlertsTable`,\r\nthe `highlightedRowMapper`. It's a callback function that receives the\r\nalert data and when it returns true, that row will be highlighted.\r\n\r\nThis allows for highlighting of rows from the outside without exposing\r\ntoo many details about the underlying data structures.\r\n\r\n**Screenshot of the alerts table with a highlightedRowMapper that\r\nhighlights building block alerts**\r\n\r\n<img width=\"1259\" alt=\"Screenshot 2023-04-21 at 13 03 54\"\r\nsrc=\"https://user-images.githubusercontent.com/68591/233620704-a56204c0-e285-4289-897a-58481f440446.png\">\r\n\r\n### Additional notes\r\n\r\n- Since the alerts table has default grid styles, it allows to pass\r\n`gridStyle` and it computes its own `rowClasses` for \"active row\"\r\nhighlighting, the logic for merging all those styles looks intimidating.\r\nI tried my best to comment that part of code to make it clear why the\r\nmerges are necessary and how they work.\r\n- While working on the issue, I noticed that active rows are not\r\nhighlighted anymore (related bug:\r\nhttps://github.com//issues/155487). The changes in this PR\r\nfix that behaviour as well as you can see in the screenshot below:\r\n\r\n<img width=\"936\" alt=\"Screenshot 2023-04-21 at 13 04 15\"\r\nsrc=\"https://user-images.githubusercontent.com/68591/233620752-d752dada-9c97-4f00-933a-5425e19a5793.png\">\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n\r\n---------\r\n\r\nCo-authored-by: Kibana Machine <[email protected]>","sha":"ce71264d88c46c45ed8ac00dc6c74bcbd05809c7"}},"sourceBranch":"main","suggestedTargetBranches":["8.8"],"targetPullRequestStates":[{"branch":"8.8","label":"v8.8.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.9.0","labelRegex":"^v8.9.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/155497","number":155497,"mergeCommit":{"message":"[SecuritySolution] Fix building block alert highlighting (#155497)\n\n## Summary\r\n\r\nAs described in #152318, we\r\nnoticed that building block alerts were not highlighted anymore after\r\nthe migration to the new alerts table.\r\n\r\nA preferred implementation of building block alert highlighting would\r\nfollow the [`EUIDataGrid` approach of row\r\nhighlighting](https://eui.elastic.co/#/tabular-content/data-grid-style-display#grid-row-classes).\r\nThe `DataGrid` allows you to pass custom CSS class names for each row\r\n(`gridStyle.rowClasses`). That would allow us to highlight table rows\r\nwith building block alerts.\r\n\r\nHowever, without access to the underlying data, we would not be able\r\ngenerate the correct `rowClasses` for rows with building block alerts.\r\nSo simply passing `gridStyle.rowClasses` to the `AlertsStateTable` was\r\nnot an option.\r\n\r\nTherefore in this PR we're introducing a new prop on the `AlertsTable`,\r\nthe `highlightedRowMapper`. It's a callback function that receives the\r\nalert data and when it returns true, that row will be highlighted.\r\n\r\nThis allows for highlighting of rows from the outside without exposing\r\ntoo many details about the underlying data structures.\r\n\r\n**Screenshot of the alerts table with a highlightedRowMapper that\r\nhighlights building block alerts**\r\n\r\n<img width=\"1259\" alt=\"Screenshot 2023-04-21 at 13 03 54\"\r\nsrc=\"https://user-images.githubusercontent.com/68591/233620704-a56204c0-e285-4289-897a-58481f440446.png\">\r\n\r\n### Additional notes\r\n\r\n- Since the alerts table has default grid styles, it allows to pass\r\n`gridStyle` and it computes its own `rowClasses` for \"active row\"\r\nhighlighting, the logic for merging all those styles looks intimidating.\r\nI tried my best to comment that part of code to make it clear why the\r\nmerges are necessary and how they work.\r\n- While working on the issue, I noticed that active rows are not\r\nhighlighted anymore (related bug:\r\nhttps://github.com//issues/155487). The changes in this PR\r\nfix that behaviour as well as you can see in the screenshot below:\r\n\r\n<img width=\"936\" alt=\"Screenshot 2023-04-21 at 13 04 15\"\r\nsrc=\"https://user-images.githubusercontent.com/68591/233620752-d752dada-9c97-4f00-933a-5425e19a5793.png\">\r\n\r\n### Checklist\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n\r\n---------\r\n\r\nCo-authored-by: Kibana Machine <[email protected]>","sha":"ce71264d88c46c45ed8ac00dc6c74bcbd05809c7"}}]}] BACKPORT--> Co-authored-by: Jan Monschke <[email protected]>
![https://user-images.githubusercontent.com/68591/233620704-a56204c0-e285-4289-897a-58481f440446.png\">\r\n\r\n###](https://user-images.githubusercontent.com/68591/233620704-a56204c0-e285-4289-897a-58481f440446.png\">\r\n\r\n###){kind=link}
![https://user-images.githubusercontent.com/68591/233620752-d752dada-9c97-4f00-933a-5425e19a5793.png\">\r\n\r\n###](https://user-images.githubusercontent.com/68591/233620752-d752dada-9c97-4f00-933a-5425e19a5793.png\">\r\n\r\n###){kind=link}
|
Fixed and backported 👍 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Describe the bug:
Kibana/Elasticsearch Stack version:
Initial setup:
Steps to reproduce:
Additional filtersand enableInclude building block alertsCurrent behavior:
Expected behavior:
Additional information:
We suspect this to have happened when we switched to the new alert table: #149128
The text was updated successfully, but these errors were encountered: