Add page for listing Group members #973
Conversation
Simplifying conditionals in preparation for adding another dynamic route for a group members page.
I was looking into tackling the TODO comment for the location prop¹ in the groups JS routing. The location prop was being passed around to be used in the NavBar component, but the use of it was removed in a recent refactoring.² So just remove all of the unused `location` props for the `NavBar` and the `GenericPage` components. ¹ <https://github.com/nextstrain/nextstrain.org/blob/1aeb1e499b9f25d9a918465fd3450fca51f40a2d/static-site/pages/groups/%5B...groupName%5D.jsx#L22> ² <ac12e32>
The OPTIONS method was included in the RESTful API docs¹ for the new Groups membership routes, but were not added to the routes themselves. I will be using this in future changes to check user permissions for viewing and editing Group members. ¹ <84a7374#diff-676fb03989c04d3661eac5e82cb55fc6a9f43df70bd94565535ba509e12f0c64>
The new page is available at `groups/<groupName>/settings/members`. I chose to use the same URL as the API so that we don't have to take up another namespace (e.g. `groups/<groupName>/members`). The main Group page will only link out to the members page if the current user is authorized to list Group members. The members list only displays the most privileged role for each member to keep the display simple. It assumes the group roles API returns the roles in the order from least to most privileged.
- remove unused `props` - remove unnecessary semicolons
There was a problem hiding this comment.
The page loads fine in local testing, so consider everything non-blocking.
My suggestions are largely influenced by GitHub's design on https://github.com/orgs/nextstrain/people
Based on feedback from @victorlin in review <#973 (comment)>
Based on feedback from @victorlin in review <#973 (comment)>
|
not a full review, but passing comment
This feels fragile. It makes the display (and eventual management UI) at odds with the realities of our data model, which leaves a space for bugs to lurk. I think it's particularly important to represent the data model accurately when dealing with permissions/ACLs/security/etc. |
Based on feedback from @genehack <#973 (comment)>
Thanks for flagging @tsibley, I agree this can be fragile if we ever decide to update the Group roles. Is the suggestion to update the UI to display multiple roles or to update the data model to only allow users to be a part of one role? I'm personally leaning towards updating the data model to a single role per user per group (unless there was a specific reason to allow a member to have multiple roles). This seems to be inline with the current provision script which removes the user from other roles when they get assigned a new role. |
|
Update the UI to display multiple roles. Updating the data model is not trivial/possible, as users can inherently be in multiple Cognito groups and we can't control that. I never saw that as a problem because RBAC is inherently multi-role and we could easily have reasons to use more than the basic three we started with. |
Based on feedback from @tsibley <#973 (comment)>
Updated in 027910d:
If there's no other feedback, I'll merge this tomorrow. |
Description of proposed changes
The new page is available at
groups/<groupName>/settings/members. I chose to use the same URL as the API so that we don't have to take up another namespace (e.g.groups/<groupName>/members). The main Group page will only link out to the members page if the current user is authorized to list Group members.The members list only displays the most privileged role for each member to keep the display simple. It assumes the group roles API returns the roles in the order from least to most privileged.
The plan is to make future changes to this page to allow Group owners to edit members here as well. I'm making that a separate change to keep this PR simple.
Related issue(s)
Related to #804
Checklist