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

[Defend Workflows] Fix bug when event filter value cannot be changed without using {backspace} #192196

Merged

Conversation

gergoabraham
Copy link
Contributor

@gergoabraham gergoabraham commented Sep 5, 2024

Summary

This PR does 2 things around editing the value field for an Event Filter.

  1. It fixes the issue when during editing an existing Event Filter you cannot update the value without pressing {backspace}, by clearing selection when user is typing: 5da28aa
    Before:
    before
    ➡️
    After:
    after

  2. Improves suggestions: before the change, suggestions were there initially, but after they are narrowed down by typing, they won't be displayed again after the user clears the input field. Therefore f9d60cf sets the suggestion search query unconditionally, helping with this issue.
    Before:
    before
    ➡️
    After:
    after

Checklist

Delete any items that are not applicable to this PR.

@gergoabraham gergoabraham added release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting Team:Defend Workflows “EDR Workflows” sub-team of Security Solution labels Sep 5, 2024
@gergoabraham gergoabraham self-assigned this Sep 5, 2024
@gergoabraham
Copy link
Contributor Author

/ci

@gergoabraham gergoabraham marked this pull request as ready for review September 6, 2024 09:00
@gergoabraham gergoabraham requested review from a team as code owners September 6, 2024 09:00
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-defend-workflows (Team:Defend Workflows)

@gergoabraham
Copy link
Contributor Author

@elasticmachine merge upstream

@gergoabraham
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@vitaliidm vitaliidm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Detection Engine area LGTM

Comment on lines 173 to 177
if (searchVal) {
onChange('');
}

setSearchQuery(searchVal);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be great to add here comments what these 2 calls doing and why they needed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added, thanks for the review!
dd2bf53

Copy link
Member

@ashokaditya ashokaditya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix 🚀


expect(mockOnChange).toHaveBeenCalledWith('value 1');
});

