Merge pull request #1556 from hypothesis/extract-quote-function

Extract `quote` function from annotation / view-filter

src/sidebar/components/annotation.js

 'use strict';
 
-const annotationMetadata = require('../util/annotation-metadata');
+const {
+  isNew,
+  isReply,
+  isPageNote,
+  quote,
+} = require('../util/annotation-metadata');
 const events = require('../events');
 const { isThirdPartyUser } = require('../util/account-id');
 const serviceConfig = require('../service-config');
 
-const isNew = annotationMetadata.isNew;
-const isReply = annotationMetadata.isReply;
-const isPageNote = annotationMetadata.isPageNote;
-
 /**
  * Return a copy of `annotation` with changes made in the editor applied.
  */
@@ -325,19 +326,7 @@ function AnnotationController(
   /**
    * Return the annotation's quote if it has one or `null` otherwise.
    */
-  this.quote = function() {
-    if (self.annotation.target.length === 0) {
-      return null;
-    }
-    const target = self.annotation.target[0];
-    if (!target.selector) {
-      return null;
-    }
-    const quoteSel = target.selector.find(function(sel) {
-      return sel.type === 'TextQuoteSelector';
-    });
-    return quoteSel ? quoteSel.exact : null;
-  };
+  this.quote = () => quote(self.annotation);
 
   this.id = function() {
     return self.annotation.id;

src/sidebar/components/test/annotation-test.js

@@ -685,41 +685,13 @@ describe('annotation', function() {
     });
 
     describe('#quote', function() {
-      it('returns `null` if the annotation has no quotes', function() {
-        const annotation = fixtures.defaultAnnotation();
-        annotation.target = [{}];
-        const controller = createDirective(annotation).controller;
-
-        assert.isNull(controller.quote());
-      });
-
-      it('returns `null` if the annotation has selectors but no quote selector', function() {
-        const annotation = fixtures.defaultAnnotation();
-        annotation.target = [
-          {
-            selector: [],
-          },
-        ];
-        const controller = createDirective(annotation).controller;
-
-        assert.isNull(controller.quote());
-      });
-
-      it("returns the first quote's text if the annotation has quotes", function() {
-        const annotation = fixtures.defaultAnnotation();
-        annotation.target = [
-          {
-            selector: [
-              {
-                type: 'TextQuoteSelector',
-                exact: 'The text that the user selected',
-              },
-            ],
-          },
+      it("returns the annotation's quote", () => {
+        const ann = fixtures.defaultAnnotation();
+        const controller = createDirective(ann).controller;
+        ann.target[0].selector = [
+          { type: 'TextQuoteSelector', exact: 'test quote' },
         ];
-        const controller = createDirective(annotation).controller;
-
-        assert.equal(controller.quote(), 'The text that the user selected');
+        assert.equal(controller.quote(), 'test quote');
       });
     });
 

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

@@ -260,40 +260,4 @@ describe('sidebar/services/view-filter', () => {
 
     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

 'use strict';
 
+const { quote } = require('../util/annotation-metadata');
+
 // Prevent Babel inserting helper code after `@ngInject` comment below which
 // breaks browserify-ngannotate.
 let unused; // eslint-disable-line
@@ -99,21 +101,7 @@ function viewFilter(unicode) {
   const fieldMatchers = {
     quote: {
       autofalse: ann => (ann.references || []).length > 0,
-      value(annotation) {
-        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];
-        const selectors = target.selector || [];
-
-        return selectors
-          .filter(s => s.type === 'TextQuoteSelector')
-          .map(s => s.exact)
-          .join('\n');
-      },
+      value: ann => quote(ann) || '',
       match: (term, value) => value.indexOf(term) > -1,
     },
     since: {

src/sidebar/util/annotation-metadata.js

@@ -200,6 +200,23 @@ function flagCount(ann) {
   return ann.moderation.flagCount;
 }
 
+/**
+ * Return the text quote that an annotation refers to.
+ *
+ * @return {string|null}
+ */
+function quote(ann) {
+  if (ann.target.length === 0) {
+    return null;
+  }
+  const target = ann.target[0];
+  if (!target.selector) {
+    return null;
+  }
+  const quoteSel = target.selector.find(s => s.type === 'TextQuoteSelector');
+  return quoteSel ? quoteSel.exact : null;
+}
+
 module.exports = {
   documentMetadata: documentMetadata,
   domainAndTitle: domainAndTitle,
@@ -212,4 +229,5 @@ module.exports = {
   isReply: isReply,
   isWaitingToAnchor: isWaitingToAnchor,
   location: location,
+  quote,
 };

src/sidebar/util/test/annotation-metadata-test.js

@@ -372,4 +372,50 @@ describe('annotation-metadata', function() {
       assert.equal(flagCount(ann), 10);
     });
   });
+
+  describe('quote', () => {
+    it('returns quote if annotation has a quote', () => {
+      const ann = {
+        target: [
+          {
+            source: 'https://publisher.org/article.pdf',
+            selector: [{ type: 'TextQuoteSelector', exact: 'expected quote' }],
+          },
+        ],
+      };
+      assert.equal(annotationMetadata.quote(ann), 'expected quote');
+    });
+
+    // FIXME - This currently happens when creating a new Page Note. Annotations
+    // from the API should always have a target.
+    //
+    // See https://github.com/hypothesis/client/issues/1290.
+    it('returns `null` if annotation has an empty target array', () => {
+      const ann = { target: [] };
+      assert.equal(annotationMetadata.quote(ann), null);
+    });
+
+    it('returns `null` if annotation has no selectors', () => {
+      const ann = {
+        target: [
+          {
+            source: 'https://publisher.org/article.pdf',
+          },
+        ],
+      };
+      assert.equal(annotationMetadata.quote(ann), null);
+    });
+
+    it('returns `null` if annotation has no text quote selector', () => {
+      const ann = {
+        target: [
+          {
+            source: 'https://publisher.org/article.pdf',
+            selector: [{ type: 'TextPositionSelector', start: 0, end: 100 }],
+          },
+        ],
+      };
+      assert.equal(annotationMetadata.quote(ann), null);
+    });
+  });
 });