Merge pull request #1651 from hypothesis/delete-flag-annotation-actions

Move `delete` and `flag` logic into `AnnotationActionBar` sub-component

src/sidebar/components/annotation-action-bar.js

@@ -2,42 +2,72 @@ const propTypes = require('prop-types');
 const { createElement } = require('preact');
 
 const { withServices } = require('../util/service-context');
+const useStore = require('../store/use-store');
 const { isShareable, shareURI } = require('../util/annotation-sharing');
 
 const AnnotationShareControl = require('./annotation-share-control');
 const Button = require('./button');
 
 /**
- * A collection of `Button`s in the footer area of an annotation.
+ * A collection of `Button`s in the footer area of an annotation that take
+ * actions on the annotation.
  */
 function AnnotationActionBar({
   annotation,
-  onDelete,
+  annotationMapper,
+  flash,
   onEdit,
-  onFlag,
   onReply,
-  groups,
   permissions,
-  session,
   settings,
 }) {
+  const userProfile = useStore(store => store.profile());
+  const annotationGroup = useStore(store => store.getGroup(annotation.group));
+
   // Is the current user allowed to take the given `action` on this annotation?
   const userIsAuthorizedTo = action => {
     return permissions.permits(
       annotation.permissions,
       action,
-      session.state.userid
+      userProfile.userid
     );
   };
 
   const showDeleteAction = userIsAuthorizedTo('delete');
   const showEditAction = userIsAuthorizedTo('update');
+
   // Anyone may flag an annotation except the annotation's author.
   // This option is even presented to anonymous users
-  const showFlagAction = session.state.userid !== annotation.user;
+  const showFlagAction = userProfile.userid !== annotation.user;
   const showShareAction = isShareable(annotation, settings);
 
-  const annotationGroup = groups.get(annotation.group);
+  const updateFlagFn = useStore(store => store.updateFlagStatus);
+
+  const updateFlag = () => {
+    updateFlagFn(annotation.id, true);
+  };
+
+  const onDelete = () => {
+    if (window.confirm('Are you sure you want to delete this annotation?')) {
+      annotationMapper.deleteAnnotation(annotation).catch(err => {
+        flash.error(err.message, 'Deleting annotation failed');
+      });
+    }
+  };
+
+  const onFlag = () => {
+    if (!userProfile.userid) {
+      flash.error(
+        'You must be logged in to report an annotation to moderators.',
+        'Log in to flag annotations'
+      );
+      return;
+    }
+    annotationMapper
+      .flagAnnotation(annotation) // Flag annotation on service
+      .then(updateFlag) // Update app state with flag
+      .catch(err => flash.error(err.message, 'Flagging annotation failed'));
+  };
 
   return (
     <div className="annotation-action-bar">
@@ -75,21 +105,19 @@ AnnotationActionBar.propTypes = {
   annotation: propTypes.object.isRequired,
   /** Callbacks for when action buttons are clicked/tapped */
   onEdit: propTypes.func.isRequired,
-  onDelete: propTypes.func.isRequired,
-  onFlag: propTypes.func.isRequired,
   onReply: propTypes.func.isRequired,
 
   // Injected services
-  groups: propTypes.object.isRequired,
+  annotationMapper: propTypes.object.isRequired,
+  flash: propTypes.object.isRequired,
   permissions: propTypes.object.isRequired,
-  session: propTypes.object.isRequired,
   settings: propTypes.object.isRequired,
 };
 
 AnnotationActionBar.injectedProps = [
-  'groups',
+  'annotationMapper',
+  'flash',
   'permissions',
-  'session',
   'settings',
 ];
 

src/sidebar/components/annotation.js

@@ -173,49 +173,6 @@ function AnnotationController(
     }
   }
 
-  /**
-   * @ngdoc method
-   * @name annotation.AnnotationController#flag
-   * @description Flag the annotation.
-   */
-  this.flag = function() {
-    if (!session.state.userid) {
-      flash.error(
-        'You must be logged in to report an annotation to the moderators.',
-        'Login to flag annotations'
-      );
-      return;
-    }
-
-    const onRejected = function(err) {
-      flash.error(err.message, 'Flagging annotation failed');
-    };
-    annotationMapper.flagAnnotation(self.annotation).then(function() {
-      store.updateFlagStatus(self.annotation.id, true);
-    }, onRejected);
-  };
-
-  /**
-   * @ngdoc method
-   * @name annotation.AnnotationController#delete
-   * @description Deletes the annotation.
-   */
-  this.delete = function() {
-    return $timeout(function() {
-      // Don't use confirm inside the digest cycle.
-      const msg = 'Are you sure you want to delete this annotation?';
-      if ($window.confirm(msg)) {
-        $scope.$apply(function() {
-          annotationMapper
-            .deleteAnnotation(self.annotation)
-            .catch(err =>
-              flash.error(err.message, 'Deleting annotation failed')
-            );
-        });
-      }
-    }, true);
-  };
-
   /**
    * @ngdoc method
    * @name annotation.AnnotationController#edit

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

 const { createElement } = require('preact');
 const { mount } = require('enzyme');
+const { act } = require('preact/test-utils');
+
+const { waitFor } = require('./util');
 
 const AnnotationActionBar = require('../annotation-action-bar');
 const mockImportedComponents = require('./mock-imported-components');
 
 describe('AnnotationActionBar', () => {
   let fakeAnnotation;
-  let fakeOnDelete;
   let fakeOnEdit;
-  let fakeOnFlag;
   let fakeOnReply;
+
+  let fakeUserProfile;
+
   // Fake services
-  let fakeGroups;
+  let fakeAnnotationMapper;
+  let fakeFlash;
   let fakePermissions;
-  let fakeSession;
   let fakeSettings;
   // Fake dependencies
   let fakeIsShareable;
+  let fakeStore;
 
   function createComponent(props = {}) {
     return mount(
       <AnnotationActionBar
         annotation={fakeAnnotation}
-        isPrivate={false}
-        onDelete={fakeOnDelete}
+        annotationMapper={fakeAnnotationMapper}
+        flash={fakeFlash}
         onEdit={fakeOnEdit}
         onReply={fakeOnReply}
-        onFlag={fakeOnFlag}
-        groups={fakeGroups}
         permissions={fakePermissions}
-        session={fakeSession}
         settings={fakeSettings}
         {...props}
       />
@@ -59,21 +61,24 @@ describe('AnnotationActionBar', () => {
       permissions: {},
       user: 'acct:bar@foo.com',
     };
-    fakeSession = {
-      state: {
-        userid: 'acct:foo@bar.com',
-      },
+
+    fakeUserProfile = {
+      userid: 'account:foo@bar.com',
     };
 
-    fakeOnEdit = sinon.stub();
-    fakeOnDelete = sinon.stub();
-    fakeOnReply = sinon.stub();
-    fakeOnFlag = sinon.stub();
+    fakeAnnotationMapper = {
+      deleteAnnotation: sinon.stub().resolves(),
+      flagAnnotation: sinon.stub().resolves(),
+    };
 
-    fakeGroups = {
-      get: sinon.stub(),
+    fakeFlash = {
+      info: sinon.stub(),
+      error: sinon.stub(),
     };
 
+    fakeOnEdit = sinon.stub();
+    fakeOnReply = sinon.stub();
+
     fakePermissions = {
       permits: sinon.stub().returns(true),
     };
@@ -81,16 +86,25 @@ describe('AnnotationActionBar', () => {
 
     fakeIsShareable = sinon.stub().returns(true);
 
+    fakeStore = {
+      profile: sinon.stub().returns(fakeUserProfile),
+      getGroup: sinon.stub().returns({}),
+      updateFlagStatus: sinon.stub(),
+    };
+
     AnnotationActionBar.$imports.$mock(mockImportedComponents());
     AnnotationActionBar.$imports.$mock({
       '../util/annotation-sharing': {
         isShareable: fakeIsShareable,
         shareURI: sinon.stub().returns('http://share.me'),
       },
+      '../store/use-store': callback => callback(fakeStore),
     });
+    sinon.stub(window, 'confirm').returns(false);
   });
 
   afterEach(() => {
+    window.confirm.restore();
     AnnotationActionBar.$imports.$restore();
   });
 
@@ -128,15 +142,45 @@ describe('AnnotationActionBar', () => {
       assert.isTrue(getButton(wrapper, 'trash').exists());
     });
 
-    it('invokes `onDelete` callback when delete button clicked', () => {
+    it('asks for confirmation before deletion', () => {
       allowOnly('delete');
       const button = getButton(createComponent(), 'trash');
 
-      button.props().onClick();
+      act(() => {
+        button.props().onClick();
+      });
+
+      assert.calledOnce(confirm);
+      assert.notCalled(fakeAnnotationMapper.deleteAnnotation);
+    });
+
+    it('invokes delete on service when confirmed', () => {
+      allowOnly('delete');
+      window.confirm.returns(true);
+      const button = getButton(createComponent(), 'trash');
+
+      act(() => {
+        button.props().onClick();
+      });
+
+      assert.calledWith(fakeAnnotationMapper.deleteAnnotation, fakeAnnotation);
+    });
+
+    it('sets a flash message if there is an error with deletion', async () => {
+      allowOnly('delete');
+      window.confirm.returns(true);
+      fakeAnnotationMapper.deleteAnnotation.rejects();
+
+      const button = getButton(createComponent(), 'trash');
+      act(() => {
+        button.props().onClick();
+      });
 
-      assert.calledOnce(fakeOnDelete);
+      await waitFor(() => fakeFlash.error.called);
     });
+  });
 
+  describe('edit action button', () => {
     it('does not show edit button if permissions do not allow', () => {
       disallowOnly('delete');
 
@@ -156,7 +200,9 @@ describe('AnnotationActionBar', () => {
     it('invokes `onReply` callback when reply button clicked', () => {
       const button = getButton(createComponent(), 'reply');
 
-      button.props().onClick();
+      act(() => {
+        button.props().onClick();
+      });
 
       assert.calledOnce(fakeOnReply);
     });
@@ -184,16 +230,63 @@ describe('AnnotationActionBar', () => {
       assert.isTrue(getButton(wrapper, 'flag').exists());
     });
 
-    it('invokes `onFlag` callback when flag button clicked', () => {
+    it('sets a flash error when clicked if user is not logged in', () => {
+      fakeStore.profile.returns({
+        userid: null,
+      });
+
       const button = getButton(createComponent(), 'flag');
 
-      button.props().onClick();
+      act(() => {
+        button.props().onClick();
+      });
+
+      assert.calledOnce(fakeFlash.error);
+      assert.notCalled(fakeAnnotationMapper.flagAnnotation);
+    });
+
+    it('invokes flag on service when clicked', () => {
+      window.confirm.returns(true);
+
+      const button = getButton(createComponent(), 'flag');
+
+      act(() => {
+        button.props().onClick();
+      });
+
+      assert.calledWith(fakeAnnotationMapper.flagAnnotation, fakeAnnotation);
+    });
+
+    it('updates flag state in store after flagging on service is successful', async () => {
+      window.confirm.returns(true);
+      fakeAnnotationMapper.flagAnnotation.resolves(fakeAnnotation);
+
+      const button = getButton(createComponent(), 'flag');
+
+      act(() => {
+        button.props().onClick();
+      });
+
+      await fakeAnnotation;
+      assert.calledWith(fakeStore.updateFlagStatus, fakeAnnotation.id, true);
+    });
+
+    it('sets flash error message if flagging fails on service', async () => {
+      window.confirm.returns(true);
+      fakeAnnotationMapper.flagAnnotation.rejects();
+
+      const button = getButton(createComponent(), 'flag');
+
+      act(() => {
+        button.props().onClick();
+      });
 
-      assert.calledOnce(fakeOnFlag);
+      await waitFor(() => fakeFlash.error.called);
+      assert.notCalled(fakeStore.updateFlagStatus);
     });
 
     it('does not show flag action button if user is author', () => {
-      fakeAnnotation.user = 'acct:foo@bar.com';
+      fakeAnnotation.user = fakeUserProfile.userid;
 
       const wrapper = createComponent();
 

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

@@ -73,11 +73,8 @@ describe('annotation', function() {
   });
 
   describe('AnnotationController', function() {
-    let $q;
     let $rootScope;
     let $scope;
-    let $timeout;
-    let $window;
     const fakeAccountID = {
       isThirdPartyUser: sinon.stub(),
     };
@@ -159,8 +156,6 @@ describe('annotation', function() {
               admin: ['acct:bill@localhost'],
             },
           }),
-          deleteAnnotation: sandbox.stub(),
-          flagAnnotation: sandbox.stub(),
         };
 
         fakeStore = {
@@ -243,10 +238,7 @@ describe('annotation', function() {
       })
     );
 
-    beforeEach(inject(function(_$q_, _$rootScope_, _$timeout_, _$window_) {
-      $window = _$window_;
-      $q = _$q_;
-      $timeout = _$timeout_;
+    beforeEach(inject(function(_$rootScope_) {
       $rootScope = _$rootScope_;
       $scope = $rootScope.$new();
     }));
@@ -659,131 +651,6 @@ describe('annotation', function() {
       });
     });
 
-    describe('#delete()', function() {
-      beforeEach(function() {
-        fakeAnnotationMapper.deleteAnnotation = sandbox.stub();
-      });
-
-      it('calls annotationMapper.delete() if the delete is confirmed', function(done) {
-        const parts = createDirective();
-        sandbox.stub($window, 'confirm').returns(true);
-        fakeAnnotationMapper.deleteAnnotation.returns($q.resolve());
-        parts.controller.delete().then(function() {
-          assert.calledWith(
-            fakeAnnotationMapper.deleteAnnotation,
-            parts.annotation
-          );
-          done();
-        });
-        $timeout.flush();
-      });
-
-      it("doesn't call annotationMapper.delete() if the delete is cancelled", function(done) {
-        const parts = createDirective();
-        sandbox.stub($window, 'confirm').returns(false);
-        parts.controller.delete().then(function() {
-          assert.notCalled(fakeAnnotationMapper.deleteAnnotation);
-          done();
-        });
-        $timeout.flush();
-      });
-
-      it('flashes an error if the delete fails on the server', function(done) {
-        const controller = createDirective().controller;
-        sandbox.stub($window, 'confirm').returns(true);
-
-        fakeAnnotationMapper.deleteAnnotation = sinon.spy(() => {
-          // nb. we only instantiate the rejected promise when
-          // `deleteAnnotation` is called to avoid triggering `$q`'s unhandled
-          // promise rejection handler during the `$timeout.flush()` call.
-          return $q.reject(new Error('500 Server Error'));
-        });
-        controller.delete().then(function() {
-          assert.calledWith(
-            fakeFlash.error,
-            '500 Server Error',
-            'Deleting annotation failed'
-          );
-          done();
-        });
-        $timeout.flush();
-      });
-
-      it("doesn't flash an error if the delete succeeds", function(done) {
-        const controller = createDirective().controller;
-        sandbox.stub($window, 'confirm').returns(true);
-        fakeAnnotationMapper.deleteAnnotation.returns($q.resolve());
-        controller.delete().then(function() {
-          assert.notCalled(fakeFlash.error);
-          done();
-        });
-        $timeout.flush();
-      });
-    });
-
-    describe('#flag()', function() {
-      beforeEach(function() {
-        fakeAnnotationMapper.flagAnnotation = sandbox.stub();
-      });
-
-      context('when the user is not logged in', function() {
-        beforeEach(function() {
-          delete fakeSession.state.userid;
-        });
-
-        it('flashes an error', function() {
-          createDirective().controller.flag();
-
-          assert.isTrue(fakeFlash.error.calledOnce);
-          assert.equal('Login to flag annotations', fakeFlash.error.args[0][1]);
-        });
-
-        it("doesn't try to flag the annotation", function() {
-          createDirective().controller.flag();
-
-          assert.isFalse(fakeAnnotationMapper.flagAnnotation.called);
-        });
-      });
-
-      context('when the user is logged in', function() {
-        it('calls annotationMapper.flag() when an annotation is flagged', function(done) {
-          const parts = createDirective();
-          fakeAnnotationMapper.flagAnnotation.returns($q.resolve());
-          parts.controller.flag();
-          assert.calledWith(
-            fakeAnnotationMapper.flagAnnotation,
-            parts.annotation
-          );
-          done();
-        });
-
-        it('flashes an error if the flag fails', function(done) {
-          const controller = createDirective().controller;
-          const err = new Error('500 Server error');
-          fakeAnnotationMapper.flagAnnotation.returns(Promise.reject(err));
-          controller.flag();
-          setTimeout(function() {
-            assert.calledWith(
-              fakeFlash.error,
-              '500 Server error',
-              'Flagging annotation failed'
-            );
-            done();
-          }, 0);
-        });
-
-        it("doesn't flash an error if the flag succeeds", function(done) {
-          const controller = createDirective().controller;
-          fakeAnnotationMapper.flagAnnotation.returns($q.resolve());
-          controller.flag();
-          setTimeout(function() {
-            assert.notCalled(fakeFlash.error);
-            done();
-          }, 0);
-        });
-      });
-    });
-
     describe('#isThirdPartyUser', function() {
       it('returns whether the user is a third party user', function() {
         const { annotation, controller } = createDirective();

src/sidebar/components/test/util.js

+/**
+ * Wait for a condition to evaluate to a truthy value.
+ *
+ * @param {() => any} condition - Function that returns a truthy value when some condition is met
+ * @param {number} timeout - Max delay in milliseconds to wait
+ * @param {string} what - Description of condition that is being waited for
+ * @return {Promise<any>} - Result of the `condition` function
+ */
+export async function waitFor(
+  condition,
+  timeout = 10,
+  what = condition.toString()
+) {
+  const result = condition();
+  if (result) {
+    return result;
+  }
+
+  const start = Date.now();
+
+  return new Promise((resolve, reject) => {
+    const timer = setInterval(() => {
+      const result = condition();
+      if (result) {
+        clearTimeout(timer);
+        resolve(result);
+      }
+      if (Date.now() - start > timeout) {
+        clearTimeout(timer);
+        reject(new Error(`waitFor(${what}) failed after ${timeout} ms`));
+      }
+    });
+  });
+}

src/sidebar/templates/annotation.html

@@ -80,8 +80,6 @@
     <div class="annotation-actions" ng-if="!vm.isSaving && !vm.editing() && vm.id()">
       <annotation-action-bar
         annotation="vm.annotation"
-        on-delete="vm.delete()"
-        on-flag="vm.flag()"
         on-edit="vm.edit()"
         on-reply="vm.reply()"></annotation-action-bar>
     </div>