From fe76786423bf5abc821982a28432b3c8b37d6117 Mon Sep 17 00:00:00 2001 From: Jim O'Donnell Date: Sat, 17 Aug 2024 11:20:01 +0100 Subject: [PATCH 1/8] fix(app-project): data-fetching for assigned workflow level Fix missing dependency errors in the `useAssignedLevel` hook by using `useSWR` to fetch and cache the assigned workflow level. SWR also adds revalidation for cases where I might classify in the same tab over several days or weeks. Also fixes the workflow assignment tests in `ClassifyPageContainer` so that they test against the correct levels. --- .../app-project/src/hooks/useAssignedLevel.js | 51 ++++++++++--------- .../ClassifyPage/ClassifyPageConnector.js | 7 ++- .../ClassifyPage/ClassifyPageContainer.js | 4 +- .../ClassifyPageContainer.spec.js | 4 +- 4 files changed, 34 insertions(+), 32 deletions(-) diff --git a/packages/app-project/src/hooks/useAssignedLevel.js b/packages/app-project/src/hooks/useAssignedLevel.js index fd39e166ab..ad8ac44864 100644 --- a/packages/app-project/src/hooks/useAssignedLevel.js +++ b/packages/app-project/src/hooks/useAssignedLevel.js @@ -6,36 +6,37 @@ import { useEffect, useState } from 'react' import { panoptes } from '@zooniverse/panoptes-js' +import useSWR from 'swr' -function useAssignedLevel(assignedWorkflowID) { - const [assignedWorkflowLevel, setAssignedWorkflowLevel] = useState(1) +const SWRoptions = { + revalidateIfStale: true, + revalidateOnMount: true, + revalidateOnFocus: true, + revalidateOnReconnect: true, + refreshInterval: 0 +} - async function checkAssignedLevel() { - const query = { - fields: 'configuration', - id: assignedWorkflowID - } - try { - const response = await panoptes.get('/workflows', query) - if (response.ok) { - const fetchedWorkflow = response.body.workflows?.[0] - setAssignedWorkflowLevel(parseInt(fetchedWorkflow?.configuration?.level), 10) - } - } catch (error) { - throw error - } +async function fetchAssignedWorkflow({ + fields = 'configuration', + assignedWorkflowID +}) { + const query = { + fields, + id: assignedWorkflowID + } + const response = await panoptes.get('/workflows', query) + if (response.ok) { + const fetchedWorkflow = response.body.workflows?.[0] + return parseInt(fetchedWorkflow?.configuration?.level, 10) } + return 1 +} - useEffect( - function () { - if (assignedWorkflowID) { - checkAssignedLevel() - } - }, - [assignedWorkflowID] - ) +function useAssignedLevel(assignedWorkflowID) { + const key = assignedWorkflowID ? { assignedWorkflowID } : null + const { data: assignedWorkflowLevel } = useSWR(key, fetchAssignedWorkflow, SWRoptions) - return assignedWorkflowLevel + return assignedWorkflowLevel || 1 } export default useAssignedLevel diff --git a/packages/app-project/src/screens/ClassifyPage/ClassifyPageConnector.js b/packages/app-project/src/screens/ClassifyPage/ClassifyPageConnector.js index d1cf47b138..658126b5a2 100644 --- a/packages/app-project/src/screens/ClassifyPage/ClassifyPageConnector.js +++ b/packages/app-project/src/screens/ClassifyPage/ClassifyPageConnector.js @@ -1,5 +1,6 @@ import { MobXProviderContext, observer } from 'mobx-react' import { useContext } from 'react' +import useAssignedLevel from '@hooks/useAssignedLevel' import ClassifyPageContainer from './ClassifyPageContainer' function useStore(store) { @@ -25,14 +26,16 @@ function ClassifyPageConnector(props) { projectPreferences, workflowAssignmentEnabled = false } = useStore(store) + const assignedWorkflowID = projectPreferences?.settings?.workflow_id + const assignedWorkflowLevel = useAssignedLevel(assignedWorkflowID) return ( ) } diff --git a/packages/app-project/src/screens/ClassifyPage/ClassifyPageContainer.js b/packages/app-project/src/screens/ClassifyPage/ClassifyPageContainer.js index 515831fafe..252272a8f4 100644 --- a/packages/app-project/src/screens/ClassifyPage/ClassifyPageContainer.js +++ b/packages/app-project/src/screens/ClassifyPage/ClassifyPageContainer.js @@ -2,10 +2,9 @@ import { useCallback, useEffect, useState } from 'react' import { array, bool, string } from 'prop-types' import ClassifyPage from './ClassifyPage' -import useAssignedLevel from '@hooks/useAssignedLevel.js' function ClassifyPageContainer ({ - assignedWorkflowID = '', + assignedWorkflowLevel = 1, subjectID, workflowAssignmentEnabled = false, workflowID, @@ -16,7 +15,6 @@ function ClassifyPageContainer ({ but can be reset by the Classifier component via onSubjectReset(). This state does not change via components of the prioritized subjects UI (Next/Prev buttons) */ const [selectedSubjectID, setSelectedSubjectID] = useState(subjectID) - const assignedWorkflowLevel = useAssignedLevel(assignedWorkflowID) let allowedWorkflows = workflows.slice() /* Double check that a volunteer navigating to url with workflowID is allowed to load that workflow */ diff --git a/packages/app-project/src/screens/ClassifyPage/ClassifyPageContainer.spec.js b/packages/app-project/src/screens/ClassifyPage/ClassifyPageContainer.spec.js index 73d5a51731..aba5461268 100644 --- a/packages/app-project/src/screens/ClassifyPage/ClassifyPageContainer.spec.js +++ b/packages/app-project/src/screens/ClassifyPage/ClassifyPageContainer.spec.js @@ -235,7 +235,7 @@ describe('Component > ClassifyPageContainer', function () { ClassifyPageContainer', function () { Date: Sun, 18 Aug 2024 20:54:02 +0100 Subject: [PATCH 2/8] Check workflows from page props for faster performance --- packages/app-project/src/hooks/useAssignedLevel.js | 11 ++++++++--- .../src/screens/ClassifyPage/ClassifyPageConnector.js | 2 +- .../WorkflowSelectButtons/LevelingUpButtons.js | 2 +- .../WorkflowSelectButtons.spec.js | 9 ++++----- 4 files changed, 14 insertions(+), 10 deletions(-) diff --git a/packages/app-project/src/hooks/useAssignedLevel.js b/packages/app-project/src/hooks/useAssignedLevel.js index ad8ac44864..d15d0d42a8 100644 --- a/packages/app-project/src/hooks/useAssignedLevel.js +++ b/packages/app-project/src/hooks/useAssignedLevel.js @@ -18,8 +18,13 @@ const SWRoptions = { async function fetchAssignedWorkflow({ fields = 'configuration', - assignedWorkflowID + assignedWorkflowID, + workflows = [] }) { + const existingWorkflow = workflows.find(workflow => workflow.id === assignedWorkflowID) + if (existingWorkflow) { + return parseInt(existingWorkflow.configuration?.level, 10) + } const query = { fields, id: assignedWorkflowID @@ -32,8 +37,8 @@ async function fetchAssignedWorkflow({ return 1 } -function useAssignedLevel(assignedWorkflowID) { - const key = assignedWorkflowID ? { assignedWorkflowID } : null +function useAssignedLevel(assignedWorkflowID, workflows = []) { + const key = assignedWorkflowID ? { assignedWorkflowID, workflows } : null const { data: assignedWorkflowLevel } = useSWR(key, fetchAssignedWorkflow, SWRoptions) return assignedWorkflowLevel || 1 diff --git a/packages/app-project/src/screens/ClassifyPage/ClassifyPageConnector.js b/packages/app-project/src/screens/ClassifyPage/ClassifyPageConnector.js index 658126b5a2..de5640c23c 100644 --- a/packages/app-project/src/screens/ClassifyPage/ClassifyPageConnector.js +++ b/packages/app-project/src/screens/ClassifyPage/ClassifyPageConnector.js @@ -27,7 +27,7 @@ function ClassifyPageConnector(props) { workflowAssignmentEnabled = false } = useStore(store) const assignedWorkflowID = projectPreferences?.settings?.workflow_id - const assignedWorkflowLevel = useAssignedLevel(assignedWorkflowID) + const assignedWorkflowLevel = useAssignedLevel(assignedWorkflowID, props.workflows) return ( WorkflowSelector > WorkflowSelectorButtons', function () { }) }) - /** Skipped because I added a custom hook to WorkflowSelectButtons > LevelingUpButtons */ describe('when workflow assignment is enabled', function () { - describe.skip('when there is an assigned workflow', function () { - it('should only render links for unlocked workflows', function () { + describe('when there is an assigned workflow', function () { + it('should only render links for unlocked workflows', async function () { const { getAllByRole } = render( ) - expect(getAllByRole('link')).to.have.lengthOf(2) + await waitFor(() => expect(getAllByRole('link')).to.have.lengthOf(2)) }) it('should render other workflows as just text', function () { From ec16796ccc14fdce4eb890c15f2bbd2f8a291651 Mon Sep 17 00:00:00 2001 From: Jim O'Donnell Date: Tue, 20 Aug 2024 09:07:35 +0100 Subject: [PATCH 3/8] The default assigned workflow level can be higher than 1 --- packages/app-project/src/hooks/useAssignedLevel.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/app-project/src/hooks/useAssignedLevel.js b/packages/app-project/src/hooks/useAssignedLevel.js index d15d0d42a8..59c3b22a25 100644 --- a/packages/app-project/src/hooks/useAssignedLevel.js +++ b/packages/app-project/src/hooks/useAssignedLevel.js @@ -40,8 +40,12 @@ async function fetchAssignedWorkflow({ function useAssignedLevel(assignedWorkflowID, workflows = []) { const key = assignedWorkflowID ? { assignedWorkflowID, workflows } : null const { data: assignedWorkflowLevel } = useSWR(key, fetchAssignedWorkflow, SWRoptions) + const existingWorkflow = workflows.find(workflow => workflow.id === assignedWorkflowID) + let defaultWorkflowLevel = existingWorkflow?.configuration?.level ? + parseInt(existingWorkflow.configuration?.level, 10) : + 1 - return assignedWorkflowLevel || 1 + return assignedWorkflowLevel || defaultWorkflowLevel } export default useAssignedLevel From 27644f499098377fb22ceb54cfa5ae8f07c56c6f Mon Sep 17 00:00:00 2001 From: Jim O'Donnell Date: Tue, 20 Aug 2024 09:12:47 +0100 Subject: [PATCH 4/8] remove unused imports --- packages/app-project/src/hooks/useAssignedLevel.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/app-project/src/hooks/useAssignedLevel.js b/packages/app-project/src/hooks/useAssignedLevel.js index 59c3b22a25..4ea761f0a2 100644 --- a/packages/app-project/src/hooks/useAssignedLevel.js +++ b/packages/app-project/src/hooks/useAssignedLevel.js @@ -3,8 +3,6 @@ so we fetch the assigned workflow's config just in case. https://github.com/zooniverse/front-end-monorepo/issues/6198 */ - -import { useEffect, useState } from 'react' import { panoptes } from '@zooniverse/panoptes-js' import useSWR from 'swr' From b8557ea90498aa0fb1a699f60940bfe1658a3823 Mon Sep 17 00:00:00 2001 From: Jim O'Donnell Date: Tue, 20 Aug 2024 09:16:00 +0100 Subject: [PATCH 5/8] avoid returning NaN for the workflow level --- .../app-project/src/hooks/useAssignedLevel.js | 23 ++++++++----------- .../WorkflowSelectButtons.spec.js | 6 ++--- 2 files changed, 13 insertions(+), 16 deletions(-) diff --git a/packages/app-project/src/hooks/useAssignedLevel.js b/packages/app-project/src/hooks/useAssignedLevel.js index 4ea761f0a2..5efd5152d5 100644 --- a/packages/app-project/src/hooks/useAssignedLevel.js +++ b/packages/app-project/src/hooks/useAssignedLevel.js @@ -17,33 +17,30 @@ const SWRoptions = { async function fetchAssignedWorkflow({ fields = 'configuration', assignedWorkflowID, - workflows = [] }) { - const existingWorkflow = workflows.find(workflow => workflow.id === assignedWorkflowID) - if (existingWorkflow) { - return parseInt(existingWorkflow.configuration?.level, 10) - } const query = { fields, id: assignedWorkflowID } const response = await panoptes.get('/workflows', query) - if (response.ok) { - const fetchedWorkflow = response.body.workflows?.[0] + if (response.ok && response.body.workflows?.length) { + const fetchedWorkflow = response.body.workflows[0] return parseInt(fetchedWorkflow?.configuration?.level, 10) } return 1 } function useAssignedLevel(assignedWorkflowID, workflows = []) { - const key = assignedWorkflowID ? { assignedWorkflowID, workflows } : null - const { data: assignedWorkflowLevel } = useSWR(key, fetchAssignedWorkflow, SWRoptions) const existingWorkflow = workflows.find(workflow => workflow.id === assignedWorkflowID) - let defaultWorkflowLevel = existingWorkflow?.configuration?.level ? - parseInt(existingWorkflow.configuration?.level, 10) : - 1 + const defaultWorkflowLevel = existingWorkflow?.configuration?.level + ? parseInt(existingWorkflow.configuration.level, 10) + : 1 + const key = !existingWorkflow && assignedWorkflowID + ? { assignedWorkflowID } + : null // skip data fetching when we already have the workflow level + const { data: fetchedWorkflowLevel } = useSWR(key, fetchAssignedWorkflow, SWRoptions) - return assignedWorkflowLevel || defaultWorkflowLevel + return fetchedWorkflowLevel || defaultWorkflowLevel } export default useAssignedLevel diff --git a/packages/app-project/src/shared/components/WorkflowSelector/components/WorkflowSelectButtons/WorkflowSelectButtons.spec.js b/packages/app-project/src/shared/components/WorkflowSelector/components/WorkflowSelectButtons/WorkflowSelectButtons.spec.js index 7551054575..598db25e8b 100644 --- a/packages/app-project/src/shared/components/WorkflowSelector/components/WorkflowSelectButtons/WorkflowSelectButtons.spec.js +++ b/packages/app-project/src/shared/components/WorkflowSelector/components/WorkflowSelectButtons/WorkflowSelectButtons.spec.js @@ -1,4 +1,4 @@ -import { render, waitFor } from '@testing-library/react' +import { render } from '@testing-library/react' import { expect } from 'chai' import { RouterContext } from 'next/dist/shared/lib/router-context.shared-runtime' import WorkflowSelectButtons from './WorkflowSelectButtons' @@ -54,13 +54,13 @@ describe('Component > WorkflowSelector > WorkflowSelectorButtons', function () { describe('when workflow assignment is enabled', function () { describe('when there is an assigned workflow', function () { - it('should only render links for unlocked workflows', async function () { + it('should only render links for unlocked workflows', function () { const { getAllByRole } = render( ) - await waitFor(() => expect(getAllByRole('link')).to.have.lengthOf(2)) + expect(getAllByRole('link')).to.have.lengthOf(2) }) it('should render other workflows as just text', function () { From 75cbd42be464c423c2af2696d7aa03615e2f049b Mon Sep 17 00:00:00 2001 From: Jim O'Donnell Date: Sat, 24 Aug 2024 22:55:21 +0100 Subject: [PATCH 6/8] don't refetch unless assignedWorkflowID changes --- packages/app-project/src/hooks/useAssignedLevel.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/app-project/src/hooks/useAssignedLevel.js b/packages/app-project/src/hooks/useAssignedLevel.js index 5efd5152d5..197de58785 100644 --- a/packages/app-project/src/hooks/useAssignedLevel.js +++ b/packages/app-project/src/hooks/useAssignedLevel.js @@ -7,10 +7,10 @@ import { panoptes } from '@zooniverse/panoptes-js' import useSWR from 'swr' const SWRoptions = { - revalidateIfStale: true, - revalidateOnMount: true, - revalidateOnFocus: true, - revalidateOnReconnect: true, + revalidateIfStale: false, + revalidateOnMount: false, + revalidateOnFocus: false, + revalidateOnReconnect: false, refreshInterval: 0 } From f22b9054c7c39ba45c908375a2bc794e32c66aef Mon Sep 17 00:00:00 2001 From: Jim O'Donnell Date: Wed, 23 Oct 2024 22:45:02 +0100 Subject: [PATCH 7/8] switch to useSWRImmutable --- packages/app-project/src/hooks/useAssignedLevel.js | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/packages/app-project/src/hooks/useAssignedLevel.js b/packages/app-project/src/hooks/useAssignedLevel.js index 197de58785..71a9affaed 100644 --- a/packages/app-project/src/hooks/useAssignedLevel.js +++ b/packages/app-project/src/hooks/useAssignedLevel.js @@ -4,15 +4,7 @@ https://github.com/zooniverse/front-end-monorepo/issues/6198 */ import { panoptes } from '@zooniverse/panoptes-js' -import useSWR from 'swr' - -const SWRoptions = { - revalidateIfStale: false, - revalidateOnMount: false, - revalidateOnFocus: false, - revalidateOnReconnect: false, - refreshInterval: 0 -} +import useSWRImmutable from 'swr/immutable' async function fetchAssignedWorkflow({ fields = 'configuration', @@ -38,7 +30,7 @@ function useAssignedLevel(assignedWorkflowID, workflows = []) { const key = !existingWorkflow && assignedWorkflowID ? { assignedWorkflowID } : null // skip data fetching when we already have the workflow level - const { data: fetchedWorkflowLevel } = useSWR(key, fetchAssignedWorkflow, SWRoptions) + const { data: fetchedWorkflowLevel } = useSWRImmutable(key, fetchAssignedWorkflow) return fetchedWorkflowLevel || defaultWorkflowLevel } From b3f66935a95fd805ad628d0ecbbce760b6d8684e Mon Sep 17 00:00:00 2001 From: Delilah <23665803+goplayoutside3@users.noreply.github.com> Date: Wed, 13 Nov 2024 10:05:01 -0600 Subject: [PATCH 8/8] useSWR instead of useSWRImmutable --- packages/app-project/src/hooks/useAssignedLevel.js | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/packages/app-project/src/hooks/useAssignedLevel.js b/packages/app-project/src/hooks/useAssignedLevel.js index 71a9affaed..54c8e1b5e0 100644 --- a/packages/app-project/src/hooks/useAssignedLevel.js +++ b/packages/app-project/src/hooks/useAssignedLevel.js @@ -4,7 +4,15 @@ https://github.com/zooniverse/front-end-monorepo/issues/6198 */ import { panoptes } from '@zooniverse/panoptes-js' -import useSWRImmutable from 'swr/immutable' +import useSWR from 'swr' + +const SWRoptions = { + revalidateIfStale: false, + revalidateOnMount: false, + revalidateOnFocus: false, + revalidateOnReconnect: false, + refreshInterval: 0 +} async function fetchAssignedWorkflow({ fields = 'configuration', @@ -30,7 +38,7 @@ function useAssignedLevel(assignedWorkflowID, workflows = []) { const key = !existingWorkflow && assignedWorkflowID ? { assignedWorkflowID } : null // skip data fetching when we already have the workflow level - const { data: fetchedWorkflowLevel } = useSWRImmutable(key, fetchAssignedWorkflow) + const { data: fetchedWorkflowLevel } = useSWR(key, fetchAssignedWorkflow, SWRoptions) return fetchedWorkflowLevel || defaultWorkflowLevel }