Merge pull request #3543 from hypothesis/match-annot-by-id-and-tag

Fix #2965 (3/N) - Match annotations by ID or tag in drafts service and when removing annotations

h/static/scripts/annotation-ui.js

@@ -81,15 +81,25 @@ var types = {
   SELECT_TAB: 'SELECT_TAB',
 };
 
+/**
+ * Return a copy of `current` with all matching annotations in `annotations`
+ * removed.
+ */
 function excludeAnnotations(current, annotations) {
-  var idsAndTags = annotations.reduce(function (map, annot) {
-    var id = annot.id || annot.$$tag;
-    map[id] = true;
-    return map;
-  }, {});
+  var ids = {};
+  var tags = {};
+  annotations.forEach(function (annot) {
+    if (annot.id) {
+      ids[annot.id] = true;
+    }
+    if (annot.$$tag) {
+      tags[annot.$$tag] = true;
+    }
+  });
   return current.filter(function (annot) {
-    var id = annot.id || annot.$$tag;
-    return !idsAndTags[id];
+    var shouldRemove = (annot.id && (annot.id in ids)) ||
+                       (annot.$$tag && (annot.$$tag in tags));
+    return !shouldRemove;
   });
 }
 

h/static/scripts/drafts.js

@@ -23,11 +23,10 @@ function DraftStore() {
    * Returns true if 'draft' is a draft for a given
    * annotation.
    *
-   * Annotations are matched by ID and annotation instance (for unsaved
-   * annotations which have no ID)
+   * Annotations are matched by ID or local tag.
    */
   function match(draft, model) {
-    return draft.model === model ||
+    return (draft.model.$$tag && model.$$tag === draft.model.$$tag) ||
            (draft.model.id && model.id === draft.model.id);
   }
 

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

 'use strict';
 
-var annotationUIFactory = require('../annotation-ui');
+var immutable = require('seamless-immutable');
 
+var annotationUIFactory = require('../annotation-ui');
 var annotationFixtures = require('./annotation-fixtures');
-
 var unroll = require('./util').unroll;
 
+var defaultAnnotation = annotationFixtures.defaultAnnotation;
+
+var fixtures = immutable({
+  pair: [
+    Object.assign(defaultAnnotation(), {id: 1, $$tag: 't1'}),
+    Object.assign(defaultAnnotation(), {id: 2, $$tag: 't2'}),
+  ],
+});
+
 describe('annotationUI', function () {
   var annotationUI;
 
@@ -36,7 +45,7 @@ describe('annotationUI', function () {
 
   describe('#addAnnotations()', function () {
     it('adds annotations to the current state', function () {
-      var annot = annotationFixtures.defaultAnnotation();
+      var annot = defaultAnnotation();
       annotationUI.addAnnotations([annot]);
       assert.deepEqual(annotationUI.getState().annotations, [annot]);
     });
@@ -44,16 +53,28 @@ describe('annotationUI', function () {
 
   describe('#removeAnnotations()', function () {
     it('removes annotations from the current state', function () {
-      var annot = annotationFixtures.defaultAnnotation();
+      var annot = defaultAnnotation();
       annotationUI.addAnnotations([annot]);
       annotationUI.removeAnnotations([annot]);
       assert.deepEqual(annotationUI.getState().annotations, []);
     });
+
+    it('matches annotations to remove by ID', function () {
+      annotationUI.addAnnotations(fixtures.pair);
+      annotationUI.removeAnnotations([{id: fixtures.pair[0].id}]);
+      assert.deepEqual(annotationUI.getState().annotations, [fixtures.pair[1]]);
+    });
+
+    it('matches annotations to remove by tag', function () {
+      annotationUI.addAnnotations(fixtures.pair);
+      annotationUI.removeAnnotations([{$$tag: fixtures.pair[0].$$tag}]);
+      assert.deepEqual(annotationUI.getState().annotations, [fixtures.pair[1]]);
+    });
   });
 
   describe('#clearAnnotations()', function () {
     it('removes all annotations', function () {
-      var annot = annotationFixtures.defaultAnnotation();
+      var annot = defaultAnnotation();
       annotationUI.addAnnotations([annot]);
       annotationUI.clearAnnotations();
       assert.deepEqual(annotationUI.getState().annotations, []);

h/static/scripts/test/drafts-test.js

+'use strict';
+
 var draftsService = require('../drafts');
 
 describe('drafts', function () {
@@ -7,7 +9,7 @@ describe('drafts', function () {
     drafts = draftsService();
   });
 
-  describe('.update', function () {
+  describe('#update', function () {
     it('should save changes', function () {
       var model = {id: 'foo'};
       assert.notOk(drafts.get(model));
@@ -31,9 +33,17 @@ describe('drafts', function () {
       drafts.update(modelB, {isPrivate:true, tags:['foo'], text:'bar'});
       assert.equal(drafts.get(modelA).text, 'bar');
     });
+
+    it('should replace drafts with the same tag', function () {
+      var modelA = {$$tag: 'foo'};
+      var modelB = {$$tag: 'foo'};
+      drafts.update(modelA, {isPrivate:true, tags:['foo'], text:'foo'});
+      drafts.update(modelB, {isPrivate:true, tags:['foo'], text:'bar'});
+      assert.equal(drafts.get(modelA).text, 'bar');
+    });
   });
 
-  describe('.remove', function () {
+  describe('#remove', function () {
     it('should remove drafts', function () {
       var model = {id: 'foo'};
       drafts.update(model, {text: 'bar'});
@@ -42,7 +52,7 @@ describe('drafts', function () {
     });
   });
 
-  describe('.unsaved', function () {
+  describe('#unsaved', function () {
     it('should return drafts for unsaved annotations', function () {
       var model = {};
       drafts.update(model, {text: 'bar'});