Merge pull request #2689 from robertknight/t152-move_unsaved_annot_to_current_group

Avoid losing unsaved changes to new or existing annotations when switching groups

h/static/scripts/app-controller.coffee

@@ -47,12 +47,9 @@ module.exports = class AppController
     }
 
     # Reload the view when the user switches accounts
-    reloadEvents = [events.USER_CHANGED];
-    reloadEvents.forEach((eventName) ->
-      $scope.$on(eventName, (event, data) ->
-        if !data || !data.initialLoad
-          $route.reload()
-      )
+    $scope.$on(events.USER_CHANGED, (event, data) ->
+      if !data || !data.initialLoad
+        $route.reload()
     );
 
     identity.watch({

h/static/scripts/app.coffee

@@ -25,8 +25,8 @@ resolve =
       threading.createIdTable([])
       threading.root = mail.messageContainer()
 
-      # Reload all unsaved annotations
-      threading.thread(drafts.all())
+      # Reload all new, unsaved annotations
+      threading.thread(drafts.unsaved())
 
       return threading
   ]

h/static/scripts/directive/annotation.coffee

@@ -51,6 +51,7 @@ AnnotationController = [
    drafts,   flash,   permissions,   tags,   time,
    annotationUI,   annotationMapper,   session,   groups) ->
 
+    # @annotation is the view model, containing the unsaved annotation changes
     @annotation = {}
     @action = 'view'
     @document = null
@@ -59,6 +60,8 @@ AnnotationController = [
     @isSidebar = false
     @timestamp = null
 
+    # 'model' is the domain model, containing the currently saved version
+    # of the annotation
     model = $scope.annotationGet()
 
     model.user ?= session.state.userid
@@ -196,7 +199,8 @@ AnnotationController = [
     # @description Switches the view to an editor.
     ###
     this.edit = ->
-      drafts.add model, => this.revert()
+      if !drafts.get(model)
+        updateDraft(model)
       @action = if model.id? then 'edit' else 'create'
       @editing = true
       @preview = 'no'
@@ -231,6 +235,16 @@ AnnotationController = [
           domainModel, viewModel,
           {tags: (tag.text for tag in viewModel.tags)})
 
+    # Create or update the existing draft for this annotation using
+    # the text and tags from the domain model in 'draft'
+    updateDraft = (draft) ->
+      # drafts only preserve the text and tags for the
+      # annotation, changes to other properties are not preserved
+      drafts.update(model, {
+        text: draft.text
+        tags: draft.tags
+      })
+
     ###*
     # @ngdoc method
     # @name annotation.AnnotationController#save
@@ -302,7 +316,13 @@ AnnotationController = [
     this.render = ->
       # Extend the view model with a copy of the domain model.
       # Note that copy is used so that deep properties aren't shared.
-      @annotation = angular.extend {}, angular.copy model
+      @annotation = angular.extend {}, angular.copy(model)
+
+      # if we have unsaved changes to this annotation, apply them
+      # to the view model
+      draft = drafts.get(model)
+      if draft
+        angular.extend @annotation, angular.copy(draft)
 
       # Set the URI
       @annotationURI = new URL("/a/#{@annotation.id}", this.baseURI).href
@@ -336,7 +356,7 @@ AnnotationController = [
         @document.title = @document.title[0..29] + '…'
 
       # Form the tags for ngTagsInput.
-      @annotation.tags = ({text} for text in (model.tags or []))
+      @annotation.tags = ({text} for text in (@annotation.tags or []))
 
     updateTimestamp = (repeat=false) =>
       @timestamp = time.toFuzzyString model.updated
@@ -351,10 +371,8 @@ AnnotationController = [
     # Export the baseURI for the share link
     this.baseURI = $document.prop('baseURI')
 
-    # Discard the draft if the scope goes away.
     $scope.$on '$destroy', ->
       updateTimestamp = angular.noop
-      drafts.remove model
 
     # watch for changes to the domain model and recreate the view model
     # when it changes
@@ -372,7 +390,7 @@ AnnotationController = [
             $rootScope.$emit('annotationCreated', model)
           highlight = false # Prevents double highlight creation.
         else
-          drafts.add model, => this.revert()
+          updateDraft(model)
 
       updateTimestamp(model is old)  # repeat on first run
       this.render()
@@ -388,8 +406,30 @@ AnnotationController = [
       model.permissions = model.permissions or permissions.default(model.group)
     )
 
-    # Start editing brand new annotations immediately
-    unless model.id? or (this.isHighlight() and highlight) then this.edit()
+    # if this is a new annotation or we have unsaved changes,
+    # then start editing immediately
+    isNewAnnotation = !(model.id || (this.isHighlight() && highlight));
+    if isNewAnnotation || drafts.get(model)
+      this.edit()
+
+    # when the current group changes, persist any unsaved changes using
+    # the drafts service. They will be restored when this annotation is
+    # next loaded.
+    $scope.$on events.GROUP_FOCUSED, ->
+      if !vm.editing
+        return
+
+      # if we have a draft, update it, otherwise (eg. when the user signs out)
+      # do not create a new one
+      if drafts.get(model)
+        draftDomainModel = {}
+        updateDomainModel(draftDomainModel, vm.annotation)
+        updateDraft(draftDomainModel)
+
+      # move any new annotations to the currently focused group when
+      # switching groups. See GH #2689 for context
+      if !model.id
+        model.group = groups.focused().id
 
     this
 ]

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

@@ -57,8 +57,9 @@ describe 'annotation', ->
       deleteAnnotation: sandbox.stub()
     fakeAnnotationUI = {}
     fakeDrafts = {
-      add: sandbox.stub()
+      update: sandbox.stub()
       remove: sandbox.stub()
+      get: sandbox.stub()
     }
     fakeFeatures = {
       flagEnabled: sandbox.stub().returns(true)
@@ -527,6 +528,84 @@ describe 'annotation', ->
 
       assert fakeFlash.error.notCalled
 
+  describe "drafts", ->
+
+    it "creates a draft when editing an annotation", ->
+      createDirective()
+      controller.edit()
+      assert.calledWith(fakeDrafts.update, annotation, {
+        text: annotation.text,
+        tags: annotation.tags,
+      })
+
+    it "starts editing immediately if there is a draft", ->
+      fakeDrafts.get.returns({
+        tags: [{text: 'unsaved'}]
+        text: 'unsaved-text'
+      })
+      createDirective()
+      assert.isTrue(controller.editing)
+
+    it "uses the text and tags from the draft if present", ->
+      fakeDrafts.get.returns({
+        tags: ['unsaved-tag']
+        text: 'unsaved-text'
+      })
+      createDirective()
+      assert.deepEqual(controller.annotation.tags, [{text: 'unsaved-tag'}])
+      assert.equal(controller.annotation.text, 'unsaved-text')
+
+    it "removes the draft when changes are discarded", ->
+      createDirective()
+      controller.edit()
+      controller.revert()
+      assert.calledWith(fakeDrafts.remove, annotation)
+
+    it "removes the draft when changes are saved", ->
+      annotation.$update = sandbox.stub().returns(Promise.resolve())
+      createDirective()
+      controller.edit()
+      controller.save()
+
+      # the controller currently removes the draft whenever an annotation
+      # update is committed on the server. This can happen either when saving
+      # locally or when an update is committed in another instance of H
+      # which is then pushed to the current instance
+      annotation.updated = (new Date()).toISOString()
+      $scope.$digest()
+      assert.calledWith(fakeDrafts.remove, annotation)
+
+  describe "when the focused group changes", ->
+
+    it "updates the current draft", ->
+      createDirective()
+      controller.edit()
+      controller.annotation.text = 'unsaved-text'
+      controller.annotation.tags = []
+      fakeDrafts.get = sinon.stub().returns({text: 'old-draft'})
+      fakeDrafts.update = sinon.stub()
+      $rootScope.$broadcast(events.GROUP_FOCUSED)
+      assert.calledWith(fakeDrafts.update, annotation, {
+        text: 'unsaved-text',
+        tags: []
+      })
+
+    it "should not create a new draft", ->
+      createDirective()
+      controller.edit()
+      fakeDrafts.update = sinon.stub()
+      fakeDrafts.get = sinon.stub().returns(null)
+      $rootScope.$broadcast(events.GROUP_FOCUSED)
+      assert.notCalled(fakeDrafts.update)
+
+    it "moves new annotations to the focused group", ->
+      annotation.id = null
+      createDirective()
+      fakeGroups.focused = sinon.stub().returns({id: 'new-group'})
+      $rootScope.$broadcast(events.GROUP_FOCUSED)
+      assert.equal(annotation.group, 'new-group')
+
+
 describe("AnnotationController", ->
 
   before(->
@@ -568,8 +647,9 @@ describe("AnnotationController", ->
       momentFilter: momentFilter or {}
       urlencodeFilter: urlencodeFilter or {}
       drafts: drafts or {
-        add: ->
+        update: ->
         remove: ->
+        get: ->
       }
       features: features or {
         flagEnabled: -> true

h/static/scripts/drafts.coffee

-module.exports = ->
-  _drafts = []
-
-  all: -> draft for {draft} in _drafts
-
-  add: (draft, cb) -> _drafts.push {draft, cb}
-
-  remove: (draft) ->
-    remove = []
-    for d, i in _drafts
-      remove.push i if d.draft is draft
-    while remove.length
-      _drafts.splice(remove.pop(), 1)
-
-  contains: (draft) ->
-    for d in _drafts
-      if d.draft is draft then return true
-    return false
-
-  isEmpty: -> _drafts.length is 0
-
-  discard: ->
-    text =
-      switch _drafts.length
-        when 0 then null
-        when 1
-          """You have an unsaved reply.
-
-          Do you really want to discard this draft?"""
-        else
-          """You have #{_drafts.length} unsaved replies.
-
-          Do you really want to discard these drafts?"""
-
-    if _drafts.length is 0 or confirm text
-      discarded = _drafts.slice()
-      _drafts = []
-      d.cb?() for d in discarded
-      true
-    else
-      false

h/static/scripts/drafts.js

+/**
+ * The drafts service provides temporary storage for unsaved edits
+ * to new or existing annotations.
+ *
+ * A draft consists of a 'model' which is the original annotation
+ * which the draft is associated with and `changes' which is
+ * a set of edits to the original annotation.
+ */
+function DraftStore() {
+  this._drafts = [];
+
+  // returns true if 'draft' is a draft for a given
+  // annotation. Annotations are matched by ID
+  // and annotation instance (for unsaved annotations
+  // which have no ID)
+  function match(draft, model) {
+    return draft.model === model ||
+           (draft.model.id && model.id === draft.model.id);
+  }
+
+  /**
+   * Returns a list of all new annotations (those with no ID) for which
+   * unsaved drafts exist.
+   */
+  this.unsaved = function unsaved() {
+    return this._drafts.filter(function (draft) {
+      return !draft.model.id;
+    }).map(function (draft) {
+      return draft.model;
+    });
+  }
+
+  /** Retrieve the draft changes for an annotation. */
+  this.get = function get(model) {
+    for (var i=0; i < this._drafts.length; i++) {
+      if (match(this._drafts[i], model)) {
+        return this._drafts[i].changes;
+      }
+    }
+  }
+
+  /**
+   * Update the draft version for a given annotation, replacing any
+   * existing draft.
+   */
+  this.update = function update(model, changes) {
+    var newDraft = {
+      model: model,
+      changes: changes,
+    };
+    this.remove(model);
+    this._drafts.push(newDraft);
+  }
+
+  /** Remove the draft version of an annotation. */
+  this.remove = function remove(model) {
+    this._drafts = this._drafts.filter(function (draft) {
+      return !match(draft, model);
+    });
+  }
+
+  /** Prompt to discard any unsaved drafts. */
+  this.discard = function discard() {
+    // TODO - Replace this with a UI which doesn't look terrible
+    var text;
+    if (this._drafts.length === 1) {
+      text = 'You have an unsaved reply.\n\n' +
+             'Do you really want to discard this draft?';
+    } else if (this._drafts.length > 1) {
+      text = 'You have ' + this._drafts.length + ' unsaved replies.\n\n'
+             'Do you really want to discard these drafts?';
+    }
+    if (this._drafts.length === 0 || window.confirm(text)) {
+      this._drafts = [];
+      return true;
+    } else {
+      return false;
+    }
+  }
+}
+
+module.exports = function () {
+  return new DraftStore();
+};

h/static/scripts/test/drafts-test.js

+var draftsService = require('../drafts');
+
+describe('drafts', function () {
+  var drafts;
+
+  beforeEach(function () {
+    drafts = draftsService();
+  });
+
+  describe('.update', function () {
+    it('should save changes', function () {
+      var model = {id: 'foo'};
+      assert.notOk(drafts.get(model));
+      drafts.update(model, {text: 'edit'});
+      assert.deepEqual(drafts.get(model), {text: 'edit'});
+    });
+
+    it('should replace existing drafts', function () {
+      var model = {id: 'foo'};
+      drafts.update(model, {text: 'foo'});
+      drafts.update(model, {text: 'bar'});
+      assert.deepEqual(drafts.get(model), {text: 'bar'});
+    });
+
+    it('should replace existing drafts with the same ID', function () {
+      var modelA = {id: 'foo'};
+      var modelB = {id: 'foo'};
+      drafts.update(modelA, {text: 'foo'});
+      drafts.update(modelB, {text: 'bar'});
+      assert.deepEqual(drafts.get(modelA), {text: 'bar'});
+    });
+  });
+
+  describe('.remove', function () {
+    it('should remove drafts', function () {
+      var model = {id: 'foo'};
+      drafts.update(model, {text: 'bar'});
+      drafts.remove(model);
+      assert.notOk(drafts.get(model));
+    });
+  });
+
+  describe('.unsaved', function () {
+    it('should return drafts for unsaved annotations', function () {
+      var model = {};
+      drafts.update(model, {text: 'bar'});
+      assert.deepEqual(drafts.unsaved(), [model]);
+    });
+
+    it('should not return drafts for saved annotations', function () {
+      var model = {id: 'foo'};
+      drafts.update(model, {text: 'baz'});
+      assert.deepEqual(drafts.unsaved(), []);
+    });
+  });
+});

h/static/scripts/test/widget-controller-test.coffee

@@ -39,11 +39,9 @@ describe 'WidgetController', ->
     fakeAuth = {user: null}
     fakeCrossFrame = {frames: []}
     fakeDrafts = {
-      all: sandbox.stub()
+      unsaved: sandbox.stub()
     }
 
-    lastSearchResult = null
-
     fakeStore = {
       SearchResource:
         get: (query, callback) ->
@@ -52,7 +50,6 @@ describe 'WidgetController', ->
           result =
             total: 100
             rows: [offset..offset+limit-1]
-          lastSearchResult = result
           callback result
     }
 
@@ -112,11 +109,15 @@ describe 'WidgetController', ->
     it 'should load annotations for the new group', ->
       fakeThreading.annotationList = sandbox.stub().returns([{id: '1'}])
       fakeCrossFrame.frames.push({uri: 'http://example.com'})
+      searchResult = {total: 10, rows: [0..10]}
+      fakeStore.SearchResource.get = (query, callback) ->
+        callback(searchResult)
+
       $scope.$broadcast(events.GROUP_FOCUSED)
 
       assert.calledWith(fakeAnnotationMapper.unloadAnnotations,
         [{id: '1'}])
       $scope.$digest();
       assert.calledWith(fakeAnnotationMapper.loadAnnotations,
-        lastSearchResult.rows)
-      assert.calledWith(fakeThreading.thread, fakeDrafts.all())
+        searchResult.rows)
+      assert.calledWith(fakeThreading.thread, fakeDrafts.unsaved())

h/static/scripts/widget-controller.coffee

@@ -22,7 +22,7 @@ module.exports = class WidgetController
       # Unload all the annotations
       annotationMapper.unloadAnnotations(threading.annotationList())
       # Reload all the drafts
-      threading.thread(drafts.all())
+      threading.thread(drafts.unsaved())
 
     _loadAnnotationsFrom = (query, offset) =>
       queryCore =