Consolidate /app and /app/features endpoints

Experience has taught us that the client needs the session
data (current user, list of groups) and set of enabled feature
flags at the same time, and also needs to invalidate them
in the same scenarios (eg. account switching).

Fetching this data via two separate requests made it more complicated
to ensure the client had a consistent view of session and feature-flag
data. To simplify things and also same a network request:

 * On the server, put the feature flag data into the session data payload.

 * On the client, use the existing central storage and cache management
   for the session data payload to manage feature flags as well.

   The features service now becomes a thin wrapper around part of
   the session state.

h/static/scripts/app.coffee

@@ -6,9 +6,6 @@ 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]
@@ -81,8 +78,6 @@ setupHttp = ['$http', ($http) ->
 
 setupHost = ['host', (host) -> ]
 
-setupFeatures = ['features', (features) -> features.fetch()]
-
 module.exports = angular.module('h', [
   'angulartics'
   'angulartics.google.analytics'
@@ -170,7 +165,6 @@ module.exports = angular.module('h', [
 .config(configureRoutes)
 .config(configureTemplates)
 
-.run(setupFeatures)
 .run(setupCrossFrame)
 .run(setupHttp)
 .run(setupHost)

h/static/scripts/features.js

 /**
- * Feature flag client.
+ * Provides access to feature flag states for the current
+ * Hypothesis user.
  *
- * This is a small utility which will periodically retrieve the application
- * feature flags from a JSON endpoint in order to expose these to the
- * client-side application.
- *
- * All feature flags implicitly start toggled off. When `flagEnabled` is first
- * called (or alternatively when `fetch` is called explicitly) an XMLHTTPRequest
- * will be made to retrieve the current feature flag values from the server.
- * Once these are retrieved, `flagEnabled` will return current values.
- *
- * If `flagEnabled` is called and the cache is more than `CACHE_TTL`
- * milliseconds old, then it will trigger a new fetch of the feature flag
- * values. Note that this is again done asynchronously, so it is only later
- * calls to `flagEnabled` that will return the updated values.
+ * This service is a thin wrapper around the feature flag data in
+ * the session state.
  *
  * Users of this service should assume that the value of any given flag can
  * change at any time and should write code accordingly. Feature flags should
@@ -21,70 +11,35 @@
  */
 'use strict';
 
-var assign = require('core-js/modules/$.object-assign');
-
-var events = require('./events');
-
-var CACHE_TTL = 5 * 60 * 1000; // 5 minutes
-
 // @ngInject
-function features ($document, $http, $log, $rootScope) {
-  var cache = null;
-  var featuresUrl = new URL('/app/features', $document.prop('baseURI')).href;
-  var fetchOperation;
-
-  $rootScope.$on(events.USER_CHANGED, function () {
-    cache = null;
-  });
-
-  function fetch() {
-    if (fetchOperation) {
-      // fetch already in progress
-      return fetchOperation;
-    }
-
-    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 || (Date.now() - cache.updated) > CACHE_TTL) {
-      fetch();
-    }
-
-    if (!cache || !cache.flags) {
-      // a fetch is either in progress or fetching the feature flags
-      // failed
+function features($log, session) {
+  /**
+   * Returns true if the flag with the given name is enabled for the current
+   * user.
+   *
+   * Returns false if session data has not been fetched for the current
+   * user yet or if the feature flag name is unknown.
+   */
+  function flagEnabled(flag) {
+    // trigger a refresh of session data, if it has not been
+    // refetched within a cache timeout managed by the session service
+    // (see CACHE_TTL in session.js)
+    session.load();
+
+    if (!session.state.features) {
+      // features data has not yet been fetched
       return false;
     }
 
-    if (!cache.flags.hasOwnProperty(name)) {
-      $log.warn('features service: looked up unknown feature:', name);
+    var features = session.state.features;
+    if (!(flag in features)) {
+      $log.warn('looked up unknown feature', flag);
       return false;
     }
-    return cache.flags[name];
+    return features[flag];
   }
 
   return {
-    fetch: fetch,
     flagEnabled: flagEnabled
   };
 }

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

 'use strict';
 
-var mock = angular.mock;
-
-var events = require('../events');
+var features = require('../features');
 
 describe('h:features', function () {
-  var $httpBackend;
-  var $rootScope;
-  var features;
-  var sandbox;
-
-  before(function () {
-    angular.module('h', [])
-    .service('features', require('../features'));
-  });
-
-  beforeEach(mock.module('h'));
 
-  beforeEach(mock.module(function ($provide) {
-    sandbox = sinon.sandbox.create();
+  var fakeLog;
+  var fakeSession;
 
-    var fakeDocument = {
-      prop: sandbox.stub()
+  beforeEach(function () {
+    fakeLog = {
+      warn: sinon.stub(),
+    };
+    fakeSession = {
+      load: sinon.stub(),
+      state: {
+        features: {
+          'feature_on': true,
+          'feature_off': false,
+        },
+      },
     };
-    fakeDocument.prop.withArgs('baseURI').returns('http://foo.com/');
-    $provide.value('$document', fakeDocument);
-  }));
-
-  beforeEach(mock.inject(function ($injector) {
-    $httpBackend = $injector.get('$httpBackend');
-    $rootScope = $injector.get('$rootScope');
-    features = $injector.get('features');
-  }));
-
-  afterEach(function () {
-    $httpBackend.verifyNoOutstandingExpectation();
-    $httpBackend.verifyNoOutstandingRequest();
-    sandbox.restore();
-  });
-
-  function defaultHandler() {
-    var handler = $httpBackend.expect('GET', 'http://foo.com/app/features');
-    handler.respond(200, {foo: true, bar: false});
-    return handler;
-  }
-
-  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();
-    });
   });
 
   describe('flagEnabled', function () {
     it('should retrieve features data', function () {
-      defaultHandler();
-      features.flagEnabled('foo');
-      $httpBackend.flush();
+      var features_ = features(fakeLog, fakeSession);
+      assert.equal(features_.flagEnabled('feature_on'), true);
+      assert.equal(features_.flagEnabled('feature_off'), false);
     });
 
-    it('should return false initially', function () {
-      defaultHandler();
-      var result = features.flagEnabled('foo');
-      $httpBackend.flush();
-
-      assert.isFalse(result);
+    it('should return false if features have not been loaded', function () {
+      var features_ = features(fakeLog, fakeSession);
+      // simulate feature data not having been loaded yet
+      fakeSession.state = {};
+      assert.equal(features_.flagEnabled('feature_on'), false);
     });
 
-    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 trigger a refresh of session data', function () {
+      var features_ = features(fakeLog, fakeSession);
+      features_.flagEnabled('feature_on');
+      assert.calledOnce(fakeSession.load);
     });
 
     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();
+      var features_ = features(fakeLog, fakeSession);
+      assert.isFalse(features_.flagEnabled('unknown_feature'));
     });
   });
 });