Skip to content
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

Classifier: Remove the faulty session id code #7233

Merged
merged 2 commits into from
Dec 10, 2024

Conversation

goplayoutside3
Copy link
Contributor

@goplayoutside3 goplayoutside3 commented Dec 9, 2024

Staging branch URL: https://pr-7233.pfe-preview.zooniverse.org

Paired with zooniverse/front-end-monorepo#6499

  • While investigating Standardize third party libraries for generating string hash and unique ids front-end-monorepo#6493, I found that the "generate a new session id after 5 minutes of inactivity" logic does not work in the classifier. As mentioned in that Issue, session id is attached to classification metadata and can be used for analysis of anonymous users contributing classifications.
  • In this PR I removed the "5 minutes" logic because its original condition of comparing a Date() string to a Date.now() number was never met, and therefore the code never worked as intended.

Required Manual Testing

  • Does the projects page render correctly?
  • Can you load project home pages?
  • Can you load the classification page?
  • Can you submit a classification?
  • Does talk load correctly?
  • Can you post a talk comment?

Review Checklist

  • Does it work in all major browsers: Firefox, Chrome, Edge, Safari?
  • Does it work on mobile?
  • Can you npm ci and app works as expected?
  • If the component is in coffeescript, is it converted to ES6? Is it free of eslint errors? Is the conversion its own commit?
  • Are the tests passing locally and on GitHub Actions?

try
storage.setItem('session_id', JSON.stringify(stored))
stored

getSessionID = () ->
{id, ttl} = JSON.parse(storage.getItem('session_id')) ? generateSessionID()
if ttl < Date.now()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This condition was comparing a Date() string to a Date.now() number and therefore never met. It was copied directly from PFE's codebase, so I'll be doing the same cleanup over there.

@coveralls
Copy link

Coverage Status

coverage: 56.684% (-0.02%) from 56.705%
when pulling 17ed9d5 on remove-faulty-session-ttl
into 39a09e8 on master.

@@ -1,29 +1,17 @@
storage = window.sessionStorage ? window.localStorage
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feature is supposed to be a session id, which refreshes just like a browser session. If researchers are using session ids to analyze anonymous users, then don't use localStorage as a backup.

See https://github.com/zooniverse/front-end-monorepo/pull/6499/files#r1861103668

@goplayoutside3
Copy link
Contributor Author

@shaunanoordin I'm requesting your review on this one as a pair to zooniverse/front-end-monorepo#6499. I've done my best educated guess about coffeescript, but need a second set of eyes on the changes.

Copy link
Member

@shaunanoordin shaunanoordin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review

Related: FEM PR 6499

This PR removes some code in our session ID-generating function. The removed portion was never working, so no actual changes in website behaviour are expected.

Behaviour:

  • a session ID (for a browser tab/window) is generated when any PFE page loads.
  • this session ID stays for the duration of the browser session (i.e. until the tab/window closes)
  • this session ID is only used when submitting classifications (i.e. on a project's Classify page). The session ID is submitted as classification.metadata.session = "some_string"

Note:

  • in theory it was possible for the generated session ID to be stored/pulled from the longer-term local storage instead of short-term session storage, but in practice this should never have realistically happened. (i.e. unlikely we need to worry about a browser with no session storage, but with local storage.) Nonetheless, the removal of the "window.localStorage" fallback is desirable to keep the code intent clear: session IDs should only live in the session.

The code changes look good, and I see no particular issues with the new Coffeescript. Functionality/behaviour tests also look good.

Testing

Tested on localhost with macOS + Chrome, on an incognito window.

Status

LGTM! 👍

@goplayoutside3 goplayoutside3 merged commit a854eb3 into master Dec 10, 2024
5 checks passed
@goplayoutside3 goplayoutside3 deleted the remove-faulty-session-ttl branch December 10, 2024 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants