Move `createAnnotation` to a service

Move `createAnnotation` out of the store thunk and into a service.
Add auto-creation of draft for new annotations.

Fixes #1834

src/sidebar/components/annotation-omega.js

 import classnames from 'classnames';
 import { createElement } from 'preact';
-import { useEffect, useState } from 'preact/hooks';
+import { useState } from 'preact/hooks';
 import propTypes from 'prop-types';
 
 import useStore from '../store/use-store';
-import {
-  isHighlight,
-  isNew,
-  isReply,
-  quote,
-} from '../util/annotation-metadata';
+import { isNew, isReply, quote } from '../util/annotation-metadata';
 import { isShared } from '../util/permissions';
 import { withServices } from '../util/service-context';
 
@@ -56,18 +51,6 @@ function AnnotationOmega({
   const toggleAction = threadIsCollapsed ? 'Show replies' : 'Hide replies';
   const toggleText = `${toggleAction} (${replyCount})`;
 
-  useEffect(() => {
-    // TEMPORARY. Create a new draft for new (non-highlight) annotations
-    // to put the component in "edit mode."
-    if (!isSaving && !draft && isNew(annotation) && !isHighlight(annotation)) {
-      createDraft(annotation, {
-        tags: annotation.tags,
-        text: annotation.text,
-        isPrivate: !isShared(annotation.permissions),
-      });
-    }
-  }, [annotation, draft, createDraft, isSaving]);
-
   const shouldShowActions = !isSaving && !isEditing && !isNew(annotation);
   const shouldShowLicense = isEditing && !isPrivate && group.type !== 'private';
   const shouldShowReplyToggle = replyCount > 0 && !isReply(annotation);
@@ -80,14 +63,7 @@ function AnnotationOmega({
     createDraft(annotation, { ...draft, text });
   };
 
-  const onReply = () => {
-    // TODO: Re-evaluate error handling here
-    if (!userid) {
-      flash.error('Please log in to reply to annotations');
-    } else {
-      annotationsService.reply(annotation, userid);
-    }
-  };
+  const onReply = () => annotationsService.reply(annotation, userid);
 
   const onSave = async () => {
     setIsSaving(true);

src/sidebar/components/annotation.js

@@ -82,15 +82,6 @@ function AnnotationController(
      * new client-side).
      */
     newlyCreatedByHighlightButton = self.annotation.$highlight || false;
-
-    // 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.
-    if (!newlyCreatedByHighlightButton) {
-      if (isNew(self.annotation) || store.getDraft(self.annotation)) {
-        self.edit();
-      }
-    }
   };
 
   /**

src/sidebar/components/new-note-btn.js

@@ -8,11 +8,10 @@ import { applyTheme } from '../util/theme';
 
 import Button from './button';
 
-function NewNoteButton({ settings }) {
+function NewNoteButton({ annotationsService, settings }) {
   const topLevelFrame = useStore(store => store.mainFrame());
   const isLoggedIn = useStore(store => store.isLoggedIn());
 
-  const createAnnotation = useStore(store => store.createAnnotation);
   const openSidebarPanel = useStore(store => store.openSidebarPanel);
 
   const onNewNoteBtnClick = function() {
@@ -24,7 +23,7 @@ function NewNoteButton({ settings }) {
       target: [],
       uri: topLevelFrame.uri,
     };
-    createAnnotation(annot);
+    annotationsService.create(annot);
   };
 
   return (
@@ -42,9 +41,10 @@ function NewNoteButton({ settings }) {
 }
 NewNoteButton.propTypes = {
   // Injected services.
+  annotationsService: propTypes.object.isRequired,
   settings: propTypes.object.isRequired,
 };
 
-NewNoteButton.injectedProps = ['settings'];
+NewNoteButton.injectedProps = ['annotationsService', 'settings'];
 
 export default withServices(NewNoteButton);

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

@@ -445,23 +445,6 @@ describe('AnnotationOmega', () => {
 
   describe('annotation actions', () => {
     describe('replying to an annotation', () => {
-      // nb: There's no reason this logic needs to stay within `AnnotationOmega`
-      // once we've migrated to it; it could happily move to `AnnotationActionBar`
-      it('should show a flash alert if user not logged in', () => {
-        // No logged-in user...
-        fakeStore.profile.returns({});
-
-        const wrapper = createComponent();
-
-        wrapper
-          .find('AnnotationActionBar')
-          .props()
-          .onReply();
-
-        assert.calledOnce(fakeFlash.error);
-        assert.notCalled(fakeAnnotationsService.reply);
-      });
-
       it('should create a reply', () => {
         const theAnnot = fixtures.defaultAnnotation();
         const wrapper = createComponent({ annotation: theAnnot });

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

@@ -235,28 +235,6 @@ describe('annotation', function() {
       sandbox.restore();
     });
 
-    describe('initialization', function() {
-      it('creates drafts for new annotations on initialization', function() {
-        const annotation = fixtures.newAnnotation();
-        createDirective(annotation);
-        assert.calledWith(fakeStore.createDraft, annotation, {
-          isPrivate: false,
-          tags: annotation.tags,
-          text: annotation.text,
-        });
-      });
-
-      it('edits annotations with drafts on initialization', function() {
-        const annotation = fixtures.oldAnnotation();
-        // The drafts store has some draft changes for this annotation.
-        fakeStore.getDraft.returns({ text: 'foo', tags: [] });
-
-        const controller = createDirective(annotation).controller;
-
-        assert.isTrue(controller.editing());
-      });
-    });
-
     describe('#editing()', function() {
       it('returns false if the annotation does not have a draft', function() {
         const controller = createDirective().controller;

src/sidebar/components/test/new-note-btn-test.js

@@ -11,13 +11,22 @@ import mockImportedComponents from '../../../test-util/mock-imported-components'
 
 describe('NewNoteButton', function() {
   let fakeStore;
+  let fakeAnnotationsService;
   let fakeSettings;
 
   function createComponent() {
-    return mount(<NewNoteButton settings={fakeSettings} />);
+    return mount(
+      <NewNoteButton
+        annotationsService={fakeAnnotationsService}
+        settings={fakeSettings}
+      />
+    );
   }
 
   beforeEach(function() {
+    fakeAnnotationsService = {
+      create: sinon.stub(),
+    };
     fakeSettings = {
       branding: {
         ctaBackgroundColor: '#00f',
@@ -77,7 +86,7 @@ describe('NewNoteButton', function() {
         .onClick();
     });
 
-    assert.calledWith(fakeStore.createAnnotation, {
+    assert.calledWith(fakeAnnotationsService.create, {
       target: [],
       uri: 'thisFrame',
     });

src/sidebar/services/annotations.js

 import SearchClient from '../search-client';
-import { isNew, isPublic } from '../util/annotation-metadata';
-import { privatePermissions, sharedPermissions } from '../util/permissions';
+import * as metadata from '../util/annotation-metadata';
+import {
+  defaultPermissions,
+  privatePermissions,
+  sharedPermissions,
+} from '../util/permissions';
+import { generateHexString } from '../util/random';
+import uiConstants from '../ui-constants';
 
 // @ngInject
 export default function annotationsService(
@@ -12,6 +18,88 @@ export default function annotationsService(
 ) {
   let searchClient = null;
 
+  /**
+   * Extend new annotation objects with defaults and permissions.
+   */
+  function initialize(annotationData, now = new Date()) {
+    const defaultPrivacy = store.getDefault('annotationPrivacy');
+    const groupid = store.focusedGroupId();
+    const profile = store.profile();
+
+    const userid = profile.userid;
+    const userInfo = profile.user_info;
+
+    // We need a unique local/app identifier for this new annotation such
+    // that we might look it up later in the store. It won't have an ID yet,
+    // as it has not been persisted to the service.
+    const $tag = generateHexString(8);
+
+    let permissions = defaultPermissions(userid, groupid, defaultPrivacy);
+
+    // Highlights are peculiar in that they always have private permissions
+    if (metadata.isHighlight(annotationData)) {
+      permissions = privatePermissions(userid);
+    }
+
+    return Object.assign(
+      {
+        created: now.toISOString(),
+        group: groupid,
+        permissions,
+        tags: [],
+        text: '',
+        updated: now.toISOString(),
+        user: userid,
+        user_info: userInfo,
+        $tag: $tag,
+      },
+      annotationData
+    );
+  }
+
+  /**
+   * Populate a new annotation object from `annotation` and add it to the store.
+   * Create a draft for it unless it's a highlight and clear other empty
+   * drafts out of the way.
+   *
+   * @param {Object} annotationData
+   * @param {Date} now
+   */
+  function create(annotationData, now = new Date()) {
+    const annotation = initialize(annotationData, now);
+
+    store.addAnnotations([annotation]);
+
+    // Remove other drafts that are in the way, and their annotations (if new)
+    store.deleteNewAndEmptyDrafts();
+
+    // Create a draft unless it's a highlight
+    if (!metadata.isHighlight(annotation)) {
+      store.createDraft(annotation, {
+        tags: annotation.tags,
+        text: annotation.text,
+        isPrivate: !metadata.isPublic(annotation),
+      });
+    }
+
+    // NB: It may make sense to move the following code at some point to
+    // the UI layer
+    // Select the correct tab
+    // If the annotation is of type note or annotation, make sure
+    // the appropriate tab is selected. If it is of type reply, user
+    // stays in the selected tab.
+    if (metadata.isPageNote(annotation)) {
+      store.selectTab(uiConstants.TAB_NOTES);
+    } else if (metadata.isAnnotation(annotation)) {
+      store.selectTab(uiConstants.TAB_ANNOTATIONS);
+    }
+
+    (annotation.references || []).forEach(parent => {
+      // Expand any parents of this annotation.
+      store.setCollapsed(parent, false);
+    });
+  }
+
   /**
    * Load annotations for all URIs and groupId.
    *
@@ -90,14 +178,14 @@ export default function annotationsService(
   function reply(annotation, userid) {
     const replyAnnotation = {
       group: annotation.group,
-      permissions: isPublic(annotation)
+      permissions: metadata.isPublic(annotation)
         ? sharedPermissions(userid, annotation.group)
         : privatePermissions(userid),
       references: (annotation.references || []).concat(annotation.id),
       target: [{ source: annotation.target[0].source }],
       uri: annotation.uri,
     };
-    store.createAnnotation(replyAnnotation);
+    create(replyAnnotation);
   }
 
   /**
@@ -110,7 +198,7 @@ export default function annotationsService(
 
     const annotationWithChanges = applyDraftChanges(annotation);
 
-    if (isNew(annotation)) {
+    if (metadata.isNew(annotation)) {
       saved = api.annotation.create({}, annotationWithChanges);
     } else {
       saved = api.annotation.update(
@@ -136,6 +224,7 @@ export default function annotationsService(
   }
 
   return {
+    create,
     load,
     reply,
     save,

src/sidebar/services/root-thread.js

@@ -39,6 +39,7 @@ const sortFns = {
 // @ngInject
 export default function RootThread(
   $rootScope,
+  annotationsService,
   store,
   searchFilter,
   viewFilter
@@ -114,7 +115,7 @@ export default function RootThread(
   });
 
   $rootScope.$on(events.BEFORE_ANNOTATION_CREATED, function(event, ann) {
-    store.createAnnotation(ann);
+    annotationsService.create(ann);
   });
 
   // Remove any annotations that are deleted or unloaded

src/sidebar/services/test/root-thread-test.js

@@ -20,6 +20,7 @@ const fixtures = immutable({
 });
 
 describe('rootThread', function() {
+  let fakeAnnotationsService;
   let fakeStore;
   let fakeBuildThread;
   let fakeSearchFilter;
@@ -31,6 +32,9 @@ describe('rootThread', function() {
   let rootThread;
 
   beforeEach(function() {
+    fakeAnnotationsService = {
+      create: sinon.stub(),
+    };
     fakeStore = {
       state: {
         annotations: {
@@ -83,6 +87,7 @@ describe('rootThread', function() {
 
     angular
       .module('app', [])
+      .value('annotationsService', fakeAnnotationsService)
       .value('store', fakeStore)
       .value('searchFilter', fakeSearchFilter)
       .value('settings', fakeSettings)
@@ -351,10 +356,10 @@ describe('rootThread', function() {
   context('when annotation events occur', function() {
     const annot = annotationFixtures.defaultAnnotation();
 
-    it('creates a new annotation in the store when BEFORE_ANNOTATION_CREATED event occurs', function() {
+    it('creates a new annotation when BEFORE_ANNOTATION_CREATED event occurs', function() {
       $rootScope.$broadcast(events.BEFORE_ANNOTATION_CREATED, annot);
       assert.notCalled(fakeStore.removeAnnotations);
-      assert.calledWith(fakeStore.createAnnotation, sinon.match(annot));
+      assert.calledWith(fakeAnnotationsService.create, sinon.match(annot));
     });
 
     [

src/sidebar/store/modules/annotations.js

@@ -5,15 +5,10 @@
 
 import { createSelector } from 'reselect';
 
-import uiConstants from '../../ui-constants';
 import * as metadata from '../../util/annotation-metadata';
 import * as arrayUtil from '../../util/array';
-import { defaultPermissions, privatePermissions } from '../../util/permissions';
 import * as util from '../util';
 
-import drafts from './drafts';
-import selection from './selection';
-
 /**
  * Return a copy of `current` with all matching annotations in `annotations`
  * removed.
@@ -241,6 +236,8 @@ function addAnnotations(annotations) {
     });
 
     // If we're not in the sidebar, we're done here.
+    // FIXME Split the annotation-adding from the anchoring code; possibly
+    // move into service
     if (!getState().viewer.isSidebar) {
       return;
     }
@@ -320,71 +317,6 @@ function hideAnnotation(id) {
   };
 }
 
-/**
- * Create a new annotation (as-yet unpersisted)
- *
- * This method has several responsibilities:
- * 1. Set some default data attributes on the annotation
- * 2. Remove any existing, empty drafts
- * 3. Add the annotation to the current collection of annotations
- * 4. Change focused tab to the applicable one for the new annotation's meta-type
- * 5. Expand all of the new annotation's parents
- *
- */
-function createAnnotation(ann, now = new Date()) {
-  return (dispatch, getState) => {
-    /**
-     * Extend the new, unsaved annotation object with defaults for some
-     * required data fields and default permissions.
-     *
-     * Note: the `created` and `updated` values will be ignored and superseded
-     * by the service when the annotation is persisted, but they are used
-     * app-side for annotation card sorting until then.
-     */
-    const groupid = getState().groups.focusedGroupId;
-    const userid = getState().session.userid;
-    ann = Object.assign(
-      {
-        created: now.toISOString(),
-        group: groupid,
-        permissions: defaultPermissions(
-          userid,
-          groupid,
-          getState().defaults.annotationPrivacy
-        ),
-        tags: [],
-        text: '',
-        updated: now.toISOString(),
-        user: userid,
-        user_info: getState().session.user_info,
-      },
-      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]));
-
-    dispatch(addAnnotations([ann]));
-    // If the annotation is of type note or annotation, make sure
-    // the appropriate tab is selected. If it is of type reply, user
-    // stays in the selected tab.
-    if (metadata.isPageNote(ann)) {
-      dispatch(selection.actions.selectTab(uiConstants.TAB_NOTES));
-    } else if (metadata.isAnnotation(ann)) {
-      dispatch(selection.actions.selectTab(uiConstants.TAB_ANNOTATIONS));
-    }
-    (ann.references || []).forEach(parent => {
-      // Expand any parents of this annotation.
-      dispatch(selection.actions.setCollapsed(parent, false));
-    });
-  };
-}
-
 /**
  * Update the local hidden state of an annotation.
  *
@@ -491,7 +423,6 @@ export default {
   actions: {
     addAnnotations,
     clearAnnotations,
-    createAnnotation,
     hideAnnotation,
     removeAnnotations,
     updateAnchorStatus,

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

 import * as fixtures from '../../../test/annotation-fixtures';
-import uiConstants from '../../../ui-constants';
 import * as metadata from '../../../util/annotation-metadata';
 import createStore from '../../create-store';
 import annotations from '../annotations';
-import { $imports } from '../annotations';
-import defaults from '../defaults';
-import drafts from '../drafts';
-import groups from '../groups';
-import selection from '../selection';
-import session from '../session';
 import viewer from '../viewer';
 
 const { actions, selectors } = annotations;
 
 function createTestStore() {
-  return createStore(
-    [annotations, selection, defaults, drafts, groups, session, viewer],
-    [{}]
-  );
+  return createStore([annotations, viewer], [{}]);
 }
 
 // Tests for most of the functionality in reducers/annotations.js are currently
 // in the tests for the whole Redux store
 
 describe('sidebar/store/modules/annotations', function() {
-  let fakeDefaultPermissions;
-  let fakePrivatePermissions;
-
-  beforeEach(() => {
-    fakeDefaultPermissions = sinon.stub();
-    fakePrivatePermissions = sinon.stub();
-    $imports.$mock({
-      '../../util/permissions': {
-        defaultPermissions: fakeDefaultPermissions,
-        privatePermissions: fakePrivatePermissions,
-      },
-    });
-  });
-
-  afterEach(() => {
-    $imports.$restore();
-  });
-
   describe('#addAnnotations()', function() {
     const ANCHOR_TIME_LIMIT = 1000;
     let clock;
@@ -72,36 +44,6 @@ describe('sidebar/store/modules/annotations', function() {
       ]);
     });
 
-    it('does not change `selectedTab` state if annotations are already loaded', function() {
-      const annot = fixtures.defaultAnnotation();
-      store.addAnnotations([annot]);
-      const page = fixtures.oldPageNote();
-      store.addAnnotations([page]);
-      assert.equal(
-        store.getState().selection.selectedTab,
-        uiConstants.TAB_ANNOTATIONS
-      );
-    });
-
-    it('sets `selectedTab` to "note" if only page notes are present', function() {
-      const page = fixtures.oldPageNote();
-      store.addAnnotations([page]);
-      assert.equal(
-        store.getState().selection.selectedTab,
-        uiConstants.TAB_NOTES
-      );
-    });
-
-    it('leaves `selectedTab` as "annotation" if annotations and/or page notes are present', function() {
-      const page = fixtures.oldPageNote();
-      const annot = fixtures.defaultAnnotation();
-      store.addAnnotations([annot, page]);
-      assert.equal(
-        store.getState().selection.selectedTab,
-        uiConstants.TAB_ANNOTATIONS
-      );
-    });
-
     it('assigns a local tag to annotations', function() {
       const annotA = Object.assign(fixtures.defaultAnnotation(), { id: 'a1' });
       const annotB = Object.assign(fixtures.defaultAnnotation(), { id: 'a2' });
@@ -444,187 +386,4 @@ describe('sidebar/store/modules/annotations', function() {
       });
     });
   });
-
-  describe('#createAnnotation', function() {
-    let clock;
-    let now;
-    let store;
-
-    beforeEach(() => {
-      // Stop the clock to keep the current date from advancing
-      clock = sinon.useFakeTimers();
-      now = new Date();
-      store = createTestStore();
-    });
-
-    afterEach(() => {
-      clock.restore();
-    });
-
-    it('should create an annotation', function() {
-      const ann = fixtures.oldAnnotation();
-      store.dispatch(actions.createAnnotation(ann));
-      assert.equal(
-        selectors.findAnnotationByID(store.getState(), ann.id).id,
-        ann.id
-      );
-    });
-
-    it('should set basic default properties on a new/empty annotation', () => {
-      store.dispatch(actions.createAnnotation({ id: 'myID' }, now));
-
-      const createdAnnotation = selectors.findAnnotationByID(
-        store.getState(),
-        'myID'
-      );
-
-      assert.include(createdAnnotation, {
-        created: now.toISOString(),
-        updated: now.toISOString(),
-        text: '',
-      });
-      assert.isArray(createdAnnotation.tags);
-    });
-
-    it('should set user properties on a new/empty annotation', () => {
-      store.dispatch(actions.createAnnotation({ id: 'myID' }, now));
-
-      const createdAnnotation = selectors.findAnnotationByID(
-        store.getState(),
-        'myID'
-      );
-
-      assert.equal(createdAnnotation.user, store.getState().session.userid);
-      assert.equal(
-        createdAnnotation.user_info,
-        store.getState().session.user_info
-      );
-    });
-
-    it('should set default permissions on a new annotation', () => {
-      fakeDefaultPermissions.returns('somePermissions');
-      store.dispatch(actions.createAnnotation({ id: 'myID' }, now));
-
-      const createdAnnotation = selectors.findAnnotationByID(
-        store.getState(),
-        'myID'
-      );
-
-      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));
-
-      const createdAnnotation = selectors.findAnnotationByID(
-        store.getState(),
-        'myID'
-      );
-
-      assert.equal(
-        createdAnnotation.group,
-        store.getState().groups.focusedGroupId
-      );
-    });
-
-    it('should set not overwrite properties if present', () => {
-      store.dispatch(
-        actions.createAnnotation(
-          {
-            id: 'myID',
-            created: 'when',
-            updated: 'then',
-            text: 'my annotation',
-            tags: ['foo', 'bar'],
-            group: 'fzzy',
-            permissions: ['whatever'],
-            user: 'acct:foo@bar.com',
-            user_info: {
-              display_name: 'Herbivore Fandango',
-            },
-          },
-          now
-        )
-      );
-
-      const createdAnnotation = selectors.findAnnotationByID(
-        store.getState(),
-        'myID'
-      );
-
-      assert.include(createdAnnotation, {
-        created: 'when',
-        updated: 'then',
-        text: 'my annotation',
-        group: 'fzzy',
-        user: 'acct:foo@bar.com',
-      });
-
-      assert.include(createdAnnotation.tags, 'foo', 'bar');
-      assert.include(createdAnnotation.permissions, 'whatever');
-      assert.equal(
-        createdAnnotation.user_info.display_name,
-        'Herbivore Fandango'
-      );
-    });
-
-    it('should change tab focus to TAB_ANNOTATIONS when a new annotation is created', function() {
-      store.dispatch(actions.createAnnotation(fixtures.oldAnnotation()));
-      assert.equal(
-        store.getState().selection.selectedTab,
-        uiConstants.TAB_ANNOTATIONS
-      );
-    });
-
-    it('should change tab focus to TAB_NOTES when a new note annotation is created', function() {
-      store.dispatch(actions.createAnnotation(fixtures.oldPageNote()));
-      assert.equal(
-        store.getState().selection.selectedTab,
-        uiConstants.TAB_NOTES
-      );
-    });
-
-    it('should expand parent of created annotation', function() {
-      const store = createTestStore();
-      store.dispatch(
-        actions.addAnnotations([
-          {
-            id: 'annotation_id',
-            $highlight: undefined,
-            target: [{ source: 'source', selector: [] }],
-            references: [],
-            text: 'This is my annotation',
-            tags: ['tag_1', 'tag_2'],
-          },
-        ])
-      );
-      // Collapse the parent.
-      store.dispatch(selection.actions.setCollapsed('annotation_id', true));
-      // Creating a new child annotation should expand its parent.
-      store.dispatch(
-        actions.createAnnotation({
-          highlight: undefined,
-          target: [{ source: 'http://example.org' }],
-          references: ['annotation_id'],
-          text: '',
-          tags: [],
-        })
-      );
-      assert.isTrue(store.getState().selection.expanded.annotation_id);
-    });
-  });
 });

src/sidebar/test/integration/threading-test.js

@@ -57,6 +57,7 @@ describe('annotation threading', function() {
       .service('store', storeFactory)
       .service('rootThread', rootThreadFactory)
       .service('searchFilter', searchFilterFactory)
+      .service('annotationsService', () => {})
       .service('viewFilter', viewFilterFactory)
       .value('features', fakeFeatures)
       .value('settings', {})