Fix map updates not propagating to the dashboard (#13589)

* 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.

src/core_plugins/spy_modes/public/req_resp_stats_spy_mode.html

@@ -14,7 +14,7 @@
   <label>
     Elasticsearch request body &nbsp;
   </label>
-  <pre>{{req.fetchParams.body | json}}</pre>
+  <pre data-test-subj="visualizationEsRequestBody">{{req.fetchParams.body | json}}</pre>
 </div>
 
 <div ng-if="spy.mode.name === 'response'">

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);

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;

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, ''));
+      });
+
+    });
+
+  });
 });

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',

src/ui/public/agg_types/controls/precision.html

@@ -40,6 +40,7 @@
 <div class="vis-option-item indented">
   <label>
     <input type="checkbox"
+           data-test-subj="isFilteredByCollarCheckbox"
            name="isFilteredByCollar"
            ng-model="agg.params.isFilteredByCollar">
     Only request data around extent of map <kbn-info info="Apply geo_bounding_box filter aggregation to narrow the subject area to the map view box with collar."></kbn-info>

src/ui/public/vis/editors/default/sidebar.html

@@ -28,6 +28,7 @@
               ng-class="{'is-vis-editor-sub-nav-link-selected': sidebar.section == 'data'}"
               ng-click="sidebar.section='data'"
               kbn-accessible-click
+              data-test-subj="visualizeEditDataLink"
             >
               Data
             </a>

src/ui/public/vis/vis.js

@@ -45,6 +45,10 @@ export function VisProvider(Private, indexPatterns, timefilter, getAppState) {
       this.setState(this.getCurrentState(), false);
       this.setUiState(uiState);
 
+      // Session state is for storing information that is transitory, and will not be saved with the visualization.
+      // For instance, map bounds, which depends on the view port, browser window size, etc.
+      this.sessionState = {};
+
       this.API = {
         indexPatterns: indexPatterns,
         timeFilter: timefilter,

src/ui/public/visualize/spy.html

@@ -27,6 +27,7 @@
         <option
           ng-repeat="mode in modes.inOrder"
           value="{{mode.name}}"
+          data-test-subj="spyModeSelect-{{ mode.name }}"
         >
           {{mode.display}}
         </option>