Skip to content

LG-14753 Bug fix for sign-in page losing user progress#11783

Merged
kevinsmaster5 merged 21 commits intomainfrom
kmas-lg-14753-sign-in-page-extendable
Feb 6, 2025
Merged

LG-14753 Bug fix for sign-in page losing user progress#11783
kevinsmaster5 merged 21 commits intomainfrom
kmas-lg-14753-sign-in-page-extendable

Conversation

@kevinsmaster5
Copy link
Contributor

@kevinsmaster5 kevinsmaster5 commented Jan 22, 2025

🎫 Ticket

Link to the relevant ticket:
LG-14753

🛠 Summary of changes

Adds a modal to sign in page to keep user progress. Utilizes similar feature as for a logged-in user that informs them their session is about to expire and be logged out.

📜 Testing Plan

(to make testing quicker set session_timeout_in_minutes: 5 in local application.yml)

  • Go to http://localhost:3000
  • Enter email, password but do not click sign-in
  • Wait for timeout to get close to elapsing
  • Observe that a modal timeout warning countdown appears
  • Click Continue Sign In
  • Note form values remain
  • Click Cancel Sign In
  • Note the page reloads and form is cleared

@kevinsmaster5 kevinsmaster5 marked this pull request as ready for review January 27, 2025 21:37
@kevinsmaster5 kevinsmaster5 requested a review from a team January 28, 2025 15:00
@kevinsmaster5 kevinsmaster5 force-pushed the kmas-lg-14753-sign-in-page-extendable branch from ba1eb66 to f11fae2 Compare February 4, 2025 18:15
fill_in 'Password', with: user.password
context 'create account' do
it 'shows the timeout modal when the session expiration approaches', js: true do
allow(Devise).to receive(:timeout_in).and_return(160)
Copy link
Contributor

Choose a reason for hiding this comment

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

If I'm following this correctly, this test will take 10 real-world seconds to complete, so the three tests here will add 30 seconds delay to every CI build. It also implicitly relies on the default session timeout warning config value being 150 seconds, so the test might break if we changed that default.

I'd suggest:

  • Minimizing the time it takes for the test to run. Could this be 151 seconds instead of 160, for example?
  • Avoiding implicit dependency on the default config value by either calculating the value using the actual config (IdentityConfig.store.session_timeout_warning_seconds + 1), or by stubbing the config value to gaurantee that it's the 150 we're expecting (expect(IdentityConfig.store).to receive(:session_timeout_warning_seconds).and_return(150))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree the timing was creating some challenges getting the fields populated before the modal showed up. 151 seems to work OK but am I still dragging the duration with wait: 10?

Copy link
Contributor

Choose a reason for hiding this comment

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

The wait should be okay, since it'll wait up to 10 seconds, but won't hold up the rest of the test if it finds what it's expecting sooner than that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you also make the change from the second bullet point in #11783 (comment) ? We're still hard-coding the value implicitly depending on the current configuration default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I got that now.

Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this change makes sense considering what the spec is testing, but I'm surprised why it's suddenly necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it was a similar timing problem that got created by adding the modal to the sign-in page.
Specifically you get an element not interactable error

 Selenium::WebDriver::Error::ElementNotInteractableError:
       element not interactable
         (Session info: chrome=132.0.6834.160)

I figured that was because the modal is in the way and it can't fill in the form in the helper.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm I wonder if that was just a flakey test failure. I've seen those before with these modals. Looking at what sign_in_before_2fa and sign_in_user are doing, I can't really imagine it should make much of a difference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like by adding the session timeout it no longer has that error.

@kevinsmaster5 kevinsmaster5 force-pushed the kmas-lg-14753-sign-in-page-extendable branch from 290c57a to 55c355a Compare February 6, 2025 18:50
@kevinsmaster5 kevinsmaster5 merged commit e02df31 into main Feb 6, 2025
@kevinsmaster5 kevinsmaster5 deleted the kmas-lg-14753-sign-in-page-extendable branch February 6, 2025 19: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