Merge pull request #233 from spenceralger/fix/discover_fetch

Fix discover fetch

TODOS.md

@@ -5,9 +5,9 @@
    - change this from event based to calling a method on dashboardApp – [L68](https://github.com/elasticsearch/kibana4/blob/master/src/kibana/apps/dashboard/directives/grid.js#L68)
  - **src/kibana/apps/discover/controllers/discover.js**
    - Switch this to watching time.string when we implement it – [L148](https://github.com/elasticsearch/kibana4/blob/master/src/kibana/apps/discover/controllers/discover.js#L148)
-   - On array fields, negating does not negate the combination, rather all terms – [L431](https://github.com/elasticsearch/kibana4/blob/master/src/kibana/apps/discover/controllers/discover.js#L431)
-   - Move to utility class – [L496](https://github.com/elasticsearch/kibana4/blob/master/src/kibana/apps/discover/controllers/discover.js#L496)
+   - On array fields, negating does not negate the combination, rather all terms – [L435](https://github.com/elasticsearch/kibana4/blob/master/src/kibana/apps/discover/controllers/discover.js#L435)
    - Move to utility class – [L506](https://github.com/elasticsearch/kibana4/blob/master/src/kibana/apps/discover/controllers/discover.js#L506)
+   - Move to utility class – [L516](https://github.com/elasticsearch/kibana4/blob/master/src/kibana/apps/discover/controllers/discover.js#L516)
  - **src/kibana/apps/settings/sections/indices/_create.js**
    - we should probably display a message of some kind – [L111](https://github.com/elasticsearch/kibana4/blob/master/src/kibana/apps/settings/sections/indices/_create.js#L111)
  - **src/kibana/apps/visualize/controllers/editor.js**

src/kibana/apps/discover/controllers/discover.js

@@ -53,6 +53,7 @@ define(function (require) {
 
     var segmentedFetch = $scope.segmentedFetch = Private(require('apps/discover/_segmented_fetch'));
     var HitSortFn = Private(require('apps/discover/_hit_sort_fn'));
+    var diffTimePickerValues = Private(require('utils/diff_time_picker_vals'));
 
     var notify = new Notifier({
       location: 'Discover'
@@ -129,8 +130,6 @@ define(function (require) {
     var init = _.once(function () {
       return $scope.updateDataSource()
       .then(function () {
-        setFields();
-
         // state fields that shouldn't trigger a fetch when changed
         var ignoreStateChanges = ['columns'];
 
@@ -148,8 +147,7 @@ define(function (require) {
 
         // TODO: Switch this to watching time.string when we implement it
         $scope.$watchCollection('globalState.time', function (newTime, oldTime) {
-          // don't fetch unless there was a previous value and the values are not loosly equal
-          if (!_.isUndefined(oldTime) && !angular.equals(newTime, oldTime)) $scope.fetch();
+          if (diffTimePickerValues(newTime, oldTime)) $scope.fetch();
         });
 
         $scope.$watch('state.sort', function (sort) {
@@ -205,7 +203,6 @@ define(function (require) {
 
       $scope.updateTime();
       if (_.isEmpty($state.columns)) refreshColumns();
-      $state.save();
       $scope.updateDataSource()
       .then(setupVisualization)
       .then(function () {
@@ -415,7 +412,6 @@ define(function (require) {
       $scope.fields = [];
       $scope.fieldsByName = {};
       $scope.formatsByName = {};
-      $state.columns = $state.columns || [];
 
       if (!indexPattern) return;
 

src/kibana/utils/diff_object.js

@@ -38,7 +38,7 @@ define(function (require) {
 
     // Find the keys that will be changed
     diff.changed = _.filter(sourceKeys, function (key) {
-      return !angular.equals(target[key]);
+      return !angular.equals(target[key], source[key]);
     });
 
     // Make a list of all the keys that are changing

src/kibana/utils/diff_time_picker_vals.js

@@ -0,0 +1,25 @@
+define(function (require) {
+  return function DiffTimePickerValuesFn() {
+    var _ = require('lodash');
+    var angular = require('angular');
+
+    var valueOf = function (o) {
+      if (o) return o.valueOf();
+    };
+
+    return function (rangeA, rangeB) {
+      if (_.isObject(rangeA) && _.isObject(rangeB)) {
+        if (
+          valueOf(rangeA.to) !== valueOf(rangeB.to)
+          || valueOf(rangeA.from) !== valueOf(rangeB.from)
+        ) {
+          return true;
+        }
+      } else {
+        return !angular.equals(rangeA, rangeB);
+      }
+
+      return false;
+    };
+  };
+});

test/unit/index.html

@@ -77,6 +77,7 @@
           'specs/factories/base_object',
           'specs/state_management/state',
           'specs/utils/diff_object',
+          'specs/utils/diff_time_picker_vals',
           'specs/factories/events'
         ], function (kibana, sinon) {
           kibana.load(function () {

test/unit/specs/utils/diff_object.js

@@ -57,5 +57,12 @@ define(function (require) {
       expect(target).to.not.have.property('$private');
     });
 
+    it('should not list any changes for similar objects', function () {
+      var target = { foo: 'bar', test: 'foo' };
+      var source = { foo: 'bar', test: 'foo', $private: 'foo' };
+      var results = diff(target, source);
+      expect(results.changed).to.be.empty();
+    });
+
   });
 });

test/unit/specs/utils/diff_time_picker_vals.js

@@ -0,0 +1,111 @@
+define(function (require) {
+  var moment = require('moment');
+
+  require('angular').module('DiffTimePickerValues', ['kibana']);
+
+  describe('Diff Time Picker Values', function () {
+    var diffTimePickerValues;
+
+    beforeEach(module('DiffTimePickerValues'));
+    beforeEach(inject(function (Private) {
+      diffTimePickerValues = Private(require('utils/diff_time_picker_vals'));
+    }));
+
+    it('accepts two undefined values', function () {
+      var diff = diffTimePickerValues(undefined, undefined);
+      expect(diff).to.be(false);
+    });
+
+    describe('datemath ranges', function () {
+      it('knows a match', function () {
+        var diff = diffTimePickerValues(
+          {
+            to: 'now',
+            from: 'now-7d'
+          },
+          {
+            to: 'now',
+            from: 'now-7d'
+          }
+        );
+
+        expect(diff).to.be(false);
+      });
+      it('knows a difference', function () {
+        var diff = diffTimePickerValues(
+          {
+            to: 'now',
+            from: 'now-7d'
+          },
+          {
+            to: 'now',
+            from: 'now-1h'
+          }
+        );
+
+        expect(diff).to.be(true);
+      });
+    });
+
+    describe('a datemath range, and a moment range', function () {
+      it('is always different', function () {
+        var diff = diffTimePickerValues(
+          {
+            to: moment(),
+            from: moment()
+          },
+          {
+            to: 'now',
+            from: 'now-1h'
+          }
+        );
+
+        expect(diff).to.be(true);
+      });
+    });
+
+    describe('moment ranges', function () {
+      it('uses the time value of moments for comparison', function () {
+        var to = moment();
+        var from = moment().add(1, 'day');
+
+        var diff = diffTimePickerValues(
+          {
+            to: to.clone(),
+            from: from.clone()
+          },
+          {
+            to: to.clone(),
+            from: from.clone()
+          }
+        );
+
+        expect(diff).to.be(false);
+      });
+
+      it('fails if any to or from is different', function () {
+        var to = moment();
+        var from = moment().add(1, 'day');
+        var from2 = moment().add(2, 'day');
+
+        var diff = diffTimePickerValues(
+          {
+            to: to.clone(),
+            from: from.clone()
+          },
+          {
+            to: to.clone(),
+            from: from2.clone()
+          }
+        );
+
+        expect(diff).to.be(true);
+      });
+    });
+
+    it('does not fall apart with unusual values', function () {
+      var diff = diffTimePickerValues({}, {});
+      expect(diff).to.be(false);
+    });
+  });
+});