test('it invokes "onChange" with empty value (i.e. clears selection) when new value searched', async () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: no need for an async block here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🦅 👁️ !
27c1c4d

Copy link
Contributor

@tomsonpl tomsonpl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@gergoabraham gergoabraham requested review from a team as code owners September 13, 2024 12:03
@gergoabraham gergoabraham removed request for a team and jpdjere September 13, 2024 12:08
@gergoabraham gergoabraham marked this pull request as ready for review September 13, 2024 12:12
@gergoabraham
Copy link
Contributor Author

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
lists 143.4KB 143.4KB +10.0B
securitySolution 20.3MB 20.3MB +10.0B
total +20.0B

History

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

cc @gergoabraham

@gergoabraham gergoabraham merged commit b31d119 into elastic:main Sep 13, 2024
39 checks passed
@gergoabraham gergoabraham added v8.16.0 and removed backport:skip This commit does not require backporting labels Sep 13, 2024
@gergoabraham gergoabraham deleted the fix-event-filter-value-change-bug branch September 16, 2024 08:24
@gergoabraham gergoabraham restored the fix-event-filter-value-change-bug branch September 16, 2024 08:53
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Sep 16, 2024
@gergoabraham
Copy link
Contributor Author

💚 All backports created successfully

Status Branch Result
8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

gergoabraham added a commit to gergoabraham/kibana that referenced this pull request Sep 16, 2024
…without using {backspace} (elastic#192196)

## Summary

This PR does 2 things around editing the value field for an Event
Filter.

1. It fixes the issue when during editing an existing Event Filter you
cannot update the value without pressing {backspace}, by clearing
selection when user is typing: 5da28aa
Before:

![before](https://github.com/user-attachments/assets/7355c788-a9fd-4ea5-81b3-89ae41db2ee7)
➡️
After:

![after](https://github.com/user-attachments/assets/aa928fa4-6203-4fad-8f9b-4e586ac4d562)

2. Improves suggestions: before the change, suggestions were there
initially, but after they are narrowed down by typing, they won't be
displayed again after the user clears the input field. Therefore
f9d60cf sets the suggestion search
query unconditionally, helping with this issue.
Before:

![before](https://github.com/user-attachments/assets/87ccfba6-5b9d-4976-a5af-13c3d56db373)
➡️
After:

![after](https://github.com/user-attachments/assets/c21a909c-4b45-470e-9e77-9edc269f07f7)

### 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

---------

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
(cherry picked from commit b31d119)
gergoabraham added a commit that referenced this pull request Sep 16, 2024
…anged without using {backspace} (#192196) (#192977)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[Defend Workflows] Fix bug when event filter value cannot be changed
without using {backspace}
(#192196)](#192196)

<!--- Backport version: 9.6.0 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sorenlouv/backport)

<!--BACKPORT [{"author":{"name":"Gergő
Ábrahám","email":"gergo.abraham@elastic.co"},"sourceCommit":{"committedDate":"2024-09-13T16:59:44Z","message":"[Defend
Workflows] Fix bug when event filter value cannot be changed without
using {backspace} (#192196)\n\n## Summary\r\n\r\nThis PR does 2 things
around editing the value field for an Event\r\nFilter.\r\n\r\n1. It
fixes the issue when during editing an existing Event Filter
you\r\ncannot update the value without pressing {backspace}, by
clearing\r\nselection when user is typing:
5da28aa\r\nBefore:\r\n\r\n![before](https://github.com/user-attachments/assets/7355c788-a9fd-4ea5-81b3-89ae41db2ee7)\r\n➡️
\r\nAfter:\r\n\r\n![after](https://github.com/user-attachments/assets/aa928fa4-6203-4fad-8f9b-4e586ac4d562)\r\n\r\n2.
Improves suggestions: before the change, suggestions were
there\r\ninitially, but after they are narrowed down by typing, they
won't be\r\ndisplayed again after the user clears the input field.
Therefore\r\nf9d60cf223644a3072d1a60fe273586338247b96 sets the
suggestion search\r\nquery unconditionally, helping with this
issue.\r\nBefore:\r\n\r\n![before](https://github.com/user-attachments/assets/87ccfba6-5b9d-4976-a5af-13c3d56db373)\r\n➡️
\r\nAfter:\r\n\r\n![after](https://github.com/user-attachments/assets/c21a909c-4b45-470e-9e77-9edc269f07f7)\r\n\r\n\r\n###
Checklist\r\n\r\nDelete any items that are not applicable to this
PR.\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: Elastic Machine
<elasticmachine@users.noreply.github.com>","sha":"b31d119e5532c362c44a134547f461fd7db56770","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","backport:skip","v9.0.0","Team:Defend
Workflows"],"title":"[Defend Workflows] Fix bug when event filter value
cannot be changed without using
{backspace}","number":192196,"url":"https://github.com/elastic/kibana/pull/192196","mergeCommit":{"message":"[Defend
Workflows] Fix bug when event filter value cannot be changed without
using {backspace} (#192196)\n\n## Summary\r\n\r\nThis PR does 2 things
around editing the value field for an Event\r\nFilter.\r\n\r\n1. It
fixes the issue when during editing an existing Event Filter
you\r\ncannot update the value without pressing {backspace}, by
clearing\r\nselection when user is typing:
5da28aa\r\nBefore:\r\n\r\n![before](https://github.com/user-attachments/assets/7355c788-a9fd-4ea5-81b3-89ae41db2ee7)\r\n➡️
\r\nAfter:\r\n\r\n![after](https://github.com/user-attachments/assets/aa928fa4-6203-4fad-8f9b-4e586ac4d562)\r\n\r\n2.
Improves suggestions: before the change, suggestions were
there\r\ninitially, but after they are narrowed down by typing, they
won't be\r\ndisplayed again after the user clears the input field.
Therefore\r\nf9d60cf223644a3072d1a60fe273586338247b96 sets the
suggestion search\r\nquery unconditionally, helping with this
issue.\r\nBefore:\r\n\r\n![before](https://github.com/user-attachments/assets/87ccfba6-5b9d-4976-a5af-13c3d56db373)\r\n➡️
\r\nAfter:\r\n\r\n![after](https://github.com/user-attachments/assets/c21a909c-4b45-470e-9e77-9edc269f07f7)\r\n\r\n\r\n###
Checklist\r\n\r\nDelete any items that are not applicable to this
PR.\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: Elastic Machine
<elasticmachine@users.noreply.github.com>","sha":"b31d119e5532c362c44a134547f461fd7db56770"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/192196","number":192196,"mergeCommit":{"message":"[Defend
Workflows] Fix bug when event filter value cannot be changed without
using {backspace} (#192196)\n\n## Summary\r\n\r\nThis PR does 2 things
around editing the value field for an Event\r\nFilter.\r\n\r\n1. It
fixes the issue when during editing an existing Event Filter
you\r\ncannot update the value without pressing {backspace}, by
clearing\r\nselection when user is typing:
5da28aa\r\nBefore:\r\n\r\n![before](https://github.com/user-attachments/assets/7355c788-a9fd-4ea5-81b3-89ae41db2ee7)\r\n➡️
\r\nAfter:\r\n\r\n![after](https://github.com/user-attachments/assets/aa928fa4-6203-4fad-8f9b-4e586ac4d562)\r\n\r\n2.
Improves suggestions: before the change, suggestions were
there\r\ninitially, but after they are narrowed down by typing, they
won't be\r\ndisplayed again after the user clears the input field.
Therefore\r\nf9d60cf223644a3072d1a60fe273586338247b96 sets the
suggestion search\r\nquery unconditionally, helping with this
issue.\r\nBefore:\r\n\r\n![before](https://github.com/user-attachments/assets/87ccfba6-5b9d-4976-a5af-13c3d56db373)\r\n➡️
\r\nAfter:\r\n\r\n![after](https://github.com/user-attachments/assets/c21a909c-4b45-470e-9e77-9edc269f07f7)\r\n\r\n\r\n###
Checklist\r\n\r\nDelete any items that are not applicable to this
PR.\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: Elastic Machine
<elasticmachine@users.noreply.github.com>","sha":"b31d119e5532c362c44a134547f461fd7db56770"}}]}]
BACKPORT-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution v8.16.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants