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.

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

Conversation

jaaydenh
Copy link
Contributor

@jaaydenh jaaydenh commented Jan 12, 2025

contributes to #15290

The goal of this PR is to port the work to implement CRUD in the UI for IDP organization sync settings and apply this to group and role IDP sync settings.

Screenshot 2025-01-16 at 20 25 21 Screenshot 2025-01-16 at 20 25 39

@jaaydenh jaaydenh self-assigned this Jan 12, 2025
@jaaydenh jaaydenh force-pushed the jaaydenh/idp-group-role-sync-crud branch from 64739b1 to d1a5f4d Compare January 15, 2025 23:27
@jaaydenh jaaydenh marked this pull request as ready for review January 16, 2025 20:20
@jaaydenh jaaydenh requested a review from Parkreiner January 16, 2025 20:20
@jaaydenh jaaydenh changed the title feat: add ability edit IDP sync configuration for groups and roles in the UI feat: add ability to edit IDP sync configuration for groups and roles in the UI Jan 16, 2025
@jaaydenh jaaydenh changed the title feat: add ability to edit IDP sync configuration for groups and roles in the UI feat: enable editing of IDP sync configuration for groups and roles in the UI Jan 16, 2025
Copy link
Member

@Parkreiner Parkreiner left a comment

Choose a reason for hiding this comment

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

I'm flagging this as a "Request Changes", but there's not a bunch of major issues here, as far as I can see
Mainly a few small things that, in the worst case scenario, could break browser behavior

@jaaydenh jaaydenh force-pushed the jaaydenh/idp-group-role-sync-crud branch from 4dabb5f to 52f34fd Compare January 20, 2025 22:35
@jaaydenh jaaydenh force-pushed the jaaydenh/idp-group-role-sync-crud branch from 151012b to 79ce400 Compare January 22, 2025 18:43
@jaaydenh jaaydenh requested a review from Parkreiner January 22, 2025 18:47
Copy link
Member

@Parkreiner Parkreiner left a comment

Choose a reason for hiding this comment

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

Approving to make sure you're not blocked, but still have some feedback/questions. The big question I still have is about using the String constructor with Yup

.first()
.click();
await expect(page.getByText("idp-org-1")).not.toBeVisible();
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

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.

Comment on lines 433 to 437
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

<div className="grid items-center gap-1">
&nbsp;
<div className="grid grid-rows-[28px_auto]">
<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

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.

/>
</div>
<div className="grid grid-rows-[28px_auto]">
<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

regex_filter: Yup.string().trim(),
auto_create_missing_groups: Yup.boolean(),
mapping: Yup.object().shape({
[`${String}`]: Yup.array().of(Yup.string()),
Copy link
Member

Choose a reason for hiding this comment

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

@jaaydenh Pinging you again about this question thread

Comment on lines 159 to 163
<Spinner size="sm" loading={form.isSubmitting} className="w-9">
<Switch
id={AUTO_CREATE_MISSING_GROUPS_ID}
checked={form.values.auto_create_missing_groups}
onCheckedChange={async (checked) => {
void form.setFieldValue("auto_create_missing_groups", checked);
form.handleSubmit();
}}
/>
</Spinner>
Copy link
Member

Choose a reason for hiding this comment

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

So to be honest, at first blush, it seemed to me less like an issue with the Spinner component itself, and more so how it was being applied here in the code

But now that I'm looking at the code, I do think there's room to beef it up so that you don't have to think about these edge cases but also remove some animation flicker on modal transitions. I'll see if I can squeeze those in sometime tomorrow

To be clear, the current code is still breaking browser behavior, and this was the main reason why I pushed back on approving at first, but I'm okay with treating this as a known shippable

@jaaydenh jaaydenh merged commit f518669 into main Jan 27, 2025
32 checks passed
@jaaydenh jaaydenh deleted the jaaydenh/idp-group-role-sync-crud branch January 27, 2025 18:31
@github-actions github-actions bot locked and limited conversation to collaborators Jan 27, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants