Add `autosaveService`; save new highlights automatically

Add a new `autosaveService`, which subscribes to the store and
automatically saves new highlights. Remove implementation of highlight
auto-save from `Annotation` controller.

.eslintrc

 {
-  "extends": [
-    "hypothesis",
-    "plugin:jsx-a11y/recommended"
-  ],
+  "extends": ["hypothesis", "plugin:jsx-a11y/recommended"],
+  "env": {
+    "es6": true
+  },
   "globals": {
     "Set": false
   },

src/shared/injector.js

-/* global Map */
-
 /**
  * @typedef Provider
  * @prop {any} [value] - The value for the object

src/sidebar/components/annotation.js

@@ -22,7 +22,6 @@ export function updateModel(annotation, changes, permissions) {
 function AnnotationController(
   $document,
   $rootScope,
-  $scope,
   $timeout,
   $window,
   store,
@@ -84,13 +83,6 @@ function AnnotationController(
      */
     newlyCreatedByHighlightButton = self.annotation.$highlight || false;
 
-    // Automatically save new highlights to the server when they're created.
-    // Note that this line also gets called when the user logs in (since
-    // AnnotationController instances are re-created on login) so serves to
-    // automatically save highlights that were created while logged out when you
-    // log in.
-    saveNewHighlight();
-
     // 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
     // server - then open the editor on AnnotationController instantiation.
@@ -101,45 +93,6 @@ function AnnotationController(
     }
   };
 
-  /** Save this annotation if it's a new highlight.
-   *
-   * The highlight will be saved to the server if the user is logged in,
-   * saved to drafts if they aren't.
-   *
-   * If the annotation is not new (it has already been saved to the server) or
-   * is not a highlight then nothing will happen.
-   *
-   */
-  function saveNewHighlight() {
-    if (!isNew(self.annotation)) {
-      // Already saved.
-      return;
-    }
-
-    if (!self.annotation.user) {
-      // Open sidebar to display error message about needing to login to create highlights.
-      bridge.call('showSidebar');
-    }
-
-    if (!self.isHighlight()) {
-      // Not a highlight,
-      return;
-    }
-
-    if (self.annotation.user) {
-      // User is logged in, save to server.
-      // Highlights are always private.
-      self.annotation.permissions = permissions.private(self.annotation.user);
-      save(self.annotation).then(function(model) {
-        model.$tag = self.annotation.$tag;
-        $rootScope.$broadcast(events.ANNOTATION_CREATED, model);
-      });
-    } else {
-      // User isn't logged in, save to drafts.
-      store.createDraft(self.annotation, self.state());
-    }
-  }
-
   /**
    * @ngdoc method
    * @name annotation.AnnotationController#edit

src/sidebar/components/test/annotation-test.js

@@ -72,7 +72,6 @@ describe('annotation', function() {
 
   describe('AnnotationController', function() {
     let $rootScope;
-    let $scope;
     const fakeAccountID = {
       isThirdPartyUser: sinon.stub(),
     };
@@ -230,7 +229,6 @@ describe('annotation', function() {
 
     beforeEach(inject(function(_$rootScope_) {
       $rootScope = _$rootScope_;
-      $scope = $rootScope.$new();
     }));
 
     afterEach(function() {
@@ -238,62 +236,6 @@ describe('annotation', function() {
     });
 
     describe('initialization', function() {
-      it('saves new highlights to the server on initialization', function() {
-        const annotation = fixtures.newHighlight();
-        // The user is logged-in.
-        annotation.user = fakeSession.state.userid = 'acct:bill@localhost';
-        createDirective(annotation);
-
-        assert.called(fakeApi.annotation.create);
-      });
-
-      it('saves new highlights to drafts if not logged in', function() {
-        const annotation = fixtures.newHighlight();
-        // The user is not logged-in.
-        annotation.user = fakeSession.state.userid = undefined;
-
-        createDirective(annotation);
-
-        assert.notCalled(fakeApi.annotation.create);
-        assert.called(fakeStore.createDraft);
-      });
-
-      it('opens the sidebar when trying to save highlights while logged out', () => {
-        // The sidebar is opened in order to draw the user's attention to
-        // the `You must be logged in to create annotations and highlights` message.
-        const annotation = fixtures.newHighlight();
-        // The user is not logged-in.
-        annotation.user = fakeSession.state.userid = undefined;
-
-        createDirective(annotation);
-
-        assert.calledWith(fakeBridge.call, 'showSidebar');
-      });
-
-      it('does not save new annotations on initialization', function() {
-        const annotation = fixtures.newAnnotation();
-
-        createDirective(annotation);
-
-        assert.notCalled(fakeApi.annotation.create);
-      });
-
-      it('does not save old highlights on initialization', function() {
-        const annotation = fixtures.oldHighlight();
-
-        createDirective(annotation);
-
-        assert.notCalled(fakeApi.annotation.create);
-      });
-
-      it('does not save old annotations on initialization', function() {
-        const annotation = fixtures.oldAnnotation();
-
-        createDirective(annotation);
-
-        assert.notCalled(fakeApi.annotation.create);
-      });
-
       it('creates drafts for new annotations on initialization', function() {
         const annotation = fixtures.newAnnotation();
         createDirective(annotation);
@@ -304,14 +246,6 @@ describe('annotation', function() {
         });
       });
 
-      it('does not create drafts for new highlights on initialization', function() {
-        const annotation = fixtures.newHighlight();
-        const controller = createDirective(annotation).controller;
-
-        assert.notOk(controller.editing());
-        assert.notCalled(fakeStore.createDraft);
-      });
-
       it('edits annotations with drafts on initialization', function() {
         const annotation = fixtures.oldAnnotation();
         // The drafts store has some draft changes for this annotation.
@@ -419,24 +353,6 @@ describe('annotation', function() {
       });
     });
 
-    describe('when the annotation is a highlight', function() {
-      let annotation;
-
-      beforeEach(function() {
-        annotation = fixtures.defaultAnnotation();
-        annotation.$highlight = true;
-      });
-
-      it('is private', function() {
-        delete annotation.id;
-        createDirective(annotation);
-        $scope.$digest();
-        assert.deepEqual(annotation.permissions, {
-          read: ['justme'],
-        });
-      });
-    });
-
     describe('#reply', function() {
       let annotation;
 

src/sidebar/index.js

@@ -123,6 +123,14 @@ function persistDefaults(persistedDefaults) {
   persistedDefaults.init();
 }
 
+/**
+ * Set up autosave-new-highlights service
+ */
+// @ngInject
+function autosave(autosaveService) {
+  autosaveService.init();
+}
+
 // Preact UI components that are wrapped for use within Angular templates.
 
 import AnnotationActionBar from './components/annotation-action-bar';
