How should FEM Classifier get the User resource?

[shaunanoordin]

I want FEM's lib-classifer to know if and which user is currently logged in. Unfortunately, only app-project is aware of the user resource, due to current separation of responsibilities.

As such, I want to discuss how lib-classifier could best get access to said resource. Does anyone have any thoughts in this? You can skip down to the "Solutions" bit to see what I'm proposing.

Context

As part of the (insert name of grant here) grant's work on the concept of "serendipitous discovery", I'm building a basic Talk interface into the Classifier. (Henceforth called Quicktalk; see PR 2324 for technical details.)

The QuickTalk component needs to know two things: 1. which Subject the user is currently classifying (hence, this component must live in lib-classifier) and 2. which user is logged in, IF we want to allow users to post comments about the Subject (we do want to allow users to post comments, so this is where the problem comes in.)

Notes:

• Yep, the QuickTalk feature is basically allows us to skip the "Done" in "Done & Talk".
• This is currently a very experimental feature.

The Problem(s)

• Our FEM packages are built around the idea of modularity separating responsibility.
• Our current thinking is, for example, that lib-classifier shouldn't need to know who's logged in, because the user identity isn't essential to the Classification process, and the Classifier should be generalised so it can be plugged into any theoretical front-end project (not just Zooniverse.org)
• I've just found a use case that contradicts "lib-classifier shouldn't need to know who's logged in"

The Solutions?

I have two ideas on how to get around this.

Option 1: pass the User resource from app-project down to lib-classifier

In app-project/ClassifyPage...

<ClassifyPage>
  <ClassifierWrapper
    workflowID
    subjectSetID
    subjectID
    userResource /* Just add this! */
  />
</ClassifyPage>

• Concept: the container for the Classifier should be responsible for managing the User resource.
• Pro: we still maintain division of responsibilities. app-project takes care of the user resource, as god.exe intended.
• Con: we're now adding additional linkages between two "mostly independent" packages.
  • Counterpoint: actually, the userResource is optional, so it's not a hard linkage. Other theoretical front ends can still use the classifier without specifying userResource. Documentation will reflect this.
• Challenge 1: passing the userResource within the Classifier, between its subcomponents, will require some thought.
  • I'm thinking of tossing it into a "Classifier.UserContext" which can then be picked up by any Classifier component that cares about the User (instead of using stores)
• Challenge 2: gotta make sure the userResource is updated correctly when user logs in/out

Option 2: lib-classifier should just fetch the user resource again if it cares so darn much about who's classifying

...Wait, actually, lib-classifier actually DOES already fetch the user resource, separate from whatever app-project is doing.

In the UserProjectPreferenceStore, we see that the checkForUser() function pulls the currently-logged-in user resource just to get their user preference, but doesn't proceed to save the user details.

I can create a separate UserStore in lib-classifier that stores said user data, and we can then call it a day.

• Pro: app-project and lib-classifier continue to be (mostly) independent from each other.
• Pro: no changes required on app-project
• Con: we'll be doing the same work (of fetching the user resource) twice.
  • Counterpoint: erm, we already are.

[srallen]

@shaunanoordin thanks for bringing this up. I noted that we're duplicating the request for the UPP in this issue since it pertains to the workflow assignment feature: #2205

My thoughts there was actually to remove it from the classifier stores and instead pass in specific functions as props to the classifier for updating the tutorial seen timestamp because that's the only thing it is used for in the classifier and keep the responsibility of loading the UPP in the project app.

I'm a little wary of making big changes to the classifier for something that is very experimental. For this, what exactly do you need from the user resource? Is it the id, display name, or something else?

[shaunanoordin]

(1) Ok, so for the purposes of the prototype, would using auth.checkCurrent be sufficient? ... Then, once we run a project using this experiment and if it decided this will be a permanent feature we can return to discussion on how to permanently add knowledge of the user to the classifier ...

👍 This works for me!

• I'll go ahead and make the User resource check self-contained to the QuickTalk experimental prototype for now.
• I'll make a note in the README that this "duplication of work" (triplication?) on fetching the user resource is on purpose, for the experiment stage.
• I also need to make a note in the README that this needs to be ripped out when we get to the long-term solution.

(2) As for removing the UPP from the classifier, I was going to write up a specific issue for that and propose the PR...

Noted, for my work, I won't touch the UPP store in the classifier, to keep things clean.

(3) My suggestion still has the noted challenge of handling the sign out condition. I don't recall how the classifier stored UPP handles this... would you be able to check this and note that here?

Can do! I've already taken a look at this, and I'm 70% sure the login/logout condition is checked as a side-effect check on authClient changing... (code)

  useEffect(function onAuthChange() {
    userProjectPreferences.checkForUser()
  }, [authClient])

...but I'll test this to confirm and update this thread. (I'm still scratching my head at why authClient itself would change on login/logout, but guess I'll find out.)

[shaunanoordin]

Oh, this is interesting. @srallen:

• Classifier's authClient is actually passed in via a param from app-project, then stored in the stores.
• There's an Effect in the root <Classifier> component that detects when authClient changes to determine log in/log out changes.
  • This should in theory observe when the User resource has changed, particularly for the purpose of the UserProjectPreference store.
• However, this is actually how it works:
  • When the page loads, the Effect kicks in and checks for user status ✅
  • When the user clicks "Sign In" and logs in successfully, the Effect kicks in ✅
  • When the user clicks "Sign Out", the Effect does NOT kick in, and therefore does NOT detect the user has logged out ❌
  • But when the user proceeds to click on "Sign In" (and the Sign In form appears, but before the user has the chance to type in anything), the Effect DOES kick and therefore detects the user has logged out ❓❓

Testing Method

1. In lib-classifier's useEffect(function onAuthChange() { ... } add some debug tracking code
2. Bootstrap, etc etc, run app-project
3. Open any project on local.zooniverse.org:3000 in Chrome or FF
4. Repeat the steps listed in "this is actually how it works" , above. ☝️ Observe changes in dev console.

I'm doing some additional testing to see if UPP is listening for anything else other than the authClient changes (and, uh, Project resource changes, I think) to double-check if it's properly detecting when it should update the "current user" state.

Consequences

• My QuickTalk component needs to be a bit careful to always check for the "current user"/logged-in state before posting, since it's possible for a user to logout, then click the "post" button. (Easily handled.)
• Not sure if there's any practical effect on a logged out user maintaining the previous user's UserProjectPreferences. (I need to figure out when the UPP matters.)

Also curious as to why app-project ain't propagating the changes down to properly but that feels like a deeper debug. The authClient param from app-project to lib-classifier is supposed to be a workaround anyhoo.

[srallen]

Thanks. The behavior is curious and perhaps isn't exactly doing what we expect. We should verify if the UPP resource is being reset on user change.... or plan on continuing on with removing it. I've created an issue to track this in #2382.

[eatyourgreens]

There's an Effect in the root component that detects when authClient changes to determine log in/log out changes.

Good catch, that's a bug that I introduced in #2101. Prior to that, it ran on every Classifier re-render.

front-end-monorepo/packages/lib-classifier/src/components/Classifier/Classifier.js

Lines 74 to 91 in 9a62abf

	componentDidUpdate (prevProps) {
	const { authClient, project, subjectSetID, workflowID } = this.props
	if (project.id !== prevProps.project.id) {
	this.setProject(project)
	}
	if (workflowID !== prevProps.workflowID) {
	this.selectWorkflow(workflowID, subjectSetID)
	}
	if (subjectSetID !== prevProps.subjectSetID) {
	this.selectWorkflow(workflowID, subjectSetID)
	}
	if (authClient) {
	this.classifierStore.userProjectPreferences.checkForUser()
	}
	}

I guess that action refreshes tokens for volunteers with sessions that run for longer than 2 hours? If it isn't running regularly, then volunteers with long-running sessions might find that their classifications are being submitted anonymously.

I'm not sure how often the top-level Classifier updates. In PFE, we check for expired sessions every time 'Done' is pressed, so that might be a better place to run this check.

[eatyourgreens]

I've opened #2394 to fix that.

[eatyourgreens]

Future work to keep in mind: interventions in the classifier (when you press Done) use user.intervention_notifications to check whether you've opted-out of notifications.
https://github.com/zooniverse/Panoptes-Front-End/blob/671cd9418219e868f2379babb1e7079c9fe0eecb/app/classifier/components/Intervention/Intervention.jsx#L29-L34

[srallen]

The interventions feature is in a "build it if it gets asked for" category since no project is actively using it.

[eatyourgreens]

I've added a usePanoptesUser hook in #2940, so that we can check user roles before loading inactive workflows. It's a wrapper for authClient.checkCurrent(), which makes an authenticated request to GET https://www.zooniverse.org/api/me.

[eatyourgreens]

#4803 updates the classifier to get the current user from auth.checkBearerToken(), falling back to auth.checkCurrent() when we haven't established a session yet (from the Panoptes session cookie.)

Panoptes session cookies last for 2 weeks (I think.) You can refresh a bearer token, which lasts for 2 hours, as long as the session cookie is valid.

[eatyourgreens]

#4838 updates the classifier store when you log in and log out.

[eatyourgreens]

#4803 now adds a user loading state to the classifier, so that we don't start fetching workflows etc. before we've authenticated to Panoptes with the _Panoptes_session cookie. That should fix weird bugs where the classifier fetches subjects twice from /subjects/queued (#4875.)

[eatyourgreens]

See #4845.