Refactor drafting and authentication interactions

Make changes to allow multiple top-level drafts, drafting before
authenticating, keeping drafts while performing page searches,
and replying to search results.

- Factor out the userAuthorize option passed to the Permissions
  plugin into a local function in controllers.coffee.

- Only load the Permissions plugin when logged in so that the
  plugin options do not leak information about the last logged in
  user and unattributed drafts have no user or permissions field.

- When switching to highlight mode, show the sidebar and flash an
  info message informing the user that they will need to log in
  before annotations are saved, but don't prohibit highlighting.

- Simplify the logic in the guest for highlight mode by offloading
  work to the annotation controller. The "inject" property is no
  longer needed. The annotation controller saves drafts of highlights
  if they have no user and persists them to the server as soon as
  the user is set.

- When logging in, the app controller simply loops through all the
  drafts are publishes 'beforeAnnotationUpdated' to give the
  Permissions plugin a chance to fire.

- When initializing the Store plugin, don't perform any search at
  all when not logged in. This change makes "single player" mode
  work even before logging in.

- Allow hiding the sidebar when editing. Simplify the guest code
  that no longer needs to track the sidebar state by listening for
  'annotationEditorHide' and 'annotationEditorSubmit' and the
  corresponding sidebar code that fired these. They're nonsensical
  now because there isn't exactly one "editor".

- Remove the concept of an 'ongoing edit' entirely, simplifying the
  sidebar code even further. Any number of edits are allowed and
  top level ones are not treated specially.

h/static/scripts/controllers.coffee

@@ -9,6 +9,32 @@ imports = [
 ]
 
 
