Merge pull request #1 from hypothesis/direct-document-link-from-stream

Show direct links on stream page when available (FIRST!)

h/static/scripts/annotation-metadata.js

@@ -12,7 +12,7 @@
  *   uri, domain and title.
  *
  */
-function extractDocumentMetadata(annotation) {
+function documentMetadata(annotation) {
   var uri = annotation.uri;
   var domain = new URL(uri).hostname;
   var title = domain;
@@ -21,8 +21,8 @@ function extractDocumentMetadata(annotation) {
     title = annotation.document.title[0];
   }
 
-  if (title.length > 30) {
-    title = title.slice(0, 30) + '…';
+  if (domain === 'localhost') {
+    domain = '';
   }
 
   return {
@@ -32,6 +32,61 @@ function extractDocumentMetadata(annotation) {
   };
 }
 
+/**
+ * Return the domain and title of an annotation for display on an annotation
+ * card.
+ */
+function domainAndTitle(annotation) {
+  return {
+    domain: domainTextFromAnnotation(annotation),
+    titleText: titleTextFromAnnotation(annotation),
+    titleLink: titleLinkFromAnnotation(annotation),
+  };
+}
+
+function titleLinkFromAnnotation(annotation) {
+  var titleLink = annotation.uri;
+
+  if (titleLink && !(titleLink.indexOf('http://') === 0 || titleLink.indexOf('https://') === 0)) {
+    // We only link to http(s) URLs.
+    titleLink = null;
+  }
+
+  if (annotation.links && annotation.links.incontext) {
+    titleLink = annotation.links.incontext;
+  }
+
+  return titleLink;
+}
+
+function domainTextFromAnnotation(annotation) {
+  var document = documentMetadata(annotation);
+
+  var domainText = '';
+  if (document.uri && document.uri.indexOf('file://') === 0 && document.title) {
+    var parts = document.uri.split('/');
+    var filename = parts[parts.length - 1];
+    if (filename) {
+      domainText = filename;
+    }
+  } else if (document.domain && document.domain !== document.title) {
+    domainText = document.domain;
+  }
+
+  return domainText;
+}
+
+function titleTextFromAnnotation(annotation) {
+  var document = documentMetadata(annotation);
+
+  var titleText = document.title;
+  if (titleText.length > 30) {
+    titleText = titleText.slice(0, 30) + '…';
+  }
+
+  return titleText;
+}
+
 /** Return `true` if the given annotation is a reply, `false` otherwise. */
 function isReply(annotation) {
   return (annotation.references || []).length > 0;
@@ -78,7 +133,8 @@ function location(annotation) {
 }
 
 module.exports = {
-  extractDocumentMetadata: extractDocumentMetadata,
+  documentMetadata: documentMetadata,
+  domainAndTitle: domainAndTitle,
   isAnnotation: isAnnotation,
   isNew: isNew,
   isPageNote: isPageNote,

h/static/scripts/directive/annotation.js

@@ -4,14 +4,11 @@
 var angular = require('angular');
 
 var annotationMetadata = require('../annotation-metadata');
-var documentDomain = require('../filter/document-domain');
-var documentTitle = require('../filter/document-title');
 var events = require('../events');
 var persona = require('../filter/persona');
 
 var isNew = annotationMetadata.isNew;
 var isReply = annotationMetadata.isReply;
-var extractDocumentMetadata = annotationMetadata.extractDocumentMetadata;
 
 /** Return a human-readable error message for the given server error.
  *
@@ -115,9 +112,7 @@ function updateViewModel($scope, domainModel,
   vm.isPrivate = permissions.isPrivate(
     domainModel.permissions, domainModel.user);
 
-  var documentMetadata = extractDocumentMetadata(domainModel);
-  vm.documentTitle = documentTitle(documentMetadata);
-  vm.documentDomain = documentDomain(documentMetadata);
+  vm.documentMeta = annotationMetadata.domainAndTitle(domainModel);
 }
 
 /**

h/static/scripts/filter/document-domain.js

-'use strict';
-
-var escapeHtml = require('escape-html');
-
-/**
- * Return a nice displayable string representation of a document's domain.
- *
- * @returns {String} The document's domain in braces, e.g. '(example.com)'.
- *   Returns '' if the document has no domain or if the document's domain is
- *   the same as its title (because we assume that the title is already
- *   displayed elsewhere and displaying it twice would be redundant).
- *
- */
-module.exports = function documentDomain(document) {
-  var uri = escapeHtml(document.uri || '');
-  var domain = escapeHtml(document.domain || '');
-  var title = escapeHtml(document.title || '');
-
-  if (uri.indexOf('file://') === 0 && title) {
-    var parts = uri.split('/');
-    var filename = parts[parts.length - 1];
-    if (filename) {
-      return '(' + decodeURIComponent(filename) + ')';
-    }
-  }
-
-  if (domain && domain !== title) {
-    return '(' + decodeURIComponent(domain) + ')';
-  } else {
-    return '';
-  }
-};

h/static/scripts/filter/document-title.js

-'use strict';
-
-var escapeHtml  = require('escape-html');
-
-/**
- * Return a nice displayable string representation of a document's title.
- *
- * @returns {String} The document's title preceded on "on " and hyperlinked
- *   to the document's URI. If the document has no http(s) URI then don't
- *   hyperlink the title. If the document has no title then return ''.
- *
- */
-module.exports = function documentTitle(document) {
-  var title = escapeHtml(document.title || '');
-  var uri = escapeHtml(document.uri || '');
-
-  if (uri && !(uri.indexOf('http://') === 0 || uri.indexOf('https://') === 0)) {
-    // We only link to http(s) URLs.
-    uri = null;
-  }
-
-  if (title && uri) {
-    return ('on &ldquo;<a target="_blank" href="' + uri + '">' + title +
-            '</a>&rdquo;');
-  } else if (title) {
-    return 'on &ldquo;' + title + '&rdquo;';
-  } else {
-    return '';
-  }
-};

h/static/scripts/filter/test/document-domain-test.js

-'use strict';
-
-var documentDomain = require('../document-domain');
-
-describe('documentDomain', function() {
-
-  it('returns the domain in braces', function() {
-    var domain = documentDomain({
-      domain: 'example.com'
-    });
-
-    assert(domain === '(example.com)');
-  });
-
-  it('returns an empty string if domain and title are the same', function() {
-    var domain = documentDomain({
-      domain: 'example.com',
-      title: 'example.com'
-    });
-
-    assert(domain === '');
-  });
-
-  it('returns an empty string if the document has no domain', function() {
-    var domain = documentDomain({
-      title: 'example.com'
-    });
-
-    assert(domain === '');
-  });
-
-  it('returns the filename for local documents with titles', function() {
-    var domain = documentDomain({
-      title: 'example.com',
-      uri: 'file:///home/seanh/MyFile.pdf'
-    });
-
-    assert(domain === '(MyFile.pdf)');
-  });
-
-  it('replaces %20 with " "', function() {
-    var domain = documentDomain({
-      title: 'example.com',
-      uri: 'file:///home/seanh/My%20File.pdf'
-    });
-
-    assert(domain === '(My File.pdf)');
-  });
-
-  it('escapes HTML in the document domain', function() {
-    var spamLink = '<a href="http://example.com/rubies">Buy rubies!!!</a>';
-
-    var domain = documentDomain({
-      title: 'title',
-      domain: '</a>' + spamLink
-    });
-
-    assert(domain.indexOf(spamLink) === -1);
-  });
-
-  it('escapes HTML in the document uri', function() {
-    var spamLink = '<a href="http://example.com/rubies">Buy rubies!!!</a>';
-
-    var domain = documentDomain({
-      title: 'title',
-      uri: 'file:///home/seanh/' + spamLink
-    });
-
-    assert(domain.indexOf(spamLink) === -1);
-  });
-});

h/static/scripts/filter/test/document-title-test.js

-'use strict';
-
-var documentTitle = require('../document-title');
-
-describe('documentTitle', function() {
-
-  it('returns the title linked if the document has title and uri', function() {
-    var title = documentTitle({
-      title: 'title',
-      uri: 'http://example.com/example.html'
-    });
-
-    assert(title === 'on &ldquo;<a target="_blank" ' +
-                     'href="http://example.com/example.html">' +
-                     'title</a>&rdquo;');
-  });
-
-  it('returns the title linked if the document has an https uri', function() {
-    var title = documentTitle({
-      title: 'title',
-      uri: 'https://example.com/example.html'
-    });
-
-    assert(title === 'on &ldquo;<a target="_blank" '+
-                     'href="https://example.com/example.html">' +
-                     'title</a>&rdquo;');
-  });
-
-  it('returns the title unlinked if doc has title but no uri', function() {
-    var title = documentTitle({
-      title: 'title',
-    });
-
-    assert(title === 'on &ldquo;title&rdquo;');
-  });
-
-  it('returns the title unlinked if doc has non-http uri', function() {
-    var title = documentTitle({
-      title: 'title',
-      uri: 'file:///home/bob/Documents/example.pdf'
-    });
-
-    assert(title === 'on &ldquo;title&rdquo;');
-  });
-
-  it('returns an empty string if the document has no title', function() {
-    var title = documentTitle({
-      uri: 'http://example.com/example.html'
-    });
-
-    assert(title === '');
-  });
-
-  it('escapes HTML in the document title', function() {
-    var spamLink = '<a href="http://example.com/rubies">Buy rubies!!!</a>';
-
-    var title = documentTitle({
-      title: '</a>' + spamLink,
-      uri: 'http://example.com/example.html'
-    });
-
-    assert(title.indexOf(spamLink) === -1);
-  });
-
-  it('escapes HTML in the document URI', function() {
-    var spamLink = '<a href="http://example.com/rubies">Buy rubies!!!</a>';
-
-    var title = documentTitle({
-      uri: 'http://</a>' + spamLink,
-      title: 'title'
-    });
-
-    assert(title.indexOf(spamLink) === -1);
-  });
-});

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

@@ -2,10 +2,11 @@
 
 var annotationMetadata = require('../annotation-metadata');
 
-var extractDocumentMetadata = annotationMetadata.extractDocumentMetadata;
+var documentMetadata = annotationMetadata.documentMetadata;
+var domainAndTitle = annotationMetadata.domainAndTitle;
 
 describe('annotation-metadata', function () {
-  describe('.extractDocumentMetadata', function() {
+  describe('.documentMetadata', function() {
 
     context('when the model has a document property', function() {
       it('returns the hostname from model.uri as the domain', function() {
@@ -14,7 +15,7 @@ describe('annotation-metadata', function () {
           uri: 'http://example.com/'
         };
 
-        assert.equal(extractDocumentMetadata(model).domain, 'example.com');
+        assert.equal(documentMetadata(model).domain, 'example.com');
       });
 
       context('when model.uri does not start with "urn"', function() {
@@ -25,7 +26,7 @@ describe('annotation-metadata', function () {
           };
 
           assert.equal(
-            extractDocumentMetadata(model).uri, 'http://example.com/');
+            documentMetadata(model).uri, 'http://example.com/');
         });
       });
 
@@ -39,7 +40,7 @@ describe('annotation-metadata', function () {
           };
 
           assert.equal(
-            extractDocumentMetadata(model).title, model.document.title[0]);
+            documentMetadata(model).title, model.document.title[0]);
         });
       });
 
@@ -50,7 +51,7 @@ describe('annotation-metadata', function () {
             uri: 'http://example.com/',
           };
 
-          assert.equal(extractDocumentMetadata(model).title, 'example.com');
+          assert.equal(documentMetadata(model).title, 'example.com');
         });
       });
     });
@@ -59,24 +60,70 @@ describe('annotation-metadata', function () {
       it('returns model.uri for the uri', function() {
         var model = {uri: 'http://example.com/'};
 
-        assert.equal(extractDocumentMetadata(model).uri, model.uri);
+        assert.equal(documentMetadata(model).uri, model.uri);
       });
 
       it('returns the hostname of model.uri for the domain', function() {
         var model = {uri: 'http://example.com/'};
 
-        assert.equal(extractDocumentMetadata(model).domain, 'example.com');
+        assert.equal(documentMetadata(model).domain, 'example.com');
       });
 
       it('returns the hostname of model.uri for the title', function() {
         var model = {uri: 'http://example.com/'};
 
-        assert.equal(extractDocumentMetadata(model).title, 'example.com');
+        assert.equal(documentMetadata(model).title, 'example.com');
       });
     });
+  });
+
+  describe('.domainAndTitle', function() {
+    context('when an annotation has a non-http(s) uri', function () {
+      it('returns no title link', function () {
+        var model = {
+          uri: 'file:///example.pdf',
+        };
 
-    context('when the title is longer than 30 characters', function() {
-      it('truncates the title with "…"', function() {
+        assert.equal(domainAndTitle(model).titleLink, null);
+      });
+    });
+
+    context('when an annotation has a direct link', function () {
+      it('returns the direct link as a title link', function () {
+        var model = {
+          links: {
+            incontext: 'https://example.com',
+          }
+        };
+
+        assert.equal(domainAndTitle(model).titleLink, 'https://example.com');
+      });
+    });
+
+    context('when an annotation has no direct link but has a http(s) uri', function () {
+      it('returns the uri as title link', function () {
+        var model = {
+          uri: 'https://example.com',
+        };
+
+        assert.equal(domainAndTitle(model).titleLink, 'https://example.com');
+      });
+    });
+
+    context('when the annotation title is shorter than 30 characters', function () {
+      it('returns the annotation title as title text', function () {
+        var model = {
+          document: {
+            title: ['A Short Document Title'],
+          },
+        };
+
+        assert.equal(domainAndTitle(model).titleText, 'A Short Document Title');
+      });
+    });
+
+    context('when the annotation title is longer than 30 characters', function() {
+      it('truncates the title text with "…"', function() {
         var model = {
           uri: 'http://example.com/',
           document: {
@@ -85,11 +132,62 @@ describe('annotation-metadata', function () {
         };
 
         assert.equal(
-          extractDocumentMetadata(model).title,
+          domainAndTitle(model).titleText,
           'My Really Really Long Document…'
         );
       });
     });
+
+    context('when the document uri refers to a filename', function () {
+      it('returns the filename as domain text', function () {
+        var model = {
+          uri: 'file:///path/to/example.pdf',
+          document: {
+            title: ['Document Title'],
+          },
+        };
+
+        assert.equal(domainAndTitle(model).domain, 'example.pdf');
+      });
+    });
+
+    context('when domain and title are the same', function () {
+      it('returns an empty domain text string', function() {
+        var model = {
+          uri: 'https://example.com',
+          document : {
+            title: ['example.com'],
+          },
+        };
+
+        assert.equal(domainAndTitle(model).domain, '');
+      });
+    });
+
+    context('when the document has no domain', function () {
+      it('returns an empty domain text string', function() {
+        var model = {
+          document : {
+            title: ['example.com'],
+          },
+        };
+
+        assert.equal(domainAndTitle(model).domain, '');
+      });
+    });
+
+    context('when the document is a local file with a title', function () {
+      it('returns the filename', function() {
+        var model = {
+          uri: 'file:///home/seanh/MyFile.pdf',
+          document: {
+            title: ['example.com'],
+          },
+        };
+
+        assert.equal(domainAndTitle(model).domain, 'MyFile.pdf');
+      });
+    });
   });
 
   describe('.location', function () {

h/templates/client/annotation.html

@@ -27,13 +27,15 @@
           <i class="h-icon-lock"></i><span class="annotation-header__group-name" ng-show="!vm.group().url">Only me</span>
         </span>
         <i class="h-icon-border-color" ng-show="vm.isHighlight() && !vm.editing()" title="This is a highlight. Click 'edit' to add a note or tag."></i>
-        <span class="annotation-citation"
-          ng-bind-html="vm.documentTitle"
-          ng-if="::vm.showDocumentInfo">
-        </span>
-        <span class="annotation-citation-domain"
-          ng-bind-html="vm.documentDomain"
-          ng-if="::vm.showDocumentInfo">
+        <span ng-if="::vm.showDocumentInfo">
+          <span class="annotation-citation" ng-if="vm.documentMeta.titleLink">
+            on "<a ng-href="{{vm.documentMeta.titleLink}}">{{vm.documentMeta.titleText}}</a>"
+          </span>
+          <span class="annotation-citation" ng-if="!vm.documentMeta.titleLink">
+            on "{{vm.documentMeta.titleText}}"
+          </span>
+          <span class="annotation-citation-domain"
+                ng-if="vm.documentMeta.domain">({{vm.documentMeta.domain}})</span>
         </span>
       </span>
     </span>