@@ -173,12 +181,13 @@ import annotationMapperService from './services/annotation-mapper';
 import annotationsService from './services/annotations';
 import apiService from './services/api';
 import apiRoutesService from './services/api-routes';
+import authService from './services/oauth-auth';
+import autosaveService from './services/autosave';
 import featuresService from './services/features';
 import flashService from './services/flash';
 import frameSyncService from './services/frame-sync';
 import groupsService from './services/groups';
 import localStorageService from './services/local-storage';
-import authService from './services/oauth-auth';
 import permissionsService from './services/permissions';
 import persistedDefaultsService from './services/persisted-defaults';
 import rootThreadService from './services/root-thread';
@@ -214,6 +223,7 @@ function startAngularApp(config) {
     .register('api', apiService)
     .register('apiRoutes', apiRoutesService)
     .register('auth', authService)
+    .register('autosaveService', autosaveService)
     .register('bridge', bridgeService)
     .register('features', featuresService)
     .register('flash', flashService)
@@ -303,6 +313,7 @@ function startAngularApp(config) {
     .service('annotationsService', () => container.get('annotationsService'))
     .service('api', () => container.get('api'))
     .service('auth', () => container.get('auth'))
+    .service('autosaveService', () => container.get('autosaveService'))
     .service('bridge', () => container.get('bridge'))
     .service('features', () => container.get('features'))
     .service('flash', () => container.get('flash'))
@@ -332,6 +343,7 @@ function startAngularApp(config) {
     .run(registerAngularServices)
 
     .run(persistDefaults)
+    .run(autosave)
     .run(sendPageView)
     .run(setupApi)
     .run(crossOriginRPC.server.start);

src/sidebar/services/autosave.js

+/**
+ * A service for automatically saving new highlights.
+ */
+import { retryPromiseOperation } from '../util/retry';
+
+// @ngInject
+export default function autosaveService(annotationsService, store) {
+  // A Set of annotation $tags that have save requests in-flight
+  const saving = new Set();
+
+  // A Set of annotation $tags that have failed to save after retries
+  const failed = new Set();
+
+  /**
+   * Determine whether we should try to send a save request for the highlight
+   * indicated by `htag`
+   *
+   * @param {string} htag - The local unique identifier for the unsaved highlight
+   * @return {boolean}
+   */
+  const shouldSaveHighlight = htag => {
+    return !saving.has(htag) && !failed.has(htag);
+  };
+
+  /**
+   * Store-subscribed call back. Automatically save new highlights.
+   */
+  const autosaveNewHighlights = () => {
+    const newHighlights = store.newHighlights();
+
+    newHighlights.forEach(highlight => {
+      // Because this is a new annotation object, it does not yet have an `id`
+      // property. Use the local `$tag` for uniqueness instead.
+      const htag = highlight.$tag;
+
+      if (!shouldSaveHighlight(htag)) {
+        return;
+      }
+
+      saving.add(htag);
+
+      retryPromiseOperation(() => annotationsService.save(highlight))
+        .catch(() => {
+          // save failed after retries
+          failed.add(htag);
+        })
+        .finally(() => {
+          // Request is complete, no longer attempting to save
+          saving.delete(htag);
+        });
+    });
+  };
+
+  return {
+    init() {
+      store.subscribe(autosaveNewHighlights);
+    },
+    isSaving() {
+      return saving.size > 0;
+    },
+  };
+}

src/sidebar/services/test/autosave-test.js

+import * as annotationFixtures from '../../test/annotation-fixtures';
+import createFakeStore from '../../test/fake-redux-store';
+import { waitFor } from '../../../test-util/wait';
+
+import autosaveService, { $imports } from '../autosave';
+
+describe('autosaveService', () => {
+  let fakeAnnotationsService;
+  let fakeNewHighlights;
+  let fakeRetryPromiseOperation;
+  let fakeStore;
+
+  beforeEach(() => {
+    fakeAnnotationsService = { save: sinon.stub().resolves() };
+    fakeNewHighlights = sinon.stub().returns([]);
+    fakeRetryPromiseOperation = sinon.stub().callsFake(callback => callback());
+    fakeStore = createFakeStore({}, { newHighlights: fakeNewHighlights });
+
+    $imports.$mock({
+      '../util/retry': {
+        retryPromiseOperation: fakeRetryPromiseOperation,
+      },
+    });
+  });
+
+  /**
+   * Make `fakeStore.newHighlights` return a single highlight fixture.
+   */
+  const oneNewHighlight = () => {
+    const newHighlight = annotationFixtures.newHighlight();
+    newHighlight.$tag = 'deadbeef';
+    fakeStore.newHighlights.returns([newHighlight]);
+    return newHighlight;
+  };
+
+  afterEach(() => {
+    $imports.$restore();
+  });
+
+  it('should subscribe to store updates and check for new highlights', () => {
+    const svc = autosaveService(fakeAnnotationsService, fakeStore);
+    svc.init();
+
+    fakeStore.setState({});
+
+    assert.calledOnce(fakeStore.newHighlights);
+  });
+
+  it('should save new highlights', () => {
+    const svc = autosaveService(fakeAnnotationsService, fakeStore);
+    svc.init();
+    const newHighlight = oneNewHighlight();
+
+    fakeStore.setState({});
+
+    assert.calledOnce(fakeAnnotationsService.save);
+    assert.calledWith(fakeAnnotationsService.save, newHighlight);
+  });
+
+  it('should not try to save a highlight that is already being saved', () => {
+    const svc = autosaveService(fakeAnnotationsService, fakeStore);
+    svc.init();
+
+    oneNewHighlight();
+
+    fakeStore.setState({});
+    assert.isTrue(svc.isSaving());
+
+    fakeStore.setState({});
+    assert.calledOnce(fakeAnnotationsService.save);
+  });
+
+  it('should not retry a highlight that failed to save', async () => {
+    fakeAnnotationsService.save.rejects(new Error('Something went wrong'));
+    const svc = autosaveService(fakeAnnotationsService, fakeStore);
+    svc.init();
+
+    oneNewHighlight();
+
+    fakeStore.setState({});
+    assert.calledOnce(fakeAnnotationsService.save);
+
+    await waitFor(() => !svc.isSaving());
+
+    fakeAnnotationsService.save.resetHistory();
+
+    fakeStore.setState({});
+    assert.notCalled(fakeAnnotationsService.save);
+  });
+});

src/sidebar/store/modules/annotations.js

@@ -8,7 +8,7 @@ import { createSelector } from 'reselect';
 import uiConstants from '../../ui-constants';
 import * as metadata from '../../util/annotation-metadata';
 import * as arrayUtil from '../../util/array';
-import { defaultPermissions } from '../../util/permissions';
+import { defaultPermissions, privatePermissions } from '../../util/permissions';
 import * as util from '../util';
 
 import drafts from './drafts';
@@ -360,6 +360,11 @@ function createAnnotation(ann, now = new Date()) {
       },
       ann
     );
+    // Highlights are peculiar in that they always have private permissions
+    if (metadata.isHighlight(ann)) {
+      ann.permissions = privatePermissions(userid);
+    }
+
     // When a new annotation is created, remove any existing annotations
     // that are empty.
     dispatch(drafts.actions.deleteNewAndEmptyDrafts([ann]));
@@ -437,6 +442,16 @@ function findAnnotationByID(state, id) {
   return findByID(state.annotations.annotations, id);
 }
 
+/**
+ * Return all loaded annotations that are highlights and have not been saved
+ * to the server.
+ */
+const newHighlights = createSelector(
+  state => state.annotations.annotations,
+  annotations =>
+    annotations.filter(ann => metadata.isNew(ann) && metadata.isHighlight(ann))
+);
+
 /**
  * Return the number of page notes.
  */
@@ -485,13 +500,14 @@ export default {
   },
 
   selectors: {
-    annotationExists,
-    noteCount,
     annotationCount,
-    orphanCount,
-    isWaitingToAnchorAnnotations,
+    annotationExists,
     findAnnotationByID,
     findIDsForTags,
+    isWaitingToAnchorAnnotations,
+    newHighlights,
+    noteCount,
+    orphanCount,
     savedAnnotations,
   },
 };

src/sidebar/store/modules/test/annotations-test.js

@@ -25,11 +25,16 @@ function createTestStore() {
 
 describe('sidebar/store/modules/annotations', function() {
   let fakeDefaultPermissions;
+  let fakePrivatePermissions;
 
   beforeEach(() => {
     fakeDefaultPermissions = sinon.stub();
+    fakePrivatePermissions = sinon.stub();
     $imports.$mock({
-      '../../util/permissions': { defaultPermissions: fakeDefaultPermissions },
+      '../../util/permissions': {
+        defaultPermissions: fakeDefaultPermissions,
+        privatePermissions: fakePrivatePermissions,
+      },
     });
   });
 
@@ -227,6 +232,39 @@ describe('sidebar/store/modules/annotations', function() {
     });
   });
 
+  describe('newHighlights', () => {
+    [
+      {
+        annotations: [fixtures.oldAnnotation(), fixtures.newAnnotation()],
+        expectedLength: 0,
+      },
+      {
+        annotations: [
+          fixtures.oldAnnotation(),
+          Object.assign(fixtures.newHighlight(), { $tag: 'atag' }),
+          Object.assign(fixtures.newHighlight(), { $tag: 'anothertag' }),
+        ],
+        expectedLength: 2,
+      },
+      {
+        annotations: [
+          fixtures.oldHighlight(),
+          Object.assign(fixtures.newHighlight(), { $tag: 'atag' }),
+          Object.assign(fixtures.newHighlight(), { $tag: 'anothertag' }),
+        ],
+        expectedLength: 2,
+      },
+    ].forEach(testCase => {
+      it('returns number of unsaved, new highlights', () => {
+        const state = { annotations: { annotations: testCase.annotations } };
+        assert.lengthOf(
+          selectors.newHighlights(state),
+          testCase.expectedLength
+        );
+      });
+    });
+  });
+
   describe('noteCount', () => {
     it('returns number of page notes', () => {
       const state = {
@@ -475,6 +513,20 @@ describe('sidebar/store/modules/annotations', function() {
       assert.equal(createdAnnotation.permissions, 'somePermissions');
     });
 
+    it('should always assign private permissions to highlights', () => {
+      fakePrivatePermissions.returns('private');
+      store.dispatch(
+        actions.createAnnotation({ id: 'myID', $highlight: true }, now)
+      );
+
+      const createdAnnotation = selectors.findAnnotationByID(
+        store.getState(),
+        'myID'
+      );
+
+      assert.equal(createdAnnotation.permissions, 'private');
+    });
+
     it('should set group to currently-focused group if not set on annotation', () => {
       store.dispatch(actions.createAnnotation({ id: 'myID' }, now));
 

src/sidebar/util/random.js

-/* global Uint8Array */
-
 function byteToHex(val) {
   const str = val.toString(16);
   return str.length === 1 ? '0' + str : str;