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

[7.x] [Security Solution][Detection Engine] Fixes indicator matches mapping UI where invalid list values can cause overwrites of other values (#89066) #89817

Merged
merged 1 commit into from
Jan 30, 2021

Conversation

FrankHassanabad
Copy link
Contributor

Backports the following commits to 7.x:

… UI where invalid list values can cause overwrites of other values (elastic#89066)

## Summary

This fixes the ReactJS keys to not use array indexes for the ReactJS keys which fixes  elastic#84893 as well as a few other bugs that I will show below. The fix for the ReactJS keys is to add a unique id version 4 `uuid.v4()` to the incoming threat_mapping and the entities. On save out to elastic I remove the id. This is considered [better practices for ReactJS keys](https://reactjs.org/docs/lists-and-keys.html)

Down the road we might augment the arrays to have that id information but for now I add them when we get the data and then remove them as we save the data.

This PR also:
* Fixes tech debt around the hooks to remove the disabling of the `react-hooks/exhaustive-deps` in a few areas
* Fixes one React Hook misnamed that would not have triggered React linter rules (_useRuleAsyn)
* Adds 23 new Cypress e2e tests
* Adds a new pattern of dealing with on button clicks for the Cypress tests that are make it less flakey
```ts
cy.get(`button[title="${indexField}"]`)
      .should('be.visible')
      .then(([e]) => e.click());
```
* Adds several new utilities to Cypress for testing rows for indicator matches and other Cypress utils to improve velocity and ergonomics
```ts
fillIndicatorMatchRow
getDefineContinueButton
getIndicatorInvalidationText
getIndicatorIndexComboField
getIndicatorDeleteButton
getIndicatorOrButton
getIndicatorAndButton
``` 

## Bug 1
Deleting row 1 can cause row 2 to be cleared out or only partial data to stick around.

Before:
![im_bug_1](https://user-images.githubusercontent.com/1151048/105916137-c57b1d80-5fed-11eb-95b7-ad25b71cf4b8.gif)

After:
![im_fix_1_1](https://user-images.githubusercontent.com/1151048/105917509-9fef1380-5fef-11eb-98eb-025c226f79fe.gif)

## Bug 2 
Deleting row 2 in the middle of 3 rows did not shift the value up correctly

Before:
![im_bug_2](https://user-images.githubusercontent.com/1151048/105917584-c01ed280-5fef-11eb-8c5b-fefb36f81008.gif)

After: 
![im_fix_2](https://user-images.githubusercontent.com/1151048/105917650-e0e72800-5fef-11eb-9fd3-020d52e4e3b1.gif)

## Bug 3
When using OR with values it does not shift up correctly similar to AND

Before:
![im_bug_3](https://user-images.githubusercontent.com/1151048/105917691-f2303480-5fef-11eb-9368-b11d23159606.gif)

After: 
![im_fix_3](https://user-images.githubusercontent.com/1151048/105917714-f9574280-5fef-11eb-9be4-1f56c207525a.gif)

### Checklist

Delete any items that are not applicable to this PR.

- [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
- [ ] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/))
- [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))
- [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))
- [x] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers)
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 2175 2177 +2

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
securitySolution 7.5MB 7.5MB +3.7KB

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@FrankHassanabad FrankHassanabad merged commit 55d4b74 into elastic:7.x Jan 30, 2021
@FrankHassanabad FrankHassanabad deleted the backport/7.x/pr-89066 branch January 30, 2021 04:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants