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.

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

fix(app-project): data-fetching for assigned workflow level #6219

Merged
merged 8 commits into from
Nov 13, 2024

Conversation

eatyourgreens
Copy link
Contributor

@eatyourgreens eatyourgreens commented Aug 17, 2024

Fix missing dependency errors in the useAssignedLevel hook by using useSWR to fetch and cache the assigned workflow level. SWR also adds revalidation of the Panoptes API request for cases where I might classify in the same tab over several days or weeks.*

Adds conditional data-fetching, so that we only fetch a workflow configuration if we don’t already have it in the workflows page prop.

Also fixes the workflow assignment tests in ClassifyPageContainer so that they test against the correct levels, and restores skipped tests to double check that nothing is broken.

Adds Mock Service Worker, for mocking API requests in stories. I've moved the story for this into #6406. It sets up a test case where the assigned workflow, level 3, is complete and must be fetched from the Panoptes API.

*do workflow levels change over time? If not, the fetched value can be cached indefinitely.

Please request review from @zooniverse/frontend team or an individual member of that team.

Package

  • app-project

Linked Issue and/or Talk Post

How to Review

Testing for this should be the same as #6201. useAssignedLevel should only call the API once for a given workflow, even if it’s invoked from multiple components.

If you load Gravity Spy with the workflow choice menu, you can check that the Classify page only makes one API request for any missing workflows, even though useAssignedLevel is used twice on the page. The assigned level should also be cached in the client-side project app for the duration of your session.
https://local.zooniverse.org:3000/projects/zooniverse/gravity-spy/classify?env=production

Checklist

PR Creator - Please cater the checklist to fit the review needed for your code changes.
PR Reviewer - Use the checklist during your review. Each point should be checkmarked or discussed before PR approval.

General

  • Tests are passing locally and on Github
  • Documentation is up to date and changelog has been updated if appropriate
  • You can yarn panic && yarn bootstrap or docker-compose up --build and FEM works as expected
  • FEM works in all major desktop browsers: Firefox, Chrome, Edge, Safari (Use Browserstack account as needed)
  • FEM works in a mobile browser

General UX

Example Staging Project: i-fancy-cats

  • All pages of a FEM project load: Home Page, Classify Page, and About Pages
  • Can submit a classification
  • Can sign-in and sign-out
  • The component is accessible

Bug Fix

  • The PR creator has listed user actions to use when testing if bug is fixed
  • The bug is fixed
  • Unit tests are added or updated

@eatyourgreens eatyourgreens force-pushed the use-assigned-level branch 6 times, most recently from 0fbf132 to d9b87f8 Compare August 17, 2024 22:57
@coveralls
Copy link

coveralls commented Aug 17, 2024

Coverage Status

coverage: 77.535% (-0.04%) from 77.571%
when pulling b3f6693 on eatyourgreens:use-assigned-level
into 7b9879d on zooniverse:master.

@eatyourgreens
Copy link
Contributor Author

eatyourgreens commented Aug 17, 2024

#6201 broke these tests for assigned workflow level in ClassifyPageContainer. I’ve fixed them here by moving the data fetching up into the connector component.

describe('when the assigned workflow level is greater than or equal to the workflow from URL level', function () {
it('should be able to load the workflow from the url', function () {
const mockStoreWithAssignment = Object.assign({}, mockStore, {
user: {
collections: {
collections: []
},
personalization: {
projectPreferences: {
promptAssignment: () => false,
settings: {
workflow_id: '5678'
}
},
sessionCount: 0,
stats: {
thisWeek: []
}
},
recents: {
recents: []
}
}
})
const wrapper = mount(
<RouterContext.Provider value={mockRouter}>
<Provider store={mockStoreWithAssignment}>
<ClassifyPageContainer
assignedWorkflowID='5678'
workflowAssignmentEnabled
workflowID='1234'
workflows={workflows}
/>
</Provider>
</RouterContext.Provider>, {
wrappingComponent: Grommet,
wrappingComponentProps: { theme: zooTheme }
}
)
expect(wrapper.find(ClassifyPage).props().workflowFromUrl).to.equal(workflows[0])
expect(wrapper.find(ClassifyPage).props().workflowID).to.equal(workflows[0].id)
})
})
describe('when the assigned workflow level is less than the workflow from URL level', function () {
it('should not be able to load the workflow from the url', function () {
const mockStoreWithAssignment = Object.assign({}, mockStore, {
user: {
collections: {
collections: []
},
personalization: {
projectPreferences: {
settings: {
workflow_id: '1234'
}
},
sessionCount: 0,
stats: {
thisWeek: []
}
},
recents: {
recents: []
}
}
})
const wrapper = mount(
<RouterContext.Provider value={mockRouter}>
<Provider store={mockStoreWithAssignment}>
<ClassifyPageContainer
assignedWorkflowID='1234'
workflowAssignmentEnabled
workflowID='5678'
workflows={workflows}
/>
</Provider>
</RouterContext.Provider>, {
wrappingComponent: Grommet,
wrappingComponentProps: { theme: zooTheme }
})
expect(wrapper.find(ClassifyPage).props().workflowFromUrl).to.be.null()
expect(wrapper.find(ClassifyPage).props().workflowID).to.be.undefined()
})
})
})

@goplayoutside3
Copy link
Contributor

@eatyourgreens Re:

*do workflow levels change over time? If not, the fetched value can be cached indefinitely.

Yes they can change levels. The project team has full control over workflow level configuration. Does that fact change any code in this PR?

@eatyourgreens
Copy link
Contributor Author

eatyourgreens commented Aug 18, 2024

I shouldn't imagine so, but you can adjust the revalidation options if the hook is constantly requesting the same level from Panoptes, for a given workflow.

