Refactor saving highlights in AnnotationController

[seanh]

This started out as me trying to do a small fix to AnnotationController, I just wanted to move the line of code that automatically saves new highlights to the server on login out of a $watch() function and to remove some duplicated state (highlight, model.$highlight, vm.isHighlight(), vm.hasContent() ...). But it ended up being a can of worms.

The changes I've ended up with are mostly just refactoring the code for clarity but this does also fix at least one broken behaviour (creating highlights while logged out, then logging in). See list of use-cases below.

Code changes:

• Rename local variable highlight to newlyCreatedByHighlightButton and add
  a docstring - clarify what this piece of state actually means.

  Yes I know this is a really long variable name but this state variable has a very particular meaning and is only used in a couple of places - I think the explicitness is justified here for clarity.

• Fix vm.isHighlight(): It now returns false for new annotations that the
  user hasn't entered any text or tags for yet (instead of true).

• Move the code for saving new highlights to the server into a
  saveNewHighlight() function that's called on AnnotationController
  instantiation instead of having it in a $watch() function that's called
  every time the local variable model changes.

  Also use the presence of model.id to avoid re-saving highlights that have
  already been saved - instead of abusing the highlight
  (now newlyCreatedByHighlightButton) variable and confusing what it
  represents.

• Don't automatically open the annotation editor on Annotationcontroller
  instantiation if the annotation is a highlight. This fixes this bug:

  1. Create a new highlight while logged out
  2. Log in
  3. Your highlight is saved to the server, but also the annotation editor is
     open on your highlight.

  The intention is that highlights are simply saved to the server, not opened
  for editing.

  Annotations created while logged out, on the other hand, are not saved
  to the server on login but are opened for editing on login.

Cases to test (these are all things that are either broken on master, or were broken at one point by the earlier versions of the code changes in this pull request:

• During all cases, make sure that bucket bar numbers are correct
• Create new highlights while logged out, then login. The intended behaviour is that your highlights are silently saved to the server on login. They aren't double saved, their annotation editors aren't opened.
• Create new highlights while logged in. Intended behaviour: highlight is created and saved to server straight away when highlight button is clicked. Annotation editor is never opened.
• Load highlights from the server: reload the page and/or move between groups and/or logout and in again. Highlights should reappear as they were. Should not be re-saved again creating more highlights.
• Create new annotations while logged out, then login. The intended behaviour is that your annotations are not automatically saved to the server on login, and their annotation editors are opened.
• Create new annotations while logged in. The intended behaviour is that your annotations are not automatically saved to the server, and their annotation editors are opened.
• Load annotations from the server: reload the page and/or move between groups and/or logout and in again.
• Create a new annotation but do not save it, change to another group, change back to original group. Intended behaviour is that the annotation moves with you between groups and any privacy, text or tag edits are preserved.
• Edit an already saved annotation but do not save it, change to another group, change back to original group. The annotation does not go with you, but when you return to the group it is still there with its editor open and any edits preserved.
• This one is still broken (but is broken on master too): edit an already saved annotation and set its text to empty, change to another group, change back to original group - annotations original text has re-appeared. It should be empty.
• Create a highlight while logged in. Edit the highlight. Do not type any text or tags yet and do not save it. Move to another group. Move back. Highlight should still be there with its editor opened. Now enter some text, move to another group and back again - edits should be preserved.

P.S. In my opinion most of the complexity here is caused by using one directive / controller for many things (annotations, highlights, replies, page notes).

[seanh]

Needs tests, and once I have those I can do a couple more little refactors on top of this

[seanh]

Needs tests, and once I have those I can do a couple more little refactors on top of this

[seanh]

Ready for review

[seanh]

Ready for review

[judell]

"But it ended up being a can of worms." No good deed goes unpunished.

[judell]

"But it ended up being a can of worms." No good deed goes unpunished.

[nickstenning]

So I don't think this should block merging this PR, but this smells really awful to me. AnnotationController shouldn't be responsible for firing these lifecycle events. Indeed, most of the rest of them have moved into annotationMapper, which makes me wonder why AnnotationController isn't using annotationMapper.createAnnotation(object) rather than ngResource.$create...

[robertknight]

Indeed, we definitely don't want to have multiple places handling the actual creation of the object on the server if possible.

[seanh]

Agreed, let's remember to do this one later

[robertknight]

I'd suggest we call these "existing" rather than "old" for consistency with comments elsewhere

[robertknight]

So overall:

1. This is looking much clearer, the comments clarify things especially
2. I'd agree with Nick's comments about annotationMapper - it would be preferable to have the submission of annotations to the server only happen in one place
3. There is a lot of duplication in the tests (eg. in terms of repeated comments explaining the same thing about the consequences of setting different attributes on a new annotation) which is going to make refactoring harder in future. Extracting these into a helper could simplify that.

eg:

it('...', function () {
  var annotation = createAnnotation({
    // properties customizing the new annotation, everything
    // else is set to a default
  });
});

[robertknight]

So overall:

1. This is looking much clearer, the comments clarify things especially
2. I'd agree with Nick's comments about annotationMapper - it would be preferable to have the submission of annotations to the server only happen in one place
3. There is a lot of duplication in the tests (eg. in terms of repeated comments explaining the same thing about the consequences of setting different attributes on a new annotation) which is going to make refactoring harder in future. Extracting these into a helper could simplify that.

eg:

it('...', function () {
  var annotation = createAnnotation({
    // properties customizing the new annotation, everything
    // else is set to a default
  });
});

[seanh]

I've factored out the annotation global from the tests to pave the way for removing duplication by adding helper functions for creating different kinds of annotation. Going to lunch now, will finish after..

[seanh]

I've factored out the annotation global from the tests to pave the way for removing duplication by adding helper functions for creating different kinds of annotation. Going to lunch now, will finish after..

[seanh]

@robertknight As long as you're happy that I ignored the old -> existing comment (I can't really be bothered to go through both files and change old -> existing everywhere unless you really want it), I think that's everything responded to

[seanh]

@robertknight As long as you're happy that I ignored the old -> existing comment (I can't really be bothered to go through both files and change old -> existing everywhere unless you really want it), I think that's everything responded to