Don't change model.group on group change

Don't change model.group in AnnotationController when the currently focused
group changes.

model is supposed to be a read-only domain model, change vm.annotation instead.

Changes to vm.annotation are copied over to model before saving model to the
server.

To make this work vm.group() should return the current group of the annotation,
accounting for any changes made to vm.annotation but not saved to model or to
the server yet. It should return vm.annotation.group not model.group.

Similarly vm.isShared() should be based on vm.annotation not on model.

Also remove some code in thread.coffee that was deliberately hiding new
annotations that don't belong to the focused group (see commit
272b2432c48a2d6bf890daa863e92f3a678b27b5). This is no longer the desired
behaviour - when changing groups any new annotations move to the new group with
us (and the GROUP_FOCUSED listener in AnnotationController will update
vm.annotation.group to match).

h/static/scripts/directive/annotation.js

@@ -280,11 +280,9 @@ function AnnotationController(
       var isShared = permissions.isShared(
         vm.annotation.permissions, vm.annotation.group);
       if (isShared) {
-        model.permissions = permissions.shared(newGroup);
-        vm.annotation.permissions = model.permissions;
+        vm.annotation.permissions = permissions.shared(newGroup);
       }
-      model.group = newGroup;
-      vm.annotation.group = model.group;
+      vm.annotation.group = newGroup;
     }
 
     if (drafts.get(model)) {
@@ -385,7 +383,7 @@ function AnnotationController(
       updateTimestamp(true);
       $scope.$digest();
     }, nextUpdate, false);
-  }
+  };
 
   /** Switches the view to a viewer, closing the editor controls if they're
    *  open.
@@ -465,7 +463,7 @@ function AnnotationController(
     * @returns {Object} The full group object associated with the annotation.
     */
   vm.group = function() {
-    return groups.get(model.group);
+    return groups.get(vm.annotation.group);
   };
 
   /**
@@ -533,7 +531,8 @@ function AnnotationController(
     * current group or with everyone).
     */
   vm.isShared = function() {
-    return permissions.isShared(vm.annotation.permissions, model.group);
+    return permissions.isShared(
+      vm.annotation.permissions, vm.annotation.group);
   };
 
   // Save on Meta + Enter or Ctrl + Enter.
@@ -666,7 +665,7 @@ function AnnotationController(
     if (privacy === 'private') {
       vm.annotation.permissions = permissions.private();
     } else if (privacy === 'shared') {
-      vm.annotation.permissions = permissions.shared(model.group);
+      vm.annotation.permissions = permissions.shared(vm.annotation.group);
     }
   };
 

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

@@ -1502,41 +1502,51 @@ describe('annotation.js', function() {
 
       it('moves new annotations to the focused group', function() {
         var annotation = defaultAnnotation();
+        // id must be null so that AnnotationController considers this a new
+        // annotation.
         annotation.id = null;
-        createDirective(annotation);
-        fakeGroups.focused = sinon.stub().returns({
-          id: 'new-group'
-        });
+        var controller = createDirective(annotation).controller;
+
+        // Change the currently focused group.
+        fakeGroups.focused = sinon.stub().returns({id: 'new-group'});
         $rootScope.$broadcast(events.GROUP_FOCUSED);
-        assert.equal(annotation.group, 'new-group');
+
+        assert.equal(
+          controller.annotation.group,
+          'new-group',
+          'It should update the group ID in the view model when the focused ' +
+          'group changes.');
       });
     });
 
     it(
       'updates perms when moving new annotations to the focused group',
       function() {
+        var annotation = defaultAnnotation();
         // id must be null so that AnnotationController considers this a new
         // annotation.
-        var annotation = defaultAnnotation();
         annotation.id = null;
         annotation.group = 'old-group';
-        annotation.permissions = {
-          read: [annotation.group]
-        };
+        annotation.permissions = {read: [annotation.group]};
         // This is a shared annotation.
         fakePermissions.isShared.returns(true);
-        createDirective(annotation);
+        var controller = createDirective(annotation).controller;
         // Make permissions.shared() behave like we expect it to.
         fakePermissions.shared = function(groupId) {
           return {
             read: [groupId]
           };
         };
-        fakeGroups.focused = sinon.stub().returns({
-          id: 'new-group'
-        });
+
+        // Change the currently focused group.
+        fakeGroups.focused = sinon.stub().returns({id: 'new-group'});
         $rootScope.$broadcast(events.GROUP_FOCUSED);
-        assert.deepEqual(annotation.permissions.read, ['new-group']);
+
+        assert.deepEqual(
+          controller.annotation.permissions.read,
+          ['new-group'],
+          'It should update the read permissions in the view model when ' +
+          'the focused group changes');
       }
     );
 

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

@@ -5,7 +5,6 @@ describe 'thread', ->
   $element = null
   $scope = null
   controller = null
-  fakeGroups = null
   fakeRender = null
   fakeAnnotationUI = null
   sandbox = null
@@ -25,9 +24,6 @@ describe 'thread', ->
 
   beforeEach module ($provide) ->
     sandbox = sinon.sandbox.create()
-    fakeGroups = {
-      focused: sandbox.stub().returns({id: '__world__'})
-    }
     fakeRender = sandbox.spy()
     fakeAnnotationUI = {
       hasSelectedAnnotations: ->
@@ -35,7 +31,6 @@ describe 'thread', ->
       isAnnotationSelected: (id) ->
         selectedAnnotations.indexOf(id) != -1
     }
-    $provide.value 'groups', fakeGroups
     $provide.value 'render', fakeRender
     $provide.value 'annotationUI', fakeAnnotationUI
     return
@@ -160,20 +155,6 @@ describe 'thread', ->
               id: 123
               group: 'wibble'
 
-        it 'is false for draft annotations not from the focused group', ->
-          # Set the focused group to one other than the annotation's group.
-          fakeGroups.focused.returns({id: 'foo'})
-
-          # Make the annotation into a "draft" annotation (make isNew() return
-          # true).
-          delete controller.container.message.id
-
-          assert.isFalse(controller.shouldShow())
-
-        it 'is true when the focused group does match', ->
-          fakeGroups.focused.returns({id: 'wibble'})
-          assert.isTrue(controller.shouldShow())
-
       describe 'filters messages based on the selection', ->
         messageID = 456
 

h/static/scripts/directive/thread.coffee

@@ -13,8 +13,8 @@ uuid = require('node-uuid')
 # the collapsing behavior.
 ###
 ThreadController = [
-  '$scope', 'groups', 'annotationUI'
-  ($scope,   groups,   annotationUI) ->
+  '$scope', 'annotationUI'
+  ($scope,   annotationUI) ->
     @container = null
     @collapsed = true
     @parent = null
@@ -44,14 +44,6 @@ ThreadController = [
     # current system state.
     ###
     this.shouldShow = ->
-      # Hide "draft" annotations (new annotations that haven't been saved to
-      # the server yet) that don't belong to the focused group. These draft
-      # annotations persist across route reloads so they have to be hidden
-      # here.
-      group = this.container?.message?.group
-      if this.isNew() and group and group != groups.focused().id
-        return false
-
       # when there is a selection, hide unselected annotations
       annotationID = this.container?.message?.id
       if annotationUI.hasSelectedAnnotations() &&