https://swr.vercel.app/docs/revalidation

You can also turn off revalidation completely for immutable resources.

https://swr.vercel.app/docs/revalidation#disable-automatic-revalidations

@eatyourgreens
Copy link
Contributor Author

eatyourgreens commented Aug 18, 2024

Right now, in production, the Classify page requests the workflow config from Panoptes on every visit to the page, even if it already has it. Workflow select buttons then repeat the request, even though the app already has the workflow config for the missing workflow.

EDIT: in Strict Mode, I see the useEffect hook make 2 API requests per component but SWR makes 1 request then reuses the cached response.

That can be improved by checking if the workflow config is already present in page props, before making a request to Panoptes, and by caching the API response for reuse.

@eatyourgreens
Copy link
Contributor Author

if (response.ok) {
const fetchedWorkflow = response.body.workflows?.[0]
setAssignedWorkflowLevel(parseInt(fetchedWorkflow?.configuration?.level), 10)
}

If the assigned workflow ID in my user preferences is an inactive workflow, then fetchedWorkflow will be undefined here and assignedWorkflowLevel will be set to NaN. I've updated this PR to catch that case and return 1.

@eatyourgreens eatyourgreens force-pushed the use-assigned-level branch 4 times, most recently from 56a2c14 to 2ae8c54 Compare August 24, 2024 22:05
@eatyourgreens eatyourgreens force-pushed the use-assigned-level branch 2 times, most recently from 084a0de to ca9cac5 Compare August 30, 2024 12:16
@eatyourgreens eatyourgreens force-pushed the use-assigned-level branch 2 times, most recently from f0edbb1 to 06c5461 Compare October 3, 2024 17:44
@goplayoutside3 goplayoutside3 self-assigned this Oct 17, 2024
Copy link
Contributor

@goplayoutside3 goplayoutside3 left a comment

Choose a reason for hiding this comment

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

Something isn't quite right with displaying locked vs unlocked in this PR. When I'm signed-in with the zootester1 account at https://www.zooniverse.org/projects/zooniverse/gravity-spy I correctly see six workflows unlocked. When I run app-project on this branch locally with yarn start at https://local.zooniverse.org:3000/projects/zooniverse/gravity-spy, I only see one workflow unlocked.

See details for expected behavior when a user account is assigned a workflow id and that workflow is inactive/complete: #6198

@eatyourgreens
Copy link
Contributor Author

eatyourgreens commented Oct 23, 2024

@goplayoutside3 that's my fault. I made the data fetching options too restrictive. Switching to useSWRImmutable should have fixed it, while still caching the workflow level for each workflow ID so that it’s only fetched once.

@eatyourgreens
Copy link
Contributor Author

eatyourgreens commented Oct 24, 2024

I've added a story for the case where the assigned workflow is missing from the workflows list, using MSW to mock the Panoptes API. It should assign you to level 3, and unlock levels 1 and 2.

EDIT: adding msw allows for mocking the Panoptes API here, but conflicts with tests that are using nock, so I'll roll that back.

@eatyourgreens
Copy link
Contributor Author

eatyourgreens commented Oct 24, 2024

If you’d like to check out the storybook with API mocks, I’ve pushed those changes here:
https://github.com/eatyourgreens/front-end-monorepo/tree/project-msw-mocks

I've opened #6406 for that branch. It adds a story to test the case where the assigned workflow is complete. The changes here shouldn't break that.

@goplayoutside3
Copy link
Contributor

goplayoutside3 commented Oct 30, 2024

This is looking much better. When signed in with various accounts, I see the expected unlocked workflows on Gravity Spy, and when signed out I see only the first level unlocked.

However, I need to double check how truly immutable our workflow levels are. Implementing useSWRImmutable might create an edge case where a workflow's level changes on the backend, and then a user assigned to it doesn't see the correct locked/unlocked UI. I'll have a quick chat with Cliff before approving this PR.

@eatyourgreens
Copy link
Contributor Author

eatyourgreens commented Oct 31, 2024

You can pass the same options into useSWRImmutable as useSWR, if you need to revalidate workflow levels during the course of a session. The immutable hook just sets some to false by default.
https://swr.vercel.app/docs/revalidation#disable-automatic-revalidations

By default, the immutable hook doesn’t refetch when you switch focus back to a tab, when you wake the computer from sleep, or when the fetched resource becomes stale, but those are all configurable.

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.
Copy link
Contributor

@goplayoutside3 goplayoutside3 left a comment

Choose a reason for hiding this comment

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

I reverted to useSWR instead of useSWRImmutable. Workflow level can be changed by project teams at any point, so I made this change to avoid future confusion.

@goplayoutside3 goplayoutside3 merged commit 2d8c929 into zooniverse:master Nov 13, 2024
8 checks passed
@eatyourgreens
Copy link
Contributor Author

eatyourgreens commented Nov 13, 2024

In that case, you'll want to set revalidateOnMount to true, otherwise I don't think it fetches at all.

That was my mistake in the original version of this PR.

@goplayoutside3
Copy link
Contributor

Locked and unlocked levels are displayed as expected after this PR. The SWR options mentioned only apply to cases where the workflows array doesn't include the id assigned to a user. I no longer have a test case with zootester1 for this scenario because Gravity Spy corrected their level progression.

If you'd like to rebase #6406 and change any SWR options in useAssignedLevel() via that PR I'm happy to include it in next week's deploy.

@eatyourgreens
Copy link
Contributor Author

Ok. The options now are set to the options for useSWRImmutable but with the addition of revalidateOnMount set to false.

@eatyourgreens eatyourgreens deleted the use-assigned-level branch November 13, 2024 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

project build error: missing useEffect dependency for assigned workflows
3 participants