Skip to content

fix: manage backend authXService errors #3190

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Jul 26, 2022
Merged

fix: manage backend authXService errors #3190

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5899c44
fix: manage backend authXService errors
AbhineetJain Jul 25, 2022
f7c1560
fix stories
AbhineetJain Jul 25, 2022
047b6c7
add new stories
AbhineetJain Jul 25, 2022
6a71be5
fix lint
AbhineetJain Jul 25, 2022
9faca3c
fix error state stories
AbhineetJain Jul 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions site/src/components/ErrorSummary/ErrorSummary.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { ApiError, getErrorDetail, getErrorMessage } from "api/errors"
import { Stack } from "components/Stack/Stack"
import { FC, useState } from "react"

const Language = {
export const Language = {
retryMessage: "Retry",
unknownErrorMessage: "An unknown error has occurred",
moreDetails: "More",
Expand Down Expand Up @@ -91,7 +91,6 @@ interface StyleProps {
const useStyles = makeStyles<Theme, StyleProps>((theme) => ({
root: {
background: darken(theme.palette.error.main, 0.6),
margin: `${theme.spacing(2)}px`,
padding: `${theme.spacing(2)}px`,
borderRadius: theme.shape.borderRadius,
gap: 0,
Expand Down
15 changes: 6 additions & 9 deletions site/src/components/SettingsAccountForm/SettingsAccountForm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import FormHelperText from "@material-ui/core/FormHelperText"
import TextField from "@material-ui/core/TextField"
import { ErrorSummary } from "components/ErrorSummary/ErrorSummary"
import { FormikContextType, FormikErrors, useFormik } from "formik"
import { FC } from "react"
import * as Yup from "yup"
import { getFormHelpers, nameValidator, onChangeTrimmed } from "../../util/formUtils"
import { getFormHelpersWithError, nameValidator, onChangeTrimmed } from "../../util/formUtils"
import { LoadingButton } from "../LoadingButton/LoadingButton"
import { Stack } from "../Stack/Stack"

Expand All @@ -28,29 +28,28 @@ export interface AccountFormProps {
isLoading: boolean
initialValues: AccountFormValues
onSubmit: (values: AccountFormValues) => void
formErrors?: AccountFormErrors
error?: string
updateProfileError?: Error | unknown
}

export const AccountForm: FC<AccountFormProps> = ({
email,
isLoading,
onSubmit,
initialValues,
formErrors = {},
error,
updateProfileError,
}) => {
const form: FormikContextType<AccountFormValues> = useFormik<AccountFormValues>({
initialValues,
validationSchema,
onSubmit,
})
const getFieldHelpers = getFormHelpers<AccountFormValues>(form, formErrors)
const getFieldHelpers = getFormHelpersWithError<AccountFormValues>(form, updateProfileError)

return (
<>
<form onSubmit={form.handleSubmit}>
<Stack>
{updateProfileError && <ErrorSummary error={updateProfileError} />}
<TextField
disabled
fullWidth
Expand All @@ -67,8 +66,6 @@ export const AccountForm: FC<AccountFormProps> = ({
variant="outlined"
/>

{error && <FormHelperText error>{error}</FormHelperText>}

<div>
<LoadingButton loading={isLoading} type="submit" variant="contained">
{isLoading ? "" : Language.updateSettings}
Expand Down
15 changes: 6 additions & 9 deletions site/src/components/SettingsSecurityForm/SettingsSecurityForm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import FormHelperText from "@material-ui/core/FormHelperText"
import TextField from "@material-ui/core/TextField"
import { ErrorSummary } from "components/ErrorSummary/ErrorSummary"
import { FormikContextType, FormikErrors, useFormik } from "formik"
import React from "react"
import * as Yup from "yup"
import { getFormHelpers, onChangeTrimmed } from "../../util/formUtils"
import { getFormHelpersWithError, onChangeTrimmed } from "../../util/formUtils"
import { LoadingButton } from "../LoadingButton/LoadingButton"
import { Stack } from "../Stack/Stack"

Expand Down Expand Up @@ -45,28 +45,27 @@ export interface SecurityFormProps {
isLoading: boolean
initialValues: SecurityFormValues
onSubmit: (values: SecurityFormValues) => void
formErrors?: SecurityFormErrors
error?: string
updateSecurityError?: Error | unknown
}

export const SecurityForm: React.FC<SecurityFormProps> = ({
isLoading,
onSubmit,
initialValues,
formErrors = {},
error,
updateSecurityError,
}) => {
const form: FormikContextType<SecurityFormValues> = useFormik<SecurityFormValues>({
initialValues,
validationSchema,
onSubmit,
})
const getFieldHelpers = getFormHelpers<SecurityFormValues>(form, formErrors)
const getFieldHelpers = getFormHelpersWithError<SecurityFormValues>(form, updateSecurityError)

return (
<>
<form onSubmit={form.handleSubmit}>
<Stack>
{updateSecurityError && <ErrorSummary error={updateSecurityError} />}
<TextField
{...getFieldHelpers("old_password")}
onChange={onChangeTrimmed(form)}
Expand Down Expand Up @@ -95,8 +94,6 @@ export const SecurityForm: React.FC<SecurityFormProps> = ({
type="password"
/>

{error && <FormHelperText error>{error}</FormHelperText>}

<div>
<LoadingButton loading={isLoading} type="submit" variant="contained">
{isLoading ? "" : Language.updatePassword}
Expand Down
23 changes: 19 additions & 4 deletions site/src/components/SignInForm/SignInForm.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ export default {
component: SignInForm,
argTypes: {
isLoading: "boolean",
authErrorMessage: "string",
onSubmit: { action: "Submit" },
},
}
Expand All @@ -16,7 +15,7 @@ const Template: Story<SignInFormProps> = (args: SignInFormProps) => <SignInForm
export const SignedOut = Template.bind({})
SignedOut.args = {
isLoading: false,
authErrorMessage: undefined,
authError: undefined,
onSubmit: () => {
return Promise.resolve()
},
Expand All @@ -33,12 +32,28 @@ Loading.args = {
}

export const WithLoginError = Template.bind({})
WithLoginError.args = { ...SignedOut.args, authErrorMessage: "Email or password was invalid" }
WithLoginError.args = {
...SignedOut.args,
authError: {
response: {
data: {
message: "Email or password was invalid",
validations: [
{
field: "password",
detail: "Password is invalid.",
},
],
},
},
isAxiosError: true,
},
}

export const WithAuthMethodsError = Template.bind({})
WithAuthMethodsError.args = {
...SignedOut.args,
methodsErrorMessage: "Failed to fetch auth methods",
methodsError: new Error("Failed to fetch auth methods"),
}

export const WithGithub = Template.bind({})
Expand Down
88 changes: 40 additions & 48 deletions site/src/components/SignInForm/SignInForm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,15 @@
import Button from "@material-ui/core/Button"
import FormHelperText from "@material-ui/core/FormHelperText"
import Link from "@material-ui/core/Link"
import { makeStyles } from "@material-ui/core/styles"
import TextField from "@material-ui/core/TextField"
import GitHubIcon from "@material-ui/icons/GitHub"
import { ErrorSummary } from "components/ErrorSummary/ErrorSummary"
import { Stack } from "components/Stack/Stack"
import { FormikContextType, useFormik } from "formik"
import { FC } from "react"
import * as Yup from "yup"
import { AuthMethods } from "../../api/typesGenerated"
import { getFormHelpers, onChangeTrimmed } from "../../util/formUtils"
import { getFormHelpersWithError, onChangeTrimmed } from "../../util/formUtils"
import { Welcome } from "../Welcome/Welcome"
import { LoadingButton } from "./../LoadingButton/LoadingButton"

Expand Down Expand Up @@ -39,17 +40,6 @@ const validationSchema = Yup.object({
})

const useStyles = makeStyles((theme) => ({
loginBtnWrapper: {
marginTop: theme.spacing(6),
borderTop: `1px solid ${theme.palette.action.disabled}`,
paddingTop: theme.spacing(3),
},
loginTextField: {
marginTop: theme.spacing(2),
},
submitBtn: {
marginTop: theme.spacing(2),
},
buttonIcon: {
width: 14,
height: 14,
Expand Down Expand Up @@ -78,8 +68,8 @@ const useStyles = makeStyles((theme) => ({
export interface SignInFormProps {
isLoading: boolean
redirectTo: string
authErrorMessage?: string
methodsErrorMessage?: string
authError?: Error | unknown
methodsError?: Error | unknown
authMethods?: AuthMethods
onSubmit: ({ email, password }: { email: string; password: string }) => Promise<void>
}
Expand All @@ -88,8 +78,8 @@ export const SignInForm: FC<SignInFormProps> = ({
authMethods,
redirectTo,
isLoading,
authErrorMessage,
methodsErrorMessage,
authError,
methodsError,
onSubmit,
}) => {
const styles = useStyles()
Expand All @@ -107,42 +97,44 @@ export const SignInForm: FC<SignInFormProps> = ({
validateOnBlur: false,
onSubmit,
})
const getFieldHelpers = getFormHelpers<BuiltInAuthFormValues>(form)
const getFieldHelpers = getFormHelpersWithError<BuiltInAuthFormValues>(form, authError)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe I'm being dense :) but why do we call this method with authError and not methodsError?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

methodsError is not linked to the sign-in form. This method attributes form errors to specific fields in the form.


return (
<>
<Welcome />
<form onSubmit={form.handleSubmit}>
<TextField
{...getFieldHelpers("email")}
onChange={onChangeTrimmed(form)}
autoFocus
autoComplete="email"
className={styles.loginTextField}
fullWidth
label={Language.emailLabel}
type="email"
variant="outlined"
/>
<TextField
{...getFieldHelpers("password")}
autoComplete="current-password"
className={styles.loginTextField}
fullWidth
id="password"
label={Language.passwordLabel}
type="password"
variant="outlined"
/>
{authErrorMessage && <FormHelperText error>{authErrorMessage}</FormHelperText>}
{methodsErrorMessage && (
<FormHelperText error>{Language.methodsErrorMessage}</FormHelperText>
)}
<div className={styles.submitBtn}>
<LoadingButton loading={isLoading} fullWidth type="submit" variant="contained">
{isLoading ? "" : Language.passwordSignIn}
</LoadingButton>
</div>
<Stack>
{authError && (
<ErrorSummary error={authError} defaultMessage={Language.authErrorMessage} />
)}
{methodsError && (
<ErrorSummary error={methodsError} defaultMessage={Language.methodsErrorMessage} />
)}
Copy link
Member

@Kira-Pilot Kira-Pilot Jul 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

A small concern I have with this pattern is that JSX complexity will grow 1:1 with API/Error complexity (if I am understanding this right). Let's say we had a more complex form that made a couple of different API calls (like 4). Would we be passing 4 additional props and rendering 4 of the above fragments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, we were debating whether handling multiple errors in the same component made sense, or if we could just render a separate ErrorSummary component for each error. Since we were not sure how common it was to have many parallel API calls with a single form, we thought this would be fine for the time being. The use case might get clearer as we explore more of the code base.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that's right @Kira-Pilot. My reason for not wanting to coalesce the errors into one ErrorSummary was aesthetic (I didn't think two error summaries stuck together would look good given that sometimes they have collapsible parts). But you raise a good point. It might be a good idea to make our next ticket one that can render a ton of errors on one page in order to find out up front if it's a problem and what the solution would be.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the explanation. Leaving for now seems reasonable to me! We can cross the complexity bridge when we get there.

<TextField
{...getFieldHelpers("email")}
onChange={onChangeTrimmed(form)}
autoFocus
autoComplete="email"
fullWidth
label={Language.emailLabel}
type="email"
variant="outlined"
/>
<TextField
{...getFieldHelpers("password")}
autoComplete="current-password"
fullWidth
id="password"
label={Language.passwordLabel}
type="password"
variant="outlined"
/>
<div>
<LoadingButton loading={isLoading} fullWidth type="submit" variant="contained">
{isLoading ? "" : Language.passwordSignIn}
</LoadingButton>
</div>
</Stack>
</form>
{authMethods?.github && (
<>
Expand Down
5 changes: 3 additions & 2 deletions site/src/pages/LoginPage/LoginPage.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,18 +52,19 @@ describe("LoginPage", () => {

it("shows an error if fetching auth methods fails", async () => {
// Given
const apiErrorMessage = "Unable to fetch methods"
server.use(
// Make login fail
rest.get("/api/v2/users/authmethods", async (req, res, ctx) => {
return res(ctx.status(500), ctx.json({ message: "nope" }))
return res(ctx.status(500), ctx.json({ message: apiErrorMessage }))
}),
)

// When
render(<LoginPage />)

// Then
const errorMessage = await screen.findByText(Language.methodsErrorMessage)
const errorMessage = await screen.findByText(apiErrorMessage)
expect(errorMessage).toBeDefined()
})

Expand Down
11 changes: 2 additions & 9 deletions site/src/pages/LoginPage/LoginPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ import { useActor } from "@xstate/react"
import React, { useContext } from "react"
import { Helmet } from "react-helmet"
import { Navigate, useLocation } from "react-router-dom"
import { isApiError } from "../../api/errors"
import { Footer } from "../../components/Footer/Footer"
import { SignInForm } from "../../components/SignInForm/SignInForm"
import { pageTitle } from "../../util/page"
Expand Down Expand Up @@ -36,12 +35,6 @@ export const LoginPage: React.FC = () => {
const [authState, authSend] = useActor(xServices.authXService)
const isLoading = authState.hasTag("loading")
const redirectTo = retrieveRedirect(location.search)
const authErrorMessage = isApiError(authState.context.authError)
? authState.context.authError.response.data.message
: undefined
const getMethodsError = authState.context.getMethodsError
? (authState.context.getMethodsError as Error).message
: undefined

const onSubmit = async ({ email, password }: { email: string; password: string }) => {
authSend({ type: "SIGN_IN", email, password })
Expand All @@ -61,8 +54,8 @@ export const LoginPage: React.FC = () => {
authMethods={authState.context.methods}
redirectTo={redirectTo}
isLoading={isLoading}
authErrorMessage={authErrorMessage}
methodsErrorMessage={getMethodsError}
authError={authState.context.authError}
methodsError={authState.context.getMethodsError as Error}
Copy link
Member

Choose a reason for hiding this comment

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

Why do we have to override the compiler here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

getMethodsError does not really throw an error from the backend, but only a generic one. This was being done earlier (in one of the lines we deleted), so just wanted to keep it the same for sanity (like referencing the message string from the Error object).

Additionally, I don't know a good way to produce a getMethodsError, so couldn't reeally play around with it. @kylecarbs do you have any suggestions about how the error could be reproduced?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really think we do have to override it. I'm guessing that ended up in there originally to make it so we didn't have to add | unknown in a component prop type. I had hoped we could narrow all the error types in the XServices but it turns out we want to leave them alone until they reach ErrorSummary and getFormHelpers, so I think we're just stuck with Error | unknown and we can delete this. I like the comment explaining that this call never returns an ApiError though.

onSubmit={onSubmit}
/>
</div>
Expand Down
5 changes: 3 additions & 2 deletions site/src/pages/UserSettingsPage/AccountPage/AccountPage.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
import { fireEvent, screen, waitFor } from "@testing-library/react"
import { Language as ErrorSummaryLanguage } from "components/ErrorSummary/ErrorSummary"
import * as API from "../../../api/api"
import { GlobalSnackbar } from "../../../components/GlobalSnackbar/GlobalSnackbar"
import * as AccountForm from "../../../components/SettingsAccountForm/SettingsAccountForm"
import { renderWithAuth } from "../../../testHelpers/renderHelpers"
import * as AuthXService from "../../../xServices/auth/authXService"
import { AccountPage, Language } from "./AccountPage"
import { AccountPage } from "./AccountPage"

const renderPage = () => {
return renderWithAuth(
Expand Down Expand Up @@ -80,7 +81,7 @@ describe("AccountPage", () => {
const { user } = renderPage()
await fillAndSubmitForm()

const errorMessage = await screen.findByText(Language.unknownError)
const errorMessage = await screen.findByText(ErrorSummaryLanguage.unknownErrorMessage)
expect(errorMessage).toBeDefined()
expect(API.updateProfile).toBeCalledTimes(1)
expect(API.updateProfile).toBeCalledWith(user.id, newData)
Expand Down
Loading