lib-user: add group manage modal with group update inputs, group delete, and members management

[mcbouslog]

Package

• lib-user

Describe your changes

• remove temporary group Edit and Delete components
• move GroupForm and GroupModal to shared components
• refactor GroupForm for editing a group, styling per Figma
• create component to list memberships
• create component for membership item in memberships list

How to Review

Helpful explanations that will make your reviewer happy:

• What Zooniverse project should my reviewer use to review UX? n/a
• What user actions should my reviewer step through to review this PR?
  • confirm can still create a group
  • edit a group
  • edit a group's members (i.e. add/remove admin access, remove from group)
• Which storybook stories should be reviewed?
  • GroupForm (only styling changes): http://localhost:6008/?path=/story/components-shared-groupform--default
  • MembersList: http://localhost:6008/?path=/story/components-groupstats-memberslist--default
  • MembersListItem: http://localhost:6008/?path=/story/components-groupstats-memberlistitem--default
• Are there plans for follow up PR’s to further fix this bug or develop this feature? not specifically, generally see linked GitHub project

Checklist

PR Creator - Please cater the checklist to fit the review needed for your code changes.
PR Reviewer - Use the checklist during your review. Each point should be checkmarked or discussed before PR approval.

General

• Tests are passing locally and on Github
• Documentation is up to date and changelog has been updated if appropriate
• You can yarn panic && yarn bootstrap or docker-compose up --build and FEM works as expected
• FEM works in all major desktop browsers: Firefox, Chrome, Edge, Safari (Use Browserstack account as needed)
• FEM works in a mobile browser

General UX

Example Staging Project: i-fancy-cats

• Can sign-in and sign-out
• The component is accessible
  • Can be used with a screen reader BBC guide to VoiceOver
  • Can be used from the keyboard WebAIM guide to keyboard testing
  • It is passing accessibility checks in the storybook

New Feature

• The PR creator has listed user actions to use when testing the new feature
• Unit tests are included for the new feature
• A storybook story has been created or updated
  • Example of SlideTutorial component with docgen comments, and its story

[coveralls]

coverage: 79.133% (-0.2%) from 79.323%
when pulling 7eeb5bb on lib-user_manage-group
into 9a7a630 on master.

[goplayoutside3]

Code read is generally looking good, and the storybooks too. Could you join this group, so I have a test group where I'm admin and multiple members? https://www.zooniverse.org/groups/1326995?join_token=0e834515f7390aa4 (staging)

Noting that the GroupModal + GroupForm is relatively tall, so it takes up the majority of my viewport height which risks hiding the Save button. The figma design for managing a group does not include the extra vertical space and borders that exist in this PR. Is the figma link the correct reference?

I've left several more suggestions and comments below...

[goplayoutside3]

This sequence is an "optimistic UI" pattern where you're assuming the deletePanoptesMembership or updatePanoptesMembership request to panoptes is successful, and therefore don't need to call usePanoptesMemberships again. Just wondering if you thought about the pros/cons of triggering usePanoptesMemberships upon delete or update, and then simply using data: membershipsData as source of "state" for this component rather than useState.

(I don't have a great answer, mainly just for my own understanding if an "optimistic UI" pattern is the easiest for this component, that's okay!)

[mcbouslog]

I'm not exactly sure how I'd implement it using membershipsData as state, as I think I'd have to add a param to the usePanoptesMemberships hook so it knew to update? Open to suggestion, but this was what felt easiest initially. Definitely have to refactor for sad paths in a few places, maybe worth creating a dedicated card/issue.

[goplayoutside3]

I thought about this concept a bit more, and I think it's worth looking into SWR's mutations. This youtube tutorial is also a great example of SWR data fetching + mutation in practice.

The majority of data handling in the user groups and stats pages simply fetches data from panoptes, which you've implemented with SWR hooks. This works well because the fetched/cached data can't be updated on the user groups or stats pages (for instance, stats are modified on classify pages or a user's display name is updated on their profile settings page). However, group memberships is the major feature that requires modifying data in panoptes while also updating the currently-viewed UI on the fly.

SWR provides hook useSWRMutation for cases where you need to POST, PUT, or DEL to the backend, yet continue to use the data returned from a GET hook like usePanoptesMemberships in MembersListContainer. useSWRMutation is an optimistic UI pattern, but without creating a separate useState variable.

The second reason I think it's worth looking into SWR mutations is to handle the forced page refresh I commented on. Forcing a page refresh after modifying group membership data feels like an anti-pattern in a React application where components in the UI should re-render when passed new data props. GroupStatsContainer also utilizes usePanoptesMemberships and passes membershipsData to GroupStats. With SWR mutations you could rely on membershipsData to update after a mutation and cause a GroupStats re-render without a total page refresh.

Please let me know your thoughts! And if you'd like to separate changes to membership data handling into another PR or continue on this one.

[mcbouslog]

The useSWRMutation SWR hook noted sounds great to me, certainly preferable. My only hesitation is adding to this PR's changes, but if that's ok with you then it's ok with me and I'd like to try refactoring accordingly.

Other user/group stats PRs aren't dependent on this PR so it won't hold anything up if this gets additional work.

[goplayoutside3]

Feel free to add to this PR if easiest in the flow of features for lib-user! You could also create a PR requesting to merge into this one with isolated changes for data mutation. Happy to review either way.

[goplayoutside3]

Continuing to review this! One more important check though -

I see both of your accounts joined https://local.zooniverse.org:3000/groups/1326995 (staging), so I tested out the group manage model and I'm confused about whether group members are related to Top Contributors. I removed one of your accounts from the group, so the manage modal looks like this:

But the account I removed still shows up on Top Contributors like this:

Is that expected? The Top Contributors is every account that's ever classified while in the group, even if they're no longer in the group?

[mcbouslog]

@goplayoutside3 that's a good question. I think that's expected, but I'm going to tag @yuenmichelle1 for the ERAS perspective on group members included/excluded in a response for request that includes 'top_contributors'.

[mcbouslog]

Regarding active/inactive members and stats inclusion, per related Slack thread:

• when a group member classifies their stats will persist as part of the group stats regardless of their group membership state
• a user is linked to a user_group through a membership (panoptes resources marked as code)
• when a member is "removed," which means a user's group membership.state is changed from "active" to "inactive", their classification stats (count and time) while an active group member will remain as part of the group's stats, but subsequent to removal the user's classification stats will not count towards the group's stats
• i.e. a group cares about inactive member classifications when they were active group members (but not before or after they were active group members)

Planned follow-up for a subsequent PR:

• update group manage modal Members section for clarity on "Remove member"
  • could include adding info near Members field title
  • could include rewording "Remove member"
• add tooltip to Top Contributors section noting includes inactive members

[goplayoutside3]

Looks good, and the modal styling update is nice 👍

I'd still like to see an in-code comment addressing joinStatusSuccess for documentation (PR comment),

In this PR, you've also added more util functions for fetching data, but none of the requests have unit tests yet. I noticed nock is setup, but unused in lib-user, so I suggest creating some unit tests for the util functions soon to ensure reliability of the utils over time. There are lots of nock examples in app-project.