From 15f7d35d9e51aa3d414f7f11816ca1e80e3b2b37 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Tue, 23 Aug 2022 15:06:56 +0000 Subject: [PATCH 1/7] RED: add unit tests for AccountForm username field --- .../SettingsAccountForm.test.tsx | 87 +++++++++++++++++++ .../SettingsAccountForm.tsx | 2 +- site/src/testHelpers/entities.ts | 10 +++ 3 files changed, 98 insertions(+), 1 deletion(-) create mode 100644 site/src/components/SettingsAccountForm/SettingsAccountForm.test.tsx diff --git a/site/src/components/SettingsAccountForm/SettingsAccountForm.test.tsx b/site/src/components/SettingsAccountForm/SettingsAccountForm.test.tsx new file mode 100644 index 0000000000000..728115f2a98f3 --- /dev/null +++ b/site/src/components/SettingsAccountForm/SettingsAccountForm.test.tsx @@ -0,0 +1,87 @@ +import { screen } from "@testing-library/react" +import { MockUser2 } from "../../testHelpers/entities" +import { render } from "../../testHelpers/renderHelpers" +import { AccountForm, AccountFormValues } from "./SettingsAccountForm" + +// NOTE: it does not matter what the role props of MockUser are set to, +// only that editable is set to true or false. This is passed from +// the call to /authorization done by authXService +describe("AccountForm", () => { + describe("for owners", () => { + it("allows updating username", async () => { + // Given + const mockInitialValues: AccountFormValues = { + username: MockUser2.username, + editable: true, + } + + // When + render( + { + return + }} + />, + ) + + // Then + const el = await screen.findByLabelText("Username") + expect(el).toBeEnabled() + }) + }) + + describe("for user admins", () => { + it("allows updating username", async () => { + // Given + const mockInitialValues: AccountFormValues = { + username: MockUser2.username, + editable: true, + } + + // When + render( + { + return + }} + />, + ) + + // Then + const el = await screen.findByLabelText("Username") + expect(el).toBeEnabled() + }) + }) + + describe("for members", () => { + it("allows updating username", async () => { + // Given + const mockInitialValues: AccountFormValues = { + username: MockUser2.username, + editable: false, + } + + // When + render( + { + return + }} + />, + ) + + // Then + const el = await screen.findByLabelText("Username") + expect(el).toBeDisabled() + }) + }) +}) diff --git a/site/src/components/SettingsAccountForm/SettingsAccountForm.tsx b/site/src/components/SettingsAccountForm/SettingsAccountForm.tsx index 040949d6e4b67..0e732801e0b43 100644 --- a/site/src/components/SettingsAccountForm/SettingsAccountForm.tsx +++ b/site/src/components/SettingsAccountForm/SettingsAccountForm.tsx @@ -7,7 +7,7 @@ import { getFormHelpersWithError, nameValidator, onChangeTrimmed } from "../../u import { LoadingButton } from "../LoadingButton/LoadingButton" import { Stack } from "../Stack/Stack" -interface AccountFormValues { +export interface AccountFormValues { username: string } diff --git a/site/src/testHelpers/entities.ts b/site/src/testHelpers/entities.ts index 5c28f574704f5..34ef3998f4aac 100644 --- a/site/src/testHelpers/entities.ts +++ b/site/src/testHelpers/entities.ts @@ -61,6 +61,16 @@ export const MockUser: TypesGen.User = { roles: [MockOwnerRole], } +export const MockUserAdmin: TypesGen.User = { + id: "test-user", + username: "TestUser", + email: "test@coder.com", + created_at: "", + status: "active", + organization_ids: ["fc0774ce-cc9e-48d4-80ae-88f7a4d4a8b0"], + roles: [MockUserAdminRole], +} + export const MockUser2: TypesGen.User = { id: "test-user-2", username: "TestUser2", From 8772cf0eff983954ca78e8c7e0de226b458c0e40 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Tue, 23 Aug 2022 19:13:40 +0000 Subject: [PATCH 2/7] hopefully make the test green --- .../SettingsAccountForm/SettingsAccountForm.tsx | 4 ++++ .../pages/UserSettingsPage/AccountPage/AccountPage.tsx | 9 +++++++-- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/site/src/components/SettingsAccountForm/SettingsAccountForm.tsx b/site/src/components/SettingsAccountForm/SettingsAccountForm.tsx index 0e732801e0b43..3755e72421596 100644 --- a/site/src/components/SettingsAccountForm/SettingsAccountForm.tsx +++ b/site/src/components/SettingsAccountForm/SettingsAccountForm.tsx @@ -8,6 +8,7 @@ import { LoadingButton } from "../LoadingButton/LoadingButton" import { Stack } from "../Stack/Stack" export interface AccountFormValues { + editable: boolean username: string } @@ -22,6 +23,7 @@ const validationSchema = Yup.object({ }) export interface AccountFormProps { + editable: boolean email: string isLoading: boolean initialValues: AccountFormValues @@ -32,6 +34,7 @@ export interface AccountFormProps { } export const AccountForm: FC> = ({ + editable, email, isLoading, onSubmit, @@ -63,6 +66,7 @@ export const AccountForm: FC> = ({ {...getFieldHelpers("username")} onChange={onChangeTrimmed(form)} autoComplete="username" + disabled={!editable} fullWidth label={Language.usernameLabel} variant="outlined" diff --git a/site/src/pages/UserSettingsPage/AccountPage/AccountPage.tsx b/site/src/pages/UserSettingsPage/AccountPage/AccountPage.tsx index acc5ef3ddd549..26f9b69578d20 100644 --- a/site/src/pages/UserSettingsPage/AccountPage/AccountPage.tsx +++ b/site/src/pages/UserSettingsPage/AccountPage/AccountPage.tsx @@ -11,7 +11,8 @@ export const Language = { export const AccountPage: React.FC = () => { const xServices = useContext(XServiceContext) const [authState, authSend] = useActor(xServices.authXService) - const { me, updateProfileError } = authState.context + const { me, permissions, updateProfileError } = authState.context + const canEditUsers = permissions && permissions.updateUsers if (!me) { throw new Error("No current user found") @@ -23,7 +24,11 @@ export const AccountPage: React.FC = () => { email={me.email} updateProfileError={updateProfileError} isLoading={authState.matches("signedIn.profile.updatingProfile")} - initialValues={{ username: me.username }} + initialValues={{ + username: me.username, + // Fail-open, as the API endpoint will check again on submit + editable: canEditUsers || false, + }} onSubmit={(data) => { authSend({ type: "UPDATE_PROFILE", From bbce1af52886c6460ab29691ea966b9a160b5634 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Tue, 23 Aug 2022 19:24:15 +0000 Subject: [PATCH 3/7] remove unnecesary tests --- .../SettingsAccountForm.test.tsx | 32 ++----------------- 1 file changed, 3 insertions(+), 29 deletions(-) diff --git a/site/src/components/SettingsAccountForm/SettingsAccountForm.test.tsx b/site/src/components/SettingsAccountForm/SettingsAccountForm.test.tsx index 728115f2a98f3..d6946249b84ef 100644 --- a/site/src/components/SettingsAccountForm/SettingsAccountForm.test.tsx +++ b/site/src/components/SettingsAccountForm/SettingsAccountForm.test.tsx @@ -7,7 +7,7 @@ import { AccountForm, AccountFormValues } from "./SettingsAccountForm" // only that editable is set to true or false. This is passed from // the call to /authorization done by authXService describe("AccountForm", () => { - describe("for owners", () => { + describe("when editable is set to true", () => { it("allows updating username", async () => { // Given const mockInitialValues: AccountFormValues = { @@ -33,34 +33,8 @@ describe("AccountForm", () => { }) }) - describe("for user admins", () => { - it("allows updating username", async () => { - // Given - const mockInitialValues: AccountFormValues = { - username: MockUser2.username, - editable: true, - } - - // When - render( - { - return - }} - />, - ) - - // Then - const el = await screen.findByLabelText("Username") - expect(el).toBeEnabled() - }) - }) - - describe("for members", () => { - it("allows updating username", async () => { + describe("when editable is set to false", () => { + it("does not allow updating username", async () => { // Given const mockInitialValues: AccountFormValues = { username: MockUser2.username, From 1800982558bf3c397fd37aefe3478102b74cce8c Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Tue, 23 Aug 2022 19:33:56 +0000 Subject: [PATCH 4/7] move things to the right place --- .../SettingsAccountForm/SettingsAccountForm.test.tsx | 4 ++-- .../components/SettingsAccountForm/SettingsAccountForm.tsx | 1 - site/src/pages/UserSettingsPage/AccountPage/AccountPage.tsx | 3 +-- 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/site/src/components/SettingsAccountForm/SettingsAccountForm.test.tsx b/site/src/components/SettingsAccountForm/SettingsAccountForm.test.tsx index d6946249b84ef..164b90d166248 100644 --- a/site/src/components/SettingsAccountForm/SettingsAccountForm.test.tsx +++ b/site/src/components/SettingsAccountForm/SettingsAccountForm.test.tsx @@ -12,12 +12,12 @@ describe("AccountForm", () => { // Given const mockInitialValues: AccountFormValues = { username: MockUser2.username, - editable: true, } // When render( { // Given const mockInitialValues: AccountFormValues = { username: MockUser2.username, - editable: false, } // When render( { return (
{ authSend({ From 8e8b4ec8038a9808a7c34c687a81a916491bf376 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Tue, 23 Aug 2022 20:43:27 +0100 Subject: [PATCH 5/7] PR comment Co-authored-by: Joe Previte --- site/src/pages/UserSettingsPage/AccountPage/AccountPage.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/site/src/pages/UserSettingsPage/AccountPage/AccountPage.tsx b/site/src/pages/UserSettingsPage/AccountPage/AccountPage.tsx index 107bd33944801..80e0645e03eaa 100644 --- a/site/src/pages/UserSettingsPage/AccountPage/AccountPage.tsx +++ b/site/src/pages/UserSettingsPage/AccountPage/AccountPage.tsx @@ -21,7 +21,7 @@ export const AccountPage: React.FC = () => { return (
Date: Tue, 23 Aug 2022 19:56:40 +0000 Subject: [PATCH 6/7] ensure button is also not enabled --- .../SettingsAccountForm/SettingsAccountForm.test.tsx | 4 ++++ .../SettingsAccountForm/SettingsAccountForm.tsx | 9 ++++++++- 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/site/src/components/SettingsAccountForm/SettingsAccountForm.test.tsx b/site/src/components/SettingsAccountForm/SettingsAccountForm.test.tsx index 164b90d166248..adbf906e8808c 100644 --- a/site/src/components/SettingsAccountForm/SettingsAccountForm.test.tsx +++ b/site/src/components/SettingsAccountForm/SettingsAccountForm.test.tsx @@ -30,6 +30,8 @@ describe("AccountForm", () => { // Then const el = await screen.findByLabelText("Username") expect(el).toBeEnabled() + const btn = await screen.findByRole("button") + expect(btn).toBeEnabled() }) }) @@ -56,6 +58,8 @@ describe("AccountForm", () => { // Then const el = await screen.findByLabelText("Username") expect(el).toBeDisabled() + const btn = await screen.findByRole("button") + expect(btn).toBeDisabled() }) }) }) diff --git a/site/src/components/SettingsAccountForm/SettingsAccountForm.tsx b/site/src/components/SettingsAccountForm/SettingsAccountForm.tsx index db8f7dcfaf0d8..9842fcc258fb1 100644 --- a/site/src/components/SettingsAccountForm/SettingsAccountForm.tsx +++ b/site/src/components/SettingsAccountForm/SettingsAccountForm.tsx @@ -64,6 +64,7 @@ export const AccountForm: FC> = ({ > = ({ />
- + {isLoading ? "" : Language.updateSettings}
From 73fc7909cc9600153f8cafcb0ba25e0b5a9ea209 Mon Sep 17 00:00:00 2001 From: Cian Johnston Date: Tue, 23 Aug 2022 20:08:43 +0000 Subject: [PATCH 7/7] =?UTF-8?q?=F0=9F=A4=AF?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../SettingsAccountForm/SettingsAccountForm.test.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/site/src/components/SettingsAccountForm/SettingsAccountForm.test.tsx b/site/src/components/SettingsAccountForm/SettingsAccountForm.test.tsx index adbf906e8808c..687c5efeddefb 100644 --- a/site/src/components/SettingsAccountForm/SettingsAccountForm.test.tsx +++ b/site/src/components/SettingsAccountForm/SettingsAccountForm.test.tsx @@ -30,7 +30,7 @@ describe("AccountForm", () => { // Then const el = await screen.findByLabelText("Username") expect(el).toBeEnabled() - const btn = await screen.findByRole("button") + const btn = await screen.findByRole("button", { name: /Update settings/i }) expect(btn).toBeEnabled() }) }) @@ -58,7 +58,7 @@ describe("AccountForm", () => { // Then const el = await screen.findByLabelText("Username") expect(el).toBeDisabled() - const btn = await screen.findByRole("button") + const btn = await screen.findByRole("button", { name: /Update settings/i }) expect(btn).toBeDisabled() }) })