-
Notifications
You must be signed in to change notification settings - Fork 1
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
Hotfix/expired tokens #369
Conversation
@@ -244,6 +244,11 @@ async function fetchData( | |||
rawResponse: response, | |||
}); | |||
} catch (error) { | |||
if (error.response.status === 401) { |
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.
Is the user then going to be presented with the login automatically again?
In that case, a message would be useful such as "Session expired, please login again".
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.
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 can't see the screenshot, the upload doesn't seem to have worked
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.
Hi @mberacochea let me know if you can see the screenshot now
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.
yeah, I can see the message now. IMO a VF info banner would be better, they are less obstructive.
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 @mberacochea I've added the banner as suggested.
made a couple more changes to make the login form use a primary button and a loading spinner
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.
Brilliant - thank you @MGS-sails ! One small query inline about possible race condition, otherwise happy to merge.
src/pages/Login/index.tsx
Outdated
@@ -82,16 +85,21 @@ const Login: React.FC = () => { | |||
setAuthToken(accessToken) as unknown as void; | |||
setUsername(''); | |||
setPassword(''); | |||
if (localStorage.getItem('mgnify.sessionExpired')) { | |||
localStorage.removeItem('mgnify.sessionExpired'); |
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.
do we need to await
this to avoid some race condition?
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 @SandyRogers actually I had meant to remove this part of the code, just pushed another commit to do that now.
I came to the conclusion that this part is not necessary. Trying to do this kind of opened a small can of worms.
The users can always just dismiss the banner. So I thought it may not be necessary to do this at this point in time
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.
Ah, perfect. Thanks!
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.
No description provided.