Opened 8 years ago

Last modified 2 years ago

#10964 new New feature

Admin for group doesn't allow to easily add users to the group

Reported by: Filip Gruszczyński Owned by: nobody
Component: Forms Version: master
Severity: Normal Keywords: admin, groups, reverse M2M
Cc: cmawebsite@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: yes

Description

Admin website for group provides only possibility of setting the name and permission, but there is no widget for adding users (from the whole list of users) to the group. When there are lots of users in the system it's not useful, but if the website is intended for a smaller group, it's useful to easily add all users to the group.

Attachments (2)

group_admin.patch (5.5 KB) - added by Filip Gruszczyński 8 years ago.
10964_r17068.diff (8.4 KB) - added by Koen Biermans 5 years ago.
reverse m2m on forms + in admin with list_horizontal

Download all attachments as: .zip

Change History (17)

comment:1 Changed 8 years ago by Filip Gruszczyński

Has patch: set
Needs tests: set
Version: 1.1-beta-1SVN

Several days ago I created custom admin for my company software and I was allowed to turn it into a patch and post it here. It changes group admin forms, so users can be added to a group directly from there.

Changed 8 years ago by Filip Gruszczyński

Attachment: group_admin.patch added

comment:2 Changed 7 years ago by Alex Gaynor

Triage Stage: UnreviewedDesign decision needed

comment:3 Changed 6 years ago by _roman

Testing the patch revealed an error so I’d like to report about it. After patching admin website has a new widget to add user to the group. But when I click on the button save or save and add another or save and continue editing appears “error 403” with message – “CSRF verification failed. Request aborted” I've used Django v1.3

comment:4 Changed 6 years ago by Luke Plant

The patch would need a lot of work before it could possibly go in. _roman highlights one issue (the csrf_protect_m decorator is not applied). There are lots of others, like:

  • the overridden add_view and change_view remove lots of functionality present in the base class methods, for no apparent reason.
  • why not just set the form = GroupForm on GroupAdmin?
  • why do we need to override the template?
  • 8 spaces for indentation instead of 4.

We also have to consider the case where people have further sub-classed GroupAdmin, or provided their own template - their customised admin interfaces should not break, as far as we can ensure that.

I think that the idea in general is good. We may need to consider whether a different approach entirely is needed - the basic problem is that while M2Ms are (nearly) symmetrical in terms of the API that is produced by the ORM on the two related models, irrespective of which model the M2M is defined on, the admin responds quite differently depending on which model has the M2M defined. This isn't ideal, as which model gets the M2M can be determined simply by the order of dependencies and things like that.

comment:5 Changed 6 years ago by Filip Gruszczyński

As you can see, this patch is nearly 2 years old, when there was no CSRF protection and I was making my first steps in Django. All your remarks are of course right and this patch is no good.

If you are interested, I can try to provide a better patch now. But since this ticket got very little attention, I am not entirely sure, if you want it anyway. I wouldn't like to devote time to something, that might not be in accord with the overall deisgn.

comment:6 Changed 6 years ago by Julien Phalip

Severity: Normal
Type: New feature

comment:7 Changed 6 years ago by Julien Phalip

UI/UX: set

Changed 5 years ago by Koen Biermans

Attachment: 10964_r17068.diff added

reverse m2m on forms + in admin with list_horizontal

comment:8 Changed 5 years ago by Koen Biermans

Easy pickings: unset
Needs documentation: set
Patch needs improvement: set

Added a patch that allows adding a descriptor of a reverse m2m (like 'user_set') to the fields list for a form.

An extra is that you can also add the same into the fields and filter_horizontal attributes of a modeladmin.

As an example this is used into the GroupAdmin to allow easy user selection for a group.

No tests or documentation yet, since I am not completely happy with the implementation yet.

I think an implementation for reverse foreign keys would also be usefull.

Feedback welcome.

comment:9 Changed 5 years ago by Aymeric Augustin

Triage Stage: Design decision neededAccepted

I agree that the idea in general is good.

comment:10 Changed 5 years ago by Aymeric Augustin

Component: contrib.admincontrib.admindocs

In fact, this is specific to the auth app, not to the admin.

comment:11 Changed 5 years ago by Aymeric Augustin

Component: contrib.admindocscontrib.auth

Typo :(

comment:12 Changed 3 years ago by Koen Biermans

I worked on this again. Progress is here: https://github.com/planop/django/tree/ticket_10964

This is working:

  • in a modelform: in the fields list, use the descriptors for reverse M2M or FK, like 'book_set'. This will add a select multiple to the form.
  • in the admin: the same + add the descriptor name to filter_horizontal and filter_vertical to have the fancy select widget versions
  • applied this to the auth admin for the Group model so you can select users in the group creation and edit views('user_set' in fields and filter_horizontal)

There are a few tests and a beginning of documentation.

I am not entirely happy with the way the descriptors are detected, but I know of no better way at the moment.
Not sure this should go in to the auth admin by default either.

comment:13 Changed 3 years ago by Koen Biermans <koen@…>

Component: contrib.authForms
Keywords: reverse M2M added
Needs documentation: unset
Needs tests: unset

WIP still here: https://github.com/planop/django/tree/ticket_10964

Excluded non-nullable foreignkeys, because you would be unable to remove an item via the multiplechoicefield, which would be very surprising and does not make sense.

comment:14 Changed 2 years ago by Collin Anderson

The is basically a duplicate of #897

comment:15 Changed 2 years ago by Collin Anderson

Cc: cmawebsite@… added

It seems to me if we want to support formfield_callback for these fields, the proper way would be create a virtual field for the reverse relationship. It would also be much easier to support custom widgets, localized_fields, labels, help_texts, error_messages, and editable.

Note: See TracTickets for help on using tickets.
Back to Top