Opened 8 years ago

Last modified 21 months ago

#27487 new Cleanup/optimization

ModelAdmin.formfield_overrides on ManyToManyField isn't compatible with CheckboxSelectMultiple

Reported by: nrogers64 Owned by:
Component: contrib.admin Version: 1.10
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

The following ModelAdmin code updates the widget, as expected:

formfield_overrides = {
    ManyToManyField: {'widget': CheckboxSelectMultiple},
}

However, it has two problems:

The first problem is that it provides the little green plus sign to add another object on the fly, but that functionality doesn't actually work properly with the CheckboxSelectMultiple widget (after adding another object, you have to refresh the page in order to see it). So I would think that this widget shouldn't provide the little green plus sign.

The second problem might not be valid because of what I'm doing in my project. Currently, the <form> in change_form.html has novalidate hard-coded in:

https://github.com/django/django/blob/1.10.3/django/contrib/admin/templates/admin/change_form.html#L40

I have overridden that in my project, removing the novalidate attribute. I understand that my doing that might make this second problem invalid. The second problem is that the user has to check each of the checkboxes in order to submit the form.

Attachments (2)

#27487.diff (26.7 KB ) - added by Adonys Alea Boffill 8 years ago.
Fixed add related object in admin popup for checkbox/radio
ticket_27487.diff (39.7 KB ) - added by Adonys Alea Boffill 8 years ago.
New patch with the corrections related to template-based widget rendering.

Download all attachments as: .zip

Change History (12)

comment:1 by Tim Graham, 8 years ago

Component: Uncategorizedcontrib.admin
Summary: Using formfield_overrides with CheckboxSelectMultiple has two problemsModelAdmin.formfield_overrides on ManyToManyField isn't compatible with CheckboxSelectMultiple
Triage Stage: UnreviewedAccepted
Type: BugCleanup/optimization

I'm not sure how much work will be required to make the green + work. It might be more feasible to remove it.

As for the second issue, I didn't try to reproduce it but I'm not sure what could be happening since CheckboxSelectMultiple shouldn't render with the required attribute.

comment:2 by Adonys Alea Boffill, 8 years ago

Owner: changed from nobody to Adonys Alea Boffill
Status: newassigned

comment:3 by Adonys Alea Boffill, 8 years ago

Has patch: set
Last edited 8 years ago by Tim Graham (previous) (diff)

comment:4 by Adonys Alea Boffill, 8 years ago

To fix this ticket, i've opted for a similar solution to the formset behavior.
For the case of ChoiceFieldRenderer used on CheckboxSelectMultiple and RadioSelect widgets, an empty item will be added as first item of the list. This item could be used as a template for create a new element of the list(<ul></ul>). Respecting all the html customizations made by the developer.
The RelatedObjectLookups.js file is changed in order to handle the result given by the admin related object popup. I added some JavaScript's tests to validate the implementation of dismissAddRelatedObjectPopup method for all cases.
Similar to the formset, I added an style class empty-choice to keep hidden the empty item choice.

comment:5 by Tim Graham, 8 years ago

Patch needs improvement: set

Thanks for the patch, but I'm not sure this approach will work. You can add the CSS for the admin, but that choice will appear everyone else Django renders forms.

By the way, your patch seems inverted -- all additions are removals and vice versa.

Also, we're hoping to merge template-based widget rendering for 1.11 so it would be nice if a patch is based on that branch. ChoiceFieldRenderer, for example, is removed.

by Adonys Alea Boffill, 8 years ago

Attachment: #27487.diff added

Fixed add related object in admin popup for checkbox/radio

comment:6 by Adonys Alea Boffill, 8 years ago

Thanks for your time... I can work to improve the patch, maybe adding an attribute with which you can change the default behavior in order to avoid render always the empty choice.
But, if ChoiceFieldRenderer will be removed, then I have not option. I hope be more useful in the future.
To avoid this situation i'll try to share my idea on django-users mailing list or #django IRC channel before implement it.
On the other hand, you are totally true, I made git diff in the inverse direction, I fixed that.
Thanks!

comment:7 by Adonys Alea Boffill, 8 years ago

Patch needs improvement: unset

Going back to the point about this ticket, and following the suggestion of that comment https://code.djangoproject.com/ticket/27487#comment:5, I implemented a new solution for this problem, this time using template-base widget rendering recently merged to master.

This problem occur when you override the defaults Select and SelectMultiple widgets in the admin by RadioSelect or CheckboxSelectMultiple respectively, and you need to add a new object of the relationship. Both widgets are rendered as UL HTML list, and there are not JavaScript support for that in RelatedObjectLookups.js. Every time when you add a new object of the relationship, you need to refresh the page to update the list with the added object.
I added support for these widgets in RelatedObjectLookups.js. To fix that problem I added two attributes to the UL list in multiple_input.html, both attributes will have "true" or "false" as possibles values and are the following:

  • data-wrap-label: Is "true" if the inputs of the list are wrapped into a label.
  • data-multiple: Is "true" if is a multiple select.

With that information I can handle the behavior of to add a new object of the relationship using RadioSelect or CheckboxSelectMultiple as widget.
To be sure of the correct operation, I added JavaScript and Selenium tests, the rest of the tests runs fine.
I will add a new patch, but if necessary I can make a Pull Request.

by Adonys Alea Boffill, 8 years ago

Attachment: ticket_27487.diff added

New patch with the corrections related to template-based widget rendering.

comment:8 by Tim Graham, 8 years ago

As noted on the PR, I'm not sure about adding the data attributes for all Django forms to solve this admin issue. Maybe you could summarize the problem and solution to the DevelopersMailingList to see how others feel.

comment:9 by Tim Graham, 8 years ago

Patch needs improvement: set

comment:10 by Mariusz Felisiak, 21 months ago

Owner: Adonys Alea Boffill removed
Status: assignednew
Note: See TracTickets for help on using tickets.
Back to Top