Refactor client-side annotation permissions generation

Move the responsibility for creating the default permissions for new
annotations according to the cached-in-local-storage shared/private setting
into permissions.coffee, where it's with other permissions related stuff and
can be tested more easily.

h/static/scripts/directive/annotation.coffee

 ### global -validate ###
 
-STORAGE_KEY = 'hypothesis.privacy'
-
 # Validate an annotation.
 # Annotations must be attributed to a user or marked as deleted.
 # A public annotation is valid only if they have a body.
@@ -46,10 +44,10 @@ errorMessage = (reason) ->
 AnnotationController = [
   '$scope', '$timeout', '$q', '$rootScope', '$document',
   'drafts', 'flash', 'permissions', 'tags', 'time',
-  'annotationUI', 'annotationMapper', 'session', 'groups', 'localStorage'
+  'annotationUI', 'annotationMapper', 'session', 'groups',
   ($scope,   $timeout,   $q,   $rootScope,   $document,
    drafts,   flash,   permissions,   tags,   time,
-   annotationUI,   annotationMapper,   session,   groups,   localStorage) ->
+   annotationUI,   annotationMapper,   session,   groups) ->
 
     @annotation = {}
     @action = 'view'
@@ -61,24 +59,12 @@ AnnotationController = [
 
     model = $scope.annotationGet()
 
-    # Set model.permissions to the default permissions for new annotations,
-    # _if_ model.permissions isn't already set.
-    setDefaultPermissions = ->
-      if not model.permissions?.read
-        defaultLevel = localStorage.getItem(STORAGE_KEY);
-        if (defaultLevel != 'private') and (defaultLevel != 'shared')
-          defaultLevel = 'shared';
-        if defaultLevel == 'private'
-          model.permissions = permissions.private()
-        else
-          model.permissions = permissions.shared(model.group)
-
     # Set the group of new annotations.
     if not model.group
       model.group = groups.focused().id
 
     # Set the permissions of new annotations.
-    setDefaultPermissions()
+    model.permissions = model.permissions or permissions.default(model.group)
 
     highlight = model.$highlight
     original = null
@@ -145,7 +131,7 @@ AnnotationController = [
       # next time they create an annotation.
       # But _don't_ cache it when they change the privacy level of a reply.
       if not model.references  # If the annotation is not a reply.
-        localStorage.setItem(STORAGE_KEY, privacy);
+        permissions.setDefault(privacy)
 
       if privacy == 'private'
         @annotation.permissions = permissions.private()
@@ -384,8 +370,6 @@ AnnotationController = [
         else
           drafts.add model, => this.revert()
 
-      setDefaultPermissions()
-
       updateTimestamp(model is old)  # repeat on first run
       this.render()
     , true
@@ -393,9 +377,15 @@ AnnotationController = [
     # Watch the current user
     # TODO: fire events instead since watchers are not free and auth is rare
     $scope.$watch (-> session.state.userid), (userid) ->
-      model.permissions ?= {}
       model.user ?= userid
 
+      # Set model.permissions on sign in, if it isn't already set.
+      # This is because you can create annotations when signed out and they
+      # will have model.permissions = null, then when you sign in we set the
+      # permissions correctly here.
+      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()
 

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

@@ -27,7 +27,6 @@ describe 'annotation', ->
   fakeTags = null
   fakeTime = null
   fakeUrlEncodeFilter = null
-  fakeLocalStorage = null
   sandbox = null
 
   createDirective = ->
@@ -71,6 +70,8 @@ describe 'annotation', ->
       permits: sandbox.stub().returns(true)
       shared: sandbox.stub().returns({read: ['everybody']})
       private: sandbox.stub().returns({read: ['justme']})
+      default: sandbox.stub().returns({read: ['default']})
+      setDefault: sandbox.stub()
     }
     fakePersonaFilter = sandbox.stub().returnsArg(0)
     fakeDocumentTitleFilter = (arg) -> ''
@@ -89,10 +90,6 @@ describe 'annotation', ->
       nextFuzzyUpdate: sandbox.stub().returns(30)
     }
     fakeUrlEncodeFilter = (v) -> encodeURIComponent(v)
-    fakeLocalStorage = {
-      setItem: sandbox.stub()
-      getItem: sandbox.stub()
-    }
 
     fakeGroups = {
       focused: -> {}
@@ -114,7 +111,6 @@ describe 'annotation', ->
     $provide.value 'tags', fakeTags
     $provide.value 'time', fakeTime
     $provide.value 'urlencodeFilter', fakeUrlEncodeFilter
-    $provide.value 'localStorage', fakeLocalStorage
     $provide.value 'groups', fakeGroups
     return
 
@@ -240,8 +236,7 @@ describe 'annotation', ->
       controller.edit();
       controller.setPrivacy('shared')
       controller.save().then(->
-        assert(fakeLocalStorage.setItem.calledWithExactly(
-          'hypothesis.privacy', 'shared'))
+        assert(fakePermissions.setDefault.calledWithExactly('shared'))
       )
 
     it 'saves the "private" visibility level to localStorage', ->
@@ -249,8 +244,7 @@ describe 'annotation', ->
       controller.edit();
       controller.setPrivacy('private')
       controller.save().then(->
-        assert(fakeLocalStorage.setItem.calledWithExactly(
-          'hypothesis.privacy', 'private'))
+        assert(fakePermissions.setDefault.calledWithExactly('private'))
       )
 
     it "doesn't save the visibility if the annotation is a reply", ->
@@ -259,7 +253,7 @@ describe 'annotation', ->
       controller.edit();
       controller.setPrivacy('private')
       controller.save().then(->
-        assert(not fakeLocalStorage.setItem.called)
+        assert(not fakePermissions.setDefault.called)
       )
 
   describe '#hasContent', ->
@@ -587,16 +581,10 @@ describe("AnnotationController", ->
         isPrivate: (permissions, user) ->
           return user in permissions.read
         permits: -> true
-        shared: (group) ->
-          if not session?.state?.userid
-            return null
-          else
-            {"read": [group]}
-        private: ->
-          if not session?.state?.userid
-            return null
-          else
-            {"read": [session.state.userid]}
+        shared: -> {}
+        private: -> {}
+        default: -> {}
+        setDefault: ->
       }
       session: session
       tags: tags or {store: ->}
@@ -657,42 +645,22 @@ describe("AnnotationController", ->
     )
   )
 
-  it("sets the permissions of new annotations from local storage", ->
-    {controller} = createAnnotationDirective({
-      localStorage: {
-        setItem: ->
-        getItem: (key) ->
-          if key == 'hypothesis.privacy'
-            return 'shared'
-          else
-            assert(false, "Wrong key requested from localStorage")
-      }
-    })
-    assert(controller.isShared())
-  )
+  it("sets the permissions of new annotations", ->
+    # This is a new annotation, doesn't have any permissions yet.
+    annotation = {group: "test-group"}
+    permissions = {
+      default: sinon.stub().returns("default permissions")
+      isShared: ->
+      isPrivate: ->
+    }
 
-  it("sets the permissions of new annotations from local storage to private", ->
-    {controller} = createAnnotationDirective({
-      localStorage: {
-        setItem: ->
-        getItem: (key) ->
-          if key == 'hypothesis.privacy'
-            return 'private'
-          else
-            assert(false, "Wrong key requested from localStorage")
-      }
-    })
-    assert(controller.isPrivate())
-  )
+    createAnnotationDirective({
+      annotation: annotation, permissions: permissions})
 
-  it("defaults to shared if no locally cached visibility", ->
-    {controller} = createAnnotationDirective({
-      localStorage: {
-        setItem: ->
-        getItem: ->
-      }
-    })
-    assert(controller.isShared())
+    assert(permissions.default.calledWithExactly("test-group"))
+    assert.equal(annotation.permissions, "default permissions",
+      "It should set a new annotation's permissions to what
+      permissions.default() returns")
   )
 
   it("doesn't overwrite permissions if the annotation already has them", ->
@@ -708,8 +676,14 @@ describe("AnnotationController", ->
     }
     # Save the original permissions for asserting against later.
     original_permissions = JSON.parse(JSON.stringify(annotation.permissions))
+    permissions = {
+      default: sinon.stub().returns("new permissions")
+      isShared: ->
+      isPrivate: ->
+    }
 
-    createAnnotationDirective({annotation: annotation})
+    createAnnotationDirective(
+      {annotation: annotation, permissions: permissions})
 
     assert.deepEqual(annotation.permissions, original_permissions)
   )
@@ -747,15 +721,37 @@ describe("AnnotationController", ->
 
   describe("when the user signs in", ->
     it("sets the permissions of unsaved annotations", ->
+      # This annotation has no permissions yet, because that's what happens
+      # when you create a new annotation while not signed in.
       annotation = {group: "__world__"}
       session = {state: {userid: null}}  # Not signed in.
+      permissions = {
+        # permissions.default() would return null, because the user isn't
+        # signed in.
+        default: -> null
+        # We just need to mock these two functions so that it doesn't crash.
+        isShared: ->
+        isPrivate: ->
+      }
 
       {$rootScope} = createAnnotationDirective(
-        {annotation: annotation, session:session})
+        {annotation: annotation, session:session, permissions: permissions})
+
+      # At this point we would not expect the permissions to have been set,
+      # even though the annotation has been created, because the user isn't
+      # signed in.
+      assert(!annotation.permissions)
+
+      # Sign the user in.
+      session.state.userid = "acct:fred@hypothes.is"
+
+      # permissions.default() would now return permissions, because the user
+      # is signed in.
+      permissions.default = -> "__default_permissions__"
 
-      session.state.userid = "acct:fred@hypothes.is"  # The user signs in.
       $rootScope.$digest()  # Trigger the $scope.$watch().
-      assert.deepEqual(annotation.permissions, {read: ["__world__"]})
+
+      assert.equal(annotation.permissions, "__default_permissions__")
     )
   )
 

h/static/scripts/permissions.coffee

+STORAGE_KEY = 'hypothesis.privacy'
+
 ###*
 # @ngdoc service
 # @name  Permissions
@@ -6,7 +8,7 @@
 # This service can set default permissions to annotations properly and
 # offers some utility functions regarding those.
 ###
-module.exports = ['session', (session) ->
+module.exports = ['session', 'localStorage', (session, localStorage) ->
   ALL_PERMISSIONS = {}
   GROUP_WORLD = 'group:__world__'
   ADMIN_PARTY = [{
@@ -37,7 +39,7 @@ module.exports = ['session', (session) ->
   # Typical use: annotation.permissions = permissions.private()
   ###
   private: ->
-    if not session?.state?.userid
+    if not session.state.userid
       return null
     else
       return {
@@ -57,7 +59,7 @@ module.exports = ['session', (session) ->
   # Typical use: annotation.permissions = permissions.shared(group)
   ###
   shared: (group) ->
-    if not session?.state?.userid
+    if not session.state.userid
       return null
     if group?
       group = 'group:' + group
@@ -70,6 +72,25 @@ module.exports = ['session', (session) ->
       admin: [session.state.userid]
     }
 
+  # Return the initial permissions for a new annotation in the given group.
+  default: (group) ->
+    defaultLevel = localStorage.getItem(STORAGE_KEY);
+    if (defaultLevel != 'private') and (defaultLevel != 'shared')
+      defaultLevel = 'shared';
+    if defaultLevel == 'private'
+      return this.private()
+    else
+      return this.shared(group)
+
+  # Set the default initial permissions for new annotations (that will be
+  # returned by default() above) to "private" or "shared" permissions.
+  #
+  # @param {string} private_or_shared "private" to make default() return the
+  #   necessary permissions for private annotations, "shared" to make it
+  #   return those for shared annotations.
+  setDefault: (private_or_shared) ->
+    localStorage.setItem(STORAGE_KEY, private_or_shared);
+
   ###*
   # @ngdoc method
   # @name permissions#isShared

h/static/scripts/test/permissions-test.coffee

@@ -3,6 +3,7 @@
 describe 'h:permissions', ->
   sandbox = null
   fakeSession = null
+  fakeLocalStorage = null
 
   before ->
     angular.module('h', [])
@@ -17,8 +18,12 @@ describe 'h:permissions', ->
         userid: 'acct:flash@gordon'
       }
     }
+    fakeLocalStorage = {
+      getItem: -> undefined
+    }
 
     $provide.value 'session', fakeSession
+    $provide.value 'localStorage', fakeLocalStorage
     return
 
   afterEach ->
@@ -64,6 +69,38 @@ describe 'h:permissions', ->
         delete fakeSession.state.userid
         assert.equal(permissions.shared("foo"), null)
 
+    describe 'default()', ->
+
+      it 'returns shared permissions if localStorage contains "shared"', ->
+        fakeLocalStorage.getItem = -> 'shared'
+        assert(permissions.isShared(permissions.default()))
+
+      it 'returns private permissions if localStorage contains "private"', ->
+        fakeLocalStorage.getItem = -> 'private'
+        assert(permissions.isPrivate(
+          permissions.default(), fakeSession.state.userid))
+
+      it 'returns shared permissions if localStorage is empty', ->
+        fakeLocalStorage.getItem = -> undefined
+        assert(permissions.isShared(permissions.default()))
+
+    describe 'setDefault()', ->
+
+      it 'sets the default permissions that default() will return', ->
+        stored = {}
+        fakeLocalStorage.setItem = (key, value) ->
+          stored[key] = value
+        fakeLocalStorage.getItem = (key) ->
+          return stored[key]
+
+        permissions.setDefault('private')
+        assert(permissions.isPrivate(
+          permissions.default(), fakeSession.state.userid))
+
+        permissions.setDefault('shared')
+        assert(permissions.isShared(
+          permissions.default('foo'), 'foo'))
+
     describe 'isPublic', ->
       it 'isPublic() true if the read permission has group:__world__ in it', ->
         permission = {