From 6a2bcfec85a964ea3a03aed71f3b8a09727e946c Mon Sep 17 00:00:00 2001 From: David Lyon Date: Thu, 5 Dec 2024 19:18:28 -0800 Subject: [PATCH] code cleanup before merging tests --- src/features/login/EnforcePolicies.tsx | 6 +- src/features/login/LogIn.tsx | 4 +- src/features/login/LogInContinue.test.tsx | 2 +- src/features/login/LogInContinue.tsx | 2 +- src/features/{auth => login}/Policies.tsx | 0 .../{auth => login}/PolicyViewer.module.scss | 0 src/features/signup/AccountInformation.tsx | 63 ++++++------ src/features/signup/ProviderSelect.tsx | 6 +- src/features/signup/SignUp.tsx | 95 +++++++++++-------- src/features/signup/SignupPolicies.tsx | 54 +++++------ src/features/signup/SignupSlice.tsx | 1 - 11 files changed, 117 insertions(+), 116 deletions(-) rename src/features/{auth => login}/Policies.tsx (100%) rename src/features/{auth => login}/PolicyViewer.module.scss (100%) diff --git a/src/features/login/EnforcePolicies.tsx b/src/features/login/EnforcePolicies.tsx index caa35720..de196255 100644 --- a/src/features/login/EnforcePolicies.tsx +++ b/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 ( diff --git a/src/features/login/LogIn.tsx b/src/features/login/LogIn.tsx index c551850c..a01eba07 100644 --- a/src/features/login/LogIn.tsx +++ b/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 ( @@ -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' diff --git a/src/features/login/LogInContinue.test.tsx b/src/features/login/LogInContinue.test.tsx index 2b5279fe..e0a8b2a6 100644 --- a/src/features/login/LogInContinue.test.tsx +++ b/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(), diff --git a/src/features/login/LogInContinue.tsx b/src/features/login/LogInContinue.tsx index a69e0219..2b58d3e6 100644 --- a/src/features/login/LogInContinue.tsx +++ b/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 = () => { diff --git a/src/features/auth/Policies.tsx b/src/features/login/Policies.tsx similarity index 100% rename from src/features/auth/Policies.tsx rename to src/features/login/Policies.tsx diff --git a/src/features/auth/PolicyViewer.module.scss b/src/features/login/PolicyViewer.module.scss similarity index 100% rename from src/features/auth/PolicyViewer.module.scss rename to src/features/login/PolicyViewer.module.scss diff --git a/src/features/signup/AccountInformation.tsx b/src/features/signup/AccountInformation.tsx index 50b766a3..28895be7 100644 --- a/src/features/signup/AccountInformation.tsx +++ b/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: 'dlyon@lbl.gov', - 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>({}); - // 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<{ - You have signed in with your {loginData.provider}{' '} - account {loginData.create[0].provemail}. This will + You have signed in with your {loginData?.provider}{' '} + account {loginData?.create[0].provemail}. This will be the account linked to your KBase account. @@ -139,22 +132,22 @@ export const AccountInformation: FC<{ 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. - 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. - 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. @@ -171,7 +164,7 @@ export const AccountInformation: FC<{ Create a new KBase Account Some field values have been pre-populated from your{' '} - {loginData.provider} account. + {loginData?.provider} account. All fields are required. diff --git a/src/features/signup/ProviderSelect.tsx b/src/features/signup/ProviderSelect.tsx index f6a02b09..f6c30c8e 100644 --- a/src/features/signup/ProviderSelect.tsx +++ b/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 ( @@ -25,7 +25,7 @@ export const ProviderSelect: FC = () => { Choose a provider {process.env.NODE_ENV === 'development' ? ( - DEV MODE: Login will occur on {loginOrigin} + DEV MODE: Signup will occur on {loginOrigin} ) : ( <> diff --git a/src/features/signup/SignUp.tsx b/src/features/signup/SignUp.tsx index c48dff2a..fe62cf90 100644 --- a/src/features/signup/SignUp.tsx +++ b/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 = () => { Sign up for KBase {signUpSteps.map((step, i) => ( - setActiveStep(i)}> + { + if (i < activeStep) setActiveStep(i); + }} + > {step} ))} {activeStep === 0 && } - {activeStep === 1 && ( - - )} - {activeStep === 2 && } + {activeStep === 1 && } + {activeStep === 2 && } ); }; 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,9 +105,9 @@ 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, }, }, }, @@ -121,18 +115,37 @@ export const useDoSignup = () => { ]); }, [ 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) => { diff --git a/src/features/signup/SignupPolicies.tsx b/src/features/signup/SignupPolicies.tsx index 19ef8740..0666bd0a 100644 --- a/src/features/signup/SignupPolicies.tsx +++ b/src/features/signup/SignupPolicies.tsx @@ -1,43 +1,50 @@ import { faArrowLeft } from '@fortawesome/free-solid-svg-icons'; import { FontAwesomeIcon } from '@fortawesome/react-fontawesome'; import { Button, Paper, Stack, Typography } from '@mui/material'; -import { FC, useState } from 'react'; -import { Navigate } from 'react-router-dom'; -import { ROOT_REDIRECT_ROUTE } from '../../app/Routes'; +import { FC, useEffect, useState } from 'react'; +import { toast } from 'react-hot-toast'; +import { useNavigate } from 'react-router-dom'; import { Loader } from '../../common/components'; -import { useAppDispatch } from '../../common/hooks'; -import { kbasePolicies, PolicyViewer } from '../auth/Policies'; +import { useAppDispatch, useAppSelector } from '../../common/hooks'; +import { kbasePolicies, PolicyViewer } from '../login/Policies'; +import { useCheckLoginDataOk } from './AccountInformation'; import { useDoSignup } from './SignUp'; import classes from './SignUp.module.scss'; import { setAccount } from './SignupSlice'; /** - * Use policy agreements for sign up flow. + * KBase policy agreements step for sign up flow. */ -export const KBasePolicies: FC<{ - setActiveStep: (step: number) => void; -}> = ({ setActiveStep }) => { +export const KBasePolicies: FC<{}> = () => { const dispatch = useAppDispatch(); + const navigate = useNavigate(); + // Check prev steps data is filled out. + useCheckLoginDataOk(); + const account = useAppSelector((state) => state.signup.account); + useEffect(() => { + if (Object.values(account).some((v) => v === undefined)) { + toast('You must fill out your account information to sign up!'); + navigate('/signup/2'); + } + }, [account, navigate]); + + // The policies the user needs to accept. const signupPolicies = Object.values(kbasePolicies).map((p) => p.id); const versionedPolicyIds = signupPolicies.map((policyId) => { return [kbasePolicies[policyId].id, kbasePolicies[policyId].version].join( '.' ); }); - const [accepted, setAccepted] = useState<{ [k in typeof signupPolicies[number]]?: boolean; }>({}); - const allAccepted = signupPolicies.every( (policyId) => accepted[policyId] === true ); - const [signupOk, doSignup, loading, complete, errors] = useDoSignup(); - // eslint-disable-next-line no-console - console.error(errors); - + // Performs signup (if all policies have been accepted) + const [doSignup, loading] = useDoSignup(); const onSubmit = () => { if (!allAccepted) return; dispatch( @@ -48,17 +55,6 @@ export const KBasePolicies: FC<{ doSignup(versionedPolicyIds); }; - if (complete) { - return ( - - ); - } - return ( @@ -88,7 +84,7 @@ export const KBasePolicies: FC<{ variant="contained" endIcon={} size="large" - disabled={!(allAccepted && signupOk) || loading} + disabled={!allAccepted || loading} onClick={onSubmit} > Create KBase account @@ -99,7 +95,7 @@ export const KBasePolicies: FC<{ color="warning" size="large" onClick={() => { - setActiveStep(0); + navigate('/signup/1'); }} > Cancel sign up @@ -108,7 +104,7 @@ export const KBasePolicies: FC<{ variant="outlined" size="large" startIcon={} - onClick={() => setActiveStep(1)} + onClick={() => navigate('/signup/2')} > Back to account information diff --git a/src/features/signup/SignupSlice.tsx b/src/features/signup/SignupSlice.tsx index b6c61738..242a4fba 100644 --- a/src/features/signup/SignupSlice.tsx +++ b/src/features/signup/SignupSlice.tsx @@ -1,7 +1,6 @@ import { createSlice, PayloadAction } from '@reduxjs/toolkit'; import { GetLoginChoiceResult } from '../../common/api/authService'; -// Define a type for the slice state export interface SignupState { loginData?: GetLoginChoiceResult; account: {