-
Notifications
You must be signed in to change notification settings - Fork 887
feat: integrate backend with idp sync page #14755
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
a40ba7d
7ff9a07
6e303f4
310726b
615c8d3
45f4d13
080b032
8e1e021
c863619
41631f0
3db9f48
431beea
f166784
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -74,7 +74,7 @@ export const AppearanceSettingsPageView: FC< | |
<PopoverContent css={{ transform: "translateY(-28px)" }}> | ||
<PopoverPaywall | ||
message="Appearance" | ||
description="With a Premium license, you can customize the appearance of your deployment." | ||
description="With a Premium license, you can customize the appearance and branding of your deployment." | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just curious: since we now have the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nothing yet, so far this has purely been focused on the enterprise and premium badges and banners. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: |
||
documentationLink="https://coder.com/docs/admin/appearance" | ||
/> | ||
</PopoverContent> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,12 +29,12 @@ import { Link as RouterLink, useNavigate } from "react-router-dom"; | |
import { docs } from "utils/docs"; | ||
import { PermissionPillsList } from "./PermissionPillsList"; | ||
|
||
export type CustomRolesPageViewProps = { | ||
interface CustomRolesPageViewProps { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have we decided on whether we prefer interfaces over type aliases? Asking because I generally avoid interfaces so that I can't do accidental interface merging, but I've been seeing a lot of recent code using interfaces There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My goal was just to be consistent and I saw more interfaces used for all the organization related pages. I don't personally have a strong preference other than being consistent. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like |
||
roles: Role[] | undefined; | ||
onDeleteRole: (role: Role) => void; | ||
canAssignOrgRole: boolean; | ||
isCustomRolesEnabled: boolean; | ||
}; | ||
} | ||
|
||
export const CustomRolesPageView: FC<CustomRolesPageViewProps> = ({ | ||
roles, | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,75 @@ | ||||||
import type { Meta, StoryObj } from "@storybook/react"; | ||||||
import { expect, fn, userEvent, waitFor, within } from "@storybook/test"; | ||||||
import { | ||||||
MockGroupSyncSettings, | ||||||
MockOrganization, | ||||||
MockRoleSyncSettings, | ||||||
} from "testHelpers/entities"; | ||||||
import { ExportPolicyButton } from "./ExportPolicyButton"; | ||||||
|
||||||
const meta: Meta<typeof ExportPolicyButton> = { | ||||||
title: "modules/resources/ExportPolicyButton", | ||||||
component: ExportPolicyButton, | ||||||
args: { | ||||||
syncSettings: MockGroupSyncSettings, | ||||||
type: "groups", | ||||||
organization: MockOrganization, | ||||||
}, | ||||||
}; | ||||||
|
||||||
export default meta; | ||||||
type Story = StoryObj<typeof ExportPolicyButton>; | ||||||
|
||||||
export const Default: Story = {}; | ||||||
|
||||||
export const ClickExportGroupPolicy: Story = { | ||||||
args: { | ||||||
syncSettings: MockGroupSyncSettings, | ||||||
type: "groups", | ||||||
organization: MockOrganization, | ||||||
download: fn(), | ||||||
}, | ||||||
play: async ({ canvasElement, args }) => { | ||||||
const canvas = within(canvasElement); | ||||||
await userEvent.click( | ||||||
canvas.getByRole("button", { name: "Export Policy" }), | ||||||
); | ||||||
await waitFor(() => | ||||||
expect(args.download).toHaveBeenCalledWith( | ||||||
expect.anything(), | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
`${MockOrganization.name}_groups-policy.json`, | ||||||
), | ||||||
); | ||||||
const blob: Blob = (args.download as jest.Mock).mock.lastCall[0]; | ||||||
await expect(blob.type).toEqual("application/json"); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we also assert that the blob contents match the |
||||||
await expect(await blob.text()).toEqual( | ||||||
JSON.stringify(MockGroupSyncSettings, null, 2), | ||||||
); | ||||||
}, | ||||||
}; | ||||||
|
||||||
export const ClickExportRolePolicy: Story = { | ||||||
args: { | ||||||
syncSettings: MockRoleSyncSettings, | ||||||
type: "roles", | ||||||
organization: MockOrganization, | ||||||
download: fn(), | ||||||
}, | ||||||
play: async ({ canvasElement, args }) => { | ||||||
const canvas = within(canvasElement); | ||||||
await userEvent.click( | ||||||
canvas.getByRole("button", { name: "Export Policy" }), | ||||||
); | ||||||
await waitFor(() => | ||||||
expect(args.download).toHaveBeenCalledWith( | ||||||
expect.anything(), | ||||||
`${MockOrganization.name}_roles-policy.json`, | ||||||
), | ||||||
); | ||||||
const blob: Blob = (args.download as jest.Mock).mock.lastCall[0]; | ||||||
await expect(blob.type).toEqual("application/json"); | ||||||
await expect(await blob.text()).toEqual( | ||||||
JSON.stringify(MockRoleSyncSettings, null, 2), | ||||||
); | ||||||
}, | ||||||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
import DownloadOutlined from "@mui/icons-material/DownloadOutlined"; | ||
import Button from "@mui/material/Button"; | ||
import type { | ||
GroupSyncSettings, | ||
Organization, | ||
RoleSyncSettings, | ||
} from "api/typesGenerated"; | ||
import { displayError } from "components/GlobalSnackbar/utils"; | ||
import { saveAs } from "file-saver"; | ||
import { type FC, useMemo, useState } from "react"; | ||
|
||
interface DownloadPolicyButtonProps { | ||
syncSettings: RoleSyncSettings | GroupSyncSettings | undefined; | ||
type: "groups" | "roles"; | ||
organization: Organization; | ||
download?: (file: Blob, filename: string) => void; | ||
} | ||
|
||
export const ExportPolicyButton: FC<DownloadPolicyButtonProps> = ({ | ||
syncSettings, | ||
type, | ||
organization, | ||
download = saveAs, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure how big of a deal this is, but the current implementation means that we can't add additional functionality without completely overriding the default How often would we have a use case where we'd want to get rid of the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was meant to just support the Storybook test case. Is there a better way to handle this? |
||
}) => { | ||
const [isDownloading, setIsDownloading] = useState(false); | ||
|
||
const policyJSON = useMemo(() => { | ||
return syncSettings?.field && syncSettings.mapping | ||
? JSON.stringify(syncSettings, null, 2) | ||
: null; | ||
}, [syncSettings]); | ||
|
||
return ( | ||
<Button | ||
startIcon={<DownloadOutlined />} | ||
disabled={!policyJSON || isDownloading} | ||
onClick={async () => { | ||
if (policyJSON) { | ||
try { | ||
setIsDownloading(true); | ||
const file = new Blob([policyJSON], { | ||
type: "application/json", | ||
}); | ||
download(file, `${organization.name}_${type}-policy.json`); | ||
} catch (e) { | ||
console.error(e); | ||
displayError("Failed to export policy json"); | ||
} finally { | ||
setIsDownloading(false); | ||
} | ||
} | ||
}} | ||
> | ||
Export Policy | ||
</Button> | ||
); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,106 @@ | ||
import { type Interpolation, type Theme, useTheme } from "@emotion/react"; | ||
import Stack from "@mui/material/Stack"; | ||
import { Pill } from "components/Pill/Pill"; | ||
import { | ||
Popover, | ||
PopoverContent, | ||
PopoverTrigger, | ||
} from "components/Popover/Popover"; | ||
import type { FC } from "react"; | ||
|
||
interface PillListProps { | ||
roles: readonly string[]; | ||
} | ||
|
||
// used to check if the role is a UUID | ||
const UUID = | ||
/^[0-9a-f]{8}-[0-9a-f]{4}-[0-5][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i; | ||
|
||
export const IdpPillList: FC<PillListProps> = ({ roles }) => { | ||
return ( | ||
<Stack direction="row" spacing={1}> | ||
{roles.length > 0 ? ( | ||
<Pill css={UUID.test(roles[0]) ? styles.errorPill : styles.pill}> | ||
{roles[0]} | ||
</Pill> | ||
) : ( | ||
<p>None</p> | ||
)} | ||
|
||
{roles.length > 1 && <OverflowPill roles={roles.slice(1)} />} | ||
</Stack> | ||
); | ||
}; | ||
|
||
interface OverflowPillProps { | ||
roles: string[]; | ||
} | ||
|
||
const OverflowPill: FC<OverflowPillProps> = ({ roles }) => { | ||
const theme = useTheme(); | ||
|
||
return ( | ||
<Popover mode="hover"> | ||
<PopoverTrigger> | ||
<Pill | ||
css={{ | ||
backgroundColor: theme.palette.background.paper, | ||
borderColor: theme.palette.divider, | ||
}} | ||
data-testid="overflow-pill" | ||
> | ||
+{roles.length} more | ||
</Pill> | ||
</PopoverTrigger> | ||
Comment on lines
+44
to
+54
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Potential accessibility issue since we're adding interactive functionality to a plain div There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This pattern originally came from UserRoleCell.tsx and also exists in PermissionsPillsList.tsx, what would you suggest to use instead of a div? or would we have to move away from using PopoverTrigger for pills? |
||
|
||
<PopoverContent | ||
disableRestoreFocus | ||
disableScrollLock | ||
css={{ | ||
".MuiPaper-root": { | ||
display: "flex", | ||
flexFlow: "column wrap", | ||
columnGap: 8, | ||
rowGap: 12, | ||
padding: "12px 16px", | ||
alignContent: "space-around", | ||
minWidth: "auto", | ||
backgroundColor: theme.palette.background.default, | ||
}, | ||
}} | ||
anchorOrigin={{ | ||
vertical: -4, | ||
horizontal: "center", | ||
}} | ||
transformOrigin={{ | ||
vertical: "bottom", | ||
horizontal: "center", | ||
}} | ||
> | ||
{roles.map((role) => ( | ||
<Pill | ||
key={role} | ||
css={UUID.test(role) ? styles.errorPill : styles.pill} | ||
> | ||
{role} | ||
</Pill> | ||
))} | ||
</PopoverContent> | ||
</Popover> | ||
); | ||
}; | ||
|
||
const styles = { | ||
pill: (theme) => ({ | ||
backgroundColor: theme.experimental.pillDefault.background, | ||
borderColor: theme.experimental.pillDefault.outline, | ||
color: theme.experimental.pillDefault.text, | ||
width: "fit-content", | ||
}), | ||
errorPill: (theme) => ({ | ||
backgroundColor: theme.roles.error.background, | ||
borderColor: theme.roles.error.outline, | ||
color: theme.roles.error.text, | ||
width: "fit-content", | ||
}), | ||
} satisfies Record<string, Interpolation<Theme>>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add this above the
getGroup...
function too pls?