quiltdata / quilt

Quilt is a data mesh for connecting people with actionable data
https://quiltdata.com
Apache License 2.0
1.32k stars 91 forks source link

Multiple roles per user (management + switching, incl. GraphQL & TS) #3982

Closed nl0 closed 2 months ago

nl0 commented 3 months ago

The primary change is adding the support of assigning multiple roles to users via admin UI and allowing users to switch between roles assigned to them (powered by the new GQL APIs):

Screenshot 2024-06-19 at 08 20 42 Screenshot 2024-06-19 at 08 21 04 Screenshot 2024-06-19 at 08 21 22 Screenshot 2024-06-19 at 08 21 41 Screenshot 2024-06-19 at 08 21 57

Secondary changes:

TODO

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 0.12210% with 818 lines in your changes missing coverage. Please review.

Project coverage is 37.19%. Comparing base (1b8d91d) to head (10d4302).

Files Patch % Lines
...talog/app/containers/Admin/UsersAndRoles/Users.tsx 0.00% 373 Missing :warning:
catalog/app/containers/NavBar/NavMenu.tsx 0.00% 190 Missing :warning:
.../app/containers/Admin/UsersAndRoles/RoleSelect.tsx 0.00% 66 Missing :warning:
catalog/app/containers/NavBar/RoleSwitcher.tsx 0.00% 49 Missing :warning:
catalog/app/utils/GraphQL/Provider.tsx 5.88% 16 Missing :warning:
catalog/app/containers/NavBar/NavBar.tsx 0.00% 15 Missing :warning:
catalog/app/utils/GlobalDialogs.tsx 0.00% 15 Missing :warning:
...talog/app/containers/Admin/UsersAndRoles/index.tsx 0.00% 9 Missing :warning:
catalog/app/containers/Admin/Form.tsx 0.00% 8 Missing :warning:
...og/app/containers/Admin/UsersAndRoles/Policies.tsx 0.00% 6 Missing :warning:
... and 30 more
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #3982 +/- ## ========================================== - Coverage 37.58% 37.19% -0.39% ========================================== Files 746 757 +11 Lines 32775 33114 +339 Branches 4769 4847 +78 ========================================== + Hits 12317 12318 +1 - Misses 19795 20154 +359 + Partials 663 642 -21 ``` | [Flag](https://app.codecov.io/gh/quiltdata/quilt/pull/3982/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quiltdata) | Coverage Δ | | |---|---|---| | [api-python](https://app.codecov.io/gh/quiltdata/quilt/pull/3982/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quiltdata) | `90.75% <ø> (ø)` | | | [catalog](https://app.codecov.io/gh/quiltdata/quilt/pull/3982/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quiltdata) | `10.55% <0.12%> (-0.17%)` | :arrow_down: | | [lambda](https://app.codecov.io/gh/quiltdata/quilt/pull/3982/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quiltdata) | `88.18% <ø> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quiltdata#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

nl0 commented 2 months ago

@fiskus i think this is ready for review, pls have a look when you get some time. we still need to update the gql schema once it's finalized on the backend and regenerate all the dependent code, but most likely it won't affect anything in this PR

nl0 commented 2 months ago

@fiskus thanks for the feedback

I think list divider looks nicer with small margin like in default MUI List view:

well, this is default list view, i didn't tweak the padding (maybe it's because of using <ListItem divider ... /> vs <Divider /> 🤷 )

radio buttons and add a submit button

yeah i was thinking about the explicit confirmation, but then i guess i just forgot about it. i don't mind adding the button if that's the consensus, tho i don't see the current ux as problematic either. @akarve wdyt?

Also, small padding at the bottom would be nic

i'll consider this

Hint "(you)" is really cool small helpful thing.

👍

nl0 commented 2 months ago

@fiskus i've added a submit button to role switching dialog, some spacing around menu separators, and incorporated your change from #3704 -- pls have a look. i'll deploy this to the dev stack shortly.

fiskus commented 2 months ago

I noticed, that "Users" table now looks prettier. Though, I wasn't sure. Then I compared to master: yes, it looks much nicer. I think, you can also remove "Actions" title. Then "+" and "Delete" buttons become confront and consistent to each other at the same time. Also, it gives slightly more space to fit into a small viewport.

I would re-use the same dialog for adding and editing role in favor to have fewer code. So, click on row can open edit dialog. But, clicking on individual items makes more sense.

nl0 commented 2 months ago

Maybe, tooltip "Switch role to %role name%" could help.

actually there is a tooltip when you hover over a radio button (tho it's easy to miss, i agree)

nl0 commented 2 months ago

rename it to "Assigned"/"All". As I understand, "available" - all roles available for the stack,

well, "available" != "all" ("available" = "all" - "assigned")

and "assigned" - available for the user to switch.

correct

nl0 commented 2 months ago

I noticed, that "Users" table now looks prettier. Though, I wasn't sure. Then I compared to master: yes, it looks much nicer.

yeah, i tried my best to polish it, thanks

I think, you can also remove "Actions" title. Then "+" and "Delete" buttons become confront and consistent to each other at the same time. Also, it gives slightly more space to fit into a small viewport.

good idea, i'll try that

I would re-use the same dialog for adding and editing role in favor to have fewer code. So, click on row can open edit dialog. But, clicking on individual items makes more sense.

i don't quite get that. you mean "invite" vs "assign roles" dialogs?

fiskus commented 2 months ago

i don't quite get that. you mean "invite" vs "assign roles" dialogs?

Yes "invite", sorry. "Invite user" has "username" + "email" + "assign role" fields, and "Edit user" could have "email" + "assign role" fields. So, "Invite/Edit user" dialog could be:

{isNew ? "Username" : null}
{"Email"}
{"Assign roles"}
nl0 commented 2 months ago

I think, you can also remove "Actions" title. Then "+" and "Delete" buttons become confront and consistent to each other at the same time. Also, it gives slightly more space to fit into a small viewport.

good idea, i'll try that

it looks ok

nl0 commented 2 months ago

i don't quite get that. you mean "invite" vs "assign roles" dialogs?

Yes "invite", sorry. "Invite user" has "username" + "email" + "assign role" fields, and "Edit user" could have "email" + "assign role" fields. So, "Invite/Edit user" dialog could be:

{isNew ? "Username" : null}
{"Email"}
{"Assign roles"}

well, yeah, i thought about unifying all the user editing functionality under a single screen / dialog, but i decided to keep things as they were (focused "atomic" editing interfaces). maybe next time we revisit this ui we'll get to unifying those dialogs.