8000 feat: integrate backend with idp sync page by jaaydenh · Pull Request #14755 · coder/coder · GitHub
[go: up one dir, main page]
Skip to content

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.

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 13 commits into from
Sep 24, 2024
Merged

feat: integrate backend with idp sync page #14755

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
a40ba7d
feat: idp sync initial commit
jaaydenh Sep 17, 2024
7ff9a07
fix: hookup backend data for groups and roles
jaaydenh Sep 20, 2024
6e303f4
chore: cleanup
jaaydenh Sep 20, 2024
310726b
feat: separate groups and roles into tabs
jaaydenh Sep 21, 2024
615c8d3
feat: implement export policy button
jaaydenh Sep 22, 2024
45f4d13
feat: handle missing groups
jaaydenh Sep 22, 2024
080b032
chore: add story for missing groups
jaaydenh Sep 22, 2024
8e1e021
chore: add stories for export policy button
jaaydenh Sep 22, 2024
c863619
fix: updates for PR review
jaaydenh Sep 23, 2024
41631f0
chore: update tests
jaaydenh Sep 24, 2024
3db9f48
chore: document uuid regex
jaaydenh Sep 24, 2024
431beea
chore: remove unused
jaaydenh Sep 24, 2024
f16 10000 6784
fix: fix stories
jaaydenh Sep 24, 2024
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
Next Next commit
feat: idp sync initial commit
  • Loading branch information
@jaaydenh
jaaydenh committed Sep 20, 2024
commit a40ba7d3de288bc71101ef03c81aa02afe7179a8
21 changes: 21 additions & 0 deletions site/src/api/api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -704,6 +704,27 @@ class ApiMethods {
return response.data;
};

getGroupIdpSyncSettingsByOrganization = async (
organization: string,
): Promise<TypesGen.GroupSyncSettings> => {
const response = await this.axios.get<TypesGen.GroupSyncSettings>(
`/api/v2/organizations/${organization}/settings/idpsync/groups`,
);
return response.data;
};

/**
* @param organization Can be the organization's ID or name
*/
Comment on lines +719 to +721
Copy link
Member

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?

getRoleIdpSyncSettingsByOrganization = async (
organization: string,
): Promise<TypesGen.RoleSyncSettings> => {
const response = await this.axios.get<TypesGen.RoleSyncSettings>(
`/api/v2/organizations/${organization}/settings/idpsync/roles`,
);
return response.data;
};

