shaunanoordin commented 5 years ago

Request for Comments: how to remove ambiguity in PFE Classifier's Workflow selection

This RFC follows the Oxford team's discussion today where we delved into the issue of the ambiguous Workflow selection (see #5111) and proposed some solutions to fix it. what we want now is to share those plans and to seek feedback from the whole Zooniverse team, as any solutions to this Workflow ambiguity problem will affect projects across the whole platform.

Problem: As described in #5111, the PFE Classifier has an issue where, when a Workflow is selected on a project with multiple Workflows, we can't be too sure which workflow resources the user sees. This leads to problematic conditions where, for example, a User selects Workflow A, sees Subjects from Workflow A, but sees the translated questions from Workflow B.

Proposed Solution We need to be extremely clear which Workflow is currently being worked by the PFE Classifier.

There are two parts to this solution:

when fetching a Workflow, we must get the ID explicitly from the URL - e.g. https://www.zooniverse.org/projects/drewdeepsouth/southern-weather-discovery/classify?workflow=6442 - and not "guess" which Workflow the user wanted to work on based on click events or etc.
the Classification needs to record which resources were seen by the user when they were classifying - this usually takes the form of an additional classification.links.translation_resource = [123] or similar.

Solution Part 1 should eliminate any sort of ambiguity in workflow selection, while Solution Part 2 is a logical followthrough which allows us to double-check that users classifying Subjects from Workflow X were indeed looking at (translated) questions from Workflow X.

Additional Notes

If we're enforcing that the PFE Classifier needs to be able to explicitly identify the workflow it's supposed to work on via URL or query params, we also need to consider what happens when none is provided - e.g. if a user visits https://www.zooniverse.org/projects/drewdeepsouth/southern-weather-discovery/classify, do we redirect to the default workflow URL (with workflow ID in the query param)? What if there's no default workflow?
Additional problems solved by having a specific Workflow ID in the URL include:
- user bookmarks consistently taking them to the same workflow
- users opening a specific workflow via "right-click->open in new tab" on a Project home page will now open the correct workflow.
When storing Translation resources, we must store them by Workflow ID. The Workflow ID adds an extra level of identity when fetching Translations, and gets around the problem where two workflows might share the same Task ID (e.g. "T0")

We're putting a strong emphasis on noting the Translations resources because that's the most noted cause of significant errors - users seeing the (translated) questions for the wrong workflow.

Status / Where we stand right now

The problem of incorrect questions on workflows has to be considered a high priority problem due to how it can invalidate the work of volunteers.

srallen commented 5 years ago

The proposed solution is essentially reverting to PFE behavior for workflow selection back in 2015. Using the query param in the URL used to be how all projects loaded the workflow, but many new features were added that complicated workflow selection that at the time, the query param was removed except for projects that needed it, i.e. NfN. I am hesitant to make this big of a refactor/revert to PFE because how it can snowball. The recent change to UPP had unintended consequences as well since the UPP was where the workflow selection of the user was stored. I would propose instead that we revert any non-essential changes. Although there were some known rare bugs with the way it worked before, but it was working at its core with how it worked in the past couple of years. We should be freezing changes to PFE and rolling in new strategies into the rewrite.

eatyourgreens commented 5 years ago

I don't think there's anything that actually breaks if we allow query params by default, but allow projects like Gravity Spy to override that if they wish.

srallen commented 5 years ago

if a user visits https://www.zooniverse.org/projects/drewdeepsouth/southern-weather-discovery/classify, do we redirect to the default workflow URL (with workflow ID in the query param)? What if there's no default workflow?

If we go this route, then if the query param is missing, then selection should continue to flow as it does now: UPP preferences selected_workflow attribute -> UPP settings workflow id attribute -> project config default workflow id -> random selection out of linked active workflow ids

Similarly if the id in the url is invalid, like the workflow is no longer active and the user does not have a role to allow loading anyway, then that cascade of selection should happen.

As far as if this change can break anything, keep in mind this was the method of selection by query param was before workflow activeness existed as a concept for workflows. I'm don't feel confident that this kind of change won't have an unintended consequence, however, I recognize that at this point it may be necessary because of the in progress cascade of changes from the past 1-2 months already in place and it might not be easy to revert.

marten commented 5 years ago

Rather than a query param, I would suggest putting it in the URL path.

srallen commented 5 years ago

Putting it the path would necessitate the rewrite's classifier library's stores to be aware of the client side router of the consuming app or it'd have to be passed in as a prop putting it back into React lifecycle, so I think query param might be preferable in that case. If it's a query param, the store can just directly check the browser's location's search attribute.

I'd like to have a solution that could be consistently implemented between PFE and the rewrite

eatyourgreens commented 5 years ago

I'm inclined to leave it as query param, if only because that's how NfN is set up: https://www.zooniverse.org/projects/zooniverse/notes-from-nature/classify?reload=1&workflow=7899

eatyourgreens commented 5 years ago

What's that reload param, out of interest?

shaunanoordin commented 5 years ago

Additional consideration: invalid workflow selection

If we (re-)introduce the ability for Workflow IDs to be specified in the URL path or the query param, we need consider one extra failure condition: what do we do when a user specifies a workflow that's...

invalid (e.g. workflow isn't public and user isn't an owner), or
doesn't exist, or
belongs to a different project?

At the moment, if a user attempts to specify in invalid workflow (for projects that allow workflow selection, such as for WildCam Darien's classrooms), the user is invisibly redirected to the default workflow which is... not great for the user experience. For example:

User access https://www.zooniverse.org/projects/wildcam/wildcam-darien/classify?workflow=3231
PFE Classifier detects that workflow 3231 doesn't exist or is invalid; the console says No workflow 3231 for project 3525
URL changes to https://www.zooniverse.org/projects/wildcam/wildcam-darien/classify (default) and the default WildCam Darien workflow 3033 is fetched instead.
User continues to work on the default workflow without being explicitly told that they're not on the workflow they were originally attempting to access.

srallen commented 5 years ago

To change to use query param, I think what would need to happen is change this line:

https://github.com/zooniverse/Panoptes-Front-End/blob/0d0d513e574baa9023d383a8d1057c68515903b4/app/pages/project/workflow-selection.jsx#L63

To:

project.experimental_tools.indexOf('workflow assignment') === -1

to exclude projects that use assignment/promotion like Gravity Spy and Snapshot WI.

Then something like this after line 98, though more robust so that it doesn't overwrite existing query params, works with react router, etc:

.then((workflow) => {
  history.replaceState({}, '', location.href + `?workflow=${workflow.id}`)
})

And add something like this to handle invalid workflows to clearInactiveWorkflows prior to the preferences update:

    if (location.search.includes(selectedWorkflowID)) {
      const searchParams = location.search.split(`workflow=${sanitisedWorkflowID}`)
      // There are other params
      if (searchParams[1]) {
        const newSearchParams = searchParams.join('&')
        history.replaceState({}, '', location.href + newSearchParams)
      }
    }

Then follow up by:

Removing the experimental tool 'allow workflow query' as it won't be needed anymore
Update workflow buttons on home page to use query param
Update links in project nav bar and in talk to use the currently stored workflow's id and add the query param to the link so the same workflow loads when going back and forth from the classifier and talk

If this works, then adding a redux store for UPP may not be necessary for PFE, but we should be aware that one is already written for the rewrite.

For the rewrite, we would then in parallel add the selection strategy for query param to have top priority and can directly check the browser's search attribute for its presence and can add the param to the url upon successful selection from other strategies if it is missing.

eatyourgreens commented 5 years ago

Thanks, that's really useful. I'll have a look in detail tomorrow.

I've added some code in #5124 to handle setting/clearing UPP selected_workflow. My thinking there is that it should be a persistent store across sessions, and use the stored workflow otherwise, but I'm not 100% sure about that.

srallen commented 5 years ago

if a user attempts to specify in invalid workflow (for projects that allow workflow selection, such as for WildCam Darien's classrooms), the user is invisibly redirected to the default workflow which is... not great for the user experience.

If the user is signed in, then order of selection preference would then use and id stored in UPP if there is one, then default if there is one, then random active workflow. If signed out, then it's default workflow if there is one, then random active workflow.

While this scenario would be possible, how likely is it to happen? Projects that allow users to choose on the home page only show active workflows, so an inactive URL would come from the following:

A user bookmark
saved link elsewhere like from an old email

For projects that use classrooms like the Wildcams and Intro2Astro, it wouldn't happen unless the project owner or collab makes the workflow inactive in the lab, which is highly unlikely (has there been a known case of this happening?)

@mcbouslog can clarify, but I believe NfN's landing site is actively updated when workflows become active and inactive.

So the main issue we have is that we could have outdated URLs from bookmarks basically. Instead of informing the user that the workflow they selected could not be loaded since I can imagine that informing the user could cause more confusion (PFE UI message: 'This workflow is inactive; we'll load a different one!' => User: 'What does that mean?!'), we could consider making it more clear which one IS loaded on the classify page perhaps displaying the display name somewhere?

srallen commented 5 years ago

Also, for classrooms projects, the UI does display a message that's basically 'You are classifying as a part of your classroom' so if that isn't present, then the student knows that they aren't classifying in the workflow for their assignment. For Darien, this messaging could be made robust to say "you're classifying as part of your classroom and your assignment :id" or something like that? This isn't applicable for I2A since it's separate projects per assignment, not separate workflows on the same project.

srallen commented 5 years ago

After studying the code today, I see that the home page workflow buttons are making API requests directly. If we're moving to use query param in the URL, then a few more changes should be made.

https://github.com/zooniverse/Panoptes-Front-End/blob/0d0d513e574baa9023d383a8d1057c68515903b4/app/pages/project/home/home-workflow-button.jsx#L19-L34

The click event here should change to only call the UPP update. The React Router link should link to the classify page with the workflow id query param.

WorkflowSelection will then be the only manager of making workflow and workflow translation API requests, so I don't think the componentDidMount in WorkflowSelection would necessarily need to check if a workflow is already requested and loaded: https://github.com/zooniverse/Panoptes-Front-End/blob/0d0d513e574baa9023d383a8d1057c68515903b4/app/pages/project/workflow-selection.jsx#L19-L26

I also think it'd be a good idea to move the actual API requests in the workflow redux store actions, so instead WorkflowSelection could perhaps trigger a redux action instead.

eatyourgreens commented 5 years ago

5124 adds an action creator for loading workflows by ID. At the moment, it's used by the workflow assignment popup and by selecting a workflow on the project home page. If we bring the workflow query param back, then those home page links can just be plain links to workflows with no javascript on click.

shaunanoordin commented 5 years ago

Ah, thanks for the clarification! I totally missed the logic that checks for stored workflows in the user preference.

While this scenario would be possible, how likely is it to happen? Projects that allow users to choose on the home page only show active workflows, so an inactive URL would come from the following: / A user bookmark / saved link elsewhere like from an old email

I would say that if we move ahead with making Workflow IDs more explicit in the URLs, then the probability of the scenario happening increases. There'll be at least some people in Talk or some published newsletter who'll use links to projects/zooniverse/i-fancy-cats/classify?workflow=1234 - instead of the correct projects/zooniverse/i-fancy-cats or the technically-correct projects/zooniverse/i-fancy-cats/classify - simply because ?workflow=1234 will soon be how people see the "correct" URL in their browser nav.

In any case, I think that the response to a user attempting to access an invalid workflow specified in URL should be to inform them that the workflow cannot be accessed (and perhaps link them back to the homepage, where they can see all valid, available workflows), instead of just redirecting them to a UPP-stored/default/random workflow as we do now. Otherwise, users might just see another flavour of the "Hey, this isn't the workflow I was looking for" problem.

Also, for classrooms projects, the UI does display a message that's basically 'You are classifying as a part of your classroom' so if that isn't present, then the student knows that they aren't classifying in the workflow for their assignment.

This is mostly true if the student already knows to expect the "you are classifying as part of your classroom" message. (i.e. they'd need to know what's supposed to be there to realise something is missing.) Recent reports (due to me messing up some Classroom links on WildCam Gorongosa) indicate that some students will just shrug and power through the standard (non-Classroom) Classification interface, thinking "that's just how it's done".

Part of this issue comes from having to better communicate "how things should work" to students/users in the Classrooms tutorials (after all, it wouldn't make sense to display a "hey you're not in a Classroom FYI" message by default on the standard Classification interface), but I nonetheless feel bad for the students who have made Classifications that didn't count towards their homework. 😞

eatyourgreens commented 5 years ago

I don't think the componentDidMount in WorkflowSelection would necessarily need to check if a workflow is already requested and loaded:

WorkflowSelection has been moved to wrap the classify page, rather than the project. So it mounts and tries to run when I return to classifying after checking my notifications or visiting Talk. If there's an active workflow in progress for the current project, workflow selection should still be skipped on mount, otherwise I could be silently redirected to a different workflow without realising it or worse, lose work in progress if I check my messages while in the middle of a classification. https://github.com/zooniverse/Panoptes-Front-End/blob/0d0d513e574baa9023d383a8d1057c68515903b4/app/pages/project/classify.jsx#L357-L373

We could move WorkflowSelection back to wrap the entire project, if that would make preserving classification state easier to manage.

srallen commented 5 years ago

@eatyourgreens I'll check out #5124 today, however, I think the click handlers in those cases still should call to update the UPP. Later we can think about adding a preferences redux store and actions to handle that.

We could move WorkflowSelection back to wrap the entire project, if that would make preserving classification state easier to manage.

No, what you said makes sense; I'm still working on my understanding of what the code does now. I don't think it'll be easier to manage to have it at the project level and in fact having it only wrap the classifier is closer to what the rewritten classifier does.

eatyourgreens commented 5 years ago

5124 updates the user preferences just before loading the workflow, saves the updated preferences if the workflow loads successfully but rolls back the update if there's an error in the API request.

srallen commented 5 years ago

In any case, I think that the response to a user attempting to access an invalid workflow specified in URL should be to inform them that the workflow cannot be accessed (and perhaps link them back to the homepage, where they can see all valid, available workflows), instead of just redirecting them to a UPP-stored/default/random workflow as we do now.

I disagree for the purposes of resolving the current bugs. The behavior for PFE workflow selection since it has launched has been to try to attempt to load something. In early days, it was attempt to load by query param, then attempt to load a random workflow. Now we have the addition of attempts for workflow ids stored in UPP or set in the project config for default; however, this is the established behavior and I would argue that the users are more or less trained for this since PFE launch in 2015. And it still may be desirable in the future to have this fallback behavior for certain cases so that the user is at a UI where they can classify something even if it was from a URL with a now inactive workflow id in it. We also can check how confusing or annoying this may be with the NfN team since they use query params now as the first loading strategy for workflows if this is a major concern.

Making this kind of behavior and UI change is the snowballing I was worried about. I think you bring up some valid concerns and good ideas, but I think this kind of change should be discussed within the context of the rewrite and not be a part of the bug fix for current PFE. For the rewrite, I do want to have a prompt to ask the user what they want to do if the workflow is complete for instance. This can expand into a greater discussion about when the user should be prompt to select a workflow on various conditions.

shaunanoordin commented 5 years ago

Making this kind of behavior and UI change is the snowballing I was worried about. I think you bring up some valid concerns and good ideas, but I think this kind of change should be discussed within the context of the rewrite and not be a part of the bug fix for current PFE. For the rewrite, I do want to have a prompt to ask the user what they want to do if the workflow is complete for instance. This can expand into a greater discussion about when the user should be prompt to select a workflow on various conditions.

This is a good point. I retract my comments about having to address the 'invalid Workflow selection' issue for the current PFE bug fix, and will instead push the issue as something to consider/improve on in the Front-End-Monorepo project. 👌

srallen commented 5 years ago

@shaunanoordin If you have the time, would you open an issue on the front-end-monorepo and reference or copy over comments from here? That'd be great to have once we shift back to dev work there.

rogerhutchings commented 5 years ago

Actually, this should probably be an ADR

srallen commented 5 years ago

I'd like to circle back to this and perhaps continue discussion in the form of an ADR with the rewrite. I think actually using the query param may be desirable if the classifier is in its own app versus the home page where we have the workflow selection buttons rendered, so we'll have to have a way to communicate that state between the apps.

stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] commented 5 years ago

Closed by probot-stale due to a lack of recent activity. Please feel free to re-open if you wish to take on this change.

zooniverse / Panoptes-Front-End

[RFC] Removing ambiguity from Workflow selection #5125

Request for Comments: how to remove ambiguity in PFE Classifier's Workflow selection

Additional Notes

Status / Where we stand right now

5124 updates the user preferences just before loading the workflow, saves the updated preferences if the workflow loads successfully but rolls back the update if there's an error in the API request.