Merge pull request #1004 from hypothesis/move-api-activity-state-to-store

Move API activity state to store

src/sidebar/components/search-input.js

 'use strict';
 
 // @ngInject
-function SearchInputController($element, $http, $scope) {
+function SearchInputController($element, store) {
   const self = this;
   const button = $element.find('button');
   const input = $element.find('input')[0];
@@ -11,20 +11,13 @@ function SearchInputController($element, $http, $scope) {
     input.focus();
   });
 
-  $scope.$watch(
-    function() {
-      return $http.pendingRequests.length;
-    },
-    function(count) {
-      self.loading = count > 0;
-    }
-  );
-
   form.onsubmit = function(e) {
     e.preventDefault();
     self.onSearch({ $query: input.value });
   };
 
+  this.isLoading = () => store.isLoading();
+
   this.inputClasses = function() {
     return { 'is-expanded': self.alwaysExpanded || self.query };
   };

src/sidebar/components/test/search-input-test.js

@@ -5,7 +5,7 @@ const angular = require('angular');
 const util = require('../../directive/test/util');
 
 describe('searchInput', function() {
-  let fakeHttp;
+  let fakeStore;
 
   before(function() {
     angular
@@ -14,9 +14,9 @@ describe('searchInput', function() {
   });
 
   beforeEach(function() {
-    fakeHttp = { pendingRequests: [] };
+    fakeStore = { isLoading: sinon.stub().returns(false) };
     angular.mock.module('app', {
-      $http: fakeHttp,
+      store: fakeStore,
     });
   });
 
@@ -45,17 +45,23 @@ describe('searchInput', function() {
   });
 
   describe('loading indicator', function() {
-    it('is hidden when there are no network requests in flight', function() {
+    it('is hidden when there are no API requests in flight', function() {
       const el = util.createDirective(document, 'search-input', {});
       const spinner = el[0].querySelector('spinner');
+
+      fakeStore.isLoading.returns(false);
+      el.scope.$digest();
+
       assert.equal(util.isHidden(spinner), true);
     });
 
-    it('is visible when there are network requests in flight', function() {
+    it('is visible when there are API requests in flight', function() {
       const el = util.createDirective(document, 'search-input', {});
       const spinner = el[0].querySelector('spinner');
-      fakeHttp.pendingRequests.push([{}]);
+
+      fakeStore.isLoading.returns(true);
       el.scope.$digest();
+
       assert.equal(util.isHidden(spinner), false);
     });
   });

src/sidebar/services/api.js

@@ -120,6 +120,18 @@ function serializeParams(params) {
  * @return {Promise<any|APIResponse>}
  */
 
+/**
+ * Configuration for an API method.
+ *
+ * @typedef {Object} APIMethodOptions
+ * @prop {() => Promise<string>} getAccessToken -
+ *   Function which acquires a valid access token for making an API request.
+ * @prop [() => any] onRequestStarted - Callback invoked when the API request starts.
+ * @prop [() => any] onRequestFinished - Callback invoked when the API request finishes.
+ */
+
+const noop = () => {};
+
 /**
  * Creates a function that will make an API call to a named route.
  *
@@ -128,18 +140,29 @@ function serializeParams(params) {
  * @param links - Object or promise for an object mapping named API routes to
  *                URL templates and methods
  * @param route - The dotted path of the named API route (eg. `annotation.create`)
- * @param {Function} tokenGetter - Function which returns a Promise for an
- *                   access token for the API.
+ * @param [APIMethodOptions] - Configuration for the API method
  * @return {APICallFunction}
  */
-function createAPICall($http, $q, links, route, tokenGetter) {
+function createAPICall(
+  $http,
+  $q,
+  links,
+  route,
+  {
+    getAccessToken = noop,
+    onRequestStarted = noop,
+    onRequestFinished = noop,
+  } = {}
+) {
   return function(params, data, options = {}) {
+    onRequestStarted();
+
     // `$q.all` is used here rather than `Promise.all` because testing code that
     // mixes native Promises with the `$q` promises returned by `$http`
     // functions gets awkward in tests.
     let accessToken;
     return $q
-      .all([links, tokenGetter()])
+      .all([links, getAccessToken()])
       .then(([links, token]) => {
         const descriptor = get(links, route);
         const url = urlUtil.replaceURLParams(descriptor.url, params);
@@ -163,6 +186,8 @@ function createAPICall($http, $q, links, route, tokenGetter) {
         return $http(req);
       })
       .then(function(response) {
+        onRequestFinished();
+
         if (options.includeMetadata) {
           return { data: response.data, token: accessToken };
         } else {
@@ -170,6 +195,8 @@ function createAPICall($http, $q, links, route, tokenGetter) {
         }
       })
       .catch(function(response) {
+        onRequestFinished();
+
         // Translate the API result into an `Error` to follow the convention that
         // Promises should be rejected with an Error or Error-like object.
         //
@@ -201,10 +228,14 @@ function createAPICall($http, $q, links, route, tokenGetter) {
  * not use authentication.
  */
 // @ngInject
-function api($http, $q, apiRoutes, auth) {
+function api($http, $q, apiRoutes, auth, store) {
   const links = apiRoutes.routes();
   function apiCall(route) {
-    return createAPICall($http, $q, links, route, auth.tokenGetter);
+    return createAPICall($http, $q, links, route, {
+      getAccessToken: auth.tokenGetter,
+      onRequestStarted: store.apiRequestStarted,
+      onRequestFinished: store.apiRequestFinished,
+    });
   }
 
   return {

src/sidebar/services/test/api-test.js

@@ -20,6 +20,7 @@ describe('sidebar.services.api', function() {
   let $httpBackend;
   let $q;
   let fakeAuth;
+  let fakeStore;
   let sandbox;
   let api;
 
@@ -50,11 +51,16 @@ describe('sidebar.services.api', function() {
     fakeAuth = {
       tokenGetter: sinon.stub(),
     };
+    fakeStore = {
+      apiRequestStarted: sinon.stub(),
+      apiRequestFinished: sinon.stub(),
+    };
 
     angular.mock.module('h', {
       apiRoutes: fakeApiRoutes,
       auth: fakeAuth,
       settings: { apiUrl: 'https://example.com/api/' },
+      store: fakeStore,
     });
 
     angular.mock.inject(function(_$q_) {
@@ -376,4 +382,30 @@ describe('sidebar.services.api', function() {
       .respond(() => [200, { userid: 'acct:user@example.com' }]);
     $httpBackend.flush();
   });
+
+  it('dispatches store actions when an API request starts and completes successfully', () => {
+    api.profile.read({}).then(() => {
+      assert.isTrue(
+        fakeStore.apiRequestFinished.calledAfter(fakeStore.apiRequestStarted)
+      );
+    });
+
+    $httpBackend
+      .expectGET('https://example.com/api/profile')
+      .respond(() => [200, { userid: 'acct:user@example.com' }]);
+    $httpBackend.flush();
+  });
+
+  it('dispatches store actions when an API request starts and fails', () => {
+    api.profile.read({}).catch(() => {
+      assert.isTrue(
+        fakeStore.apiRequestFinished.calledAfter(fakeStore.apiRequestStarted)
+      );
+    });
+
+    $httpBackend
+      .expectGET('https://example.com/api/profile')
+      .respond(() => [400, { reason: 'Something went wrong' }]);
+    $httpBackend.flush();
+  });
 });

src/sidebar/store/index.js

@@ -34,6 +34,7 @@
 const createStore = require('./create-store');
 const debugMiddleware = require('./debug-middleware');
 
+const activity = require('./modules/activity');
 const annotations = require('./modules/annotations');
 const frames = require('./modules/frames');
 const links = require('./modules/links');
@@ -83,6 +84,7 @@ function store($rootScope, settings) {
   ];
 
   const modules = [
+    activity,
     annotations,
     frames,
     links,

src/sidebar/store/modules/activity.js

+'use strict';
+
+/**
+ * Store module which tracks activity happening in the application that may
+ * need to be reflected in the UI.
+ */
+
+const { actionTypes } = require('../util');
+
+function init() {
+  return {
+    activity: {
+      /**
+       * The number of API requests that have started and not yet completed.
+       */
+      activeApiRequests: 0,
+    },
+  };
+}
+
+const update = {
+  API_REQUEST_STARTED(state) {
+    const { activity } = state;
+    return {
+      activity: {
+        ...activity,
+        activeApiRequests: activity.activeApiRequests + 1,
+      },
+    };
+  },
+
+  API_REQUEST_FINISHED(state) {
+    const { activity } = state;
+    if (activity.activeApiRequests === 0) {
+      throw new Error(
+        'API_REQUEST_FINISHED action when no requests were active'
+      );
+    }
+
+    return {
+      activity: {
+        ...activity,
+        activeApiRequests: activity.activeApiRequests - 1,
+      },
+    };
+  },
+};
+
+const actions = actionTypes(update);
+
+function apiRequestStarted() {
+  return { type: actions.API_REQUEST_STARTED };
+}
+
+function apiRequestFinished() {
+  return { type: actions.API_REQUEST_FINISHED };
+}
+
+/**
+ * Return true when any activity is happening in the app that needs to complete
+ * before the UI will be idle.
+ */
+function isLoading(state) {
+  return state.activity.activeApiRequests > 0;
+}
+
+module.exports = {
+  init,
+  update,
+
+  actions: {
+    apiRequestStarted,
+    apiRequestFinished,
+  },
+
+  selectors: {
+    isLoading,
+  },
+};

src/sidebar/store/modules/test/activity-test.js

+'use strict';
+
+const createStore = require('../../create-store');
+const activity = require('../activity');
+
+describe('sidebar/store/modules/activity', () => {
+  let store;
+
+  beforeEach(() => {
+    store = createStore([activity]);
+  });
+
+  describe('#isLoading', () => {
+    it('returns false with the initial state', () => {
+      assert.equal(store.isLoading(), false);
+    });
+
+    it('returns true when API requests are in flight', () => {
+      store.apiRequestStarted();
+      assert.equal(store.isLoading(), true);
+    });
+
+    it('returns false when all requests end', () => {
+      store.apiRequestStarted();
+      store.apiRequestStarted();
+      store.apiRequestFinished();
+
+      assert.equal(store.isLoading(), true);
+
+      store.apiRequestFinished();
+
+      assert.equal(store.isLoading(), false);
+    });
+  });
+
+  describe('#apiRequestFinished', () => {
+    it('triggers an error if no requests are in flight', () => {
+      assert.throws(() => {
+        store.apiRequestFinished();
+      });
+    });
+  });
+});

src/sidebar/templates/search-input.html

@@ -4,11 +4,11 @@
   <input class="simple-search-input"
          type="text"
          name="query"
-         placeholder="{{vm.loading && 'Loading' || 'Search'}}…"
-         ng-disabled="vm.loading"
+         placeholder="{{vm.isLoading() && 'Loading' || 'Search'}}…"
+         ng-disabled="vm.isLoading()"
          ng-class="vm.inputClasses()"/>
-  <button type="button" class="simple-search-icon top-bar__btn" ng-hide="vm.loading">
+  <button type="button" class="simple-search-icon top-bar__btn" ng-hide="vm.isLoading()">
     <i class="h-icon-search"></i>
   </button>
-  <spinner class="top-bar__btn" ng-show="vm.loading" title="Loading…"></spinner>
+  <spinner class="top-bar__btn" ng-show="vm.isLoading()" title="Loading…"></spinner>
 </form>