Clear interventions after they've been displayed (#5341)

* Add a clearIntervention action creator Add a CLEAR_INTERVENTION action to clear any stored interventions. Don't clear interventions on next subject. Update tests. * Add onUnmount to Intervention Add an onUnmount callback to the Intervention component, and call it on component cleanup. Memo-ise Intervention to prevent unnecessary renders. Update tests. * Clear interventions on unmount Call the clearIntervention action when the Intervention component unmounts.
zooniverse · May 16, 2019 · 9461eb8 · 9461eb8
1 parent 33eb94a
commit 9461eb8
Show file tree

Hide file tree

Showing 5 changed files with 69 additions and 13 deletions.
diff --git a/app/classifier/classifier.jsx b/app/classifier/classifier.jsx
@@ -370,6 +370,7 @@ class Classifier extends React.Component {
             {showIntervention &&
               <Intervention
                 intervention={intervention}
+                onUnmount={actions.classify.clearIntervention}
                 user={user}
               />
             }

diff --git a/app/classifier/components/Intervention/Intervention.jsx b/app/classifier/components/Intervention/Intervention.jsx
@@ -1,4 +1,4 @@
-import React from 'react';
+import React, { memo, useEffect } from 'react';
 import PropTypes from 'prop-types';
 import counterpart from 'counterpart';
 import styled from 'styled-components';
@@ -14,10 +14,15 @@ const StyledInterventionMessage = styled.div`
   }
 `;
 
-function Intervention({ intervention, user }) {
+function Intervention({ onUnmount, intervention, user }) {
   const { message } = intervention;
   const checkbox = React.createRef();
 
+  useEffect(() => {
+    // the return value of an effect will be called to clean up after the component
+    return onUnmount;
+  });
+
   function onChange() {
     // Invert the checked value because true means do not send me messages.
     user
@@ -40,13 +45,19 @@ function Intervention({ intervention, user }) {
   );
 }
 
+Intervention.defaultProps = {
+  onUnmount: () => true
+};
+
 Intervention.propTypes = {
   intervention: PropTypes.shape({
       message: PropTypes.string
     }).isRequired,
+    onUnmount: PropTypes.func,
   user: PropTypes.shape({
     intervention_notifications: PropTypes.bool
   }).isRequired
 };
 
-export default Intervention;
+export default memo(Intervention);
+export { Intervention }
diff --git a/app/classifier/components/Intervention/Intervention.spec.js b/app/classifier/components/Intervention/Intervention.spec.js
@@ -2,7 +2,7 @@ import React from 'react';
 import { mount } from 'enzyme';
 import { expect } from 'chai';
 import sinon from 'sinon';
-import Intervention from './Intervention';
+import { Intervention } from './Intervention';
 import { Markdown } from 'markdownz';
 
 describe('Intervention', function () {
@@ -15,10 +15,13 @@ describe('Intervention', function () {
       return { save: () => true };
     })
   };
+  const onUnmount = sinon.stub()
+
   before(function () {
     wrapper = mount(
       <Intervention
         intervention={intervention}
+        onUnmount={onUnmount}
         user={user}
       />);
   });
@@ -46,4 +49,14 @@ describe('Intervention', function () {
       expect(user.update.calledWith(changes)).to.be.true;
     })
   });
+
+  describe('on unmount', function () {
+    before(function () {
+      wrapper.unmount()
+    });
+
+    it('should call onUnmount', function () {
+      expect(onUnmount).to.have.been.calledOnce
+    })
+  })
 });
diff --git a/app/redux/ducks/classify.js b/app/redux/ducks/classify.js
@@ -122,6 +122,7 @@ const initialState = {
 };
 
 const ADD_INTERVENTION = 'pfe/classify/ADD_INTERVENTION';
+const CLEAR_INTERVENTION = 'pfe/classify/CLEAR_INTERVENTION';
 const APPEND_SUBJECTS = 'pfe/classify/APPEND_SUBJECTS';
 const FETCH_SUBJECTS = 'pfe/classify/FETCH_SUBJECTS';
 const PREPEND_SUBJECTS = 'pfe/classify/PREPEND_SUBJECTS';
@@ -147,6 +148,10 @@ export default function reducer(state = initialState, action = {}) {
       }
       return state;
     }
+    case CLEAR_INTERVENTION: {
+      const intervention = null;
+      return Object.assign({}, state, { intervention });
+    }
     case APPEND_SUBJECTS: {
       const { subjects, workflowID } = action.payload;
       const { workflow } = state;
@@ -190,12 +195,10 @@ export default function reducer(state = initialState, action = {}) {
       const subject = upcomingSubjects[0];
       if (subject) {
         const classification = createNewClassification(project, workflow, subject, goldStandardMode);
-        const intervention = null;
-        return Object.assign({}, state, { classification, intervention, upcomingSubjects });
+        return Object.assign({}, state, { classification, upcomingSubjects });
       }
       return Object.assign({}, state, {
         classification: null,
-        intervention: null,
         upcomingSubjects: []
       });
     }
@@ -260,6 +263,12 @@ export function addIntervention(data) {
   };
 }
 
+export function clearIntervention() {
+  return {
+    type: CLEAR_INTERVENTION
+  };
+}
+
 export function appendSubjects(subjects, workflowID) {
   return {
     type: APPEND_SUBJECTS,

diff --git a/app/redux/ducks/classify.spec.js b/app/redux/ducks/classify.spec.js
@@ -51,6 +51,22 @@ describe('Classifier actions', function () {
       });
     });
   });
+
+  describe('clear intervention', function () {
+    const action = {
+      type: 'pfe/classify/CLEAR_INTERVENTION',
+    };
+    const state = {
+      intervention: {
+        message: 'this is an intervention'
+      }
+    };
+    it('should clear intervention messages', function () {
+      const newState = reducer(state, action);
+      expect(newState.intervention).to.be.null;
+    });
+  });
+
   describe('append subjects', function () {
     const subjects = [
         mockSubject('3'),
@@ -157,7 +173,10 @@ describe('Classifier actions', function () {
       const state = {
         classification: { id: '1' },
         workflow: { id: '1' },
-        upcomingSubjects: [subjects[0]]
+        upcomingSubjects: [subjects[0]],
+        intervention: {
+          message: 'This is a test intervention'
+        }
       };
       it('should empty the queue', function () {
         const newState = reducer(state, action);
@@ -167,15 +186,18 @@ describe('Classifier actions', function () {
         const newState = reducer(state, action);
         expect(newState.classification).to.be.null;
       });
-      it('should clear any stored interventions', function () {
+      it('should not clear any stored interventions', function () {
         const newState = reducer(state, action);
-        expect(newState.intervention).to.be.null;
+        expect(newState.intervention).to.eql(state.intervention);
       });
     });
     describe('with multiple subjects in the queue', function () {
       const state = {
         workflow: { id: '1' },
-        upcomingSubjects: subjects
+        upcomingSubjects: subjects,
+        intervention: {
+          message: 'This is a test intervention'
+        }
       };
       it('should shift the first subject off the queue', function () {
         const newState = reducer(state, action);
@@ -185,9 +207,9 @@ describe('Classifier actions', function () {
         const newState = reducer(state, action);
         expect(newState.classification.links.subjects).to.deep.equal(['2']);
       });
-      it('should clear any stored interventions', function () {
+      it('should not clear any stored interventions', function () {
         const newState = reducer(state, action);
-        expect(newState.intervention).to.be.null;
+        expect(newState.intervention).to.eql(state.intervention);
       });
     });
   });