Merge pull request #2963 from hypothesis/convert-filters-to-functions

Convert filters in the annotation directive to plain functions

h/static/scripts/app.coffee

@@ -113,10 +113,6 @@ module.exports = angular.module('h', [
 .directive('topBar', require('./directive/top-bar'))
 
 .filter('converter', require('./filter/converter'))
-.filter('persona', require('./filter/persona').filter)
-.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/annotation.js

@@ -4,7 +4,10 @@
 var Promise = require('core-js/library/es6/promise');
 
 var dateUtil = require('../date-util');
+var documentDomain = require('../filter/document-domain');
+var documentTitle = require('../filter/document-title');
 var events = require('../events');
+var persona = require('../filter/persona');
 
 /** Return a domainModel tags array from the given vm tags array.
  *
@@ -192,6 +195,10 @@ function updateViewModel($scope, time, domainModel, vm, permissions) {
      });
     updateTimestamp();
   }
+
+  var documentMetadata = extractDocumentMetadata(domainModel);
+  vm.documentTitle = documentTitle(documentMetadata);
+  vm.documentDomain = documentDomain(documentMetadata);
 }
 
 /** Return a vm tags array from the given domainModel tags array.
@@ -460,11 +467,6 @@ function AnnotationController(
     }, true);
   };
 
-  /** Return some metadata extracted from the annotated document. */
-  vm.document = function() {
-    return extractDocumentMetadata(domainModel);
-  };
-
   /**
     * @ngdoc method
     * @name annotation.AnnotationController#edit
@@ -728,6 +730,10 @@ function AnnotationController(
     return $q.when(tags.filter(query));
   };
 
+  vm.tagStreamURL = function(tag) {
+    return vm.serviceUrl + 'stream?q=tag:' + encodeURIComponent(tag);
+  };
+
   vm.target = function() {
     return domainModel.target;
   };
@@ -738,7 +744,11 @@ function AnnotationController(
 
   vm.user = function() {
     return domainModel.user;
-  }
+  };
+
+  vm.username = function() {
+    return persona.username(domainModel.user);
+  };
 
   /** Sets whether or not the controls for
    * expanding/collapsing the body of lengthy annotations

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

@@ -2,6 +2,7 @@
 'use strict';
 
 var Promise = require('core-js/library/es6/promise');
+var proxyquire = require('proxyquire');
 
 var events = require('../../events');
 var util = require('./util');
@@ -9,6 +10,23 @@ var util = require('./util');
 var module = angular.mock.module;
 var inject = angular.mock.inject;
 
+/**
+ * Returns the annotation directive with helpers stubbed out.
+ */
+function annotationDirective() {
+  var noop = function () { return '' };
+
+  var annotation = proxyquire('../annotation', {
+    '../filter/document-domain': noop,
+    '../filter/document-title': noop,
+    '../filter/persona': {
+      username: noop,
+    }
+  });
+
+  return annotation.directive;
+}
+
 /** Return Angular's $compile service. */
 function compileService() {
   var $compile;
@@ -534,7 +552,7 @@ describe('annotation', function() {
 
     before(function() {
       angular.module('h', [])
-        .directive('annotation', require('../annotation').directive);
+        .directive('annotation', annotationDirective());
     });
 
     beforeEach(module('h'));
@@ -588,16 +606,6 @@ describe('annotation', function() {
         setDefault: sandbox.stub()
       };
 
-      fakePersonaFilter = sandbox.stub().returnsArg(0);
-
-      fakeDocumentTitleFilter = function(arg) {
-        return '';
-      };
-
-      fakeDocumentDomainFilter = function(arg) {
-        return '';
-      };
-
       fakeSession = {
         state: {
           userid: 'acct:bill@localhost'
@@ -618,10 +626,6 @@ describe('annotation', function() {
         decayingInterval: function () {},
       };
 
-      fakeUrlEncodeFilter = function(v) {
-        return encodeURIComponent(v);
-      };
-
       fakeGroups = {
         focused: function() {
           return {};
@@ -636,14 +640,10 @@ describe('annotation', function() {
       $provide.value('flash', fakeFlash);
       $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('settings', fakeSettings);
       $provide.value('tags', fakeTags);
       $provide.value('time', fakeTime);
-      $provide.value('urlencodeFilter', fakeUrlEncodeFilter);
       $provide.value('groups', fakeGroups);
     }));
 
@@ -1598,7 +1598,7 @@ describe('annotation', function() {
   describe('AnnotationController', function() {
     before(function() {
       angular.module('h', [])
-        .directive('annotation', require('../annotation').directive);
+        .directive('annotation', annotationDirective());
     });
 
     beforeEach(module('h'));

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

@@ -2,34 +2,31 @@
 
 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 uri = escapeHtml(document.uri || '');
-    var domain = escapeHtml(document.domain || '');
-    var title = escapeHtml(document.title || '');
+/**
+ * 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 (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 '';
-    }
+  if (domain && domain !== title) {
+    return '(' + decodeURIComponent(domain) + ')';
+  } else {
+    return '';
   }
-  return documentDomainFilter;
 };

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

@@ -2,32 +2,29 @@
 
 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 || '');
+/**
+ * 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 (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 '';
-    }
+  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/persona.js

@@ -17,17 +17,18 @@ function parseAccountID(user) {
   };
 }
 
+/**
+ * Returns the username part of an account ID or an empty string.
+ */
+function username(user) {
+  var account = parseAccountID(user);
+  if (!account) {
+    return '';
+  }
+  return account.username;
+}
+
 module.exports = {
   parseAccountID: parseAccountID,
-
-  /** Export parseAccountID() as an Angular filter */
-  filter: function () {
-    return function (user, part) {
-      var account = parseAccountID(user);
-      if (!account) {
-        return null;
-      }
-      return account[part || 'username'];
-    };
-  }
+  username: username,
 };

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

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

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

 'use strict';
 
-var documentTitleFilterProvider = require('../document-title');
+var documentTitle = require('../document-title');
 
 describe('documentTitle', function() {
 
   it('returns the title linked if the document has title and uri', function() {
-    var title = documentTitleFilterProvider()({
+    var title = documentTitle({
       title: 'title',
       uri: 'http://example.com/example.html'
     });
@@ -16,7 +16,7 @@ describe('documentTitle', function() {
   });
 
   it('returns the title linked if the document has an https uri', function() {
-    var title = documentTitleFilterProvider()({
+    var title = documentTitle({
       title: 'title',
       uri: 'https://example.com/example.html'
     });
@@ -27,7 +27,7 @@ describe('documentTitle', function() {
   });
 
   it('returns the title unlinked if doc has title but no uri', function() {
-    var title = documentTitleFilterProvider()({
+    var title = documentTitle({
       title: 'title',
     });
 
@@ -35,7 +35,7 @@ describe('documentTitle', function() {
   });
 
   it('returns the title unlinked if doc has non-http uri', function() {
-    var title = documentTitleFilterProvider()({
+    var title = documentTitle({
       title: 'title',
       uri: 'file:///home/bob/Documents/example.pdf'
     });
@@ -44,7 +44,7 @@ describe('documentTitle', function() {
   });
 
   it('returns an empty string if the document has no title', function() {
-    var title = documentTitleFilterProvider()({
+    var title = documentTitle({
       uri: 'http://example.com/example.html'
     });
 
@@ -54,7 +54,7 @@ describe('documentTitle', function() {
   it('escapes HTML in the document title', function() {
     var spamLink = '<a href="http://example.com/rubies">Buy rubies!!!</a>';
 
-    var title = documentTitleFilterProvider()({
+    var title = documentTitle({
       title: '</a>' + spamLink,
       uri: 'http://example.com/example.html'
     });
@@ -65,7 +65,7 @@ describe('documentTitle', function() {
   it('escapes HTML in the document URI', function() {
     var spamLink = '<a href="http://example.com/rubies">Buy rubies!!!</a>';
 
-    var title = documentTitleFilterProvider()({
+    var title = documentTitle({
       uri: 'http://</a>' + spamLink,
       title: 'title'
     });

h/static/scripts/filter/test/persona-test.js

-var module = angular.mock.module;
-var inject = angular.mock.inject;
+var persona = require('../persona');
 
 describe('persona', function () {
-  var filter = null;
   var term = 'acct:hacker@example.com';
 
-  before(function () {
-    angular.module('h', []).filter('persona', require('../persona').filter);
-  });
-
-  beforeEach(module('h'));
+  describe('parseAccountID', function() {
+    it('should extract the username and provider', function () {
+      assert.deepEqual(persona.parseAccountID(term), {
+        username: 'hacker',
+        provider: 'example.com',
+      });
+    });
 
-  beforeEach(inject(function ($filter) {
-    filter = $filter('persona');
-  }));
-
-  it('should return the requested part', function () {
-    assert.equal(filter(term), 'hacker');
-    assert.equal(filter(term, 'username'), 'hacker');
-    assert.equal(filter(term, 'provider'), 'example.com');
+    it('should return null if the ID is invalid', function () {
+      assert.equal(persona.parseAccountID('bogus'), null);
+    });
   });
 
-  it('should filter out invalid account IDs', function () {
-    assert.equal(filter('bogus'), null);
-    assert.equal(filter('bogus', 'username'), null);
-    assert.notOk(filter('bogus', 'provider'), null);
+  describe('username', function () {
+    it('should return the username from the account ID', function () {
+      assert.equal(persona.username(term), 'hacker');
+    });
+
+    it('should return an empty string if the ID is invalid', function () {
+      assert.equal(persona.username('bogus'), '');
+    });
   });
 });

h/static/scripts/filter/test/urlencode-test.js

-var module = angular.mock.module;
-var inject = angular.mock.inject;
-
-describe('urlencode', function () {
-  var filter = null;
-
-  before(function () {
-    angular.module('h', []).filter('urlencode', require('../urlencode'));
-  });
-
-  beforeEach(module('h'));
-
-  beforeEach(inject(function ($filter) {
-    filter = $filter('urlencode');
-  }));
-
-  it('encodes reserved characters in the term', function () {
-    assert.equal(filter('#hello world'), '%23hello%20world');
-  });
-});

h/static/scripts/filter/urlencode.js

-module.exports = function () {
-  return function (value) {
-    return encodeURIComponent(value);
-  };
-};

h/templates/client/annotation.html

@@ -10,7 +10,7 @@
         <a class="annotation-user"
            target="_blank"
            ng-href="{{::vm.serviceUrl}}u/{{vm.user()}}"
-           >{{vm.user() | persona}}</a>
+           >{{vm.username()}}</a>
       </span>
 
       <span class="annotation-collapsed-replies">
@@ -32,11 +32,11 @@
         </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.document() | documentTitle"
+              ng-bind-html="vm.documentTitle"
               ng-if="::!vm.isSidebar">
         </span>
         <span class="annotation-citation-domain"
-              ng-bind-html="vm.document() | documentDomain"
+              ng-bind-html="vm.documentDomain"
               ng-if="::!vm.isSidebar">
         </span>
       </span>
@@ -104,7 +104,7 @@
        ng-if="(vm.canCollapseBody || vm.form.tags.length) && !vm.editing()">
     <ul class="tag-list">
       <li class="tag-item" ng-repeat="tag in vm.form.tags">
-        <a href="{{::vm.serviceUrl}}stream?q=tag:'{{tag.text|urlencode}}'" target="_blank">{{tag.text}}</a>
+        <a ng-href="vm.tagStreamURL(tag.text)" target="_blank">{{tag.text}}</a>
       </li>
     </ul>
     <div class="u-stretch"></div>