feat: integrate backend with idp sync page #14755

jaaydenh · 2024-09-20T21:07:31Z

resolves #14424

Populate the groups field, roles field, group regex filter, auto create groups boolean with data
Populate groups and role sync settings mapping tables
Display group UUID in an error state when a mapped coder group doesn't exist
Use tabs to separate groups and roles UI
Display disabled UI state for group and role sync fields when they are empty
Check for correct experiment, license and permissions
Add storybook stories for data driven UI states
Handle 'export Policy' button download behavior for groups and roles

Parkreiner

A lot of this looks good, but if this gets pushed through as-is, it will blow up the Coder app at some point

Tried to leave as much feedback as I could, but I think the design/styling issues can be pushed back in favor of making sure the code problems are addressed

Parkreiner · 2024-09-23T15:14:26Z

site/src/api/queries/organizations.ts

@@ -141,6 +141,32 @@ export const provisionerDaemonGroups = (organization: string) => {
 	};
 };

+export const getGroupIdpSyncSettingsKey = (organization: string) => [
+	"organization",


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

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

I try to match the routes on the backend (which all do include the s)

@aslilac ok I will use plural here, but there are still several keys that do not match the routes, organizationMembersKey, getProvisionerDaemonGroupsKey,

Parkreiner · 2024-09-23T15:19:47Z

site/src/pages/DeploySettingsPage/AppearanceSettingsPage/AppearanceSettingsPageView.tsx

@@ -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."


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?

Nothing yet, so far this has purely been focused on the enterprise and premium badges and banners.

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

Parkreiner · 2024-09-23T15:20:17Z

site/src/pages/ManagementSettingsPage/CustomRolesPage/CustomRolesPageView.tsx

@@ -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 {


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

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.

I like interface because I think interface WibbleProps extends WobbleProps {} is a lot clearer than type WibbleProps = WobbleProps & {} in the places where we need it. and I think we just generally use it in more places rn.

Parkreiner · 2024-09-23T15:22:26Z

site/src/testHelpers/entities.ts

+		],
+		"idp-group-2": ["fbd2116a-8961-4954-87ae-e4575bd29ce0"],
+	},
+	regex_filter: "@[a-zA-Z0-9_]+",


I know it's mock data, but just in case it has any bearing on regex code, this regex can be simplified as @\w+. The word class includes digits and underscores by default

the regex really can be anything, I did want to see a regex with a bit longer length.

Parkreiner · 2024-09-23T15:28:37Z

site/src/pages/ManagementSettingsPage/IdpSyncPage/ExportPolicyButton.tsx

+	policy,
+	type,
+	organization,
+	download = saveAs,


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 saveAs function. Every consumer would have to import saveAs themselves, and then wire up the functionality for component implementation

How often would we have a use case where we'd want to get rid of the saveAs functionality vs wanting to always have saveAs, and then potentially add more functionality on top?

This was meant to just support the Storybook test case. Is there a better way to handle this?

Parkreiner · 2024-09-23T16:16:18Z

site/src/pages/ManagementSettingsPage/IdpSyncPage/IdpSyncPageView.tsx

+	const rolePolicy =
+		roleSyncSettings?.field && roleSyncSettings.mapping
+			? JSON.stringify(roleSyncSettings, null, 2)
+			: null;
+	const groupPolicy =
+		groupSyncSettings?.field && groupSyncSettings.mapping
+			? JSON.stringify(groupSyncSettings, null, 2)
+			: null;


This is dangerous – we need to be super, super careful with any JSON.stringify calls inside components:

Stringifying on every render can be slow

JSON.stringify can throw, so if it fails, we might blow up the entire app

site/src/pages/ManagementSettingsPage/IdpSyncPage/IdpSyncPageView.tsx

Parkreiner · 2024-09-23T16:34:33Z

site/src/pages/ManagementSettingsPage/IdpSyncPage/IdpSyncPageView.tsx

+	organization: Organization;
+}
+
+export const IdpSyncPageView: FC<IdpSyncPageViewProps> = ({


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:

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

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.

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.

Parkreiner · 2024-09-23T16:38:15Z

site/src/pages/ManagementSettingsPage/IdpSyncPage/IdpSyncPageView.tsx

+			{fieldText ? (
+				<p css={styles.fieldText}>{fieldText}</p>
+			) : (
+				showStatusIndicator && (


Is this the right name for the prop if it's only being used to show disabled statuses?

I thought it may be used for more in the future but Ill rename it for now.

aslilac

looks good, just a few small things!

aslilac · 2024-09-23T18:17:56Z

site/src/api/api.ts

+	/**
+	 * @param organization Can be the organization's ID or name
+	 */


can you add this above the getGroup... function too pls?

aslilac · 2024-09-23T18:18:41Z

site/src/api/queries/organizations.ts

@@ -141,6 +141,32 @@ export const provisionerDaemonGroups = (organization: string) => {
 	};
 };

+export const getGroupIdpSyncSettingsKey = (organization: string) => [
+	"organization",


I try to match the routes on the backend (which all do include the s)

aslilac · 2024-09-23T18:19:26Z

site/src/pages/DeploySettingsPage/AppearanceSettingsPage/AppearanceSettingsPageView.tsx

@@ -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."


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

aslilac · 2024-09-23T18:21:10Z

site/src/pages/ManagementSettingsPage/CustomRolesPage/CustomRolesPageView.tsx

@@ -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 {


I like interface because I think interface WibbleProps extends WobbleProps {} is a lot clearer than type WibbleProps = WobbleProps & {} in the places where we need it. and I think we just generally use it in more places rn.

aslilac · 2024-09-23T18:22:22Z

site/src/pages/ManagementSettingsPage/IdpSyncPage/ExportPolicyButton.stories.tsx

+				`${MockOrganization.name}_groups-policy.json`,
+			),
+		);
+		const blob: Blob = (args.download as jest.Mock).mock.calls[0][0];


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!

site/src/pages/ManagementSettingsPage/IdpSyncPage/IdpPillList.tsx

Parkreiner

Not sure if you ended up pushing up the lastCalled change (it's not showing up in GitHub), but the changes look good overall!

I'll make a separate issue about the accessibility issues for PopoverTrigger

jaaydenh self-assigned this Sep 20, 2024

jaaydenh added 3 commits September 20, 2024 22:29

feat: idp sync initial commit

a40ba7d

fix: hookup backend data for groups and roles

7ff9a07

chore: cleanup

6e303f4

jaaydenh force-pushed the jaaydenh/idp-sync branch from ddc86ba to 6e303f4 Compare September 20, 2024 22:29

jaaydenh added 5 commits September 21, 2024 19:24

feat: separate groups and roles into tabs

310726b

feat: implement export policy button

615c8d3

feat: handle missing groups

45f4d13

chore: add story for missing groups

080b032

chore: add stories for export policy button

8e1e021

jaaydenh marked this pull request as ready for review September 23, 2024 14:54

jaaydenh requested review from Parkreiner and aslilac September 23, 2024 15:03

Parkreiner requested changes Sep 23, 2024

View reviewed changes

aslilac approved these changes Sep 23, 2024

View reviewed changes

fix: updates for PR review

c863619

Parkreiner approved these changes Sep 24, 2024

View reviewed changes

Parkreiner mentioned this pull request Sep 24, 2024

Site: PopoverTrigger is not keyboard-friendly for hover variant #14776

Open

jaaydenh added 4 commits September 24, 2024 00:39

chore: update tests

41631f0

chore: document uuid regex

3db9f48

chore: remove unused

431beea

fix: fix stories

f166784

jaaydenh merged commit a3ebcd7 into main Sep 24, 2024
27 checks passed

jaaydenh deleted the jaaydenh/idp-sync branch September 24, 2024 02:07

github-actions bot locked and limited conversation to collaborators Sep 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: integrate backend with idp sync page #14755

feat: integrate backend with idp sync page #14755

jaaydenh commented Sep 20, 2024 •

edited

Loading

Parkreiner left a comment •

edited

Loading

Parkreiner Sep 23, 2024

jaaydenh Sep 23, 2024

aslilac Sep 23, 2024

jaaydenh Sep 23, 2024

Parkreiner Sep 23, 2024

jaaydenh Sep 23, 2024

aslilac Sep 23, 2024

Parkreiner Sep 23, 2024

jaaydenh Sep 23, 2024

aslilac Sep 23, 2024

Parkreiner Sep 23, 2024

jaaydenh Sep 23, 2024

Parkreiner Sep 23, 2024

jaaydenh Sep 23, 2024

Parkreiner Sep 23, 2024

Parkreiner Sep 23, 2024

Parkreiner Sep 23, 2024

jaaydenh Sep 23, 2024

jaaydenh Sep 23, 2024

Parkreiner Sep 23, 2024

jaaydenh Sep 23, 2024 •

edited

Loading

aslilac left a comment

aslilac Sep 23, 2024

aslilac Sep 23, 2024

aslilac Sep 23, 2024

aslilac Sep 23, 2024

aslilac Sep 23, 2024

Parkreiner left a comment

feat: integrate backend with idp sync page #14755

feat: integrate backend with idp sync page #14755

Conversation

jaaydenh commented Sep 20, 2024 • edited Loading

Parkreiner left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaaydenh Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

aslilac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Parkreiner left a comment

Choose a reason for hiding this comment

jaaydenh commented Sep 20, 2024 •

edited

Loading

Parkreiner left a comment •

edited

Loading

jaaydenh Sep 23, 2024 •

edited

Loading