+# User authorization function for the Permissions plugin.
+authorizeAction = (action, annotation, user) ->
+  if annotation.permissions
+    tokens = annotation.permissions[action] || []
+
+    if tokens.length == 0
+      # Empty or missing tokens array: only admin can perform action.
+      return false
+
+    for token in tokens
+      if user == token
+        return true
+      if token == 'group:__world__'
+        return true
+
+    # No tokens matched: action should not be performed.
+    return false
+
+  # Coarse-grained authorization
+  else if annotation.user
+    return user and user == annotation.user
+
+  # No authorization info on annotation: free-for-all!
+  true
+
+
 class App
   this.$inject = [
     '$location', '$q', '$route', '$scope', '$timeout',
@@ -75,18 +101,20 @@ class App
       """Initialize the storage component."""
       Store = plugins.Store
       delete plugins.Store
-      annotator.addPlugin 'Store', annotator.options.Store
 
-      $scope.store = plugins.Store
+      if $scope.persona or annotator.socialView.name is 'none'
+        annotator.addPlugin 'Store', annotator.options.Store
+
+        $scope.store = plugins.Store
 
-      _id = $route.current.params.id
-      _promise = null
+        _id = $route.current.params.id
+        _promise = null
 
-      # Load any initial annotations that should be displayed
-      if _id
-        # XXX: Two requests here is less than ideal
-        plugins.Store.loadAnnotationsFromSearch({_id}).then ->
-          plugins.Store.loadAnnotationsFromSearch({references: _id})
+        # Load any initial annotations that should be displayed
+        if _id
+          # XXX: Two requests here is less than ideal
+          plugins.Store.loadAnnotationsFromSearch({_id}).then ->
+            plugins.Store.loadAnnotationsFromSearch({references: _id})
 
       return unless Store
       Store.destroy()
@@ -112,7 +140,7 @@ class App
       cull = (acc, annotation) ->
         if annotator.tool is 'highlight' and annotation.user != user
           acc.drop.push annotation
-        else if plugins.Permissions.authorize('read', annotation, user)
+        else if authorizeAction 'read', annotation, user
           acc.keep.push annotation
         else
           acc.drop.push annotation
@@ -120,7 +148,11 @@ class App
 
       {keep, drop} = Store.annotations.reduce cull, {keep: [], drop: []}
       Store.annotations = []
-      plugins.Store.annotations = keep
+
+      if plugins.Store?
+        plugins.Store.annotations = keep
+      else
+        drop = drop.concat keep
 
       # Clean up the ones that should be removed.
       do cleanup = (drop) ->
@@ -177,10 +209,6 @@ class App
       _dfdSock.promise
 
     onlogin = (assertion) ->
-      # Delete any old Auth plugin.
-      plugins.Auth?.destroy()
-      delete plugins.Auth
-
       # Configure the Auth plugin with the issued assertion as refresh token.
       annotator.addPlugin 'Auth',
         tokenUrl: documentHelpers.absoluteURI(
@@ -188,22 +216,27 @@ class App
 
       # Set the user from the token.
       plugins.Auth.withToken (token) ->
-        plugins.Permissions._setAuthFromToken(token)
-        loggedInUser = plugins.Permissions.user.replace /^acct:/, ''
+        annotator.addPlugin 'Permissions',
+          user: token.userId
+          userAuthorize: authorizeAction
+          permissions:
+            read: [token.userId]
+            update: [token.userId]
+            delete: [token.userId]
+            admin: [token.userId]
+        loggedInUser = token.userId.replace /^acct:/, ''
         reset()
 
     onlogout = ->
+      return unless drafts.discard()
+
       plugins.Auth?.element.removeData('annotator:headers')
+      plugins.Auth?.destroy()
       delete plugins.Auth
 
-      plugins.Permissions.setUser(null)
-
-      # XXX: Temporary workaround until Annotator v2.0 or v1.2.10
-      plugins.Permissions.options.permissions =
-        read: []
-        update: []
-        delete: []
-        admin: []
+      plugins.Permissions?.setUser(null)
+      plugins.Permissions?.destroy()
+      delete plugins.Permissions
 
       loggedInUser = null
       reset()
@@ -212,11 +245,9 @@ class App
       # Do not rely on the identity service to invoke callbacks within an
       # angular digest cycle.
       $scope.$evalAsync ->
-        if $scope.ongoingHighlightSwitch
-          $scope.ongoingHighlightSwitch = false
-          annotator.setTool 'highlight'
-        else
-          annotator.setTool 'comment'
+        # Update any edits in progress.
+        for draft in drafts.all()
+          annotator.publish 'beforeAnnotationCreated', draft
 
         # Convert the verified user id to the format used by the API.
         persona = loggedInUser
@@ -231,19 +262,6 @@ class App
           initStore()
           initUpdater()
 
-    annotator.subscribe 'annotationCreated', (annotation) ->
-      if annotation is $scope.ongoingEdit?.message
-        delete $scope.ongoingEdit
-        for p in providers
-          p.channel.notify method: 'onEditorSubmit'
-          p.channel.notify method: 'onEditorHide'
-
-    annotator.subscribe 'annotationDeleted', (annotation) ->
-      if annotation is $scope.ongoingEdit?.message
-        delete $scope.ongoingEdit
-        for p in providers
-          p.channel.notify method: 'onEditorHide'
-
     annotator.subscribe 'serviceDiscovery', (options) ->
       annotator.options.Store ?= {}
       angular.extend annotator.options.Store, options
@@ -253,21 +271,11 @@ class App
 
     $scope.$watch 'socialView.name', (newValue, oldValue) ->
       return if newValue is oldValue
-
-      if $scope.persona
-        initStore()
-      else if newValue is 'single-player'
-        identity.request()
-
-    $scope.$watch 'frame.visible', (newValue, oldValue) ->
-      if newValue
+      initStore()
+      if newValue is 'single-player' and not $scope.persona
         annotator.show()
-        annotator.host.notify method: 'showFrame'
-      else if oldValue
-        annotator.hide()
-        annotator.host.notify method: 'hideFrame'
-        for p in annotator.providers
-          p.channel.notify method: 'setActiveHighlights'
+        flash 'info',
+          'You will need to sign in for your highlights to be saved.'
 
     $scope.$watch 'sort.name', (name) ->
       return unless name
@@ -299,8 +307,6 @@ class App
         sock.send(JSON.stringify(sockmsg))
 
     $scope.authTimeout = ->
-      delete $scope.ongoingEdit
-      $scope.ongoingHighlightSwitch = false
       flash 'info',
         'For your security, the forms have been reset due to inactivity.'
 
@@ -310,7 +316,6 @@ class App
       $scope.selectedAnnotationsCount = 0
 
     $scope.baseURI = documentHelpers.baseURI
-    $scope.frame = visible: false
     $scope.id = identity
 
     $scope.model = persona: undefined
@@ -324,8 +329,7 @@ class App
 
       update: (query) ->
         unless angular.equals $location.search()['q'], query
-          if drafts.discard()
-            $location.search('q', query or null)
+          $location.search('q', query or null)
 
     $scope.socialView = annotator.socialView
     $scope.sort = name: 'Location'
@@ -372,8 +376,6 @@ class Viewer
     $scope.activate = (annotation) ->
       if angular.isObject annotation
         highlights = [annotation.$$tag]
-      else if $scope.ongoingEdit
-        highlights = [$scope.ongoingEdit.message.$$tag]
       else
         highlights = []
       for p in annotator.providers
@@ -383,7 +385,7 @@ class Viewer
 
     $scope.shouldShowThread = (container) ->
       if $routeParams.q
-        container.shown
+        container.shown or not container.message.id
       else if $scope.selectedAnnotations?
         if container.parent is $scope.threading.root
           $scope.selectedAnnotations[container.message?.id]
@@ -409,10 +411,11 @@ class Viewer
 
     render = (acc, container) ->
       {lastShown, renderList} = acc
+      id = container.message?.id
       container.more = 0
       container.order = renderList.length
       container.renderList = renderList
-      container.shown = matchSet is null or matchSet[container.message?.id]?
+      container.shown = matchSet is null or not id or matchSet[id]
       renderList.push container
 
       if container.shown

h/static/scripts/directives.coffee

@@ -225,8 +225,11 @@ repeatAnim = ->
 
 username = ['$filter', '$window', ($filter, $window) ->
   link: (scope, elem, attr) ->
-    scope.$watch 'user', ->
-      scope.uname = $filter('persona')(scope.user, 'username')
+    scope.$watch 'user', (user) ->
+      if user
+        scope.uname = $filter('persona')(scope.user, 'username')
+      else
+        scope.uname =  '<nobody>'
 
     scope.uclick = (event) ->
       event.preventDefault()

h/static/scripts/directives/annotation.coffee

@@ -14,7 +14,6 @@ extractURIComponent = (uri, component) ->
 # A non-public annotation requires only a target (e.g. a highlight).
 validate = (value) ->
   return unless angular.isObject value
-  return unless value.user or value.deleted
   worldReadable = 'group:__world__' in (value.permissions?.read or [])
   (value.tags?.length or value.text?.length) or
   (value.target?.length and not worldReadable)
@@ -38,8 +37,8 @@ validate = (value) ->
 # {@link annotator annotator service} for persistence.
 ###
 AnnotationController = [
-  '$scope', 'annotator', 'drafts',
-  ($scope,   annotator,   drafts) ->
+  '$scope', 'annotator', 'drafts', 'flash'
+  ($scope,   annotator,   drafts,   flash) ->
     @annotation = {}
     @action = 'view'
     @document = null
@@ -47,7 +46,8 @@ AnnotationController = [
     @editing = false
     @embedded = false
 
-    model = null
+    highlight = annotator.tool is 'highlight'
+    model = $scope.annotationGet()
     original = null
     vm = this
 
@@ -80,7 +80,7 @@ AnnotationController = [
     # @description Switches the view to an editor.
     ###
     this.edit = ->
-      drafts.add model, => this.revert()
+      drafts.add model, -> vm.revert()
       @action = if model.id? then 'edit' else 'create'
       @editing = true
       @preview = 'no'
@@ -105,7 +105,10 @@ AnnotationController = [
     # @description Saves any edits and returns to the viewer.
     ###
     this.save = ->
-      return unless validate(@annotation)
+      unless model.user or model.deleted
+        return flash 'info', 'Please sign in to save annotations.'
+      unless validate(@annotation)
+        return flash 'info', 'Please add text or a tag before publishing.'
       @editing = false
 
       angular.extend model, @annotation,
@@ -124,11 +127,6 @@ AnnotationController = [
     # changes. Initializes brand new annotations and
     ###
     this.render = ->
-      # Initialize brand new annotations.
-      unless model.id? or drafts.contains model
-        annotator.publish 'beforeAnnotationCreated', model
-        this.edit()
-
       # Extend the view model with a copy of the domain model.
       # Note that copy is used so that deep properties aren't shared.
       angular.extend @annotation, angular.copy model
@@ -158,12 +156,26 @@ AnnotationController = [
     $scope.$on 'threadCollapse', (event) ->
       event.preventDefault() if vm.editing
 
-    # Render on updates
-    $scope.$watchCollection (-> $scope.annotationGet()), (value) ->
-      model = value
-      if value?
-        if value.updated then drafts.remove model
-        vm.render()
+    # Render on updates.
+    $scope.$watch (-> model.updated), (updated) ->
+      if updated then drafts.remove model
+      vm.render()
+
+    # Save highlights once logged in.
+    $scope.$watch (-> model.user), (user) ->
+      return unless highlight and not model.references
+      if user
+        annotator.publish 'annotationCreated', model
+      else
+        drafts.add model, -> vm.revert()
+
+    # Initialize brand now annotations
+    unless model.id?
+      if model.references
+        annotator.publish 'beforeAnnotationCreated', model
+        vm.edit()
+      else if not highlight
+        vm.edit()
 
     this
 ]

h/static/scripts/directives/thread.coffee

@@ -17,8 +17,8 @@ COLLAPSED_CLASS = 'thread-collapsed'
 # replying and sharing.
 ###
 ThreadController = [
-  '$attrs', '$element', '$parse', '$scope', 'render',
-  ($attrs,   $element,   $parse,   $scope,   render) ->
+  '$attrs', '$element', '$parse', '$scope', 'flash', 'render',
+  ($attrs,   $element,   $parse,   $scope,   flash,   render) ->
     @container = null
     @collapsed = false
     @hover = false
@@ -33,6 +33,8 @@ ThreadController = [
     # Creates a new message in reply to this thread.
     ###
     this.reply = ->
+      unless @container.message.id
+        return flash 'error', 'You must publish this before replying to it.'
       if @collapsed then this.toggleCollapsed()
 
       # Extract the references value from this container.
@@ -71,6 +73,8 @@ ThreadController = [
     # Toggle the shared property.
     ###
     this.toggleShared = ->
+      unless @container.message.id
+        return flash 'error', 'You must publish this before sharing it.'
       @shared = not @shared
       if @shared
         # Focus and select the share link

h/static/scripts/guest.coffee

@@ -191,8 +191,12 @@ class Annotator.Guest extends Annotator
 
   _setupWrapper: ->
     @wrapper = @element
-    .on 'click', =>
-      unless @selectedTargets?.length
+    .on 'click', (event) =>
+      if @selectedTargets?.length
+        if @tool is 'highlight'
+          # Create the annotation
+          annotation = this.setupAnnotation(this.createAnnotation())
+      else
         @hideFrame()
     this
 
@@ -269,23 +273,14 @@ class Annotator.Guest extends Annotator
     return confirm "You have selected a very short piece of text: only " + length + " chars. Are you sure you want to highlight this?"
 
   onSuccessfulSelection: (event, immediate) ->
+    return unless @canAnnotate
     if @tool is 'highlight'
-
-      # Describe the selection with targets
-      @selectedTargets = (@_getTargetFromSelection(s) for s in event.segments)
-
       # Do we really want to make this selection?
       return false unless this.confirmSelection()
-
-      # Create the annotation
-      annotation = {inject: true}
-      this.publish 'beforeAnnotationCreated', annotation
-
-      # Set up the annotation
-      annotation = this.setupAnnotation annotation
-      this.publish 'annotationCreated', annotation
-    else
-      super
+      # Describe the selection with targets
+      @selectedTargets = (@_getTargetFromSelection(s) for s in event.segments)
+      return
+    super
 
   onAnchorMouseover: (event) ->
     this.addEmphasis event.data.getAnnotations event
@@ -333,18 +328,7 @@ class Annotator.Guest extends Annotator
         @element.removeClass markerClass
 
   addComment: ->
-    sel = @selectedTargets   # Save the selection
-    # Nuke the selection, since we won't be using that.
-    # We will attach this to the end of the document.
-    # Our override for setupAnnotation will add that highlight.
-    @selectedTargets = []
-    this.onAdderClick()     # Open editor (with 0 targets)
-    @selectedTargets = sel # restore the selection
-
-  # Is this annotation a comment?
-  isComment: (annotation) ->
-    # No targets and no references means that this is a comment.
-    not (annotation.inject or annotation.references?.length or annotation.target?.length)
+    this.showEditor(this.createAnnotation())
 
   # Open the sidebar
   showFrame: ->
@@ -359,10 +343,12 @@ class Annotator.Guest extends Annotator
       method: 'addToken'
       params: token
 
+  onAdderMousedown: angular.noop
+
   onAdderClick: (event) =>
-    event?.preventDefault()
+    event.preventDefault()
+    event.stopPropagation()
     @adder.hide()
-    @inAdderClick = false
     annotation = this.setupAnnotation(this.createAnnotation())
     this.showEditor(annotation)
 

h/static/scripts/services.coffee

@@ -21,6 +21,7 @@ renderFactory = ['$$rAF', ($$rAF) ->
 
 class Hypothesis extends Annotator
   events:
+    'beforeAnnotationCreated': 'digest'
     'annotationCreated': 'digest'
     'annotationDeleted': 'digest'
     'annotationUpdated': 'digest'
@@ -30,34 +31,6 @@ class Hypothesis extends Annotator
   options:
     noDocAccess: true
     Discovery: {}
-    Permissions:
-      userAuthorize: (action, annotation, user) ->
-        if annotation.permissions
-          tokens = annotation.permissions[action] || []
-
-          if tokens.length == 0
-            # Empty or missing tokens array: only admin can perform action.
-            return false
-
-          for token in tokens
-            if this.userId(user) == token
-              return true
-            if token == 'group:__world__'
-              return true
-            if token == 'group:__authenticated__' and this.user?
-              return true
-
-          # No tokens matched: action should not be performed.
-          return false
-
-        # Coarse-grained authorization
-        else if annotation.user
-          return user and this.userId(user) == this.userId(annotation.user)
-
-        # No authorization info on annotation: free-for-all!
-        true
-      showEditPermissionsCheckbox: false,
-      showViewPermissionsCheckbox: false,
     Threading: {}
 
   # Internal state
@@ -69,8 +42,10 @@ class Hypothesis extends Annotator
 
   # Here as a noop just to make the Permissions plugin happy
   # XXX: Change me when Annotator stops assuming things about viewers
+  editor:
+    addField: angular.noop
   viewer:
-    addField: (-> )
+    addField: angular.noop
 
   this.$inject = ['$document', '$window']
   constructor:   ( $document,   $window ) ->
@@ -146,27 +121,6 @@ class Hypothesis extends Annotator
       unless annotation.highlights?
         annotation.highlights = []
 
-      # Register it with the draft service, except when it's an injection
-       # This is an injection. Delete the marker.
-      if annotation.inject
-        # Set permissions for private
-        permissions = @plugins.Permissions
-        userId = permissions.options.userId permissions.user
-        annotation.permissions =
-          read: [userId]
-          admin: [userId]
-          update: [userId]
-          delete: [userId]
-
-    # Set default owner permissions on all annotations
-    for event in ['beforeAnnotationCreated', 'beforeAnnotationUpdated']
-      this.subscribe event, (annotation) =>
-        permissions = @plugins.Permissions
-        if permissions.user?
-          userId = permissions.options.userId(permissions.user)
-          for action, roles of annotation.permissions
-            unless userId in roles then roles.push userId
-
   _setupXDM: (options) ->
     # jschannel chokes FF and Chrome extension origins.
     if (options.origin.match /^chrome-extension:\/\//) or
@@ -187,9 +141,7 @@ class Hypothesis extends Annotator
 
     .bind('back', =>
       # Navigate "back" out of the interface.
-      @element.scope().$apply =>
-        return if @element.scope().ongoingEdit
-        this.hide()
+      @element.scope().$apply => this.hide()
     )
 
     .bind('open', =>
@@ -294,17 +246,15 @@ class Hypothesis extends Annotator
     this
 
   showEditor: (annotation) ->
-    scope = @element.scope()
-    scope.ongoingEdit = mail.messageContainer(annotation)
-    delete scope.selectedAnnotations
+    delete @element.scope().selectedAnnotations
     this.show()
     this
 
   show: ->
-    @element.scope().frame.visible = true
+    @host.notify method: 'showFrame'
 
   hide: ->
-    @element.scope().frame.visible = false
+    @host.notify method: 'hideFrame'
 
   digest: ->
     @element.scope().$evalAsync angular.noop
@@ -375,18 +325,8 @@ class Hypothesis extends Annotator
 
   setTool: (name) ->
     return if name is @tool
-    return unless @element.injector().get('drafts').discard()
 
     if name is 'highlight'
-      # Check login state first
-      unless @plugins.Permissions?.user
-        scope = @element.scope()
-        # If we are not logged in, start the auth process
-        scope.ongoingHighlightSwitch = true
-        @element.injector().get('identity').request()
-        this.show()
-        return
-
       this.socialView.name = 'single-player'
     else
       this.socialView.name = 'none'
@@ -409,45 +349,47 @@ class Hypothesis extends Annotator
 
 
 class DraftProvider
-  drafts: null
+  _drafts: null
 
   constructor: ->
-    this.drafts = []
+    this._drafts = []
 
   $get: -> this
 
-  add: (draft, cb) -> @drafts.push {draft, cb}
+  all: -> draft for {draft} in @_drafts
+
+  add: (draft, cb) -> @_drafts.push {draft, cb}
 
   remove: (draft) ->
     remove = []
-    for d, i in @drafts
+    for d, i in @_drafts
       remove.push i if d.draft is draft
     while remove.length
-      @drafts.splice(remove.pop(), 1)
+      @_drafts.splice(remove.pop(), 1)
 
   contains: (draft) ->
-    for d in @drafts
+    for d in @_drafts
       if d.draft is draft then return true
     return false
 
-  isEmpty: -> @drafts.length is 0
+  isEmpty: -> @_drafts.length is 0
 
   discard: ->
     text =
-      switch @drafts.length
+      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.
+          """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 = []
+    if @_drafts.length is 0 or confirm text
+      discarded = @_drafts.slice()
+      @_drafts = []
       d.cb?() for d in discarded
       true
     else

h/static/styles/annotations.scss

@@ -9,10 +9,6 @@
     color: $link-color;
   }
 
-  .magicontrol.dropdown {
-    top: 4px;
-  }
-
   fuzzytime {
     line-height: 2;
 
@@ -35,7 +31,7 @@
 }
 
 .annotation-actions {
-  margin-top: 4px;
+  margin-top: .4em;
 }
 
 .annotation-quote {

tests/js/directives/annotation-test.coffee

@@ -4,6 +4,7 @@ describe 'h.directives.annotation', ->
   $scope = null
   annotation = null
   createController = null
+  flash = null
 
   beforeEach module('h.directives')
 
@@ -15,11 +16,13 @@ describe 'h.directives.annotation', ->
         title: 'A special document'
       target: [{}]
       uri: 'http://example.com'
+    flash = sinon.spy()
 
     createController = ->
       $controller 'AnnotationController',
         $scope: $scope
         annotator: {plugins: {}, publish: sinon.spy()}
+        flash: flash
 
   it 'provides a document title', ->
     controller = createController()

tests/js/directives/thread-test.coffee

@@ -7,6 +7,7 @@ describe 'h.directives.thread', ->
   $element = null
   container = null
   createController = null
+  flash = null
 
   beforeEach module('h.directives')
 
@@ -16,13 +17,14 @@ describe 'h.directives.thread', ->
       $removeClass: sinon.spy()
       thread: 'thread'
     $scope = $rootScope.$new()
+    flash = sinon.spy()
     render = (value, cb) -> cb(value)
 
     createController = ->
       $scope.thread = mail.messageContainer()
       $scope.thread.message = id: 'foo', uri: 'http://example.com/'
       $element = angular.element('<div thread="thread"><input /></div>')
-      $controller 'ThreadController', {$attrs, $element, $scope, render}
+      $controller 'ThreadController', {$attrs, $element, $scope, flash, render}
 
   afterEach ->
     sandbox.restore()