-
Notifications
You must be signed in to change notification settings - Fork 56
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
fix: Show login modal when user session expires. #2541
fix: Show login modal when user session expires. #2541
Conversation
1fd712a
to
6382e5e
Compare
Improved this PR to comprise all possible expired sessions errors, by improving the |
This commit improves the user session control, by displaying the login modal when some user session expires.
1739c58
to
360ce97
Compare
360ce97
to
03f8c13
Compare
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.
This currently displays the modal as soon as the session expires. This is not quite what we want. The modal should be displayed once the session expires AND the user attempts to access a route that requires the user to be logged in (ex. proposal submission, comment submission, etc).
@lukebp handling all requests and displaying the login modal for them will require an extensive refactor because we need to change the way we handle auth errors on forms and other private actions. The Login modal display is an async procedure, so all 403 errors would be displayed in the form error message before the login modal shows up, just like other response errors. I opted to go for this approach because this way we can avoid those syncing problems mentioned above. After talking to @tiagoalvesdulce, we both agreed that it should be included in the new architecture. |
Ok, that's fine. We definitely don't want to display the login modal as soon as the session expires though. We've done this before and it was very annoying for users because they would leave tabs open when reading proposals and would constantly have to log back in. We have two options:
or
Thoughts? |
1 for me is the best. |
Ok, go with option 1 then. |
I'm OK with option 1. |
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.
tACK
@victorgcramos can you fix the merge conflicts? This needs final approval from @tiagoalvesdulce. |
Let's wait for #2549 first. |
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.
LGTM
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.
Good job, tested and works fine.
I suggest we show the modal also on comment vote.
Also, added a minor inline comment.
password: "password" | ||
}; | ||
cy.login(user); | ||
cy.intercept("/api/ticketvote/v1/inventory").as("inventory"); |
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.
want to add this as a middleware ?
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.
I'd wait until the mocked api structure is done to mock all requests. This is just an alias without any response changes.
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.
Sweet
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.
nice work, LGTM!
Please update your commit message. Same comment that I said here. |
Close #2538
This diff fixes an issue that reported the absence of some login modal
when some user submits a proposal with an expired session.
Dependencies
Depends on decred/pi-ui#369
UI Changes Screenshot