Merge pull request #1255 from hypothesis/refactor-annotation-events

Refactor annotation events

src/sidebar/services/root-thread.js

@@ -5,7 +5,6 @@ const events = require('../events');
 const memoize = require('../util/memoize');
 const metadata = require('../util/annotation-metadata');
 const tabs = require('../util/tabs');
-const uiConstants = require('../ui-constants');
 
 function truthyKeys(map) {
   return Object.keys(map).filter(function(k) {
@@ -82,18 +81,6 @@ function RootThread($rootScope, store, searchFilter, viewFilter) {
     });
   }
 
-  function deleteNewAndEmptyAnnotations() {
-    store
-      .getState()
-      .annotations.filter(function(ann) {
-        return metadata.isNew(ann) && !store.getDraftIfNotEmpty(ann);
-      })
-      .forEach(function(ann) {
-        store.removeDraft(ann);
-        $rootScope.$broadcast(events.ANNOTATION_DELETED, ann);
-      });
-  }
-
   // Listen for annotations being created or loaded
   // and show them in the UI.
   //
@@ -111,33 +98,14 @@ function RootThread($rootScope, store, searchFilter, viewFilter) {
   });
 
   $rootScope.$on(events.BEFORE_ANNOTATION_CREATED, function(event, ann) {
-    // When a new annotation is created, remove any existing annotations
-    // that are empty.
-    deleteNewAndEmptyAnnotations();
-
-    store.addAnnotations([ann]);
-
-    // If the annotation is of type note or annotation, make sure
-    // the appropriate tab is selected. If it is of type reply, user
-    // stays in the selected tab.
-    if (metadata.isPageNote(ann)) {
-      store.selectTab(uiConstants.TAB_NOTES);
-    } else if (metadata.isAnnotation(ann)) {
-      store.selectTab(uiConstants.TAB_ANNOTATIONS);
-    }
-
-    (ann.references || []).forEach(function(parent) {
-      store.setCollapsed(parent, false);
-    });
+    store.createAnnotation(ann);
   });
 
   // Remove any annotations that are deleted or unloaded
   $rootScope.$on(events.ANNOTATION_DELETED, function(event, annotation) {
     store.removeAnnotations([annotation]);
-    if (annotation.id) {
-      store.removeSelectedAnnotation(annotation.id);
-    }
   });
+
   $rootScope.$on(events.ANNOTATIONS_UNLOADED, function(event, annotations) {
     store.removeAnnotations(annotations);
   });

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

@@ -39,6 +39,7 @@ describe('rootThread', function() {
       state: {
         annotations: [],
         expanded: {},
+        drafts: [],
         filterQuery: null,
         focusedAnnotationMap: null,
         forceVisible: {},
@@ -63,6 +64,7 @@ describe('rootThread', function() {
       selectTab: sinon.stub(),
       getDraftIfNotEmpty: sinon.stub().returns(null),
       removeDraft: sinon.stub(),
+      createAnnotation: sinon.stub(),
     };
 
     fakeBuildThread = sinon.stub().returns(fixtures.emptyThread);
@@ -341,6 +343,12 @@ describe('rootThread', function() {
   context('when annotation events occur', function() {
     const annot = annotationFixtures.defaultAnnotation();
 
+    it('creates a new annotation in the store when BEFORE_ANNOTATION_CREATED event occurs', function() {
+      $rootScope.$broadcast(events.BEFORE_ANNOTATION_CREATED, annot);
+      assert.notCalled(fakeStore.removeAnnotations);
+      assert.calledWith(fakeStore.createAnnotation, sinon.match(annot));
+    });
+
     unroll(
       'adds or updates annotations when #event event occurs',
       function(testCase) {
@@ -350,38 +358,20 @@ describe('rootThread', function() {
         assert.calledWith(fakeStore.addAnnotations, sinon.match(annotations));
       },
       [
-        { event: events.BEFORE_ANNOTATION_CREATED, annotations: annot },
         { event: events.ANNOTATION_CREATED, annotations: annot },
         { event: events.ANNOTATION_UPDATED, annotations: annot },
         { event: events.ANNOTATIONS_LOADED, annotations: [annot] },
       ]
     );
 
-    it('expands the parents of new annotations', function() {
-      const reply = annotationFixtures.oldReply();
-      $rootScope.$broadcast(events.BEFORE_ANNOTATION_CREATED, reply);
-      assert.calledWith(fakeStore.setCollapsed, reply.references[0], false);
+    it('removes annotations when ANNOTATION_DELETED event occurs', function() {
+      $rootScope.$broadcast(events.ANNOTATION_DELETED, annot);
+      assert.calledWith(fakeStore.removeAnnotations, sinon.match([annot]));
     });
 
-    unroll(
-      'removes annotations when #event event occurs',
-      function(testCase) {
-        $rootScope.$broadcast(testCase.event, testCase.annotations);
-        const annotations = [].concat(testCase.annotations);
-        assert.calledWith(
-          fakeStore.removeAnnotations,
-          sinon.match(annotations)
-        );
-      },
-      [
-        { event: events.ANNOTATION_DELETED, annotations: annot },
-        { event: events.ANNOTATIONS_UNLOADED, annotations: [annot] },
-      ]
-    );
-
-    it('deselects deleted annotations', function() {
-      $rootScope.$broadcast(events.ANNOTATION_DELETED, annot);
-      assert.calledWith(fakeStore.removeSelectedAnnotation, annot.id);
+    it('removes annotations when ANNOTATIONS_UNLOADED event occurs', function() {
+      $rootScope.$broadcast(events.ANNOTATIONS_UNLOADED, annot);
+      assert.calledWith(fakeStore.removeAnnotations, sinon.match(annot));
     });
 
     describe('when a new annotation is created', function() {
@@ -395,24 +385,6 @@ describe('rootThread', function() {
         fakeStore.state.annotations.push(existingNewAnnot);
       });
 
-      it('removes drafts for new and empty annotations', function() {
-        fakeStore.getDraftIfNotEmpty.returns(null);
-        const annotation = annotationFixtures.newEmptyAnnotation();
-
-        $rootScope.$broadcast(events.BEFORE_ANNOTATION_CREATED, annotation);
-
-        assert.calledWith(fakeStore.removeDraft, existingNewAnnot);
-      });
-
-      it('deletes new and empty annotations', function() {
-        fakeStore.getDraftIfNotEmpty.returns(null);
-        const annotation = annotationFixtures.newEmptyAnnotation();
-
-        $rootScope.$broadcast(events.BEFORE_ANNOTATION_CREATED, annotation);
-
-        assert.calledWithMatch(onDelete, sinon.match.any, existingNewAnnot);
-      });
-
       it('does not remove annotations that have non-empty drafts', function() {
         fakeStore.getDraftIfNotEmpty.returns(fixtures.nonEmptyDraft);
 

src/sidebar/store/modules/annotations.js

@@ -12,6 +12,7 @@ const metadata = require('../../util/annotation-metadata');
 const uiConstants = require('../../ui-constants');
 
 const selection = require('./selection');
+const drafts = require('./drafts');
 const util = require('../util');
 
 /**
@@ -333,6 +334,36 @@ function hideAnnotation(id) {
   };
 }
 
+/**
+ * Create a new annotation
+ *
+ * The method does 4 tasks:
+ * 1. Removes any existing empty drafts.
+ * 2. Creates a new annotation.
+ * 3. Changes the focused tab to match that of the newly created annotation.
+ * 4. Expands the collapsed state of all new annotation's parents.
+ */
+function createAnnotation(ann) {
+  return dispatch => {
+    // When a new annotation is created, remove any existing annotations
+    // that are empty.
+    dispatch(drafts.actions.deleteNewAndEmptyDrafts([ann]));
+    dispatch(addAnnotations([ann]));
+    // If the annotation is of type note or annotation, make sure
+    // the appropriate tab is selected. If it is of type reply, user
+    // stays in the selected tab.
+    if (metadata.isPageNote(ann)) {
+      dispatch(selection.actions.selectTab(uiConstants.TAB_NOTES));
+    } else if (metadata.isAnnotation(ann)) {
+      dispatch(selection.actions.selectTab(uiConstants.TAB_ANNOTATIONS));
+    }
+    (ann.references || []).forEach(parent => {
+      // Expand any parents of this annotation.
+      dispatch(selection.actions.setCollapsed(parent, false));
+    });
+  };
+}
+
 /**
  * Update the local hidden state of an annotation.
  *
@@ -426,13 +457,14 @@ module.exports = {
   init: init,
   update: update,
   actions: {
-    addAnnotations: addAnnotations,
-    clearAnnotations: clearAnnotations,
-    removeAnnotations: removeAnnotations,
-    updateAnchorStatus: updateAnchorStatus,
-    updateFlagStatus: updateFlagStatus,
-    hideAnnotation: hideAnnotation,
-    unhideAnnotation: unhideAnnotation,
+    addAnnotations,
+    clearAnnotations,
+    createAnnotation,
+    hideAnnotation,
+    removeAnnotations,
+    updateAnchorStatus,
+    updateFlagStatus,
+    unhideAnnotation,
   },
 
   selectors: {

src/sidebar/store/modules/drafts.js

 'use strict';
 
+const metadata = require('../../util/annotation-metadata');
 const util = require('../util');
 
 /**
@@ -96,14 +97,41 @@ function createDraft(annotation, changes) {
   };
 }
 
-/** Remove all drafts. */
+/**
+ * Remove any drafts that are empty.
+ *
+ * An empty draft has no text and no reference tags.
+ */
+
+function deleteNewAndEmptyDrafts() {
+  const annotations = require('./annotations');
+  return (dispatch, getState) => {
+    const newDrafts = getState().drafts.filter(draft => {
+      return (
+        metadata.isNew(draft.annotation) &&
+        !getDraftIfNotEmpty(getState(), draft.annotation)
+      );
+    });
+    const removedAnnotations = newDrafts.map(draft => {
+      dispatch(removeDraft(draft.annotation));
+      return draft.annotation;
+    });
+    dispatch(annotations.actions.removeAnnotations(removedAnnotations));
+  };
+}
+
+/**
+ * Remove all drafts.
+ * */
 function discardAllDrafts() {
   return {
     type: actions.DISCARD_ALL_DRAFTS,
   };
 }
 
-/** Remove the draft version of an annotation. */
+/**
+ * Remove the draft version of an annotation.
+ */
 function removeDraft(annotation) {
   return {
     type: actions.REMOVE_DRAFT,
@@ -169,6 +197,7 @@ module.exports = {
   update,
   actions: {
     createDraft,
+    deleteNewAndEmptyDrafts,
     discardAllDrafts,
     removeDraft,
   },

src/sidebar/store/modules/selection.js

@@ -176,6 +176,18 @@ const update = {
     return {};
   },
 
+  REMOVE_ANNOTATIONS: function(state, action) {
+    const selection = Object.assign({}, state.selectedAnnotationMap);
+    action.annotations.forEach(annotation => {
+      if (annotation.id) {
+        delete selection[annotation.id];
+      }
+    });
+    return {
+      selectedAnnotationMap: freeze(selection),
+    };
+  },
+
   SET_FILTER_QUERY: function(state, action) {
     return {
       filterQuery: action.query,
@@ -317,20 +329,6 @@ function hasSelectedAnnotations(state) {
   return !!state.selectedAnnotationMap;
 }
 
-/** De-select an annotation. */
-function removeSelectedAnnotation(id) {
-  // FIXME: This should be converted to a plain action and accessing the state
-  // should happen in the update() function
-  return function(dispatch, getState) {
-    const selection = Object.assign({}, getState().selectedAnnotationMap);
-    if (!selection || !id) {
-      return;
-    }
-    delete selection[id];
-    dispatch(select(selection));
-  };
-}
-
 /** De-select all annotations. */
 function clearSelectedAnnotations() {
   return { type: actions.CLEAR_SELECTED_ANNOTATIONS };
@@ -365,7 +363,6 @@ module.exports = {
     clearSelection: clearSelection,
     focusAnnotations: focusAnnotations,
     highlightAnnotations: highlightAnnotations,
-    removeSelectedAnnotation: removeSelectedAnnotation,
     selectAnnotations: selectAnnotations,
     selectTab: selectTab,
     setCollapsed: setCollapsed,

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

 'use strict';
 
-const redux = require('redux');
-// `.default` is needed because 'redux-thunk' is built as an ES2015 module
-const thunk = require('redux-thunk').default;
-
 const annotations = require('../annotations');
+const createStoreFromModules = require('../../create-store');
+const drafts = require('../drafts');
 const fixtures = require('../../../test/annotation-fixtures');
-const util = require('../../util');
+const selection = require('../selection');
+const uiConstants = require('../../../ui-constants');
 const unroll = require('../../../../shared/test/util').unroll;
 
 const { actions, selectors } = annotations;
 
 /**
- * Create a Redux store which only handles annotation actions.
+ * Create a Redux store which handles annotation, selection and draft actions.
  */
 function createStore() {
-  // Thunk middleware is needed for the ADD_ANNOTATIONS action.
-  const enhancer = redux.applyMiddleware(thunk);
-  const reducer = util.createReducer(annotations.update);
-  return redux.createStore(reducer, annotations.init(), enhancer);
+  return createStoreFromModules([annotations, selection, drafts], [{}, {}, {}]);
 }
 
 // Tests for most of the functionality in reducers/annotations.js are currently
@@ -144,6 +140,16 @@ describe('annotations reducer', function() {
     });
   });
 
+  describe('#removeAnnotations', function() {
+    it('removes the annotation', function() {
+      const store = createStore();
+      const ann = fixtures.defaultAnnotation();
+      store.dispatch(actions.addAnnotations([ann]));
+      store.dispatch(actions.removeAnnotations([ann]));
+      assert.equal(store.getState().annotations.length, 0);
+    });
+  });
+
   describe('#updateFlagStatus', function() {
     unroll(
       'updates the flagged status of an annotation',
@@ -206,4 +212,57 @@ describe('annotations reducer', function() {
       ]
     );
   });
+
+  describe('#createAnnotation', function() {
+    it('should create an annotation', function() {
+      const store = createStore();
+      const ann = fixtures.oldAnnotation();
+      store.dispatch(actions.createAnnotation(ann));
+      assert.equal(
+        selectors.findAnnotationByID(store.getState(), ann.id).id,
+        ann.id
+      );
+    });
+
+    it('should change tab focus to TAB_ANNOTATIONS when a new annotation is created', function() {
+      const store = createStore();
+      store.dispatch(actions.createAnnotation(fixtures.oldAnnotation()));
+      assert.equal(store.getState().selectedTab, uiConstants.TAB_ANNOTATIONS);
+    });
+
+    it('should change tab focus to TAB_NOTES when a new note annotation is created', function() {
+      const store = createStore();
+      store.dispatch(actions.createAnnotation(fixtures.oldPageNote()));
+      assert.equal(store.getState().selectedTab, uiConstants.TAB_NOTES);
+    });
+
+    it('should expand parent of created annotation', function() {
+      const store = createStore();
+      store.dispatch(
+        actions.addAnnotations([
+          {
+            id: 'annotation_id',
+            $highlight: undefined,
+            target: [{ source: 'source', selector: [] }],
+            references: [],
+            text: 'This is my annotation',
+            tags: ['tag_1', 'tag_2'],
+          },
+        ])
+      );
+      // Collapse the parent.
+      store.dispatch(selection.actions.setCollapsed('annotation_id', true));
+      // Creating a new child annotation should expand its parent.
+      store.dispatch(
+        actions.createAnnotation({
+          highlight: undefined,
+          target: [{ source: 'http://example.org' }],
+          references: ['annotation_id'],
+          text: '',
+          tags: [],
+        })
+      );
+      assert.isTrue(store.getState().expanded.annotation_id);
+    });
+  });
 });

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

@@ -28,7 +28,7 @@ const fixtures = immutable({
   },
 });
 
-describe('Drafts Store', () => {
+describe('store/modules/drafts', () => {
   let store;
 
   beforeEach(() => {
@@ -225,4 +225,46 @@ describe('Drafts Store', () => {
       assert.deepEqual(store.unsavedAnnotations(), []);
     });
   });
+
+  describe('#deleteNewAndEmptyDrafts', () => {
+    [
+      {
+        key: 'should remove new and empty drafts',
+        annotation: {
+          id: undefined,
+          $tag: 'my_annotation_tag',
+        },
+        draft: fixtures.emptyDraft,
+        shouldRemove: true,
+      },
+      {
+        key: 'should not remove drafts with an id',
+        annotation: {
+          id: 'my_id',
+          $tag: 'my_annotation_tag',
+        },
+        draft: fixtures.emptyDraft,
+        shouldRemove: false,
+      },
+      {
+        key: 'should not remove drafts with text',
+        annotation: {
+          id: undefined,
+          $tag: 'my_annotation_tag',
+        },
+        draft: fixtures.draftWithText,
+        shouldRemove: false,
+      },
+    ].forEach(test => {
+      it(test.key, () => {
+        store.createDraft(test.annotation, test.draft);
+        store.deleteNewAndEmptyDrafts([test.annotation]);
+        if (test.shouldRemove) {
+          assert.isNotOk(store.getDraft(test.annotation));
+        } else {
+          assert.isOk(store.getDraft(test.annotation));
+        }
+      });
+    });
+  });
 });

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

 'use strict';
 
+const annotations = require('../annotations');
 const createStore = require('../../create-store');
 const selection = require('../selection');
 const uiConstants = require('../../../ui-constants');
 
-describe('selection', () => {
+describe('store/modules/selection', () => {
   let store;
-  let fakeSettings = {};
+  let fakeSettings = [{}, {}];
 
   beforeEach(() => {
-    store = createStore([selection], [fakeSettings]);
+    store = createStore([annotations, selection], fakeSettings);
   });
 
   describe('getFirstSelectedAnnotationId', function() {
@@ -145,21 +146,15 @@ describe('selection', () => {
     });
   });
 
-  describe('removeSelectedAnnotation()', function() {
-    it('removes an annotation from the selectedAnnotationMap', function() {
+  describe('#REMOVE_ANNOTATIONS', function() {
+    it('removing an annotation should also remove it from selectedAnnotationMap', function() {
       store.selectAnnotations([1, 2, 3]);
-      store.removeSelectedAnnotation(2);
+      store.removeAnnotations([{ id: 2 }]);
       assert.deepEqual(store.getState().selectedAnnotationMap, {
         1: true,
         3: true,
       });
     });
-
-    it('nulls the map if no annotations are selected', function() {
-      store.selectAnnotations([1]);
-      store.removeSelectedAnnotation(1);
-      assert.isNull(store.getState().selectedAnnotationMap);
-    });
   });
 
   describe('clearSelectedAnnotations()', function() {