Fix broken card titles for some annotations

This fixes two issues on the annotation cards for some annotations:

1. If the annotated document has a non-http(s) URI (e.g. a local file://
   URI) then don't hyperlink the document's title to that URI

2. If the annotated document has no .domain value or its domain is the
   same as its title then don't display anything, instead of displaying
   a pair of emtpy braces ().

To fix this move the logic for formatting an annotation's document's title
and domain into AngularJS filters where it can be tested, instead of in
the template.

h/static/scripts/app.coffee

@@ -152,6 +152,8 @@ module.exports = angular.module('h', [
 .filter('moment', require('./filter/moment'))
 .filter('persona', require('./filter/persona'))
 .filter('urlencode', require('./filter/urlencode'))
+.filter('documentTitle', require('./filter/document-title'))
+.filter('documentDomain', require('./filter/document-domain'))
 
 .provider('identity', require('./identity'))
 

h/static/scripts/directive/test/annotation-test.coffee

@@ -18,6 +18,8 @@ describe 'annotation', ->
   fakeMomentFilter = null
   fakePermissions = null
   fakePersonaFilter = null
+  fakeDocumentTitleFilter = null
+  fakeDocumentDomainFilter = null
   fakeSession = null
   fakeStore = null
   fakeTags = null
@@ -65,6 +67,8 @@ describe 'annotation', ->
       private: sandbox.stub().returns({read: ['justme']})
     }
     fakePersonaFilter = sandbox.stub().returnsArg(0)
+    fakeDocumentTitleFilter = (arg) -> ''
+    fakeDocumentDomainFilter = (arg) -> ''
 
     fakeSession =
       state:
@@ -92,6 +96,8 @@ describe 'annotation', ->
     $provide.value 'momentFilter', fakeMomentFilter
     $provide.value 'permissions', fakePermissions
     $provide.value 'personaFilter', fakePersonaFilter
+    $provide.value('documentTitleFilter', fakeDocumentTitleFilter)
+    $provide.value('documentDomainFilter', fakeDocumentDomainFilter)
     $provide.value 'session', fakeSession
     $provide.value 'store', fakeStore
     $provide.value 'tags', fakeTags
@@ -564,7 +570,8 @@ describe("AnnotationController", ->
   createAnnotationDirective = ({annotation, personaFilter, momentFilter,
                                 urlencodeFilter, drafts, flash,
                                 permissions, session, tags, time, annotationUI,
-                                annotationMapper, groups}) ->
+                                annotationMapper, groups,
+                                documentTitleFilter, documentDomainFilter}) ->
     locals = {
       personaFilter: personaFilter or ->
       momentFilter: momentFilter or {}
@@ -594,6 +601,8 @@ describe("AnnotationController", ->
         get: ->
         focused: -> {}
       }
+      documentTitleFilter: documentTitleFilter or -> ''
+      documentDomainFilter: documentDomainFilter or -> ''
     }
     module(($provide) ->
       $provide.value("personaFilter", locals.personaFilter)
@@ -608,6 +617,8 @@ describe("AnnotationController", ->
       $provide.value("annotationUI", locals.annotationUI)
       $provide.value("annotationMapper", locals.annotationMapper)
       $provide.value("groups", locals.groups)
+      $provide.value("documentTitleFilter", locals.documentTitleFilter)
+      $provide.value("documentDomainFilter", locals.documentDomainFilter)
       return
     )
 

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

+'use strict';
+
+var escapeHtml = require('escape-html');
+
+module.exports = function() {
+  /**
+   * 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).
+   *
+   */
+  function documentDomainFilter(document) {
+    var domain = escapeHtml(document.domain || '');
+    var title = escapeHtml(document.title || '');
+
+    if (domain && domain !== title) {
+      return '(' + domain + ')';
+    } else {
+      return '';
+    }
+  }
+  return documentDomainFilter;
+};

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

+'use strict';
+
+var escapeHtml  = require('escape-html');
+
+module.exports = function() {
+  /**
+   * 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 ''.
+   *
+   */
+  function documentTitleFilter(document) {
+    var title = escapeHtml(document.title || '');
+    var uri = escapeHtml(document.uri || '');
+
+    if (uri && !(uri.startsWith('http://') || uri.startsWith('https://'))) {
+      // 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 '';
+    }
+  }
+  return documentTitleFilter;
+};

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

+'use strict';
+
+var documentDomainFilterProvider = require('../document-domain');
+
+describe('documentDomain', function() {
+
+  it('returns the domain in braces', function() {
+    var domain = documentDomainFilterProvider()({
+      domain: 'example.com'
+    });
+
+    assert(domain === '(example.com)');
+  });
+
+  it('returns an empty string if domain and title are the same', function() {
+    var domain = documentDomainFilterProvider()({
+      domain: 'example.com',
+      title: 'example.com'
+    });
+
+    assert(domain === '');
+  });
+
+  it('returns an empty string if the document has no domain', function() {
+    var domain = documentDomainFilterProvider()({
+      title: 'example.com'
+    });
+
+    assert(domain === '');
+  });
+
+  it('escapes HTML in the document domain', function() {
+    var spamLink = '<a href="http://example.com/rubies">Buy rubies!!!</a>';
+
+    var domain = documentDomainFilterProvider()({
+      title: 'title',
+      domain: '</a>' + spamLink
+    });
+
+    assert(domain.indexOf(spamLink) === -1);
+  });
+});

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

+'use strict';
+
+var documentTitleFilterProvider = require('../document-title');
+
+describe('documentTitle', function() {
+
+  it('returns the title linked if the document has title and uri', function() {
+    var title = documentTitleFilterProvider()({
+      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 = documentTitleFilterProvider()({
+      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 = documentTitleFilterProvider()({
+      title: 'title',
+    });
+
+    assert(title === 'on &ldquo;title&rdquo;');
+  });
+
+  it('returns the title unlinked if doc has non-http uri', function() {
+    var title = documentTitleFilterProvider()({
+      title: 'title',
+      uri: 'file:///home/bob/Documents/example.pdf'
+    });
+
+    assert(title === 'on &ldquo;title&rdquo;');
+  });
+
+  it('returns the title linked if the document has title and uri', function() {
+    var title = documentTitleFilterProvider()({
+      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 an empty string if the document has no title', function() {
+    var title = documentTitleFilterProvider()({
+      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 = documentTitleFilterProvider()({
+      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 = documentTitleFilterProvider()({
+      uri: 'http://</a>' + spamLink,
+      title: 'title'
+    });
+
+    assert(title.indexOf(spamLink) === -1);
+  });
+});

h/templates/client/annotation.html

@@ -25,15 +25,16 @@
             title="This annotation is visible only to you.">
         <i class="h-icon-lock"></i> Only Me
       </span>
+
       <span class="annotation-citation"
-            ng-if="!vm.embedded"
-            ng-show="vm.document.title">
-        on &ldquo;<a href="{{vm.document.uri}}" target="_blank"
-                     >{{vm.document.title}}</a>&rdquo;
+            ng-bind-html="vm.document | documentTitle"
+            ng-if="!vm.embedded">
+      </span>
       <span class="annotation-citation-domain"
-              ng-show="vm.document.domain != vm.document.title"
-              >({{vm.document.domain}})</span>
+            ng-bind-html="vm.document | documentDomain"
+            ng-if="!vm.embedded">
       </span>
+
       <!-- Editing controls -->
       <aside class="pull-right" ng-if="vm.editing">
         <privacy ng-click="$event.stopPropagation()"

package.json

@@ -29,6 +29,7 @@
     "dom-anchor-text-position": "^2.0.0",
     "dom-anchor-text-quote": "^2.0.0",
     "dom-seek": "^1.0.1",
+    "escape-html": "^1.0.3",
     "extend": "^2.0.0",
     "frame-rpc": "^1.3.1",
     "hammerjs": "^2.0.4",