Preserve unsaved changes when switching groups

Refactor the drafts service to preserve unsaved edits
when switching groups.

This rewrites the drafts service so that it can preserve
unsaved changes for new and existing annotations when
switching groups.

For each annotation, the drafts service now maintains
an object containing the unsaved changes in addition to
the model for the annotation which was being edited.

 * For new annotations, the annotation is moved to the current
   group when switching groups.

 * For edits to existing annotations, the unsaved changes are
   saved to the drafts service.

 * When an annotation card is created, switch to editing mode
   automatically if a draft is present.

 * Avoid automatically discarding the draft when an annotation
   is unloaded. This allows unsaved edits to existing annotations
   in a group to be restored when switching back to the group.

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,27 @@ 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
+
+      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)
@@ -568,8 +569,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

@@ -77,6 +77,10 @@ describe 'WidgetController', ->
       focused: -> {id: 'foo'}
     }
 
+    fakeDrafts = {
+      unsaved: []
+    }
+
     $provide.value 'annotationMapper', fakeAnnotationMapper
     $provide.value 'annotationUI', fakeAnnotationUI
     $provide.value 'crossframe', fakeCrossFrame