Merge pull request #2761 from hypothesis/2728-refactor-annotation-controller

Refactor annotation controller, part 1

h/static/scripts/directive/annotation.js

@@ -152,7 +152,8 @@ function errorMessage(reason) {
 // @ngInject
 function AnnotationController(
   $document, $q, $rootScope, $scope, $timeout, $window, annotationUI,
-  annotationMapper, drafts, flash, groups, permissions, session, tags, time) {
+  annotationMapper, drafts, flash, features, groups, permissions, session,
+  tags, time) {
 
   var vm = this;
 
@@ -162,9 +163,12 @@ function AnnotationController(
 
   vm.action = 'view';
   vm.document = null;
-  vm.editing = false;
-  vm.isSidebar = false;
-  vm.preview = 'no';
+  // Give the template access to the feature flags.
+  vm.feature = features.flagEnabled;
+  // Copy isSidebar from $scope onto vm for consistency (we want this
+  // directive's templates to always access variables from vm rather than
+  // directly from scope).
+  vm.isSidebar = $scope.isSidebar;
   vm.timestamp = null;
 
   /** The domain model, contains the currently saved version of the annotation
@@ -184,6 +188,20 @@ function AnnotationController(
 
   var highlight = model.$highlight;
 
+  /**
+   * @ngdoc method
+   * @name annotation.AnnotationController#editing.
+   * @returns {boolean} `true` if this annotation is currently being edited
+   *   (i.e. the annotation editor form should be open), `false` otherwise.
+   */
+  vm.editing = function() {
+    if (vm.action === 'create' || vm.action === 'edit') {
+      return true;
+    } else {
+      return false;
+    }
+  };
+
   /**
     * @ngdoc method
     * @name annotation.AnnotationController#group.
@@ -193,6 +211,14 @@ function AnnotationController(
     return groups.get(model.group);
   };
 
+  // Save on Meta + Enter or Ctrl + Enter.
+  vm.onKeydown = function(event) {
+    if (event.keyCode === 13 && (event.metaKey || event.ctrlKey)) {
+      event.preventDefault();
+      vm.save();
+    }
+  };
+
   /**
     * @ngdoc method
     * @name annotation.AnnotationController#tagsAutoComplete.
@@ -260,6 +286,19 @@ function AnnotationController(
     }
   };
 
+  vm.share = function(event) {
+    var $container = angular.element(event.currentTarget).parent();
+    $container.addClass('open').find('input').focus().select();
+
+    // We have to stop propagation here otherwise this click event will
+    // re-close the share dialog immediately.
+    event.stopPropagation();
+
+    $document.one('click', function() {
+      $container.removeClass('open');
+    });
+  };
+
   /**
     * @ngdoc method
     * @name annotation.AnnotaitonController#hasContent
@@ -292,9 +331,6 @@ function AnnotationController(
     * the annotation.
     */
   vm.authorize = function(action) {
-    if (model === null) {
-      return false;
-    }
     // TODO: this should use auth instead of permissions but we might need
     // an auth cache or the JWT -> userid decoding might start to be a
     // performance bottleneck and we would need to get the id token into the
@@ -334,8 +370,6 @@ function AnnotationController(
       updateDraft(model);
     }
     vm.action = model.id ? 'edit' : 'create';
-    vm.editing = true;
-    vm.preview = 'no';
   };
 
   /**
@@ -345,7 +379,6 @@ function AnnotationController(
     *              if they are open.
     */
   vm.view = function() {
-    vm.editing = false;
     vm.action = 'view';
   };
 
@@ -582,7 +615,7 @@ function AnnotationController(
   // the drafts service. They will be restored when this annotation is
   // next loaded.
   $scope.$on(events.GROUP_FOCUSED, function() {
-    if (!vm.editing) {
+    if (!vm.editing()) {
       return;
     }
 
@@ -622,47 +655,19 @@ function AnnotationController(
   *
   */
 // @ngInject
-function annotation($document, features) {
+function annotation($document) {
   function linkFn(scope, elem, attrs, controllers) {
     var ctrl = controllers[0];
     var thread = controllers[1];
     var threadFilter = controllers[2];
     var counter = controllers[3];
 
-    attrs.$observe('isSidebar', function(value) {
-      if (value && value !== 'false') {
-        ctrl.isSidebar = true;
-      } else {
-        ctrl.isSidebar = false;
-      }
-    });
-
-    // Save on Meta + Enter or Ctrl + Enter.
-    elem.on('keydown', function(event) {
-      if (event.keyCode === 13 && (event.metaKey || event.ctrlKey)) {
-        event.preventDefault();
-        scope.$evalAsync(function() {
-          ctrl.save();
-        });
-      }
-    });
-
-    // Give template access to feature flags.
-    scope.feature = features.flagEnabled;
-
-    scope.share = function(event) {
-      var $container = angular.element(event.currentTarget).parent();
-      $container.addClass('open').find('input').focus().select();
-
-      // We have to stop propagation here otherwise this click event will
-      // re-close the share dialog immediately.
-      event.stopPropagation();
-
-      $document.one('click', function() {
-        $container.removeClass('open');
-      });
-    };
+    elem.on('keydown', ctrl.onKeydown);
 
+    // FIXME: Replace this counting code with something more sane, and
+    // something that doesn't involve so much untested logic in the link
+    // function (as opposed to unit-tested methods on the AnnotationController,
+    // for example).
     // Keep track of edits going on in the thread.
     if (counter !== null) {
       // Expand the thread if descendants are editing.
@@ -711,7 +716,8 @@ function annotation($document, features) {
       isLastReply: '=',
       replyCount: '@annotationReplyCount',
       replyCountClick: '&annotationReplyCountClick',
-      showReplyCount: '@annotationShowReplyCount'
+      showReplyCount: '@annotationShowReplyCount',
+      isSidebar: '='
     },
     templateUrl: 'annotation.html'
   };

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

@@ -358,6 +358,23 @@ describe('annotation', function() {
     });
   });
 
+  describe('AnnotationController.editing()', function() {
+    it('returns true if action is "create"', function() {
+      controller.action = 'create';
+      assert(controller.editing());
+    });
+
+    it('returns true if action is "edit"', function() {
+      controller.action = 'edit';
+      assert(controller.editing());
+    });
+
+    it('returns false if action is "view"', function() {
+      controller.action = 'view';
+      assert(!controller.editing());
+    });
+  });
+
   describe('when the annotation is a highlight', function() {
     beforeEach(function() {
       annotation.$highlight = true;
@@ -922,7 +939,7 @@ describe('annotation', function() {
         text: 'unsaved-text'
       });
       createDirective();
-      assert.isTrue(controller.editing);
+      assert.isTrue(controller.editing());
     });
 
     it('uses the text and tags from the draft if present', function() {

h/templates/client/annotation.html

@@ -30,7 +30,7 @@
               title="This annotation is visible only to you.">
           <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>
+        <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-if="!vm.isSidebar">
@@ -48,7 +48,7 @@
     <a class="annotation-timestamp"
        target="_blank"
        title="{{vm.annotation.updated | moment:'LLLL'}}"
-       ng-if="!vm.editing && vm.annotation.updated"
+       ng-if="!vm.editing() && vm.annotation.updated"
        ng-href="{{vm.baseURI}}a/{{vm.annotation.id}}"
        >{{vm.timestamp}}</a>
   </header>
@@ -57,7 +57,7 @@
   <section class="annotation-quote-list"
            ng-repeat="target in vm.annotation.target track by $index"
            ng-if="vm.hasQuotes()">
-    <excerpt enabled="feature('truncate_annotations')">
+    <excerpt enabled="vm.feature('truncate_annotations')">
       <blockquote class="annotation-quote"
                   ng-bind-html="selector.exact"
                   ng-repeat="selector in target.selector
@@ -70,16 +70,16 @@
 
   <!-- Body -->
   <section name="text" class="annotation-body">
-    <excerpt enabled="feature('truncate_annotations') && !vm.editing">
+    <excerpt enabled="vm.feature('truncate_annotations') && !vm.editing()">
       <markdown ng-model="vm.annotation.text"
-           read-only="!vm.editing"
+           read-only="!vm.editing()"
            ></markdown>
     </excerpt>
   </section>
   <!-- / Body -->
 
   <!-- Tags -->
-  <div class="annotation-body form-field" ng-if="vm.editing">
+  <div class="annotation-body form-field" ng-if="vm.editing()">
     <tags-input ng-model="vm.annotation.tags"
                 name="tags"
                 class="tags"
@@ -94,7 +94,7 @@
   </div>
 
   <div class="annotation-body tags tags-read-only"
-       ng-if="vm.annotation.tags.length && !vm.editing">
+       ng-if="vm.annotation.tags.length && !vm.editing()">
     <ul class="tag-list">
       <li class="tag-item" ng-repeat="tag in vm.annotation.tags">
         <a href="/stream?q=tag:'{{tag.text|urlencode}}'" target="_blank">{{tag.text}}</a>
@@ -104,7 +104,7 @@
   <!-- / Tags -->
 
   <footer class="annotation-footer">
-    <div class="annotation-form-actions" ng-if="vm.editing" ng-switch="vm.action">
+    <div class="annotation-form-actions" ng-if="vm.editing()" ng-switch="vm.action">
       <button ng-switch-when="delete"
               ng-click="vm.save()"
               class="dropdown-menu-btn"><i class="h-icon-check btn-icon"></i> Delete</button>
@@ -119,7 +119,7 @@
     </div>
 
     <div class="annotation-section annotation-license"
-         ng-show="vm.isShared() && vm.editing">
+         ng-show="vm.isShared() && vm.editing()">
       <a href="http://creativecommons.org/publicdomain/zero/1.0/"
          title="View more information about the Creative Commons Public Domain license"
          target="_blank">
@@ -135,13 +135,13 @@
          when="{'0': '', 'one': '1 reply', 'other': '{} replies'}"></a>
     </div>
 
-    <div class="annotation-actions" ng-if="!vm.editing && vm.annotation.id">
+    <div class="annotation-actions" ng-if="!vm.editing() && vm.annotation.id">
       <button class="small btn btn-clean"
               ng-click="vm.reply()"
               ><i class="h-icon-reply btn-icon"></i> Reply</button>
       <span class="share-dialog-wrapper">
         <button class="small btn btn-clean"
-                ng-click="share($event)"
+                ng-click="vm.share($event)"
                 ><i class="h-icon-link btn-icon"></i> Link</button>
         <span class="share-dialog" ng-click="$event.stopPropagation()">
           <a target="_blank"

h/templates/client/thread.html

@@ -15,7 +15,7 @@
          name="annotation"
          annotation="vm.container.message"
          is-last-reply="$last"
-         is-sidebar="{{isSidebar}}"
+         is-sidebar="isSidebar"
          annotation-show-reply-count="{{vm.shouldShowNumReplies()}}"
          annotation-reply-count="{{vm.numReplies()}}"
          annotation-reply-count-click="vm.toggleCollapsed()"