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

SessionRenewal: Rework to probe server to warn about session expiration instead of using heuristic #2377

Merged
merged 1 commit into from Jan 29, 2019

Conversation

Projects
None yet
2 participants
@kevinrobinson
Copy link
Contributor

kevinrobinson commented Jan 28, 2019

Follow-on to #2370 and #2366.

Who is this PR for?

educators

What problem does this PR fix?

There were some user reports of being signed out unexpectedly, and in investigating and in looking at Rollbar errors related to .slice in parsing dates, this looks like it's coming from users interacting with the server (eg, to save a note) after the session has expired. In that case, it means the educators loses what they just typed in.

What does this PR do?

Reworks <SessionRenewal />. The core loop is probing the server to see if the user is still signed in. This is with a new /educators/probe endpoint that skips the normal Devise Timeoutable behavior of keeping the session alive. This endpoint returns how much time is left in the session, and then <SessionRenewal /> shows an expiration notice if it's within the warning window. Renewing the session makes the same request to the server as before.

This works better than the previous approach because it avoids UI-side heuristics, and just checks with the server. If the session has already expired, the UI redirects to close the page, since any data from interactions would be lost anyway. The expiration message should make it clear to users this would happen beforehand, and this PR changes the background color to a warning orange instead of blue.

This revises the Rollbar instrumentation and keeps it in place, so we can verify this is working as expected.

Checklists

Which features or pages does this PR touch?

  • Core

Does this PR use tests to help verify we can deploy these changes quickly and confidently?

  • Included specs for changes
  • Manual testing made more sense here
@studentinsights-bot

This comment has been minimized.

Copy link

studentinsights-bot commented Jan 28, 2019

@kevinrobinson, this looks like it might be worth double-checking! @kevinrobinson might be able to help.

@kevinrobinson kevinrobinson changed the title Patch/session renewal probe SessionRenewal: Rework to probe server to warn about session expiration instead of using heuristic Jan 28, 2019

Rewrite SessionRenewal to remove UI-side heuristic
Fixup SessionRenewal JS specs

@kevinrobinson kevinrobinson force-pushed the patch/session-renewal-probe branch from 0c85132 to 89f104d Jan 29, 2019

@kevinrobinson

This comment has been minimized.

Copy link
Contributor Author

kevinrobinson commented Jan 29, 2019

selfie

@kevinrobinson kevinrobinson merged commit 810f1ac into master Jan 29, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@kevinrobinson kevinrobinson deleted the patch/session-renewal-probe branch Jan 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment