-
Notifications
You must be signed in to change notification settings - Fork 30
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
Engaging Crowds: Add URL workflow and subject set to WorkflowMenu #2169
Conversation
868a41f
to
8cb199c
Compare
8cb199c
to
021f772
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've gone through the test projects provided. I'm unsure because of the time between this opening and my review if something about the project has changed or if there's something broken in the code, but the first listed URL to test from Scarlets and Blues, loads subject sets to pick from, however, clicking a subject to classify, does not load the classifier for me.
I see the classifier attempts to load subjects prior to subject set selection and will error, bubbling the error up to the error boundary in the ClassifierWrapper
.
I can select a subject set and pick the first one, "Minutes-Beta", and the modal closes, but this doesn't seem to trigger loading a subject.
If I refresh the page, with subject set selected still in the URL, then a subject loads as expected from the start.
The other URLs appear to work as expected, so it's just the first one that I can't get to work.
(As an aside I see several prop type errors for SubjectSetCard
, out of scope for this PR. Several required prop types declared incorrectly and should use isRequired
).
Sounds like a bug. Does it happen on staging? https://frontend.preview.zooniverse.org/projects/bogden/scarlets-and-blues EDIT: I'm seeing invalid URLs for the subject requests, like the following. I'll look into that tomorrow. Thanks for taking a look @srallen. On staging, I see this request work just fine. |
aa06188
to
4ed685b
Compare
@srallen I've pushed a fix for that bug. The classifier crashes if incomplete query parameters are passed in as props eg. workflow ID without subject set ID for grouped subject selection. I've changed it to wait until all required parameters are ready together. |
375f722
to
023c52e
Compare
33ae1ce
to
c1a032a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of the test cases appear to be working now. 👍
packages/app-project/src/screens/ClassifyPage/ClassifyPage.spec.js
Outdated
Show resolved
Hide resolved
If workflowID and/or subjectSetID are defined in the Classify page URL, this skips past workflow selection and/or subject set selection when opening the workflow menu.
Test the workflow menu logic with different combinations of props. Add some explanatory comments.
Wait until canClassify is true before setting workflowID, subjectSetID or subjectID in the classifier. Otherwise, subject queries can be sent with incomplete URLs.
b85c429
to
710757d
Compare
If workflowID and/or subjectSetID are defined in the Classify page URL, this skips past workflow selection and/or subject set selection when opening the workflow menu.
Something I missed in #2132: if you link directly to a workflow or subject set, then that workflow or set should already be selected in the workflow menu when the Classify page opens.
Test URLs:
Workflow selected, jump straight to subject set selection
http://local.zooniverse.org:3000/projects/bogden/scarlets-and-blues/classify/workflow/17077?env=production&demo=true
Subject set selected, jump straight to subject selection
http://local.zooniverse.org:3000/projects/bogden/scarlets-and-blues/classify/workflow/17077/subject-set/92751?env=production&demo=true
Subject set selected but workflow doesn't use subject selection
http://local.zooniverse.org:3000/projects/bogden/scarlets-and-blues/classify/workflow/16848/subject-set/90488?env=production&demo=true
A workflow that doesn't use the
WorkflowMenu
componenthttp://local.zooniverse.org:3000/projects/brooke/i-fancy-cats/classify?env=staging&demo=true
I Fancy Cats with a specific subject
http://local.zooniverse.org:3000/projects/brooke/i-fancy-cats/classify/workflow/693/subject/4235?env=staging&demo=true
Package:
app-project
Review Checklist
General
Components
Apps
yarn panic && yarn bootstrap
ordocker-compose up --build
and app works as expected?Publishing
Post-merging