Improve behavior when selecting pre-created annotations

Selecting a pre-created annotation in the page resulted in a
'You do not have permission to see this annotation' being displayed
because the selection mechanism only supports annotations which have a
server-assigned ID.

This commit improves that behavior by ignoring annotations without IDs
when selecting annotations.

It also fixes an issue in AnnotationUISync that prevents Annotation
objects being replaced in the UI state after they are updated on the
server. Previously AnnotationUISync would receive the tags of
annotations that had been selected in the page and would then look up
annotations in a cache maintained by AnnotationSync in order to get
Annotation objects that could be passed to `annotationUI` functions
which needed objects with an ID.

Fixes #3547

h/static/scripts/annotation-sync.coffee

@@ -53,11 +53,6 @@ module.exports = class AnnotationSync
       this._syncCache(channel)
     @bridge.onConnect(onConnect)
 
-  # Provide a public interface to the annotation cache so that other
-  # sync services can lookup annotations by tag.
-  getAnnotationForTag: (tag) ->
-    @cache[tag] or null
-
   sync: (annotations) ->
     annotations = (this._format a for a in annotations)
     @bridge.call 'sync', annotations, (err, annotations = []) =>

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

@@ -21,7 +21,7 @@ function AnnotationUIController($rootScope, $scope, annotationUI) {
   });
 
   $rootScope.$on(events.ANNOTATION_DELETED, function (event, annotation) {
-    annotationUI.removeSelectedAnnotation(annotation);
+    annotationUI.removeSelectedAnnotation(annotation.id);
   });
 }
 

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

@@ -9,7 +9,6 @@ var uiConstants = require('./ui-constants');
  * @name AnnotationUISync
  * @param {$window} $window An Angular window service.
  * @param {Bridge} bridge
- * @param {AnnotationSync} annotationSync
  * @param {AnnotationUI} annotationUI An instance of the AnnotatonUI service
  * @description
  * Listens for incoming events over the bridge concerning the annotation
@@ -17,29 +16,36 @@ var uiConstants = require('./ui-constants');
  * that the messages are broadcast out to other frames.
  */
 // @ngInject
