Extract the annotation count service client out of TabState

Separate out the logic for querying the server for annotation counts and processing the response from the logic for updating the tab state. * Move duplicated code for setting up an XHR and returning a Promise for the result into a separate module. At some point in future we'll be able to just replace it with a call to window.fetch() * Move the client code for the Hypothesis /badge endpoint out of TabState into its own module and divide up the tests accordingly. * Ensure the annotation count for a tab is reset to zero if querying the /badge endpoint fails.
hypothesis · Jan 7, 2016 · e3d5fec · e3d5fec
1 parent b4dd3d1
commit e3d5fec
Show file tree

Hide file tree

Showing 9 changed files with 181 additions and 143 deletions.
diff --git a/h/browser/chrome/lib/fetch.js b/h/browser/chrome/lib/fetch.js
@@ -0,0 +1,20 @@
+/**
+ * Performs a network fetch for JSON data and returns a Promise
+ * for the result.
+ */
+function fetchJSON(url) {
+  return new Promise(function (resolve, reject) {
+    var xhr = new XMLHttpRequest();
+    xhr.onload = function () {
+      try {
+        resolve(JSON.parse(this.response));
+      } catch (err) {
+        reject(err);
+      }
+    }
+    xhr.open('GET', url);
+    xhr.send();
+  });
+}
+
+module.exports = fetchJSON;
diff --git a/h/browser/chrome/lib/hypothesis-chrome-extension.js b/h/browser/chrome/lib/hypothesis-chrome-extension.js
@@ -3,7 +3,6 @@
 var TabState = require('./tab-state');
 var BrowserAction = require('./browser-action');
 var HelpPage = require('./help-page');
-var settings = require('./settings');
 var SidebarInjector = require('./sidebar-injector');
 var TabStore = require('./tab-store');
 
@@ -145,10 +144,7 @@ function HypothesisChromeExtension(dependencies) {
       annotationCount: 0,
       extensionSidebarInstalled: false,
     });
-
-    settings.then(function(settings) {
-      state.updateAnnotationCount(tabId, url, settings.apiUrl);
-    });
+    state.updateAnnotationCount(tabId, url);
   }
 
   // This function will be called multiple times as the tab reloads.
@@ -175,10 +171,8 @@ function HypothesisChromeExtension(dependencies) {
     });
     state.clearTab(removedTabId);
 
-    settings.then(function (settings) {
-      chromeTabs.get(addedTabId, function (tab) {
-        state.updateAnnotationCount(addedTabId, tab.url, settings.apiUrl);
-      });
+    chromeTabs.get(addedTabId, function (tab) {
+      state.updateAnnotationCount(addedTabId, tab.url);
     });
   }
 

diff --git a/h/browser/chrome/lib/settings.js b/h/browser/chrome/lib/settings.js
@@ -4,30 +4,24 @@
  */
 'use strict';
 
+var fetchJSON = require('./fetch');
+
 /**
  * Validate and normalize the given settings data.
  *
  * @param {Object} settings The settings from the settings.json file.
  */
 function normalizeSettings(settings) {
-
   // Make sure that apiUrl does not end with a /.
   if (settings.apiUrl.charAt(settings.apiUrl.length - 1) === '/') {
     settings.apiUrl = settings.apiUrl.slice(0, -1);
   }
-
+  return settings;
 }
 
 function getSettings() {
-  return new Promise(function(resolve, reject) {
-    var request = new XMLHttpRequest();
-    request.onload = function() {
-      var settings = JSON.parse(this.responseText);
-      normalizeSettings(settings);
-      resolve(settings);
-    };
-    request.open('GET', '/settings.json');
-    request.send(null);
+  return fetchJSON('/settings.json').then(function (settings) {
+    return normalizeSettings(settings);
   });
 }
 

diff --git a/h/browser/chrome/lib/tab-state.js b/h/browser/chrome/lib/tab-state.js
@@ -3,6 +3,8 @@
 var assign = require('core-js/modules/$.object-assign');
 var isShallowEqual = require('is-equal-shallow');
 
+var uriInfo = require('./uri-info');
+
 var states = {
   ACTIVE:   'active',
   INACTIVE: 'inactive',
@@ -27,11 +29,6 @@ var DEFAULT_STATE = {
   error: undefined,
 };
 
-/** encodeUriQuery encodes a string for use in a query parameter */
-function encodeUriQuery(val) {
-  return encodeURIComponent(val).replace(/%20/g, '+');
-}
-
 /** TabState stores the H state for a tab. This state includes:
  *
  * - Whether the extension has been activated on a tab
@@ -151,31 +148,13 @@ function TabState(initialState, onchange) {
    * @param {string} apiUrl The URL of the Hypothesis API.
    */
   this.updateAnnotationCount = function(tabId, tabUrl, apiUrl) {
-    // Fetch the number of annotations of the current page from the server,
-    // and display it as a badge on the browser action button.
     var self = this;
-    var xhr = new XMLHttpRequest();
-    xhr.onload = function() {
-      var total;
-
-      try {
-        total = JSON.parse(this.response).total;
-      } catch (e) {
-        console.error(
-          'updateAnnotationCount() received invalid JSON from the server: ' + e);
-        return;
-      }
-
-      if (typeof total !== 'number') {
-        console.error('annotation count is not a number');
-        return;
-      }
-
-      self.setState(tabId, {annotationCount: total});
-    };
-
-    xhr.open('GET', apiUrl + '/badge?uri=' + encodeUriQuery(tabUrl));
-    xhr.send();
+    return uriInfo.query(tabUrl).then(function (result) {
+      self.setState(tabId, { annotationCount: result.total });
+    }).catch(function (err) {
+      self.setState(tabId, { annotationCount: 0 });
+      console.error('Failed to fetch annotation count for %s: %s', tabUrl, err)
+    });
   };
 
   this.load(initialState || {});

diff --git a/h/browser/chrome/lib/uri-info.js b/h/browser/chrome/lib/uri-info.js
@@ -0,0 +1,26 @@
+var fetchJSON = require('./fetch');
+var settings = require('./settings');
+
+/** encodeUriQuery encodes a string for use in a query parameter */
+function encodeUriQuery(val) {
+  return encodeURIComponent(val).replace(/%20/g, '+');
+}
+
+/**
+ * Queries the Hypothesis service that provides
+ * statistics about the annotations for a given URL.
+ */
+function query(uri) {
+  return settings.then(function (settings) {
+    return fetchJSON(settings.apiUrl + '/badge?uri=' + encodeUriQuery(uri));
+  }).then(function (data) {
+    if (typeof data.total !== 'number') {
+      throw new Error('Annotation count is not a number');
+    }
+    return data;
+  });
+}
+
+module.exports = {
+  query: query
+};
diff --git a/h/browser/chrome/test/hypothesis-chrome-extension-test.js b/h/browser/chrome/test/hypothesis-chrome-extension-test.js
@@ -78,6 +78,7 @@ describe('HypothesisChromeExtension', function () {
       setState: sandbox.spy(),
       clearTab: sandbox.spy(),
       load: sandbox.spy(),
+      updateAnnotationCount: sandbox.spy(),
     };
     fakeTabState.deactivateTab = sinon.spy();
     fakeBrowserAction = {

diff --git a/h/browser/chrome/test/settings.json b/h/browser/chrome/test/settings.json
@@ -1,4 +1,4 @@
 {
-  "apiUrl": "http://127.0.0.1:5000/api/",
+  "apiUrl": "http://127.0.0.1:5000/api",
   "blocklist": {"twitter.com": {}, "finance.yahoo.com": {}, "*.google.com": {}}
 }
diff --git a/h/browser/chrome/test/tab-state-test.js b/h/browser/chrome/test/tab-state-test.js
@@ -1,7 +1,10 @@
-describe('TabState', function () {
-  'use strict';
+'use strict';
+
+var proxyquire = require('proxyquire');
 
-  var TabState = require('../lib/tab-state');
+var TabState = require('../lib/tab-state');
+
+describe('TabState', function () {
   var states = TabState.states;
 
   var state;
@@ -116,102 +119,47 @@ describe('TabState', function () {
     });
   });
 
-  describe('.updateAnnotationCount()', function() {
-    var server;
-
-    beforeEach(function() {
-      server = sinon.fakeServer.create({
-        autoRespond: true,
-        respondImmediately: true
-      });
-      server.respondWith(
-        "GET", "http://example.com/badge?uri=tabUrl",
-        [200, {}, '{"total": 1}']
-      );
+  describe('.updateAnnotationCount', function () {
+    beforeEach(function () {
       sinon.stub(console, 'error');
     });
 
-    afterEach(function() {
-      server.restore();
+    afterEach(function () {
       console.error.restore();
     });
 
-    it('sends the correct XMLHttpRequest to the server', function() {
-      state.updateAnnotationCount("tabId", "tabUrl", "http://example.com");
-
-      assert.equal(server.requests.length, 1);
-      var request = server.requests[0];
-      assert.equal(request.method, "GET");
-      assert.equal(request.url, "http://example.com/badge?uri=tabUrl");
-    });
-
-    it('urlencodes the tabUrl appropriately', function() {
-      state.updateAnnotationCount("tabId", "http://foo.com?bar=baz qüx", "http://example.com");
-
-      assert.equal(server.requests.length, 1);
-      var request = server.requests[0];
-      assert.equal(request.method, "GET");
-      assert.equal(request.url, "http://example.com/badge?uri=http%3A%2F%2Ffoo.com%3Fbar%3Dbaz+q%C3%BCx");
-    });
-
-    it("doesn't set the annotation count if the server's JSON is invalid", function() {
-      server.respondWith(
-        "GET", "http://example.com/badge?uri=tabUrl",
-        [200, {}, 'this is not valid json']
-      );
-
-      state.updateAnnotationCount("tabId", "tabUrl", "http://example.com");
-      assert.equal(state.annotationCount("tabId"), 0);
-    });
-
-    it("logs an error if the server's JSON is invalid", function() {
-      server.respondWith(
-        "GET", "http://example.com/badge?uri=tabUrl",
-        [200, {}, 'this is not valid json']
-      );
-
-      state.updateAnnotationCount("tabId", "tabUrl", "http://example.com");
-      assert(console.error.called);
-    });
-
-    it("doesn't set the annotation count if the server's total is invalid", function() {
-      server.respondWith(
-        "GET", "http://example.com/badge?uri=tabUrl",
-        [200, {}, '{"total": "not a valid number"}']
-      );
-
-      state.updateAnnotationCount("tabId", "tabUrl", "http://example.com");
-      assert.equal(state.annotationCount("tabId"), 0);
-    });
-
-    it("logs an error if the server's total is invalid", function() {
-      server.respondWith(
-        "GET", "http://example.com/badge?uri=tabUrl",
-        [200, {}, '{"total": "not a valid number"}']
-      );
-
-      state.updateAnnotationCount("tabId", "tabUrl", "http://example.com");
-      assert(console.error.called);
-    });
-
-    it("doesn't set the annotation count if the server response has no total", function() {
-      server.respondWith(
-        "GET", "http://example.com/badge?uri=tabUrl",
-        [200, {}, '{"rows": []}']
-      );
-
-      state.updateAnnotationCount("tabId", "tabUrl", "http://example.com");
-      assert.equal(state.annotationCount("tabId"), 0);
-    });
-
-    it("logs an error if the server response has no total", function() {
-      server.respondWith(
-        "GET", "http://example.com/badge?uri=tabUrl",
-        [200, {}, '{"rows": []}']
-      );
-
-      state.updateAnnotationCount("tabId", "tabUrl", "http://example.com");
-      assert(console.error.called);
+    it('queries the service and sets the annotation count', function () {
+      var queryStub = sinon.stub().returns(Promise.resolve({total: 42}));
+      var TabState = proxyquire('../lib/tab-state', {
+        './uri-info': {
+          query: queryStub,
+        }
+      });
+      var tabState = new TabState({1: {state: states.ACTIVE}});
+      return tabState.updateAnnotationCount(1, 'foobar.com')
+        .then(function () {
+          assert.called(queryStub);
+          assert.equal(tabState.getState(1).annotationCount, 42);
+        });
+    });
+
+    it('resets the count if an error occurred', function () {
+      var queryStub = sinon.stub().returns(Promise.reject(new Error('err')));
+      var TabState = proxyquire('../lib/tab-state', {
+        './uri-info': {
+          query: queryStub,
+        }
+      });
+      var tabState = new TabState({1: {
+        state: states.ACTIVE,
+        annotationCount: 42,
+      }});
+      return tabState.updateAnnotationCount(1, 'foobar.com')
+        .then(function () {
+          assert.called(queryStub);
+          assert.called(console.error);
+          assert.equal(tabState.getState(1).annotationCount, 0);
+        });
     });
   });
 });