Merge pull request #1875 from hypothesis/split-annotations-service

Split `loadAnnotationsService` out of `annotationsService`

src/sidebar/components/sidebar-content.js

@@ -6,7 +6,7 @@ import * as tabs from '../util/tabs';
 function SidebarContentController(
   $scope,
   analytics,
-  annotationsService,
+  loadAnnotationsService,
   store,
   frameSync,
   rootThread,
@@ -120,7 +120,7 @@ function SidebarContentController(
       }
 
       const searchUris = store.searchUris();
-      annotationsService.load(searchUris, currentGroupId);
+      loadAnnotationsService.load(searchUris, currentGroupId);
     },
     true
   );

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

@@ -20,7 +20,7 @@ describe('sidebar.components.sidebar-content', function() {
   let store;
   let ctrl;
   let fakeAnalytics;
-  let fakeAnnotations;
+  let fakeLoadAnnotationsService;
   let fakeFrameSync;
   let fakeRootThread;
   let fakeSettings;
@@ -56,7 +56,7 @@ describe('sidebar.components.sidebar-content', function() {
         reconnect: sandbox.stub(),
       };
 
-      fakeAnnotations = {
+      fakeLoadAnnotationsService = {
         load: sinon.stub(),
       };
 
@@ -68,7 +68,7 @@ describe('sidebar.components.sidebar-content', function() {
       $provide.value('frameSync', fakeFrameSync);
       $provide.value('rootThread', fakeRootThread);
       $provide.value('streamer', fakeStreamer);
-      $provide.value('annotationsService', fakeAnnotations);
+      $provide.value('loadAnnotationsService', fakeLoadAnnotationsService);
       $provide.value('settings', fakeSettings);
     });
   });
@@ -167,7 +167,7 @@ describe('sidebar.components.sidebar-content', function() {
   function connectFrameAndPerformInitialFetch() {
     setFrames([{ uri: 'https://a-page.com' }]);
     $scope.$digest();
-    fakeAnnotations.load.reset();
+    fakeLoadAnnotationsService.load.reset();
   }
 
   it('generates the thread list', () => {
@@ -184,7 +184,7 @@ describe('sidebar.components.sidebar-content', function() {
       $scope.$digest();
 
       assert.calledWith(
-        fakeAnnotations.load,
+        fakeLoadAnnotationsService.load,
         ['https://a-page.com', 'https://new-frame.com'],
         'group-id'
       );
@@ -203,7 +203,7 @@ describe('sidebar.components.sidebar-content', function() {
       $scope.$digest();
 
       assert.calledWith(
-        fakeAnnotations.load,
+        fakeLoadAnnotationsService.load,
         ['https://a-page.com'],
         'group-id'
       );
@@ -219,7 +219,7 @@ describe('sidebar.components.sidebar-content', function() {
       store.updateSession(newProfile);
       $scope.$digest();
 
-      assert.notCalled(fakeAnnotations.load);
+      assert.notCalled(fakeLoadAnnotationsService.load);
     });
   });
 
@@ -250,7 +250,7 @@ describe('sidebar.components.sidebar-content', function() {
       store.addAnnotations = sinon.stub();
       setFrames([{ uri: uri }]);
       $scope.$digest();
-      fakeAnnotations.load = sinon.stub();
+      fakeLoadAnnotationsService.load = sinon.stub();
     });
 
     it('should load annotations for the new group', () => {
@@ -260,7 +260,7 @@ describe('sidebar.components.sidebar-content', function() {
       $scope.$digest();
 
       assert.calledWith(
-        fakeAnnotations.load,
+        fakeLoadAnnotationsService.load,
         ['http://example.com'],
         'different-group'
       );

src/sidebar/index.js

@@ -179,6 +179,7 @@ import featuresService from './services/features';
 import flashService from './services/flash';
 import frameSyncService from './services/frame-sync';
 import groupsService from './services/groups';
+import loadAnnotationsService from './services/load-annotations';
 import localStorageService from './services/local-storage';
 import permissionsService from './services/permissions';
 import persistedDefaultsService from './services/persisted-defaults';
@@ -221,6 +222,7 @@ function startAngularApp(config) {
     .register('flash', flashService)
     .register('frameSync', frameSyncService)
     .register('groups', groupsService)
+    .register('loadAnnotationsService', loadAnnotationsService)
     .register('localStorage', localStorageService)
     .register('permissions', permissionsService)
     .register('persistedDefaults', persistedDefaultsService)
@@ -307,6 +309,9 @@ function startAngularApp(config) {
     .service('flash', () => container.get('flash'))
     .service('frameSync', () => container.get('frameSync'))
     .service('groups', () => container.get('groups'))
+    .service('loadAnnotationsService', () =>
+      container.get('loadAnnotationsService')
+    )
     .service('permissions', () => container.get('permissions'))
     .service('persistedDefaults', () => container.get('persistedDefaults'))
     .service('rootThread', () => container.get('rootThread'))

src/sidebar/services/annotations.js

-import SearchClient from '../search-client';
+/**
+ * A service for creating, manipulating and persisting annotations and their
+ * application-store representations. Interacts with API services as needed.
+ */
+
 import * as metadata from '../util/annotation-metadata';
 import {
   defaultPermissions,
@@ -9,14 +13,26 @@ import { generateHexString } from '../util/random';
 import uiConstants from '../ui-constants';
 
 // @ngInject
-export default function annotationsService(
-  annotationMapper,
-  api,
-  store,
-  streamer,
-  streamFilter
-) {
-  let searchClient = null;
+export default function annotationsService(api, store) {
+  /**
+   * Apply changes for the given `annotation` from its draft in the store (if
+   * any) and return a new object with those changes integrated.
+   */
+  function applyDraftChanges(annotation) {
+    const changes = {};
+    const draft = store.getDraft(annotation);
+
+    if (draft) {
+      changes.tags = draft.tags;
+      changes.text = draft.text;
+      changes.permissions = draft.isPrivate
+        ? privatePermissions(annotation.user)
+        : sharedPermissions(annotation.user, annotation.group);
+    }
+
+    // Integrate changes from draft into object to be persisted
+    return { ...annotation, ...changes };
+  }
 
   /**
    * Extend new annotation objects with defaults and permissions.
@@ -100,75 +116,6 @@ export default function annotationsService(
     });
   }
 
-  /**
-   * Load annotations for all URIs and groupId.
-   *
-   * @param {string[]} uris
-   * @param {string} groupId
-   */
-  function load(uris, groupId) {
-    annotationMapper.unloadAnnotations(store.savedAnnotations());
-
-    // Cancel previously running search client.
-    if (searchClient) {
-      searchClient.cancel();
-    }
-
-    if (uris.length > 0) {
-      searchAndLoad(uris, groupId);
-
-      streamFilter.resetFilter().addClause('/uri', 'one_of', uris);
-      streamer.setConfig('filter', { filter: streamFilter.getFilter() });
-    }
-  }
-
-  function searchAndLoad(uris, groupId) {
-    searchClient = new SearchClient(api.search, {
-      incremental: true,
-    });
-    searchClient.on('results', results => {
-      if (results.length) {
-        annotationMapper.loadAnnotations(results);
-      }
-    });
-    searchClient.on('error', error => {
-      console.error(error);
-    });
-    searchClient.on('end', () => {
-      // Remove client as it's no longer active.
-      searchClient = null;
-
-      store.frames().forEach(function(frame) {
-        if (0 <= uris.indexOf(frame.uri)) {
-          store.updateFrameAnnotationFetchStatus(frame.uri, true);
-        }
-      });
-      store.annotationFetchFinished();
-    });
-    store.annotationFetchStarted();
-    searchClient.get({ uri: uris, group: groupId });
-  }
-
-  /**
-   * Apply changes for the given `annotation` from its draft in the store (if
-   * any) and return a new object with those changes integrated.
-   */
-  function applyDraftChanges(annotation) {
-    const changes = {};
-    const draft = store.getDraft(annotation);
-
-    if (draft) {
-      changes.tags = draft.tags;
-      changes.text = draft.text;
-      changes.permissions = draft.isPrivate
-        ? privatePermissions(annotation.user)
-        : sharedPermissions(annotation.user, annotation.group);
-    }
-
-    // Integrate changes from draft into object to be persisted
-    return { ...annotation, ...changes };
-  }
-
   /**
    * Create a reply to `annotation` by the user `userid` and add to the store.
    *
@@ -225,7 +172,6 @@ export default function annotationsService(
 
   return {
     create,
-    load,
     reply,
     save,
   };

src/sidebar/services/load-annotations.js

+/**
+ * A service for fetching annotations, filtered by document URIs and group.
+ */
+import SearchClient from '../search-client';
+
+// @ngInject
+export default function loadAnnotationsService(
+  annotationMapper,
+  api,
+  store,
+  streamer,
+  streamFilter
+) {
+  let searchClient = null;
+
+  /**
+   * Load annotations for all URIs and groupId.
+   *
+   * @param {string[]} uris
+   * @param {string} groupId
+   */
+  function load(uris, groupId) {
+    annotationMapper.unloadAnnotations(store.savedAnnotations());
+
+    // Cancel previously running search client.
+    if (searchClient) {
+      searchClient.cancel();
+    }
+
+    if (uris.length > 0) {
+      searchAndLoad(uris, groupId);
+
+      streamFilter.resetFilter().addClause('/uri', 'one_of', uris);
+      streamer.setConfig('filter', { filter: streamFilter.getFilter() });
+    }
+  }
+
+  function searchAndLoad(uris, groupId) {
+    searchClient = new SearchClient(api.search, {
+      incremental: true,
+    });
+    searchClient.on('results', results => {
+      if (results.length) {
+        annotationMapper.loadAnnotations(results);
+      }
+    });
+    searchClient.on('error', error => {
+      console.error(error);
+    });
+    searchClient.on('end', () => {
+      // Remove client as it's no longer active.
+      searchClient = null;
+
+      store.frames().forEach(function(frame) {
+        if (0 <= uris.indexOf(frame.uri)) {
+          store.updateFrameAnnotationFetchStatus(frame.uri, true);
+        }
+      });
+      store.annotationFetchFinished();
+    });
+    store.annotationFetchStarted();
+    searchClient.get({ uri: uris, group: groupId });
+  }
+
+  return {
+    load,
+  };
+}

src/sidebar/services/test/load-annotations-test.js

+import EventEmitter from 'tiny-emitter';
+
+import loadAnnotationsService, { $imports } from '../load-annotations';
+
+let searchClients;
+let longRunningSearchClient = false;
+class FakeSearchClient extends EventEmitter {
+  constructor(searchFn, opts) {
+    super();
+
+    assert.ok(searchFn);
+    searchClients.push(this);
+    this.cancel = sinon.stub();
+    this.incremental = !!opts.incremental;
+
+    this.get = sinon.spy(query => {
+      assert.ok(query.uri);
+
+      for (let i = 0; i < query.uri.length; i++) {
+        const uri = query.uri[i];
+        this.emit('results', [{ id: uri + '123', group: '__world__' }]);
+        this.emit('results', [{ id: uri + '456', group: 'private-group' }]);
+      }
+      if (!longRunningSearchClient) {
+        this.emit('end');
+      }
+    });
+  }
+}
+
+describe('loadAnnotationsService', () => {
+  let fakeAnnotationMapper;
+  let fakeApi;
+  let fakeStore;
+  let fakeStreamer;
+  let fakeStreamFilter;
+
+  const fakeGroupId = 'group-id';
+  let fakeUris;
+
+  beforeEach(() => {
+    sinon.stub(console, 'error');
+    searchClients = [];
+    longRunningSearchClient = false;
+
+    fakeAnnotationMapper = {
+      loadAnnotations: sinon.stub(),
+      unloadAnnotations: sinon.stub(),
+    };
+
+    fakeApi = {
+      search: sinon.stub(),
+    };
+
+    fakeStore = {
+      annotationFetchFinished: sinon.stub(),
+      annotationFetchStarted: sinon.stub(),
+      frames: sinon.stub(),
+      savedAnnotations: sinon.stub(),
+      updateFrameAnnotationFetchStatus: sinon.stub(),
+    };
+
+    fakeStreamer = {
+      setConfig: sinon.stub(),
+      connect: sinon.stub(),
+      reconnect: sinon.stub(),
+    };
+    fakeStreamFilter = {
+      resetFilter: sinon.stub().returns({
+        addClause: sinon.stub(),
+      }),
+      getFilter: sinon.stub().returns({}),
+    };
+
+    fakeUris = ['http://example.com'];
+    $imports.$mock({
+      '../search-client': FakeSearchClient,
+    });
+  });
+
+  afterEach(() => {
+    console.error.restore();
+    $imports.$restore();
+  });
+
+  function createService() {
+    fakeStore.frames.returns(
+      fakeUris.map(uri => {
+        return { uri: uri };
+      })
+    );
+    return loadAnnotationsService(
+      fakeAnnotationMapper,
+      fakeApi,
+      fakeStore,
+      fakeStreamer,
+      fakeStreamFilter
+    );
+  }
+
+  describe('load', () => {
+    it('unloads any existing annotations', () => {
+      // When new clients connect, all existing annotations should be unloaded
+      // before reloading annotations for each currently-connected client.
+      fakeStore.savedAnnotations.returns([
+        { id: fakeUris[0] + '123' },
+        { id: fakeUris[0] + '456' },
+      ]);
+      const svc = createService();
+
+      svc.load(fakeUris, fakeGroupId);
+      assert.calledWith(fakeAnnotationMapper.unloadAnnotations, [
+        sinon.match({ id: fakeUris[0] + '123' }),
+        sinon.match({ id: fakeUris[0] + '456' }),
+      ]);
+    });
+
+    it('loads all annotations for a URI', () => {
+      const svc = createService();
+
+      svc.load(fakeUris, fakeGroupId);
+      assert.calledWith(fakeAnnotationMapper.loadAnnotations, [
+        sinon.match({ id: fakeUris[0] + '123' }),
+      ]);
+      assert.calledWith(fakeAnnotationMapper.loadAnnotations, [
+        sinon.match({ id: fakeUris[0] + '456' }),
+      ]);
+    });
+
+    it('loads all annotations for a frame with multiple URIs', () => {
+      const uri = 'http://example.com/test.pdf';
+      const fingerprint = 'urn:x-pdf:fingerprint';
+      fakeUris = [uri, fingerprint];
+      const svc = createService();
+      // Override the default frames set by the service call above.
+      fakeStore.frames.returns([
+        {
+          uri: uri,
+          metadata: {
+            documentFingerprint: 'fingerprint',
+            link: [
+              {
+                href: fingerprint,
+              },
+              {
+                href: uri,
+              },
+            ],
+          },
+        },
+      ]);
+
+      svc.load(fakeUris, fakeGroupId);
+      assert.calledWith(fakeAnnotationMapper.loadAnnotations, [
+        sinon.match({ id: uri + '123' }),
+      ]);
+      assert.calledWith(fakeAnnotationMapper.loadAnnotations, [
+        sinon.match({ id: fingerprint + '123' }),
+      ]);
+      assert.calledWith(fakeAnnotationMapper.loadAnnotations, [
+        sinon.match({ id: uri + '456' }),
+      ]);
+      assert.calledWith(fakeAnnotationMapper.loadAnnotations, [
+        sinon.match({ id: fingerprint + '456' }),
+      ]);
+    });
+
+    it('loads all annotations for all URIs', () => {
+      fakeUris = ['http://example.com', 'http://foobar.com'];
+      const svc = createService();
+
+      svc.load(fakeUris, fakeGroupId);
+
+      [
+        fakeUris[0] + '123',
+        fakeUris[0] + '456',
+        fakeUris[1] + '123',
+        fakeUris[1] + '456',
+      ].forEach(uri => {
+        assert.calledWith(fakeAnnotationMapper.loadAnnotations, [
+          sinon.match({ id: uri }),
+        ]);
+      });
+    });
+
+    it('updates annotation fetch status for all frames', () => {
+      fakeUris = ['http://example.com', 'http://foobar.com'];
+      const svc = createService();
+
+      svc.load(fakeUris, fakeGroupId);
+      assert.calledWith(
+        fakeStore.updateFrameAnnotationFetchStatus,
+        fakeUris[0],
+        true
+      );
+      assert.calledWith(
+        fakeStore.updateFrameAnnotationFetchStatus,
+        fakeUris[1],
+        true
+      );
+    });
+
+    it('fetches annotations for the specified group', () => {
+      const svc = createService();
+
+      svc.load(fakeUris, fakeGroupId);
+      assert.calledWith(searchClients[0].get, {
+        uri: fakeUris,
+        group: fakeGroupId,
+      });
+    });
+
+    it('loads annotations in batches', () => {
+      const svc = createService();
+
+      svc.load(fakeUris, fakeGroupId);
+      assert.ok(searchClients[0].incremental);
+    });
+
+    it("cancels previously search client if it's still running", () => {
+      const svc = createService();
+
+      // Issue a long running load annotations request.
+      longRunningSearchClient = true;
+      svc.load(fakeUris, fakeGroupId);
+      // Issue another load annotations request while the
+      // previous annotation load is still running.
+      svc.load(fakeUris, fakeGroupId);
+
+      assert.calledOnce(searchClients[0].cancel);
+    });
+
+    it('does not load annotations if URIs list is empty', () => {
+      fakeUris = [];
+      const svc = createService();
+
+      svc.load(fakeUris, fakeGroupId);
+      assert.notCalled(fakeAnnotationMapper.loadAnnotations);
+    });
+
+    it('calls annotationFetchStarted when it starts searching for annotations', () => {
+      const svc = createService();
+
+      svc.load(fakeUris, fakeGroupId);
+
+      assert.calledOnce(fakeStore.annotationFetchStarted);
+    });
+
+    it('calls annotationFetchFinished when all annotations have been found', () => {
+      const svc = createService();
+
+      svc.load(fakeUris, fakeGroupId);
+
+      assert.calledOnce(fakeStore.annotationFetchFinished);
+    });
+
+    it('logs an error to the console if the search client runs into an error', () => {
+      const svc = createService();
+      const error = new Error('search for annotations failed');
+
+      svc.load(fakeUris, fakeGroupId);
+      searchClients[0].emit('error', error);
+
+      assert.calledWith(console.error, error);
+    });
+  });
+});