-
Notifications
You must be signed in to change notification settings - Fork 943
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 8 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 |
---|---|---|
|
@@ -141,6 +141,32 @@ export const provisionerDaemonGroups = (organization: string) => { | |
}; | ||
}; | ||
|
||
export const getGroupIdpSyncSettingsKey = (organization: string) => [ | ||
"organization", | ||
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 what we want to go with, but we have some inconsistencies in our query keys right now. We want all query keys that involve organizations to have the exact same prefix so that they can grouped and invalidated together Right now we're using 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. @aslilac Any preference for changing all query keys to be 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 try to match the routes on the backend (which all do include the s) 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. @aslilac ok I will use plural here, but there are still several keys that do not match the routes, |
||
organization, | ||
"groupIdpSyncSettings", | ||
]; | ||
|
||
export const groupIdpSyncSettings = (organization: string) => { | ||
return { | ||
queryKey: getGroupIdpSyncSettingsKey(organization), | ||
queryFn: () => API.getGroupIdpSyncSettingsByOrganization(organization), | ||
}; | ||
}; | ||
|
||
export const getRoleIdpSyncSettingsKey = (organization: string) => [ | ||
"organization", | ||
organization, | ||
"roleIdpSyncSettings", | ||
]; | ||
|
||
export const roleIdpSyncSettings = (organization: string) => { | ||
return { | ||
queryKey: getRoleIdpSyncSettingsKey(organization), | ||
queryFn: () => API.getRoleIdpSyncSettingsByOrganization(organization), | ||
}; | ||
}; | ||
|
||
/** | ||
* Fetch permissions for a single organization. | ||
* | ||
|
@@ -243,6 +269,13 @@ export const organizationsPermissions = ( | |
}, | ||
action: "read", | ||
}, | ||
viewIdpSyncSettings: { | ||
object: { | ||
resource_type: "idpsync_settings", | ||
organization_id: organizationId, | ||
}, | ||
action: "read", | ||
}, | ||
}); | ||
|
||
// The endpoint takes a flat array, so to avoid collisions prepend each | ||
|
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,69 @@ | ||||||
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: { | ||||||
policy: JSON.stringify(MockGroupSyncSettings, null, 2), | ||||||
type: "groups", | ||||||
organization: MockOrganization, | ||||||
}, | ||||||
}; | ||||||
|
||||||
export default meta; | ||||||
type Story = StoryObj<typeof ExportPolicyButton>; | ||||||
|
||||||
export const Default: Story = {}; | ||||||
|
||||||
export const ClickExportGroupPolicy: Story = { | ||||||
args: { | ||||||
policy: JSON.stringify(MockGroupSyncSettings, null, 2), | ||||||
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.calls[0][0]; | ||||||
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. oh this is so gross (but I don't know what to suggest to make it better) could you add a comment above it to elaborate why this incantation is justified/necessary? I think I understand what it's doing, but clarity is good, and more junior devs might not get it! |
||||||
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 |
||||||
}, | ||||||
}; | ||||||
|
||||||
export const ClickExportRolePolicy: Story = { | ||||||
args: { | ||||||
policy: JSON.stringify(MockRoleSyncSettings, null, 2), | ||||||
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.calls[0][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. Same comments for the group story apply here |
||||||
}, | ||||||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
import DownloadOutlined from "@mui/icons-material/DownloadOutlined"; | ||
import Button from "@mui/material/Button"; | ||
import type { Organization } from "api/typesGenerated"; | ||
import { displayError } from "components/GlobalSnackbar/utils"; | ||
import { saveAs } from "file-saver"; | ||
import { type FC, useState } from "react"; | ||
|
||
interface DownloadPolicyButtonProps { | ||
policy: string | null; | ||
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. Is there any information on how policies are defined? I'm seeing that we're downloading just the policy itself when the user clicks the button 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. The policy is meant to be a json representation of |
||
type: "groups" | "roles"; | ||
organization: Organization; | ||
download?: (file: Blob, filename: string) => void; | ||
} | ||
|
||
export const ExportPolicyButton: FC<DownloadPolicyButtonProps> = ({ | ||
policy, | ||
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); | ||
|
||
return ( | ||
<Button | ||
startIcon={<DownloadOutlined />} | ||
disabled={!policy || isDownloading} | ||
onClick={async () => { | ||
if (policy) { | ||
try { | ||
setIsDownloading(true); | ||
const file = new Blob([policy], { | ||
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,105 @@ | ||
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[]; | ||
} | ||
|
||
const UUID = | ||
/^[0-9a-f]{8}-[0-9a-f]{4}-[1-5][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i; | ||
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 regex is long enough that I think it needs a comment on what it's trying to account for Looks like 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. The goal here is just if a string is a UUID used for a group |
||
|
||
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> | ||
); | ||
}; | ||
|
||
type OverflowPillProps = { | ||
roles: string[]; | ||
}; | ||
jaaydenh marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
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?