Merge pull request #1884 from hypothesis/fix-change-group-mutation-violation

Fix mutation violation when changing focused group

src/sidebar/components/group-list-item.js

@@ -33,14 +33,13 @@ function GroupListItem({
   const actions = useStore(store => ({
     clearDirectLinkedGroupFetchFailed: store.clearDirectLinkedGroupFetchFailed,
     clearDirectLinkedIds: store.clearDirectLinkedIds,
-    focusGroup: store.focusGroup,
   }));
 
   const focusGroup = () => {
     analytics.track(analytics.events.GROUP_SWITCH);
     actions.clearDirectLinkedGroupFetchFailed();
     actions.clearDirectLinkedIds();
-    actions.focusGroup(group.id);
+    groupsService.focus(group.id);
   };
 
   const leaveGroup = () => {

src/sidebar/components/test/group-list-item-test.js

@@ -29,7 +29,6 @@ describe('GroupListItem', () => {
     };
 
     fakeStore = {
-      focusGroup: sinon.stub(),
       focusedGroupId: sinon.stub().returns('groupid'),
       clearDirectLinkedIds: sinon.stub(),
       clearDirectLinkedGroupFetchFailed: sinon.stub(),
@@ -50,6 +49,7 @@ describe('GroupListItem', () => {
     };
 
     fakeGroupsService = {
+      focus: sinon.stub(),
       leave: sinon.stub(),
     };
 
@@ -111,7 +111,7 @@ describe('GroupListItem', () => {
       .props()
       .onClick();
 
-    assert.calledWith(fakeStore.focusGroup, fakeGroup.id);
+    assert.calledWith(fakeGroupsService.focus, fakeGroup.id);
     assert.calledWith(fakeAnalytics.track, fakeAnalytics.events.GROUP_SWITCH);
   });
 

src/sidebar/events.js

@@ -18,9 +18,6 @@ export default {
 
   // UI state changes
 
-  /** The currently selected group changed */
-  GROUP_FOCUSED: 'groupFocused',
-
   // Annotation events
 
   /** A new annotation has been created locally. */

src/sidebar/services/groups.js

-const STORAGE_KEY = 'hypothesis.groups.focus';
+import events from '../events';
+import serviceConfig from '../service-config';
+import { isReply } from '../util/annotation-metadata';
+import { combineGroups } from '../util/groups';
+import { awaitStateChange } from '../util/state';
+
 const DEFAULT_ORG_ID = '__default__';
 
 /**
@@ -11,18 +16,12 @@ const DEFAULT_ORGANIZATION = {
     encodeURIComponent(require('../../images/icons/logo.svg')),
 };
 
-import events from '../events';
-import serviceConfig from '../service-config';
-import { combineGroups } from '../util/groups';
-import { awaitStateChange } from '../util/state';
-
 // @ngInject
 export default function groups(
   $rootScope,
   store,
   api,
   isSidebar,
-  localStorage,
   serviceUrl,
   session,
   settings,
@@ -42,6 +41,40 @@ export default function groups(
     return awaitStateChange(store, mainUri);
   }
 
+  /**
+   * Update the focused group. Update the store, then check to see if
+   * there are any new (unsaved) annotations—if so, update those annotations
+   * such that they are associated with the newly-focused group.
+   */
+  function focus(groupId) {
+    const prevGroupId = store.focusedGroupId();
+
+    store.focusGroup(groupId);
+
+    const newGroupId = store.focusedGroupId();
+
+    const groupHasChanged = prevGroupId !== newGroupId && prevGroupId !== null;
+    if (groupHasChanged) {
+      // Move any top-level new annotations to the newly-focused group.
+      // Leave replies where they are.
+      const updatedAnnotations = [];
+      store.newAnnotations().forEach(annot => {
+        if (!isReply(annot)) {
+          updatedAnnotations.push(
+            Object.assign({}, annot, { group: newGroupId })
+          );
+        }
+      });
+
+      if (updatedAnnotations.length) {
+        store.addAnnotations(updatedAnnotations);
+      }
+
+      // Persist this group as the last focused group default
+      store.setDefault('focusedGroup', newGroupId);
+    }
+  }
+
   /**
    * Filter the returned list of groups from the API.
    *
@@ -276,17 +309,17 @@ export default function groups(
     // Step 5. Load the groups into the store and focus the appropriate
     // group.
     const isFirstLoad = store.allGroups().length === 0;
-    const prevFocusedGroup = localStorage.getItem(STORAGE_KEY);
+    const prevFocusedGroup = store.getDefault('focusedGroup');
 
     store.loadGroups(groups);
 
     if (isFirstLoad) {
       if (groups.some(g => g.id === directLinkedAnnotationGroupId)) {
-        store.focusGroup(directLinkedAnnotationGroupId);
+        focus(directLinkedAnnotationGroupId);
       } else if (groups.some(g => g.id === directLinkedGroupId)) {
-        store.focusGroup(directLinkedGroupId);
+        focus(directLinkedGroupId);
       } else if (groups.some(g => g.id === prevFocusedGroup)) {
-        store.focusGroup(prevFocusedGroup);
+        focus(prevFocusedGroup);
       }
     }
 
@@ -316,34 +349,6 @@ export default function groups(
     });
   }
 
-  /** Return the currently focused group. If no group is explicitly focused we
-   * will check localStorage to see if we have persisted a focused group from
-   * a previous session. Lastly, we fall back to the first group available.
-   */
-  function focused() {
-    return store.focusedGroup();
-  }
-
-  /** Set the group with the passed id as the currently focused group. */
-  function focus(id) {
-    store.focusGroup(id);
-  }
-
-  // Persist the focused group to storage when it changes.
-  let prevFocusedId = store.focusedGroupId();
-  store.subscribe(() => {
-    const focusedId = store.focusedGroupId();
-    // The focused group may be null when the user login state changes.
-    if (focusedId !== null && 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, focusedId);
-    }
-  });
-
   // refetch the list of groups when user changes
   $rootScope.$on(events.USER_CHANGED, () => {
     // FIXME Makes a second api call on page load. better way?
@@ -368,7 +373,6 @@ export default function groups(
     leave: leave,
     load: load,
 
-    focused: focused,
     focus: focus,
   };
 }

src/sidebar/services/persisted-defaults.js

@@ -5,6 +5,7 @@
 
 const DEFAULT_KEYS = {
   annotationPrivacy: 'hypothesis.privacy',
+  focusedGroup: 'hypothesis.groups.focus',
 };
 
 // @ngInject

src/sidebar/services/root-thread.js

@@ -123,24 +123,6 @@ export default function RootThread(
     store.removeAnnotations([annotation]);
   });
 
-  // Once the focused group state is moved to the app state store, then the
-  // logic in this event handler can be moved to the annotations reducer.
-  $rootScope.$on(events.GROUP_FOCUSED, function(event, focusedGroupId) {
-    const updatedAnnots = store
-      .getState()
-      .annotations.annotations.filter(function(ann) {
-        return metadata.isNew(ann) && !metadata.isReply(ann);
-      })
-      .map(function(ann) {
-        return Object.assign(ann, {
-          group: focusedGroupId,
-        });
-      });
-    if (updatedAnnots.length > 0) {
-      store.addAnnotations(updatedAnnots);
-    }
-  });
-
   /**
    * Build the root conversation thread from the given UI state.
    * @return {Thread}

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

 import events from '../../events';
 import fakeReduxStore from '../../test/fake-redux-store';
-import groups from '../groups';
+import groups, { $imports } from '../groups';
 
 /**
  * Generate a truth table containing every possible combination of a set of
@@ -44,15 +44,19 @@ describe('groups', function() {
   let fakeSession;
   let fakeSettings;
   let fakeApi;
-  let fakeLocalStorage;
   let fakeRootScope;
   let fakeServiceUrl;
+  let fakeMetadata;
 
   beforeEach(function() {
     fakeAuth = {
       tokenGetter: sinon.stub().returns('1234'),
     };
 
+    fakeMetadata = {
+      isReply: sinon.stub(),
+    };
+
     fakeStore = fakeReduxStore(
       {
         frames: [{ uri: 'http://example.org' }],
@@ -66,9 +70,13 @@ describe('groups', function() {
         },
       },
       {
+        addAnnotations: sinon.stub(),
         focusGroup: sinon.stub(),
+        focusedGroupId: sinon.stub(),
+        getDefault: sinon.stub(),
         getGroup: sinon.stub(),
         loadGroups: sinon.stub(),
+        newAnnotations: sinon.stub().returns([]),
         allGroups() {
           return this.getState().groups.groups;
         },
@@ -78,20 +86,13 @@ describe('groups', function() {
         mainFrame() {
           return this.getState().frames[0];
         },
-        focusedGroupId() {
-          const group = this.getState().groups.focusedGroup;
-          return group ? group.id : null;
-        },
+        setDefault: sinon.stub(),
         setDirectLinkedGroupFetchFailed: sinon.stub(),
         clearDirectLinkedGroupFetchFailed: sinon.stub(),
       }
     );
     fakeSession = sessionWithThreeGroups();
     fakeIsSidebar = true;
-    fakeLocalStorage = {
-      getItem: sinon.stub(),
-      setItem: sinon.stub(),
-    };
     fakeRootScope = {
       eventCallbacks: {},
 
@@ -129,6 +130,14 @@ describe('groups', function() {
     };
     fakeServiceUrl = sinon.stub();
     fakeSettings = { group: null };
+
+    $imports.$mock({
+      '../util/annotation-metadata': fakeMetadata,
+    });
+  });
+
+  afterEach(() => {
+    $imports.$restore();
   });
 
   function service() {
@@ -137,7 +146,6 @@ describe('groups', function() {
       fakeStore,
       fakeApi,
       fakeIsSidebar,
-      fakeLocalStorage,
       fakeServiceUrl,
       fakeSession,
       fakeSettings,
@@ -145,6 +153,81 @@ describe('groups', function() {
     );
   }
 
+  describe('#focus', () => {
+    it('updates the focused group in the store', () => {
+      const svc = service();
+      fakeStore.focusedGroupId.returns('whatever');
+
+      svc.focus('whatnot');
+
+      assert.calledOnce(fakeStore.focusGroup);
+      assert.calledWith(fakeStore.focusGroup, 'whatnot');
+    });
+
+    context('focusing to a different group than before', () => {
+      beforeEach(() => {
+        fakeStore.focusedGroupId.returns('newgroup');
+        fakeStore.focusedGroupId.onFirstCall().returns('whatnot');
+      });
+
+      it('moves top-level annotations to the newly-focused group', () => {
+        const fakeAnnotations = [
+          { $tag: '1', group: 'groupA' },
+          { $tag: '2', group: 'groupB' },
+        ];
+        fakeMetadata.isReply.returns(false);
+        fakeStore.newAnnotations.returns(fakeAnnotations);
+
+        const svc = service();
+        svc.focus('newgroup');
+
+        assert.calledWith(
+          fakeStore.addAnnotations,
+          sinon.match([
+            { $tag: '1', group: 'newgroup' },
+            { $tag: '2', group: 'newgroup' },
+          ])
+        );
+
+        const updatedAnnotations = fakeStore.addAnnotations.getCall(0).args[0];
+        updatedAnnotations.forEach(annot => {
+          assert.equal(annot.group, 'newgroup');
+        });
+      });
+
+      it('does not move replies to the newly-focused group', () => {
+        fakeMetadata.isReply.returns(true);
+        fakeStore.newAnnotations.returns([
+          { $tag: '1', group: 'groupA' },
+          { $tag: '2', group: 'groupB' },
+        ]);
+
+        const svc = service();
+        svc.focus('newgroup');
+
+        assert.calledTwice(fakeMetadata.isReply);
+        assert.notCalled(fakeStore.addAnnotations);
+      });
+
+      it('updates the focused-group default', () => {
+        const svc = service();
+        svc.focus('newgroup');
+
+        assert.calledOnce(fakeStore.setDefault);
+        assert.calledWith(fakeStore.setDefault, 'focusedGroup', 'newgroup');
+      });
+    });
+
+    it('does not update the focused-group default if the group has not changed', () => {
+      fakeStore.focusedGroupId.returns('samegroup');
+
+      const svc = service();
+      svc.focus('samegroup');
+
+      assert.notCalled(fakeStore.setDefault);
+    });
+  });
+
   describe('#all', function() {
     it('returns all groups from store.allGroups', () => {
       const svc = service();
@@ -157,7 +240,7 @@ describe('groups', function() {
   describe('#load', function() {
     it('filters out direct-linked groups that are out of scope and scope enforced', () => {
       const svc = service();
-      fakeLocalStorage.getItem.returns(dummyGroups[0].id);
+      fakeStore.getDefault.returns(dummyGroups[0].id);
       const outOfScopeEnforcedGroup = {
         id: 'oos',
         scopes: { enforced: true, uri_patterns: ['http://foo.com'] },
@@ -180,7 +263,7 @@ describe('groups', function() {
 
     it('catches error from api.group.read request', () => {
       const svc = service();
-      fakeLocalStorage.getItem.returns(dummyGroups[0].id);
+      fakeStore.getDefault.returns(dummyGroups[0].id);
       fakeStore.setState({
         directLinked: {
           directLinkedGroupId: 'does-not-exist',
@@ -325,7 +408,7 @@ describe('groups', function() {
 
     it('sets the focused group from the value saved in local storage', () => {
       const svc = service();
-      fakeLocalStorage.getItem.returns(dummyGroups[1].id);
+      fakeStore.getDefault.returns(dummyGroups[1].id);
       return svc.load().then(() => {
         assert.calledWith(fakeStore.focusGroup, dummyGroups[1].id);
       });
@@ -339,7 +422,7 @@ describe('groups', function() {
           directLinkedGroupId: dummyGroups[1].id,
         },
       });
-      fakeLocalStorage.getItem.returns(dummyGroups[0].id);
+      fakeStore.getDefault.returns(dummyGroups[0].id);
       fakeApi.groups.list.returns(Promise.resolve(dummyGroups));
       fakeApi.annotation.get.returns(
         Promise.resolve({
@@ -360,7 +443,7 @@ describe('groups', function() {
         },
       });
       fakeApi.groups.list.returns(Promise.resolve(dummyGroups));
-      fakeLocalStorage.getItem.returns(dummyGroups[0].id);
+      fakeStore.getDefault.returns(dummyGroups[0].id);
       fakeApi.annotation.get.returns(
         Promise.resolve({
           id: 'ann-id',
@@ -379,7 +462,7 @@ describe('groups', function() {
           directLinkedGroupId: dummyGroups[1].id,
         },
       });
-      fakeLocalStorage.getItem.returns(dummyGroups[0].id);
+      fakeStore.getDefault.returns(dummyGroups[0].id);
       fakeApi.groups.list.returns(Promise.resolve(dummyGroups));
       return svc.load().then(() => {
         assert.calledWith(fakeStore.focusGroup, dummyGroups[1].id);
@@ -416,7 +499,7 @@ describe('groups', function() {
     [null, 'some-group-id'].forEach(groupId => {
       it('does not set the focused group if not present in the groups list', () => {
         const svc = service();
-        fakeLocalStorage.getItem.returns(groupId);
+        fakeStore.getDefault.returns(groupId);
         return svc.load().then(() => {
           assert.notCalled(fakeStore.focusGroup);
         });
@@ -745,69 +828,6 @@ describe('groups', function() {
     });
   });
 
-  describe('#focused', function() {
-    it('returns the focused group', function() {
-      const svc = service();
-      fakeStore.setState({
-        groups: { groups: dummyGroups, focusedGroup: dummyGroups[2] },
-      });
-      assert.equal(svc.focused(), dummyGroups[2]);
-    });
-  });
-
-  describe('#focus', function() {
-    it('sets the focused group to the named group', function() {
-      const svc = service();
-      svc.focus('foo');
-      assert.calledWith(fakeStore.focusGroup, 'foo');
-    });
-  });
-
-  context('when the focused group changes', () => {
-    it('stores the focused group id in localStorage', function() {
-      service();
-
-      fakeStore.setState({
-        groups: { groups: dummyGroups, focusedGroup: dummyGroups[1] },
-      });
-
-      assert.calledWithMatch(
-        fakeLocalStorage.setItem,
-        sinon.match.any,
-        dummyGroups[1].id
-      );
-    });
-
-    it('emits the GROUP_FOCUSED event if the focused group changed', function() {
-      service();
-
-      fakeStore.setState({
-        groups: { groups: dummyGroups, focusedGroup: dummyGroups[1] },
-      });
-
-      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: { groups: dummyGroups, focusedGroup: dummyGroups[1] },
-      });
-
-      fakeRootScope.$broadcast.reset();
-      fakeStore.setState({
-        groups: { groups: dummyGroups, focusedGroup: dummyGroups[1] },
-      });
-
-      assert.notCalled(fakeRootScope.$broadcast);
-    });
-  });
-
   describe('#leave', function() {
     it('should call the group leave API', function() {
       const s = service();

src/sidebar/services/test/persisted-defaults-test.js

@@ -3,6 +3,7 @@ import persistedDefaults from '../persisted-defaults';
 
 const DEFAULT_KEYS = {
   annotationPrivacy: 'hypothesis.privacy',
+  focusedGroup: 'hypothesis.groups.focus',
 };
 
 describe('sidebar/services/persisted-defaults', function() {
@@ -41,14 +42,15 @@ describe('sidebar/services/persisted-defaults', function() {
 
       svc.init();
 
-      // Retrieving the one default from localStorage
-      assert.calledOnce(fakeLocalStorage.getItem);
-      assert.calledWith(
-        fakeLocalStorage.getItem,
-        DEFAULT_KEYS.annotationPrivacy
+      // Retrieving each known default from localStorage...
+      assert.equal(
+        fakeLocalStorage.getItem.callCount,
+        Object.keys(DEFAULT_KEYS).length
       );
-      // Setting the one default in the store
-      assert.calledOnce(fakeStore.setDefault);
+
+      Object.keys(DEFAULT_KEYS).forEach(defaultKey => {
+        assert.calledWith(fakeLocalStorage.getItem, DEFAULT_KEYS[defaultKey]);
+      });
     });
 
     it('should set defaults on the store with the values returned by `localStorage`', () => {
@@ -57,7 +59,9 @@ describe('sidebar/services/persisted-defaults', function() {
 
       svc.init();
 
-      assert.calledWith(fakeStore.setDefault, 'annotationPrivacy', 'bananas');
+      Object.keys(DEFAULT_KEYS).forEach(defaultKey => {
+        assert.calledWith(fakeStore.setDefault, defaultKey, 'bananas');
+      });
     });
 
     it('should set default to `null` if key non-existent in storage', () => {
@@ -66,7 +70,9 @@ describe('sidebar/services/persisted-defaults', function() {
 
       svc.init();
 
-      assert.calledWith(fakeStore.setDefault, 'annotationPrivacy', null);
+      Object.keys(DEFAULT_KEYS).forEach(defaultKey => {
+        assert.calledWith(fakeStore.setDefault, defaultKey, null);
+      });
     });
 
     context('when defaults change in the store', () => {
@@ -113,7 +119,7 @@ describe('sidebar/services/persisted-defaults', function() {
       });
 
       it('should not update local storage if default has not changed', () => {
-        const defaults = { annotationPrivacy: 'carrots' };
+        const defaults = { focusedGroup: 'carrots' };
         fakeLocalStorage.getItem.returns('carrots');
         fakeStore.getDefaults.returns(defaults);
         fakeStore.setState({ defaults: defaults });

src/sidebar/services/test/root-thread-test.js

@@ -416,40 +416,4 @@ describe('rootThread', function() {
       });
     });
   });
-
-  describe('when the focused group changes', function() {
-    it('moves new annotations to the focused group', function() {
-      fakeStore.state.annotations.annotations = [{ $tag: 'a-tag' }];
-
-      $rootScope.$broadcast(events.GROUP_FOCUSED, 'private-group');
-
-      assert.calledWith(
-        fakeStore.addAnnotations,
-        sinon.match([
-          {
-            $tag: 'a-tag',
-            group: 'private-group',
-          },
-        ])
-      );
-    });
-
-    it('does not move replies to the new group', function() {
-      fakeStore.state.annotations.annotations = [annotationFixtures.newReply()];
-
-      $rootScope.$broadcast(events.GROUP_FOCUSED, 'private-group');
-
-      assert.notCalled(fakeStore.addAnnotations);
-    });
-
-    it('does not move saved annotations to the new group', function() {
-      fakeStore.state.annotations.annotations = [
-        annotationFixtures.defaultAnnotation(),
-      ];
-
-      $rootScope.$broadcast(events.GROUP_FOCUSED, 'private-group');
-
-      assert.notCalled(fakeStore.addAnnotations);
-    });
-  });
 });

src/sidebar/store/modules/annotations.js

@@ -383,6 +383,16 @@ function findAnnotationByID(state, id) {
   return findByID(state.annotations.annotations, id);
 }
 
+/**
+ * Return all loaded annotations that are not highlights and have not been saved
+ * to the server.
+ */
+const newAnnotations = createSelector(
+  state => state.annotations.annotations,
+  annotations =>
+    annotations.filter(ann => metadata.isNew(ann) && !metadata.isHighlight(ann))
+);
+
 /**
  * Return all loaded annotations that are highlights and have not been saved
  * to the server.
@@ -445,6 +455,7 @@ export default {
     findAnnotationByID,
     findIDsForTags,
     isWaitingToAnchorAnnotations,
+    newAnnotations,
     newHighlights,
     noteCount,
     orphanCount,

src/sidebar/store/modules/defaults.js

@@ -23,6 +23,7 @@ function init() {
    */
   return {
     annotationPrivacy: null,
+    focusedGroup: null,
   };
 }
 

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

@@ -174,6 +174,41 @@ describe('sidebar/store/modules/annotations', function() {
     });
   });
 
+  describe('newAnnotations', () => {
+    [
+      {
+        annotations: [
+          fixtures.oldAnnotation(), // no
+          fixtures.newAnnotation(), // yes
+          fixtures.newAnnotation(), // yes
+          fixtures.newReply(), // yes
+        ],
+        expectedLength: 3,
+      },
+      {
+        annotations: [fixtures.oldAnnotation(), fixtures.newHighlight()],
+        expectedLength: 0,
+      },
+      {
+        annotations: [
+          fixtures.newHighlight(), // no
+          fixtures.newReply(), // yes
+          fixtures.oldAnnotation(), // no
+          fixtures.newPageNote(), // yes
+        ],
+        expectedLength: 2,
+      },
+    ].forEach(testCase => {
+      it('returns number of unsaved, new annotations', () => {
+        const state = { annotations: { annotations: testCase.annotations } };
+        assert.lengthOf(
+          selectors.newAnnotations(state),
+          testCase.expectedLength
+        );
+      });
+    });
+  });
+
   describe('newHighlights', () => {
     [
       {

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

@@ -50,7 +50,11 @@ describe('store/modules/defaults', function() {
 
         const latestDefaults = store.getDefaults();
 
-        assert.hasAllKeys(latestDefaults, ['foo', 'annotationPrivacy']);
+        assert.hasAllKeys(latestDefaults, [
+          'foo',
+          'annotationPrivacy',
+          'focusedGroup',
+        ]);
       });
     });
   });

src/sidebar/test/annotation-fixtures.js

@@ -104,6 +104,18 @@ export function newHighlight() {
   };
 }
 
+/** Return an annotation domain model object for a new page note.
+ */
+export function newPageNote() {
+  return {
+    $highlight: undefined,
+    target: [{ source: 'http://example.org' }],
+    references: [],
+    text: '',
+    tags: [],
+  };
+}
+
 /** Return an annotation domain model object for an existing annotation
  *  received from the server.
  */
@@ -137,7 +149,8 @@ export function oldHighlight() {
  */
 export function oldPageNote() {
   return {
-    highlight: undefined,
+    id: 'note_id',
+    $highlight: undefined,
     target: [{ source: 'http://example.org' }],
     references: [],
     text: '',