Skip to content
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

Filter user management groups by the organization of the admin #47

Merged
merged 3 commits into from
Dec 18, 2020

Conversation

yasmin2496
Copy link

@yasmin2496 yasmin2496 commented Dec 1, 2020

Purpose

To show a filtered view of users by admin's organization details. For global admin, the user's organization name is to be shown also

Further info

1. Global Admin - See all users with their organization name besides
2. Admin - See users filtered by organization

Ticket number

14

Issue Link

buildlyio#14

@yasmin2496 yasmin2496 added the Story/ Sub Feature Description of one or more features label Dec 1, 2020
@yasmin2496 yasmin2496 self-assigned this Dec 1, 2020
@yasmin2496
Copy link
Author

image
Attaching screenshot for reference

@andrewshortall
Copy link

andrewshortall commented Dec 8, 2020

Just a few things I noticed. I’m not sure who can answer these questions. @jefmoura @glind

Firstly, how do we define what an org admin is? Is it really as simple as checking for the string ‘admin’? Or would it make more sense to use a certain combination of permissions?

I don't think this question makes sense but I'm going to ask anyway to clarify:

Is it possible to be part of multiple core groups that are attached to different organizations without being a global admin? Or are all of your core groups always part of your organization?

For example,
There are 3 organizations: X, Y and Z.
There is user who is part of:
X Admins, X Users, Y Users
but not a part of:
Y Admins, Z Admins, Z Users

Do we allow this?

@andrewshortall
Copy link

@yasmin2496 Let's ignore my second question for now. I'll discuss this with Greg and Jeferson later.
However, for the function to check if a user is an admin, I think it makes more sense to base this on the permissions and not on whether or not the string admin is included in the title.

The core groups can be called anything and don't necessarily need to have admin in the title. Likewise, if a core group has no permissions but for some reason has admin in the title, they shouldn't be classified as an admin. I guess a typical admin has all permissions [read, write, update, delete] set as true.

@yasmin2496
Copy link
Author

@andrewshortall I will update the admin identification using permissions check [read, write, update, delete]

@RadhikaPPatel
Copy link

@andrewshortall I have a small doubt here. Both Org and Global admin have the same permission level (read, write, update, delete). How can we then distinguish which is which?

@andrewshortall
Copy link

andrewshortall commented Dec 14, 2020

@RadhikaPPatel there's a flag is_global on the group, right?

In my opinion it doesn't make sense to base the permission on a string. Someone with the same permissions as an admin group should have the same access. The is_global flag differentiates a global admin from an org admin. But I understand your point. The UI probably needs to make it clear which group is actually global.

@glind
Copy link

glind commented Dec 14, 2020

Just a few things I noticed. I’m not sure who can answer these questions. @jefmoura @glind

Firstly, how do we define what an org admin is? Is it really as simple as checking for the string ‘admin’? Or would it make more sense to use a certain combination of permissions?

I don't think this question makes sense but I'm going to ask anyway to clarify:

Is it possible to be part of multiple core groups that are attached to different organizations without being a global admin? Or are all of your core groups always part of your organization?

For example,
There are 3 organizations: X, Y and Z.
There is user who is part of:
X Admins, X Users, Y Users
but not a part of:
Y Admins, Z Admins, Z Users

Do we allow this?

As of right now a user is a member of only one organization. Data can be shared between organizations but users can not.

@glind
Copy link

glind commented Dec 14, 2020

I think for this particular check it only affects the global admin as they are the only ones who will be able to see more then one organization at a time in a multi-tenant deployment of buildly. The definition of an admin is a good point though. Is it solely defined by the permissions or can an editor have the same permissions but not be an "admin" in which case the label is what matters.. or is it both. Label + Permissions = "admin" ??

@andrewshortall
Copy link

In my opinion, it's not clear that someone needs to include the label 'admin' in the title if they want that user group to have these permissions. Since we allow custom naming, we shouldn't be forcing a label. If we need to differentiate between someone who has all permissions and someone who is an admin, we should add another flag on the backend for this. Otherwise, what happens if you accidentally rename the Admins group? All org admins will lose access to the user management.

@andrewshortall
Copy link

andrewshortall commented Dec 16, 2020

@yasmin2496 Greg, Karrla and I discussed the issue and we decided that the best solution for now is to use the permissions to determine the admin access (but the label should not matter).

@yasmin2496
Copy link
Author

@andrewshortall Okay got it. I will implement the change as we have discussed with permissions [read, write, update, delete] for identifying Admin in the system. To differentiate between Organization Admin and Global Admin, will use is_global flag in the core groups.

@andrewshortall andrewshortall merged commit 80fad40 into master Dec 18, 2020
@RadhikaPPatel RadhikaPPatel deleted the feature/user_management_organization branch March 11, 2022 07:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Story/ Sub Feature Description of one or more features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants