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

Group users list #5854

Closed
wants to merge 3 commits into from
Closed

Conversation

karranb
Copy link
Contributor

@karranb karranb commented Feb 22, 2020

Description

This PR implements a group user listing endpoint.

Issue

#5801

@squash-labs
Copy link

squash-labs bot commented Feb 22, 2020

Manage this branch in Squash

Test this branch here: https://karranbfeatgroup-users-list-4nml0.squash.io

@isandunk
Copy link

isandunk commented Mar 6, 2020

Thanks so much for your work on this @karranb

I've tested this in Squash. I have a couple of suggestions from a UX point of view:

  • Perhaps we could rename the button so that it is more obvious what it does? So something like "View users of this group" or "View users of <group.id/>"
  • In other sections of wagtail, like Images or Documents, they have actions like this in the header (eg. In Documents, it has "Add Document"). Perhaps this button would be better situated there?

Again, thanks for you work.

@karranb
Copy link
Contributor Author

karranb commented Mar 6, 2020

Thanks for the review. Awesome suggestions, I'll work on them 😄

@karranb
Copy link
Contributor Author

karranb commented Mar 7, 2020

Hey @isandunk, I had to make a small refactor on the heading component to accept other buttons besides 'add' ones. I can implement using another approach if you find one that fits better 😄

@isandunk
Copy link

isandunk commented Mar 9, 2020

Thanks @karranb! I think that's a great solution!

Copy link
Member

@lb- lb- left a comment

Choose a reason for hiding this comment

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

@karranb - this works great, I understand the reasoning of renaming the add_link to action_url thanks for doing a bit of house keeping there. In future it might be good to split those kinds of changes to two PRs (just easier to review).

Nonetheless, works well, code looks good and thanks for adding tests.

We will get these merged in soon, one note is that in the listing of users when filtered by groups there can be cases where there are no users in that group. I have revised the translation in this scenario to be group specific as it can be a bit confusing.

Also noticed an unrelated bug where the 'add one' (link to add users) did not actually work, have fixed this also.

Finally, a future PR (would love to see it if you want) would be to add a link to the users for a group on the group listing page also (in each row), maybe with the user count. :)

@lb-
Copy link
Member

lb- commented Apr 26, 2020

merged in va 7565248

thanks again @karranb

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants