[lens] Use top nav in Lens app #46190

wylieconlon · 2019-09-19T21:04:42Z

Integrates the Top Nav component and saved queries into Lens. Also implements basic "read-only" permissioning using feature-level security.

With all permissions, the user can save the query and the entire visualization:

With read-only permissions the user can use saved queries, and can run visualizations, but cannot save them:

elasticmachine · 2019-09-19T22:41:38Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

TinaHeiligers

Made comments where the saved query addition assumptions are incorrect.

TinaHeiligers · 2019-09-20T16:37:47Z

src/legacy/core_plugins/data/public/search/search_bar/components/search_bar.tsx

  onSavedQueryUpdated?: (savedQuery: SavedQuery) => void;
+  // User has cleared the active query, which should not clear filters


All the filters are removed when an active saved query is loaded. This includes those that one might have added in addition to the filters that come from the active saved query.

TinaHeiligers · 2019-09-20T16:45:35Z

x-pack/legacy/plugins/lens/public/app_plugin/app.tsx

+                ...state,
+                savedQuery,
+                filters: savedQuery.attributes.filters || state.filters,
+                query: savedQuery.attributes.query || state.query,


A saved query will always have a query, regardless of if one was added or not. If a saved query only has contents for filters and/or a time filter and no query was added in the query bar, then the saved query will have an empty string as the query bar input. This means that savedQuery.attributes.query || state.query will always be truthy because savedQuery.attributes.query will be true.

TinaHeiligers

Some assumptions about saved queries are incorrect:

When an active saved query is cleared, all the filters are removed, regardless of whether they come from the saved query or not. On clearing a saved query, the filters are replaced with an empty array.
A saved query will always contain a query, even if it is an empty one.

elasticmachine · 2019-09-24T00:22:39Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-09-24T20:36:19Z

Pinging @elastic/kibana-app

elasticmachine · 2019-09-24T22:12:22Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

lizozom

Added a few comments on data plugin usage

lizozom · 2019-09-25T07:10:35Z

x-pack/legacy/plugins/lens/public/app_plugin/app.tsx

+            }}
+            query={state.query}
+            dateRangeFrom={state.dateRange.fromDate}
+            dateRangeTo={state.dateRange.toDate}
            toasts={core.notifications.toasts}
            uiSettings={core.uiSettings}
            savedObjectsClient={savedObjectsClient}


You should pass core.savedObjects.client in here.

x-pack/legacy/plugins/lens/public/app_plugin/app.tsx

lizozom · 2019-09-25T07:14:51Z

x-pack/legacy/plugins/lens/public/app_plugin/app.tsx

+  SavedQuery,
+} from 'src/legacy/core_plugins/data/public';
+import { Filter } from '@kbn/es-query';
+import { TopNavMenu } from '../../../../../../src/legacy/core_plugins/kibana_react/public';
 import { Query } from '../../../../../../src/legacy/core_plugins/data/public/query';


