Update the displayed annotation timestamp immediately after creation/update

There were two problems here:

 1. The periodic interval for updating the timestamp was never started
    for new annotations because the initial call to updateTimestamp()
    was a no-op for annotations that had no timestamp yet.

 2. domainModel.updated was never updated after an annotation
    was saved to the server and hence the call to
    updateTimestamp() re-used the old timestamp.

(1) is fixed now by restarting the interval using the current
last-updated timestamp whenever the view model is updated.

(2) is fixed indirectly now by moving the logic to update
the timestamp into updateViewModel(), so that all updates
to it happen in one place. When an annotation is updated,
the updated domain model returned by the server is the one
passed into updateViewModel() to update it.

 * Separate out the logic for creating a decaying interval
   whose frequency depends on the age of some input timestamp,
   and updating the timestamp string on each interval.

   The decaying interval is now handled by 'time.decayingInterval'

 * Trigger a restart of the interval whenever the view model is
   updated in response to a change to the annotation

Fixes #2819
Fixes #2822

h/static/scripts/directive/annotation.js

@@ -166,7 +166,8 @@ function updateDomainModel(domainModel, vm, permissions, groups) {
 }
 
 /** Update the view model from the domain model changes. */
-function updateViewModel(domainModel, vm, permissions) {
+function updateViewModel($scope, time, domainModel, vm, permissions) {
+
   vm.form = {
     text: domainModel.text,
     tags: viewModelTagsFromDomainModelTags(domainModel.tags),
@@ -174,6 +175,21 @@ function updateViewModel(domainModel, vm, permissions) {
   vm.annotationURI = new URL('/a/' + domainModel.id, vm.baseURI).href;
   vm.isPrivate = permissions.isPrivate(
     domainModel.permissions, domainModel.user);
+
+  function updateTimestamp() {
+    vm.timestamp = time.toFuzzyString(domainModel.updated);
+  }
+
+  if (domainModel.updated) {
+    if (vm.cancelTimestampRefresh) {
+      vm.cancelTimestampRefresh();
+    }
+    vm.cancelTimestampRefresh =
+     time.decayingInterval(domainModel.updated, function () {
+       $scope.$apply(updateTimestamp);
+     });
+    updateTimestamp();
+  }
 }
 
 /** Return truthy if the given annotation is valid, falsy otherwise.
@@ -282,6 +298,9 @@ function AnnotationController(
      */
     vm.timestamp = null;
 
+    /** A callback for resetting the automatic refresh of vm.timestamp */
+    vm.cancelTimestampRefresh = undefined;
+
     /** The domain model, contains the currently saved version of the
       * annotation from the server (or in the case of new annotations that
       * haven't been saved yet - the data that will be saved to the server when
@@ -330,8 +349,7 @@ function AnnotationController(
     // log in.
     saveNewHighlight();
 
-    updateTimestamp(true);
-    updateViewModel(domainModel, vm, permissions);
+    updateViewModel($scope, time, domainModel, vm, permissions);
 
     // If this annotation is not a highlight and if it's new (has just been
     // created by the annotate button) or it has edits not yet saved to the
@@ -345,12 +363,14 @@ function AnnotationController(
 
   function onAnnotationUpdated(event, updatedDomainModel) {
     if (updatedDomainModel.id === domainModel.id) {
-      updateViewModel(updatedDomainModel, vm, permissions);
+      updateViewModel($scope, time, updatedDomainModel, vm, permissions);
     }
   }
 
   function onDestroy() {
-    updateTimestamp = angular.noop;
+    if (vm.cancelTimestampRefresh) {
+      vm.cancelTimestampRefresh();
+    }
   }
 
   function onGroupFocused() {
@@ -404,32 +424,6 @@ function AnnotationController(
     }
   }
 
-  // We use `var foo = function() {...}` here instead of `function foo() {...}`
-  // because updateTimestamp gets redefined later on.
-  var updateTimestamp = function(repeat) {
-    repeat = repeat || false;
-
-    // New (not yet saved to the server) annotations don't have any .updated
-    // yet, so we can't update their timestamp.
-    if (!domainModel.updated) {
-      return;
-    }
-
-    vm.timestamp = time.toFuzzyString(domainModel.updated);
-
-    if (!repeat) {
-      return;
-    }
-
-    var fuzzyUpdate = time.nextFuzzyUpdate(domainModel.updated);
-    var nextUpdate = (1000 * fuzzyUpdate) + 500;
-
-    $timeout(function() {
-      updateTimestamp(true);
-      $scope.$digest();
-    }, nextUpdate, false);
-  };
-
   /** Switches the view to a viewer, closing the editor controls if they're
    *  open.
     * @name annotation.AnnotationController#view
@@ -622,7 +616,7 @@ function AnnotationController(
     if (vm.action === 'create') {
       $rootScope.$emit('annotationDeleted', domainModel);
     } else {
-      updateViewModel(domainModel, vm, permissions);
+      updateViewModel($scope, time, domainModel, vm, permissions);
       view();
     }
   };
@@ -654,7 +648,7 @@ function AnnotationController(
 
         var onFulfilled = function() {
           $rootScope.$emit('annotationCreated', domainModel);
-          updateViewModel(domainModel, vm, permissions);
+          updateViewModel($scope, time, domainModel, vm, permissions);
           view();
           drafts.remove(domainModel);
         };

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

@@ -24,7 +24,7 @@ function documentService() {
   return $document;
 }
 
-describe('annotation.js', function() {
+describe('annotation', function() {
 
   describe('extractDocumentMetadata()', function() {
     var extractDocumentMetadata = require('../annotation')
@@ -570,7 +570,7 @@ describe('annotation.js', function() {
 
       fakeTime = {
         toFuzzyString: sandbox.stub().returns('a while ago'),
-        nextFuzzyUpdate: sandbox.stub().returns(30)
+        decayingInterval: function () {},
       };
 
       fakeUrlEncodeFilter = function(v) {
@@ -1080,6 +1080,52 @@ describe('annotation.js', function() {
         assert.equal(controller.timestamp, null);
       });
 
+      it('is updated when a new annotation is saved', function () {
+        fakeTime.decayingInterval = function (date, callback) {
+          callback();
+        };
+
+        // fake clocks are not required for this test
+        clock.restore();
+
+        annotation.updated = null;
+        annotation.$create = function () {
+          annotation.updated = (new Date).toString();
+          return Promise.resolve(annotation);
+        };
+        var controller = createDirective(annotation).controller;
+        controller.action = 'create';
+        return controller.save().then(function () {
+          assert.equal(controller.timestamp, 'a while ago');
+        });
+      });
+
+      it('is updated when a change to an existing annotation is saved',
+       function () {
+        fakeTime.toFuzzyString = function(date) {
+          var ONE_MINUTE = 60 * 1000;
+          if (Date.now() - new Date(date) < ONE_MINUTE) {
+            return 'just now';
+          } else {
+            return 'ages ago';
+          }
+        };
+
+        clock.tick(10 * 60 * 1000);
+
+        annotation.$update = function () {
+          this.updated = (new Date).toString();
+          return Promise.resolve(this);
+        }
+        var controller = createDirective(annotation).controller;
+        assert.equal(controller.timestamp, 'ages ago');
+        controller.edit();
+        clock.restore();
+        return controller.save().then(function () {
+          assert.equal(controller.timestamp, 'just now');
+        });
+      });
+
       it('is updated on first digest', function() {
         var controller = createDirective(annotation).controller;
         $scope.$digest();
@@ -1087,12 +1133,13 @@ describe('annotation.js', function() {
       });
 
       it('is updated after a timeout', function() {
+        fakeTime.decayingInterval = function (date, callback) {
+          setTimeout(callback, 10);
+        };
         var controller = createDirective(annotation).controller;
-        fakeTime.nextFuzzyUpdate.returns(10);
         fakeTime.toFuzzyString.returns('ages ago');
         $scope.$digest();
         clock.tick(11000);
-        $timeout.flush();
         assert.equal(controller.timestamp, 'ages ago');
       });
 
@@ -1521,7 +1568,7 @@ describe('annotation.js', function() {
         },
         time: args.time || {
           toFuzzyString: function() {},
-          nextFuzzyUpdate: function() {}
+          decayingInterval: function() {}
         },
         annotationUI: args.annotationUI || {},
         annotationMapper: args.annotationMapper || {},

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

@@ -6,7 +6,6 @@ day = hour * 24
 month = day * 30
 year = day * 365
 
-
 FIXTURES_TO_FUZZY_STRING = [
   [10, 'moments ago']
   [29, 'moments ago']
@@ -68,6 +67,37 @@ describe 'time', ->
     for f, i in FIXTURES_TO_FUZZY_STRING
       it "creates correct fuzzy string for fixture #{i}", testFixture(f)
 
+  describe '.decayingInterval', ->
+    it 'invokes the callback quickly for recent timestamps', ->
+      date = new Date()
+      callback = sandbox.stub()
+      time.decayingInterval(date, callback)
+      sandbox.clock.tick(6 * 1000)
+      assert.calledWith(callback, date)
+      sandbox.clock.tick(6 * 1000)
+      assert.calledTwice(callback)
+
+    it 'invokes the callback after a longer delay for older timestamps', ->
+      date = new Date();
+      ONE_MINUTE = minute * 1000
+      sandbox.clock.tick(10 * ONE_MINUTE)
+      callback = sandbox.stub()
+      time.decayingInterval(date, callback)
+      sandbox.clock.tick(ONE_MINUTE / 2)
+      assert.notCalled(callback)
+      sandbox.clock.tick(ONE_MINUTE)
+      assert.calledWith(callback, date)
+      sandbox.clock.tick(ONE_MINUTE)
+      assert.calledTwice(callback)
+
+    it 'returned function cancels the timer', ->
+      date = new Date()
+      callback = sandbox.stub()
+      cancel = time.decayingInterval(date, callback)
+      cancel()
+      sandbox.clock.tick(minute * 1000)
+      assert.notCalled(callback)
+
   describe '.nextFuzzyUpdate', ->
     it 'Handles empty dates', ->
       t = null

h/static/scripts/time.coffee

@@ -36,8 +36,40 @@ getBreakpoint = (date) ->
   delta: delta
   breakpoint: arrayFind(BREAKPOINTS, (x) -> x[0] > delta)
 
+nextFuzzyUpdate = (date) ->
+  return null if not date
+  {_, breakpoint} = getBreakpoint(date)
+  return null unless breakpoint
+  secs = breakpoint[2]
+
+  # We don't want to refresh anything more often than 5 seconds
+  secs = Math.max secs, 5
+
+  # setTimeout limit is MAX_INT32=(2^31-1) (in ms),
+  # which is about 24.8 days. So we don't set up any timeouts
+  # longer than 24 days, that is, 2073600 seconds.
+  secs = Math.min secs, 2073600
 
 module.exports = ->
+  # Starts an interval whose frequency decays depending on the relative
+  # age of 'date'. This can be used to refresh parts of a UI whose
+  # update frequency depends on the age of a timestamp.
+  #
+  # Returns a function that cancels the automatic refresh.
+  decayingInterval: (date, callback) ->
+    timer = undefined
+    update = ->
+      fuzzyUpdate = nextFuzzyUpdate(date)
+      nextUpdate = (1000 * fuzzyUpdate) + 500
+      timer = setTimeout(->
+        callback(date)
+        update()
+      , nextUpdate)
+    update()
+
+    return ->
+      clearTimeout(timer)
+
   toFuzzyString: (date) ->
     return '' unless date
     {delta, breakpoint} = getBreakpoint(date)
@@ -46,16 +78,4 @@ module.exports = ->
     resolution = breakpoint[2]
     return template.replace('{}', String(Math.floor(delta / resolution)))
 
-  nextFuzzyUpdate: (date) ->
-    return null if not date
-    {_, breakpoint} = getBreakpoint(date)
-    return null unless breakpoint
-    secs = breakpoint[2]
-
-    # We don't want to refresh anything more often than 5 seconds
-    secs = Math.max secs, 5
-
-    # setTimeout limit is MAX_INT32=(2^31-1) (in ms),
-    # which is about 24.8 days. So we don't set up any timeouts
-    # longer than 24 days, that is, 2073600 seconds.
-    secs = Math.min secs, 2073600
+  nextFuzzyUpdate: nextFuzzyUpdate