Merge pull request #1762 from hypothesis/annotation-save-method

Add `save` method to `AnnotationsService`; use it in `AnnotationOmega`

src/boot/boot.js

@@ -72,6 +72,7 @@ function bootHypothesisClient(doc, config) {
     'es2015',
     'es2016',
     'es2017',
+    'es2018',
     'url',
   ]);
 

src/shared/polyfills/es2018.js

+import 'core-js/es/promise/finally';

src/shared/polyfills/index.js

@@ -57,6 +57,10 @@ const needsPolyfill = {
     return !hasMethods(Object, 'entries', 'values');
   },
 
+  es2018: () => {
+    return !hasMethods(Promise.prototype, 'finally');
+  },
+
   // Test for a fully-working URL constructor.
   url: () => {
     try {

src/shared/polyfills/test/index-test.js

@@ -36,6 +36,10 @@ describe('shared/polyfills/index', () => {
         set: 'es2017',
         providesMethod: [Object, 'entries'],
       },
+      {
+        set: 'es2018',
+        providesMethod: [Promise.prototype, 'finally'],
+      },
       {
         set: 'string.prototype.normalize',
         providesMethod: [String.prototype, 'normalize'],

src/sidebar/components/annotation-omega.js

 import { createElement } from 'preact';
-import { useEffect } from 'preact/hooks';
+import { useEffect, useState } from 'preact/hooks';
 import propTypes from 'prop-types';
 
 import useStore from '../store/use-store';
 import { isHighlight, isNew, quote } from '../util/annotation-metadata';
 import { isShared } from '../util/permissions';
+import { withServices } from '../util/service-context';
 
 import AnnotationActionBar from './annotation-action-bar';
 import AnnotationBody from './annotation-body';
@@ -20,6 +21,8 @@ import TagList from './tag-list';
  */
 function AnnotationOmega({
   annotation,
+  annotationsService,
+  flash,
   onReplyCountClick,
   replyCount,
   showDocumentInfo,
@@ -30,26 +33,27 @@ function AnnotationOmega({
   const draft = useStore(store => store.getDraft(annotation));
   const group = useStore(store => store.getGroup(annotation.group));
 
+  const isPrivate = draft ? draft.isPrivate : !isShared(annotation.permissions);
+  const tags = draft ? draft.tags : annotation.tags;
+  const text = draft ? draft.text : annotation.text;
+
+  const hasQuote = !!quote(annotation);
+  const isEmpty = !text && !tags.length;
+
+  const [isSaving, setIsSaving] = useState(false);
+  const isEditing = !!draft && !isSaving;
+
   useEffect(() => {
     // TEMPORARY. Create a new draft for new (non-highlight) annotations
     // to put the component in "edit mode."
-    if (!draft && isNew(annotation) && !isHighlight(annotation)) {
+    if (!isSaving && !draft && isNew(annotation) && !isHighlight(annotation)) {
       createDraft(annotation, {
         tags: annotation.tags,
         text: annotation.text,
         isPrivate: !isShared(annotation.permissions),
       });
     }
-  }, [annotation, draft, createDraft]);
-
-  const isPrivate = draft ? draft.isPrivate : !isShared(annotation.permissions);
-  const tags = draft ? draft.tags : annotation.tags;
-  const text = draft ? draft.text : annotation.text;
-
-  const hasQuote = !!quote(annotation);
-  const isEmpty = !text && !tags.length;
-  const isSaving = false;
-  const isEditing = !!draft && !isSaving;
+  }, [annotation, draft, createDraft, isSaving]);
 
   const shouldShowActions = !isEditing && !isNew(annotation);
   const shouldShowLicense = isEditing && !isPrivate && group.type !== 'private';
@@ -62,9 +66,19 @@ function AnnotationOmega({
     createDraft(annotation, { ...draft, text });
   };
 
+  const onSave = async () => {
+    setIsSaving(true);
+    try {
+      await annotationsService.save(annotation);
+    } catch (err) {
+      flash.error(err.message, 'Saving annotation failed');
+    } finally {
+      setIsSaving(false);
+    }
+  };
+
   // TODO
   const fakeOnReply = () => alert('Reply: TBD');
-  const fakeOnSave = () => alert('Save changes: TBD');
 
   return (
     <div className="annotation-omega">
@@ -90,7 +104,7 @@ function AnnotationOmega({
             <AnnotationPublishControl
               annotation={annotation}
               isDisabled={isEmpty}
-              onSave={fakeOnSave}
+              onSave={onSave}
             />
           )}
         </div>
@@ -117,6 +131,12 @@ AnnotationOmega.propTypes = {
   replyCount: propTypes.number.isRequired,
   /** Should extended document info be rendered (e.g. in non-sidebar contexts)? */
   showDocumentInfo: propTypes.bool.isRequired,
+
+  /* Injected services */
+  annotationsService: propTypes.object.isRequired,
+  flash: propTypes.object.isRequired,
 };
 
-export default AnnotationOmega;
+AnnotationOmega.injectedProps = ['annotationsService', 'flash'];
+
+export default withServices(AnnotationOmega);

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

@@ -5,6 +5,7 @@ import { act } from 'preact/test-utils';
 import * as fixtures from '../../test/annotation-fixtures';
 
 import mockImportedComponents from '../../../test-util/mock-imported-components';
+import { waitFor } from '../../../test-util/wait';
 
 // @TODO Note this import as `Annotation` for easier updating later
 
@@ -17,6 +18,10 @@ describe('AnnotationOmega', () => {
   // Dependency Mocks
   let fakeMetadata;
   let fakePermissions;
+
+  // Injected dependency mocks
+  let fakeAnnotationsService;
+  let fakeFlash;
   let fakeStore;
 
   const setEditingMode = (isEditing = true) => {
@@ -32,6 +37,8 @@ describe('AnnotationOmega', () => {
     return mount(
       <Annotation
         annotation={fixtures.defaultAnnotation()}
+        annotationsService={fakeAnnotationsService}
+        flash={fakeFlash}
         onReplyCountClick={fakeOnReplyCountClick}
         replyCount={0}
         showDocumentInfo={false}
@@ -43,6 +50,14 @@ describe('AnnotationOmega', () => {
   beforeEach(() => {
     fakeOnReplyCountClick = sinon.stub();
 
+    fakeAnnotationsService = {
+      save: sinon.stub().resolves(),
+    };
+
+    fakeFlash = {
+      error: sinon.stub(),
+    };
+
     fakeMetadata = {
       isNew: sinon.stub(),
       quote: sinon.stub(),
@@ -72,6 +87,8 @@ describe('AnnotationOmega', () => {
     $imports.$restore();
   });
 
+  it('should test `isSaving`');
+
   describe('annotation quote', () => {
     it('renders quote if annotation has a quote', () => {
       fakeMetadata.quote.returns('quote');
@@ -179,6 +196,36 @@ describe('AnnotationOmega', () => {
         wrapper.find('AnnotationPublishControl').props().isDisabled
       );
     });
+
+    context('saving an annotation', () => {
+      it('should save the annotation when the publish control invokes the `onSave` callback', () => {
+        setEditingMode(true);
+
+        const wrapper = createComponent();
+        wrapper
+          .find('AnnotationPublishControl')
+          .props()
+          .onSave();
+
+        assert.calledWith(
+          fakeAnnotationsService.save,
+          wrapper.props().annotation
+        );
+      });
+
+      it('should flash an error message on failure', async () => {
+        setEditingMode(true);
+        fakeAnnotationsService.save.rejects();
+
+        const wrapper = createComponent();
+        wrapper
+          .find('AnnotationPublishControl')
+          .props()
+          .onSave();
+
+        await waitFor(() => fakeFlash.error.called);
+      });
+    });
   });
 
   describe('license information', () => {

src/sidebar/services/annotations.js

 import SearchClient from '../search-client';
+import { isNew } from '../util/annotation-metadata';
+import { privatePermissions, sharedPermissions } from '../util/permissions';
 
 // @ngInject
 export default function annotationsService(
@@ -59,7 +61,63 @@ export default function annotationsService(
     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 };
+  }
+
+  /**
+   * Save new (or update existing) annotation. On success,
+   * the annotation's `Draft` will be removed and the annotation added
+   * to the store.
+   */
+  async function save(annotation) {
+    let saved;
+
+    const annotationWithChanges = applyDraftChanges(annotation);
+
+    if (isNew(annotation)) {
+      saved = api.annotation.create({}, annotationWithChanges);
+    } else {
+      saved = api.annotation.update(
+        { id: annotation.id },
+        annotationWithChanges
+      );
+    }
+
+    const savedAnnotation = await saved;
+
+    Object.keys(annotation).forEach(key => {
+      if (key[0] === '$') {
+        savedAnnotation[key] = annotation[key];
+      }
+    });
+
+    // Clear out any pending changes (draft)
+    store.removeDraft(annotation);
+
+    // Add (or, in effect, update) the annotation to the store's collection
+    store.addAnnotations([savedAnnotation]);
+    return savedAnnotation;
+  }
+
   return {
     load,
+    save,
   };
 }

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

 import EventEmitter from 'tiny-emitter';
 
+import * as fixtures from '../../test/annotation-fixtures';
+
 import annotationsService from '../annotations';
 import { $imports } from '../annotations';
 
@@ -33,6 +35,7 @@ describe('annotationService', () => {
   let fakeStore;
   let fakeApi;
   let fakeAnnotationMapper;
+  let fakeIsNew;
   let fakeStreamer;
   let fakeStreamFilter;
 
@@ -48,17 +51,25 @@ describe('annotationService', () => {
       unloadAnnotations: sinon.stub(),
     };
     fakeApi = {
+      annotation: {
+        create: sinon.stub().resolves(fixtures.defaultAnnotation()),
+        update: sinon.stub().resolves(fixtures.defaultAnnotation()),
+      },
       search: sinon.stub(),
     };
+    fakeIsNew = sinon.stub().returns(true);
     fakeStore = {
-      getState: sinon.stub(),
+      addAnnotations: sinon.stub(),
+      annotationFetchFinished: sinon.stub(),
+      annotationFetchStarted: sinon.stub(),
       frames: sinon.stub(),
+      getDraft: sinon.stub().returns(null),
+      getState: sinon.stub(),
+      hasSelectedAnnotations: sinon.stub(),
+      removeDraft: sinon.stub(),
       searchUris: sinon.stub(),
       savedAnnotations: sinon.stub(),
-      hasSelectedAnnotations: sinon.stub(),
       updateFrameAnnotationFetchStatus: sinon.stub(),
-      annotationFetchStarted: sinon.stub(),
-      annotationFetchFinished: sinon.stub(),
     };
     fakeStreamer = {
       setConfig: sinon.stub(),
@@ -76,6 +87,7 @@ describe('annotationService', () => {
 
     $imports.$mock({
       '../search-client': FakeSearchClient,
+      '../util/annotation-metadata': { isNew: fakeIsNew },
     });
   });
 
@@ -265,4 +277,119 @@ describe('annotationService', () => {
       assert.calledWith(console.error, error);
     });
   });
+
+  describe('save', () => {
+    let svc;
+
+    beforeEach(() => {
+      svc = service();
+    });
+
+    it('calls the `create` API service for new annotations', () => {
+      fakeIsNew.returns(true);
+      // Using the new-annotation fixture has no bearing on which API method
+      // will get called because `isNew` is mocked, but it has representative
+      // properties
+      const annotation = fixtures.newAnnotation();
+      return svc.save(annotation).then(() => {
+        assert.calledOnce(fakeApi.annotation.create);
+        assert.notCalled(fakeApi.annotation.update);
+      });
+    });
+
+    it('calls the `update` API service for pre-existing annotations', () => {
+      fakeIsNew.returns(false);
+
+      const annotation = fixtures.defaultAnnotation();
+      return svc.save(annotation).then(() => {
+        assert.calledOnce(fakeApi.annotation.update);
+        assert.notCalled(fakeApi.annotation.create);
+      });
+    });
+
+    it('calls the relevant API service with an object that has any draft changes integrated', () => {
+      const annotation = fixtures.defaultAnnotation();
+      annotation.text = 'not this';
+      annotation.tags = ['nope'];
+
+      fakeStore.getDraft.returns({
+        tags: ['one', 'two'],
+        text: 'my text',
+        isPrivate: true,
+        annotation: fixtures.defaultAnnotation(),
+      });
+
+      return svc.save(fixtures.defaultAnnotation()).then(() => {
+        const annotationWithChanges = fakeApi.annotation.create.getCall(0)
+          .args[1];
+        assert.equal(annotationWithChanges.text, 'my text');
+        assert.sameMembers(annotationWithChanges.tags, ['one', 'two']);
+        // Permissions converted to "private"
+        assert.include(
+          annotationWithChanges.permissions.read,
+          fixtures.defaultAnnotation().user
+        );
+        assert.notInclude(annotationWithChanges.permissions.read, [
+          'group:__world__',
+        ]);
+      });
+    });
+
+    context('successful save', () => {
+      it('copies over internal app-specific keys to the annotation object', () => {
+        fakeIsNew.returns(false);
+        const annotation = fixtures.defaultAnnotation();
+        annotation.$tag = 'mytag';
+        annotation.$foo = 'bar';
+
+        // The fixture here has no `$`-prefixed props
+        fakeApi.annotation.update.resolves(fixtures.defaultAnnotation());
+
+        return svc.save(annotation).then(() => {
+          const savedAnnotation = fakeStore.addAnnotations.getCall(0)
+            .args[0][0];
+          assert.equal(savedAnnotation.$tag, 'mytag');
+          assert.equal(savedAnnotation.$foo, 'bar');
+        });
+      });
+
+      it('removes the annotation draft', () => {
+        const annotation = fixtures.defaultAnnotation();
+
+        return svc.save(annotation).then(() => {
+          assert.calledWith(fakeStore.removeDraft, annotation);
+        });
+      });
+
+      it('adds the updated annotation to the store', () => {
+        const annotation = fixtures.defaultAnnotation();
+        fakeIsNew.returns(false);
+        fakeApi.annotation.update.resolves(annotation);
+
+        return svc.save(annotation).then(() => {
+          assert.calledWith(fakeStore.addAnnotations, [annotation]);
+        });
+      });
+    });
+
+    context('error on save', () => {
+      it('does not remove the annotation draft', () => {
+        fakeApi.annotation.update.rejects();
+        fakeIsNew.returns(false);
+
+        return svc.save(fixtures.defaultAnnotation()).catch(() => {
+          assert.notCalled(fakeStore.removeDraft);
+        });
+      });
+
+      it('does not add the annotation to the store', () => {
+        fakeApi.annotation.update.rejects();
+        fakeIsNew.returns(false);
+
+        return svc.save(fixtures.defaultAnnotation()).catch(() => {
+          assert.notCalled(fakeStore.addAnnotations);
+        });
+      });
+    });
+  });
 });