rill icon indicating copy to clipboard operation
rill copied to clipboard

Share Project Popover

Open lovincyrus opened this issue 1 year ago • 11 comments

closes #5815

https://www.figma.com/design/Qt6EyotCBS3V6O31jVhMQ7/RILL-Latest?node-id=6801-232076&node-type=canvas&t=RbLXuShzLAEx5sVX-0

  • [x] revisit list item name change
  • [x] pending photo url in the avatar https://github.com/rilldata/rill/issues/5889 https://github.com/rilldata/rill/pull/5888
  • [x] "You should be able to modify the roles for all-users – if not, let me know and we’ll fix it"
  • [x] "Everyone at Rill Data"
  • [x] add info icon for project perm manage members https://github.com/rilldata/rill/pull/5887#issuecomment-2415065713
  • [ ] https://github.com/rilldata/rill/pull/5887#discussion_r1799955918

CleanShot 2024-10-11 at 12 02 19@2x

lovincyrus avatar Oct 11 '24 18:10 lovincyrus

  • Project Viewers should not be allowed to change a Project Member's role. We should draw from the manage_project_members project permission. image

Only users that can manageProjectMembers will see the user invite button, see: https://github.com/rilldata/rill/blob/main/web-admin/src/features/navigation/TopNavigationBar.svelte#L180-L182

lovincyrus avatar Oct 15 '24 16:10 lovincyrus

UXQA:

  • [x] Project Viewers should not be allowed to change a Project Member's role. We should draw from the manage_project_members project permission. image
  • [ ] I don't see the ability to set the default role for "epg" Organization Members. The ability to set this role should draw from the manage_org_members organization permission.
  • [ ] Should we hide the default "all-users" group from this UI? It represents all users in the organization, which is duplicative to the "Organization" section/setting. (CC @ericokuma, @jkhwu)
  • [x] Should I be able to downgrade myself from "Admin" to "Viewer"? (CC @ericokuma, @jkhwu)
  • [x] I'm seeing a cursor-pointer when I hover over the Organization and Group list items, yet nothing happens when I click.
  • [x] I'm not seeing the text "Everyone from" [org] or "Everyone from" [group]. This could relate to this warning in my browser console: image
  1. Yes, we should hide the all-users group
  2. Hmm, currently in other systems, you can downgrade yourself. You can also downgrade your role via CLI. So you should be able to downgrade yourself via UI?

ericokuma avatar Oct 15 '24 17:10 ericokuma

  • I don't see the ability to set the default role for "epg" Organization Members. The ability to set this role should draw from the manage_org_members organization permission.

I didn't implement this right off the bat because I wanted to keep this PR contained to project user management changeset. If we choose to include this org user role change from this project popover, we would have to individually set the role for each org user using createAdminServiceSetOrganizationMemberUserRole

{
  organization: string;
  email: string;
  data: {
    role?: string;
  };
}

and if we have 300 users in this project, we'd have to fire the request 300 times.

lovincyrus avatar Oct 15 '24 19:10 lovincyrus

Only users that can manageProjectMembers will see the user invite button, see: https://github.com/rilldata/rill/blob/main/web-admin/src/features/navigation/TopNavigationBar.svelte#L180-L182

Oh, I see what happened. My user was an Org Admin, so I got the manage_project_members permission by default.

So this is a case where I'd expect to see the exclamation mark w/ tooltip to indicate that the Project Viewer role is overridden/overpowered by the Organization Admin role.

image

ericpgreen2 avatar Oct 15 '24 20:10 ericpgreen2

  • I don't see the ability to set the default role for "epg" Organization Members. The ability to set this role should draw from the manage_org_members organization permission.

I didn't implement this right off the bat because I wanted to keep this PR contained to project user management changeset. If we choose to include this org user role change from this project popover, we would have to individually set the role for each org user using createAdminServiceSetOrganizationMemberUserRole

{
  organization: string;
  email: string;
  data: {
    role?: string;
  };
}

and if we have 300 users in this project, we'd have to fire the request 300 times.

Oh I see. I agree that e.g. 300 individual requests would not be the way forward. We shouldn't be setting user_orgs_roles from this Project popover.

The designs seem to imply that we have an organizations_projects_roles table. We don't, but we do have a usergroups_projects_roles table. I could imagine an automatic <organization>-users usergroup for every organization, or I could imagine a dedicated organizations_projects_roles table. @begelundmuller @ericokuma, thoughts on this?

ericpgreen2 avatar Oct 15 '24 21:10 ericpgreen2

Hmm, currently in other systems, you can downgrade yourself. You can also downgrade your role via CLI. So you should be able to downgrade yourself via UI?

@ericokuma, the "Share" button is only available to Project Admins, right? So if I use the popover to downgrade myself from Admin to Viewer, I'll be ejected from the popover I think. Open to anything, but thoughts on this?

