refactor: roles - make better plan assumptions

[nunogois]

https://linear.app/unleash/issue/2-1171/refactor-custom-root-roles-with-correct-plan-assumptions

This cleans up the hotfix RoleSelect2 component and makes RoleSelect take in a roles prop from the parent component.

This also simplifies the role hooks again to assume Enterprise plan by default. This means, however, that we must ensure that we only call these hooks in Enterprise features or, if we do call them in other plans, that we provide a graceful fallback for non-Enterprise. Non-Enterprise instances do not have this endpoint, and so they are currently grabbing role information from e.g. useUsers and useServiceAccounts.

I'm not sure how I feel about this. Roles are an overarching concept of Unleash. To me, having to be extremely conscious about the exact scenario in which you're using such a hook feels like a trap, instead of "I need roles, so I'll grab the useRoles hook and not think much about it". I also don't like the way roles are currently tied to the users, service accounts, project access, (...) instead of being its own thing.

This could be solved by a RoleController exposing the GET endpoints in OSS, since all of the logic we need for this use-case lives there anyways. This would then be overridden with the Enterprise-specific controller when wrapped. This way we could assume the endpoint is always there, no matter the plan.
This is just an idea and not something I explored in the PR. For now I'm just focusing on leaving this feature in a sane state.

Tested this manually on Pro and Enterprise and I believe everything is acting the way we intend, but would love some extra eyes.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
unleash-monorepo-frontend	✅ Ready (Inspect)	Visit Preview	Jun 28, 2023 1:05pm

1 Ignored Deployment

Name	Status	Preview	Updated (UTC)
unleash-docs	⬜️ Ignored (Inspect)		Jun 28, 2023 1:05pm

[gastonfournier]

I feel the code is much cleaner, but let me also give you some answers to your questions:

I also don't like the way roles are currently tied to the users, service accounts, project access, (...) instead of being its own thing.

I believe this comes from a misuse of the API. Users API is built to provide the list of users, so to avoid repeating the role information on every user, I believe we split that into 2 lists: a list of users, each one with its role id, and a list of roles. Then the UI would use that information to paint the users' table.

Why not 2 calls? One to fetch users with role ids and another one to fetch the roles? Not sure, probably it was a simplification when this part was developed to optimize and avoid making 2 network calls. From a REST point of view, I would prefer making 2 calls, basically 👇

This could be solved by a RoleController exposing the GET endpoints in OSS

I think that roles should not be exposed from useUsers hook. That's an internal representation just for the users' hook and exposing it breaks encapsulation. But that can be addressed as a follow-up

[gastonfournier]

Not sure I understand this. If there's no role, why would we show a successful badge?

If there's no role, it means we couldn't find the role, but in the example below we're also trying to show the role name as children

[nunogois]

Previously this component took in a roleId as prop and used the useRole hook to fetch the role information. This worked great if you were Enterprise, not so much if you were not, since the GET role endpoint would not be available.

This PR adapts this component to have an optional graceful fallback in the children slot.

if (!role) { // Confirms that we in fact could not fetch the role information based on the `roleId`, which means we're probably not Enterprise
  if (children) // There was a fallback children passed in
    return (
      <Badge color="success" icon={<UserIcon />}>
        {children} // Return a badge with the fallback info
      </Badge>
    );
return null; // Worst case scenario, do not render the component at all
}

If you have any ideas on how to implement this in a cleaner way please let me know 👍

[gastonfournier]

What if instead, you receive either a roleId or a role object (in the case the application already has the role)? I the UI has the role object from somewhere, you'd just provide it, otherwise you use the id to fetch the role (ofc when that endpoint is available)

[nunogois]

It's a bit more complicated than that because only the useRole endpoint returns IRoleWithPermissions, most of the time we're dealing with IRole or even just the roleId. So if we want to provide detailed information it's better to accept a roleId and fetch the IRoleWithPermissions version of it.

We're basically saying the same thing but in a different order, where I prioritize roleId over role, and in this case my role is children which just includes the role name.

We could probably go through a heavier refactor here but maybe it belongs in a separate PR at a later point in time.

[nunogois]

I feel the code is much cleaner, but let me also give you some answers to your questions:

I also don't like the way roles are currently tied to the users, service accounts, project access, (...) instead of being its own thing.

I believe this comes from a misuse of the API. Users API is built to provide the list of users, so to avoid repeating the role information on every user, I believe we split that into 2 lists: a list of users, each one with its role id, and a list of roles. Then the UI would use that information to paint the users' table.

Why not 2 calls? One to fetch users with role ids and another one to fetch the roles? Not sure, probably it was a simplification when this part was developed to optimize and avoid making 2 network calls. From a REST point of view, I would prefer making 2 calls, basically 👇

This could be solved by a RoleController exposing the GET endpoints in OSS

I think that roles should not be exposed from useUsers hook. That's an internal representation just for the users' hook and exposing it breaks encapsulation. But that can be addressed as a follow-up

Thanks for your input @gastonfournier, 100% agreed here and that's the point I was trying to make. Currently we cannot use the 2 calls approach for the 2 different resources because one of them is Enterprise only, even though it's in fact an overarching concept of Unleash, at least the read part of it.

Would be nice to implement this in the future and clean up the appended roles in the other endpoints.

[gastonfournier]

Would be nice to implement this in the future and clean up the appended roles in the other endpoints.

I think as @ivarconr suggested, having a /roles endpoint in OSS that just returns [] would work. After all, we have the API documentation https://docs.getunleash.io/reference/api/unleash/get-roles available to everyone