-
Notifications
You must be signed in to change notification settings - Fork 45
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
Role requests UI #222
base: main
Are you sure you want to change the base?
Role requests UI #222
Conversation
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.
LGTM, some nits but no blockers. Might follow up to add a style linter to migrate to ===
and !==
for behavior consistency...eventually...
)} | ||
</TableCell> | ||
<TableCell> | ||
{row.resolver == null && row.status != 'PENDING' ? ( |
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.
nit - this ternary operator chain should be a function that returns a component instead of inline for readability and/or if someone wants to add a new condition but NAB
return params; | ||
}); | ||
setRowsPerPage(parseInt(event.target.value, 10)); | ||
setPage(0); |
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.
I know the other search pages work similarly, but how would you feel about clearing all of the search params instead of setting the page explicitly to page 0? There's some inconsistent behavior I've noticed elsewhere due to this 🤷 NAB but was planning to make changes along this line at some later point
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.
I'll probably leave this for now since (while maybe not ideal, I have no idea what front end best practices are) it's consistent with most of the other pages
} | ||
|
||
// Avoid a layout jump when reaching the last page with empty rows. | ||
const emptyRows = rowsPerPage - rows.length; |
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.
🤔 is it possible for rows.length to ever be greater than rowsPerPage or is that handled? Wondering if this should have a Math.max(emptyRows, 0)
wrapper or something
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.
I don't think so with how the pagination works on the API side? That said, Peter wrote that part and we've just copied it into all the list pages. Hasn't broken yet though..?
Adding in Role Requests to the Access UI
In this PR, I:
General Workflow
If you own at least one role, a 'Create a request button' will be shown. You will have the option to choose for which of the roles you own you'd like to make a request, to which non-role group you'd like the role to be added to, membership/ownership, an amount of time for access, and the ability to provide a reason.
Owners or admins should be notified on role request creation.
Owners who are not blocked by group tag constraints can respond to the request. If the owner is blocked, a note will be shown.
If all group owners are blocked, Access admins will need to respond to the request. The request is added to admins' 'Assigned to me' list and a note is shown that all owners are blocked on the request page.
After a decision is made on the request, the requester is notified of the decision.
Regular role request read page:
Viewing role members who will be added if the request is approved:
Blocked owner role request read page:
Admin view all owners blocked request read page: