Skip to content

Commit

Permalink
Simplify classifier effect hooks (#3873)
Browse files Browse the repository at this point in the history 
Remove useEffect hooks that could be replaced by if statements. Make sure that the project is stored before workflow selection runs.

Remove the `project` prop from the `Classifier` component. Use the active project instead.
  • Loading branch information
@eatyourgreens
eatyourgreens authored Nov 28, 2022
1 parent 2dff63c commit 9a89c35
Show file tree
Hide file tree
Showing 4 changed files with 90 additions and 54 deletions.
39 changes: 14 additions & 25 deletions packages/lib-classifier/src/components/Classifier/Classifier.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,28 +14,40 @@ export default function Classifier({
classifierStore,
locale,
onError = () => true,
project,
showTutorial = false,
subjectID,
subjectSetID,
workflowSnapshot,
}) {

const project = classifierStore.projects.active
const workflowID = workflowSnapshot?.id
const workflowStrings = workflowSnapshot?.strings
const user = usePanoptesUser()
const projectRoles = useProjectRoles(project?.id, user?.id)
let workflowVersionChanged = false

if (workflowSnapshot) {
const storedWorkflow = classifierStore.workflows.resources.get(workflowSnapshot.id)
const { workflows } = classifierStore
const storedWorkflow = workflows.resources.get(workflowSnapshot.id)
workflowVersionChanged = workflowSnapshot.version !== storedWorkflow?.version
/*
Merge the new snapshot into the existing workflow,
to preserve properties, such as workflow.subjectSet,
that aren't in the Panoptes data.
*/
workflowSnapshot = storedWorkflow ? { ...getSnapshot(storedWorkflow), ...workflowSnapshot } : workflowSnapshot
/*
This should run when a project owner edits a workflow, but not when a workflow updates
as a result of receiving classifications eg. workflow.completeness.
It refreshes the stored workflow then resets any classifications in progress.
*/
if (workflowVersionChanged) {
console.log('Refreshing workflow snapshot', workflowSnapshot.id)
workflows.setResources([workflowSnapshot])
// TODO: the task area crashes without the following line. Why is that?
classifierStore.startClassification()
}
}

const upp = useProjectPreferences(project?.id, user?.id)
Expand Down Expand Up @@ -69,12 +81,6 @@ export default function Classifier({
}
}, [locale])

useEffect(function onProjectChange() {
const { projects } = classifierStore
projects.setResources([project])
projects.setActive(project.id)
}, [project.id])

useEffect(function onURLChange() {
const { workflows } = classifierStore
if (workflowID) {
Expand All @@ -92,20 +98,6 @@ export default function Classifier({
applySnapshot(workflow.strings, workflowStrings)
}
}, [workflowID, workflowStrings])
/*
This should run when a project owner edits a workflow, but not when a workflow updates
as a result of receiving classifications eg. workflow.completeness.
It refreshes the stored workflow then resets any classifications in progress.
*/
useEffect(function onWorkflowVersionChange() {
const { workflows } = classifierStore
if (workflowVersionChanged) {
console.log('Refreshing workflow snapshot', workflowSnapshot.id)
workflows.setResources([workflowSnapshot])
// TODO: the task area crashes without the following line. Why is that?
classifierStore.startClassification()
}
}, [workflowVersionChanged])

try {
return (
Expand All @@ -128,9 +120,6 @@ Classifier.propTypes = {
classifierStore: PropTypes.object.isRequired,
locale: PropTypes.string,
onError: PropTypes.func,
project: PropTypes.shape({
id: PropTypes.string.isRequired
}).isRequired,
showTutorial: PropTypes.bool,
subjectSetID: PropTypes.string,
subjectID: PropTypes.string,
Expand Down
82 changes: 56 additions & 26 deletions packages/lib-classifier/src/components/Classifier/Classifier.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,14 +71,12 @@ describe('Components > Classifier', function () {
const subject = SubjectFactory.build({ locations: [{ 'image/png': 'https://foo.bar/example.png' }] })
const store = mockStore({ subject })
const project = store.projects.active
const projectSnapshot = { ...getSnapshot(project) }
workflow = store.workflows.active
const workflowSnapshot = { ...getSnapshot(workflow) }
workflowSnapshot.strings = workflowStrings
render(
<Classifier
classifierStore={store}
project={projectSnapshot}
workflowSnapshot={workflowSnapshot}
/>,
{
Expand Down Expand Up @@ -137,14 +135,12 @@ describe('Components > Classifier', function () {
const subject = SubjectFactory.build({ locations: [{ 'image/png': 'https://foo.bar/example.png' }] })
const store = mockStore({ subject })
const project = store.projects.active
const projectSnapshot = { ...getSnapshot(project) }
workflow = store.workflows.active
const workflowSnapshot = { ...getSnapshot(workflow) }
workflowSnapshot.strings = workflowStrings
render(
<Classifier
classifierStore={store}
project={projectSnapshot}
workflowSnapshot={workflowSnapshot}
/>,
{
Expand Down Expand Up @@ -227,13 +223,19 @@ describe('Components > Classifier', function () {
}
})

const store = RootStore.create({}, { authClient, client })
const store = RootStore.create({
projects: {
active: projectSnapshot.id,
resources: {
[projectSnapshot.id]: projectSnapshot
}
}
}, { authClient, client })

const { rerender } = render(
<Classifier
classifierStore={store}
locale='en'
project={projectSnapshot}
workflowSnapshot={workflowSnapshot}
/>,
{
Expand All @@ -248,7 +250,6 @@ describe('Components > Classifier', function () {
<Classifier
classifierStore={store}
locale='fr'
project={projectSnapshot}
workflowSnapshot={workflowSnapshot}
/>,
)
Expand All @@ -263,7 +264,6 @@ describe('Components > Classifier', function () {
<Classifier
classifierStore={store}
locale='fr'
project={projectSnapshot}
workflowSnapshot={frenchSnapshot}
/>,
)
Expand Down Expand Up @@ -373,11 +373,17 @@ describe('Components > Classifier', function () {
const checkCurrent = sinon.stub().callsFake(() => Promise.resolve({ id: 123, login: 'mockUser' }))
const authClient = { ...defaultAuthClient, checkBearerToken, checkCurrent }
const client = { ...defaultClient, panoptes }
const store = RootStore.create({}, { authClient, client })
const store = RootStore.create({
projects: {
active: projectSnapshot.id,
resources: {
[projectSnapshot.id]: projectSnapshot
}
}
}, { authClient, client })
const { rerender } = render(
<Classifier
classifierStore={store}
project={projectSnapshot}
workflowSnapshot={workflowSnapshot}
/>,
{
Expand All @@ -404,7 +410,6 @@ describe('Components > Classifier', function () {
rerender(
<Classifier
classifierStore={store}
project={projectSnapshot}
workflowSnapshot={newSnapshot}
/>,
{
Expand Down Expand Up @@ -508,11 +513,17 @@ describe('Components > Classifier', function () {
const checkCurrent = sinon.stub().callsFake(() => Promise.resolve({ id: 123, login: 'mockUser' }))
const authClient = { ...defaultAuthClient, checkBearerToken, checkCurrent }
const client = { ...defaultClient, panoptes }
const store = RootStore.create({}, { authClient, client })
const store = RootStore.create({
projects: {
active: projectSnapshot.id,
resources: {
[projectSnapshot.id]: projectSnapshot
}
}
}, { authClient, client })
render(
<Classifier
classifierStore={store}
project={projectSnapshot}
workflowSnapshot={workflowSnapshot}
/>,
{
Expand Down Expand Up @@ -593,11 +604,17 @@ describe('Components > Classifier', function () {
const checkCurrent = sinon.stub().callsFake(() => Promise.resolve({ id: 123, login: 'mockUser' }))
const authClient = { ...defaultAuthClient, checkBearerToken, checkCurrent }
const client = { ...defaultClient, panoptes }
const store = RootStore.create({}, { authClient, client })
const store = RootStore.create({
projects: {
active: projectSnapshot.id,
resources: {
[projectSnapshot.id]: projectSnapshot
}
}
}, { authClient, client })
render(
<Classifier
classifierStore={store}
project={projectSnapshot}
workflowSnapshot={workflowSnapshot}
/>,
{
Expand Down Expand Up @@ -654,14 +671,12 @@ describe('Components > Classifier', function () {
const store = mockStore({ subject })
store.tutorials.setResources([workflowTutorial])
store.tutorials.setActive(workflowTutorial.id)
const projectSnapshot = { ...getSnapshot(store.projects.active) }
const workflowSnapshot = { ...getSnapshot(store.workflows.active) }
workflowSnapshot.strings = workflowStrings
render(
<Classifier
classifierStore={store}
locale='en'
project={projectSnapshot}
showTutorial
workflowSnapshot={workflowSnapshot}
/>,
Expand Down Expand Up @@ -695,14 +710,12 @@ describe('Components > Classifier', function () {
const store = mockStore({ subject })
store.tutorials.setResources([workflowTutorial])
store.tutorials.setActive(workflowTutorial.id)
const projectSnapshot = { ...getSnapshot(store.projects.active) }
const workflowSnapshot = { ...getSnapshot(store.workflows.active) }
workflowSnapshot.strings = workflowStrings
render(
<Classifier
classifierStore={store}
locale='en'
project={projectSnapshot}
workflowSnapshot={workflowSnapshot}
/>,
{
Expand Down Expand Up @@ -761,11 +774,17 @@ describe('Components > Classifier', function () {
const checkCurrent = sinon.stub().callsFake(() => Promise.resolve({ id: 123, login: 'mockUser' }))
const authClient = { ...defaultAuthClient, checkBearerToken, checkCurrent }
const client = { ...defaultClient, panoptes }
const store = RootStore.create({}, { authClient, client })
const store = RootStore.create({
projects: {
active: projectSnapshot.id,
resources: {
[projectSnapshot.id]: projectSnapshot
}
}
}, { authClient, client })
const { rerender } = render(
<Classifier
classifierStore={store}
project={projectSnapshot}
subjectSetID='1'
workflowSnapshot={workflowSnapshot}
/>,
Expand All @@ -777,7 +796,6 @@ describe('Components > Classifier', function () {
rerender(
<Classifier
classifierStore={store}
project={projectSnapshot}
subjectSetID='2'
workflowSnapshot={workflowSnapshot}
/>,
Expand Down Expand Up @@ -859,12 +877,18 @@ describe('Components > Classifier', function () {
const checkCurrent = sinon.stub().callsFake(() => Promise.resolve({ id: 123, login: 'mockAdmin', admin: true }))
const authClient = { ...defaultAuthClient, checkBearerToken, checkCurrent }
const client = { ...defaultClient, panoptes }
const store = RootStore.create({}, { authClient, client })
const store = RootStore.create({
projects: {
active: projectSnapshot.id,
resources: {
[projectSnapshot.id]: projectSnapshot
}
}
}, { authClient, client })
render(
<Classifier
adminMode
classifierStore={store}
project={projectSnapshot}
workflowSnapshot={workflowSnapshot}
/>,
{
Expand Down Expand Up @@ -923,11 +947,17 @@ describe('Components > Classifier', function () {
const checkCurrent = sinon.stub().callsFake(() => Promise.resolve({ id: 123, login: 'mockAdmin', admin: true }))
const authClient = { ...defaultAuthClient, checkBearerToken, checkCurrent }
const client = { ...defaultClient, panoptes }
const store = RootStore.create({}, { authClient, client })
const store = RootStore.create({
projects: {
active: projectSnapshot.id,
resources: {
[projectSnapshot.id]: projectSnapshot
}
}
}, { authClient, client })
render(
<Classifier
classifierStore={store}
project={projectSnapshot}
workflowSnapshot={workflowSnapshot}
/>,
{
Expand Down
12 changes: 9 additions & 3 deletions packages/lib-classifier/src/components/Classifier/Classifier.workflows.spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import Classifier from './Classifier'

describe('Classifier > workflow types', function () {
// this turns off Mocha's time limit for slow tests
this.timeout(0)
this.timeout(5000)

const cases = [
{
Expand Down Expand Up @@ -111,11 +111,17 @@ function testWorkflow(workflowSnapshot, workflowStrings) {
const checkCurrent = sinon.stub().callsFake(() => Promise.resolve({ id: 123, login: 'mockUser' }))
const authClient = { ...defaultAuthClient, checkBearerToken, checkCurrent }
const client = { ...defaultClient, panoptes }
const store = RootStore.create({}, { authClient, client })
const store = RootStore.create({
projects: {
active: projectSnapshot.id,
resources: {
[projectSnapshot.id]: projectSnapshot
}
}
}, { authClient, client })
render(
<Classifier
classifierStore={store}
project={projectSnapshot}
workflowSnapshot={workflowSnapshot}
/>,
{
Expand Down
11 changes: 11 additions & 0 deletions packages/lib-classifier/src/components/Classifier/ClassifierContainer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,17 @@ export default function ClassifierContainer({

const classifierStore = useHydratedStore(storeEnvironment, cachePanoptesData, `fem-classifier-${project.id}`)

if (project?.id) {
const storedProject = classifierStore.projects.active
const projectChanged = project.id !== storedProject?.id

if (projectChanged) {
const { projects } = classifierStore
projects.setResources([project])
projects.setActive(project.id)
}
}

useEffect(function onMount() {
/*
This should run after the store is created and hydrated.
Expand Down

0 comments on commit 9a89c35

Please sign in to comment.