Skip to content
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(form-v2): remove additional log in step when switching from AngularJS to React #4459

Merged
merged 4 commits into from
Aug 3, 2022

Conversation

wanlingt
Copy link
Contributor

@wanlingt wanlingt commented Aug 2, 2022

Problem

User appears logged out after switching from AngularJS to React

Closes #4287

Solution

  • Instead of refreshing the /#!/forms page, navigate to /workspace
  • Query for user to ascertain logged in state

Breaking Changes

  • No - this PR is backwards compatible
    Features:

Before & After Screenshots

BEFORE:
Recording 2022-08-02 at 12 16 09

AFTER:
Recording 2022-08-02 at 13 04 42

Tests

  • Make sure is-logged-in is not present in localStorage.
    Log in to the AngularJS app. Click the banner message to switch to React.
    You should be brought to the Workspace page (/workspace) on the React app and remain logged in.
    is-logged-in in localStorage should have the value true
    v2-admin-ui cookie should have the value react.

@wanlingt wanlingt requested review from timotheeg, karrui and justynoh and removed request for timotheeg, karrui and justynoh August 2, 2022 03:00
@wanlingt wanlingt marked this pull request as draft August 2, 2022 03:02
@wanlingt wanlingt changed the title fix(form-v2): remove additional log in step when switching from AngularJS to React (WIP) fix(form-v2): remove additional log in step when switching from AngularJS to React Aug 2, 2022
@wanlingt wanlingt temporarily deployed to staging-alt2 August 2, 2022 04:12 Inactive
@wanlingt wanlingt marked this pull request as ready for review August 2, 2022 05:08
@wanlingt wanlingt changed the title (WIP) fix(form-v2): remove additional log in step when switching from AngularJS to React fix(form-v2): remove additional log in step when switching from AngularJS to React Aug 2, 2022
Copy link
Contributor

@karrui karrui left a comment

Choose a reason for hiding this comment

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

Lgtm

@@ -35,7 +41,15 @@ export const useAuth = (): AuthContextProps => {

// Provider hook that creates auth object and handles state
const useProvideAuth = () => {
const [isAuthenticated] = useLocalStorage<boolean>(LOGGED_IN_KEY)
// TODO #4279: Remove after React rollout is complete
const { data: user } = useQuery<UserDto>(
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment: could we use the useUser hook directly? Or does the use user hook look at the localStorage value before enabling? Maybe we can add an override enabled prop if that is the case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or does the use user hook look at the localStorage value before enabling?

the useUser hook uses useAuth to get the localStorage value (so there's a circular reference). To get around this I replaced useAuth with useLocalStorage in useUser

Maybe we can add an override enabled prop if that is the case

Ok! added this too

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, now it looks messy. i reviewed it without completely looking at the hook. suggest reverting haha ><. The previous code was more readable, and considering this is "throwaway" code i think we can get away with some duplication.

or we can set the state of isAuthenticated to be contingent on return value of useUser instead of the other way round:

export const useUser = (): UseUserReturn => {
  const [isAuthenticated, setIsAuthenticated] = useLocalStorage<boolean>(LOGGED_IN_KEY)

  const { data: user, isLoading } = useQuery<UserDto>(
    userKeys.base,
    () => fetchUser(),
    // 10 minutes staletime, do not need to retrieve so often.
    { 
      staleTime: 600000,
      // Removed enabled state, just retrieve every 10 minutes
      // new functions, update auth states
      onSuccess: () => setIsAuthenticated(true),
      onError: () => setIsAuthenticated(undefined) // remove login on error, may not be needed since we already have ApiService
    },
  )

  return {
    user: isAuthenticated ? user : undefined,
    isLoading,
  }
}

Copy link
Contributor Author

@wanlingt wanlingt Aug 2, 2022

Choose a reason for hiding this comment

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

haha it's ok! I will revert :) I think that option's the least messy / easiest to remove after the rollout

@wanlingt wanlingt force-pushed the form-v2/angular-to-react-loggedin-switch branch from d017e2e to 7a36e5a Compare August 2, 2022 06:17
@wanlingt wanlingt temporarily deployed to staging-alt2 August 2, 2022 06:19 Inactive
@wanlingt wanlingt force-pushed the form-v2/angular-to-react-loggedin-switch branch from 7a36e5a to 564142c Compare August 2, 2022 08:02
userKeys.base,
() => fetchUser(),
// 10 minutes staletime, do not need to retrieve so often.
{ staleTime: 600000 },
Copy link
Contributor

Choose a reason for hiding this comment

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

should we still add an onSuccess to set localStorage to the authenticated state?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh yes good point! added

@wanlingt wanlingt temporarily deployed to staging-alt2 August 3, 2022 01:57 Inactive
@wanlingt wanlingt merged commit d98e574 into form-v2/develop Aug 3, 2022
@wanlingt wanlingt deleted the form-v2/angular-to-react-loggedin-switch branch August 3, 2022 02:34
@justynoh justynoh mentioned this pull request Oct 5, 2022
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