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

[Discover] Include current filters into "Test query" request #134184

Merged
merged 4 commits into from
Jun 14, 2022

Conversation

jughosta
Copy link
Contributor

@jughosta jughosta commented Jun 13, 2022

Closes #134180

Summary

This PR fixes "Test query" button when user changes current Rule filters.

It also removes number of matches from a previous test run if configuration (query, filter, etc) was updated. This way user would be confused whether it's a previous calculation or a new one.

Steps to test:

  1. Start creating an alert from Discover page
  2. Press "Test query" and note number of found matches
  3. Edit filters or query for the rule
  4. Notice that the previous number of found matches is removed
  5. Press "Test query" again to check if it made an effect on number of matches

@jughosta jughosta added Feature:Discover Discover Application release_note:fix Feature:Alerting auto-backport Deprecated: Automatically backport this PR after it's merged Team:DataDiscovery Discover App Team (Document Explorer, Saved Search, Surrounding documents, Graph) v8.3.0 v8.4.0 v8.3.1 labels Jun 13, 2022
@jughosta jughosta self-assigned this Jun 13, 2022
@jughosta jughosta marked this pull request as ready for review June 13, 2022 12:39
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

@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
stackAlerts 203.7KB 203.9KB +143.0B

History

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

cc @jughosta

@jughosta jughosta added this to PRs in Discover via automation Jun 13, 2022
@brianseeders brianseeders added backport:prev-minor Backport to the previous minor version (i.e. one version back from main) and removed auto-backport Deprecated: Automatically backport this PR after it's merged v8.3.0 v8.4.0 v8.3.1 labels Jun 13, 2022
Comment on lines +39 to +43
setTestQueryResponse({
result: null,
error: null,
isLoading: true,
});
Copy link
Member

Choose a reason for hiding this comment

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

👍 love this improvements!

testSearchSource.setField(
'filter',
timeFilter ? [timeFilter, ...ruleConfiguration.filter] : ruleConfiguration.filter
);
const { rawResponse } = await firstValueFrom(testSearchSource.fetch$());
Copy link
Member

Choose a reason for hiding this comment

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

qq @Dosant, unrelated to what we fix with this PR, but this should be changed to lastValueFrom, right?

Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

LGTM! Tested locally and it now works as expected. For this fix, since this part of the code didn't make it into production, I think it's fine to merge without a covering test (however, adding one on top would of course be 🍰 )

@kertal
Copy link
Member

kertal commented Jun 14, 2022

note that we might have to change the observable handling ... but this could also be done in a follow up

@jughosta jughosta merged commit 7b17093 into elastic:main Jun 14, 2022
Discover automation moved this from PRs to Done Jun 14, 2022
@jughosta jughosta deleted the 134032-saved-search-full-screen branch June 14, 2022 10:39
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.3

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 Jun 14, 2022
#134323)

* [Discover] Include current filters into "Test query" request

* [Discover] Reset query results when criteria is changed

* [Discover] Add a comment

* [Discover] Reset "Test query" when any rule configuration changes

(cherry picked from commit 7b17093)

Co-authored-by: Julia Rechkunova <julia.rechkunova@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to the previous minor version (i.e. one version back from main) Feature:Alerting Feature:Discover Discover Application release_note:fix Team:DataDiscovery Discover App Team (Document Explorer, Saved Search, Surrounding documents, Graph) v8.3.0 v8.4.0
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

[Discover] Adding/Changing a filter to an Elasticsearch query alert rule doesn't change test results
6 participants