Fix GroupSelect showing groups section title even if section is empty (#28348)

frontend/src/metabase/admin/people/components/GroupSelect.jsx → frontend/src/metabase/admin/people/components/GroupSelect/GroupSelect.tsx

-/* 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
     />
   );

frontend/src/metabase/admin/people/components/GroupSelect/GroupSelect.unit.spec.tsx

+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);
+    });
+  });
+});

frontend/src/metabase/admin/people/components/GroupSelect/index.tsx

+export { default } from "./GroupSelect";

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;

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,

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 },
+      );
     });
   });
 

frontend/src/metabase/admin/settings/components/widgets/GroupMappingsWidget/GroupSelect/GroupSelect.tsx

 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;

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";
 

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();
 };