Merge pull request #2887 from hypothesis/optimistic-save

Prevent double-posting of new annotations

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"