Preserve local tag and anchoring status flags when annotations are up…

…dated (#87)

When receiving annotation updates via the WebSocket, merge the updated
annotation with the existing local annotation, preserving the local tag
and anchoring status flags.

This fixes a problem where Annotations would be shown as Orphans after
an update was received via the WebSocket

 * When an annotation update is received, merge the current/new
   versions rather than removing the current version and replacing
   it with the new one.

 * Remove mutation of existing annotation in `loadAnnotations()`,
   since the reducer function is now responsible for merging changes
   and triggering UI updates

h/static/scripts/annotation-mapper.js

@@ -20,8 +20,7 @@ function annotationMapper($rootScope, annotationUI, store) {
     annotations.forEach(function (annotation) {
       var existing = getExistingAnnotation(annotationUI, annotation.id);
       if (existing) {
-        angular.copy(annotation, existing);
-        $rootScope.$broadcast(events.ANNOTATION_UPDATED, existing);
+        $rootScope.$broadcast(events.ANNOTATION_UPDATED, annotation);
         return;
       }
       loaded.push(annotation);

h/static/scripts/annotation-ui.js

@@ -129,11 +129,60 @@ function excludeAnnotations(current, annotations) {
   });
 }
 
+function findByID(annotations, id) {
+  return annotations.find(function (annot) {
+    return annot.id === id;
+  });
+}
+
+function findByTag(annotations, tag) {
+  return annotations.find(function (annot) {
+    return annot.$$tag === tag;
+  });
+}
+
 function annotationsReducer(state, action) {
   switch (action.type) {
   case types.ADD_ANNOTATIONS:
-    return Object.assign({}, state,
-        {annotations: state.annotations.concat(action.annotations)});
+    {
+      var updatedIDs = {};
+      var updatedTags = {};
+
+      var added = [];
+      var unchanged = [];
+      var updated = [];
+
+      action.annotations.forEach(function (annot) {
+        var existing = findByID(state.annotations, annot.id);
+        if (!existing && annot.$$tag) {
+          existing = findByTag(state.annotations, annot.$$tag);
+        }
+
+        if (existing) {
+          // Merge the updated annotation with the private fields from the local
+          // annotation
+          updated.push(Object.assign({}, existing, annot));
+          if (annot.id) {
+            updatedIDs[annot.id] = true;
+          }
+          if (existing.$$tag) {
+            updatedTags[existing.$$tag] = true;
+          }
+        } else {
+          added.push(annot);
+        }
+      });
+
+      state.annotations.forEach(function (annot) {
+        if (!updatedIDs[annot.id] && !updatedTags[annot.$$tag]) {
+          unchanged.push(annot);
+        }
+      });
+
+      return Object.assign({}, state, {
+        annotations: added.concat(updated).concat(unchanged),
+      });
+    }
   case types.REMOVE_ANNOTATIONS:
     {
       var annots = excludeAnnotations(state.annotations, action.annotations);
@@ -375,8 +424,12 @@ module.exports = function ($rootScope, settings) {
 
     /** Add annotations to the currently displayed set. */
     addAnnotations: function (annotations) {
+      var added = annotations.filter(function (annot) {
+        return !findByID(annot.id);
+      });
+
       store.dispatch({
-        type: 'ADD_ANNOTATIONS',
+        type: types.ADD_ANNOTATIONS,
         annotations: annotations,
       });
 
@@ -389,7 +442,7 @@ module.exports = function ($rootScope, settings) {
       // successfully anchor then the status will be updated.
       var ANCHORING_TIMEOUT = 500;
 
-      var anchoringAnnots = annotations.filter(metadata.isWaitingToAnchor);
+      var anchoringAnnots = added.filter(metadata.isWaitingToAnchor);
       if (anchoringAnnots.length) {
         setTimeout(function () {
           arrayUtil

h/static/scripts/root-thread.js

@@ -119,10 +119,7 @@ function RootThread($rootScope, annotationUI, features, searchFilter, viewFilter
     $rootScope.$on(event, function (event, annotation) {
       var annotations = [].concat(annotation);
 
-      // Remove any annotations which are already loaded
-      annotationUI.removeAnnotations(annotations);
-
-      // Add the new annotations
+      // Add or update annotations
       annotationUI.addAnnotations(annotations);
 
       // Ensure that newly created annotations are always visible

h/static/scripts/test/annotation-mapper-test.js

@@ -1,6 +1,7 @@
 'use strict';
 
 var angular = require('angular');
+var immutable = require('seamless-immutable');
 
 var events = require('../events');
 
@@ -57,7 +58,7 @@ describe('annotationMapper', function() {
 
     it('triggers the annotationUpdated event for each loaded annotation', function () {
       sandbox.stub($rootScope, '$broadcast');
-      var annotations = [{id: 1}, {id: 2}, {id: 3}];
+      var annotations = immutable([{id: 1}, {id: 2}, {id: 3}]);
       annotationUI.addAnnotations(angular.copy(annotations));
 
       annotationMapper.loadAnnotations(annotations);

h/static/scripts/test/annotation-ui-test.js

@@ -59,12 +59,47 @@ describe('annotationUI', function () {
       clock.restore();
     });
 
-    it('adds annotations to the current state', function () {
+    it('adds annotations not in the store', function () {
       var annot = defaultAnnotation();
       annotationUI.addAnnotations([annot]);
       assert.deepEqual(annotationUI.getState().annotations, [annot]);
     });
 
+    it('updates annotations with matching IDs in the store', function () {
+      var annot = defaultAnnotation();
+      annotationUI.addAnnotations([annot]);
+      var update = Object.assign({}, defaultAnnotation(), {text: 'update'});
+      annotationUI.addAnnotations([update]);
+
+      var updatedAnnot = annotationUI.getState().annotations[0];
+      assert.equal(updatedAnnot.text, 'update');
+    });
+
+    it('updates annotations with matching tags in the store', function () {
+      var annot = annotationFixtures.newAnnotation();
+      annot.$$tag = 'local-tag';
+      annotationUI.addAnnotations([annot]);
+
+      var saved = Object.assign({}, annot, {id: 'server-id'});
+      annotationUI.addAnnotations([saved]);
+
+      var annots = annotationUI.getState().annotations;
+      assert.equal(annots.length, 1);
+      assert.equal(annots[0].id, 'server-id');
+    });
+
+    it('preserves anchoring status of updated annotations', function () {
+      var annot = defaultAnnotation();
+      annotationUI.addAnnotations([annot]);
+      annotationUI.updateAnchorStatus(annot.id, null, false /* not an orphan */);
+
+      var update = Object.assign({}, defaultAnnotation(), {text: 'update'});
+      annotationUI.addAnnotations([update]);
+
+      var updatedAnnot = annotationUI.getState().annotations[0];
+      assert.isFalse(updatedAnnot.$orphan);
+    });
+
     it('marks annotations as orphans if they fail to anchor within a time limit', function () {
       var isOrphan = function () {
         return !!metadata.isOrphan(annotationUI.getState().annotations[0]);

h/static/scripts/test/root-thread-test.js

@@ -303,10 +303,10 @@ describe('rootThread', function () {
   context('when annotation events occur', function () {
     var annot = annotationFixtures.defaultAnnotation();
 
-    unroll('removes and reloads annotations when #event event occurs', function (testCase) {
+    unroll('adds or updates annotations when #event event occurs', function (testCase) {
       $rootScope.$broadcast(testCase.event, testCase.annotations);
       var annotations = [].concat(testCase.annotations);
-      assert.calledWith(fakeAnnotationUI.removeAnnotations, sinon.match(annotations));
+      assert.notCalled(fakeAnnotationUI.removeAnnotations);
       assert.calledWith(fakeAnnotationUI.addAnnotations, sinon.match(annotations));
     }, [
       {event: events.BEFORE_ANNOTATION_CREATED, annotations: annot},