Skip to content

feat: enable editing of IDP sync configuration for groups and roles in the UI #16098

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.

Sign up for GitHub

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

Merged
merged 40 commits into from
Jan 27, 2025
Merged

feat: enable editing of IDP sync configuration for groups and roles in the UI #16098

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
96b0c09
chore: remove stack component
jaaydenh Jan 10, 2025
a88b72f
chore: styling cleanup
jaaydenh Jan 10, 2025
cb00c73
feat: add form and mutation queries
jaaydenh Jan 10, 2025
4b15a2a
feat: add form fields
jaaydenh Jan 12, 2025
0befeca
chore: add delete button
jaaydenh Jan 14, 2025
f8349de
fix: update input component to 40px height
jaaydenh Jan 14, 2025
7124c11
fix: fix styles for MultiSelectCombobox
jaaydenh Jan 14, 2025
30c8a9c
chore: update export policy button design
jaaydenh Jan 14, 2025
a7c954b
fix: update copy and spacing
jaaydenh Jan 14, 2025
c09a3f3
chore: update styles
jaaydenh Jan 14, 2025
8714f5e
fix: fix storybook tests
jaaydenh Jan 14, 2025
b558f35
fix: fix legacy group mappings styles
jaaydenh Jan 14, 2025
aa0e53c
fix: format
jaaydenh Jan 14, 2025
7635856
feat: create link component
jaaydenh Jan 14, 2025
91ca380
fix: use new Link component
jaaydenh Jan 15, 2025
328ce2f
fix: use correct field value
jaaydenh Jan 15, 2025
52d563c
fix: update error handling
jaaydenh Jan 15, 2025
b1d7649
feat: add e2e tests for group and role sync
jaaydenh Jan 15, 2025
b3812f1
chore: update link components
jaaydenh Jan 15, 2025
d19a0dc
fix: remove unnecessary arbitrary values
jaaydenh Jan 16, 2025
43159ff
chore: extract Form components
jaaydenh Jan 16, 2025
528f2d8
fix: adding loading spinners
jaaydenh Jan 16, 2025
d283fa1
fix: update icon button sizing to match design
jaaydenh Jan 17, 2025
ad2fc45
chore: improve e2e tests
jaaydenh Jan 20, 2025
c99e009
fix: change to min width fit
jaaydenh Jan 20, 2025
31cd599
fix: updates for PR review comments
jaaydenh Jan 20, 2025
2354883
fix: remove async from onClick
jaaydenh Jan 22, 2025
3c63c8c
fix: fix tabs styling
jaaydenh Jan 22, 2025
623a50e
fix: format
jaaydenh Jan 22, 2025
5779c8d
fix: updates for PR comments
jaaydenh Jan 22, 2025
395fdc8
chore: switch to useId for form ids
jaaydenh Jan 22, 2025
79ce400
fix: styling
jaaydenh Jan 22, 2025
854684a
fix: add resolves
jaaydenh Jan 22, 2025
50c12db
fix: revert button styles
jaaydenh Jan 22, 2025
16665e2
chore: use Link component
jaaydenh Jan 22, 2025
3dbcee3
fix: show form errors and update mapping schema validation
jaaydenh Jan 27, 2025
9b709ae
fix: remove unnecessary async
jaaydenh Jan 27, 2025
bee5783
fix: remove useEffect
jaaydenh Jan 27, 2025
d3b64b2
fix: error display
jaaydenh Jan 27, 2025
0362c16
fix: format
jaaydenh Jan 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
fix: updates for PR review comments
  • Loading branch information
@jaaydenh
jaaydenh committed Jan 22, 2025
commit 31cd599cb3cb4669a407b5a02694fffe4c48c5cd
4 changes: 2 additions & 2 deletions site/src/components/Link/Link.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,12 @@ export const linkVariants = cva(
after:hover:content-[''] after:hover:absolute after:hover:left-0 after:hover:w-full after:hover:h-px after:hover:bg-current after:hover:bottom-px
focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-content-link
focus-visible:ring-offset-2 focus-visible:ring-offset-surface-primary focus-visible:rounded-sm
visited:text-content-link pl-0.5`, //pl-[2px] adjusts the underline spacing to align with the icon on the right.
visited:text-content-link pl-0.5`, //pl-0.5 adjusts the underline spacing to align with the icon on the right.
{
variants: {
size: {
lg: "text-sm gap-0.5 [&_svg]:size-icon-sm [&_svg]:p-0.5 leading-6",
sm: "text-xs gap-1 [&_svg]:size-icon-xs [&_svg]:p-px leading-[18px]",
sm: "text-xs gap-1 [&_svg]:size-icon-xs [&_svg]:p-px leading-5",
},
},
defaultVariants: {
Expand Down
6 changes: 6 additions & 0 deletions site/src/components/MultiSelectCombobox/MultiSelectCombobox.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -430,6 +430,12 @@ export const MultiSelectCombobox = forwardRef<
return undefined;
};

useEffect(() => {
if (inputRef.current && inputProps?.id) {
inputRef.current.id = inputProps?.id;
}
}, [inputProps?.id]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this effect call trying to sync? I would think that you would normally be able to pass the ID down to the base element directly via props

With this current setup, React's reconciler might get out of sync with the DOM:

  1. Input mounts with ID value 1
  2. Re-render happens with ID value 2
  3. Effect fires after repaint, and the underlying DOM element is mutated
  4. At this point, even though all the updates happened through React, React thinks that the ID is still value 1, even though it's now value 2. Specifically because the value changed outside of the main render logic

I don't see anything complicated enough that would require this kind of risk, and I would hope the component primitive would let you bind the ID to the element directly in the render step

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved this logic outside of the useEffect


const fixedOptions = selected.filter((s) => s.fixed);

return (
Expand Down
4 changes: 3 additions & 1 deletion site/src/components/Tabs/Tabs.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,9 @@ export const TabLink: FC<TabLinkProps> = ({ value, ...linkProps }) => {
<Link
{...linkProps}
className={cn(
"text-sm text-content-secondary no-underline font-medium py-2 px-3 hover:text-content-primary rounded-md",
`text-sm text-content-secondary no-underline font-medium py-3 px-1 mr-6 hover:text-content-primary rounded-md
focus-visible:ring-offset-1 focus-visible:ring-offset-surface-primary
focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-content-link focus-visible:rounded-sm`,
{
"text-content-primary relative before:absolute before:bg-surface-invert-primary before:left-0 before:w-full before:h-px before:-bottom-px before:content-['']":
isActive,
Expand Down
11 changes: 7 additions & 4 deletions site/src/pages/DeploymentSettingsPage/IdpOrgSyncPage/IdpOrgSyncPageView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ export const IdpOrgSyncPageView: FC<IdpSyncPageViewProps> = ({
const SYNC_FIELD_ID = "sync-field";
const ORGANIZATION_ASSIGN_DEFAULT_ID = "organization-assign-default";
const IDP_ORGANIZATION_NAME_ID = "idp-organization-name";
const CODER_ORG_ID = "coder-org";

return (
<div className="flex flex-col gap-2">
Expand Down Expand Up @@ -183,10 +184,13 @@ export const IdpOrgSyncPageView: FC<IdpSyncPageViewProps> = ({
/>
</div>
<div className="grid items-center gap-1 flex-1">
<Label className="text-sm" htmlFor=":r1d:">
<Label className="text-sm" htmlFor={CODER_ORG_ID}>
Coder organization
</Label>
<MultiSelectCombobox
inputProps={{
id: CODER_ORG_ID,
}}
className="min-w-60 max-w-3xl"
value={coderOrgs}
onChange={setCoderOrgs}
Expand All @@ -204,9 +208,8 @@ export const IdpOrgSyncPageView: FC<IdpSyncPageViewProps> = ({
/>
</div>
<div className="grid grid-rows-[28px_auto]">
&nbsp;
<div />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an improvement for screen readers, but I'm still not sure why the alignment couldn't be set up with pure CSS?

Copy link
Contributor Author

@jaaydenh jaaydenh Jan 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am definitely not an expert with css grid as I usually use flexbox but I wanted to try to make this work with a grid. I thought something needs to exist in each cell of the grid for it to display correctly. Is there something better to use than a div or a nbsp?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that I'm writing this out, I'm realizng that I've never used this grid property before, but I would imagine that align-items: end would do the trick

If that doesn't work with grid, it should definitely work with flexbox. Either way, the approach I've been thinking about is having the element be aligned to the end bound of the cross-axis

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went through every combination of align-items and align-self on the row and each cell and none of these resolved the alignment issue.

<Button
className="mb-px"
type="submit"
disabled={!idpOrgName || coderOrgs.length === 0}
onClick={async () => {
Expand All @@ -233,7 +236,7 @@ export const IdpOrgSyncPageView: FC<IdpSyncPageViewProps> = ({
<IdpMappingTable isEmpty={organizationMappingCount === 0}>
{form.values.mapping &&
Object.entries(form.values.mapping)
.sort()
.sort(([a], [b]) => a.toLowerCase().localeCompare(b.toLowerCase()))
.map(([idpOrg, organizations]) => (
<OrganizationRow
key={idpOrg}
Expand Down
17 changes: 10 additions & 7 deletions site/src/pages/ManagementSettingsPage/IdpSyncPage/IdpGroupSyncForm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ export const IdpGroupSyncForm = ({
const REGEX_FILTER_ID = "regex-filter";
const AUTO_CREATE_MISSING_GROUPS_ID = "auto-create-missing-groups";
const IDP_GROUP_NAME_ID = "idp-group-name";
const CODER_GROUP_ID = "coder-group";

return (
<form onSubmit={form.handleSubmit}>
Expand All @@ -116,7 +117,7 @@ export const IdpGroupSyncForm = ({
<Label className="text-sm" htmlFor={SYNC_FIELD_ID}>
Group sync field
</Label>
<Label className="text-sm" htmlFor={SYNC_FIELD_ID}>
<Label className="text-sm" htmlFor={REGEX_FILTER_ID}>
Regex filter
</Label>
<Input
Expand Down Expand Up @@ -188,10 +189,13 @@ export const IdpGroupSyncForm = ({
/>
</div>
<div className="grid items-center gap-1 flex-1">
<Label className="text-sm" htmlFor=":r1d:">
<Label className="text-sm" htmlFor={CODER_GROUP_ID}>
Coder group
</Label>
<MultiSelectCombobox
inputProps={{
id: CODER_GROUP_ID,
}}
className="min-w-60 max-w-3xl"
value={coderGroups}
onChange={setCoderGroups}
Expand All @@ -209,9 +213,8 @@ export const IdpGroupSyncForm = ({
/>
</div>
<div className="grid grid-rows-[28px_auto]">
&nbsp;
<div />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flagging this empty div, too – not sure why this couldn't be CSS

<Button
className="mb-px"
type="submit"
disabled={!idpGroupName || coderGroups.length === 0}
onClick={async () => {
Expand Down Expand Up @@ -240,7 +243,7 @@ export const IdpGroupSyncForm = ({
<IdpMappingTable type="Group" rowCount={groupMappingCount}>
{groupSyncSettings?.mapping &&
Object.entries(groupSyncSettings.mapping)
.sort()
.sort(([a], [b]) => a.toLowerCase().localeCompare(b.toLowerCase()))
.map(([idpGroup, groups]) => (
<GroupRow
key={idpGroup}
Expand All @@ -256,10 +259,10 @@ export const IdpGroupSyncForm = ({
<LegacyGroupSyncHeader />
<IdpMappingTable type="Group" rowCount={legacyGroupMappingCount}>
{Object.entries(groupSyncSettings.legacy_group_name_mapping)
.sort()
.sort(([a], [b]) => a.toLowerCase().localeCompare(b.toLowerCase()))
.map(([idpGroup, groupId]) => (
<GroupRow
key={idpGroup}
key={groupId}
idpGroup={idpGroup}
coderGroup={getGroupNames([groupId])}
onDelete={handleDelete}
Expand Down
28 changes: 0 additions & 28 deletions site/src/pages/ManagementSettingsPage/IdpSyncPage/IdpMappingTable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import Skeleton from "@mui/material/Skeleton";
import Table from "@mui/material/Table";
import TableBody from "@mui/material/TableBody";
import TableCell from "@mui/material/TableCell";
Expand All @@ -8,10 +7,6 @@ import TableRow from "@mui/material/TableRow";
import { ChooseOne, Cond } from "components/Conditionals/ChooseOne";
import { EmptyState } from "components/EmptyState/EmptyState";
import { Link } from "components/Link/Link";
import {
TableLoaderSkeleton,
TableRowSkeleton,
} from "components/TableLoader/TableLoader";
import type { FC } from "react";
import { docs } from "utils/docs";

Expand All @@ -26,8 +21,6 @@ export const IdpMappingTable: FC<IdpMappingTableProps> = ({
rowCount,
children,
}) => {
const isLoading = false;

return (
<div className="flex flex-col w-full gap-2">
<TableContainer>
Expand All @@ -43,9 +36,6 @@ export const IdpMappingTable: FC<IdpMappingTableProps> = ({
</TableHead>
<TableBody>
<ChooseOne>
<Cond condition={isLoading}>
<TableLoader />
</Cond>
<Cond condition={rowCount === 0}>
<TableRow>
<TableCell colSpan={999}>
Expand Down Expand Up @@ -75,21 +65,3 @@ export const IdpMappingTable: FC<IdpMappingTableProps> = ({
</div>
);
};

const TableLoader = () => {
return (
<TableLoaderSkeleton>
<TableRowSkeleton>
<TableCell>
<Skeleton variant="text" width="25%" />
</TableCell>
<TableCell>
<Skeleton variant="text" width="25%" />
</TableCell>
<TableCell>
<Skeleton variant="text" width="25%" />
</TableCell>
</TableRowSkeleton>
</TableLoaderSkeleton>
);
};
11 changes: 7 additions & 4 deletions site/src/pages/ManagementSettingsPage/IdpSyncPage/IdpRoleSyncForm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ export const IdpRoleSyncForm = ({

const SYNC_FIELD_ID = "sync-field";
const IDP_ROLE_NAME_ID = "idp-role-name";
const CODER_ROLE_ID = "coder-role";

return (
<form onSubmit={form.handleSubmit}>
Expand Down Expand Up @@ -130,10 +131,13 @@ export const IdpRoleSyncForm = ({
/>
</div>
<div className="grid items-center gap-1 flex-1">
<Label className="text-sm" htmlFor=":r1d:">
<Label className="text-sm" htmlFor={CODER_ROLE_ID}>
Coder role
</Label>
<MultiSelectCombobox
inputProps={{
id: CODER_ROLE_ID,
}}
className="min-w-60 max-w-3xl"
value={coderRoles}
onChange={setCoderRoles}
Expand All @@ -151,9 +155,8 @@ export const IdpRoleSyncForm = ({
/>
</div>
<div className="grid grid-rows-[28px_auto]">
&nbsp;
<div />
<Button
className="mb-px"
type="submit"
disabled={!idpRoleName || coderRoles.length === 0}
onClick={async () => {
Expand All @@ -180,7 +183,7 @@ export const IdpRoleSyncForm = ({
<IdpMappingTable type="Role" rowCount={roleMappingCount}>
{roleSyncSettings?.mapping &&
Object.entries(roleSyncSettings.mapping)
.sort()
.sort(([a], [b]) => a.toLowerCase().localeCompare(b.toLowerCase()))
.map(([idpRole, roles]) => (
<RoleRow
key={idpRole}
Expand Down