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

Fix search issue with Alert search bar #155796

Merged
merged 7 commits into from
May 2, 2023

Conversation

benakansara
Copy link
Contributor

@benakansara benakansara commented Apr 25, 2023

Fixes #155655

Added onQuerySubmit as optional parameter to AlertsSearchBar and made onQueryChange as optional parameter as this is not needed for Observability Alerts.

[Observability] Alerts page:

Screen.Recording.2023-04-26.at.17.17.55.mov

[Security] Alerts filter in action:

Screen.Recording.2023-04-26.at.17.14.58.mov

@benakansara benakansara added backport Team: Actionable Observability - DEPRECATED For Observability Alerting and SLOs use "Team:obs-ux-management", for AIops "Team:obs-knowledge" v8.8.0 labels Apr 25, 2023
@benakansara benakansara requested a review from a team April 25, 2023 20:42
@benakansara benakansara self-assigned this Apr 25, 2023
@benakansara benakansara requested a review from a team as a code owner April 25, 2023 20:42
@elasticmachine
Copy link
Contributor

Pinging @elastic/actionable-observability (Team: Actionable Observability)

@apmmachine
Copy link
Contributor

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

  • /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@CoenWarmer
Copy link
Contributor

@benakansara thanks for investigating this. This was introduced by #154680. Removing this prop might break functionality introduced in that PR. Best to check with @Zacqary.

@benakansara
Copy link
Contributor Author

@CoenWarmer that's right. The search bar is shared by other applications too, I didn't take that into account. Then this needs more investigation.

@benakansara benakansara marked this pull request as draft April 25, 2023 20:55
@benakansara benakansara reopened this Apr 25, 2023
@benakansara
Copy link
Contributor Author

benakansara commented Apr 25, 2023

at a second look, I think the change only applies to the alert search bar in Observability Alerts and APM Services -> Alerts. @maryam-saeidi do you know if this component is used elsewhere?

@Zacqary can we safely remove onQueryChange from AlertsSearchBar?

@ersin-erdal
Copy link
Contributor

at a second look, I think the change only applies to the alert search bar in Observability Alerts and APM Services -> Alerts. @maryam-saeidi do you know if this component is used elsewhere?

@Zacqary can we safely remove onQueryChange from AlertsSearchBar?

I added it there because if a user changes the query and clicks directly on the save button it doesn't save the new value.
Without onChange method the only way to set the new value in the state is onBlur...

@benakansara
Copy link
Contributor Author

I added it there because if a user changes the query and clicks directly on the save button it doesn't save the new value. Without onChange method the only way to set the new value in the state is onBlur...

How can I verify if the new value is set correctly? I tested after removing onQueryChange. I can see the value updated in the query string when I click on Update/Refresh button or press Enter. It is not updating as I type (which was causing the error).

Screenshot 2023-04-26 at 11 06 14

@ersin-erdal
Copy link
Contributor

I added it there because if a user changes the query and clicks directly on the save button it doesn't save the new value. Without onChange method the only way to set the new value in the state is onBlur...

How can I verify if the new value is set correctly? I tested after removing onQueryChange. I can see the value updated in the query string when I click on Update/Refresh button or press Enter. It is not updating as I type (which was causing the error).

It's not happening on the "Alerts" page but on the new alerts filter query input. I think the same search component is used in the both components. On Alerts page it's used as a search bar (as it is developed for) but on actions tab of a rule it's used as a query input box...

@maryam-saeidi
Copy link
Member

maryam-saeidi commented Apr 26, 2023

@benakansara Places where we have an alert search bar are:

  1. Alerts page
  2. Rule details page > Alerts tab
  3. APM Services -> Alerts
  4. Hosts > Alerts tab sorry this has alerts table not search bar

@maryam-saeidi
Copy link
Member

It's not happening on the "Alerts" page but on the new alerts filter query input. I think the same search component is used in the both components. On Alerts page it's used as a search bar (as it is developed for) but on actions tab of a rule it's used as a query input box...

@ersin-erdal I didn't understand where you are referring to, where is actions tab of a rule ? Do you have a ticket or PR for the previous change that you mentioned here:

I added it there because if a user changes the query and clicks directly on the save button it doesn't save the new value.

@benakansara
Copy link
Contributor Author

@ersin-erdal tbh, I couldn't see any difference in actions tab of a rule after removing onQueryChange. Maybe I am missing something. Should something happen when I type in actions tab query filter?
I have modified the code to address this differently. Wdyt about adding onQuerySubmit in AlertsSearchBarProps as optional and making onQueryChange also optional?

@maryam-saeidi I think actions tab of a rule refers to the Security rules -

Screenshot 2023-04-26 at 12 40 49

@ersin-erdal
Copy link
Contributor

ersin-erdal commented Apr 26, 2023

@ersin-erdal tbh, I couldn't see any difference in actions tab of a rule after removing onQueryChange. Maybe I am missing something. Should something happen when I type in actions tab query filter?
I have modified the code to address this differently. Wdyt about adding onQuerySubmit in AlertsSearchBarProps as optional and making onQueryChange also optional?

let me quote my self :)

if a user changes the query and clicks directly on the save button it doesn't save the new value.

1- Create and save a rule with alerts filter query,
2- Edit rule by modifying the query and click on the save button directly, nowhere else before.

It doesn't save the value as we set the new value in the redux store onBlur. Kind of race condition, Save event happens before onBlur.

Please see the comment on this PR

I think we need a separate onChange function on action details to pass the value to redux store. It can be removed from the "Alerts" page.

@benakansara
Copy link
Contributor Author

Thank you for the explanation @ersin-erdal.

I think we need a separate onChange function on action details to pass the value to redux store. It can be removed from the "Alerts" page.

I think we are talking about same thing (adding onQuerySubmit in AlertsSearchBarProps for Obs Alerts and making onQueryChange as optional so it can be omitted from Obs Alerts, Actions filter will have both onQuerySubmit and onQueryChange).

@benakansara benakansara marked this pull request as ready for review April 26, 2023 15:27
Copy link
Contributor

@CoenWarmer CoenWarmer left a comment

Choose a reason for hiding this comment

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

Tested in Observability app. Works as expected 🥳

Copy link
Member

@maryam-saeidi maryam-saeidi left a comment

Choose a reason for hiding this comment

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

Nice job! 👍🏻

@XavierM XavierM added v8.9.0 bug Fixes for quality problems that affect the customer experience impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. labels May 2, 2023
@maryam-saeidi
Copy link
Member

maryam-saeidi commented May 2, 2023

@benakansara You don't need to use backport label for this PR (this label is automatically added to the related backport PRs). If you want to backport to the previous minor version (in this case v8.8) you can use backport:prev-minor label (and by default it will be merged to main which is v8.9)

Copy link
Contributor

@Zacqary Zacqary left a comment

Choose a reason for hiding this comment

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

Tested and works with Conditional Action

@benakansara benakansara enabled auto-merge (squash) May 2, 2023 17:07
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

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
triggersActionsUi 1.4MB 1.4MB +248.0B
Unknown metric groups

ESLint disabled line counts

id before after diff
enterpriseSearch 19 21 +2
securitySolution 398 401 +3
total +5

Total ESLint disabled count

id before after diff
enterpriseSearch 20 22 +2
securitySolution 478 481 +3
total +5

History

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

cc @benakansara

@benakansara benakansara added the release_note:skip Skip the PR/issue when compiling release notes label May 2, 2023
@benakansara benakansara merged commit 44c40a3 into elastic:main May 2, 2023
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request May 2, 2023
Fixes elastic#155655

Added `onQuerySubmit` ~as optional~ parameter to `AlertsSearchBar` and
made `onQueryChange` as optional parameter as this is not needed for
Observability Alerts.

### [Observability] Alerts page:

https://user-images.githubusercontent.com/69037875/234622864-c18e338c-2ea3-4c79-9340-30c5e8fa470e.mov

### [Security] Alerts filter in action:

https://user-images.githubusercontent.com/69037875/234623243-c882a866-83fa-4d09-a1fb-d36588922f30.mov
(cherry picked from commit 44c40a3)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.8

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

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request May 2, 2023
# Backport

This will backport the following commits from `main` to `8.8`:
- [Fix search issue with Alert search bar
(#155796)](#155796)

<!--- Backport version: 8.9.7 -->

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

<!--BACKPORT [{"author":{"name":"Bena
Kansara","email":"69037875+benakansara@users.noreply.github.com"},"sourceCommit":{"committedDate":"2023-05-02T18:34:29Z","message":"Fix
search issue with Alert search bar (#155796)\n\nFixes
#155655
`onQuerySubmit` ~as optional~ parameter to `AlertsSearchBar` and\r\nmade
`onQueryChange` as optional parameter as this is not needed
for\r\nObservability Alerts.\r\n\r\n### [Observability] Alerts
page:\r\n\r\n\r\nhttps://user-images.githubusercontent.com/69037875/234622864-c18e338c-2ea3-4c79-9340-30c5e8fa470e.mov\r\n\r\n###
[Security] Alerts filter in
action:\r\n\r\n\r\nhttps://user-images.githubusercontent.com/69037875/234623243-c882a866-83fa-4d09-a1fb-d36588922f30.mov","sha":"44c40a38e7dafe566823f56edb167d6b3b17a6f3","branchLabelMapping":{"^v8.9.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","release_note:skip","impact:high","Team:
Actionable
Observability","v8.8.0","v8.9.0"],"number":155796,"url":"#155796
search issue with Alert search bar (#155796)\n\nFixes
#155655
`onQuerySubmit` ~as optional~ parameter to `AlertsSearchBar` and\r\nmade
`onQueryChange` as optional parameter as this is not needed
for\r\nObservability Alerts.\r\n\r\n### [Observability] Alerts
page:\r\n\r\n\r\nhttps://user-images.githubusercontent.com/69037875/234622864-c18e338c-2ea3-4c79-9340-30c5e8fa470e.mov\r\n\r\n###
[Security] Alerts filter in
action:\r\n\r\n\r\nhttps://user-images.githubusercontent.com/69037875/234623243-c882a866-83fa-4d09-a1fb-d36588922f30.mov","sha":"44c40a38e7dafe566823f56edb167d6b3b17a6f3"}},"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":"#155796
search issue with Alert search bar (#155796)\n\nFixes
#155655
`onQuerySubmit` ~as optional~ parameter to `AlertsSearchBar` and\r\nmade
`onQueryChange` as optional parameter as this is not needed
for\r\nObservability Alerts.\r\n\r\n### [Observability] Alerts
page:\r\n\r\n\r\nhttps://user-images.githubusercontent.com/69037875/234622864-c18e338c-2ea3-4c79-9340-30c5e8fa470e.mov\r\n\r\n###
[Security] Alerts filter in
action:\r\n\r\n\r\nhttps://user-images.githubusercontent.com/69037875/234623243-c882a866-83fa-4d09-a1fb-d36588922f30.mov","sha":"44c40a38e7dafe566823f56edb167d6b3b17a6f3"}}]}]
BACKPORT-->

Co-authored-by: Bena Kansara <69037875+benakansara@users.noreply.github.com>
@benakansara benakansara deleted the bugfix/alert-search-bar branch October 23, 2023 07:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. release_note:skip Skip the PR/issue when compiling release notes Team: Actionable Observability - DEPRECATED For Observability Alerting and SLOs use "Team:obs-ux-management", for AIops "Team:obs-knowledge" v8.8.0 v8.9.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error thrown while typing in Alert page search bar
10 participants