Opened 12 months ago

Last modified 5 months ago

#27487 assigned Cleanup/optimization

ModelAdmin.formfield_overrides on ManyToManyField isn't compatible with CheckboxSelectMultiple

Reported by: nrogers64 Owned by: Adonys Alea Boffill
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 12 months ago.
Fixed add related object in admin popup for checkbox/radio
ticket_27487.diff (39.7 KB) - added by Adonys Alea Boffill 11 months ago.
New patch with the corrections related to template-based widget rendering.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 12 months ago by Tim Graham

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 Changed 12 months ago by Adonys Alea Boffill

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

comment:3 Changed 12 months ago by Adonys Alea Boffill

Has patch: set
Last edited 5 months ago by Tim Graham (previous) (diff)

comment:4 Changed 12 months ago by Adonys Alea Boffill

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 Changed 12 months ago by Tim Graham

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.

Changed 12 months ago by Adonys Alea Boffill

Attachment: #27487.diff added

Fixed add related object in admin popup for checkbox/radio

comment:6 Changed 12 months ago by Adonys Alea Boffill

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 Changed 11 months ago by Adonys Alea Boffill

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.

Changed 11 months ago by Adonys Alea Boffill

Attachment: ticket_27487.diff added

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

comment:8 Changed 5 months ago by Tim Graham

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 Changed 5 months ago by Tim Graham

Patch needs improvement: set
Note: See TracTickets for help on using tickets.
Back to Top