From d853fcae11d2b9aa49c62e9d0c5ce20dd02f856a Mon Sep 17 00:00:00 2001 From: Stacey Gammon Date: Thu, 7 Sep 2017 08:43:19 -0400 Subject: [PATCH] Fix map updates not propagating to the dashboard (#13589) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Add failing tests * Add fix by preventing uiState from being directly updated in visualization. * Add test that would catch error caused by this PR in regards to filter agg * Fix issue with uiState triggering dirty dashboard state by introducing temporary "sessionState" on a vis * Click go after toggling the switch * add more tests to ensure getRequestAggs functions as intented * Go back to old zoom calculations. Update vis test data I think because mapCollar is no longer saved in uiState, the save recenters the data and we get slightly different data points from the test data. As far as my eye can tell, everything is working as intended. * fixes and tests - incorporate the new init function which fixes the bug where we lose map bounds data on a fresh save - add a test that would have caught that - adjust tests due to bug where map bounds is changing slightly. File another issue for that separately as it doesn’t actually affect the users map experience. * Fix tests Tests relied on my original logic of defaulting to the saved zoom state and not relying on uiState, so I went back to that logic. Also found another bug where mapZoom of 0 was being considered invalid, but it is actually a valid zoom level. * Since leaflet upgrade 'path.leaflet-clickable' can't be used to retrieve circles anymore * Avoid stale element reference I suspect because the page is changing, you have to keep fetching the element afresh. I don’t see this error on my local but saw it on jenkins. * remove spy select from PageObjects.visualize.getDataTableData The function is used in the Data Table visualization where the spy pane select doesn’t exist. --- .../public/req_resp_stats_spy_mode.html | 2 +- .../tile_map/public/kibana_map.js | 2 +- .../tile_map/public/maps_visualization.js | 13 +- .../agg_types/__tests__/buckets/_geo_hash.js | 192 ++++++++++++- src/ui/public/agg_types/buckets/geo_hash.js | 35 +-- .../public/agg_types/controls/precision.html | 1 + .../public/vis/editors/default/sidebar.html | 1 + src/ui/public/vis/vis.js | 4 + src/ui/public/visualize/spy.html | 1 + .../apps/dashboard/_dashboard_state.js | 48 ++++ test/functional/apps/dashboard/index.js | 1 + test/functional/apps/visualize/_tile_map.js | 257 ++++++++---------- .../functional/page_objects/dashboard_page.js | 5 + .../functional/page_objects/visualize_page.js | 60 ++-- test/functional/services/test_subjects.js | 5 + 15 files changed, 427 insertions(+), 200 deletions(-) create mode 100644 test/functional/apps/dashboard/_dashboard_state.js diff --git a/src/core_plugins/spy_modes/public/req_resp_stats_spy_mode.html b/src/core_plugins/spy_modes/public/req_resp_stats_spy_mode.html index c4a97803d2db..37321e173737 100644 --- a/src/core_plugins/spy_modes/public/req_resp_stats_spy_mode.html +++ b/src/core_plugins/spy_modes/public/req_resp_stats_spy_mode.html @@ -14,7 +14,7 @@ - 
{{req.fetchParams.body | json}}
+ 
{{req.fetchParams.body | json}}
diff --git a/src/core_plugins/tile_map/public/kibana_map.js b/src/core_plugins/tile_map/public/kibana_map.js index 958b16e0ca5b..a9959daee390 100644 --- a/src/core_plugins/tile_map/public/kibana_map.js +++ b/src/core_plugins/tile_map/public/kibana_map.js @@ -663,7 +663,7 @@ export class KibanaMap extends EventEmitter { if (!centerFromUIState || centerFromMap.lon !== centerFromUIState[1] || centerFromMap.lat !== centerFromUIState[0]) { visualization.uiStateVal('mapCenter', [centerFromMap.lat, centerFromMap.lon]); } - uiState.set('mapBounds', this.getUntrimmedBounds()); + visualization.sessionState.mapBounds = this.getUntrimmedBounds(); } this.on('dragend', persistMapStateInUiState); diff --git a/src/core_plugins/tile_map/public/maps_visualization.js b/src/core_plugins/tile_map/public/maps_visualization.js index 53a1b7746c06..4694afb1ad06 100644 --- a/src/core_plugins/tile_map/public/maps_visualization.js +++ b/src/core_plugins/tile_map/public/maps_visualization.js @@ -20,13 +20,11 @@ export function MapsVisualizationProvider(serviceSettings, Notifier, getAppState class MapsVisualization { constructor(element, vis) { - this.vis = vis; this.$el = $(element); this._$container = this.$el; this._geohashLayer = null; this._kibanaMap = null; - this._kibanaMapReady = this._makeKibanaMap(); this._baseLayerDirty = true; this._currentParams = null; } @@ -37,6 +35,11 @@ export function MapsVisualizationProvider(serviceSettings, Notifier, getAppState } } + init() { + this._kibanaMapReady = this._makeKibanaMap(); + return this._kibanaMapReady; + } + async render(esResponse, status) { return new Promise(async(resolve) => { @@ -66,7 +69,6 @@ export function MapsVisualizationProvider(serviceSettings, Notifier, getAppState //********************************************************************************************************** async _makeKibanaMap() { - try { this._tmsService = await serviceSettings.getTMSService(); this._tmsError = null; @@ -88,8 +90,8 @@ export function MapsVisualizationProvider(serviceSettings, Notifier, getAppState options.center = centerFromUIState ? centerFromUIState : this.vis.type.visConfig.defaults.mapCenter; this._kibanaMap = new KibanaMap(containerElement, options); - uiState.set('mapZoom', this._kibanaMap.getZoomLevel()); - uiState.set('mapBounds', this._kibanaMap.getUntrimmedBounds()); + this.vis.sessionState.mapBounds = this._kibanaMap.getUntrimmedBounds(); + this._kibanaMap.addDrawControl(); this._kibanaMap.addFitControl(); this._kibanaMap.addLegendControl(); @@ -170,7 +172,6 @@ export function MapsVisualizationProvider(serviceSettings, Notifier, getAppState * called on options change (vis.params change) */ async _updateParams() { - const mapParams = this._getMapsParams(); if (_.eq(this._currentParams, mapParams)) { return; diff --git a/src/ui/public/agg_types/__tests__/buckets/_geo_hash.js b/src/ui/public/agg_types/__tests__/buckets/_geo_hash.js index 4f5928689356..f4e89091c143 100644 --- a/src/ui/public/agg_types/__tests__/buckets/_geo_hash.js +++ b/src/ui/public/agg_types/__tests__/buckets/_geo_hash.js @@ -1,17 +1,48 @@ import expect from 'expect.js'; import { AggTypesBucketsGeoHashProvider } from 'ui/agg_types/buckets/geo_hash'; -describe('Geohash Agg', function () { +describe('Geohash Agg', () => { - // Mock BucketAggType that does not create an AggType, but instead returns the AggType parameters + const intialZoom = 10; + const initialMapBounds = { + top_left: { lat: 1.0, lon: -1.0 }, + bottom_right: { lat: -1.0, lon: 1.0 } + }; + const aggMock = { + getField: () => { + return { + name: 'location' + }; + }, + params: { + isFilteredByCollar: true, + useGeocentroid: true + }, + vis: { + hasUiState: () => { + return false; + }, + params: { + mapZoom: intialZoom + }, + sessionState: {} + }, + type: 'geohash_grid', + }; const BucketAggTypeMock = (aggOptions) => { - return aggOptions.params; + return aggOptions; + }; + const AggConfigMock = (vis, aggOptions) => { + return aggOptions; }; const PrivateMock = (provider) => { switch (provider.name) { case 'AggTypesBucketsBucketAggTypeProvider': return BucketAggTypeMock; break; + case 'VisAggConfigProvider': + return AggConfigMock; + break; default: return () => {}; } @@ -21,18 +52,54 @@ describe('Geohash Agg', function () { return 7;//"visualization:tileMap:maxPrecision" } }; - const params = AggTypesBucketsGeoHashProvider(PrivateMock, configMock); // eslint-disable-line new-cap - describe('precision parameter', function () { + function initVisSessionState() { + aggMock.vis.sessionState = { + mapBounds: initialMapBounds + }; + } + + function initAggParams() { + aggMock.params.isFilteredByCollar = true; + aggMock.params.useGeocentroid = true; + } + + function zoomMap(zoomChange) { + aggMock.vis.params.mapZoom += zoomChange; + } + + function moveMap(newBounds) { + aggMock.vis.sessionState.mapBounds = newBounds; + } + + function resetMap() { + aggMock.vis.params.mapZoom = intialZoom; + aggMock.vis.sessionState.mapBounds = initialMapBounds; + aggMock.vis.sessionState.mapCollar = { + top_left: { lat: 1.5, lon: -1.5 }, + bottom_right: { lat: -1.5, lon: 1.5 }, + zoom: intialZoom + }; + } + + let geohashAgg; + beforeEach(() => { + geohashAgg = AggTypesBucketsGeoHashProvider(PrivateMock, configMock); // eslint-disable-line new-cap + }); + + describe('precision parameter', () => { const PRECISION_PARAM_INDEX = 6; - const precisionParam = params[PRECISION_PARAM_INDEX]; + let precisionParam; + beforeEach(() => { + precisionParam = geohashAgg.params[PRECISION_PARAM_INDEX]; + }); it('should select precision parameter', () => { expect(precisionParam.name).to.equal('precision'); }); - describe('precision parameter write', function () { + describe('precision parameter write', () => { const zoomToGeoHashPrecision = { 0: 1, @@ -77,4 +144,115 @@ describe('Geohash Agg', function () { }); }); + + describe('getRequestAggs', () => { + + describe('initial aggregation creation', () => { + let requestAggs; + beforeEach(() => { + initVisSessionState(); + initAggParams(); + requestAggs = geohashAgg.getRequestAggs(aggMock); + }); + + it('should create filter, geohash_grid, and geo_centroid aggregations', () => { + expect(requestAggs.length).to.equal(3); + expect(requestAggs[0].type).to.equal('filter'); + expect(requestAggs[1].type).to.equal('geohash_grid'); + expect(requestAggs[2].type).to.equal('geo_centroid'); + }); + + it('should set mapCollar in vis session state', () => { + expect(aggMock.vis.sessionState).to.have.property('mapCollar'); + expect(aggMock.vis.sessionState.mapCollar).to.have.property('top_left'); + expect(aggMock.vis.sessionState.mapCollar).to.have.property('bottom_right'); + expect(aggMock.vis.sessionState.mapCollar).to.have.property('zoom'); + }); + + // there was a bug because of an "&& mapZoom" check which excluded 0 as a valid mapZoom, but it is. + it('should create filter, geohash_grid, and geo_centroid aggregations when zoom level 0', () => { + aggMock.vis.params.mapZoom = 0; + requestAggs = geohashAgg.getRequestAggs(aggMock); + expect(requestAggs.length).to.equal(3); + expect(requestAggs[0].type).to.equal('filter'); + expect(requestAggs[1].type).to.equal('geohash_grid'); + expect(requestAggs[2].type).to.equal('geo_centroid'); + }); + }); + + describe('aggregation options', () => { + + beforeEach(() => { + initVisSessionState(); + initAggParams(); + }); + + it('should only create geohash_grid and geo_centroid aggregations when isFilteredByCollar is false', () => { + aggMock.params.isFilteredByCollar = false; + const requestAggs = geohashAgg.getRequestAggs(aggMock); + expect(requestAggs.length).to.equal(2); + expect(requestAggs[0].type).to.equal('geohash_grid'); + expect(requestAggs[1].type).to.equal('geo_centroid'); + }); + + it('should only create filter and geohash_grid aggregations when useGeocentroid is false', () => { + aggMock.params.useGeocentroid = false; + const requestAggs = geohashAgg.getRequestAggs(aggMock); + expect(requestAggs.length).to.equal(2); + expect(requestAggs[0].type).to.equal('filter'); + expect(requestAggs[1].type).to.equal('geohash_grid'); + + }); + }); + + describe('aggregation creation after map interaction', () => { + + let origRequestAggs; + let origMapCollar; + beforeEach(() => { + resetMap(); + initAggParams(); + origRequestAggs = geohashAgg.getRequestAggs(aggMock); + origMapCollar = aggMock.vis.sessionState.mapCollar; + }); + + it('should not change geo_bounding_box filter aggregation and vis session state when map movement is within map collar', () => { + moveMap({ + top_left: { lat: 1.1, lon: -1.1 }, + bottom_right: { lat: -0.9, lon: 0.9 } + }); + + const newRequestAggs = geohashAgg.getRequestAggs(aggMock); + expect(JSON.stringify(origRequestAggs[0].params, null, '')).to.equal(JSON.stringify(newRequestAggs[0].params, null, '')); + + const newMapCollar = aggMock.vis.sessionState.mapCollar; + expect(JSON.stringify(origMapCollar, null, '')).to.equal(JSON.stringify(newMapCollar, null, '')); + }); + + it('should change geo_bounding_box filter aggregation and vis session state when map movement is outside map collar', () => { + moveMap({ + top_left: { lat: 10.0, lon: -10.0 }, + bottom_right: { lat: 9.0, lon: -9.0 } + }); + + const newRequestAggs = geohashAgg.getRequestAggs(aggMock); + expect(JSON.stringify(origRequestAggs[0].params, null, '')).not.to.equal(JSON.stringify(newRequestAggs[0].params, null, '')); + + const newMapCollar = aggMock.vis.sessionState.mapCollar; + expect(JSON.stringify(origMapCollar, null, '')).not.to.equal(JSON.stringify(newMapCollar, null, '')); + }); + + it('should change geo_bounding_box filter aggregation and vis session state when map zoom level changes', () => { + zoomMap(-1); + + const newRequestAggs = geohashAgg.getRequestAggs(aggMock); + expect(JSON.stringify(origRequestAggs[0].params, null, '')).not.to.equal(JSON.stringify(newRequestAggs[0].params, null, '')); + + const newMapCollar = aggMock.vis.sessionState.mapCollar; + expect(JSON.stringify(origMapCollar, null, '')).not.to.equal(JSON.stringify(newMapCollar, null, '')); + }); + + }); + + }); }); diff --git a/src/ui/public/agg_types/buckets/geo_hash.js b/src/ui/public/agg_types/buckets/geo_hash.js index 231f1c472ef5..cfdda6cd002e 100644 --- a/src/ui/public/agg_types/buckets/geo_hash.js +++ b/src/ui/public/agg_types/buckets/geo_hash.js @@ -45,6 +45,14 @@ export function AggTypesBucketsGeoHashProvider(Private, config) { return precision; } + function getMapZoom(vis) { + if (vis.hasUiState() && parseInt(vis.uiStateVal('mapZoom')) >= 0) { + return parseInt(vis.uiStateVal('mapZoom')); + } + + return vis.params.mapZoom; + } + function isOutsideCollar(bounds, collar) { return bounds && collar && !geoContains(collar, bounds); } @@ -89,11 +97,8 @@ export function AggTypesBucketsGeoHashProvider(Private, config) { }, write: function (aggConfig, output) { const vis = aggConfig.vis; - let currZoom; - if (vis.hasUiState()) { - currZoom = parseInt(vis.uiStateVal('mapZoom'), 10); - } - const autoPrecisionVal = zoomPrecision[currZoom >= 0 ? currZoom : parseInt(vis.params.mapZoom)]; + const currZoom = getMapZoom(vis); + const autoPrecisionVal = zoomPrecision[currZoom]; output.params.precision = aggConfig.params.autoPrecision ? autoPrecisionVal : getPrecision(aggConfig.params.precision); } } @@ -103,25 +108,23 @@ export function AggTypesBucketsGeoHashProvider(Private, config) { if (agg.params.isFilteredByCollar && agg.getField()) { const vis = agg.vis; - let mapBounds; - let mapZoom; - if (vis.hasUiState()) { - mapBounds = vis.uiStateVal('mapBounds'); - mapZoom = vis.uiStateVal('mapZoom'); - } - if (mapBounds && mapZoom) { - const lastMapCollar = vis.uiStateVal('mapCollar'); + const mapBounds = vis.sessionState.mapBounds; + const mapZoom = getMapZoom(vis); + if (mapBounds) { + const lastMapCollar = vis.sessionState.mapCollar; let mapCollar; if (!lastMapCollar || lastMapCollar.zoom !== mapZoom || isOutsideCollar(mapBounds, lastMapCollar)) { mapCollar = scaleBounds(mapBounds); mapCollar.zoom = mapZoom; - vis.getUiState().set('mapCollar', mapCollar); + vis.sessionState.mapCollar = mapCollar; } else { mapCollar = lastMapCollar; } const boundingBox = {}; - delete mapCollar.zoom; // zoom is not part of bounding box filter - boundingBox[agg.getField().name] = mapCollar; + boundingBox[agg.getField().name] = { + top_left: mapCollar.top_left, + bottom_right: mapCollar.bottom_right + }; aggs.push(new AggConfig(agg.vis, { type: 'filter', id: 'filter_agg', diff --git a/src/ui/public/agg_types/controls/precision.html b/src/ui/public/agg_types/controls/precision.html index 5a83f224aa51..c567f3d56593 100644 --- a/src/ui/public/agg_types/controls/precision.html +++ b/src/ui/public/agg_types/controls/precision.html @@ -40,6 +40,7 @@