Classifier: load specific workflows and subject sets #1793

eatyourgreens · 2020-09-08T11:12:12Z

Package:
lib-classifier

Explicitly call the workflows.selectWorkflow(workflowID) action when the classifier mounts. Pass in any workflowID prop that was passed to the classifier, otherwise fallback to default project workflow selection. Remove the selectWorkflow() actions that automatically run when the project or UPP loads.

Add an optional subjectSetID parameter, which will restrict subject selection to that subject set: workflows.selectWorkflow(workflowID, subjectSetID).

Workflow and subject set IDs are validated against a project's linked active workflows and a workflow's linked subject sets before being loaded.

Review Checklist

General

Are the tests passing locally and on Travis?
Is the documentation up to date?

Components

Has a storybook story been created or updated?
Is the component accessible?
- Can it be used with a screen reader? BBC guide to testing with VoiceOver
- Can it be used from the keyboard? WebAIM guide to keyboard testing
- Is it passing accessibility checks in the storybook?

Apps

Does it work in all major browsers: Firefox, Chrome, Edge, Safari?
Does it work on mobile?
Can you yarn panic && yarn bootstrap or docker-compose up --build and app works as expected?

Publishing

Is the changelog updated?
Are the dependencies updated for apps and libraries that are using the newly published library?

Post-merging

Did the app deploy to https://frontend.preview.zooniverse.org/projects/:project-name/:owner or https://frontend.preview.zooniverse.org/about?
Is the new feature working or bug now fixed?
- Is there a Talk or blog post written to announce the new feature(s)?
Is the design working across browsers (Firefox, Chrome, Edge, Safari) and mobile?
- Is this approved by our designer?
Is this ready for production deployment?

eatyourgreens · 2020-09-08T14:31:20Z

This works locally, which is promising. Tested in the standalone classifier and in the project app, using both server-side and client-side rendering of the classify page. However, the tests for WorkflowStepStore are all broken and I can't see why.

EDIT:
I think this setup line in the WorkflowStepStore tests

rootStore = setupStores(panoptesClientStub, project)

should be

rootStore = setupStores(panoptesClientStub, project, workflow)

srallen

I'm assuming this is ready for review with the change from being a draft PR... is there a project to test this with in the project app using routes? This works with the dev classifier testing between loading different workflows with staging project 1754 (ASM testing) and using query param.

packages/lib-classifier/dev/components/App/App.js

eatyourgreens · 2020-09-15T12:22:33Z

I've tested with the Classic vs. Enhanced workflows for Galaxy Zoo, though it's hard to tell exactly which one is loaded in the classifier. I've also got a staging project with a bunch of different workflows. https://master.pfe-preview.zooniverse.org/projects/eatyourgreens/-project-testing-ground

eatyourgreens · 2020-09-15T15:19:04Z

packages/lib-classifier/src/store/WorkflowStore/WorkflowStore.js

@@ -22,8 +22,6 @@ const WorkflowStore = types
        const validProjectReference = isValidReference(() => getRoot(self).projects?.active)
        if (validProjectReference) {
          self.reset()


Should this reset move into the selectWorkflow action, so that we always reset the store before setting a new workflow?

eatyourgreens · 2020-09-15T15:19:48Z

packages/lib-classifier/src/store/WorkflowStore/WorkflowStore.js

@@ -35,25 +33,11 @@ const WorkflowStore = types
        const validWorkflowReference = isValidReference(() => self.active)
        if (validUPPReference && !validWorkflowReference) {
          self.reset()


Same question about resetting the store here.

eatyourgreens · 2020-09-22T09:05:30Z

This seems to work for anonymous users but I'm seeing an error from the state tree if I try to load the classifier while logged in.

Error: [mobx-state-tree] You are trying to read or write to an object that is no longer part of a state tree. 
(Object type: 'SubjectSetResourceStore', 
Path upon death: '/workflows/resources/15652/subjectSets', 
Subpath: 'headers', 
Action: '/workflows/resources/15652/subjectSets.e()'). 
Either detach nodes first, or don't use objects after removing / replacing them in the tree.

http://localhost.zooniverse.org:3000/projects/darkeshard/engaging-the-crowds-2020/classify/workflow/15652/subject-set/85773?env=production

I'm also finding that the project app dies with an MST error if I run

yarn dev

with the dev tools open. It runs fine with the dev tools closed (which isn't very useful) or if I run a production build with dev tools:

yarn build
yarn start

EDIT: removing the WorkflowStores's Project and UPP observers, which reset the store on change, seems to fix this. I'm not sure what those observers are intended to do but nothing seems broken without them.

eatyourgreens · 2020-09-22T12:02:32Z

I'm testing on a staging project withe the project and UPP observers removed:
http://localhost.zooniverse.org:3000/projects/eatyourgreens/-project-testing-ground/classify/workflow/3371

I see workflow selection happen while the classifier is still loading the UPP, but I don't see that make any difference to the subject selection request, so it seems like the changes here load the correct workflow and subject queue for a volunteer, even if the UPP hasn't loaded in the classifier yet.

Move WorkflowStore files into their own directory.

Remove the workflow query param from the workflow store. Pass in workflowID as a prop instead.

Use the workflowID prop to select a workflow on mount. Fallback to the project default selection if no workflow ID is specified. Remove the selectWorkflow() that automatically ran on project change and UPP change.

Get the dev classifier workflow, if any, from window.location and pass it into the App component as a prop.

Set a subject set ID with workflow.selectSubjectSet(subjectSetID) and return the selected subject set from the subjectSetId view. Add an optional subject set ID to workflowStore.selectWorkflow. If present, select that subject set for the active workflow. store.setActive(id) will make an API request if a resource isn't already stored, so make workflowStore.selectWorkflow asynchronous. Fetch the workflow, then set any selected subject set ID, then set the active workflow in order to initiate subject selection from the API. Add resourceStore.getResource(id). Sometimes we want to get a resource by ID without setting it as the active resource. `getResource(id)` will return any existing, stored resource, or fetch from the API for new resources. Add workflow.subjectSets, which is a SubjectSetStore. Use workflow.subjectSets.active for subject selection. Add subject sets to the workflow factory. Add a subject set factory. Add subjectSet as a query param for the dev classifier.

Check subject set ID against workflow.links.subject_sets and error if the subject set isn't linked to the workflow.

Only set the selected subject set ID if a subject set has been explicitly set in the classifier's props.

Remove the WorkflowStore's project and UPP observers. They reset the store on change, removing any stored workflow and subject sets.

Add workflows.project, which returns the active project or undefined.

Check any supplied workflow ID against the project's active workflows before loading it.

eatyourgreens · 2020-09-22T13:53:13Z

One potential enhancement of this (but outside the scope of this PR): fetch the workflow and subject set in getServerSideProps so that the workflow and subject set titles can be added to the page title (useful for bookmarking and for SEO.) The full workflow and subject set objects could then be passed into the classifier as props, similar to how projects are handled at the moment.

front-end-monorepo/packages/app-project/pages/projects/[owner]/[project]/classify/workflow/[workflowID]/index.js

Lines 3 to 7 in d8f1071

    
           export function getServerSideProps({ params, req, res }) { 
        
             const { workflowID } = params 
        
             const props = { workflowID } 
        
             return ({ props }) 
        
           }

srallen

EDIT: removing the WorkflowStores's Project and UPP observers, which reset the store on change, seems to fix this. I'm not sure what those observers are intended to do but nothing seems broken without them.

To be able to use the classifier as stand-alone from the project app, the workflow should request a new workflow if the project changes
To setup to be able to validate a workflow that is requested against the workflow id stored in the UPP. Therefore, if the UPP changes, request a new workflow. This part isn't finished yet.

Removing these observers limits what the classifier lib can do independently now. However, since neither of these are required by the Engaging Crowds feature, I would suggest we leave in the observers, but with todo comments on future enhancement and don't have them reactively do anything yet:

    function createProjectObserver() {
      const projectDisposer = autorun(() => {
        const project = self.project
        const workflow = tryReference(() => self.active)
        if (project && !workflow) {
          // TODO request for the workflow if project has changed and there isn't one already
          // self.reset()
          // selectWorkflow(workflowID, workflow.subjectSetID)
        }
      }, { name: 'Workflow Store Project Observer autorun' })
      addDisposer(self, projectDisposer)
    }

    function createUPPObserver() {
      const uppDisposer = autorun(() => {
        const upp = tryReference(() => getRoot(self).userProjectPreferences?.active)
        const workflow = tryReference(() => self.active)
        if (upp && !workflow) {
          // TODO validate and get workflow id from UPP to select
          // self.reset()
          // selectWorkflow()
        }
      }, { name: 'Workflow Store UPP Observer autorun' })
      addDisposer(self, uppDisposer)
    }

One potential enhancement of this (but outside the scope of this PR): fetch the workflow and subject set in getServerSideProps so that the workflow and subject set titles can be added to the page title (useful for bookmarking and for SEO.) The full workflow and subject set objects could then be passed into the classifier as props, similar to how projects are handled at the moment.

Agreed that this is out of scope, but again, I think I'd prefer we lean toward taking a snapshot (from prop or somewhere else?), which could be used in the constructor of the Classifier where the store is initialized. The snapshot could be defined to have full resources or to just have ids that the classifier could then use to make requests as needed. I don't want to have to depend a whole lot on logic happening in either a componentDidMount or hooks to setup the classifier.

Anyway, I've tested this using the production project app starting with this URL:
http://localhost.zooniverse.org:3000/projects/darkeshard/engaging-the-crowds-2020/classify/workflow/15652/subject-set/85773?env=production

And confirmed that the requested workflow matched 15652, subject set matched 85773, and the queued endpoint correctly added those as query params.

Then I navigated to the home page and selected a new workflow and subject set:
http://localhost.zooniverse.org:3000/projects/darkeshard/engaging-the-crowds-2020/classify/workflow/16106/subject-set/85771

And I confirmed the requested workflow id is 16106, subject set is 85771, and the queued endpoint correctly added those query params.

Then back to the homepage and selected a non-grouped workflow, id 15651 aka "Drawing Workflow", and it correctly requested it and used it for the queued endpoint, but the route in the URL did not have the id. This is out of scope here, but I would expect the URL to have this workflow id as well.

srallen · 2020-09-22T17:06:20Z

packages/lib-classifier/src/components/Classifier/Classifier.js

    this.setProject(project)
    this.classifierStore.setOnAddToCollection(onAddToCollection)
    this.classifierStore.classifications.setOnComplete(onCompleteClassification)
    this.classifierStore.setOnToggleFavourite(onToggleFavourite)
+    this.classifierStore.workflows.selectWorkflow(workflowID, subjectSetID)


I think I would prefer on continuing to rely the reactive paradigm we have setup using observers in the stores for asynchronous request or setup of resources in the store. However, I think that will take some additional refactoring that is out of scope here.

I couldn't come up with a good way to pass the classifier props down into the workflow store, except by calling an action directly. If this block could be replaced with store.create(snapshot), that would be great.

eatyourgreens · 2020-09-22T17:33:45Z

I think 15651 might be the default workflow. Those are routed to /classify in the workflow selector code.

front-end-monorepo/packages/app-project/src/screens/ProjectHomePage/components/Hero/components/WorkflowSelector/components/WorkflowSelectButton/WorkflowSelectButton.js

Lines 26 to 31 in d8f1071

    
           const url = (workflow.default) 
        
             ? `/projects/${owner}/${project}/classify` 
        
             : `/projects/${owner}/${project}/classify/workflow/${workflow.id}` 
        
           const href = (workflow.default) 
        
             ? '/projects/[owner]/[project]/classify' 
        
             : '/projects/[owner]/[project]/classify/workflow/[workflowID]'

eatyourgreens · 2020-09-23T08:34:12Z

I'm going to open a PR to restore the observers. One scenario I tested here in the project app was to log in, check that the subject queue refreshed with my personal subject queue, then log out and check that it refreshed using the random subject selector. I can see a case for observing user changes in order to handle that inside the standalone classifier.

Restore the workflow store observers, which reload workflows when the stored project or user project preferences change. These were deleted in #1793 because they either override the stored workflow with the project default, or throw reference errors in the store.

Restore the workflow store observers, which reload workflows when the stored project or user project preferences change. These were deleted in #1793 because they either override the stored workflow with the project default, or throw reference errors in the store. Co-authored-by: srallen <[email protected]>

eatyourgreens added the enhancement New feature or request label Sep 8, 2020

eatyourgreens force-pushed the classifier-specify-workflow branch from 382cd2c to e8b0c92 Compare September 8, 2020 14:29

eatyourgreens marked this pull request as ready for review September 8, 2020 16:50

srallen reviewed Sep 14, 2020

View reviewed changes

packages/lib-classifier/dev/components/App/App.js Outdated Show resolved Hide resolved

eatyourgreens force-pushed the classifier-specify-workflow branch from 0e2f8b3 to c9b8904 Compare September 15, 2020 15:01

eatyourgreens commented Sep 15, 2020

View reviewed changes

eatyourgreens mentioned this pull request Sep 15, 2020

[RFC] How to handle workflow selection? #1132

Closed

eatyourgreens force-pushed the classifier-specify-workflow branch from 7587667 to 96e9e42 Compare September 16, 2020 10:13

eatyourgreens mentioned this pull request Sep 16, 2020

Classifier: specify subject set #1804

Merged

18 tasks

eatyourgreens force-pushed the classifier-specify-workflow branch 2 times, most recently from 9c6d9db to 3a940b0 Compare September 18, 2020 13:59

eatyourgreens changed the title ~~Classifier: load specific workflows~~ Classifier: load specific workflows and subject sets Sep 21, 2020

eatyourgreens force-pushed the classifier-specify-workflow branch from 12f12f5 to 64fb48d Compare September 21, 2020 10:31

eatyourgreens added 11 commits September 22, 2020 13:03

Classifier: add @store/WorkflowStore directory

09d590b

Move WorkflowStore files into their own directory.

Remove workflow query param

cc9901f

Remove the workflow query param from the workflow store. Pass in workflowID as a prop instead.

Select a workflow

b6371e5

Use the workflowID prop to select a workflow on mount. Fallback to the project default selection if no workflow ID is specified. Remove the selectWorkflow() that automatically ran on project change and UPP change.

Fix WorkflowStepStore tests

24be592

Add some tests for selectWorkflow

b80bcb0

Add workflowID prop to dev classifier

9f5ac74

Get the dev classifier workflow, if any, from window.location and pass it into the App component as a prop.

Validate subject sets against workflow links

c53869c

Check subject set ID against workflow.links.subject_sets and error if the subject set isn't linked to the workflow.

Default workflow.subjectSetId to undefined

008106f

Only set the selected subject set ID if a subject set has been explicitly set in the classifier's props.

Remove project and UPP observers

c59980c

Remove the WorkflowStore's project and UPP observers. They reset the store on change, removing any stored workflow and subject sets.

Add workflows.project

f1c1f27

Add workflows.project, which returns the active project or undefined.

eatyourgreens force-pushed the classifier-specify-workflow branch from 133654a to f1c1f27 Compare September 22, 2020 12:04

Validate workflows against the active project

6018468

Check any supplied workflow ID against the project's active workflows before loading it.

srallen approved these changes Sep 22, 2020

View reviewed changes

github-actions bot added the approved This PR is approved for merging label Sep 22, 2020

eatyourgreens merged commit c9442d9 into master Sep 23, 2020

eatyourgreens deleted the classifier-specify-workflow branch September 23, 2020 08:16

eatyourgreens mentioned this pull request Sep 23, 2020

Restore workflowStore observers #1817

Merged

18 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Classifier: load specific workflows and subject sets #1793

Classifier: load specific workflows and subject sets #1793

eatyourgreens commented Sep 8, 2020 •

edited

Loading

eatyourgreens commented Sep 8, 2020 •

edited

Loading

srallen left a comment

eatyourgreens commented Sep 15, 2020

eatyourgreens Sep 15, 2020

eatyourgreens Sep 15, 2020

eatyourgreens commented Sep 22, 2020 •

edited

Loading

eatyourgreens commented Sep 22, 2020

eatyourgreens commented Sep 22, 2020 •

edited

Loading

srallen left a comment

srallen Sep 22, 2020

eatyourgreens Sep 23, 2020

eatyourgreens commented Sep 22, 2020 •

edited

Loading

eatyourgreens commented Sep 23, 2020

Classifier: load specific workflows and subject sets #1793

Classifier: load specific workflows and subject sets #1793

Conversation

eatyourgreens commented Sep 8, 2020 • edited Loading

Review Checklist

General

Components

Apps

Publishing

Post-merging

eatyourgreens commented Sep 8, 2020 • edited Loading

srallen left a comment

Choose a reason for hiding this comment

eatyourgreens commented Sep 15, 2020

eatyourgreens Sep 15, 2020

Choose a reason for hiding this comment

eatyourgreens Sep 15, 2020

Choose a reason for hiding this comment

eatyourgreens commented Sep 22, 2020 • edited Loading

eatyourgreens commented Sep 22, 2020

eatyourgreens commented Sep 22, 2020 • edited Loading

srallen left a comment

Choose a reason for hiding this comment

srallen Sep 22, 2020

Choose a reason for hiding this comment

eatyourgreens Sep 23, 2020

Choose a reason for hiding this comment

eatyourgreens commented Sep 22, 2020 • edited Loading

eatyourgreens commented Sep 23, 2020

eatyourgreens commented Sep 8, 2020 •

edited

Loading

eatyourgreens commented Sep 8, 2020 •

edited

Loading

eatyourgreens commented Sep 22, 2020 •

edited

Loading

eatyourgreens commented Sep 22, 2020 •

edited

Loading

eatyourgreens commented Sep 22, 2020 •

edited

Loading