refactor: Improve roles UI (#5576)

site/src/components/EditRolesButton/EditRolesButton.stories.tsx

@@ -0,0 +1,40 @@
+import { ComponentMeta, Story } from "@storybook/react"
+import {
+  MockOwnerRole,
+  MockSiteRoles,
+  MockUserAdminRole,
+} from "testHelpers/entities"
+import { EditRolesButtonProps, EditRolesButton } from "./EditRolesButton"
+
+export default {
+  title: "components/EditRolesButton",
+  component: EditRolesButton,
+  argTypes: {
+    defaultIsOpen: {
+      defaultValue: true,
+    },
+  },
+} as ComponentMeta<typeof EditRolesButton>
+
+const Template: Story<EditRolesButtonProps> = (args) => (
+  <EditRolesButton {...args} />
+)
+
+export const Open = Template.bind({})
+Open.args = {
+  roles: MockSiteRoles,
+  selectedRoles: [MockUserAdminRole, MockOwnerRole],
+}
+Open.parameters = {
+  chromatic: { delay: 300 },
+}
+
+export const Loading = Template.bind({})
+Loading.args = {
+  isLoading: true,
+  roles: MockSiteRoles,
+  selectedRoles: [MockUserAdminRole, MockOwnerRole],
+}
+Loading.parameters = {
+  chromatic: { delay: 300 },
+}

site/src/components/EditRolesButton/EditRolesButton.tsx

@@ -0,0 +1,196 @@
+import IconButton from "@material-ui/core/IconButton"
+import { EditSquare } from "components/Icons/EditSquare"
+import { useRef, useState, FC } from "react"
+import { makeStyles } from "@material-ui/core/styles"
+import { useTranslation } from "react-i18next"
+import Popover from "@material-ui/core/Popover"
+import { Stack } from "components/Stack/Stack"
+import Checkbox from "@material-ui/core/Checkbox"
+import UserIcon from "@material-ui/icons/PersonOutline"
+import { Role } from "api/typesGenerated"
+
+const Option: React.FC<{
+  value: string
+  name: string
+  description: string
+  isChecked: boolean
+  onChange: (roleName: string) => void
+}> = ({ value, name, description, isChecked, onChange }) => {
+  const styles = useStyles()
+
+  return (
+    <label htmlFor={name} className={styles.option}>
+      <Stack direction="row" alignItems="flex-start">
+        <Checkbox
+          id={name}
+          size="small"
+          color="primary"
+          className={styles.checkbox}
+          value={value}
+          checked={isChecked}
+          onChange={(e) => {
+            onChange(e.currentTarget.value)
+          }}
+        />
+        <Stack spacing={0.5}>
+          <strong>{name}</strong>
+          <span className={styles.optionDescription}>{description}</span>
+        </Stack>
+      </Stack>
+    </label>
+  )
+}
+
+export interface EditRolesButtonProps {
+  isLoading: boolean
+  roles: Role[]
+  selectedRoles: Role[]
+  onChange: (roles: Role["name"][]) => void
+  defaultIsOpen?: boolean
+}
+
+export const EditRolesButton: FC<EditRolesButtonProps> = ({
+  roles,
+  selectedRoles,
+  onChange,
+  isLoading,
+  defaultIsOpen = false,
+}) => {
+  const styles = useStyles()
+  const { t } = useTranslation("usersPage")
+  const anchorRef = useRef<HTMLButtonElement>(null)
+  const [isOpen, setIsOpen] = useState(defaultIsOpen)
+  const id = isOpen ? "edit-roles-popover" : undefined
+  const selectedRoleNames = selectedRoles.map((role) => role.name)
+
+  const handleChange = (roleName: string) => {
+    if (selectedRoleNames.includes(roleName)) {
+      onChange(selectedRoleNames.filter((role) => role !== roleName))
+      return
+    }
+
+    onChange([...selectedRoleNames, roleName])
+  }
+
+  return (
+    <>
+      <IconButton
+        ref={anchorRef}
+        size="small"
+        className={styles.editButton}
+        title={t("editUserRolesTooltip")}
+        onClick={() => setIsOpen(true)}
+      >
+        <EditSquare />
+      </IconButton>
+
+      <Popover
+        id={id}
+        open={isOpen}
+        anchorEl={anchorRef.current}
+        onClose={() => setIsOpen(false)}
+        anchorOrigin={{
+          vertical: "bottom",
+          horizontal: "left",
+        }}
+        transformOrigin={{
+          vertical: "top",
+          horizontal: "left",
+        }}
+        classes={{ paper: styles.popoverPaper }}
+      >
+        <fieldset
+          className={styles.fieldset}
+          disabled={isLoading}
+          title={t("fieldSetRolesTooltip")}
+        >
+          <Stack className={styles.options} spacing={3}>
+            {roles.map((role) => (
+              <Option
+                key={role.name}
+                onChange={handleChange}
+                isChecked={selectedRoleNames.includes(role.name)}
+                value={role.name}
+                name={role.display_name}
+                description={t(`roleDescription.${role.name}`)}
+              />
+            ))}
+          </Stack>
+        </fieldset>
+        <div className={styles.footer}>
+          <Stack direction="row" alignItems="flex-start">
+            <UserIcon className={styles.userIcon} />
+            <Stack spacing={0.5}>
+              <strong>{t("member")}</strong>
+              <span className={styles.optionDescription}>
+                {t("roleDescription.member")}
+              </span>
+            </Stack>
+          </Stack>
+        </div>
+      </Popover>
+    </>
+  )
+}
+
+const useStyles = makeStyles((theme) => ({
+  editButton: {
+    color: theme.palette.text.secondary,
+
+    "& .MuiSvgIcon-root": {
+      width: theme.spacing(2),
+      height: theme.spacing(2),
+      position: "relative",
+      top: -2, // Align the pencil square
+    },
+
+    "&:hover": {
+      color: theme.palette.text.primary,
+      backgroundColor: "transparent",
+    },
+  },
+  popoverPaper: {
+    width: theme.spacing(45),
+    marginTop: theme.spacing(1),
+    background: theme.palette.background.paperLight,
+  },
+  fieldset: {
+    border: 0,
+    margin: 0,
+    padding: 0,
+
+    "&:disabled": {
+      opacity: 0.5,
+    },
+  },
+  options: {
+    padding: theme.spacing(3),
+  },
+  option: {
+    cursor: "pointer",
+  },
+  checkbox: {
+    padding: 0,
+    position: "relative",
+    top: 1, // Alignment
+
+    "& svg": {
+      width: theme.spacing(2.5),
+      height: theme.spacing(2.5),
+    },
+  },
+  optionDescription: {
+    fontSize: 12,
+    color: theme.palette.text.secondary,
+  },
+  footer: {
+    padding: theme.spacing(3),
+    backgroundColor: theme.palette.background.paper,
+    borderTop: `1px solid ${theme.palette.divider}`,
+  },
+  userIcon: {
+    width: theme.spacing(2.5), // Same as the checkbox
+    height: theme.spacing(2.5),
+    color: theme.palette.primary.main,
+  },
+}))

site/src/components/Icons/EditSquare.tsx

@@ -0,0 +1,7 @@
+import SvgIcon, { SvgIconProps } from "@material-ui/core/SvgIcon"
+
+export const EditSquare = (props: SvgIconProps): JSX.Element => (
+  <SvgIcon {...props} viewBox="0 0 48 48">
+    <path d="M9 47.4q-1.2 0-2.1-.9-.9-.9-.9-2.1v-30q0-1.2.9-2.1.9-.9 2.1-.9h20.25l-3 3H9v30h30V27l3-3v20.4q0 1.2-.9 2.1-.9.9-2.1.9Zm15-18Zm9.1-17.6 2.15 2.1L21 28.1v4.3h4.25l14.3-14.3 2.1 2.1L26.5 35.4H18v-8.5Zm8.55 8.4-8.55-8.4 5-5q.85-.85 2.125-.85t2.125.9l4.2 4.25q.85.9.85 2.125t-.9 2.075Z" />
+  </SvgIcon>
+)

site/src/components/RoleSelect/RoleSelect.stories.tsx

@@ -1,45 +0,0 @@
-import { ComponentMeta, Story } from "@storybook/react"
-import {
-  assignableRole,
-  MockAuditorRole,
-  MockMemberRole,
-  MockOwnerRole,
-  MockTemplateAdminRole,
-  MockUserAdminRole,
-} from "../../testHelpers/renderHelpers"
-import { RoleSelect, RoleSelectProps } from "./RoleSelect"
-
-export default {
-  title: "components/RoleSelect",
-  component: RoleSelect,
-} as ComponentMeta<typeof RoleSelect>
-
-const Template: Story<RoleSelectProps> = (args) => <RoleSelect {...args} />
-
-// Include 4 roles:
-// - owner (disabled, not checked)
-// - template admin (disabled, checked)
-// - auditor (enabled, not checked)
-// - user admin (enabled, checked)
-export const Close = Template.bind({})
-Close.args = {
-  roles: [
-    assignableRole(MockOwnerRole, false),
-    assignableRole(MockTemplateAdminRole, false),
-    assignableRole(MockAuditorRole, true),
-    assignableRole(MockUserAdminRole, true),
-  ],
-  selectedRoles: [MockUserAdminRole, MockTemplateAdminRole, MockMemberRole],
-}
-
-export const Open = Template.bind({})
-Open.args = {
-  open: true,
-  roles: [
-    assignableRole(MockOwnerRole, false),
-    assignableRole(MockTemplateAdminRole, false),
-    assignableRole(MockAuditorRole, true),
-    assignableRole(MockUserAdminRole, true),
-  ],
-  selectedRoles: [MockUserAdminRole, MockTemplateAdminRole, MockMemberRole],
-}

site/src/components/RoleSelect/RoleSelect.test.tsx

@@ -1,50 +0,0 @@
-import { screen } from "@testing-library/react"
-import {
-  assignableRole,
-  MockAuditorRole,
-  MockMemberRole,
-  MockOwnerRole,
-  MockTemplateAdminRole,
-  MockUserAdminRole,
-  render,
-} from "testHelpers/renderHelpers"
-import { RoleSelect } from "./RoleSelect"
-
-describe("UserRoleSelect", () => {
-  it("renders content", async () => {
-    // When
-    render(
-      <RoleSelect
-        roles={[
-          assignableRole(MockOwnerRole, false),
-          assignableRole(MockTemplateAdminRole, false),
-          assignableRole(MockAuditorRole, true),
-          assignableRole(MockUserAdminRole, true),
-        ]}
-        selectedRoles={[
-          MockUserAdminRole,
-          MockTemplateAdminRole,
-          MockMemberRole,
-        ]}
-        loading={false}
-        onChange={jest.fn()}
-        open
-      />,
-    )
-
-    // Then
-    const owner = await screen.findByText(MockOwnerRole.display_name)
-    const templateAdmin = await screen.findByText(
-      MockTemplateAdminRole.display_name,
-    )
-    const auditor = await screen.findByText(MockAuditorRole.display_name)
-    const userAdmin = await screen.findByText(MockUserAdminRole.display_name)
-
-    // The attributes are "strings", not boolean types.
-    expect(owner.getAttribute("aria-disabled")).toBe("true")
-    expect(templateAdmin.getAttribute("aria-disabled")).toBe("true")
-
-    expect(userAdmin.getAttribute("aria-disabled")).toBe("false")
-    expect(auditor.getAttribute("aria-disabled")).toBe("false")
-  })
-})

site/src/components/RoleSelect/RoleSelect.tsx

@@ -1,79 +0,0 @@
-import Checkbox from "@material-ui/core/Checkbox"
-import MenuItem from "@material-ui/core/MenuItem"
-import Select from "@material-ui/core/Select"
-import { makeStyles, Theme } from "@material-ui/core/styles"
-import { FC } from "react"
-import { AssignableRoles, Role } from "../../api/typesGenerated"
-
-export const Language = {
-  label: "Roles",
-}
-export interface RoleSelectProps {
-  roles: AssignableRoles[]
-  selectedRoles: Role[]
-  onChange: (roles: Role["name"][]) => void
-  loading?: boolean
-  open?: boolean
-}
-
-export const RoleSelect: FC<React.PropsWithChildren<RoleSelectProps>> = ({
-  roles,
-  selectedRoles,
-  loading,
-  onChange,
-  open,
-}) => {
-  const styles = useStyles()
-  const value = selectedRoles.map((r) => r.name)
-  const renderValue = () => selectedRoles.map((r) => r.display_name).join(", ")
-  const sortedRoles = roles.sort((a, b) =>
-    a.display_name.localeCompare(b.display_name),
-  )
-
-  return (
-    <Select
-      aria-label={Language.label}
-      open={open}
-      multiple
-      value={value}
-      renderValue={renderValue}
-      variant="outlined"
-      className={styles.select}
-      onChange={(e) => {
-        const { value } = e.target
-        onChange(value as string[])
-      }}
-    >
-      {sortedRoles.map((r) => {
-        const isChecked = selectedRoles.some(
-          (selectedRole) => selectedRole.name === r.name,
-        )
-
-        return (
-          <MenuItem
-            key={r.name}
-            value={r.name}
-            disabled={loading || !r.assignable}
-          >
-            <Checkbox size="small" color="primary" checked={isChecked} />{" "}
-            {r.display_name}
-          </MenuItem>
-        )
-      })}
-    </Select>
-  )
-}
-
-const useStyles = makeStyles((theme: Theme) => ({
-  select: {
-    margin: 0,
-    // Set a fixed width for the select. It avoids selects having different sizes
-    // depending on how many roles they have selected.
-    width: theme.spacing(32),
-    "& .MuiSelect-root": {
-      // Adjusting padding because it does not have label
-      paddingTop: theme.spacing(1.5),
-      paddingBottom: theme.spacing(1.5),
-    },
-  },
-}))

site/src/components/UsersTable/UsersTable.tsx

@@ -54,15 +54,16 @@ export const UsersTable: FC<React.PropsWithChildren<UsersTableProps>> = ({
       <Table>
         <TableHead>
           <TableRow>
-            <TableCell width="35%">{Language.usernameLabel}</TableCell>
-            <TableCell width="15%">{Language.statusLabel}</TableCell>
-            <TableCell width="15%">{Language.lastSeenLabel}</TableCell>
-            <TableCell width="35%">
+            <TableCell width="30%">{Language.usernameLabel}</TableCell>
+            <TableCell width="40%">
               <Stack direction="row" spacing={1} alignItems="center">
                 <span>{Language.rolesLabel}</span>
                 <UserRoleHelpTooltip />
               </Stack>
             </TableCell>
+            <TableCell width="15%">{Language.statusLabel}</TableCell>
+            <TableCell width="15%">{Language.lastSeenLabel}</TableCell>
+
             {/* 1% is a trick to make the table cell width fit the content */}
             {canEditUsers && <TableCell width="1%" />}
           </TableRow>

site/src/components/UsersTable/UsersTableBody.tsx

@@ -11,9 +11,22 @@ import * as TypesGen from "../../api/typesGenerated"
 import { combineClasses } from "../../util/combineClasses"
 import { AvatarData } from "../AvatarData/AvatarData"
 import { EmptyState } from "../EmptyState/EmptyState"
-import { RoleSelect } from "../RoleSelect/RoleSelect"
 import { TableLoader } from "../TableLoader/TableLoader"
 import { TableRowMenu } from "../TableRowMenu/TableRowMenu"
+import { EditRolesButton } from "components/EditRolesButton/EditRolesButton"
+import { Stack } from "components/Stack/Stack"
+
+const isOwnerRole = (role: TypesGen.Role): boolean => {
+  return role.name === "owner"
+}
+
+const roleOrder = ["owner", "user-admin", "template-admin", "auditor"]
+
+const sortRoles = (roles: TypesGen.Role[]) => {
+  return roles.slice(0).sort((a, b) => {
+    return roleOrder.indexOf(a.name) - roleOrder.indexOf(b.name)
+  })
+}
 
 interface UsersTableBodyProps {
   users?: TypesGen.User[]
@@ -89,7 +102,7 @@ export const UsersTableBody: FC<
                 display_name: "Member",
               }
               const userRoles =
-                user.roles.length === 0 ? [fallbackRole] : user.roles
+                user.roles.length === 0 ? [fallbackRole] : sortRoles(user.roles)
 
               return (
                 <TableRow key={user.id}>
@@ -109,6 +122,34 @@ export const UsersTableBody: FC<
                       }
                     />
                   </TableCell>
+                  <TableCell>
+                    <Stack direction="row" spacing={1}>
+                      {canEditUsers && (
+                        <EditRolesButton
+                          roles={roles ? sortRoles(roles) : []}
+                          selectedRoles={userRoles}
+                          isLoading={Boolean(isUpdatingUserRoles)}
+                          onChange={(roles) => {
+                            // Remove the fallback role because it is only for the UI
+                            const rolesWithoutFallback = roles.filter(
+                              (role) => role !== fallbackRole.name,
+                            )
+                            onUpdateUserRoles(user, rolesWithoutFallback)
+                          }}
+                        />
+                      )}
+                      {userRoles.map((role) => (
+                        <Pill
+                          key={role.name}
+                          text={role.display_name}
+                          className={combineClasses({
+                            [styles.rolePill]: true,
+                            [styles.rolePillOwner]: isOwnerRole(role),
+                          })}
+                        />
+                      ))}
+                    </Stack>
+                  </TableCell>
                   <TableCell
                     className={combineClasses([
                       styles.status,
@@ -122,32 +163,6 @@ export const UsersTableBody: FC<
                   <TableCell>
                     <LastUsed lastUsedAt={user.last_seen_at} />
                   </TableCell>
-                  <TableCell>
-                    {canEditUsers ? (
-                      <RoleSelect
-                        roles={roles ?? []}
-                        selectedRoles={userRoles}
-                        loading={isUpdatingUserRoles}
-                        onChange={(roles) => {
-                          // Remove the fallback role because it is only for the UI
-                          roles = roles.filter(
-                            (role) => role !== fallbackRole.name,
-                          )
-                          onUpdateUserRoles(user, roles)
-                        }}
-                      />
-                    ) : (
-                      <div className={styles.roles}>
-                        {userRoles.map((role) => (
-                          <Pill
-                            key={role.name}
-                            text={role.display_name}
-                            className={styles.rolePill}
-                          />
-                        ))}
-                      </div>
-                    )}
-                  </TableCell>
                   {canEditUsers && (
                     <TableCell>
                       <TableRowMenu
@@ -206,13 +221,12 @@ const useStyles = makeStyles((theme) => ({
     height: theme.spacing(4.5),
     borderRadius: "100%",
   },
-  roles: {
-    display: "flex",
-    gap: theme.spacing(1),
-    flexWrap: "wrap",
-  },
   rolePill: {
     backgroundColor: theme.palette.background.paperLight,
     borderColor: theme.palette.divider,
   },
+  rolePillOwner: {
+    backgroundColor: theme.palette.info.dark,
+    borderColor: theme.palette.info.light,
+  },
 }))

site/src/i18n/en/usersPage.json

@@ -5,5 +5,15 @@
   "deleteMenuItem": "Delete",
   "listWorkspacesMenuItem": "View workspaces",
   "activateMenuItem": "Activate",
-  "resetPasswordMenuItem": "Reset password"
+  "resetPasswordMenuItem": "Reset password",
+  "editUserRolesTooltip": "Edit user roles",
+  "fieldSetRolesTooltip": "Available roles",
+  "roleDescription": {
+    "owner": "Owner can manage all resources, including users and their permissions",
+    "user-admin": "User admin can manage all users and groups",
+    "template-admin": "Template admin can manage all templates and permissions",
+    "auditor": "Auditor can access the audit logs",
+    "member": "Everybody is a member. This is a shared and default role for all users"
+  },
+  "member": "Member"
 }

site/src/pages/UsersPage/UsersPage.test.tsx

@@ -7,9 +7,9 @@ import * as API from "../../api/api"
 import { Role } from "../../api/typesGenerated"
 import { Language as ResetPasswordDialogLanguage } from "../../components/Dialogs/ResetPasswordDialog/ResetPasswordDialog"
 import { GlobalSnackbar } from "../../components/GlobalSnackbar/GlobalSnackbar"
-import { Language as RoleSelectLanguage } from "../../components/RoleSelect/RoleSelect"
 import {
   MockAuditorRole,
+  MockOwnerRole,
   MockUser,
   MockUser2,
   renderWithAuth,
@@ -156,32 +156,27 @@ const resetUserPassword = async (setupActionSpies: () => void) => {
 const updateUserRole = async (setupActionSpies: () => void, role: Role) => {
   // Get the first user in the table
   const users = await screen.findAllByText(/.*@coder.com/)
-  const firstUserRow = users[0].closest("tr")
-  if (!firstUserRow) {
+  const userRow = users[0].closest("tr")
+  if (!userRow) {
     throw new Error("Error on get the first user row")
   }
 
-  // Click on the "roles" menu to display the role options
-  const rolesLabel = within(firstUserRow).getByLabelText(
-    RoleSelectLanguage.label,
-  )
-  const rolesMenuTrigger = within(rolesLabel).getByRole("button")
-  // For MUI v4, the Select was changed to open on mouseDown instead of click
-  // https://github.com/mui-org/material-ui/pull/17978
-  fireEvent.mouseDown(rolesMenuTrigger)
+  // Click on the "edit icon" to display the role options
+  const buttonTitle = t("editUserRolesTooltip", { ns: "usersPage" })
+  const editButton = within(userRow).getByTitle(buttonTitle)
+  fireEvent.click(editButton)
 
   // Setup spies to check the actions after
   setupActionSpies()
 
   // Click on the role option
-  const listBox = screen.getByRole("listbox")
-  const auditorOption = within(listBox).getByRole("option", {
-    name: role.display_name,
-  })
+  const fieldsetTitle = t("fieldSetRolesTooltip", { ns: "usersPage" })
+  const fieldset = await screen.findByTitle(fieldsetTitle)
+  const auditorOption = within(fieldset).getByText(role.display_name)
   fireEvent.click(auditorOption)
 
   return {
-    rolesMenuTrigger,
+    userRow,
   }
 }
 
@@ -402,7 +397,7 @@ describe("UsersPage", () => {
       it("updates the roles", async () => {
         renderPage()
 
-        const { rolesMenuTrigger } = await updateUserRole(() => {
+        const { userRow } = await updateUserRole(() => {
           jest.spyOn(API, "updateUserRoles").mockResolvedValueOnce({
             ...MockUser,
             roles: [...MockUser.roles, MockAuditorRole],
@@ -410,9 +405,10 @@ describe("UsersPage", () => {
         }, MockAuditorRole)
 
         // Check if the select text was updated with the Auditor role
-        await waitFor(() =>
-          expect(rolesMenuTrigger).toHaveTextContent("Owner, Auditor"),
-        )
+        await waitFor(() => {
+          expect(userRow).toHaveTextContent(MockOwnerRole.display_name)
+          expect(userRow).toHaveTextContent(MockAuditorRole.display_name)
+        })
 
         // Check if the API was called correctly
         const currentRoles = MockUser.roles.map((r) => r.name)

site/src/theme/theme.ts

@@ -5,6 +5,7 @@ import { getOverrides } from "./overrides"
 import { darkPalette } from "./palettes"
 import { props } from "./props"
 import { typography } from "./typography"
+import isChromatic from "chromatic/isChromatic"
 
 const makeTheme = (palette: PaletteOptions) => {
   const theme = createTheme({
@@ -16,6 +17,13 @@ const makeTheme = (palette: PaletteOptions) => {
     props,
   })
 
+  // We want to disable transitions during chromatic snapshots
+  // https://www.chromatic.com/docs/animations#javascript-animations
+  // https://github.com/mui/material-ui/issues/10560#issuecomment-439147374
+  if (isChromatic()) {
+    theme.transitions.create = () => "none"
+  }
+
   theme.overrides = getOverrides(theme)
 
   return theme