Skip to content

Commit

Permalink
Auth form validation bug (#425)
Browse files Browse the repository at this point in the history 
* pass down optional onClick handler for trigger func in auth form

* use default rules prop from UseController props.

* leverage form to trigger required git token field if not filled in, as well as error states from calls to api

* clear errors when fetching git user and clear any other relevant fields that should not be there. use meta args on successful calls to get git user and orgs/projects to set token instead of dispatching seperate actions setting a token which could possibly be invalid.
  • Loading branch information
@D-B-Hawk
D-B-Hawk authored Dec 19, 2023
1 parent 77b37c9 commit 24bb3b2
Show file tree
Hide file tree
Showing 6 changed files with 100 additions and 79 deletions.
3 changes: 3 additions & 0 deletions components/autocomplete/index.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ export interface IAutocompleteProps extends ControllerRenderProps<FieldValues> {
disabled?: boolean;
// eslint-disable-next-line @typescript-eslint/no-explicit-any
filterOptions?: (options: any[]) => any[];
onClick?: () => void;
}

const AutocompleteComponent: FunctionComponent<IAutocompleteProps> = ({
Expand All @@ -35,6 +36,7 @@ const AutocompleteComponent: FunctionComponent<IAutocompleteProps> = ({
sx,
filterOptions,
disabled,
onClick,
...props
}) => {
// Unfortunately MaterialUI doesn't not update the dom state when no options are available
Expand Down Expand Up @@ -75,6 +77,7 @@ const AutocompleteComponent: FunctionComponent<IAutocompleteProps> = ({
value={value}
{...params}
label={label}
onClick={onClick}
/>
)}
/>
Expand Down
3 changes: 3 additions & 0 deletions components/controlledFields/AutoComplete.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ export interface ControlledTextFieldProps<T extends FieldValues> extends UseCont
filterOptions?: (options: any[]) => any[];
onChange?: (value: string) => void;
options: Array<{ value: string; label: string }>;
onClick?: () => void;
}

function ControlledAutocomplete<T extends FieldValues>({
Expand All @@ -32,6 +33,7 @@ function ControlledAutocomplete<T extends FieldValues>({
disabled,
placeholder,
loading,
onClick,
...props
}: ControlledTextFieldProps<T>) {
const [isBlur, setIsBlur] = useState(false);
Expand Down Expand Up @@ -62,6 +64,7 @@ function ControlledAutocomplete<T extends FieldValues>({
options={options}
label={label}
error={isBlur && error !== undefined}
onClick={onClick}
/>
)}
/>
Expand Down
4 changes: 0 additions & 4 deletions components/controlledFields/Password.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,6 @@ export interface ControlledTextFieldProps<T extends FieldValues> extends UseCont
label: string;
required?: boolean;
control: Control<T>;
rules: {
required: boolean;
pattern?: RegExp;
};
helperText?: string;
error?: boolean;
onErrorText?: string;
Expand Down
100 changes: 55 additions & 45 deletions containers/clusterForms/shared/authForm/index.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,7 @@ import {
getGitlabUser,
} from '../../../../redux/thunks/git.thunk';
import {
setToken,
clearUserError,
setGitOwner,
clearGitState,
setIsGitSelected,
} from '../../../../redux/slices/git.slice';
Expand All @@ -42,10 +40,9 @@ import CheckBoxWithRef from '@/components/checkbox';
import Column from '@/components/column';
import { hasProjectId } from '@/utils/hasProjectId';
import { getDigitalOceanUser } from '@/redux/thunks/digitalOcean.thunk';
import { GIT_PROVIDER_DISPLAY_NAME } from '@/constants';

const AuthForm: FunctionComponent = () => {
const [isGitRequested, setIsGitRequested] = useState<boolean>();
const [gitUserName, setGitUserName] = useState<string>();
const [showGoogleKeyFile, setShowGoogleKeyFile] = useState(false);

const dispatch = useAppDispatch();
Expand All @@ -62,7 +59,6 @@ const AuthForm: FunctionComponent = () => {
gitStateLoading,
installationType,
isGitSelected,
isTokenValid,
installMethod,
token = '',
} = useAppSelector(({ config, installation, git, digitalOcean }) => ({
Expand All @@ -85,47 +81,58 @@ const AuthForm: FunctionComponent = () => {
const {
control,
reset,
resetField,
watch,
setValue,
setError,
clearErrors,
trigger,
formState: { errors },
} = useFormContext<InstallValues>();

const [googleKeyFile, doToken] = watch(['google_auth.key_file', 'do_auth.token']);

const isGitHub = useMemo(() => gitProvider === GitProvider.GITHUB, [gitProvider]);

const validateGitOwner = async (gitOwner: string) => {
dispatch(setGitOwner(gitOwner));
dispatch(clearUserError());

if (gitOwner) {
if (isGitHub) {
await dispatch(getGitHubOrgRepositories({ token, organization: gitOwner })).unwrap();
await dispatch(getGitHubOrgTeams({ token, organization: gitOwner }));
} else {
await dispatch(getGitLabProjects({ token, group: gitOwner })).unwrap();
await dispatch(getGitLabSubgroups({ token, group: gitOwner }));
const validateGitOwner = useCallback(
async (gitOwner: string) => {
if (gitOwner) {
if (isGitHub) {
await dispatch(getGitHubOrgRepositories({ token, organization: gitOwner })).unwrap();
await dispatch(getGitHubOrgTeams({ token, organization: gitOwner }));
} else {
await dispatch(getGitLabProjects({ token, group: gitOwner })).unwrap();
await dispatch(getGitLabSubgroups({ token, group: gitOwner }));
}
}
}
};

const handleGitTokenBlur = async (token: string) => {
dispatch(setToken(token));
setIsGitRequested(true);
},
[isGitHub, dispatch, token],
);

if (isGitHub) {
// only care about return of getGithubUser as if it fails will exit and not run getGithubUserOrganizations
await dispatch(getGithubUser(token)).unwrap();
await dispatch(getGithubUserOrganizations(token));
} else {
// only care about return of getGitlabUser as if it fails will exit and not run getGitlabGroups
await dispatch(getGitlabUser(token)).unwrap();
await dispatch(getGitlabGroups(token));
}
};
const handleGitToken = useCallback(
async (token: string) => {
if (token) {
try {
if (isGitHub) {
// only care about return of getGithubUser as if it fails will exit and not run getGithubUserOrganizations
await dispatch(getGithubUser(token)).unwrap();
await dispatch(getGithubUserOrganizations(token));
} else {
// only care about return of getGitlabUser as if it fails will exit and not run getGitlabGroups
await dispatch(getGitlabUser(token)).unwrap();
await dispatch(getGitlabGroups(token));
}
clearErrors('gitToken');
resetField('gitOwner');
} catch (error) {
setError('gitToken', { type: 'validate', message: 'Please enter a valid token.' });
}
}
},
[isGitHub, dispatch, setError, clearErrors, resetField],
);

const handleGitProviderChange = (provider: GitProvider) => {
setGitUserName('');
reset({ gitToken: '', gitOwner: '' });
dispatch(clearGitState());
dispatch(setIsGitSelected(true));
Expand All @@ -143,7 +150,10 @@ const AuthForm: FunctionComponent = () => {
[dispatch],
);

const gitLabel = useMemo(() => (isGitHub ? 'GitHub' : 'GitLab'), [isGitHub]);
const gitLabel = useMemo(
() => GIT_PROVIDER_DISPLAY_NAME[gitProvider as GitProvider],
[gitProvider],
);

const isMarketplace = useMemo(() => installMethod?.includes('marketplace'), [installMethod]);

Expand All @@ -152,11 +162,10 @@ const AuthForm: FunctionComponent = () => {
[installationType],
);

useEffect(() => {
if (githubUser?.login || gitlabUser?.name) {
setGitUserName(githubUser?.login || gitlabUser?.name);
}
}, [dispatch, githubUser, gitlabUser, setValue]);
const gitUserName = useMemo(
() => githubUser?.login || gitlabUser?.name,
[githubUser, gitlabUser],
);

useEffect(() => {
return () => {
Expand Down Expand Up @@ -213,11 +222,10 @@ const AuthForm: FunctionComponent = () => {
label={`${gitLabel} personal access token`}
required
rules={{
required: true,
required: 'Required.',
}}
error={isGitRequested && !gitStateLoading && !isTokenValid}
onBlur={handleGitTokenBlur}
onErrorText="Invalid token."
onBlur={handleGitToken}
onErrorText={errors.gitToken?.message}
/>
</Row>
<GitUserField data-test-id="gitUser" style={{ width: '216px' }}>
Expand Down Expand Up @@ -248,7 +256,8 @@ const AuthForm: FunctionComponent = () => {
githubUserOrganizations.map(({ login }) => ({ label: login, value: login }))
}
loading={gitStateLoading}
label="GitHub organization name"
label={`${gitLabel} organization name`}
onClick={() => trigger('gitToken', { shouldFocus: true })}
/>
) : (
<ControlledAutocomplete
Expand All @@ -261,7 +270,8 @@ const AuthForm: FunctionComponent = () => {
gitlabGroups && gitlabGroups.map(({ name, path }) => ({ label: name, value: path }))
}
loading={gitStateLoading}
label="GitLab group name"
label={`${gitLabel} group name`}
onClick={() => trigger('gitToken', { shouldFocus: true })}
/>
)}
{installationType === InstallationType.GOOGLE && (
Expand Down
57 changes: 31 additions & 26 deletions redux/slices/git.slice.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,6 @@ const gitSlice = createSlice({
name: 'git',
initialState,
reducers: {
setToken: (state, action) => {
state.token = action.payload;
},
setGitOwner: (state, action) => {
state.gitOwner = action.payload;
},
Expand Down Expand Up @@ -77,13 +74,19 @@ const gitSlice = createSlice({
/* GitHub */
.addCase(getGithubUser.pending, (state) => {
state.isLoading = true;
state.errors = [];
})
.addCase(getGithubUser.fulfilled, (state, action) => {
state.token = action.meta.arg;
state.githubUser = action.payload;
state.isLoading = false;
state.isTokenValid = true;
})
.addCase(getGithubUser.rejected, (state, action) => {
state.isTokenValid = false;
state.isLoading = false;
state.githubUser = null;
state.githubUserOrganizations = [];
state.responseError = action.error.message;
})
.addCase(getGithubUserOrganizations.pending, (state) => {
Expand All @@ -104,20 +107,24 @@ const gitSlice = createSlice({
.addCase(getGitHubOrgRepositories.pending, (state) => {
state.isLoading = true;
})
.addCase(getGitHubOrgRepositories.fulfilled, (state, { payload: organizationRepos }) => {
const kubefirstRepos = organizationRepos.filter(({ name }) =>
KUBEFIRST_REPOSITORIES.includes(name),
);
if (kubefirstRepos.length) {
state.errors.push(
createGitOrgErrorMessage({
git: GitProvider.GITHUB,
type: 'repo',
gitOwner: state.gitOwner,
}),
.addCase(
getGitHubOrgRepositories.fulfilled,
(state, { meta, payload: organizationRepos }) => {
state.gitOwner = meta.arg.organization;
const kubefirstRepos = organizationRepos.filter(({ name }) =>
KUBEFIRST_REPOSITORIES.includes(name),
);
}
})
if (kubefirstRepos.length) {
state.errors.push(
createGitOrgErrorMessage({
git: GitProvider.GITHUB,
type: 'repo',
gitOwner: meta.arg.organization,
}),
);
}
},
)
.addCase(getGitHubOrgRepositories.rejected, (state, action) => {
state.isLoading = false;
state.isTokenValid = false;
Expand Down Expand Up @@ -152,13 +159,16 @@ const gitSlice = createSlice({
state.isLoading = true;
})
.addCase(getGitlabUser.fulfilled, (state, action) => {
state.token = action.meta.arg;
state.isLoading = false;
state.gitlabUser = action.payload;
state.isTokenValid = true;
})
.addCase(getGitlabUser.rejected, (state, action) => {
state.isLoading = false;
state.isTokenValid = false;
state.gitlabUser = null;
state.gitlabGroups = [];
state.responseError = action.error.message;
})
.addCase(getGitlabGroups.pending, (state) => {
Expand All @@ -177,7 +187,8 @@ const gitSlice = createSlice({
.addCase(getGitLabProjects.pending, (state) => {
state.isLoading = true;
})
.addCase(getGitLabProjects.fulfilled, (state, { payload: groupProjects }) => {
.addCase(getGitLabProjects.fulfilled, (state, { meta, payload: groupProjects }) => {
state.gitOwner = meta.arg.group;
state.isLoading = false;
const kubefirstRepos = groupProjects.filter(({ name }) =>
KUBEFIRST_REPOSITORIES.includes(name),
Expand All @@ -188,7 +199,7 @@ const gitSlice = createSlice({
createGitOrgErrorMessage({
git: GitProvider.GITLAB,
type: 'repo',
gitOwner: state.gitOwner,
gitOwner: meta.arg.group,
}),
);
}
Expand Down Expand Up @@ -223,13 +234,7 @@ const gitSlice = createSlice({
},
});

export const {
clearGitState,
clearUserError,
setIsGitSelected,
setGitOwner,
setToken,
clearResponseError,
} = gitSlice.actions;
export const { clearGitState, clearUserError, setIsGitSelected, clearResponseError, setGitOwner } =
gitSlice.actions;

export const gitReducer = gitSlice.reducer;
12 changes: 8 additions & 4 deletions redux/thunks/__tests__/git.thunk.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,11 +48,13 @@ describe('redux/thunks/git', () => {

test('getGithubUser - successful responses', async () => {
githubMock.onGet().reply(200, mockGithubUser);
await reduxStore.dispatch(getGithubUser('token'));
const githubToken = 'token';
await reduxStore.dispatch(getGithubUser(githubToken));

const { githubUser, isTokenValid } = reduxStore.getState().git;
const { githubUser, token, isTokenValid } = reduxStore.getState().git;

expect(githubUser).toStrictEqual(mockGithubUser);
expect(token).toStrictEqual(githubToken);
expect(isTokenValid).toStrictEqual(true);
});

Expand Down Expand Up @@ -179,11 +181,13 @@ describe('redux/thunks/git', () => {

test('getGitlabUser - successful responses', async () => {
gitlabMock.onGet().reply(200, mockGitlabUser);
await reduxStore.dispatch(getGitlabUser('token'));
const gitlabToken = 'token';
await reduxStore.dispatch(getGitlabUser(gitlabToken));

const { gitlabUser, isTokenValid } = reduxStore.getState().git;
const { gitlabUser, token, isTokenValid } = reduxStore.getState().git;

expect(gitlabUser).toStrictEqual(mockGitlabUser);
expect(token).toStrictEqual(gitlabToken);
expect(isTokenValid).toStrictEqual(true);
});

Expand Down

0 comments on commit 24bb3b2

Please sign in to comment.