From 195e89f8a6cb7f1a67381311f9a50ca0c6316c59 Mon Sep 17 00:00:00 2001
From: Ryan Laurie <30528226+iethree@users.noreply.github.com>
Date: Tue, 6 Aug 2024 17:01:19 -0600
Subject: [PATCH] Allow edits to users with empty name fields (#46511)
* allow edits to uers with empty name fields
* add repro for 40750
* whoopsie
---
.../{ => EditUserModal}/EditUserModal.tsx | 26 ++--
.../people/containers/EditUserModal/index.ts | 1 +
.../EditUserModal/test/common.unit.spec.tsx | 138 ++++++++++++++++++
.../test/enterprise.unit.spec.tsx | 80 ++++++++++
.../containers/EditUserModal/test/setup.tsx | 66 +++++++++
.../metabase/admin/people/forms/UserForm.tsx | 8 +-
.../test/__support__/server-mocks/user.ts | 1 +
7 files changed, 306 insertions(+), 14 deletions(-)
rename frontend/src/metabase/admin/people/containers/{ => EditUserModal}/EditUserModal.tsx (63%)
create mode 100644 frontend/src/metabase/admin/people/containers/EditUserModal/index.ts
create mode 100644 frontend/src/metabase/admin/people/containers/EditUserModal/test/common.unit.spec.tsx
create mode 100644 frontend/src/metabase/admin/people/containers/EditUserModal/test/enterprise.unit.spec.tsx
create mode 100644 frontend/src/metabase/admin/people/containers/EditUserModal/test/setup.tsx
diff --git a/frontend/src/metabase/admin/people/containers/EditUserModal.tsx b/frontend/src/metabase/admin/people/containers/EditUserModal/EditUserModal.tsx
similarity index 63%
rename from frontend/src/metabase/admin/people/containers/EditUserModal.tsx
rename to frontend/src/metabase/admin/people/containers/EditUserModal/EditUserModal.tsx
index ee913ed2b93..2b176bf48e0 100644
--- a/frontend/src/metabase/admin/people/containers/EditUserModal.tsx
+++ b/frontend/src/metabase/admin/people/containers/EditUserModal/EditUserModal.tsx
@@ -1,14 +1,14 @@
import { useMemo } from "react";
import type { Params } from "react-router/lib/Router";
-import { useUserQuery } from "metabase/common/hooks";
+import { skipToken, useGetUserQuery } from "metabase/api";
import LoadingAndErrorWrapper from "metabase/components/LoadingAndErrorWrapper";
import ModalContent from "metabase/components/ModalContent";
import Users from "metabase/entities/users";
import { useDispatch } from "metabase/lib/redux";
import type { User as UserType } from "metabase-types/api";
-import { UserForm } from "../forms/UserForm";
+import { UserForm } from "../../forms/UserForm";
interface EditUserModalProps {
onClose: () => void;
@@ -17,15 +17,21 @@ interface EditUserModalProps {
export const EditUserModal = ({ onClose, params }: EditUserModalProps) => {
const dispatch = useDispatch();
-
- const { data: user, isLoading } = useUserQuery({
- id: parseInt(params.userId),
- });
+ const userId = params.userId ? parseInt(params.userId) : null;
+ const { data: user, isLoading } = useGetUserQuery(userId ?? skipToken);
const initialValues = useMemo(() => getInitialValues(user), [user]);
- const handleSubmit = async (val: Partial<UserType>) => {
- await dispatch(Users.actions.update({ id: user?.id, ...val }));
+ const handleSubmit = async (newValues: Partial<UserType>) => {
+ // first name and last name keys need to be present so they can
+ // potentially be removed
+ const submitValues = {
+ first_name: null,
+ last_name: null,
+ ...newValues,
+ };
+ // can't use metabase/api hook here until the people list uses it too
+ await dispatch(Users.actions.update({ id: user?.id, ...submitValues }));
onClose();
};
@@ -48,8 +54,8 @@ export const EditUserModal = ({ onClose, params }: EditUserModalProps) => {
const getInitialValues = (user?: UserType) => {
return {
- first_name: user?.first_name ?? "",
- last_name: user?.last_name ?? "",
+ first_name: user?.first_name,
+ last_name: user?.last_name,
email: user?.email,
user_group_memberships: user?.user_group_memberships || [],
login_attributes: user?.login_attributes || {},
diff --git a/frontend/src/metabase/admin/people/containers/EditUserModal/index.ts b/frontend/src/metabase/admin/people/containers/EditUserModal/index.ts
new file mode 100644
index 00000000000..2b17c3c98c7
--- /dev/null
+++ b/frontend/src/metabase/admin/people/containers/EditUserModal/index.ts
@@ -0,0 +1 @@
+export * from "./EditUserModal";
diff --git a/frontend/src/metabase/admin/people/containers/EditUserModal/test/common.unit.spec.tsx b/frontend/src/metabase/admin/people/containers/EditUserModal/test/common.unit.spec.tsx
new file mode 100644
index 00000000000..6d018f90c95
--- /dev/null
+++ b/frontend/src/metabase/admin/people/containers/EditUserModal/test/common.unit.spec.tsx
@@ -0,0 +1,138 @@
+import { screen } from "@testing-library/react";
+import userEvent from "@testing-library/user-event";
+import fetchMock from "fetch-mock";
+
+import { setup, defaultUser } from "./setup";
+
+describe("Edit user modal", () => {
+ it("should populate existing data", async () => {
+ setup({ userData: defaultUser });
+ expect(await screen.findByText("Edit user")).toBeInTheDocument();
+ expect(await screen.findByDisplayValue("Ash")).toBeInTheDocument();
+ expect(await screen.findByDisplayValue("Ketchum")).toBeInTheDocument();
+ expect(
+ await screen.findByDisplayValue("pikachuboy97@example.com"),
+ ).toBeInTheDocument();
+ });
+
+ it("should close the modal when the close button is clicked", async () => {
+ const { onCloseSpy } = setup({ userData: defaultUser });
+ await userEvent.click(await screen.findByText("Cancel"));
+ expect(onCloseSpy).toHaveBeenCalled();
+ });
+
+ it("should send name updates to the API", async () => {
+ setup({ userData: defaultUser });
+ const firstNameField = await screen.findByLabelText("First name");
+ const submitButton = await screen.findByText("Update");
+
+ await userEvent.clear(firstNameField);
+ await userEvent.type(firstNameField, "Misty");
+
+ expect(submitButton).toBeEnabled();
+
+ await userEvent.click(submitButton);
+
+ const call = fetchMock.lastCall("path:/api/user/97", { method: "PUT" });
+
+ const req = await call?.request?.json();
+
+ expect(req).toEqual({
+ first_name: "Misty",
+ last_name: "Ketchum",
+ email: "pikachuboy97@example.com",
+ user_group_memberships: [],
+ login_attributes: {},
+ });
+ });
+
+ describe("users with empty name fields (metabase#46446)", () => {
+ it("can update the first name of a user with no last name", async () => {
+ setup({
+ userData: {
+ id: 11,
+ first_name: "Prince",
+ last_name: null,
+ email: "name@example.com",
+ },
+ });
+
+ const firstNameField = await screen.findByLabelText("First name");
+ const submitButton = await screen.findByText("Update");
+
+ await userEvent.clear(firstNameField);
+ await userEvent.type(firstNameField, "Madonna");
+ await userEvent.click(submitButton);
+
+ const call = fetchMock.lastCall("path:/api/user/11", { method: "PUT" });
+ const req = await call?.request?.json();
+
+ expect(req).toEqual({
+ first_name: "Madonna",
+ last_name: null, // this null key must be present
+ email: "name@example.com",
+ user_group_memberships: [],
+ login_attributes: {},
+ });
+ });
+
+ it("can update the email of a user with no name", async () => {
+ setup({
+ userData: {
+ id: 11,
+ first_name: null,
+ last_name: null,
+ email: "neo@example.com",
+ },
+ });
+
+ const emailField = await screen.findByLabelText("Email *");
+ const submitButton = await screen.findByText("Update");
+
+ await userEvent.clear(emailField);
+ await userEvent.type(emailField, "morpheus@example.com");
+ await userEvent.click(submitButton);
+
+ const call = fetchMock.lastCall("path:/api/user/11", { method: "PUT" });
+ const req = await call?.request?.json();
+
+ expect(req).toEqual({
+ first_name: null, // this null key must be present
+ last_name: null, // this null key must be present
+ email: "morpheus@example.com",
+ user_group_memberships: [],
+ login_attributes: {},
+ });
+ });
+
+ it("can remove a user's name", async () => {
+ setup({
+ userData: {
+ id: 11,
+ first_name: "Simon",
+ last_name: "Garfunkel",
+ email: "s+g@example.com",
+ },
+ });
+
+ const firstNameField = await screen.findByLabelText("First name");
+ const lastNameField = await screen.findByLabelText("Last name");
+ const submitButton = await screen.findByText("Update");
+
+ await userEvent.clear(firstNameField);
+ await userEvent.clear(lastNameField);
+ await userEvent.click(submitButton);
+
+ const call = fetchMock.lastCall("path:/api/user/11", { method: "PUT" });
+ const req = await call?.request?.json();
+
+ expect(req).toEqual({
+ first_name: null, // this null key must be present
+ last_name: null, // this null key must be present
+ email: "s+g@example.com",
+ user_group_memberships: [],
+ login_attributes: {},
+ });
+ });
+ });
+});
diff --git a/frontend/src/metabase/admin/people/containers/EditUserModal/test/enterprise.unit.spec.tsx b/frontend/src/metabase/admin/people/containers/EditUserModal/test/enterprise.unit.spec.tsx
new file mode 100644
index 00000000000..fb08821eb41
--- /dev/null
+++ b/frontend/src/metabase/admin/people/containers/EditUserModal/test/enterprise.unit.spec.tsx
@@ -0,0 +1,80 @@
+import { screen } from "@testing-library/react";
+import userEvent from "@testing-library/user-event";
+import fetchMock from "fetch-mock";
+
+import { setup, defaultUser } from "./setup";
+
+describe("EditUserModal - enterprise", () => {
+ it("can add a user attribute", async () => {
+ setup({ userData: defaultUser, hasEnterprisePlugins: true });
+
+ const submitButton = await screen.findByText("Update");
+ const addAttributeButton = await screen.findByText("Add an attribute");
+
+ await userEvent.click(addAttributeButton);
+
+ await userEvent.type(
+ await screen.findByPlaceholderText("Key"),
+ "favorite_pokemon",
+ );
+ await userEvent.type(
+ await screen.findByPlaceholderText("Value"),
+ "Pikachu",
+ );
+
+ await userEvent.click(submitButton);
+
+ const call = fetchMock.lastCall("path:/api/user/97", { method: "PUT" });
+ const req = await call?.request?.json();
+
+ expect(req).toEqual({
+ first_name: "Ash",
+ last_name: "Ketchum",
+ email: "pikachuboy97@example.com",
+ user_group_memberships: [],
+ login_attributes: {
+ favorite_pokemon: "Pikachu",
+ },
+ });
+ });
+
+ it("can add a user attribute to a user with no name (metabase#40750)", async () => {
+ setup({
+ userData: {
+ ...defaultUser,
+ first_name: null,
+ last_name: null,
+ },
+ hasEnterprisePlugins: true,
+ });
+
+ const submitButton = await screen.findByText("Update");
+ const addAttributeButton = await screen.findByText("Add an attribute");
+
+ await userEvent.click(addAttributeButton);
+
+ await userEvent.type(
+ await screen.findByPlaceholderText("Key"),
+ "favorite_pokemon",
+ );
+ await userEvent.type(
+ await screen.findByPlaceholderText("Value"),
+ "Pikachu",
+ );
+
+ await userEvent.click(submitButton);
+
+ const call = fetchMock.lastCall("path:/api/user/97", { method: "PUT" });
+ const req = await call?.request?.json();
+
+ expect(req).toEqual({
+ first_name: null,
+ last_name: null,
+ email: "pikachuboy97@example.com",
+ user_group_memberships: [],
+ login_attributes: {
+ favorite_pokemon: "Pikachu",
+ },
+ });
+ });
+});
diff --git a/frontend/src/metabase/admin/people/containers/EditUserModal/test/setup.tsx b/frontend/src/metabase/admin/people/containers/EditUserModal/test/setup.tsx
new file mode 100644
index 00000000000..4453bdc2627
--- /dev/null
+++ b/frontend/src/metabase/admin/people/containers/EditUserModal/test/setup.tsx
@@ -0,0 +1,66 @@
+import fetchMock from "fetch-mock";
+
+import { setupEnterprisePlugins } from "__support__/enterprise";
+import { setupUserEndpoints } from "__support__/server-mocks";
+import { mockSettings } from "__support__/settings";
+import { createMockEntitiesState } from "__support__/store";
+import { renderWithProviders } from "__support__/ui";
+import type { User, UserListResult } from "metabase-types/api";
+import {
+ createMockUser,
+ createMockSettings,
+ createMockTokenFeatures,
+} from "metabase-types/api/mocks";
+import { createMockState } from "metabase-types/store/mocks";
+
+import { EditUserModal } from "../EditUserModal";
+
+export const defaultUser = createMockUser({
+ id: 97,
+ first_name: "Ash",
+ last_name: "Ketchum",
+ email: "pikachuboy97@example.com",
+ login_attributes: {},
+});
+
+export const setup = ({
+ userData,
+ hasEnterprisePlugins = false,
+}: {
+ userData: Partial<User>;
+ hasEnterprisePlugins?: boolean;
+}) => {
+ setupUserEndpoints(createMockUser(userData) as unknown as UserListResult);
+
+ const storeInitialState = createMockState({
+ entities: createMockEntitiesState({}),
+ settings: mockSettings(
+ createMockSettings({
+ "token-features": createMockTokenFeatures({
+ advanced_permissions: true,
+ sandboxes: true,
+ }),
+ }),
+ ),
+ });
+
+ if (hasEnterprisePlugins) {
+ setupEnterprisePlugins();
+ }
+
+ fetchMock.get("path:/api/permissions/group", []);
+ fetchMock.get("path:/api/permissions/membership", {});
+
+ // setupPermissionsGraphEndpoints([]);
+ const onCloseSpy = jest.fn();
+ renderWithProviders(
+ <EditUserModal
+ params={{ userId: String(userData.id) }}
+ onClose={onCloseSpy}
+ />,
+ {
+ storeInitialState,
+ },
+ );
+ return { onCloseSpy };
+};
diff --git a/frontend/src/metabase/admin/people/forms/UserForm.tsx b/frontend/src/metabase/admin/people/forms/UserForm.tsx
index 0c107288afa..d779f23327a 100644
--- a/frontend/src/metabase/admin/people/forms/UserForm.tsx
+++ b/frontend/src/metabase/admin/people/forms/UserForm.tsx
@@ -15,9 +15,9 @@ import { PLUGIN_ADMIN_USER_FORM_FIELDS } from "metabase/plugins";
import { Button } from "metabase/ui";
import type { User } from "metabase-types/api";
-const localUserScmeha = Yup.object({
- first_name: Yup.string().max(100, Errors.maxLength).default(""),
- last_name: Yup.string().max(100, Errors.maxLength).default(""),
+const localUserSchema = Yup.object({
+ first_name: Yup.string().nullable().max(100, Errors.maxLength).default(null),
+ last_name: Yup.string().nullable().max(100, Errors.maxLength).default(null),
email: Yup.string().email().required(Errors.required),
});
@@ -37,7 +37,7 @@ export const UserForm = ({
return (
<FormProvider
initialValues={initialValues}
- validationSchema={localUserScmeha}
+ validationSchema={localUserSchema}
enableReinitialize
onSubmit={onSubmit}
>
diff --git a/frontend/test/__support__/server-mocks/user.ts b/frontend/test/__support__/server-mocks/user.ts
index 3cb10c2cabf..6c8cba4bf2b 100644
--- a/frontend/test/__support__/server-mocks/user.ts
+++ b/frontend/test/__support__/server-mocks/user.ts
@@ -5,6 +5,7 @@ import type { User, UserAttribute, UserListResult } from "metabase-types/api";
export function setupUserEndpoints(user: UserListResult) {
fetchMock.get(`path:/api/user/${user.id}`, user);
+ fetchMock.put(`path:/api/user/${user.id}`, user);
}
export function setupUsersEndpoints(users: UserListResult[]) {
--
GitLab