Merge pull request #754 from hypothesis/fix-group-focused-null-error

Fix `groups.focused()` error if localStorage is empty when client loads

src/sidebar/services/groups.js

@@ -62,12 +62,12 @@ function groups($rootScope, store, api, isSidebar, localStorage, serviceUrl, ses
         params.document_uri = uri;
       }
       return api.groups.list(params);
-    }).then(gs => {
+    }).then(groups => {
       var isFirstLoad = store.allGroups().length === 0;
       var prevFocusedGroup = localStorage.getItem(STORAGE_KEY);
 
-      store.loadGroups(gs);
-      if (isFirstLoad) {
+      store.loadGroups(groups);
+      if (isFirstLoad && groups.some(g => g.id === prevFocusedGroup)) {
         store.focusGroup(prevFocusedGroup);
       }
 

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

@@ -132,6 +132,16 @@ describe('groups', function() {
       });
     });
 
+    [null, 'some-group-id'].forEach(groupId => {
+      it('does not set the focused group if not present in the groups list', () => {
+        var svc = service();
+        fakeLocalStorage.getItem.returns(groupId);
+        return svc.load().then(() => {
+          assert.notCalled(fakeStore.focusGroup);
+        });
+      });
+    });
+
     context('in the sidebar', () => {
       it('waits for the document URL to be determined', () => {
         var svc = service();

src/sidebar/store/modules/groups.js

@@ -21,7 +21,11 @@ function init() {
 const update = {
   FOCUS_GROUP(state, action) {
     const group = state.groups.find(g => g.id === action.id);
-    return { focusedGroupId: group ? action.id : null };
+    if (!group) {
+      console.error(`Attempted to focus group ${action.id} which is not loaded`);
+      return {};
+    }
+    return { focusedGroupId: action.id };
   },
 
   LOAD_GROUPS(state, action) {

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

@@ -21,16 +21,30 @@ describe('sidebar.store.modules.groups', () => {
   });
 
   describe('focusGroup', () => {
+    beforeEach(() => {
+      sinon.stub(console, 'error');
+    });
+
+    afterEach(() => {
+      console.error.restore();
+    });
+
     it('updates the focused group if valid', () => {
       store.loadGroups([publicGroup]);
+
       store.focusGroup(publicGroup.id);
+
       assert.equal(store.getState().focusedGroupId, publicGroup.id);
+      assert.notCalled(console.error);
     });
 
-    it('does not set the focused group if invalid', () => {
+    it('does not update focused group if not valid', () => {
       store.loadGroups([publicGroup]);
+
       store.focusGroup(privateGroup.id);
-      assert.equal(store.getState().focusedGroupId, null);
+
+      assert.equal(store.getState().focusedGroupId, publicGroup.id);
+      assert.called(console.error);
     });
   });