-
Notifications
You must be signed in to change notification settings - Fork 364
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
fix: [M3-7862] - Fix display of TagsPanel in read-only entities #10275
fix: [M3-7862] - Fix display of TagsPanel in read-only entities #10275
Conversation
const isLinodesGrantReadOnly = useIsResourceRestricted({ | ||
grantLevel: 'read_only', | ||
grantType: 'linode', | ||
id: entityId, | ||
}); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's better to call this hook in each respective component instead of cluttering TagsPanel
with unrelated logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like restricted users can't view Kubernetes clusters at all, so the logic in this file may have no effect.
Coverage Report: ❌ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For Domains, should we provide some sort of indication as to why the tags are disabled? For NodeBalancers, we have a notice at the top so the reason can at least be ascertained. Maybe explanatory hover text could be appropriate here in the Domain case? Alternatively, we could give a similar treatment to Domains, with a notice at the top (this could be a separate ticket).
NodeBalancer | Domain |
---|---|
![]() |
![]() |
Going to defer to @jaalah-akamai, who's leading this project -- maybe this should be a follow-up change? |
@dwiley-akamai No because we're going to update the domains section next, so it will also have a banner. We're currently working our way through each feature or resource entity. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Disabled TagsPanel
for restricted users on read-only entities for Linodes (card view & on Details page), Domains, and NodeBalancers ✅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the read-only logic isn't doing anything for LKE, we should remove the code IMO. Or if API will support it in the future, add a @todo
placeholder about it and comment the grant-related code out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There seems to be an issue loading tags for a restricted user that has r/w Nodebalancer access
Screen.Recording.2024-03-22.at.10.55.46.AM.mov
Good catch! Seems to be caused by the way react query v4 defines |
Description 📝
#10227 disables the
TagPanel
component for read-only entities. However, this change would not work correctly for non-Linode entities, such as domains and nodebalancers, since it was hard-coded tograntType: 'linode'
.Changes 🔄
grantType
valueuseIsResourceRestricted
out ofTagsPanel
and into each respective componentTarget release date 🗓️
3/18/2024
How to test 🧪
Prerequisites
Reproduction steps
Verification steps
As an Author I have considered 🤔