Merge pull request #712 from hypothesis/move-groups-to-app-state

Move focused group and list of loaded groups to store

src/sidebar/services/groups.js

@@ -20,12 +20,6 @@ var serviceConfig = require('../service-config');
 // @ngInject
 function groups($rootScope, store, api, isSidebar, localStorage, serviceUrl, session,
                 settings) {
-  // 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 focusedGroupId;
-  var groups = [];
   var documentUri;
 
   var svc = serviceConfig(settings);
@@ -69,33 +63,25 @@ function groups($rootScope, store, api, isSidebar, localStorage, serviceUrl, ses
       }
       return api.groups.list(params);
     }).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;
+      var isFirstLoad = store.allGroups().length === 0;
+      var prevFocusedGroup = localStorage.getItem(STORAGE_KEY);
+
+      store.loadGroups(gs);
+      if (isFirstLoad) {
+        store.focusGroup(prevFocusedGroup);
+      }
+
+      return store.allGroups();
     });
   }
 
   function all() {
-    return groups;
+    return store.allGroups();
   }
 
   // Return the full object for the group with the given id.
   function get(id) {
-    var gs = all();
-    for (var i = 0, max = gs.length; i < max; i++) {
-      if (gs[i].id === id) {
-        return gs[i];
-      }
-    }
-    return null;
+    return store.getGroup(id);
   }
 
   /**
@@ -118,32 +104,26 @@ function groups($rootScope, store, api, isSidebar, localStorage, serviceUrl, ses
    * a previous session. Lastly, we fall back to the first group available.
    */
   function focused() {
-    if (focusedGroupId) {
-      return get(focusedGroupId);
-    }
-
-    var fromStorage = get(localStorage.getItem(STORAGE_KEY));
-    if (fromStorage) {
-      focusedGroupId = fromStorage.id;
-      return fromStorage;
-    }
-
-    return all()[0];
+    return store.focusedGroup();
   }
 
   /** Set the group with the passed id as the currently focused group. */
   function focus(id) {
-    var prevFocused = focused();
-    var g = get(id);
-    if (g) {
-      focusedGroupId = g.id;
-      localStorage.setItem(STORAGE_KEY, g.id);
-      if (prevFocused.id !== g.id) {
-        $rootScope.$broadcast(events.GROUP_FOCUSED, g.id);
-      }
-    }
+    store.focusGroup(id);
   }
 
+  // Persist the focused group to storage when it changes.
+  var prevFocused = store.focusedGroup();
+  store.subscribe(() => {
+    var focused = store.focusedGroup();
+    if (focused && focused !== prevFocused) {
+      localStorage.setItem(STORAGE_KEY, focused.id);
+
+      // Emit the `GROUP_FOCUSED` event for code that still relies on it.
+      $rootScope.$broadcast(events.GROUP_FOCUSED, focused.id);
+    }
+  });
+
   // reset the focused group if the user leaves it
   $rootScope.$on(events.GROUPS_CHANGED, function () {
     // return for use in test

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

@@ -12,6 +12,12 @@ var sessionWithThreeGroups = function() {
   };
 };
 
+var dummyGroups = [
+  { name: 'Group 1', id: 'id1'},
+  { name: 'Group 2', id: 'id2'},
+  { name: 'Group 3', id: 'id3'},
+];
+
 describe('groups', function() {
   var fakeStore;
   var fakeIsSidebar;
@@ -28,7 +34,18 @@ describe('groups', function() {
 
     fakeStore = fakeReduxStore({
       searchUris: ['http://example.org'],
+      focusedGroup: null,
+      groups: [],
     },{
+      focusGroup: sinon.stub(),
+      getGroup: sinon.stub(),
+      loadGroups: sinon.stub(),
+      allGroups() {
+        return this.getState().groups;
+      },
+      focusedGroup() {
+        return this.getState().focusedGroup;
+      },
       searchUris() {
         return this.getState().searchUris;
       },
@@ -61,11 +78,7 @@ describe('groups', function() {
         },
       },
       groups: {
-        list: sandbox.stub().returns(Promise.resolve([
-          {name: 'Group 1', id: 'id1'},
-          {name: 'Group 2', id: 'id2'},
-          {name: 'Group 3', id: 'id3'},
-        ])),
+        list: sandbox.stub().returns(Promise.resolve(dummyGroups)),
       },
     };
     fakeServiceUrl = sandbox.stub();
@@ -81,36 +94,20 @@ describe('groups', function() {
       fakeSession, fakeSettings);
   }
 
-  describe('#all()', function() {
-    it('returns no groups if there are none in the session', function() {
-      fakeSession = {state: {}};
-
-      var groups = service().all();
-
-      assert.equal(groups.length, 0);
-    });
-
-    it('returns the groups when there are some', function() {
+  describe('#all', function() {
+    it('returns all groups', 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'},
-        ]);
-      });
+      fakeStore.setState({ groups: dummyGroups });
+      assert.deepEqual(svc.all(), dummyGroups);
     });
   });
 
-  describe('#load() method', function() {
+  describe('#load', function() {
     it('loads all available groups', function() {
       var svc = service();
 
       return svc.load().then(() => {
-        assert.equal(svc.all().length, 3);
+        assert.calledWith(fakeStore.loadGroups, dummyGroups);
       });
     });
 
@@ -123,19 +120,11 @@ describe('groups', function() {
       });
     });
 
-    it('focuses on the first in the list of groups if user leaves the focused group', function () {
-      var svc = service();
-
+    it('sets the focused group from the value saved in local storage', () => {
+      var svc  = service();
+      fakeLocalStorage.getItem.returns(dummyGroups[1].id);
       return svc.load().then(() => {
-        svc.focus('id2');
-      }).then(() => {
-        fakeApi.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');
+        assert.calledWith(fakeStore.focusGroup, dummyGroups[1].id);
       });
     });
 
@@ -181,106 +170,50 @@ describe('groups', function() {
     });
   });
 
-  describe('#get() method', function() {
+  describe('#get', function() {
     it('returns the requested group', function() {
       var svc = service();
+      fakeStore.getGroup.withArgs('foo').returns(dummyGroups[1]);
 
-      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 svc = service();
-
-      return svc.load().then(() => {
-        var group = svc.get('foobar');
-        assert.isNull(group);
-      });
+      assert.equal(svc.get('foo'), dummyGroups[1]);
     });
   });
 
-  describe('#focused() method', function() {
+  describe('#focused', function() {
     it('returns the focused group', function() {
       var svc = service();
-
-      return svc.load().then(() => {
-        svc.focus('id2');
-        assert.equal(svc.focused().id, 'id2');
-      });
-    });
-
-    it('returns the first group initially', function() {
-      var svc = service();
-
-      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 svc = service();
-
-      return svc.load().then(() => {
-        assert.equal(svc.focused().id, 'id3');
-      });
+      fakeStore.setState({ groups: dummyGroups, focusedGroup: dummyGroups[2] });
+      assert.equal(svc.focused(), dummyGroups[2]);
     });
   });
 
-  describe('#focus()', function() {
+  describe('#focus', function() {
     it('sets the focused group to the named group', function() {
       var svc = service();
-
-      return svc.load().then(() => {
-        svc.focus('id2');
-
-        assert.equal(svc.focused().id, 'id2');
-      });
-    });
-
-    it('does nothing if the named group isn\'t recognised', function() {
-      var svc = service();
-
-      return svc.load().then(() => {
-        svc.focus('foobar');
-
-        assert.equal(svc.focused().id, 'id1');
-      });
+      svc.focus('foo');
+      assert.calledWith(fakeStore.focusGroup, 'foo');
     });
+  });
 
+  context('when the focused group changes', () => {
     it('stores the focused group id in localStorage', function() {
-      var svc = service();
+      service();
 
-      return svc.load().then(() => {
-        svc.focus('id3');
+      fakeStore.setState({ groups: dummyGroups, focusedGroup: dummyGroups[1] });
 
-        assert.calledWithMatch(fakeLocalStorage.setItem, sinon.match.any, 'id3');
-      });
+      assert.calledWithMatch(fakeLocalStorage.setItem, sinon.match.any, dummyGroups[1].id);
     });
 
     it('emits the GROUP_FOCUSED event if the focused group changed', function () {
-      var svc = service();
+      service();
 
-      return svc.load().then(() => {
-        svc.focus('id3');
-        assert.calledWith(fakeRootScope.$broadcast, events.GROUP_FOCUSED, 'id3');
-      });
-    });
+      fakeStore.setState({ groups: dummyGroups, focusedGroup: dummyGroups[1] });
 
-    it('does not emit GROUP_FOCUSED if the focused group did not change', function () {
-      var svc = service();
-      return svc.load().then(() => {
-        svc.focus('id3');
-        fakeRootScope.$broadcast = sinon.stub();
-        svc.focus('id3');
-        assert.notCalled(fakeRootScope.$broadcast);
-      });
+      assert.calledWith(fakeRootScope.$broadcast, events.GROUP_FOCUSED, dummyGroups[1].id);
     });
   });
 
-  describe('#leave()', function () {
+  describe('#leave', function () {
     it('should call the group leave API', function () {
       var s = service();
       return s.leave('id2').then(() => {

src/sidebar/store/create-store.js

@@ -25,7 +25,7 @@ const { createReducer, bindSelectors } = require('./util');
  * @param {any[]} initArgs - Arguments to pass to each state module's `init` function
  * @param [any[]] middleware - List of additional Redux middlewares to use.
  */
-function createStore(modules, initArgs, middleware = []) {
+function createStore(modules, initArgs = [], middleware = []) {
   // Create the initial state and state update function.
   const initialState = Object.assign({}, ...modules.map(m => m.init(...initArgs)));
   const reducer = createReducer(...modules.map(m => m.update));

src/sidebar/store/index.js

@@ -37,6 +37,7 @@ var debugMiddleware = require('./debug-middleware');
 var annotations = require('./modules/annotations');
 var frames = require('./modules/frames');
 var links = require('./modules/links');
+var groups = require('./modules/groups');
 var selection= require('./modules/selection');
 var session = require('./modules/session');
 var viewer = require('./modules/viewer');
@@ -85,6 +86,7 @@ function store($rootScope, settings) {
     annotations,
     frames,
     links,
+    groups,
     selection,
     session,
     viewer,

src/sidebar/store/modules/groups.js

+'use strict';
+
+const util = require('../util');
+
+function init() {
+  return {
+    /**
+     * List of groups.
+     * @type {Group[]}
+     */
+    groups: [],
+
+    /**
+     * ID of currently selected group.
+     * @type {string|null}
+     */
+    focusedGroupId: null,
+  };
+}
+
+const update = {
+  FOCUS_GROUP(state, action) {
+    const group = state.groups.find(g => g.id === action.id);
+    return { focusedGroupId: group ? action.id : null };
+  },
+
+  LOAD_GROUPS(state, action) {
+    const groups = action.groups;
+    let focusedGroupId = state.focusedGroupId;
+
+    // Reset focused group if not in the new set of groups.
+    if (state.focusedGroupId === null || !groups.find(g => g.id === state.focusedGroupId)) {
+      if (groups.length > 0) {
+        focusedGroupId = groups[0].id;
+      } else {
+        focusedGroupId = null;
+      }
+    }
+
+    return {
+      focusedGroupId,
+      groups: action.groups,
+    };
+  },
+};
+
+const actions = util.actionTypes(update);
+
+/**
+ * Set the current focused group.
+ *
+ * @param {string} id
+ */
+function focusGroup(id) {
+  return {
+    type: actions.FOCUS_GROUP,
+    id,
+  };
+}
+
+/**
+ * Update the set of loaded groups.
+ *
+ * @param {Group[]} groups
+ */
+function loadGroups(groups) {
+  return {
+    type: actions.LOAD_GROUPS,
+    groups,
+  };
+}
+
+/**
+ * Return the currently focused group.
+ *
+ * @return {Group|null}
+ */
+function focusedGroup(state) {
+  if (!state.focusedGroupId) {
+    return null;
+  }
+  return getGroup(state, state.focusedGroupId);
+}
+
+/**
+ * Return the list of all groups.
+ *
+ * @return {Group[]}
+ */
+function allGroups(state) {
+  return state.groups;
+}
+
+/**
+ * Return the group with the given ID.
+ *
+ * @return {Group|undefined}
+ */
+function getGroup(state, id) {
+  return state.groups.find(g => g.id === id);
+}
+
+module.exports = {
+  init,
+  update,
+  actions: {
+    focusGroup,
+    loadGroups,
+  },
+  selectors: {
+    allGroups,
+    getGroup,
+    focusedGroup,
+  },
+};

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

+'use strict';
+
+const createStore = require('../../create-store');
+const groups = require('../groups');
+
+describe('sidebar.store.modules.groups', () => {
+  const publicGroup = {
+    id: '__world__',
+    name: 'Public',
+  };
+
+  const privateGroup = {
+    id: 'foo',
+    name: 'Private',
+  };
+
+  let store;
+
+  beforeEach(() => {
+    store = createStore([groups]);
+  });
+
+  describe('focusGroup', () => {
+    it('updates the focused group if valid', () => {
+      store.loadGroups([publicGroup]);
+      store.focusGroup(publicGroup.id);
+      assert.equal(store.getState().focusedGroupId, publicGroup.id);
+    });
+
+    it('does not set the focused group if invalid', () => {
+      store.loadGroups([publicGroup]);
+      store.focusGroup(privateGroup.id);
+      assert.equal(store.getState().focusedGroupId, null);
+    });
+  });
+
+  describe('loadGroups', () => {
+    it('updates the set of groups', () => {
+      store.loadGroups([publicGroup]);
+      assert.deepEqual(store.getState().groups, [publicGroup]);
+    });
+
+    it('resets the focused group if not in new set of groups', () => {
+      store.loadGroups([publicGroup]);
+      store.focusGroup(publicGroup.id);
+      store.loadGroups([]);
+
+      assert.equal(store.getState().focusedGroupId, null);
+    });
+
+    it('leaves focused group unchanged if in new set of groups', () => {
+      store.loadGroups([publicGroup]);
+      store.focusGroup(publicGroup.id);
+      store.loadGroups([publicGroup, privateGroup]);
+
+      assert.equal(store.getState().focusedGroupId, publicGroup.id);
+    });
+  });
+
+  describe('allGroups', () => {
+    it('returns all groups', () => {
+      store.loadGroups([publicGroup, privateGroup]);
+      assert.deepEqual(store.allGroups(), [publicGroup, privateGroup]);
+    });
+  });
+
+  describe('getGroup', () => {
+    it('returns the group with the given ID', () => {
+      store.loadGroups([publicGroup, privateGroup]);
+      assert.deepEqual(store.getGroup(privateGroup.id), privateGroup);
+    });
+  });
+
+  describe('focusedGroup', () => {
+    it('returns `null` if no group is focused', () => {
+      assert.equal(store.focusedGroup(), null);
+    });
+
+    it('returns the focused group if a group has been focused', () => {
+      store.loadGroups([privateGroup]);
+      store.focusGroup(privateGroup.id);
+      assert.deepEqual(store.focusedGroup(), privateGroup);
+    });
+  });
+});

src/sidebar/test/fake-redux-store.js

@@ -16,7 +16,7 @@ var redux = require('redux');
 function fakeStore(initialState, methods) {
   function update(state, action) {
     if (action.state) {
-      return action.state;
+      return Object.assign({}, state, action.state);
     } else {
       return state;
     }