Merge pull request #2684 from hypothesis/gh2675-wait_for_features

Wait for feature flag data before loading sidebar view

h/static/scripts/app.coffee

@@ -7,6 +7,9 @@ require('angular-jwt')
 streamer = require('./streamer')
 
 resolve =
+  # Ensure that we have feature flags available before we load the main
+  # view as features such as groups affect which annotations are loaded
+  featuresLoaded: ['features', (features) -> features.fetch()]
   # Ensure that we have available a) the current authenticated userid, and b)
   # the list of user groups.
   sessionState: ['session', (session) -> session.load().$promise]

h/static/scripts/features.js

@@ -21,58 +21,66 @@
  */
 'use strict';
 
-var retry = require('retry');
+var assign = require('core-js/modules/$.assign');
+
+var events = require('./events');
 
 var CACHE_TTL = 5 * 60 * 1000; // 5 minutes
 
-function features ($document, $http, $log) {
+// @ngInject
+function features ($document, $http, $log, $rootScope) {
   var cache = null;
-  var operation = null;
   var featuresUrl = new URL('/app/features', $document.prop('baseURI')).href;
+  var fetchOperation;
 
-  function fetch() {
-    // Short-circuit if a fetch is already in progress...
-    if (operation) {
-      return;
-    }
-    operation = retry.operation({retries: 10, randomize: true});
-
-    function success(data) {
-      cache = [Date.now(), data];
-      operation = null;
-    }
+  $rootScope.$on(events.USER_CHANGED, function () {
+    cache = null;
+  });
 
-    function failure(data, status) {
-      if (!operation.retry('failed to load - remote status was ' + status)) {
-        // All retries have failed, and we will now stop polling the endpoint.
-        $log.error('features service:', operation.mainError());
-      }
+  function fetch() {
+    if (fetchOperation) {
+      // fetch already in progress
+      return fetchOperation;
     }
 
-    operation.attempt(function () {
-      $http.get(featuresUrl)
-        .success(success)
-        .error(failure);
+    fetchOperation = $http.get(featuresUrl).then(function (response) {
+      cache = {
+        updated: Date.now(),
+        flags: response.data,
+      };
+    }).catch(function (err) {
+      // if for any reason fetching features fails, we behave as
+      // if all flags are turned off
+      $log.warn('failed to fetch feature data', err);
+      cache = assign({}, cache, {
+        updated: Date.now(),
+      });
+    }).finally(function () {
+      fetchOperation = null;
     });
+
+    return fetchOperation;
   }
 
   function flagEnabled(name) {
     // Trigger a fetch if the cache is more than CACHE_TTL milliseconds old.
     // We don't wait for the fetch to complete, so it's not this call that
     // will see new data.
-    if (cache === null || (Date.now() - cache[0]) > CACHE_TTL) {
+    if (!cache || (Date.now() - cache.updated) > CACHE_TTL) {
       fetch();
     }
 
-    if (cache === null) {
+    if (!cache || !cache.flags) {
+      // a fetch is either in progress or fetching the feature flags
+      // failed
       return false;
     }
-    var flags = cache[1];
-    if (!flags.hasOwnProperty(name)) {
+
+    if (!cache.flags.hasOwnProperty(name)) {
       $log.warn('features service: looked up unknown feature:', name);
       return false;
     }
-    return flags[name];
+    return cache.flags[name];
   }
 
   return {
@@ -81,4 +89,4 @@ function features ($document, $http, $log) {
   };
 }
 
-module.exports = ['$document', '$http', '$log', features];
+module.exports = features;

h/static/scripts/test/features-test.js

-"use strict";
+'use strict';
 
 var mock = angular.mock;
 
+var events = require('../events');
+
 describe('h:features', function () {
   var $httpBackend;
+  var $rootScope;
   var features;
   var sandbox;
 
@@ -26,6 +29,7 @@ describe('h:features', function () {
 
   beforeEach(mock.inject(function ($injector) {
     $httpBackend = $injector.get('$httpBackend');
+    $rootScope = $injector.get('$rootScope');
     features = $injector.get('features');
   }));
 
@@ -41,71 +45,101 @@ describe('h:features', function () {
     return handler;
   }
 
-  it('fetch should retrieve features data', function () {
-    defaultHandler();
-    features.fetch();
-    $httpBackend.flush();
-  });
-
-  it('fetch should not explode for errors fetching features data', function () {
-    defaultHandler().respond(500, "ASPLODE!");
-    features.fetch();
-    $httpBackend.flush();
-  });
-
-  it('fetch should only send one request at a time', function () {
-    defaultHandler();
-    features.fetch();
-    features.fetch();
-    $httpBackend.flush();
-  });
-
-  it('flagEnabled should retrieve features data', function () {
-    defaultHandler();
-    features.flagEnabled('foo');
-    $httpBackend.flush();
-  });
-
-  it('flagEnabled should return false initially', function () {
-    defaultHandler();
-    var result = features.flagEnabled('foo');
-    $httpBackend.flush();
-
-    assert.isFalse(result);
+  describe('fetch', function() {
+    it('should retrieve features data', function () {
+      defaultHandler();
+      features.fetch();
+      $httpBackend.flush();
+      assert.equal(features.flagEnabled('foo'), true);
+    });
+
+    it('should return a promise', function () {
+      defaultHandler();
+      features.fetch().then(function () {
+        assert.equal(features.flagEnabled('foo'), true);
+      });
+      $httpBackend.flush();
+    });
+
+    it('should not explode for errors fetching features data', function () {
+      defaultHandler().respond(500, "ASPLODE!");
+      var handler = sinon.stub();
+      features.fetch().then(handler);
+      $httpBackend.flush();
+      assert.calledOnce(handler);
+    });
+
+    it('should only send one request at a time', function () {
+      defaultHandler();
+      features.fetch();
+      features.fetch();
+      $httpBackend.flush();
+    });
   });
 
-  it('flagEnabled should return flag values when data is loaded', function () {
-    defaultHandler();
-    features.fetch();
-    $httpBackend.flush();
-
-    var foo = features.flagEnabled('foo');
-    assert.isTrue(foo);
-
-    var bar = features.flagEnabled('bar');
-    assert.isFalse(bar);
-  });
-
-  it('flagEnabled should return false for unknown flags', function () {
-    defaultHandler();
-    features.fetch();
-    $httpBackend.flush();
-
-    var baz = features.flagEnabled('baz');
-    assert.isFalse(baz);
-  });
-
-  it('flagEnabled should trigger a new fetch after cache expiry', function () {
-    var clock = sandbox.useFakeTimers();
-
-    defaultHandler();
-    features.flagEnabled('foo');
-    $httpBackend.flush();
-
-    clock.tick(301 * 1000);
-
-    defaultHandler();
-    features.flagEnabled('foo');
-    $httpBackend.flush();
+  describe('flagEnabled', function () {
+    it('should retrieve features data', function () {
+      defaultHandler();
+      features.flagEnabled('foo');
+      $httpBackend.flush();
+    });
+
+    it('should return false initially', function () {
+      defaultHandler();
+      var result = features.flagEnabled('foo');
+      $httpBackend.flush();
+
+      assert.isFalse(result);
+    });
+
+    it('should return flag values when data is loaded', function () {
+      defaultHandler();
+      features.fetch();
+      $httpBackend.flush();
+
+      var foo = features.flagEnabled('foo');
+      assert.isTrue(foo);
+
+      var bar = features.flagEnabled('bar');
+      assert.isFalse(bar);
+    });
+
+    it('should return false for unknown flags', function () {
+      defaultHandler();
+      features.fetch();
+      $httpBackend.flush();
+
+      var baz = features.flagEnabled('baz');
+      assert.isFalse(baz);
+    });
+
+    it('should trigger a new fetch after cache expiry', function () {
+      var clock = sandbox.useFakeTimers();
+
+      defaultHandler();
+      features.flagEnabled('foo');
+      $httpBackend.flush();
+
+      clock.tick(301 * 1000);
+
+      defaultHandler();
+      features.flagEnabled('foo');
+      $httpBackend.flush();
+    });
+
+    it('should clear the features data when the user changes', function () {
+      // fetch features and check that the flag is set
+      defaultHandler();
+      features.fetch();
+      $httpBackend.flush();
+      assert.isTrue(features.flagEnabled('foo'));
+
+      // simulate a change of logged-in user which should clear
+      // the features cache
+      $rootScope.$broadcast(events.USER_CHANGED, {});
+      defaultHandler();
+      assert.isFalse(features.flagEnabled('foo'));
+      $httpBackend.flush();
+    });
   });
 });

package.json

@@ -44,7 +44,6 @@
     "node-uuid": "^1.4.3",
     "postcss": "^5.0.6",
     "raf": "^3.1.0",
-    "retry": "^0.8.0",
     "scroll-into-view": "^1.3.1",
     "showdown": "^1.2.1",
     "uglify-js": "^2.4.14",