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

Add saved queries to maps #44442

Merged
merged 25 commits into from
Sep 9, 2019

Conversation

TinaHeiligers
Copy link
Contributor

@TinaHeiligers TinaHeiligers commented Aug 29, 2019

Summary

Saved queries were introduced in 7.4 in applications that used the full search bar and, at that time, the scope was Discover, Dashboard and Visualize. Maps introduced filters during the course of saved queries development and was not within the original scope of the feature introduction. Now that the full search bar is used, this PR introduces "saved queries" to Maps.

What are saved queries?
From Implement saved queries and filters

Saved queries are a new saved object type similar to saved searches but more limited in scope. They allow users to store the the query string in the query bar and optionally the set of filters and timefilter in order to reload them anywhere a query is expected: Discover, Visualize, Dashboard, anywhere that uses our full SearchBar component.

saved_queries_in_maps_v2

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

- [] This was checked for cross-browser compatibility, including a check against IE11
- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
- [ ] Documentation was added for features that require explanation or tutorials

For maintainers

@TinaHeiligers TinaHeiligers added [Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.5.0 labels Aug 29, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-gis

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@nreese nreese 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 adding saved queries to Maps.

@Bargs Bargs added the v8.0.0 label Sep 3, 2019
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@TinaHeiligers
Copy link
Contributor Author

@Bargs do we need to add the v8.0.0 label to all new PR's from now? I thought that this work was targeted for v7.5.

@Bargs
Copy link
Contributor

Bargs commented Sep 3, 2019

@TinaHeiligers we always label PRs with every version that they will get merged into.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

$state.savedQuery = newSavedQuery.id;

if (newSavedQuery.id === (oldSavedQuery && oldSavedQuery.id)) {
updateStateFromSavedQuery(newSavedQuery);
Copy link
Contributor

@nreese nreese Sep 3, 2019

Choose a reason for hiding this comment

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

Calling updateStateFromSavedQuery inside the savedQuery watch results in dispatching onQueryChange multiple times. on-query-submit callback is called when saved query is saved. That triggers onQueryChange. Is there ever an instance where savedQuery changes and on-query-submit, on-refresh-change, or on-filters-updated has not already been triggered?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nreese after consulting with @Bargs, I deleted a redundant call to onQuerySubmit from the search bar, thereby reducing the number of time onQueryChange is called. @Bargs, please confirm the change is what we decided on as a solution.

Copy link
Contributor Author

@TinaHeiligers TinaHeiligers Sep 4, 2019

Choose a reason for hiding this comment

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

@nreese If I understand your comment, the trouble also lies with calling updateStateFromSavedQuery inside the watchers on $state.savedQuery and $scope.savedQuery. I'm looking into that too. Done: 1b8333f#diff-8f39ea21edecd35fad6f4c0df45681a3L195

Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't call the line removed in 1b8333f redundant. Things only work now because the watcher on $scope.savedQuery runs after the async request for the saved query completes. At that point the old and new saved query IDs are the same so the conditional passes and we call updateStateFromSavedQuery. But that's not why we added that conditional in the first place. It's an unintentional and confusing side effect and I don't think we should rely on it.

I was thinking about how we set up these watchers some more and I think we're unnecessarily triggering a request for the saved query from the server when we already have it in hand. That's ultimately causing the unnecessary extra call of updateStateFromSavedQuery as well. I didn't think I could explain this idea very well in text, so @TinaHeiligers I submitted a PR against your branch to demonstrate the change I'm suggesting.

https://github.com/TinaHeiligers/kibana/pull/4/files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Bargs the changes have been implemented and onQueryChange now only dispatches once per event.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would you mind applying these changes to Discover, Visualize, and Dashboard as well? updateStateFromSavedQuery also gets called twice in those apps, and this should solve it.

Copy link
Contributor Author

@TinaHeiligers TinaHeiligers Sep 6, 2019

Choose a reason for hiding this comment

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

@Bargs Sure, I'll create a new branch for that.
PR #45046

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@nreese nreese 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 adding Saved query support to Maps. This is going to be really useful

LGTM
Code review, tested in chrome

@TinaHeiligers TinaHeiligers merged commit 2541850 into elastic:master Sep 9, 2019
TinaHeiligers added a commit to TinaHeiligers/kibana that referenced this pull request Sep 9, 2019
* initial attempt at adding saved-queries to maps

* Adds savedQuery methods to map_controller

* Adds saveQuery to Maps ui

* Fixes watcher on state

* Fixes adding filters, still debuggin why the timefilter automatically changes to last 15 min on saving a query with a time filter. The time filter saves correctly in the saved query

* Gets time filter working with saved queries

* Adds saved query management component functionality tests to maps for all privileges

* Adds a saved query to the mapping and data for maps security functional tests

* Updates test saved query doc

* Adds functional tests for saved queries as a maps read-only user

* Adds 'skip' back to maps security functional tests

* uses onRefreshChange in map_controller

* Removes redundant trigger of onQuerySubmit call in the search bar

* deletes redundant call to updateStateFromSavedQuery in state.savedQuery watcher

* Refactors savedQuery watchers in map controller
TinaHeiligers added a commit that referenced this pull request Sep 9, 2019
* initial attempt at adding saved-queries to maps

* Adds savedQuery methods to map_controller

* Adds saveQuery to Maps ui

* Fixes watcher on state

* Fixes adding filters, still debuggin why the timefilter automatically changes to last 15 min on saving a query with a time filter. The time filter saves correctly in the saved query

* Gets time filter working with saved queries

* Adds saved query management component functionality tests to maps for all privileges

* Adds a saved query to the mapping and data for maps security functional tests

* Updates test saved query doc

* Adds functional tests for saved queries as a maps read-only user

* Adds 'skip' back to maps security functional tests

* uses onRefreshChange in map_controller

* Removes redundant trigger of onQuerySubmit call in the search bar

* deletes redundant call to updateStateFromSavedQuery in state.savedQuery watcher

* Refactors savedQuery watchers in map controller
@TinaHeiligers TinaHeiligers deleted the add-saved-queries-to-maps branch September 9, 2019 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Deprecated-Use Team:Presentation]Team:Geo Former Team Label for Geo Team. Now use Team:Presentation release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.5.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants