-
Notifications
You must be signed in to change notification settings - Fork 4
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
DCJ-668: Bug Fixes for Terms of Service Acceptance #2671
Conversation
@@ -118,13 +128,6 @@ export const SignInButton = (props: SignInButtonProps) => { | |||
|
|||
const shouldRedirectTo = (page: string): boolean => page !== '/' && page !== '/home'; | |||
|
|||
const attemptSignInCheckToSAndRedirect = async (redirectTo:string, shouldRedirect: boolean) => { | |||
await checkToSAndRedirect(shouldRedirect ? redirectTo : null); |
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 moved this line to onSuccess
to simplify the logic checks
Metrics.identify(Storage.getAnonymousId()); | ||
Metrics.syncProfile(); | ||
Metrics.captureEvent(eventList.userSignIn); |
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 moved these to checkToSAndRedirect
in the place where we know the user is authenticated and accepted ToS.
await Auth.signOut(); | ||
// User has authenticated, but has not accepted ToS | ||
Storage.setUserIsLogged(false); |
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 is the fix that allows for an authenticated user to access /tos_acceptance
if (oidcUser !== null) { | ||
Storage.setUserIsLogged(true); | ||
} else { | ||
await Auth.signOut(); | ||
} |
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 fix allows for page refreshes without signing out the user.
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.
looks good 👍
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.
changes look good
Addresses
https://broadworkbench.atlassian.net/browse/DCJ-668
Summary
#2657 introduced a couple of regressions that this PR fixes.
Suggested that you hide whitespace changes due to the large number of lint fixes.
Have you read Terra's Contributing Guide lately? If not, do that first.