code cleanup before merging tests

src/features/login/EnforcePolicies.tsx

@@ -4,7 +4,7 @@ import { Alert, Button, Container, Paper } from '@mui/material';
 import { Stack } from '@mui/system';
 import { useState } from 'react';
 import classes from '../signup/SignUp.module.scss';
-import { kbasePolicies, PolicyViewer } from '../auth/Policies';
+import { kbasePolicies, PolicyViewer } from './Policies';
 
 export const EnforcePolicies = ({
   policyIds,
@@ -13,16 +13,16 @@ export const EnforcePolicies = ({
   policyIds: string[];
   onAccept: (versionedPolicyIds: string[]) => void;
 }) => {
+  // Get policy information
   const targetPolicies = policyIds.map((id) => kbasePolicies[id]);
-
   const [accepted, setAccepted] = useState<{
     [k in typeof targetPolicies[number]['id']]?: boolean;
   }>({});
-
   const allAccepted = targetPolicies.every(
     (policy) => accepted[policy.id] === true
   );
 
+  // Message to user, uses a special message when agreeing to kbase-user.2
   let message =
     'To continue to your account, you must agree to the following KBase use policies.'; // Default message
   if (

src/features/login/LogIn.tsx

@@ -75,7 +75,7 @@ export const LogIn: FC = () => {
   const nextRequest = useAppParam('nextRequest');
   useCheckLoggedIn(nextRequest);
   const { loginActionUrl, loginRedirectUrl, loginOrigin } =
-    makelLoginURLs(nextRequest);
+    makeLoginURLs(nextRequest);
 
   return (
     <Container maxWidth="sm">
@@ -146,7 +146,7 @@ export const LogIn: FC = () => {
   );
 };
 
-export const makelLoginURLs = (nextRequest?: string) => {
+export const makeLoginURLs = (nextRequest?: string) => {
   // OAuth Login wont work in dev mode, but send dev users to CI so they can grab their token
   const loginOrigin =
     process.env.NODE_ENV === 'development'

src/features/login/LogInContinue.test.tsx

@@ -8,7 +8,7 @@ import { LogInContinue } from './LogInContinue';
 import fetchMock from 'jest-fetch-mock';
 import { toast } from 'react-hot-toast';
 import { noOp } from '../common';
-import { kbasePolicies } from '../auth/Policies';
+import { kbasePolicies } from './Policies';
 
 jest.mock('react-hot-toast', () => ({
   toast: jest.fn(),

src/features/login/LogInContinue.tsx

@@ -11,7 +11,7 @@ import { useNavigate } from 'react-router-dom';
 import { LOGIN_ROUTE } from '../../app/Routes';
 import { useAppDispatch } from '../../common/hooks';
 import { setLoginData } from '../signup/SignupSlice';
-import { kbasePolicies } from '../auth/Policies';
+import { kbasePolicies } from './Policies';
 import { EnforcePolicies } from './EnforcePolicies';
 
 export const LogInContinue: FC = () => {

src/features/signup/AccountInformation.tsx

@@ -17,7 +17,7 @@ import {
   TextField,
   Typography,
 } from '@mui/material';
-import { FC, useState } from 'react';
+import { FC, useEffect, useState } from 'react';
 import { toast } from 'react-hot-toast';
 import { useNavigate } from 'react-router-dom';
 import { useAppDispatch, useAppSelector } from '../../common/hooks';
@@ -27,40 +27,32 @@ import { loginUsernameSuggest } from '../../common/api/authService';
 import { useForm } from 'react-hook-form';
 import { setAccount, setProfile } from './SignupSlice';
 
+export const useCheckLoginDataOk = () => {
+  const navigate = useNavigate();
+  const loginData = useAppSelector((state) => state.signup.loginData);
+  useEffect(() => {
+    if (!loginData) {
+      toast('You must login using a provider first to sign up!');
+      navigate('/signup/1');
+    }
+  }, [loginData, navigate]);
+};
+
 /**
  * Account information form for sign up flow
  */
-export const AccountInformation: FC<{
-  setActiveStep: (step: number) => void;
-}> = ({ setActiveStep }) => {
+export const AccountInformation: FC<{}> = () => {
   const navigate = useNavigate();
   const dispatch = useAppDispatch();
 
+  useCheckLoginDataOk();
+
   // Login Data
-  const loginData = useAppSelector(
-    (state) =>
-      state.signup.loginData || {
-        provider: 'ORCiD',
-        create: [
-          {
-            availablename: 'dlyon',
-            id: 'someuserid',
-            provemail: '[email protected]',
-            provfullname: 'David Lyon',
-            provusername: 'dlyon',
-          },
-        ],
-      }
-  );
+  const loginData = useAppSelector((state) => state.signup.loginData);
 
   // Account data
   const account = useAppSelector((state) => state.signup.account);
 
-  if (!loginData) {
-    toast('You must login using a provider first to sign up!');
-    navigate('/signup/1');
-  }
-
   //username availibility
   const [username, setUsername] = useState(account.username ?? '');
   const userAvail = loginUsernameSuggest.useQuery(username);
@@ -71,7 +63,7 @@ export const AccountInformation: FC<{
   const surveyQuestion = 'How did you hear about us? (select all that apply)';
   const [optionalText, setOptionalText] = useState<Record<string, string>>({});
 
-  // Form
+  // Form state
   const { register, handleSubmit } = useForm({
     defaultValues: {
       account: account,
@@ -90,9 +82,10 @@ export const AccountInformation: FC<{
     },
   });
 
+  // Form submission
   const onSubmit = handleSubmit(async (fieldValues, event) => {
     event?.preventDefault();
-    // Add in the optional survey text content
+    // Add in survey text content from form
     ReferalSources.forEach((src) => {
       if (
         src.customText &&
@@ -102,7 +95,7 @@ export const AccountInformation: FC<{
         fieldValues.profile.surveydata.referralSources.response[src.value] =
           optionalText[src.value];
     });
-    // dispatch form data to state
+    // dispatch form data to signup state
     dispatch(setAccount(fieldValues.account));
     dispatch(
       setProfile({
@@ -123,8 +116,8 @@ export const AccountInformation: FC<{
       <Alert>
         <Stack spacing={1}>
           <span>
-            You have signed in with your <strong>{loginData.provider}</strong>{' '}
-            account <strong>{loginData.create[0].provemail}</strong>. This will
+            You have signed in with your <strong>{loginData?.provider}</strong>{' '}
+            account <strong>{loginData?.create[0].provemail}</strong>. This will
             be the account linked to your KBase account.
           </span>
           <Accordion className={classes['collapsible-message']} disableGutters>
@@ -139,22 +132,22 @@ export const AccountInformation: FC<{
               <Stack spacing={1}>
                 <span>
                   If the account you see above is not the one you want, use the
-                  link below to log out of {loginData.provider}, and then try
+                  link below to log out of {loginData?.provider}, and then try
                   again.
                 </span>
                 <Box>
                   <Button variant="outlined">
-                    Log out from {loginData.provider}
+                    Log out from {loginData?.provider}
                   </Button>
                 </Box>
                 <span>
-                  If you are trying to sign up with a {loginData.provider}{' '}
+                  If you are trying to sign up with a {loginData?.provider}{' '}
                   account that is already linked to a KBase account, you will be
                   unable to create a new KBase account using that{' '}
-                  {loginData.provider} account.
+                  {loginData?.provider} account.
                 </span>
                 <span>
-                  After signing out from {loginData.provider} you will need to
+                  After signing out from {loginData?.provider} you will need to
                   restart the sign up process.
                 </span>
                 <Box>
@@ -171,7 +164,7 @@ export const AccountInformation: FC<{
             <Typography variant="h2">Create a new KBase Account</Typography>
             <Typography>
               Some field values have been pre-populated from your{' '}
-              <strong>{loginData.provider}</strong> account.
+              <strong>{loginData?.provider}</strong> account.
               <strong> All fields are required.</strong>
             </Typography>
             <FormControl>

src/features/signup/ProviderSelect.tsx

@@ -8,14 +8,14 @@ import {
   Typography,
 } from '@mui/material';
 import { FC } from 'react';
-import { LoginButtons, makelLoginURLs } from '../login/LogIn';
+import { LoginButtons, makeLoginURLs } from '../login/LogIn';
 import classes from './SignUp.module.scss';
 
 /**
  * Provider selection screen for sign up flow
  */
 export const ProviderSelect: FC = () => {
-  const { loginActionUrl, loginRedirectUrl, loginOrigin } = makelLoginURLs();
+  const { loginActionUrl, loginRedirectUrl, loginOrigin } = makeLoginURLs();
 
   return (
     <Stack justifyContent="center">
@@ -25,7 +25,7 @@ export const ProviderSelect: FC = () => {
             <Typography variant="h2">Choose a provider</Typography>
             {process.env.NODE_ENV === 'development' ? (
               <Alert severity="error">
-                DEV MODE: Login will occur on {loginOrigin}
+                DEV MODE: Signup will occur on {loginOrigin}
               </Alert>
             ) : (
               <></>

src/features/signup/SignUp.tsx

@@ -16,6 +16,7 @@ import { AccountInformation } from './AccountInformation';
 import { ProviderSelect } from './ProviderSelect';
 import { KBasePolicies } from './SignupPolicies';
 import { md5 } from 'js-md5';
+import { ROOT_REDIRECT_ROUTE } from '../../app/Routes';
 
 const signUpSteps = [
   'Sign up with a supported provider',
@@ -47,61 +48,54 @@ export const SignUp: FC = () => {
         <Typography variant="h1">Sign up for KBase</Typography>
         <Stepper activeStep={activeStep}>
           {signUpSteps.map((step, i) => (
-            <Step key={step} onClick={() => setActiveStep(i)}>
+            <Step
+              key={step}
+              onClick={() => {
+                if (i < activeStep) setActiveStep(i);
+              }}
+            >
               <StepLabel>{step}</StepLabel>
             </Step>
           ))}
         </Stepper>
         {activeStep === 0 && <ProviderSelect />}
-        {activeStep === 1 && (
-          <AccountInformation setActiveStep={setActiveStep} />
-        )}
-        {activeStep === 2 && <KBasePolicies setActiveStep={setActiveStep} />}
+        {activeStep === 1 && <AccountInformation />}
+        {activeStep === 2 && <KBasePolicies />}
       </Stack>
     </Container>
   );
 };
 
 export const useDoSignup = () => {
-  const data = useAppSelector((state) => state.signup);
-
-  const signupOk = !!data.loginData;
-  const [triggerAccount, accountResult] = loginCreate.useMutation();
-  const [triggerProfile, profileResult] = setUserProfile.useMutation();
-
-  const loading =
-    !accountResult.isUninitialized &&
-    (accountResult.isLoading || profileResult.isLoading);
-
-  const complete =
-    !loading &&
-    !accountResult.isUninitialized &&
-    !profileResult.isUninitialized &&
-    accountResult.isSuccess &&
-    profileResult.isSuccess;
+  const signupData = useAppSelector((state) => state.signup);
+  const navigate = useNavigate();
 
+  // Queries for creating an account and a profile for the user.
+  const [triggerCreateAccount, accountResult] = loginCreate.useMutation();
+  const [triggerCreateProfile, profileResult] = setUserProfile.useMutation();
   const error = accountResult.error || profileResult.error;
 
+  // Callback to trigger the first call. Consumer should check signup data is present before calling!
   const doSignup = (policyIds: string[]) => {
-    if (!signupOk) return;
-    triggerAccount({
-      id: String(data.loginData?.create[0].id),
-      user: String(data.account.username),
-      display: String(data.account.display),
-      email: String(data.account.email),
+    triggerCreateAccount({
+      id: String(signupData.loginData?.create[0].id),
+      user: String(signupData.account.username),
+      display: String(signupData.account.display),
+      email: String(signupData.account.email),
       policyids: policyIds,
       linkall: false,
     });
   };
 
+  // Once the account is created, use the account token to set the account profile.
   useEffect(() => {
     if (!accountResult.data?.token.token) return;
-    triggerProfile([
+    triggerCreateProfile([
       {
         profile: {
           user: {
-            realname: String(data.account.display),
-            username: String(data.account.username),
+            realname: String(signupData.account.display),
+            username: String(signupData.account.username),
           },
           profile: {
             metadata: {
@@ -111,28 +105,47 @@ export const useDoSignup = () => {
             // was globus info, no longer used
             preferences: {},
             synced: {
-              gravatarHash: gravatarHash(data.account.email || ''),
+              gravatarHash: gravatarHash(signupData.account.email || ''),
             },
-            ...data.profile,
+            ...signupData.profile,
           },
         },
       },
       accountResult.data?.token.token ?? '',
     ]);
   }, [
     accountResult,
-    data.account.display,
-    data.account.email,
-    data.account.username,
-    data.profile,
-    triggerProfile,
+    signupData.account.display,
+    signupData.account.email,
+    signupData.account.username,
+    signupData.profile,
+    triggerCreateProfile,
   ]);
 
-  // Once everything completes, try auth from token.
-  const tryToken = complete ? accountResult.data.token.token : undefined;
-  useTryAuthFromToken(tryToken);
+  const createLoading =
+    !accountResult.isUninitialized &&
+    (accountResult.isLoading || profileResult.isLoading);
+
+  const createComplete =
+    !createLoading &&
+    !accountResult.isUninitialized &&
+    !profileResult.isUninitialized &&
+    accountResult.isSuccess &&
+    profileResult.isSuccess;
+
+  // Once create completes, try auth from token.
+  const tryToken = createComplete ? accountResult.data.token.token : undefined;
+  const tokenQuery = useTryAuthFromToken(tryToken);
+
+  const complete = createComplete && tokenQuery.isSuccess;
+  const loading = createLoading || !complete;
+
+  // once everything completes and we're authed from the token, redirect to root.
+  useEffect(() => {
+    if (complete) navigate(ROOT_REDIRECT_ROUTE);
+  }, [complete, navigate]);
 
-  return [signupOk, doSignup, loading, complete, error] as const;
+  return [doSignup, loading, complete, error] as const;
 };
 
 const gravatarHash = (email: string) => {