Skip to content

Fix saved query in app state in discvoer#47849

Merged
flash1293 merged 3 commits intoelastic:masterfrom
flash1293:flash1293/saved-query-load-fix
Oct 16, 2019
Merged

Fix saved query in app state in discvoer#47849
flash1293 merged 3 commits intoelastic:masterfrom
flash1293:flash1293/saved-query-load-fix

Conversation

@flash1293
Copy link
Contributor

Fixes #46363

This bug got introduced in 0656e4f because it truthy-checks $scope.savedQuery before loading a query which never gets populated because it references a $route.current.locals.savedQuery that never gets resolved (line 223) - this line seems like a leftover to me.

The fix in this PR solves the problem by fixing the condition when loading the query and making sure the search on initial page load is not triggered if a saved query is referenced because it will be loaded async and trigger a search itself.

There is another option to implement this - it would also be possible to resolve the query like index pattern and saved search are resolved as part of the resolve prop of the route specification. I didn't chose that way because it relies on the angular way of doing things and might be more difficult to clean up later on - I expect this controller will be broken down and migrated away from angular in the near future anyway. Maybe there is a good reason to follow through with the initial implementation approach, happy to change this.

@flash1293 flash1293 added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.5.0 labels Oct 10, 2019
@elasticmachine
Copy link
Contributor

💔 Build Failed

@TinaHeiligers
Copy link
Contributor

@flash1293 this fix works, thanks for that. As for the approach, I'm not sure if relying on Angular's way of doing things is better because, as you mention, it might make clean up later difficult.

@flash1293 flash1293 marked this pull request as ready for review October 11, 2019 10:31
@flash1293
Copy link
Contributor Author

Jenkins, test this.

@flash1293 flash1293 added the Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// label Oct 11, 2019
@flash1293 flash1293 requested a review from Bargs October 11, 2019 10:31
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@TinaHeiligers TinaHeiligers 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 Chrome and it works as expected.
Code looks fine.
LGTM!

Copy link
Contributor

@Bargs Bargs left a comment

Choose a reason for hiding this comment

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

Fix looks good!

We have the same code in visualize and dashboard, do you mind updating those as well?

if ($scope.savedQuery && newSavedQueryId !== $scope.savedQuery.id) {
if ($scope.savedQuery && newSavedQueryId !== $scope.savedQuery.id) {

@flash1293 flash1293 requested a review from Bargs October 14, 2019 12:35
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@Bargs Bargs left a comment

Choose a reason for hiding this comment

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

LGTM

@flash1293 flash1293 merged commit c39eee8 into elastic:master Oct 16, 2019
@flash1293 flash1293 deleted the flash1293/saved-query-load-fix branch October 16, 2019 07:35
flash1293 added a commit to flash1293/kibana that referenced this pull request Oct 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// v7.5.0 v8.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Saved query not loaded from Management -> Saved Objects page

4 participants