-
Notifications
You must be signed in to change notification settings - Fork 2
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
Refactor the remaining pieces of the sign-in process #1512
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks @terrazoon! On the whole this looks good to me and it appears like it'll at least address the issue, though I still need to test locally. I had a few comments and questions that I've left in the mean time as I continue going through things and others get a chance to take a look.
By all accounts it looks like this takes care of everything in #1459 too, except possibly some E2E test things with the Login.gov integration, but that's fine to save for another issue (which we should have, I'll check the board).
Nicely done with the test cleanup and updates as well, thank you! 🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One final change and I think we're good to merge this to staging for final testing - all looked good to me locally, but I haven't been able to replicate the issue locally it seems. Only in staging or production.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @terrazoon! Let's get this into staging for some more testing!
Description
Security Considerations