Skip to content

Remove redundant "remaining" value from session timeout API (Part 1)#8084

Merged
aduth merged 4 commits intomainfrom
aduth-rm-session-active-remaining
Mar 29, 2023
Merged

Remove redundant "remaining" value from session timeout API (Part 1)#8084
aduth merged 4 commits intomainfrom
aduth-rm-session-active-remaining

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Mar 28, 2023

🛠 Summary of changes

Extracted from #7966

Removes the remaining value from the session active / keepalive endpoints. This is an iteration toward a refactoring of session timeout behaviors, partially implemented in #7966.

The remaining value is unnecessary since, just as it was computed server-side, it can be derived on the client-side by comparing the timeout and current time. Arguably this would produce a more accurate value, since it would not be subject to time drift delays in the same way that a network response would be.

📜 Testing Plan

  1. Sign in
  2. Wait for session timeout modal
  3. Observe that redirect happens once countdown reaches 0
    • This is an effective test since the timeRemaining is used to schedule the final polling request if the session inactivation would occur sooner than the regularly scheduled poll

It's easier to test by setting a very low session timeout in local config/application.yml:

session_timeout_in_minutes: 1

changelog: Internal, Session Timeout, Remove redundant timeout value from session timeout API
@aduth aduth changed the title Remove redundant "remaining" value from timeout API Remove redundant "remaining" value from session timeout API Mar 28, 2023
Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

@aduth
Copy link
Contributor Author

aduth commented Mar 28, 2023

I'll take a closer look at the failing spec this afternoon. I suspect it's more to do with how it's stubbed in the feature specs, and the increased precision of the new timeRemaining (i.e. precision at the milliseconds level rather than at the seconds level, conflicting with the use of < operator).

1. Fixes specs
2. Better user experience, don't have to wait for request to finish before modal is closed, thus making app feel more responsive
3. The previous result handling with `success` would cause a second polling interval to begin
@aduth
Copy link
Contributor Author

aduth commented Mar 28, 2023

I'll take a closer look at the failing spec this afternoon. I suspect it's more to do with how it's stubbed in the feature specs, and the increased precision of the new timeRemaining (i.e. precision at the milliseconds level rather than at the seconds level, conflicting with the use of < operator).

This is exactly what the issue was. I tried a few different approaches to resolving it, most of which required making the test run a little longer in real time to wait for the modal to appear.

I went a different direction in 97265e2, which is something I was already proposing in #7966. Essentially, the idea is to close the modal immediately on click and optimistically assume a successful response. This makes the app feel more responsive, and also avoids a bug where a duplicate polling interval is started for every time the user clicks "Keep me signed in". In the worst case, the next polling ping 30 seconds later would show the modal again if the keepalive request was unsuccessful for some reason.

The CSRF won't be updated until after the request finishes, even if the modal will be disappeared immediately
Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

@aduth aduth changed the title Remove redundant "remaining" value from session timeout API Remove redundant "remaining" value from session timeout API (Part 1) Mar 29, 2023
@aduth aduth merged commit fa59166 into main Mar 29, 2023
@aduth aduth deleted the aduth-rm-session-active-remaining branch March 29, 2023 13:18
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.

2 participants