Prevent tasks linking to themselves #5925
Conversation
Add a task prop to the NextTaskSelector in the lab and pass down the current task. Filter the current task out of the list of next task options.
|
I'm trying to think of scenarios where a Task WOULD like to link to themselves. Uh... T0: Are we there yet? Are we there yet? Are we there yet? That doesn't make sense, so it the "let's not allow tasks to link to themselves" rule is valid. Let me think of Combo Tasks... |
|
I suppose, in theory, it's possible somebody might want to create a combo task like... T0: Do you see one or more muppets in this photo? T1.0: Describe one of the muppets. T1.1: Is there another muppet you want to talk about? EDIT: I've just tested this on PFE, and it's NOT possible to branch from a Combo task. A Combo Task on the PFE classifier has only ONE "Next Task", and ignores any "Next Task" logic of its children Tasks. (Maybe I'm thinking of the front-end-monorepo... does that have more complicated branching logic?) In any case, I see no logical reason for a Task to ever loop in on itself so immediately, so the goal of this PR is legit. I'll proceed with functionality tests next. NOTE: It will still be possible to lock yourself in a two-step loop, e.g. T0 leads to T1, leads to T0, leads to T1, etc but this PR can only fix so much insanity. |
|
Ha! Every now and again we get a report of a broken workflow where it turns out the solution is that Next Task has been accidentally set to something other than ‘submit the classification’. We could maybe have a check for that too. Might be complicated for branching workflows. |
There was a problem hiding this comment.
PR Review
This PR changes the Workflow editor page, preventing any given Task from setting its "next task" to be itself.
- i.e. this PR prevents the workflow logic from looping endlessly on one task.
⚠️ It is STILL possible to create a two-step loop, e.g. T0's next task is T1, and T1's next task is T0. This PR will not fix such a scenario, and trying to detect infinite loops in the logic is a gateway to madness.
Code read looks good, and functionality checks out.
Testing
Tested on localhost + macOS 10 + Chrome 89, using a random test project.
Replication steps
- create 3 tasks, T0, T1, T2
- for T0, look at the options available for "Next Task"
- The list should include T1, T2, and "submit"
- The list shouldn't include T0
- Repeat for T0
- Repeat for different Task types
Tests look good for the Task types: Question, Drawing, Text, Survey, Combo. I'm happy to give this a blanket 👍 for all task types.
Additional notes on testing further Task types:
- Main logic change is in , which now requires a "current task" prop to check against.
- A quick look through every use of in the repo indicates that this "current" task is being passed down properly.
- This propagated logic seems legit, so a thorough test of e.g. Dropdown Task seems like overkill.
Status
LGTM! As noted in earlier comments, I don't see any logical reason for a Task to link to itself, so this PR goals are legit to me.
A second go at fixing this, previously fixed in zooniverse#5925. Pass the current task key as a prop to `NextTaskSelector`. Filter the list of tasks to exclude that task key, so that a task can't link back to itself. This has to be edited in the editors for each individual task type, but I think I got them all.
A second go at fixing this, previously fixed in zooniverse#5925. Pass the current task key as a prop to `NextTaskSelector`. Filter the list of tasks to exclude that task key, so that a task can't link back to itself. This has to be edited in the editors for each individual task type, but I think I got them all.
A second go at fixing this, previously fixed in zooniverse#5925. Pass the current task key as a prop to `NextTaskSelector`. Filter the list of tasks to exclude that task key, so that a task can't link back to itself. This has to be edited in the editors for each individual task type, but I think I got them all.
A second go at fixing this, previously fixed in zooniverse#5925. Pass the current task key as a prop to `NextTaskSelector`. Filter the list of tasks to exclude that task key, so that a task can't link back to itself. This has to be edited in the editors for each individual task type, but I think I got them all.
A second go at fixing this, previously fixed in zooniverse#5925. Pass the current task key as a prop to `NextTaskSelector`. Filter the list of tasks to exclude that task key, so that a task can't link back to itself. This has to be edited in the editors for each individual task type, but I think I got them all.
A second go at fixing this, previously fixed in zooniverse#5925. Pass the current task key as a prop to `NextTaskSelector`. Filter the list of tasks to exclude that task key, so that a task can't link back to itself. This has to be edited in the editors for each individual task type, but I think I got them all.
A second go at fixing this, previously fixed in zooniverse#5925. Pass the current task key as a prop to `NextTaskSelector`. Filter the list of tasks to exclude that task key, so that a task can't link back to itself. This has to be edited in the editors for each individual task type, but I think I got them all.
A second go at fixing this, previously fixed in zooniverse#5925. Pass the current task key as a prop to `NextTaskSelector`. Filter the list of tasks to exclude that task key, so that a task can't link back to itself. This has to be edited in the editors for each individual task type, but I think I got them all.
A second go at fixing this, previously fixed in zooniverse#5925. Pass the current task key as a prop to `NextTaskSelector`. Filter the list of tasks to exclude that task key, so that a task can't link back to itself. This has to be edited in the editors for each individual task type, but I think I got them all.
A second go at fixing this, previously fixed in zooniverse#5925. Pass the current task key as a prop to `NextTaskSelector`. Filter the list of tasks to exclude that task key, so that a task can't link back to itself. This has to be edited in the editors for each individual task type, but I think I got them all.
Add a task prop to the
NextTaskSelectorin the lab and pass down the current task. Filter the current task out of the list of next task options.This turned out to be more complicated than I thought because each task (
app/classifier/tasks) implements its own 'Next task' menu in its editor component. I think I got them all. You can check it by editing tasks in the project builder. You shouldn't be able to link any task back to itself any more.Staging branch URL: https://pr-5925.pfe-preview.zooniverse.org
Fixes #2512.
Required Manual Testing
Review Checklist
rm -rf node_modules/ && npm installand app works as expected?Optional
ChangeListenerorPromiseRenderercomponents with code that updates component state?