You can do this import from data/public (without query in the end, which won't work in NP)

x-pack/legacy/plugins/lens/public/app_plugin/plugin.tsx

lizozom · 2019-09-25T07:18:51Z

x-pack/legacy/plugins/lens/public/app_plugin/plugin.tsx

@@ -60,6 +65,7 @@ export class AppPlugin {
      return (
        <App
          core={core}
+          data={plugins.data}
          editorFrame={this.instance!}
          store={new Storage(localStorage)}
          savedObjectsClient={chrome.getSavedObjectsClient()}


Core.savedObjects.client

wylieconlon · 2019-09-25T13:29:11Z

x-pack/legacy/plugins/lens/public/app_plugin/app.tsx

            toasts={core.notifications.toasts}
            uiSettings={core.uiSettings}
            savedObjectsClient={savedObjectsClient}
            http={core.http}
+            timeHistory={data.timefilter.history}


@lizozom We're using the time history here,

x-pack/legacy/plugins/lens/public/app_plugin/app.tsx

x-pack/legacy/plugins/lens/public/app_plugin/plugin.tsx

TinaHeiligers

The top nav works as expected when using saved queries, the normal query bar and the date picker. Good job there!
Unfortunately, the top nav filters are not updating the data source as one would expect. There's also no indication in the search bar that the data source has not changed if a filter is added/edited or otherwise changed:

I went digging into the code and tried to work out which effect wasn't triggering when the data source should be changing but couldn't track down where the issue is.
Maybe we can somehow let users know that filter changes don't effect the data source automatically?

TinaHeiligers · 2019-10-01T21:46:20Z

x-pack/legacy/plugins/lens/public/indexpattern_plugin/field_item.tsx

@@ -111,7 +112,7 @@ export function FieldItem(props: FieldItemProps) {
    core.http
      .post(`/api/lens/index_stats/${indexPattern.title}/field`, {
        body: JSON.stringify({
-          query: toElasticsearchQuery(query, indexPattern),
+          dslQuery: buildEsQuery(indexPattern, query, filters),


buildEsQuery also takes in an optional config that allows us to use the global Kibana query language (either KQL or lucene) and the timezone users choose to set in their Kibana advanced settings. We have access to those from KibanaContextProvider services. There are defaults though, so it's optional.

Adding the buildEsQuery call here.

elasticmachine · 2019-10-02T03:46:06Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

TinaHeiligers

Filters are almost working now, I can add a filter and the update button shows. Removing a filter triggers an update automatically and some of the filter editor methods also trigger an automatic update.
However, adding a new filter only fetches fields from my default index pattern and not the one selected for the vis. There's no way of changing the index pattern from which to select a new filters' fields.
In the screen shot below, the index pattern for the vis is kibana_sample_data_ecommerce but the fields for the new filter are only from kibana_sample_data_logs.

wylieconlon · 2019-10-02T17:02:32Z

@TinaHeiligers That's not what I'm seeing when I have two layers:

elasticmachine · 2019-10-02T17:51:50Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

chrisdavies

LGTM w/ possible tweaks. The tweaks aren't necessary.

chrisdavies · 2019-10-02T18:13:54Z

x-pack/legacy/plugins/lens/public/app_plugin/app.tsx

+                          }
+                        })
+                        .catch(reason => {
+                          core.notifications.toasts.addDanger(


You can use addError instead, which takes the error object directly. I think that would probably be preferable here.

Oh I think you found a potential issue- I don't think these are human-readable error messages, so showing "Error saving document" was my attempt to humanize it. There's a test that verifies that if there's a save error it lets you retry.

chrisdavies · 2019-10-02T18:15:07Z

x-pack/legacy/plugins/lens/public/app_plugin/plugin.tsx

 }
 export class AppPlugin {
  private instance: EditorFrameInstance | null = null;
  private store: SavedObjectIndexStore | null = null;

  constructor() {}

-  setup(core: CoreSetup) {
+  setup(core: CoreSetup, plugins: {}) {


What is the point of this second argument?

This is a bit closer to what the new platform will do. @lizozom has been moving code out of the legacy data plugin, and into a new platform data plugin, and we will soon need to pass both in.

TinaHeiligers · 2019-10-02T19:03:05Z

@TinaHeiligers That's not what I'm seeing when I have two layers:

Yes, with two layers, we get to choose the index pattern for the filter fields.
The problem is when we only have one layer. In this case, regardless of if we have more than one index pattern loaded, there isn't an option to choose the index pattern from which to select filter fields.
To reproduce:

Load kibana_sample_data_logs, kibana_sample_data_ecommerce and kibana_sample_data_flights
create a new Lens visualization
Add data to the vis in a single layer. Do not add a second layer
add a new filter from the search bar. Observe that one does not get the option to select an index pattern and that the fields available to create a filter from are from the default index pattern.

wylieconlon · 2019-10-02T19:57:01Z

@TinaHeiligers I still can't reproduce the behavior you're describing with a non-default index pattern.

TinaHeiligers

Adding a new filter with only one layer in the vis built from data that's not the default index pattern now works perfectly.
LGTM after adding the test for the change that fixed the problem.

elasticmachine · 2019-10-02T21:34:53Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 3bf24ef

elasticmachine · 2019-10-02T23:01:09Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: dcd2495

wylieconlon · 2019-10-02T23:48:29Z

retest

elasticmachine · 2019-10-03T01:01:28Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: dcd2495

wylieconlon · 2019-10-03T01:07:37Z

There was a flaky ML test failure, it's unrelated to my changes

lizozom · 2019-10-03T14:16:33Z

x-pack/legacy/plugins/lens/public/app_plugin/app.tsx

              }}
-              isDirty={isLocalStateDirty(state.localQueryBarState, state.query, state.dateRange)}
-              indexPatterns={state.indexPatternTitles}
+              appName={'lens'}


//nit
showQueryBar, showFilterBar and showDatePicker are true by default

lizozom

LGTM in terms of TopNavMenu integration

* [lens] Use top nav in Lens app * Add tests for saved query, pass filters around more places * Fix filter passing * Add unit test for field popover making correct queries * Respond to review feedback * Fix type errors * Respond to all review comments * Remove commented code * Top nav should be compatible as angular directive * Fix rendering issue with filter updates * Respond to review comments and add onChange test * Add specific test for the index pattern bug from Tina

[lens] Use top nav in Lens app

27b6824

wylieconlon added the Feature:Lens label Sep 19, 2019

wylieconlon requested a review from TinaHeiligers September 19, 2019 21:04

TinaHeiligers reviewed Sep 20, 2019

View reviewed changes

TinaHeiligers requested changes Sep 20, 2019

View reviewed changes

wylieconlon added 2 commits September 23, 2019 13:54

Merge remote-tracking branch 'origin/master' into lens/use-top-nav

9ace121

Add tests for saved query, pass filters around more places

2c6817c

wylieconlon added 5 commits September 24, 2019 11:56

Merge remote-tracking branch 'origin/master' into lens/use-top-nav

d69977c

Fix filter passing

a787f94

Merge remote-tracking branch 'origin/master' into lens/use-top-nav

cfd7b8b

Add unit test for field popover making correct queries

761be14

Merge remote-tracking branch 'origin/master' into lens/use-top-nav

9aad4b8

wylieconlon added release_note:skip Skip the PR/issue when compiling release notes v7.5.0 v8.0.0 labels Sep 24, 2019

wylieconlon requested review from flash1293, chrisdavies and lizozom September 24, 2019 20:21

wylieconlon marked this pull request as ready for review September 24, 2019 20:21

wylieconlon requested review from a team and TinaHeiligers September 24, 2019 20:21

wylieconlon added the Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) label Sep 24, 2019

lizozom suggested changes Sep 25, 2019

View reviewed changes

wylieconlon commented Sep 25, 2019

View reviewed changes

wylieconlon added 2 commits September 25, 2019 14:41

Respond to review feedback

0bf1b44

Merge remote-tracking branch 'origin/master' into lens/use-top-nav

588635e

TinaHeiligers requested changes Oct 1, 2019

View reviewed changes

Fix rendering issue with filter updates

0639138

wylieconlon requested a review from TinaHeiligers October 2, 2019 02:39

TinaHeiligers requested changes Oct 2, 2019

View reviewed changes

Merge remote-tracking branch 'origin/master' into lens/use-top-nav

449155f

wylieconlon requested a review from TinaHeiligers October 2, 2019 17:43

Merge remote-tracking branch 'origin/master' into lens/use-top-nav

85c089a

wylieconlon added release_note:skip Skip the PR/issue when compiling release notes and removed release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. labels Oct 2, 2019

chrisdavies approved these changes Oct 2, 2019

View reviewed changes

Respond to review comments and add onChange test

3bf24ef

TinaHeiligers approved these changes Oct 2, 2019

View reviewed changes

Add specific test for the index pattern bug from Tina

dcd2495

lizozom reviewed Oct 3, 2019

View reviewed changes

lizozom approved these changes Oct 3, 2019

View reviewed changes

wylieconlon merged commit ab05bbd into elastic:master Oct 3, 2019

wylieconlon deleted the lens/use-top-nav branch October 3, 2019 15:24

wylieconlon mentioned this pull request Oct 3, 2019

[7.x] [lens] Use top nav in Lens app (#46190) #47238

Merged

		onSavedQueryUpdated?: (savedQuery: SavedQuery) => void;
		// User has cleared the active query, which should not clear filters

[lens] Use top nav in Lens app #46190

[lens] Use top nav in Lens app #46190

Conversation

wylieconlon commented Sep 19, 2019 • edited Loading

elasticmachine commented Sep 19, 2019

💔 Build Failed

TinaHeiligers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TinaHeiligers left a comment

Choose a reason for hiding this comment

elasticmachine commented Sep 24, 2019

💔 Build Failed

elasticmachine commented Sep 24, 2019

elasticmachine commented Sep 24, 2019

💔 Build Failed

lizozom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TinaHeiligers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Oct 2, 2019

💚 Build Succeeded

TinaHeiligers left a comment

Choose a reason for hiding this comment

wylieconlon commented Oct 2, 2019

elasticmachine commented Oct 2, 2019

💚 Build Succeeded

chrisdavies left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TinaHeiligers commented Oct 2, 2019

wylieconlon commented Oct 2, 2019

TinaHeiligers left a comment

Choose a reason for hiding this comment

elasticmachine commented Oct 2, 2019

💚 Build Succeeded

elasticmachine commented Oct 2, 2019

💔 Build Failed

wylieconlon commented Oct 2, 2019

elasticmachine commented Oct 3, 2019

💚 Build Succeeded

wylieconlon commented Oct 3, 2019

Choose a reason for hiding this comment

lizozom left a comment

Choose a reason for hiding this comment

wylieconlon commented Sep 19, 2019 •

edited

Loading