Merge pull request #724 from hypothesis/fix-focused-group-comparison

Fix group focus change checks

src/sidebar/services/groups.js

@@ -113,14 +113,16 @@ function groups($rootScope, store, api, isSidebar, localStorage, serviceUrl, ses
   }
 
   // Persist the focused group to storage when it changes.
-  var prevFocused = store.focusedGroup();
+  var prevFocusedId = store.focusedGroupId();
   store.subscribe(() => {
-    var focused = store.focusedGroup();
-    if (focused && focused !== prevFocused) {
-      localStorage.setItem(STORAGE_KEY, focused.id);
+    var focusedId = store.focusedGroupId();
+    if (focusedId !== prevFocusedId) {
+      prevFocusedId = focusedId;
+
+      localStorage.setItem(STORAGE_KEY, focusedId);
 
       // Emit the `GROUP_FOCUSED` event for code that still relies on it.
-      $rootScope.$broadcast(events.GROUP_FOCUSED, focused.id);
+      $rootScope.$broadcast(events.GROUP_FOCUSED, focusedId);
     }
   });
 

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

@@ -49,6 +49,10 @@ describe('groups', function() {
       searchUris() {
         return this.getState().searchUris;
       },
+      focusedGroupId() {
+        var group = this.getState().focusedGroup;
+        return group ? group.id : null;
+      },
     });
     fakeSession = sessionWithThreeGroups();
     fakeIsSidebar = true;
@@ -211,6 +215,16 @@ describe('groups', function() {
 
       assert.calledWith(fakeRootScope.$broadcast, events.GROUP_FOCUSED, dummyGroups[1].id);
     });
+
+    it('does not emit GROUP_FOCUSED if the focused group did not change', () => {
+      service();
+
+      fakeStore.setState({ groups: dummyGroups, focusedGroup: dummyGroups[1] });
+      fakeRootScope.$broadcast.reset();
+      fakeStore.setState({ groups: dummyGroups, focusedGroup: dummyGroups[1] });
+
+      assert.notCalled(fakeRootScope.$broadcast);
+    });
   });
 
   describe('#leave', function () {

src/sidebar/store/modules/groups.js

@@ -82,6 +82,15 @@ function focusedGroup(state) {
   return getGroup(state, state.focusedGroupId);
 }
 
+/**
+ * Return the current focused group ID or `null`.
+ *
+ * @return {string|null}
+ */
+function focusedGroupId(state) {
+  return state.focusedGroupId;
+}
+
 /**
  * Return the list of all groups.
  *
@@ -111,5 +120,6 @@ module.exports = {
     allGroups,
     getGroup,
     focusedGroup,
+    focusedGroupId,
   },
 };

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

@@ -82,4 +82,16 @@ describe('sidebar.store.modules.groups', () => {
       assert.deepEqual(store.focusedGroup(), privateGroup);
     });
   });
+
+  describe('focusedGroupId', () => {
+    it('returns `null` if no group is focused', () => {
+      assert.equal(store.focusedGroupId(), null);
+    });
+
+    it('returns the focused group ID if a group has been focused', () => {
+      store.loadGroups([privateGroup]);
+      store.focusGroup(privateGroup.id);
+      assert.equal(store.focusedGroupId(), privateGroup.id);
+    });
+  });
 });