ericpgreen2 avatar Oct 15 '24 21:10 ericpgreen2

  • I don't see the ability to set the default role for "epg" Organization Members. The ability to set this role should draw from the manage_org_members organization permission.

I didn't implement this right off the bat because I wanted to keep this PR contained to project user management changeset. If we choose to include this org user role change from this project popover, we would have to individually set the role for each org user using createAdminServiceSetOrganizationMemberUserRole

{
  organization: string;
  email: string;
  data: {
    role?: string;
  };
}

and if we have 300 users in this project, we'd have to fire the request 300 times.

Oh I see. I agree that e.g. 300 individual requests would not be the way forward. We shouldn't be setting user_orgs_roles from this Project popover.

The designs seem to imply that we have an organizations_projects_roles table. We don't, but we do have a usergroups_projects_roles table. I could imagine an automatic <organization>-users usergroup for every organization, or I could imagine a dedicated organizations_projects_roles table. @begelundmuller @ericokuma, thoughts on this?

Ah, the designs were just assuming that all users who have access to an org will have access to projects. I think have a dedicated organizations_projects_roles table might be nice although wouldn't the all-users user group suffice?

ericokuma avatar Oct 15 '24 21:10 ericokuma

Hmm, currently in other systems, you can downgrade yourself. You can also downgrade your role via CLI. So you should be able to downgrade yourself via UI?

@ericokuma, the "Share" button is only available to Project Admins, right? So if I use the popover to downgrade myself from Admin to Viewer, I'll be ejected from the popover I think. Open to anything, but thoughts on this?

Ah yeah that's true. I guess to safeguard, I'm fine blocking that for now!

ericokuma avatar Oct 15 '24 21:10 ericokuma

  • I don't see the ability to set the default role for "epg" Organization Members. The ability to set this role should draw from the manage_org_members organization permission.

I didn't implement this right off the bat because I wanted to keep this PR contained to project user management changeset. If we choose to include this org user role change from this project popover, we would have to individually set the role for each org user using createAdminServiceSetOrganizationMemberUserRole

{
  organization: string;
  email: string;
  data: {
    role?: string;
  };
}

and if we have 300 users in this project, we'd have to fire the request 300 times.

Oh I see. I agree that e.g. 300 individual requests would not be the way forward. We shouldn't be setting user_orgs_roles from this Project popover. The designs seem to imply that we have an organizations_projects_roles table. We don't, but we do have a usergroups_projects_roles table. I could imagine an automatic <organization>-users usergroup for every organization, or I could imagine a dedicated organizations_projects_roles table. @begelundmuller @ericokuma, thoughts on this?

Ah, the designs were just assuming that all users who have access to an org will have access to projects. I think have a dedicated organizations_projects_roles table might be nice although wouldn't the all-users user group suffice?

Oh yeah, the all-users group is exactly the automatic <organization>-users usergroup I was thinking of! The "Viewer" toggle in the design implies that you can change Organization Members' default Project role from "Viewer" to "Admin", if you'd like. So to implement this @lovincyrus, the toggle should change the "all-users" Project role to "Admin".

image

ericpgreen2 avatar Oct 15 '24 21:10 ericpgreen2

  • I don't see the ability to set the default role for "epg" Organization Members. The ability to set this role should draw from the manage_org_members organization permission.

I didn't implement this right off the bat because I wanted to keep this PR contained to project user management changeset. If we choose to include this org user role change from this project popover, we would have to individually set the role for each org user using createAdminServiceSetOrganizationMemberUserRole

{
  organization: string;
  email: string;
  data: {
    role?: string;
  };
}

and if we have 300 users in this project, we'd have to fire the request 300 times.

Oh I see. I agree that e.g. 300 individual requests would not be the way forward. We shouldn't be setting user_orgs_roles from this Project popover. The designs seem to imply that we have an organizations_projects_roles table. We don't, but we do have a usergroups_projects_roles table. I could imagine an automatic <organization>-users usergroup for every organization, or I could imagine a dedicated organizations_projects_roles table. @begelundmuller @ericokuma, thoughts on this?

Ah, the designs were just assuming that all users who have access to an org will have access to projects. I think have a dedicated organizations_projects_roles table might be nice although wouldn't the all-users user group suffice?

Oh yeah, the all-users group is exactly the automatic <organization>-users usergroup I was thinking of! The "Viewer" toggle in the design implies that you can change Organization Members' default Project role from "Viewer" to "Admin", if you'd like. So to implement this @lovincyrus, the toggle should change the "all-users" Project role to "Admin".

image

~~all-users is a special usergroup that cannot be modified. By default, there is no role for all-users.~~

~~See screenshot~~ CleanShot 2024-10-15 at 15 19 40@2x

Update: all-users at the project level is modifiable

lovincyrus avatar Oct 15 '24 22:10 lovincyrus

Yikes...so probably for now, we can omit the role for the org-project relationship and just inherit the org-user-role to determine a user's role for a project?

cc @jkhwu

ericokuma avatar Oct 15 '24 23:10 ericokuma