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: format
  • Loading branch information
@jaaydenh
jaaydenh committed Jan 22, 2025
commit 623a50e07760ba3ebe4d696cd201174aaa7e8e6c
25 changes: 16 additions & 9 deletions site/e2e/tests/deployment/idpOrgSync.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,20 @@ test.beforeEach(async ({ page }) => {
});

test.describe("IdpOrgSyncPage", () => {

test("show empty table when no org mappings are present", async ({ page }) => {
test("show empty table when no org mappings are present", async ({
page,
}) => {
requiresLicense();
await page.goto("/deployment/idp-org-sync", {
waitUntil: "domcontentloaded",
});

await expect(page.getByRole("row", { name: "idp-org-1" })).not.toBeVisible();
await expect(page.getByRole("heading", { name: "No organization mappings" })).toBeVisible();
await expect(
page.getByRole("row", { name: "idp-org-1" }),
).not.toBeVisible();
await expect(
page.getByRole("heading", { name: "No organization mappings" }),
).toBeVisible();
});

test("add new IdP organization mapping with API", async ({ page }) => {
Expand Down Expand Up @@ -60,10 +65,10 @@ test.describe("IdpOrgSyncPage", () => {

const row = page.getByTestId("idp-org-idp-org-1");
Copy link
Member

Choose a reason for hiding this comment

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

Do we need a test ID selector here? I'm not sure why the row selectors wouldn't work here, but would still work for other test cases

await expect(row.getByRole("cell", { name: "idp-org-1" })).toBeVisible();
await row
.getByRole("button", { name: /delete/i })
.click();
await expect(row.getByRole("cell", { name: "idp-org-1" })).not.toBeVisible();
await row.getByRole("button", { name: /delete/i }).click();
await expect(
row.getByRole("cell", { name: "idp-org-1" }),
).not.toBeVisible();
await expect(
page.getByText("Organization sync settings updated."),
).toBeVisible();
Expand Down Expand Up @@ -165,7 +170,9 @@ test.describe("IdpOrgSyncPage", () => {
// Verify new mapping appears in table
const newRow = page.getByTestId("idp-org-new-idp-org");
await expect(newRow).toBeVisible();
await expect(newRow.getByRole("cell", { name: "new-idp-org" })).toBeVisible();
await expect(
newRow.getByRole("cell", { name: "new-idp-org" }),
).toBeVisible();
await expect(newRow.getByRole("cell", { name: orgName })).toBeVisible();

await expect(
Expand Down
17 changes: 10 additions & 7 deletions site/e2e/tests/organizations/idpGroupSync.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,16 +16,21 @@ test.beforeEach(async ({ page }) => {
});

test.describe("IdpGroupSyncPage", () => {

test("show empty table when no group mappings are present", async ({ page }) => {
test("show empty table when no group mappings are present", async ({
page,
}) => {
requiresLicense();
const org = await createOrganizationWithName(randomName());
await page.goto(`/organizations/${org.name}/idp-sync?tab=groups`, {
waitUntil: "domcontentloaded",
});

await expect(page.getByRole("row", { name: "idp-group-1" })).not.toBeVisible();
await expect(page.getByRole("heading", { name: "No group mappings" })).toBeVisible();
await expect(
page.getByRole("row", { name: "idp-group-1" }),
).not.toBeVisible();
await expect(
page.getByRole("heading", { name: "No group mappings" }),
).toBeVisible();

await deleteOrganization(org.name);
Copy link
Member

Choose a reason for hiding this comment

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

I'm asking because I haven't had much chance to touch Playwright yet, and don't know what it does for test setup: is deleteOrganization needed for test isolation?

I'm mainly wondering why the call is present for the first two tests as (what looks like) a cleanup step, but some of the later, non-deletion tests in this file don't have it

Copy link
Member

Choose a reason for hiding this comment

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

I'm also not sure if Playwright supports any "true concurrency", rather than doing everything through the event loop

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 originally added this to fix some test failures across tests that were creating organizations. From the documentation and my testing, it does appear tests are run in parallel. The intention was to add this cleanup step anytime an org is created to avoid test flakes in the future.

});
Expand Down Expand Up @@ -67,9 +72,7 @@ test.describe("IdpGroupSyncPage", () => {

const row = page.getByTestId("group-idp-group-1");
await expect(row.getByRole("cell", { name: "idp-group-1" })).toBeVisible();
await row
.getByRole("button", { name: /delete/i })
.click();
await row.getByRole("button", { name: /delete/i }).click();
await expect(
row.getByRole("cell", { name: "idp-group-1" }),
).not.toBeVisible();
Expand Down
29 changes: 19 additions & 10 deletions site/e2e/tests/organizations/idpRoleSync.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,16 +16,21 @@ test.beforeEach(async ({ page }) => {
});

test.describe("IdpRoleSyncPage", () => {

test("show empty table when no role mappings are present", async ({ page }) => {
test("show empty table when no role mappings are present", async ({
page,
}) => {
requiresLicense();
const org = await createOrganizationWithName(randomName());
await page.goto(`/organizations/${org.name}/idp-sync?tab=roles`, {
waitUntil: "domcontentloaded",
});

await expect(page.getByRole("row", { name: "idp-role-1" })).not.toBeVisible();
await expect(page.getByRole("heading", { name: "No role mappings" })).toBeVisible();
await expect(
page.getByRole("row", { name: "idp-role-1" }),
).not.toBeVisible();
await expect(
page.getByRole("heading", { name: "No role mappings" }),
).toBeVisible();

await deleteOrganization(org.name);
});
Expand Down Expand Up @@ -62,10 +67,10 @@ test.describe("IdpRoleSyncPage", () => {
});
const row = page.getByTestId("role-idp-role-1");
await expect(row.getByRole("cell", { name: "idp-role-1" })).toBeVisible();
await row
.getByRole("button", { name: /delete/i })
.click();
await expect(row.getByRole("cell", { name: "idp-role-1" })).not.toBeVisible();
await row.getByRole("button", { name: /delete/i }).click();
await expect(
row.getByRole("cell", { name: "idp-role-1" }),
).not.toBeVisible();
await expect(
page.getByText("IdP Role sync settings updated."),
).toBeVisible();
Expand Down Expand Up @@ -146,8 +151,12 @@ test.describe("IdpRoleSyncPage", () => {
// Verify new mapping appears in table
const newRow = page.getByTestId("role-new-idp-role");
await expect(newRow).toBeVisible();
await expect(newRow.getByRole("cell", { name: "new-idp-role" })).toBeVisible();
await expect(newRow.getByRole("cell", { name: "organization-admin" })).toBeVisible();
await expect(
newRow.getByRole("cell", { name: "new-idp-role" }),
).toBeVisible();
await expect(
newRow.getByRole("cell", { name: "organization-admin" }),
).toBeVisible();

await expect(
page.getByText("IdP Role sync settings updated."),
Expand Down
4 changes: 3 additions & 1 deletion site/src/pages/DeploymentSettingsPage/IdpOrgSyncPage/IdpOrgSyncPageView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,9 @@ export const IdpOrgSyncPageView: FC<IdpSyncPageViewProps> = ({
<IdpMappingTable isEmpty={organizationMappingCount === 0}>
{form.values.mapping &&
Object.entries(form.values.mapping)
.sort(([a], [b]) => a.toLowerCase().localeCompare(b.toLowerCase()))
.sort(([a], [b]) =>
a.toLowerCase().localeCompare(b.toLowerCase()),
)
.map(([idpOrg, organizations]) => (
<OrganizationRow
key={idpOrg}
Expand Down
8 changes: 6 additions & 2 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 @@ -243,7 +243,9 @@ export const IdpGroupSyncForm = ({
<IdpMappingTable type="Group" rowCount={groupMappingCount}>
{groupSyncSettings?.mapping &&
Object.entries(groupSyncSettings.mapping)
.sort(([a], [b]) => a.toLowerCase().localeCompare(b.toLowerCase()))
.sort(([a], [b]) =>
a.toLowerCase().localeCompare(b.toLowerCase()),
)
.map(([idpGroup, groups]) => (
<GroupRow
key={idpGroup}
Expand All @@ -259,7 +261,9 @@ export const IdpGroupSyncForm = ({
<LegacyGroupSyncHeader />
<IdpMappingTable type="Group" rowCount={legacyGroupMappingCount}>
{Object.entries(groupSyncSettings.legacy_group_name_mapping)
.sort(([a], [b]) => a.toLowerCase().localeCompare(b.toLowerCase()))
.sort(([a], [b]) =>
a.toLowerCase().localeCompare(b.toLowerCase()),
)
.map(([idpGroup, groupId]) => (
<GroupRow
key={groupId}
Expand Down
4 changes: 3 additions & 1 deletion site/src/pages/ManagementSettingsPage/IdpSyncPage/IdpRoleSyncForm.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,9 @@ export const IdpRoleSyncForm = ({
<IdpMappingTable type="Role" rowCount={roleMappingCount}>
{roleSyncSettings?.mapping &&
Object.entries(roleSyncSettings.mapping)
.sort(([a], [b]) => a.toLowerCase().localeCompare(b.toLowerCase()))
.sort(([a], [b]) =>
a.toLowerCase().localeCompare(b.toLowerCase()),
)
.map(([idpRole, roles]) => (
<RoleRow
key={idpRole}
Expand Down
5 changes: 1 addition & 4 deletions site/src/pages/TemplatePage/TemplateLayout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,10 +108,7 @@ export const TemplateLayout: FC<PropsWithChildren> = ({
}}
/>

<Tabs
active={activeTab}
className="mb-10 -mt-3"
>
<Tabs active={activeTab} className="mb-10 -mt-3">
<Margins>
<TabsList>
<TabLink to="" value="summary">
Expand Down