Defer project kebab check and disable #3003
Conversation
emilys314
force-pushed
the
defer-project-kebab-check-and-disable
branch
from
a668565 to
073bee0
Compare
frontend/src/pages/projects/screens/projects/useProjectTableRowItems.ts
Outdated
Show resolved
Hide resolved
frontend/src/pages/projects/screens/projects/useProjectTableRowItems.ts
Outdated
Show resolved
Hide resolved
emilys314
force-pushed
the
defer-project-kebab-check-and-disable
branch
2 times, most recently
from
6e59503 to
0e6ef65
Compare
| setShouldRunCheck(true); | ||
| }, []); | ||
|
|
||
| const noPermissionToolTip = (allow: boolean, loaded: boolean) => |
There was a problem hiding this comment.
Technically a nit because our linter should be doing this if we wanted it...
But typically you always want to type functions for both input and return -- in this case, it's a little less important, but I think it's a good practice to get into.
If you create this function in the root, I believe our linter yells at you to add a return type.
| const noPermissionToolTip = (allow: boolean, loaded: boolean) => | |
| const noPermissionToolTip = (allow: boolean, loaded: boolean): Partial<KebabItem> => |
There was a problem hiding this comment.
Sure thing, I just added the Partial<KebabItem> | undefined return type onto it. Oddly the linter doesn't complain if the function is at the root either.
There was a problem hiding this comment.
Interesting... I swore we had that linter. cc @christianvogt ?
There was a problem hiding this comment.
Looks like the eslint rule is disabled for that
in frontend/.eslintrc on line 156
"@typescript-eslint/explicit-function-return-type": "off",
There was a problem hiding this comment.
We do have this rule enabled: @typescript-eslint/explicit-module-boundary-types
This forces a type for all exported properties. Anything else internal to the module can use inference.
emilys314
force-pushed
the
defer-project-kebab-check-and-disable
branch
from
0e6ef65 to
b9392f5
Compare
There was a problem hiding this comment.
Looks good. We may end up eventually being "wrong" if the user gets permissions mid-viewing of the page.
Example: I invited someone as a contributor to my project. They wanted to change the description because of a typo. I go 'ah okay, let me promote you to Admin' -- they'd need to refresh or paginate forward/back to get a new state for the dropdown.
It's minor, and very specific changing of permissions that is not likely to happen. Our solution may even just be a timeout reset of the hover state. Either way, I don't imagine this to be a big concern. if we get the bug we can handle it at that time for the use-case that is provided.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andrewballantyne The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Closes RHOAIENG-1163 and #1932
Description
When a user doesn't have permission to Edit Project, Delete Project, or Edit Permissions, the actions will be grayed out and disabled. This prevents a user from trying to do the action and later getting an insufficient permissions error. (We decided to not hide them, because then we would have to hide the entire kebab menu if there were no permissions, which requires a check on display of the table.)
This also defers the project permission checks until the menu will be opened. This saves us from having to make 1 (or more) api calls per project being displayed on the table on page load.
How Has This Been Tested?
Tested locally with varying user permissions. Checked that projects with sufficient permissions allow actions, and without required permissions, the options will be disabled and the tooltip will show.
Added a test to check the insufficient permissions behaviour.
Test Impact
Mocked access review calls should be done if actions in the kebab menu are going to be used, since they are gated now.
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
aria-disabledbeing used to add tooltip for a disabled item:Throttled testing to show permissions disabled while network call is still loading:
Screencast.from.2024-07-15.08-52-48.mp4
After the PR is posted & before it merges:
main