-function AnnotationUISync($rootScope, $window, bridge, annotationSync,
-  annotationUI) {
-  // Retrieves annotations from the annotationSync cache.
-  var getAnnotationsByTags = function (tags) {
-    return tags.map(annotationSync.getAnnotationForTag, annotationSync);
-  };
+function AnnotationUISync($rootScope, $window, annotationUI, bridge) {
+  function lookupByTag(tag) {
+    return annotationUI.getState().annotations.find(function (annot) {
+      return annot.$$tag === tag;
+    });
+  }
+
+  function findIDsForTags(tags) {
+    var ids = [];
+    tags.forEach(function (tag) {
+      var annot = lookupByTag(tag);
+      if (annot && annot.id) {
+        ids.push(annot.id);
+      }
+    });
+    return ids;
+  }
 
   var channelListeners = {
     showAnnotations: function (tags) {
-      tags = tags || [];
-      var annotations = getAnnotationsByTags(tags);
-      annotationUI.selectAnnotations(annotations);
+      var ids = findIDsForTags(tags || []);
+      annotationUI.selectAnnotations(ids);
       annotationUI.selectTab(uiConstants.TAB_ANNOTATIONS);
     },
     focusAnnotations: function (tags) {
-      tags = tags || [];
-      var annotations = getAnnotationsByTags(tags);
-      annotationUI.focusAnnotations(annotations);
+      annotationUI.focusAnnotations(tags || []);
     },
     toggleAnnotationSelection: function (tags) {
-      tags = tags || [];
-      var annotations = getAnnotationsByTags(tags);
-      annotationUI.toggleSelectedAnnotations(annotations);
+      var ids = findIDsForTags(tags || []);
+      annotationUI.toggleSelectedAnnotations(ids);
     },
     setVisibleHighlights: function (state) {
       if (typeof state !== 'boolean') {

h/static/scripts/annotation-ui.js

@@ -20,6 +20,13 @@ function freeze(selection) {
   }
 }
 
+function toSet(list) {
+  return list.reduce(function (set, key) {
+    set[key] = true;
+    return set;
+  }, {});
+}
+
 function initialSelection(settings) {
   var selection = {};
   if (settings.annotations) {
@@ -197,17 +204,12 @@ module.exports = function (settings) {
     /**
      * Sets which annotations are currently focused.
      *
-     * @param {Array<Annotation>} annotations
+     * @param {Array<string>} Tags of annotations to focus
      */
-    focusAnnotations: function (annotations) {
-      var selection = {};
-      for (var i = 0, annotation; i < annotations.length; i++) {
-        annotation = annotations[i];
-        selection[annotation.$$tag] = true;
-      }
+    focusAnnotations: function (tags) {
       store.dispatch({
         type: types.FOCUS_ANNOTATIONS,
-        focused: freeze(selection),
+        focused: freeze(toSet(tags)),
       });
     },
 
@@ -258,28 +260,16 @@ module.exports = function (settings) {
 
     /**
      * Set the currently selected annotation IDs.
-     *
-     * @param {Array<string|{id:string}>} annotations - Annotations or IDs
-     *        of annotations to select.
      */
-    selectAnnotations: function (annotations) {
-      var selection = {};
-      for (var i = 0; i < annotations.length; i++) {
-        if (typeof annotations[i] === 'string') {
-          selection[annotations[i]] = true;
-        } else {
-          selection[annotations[i].id] = true;
-        }
-      }
-      select(selection);
+    selectAnnotations: function (ids) {
+      select(toSet(ids));
     },
 
     /** Toggle whether annotations are selected or not. */
-    toggleSelectedAnnotations: function (annotations) {
+    toggleSelectedAnnotations: function (ids) {
       var selection = Object.assign({}, store.getState().selectedAnnotationMap);
-      for (var i = 0, annotation; i < annotations.length; i++) {
-        annotation = annotations[i];
-        var id = annotation.id;
+      for (var i = 0; i < ids.length; i++) {
+        var id = ids[i];
         if (selection[id]) {
           delete selection[id];
         } else {
@@ -290,12 +280,13 @@ module.exports = function (settings) {
     },
 
     /** De-select an annotation. */
-    removeSelectedAnnotation: function (annotation) {
+    removeSelectedAnnotation: function (id) {
       var selection = Object.assign({}, store.getState().selectedAnnotationMap);
-      if (selection) {
-        delete selection[annotation.id];
-        select(selection);
+      if (!selection || !id) {
+        return;
       }
+      delete selection[id];
+      select(selection);
     },
 
     /** De-select all annotations. */

h/static/scripts/cross-frame.coffee

@@ -39,7 +39,7 @@ module.exports = class CrossFrame
       new AnnotationSync(bridge, options)
 
     createAnnotationUISync = (annotationSync) ->
-      new AnnotationUISync($rootScope, $window, bridge, annotationSync, annotationUI)
+      new AnnotationUISync($rootScope, $window, annotationUI, bridge)
 
     addFrame = (channel) =>
       channel.call 'getDocumentInfo', (err, info) =>

h/static/scripts/test/annotation-sync-test.coffee

@@ -58,20 +58,6 @@ describe 'AnnotationSync', ->
 
       assert.notCalled(channel.call)
 
-  describe '.getAnnotationForTag', ->
-    it 'returns the annotation if present in the cache', ->
-      ann = {id: 1, $$tag: 'tag1'}
-      annSync = createAnnotationSync()
-      annSync.cache['tag1'] = ann
-
-      cached = annSync.getAnnotationForTag('tag1')
-      assert.equal(cached, ann)
-
-    it 'returns null if not present in the cache', ->
-      annSync = createAnnotationSync()
-      cached = annSync.getAnnotationForTag('tag1')
-      assert.isNull(cached)
-
   describe 'channel event handlers', ->
     assertBroadcast = (channelEvent, publishEvent) ->
       it 'broadcasts the "' + publishEvent + '" event over the local event bus', ->

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

@@ -53,10 +53,7 @@ describe('AnnotationUIController', function () {
   });
 
   it('updates the focused annotations when the focus map changes', function () {
-    annotationUI.focusAnnotations([
-      {$$tag: '1'},
-      {$$tag: '2'},
-    ]);
+    annotationUI.focusAnnotations(['1', '2']);
     assert.deepEqual($scope.focusedAnnotations, { 1: true, 2: true });
   });
 

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

@@ -10,7 +10,6 @@ describe('AnnotationUISync', function () {
   var publish;
   var fakeBridge;
   var annotationUI;
-  var fakeAnnotationSync;
   var createAnnotationUISync;
   var createChannel = function () {
     return { call: sandbox.stub() };
@@ -43,20 +42,15 @@ describe('AnnotationUISync', function () {
       ]
     };
 
-    fakeAnnotationSync = {
-      getAnnotationForTag: function (tag) {
-        return {
-          id: Number(tag.replace('tag', '')),
-          $$tag: tag,
-        };
-      }
-    };
-
     annotationUI = annotationUIFactory({});
+    annotationUI.addAnnotations([
+      {id: 'id1', $$tag: 'tag1'},
+      {id: 'id2', $$tag: 'tag2'},
+      {id: 'id3', $$tag: 'tag3'},
+    ]);
     createAnnotationUISync = function () {
       new AnnotationUISync(
-        $rootScope, fakeWindow, fakeBridge, fakeAnnotationSync,
-        annotationUI
+        $rootScope, fakeWindow, annotationUI, fakeBridge
       );
     };
   }));
@@ -89,14 +83,18 @@ describe('AnnotationUISync', function () {
   });
 
   describe('on "showAnnotations" event', function () {
-    it('updates the annotationUI to include the shown annotations', function () {
+    it('selects annotations which have an ID', function () {
       createAnnotationUISync();
+      annotationUI.selectAnnotations = sinon.stub();
       publish('showAnnotations', ['tag1', 'tag2', 'tag3']);
-      assert.deepEqual(annotationUI.getState().selectedAnnotationMap, {
-        1: true,
-        2: true,
-        3: true,
+      assert.calledWith(annotationUI.selectAnnotations, ['id1', 'id2', 'id3']);
     });
+
+    it('does not select annotations which do not have an ID', function () {
+      createAnnotationUISync();
+      annotationUI.selectAnnotations = sinon.stub();
+      publish('showAnnotations', ['tag1', 'tag-for-a-new-annotation']);
+      assert.calledWith(annotationUI.selectAnnotations, ['id1']);
     });
 
     it('triggers a digest', function () {
@@ -107,7 +105,7 @@ describe('AnnotationUISync', function () {
   });
 
   describe('on "focusAnnotations" event', function () {
-    it('updates the annotationUI to show the provided annotations', function () {
+    it('focuses the annotations', function () {
       createAnnotationUISync();
       publish('focusAnnotations', ['tag1', 'tag2', 'tag3']);
       assert.deepEqual(annotationUI.getState().focusedAnnotationMap, {
@@ -125,14 +123,11 @@ describe('AnnotationUISync', function () {
   });
 
   describe('on "toggleAnnotationSelection" event', function () {
-    it('updates the annotationUI to show the provided annotations', function () {
+    it('toggles the selected state of the annotations', function () {
       createAnnotationUISync();
+      annotationUI.toggleSelectedAnnotations = sinon.stub();
       publish('toggleAnnotationSelection', ['tag1', 'tag2', 'tag3']);
-      assert.deepEqual(annotationUI.getState().selectedAnnotationMap, {
-        1: true,
-        2: true,
-        3: true,
-      });
+      assert.calledWith(annotationUI.toggleSelectedAnnotations, ['id1', 'id2', 'id3']);
     });
 
     it('triggers a digest', function () {

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

@@ -116,22 +116,22 @@ describe('annotationUI', function () {
 
   describe('#focusAnnotations()', function () {
     it('adds the passed annotations to the focusedAnnotationMap', function () {
-      annotationUI.focusAnnotations([{ $$tag: 1 }, { $$tag: 2 }, { $$tag: 3 }]);
+      annotationUI.focusAnnotations([1, 2, 3]);
       assert.deepEqual(annotationUI.getState().focusedAnnotationMap, {
         1: true, 2: true, 3: true
       });
     });
 
     it('replaces any annotations originally in the map', function () {
-      annotationUI.focusAnnotations([{ $$tag: 1 }]);
-      annotationUI.focusAnnotations([{ $$tag: 2 }, { $$tag: 3 }]);
+      annotationUI.focusAnnotations([1]);
+      annotationUI.focusAnnotations([2, 3]);
       assert.deepEqual(annotationUI.getState().focusedAnnotationMap, {
         2: true, 3: true
       });
     });
 
     it('nulls the map if no annotations are focused', function () {
-      annotationUI.focusAnnotations([{$$tag: 1}]);
+      annotationUI.focusAnnotations([1]);
       annotationUI.focusAnnotations([]);
       assert.isNull(annotationUI.getState().focusedAnnotationMap);
     });
@@ -139,7 +139,7 @@ describe('annotationUI', function () {
 
   describe('#hasSelectedAnnotations', function () {
     it('returns true if there are any selected annotations', function () {
-      annotationUI.selectAnnotations([{id: 1}]);
+      annotationUI.selectAnnotations([1]);
       assert.isTrue(annotationUI.hasSelectedAnnotations());
     });
 
@@ -150,12 +150,12 @@ describe('annotationUI', function () {
 
   describe('#isAnnotationSelected', function () {
     it('returns true if the id provided is selected', function () {
-      annotationUI.selectAnnotations([{id: 1}]);
+      annotationUI.selectAnnotations([1]);
       assert.isTrue(annotationUI.isAnnotationSelected(1));
     });
 
     it('returns false if the id provided is not selected', function () {
-      annotationUI.selectAnnotations([{id: 1}]);
+      annotationUI.selectAnnotations([1]);
       assert.isFalse(annotationUI.isAnnotationSelected(2));
     });
 
@@ -166,22 +166,22 @@ describe('annotationUI', function () {
 
   describe('#selectAnnotations()', function () {
     it('adds the passed annotations to the selectedAnnotationMap', function () {
-      annotationUI.selectAnnotations([{ id: 1 }, { id: 2 }, { id: 3 }]);
+      annotationUI.selectAnnotations([1, 2, 3]);
       assert.deepEqual(annotationUI.getState().selectedAnnotationMap, {
         1: true, 2: true, 3: true
       });
     });
 
     it('replaces any annotations originally in the map', function () {
-      annotationUI.selectAnnotations([{ id:1 }]);
-      annotationUI.selectAnnotations([{ id: 2 }, { id: 3 }]);
+      annotationUI.selectAnnotations([1]);
+      annotationUI.selectAnnotations([2, 3]);
       assert.deepEqual(annotationUI.getState().selectedAnnotationMap, {
         2: true, 3: true
       });
     });
 
     it('nulls the map if no annotations are selected', function () {
-      annotationUI.selectAnnotations([{id:1}]);
+      annotationUI.selectAnnotations([1]);
       annotationUI.selectAnnotations([]);
       assert.isNull(annotationUI.getState().selectedAnnotationMap);
     });
@@ -189,45 +189,45 @@ describe('annotationUI', function () {
 
   describe('#toggleSelectedAnnotations()', function () {
     it('adds annotations missing from the selectedAnnotationMap', function () {
-      annotationUI.selectAnnotations([{ id: 1 }, { id: 2}]);
-      annotationUI.toggleSelectedAnnotations([{ id: 3 }, { id: 4 }]);
+      annotationUI.selectAnnotations([1, 2]);
+      annotationUI.toggleSelectedAnnotations([3, 4]);
       assert.deepEqual(annotationUI.getState().selectedAnnotationMap, {
         1: true, 2: true, 3: true, 4: true
       });
     });
 
     it('removes annotations already in the selectedAnnotationMap', function () {
-      annotationUI.selectAnnotations([{id: 1}, {id: 3}]);
-      annotationUI.toggleSelectedAnnotations([{ id: 1 }, { id: 2 }]);
+      annotationUI.selectAnnotations([1, 3]);
+      annotationUI.toggleSelectedAnnotations([1, 2]);
       assert.deepEqual(annotationUI.getState().selectedAnnotationMap, { 2: true, 3: true });
     });
 
     it('nulls the map if no annotations are selected', function () {
-      annotationUI.selectAnnotations([{id: 1}]);
-      annotationUI.toggleSelectedAnnotations([{ id: 1 }]);
+      annotationUI.selectAnnotations([1]);
+      annotationUI.toggleSelectedAnnotations([1]);
       assert.isNull(annotationUI.getState().selectedAnnotationMap);
     });
   });
 
   describe('#removeSelectedAnnotation()', function () {
     it('removes an annotation from the selectedAnnotationMap', function () {
-      annotationUI.selectAnnotations([{id: 1}, {id: 2}, {id: 3}]);
-      annotationUI.removeSelectedAnnotation({ id: 2 });
+      annotationUI.selectAnnotations([1, 2, 3]);
+      annotationUI.removeSelectedAnnotation(2);
       assert.deepEqual(annotationUI.getState().selectedAnnotationMap, {
         1: true, 3: true
       });
     });
 
     it('nulls the map if no annotations are selected', function () {
-      annotationUI.selectAnnotations([{id: 1}]);
-      annotationUI.removeSelectedAnnotation({ id: 1 });
+      annotationUI.selectAnnotations([1]);
+      annotationUI.removeSelectedAnnotation(1);
       assert.isNull(annotationUI.getState().selectedAnnotationMap);
     });
   });
 
   describe('#clearSelectedAnnotations()', function () {
     it('removes all annotations from the selection', function () {
-      annotationUI.selectAnnotations([{id: 1}]);
+      annotationUI.selectAnnotations([1]);
       annotationUI.clearSelectedAnnotations();
       assert.isNull(annotationUI.getState().selectedAnnotationMap);
     });