From 196ac96a093a731ea5afdc7d3b3f41e1a8c94e0d Mon Sep 17 00:00:00 2001 From: Brett Kolodny Date: Thu, 20 Feb 2025 16:06:48 +0000 Subject: [PATCH 1/6] fix: include link and more useful message for 403 errors --- site/src/api/errors.ts | 8 +++++ site/src/components/Alert/ErrorAlert.tsx | 42 ++++++++++++++++++++---- 2 files changed, 43 insertions(+), 7 deletions(-) diff --git a/site/src/api/errors.ts b/site/src/api/errors.ts index f1e63d1e39caf..873163e11a68d 100644 --- a/site/src/api/errors.ts +++ b/site/src/api/errors.ts @@ -133,6 +133,14 @@ export const getErrorDetail = (error: unknown): string | undefined => { return undefined; }; +export const getErrorStatus = (error: unknown): number | undefined => { + if (isApiError(error)) { + return error.status; + } + + return undefined; +}; + export class DetailedError extends Error { constructor( message: string, diff --git a/site/src/components/Alert/ErrorAlert.tsx b/site/src/components/Alert/ErrorAlert.tsx index 73d9c62480ab8..b5611d060ba8f 100644 --- a/site/src/components/Alert/ErrorAlert.tsx +++ b/site/src/components/Alert/ErrorAlert.tsx @@ -1,5 +1,6 @@ import AlertTitle from "@mui/material/AlertTitle"; -import { getErrorDetail, getErrorMessage } from "api/errors"; +import { getErrorDetail, getErrorMessage, getErrorStatus } from "api/errors"; +import { Link } from "components/Link/Link"; import type { FC } from "react"; import { Alert, AlertDetail, type AlertProps } from "./Alert"; @@ -8,21 +9,48 @@ export const ErrorAlert: FC< > = ({ error, ...alertProps }) => { const message = getErrorMessage(error, "Something went wrong."); const detail = getErrorDetail(error); + const status = getErrorStatus(error); // For some reason, the message and detail can be the same on the BE, but does // not make sense in the FE to showing them duplicated const shouldDisplayDetail = message !== detail; - return ( - - {detail ? ( + const body = () => { + // When the error is a Forbidden response we include a link for the user to + // go back to a known viewable page. + // Additionally since the error messages and details from the server can be + // missing or confusing for an end user we render a friendlier message + // regardless of the response from the server. + if (status === 403) { + return ( + <> + You don't have permission to view this page + + If you believe this is a mistake, please contact your administrator + or try signing in with different credentials.{" "} + + Go to workspaces + + + + ); + } + + if (detail) { + return ( <> {message} {shouldDisplayDetail && {detail}} - ) : ( - message - )} + ); + } + + return message; + }; + + return ( + + {body()} ); }; From b6f2e5a17df5e77279ca80f2fd2e7c6116ea0b2b Mon Sep 17 00:00:00 2001 From: Brett Kolodny Date: Thu, 20 Feb 2025 16:16:58 +0000 Subject: [PATCH 2/6] fix: fmt --- site/src/components/Alert/ErrorAlert.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/site/src/components/Alert/ErrorAlert.tsx b/site/src/components/Alert/ErrorAlert.tsx index b5611d060ba8f..a581b243d7cee 100644 --- a/site/src/components/Alert/ErrorAlert.tsx +++ b/site/src/components/Alert/ErrorAlert.tsx @@ -16,7 +16,7 @@ export const ErrorAlert: FC< const shouldDisplayDetail = message !== detail; const body = () => { - // When the error is a Forbidden response we include a link for the user to + // When the error is a Forbidden response we include a link for the user to // go back to a known viewable page. // Additionally since the error messages and details from the server can be // missing or confusing for an end user we render a friendlier message From 50404eef5f6bba936bbde9a9b831976d3a8a8887 Mon Sep 17 00:00:00 2001 From: Brett Kolodny Date: Fri, 21 Feb 2025 19:46:13 +0000 Subject: [PATCH 3/6] fix: move Language object to it's own file so playwright doesn't cry --- site/e2e/setup/addUsersAndLicense.spec.ts | 2 +- site/src/components/Alert/ErrorAlert.tsx | 2 +- site/src/pages/CreateUserPage/CreateUserForm.tsx | 13 +------------ .../pages/CreateUserPage/CreateUserPage.test.tsx | 2 +- site/src/pages/CreateUserPage/Language.ts | 11 +++++++++++ 5 files changed, 15 insertions(+), 15 deletions(-) create mode 100644 site/src/pages/CreateUserPage/Language.ts diff --git a/site/e2e/setup/addUsersAndLicense.spec.ts b/site/e2e/setup/addUsersAndLicense.spec.ts index f6817e0fd423d..bcaa8c9281cf8 100644 --- a/site/e2e/setup/addUsersAndLicense.spec.ts +++ b/site/e2e/setup/addUsersAndLicense.spec.ts @@ -1,6 +1,6 @@ import { expect, test } from "@playwright/test"; import { API } from "api/api"; -import { Language } from "pages/CreateUserPage/CreateUserForm"; +import { Language } from "pages/CreateUserPage/Language"; import { coderPort, license, premiumTestsRequired, users } from "../constants"; import { expectUrl } from "../expectUrl"; import { createUser } from "../helpers"; diff --git a/site/src/components/Alert/ErrorAlert.tsx b/site/src/components/Alert/ErrorAlert.tsx index a581b243d7cee..79f110c1eca0c 100644 --- a/site/src/components/Alert/ErrorAlert.tsx +++ b/site/src/components/Alert/ErrorAlert.tsx @@ -1,7 +1,7 @@ import AlertTitle from "@mui/material/AlertTitle"; import { getErrorDetail, getErrorMessage, getErrorStatus } from "api/errors"; -import { Link } from "components/Link/Link"; import type { FC } from "react"; +import { Link } from "../Link/Link"; import { Alert, AlertDetail, type AlertProps } from "./Alert"; export const ErrorAlert: FC< diff --git a/site/src/pages/CreateUserPage/CreateUserForm.tsx b/site/src/pages/CreateUserPage/CreateUserForm.tsx index aebdd36e45adc..be8b4a15797b5 100644 --- a/site/src/pages/CreateUserPage/CreateUserForm.tsx +++ b/site/src/pages/CreateUserPage/CreateUserForm.tsx @@ -19,18 +19,7 @@ import { onChangeTrimmed, } from "utils/formUtils"; import * as Yup from "yup"; - -export const Language = { - emailLabel: "Email", - passwordLabel: "Password", - usernameLabel: "Username", - nameLabel: "Full name", - emailInvalid: "Please enter a valid email address.", - emailRequired: "Please enter an email address.", - passwordRequired: "Please enter a password.", - createUser: "Create", - cancel: "Cancel", -}; +import { Language } from "./Language"; export const authMethodLanguage = { password: { diff --git a/site/src/pages/CreateUserPage/CreateUserPage.test.tsx b/site/src/pages/CreateUserPage/CreateUserPage.test.tsx index f8b256e2d0cbb..7c26930efe254 100644 --- a/site/src/pages/CreateUserPage/CreateUserPage.test.tsx +++ b/site/src/pages/CreateUserPage/CreateUserPage.test.tsx @@ -4,7 +4,7 @@ import { renderWithAuth, waitForLoaderToBeRemoved, } from "testHelpers/renderHelpers"; -import { Language as FormLanguage } from "./CreateUserForm"; +import { Language as FormLanguage } from "./Language"; import { CreateUserPage } from "./CreateUserPage"; const renderCreateUserPage = async () => { diff --git a/site/src/pages/CreateUserPage/Language.ts b/site/src/pages/CreateUserPage/Language.ts new file mode 100644 index 0000000000000..d449829aea89d --- /dev/null +++ b/site/src/pages/CreateUserPage/Language.ts @@ -0,0 +1,11 @@ +export const Language = { + emailLabel: "Email", + passwordLabel: "Password", + usernameLabel: "Username", + nameLabel: "Full name", + emailInvalid: "Please enter a valid email address.", + emailRequired: "Please enter an email address.", + passwordRequired: "Please enter a password.", + createUser: "Create", + cancel: "Cancel", +}; From 1b8cf2a85989d0a8be5c93d8893f028eae9ab66e Mon Sep 17 00:00:00 2001 From: Brett Kolodny Date: Fri, 21 Feb 2025 19:57:52 +0000 Subject: [PATCH 4/6] fix: add details to API response on backend --- coderd/httpapi/httpapi.go | 1 + site/src/components/Alert/ErrorAlert.tsx | 5 ++--- site/src/pages/CreateUserPage/CreateUserPage.test.tsx | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/coderd/httpapi/httpapi.go b/coderd/httpapi/httpapi.go index cd55a09d51525..1afce55d1efad 100644 --- a/coderd/httpapi/httpapi.go +++ b/coderd/httpapi/httpapi.go @@ -154,6 +154,7 @@ func ResourceNotFound(rw http.ResponseWriter) { func Forbidden(rw http.ResponseWriter) { Write(context.Background(), rw, http.StatusForbidden, codersdk.Response{ Message: "Forbidden.", + Detail: "You don't have permission to view this page. If you believe this is a mistake, please contact your administrator or try signing in with different credentials.", }) } diff --git a/site/src/components/Alert/ErrorAlert.tsx b/site/src/components/Alert/ErrorAlert.tsx index 79f110c1eca0c..74cfa30e8b77b 100644 --- a/site/src/components/Alert/ErrorAlert.tsx +++ b/site/src/components/Alert/ErrorAlert.tsx @@ -24,10 +24,9 @@ export const ErrorAlert: FC< if (status === 403) { return ( <> - You don't have permission to view this page + {message} - If you believe this is a mistake, please contact your administrator - or try signing in with different credentials.{" "} + {detail}{" "} Go to workspaces diff --git a/site/src/pages/CreateUserPage/CreateUserPage.test.tsx b/site/src/pages/CreateUserPage/CreateUserPage.test.tsx index 7c26930efe254..ec75fc9a8e244 100644 --- a/site/src/pages/CreateUserPage/CreateUserPage.test.tsx +++ b/site/src/pages/CreateUserPage/CreateUserPage.test.tsx @@ -4,8 +4,8 @@ import { renderWithAuth, waitForLoaderToBeRemoved, } from "testHelpers/renderHelpers"; -import { Language as FormLanguage } from "./Language"; import { CreateUserPage } from "./CreateUserPage"; +import { Language as FormLanguage } from "./Language"; const renderCreateUserPage = async () => { renderWithAuth(, { From 94907f28b5294523dbde7c2ffcd5347348f4ec99 Mon Sep 17 00:00:00 2001 From: Brett Kolodny Date: Fri, 21 Feb 2025 20:59:42 +0000 Subject: [PATCH 5/6] fix: error detail copy --- coderd/httpapi/httpapi.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/coderd/httpapi/httpapi.go b/coderd/httpapi/httpapi.go index 1afce55d1efad..a9687d58a0604 100644 --- a/coderd/httpapi/httpapi.go +++ b/coderd/httpapi/httpapi.go @@ -154,7 +154,7 @@ func ResourceNotFound(rw http.ResponseWriter) { func Forbidden(rw http.ResponseWriter) { Write(context.Background(), rw, http.StatusForbidden, codersdk.Response{ Message: "Forbidden.", - Detail: "You don't have permission to view this page. If you believe this is a mistake, please contact your administrator or try signing in with different credentials.", + Detail: "You don't have permission to view this content. If you believe this is a mistake, please contact your administrator or try signing in with different credentials.", }) } From 169d41c7cc1403f0353072a7b589071e5759a498 Mon Sep 17 00:00:00 2001 From: Brett Kolodny Date: Fri, 21 Feb 2025 21:04:24 +0000 Subject: [PATCH 6/6] fix: move body to ternary --- site/src/components/Alert/ErrorAlert.tsx | 55 ++++++++++-------------- 1 file changed, 22 insertions(+), 33 deletions(-) diff --git a/site/src/components/Alert/ErrorAlert.tsx b/site/src/components/Alert/ErrorAlert.tsx index 74cfa30e8b77b..0198ea4e99540 100644 --- a/site/src/components/Alert/ErrorAlert.tsx +++ b/site/src/components/Alert/ErrorAlert.tsx @@ -15,41 +15,30 @@ export const ErrorAlert: FC< // not make sense in the FE to showing them duplicated const shouldDisplayDetail = message !== detail; - const body = () => { - // When the error is a Forbidden response we include a link for the user to - // go back to a known viewable page. - // Additionally since the error messages and details from the server can be - // missing or confusing for an end user we render a friendlier message - // regardless of the response from the server. - if (status === 403) { - return ( - <> - {message} - - {detail}{" "} - - Go to workspaces - - - - ); - } - - if (detail) { - return ( - <> - {message} - {shouldDisplayDetail && {detail}} - - ); - } - - return message; - }; - return ( - {body()} + { + // When the error is a Forbidden response we include a link for the user to + // go back to a known viewable page. + status === 403 ? ( + <> + {message} + + {detail}{" "} + + Go to workspaces + + + + ) : detail ? ( + <> + {message} + {shouldDisplayDetail && {detail}} + + ) : ( + message + ) + } ); };