Merge pull request #1307 from hypothesis/error-when-searching2

Fix JS error when searching after creating a page draft

src/sidebar/services/test/view-filter-test.js

@@ -74,9 +74,10 @@ describe('sidebar/services/view-filter', () => {
   describe('"any" field', () => {
     it('finds matches in any field', () => {
       const annotations = [
-        { id: 1, text: poem.tiger },
-        { id: 2, user: 'Tiger' },
-        { id: 3, tags: ['Tiger'] },
+        { id: 1, text: poem.tiger, target: [{}] },
+        { id: 4, user: 'lion', target: [{}] },
+        { id: 2, user: 'Tiger', target: [{}] },
+        { id: 3, tags: ['Tiger'], target: [{}] },
       ];
       const filters = { any: { terms: ['Tiger'], operator: 'and' } };
 
@@ -191,6 +192,7 @@ describe('sidebar/services/view-filter', () => {
       const annotation = {
         id: 1,
         updated: isoDateWithAge(50),
+        target: [{}],
       };
       const filters = {
         since: { terms: [100], operator: 'and' },
@@ -205,6 +207,7 @@ describe('sidebar/services/view-filter', () => {
       const annotation = {
         id: 1,
         updated: isoDateWithAge(150),
+        target: [{}],
       };
       const filters = {
         since: { terms: [100], operator: 'and' },
@@ -217,7 +220,11 @@ describe('sidebar/services/view-filter', () => {
   });
 
   it('ignores filters with no terms in the query', () => {
-    const annotation = { id: 1, tags: ['foo'] };
+    const annotation = {
+      id: 1,
+      tags: ['foo'],
+      target: [{}],
+    };
     const filters = {
       any: {
         terms: ['foo'],
@@ -233,4 +240,57 @@ describe('sidebar/services/view-filter', () => {
 
     assert.deepEqual(result, [1]);
   });
+
+  it('ignores annotations (drafts) with no id', () => {
+    const annotation = {
+      tags: ['foo'],
+      target: [{}],
+    };
+    const filters = {
+      any: {
+        terms: ['foo'],
+        operator: 'and',
+      },
+    };
+
+    const result = viewFilter.filter([annotation], filters);
+
+    assert.deepEqual(result, []);
+  });
+
+  describe('malformed target object', () => {
+    it('should not fail on annotations without a target object', () => {
+      const annotation = {
+        id: 1,
+        text: 'foo',
+        // Missing target
+      };
+
+      const filters = {
+        any: {
+          terms: ['foo'],
+          operator: 'or',
+        },
+      };
+
+      viewFilter.filter([annotation], filters);
+    });
+
+    it('should not fail on annotations without a target object item', () => {
+      const annotation = {
+        id: 1,
+        text: 'foo',
+        target: [], // Missing target item
+      };
+
+      const filters = {
+        any: {
+          terms: ['foo'],
+          operator: 'or',
+        },
+      };
+
+      viewFilter.filter([annotation], filters);
+    });
+  });
 });

src/sidebar/services/view-filter.js

@@ -100,9 +100,10 @@ function viewFilter(unicode) {
     quote: {
       autofalse: ann => (ann.references || []).length > 0,
       value(annotation) {
-        if (!annotation.target) {
-          // FIXME: All annotations *must* have a target, so this check should
-          // not be required.
+        if (!annotation.target || !annotation.target.length) {
+          // Sanity check that ignores any annotation without a target. We should
+          // never arrive at this place in the code, but its a safe guard against
+          // anything from the server that may be malformed.
           return '';
         }
         const target = annotation.target[0];
@@ -183,7 +184,9 @@ function viewFilter(unicode) {
     const rootFilter = new BinaryOpFilter('and', fieldFilters);
 
     return annotations
-      .filter(ann => rootFilter.matches(ann))
+      .filter(ann => {
+        return ann.id && rootFilter.matches(ann);
+      })
       .map(ann => ann.id);
   };
 }