Merge pull request #2197 from...

Merge pull request #2197 from hypothesis/1142-no-client-side-error-message-when-saving-annotation-fails

Add a client-side error when saving an annotation fails

h/static/scripts/directive/annotation.coffee

@@ -11,6 +11,18 @@ validate = (value) ->
   (value.target?.length and not worldReadable)
 
 
+# Return an error message based on a server response.
+errorMessage = (reason) ->
+  if reason.status is 0
+    message = "Service unreachable."
+  else
+    message = reason.status + " " + reason.statusText
+    if reason.data.reason
+      message = message + ": " + reason.data.reason
+
+  return message
+
+
 ###*
 # @ngdoc type
 # @name annotation.AnnotationController
@@ -105,8 +117,10 @@ AnnotationController = [
     this.delete = ->
       $timeout ->  # Don't use confirm inside the digest cycle
         if confirm "Are you sure you want to delete this annotation?"
+          onRejected = (reason) =>
+            flash.error(errorMessage(reason), "Deleting annotation failed")
           $scope.$apply ->
-            annotationMapper.deleteAnnotation model
+            annotationMapper.deleteAnnotation(model).then(null, onRejected)
       , true
 
     ###*
@@ -123,6 +137,16 @@ AnnotationController = [
     ###*
     # @ngdoc method
     # @name annotation.AnnotationController#view
+    # @description Switches the view to a viewer, closing the editor controls
+    #              if they are open.
+    ###
+    this.view = ->
+      @editing = false
+      @action = 'view'
+
+    ###*
+    # @ngdoc method
+    # @name annotation.AnnotationController#revert
     # @description Reverts an edit in progress and returns to the viewer.
     ###
     this.revert = ->
@@ -131,8 +155,7 @@ AnnotationController = [
         $rootScope.$emit('annotationDeleted', model)
       else
         this.render()
-        @action = 'view'
-        @editing = false
+        this.view()
 
     # Calculates the visual diff flags from the targets
     #
@@ -147,6 +170,13 @@ AnnotationController = [
 
       {hasDiff, shouldShowDiff}
 
+    # Update the given annotation domain model object with the data from the
+    # given annotation view model object.
+    updateDomainModel = (domainModel, viewModel) ->
+        angular.extend(
+          domainModel, viewModel,
+          {tags: (tag.text for tag in viewModel.tags)})
+
     ###*
     # @ngdoc method
     # @name annotation.AnnotationController#save
@@ -163,19 +193,27 @@ AnnotationController = [
         tag.text not in (model.tags or [])
       tags.store(newTags)
 
-      angular.extend model, @annotation,
-        tags: (tag.text for tag in @annotation.tags)
-
       switch @action
         when 'create'
-          model.$create().then ->
+          updateDomainModel(model, @annotation)
+          onFulfilled = =>
             $rootScope.$emit('annotationCreated', model)
-        when 'delete', 'edit'
-          model.$update(id: model.id).then ->
+            @view()
+          onRejected = (reason) =>
+            flash.error(errorMessage(reason), "Saving annotation failed")
+          model.$create().then(onFulfilled, onRejected)
+        when 'edit'
+          updatedModel = angular.copy(model)
+          updateDomainModel(updatedModel, @annotation)
+          onFulfilled = =>
+            angular.copy(updatedModel, model)
             $rootScope.$emit('annotationUpdated', model)
+            @view()
+          onRejected = (reason) =>
+            flash.error(errorMessage(reason), "Saving annotation failed")
+          updatedModel.$update(id: updatedModel.id).then(
+            onFulfilled, onRejected)
 
-      @editing = false
-      @action = 'view'
 
     ###*
     # @ngdoc method

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

@@ -7,6 +7,7 @@ describe 'annotation', ->
   $compile = null
   $document = null
   $element = null
+  $rootScope = null
   $scope = null
   $timeout = null
   annotation = null
@@ -69,7 +70,8 @@ describe 'annotation', ->
     }
     fakePersonaFilter = sandbox.stub().returnsArg(0)
     fakeTags = {
-      filter: sandbox.stub().returns('a while ago')
+      filter: sandbox.stub().returns('a while ago'),
+      store: sandbox.stub()
     }
     fakeTime = {
       toFuzzyString: sandbox.stub().returns('a while ago')
@@ -91,10 +93,11 @@ describe 'annotation', ->
     $provide.value 'urlencodeFilter', fakeUrlEncodeFilter
     return
 
-  beforeEach inject (_$compile_, _$document_, $rootScope, _$timeout_) ->
+  beforeEach inject (_$compile_, _$document_, _$rootScope_, _$timeout_) ->
     $compile = _$compile_
     $document = _$document_
     $timeout = _$timeout_
+    $rootScope = _$rootScope_
     $scope = $rootScope.$new()
     $scope.annotation = annotation =
       id: 'deadbeef'
@@ -387,3 +390,283 @@ describe 'annotation', ->
       annotation.updated = '456'
       $scope.$digest()
       assert.calledWith(isolateScope.$emit, 'annotationUpdate')
+
+  describe "deleteAnnotation() method", ->
+    before ->
+      sinon.stub(window, "confirm")
+
+    beforeEach ->
+      createDirective()
+      fakeAnnotationMapper.deleteAnnotation = sandbox.stub()
+      fakeFlash.error = sandbox.stub()
+
+    it "calls annotationMapper.delete() if the delete is confirmed", ->
+      window.confirm.returns(true)
+      fakeAnnotationMapper.deleteAnnotation.returns(Promise.resolve())
+      controller.delete().then(->
+        assert fakeAnnotationMapper.deleteAnnotation.calledWith(annotation)
+      )
+      $timeout.flush()
+
+    it "doesn't call annotationMapper.delete() if the delete is cancelled", ->
+      window.confirm.returns(false)
+      assert fakeAnnotationMapper.deleteAnnotation.notCalled
+
+    it "flashes a generic error if the server cannot be reached", ->
+      window.confirm.returns(true)
+      fakeAnnotationMapper.deleteAnnotation.returns(Promise.reject({status: 0}))
+
+      controller.delete().then(->
+        assert fakeFlash.error.calledWith(
+          "Service unreachable.", "Saving annotation failed")
+      )
+      $timeout.flush()
+
+    it "flashes an error if the delete fails on the server", ->
+      window.confirm.returns(true)
+      fakeAnnotationMapper.deleteAnnotation.returns(Promise.reject({
+          status: 500,
+          statusText: "Server Error",
+          data: {}
+        })
+      )
+      controller.delete().then(->
+        assert fakeFlash.error.calledWith(
+          "500 Server Error", "Deleting annotation failed")
+      )
+      $timeout.flush()
+
+    it "doesn't flash an error if the delete succeeds", ->
+      window.confirm.returns(true)
+      fakeAnnotationMapper.deleteAnnotation.returns(Promise.resolve())
+      controller.delete().then(->
+        assert fakeFlash.error.notCalled
+      )
+      $timeout.flush()
+
+  describe "creating a new annotation", ->
+    beforeEach ->
+      createDirective()
+      fakeFlash.error = sandbox.stub()
+      controller.action = 'create'
+      annotation.$create = sandbox.stub()
+
+    it "emits annotationCreated when saving an annotation succeeds", ->
+      sandbox.spy($rootScope, '$emit')
+      annotation.$create.returns(Promise.resolve())
+      controller.save().then(->
+        assert $rootScope.$emit.calledWith("annotationCreated")
+      )
+
+    it "flashes a generic error if the server cannot be reached", ->
+      annotation.$create.returns(Promise.reject({status: 0}))
+
+      controller.save().then(->
+        assert fakeFlash.error.calledWith(
+          "Service unreachable.", "Saving annotation failed")
+      )
+
+    it "flashes an error if saving the annotation fails on the server", ->
+      annotation.$create.returns(Promise.reject({
+          status: 500,
+          statusText: "Server Error",
+          data: {}
+        })
+      )
+      controller.save().then(->
+        assert fakeFlash.error.calledWith(
+          "500 Server Error", "Saving annotation failed")
+      )
+
+    it "doesn't flash an error when saving an annotation succeeds", ->
+      annotation.$create.returns(Promise.resolve())
+      controller.save().then(->
+        assert fakeFlash.error.notCalled
+      )
+
+  describe "editing an annotation", ->
+
+    beforeEach ->
+      createDirective()
+      fakeFlash.error = sandbox.stub()
+      controller.action = 'edit'
+      annotation.$update = sandbox.stub()
+
+    it "flashes a generic error if the server cannot be reached", ->
+      annotation.$update.returns(Promise.reject({status: 0}))
+
+      controller.save().then(->
+        assert fakeFlash.error.calledWith(
+          "Service unreachable.", "Saving annotation failed")
+      )
+
+    it "flashes an error if saving the annotation fails on the server", ->
+      annotation.$update.returns(Promise.reject({
+          status: 500,
+          statusText: "Server Error",
+          data: {}
+        })
+      )
+
+      controller.save().then(->
+        assert fakeFlash.error.calledWith(
+          "500 Server Error", "Saving annotation failed")
+      )
+
+    it "doesn't flash an error if saving the annotation succeeds", ->
+      annotation.$update.returns(Promise.resolve())
+
+      controller.save().then(->
+        assert fakeFlash.error.notCalled
+      )
+
+describe("AnnotationController", ->
+
+  before(->
+    angular.module("h", [])
+    .directive("annotation", require("../annotation"))
+  )
+
+  beforeEach(module("h"))
+
+  beforeEach(module("h.templates"))
+
+  # Return Angular's $compile service.
+  getCompileService = ->
+    $compile = null
+    inject((_$compile_) ->
+      $compile = _$compile_
+    )
+    $compile
+
+  # Return Angular's $rootScope.
+  getRootScope = ->
+    $rootScope = null
+    inject((_$rootScope_) ->
+      $rootScope = _$rootScope_
+    )
+    $rootScope
+
+  ###
+  Return an annotation directive instance and stub services etc.
+  ###
+  createAnnotationDirective = ({annotation, personaFilter, momentFilter,
+                                urlencodeFilter, auth, drafts, flash,
+                                permissions, tags, time, annotationUI,
+                                annotationMapper}) ->
+    locals = {
+      personaFilter: personaFilter or {}
+      momentFilter: momentFilter or {}
+      urlencodeFilter: urlencodeFilter or {}
+      auth: auth or {}
+      drafts: drafts or {
+        add: ->
+      }
+      flash: flash or {}
+      permissions: permissions or {}
+      tags: tags or {}
+      time: time or {
+        toFuzzyString: ->
+        nextFuzzyUpdate: ->
+      }
+      annotationUI: annotationUI or {}
+      annotationMapper: annotationMapper or {}
+    }
+    module(($provide) ->
+      $provide.value("personaFilter", locals.personaFilter)
+      $provide.value("momentFilter", locals.momentFilter)
+      $provide.value("urlencodeFilter", locals.urlencodeFilter)
+      $provide.value("auth", locals.auth)
+      $provide.value("drafts", locals.drafts)
+      $provide.value("flash", locals.flash)
+      $provide.value("permissions", locals.permissions)
+      $provide.value("tags", locals.tags)
+      $provide.value("time", locals.time)
+      $provide.value("annotationUI", locals.annotationUI)
+      $provide.value("annotationMapper", locals.annotationMapper)
+      return
+    )
+
+    locals.element = angular.element('<div annotation="annotation">')
+    compiledElement = getCompileService()(locals.element)
+    locals.$rootScope = getRootScope()
+    locals.parentScope = locals.$rootScope.$new()
+    locals.parentScope.annotation = annotation or {}
+    locals.directive = compiledElement(locals.parentScope)
+
+    locals.$rootScope.$digest()
+
+    locals.controller = locals.element.controller("annotation")
+    locals.isolateScope = locals.element.isolateScope()
+
+    locals
+
+  describe("createAnnotationDirective", ->
+    it("creates the directive without crashing", ->
+      createAnnotationDirective({})
+    )
+  )
+
+  ###
+  Simulate what happens when the user edits an annotation, clicks Save,
+  gets an error because the server fails to save the annotation, then clicks
+  Cancel - in the frontend the annotation should be restored to its original
+  value, the edits lost.
+  ###
+  it "restores the original text when editing is cancelled", ->
+    {controller} = createAnnotationDirective(
+      annotation: {
+        id: "test-annotation-id"
+        user: "acct:bill@localhost"
+        text: "Initial annotation body text"
+        # Allow the initial save of the annotation to succeed.
+        $create: ->
+          Promise.resolve()
+        # Simulate saving the edit of the annotation to the server failing.
+        $update: ->
+          Promise.reject({
+            status: 500,
+            statusText: "Server Error",
+            data: {}
+          })
+      }
+      flash: {
+        info: ->
+        error: ->
+      }
+      personaFilter: ->
+      permissions: {
+        isPrivate: -> false
+        permits: -> true
+      }
+      tags: {
+        store: ->
+      }
+      drafts: {
+        add: ->
+        remove: ->
+      }
+    )
+
+    original_text = controller.annotation.text
+
+    # Simulate the user clicking the Edit button on the annotation.
+    controller.edit()
+
+    # Simulate the user typing some text into the annotation editor textarea.
+    controller.annotation.text = "changed by test code"
+
+    # Simulate the user hitting the Save button and wait for the
+    # (unsuccessful) response from the server.
+    controller.save().then(->
+      # At this point the annotation editor controls are still open, and the
+      # annotation's text is still the modified (unsaved) text.
+      assert controller.annotation.text == "changed by test code"
+
+      # Simulate the user clicking the Cancel button.
+      controller.revert()
+
+      # Now the original text should be restored.
+      assert controller.annotation.text == original_text
+    )
+)

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

@@ -122,7 +122,9 @@ describe 'annotationMapper', ->
       p.catch ->
         assert.notCalled($rootScope.$emit)
 
-    it 'returns the annotation', ->
+    it 'return a promise that resolves to the deleted annotation', ->
       p = Promise.resolve()
       ann  = {$delete: sandbox.stub().returns(p)}
-      assert.equal(annotationMapper.deleteAnnotation(ann), ann)
+      return annotationMapper.deleteAnnotation(ann).then((value) ->
+        assert.equal(value, ann)
+      )