Merge pull request #3524 from hypothesis/update-pdf-fingerprint-handling

Update PDF fingerprint searching / handling

h/static/scripts/annotator/plugin/pdf-metadata.js

@@ -27,7 +27,11 @@ function PDFMetadata(app) {
  */
 PDFMetadata.prototype.getUri = function () {
   return this._loaded.then(function (app) {
-    return fingerprintToURN(app.documentFingerprint);
+    var uri = getPDFURL(app);
+    if (!uri) {
+      uri = fingerprintToURN(app.documentFingerprint);
+    }
+    return uri;
   });
 };
 
@@ -52,11 +56,9 @@ PDFMetadata.prototype.getMetadata = function () {
       {href: fingerprintToURN(app.documentFingerprint)}
     ];
 
-    // Local file:// URLs should not be saved in document metadata.
-    // Entries in document.link should be URIs. In the case of
-    // local files, omit the URL.
-    if (app.url.indexOf('file://') !== 0) {
-      link.push({href: app.url});
+    var url = getPDFURL(app);
+    if (url) {
+      link.push({href: url});
     }
 
     return {
@@ -71,4 +73,15 @@ function fingerprintToURN(fingerprint) {
   return 'urn:x-pdf:' + String(fingerprint);
 }
 
+function getPDFURL(app) {
+  // Local file:// URLs should not be saved in document metadata.
+  // Entries in document.link should be URIs. In the case of
+  // local files, omit the URL.
+  if (app.url.indexOf('file://') !== 0) {
+    return app.url;
+  }
+
+  return null;
+}
+
 module.exports = PDFMetadata;

h/static/scripts/annotator/plugin/test/pdf-metadata-test.js

@@ -9,19 +9,23 @@ describe('pdf-metadata', function () {
 
     var event = document.createEvent('Event');
     event.initEvent('documentload', false, false);
+    fakeApp.url = 'http://fake.com';
     fakeApp.documentFingerprint = 'fakeFingerprint';
     window.dispatchEvent(event);
 
     return pdfMetadata.getUri().then(function (uri) {
-      assert.equal(uri, 'urn:x-pdf:fakeFingerprint');
+      assert.equal(uri, 'http://fake.com');
     });
   });
 
   it('does not wait for the PDF to load if it has already loaded', function () {
-    var fakePDFViewerApplication = {documentFingerprint: 'fakeFingerprint'};
+    var fakePDFViewerApplication = {
+      url: 'http://fake.com',
+      documentFingerprint: 'fakeFingerprint',
+    };
     var pdfMetadata = new PDFMetadata(fakePDFViewerApplication);
     return pdfMetadata.getUri().then(function (uri) {
-      assert.equal(uri, 'urn:x-pdf:fakeFingerprint');
+      assert.equal(uri, 'http://fake.com');
     });
   });
 
@@ -37,7 +41,7 @@ describe('pdf-metadata', function () {
           'dc:title': 'fakeTitle',
         }
       },
-      url: 'fakeUrl',
+      url: 'http://fake.com',
     };
 
     beforeEach(function () {
@@ -45,7 +49,19 @@ describe('pdf-metadata', function () {
     });
 
     describe('#getUri', function () {
-      it('returns the URN-ified document fingerprint as its URI', function () {
+      it('returns the non-file URI', function() {
+        return pdfMetadata.getUri().then(function (uri) {
+          assert.equal(uri, 'http://fake.com');
+        });
+      });
+
+      it('returns the fingerprint as a URN when the PDF URL is a local file', function () {
+        var fakePDFViewerApplication = {
+          url: 'file:///test.pdf',
+          documentFingerprint: 'fakeFingerprint',
+        };
+        var pdfMetadata = new PDFMetadata(fakePDFViewerApplication);
+
         return pdfMetadata.getUri().then(function (uri) {
           assert.equal(uri, 'urn:x-pdf:fakeFingerprint');
         });
@@ -88,7 +104,7 @@ describe('pdf-metadata', function () {
         var pdfMetadata;
         var fakePDFViewerApplication = {
           documentFingerprint: 'fakeFingerprint',
-          url: 'file://fakeUrl',
+          url: 'file://fake.pdf',
         };
         var expectedMetadata = {
           link: [{href: 'urn:x-pdf:' + fakePDFViewerApplication.documentFingerprint}],

h/static/scripts/cross-frame.coffee

@@ -46,11 +46,20 @@ module.exports = class CrossFrame
         if err
           channel.destroy()
         else
+          searchUris = [info.uri]
+
           documentFingerprint = null
           if info.metadata and info.metadata.documentFingerprint
             documentFingerprint = info.metadata.documentFingerprint
+            searchUris = info.metadata.link.map((link) -> link.href)
+
           $rootScope.$apply =>
-            @frames.push({channel: channel, uri: info.uri, documentFingerprint: documentFingerprint})
+            @frames.push({
+              channel: channel,
+              uri: info.uri,
+              searchUris: searchUris,
+              documentFingerprint: documentFingerprint
+            })
 
     this.connect = ->
       discovery = createDiscovery()

h/static/scripts/test/cross-frame-test.coffee

@@ -72,5 +72,23 @@ describe 'CrossFrame', ->
       fakeBridge.onConnect.yields(channel)
       crossframe.connect()
       assert.deepEqual(crossframe.frames, [
-        {channel: channel, uri: uri, documentFingerprint: null}
+        {channel: channel, uri: uri, searchUris: [uri], documentFingerprint: null}
+      ])
+
+    it 'updates the frames array with multiple search uris when the document is a PDF', ->
+      uri = 'http://example.com/test.pdf'
+      fingerprint = 'urn:x-pdf:fingerprint'
+      channel = {
+        call: sandbox.stub().yields(null, {
+          uri: uri,
+          metadata: {
+            link: [{href: uri}, {href: fingerprint}],
+            documentFingerprint: fingerprint,
+          },
+        })
+      }
+      fakeBridge.onConnect.yields(channel)
+      crossframe.connect()
+      assert.deepEqual(crossframe.frames, [
+        {channel: channel, uri: uri, searchUris: [uri, fingerprint], documentFingerprint: fingerprint}
       ])

h/static/scripts/test/widget-controller-test.js

@@ -19,8 +19,12 @@ function FakeSearchClient(resource, opts) {
   this.get = sinon.spy(function (query) {
     assert.ok(query.uri);
 
-    this.emit('results', [{id: query.uri + '123', group: '__world__'}]);
-    this.emit('results', [{id: query.uri + '456', group: 'private-group'}]);
+    for (var i = 0; i < query.uri.length; i++) {
+      var uri = query.uri[i];
+      this.emit('results', [{id: uri + '123', group: '__world__'}]);
+      this.emit('results', [{id: uri + '456', group: 'private-group'}]);
+    }
+
     this.emit('end');
   });
 }
@@ -143,10 +147,12 @@ describe('WidgetController', function () {
       // When new clients connect, all existing annotations should be unloaded
       // before reloading annotations for each currently-connected client
       annotationUI.addAnnotations([{id: '123'}]);
-      fakeCrossFrame.frames.push({uri: 'http://example.com/page-a'});
+      var uri1 = 'http://example.com/page-a';
+      fakeCrossFrame.frames.push({uri: uri1, searchUris: [uri1]});
       $scope.$digest();
       fakeAnnotationMapper.unloadAnnotations = sandbox.spy();
-      fakeCrossFrame.frames.push({uri: 'http://example.com/page-b'});
+      var uri2 = 'http://example.com/page-b';
+      fakeCrossFrame.frames.push({uri: uri2, searchUris: [uri2]});
       $scope.$digest();
       assert.calledWith(fakeAnnotationMapper.unloadAnnotations,
         annotationUI.getState().annotations);
@@ -154,17 +160,29 @@ describe('WidgetController', function () {
 
     it('loads all annotations for a frame', function () {
       var uri = 'http://example.com';
-      fakeCrossFrame.frames.push({uri: uri});
+      fakeCrossFrame.frames.push({uri: uri, searchUris: [uri]});
+      $scope.$digest();
+      var loadSpy = fakeAnnotationMapper.loadAnnotations;
+      assert.calledWith(loadSpy, [sinon.match({id: uri + '123'})]);
+      assert.calledWith(loadSpy, [sinon.match({id: uri + '456'})]);
+    });
+
+    it('loads all annotations for a frame with multiple urls', function () {
+      var uri = 'http://example.com/test.pdf';
+      var fingerprint = 'urn:x-pdf:fingerprint';
+      fakeCrossFrame.frames.push({uri: uri, searchUris: [uri, fingerprint]});
       $scope.$digest();
       var loadSpy = fakeAnnotationMapper.loadAnnotations;
       assert.calledWith(loadSpy, [sinon.match({id: uri + '123'})]);
+      assert.calledWith(loadSpy, [sinon.match({id: fingerprint + '123'})]);
       assert.calledWith(loadSpy, [sinon.match({id: uri + '456'})]);
+      assert.calledWith(loadSpy, [sinon.match({id: fingerprint + '456'})]);
     });
 
     it('loads all annotations for all frames', function () {
       var uris = ['http://example.com', 'http://foobar.com'];
       fakeCrossFrame.frames = uris.map(function (uri) {
-        return {uri: uri};
+        return {uri: uri, searchUris: [uri]};
       });
       $scope.$digest();
       var loadSpy = fakeAnnotationMapper.loadAnnotations;
@@ -179,7 +197,7 @@ describe('WidgetController', function () {
       var id = uri + '123';
 
       beforeEach(function () {
-        fakeCrossFrame.frames = [{uri: uri}];
+        fakeCrossFrame.frames = [{uri: uri, searchUris: [uri]}];
         annotationUI.selectAnnotations([id]);
         $scope.$digest();
       });
@@ -197,7 +215,7 @@ describe('WidgetController', function () {
       });
 
       it('fetches annotations for all groups', function () {
-        assert.calledWith(searchClients[0].get, {uri: uri, group: null});
+        assert.calledWith(searchClients[0].get, {uri: [uri], group: null});
       });
 
       it('loads annotations in one batch', function () {
@@ -209,7 +227,7 @@ describe('WidgetController', function () {
       var uri = 'http://example.com';
 
       beforeEach(function () {
-        fakeCrossFrame.frames = [{uri: uri}];
+        fakeCrossFrame.frames = [{uri: uri, searchUris: [uri]}];
         fakeGroups.focused = function () { return { id: 'a-group' }; };
         $scope.$digest();
       });
@@ -219,7 +237,7 @@ describe('WidgetController', function () {
       });
 
       it('fetches annotations for the current group', function () {
-        assert.calledWith(searchClients[0].get, {uri: uri, group: 'a-group'});
+        assert.calledWith(searchClients[0].get, {uri: [uri], group: 'a-group'});
       });
 
       it('loads annotations in batches', function () {
@@ -232,7 +250,7 @@ describe('WidgetController', function () {
       var id = uri + 'does-not-exist';
 
       beforeEach(function () {
-        fakeCrossFrame.frames = [{uri: uri}];
+        fakeCrossFrame.frames = [{uri: uri, searchUris: [uri]}];
         annotationUI.selectAnnotations([id]);
         fakeGroups.focused = function () { return { id: 'private-group' }; };
         $scope.$digest();
@@ -250,7 +268,7 @@ describe('WidgetController', function () {
     it('focuses and scrolls to the annotation if already selected', function () {
       var uri = 'http://example.com';
       annotationUI.selectAnnotations(['123']);
-      fakeCrossFrame.frames.push({uri: uri});
+      fakeCrossFrame.frames.push({uri: uri, searchUris: [uri]});
       var annot = {
         $$tag: 'atag',
         id: '123',
@@ -272,7 +290,7 @@ describe('WidgetController', function () {
 
       fakeDrafts.unsaved.returns([{id: uri + '123'}, {id: uri + '456'}]);
 
-      fakeCrossFrame.frames.push({uri: uri});
+      fakeCrossFrame.frames.push({uri: uri, searchUris: [uri]});
       var loadSpy = fakeAnnotationMapper.loadAnnotations;
 
       $scope.$broadcast(events.GROUP_FOCUSED);

h/static/scripts/widget-controller.js

@@ -143,7 +143,7 @@ module.exports = function WidgetController(
     annotationUI.addAnnotations(drafts.unsaved());
   }
 
-  function _loadAnnotationsFor(uri, group) {
+  function _loadAnnotationsFor(uris, group) {
     var searchClient = new SearchClient(store.SearchResource, {
       // If no group is specified, we are fetching annotations from
       // all groups in order to find out which group contains the selected
@@ -177,7 +177,7 @@ module.exports = function WidgetController(
       // Remove client from list of active search clients
       searchClients.splice(searchClients.indexOf(searchClient), 1);
     });
-    searchClient.get({uri: uri, group: group});
+    searchClient.get({uri: uris, group: group});
   }
 
   function isLoading() {
@@ -197,12 +197,14 @@ module.exports = function WidgetController(
       client.cancel();
     });
 
-    var urls = frames.reduce(function (urls, frame) {
-      if (urls.indexOf(frame.uri) !== -1) {
-        return urls;
-      } else {
-        return urls.concat(frame.uri);
+    var searchUris = frames.reduce(function (uris, frame) {
+      for (var i = 0; i < frame.searchUris.length; i++) {
+        var uri = frame.searchUris[i];
+        if (uris.indexOf(uri) === -1) {
+          uris.push(uri);
+        }
       }
+      return uris;
     }, []);
 
     // If there is no selection, load annotations only for the focused group.
@@ -219,12 +221,10 @@ module.exports = function WidgetController(
     var group = annotationUI.hasSelectedAnnotations() ?
       null : groups.focused().id;
 
-    for (var i=0; i < urls.length; i++) {
-      _loadAnnotationsFor(urls[i], group);
-    }
+    if (searchUris.length > 0) {
+      _loadAnnotationsFor(searchUris, group);
 
-    if (urls.length > 0) {
-      streamFilter.resetFilter().addClause('/uri', 'one_of', urls);
+      streamFilter.resetFilter().addClause('/uri', 'one_of', searchUris);
       streamer.setConfig('filter', {filter: streamFilter.getFilter()});
     }
   }