Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clear interventions after they've been displayed #5341

Merged
merged 3 commits into from
May 16, 2019
Merged

Clear interventions after they've been displayed #5341

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
faad76b
Add a clearIntervention action creator
eatyourgreens May 15, 2019
8089a65
Add onUnmount to Intervention
eatyourgreens May 15, 2019
3d7f3ed
Clear interventions on unmount
eatyourgreens May 15, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions app/classifier/classifier.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -370,6 +370,7 @@ class Classifier extends React.Component {
{showIntervention &&
<Intervention
intervention={intervention}
onUnmount={actions.classify.clearIntervention}
user={user}
/>
}
Expand Down
17 changes: 14 additions & 3 deletions app/classifier/components/Intervention/Intervention.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My reading suggests this will only run on unmount but it looks like effects will cleanup up if props change as well - https://reactjs.org/docs/hooks-effect.html#explanation-why-effects-run-on-each-update

I don't think this is an issue as we don't pass props to this function, thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. The props here are onUnmount, intervention, user and we probably do want cleanup to run if any of those change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be useful.

If you want to run an effect and clean it up only once (on mount and unmount), you can pass an empty array ([]) as a second argument. This tells React that your effect doesn’t depend on any values from props or state, so it never needs to re-run. This isn’t handled as a special case — it follows directly from how the dependencies array always works.
https://reactjs.org/docs/hooks-effect.html#tip-optimizing-performance-by-skipping-effects

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been playing around with this a bit and I think the only time it might happen is if the intervention prop changes while the component is being displayed. Is there a case where I might get a new intervention message while I'm already reading one?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes - though in practice it would be slim. They don't send many messages but I could receive one while i'm viewing one, especially if the network / gateway has issues, i.e. received message at gateway, send to sugar but timeout to client who retries.

Would the current code unmount and then update with the newly received message? Should we drop incoming messages until there are no interventions in the store?

I'd say no to switching the store to a list of messages, unless the incoming message has a validity period attached to it, zooniverse/interventions-gateway#22

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My reading of the code is the classifier would receive a new intervention prop, which it would pass down to the Intervention component, which would then run useEffect() because of the props change? I'm not sure what would happen then: maybe the new intervention would render briefly, then be cleared by the cleanup code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to add the validity period to the incoming messages via the gateway API if it helps simplify this issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could use the second argument to useEffect to ignore changes in the intervention prop. I'm not sure what the implications of that are, but it sounds like a case that shouldn't occur anyway.

At the moment, if a second message were received while you're currently reading one, it would overwrite it and the screen would update to show the new message.

});

function onChange() {
// Invert the checked value because true means do not send me messages.
user
Expand All @@ -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);
camallen marked this conversation as resolved.
Show resolved Hide resolved
export { Intervention }
15 changes: 14 additions & 1 deletion app/classifier/components/Intervention/Intervention.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 () {
Expand All @@ -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}
/>);
});
Expand Down Expand Up @@ -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
})
})
});
15 changes: 12 additions & 3 deletions app/redux/ducks/classify.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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;
Expand Down Expand Up @@ -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: []
});
}
Expand Down Expand Up @@ -260,6 +263,12 @@ export function addIntervention(data) {
};
}

export function clearIntervention() {
return {
type: CLEAR_INTERVENTION
};
}

export function appendSubjects(subjects, workflowID) {
return {
type: APPEND_SUBJECTS,
Expand Down
34 changes: 28 additions & 6 deletions app/redux/ducks/classify.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'),
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand All @@ -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);
});
});
});
Expand Down