From 259560cad1b176ac1103d1cfc8f294bf580eb514 Mon Sep 17 00:00:00 2001
From: Gustavo Saiani <gus@metabase.com>
Date: Sat, 18 Feb 2023 14:15:33 -0300
Subject: [PATCH] Fix GroupSelect showing groups section title even if section
is empty (#28348)
---
.../GroupSelect.tsx} | 63 ++++++++---
.../GroupSelect/GroupSelect.unit.spec.tsx | 47 ++++++++
.../people/components/GroupSelect/index.tsx | 1 +
.../AddMappingRow/AddMappingRow.tsx | 2 +-
.../DeleteGroupMappingModal.tsx | 5 +-
.../GroupMappingsWidget.unit.spec.tsx | 9 +-
.../GroupSelect/GroupSelect.tsx | 107 +++++++++++++++---
.../MappingRow/MappingRow.tsx | 7 +-
.../widgets/GroupMappingsWidget => }/types.ts | 0
.../admin/settings/sso/ldap.cy.spec.js | 2 +-
10 files changed, 199 insertions(+), 44 deletions(-)
rename frontend/src/metabase/admin/people/components/{GroupSelect.jsx => GroupSelect/GroupSelect.tsx} (55%)
create mode 100644 frontend/src/metabase/admin/people/components/GroupSelect/GroupSelect.unit.spec.tsx
create mode 100644 frontend/src/metabase/admin/people/components/GroupSelect/index.tsx
rename frontend/src/metabase/admin/{settings/components/widgets/GroupMappingsWidget => }/types.ts (100%)
diff --git a/frontend/src/metabase/admin/people/components/GroupSelect.jsx b/frontend/src/metabase/admin/people/components/GroupSelect/GroupSelect.tsx
similarity index 55%
rename from frontend/src/metabase/admin/people/components/GroupSelect.jsx
rename to frontend/src/metabase/admin/people/components/GroupSelect/GroupSelect.tsx
index 9972506be0e..76d53aa280f 100644
--- a/frontend/src/metabase/admin/people/components/GroupSelect.jsx
+++ b/frontend/src/metabase/admin/people/components/GroupSelect/GroupSelect.tsx
@@ -1,7 +1,7 @@
-/* eslint-disable react/prop-types */
import React from "react";
import { t } from "ttag";
+import { isNotNull } from "metabase/core/utils/types";
import Icon from "metabase/components/Icon";
import Select from "metabase/core/components/Select";
import PopoverWithTrigger from "metabase/components/PopoverWithTrigger";
@@ -13,7 +13,39 @@ import {
getGroupColor,
getGroupNameLocalized,
} from "metabase/lib/groups";
-import GroupSummary from "./GroupSummary";
+import { GroupIds, UserGroupType, UserGroupsType } from "metabase/admin/types";
+import GroupSummary from "../GroupSummary";
+
+type GroupSelectProps = {
+ groups: UserGroupsType;
+ selectedGroupIds: GroupIds;
+ onGroupChange: (group: UserGroupType, selected: boolean) => void;
+ isCurrentUser?: boolean;
+ emptyListMessage?: string;
+};
+
+function getSections(groups: UserGroupsType) {
+ const adminGroup = groups.find(isAdminGroup);
+ const defaultGroup = groups.find(isDefaultGroup);
+ const topGroups = [defaultGroup, adminGroup].filter(g => g != null);
+ const groupsExceptDefaultAndAdmin = groups.filter(
+ g => !isAdminGroup(g) && !isDefaultGroup(g),
+ );
+
+ if (topGroups.length === 0) {
+ return [{ items: groupsExceptDefaultAndAdmin }];
+ }
+
+ return [
+ { items: topGroups },
+ groupsExceptDefaultAndAdmin.length > 0
+ ? {
+ items: groupsExceptDefaultAndAdmin as any,
+ name: t`Groups`,
+ }
+ : null,
+ ].filter(isNotNull);
+}
export const GroupSelect = ({
groups,
@@ -21,7 +53,7 @@ export const GroupSelect = ({
onGroupChange,
isCurrentUser = false,
emptyListMessage = t`No groups`,
-}) => {
+}: GroupSelectProps) => {
const triggerElement = (
<div className="flex align-center">
<span className="mr1 text-medium">
@@ -38,35 +70,32 @@ export const GroupSelect = ({
</PopoverWithTrigger>
);
}
- const other = groups.filter(g => !isAdminGroup(g) && !isDefaultGroup(g));
- const adminGroup = groups.find(isAdminGroup);
- const defaultGroup = groups.find(isDefaultGroup);
- const topGroups = [defaultGroup, adminGroup].filter(g => g != null);
+
+ const sections = getSections(groups);
return (
<Select
triggerElement={triggerElement}
- onChange={({ target: { value } }) => {
+ onChange={({ target: { value } }: { target: { value: any } }) => {
groups
.filter(
// find the differing groups between the new `value` on previous `selectedGroupIds`
group =>
- selectedGroupIds.includes(group.id) ^ value.includes(group.id),
+ (selectedGroupIds.includes(group.id) as any) ^
+ value.includes(group.id),
)
.forEach(group => onGroupChange(group, value.includes(group.id)));
}}
- optionDisabledFn={group =>
+ optionDisabledFn={(group: UserGroupType) =>
(isAdminGroup(group) && isCurrentUser) || !canEditMembership(group)
}
- optionValueFn={group => group.id}
+ optionValueFn={(group: UserGroupType) => group.id}
optionNameFn={getGroupNameLocalized}
- optionStylesFn={group => ({ color: getGroupColor(group) })}
+ optionStylesFn={(group: UserGroupType) => ({
+ color: getGroupColor(group),
+ })}
value={selectedGroupIds}
- sections={
- topGroups.length > 0
- ? [{ items: topGroups }, { items: other, name: t`Groups` }]
- : [{ items: other }]
- }
+ sections={sections}
multiple
/>
);
diff --git a/frontend/src/metabase/admin/people/components/GroupSelect/GroupSelect.unit.spec.tsx b/frontend/src/metabase/admin/people/components/GroupSelect/GroupSelect.unit.spec.tsx
new file mode 100644
index 00000000000..964207b309c
--- /dev/null
+++ b/frontend/src/metabase/admin/people/components/GroupSelect/GroupSelect.unit.spec.tsx
@@ -0,0 +1,47 @@
+import React from "react";
+import { render, screen } from "@testing-library/react";
+import userEvent from "@testing-library/user-event";
+
+import GroupSelect from "./GroupSelect";
+
+const adminGroup = { id: 2, name: "Administrators", member_count: 1 };
+
+const setup = ({ groups = [adminGroup], selectedGroupIds = [] } = {}) => {
+ const onGroupChangeSpy = jest.fn();
+
+ render(
+ <GroupSelect
+ groups={groups}
+ selectedGroupIds={selectedGroupIds}
+ onGroupChange={onGroupChangeSpy}
+ />,
+ );
+
+ return { onGroupChangeSpy };
+};
+
+describe("GroupSelect", () => {
+ afterEach(() => {
+ jest.clearAllMocks();
+ });
+
+ describe("when only the Administrators group is passed", () => {
+ it("should not show a Groups section (metabase#27728)", () => {
+ setup();
+
+ userEvent.click(screen.getByText("Default"));
+
+ expect(screen.queryByText("Groups")).not.toBeInTheDocument();
+ });
+
+ it("should allow you to select the Administrators group", () => {
+ const { onGroupChangeSpy } = setup();
+
+ userEvent.click(screen.getByText("Default"));
+ userEvent.click(screen.getByText("Administrators"));
+
+ expect(onGroupChangeSpy).toHaveBeenCalledTimes(1);
+ expect(onGroupChangeSpy).toHaveBeenCalledWith(adminGroup, true);
+ });
+ });
+});
diff --git a/frontend/src/metabase/admin/people/components/GroupSelect/index.tsx b/frontend/src/metabase/admin/people/components/GroupSelect/index.tsx
new file mode 100644
index 00000000000..f30bebc756f
--- /dev/null
+++ b/frontend/src/metabase/admin/people/components/GroupSelect/index.tsx
@@ -0,0 +1 @@
+export { default } from "./GroupSelect";
diff --git a/frontend/src/metabase/admin/settings/components/widgets/GroupMappingsWidget/AddMappingRow/AddMappingRow.tsx b/frontend/src/metabase/admin/settings/components/widgets/GroupMappingsWidget/AddMappingRow/AddMappingRow.tsx
index 03484585c00..3073b393a08 100644
--- a/frontend/src/metabase/admin/settings/components/widgets/GroupMappingsWidget/AddMappingRow/AddMappingRow.tsx
+++ b/frontend/src/metabase/admin/settings/components/widgets/GroupMappingsWidget/AddMappingRow/AddMappingRow.tsx
@@ -4,7 +4,7 @@ import { t } from "ttag";
import Button from "metabase/core/components/Button";
-import type { MappingsType } from "../types";
+import type { MappingsType } from "metabase/admin/types";
type AddMappingRowProps = {
mappings: MappingsType;
diff --git a/frontend/src/metabase/admin/settings/components/widgets/GroupMappingsWidget/DeleteGroupMappingModal/DeleteGroupMappingModal.tsx b/frontend/src/metabase/admin/settings/components/widgets/GroupMappingsWidget/DeleteGroupMappingModal/DeleteGroupMappingModal.tsx
index a404b35b753..c3f2dedf83b 100644
--- a/frontend/src/metabase/admin/settings/components/widgets/GroupMappingsWidget/DeleteGroupMappingModal/DeleteGroupMappingModal.tsx
+++ b/frontend/src/metabase/admin/settings/components/widgets/GroupMappingsWidget/DeleteGroupMappingModal/DeleteGroupMappingModal.tsx
@@ -6,7 +6,10 @@ import { ModalFooter } from "metabase/components/ModalContent";
import Radio from "metabase/core/components/Radio";
import Button from "metabase/core/components/Button";
-import type { DeleteMappingModalValueType, GroupIds } from "../types";
+import type {
+ DeleteMappingModalValueType,
+ GroupIds,
+} from "metabase/admin/types";
import {
ModalHeader,
ModalSubtitle,
diff --git a/frontend/src/metabase/admin/settings/components/widgets/GroupMappingsWidget/GroupMappingsWidget.unit.spec.tsx b/frontend/src/metabase/admin/settings/components/widgets/GroupMappingsWidget/GroupMappingsWidget.unit.spec.tsx
index 406644c429a..e5bf152fb3f 100644
--- a/frontend/src/metabase/admin/settings/components/widgets/GroupMappingsWidget/GroupMappingsWidget.unit.spec.tsx
+++ b/frontend/src/metabase/admin/settings/components/widgets/GroupMappingsWidget/GroupMappingsWidget.unit.spec.tsx
@@ -63,9 +63,12 @@ describe("GroupMappingsWidget", () => {
// Confirm remove
userEvent.click(screen.getByText("Yes"));
- await waitFor(() => {
- expect(onChangeSettingSpy).toHaveBeenCalledTimes(1);
- });
+ await waitFor(
+ () => {
+ expect(onChangeSettingSpy).toHaveBeenCalledTimes(1);
+ },
+ { timeout: 10000 },
+ );
});
});
diff --git a/frontend/src/metabase/admin/settings/components/widgets/GroupMappingsWidget/GroupSelect/GroupSelect.tsx b/frontend/src/metabase/admin/settings/components/widgets/GroupMappingsWidget/GroupSelect/GroupSelect.tsx
index 7015d9bc508..f6df0694730 100644
--- a/frontend/src/metabase/admin/settings/components/widgets/GroupMappingsWidget/GroupSelect/GroupSelect.tsx
+++ b/frontend/src/metabase/admin/settings/components/widgets/GroupMappingsWidget/GroupSelect/GroupSelect.tsx
@@ -1,31 +1,104 @@
import React from "react";
import { t } from "ttag";
-import LoadingSpinner from "metabase/components/LoadingSpinner";
-import GroupSelect from "metabase/admin/people/components/GroupSelect";
+import { isNotNull } from "metabase/core/utils/types";
+import Icon from "metabase/components/Icon";
+import Select from "metabase/core/components/Select";
+import PopoverWithTrigger from "metabase/components/PopoverWithTrigger";
-import type { GroupIds, UserGroupsType } from "../types";
+import {
+ isDefaultGroup,
+ isAdminGroup,
+ canEditMembership,
+ getGroupColor,
+ getGroupNameLocalized,
+} from "metabase/lib/groups";
+import { GroupIds, UserGroupType, UserGroupsType } from "metabase/admin/types";
+import GroupSummary from "metabase/admin/people/components/GroupSummary";
-type Props = {
+type GroupSelectProps = {
groups: UserGroupsType;
selectedGroupIds: GroupIds;
- onGroupChange: () => void;
+ onGroupChange: (group: UserGroupType, selected: boolean) => void;
+ isCurrentUser?: boolean;
+ emptyListMessage?: string;
};
-const GroupMappingsWidgetGroupSelect = ({
+function getSections(groups: UserGroupsType) {
+ const adminGroup = groups.find(isAdminGroup);
+ const defaultGroup = groups.find(isDefaultGroup);
+ const topGroups = [defaultGroup, adminGroup].filter(g => g != null);
+ const groupsExceptDefaultAndAdmin = groups.filter(
+ g => !isAdminGroup(g) && !isDefaultGroup(g),
+ );
+
+ if (topGroups.length === 0) {
+ return [{ items: groupsExceptDefaultAndAdmin }];
+ }
+
+ return [
+ { items: topGroups },
+ groupsExceptDefaultAndAdmin.length > 0
+ ? {
+ items: groupsExceptDefaultAndAdmin as any,
+ name: t`Groups`,
+ }
+ : null,
+ ].filter(isNotNull);
+}
+
+export const GroupSelect = ({
groups,
- selectedGroupIds,
+ selectedGroupIds = [],
onGroupChange,
-}: Props) =>
- groups ? (
- <GroupSelect
- groups={groups}
- selectedGroupIds={selectedGroupIds}
- onGroupChange={onGroupChange}
- emptyListMessage={t`No mappable groups`}
+ isCurrentUser = false,
+ emptyListMessage = t`No groups`,
+}: GroupSelectProps) => {
+ const triggerElement = (
+ <div className="flex align-center">
+ <span className="mr1 text-medium">
+ <GroupSummary groups={groups} selectedGroupIds={selectedGroupIds} />
+ </span>
+ <Icon className="text-light" name="chevrondown" size={10} />
+ </div>
+ );
+
+ if (groups.length === 0) {
+ return (
+ <PopoverWithTrigger triggerElement={triggerElement}>
+ <span className="p1">{emptyListMessage}</span>
+ </PopoverWithTrigger>
+ );
+ }
+
+ const sections = getSections(groups);
+
+ return (
+ <Select
+ triggerElement={triggerElement}
+ onChange={({ target: { value } }: { target: { value: any } }) => {
+ groups
+ .filter(
+ // find the differing groups between the new `value` on previous `selectedGroupIds`
+ group =>
+ (selectedGroupIds.includes(group.id) as any) ^
+ value.includes(group.id),
+ )
+ .forEach(group => onGroupChange(group, value.includes(group.id)));
+ }}
+ optionDisabledFn={(group: UserGroupType) =>
+ (isAdminGroup(group) && isCurrentUser) || !canEditMembership(group)
+ }
+ optionValueFn={(group: UserGroupType) => group.id}
+ optionNameFn={getGroupNameLocalized}
+ optionStylesFn={(group: UserGroupType) => ({
+ color: getGroupColor(group),
+ })}
+ value={selectedGroupIds}
+ sections={sections}
+ multiple
/>
- ) : (
- <LoadingSpinner />
);
+};
-export default GroupMappingsWidgetGroupSelect;
+export default GroupSelect;
diff --git a/frontend/src/metabase/admin/settings/components/widgets/GroupMappingsWidget/MappingRow/MappingRow.tsx b/frontend/src/metabase/admin/settings/components/widgets/GroupMappingsWidget/MappingRow/MappingRow.tsx
index c7a44d99e9e..19dd9c608cb 100644
--- a/frontend/src/metabase/admin/settings/components/widgets/GroupMappingsWidget/MappingRow/MappingRow.tsx
+++ b/frontend/src/metabase/admin/settings/components/widgets/GroupMappingsWidget/MappingRow/MappingRow.tsx
@@ -8,14 +8,13 @@ import Tooltip from "metabase/core/components/Tooltip";
import Icon from "metabase/components/Icon";
import Confirm from "metabase/components/Confirm";
-import Selectbox from "../GroupSelect";
-import DeleteGroupMappingModal from "../DeleteGroupMappingModal";
-
import type {
DeleteMappingModalValueType,
GroupIds,
UserGroupsType,
-} from "../types";
+} from "metabase/admin/types";
+import Selectbox from "../GroupSelect";
+import DeleteGroupMappingModal from "../DeleteGroupMappingModal";
import { DeleteMappingButton } from "./MappingRow.styled";
diff --git a/frontend/src/metabase/admin/settings/components/widgets/GroupMappingsWidget/types.ts b/frontend/src/metabase/admin/types.ts
similarity index 100%
rename from frontend/src/metabase/admin/settings/components/widgets/GroupMappingsWidget/types.ts
rename to frontend/src/metabase/admin/types.ts
diff --git a/frontend/test/metabase/scenarios/admin/settings/sso/ldap.cy.spec.js b/frontend/test/metabase/scenarios/admin/settings/sso/ldap.cy.spec.js
index c0294cd104c..b2891770f3b 100644
--- a/frontend/test/metabase/scenarios/admin/settings/sso/ldap.cy.spec.js
+++ b/frontend/test/metabase/scenarios/admin/settings/sso/ldap.cy.spec.js
@@ -182,7 +182,7 @@ const enterLdapSettings = () => {
const createMapping = name => {
cy.button("New mapping").click();
- cy.focused().type(name);
+ cy.findByPlaceholderText("cn=People,ou=Groups,dc=metabase,dc=com").type(name);
cy.button("Add").click();
};
--
GitLab