Classifier: check user auth on any change

[eatyourgreens]

Prior to #2101, the Classifier checked the logged-in user on every component update. This restores that behaviour by removing the authClient dependency from useEffect.

Package:
lib-classifier

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?

[shaunanoordin]

PR Review

Package: lib-classifier
Context: part of the "how should/does the Classifier handle the User resource?" discussion
Follows: this discussion comment

This PR ensures the Classifier's "who's the user right now?" check triggers any time the <Classifier> component updates (gets new params), not just when the authClient param changes.

• Previously we were monitoring authClient because we assumed that's how we observe when a user logs in/out(, I assume that's what we were assuming (?))
• However, we recently realised that we're not detecting when a user logs out correctly. (See this investigation)
• ⚠️ Monitoring every component update to <Classifier> SHOULD in theory solve this, but we're still not correctly detecting when a user logs out (???)

Please see Testing for details.

Testing

Tested on app-project (not lib-classifier) on localhost (any staging project on local.zooniverse.org:3000) with macOS 11.5.2 + Chrome 92

• Pre-test setup:
  • added some debug code to monitor when the <Classifier> component updates

    useEffect(function onUpdate() {
      console.log('+++ Classifier.onUpdate()')
      userProjectPreferences.checkForUser()
    })
    

  • Optional: modify Classifier's MetaTools to print {upp?.id} to give a visual indicator of when UPPs change
  • yarn bootstrap at root then yarn dev on app-project. Open the project on Chrome in an Incognito window.
• Go to the Classify page. When Classify page loads, Classifier.onUpdate() triggers ✅
• Click the Sign In button to open the Sign In form. This immediately triggers Classifier.onUpdate(). (This is NEW behaviour from this PR's changes)
• Actually sign in. Classifier.onUpdate() triggers ✅
• Go to the Classify page. When Classify page loads, Classifier.onUpdate() triggers ✅
• Make some annotations and submit some classifications. Classifier.onUpdate() does NOT trigger, as expected (phew) ✅
• Click the Sign Out button. Classifier.onUpdate() still does NOT trigger as expected. ❌
• Click the Sign In button to open the Sign In form. This immediately triggers Classifier.onUpdate() (this is EXISTING behaviour, believe it or not)

Note: if MetaTools was optionally modified, we can see that the {upp?.id} provides additional confirmation to the trace above. i.e. after clicking "Sign Out", the upp.id still shows, until you click "Sign In" again.

Status

If our goal for this PR is to correctly observe when a user logs out, then I don't think the code changes in lib-classifier's <Classifier> are enough.

I suspect it may be something one level up, at app-project's <ClassifierWrapper>. I know that the Sign Out action simply clears (User.clear()) the user's id, display_name, and login, and these things may not be listened for properly on the ClassifierWrapper. (Hence the wrapper won't in turn tell Classifier to update when user signs out)

I'm going to investigate my theory on app-project, but please let me know if it's useful in any case to get this PR approved. (e.g. if there's something else this change is supposed to fix, since I was very focused on only testing for User Sign In / Sign Out events.)

[eatyourgreens]

This PR restores the behaviour of the original class component, but that could have been broken too.

[eatyourgreens]

I did some looking around in the classifier code this week, and submitting a classification also checks/refreshes the current token.

front-end-monorepo/packages/lib-classifier/src/store/utils/ClassificationQueue.js

Lines 63 to 65 in 657ffbc

	return getBearerToken(this.authClient)
	.then((authorization) => {
	return Promise.all(pendingClassifications.map((classificationData) => {

[srallen]

Since this doesn't resolve the problem @shaunanoordin noted in the discussion, then could this close? I think the path forward is #2382

[eatyourgreens]

Sure. I’m kind of curious as to why the original code called checkForUser on every render but accidentally removing that doesn’t seem to have actually broken anything.