Prevent double-posting of new annotations

When the user clicks the 'Post' button to create an annotation,
optimistically switch the card back to View mode but display
a 'Saving...' indicator in place of the Reply/Edit/Delete links.

This makes the UI appear more responsive when the user clicks
the Post button and also prevents an issue where the user could
click 'Post' multiple times during the save and create multiple
annotations.

 * Fix a possible inconsistency between the 'Post' button's enabled
   state and whether or not the save() function can succeed.

   The hasContent() and isShared() methods also already have tests,
   so this lets us remove several redundant tests.

 * Fix inconsistency in the return type of the save() function -
   always return a promise.

 * Treat negative status values as network errors as well as 0.
   If the server is unreachable, the real status value may be -1.

Fixes #2864

h/static/scripts/directive/annotation.js

@@ -27,11 +27,11 @@ function domainModelTagsFromViewModelTags(viewModelTags) {
  */
 function errorMessage(reason) {
   var message;
-  if (reason.status === 0) {
+  if (reason.status <= 0) {
     message = 'Service unreachable.';
   } else {
     message = reason.status + ' ' + reason.statusText;
-    if (reason.data.reason) {
+    if (reason.data && reason.data.reason) {
       message = message + ': ' + reason.data.reason;
     }
   }
@@ -193,43 +193,6 @@ function updateViewModel($scope, time, domainModel, vm, permissions) {
   }
 }
 
-/** Return truthy if the given annotation is valid, falsy otherwise.
- *
-* A public annotation has to have some text and/or some tags to be valid,
-* public annotations with no text or tags (i.e. public highlights) are not
-* valid.
-*
-* Non-public annotations just need to have a target to be valid, non-public
-* highlights are valid.
-*
-* @param {object} annotation The annotation to be validated
-*
-*/
-function validate(domainModel) {
-  if (!angular.isObject(domainModel)) {
-    return;
-  }
-
-  var permissions = domainModel.permissions || {};
-  var readPermissions = permissions.read || [];
-  var targets = domainModel.target || [];
-
-  if (domainModel.tags && domainModel.tags.length) {
-    return domainModel.tags.length;
-  }
-
-  if (domainModel.text && domainModel.text.length) {
-    return domainModel.text.length;
-  }
-
-  var worldReadable = false;
-  if (readPermissions.indexOf('group:__world__') !== -1) {
-    worldReadable = true;
-  }
-
-  return (targets.length && !worldReadable);
-}
-
 /** Return a vm tags array from the given domainModel tags array.
  *
  * domainModel.tags and vm.form.tags use different formats.  This
@@ -318,6 +281,9 @@ function AnnotationController(
     /** Determines whether the annotation body should be collapsed. */
     vm.collapseBody = true;
 
+    /** True if the annotation is currently being saved. */
+    vm.isSaving = false;
+
     /** The domain model, contains the currently saved version of the
       * annotation from the server (or in the case of new annotations that
       * haven't been saved yet - the data that will be saved to the server when
@@ -661,7 +627,13 @@ function AnnotationController(
     */
   vm.save = function() {
     if (!domainModel.user) {
-      return flash.info('Please sign in to save your annotations.');
+      flash.info('Please sign in to save your annotations.');
+      return Promise.resolve();
+    }
+    if ((vm.action === 'create' || vm.action === 'edit') &&
+        !vm.hasContent() && vm.isShared()) {
+      flash.info('Please add text or a tag before publishing.');
+      return Promise.resolve();
     }
 
     // Update stored tags with the new tags of this annotation.
@@ -671,47 +643,45 @@ function AnnotationController(
     });
     tags.store(newTags);
 
+    var saved;
     switch (vm.action) {
       case 'create':
         updateDomainModel(domainModel, vm, permissions, groups);
-
-        if (!validate(domainModel)) {
-          return flash.info('Please add text or a tag before publishing.');
-        }
-
-        var onFulfilled = function() {
+        saved = domainModel.$create().then(function () {
           $rootScope.$emit('annotationCreated', domainModel);
           updateView(domainModel);
-          view();
           drafts.remove(domainModel);
-        };
-        var onRejected = function(reason) {
-          flash.error(
-            errorMessage(reason), 'Saving annotation failed');
-        };
-        return domainModel.$create().then(onFulfilled, onRejected);
+        });
+        break;
 
       case 'edit':
         var updatedModel = angular.copy(domainModel);
         updateDomainModel(updatedModel, vm, permissions, groups);
+        saved = updatedModel.$update({
+          id: updatedModel.id
+        }).then(function () {
+          drafts.remove(domainModel);
+          $rootScope.$emit('annotationUpdated', updatedModel);
+        });
+        break;
 
-        if (!validate(updatedModel)) {
-          return flash.info('Please add text or a tag before publishing.');
+      default:
+        throw new Error('Tried to save an annotation that is not being edited');
     }
 
-        onFulfilled = function() {
-          drafts.remove(domainModel);
-          $rootScope.$emit('annotationUpdated', updatedModel);
+    // optimistically switch back to view mode and display the saving
+    // indicator
+    vm.isSaving = true;
     view();
-        };
-        onRejected = function(reason) {
+
+    return saved.then(function () {
+      vm.isSaving = false;
+    }).catch(function (reason) {
+      vm.isSaving = false;
+      vm.edit();
       flash.error(
         errorMessage(reason), 'Saving annotation failed');
-        };
-        return updatedModel.$update({
-          id: updatedModel.id
-        }).then(onFulfilled, onRejected);
-    }
+    });
   };
 
   /**
@@ -863,7 +833,6 @@ module.exports = {
   extractDocumentMetadata: extractDocumentMetadata,
   link: link,
   updateDomainModel: updateDomainModel,
-  validate: validate,
 
   // These are meant to be the public API of this module.
   directive: annotation,

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

@@ -262,99 +262,6 @@ describe('annotation', function() {
     });
   });
 
-  describe('validate()', function() {
-    var validate = require('../annotation').validate;
-
-    it('returns undefined if value is not an object', function() {
-      var i;
-      var values = [2, 'foo', true, null];
-      for (i = 0; i < values.length; i++) {
-        assert.equal(validate(values[i]));
-      }
-    });
-
-    it(
-      'returns the length if the value contains a non-empty tags array',
-      function() {
-        assert.equal(
-          validate({
-            tags: ['foo', 'bar'],
-            permissions: {
-              read: ['group:__world__']
-            },
-            target: [1, 2, 3]
-          }),
-          2);
-      }
-    );
-
-    it(
-      'returns the length if the value contains a non-empty text string',
-      function() {
-        assert.equal(
-          validate({
-            text: 'foobar',
-            permissions: {
-              read: ['group:__world__']
-            },
-            target: [1, 2, 3]
-          }),
-          6);
-      }
-    );
-
-    it('returns true for private highlights', function() {
-      assert.equal(
-        validate({
-          permissions: {
-            read: ['acct:seanh@hypothes.is']
-          },
-          target: [1, 2, 3]
-        }),
-        true);
-    });
-
-    it('returns true for group highlights', function() {
-      assert.equal(
-        validate({
-          permissions: {
-            read: ['group:foo']
-          },
-          target: [1, 2, 3]
-        }),
-        true);
-    });
-
-    it('returns false for public highlights', function() {
-      assert.equal(
-        validate(
-          {text: undefined, tags: undefined, target: [1, 2, 3],
-           permissions: {read: ['group:__world__']}}
-        ),
-        false);
-    });
-
-    it('handles values with no permissions', function() {
-      assert.equal(
-        validate({
-          permissions: void 0,
-          target: [1, 2, 3]
-        }),
-        true);
-    });
-
-    it('handles permissions objects with no read', function() {
-      assert.equal(
-        validate({
-          permissions: {
-            read: void 0
-          },
-          target: [1, 2, 3]
-        }),
-        true);
-    });
-  });
-
   describe('link', function () {
     var link = require('../annotation').link;
 
@@ -1120,6 +1027,7 @@ describe('annotation', function() {
         parts.controller.isPrivate = true;
         parts.annotation.$update = sinon.stub().returns(Promise.resolve());
         parts.controller.edit();
+        parts.controller.form.text = 'test';
         parts.controller.setPrivacy('shared');
         return parts.controller.save().then(function() {
           assert.equal(parts.controller.isPrivate, false);
@@ -1131,6 +1039,7 @@ describe('annotation', function() {
         parts.annotation.$update = sinon.stub().returns(Promise.resolve());
         parts.controller.edit();
         parts.controller.setPrivacy('shared');
+        parts.controller.form.text = 'test';
         return parts.controller.save().then(function() {
           assert(fakePermissions.setDefault.calledWithExactly('shared'));
         });
@@ -1245,6 +1154,7 @@ describe('annotation', function() {
         };
         var controller = createDirective(annotation).controller;
         controller.action = 'create';
+        controller.form.text = 'test';
         return controller.save().then(function () {
           assert.equal(controller.relativeTimestamp, 'a while ago');
         });
@@ -1270,6 +1180,7 @@ describe('annotation', function() {
         var controller = createDirective(annotation).controller;
         assert.equal(controller.relativeTimestamp, 'ages ago');
         controller.edit();
+        controller.form.text = 'test';
         clock.restore();
         return controller.save().then(function () {
           assert.equal(controller.relativeTimestamp, 'just now');
@@ -1414,6 +1325,7 @@ describe('annotation', function() {
       function controllerWithActionCreate() {
         var controller = createDirective(annotation).controller;
         controller.action = 'create';
+        controller.form.text = 'new annotation';
         return controller;
       }
 
@@ -1471,6 +1383,36 @@ describe('annotation', function() {
           assert(fakeFlash.error.notCalled);
         }
       );
+
+      it('shows a saving indicator when saving an annotation', function() {
+        var controller = controllerWithActionCreate();
+        var create;
+        annotation.$create.returns(new Promise(function (resolve) {
+          create = resolve;
+        }));
+        var saved = controller.save();
+        assert.equal(controller.isSaving, true);
+        assert.equal(controller.action, 'view');
+        create();
+        return saved.then(function () {
+          assert.equal(controller.isSaving, false);
+        });
+      });
+
+      it('reverts to edit mode if saving fails', function () {
+        var controller = controllerWithActionCreate();
+        var failCreation;
+        annotation.$create.returns(new Promise(function (resolve, reject) {
+          failCreation = reject;
+        }));
+        var saved = controller.save();
+        assert.equal(controller.isSaving, true);
+        failCreation({status: -1});
+        return saved.then(function () {
+          assert.equal(controller.isSaving, false);
+          assert.ok(controller.editing());
+        });
+      });
     });
 
     describe('saving an edited an annotation', function() {
@@ -1485,37 +1427,36 @@ describe('annotation', function() {
       function controllerWithActionEdit() {
         var controller = createDirective(annotation).controller;
         controller.action = 'edit';
+        controller.form.text = 'updated text';
         return controller;
       }
 
       it(
         'flashes a generic error if the server cannot be reached',
-        function(done) {
+        function() {
           var controller = controllerWithActionEdit();
           annotation.$update.returns(Promise.reject({
-            status: 0
+            status: -1
           }));
-          controller.save().then(function() {
+          return controller.save().then(function() {
             assert(fakeFlash.error.calledWith(
               'Service unreachable.', 'Saving annotation failed'));
-            done();
           });
         }
       );
 
       it(
         'flashes an error if saving the annotation fails on the server',
-        function(done) {
+        function() {
           var controller = controllerWithActionEdit();
           annotation.$update.returns(Promise.reject({
             status: 500,
             statusText: 'Server Error',
             data: {}
           }));
-          controller.save().then(function() {
+          return controller.save().then(function() {
             assert(fakeFlash.error.calledWith(
               '500 Server Error', 'Saving annotation failed'));
-            done();
           });
         }
       );
@@ -1525,6 +1466,7 @@ describe('annotation', function() {
         function() {
           var controller = controllerWithActionEdit();
           annotation.$update.returns(Promise.resolve());
+          controller.form.text = 'updated text';
           controller.save();
           assert(fakeFlash.error.notCalled);
         }
@@ -1571,6 +1513,7 @@ describe('annotation', function() {
         annotation.$update = sandbox.stub().returns(Promise.resolve());
         var controller = createDirective(annotation).controller;
         controller.edit();
+        controller.form.text = 'test annotation';
         return controller.save().then(function() {
           assert.calledWith(fakeDrafts.remove, annotation);
         });

h/templates/client/annotation.html

@@ -149,7 +149,12 @@
          when="{'0': '', 'one': '1 reply', 'other': '{} replies'}"></a>
     </div>
 
-    <div class="annotation-actions" ng-if="!vm.editing() && vm.id()">
+    <div class="annotation-actions" ng-if="vm.isSaving">
+      Saving...
+    </div>
+
+    <div class="annotation-actions" ng-if="!vm.isSaving && !vm.editing() && vm.id()">
+      <div ng-show="vm.isSaving">Saving…</div>
       <button class="small btn btn-clean"
               ng-click="vm.reply()"
               ><i class="h-icon-reply btn-icon"></i> Reply</button>
@@ -166,7 +171,7 @@
         </span>
       </span>
       <button class="small btn btn-clean"
-              ng-show="vm.authorize('update')"
+              ng-show="vm.authorize('update') && !vm.isSaving"
               ng-click="vm.edit()"
               ><i class="h-icon-edit btn-icon"></i> Edit</button>
       <button class="small btn btn-clean"