Conversation
zburke
left a comment
zburke
left a comment
There was a problem hiding this comment.
- Is there a 1::1 correspondence between "can see the roles assigned to a user in ui-users" and "can see the role details in settings in ui-authorization-roles"? If yes, great. If not, all these changes need to be guarded by
<IfPermission>, showing a link when the user has permission and the bare role-name otherwise. - Why do the links open in a new tab? This is inconsistent with how other links in FOLIO behave and is not specified in the ticket.
@zburke Good catch thank you! There is, in fact, a separate perm for reaching the Settings > Role details page. Will include a check for it.
New tab works for forms/modals where there's some state that's potentially lost with navigation. Since these are included on the edit form, the details form just follows for consistency. This is has been regarded as something that could easily change later depending on user feedback. |
There was a problem hiding this comment.
@JohnC-80, 👌, I'm requesting changes for the missing <IfPermission> checks.
Opening links from a modal in a new window is sensible to avoid the navigation prompt. I like that pattern. Can/should we document it on the TextLink readme? On the TextLink storybook? I'm not convinced it's the right pattern, however, when a role-name is displayed in an accordion on the main window. If you want to leave this as-is, ok, but I think there's a solid argument that only the modal's links should behave this way.
|
@zburke Wrapped the permission check up in a small component and used that in place of the role names. 🧹 |
…nt is the same as the selected affiliation
|
Included an additional check to see if the user has their login affiliation selected from the |
zburke
left a comment
zburke
left a comment
There was a problem hiding this comment.
Functionally, this gets the job done: links are present on roles that match the current tenant of the authenticated user, otherwise, the role name is displayed. So, approved ✅
That said, I had a hard time with the renaming of the return value from matchesLoginTenant as it was drilled through to RoleNameLink. It effectively gets renamed to viewingLoginAffiliation in EditUserRoles, and then to canRenderLink in RoleNameLink. It's likewise renamed to displayRoleDetailLinks in UserRolesModal and UserRolesList. Additionally, is "login-tenant" really the right label, or is it actually "current-tenant"?
Maybe contexts (for each user's affiliations, and nested within that for each user's currently-selected affiliation) would allow matchesLoginTenant (maybe just matchesTenant?) to be implemented directly within RoleNameLink? Or maybe clean code doesn't matter any longer now that robots can write all our code?
| const RoleNameLink = ({ role, canRenderLink = true }) => { | ||
| return ( | ||
| <IfPermission perm="ui-authorization-roles.settings.view"> | ||
| {({ hasPermission }) => (hasPermission && canRenderLink | ||
| ? <TextLink to={getRoleDetailPath(role.id)} target="_blank">{role.name}</TextLink> | ||
| : <>{role.name}</> | ||
| )} | ||
| </IfPermission> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
target should not be hard-coded here. Make it an optional prop?
|
2 participants
UIU-3550
Setting up links in Role lists on the use details page, the edit page and in the role selection modal.
The links will only render if:
All links open a new tab.
2026-09-10_16h04_22.mp4