Feature/conditionally hide UI based on roles #1377
Conversation
|
|
||
| const userCanReview = useMemo( | ||
| () => | ||
| hasRole(currentUserRoles, ['msro', 'mtr']) && |
There was a problem hiding this comment.
I'm a little apprehensive about having authorisation now in both the frontend as well as the backend as it's duplication that will not be good for maintainability. Specifically here it's important to remember that in the near future that the roles for reviews will be dynamic and defined in the schema.
There was a problem hiding this comment.
Agreed. I was conscious of that when writing this and have made a mental note to prepare for the amount of added effort this work will add to dynamic roles. I've tried to limit that by using the same util functions for most of this.
There's definitely some changes that will need to be made once dynamic roles come in, but I'm hoping that's as simple as updating the getCurrentUserRoles call. (At least for this current solution, so it shouldn't block dynamic roles)
As for the duplication of authorisation being in the frontend and backend, I'm not sure what we can do to solve that right now. This also leads into the read-only logic for mirrored models, which adds another layer of complexity, so I think we need to design a better solution that works for both. If we can come up with a solution that contains auth to the backend, even better.
It's worth noting that this work only expands on our existing roles implementation in the frontend, so imo we should create a ticket to address this issue for both roles and mirrored models rather than doing it in this PR.
| | { | ||
| canUserEditOrDelete?: never | ||
| actionButtonsTooltip?: never | ||
| } | ||
| ) |
There was a problem hiding this comment.
Do we need this. I can't see a place where these props wouldn't be resolved. If not would it be more efficient doing something like this.
{
canUserEditOrDelete?: boolean | never
actionButtonsTooltip?: string | never
}There was a problem hiding this comment.
I was doing this to make EditableFormHeading more generic by forcing either both props to be provided or neither. That way if someone wants to disable the buttons, they're forced to also provide a tooltip for the user to understand why the buttons are disabled.
However, I agree that there's not going to be a case where we don't want this protection now that it's being added to inferencing, so I'll make these required props.
| <LoadingButton | ||
| fullWidth | ||
| variant='contained' | ||
| disabled={isReadOnly || true} |
There was a problem hiding this comment.
Very small. But might be better using {true || isReadOnly} for now, as Logical ORs are evaluated left-to-right?
| view: <Settings entry={model} currentUserRoles={currentUserRoles} />, | ||
| }, |
There was a problem hiding this comment.
Not sure how I feel about this. In my opinion the settings tab itself should be restricted?
There was a problem hiding this comment.
I asked the same thing, but it's important for any user to be able to see the collaborators in the entry access tab. We could simplify a lot of the settings roles logic if we find another place to display that information. I'll make a ticket to explore this idea.
No description provided.