getTemplate = async (templateId: string): Promise<TypesGen.Template> => {
const response = await this.axios.get<TypesGen.Template>(
`/api/v2/templates/${templateId}`,
Expand Down
26 changes: 26 additions & 0 deletions site/src/api/queries/organizations.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,32 @@ export const provisionerDaemonGroups = (organization: string) => {
};
};

export const getGroupIdpSyncSettingsKey = (organization: string) => [
"organization",
Copy link
Member

Choose a reason for hiding this comment

The 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 organization (singular) and organizations (plural). My gut feeling would be to use the plural version for everything, like you would for an HTTP route, but I don't have enough context

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aslilac Any preference for changing all query keys to be consistent?

Copy link
Member

Choose a reason for hiding this comment

The 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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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, organizationMembersKey, getProvisionerDaemonGroupsKey,

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.
*
Expand Down
2 changes: 1 addition & 1 deletion site/src/pages/DeploySettingsPage/AppearanceSettingsPage/AppearanceSettingsPageView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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."
Copy link
Member

Choose a reason for hiding this comment

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

Just curious: since we now have the branding role, have there been any conversations about extending that down the line to work with customer-specific branding?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

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

nit: theme.branding is not a role, it lives outside of theme.roles, it's just an object/namespace

documentationLink="https://coder.com/docs/admin/appearance"
/>
</PopoverContent>
Expand Down
49 changes: 41 additions & 8 deletions site/src/pages/ManagementSettingsPage/IdpSyncPage/IdpSyncPage.tsx
341A
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,20 @@ import { SettingsHeader } from "components/SettingsHeader/SettingsHeader";
import { Stack } from "components/Stack/Stack";
import type { FC } from "react";
import { Helmet } from "react-helmet-async";
import { Link as RouterLink } from "react-router-dom";
import { Link as RouterLink, useParams } from "react-router-dom";
import { docs } from "utils/docs";
import { pageTitle } from "utils/page";
import { IdpSyncHelpTooltip } from "./IdpSyncHelpTooltip";
import IdpSyncPageView from "./IdpSyncPageView";
import {
organizationsPermissions,
groupIdpSyncSettings,
roleIdpSyncSettings,
} from "api/queries/organizations";
import { useQuery } from "react-query";
import { useOrganizationSettings } from "../ManagementSettingsLayout";
import { Loader } from "components/Loader/Loader";
import { EmptyState } from "components/EmptyState/EmptyState";

const mockOIDCConfig = {
allow_signups: true,
Expand Down Expand Up @@ -45,19 +54,39 @@ const mockOIDCConfig = {
};

export const IdpSyncPage: FC = () => {
const { organization: organizationName } = useParams() as {
organization: string;
};
Comment on lines +25 to +27
Copy link
Member

Choose a reason for hiding this comment

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

Not dinging you for this, but commenting to flag that this is something we should look into down the line:

This logic should be extracted into a more type-safe custom hook. There's something to be said for avoiding hasty abstractions, but we have 33 instances of this pattern already

Copy link
Member

Choose a reason for hiding this comment

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

@Parkreiner I'm actually working on that in another branch rn


// feature visibility and permissions to be implemented when integrating with backend
// const feats = useFeatureVisibility();
// const { organization: organizationName } = useParams() as {
// organization: string;
// };
// const { organizations } = useOrganizationSettings();
// const organization = organizations?.find((o) => o.name === organizationName);
// const permissionsQuery = useQuery(organizationPermissions(organization?.id));
const { organizations } = useOrganizationSettings();
const organization = organizations?.find((o) => o.name === organizationName);
const permissionsQuery = useQuery(
organizationsPermissions(organizations?.map((o) => o.id)),
);
const groupIdpSyncSettingsQuery = useQuery(
groupIdpSyncSettings(organizationName),
);
const roleIdpSyncSettingsQuery = useQuery(
roleIdpSyncSettings(organizationName),
);
// const permissions = permissionsQuery.data;

// if (!permissions) {
// return <Loader />;
// }
if (!organization) {
return <EmptyState message="Organization not found" />;
}

if (
permissionsQuery.isLoading ||
groupIdpSyncSettingsQuery.isLoading ||
roleIdpSyncSettingsQuery.isLoading
) {
return <Loader />;
}

return (
<>
Expand Down Expand Up @@ -91,7 +120,11 @@ export const IdpSyncPage: FC = () => {
</Stack>
</Stack>

<IdpSyncPageView oidcConfig={mockOIDCConfig} />
<IdpSyncPageView
oidcConfig={mockOIDCConfig}
groupSyncSettings={groupIdpSyncSettingsQuery.data}
roleSyncSettings={roleIdpSyncSettingsQuery.data}
/>
</>
);
};
Expand Down
33 changes: 22 additions & 11 deletions site/src/pages/ManagementSettingsPage/IdpSyncPage/IdpSyncPageView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,11 @@ import TableCell from "@mui/material/TableCell";
import TableContainer from "@mui/material/TableContainer";
import TableHead from "@mui/material/TableHead";
import TableRow from "@mui/material/TableRow";
import type { OIDCConfig } from "api/typesGenerated";
import type {
OIDCConfig,
GroupSyncSettings,
RoleSyncSettings,
} from "api/typesGenerated";
import { ChooseOne, Cond } from "components/Conditionals/ChooseOne";
import { EmptyState } from "components/EmptyState/EmptyState";
import { Paywall } from "components/Paywall/Paywall";
Expand All @@ -25,16 +29,17 @@ import { docs } from "utils/docs";

export type IdpSyncPageViewProps = {
oidcConfig: OIDCConfig | undefined;
groupSyncSettings: GroupSyncSettings | undefined;
roleSyncSettings: RoleSyncSettings | undefined;
};

export const IdpSyncPageView: FC<IdpSyncPageViewProps> = ({ oidcConfig }) => {
export const IdpSyncPageView: FC<IdpSyncPageViewProps> = ({
Copy link
Member

Choose a reason for hiding this comment

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

Have to put the comment here because the code was already in place previously, and it's not selectable for review:

I think we need to add some CSS optical adjustments to the key-value pairs for the IdpField component. Since we're using two different typefaces, their baselines are slightly different. And when you place them right next to each other on the same line, there's enough variation that the text looks sloppy

You might have to open this in a new tab, but the baselines are ever so slightly off, and the cap-heights are, too:
Screenshot 2024-09-23 at 12 21 28 PM

Copy link
Member

Choose a reason for hiding this comment

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

Though a slightly easier and probably much more manageable alternative would be to set all the text in monospace

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The monospace font was supposed to be a temporary measure until this page becomes a form and these would become fields. I'll see if I can improve it for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as a temporary fix, I can add some padding to make the baselines match. I really wanted to use the monospace font on only the field value to signify that it is the value of the configuration.

oidcConfig,
groupSyncSettings,
roleSyncSettings,
}) => {
const theme = useTheme();
const {
groups_field,
user_role_field,
group_regex_filter,
group_auto_create,
} = oidcConfig || {};
const { user_role_field } = oidcConfig || {};
return (
<>
<ChooseOne>
Expand All @@ -54,16 +59,22 @@ export const IdpSyncPageView: FC<IdpSyncPageViewProps> = ({ oidcConfig }) => {
<Stack direction={"row"} alignItems={"center"} spacing={8}>
<IdpField
name={"Sync Field"}
fieldText={groups_field}
fieldText={groupSyncSettings?.field}
showStatusIndicator
/>
<IdpField
name={"Regex Filter"}
fieldText={group_regex_filter}
fieldText={
typeof groupSyncSettings?.regex_filter === "string"
? groupSyncSettings?.regex_filter
: ""
}
/>
<IdpField
name={"Auto Create"}
fieldText={group_auto_create?.toString()}
fieldText={String(
groupSyncSettings?.auto_create_missing_groups,
)}
/>
</Stack>
</fieldset>
Expand Down
0