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

Simplify session timeout modal presenter #7985

Merged
merged 3 commits into from
Mar 15, 2023

Conversation

aduth
Copy link
Member

@aduth aduth commented Mar 14, 2023

Extracted from #7966

🛠 Summary of changes

Simplifies the behavior of the session timeout modal presenter:

  • Flatten separate and largely repeated classes FullySignedInModalPresenter and PartiallySignedInModalPresenter to a single SessionTimeoutModalPresenter
    • Name presenter such that it's obvious its purpose in handling session timeout behaviors
  • Move view rendering responsibilities out of presenter class and into view partial, limiting scope of the presenter class to handle differences between "fully" and "partially" signed-in users
  • Use standard design system buttons implemented through ButtonComponent
  • Avoid unclear abbreviation "sr" in favor of live_region_ qualifier describing its context in a ARIA live region, which notably can benefit more than just screen readers
  • Avoid setting initial expiration time in modal content, expecting that this will be assigned by the session timeout JavaScript [1] [2]

📜 Testing Plan

Confirm no regressions in the behavior of the session timeout modal, including specifically:

  • Differences between "partially" and "fully" signed in (i.e. before and after MFA authenticated)
  • "Stay signed in" and "Sign out" buttons work as expected
  • Modal text counts down

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

session_timeout_in_minutes: 1

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 and others added 3 commits March 14, 2023 16:45
changelog: Internal, Refactoring, Simplify logic for session timeout modal behavior
See: https://github.com/18F/identity-idp/pull/7985/files#r1136148250
Co-Authored-By: Zach Margolis <zachmargolis@users.noreply.github.com>
@aduth aduth force-pushed the aduth-session-timeout-modal-presenter branch from 0a76851 to 857e81f Compare March 14, 2023 20:48
@aduth aduth merged commit 244a837 into main Mar 15, 2023
@aduth aduth deleted the aduth-session-timeout-modal-presenter branch March 15, 2023 14:40
svalexander pushed a commit that referenced this pull request Mar 16, 2023
* Simplify session timeout modal presenter

changelog: Internal, Refactoring, Simplify logic for session timeout modal behavior

* Mark translations as used

See: https://github.com/18F/identity-idp/pull/7985/files#r1136148250
Co-Authored-By: Zach Margolis <zachmargolis@users.noreply.github.com>

* Rename locale_scope as translation_scope

See: https://github.com/18F/identity-idp/pull/7985/files#r1136148250

---------

Co-authored-by: Zach Margolis <zachmargolis@users.noreply.github.com>
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