-
Notifications
You must be signed in to change notification settings - Fork 874
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
Conversation
64739b1
to
d1a5f4d
Compare
There was a problem hiding this 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
site/src/pages/ManagementSettingsPage/IdpSyncPage/IdpGroupSyncForm.tsx
Outdated
Show resolved
Hide resolved
site/src/pages/ManagementSettingsPage/IdpSyncPage/IdpGroupSyncForm.tsx
Outdated
Show resolved
Hide resolved
site/src/pages/ManagementSettingsPage/IdpSyncPage/IdpGroupSyncForm.tsx
Outdated
Show resolved
Hide resolved
site/src/pages/ManagementSettingsPage/IdpSyncPage/IdpMappingTable.tsx
Outdated
Show resolved
Hide resolved
site/src/pages/ManagementSettingsPage/IdpSyncPage/IdpRoleSyncForm.tsx
Outdated
Show resolved
Hide resolved
4dabb5f
to
52f34fd
Compare
site/src/pages/ManagementSettingsPage/IdpSyncPage/IdpGroupSyncForm.tsx
Outdated
Show resolved
Hide resolved
151012b
to
79ce400
Compare
There was a problem hiding this 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"); |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
useEffect(() => { | ||
if (inputRef.current && inputProps?.id) { | ||
inputRef.current.id = inputProps?.id; | ||
} | ||
}, [inputProps?.id]); |
There was a problem hiding this comment.
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:
- Input mounts with ID value 1
- Re-render happens with ID value 2
- Effect fires after repaint, and the underlying DOM element is mutated
- 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
There was a problem hiding this comment.
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"> | ||
| ||
<div className="grid grid-rows-[28px_auto]"> | ||
<div /> |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
site/src/pages/ManagementSettingsPage/IdpSyncPage/IdpGroupSyncForm.tsx
Outdated
Show resolved
Hide resolved
site/src/pages/ManagementSettingsPage/IdpSyncPage/IdpGroupSyncForm.tsx
Outdated
Show resolved
Hide resolved
/> | ||
</div> | ||
<div className="grid grid-rows-[28px_auto]"> | ||
<div /> |
There was a problem hiding this comment.
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()), |
There was a problem hiding this comment.
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
<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> |
There was a problem hiding this comment.
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
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.