Skip to content
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

Manage members in app #804

Merged
merged 18 commits into from
Jul 27, 2023
Merged

Manage members in app #804

merged 18 commits into from
Jul 27, 2023

Conversation

mejo-
Copy link
Member

@mejo- mejo- commented Jul 24, 2023

📝 Summary

This PR adds functionality to manage members for a collective inside the Collectives app. This removes our dependency on the Contacts app to do so.

It's possible to add and remove members and promote/demote their membership level.

🖼️ Screenshots

Searching for members Changing membership level
grafik grafik

🖼️ Screencast

🏡 Walkthrough creating a collective and managing its members
recording1.webm

🚧 TODO

  • Cypress tests
  • Don't allow to delete circle/group if current admin is member through it
  • Always show option to leave collective (also to admins)
  • Remove link to manage members in contacts
  • Remove dependency checks on contacts app (and close Remove the notification "The contacts app is required to manage members" #779)
  • Filtering out member groups doesn't work (groups have type 16 in circle response)

🏁 Checklist

  • Code is properly formatted (npm run lint / npm run stylelint / composer run cs:check)
  • Sign-off message is added to all commits
  • Tests (unit, integration and/or end-to-end) passing and the changes are covered with tests
  • Documentation (README or documentation) has been updated or is not required

@mejo-
Copy link
Member Author

mejo- commented Jul 24, 2023

@nextcloud/designers I'd appreciate feedback from you 😊 In particular on the icons used for admin/moderator/member levels and on the way the loading indicator icon is used.

@cypress
Copy link

cypress bot commented Jul 24, 2023

Passing run #972 ↗︎

0 75 0 0 Flakiness 0

Details:

Manage members in app
Project: Collectives Commit: 0d4ec5590c
Status: Passed Duration: 04:10 💡
Started: Jul 27, 2023 3:27 PM Ended: Jul 27, 2023 3:31 PM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@mejo- mejo- changed the title Enh/manage members Manage members in app Jul 24, 2023
Copy link
Member

@marcoambrosini marcoambrosini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a design perspective, I'm sorry but I don't have any constructive feedback to share. It looks very neat and easy to use, congrats!! :)

Copy link
Member

@juliushaertl juliushaertl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very nice, code also looks good, just a few commments from testing:

  • The search results are not keyboard accessible
  • The scroll in the modal seems a bit odd with two scroll areas:
Screenshot 2023-07-25 at 09 30 04
  • Maybe we can hide the "Add users, groups or circles ..." heading if the search is not triggered, the placeholder hint to search to add groups seems enough and the members also don't have a heading in the default state
Screenshot 2023-07-25 at 09 32 39

@mejo-
Copy link
Member Author

mejo- commented Jul 25, 2023

Maybe we can hide the "Add users, groups or circles ..." heading if the search is not triggered, the placeholder hint to search to add groups seems enough and the members also don't have a heading in the default state

@juliushaertl I would prefer to keep it. Talk behaves the same in the participants tab and I think its good to emphasize that you can search for new members to add.

@mejo- mejo- requested a review from juliushaertl July 25, 2023 16:35
@mejo- mejo- marked this pull request as ready for review July 26, 2023 13:45
Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, looks nice design-wise! :)

@max-nextcloud
Copy link
Collaborator

Looking at the screencast it seems that people, circles and groups that are already in the collective are still listed in the sections to add members. I tried in talk and it seems to filter them out. I think that would be good.

@mejo-
Copy link
Member Author

mejo- commented Jul 27, 2023

Looking at the screencast it seems that people, circles and groups that are already in the collective are still listed in the sections to add members. I tried in talk and it seems to filter them out. I think that would be good.

This is already fixed in the PR, but I didn't update the screencast ☺️

@nimishavijay
Copy link
Member

Looks super nice! Only some minor points :)

  • We generally don't use the "light" font weight very much, instead we use the regular font weight in --color-text-maxcontrast
  • We could switch the icons for admin and moderator since the admin role has the most permissions
  • Since there are 2 levels of permissions, there could be an simple explanation in a subline for all the roles to avoid confusion between admin and moderators
    • Admin: Manage collective members, view, edit, share and delete collectives
    • Moderators: View collectives, edit and share collectives if permitted
    • Members: View collectives, edit and share collectives if permitted

* Refactor MemberPicker and MemberSearchResult, group search results
  inside MemberSearchResults.
* Use `core/autocomplete/get` endpoint to get search results.
* Fetch search results with empty search string at the beginning.

Signed-off-by: Jonas <[email protected]>
Preparations for reusing MemberPicker for the membership management
modal.

Signed-off-by: Jonas <[email protected]>
mejo- added 13 commits July 27, 2023 17:04
Adds a modal for admin users that lists current members and allows to
search for new ones.

Current members have actions to get their level promoted/demoted and to
get removed.

Found new members can be added.

Further refactoring of the MemberPicker is part of this commit.

Signed-off-by: Jonas <[email protected]>
We have our own membership management in Collectives now.

Also show collective action to leave a collective to admins.

Fixes: #779

Signed-off-by: Jonas <[email protected]>
@mejo-
Copy link
Member Author

mejo- commented Jul 27, 2023

Thanks @nimishavijay. I already changed the font color to --color-text-maxcontrast and switched the icons. For the level explanations this is a bigger thing and I'd like to postpone it, as I either would have to reimplement or extend NcActionButton.

In CI, our Cypress tests run against a single process PHP webserver.
This causes problems with async circles events.

Removing members from a circle is async. Disable the check for member
being removed from UI for now.

Signed-off-by: Jonas <[email protected]>
@mejo- mejo- merged commit e0fd485 into main Jul 27, 2023
40 checks passed
@delete-merged-branch delete-merged-branch bot deleted the enh/manage_members branch July 27, 2023 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants