Merge pull request #665 from hypothesis/groups-change

Get the list of groups from the new endpoint and update it where applicable

src/sidebar/frame-sync.js

@@ -192,7 +192,6 @@ function FrameSync($rootScope, $window, Discovery, annotationUI, bridge) {
       }
 
       $rootScope.$broadcast(events.FRAME_CONNECTED);
-
       annotationUI.connectFrame({
         id: info.frameIdentifier,
         metadata: info.metadata,

src/sidebar/groups.js

@@ -14,17 +14,59 @@
 var STORAGE_KEY = 'hypothesis.groups.focus';
 
 var events = require('./events');
+var { awaitStateChange } = require('./util/state-util');
 
 // @ngInject
-function groups(localStorage, serviceUrl, session, $rootScope, store) {
+function groups(annotationUI, localStorage, serviceUrl, session, $rootScope, store) {
   // The currently focused group. This is the group that's shown as selected in
   // the groups dropdown, the annotations displayed are filtered to only ones
   // that belong to this group, and any new annotations that the user creates
   // will be created in this group.
-  var focusedGroup;
+  var focusedGroupId;
+  var groups = [];
+  var documentUri;
+
+  function getDocumentUriForGroupSearch() {
+    function mainUri() {
+      var uris = annotationUI.searchUris();
+      if (uris.length === 0) {
+        return null;
+      }
+
+      // We get the first HTTP URL here on the assumption that group scopes must
+      // be domains (+paths)? and therefore we need to look up groups based on
+      // HTTP URLs (so eg. we cannot use a "file:" URL or PDF fingerprint).
+      return uris.find(uri => uri.startsWith('http'));
+    }
+    return awaitStateChange(annotationUI, mainUri);
+  }
+
+  /**
+   * Fetch the list of applicable groups from the API.
+   *
+   * The list of applicable groups depends on the current userid and the URI of
+   * the attached frames.
+   */
+  function load() {
+    return getDocumentUriForGroupSearch().then(uri => {
+      return store.groups.list({ document_uri: uri });
+    }).then(gs => {
+      $rootScope.$apply(() => {
+        var focGroup = focused();
+        if (focGroup) {
+          var focusedGroupInFetchedList = gs.some(g => g.id === focGroup.id);
+          if (!focusedGroupInFetchedList) {
+            focus(gs[0].id);
+          }
+        }
+        groups = gs;
+      });
+      return gs;
+    });
+  }
 
   function all() {
-    return session.state.groups || [];
+    return groups;
   }
 
   // Return the full object for the group with the given id.
@@ -58,14 +100,16 @@ function groups(localStorage, serviceUrl, session, $rootScope, store) {
    * a previous session. Lastly, we fall back to the first group available.
    */
   function focused() {
-    if (focusedGroup) {
-      return focusedGroup;
+    if (focusedGroupId) {
+      return get(focusedGroupId);
     }
+
     var fromStorage = get(localStorage.getItem(STORAGE_KEY));
     if (fromStorage) {
-      focusedGroup = fromStorage;
-      return focusedGroup;
+      focusedGroupId = fromStorage.id;
+      return fromStorage;
     }
+
     return all()[0];
   }
 
@@ -74,22 +118,37 @@ function groups(localStorage, serviceUrl, session, $rootScope, store) {
     var prevFocused = focused();
     var g = get(id);
     if (g) {
-      focusedGroup = g;
+      focusedGroupId = g.id;
       localStorage.setItem(STORAGE_KEY, g.id);
       if (prevFocused.id !== g.id) {
         $rootScope.$broadcast(events.GROUP_FOCUSED, g.id);
       }
     }
   }
-
+  
   // reset the focused group if the user leaves it
   $rootScope.$on(events.GROUPS_CHANGED, function () {
-    if (focusedGroup) {
-      focusedGroup = get(focusedGroup.id);
-      if (!focusedGroup) {
-        $rootScope.$broadcast(events.GROUP_FOCUSED, focused());
+    // return for use in test
+    return load();
+  });
+
+  // refetch the list of groups when user changes
+  $rootScope.$on(events.USER_CHANGED, () => {
+    // FIXME Makes a second api call on page load. better way?
+    // return for use in test
+    return load();
+  });
+
+  // refetch the list of groups when document url changes
+  $rootScope.$on(events.FRAME_CONNECTED, () => {
+    // FIXME Makes a third api call on page load. better way?
+    // return for use in test
+    return getDocumentUriForGroupSearch().then(uri => {
+      if (documentUri !== uri) {
+        documentUri = uri;
+        load();
       }
-    }
+    });
   });
 
   return {
@@ -97,6 +156,7 @@ function groups(localStorage, serviceUrl, session, $rootScope, store) {
     get: get,
 
     leave: leave,
+    load: load,
 
     focused: focused,
     focus: focus,

src/sidebar/index.js

@@ -53,11 +53,11 @@ if(settings.googleAnalytics){
 }
 
 // Fetch external state that the app needs before it can run. This includes the
-// authenticated user state, the API endpoint URLs and WebSocket connection.
+// user's profile and list of groups.
 var resolve = {
   // @ngInject
-  sessionState: function (session) {
-    return session.load();
+  state: function (groups, session) {
+    return Promise.all([groups.load(), session.load()]);
   },
 };
 

src/sidebar/reducers/session.js

@@ -13,8 +13,6 @@ function init() {
     session: {
       /** A map of features that are enabled for the current user. */
       features: {},
-      /** List of groups that the current user is a member of. */
-      groups: [],
       /** A map of preference names and values. */
       preferences: {},
       /**

src/sidebar/session.js

 'use strict';
 
-var angular = require('angular');
-
 var events = require('./events');
 var retryUtil = require('./retry-util');
 
@@ -97,7 +95,6 @@ function session($q, $rootScope, analytics, annotationUI, auth,
   function update(model) {
     var prevSession = annotationUI.getState().session;
     var userChanged = model.userid !== prevSession.userid;
-    var groupsChanged = !angular.equals(model.groups, prevSession.groups);
 
     // Update the session model used by the application
     annotationUI.updateSession(model);
@@ -120,10 +117,6 @@ function session($q, $rootScope, analytics, annotationUI, auth,
       }
     }
 
-    if (groupsChanged) {
-      $rootScope.$broadcast(events.GROUPS_CHANGED);
-    }
-
     // Return the model
     return model;
   }

src/sidebar/store.js

@@ -178,7 +178,11 @@ function store($http, $q, apiRoutes, auth) {
         delete: apiCall('group.member.delete'),
       },
     },
+    groups: {
+      list: apiCall('groups.read'),
+    },
     profile: {
+      groups: apiCall('profile.groups'),
       read: apiCall('profile.read'),
       update: apiCall('profile.update'),
     },

src/sidebar/streamer.js

@@ -92,6 +92,7 @@ function Streamer($rootScope, annotationMapper, annotationUI, auth,
 
   function handleSessionChangeNotification(message) {
     session.update(message.model);
+    groups.load();
   }
 
   function handleSocketOnError (event) {

src/sidebar/test/groups-test.js

@@ -2,21 +2,17 @@
 
 var events = require('../events');
 var groups = require('../groups');
+var unroll = require('../../shared/test/util').unroll;
 
 // Return a mock session service containing three groups.
 var sessionWithThreeGroups = function() {
   return {
-    state: {
-      groups: [
-        {name: 'Group 1', id: 'id1'},
-        {name: 'Group 2', id: 'id2'},
-        {name: 'Group 3', id: 'id3'},
-      ],
-    },
+    state: {},
   };
 };
 
 describe('groups', function() {
+  var fakeAnnotationUI;
   var fakeSession;
   var fakeStore;
   var fakeLocalStorage;
@@ -27,21 +23,28 @@ describe('groups', function() {
   beforeEach(function() {
     sandbox = sinon.sandbox.create();
 
+    fakeAnnotationUI = {
+      searchUris: sinon.stub().returns(['http://example.org']),
+    };
     fakeSession = sessionWithThreeGroups();
     fakeLocalStorage = {
       getItem: sandbox.stub(),
       setItem: sandbox.stub(),
     };
     fakeRootScope = {
-      eventCallbacks: [],
+      eventCallbacks: {},
 
-      $broadcast: sandbox.stub(),
+      $apply: function(callback) {
+        callback();
+      },
 
       $on: function(event, callback) {
-        if (event === events.GROUPS_CHANGED) {
-          this.eventCallbacks.push(callback);
+        if (event === events.GROUPS_CHANGED || event === events.USER_CHANGED || event === events.FRAME_CONNECTED) {
+          this.eventCallbacks[event] = callback;
         }
       },
+
+      $broadcast: sandbox.stub(),
     };
     fakeStore = {
       group: {
@@ -49,6 +52,13 @@ describe('groups', function() {
           delete: sandbox.stub().returns(Promise.resolve()),
         },
       },
+      groups: {
+        list: sandbox.stub().returns(Promise.resolve([
+          {name: 'Group 1', id: 'id1'},
+          {name: 'Group 2', id: 'id2'},
+          {name: 'Group 3', id: 'id3'},
+        ])),
+      },
     };
     fakeServiceUrl = sandbox.stub();
   });
@@ -58,125 +68,160 @@ describe('groups', function() {
   });
 
   function service() {
-    return groups(fakeLocalStorage, fakeServiceUrl, fakeSession,
+    return groups(fakeAnnotationUI, fakeLocalStorage, fakeServiceUrl, fakeSession,
       fakeRootScope, fakeStore);
   }
 
-  describe('.all()', function() {
+  describe('#all()', function() {
     it('returns no groups if there are none in the session', function() {
-      fakeSession = {state: {groups: []}};
+      fakeSession = {state: {}};
 
       var groups = service().all();
 
       assert.equal(groups.length, 0);
     });
 
-    it('returns the groups from the session when there are some', function() {
-      var groups = service().all();
+    it('returns the groups when there are some', function() {
+      var svc = service();
+
+      return svc.load().then(() => {
+        var groups = svc.all();
+        assert.equal(groups.length, 3);
+        assert.deepEqual(groups, [
+          {name: 'Group 1', id: 'id1'},
+          {name: 'Group 2', id: 'id2'},
+          {name: 'Group 3', id: 'id3'},
+        ]);
+      });
+    });
+  });
+
+  describe('#load() method', function() {
+    it('loads all available groups', function() {
+      var svc = service();
+
+      return svc.load().then(() => {
+        assert.equal(svc.all().length, 3);
+      });
+    });
 
-      assert.equal(groups.length, 3);
-      assert.deepEqual(groups, [
-        {name: 'Group 1', id: 'id1'},
-        {name: 'Group 2', id: 'id2'},
-        {name: 'Group 3', id: 'id3'},
-      ]);
+    it('focuses on the first in the list of groups if user leaves the focused group', function () {
+      var svc = service();
+
+      return svc.load().then(() => {
+        svc.focus('id2');
+      }).then(() => {
+        fakeStore.groups.list = sandbox.stub().returns(Promise.resolve([
+          {name: 'Group 3', id: 'id3'},
+          {name: 'Group 1', id: 'id1'},
+        ]));
+        return svc.load();
+      }).then(() => {
+        assert.equal(svc.focused().id, 'id3');
+      });
     });
   });
 
-  describe('.get() method', function() {
+  describe('#get() method', function() {
     it('returns the requested group', function() {
-      var group = service().get('id2');
+      var svc = service();
 
-      assert.equal(group.id, 'id2');
+      return svc.load().then(() => {
+        var group = svc.get('id2');
+        assert.equal(group.id, 'id2');
+      });
     });
 
     it("returns null if the group doesn't exist", function() {
-      var group = service().get('foobar');
+      var svc = service();
 
-      assert.isNull(group);
+      return svc.load().then(() => {
+        var group = svc.get('foobar');
+        assert.isNull(group);
+      });
     });
   });
 
-  describe('.focused() method', function() {
+  describe('#focused() method', function() {
     it('returns the focused group', function() {
-      var s = service();
-      s.focus('id2');
+      var svc = service();
 
-      assert.equal(s.focused().id, 'id2');
+      return svc.load().then(() => {
+        svc.focus('id2');
+        assert.equal(svc.focused().id, 'id2');
+      });
     });
 
     it('returns the first group initially', function() {
-      var s = service();
+      var svc = service();
 
-      assert.equal(s.focused().id, 'id1');
+      return svc.load().then(() => {
+        assert.equal(svc.focused().id, 'id1');
+      });
     });
 
     it('returns the group selected in localStorage if available', function() {
       fakeLocalStorage.getItem.returns('id3');
-      var s = service();
-
-      assert.equal(s.focused().id, 'id3');
-    });
+      var svc = service();
 
-    it('should update if the user leaves the focused group', function () {
-      var s = service();
-      s.focus('id2');
-
-      var leaveGroup = function(id) {
-        fakeSession.state.groups =
-          fakeSession.state.groups.slice().filter(function (group) {
-            return group.id !== id;
-          });
-        fakeRootScope.eventCallbacks.forEach(function (callback) {
-          callback();
-        });
-      };
-
-      leaveGroup('id3');
-      assert.equal(s.focused().id, 'id2');
-      leaveGroup('id2');
-      assert.notEqual(s.focused().id, 'id2');
+      return svc.load().then(() => {
+        assert.equal(svc.focused().id, 'id3');
+      });
     });
   });
 
-  describe('.focus()', function() {
+  describe('#focus()', function() {
     it('sets the focused group to the named group', function() {
-      var s = service();
-      s.focus('id2');
+      var svc = service();
+
+      return svc.load().then(() => {
+        svc.focus('id2');
 
-      assert.equal(s.focused().id, 'id2');
+        assert.equal(svc.focused().id, 'id2');
+      });
     });
 
     it('does nothing if the named group isn\'t recognised', function() {
-      var s = service();
-      s.focus('foobar');
+      var svc = service();
+
+      return svc.load().then(() => {
+        svc.focus('foobar');
 
-      assert.equal(s.focused().id, 'id1');
+        assert.equal(svc.focused().id, 'id1');
+      });
     });
 
     it('stores the focused group id in localStorage', function() {
-      var s = service();
-      s.focus('id3');
+      var svc = service();
+
+      return svc.load().then(() => {
+        svc.focus('id3');
 
-      assert.calledWithMatch(fakeLocalStorage.setItem, sinon.match.any, 'id3');
+        assert.calledWithMatch(fakeLocalStorage.setItem, sinon.match.any, 'id3');
+      });
     });
 
     it('emits the GROUP_FOCUSED event if the focused group changed', function () {
-      var s = service();
-      s.focus('id3');
-      assert.calledWith(fakeRootScope.$broadcast, events.GROUP_FOCUSED, 'id3');
+      var svc = service();
+
+      return svc.load().then(() => {
+        svc.focus('id3');
+        assert.calledWith(fakeRootScope.$broadcast, events.GROUP_FOCUSED, 'id3');
+      });
     });
 
     it('does not emit GROUP_FOCUSED if the focused group did not change', function () {
-      var s = service();
-      s.focus('id3');
-      fakeRootScope.$broadcast = sinon.stub();
-      s.focus('id3');
-      assert.notCalled(fakeRootScope.$broadcast);
+      var svc = service();
+      return svc.load().then(() => {
+        svc.focus('id3');
+        fakeRootScope.$broadcast = sinon.stub();
+        svc.focus('id3');
+        assert.notCalled(fakeRootScope.$broadcast);
+      });
     });
   });
 
-  describe('.leave()', function () {
+  describe('#leave()', function () {
     it('should call the group leave API', function () {
       var s = service();
       return s.leave('id2').then(() => {
@@ -187,4 +232,20 @@ describe('groups', function() {
       });
     });
   });
+
+  describe('calls load on various events', function () {
+    var changeEvents = [
+      {event: events.GROUPS_CHANGED},
+      {event: events.USER_CHANGED},
+      {event: events.FRAME_CONNECTED},
+    ];
+
+    unroll('should fetch the list of groups from the server when #event occurs', function (testCase) {
+      service();
+
+      return fakeRootScope.eventCallbacks[testCase.event]().then(() => {
+        assert.calledOnce(fakeStore.groups.list);
+      });
+    }, changeEvents);
+  });
 });

src/sidebar/test/session-test.js

@@ -171,17 +171,6 @@ describe('sidebar.session', function () {
   });
 
   describe('#update()', function () {
-    it('broadcasts GROUPS_CHANGED when the groups change', function () {
-      var groupChangeCallback = sinon.stub();
-      $rootScope.$on(events.GROUPS_CHANGED, groupChangeCallback);
-      session.update({
-        groups: [{
-          id: 'groupid',
-        }],
-      });
-      assert.calledOnce(groupChangeCallback);
-    });
-
     it('broadcasts USER_CHANGED when the user changes', function () {
       var userChangeCallback = sinon.stub();
       $rootScope.$on(events.USER_CHANGED, userChangeCallback);

src/sidebar/test/streamer-test.js

@@ -124,6 +124,7 @@ describe('Streamer', function () {
 
     fakeGroups = {
       focused: sinon.stub().returns({id: 'public'}),
+      load: sinon.stub(),
     };
 
     fakeSession = {
@@ -416,6 +417,7 @@ describe('Streamer', function () {
           model: model,
         });
         assert.ok(fakeSession.update.calledWith(model));
+        assert.calledOnce(fakeGroups.load);
       });
     });
   });

src/sidebar/util/state-util.js

+'use strict';
+
+/**
+ * Return a value from app state when it meets certain criteria.
+ *
+ * `await` returns a Promise which resolves when a selector function,
+ * which reads values from a Redux store, returns non-null.
+ *
+ * @param {Object} store - Redux store
+ * @param {Function<T|null>} selector - Function which returns a value from the
+ *   store if the criteria is met or `null` otherwise.
+ * @return {Promise<T>}
+ */
+function awaitStateChange(store, selector) {
+  var result = selector(store);
+  if (result !== null) {
+    return Promise.resolve(result);
+  }
+  return new Promise(resolve => {
+    var unsubscribe = store.subscribe(() => {
+      var result = selector(store);
+      if (result !== null) {
+        unsubscribe();
+        resolve(result);
+      }
+    });
+  });
+}
+
+module.exports = { awaitStateChange } ;

src/sidebar/util/test/state-util-test.js

+'use strict';
+
+var fakeStore = require('../../test/fake-redux-store');
+var stateUtil = require('../state-util');
+
+describe('state-util', function () {
+  var store;
+
+  beforeEach(function() {
+    store = fakeStore({ val: 0 });
+  });
+
+  describe('awaitStateChange()', function () {
+    function getValWhenGreaterThanTwo(store) {
+      if (store.getState().val < 3) {
+        return null;
+      }
+      return store.getState().val;
+    }
+
+    it('should return promise that resolves to a non-null value', function () {
+      var expected = 5;
+
+      store.setState({ val: 5 });
+
+      return stateUtil.awaitStateChange(store, getValWhenGreaterThanTwo).then(function (actual) {
+        assert.equal(actual, expected);
+      });
+    });
+
+    it('should wait for awaitStateChange to return a non-null value', function () {
+      var valPromise;
+      var expected = 5;
+
+      store.setState({ val: 2 });
+      valPromise = stateUtil.awaitStateChange(store, getValWhenGreaterThanTwo);
+      store.setState({ val: 5 });
+
+      return valPromise.then(function (actual) {
+        assert.equal(actual, expected);
+      });
+    });
+  });
+});