Opened 6 years ago

Closed 4 years ago

Last modified 4 years ago

#30231 closed Bug (fixed)

Field's verbose_name is ignored in FilteredSelectMultiple widget for inlines created with "Add another" button

Reported by: Florian Schuler Owned by: David Smith
Component: contrib.admin Version: 2.1
Severity: Normal Keywords:
Cc: Florian Schuler, Carlton Gibson Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: yes

Description

When using the FilteredSelectMultiple widget (filter_horizontal) in a TabularInline view on a ModelAdmin page the "verbose_name" of a ManyToManyField is ignored for newly added objects via the "add another" button. It simply displays the field name for those elements. The FilteredSelectMultiple widgets of initially loaded / already existing objects is displayed correctly with the use of the "verbose_name" of the regarding field.

It seems like a small JS Bug, because when checking out the generated HTML source code, the "select"-element has always the correct "data-field-name" attribute which fits to the "verbose_name" (for the newly added objects too). But when the FilteredSelectMultiple widget of newly added objects is initialized the generation of the heading and the help texts seems to ignore this "data-field-name" attribute / the "verbose_name" and instead uses simply the field name.

See attached screenshot for explanation.

Attachments (1)

screenshot.jpg (116.0 KB ) - added by Florian Schuler 6 years ago.
Screenshot which shows wrong display of field verbose_name

Download all attachments as: .zip

Change History (23)

by Florian Schuler, 6 years ago

Attachment: screenshot.jpg added

Screenshot which shows wrong display of field verbose_name

comment:1 by Florian Schuler, 6 years ago

Cc: Florian Schuler added

comment:2 by Ahisahar Pretel, 6 years ago

Owner: changed from nobody to Ahisahar Pretel
Status: newassigned

comment:3 by Florian Schuler, 6 years ago

Hey Ahisahar, were you already able to reproduce the bug on your side?

comment:4 by Ahisahar Pretel, 6 years ago

Hello @Florian Schuler, actually not. It happens in admin.TabularInline view or in the admin modelview once that we have a form with FilteredSelectMultiple?

Thank you

comment:5 by Tim Graham, 6 years ago

Easy pickings: unset
Summary: "verbose_name" is ignored in FilteredSelectMultiple widget for new objectsField's verbose_name is ignored in FilteredSelectMultiple widget for inlines created with "Add another" button
Triage Stage: UnreviewedAccepted

I reproduced at 9681e968ebdcd58cac99c1e60f0a6932abd4e5c9 with this patch on the tutorial:

  • polls/admin.py

    diff --git a/polls/admin.py b/polls/admin.py
    index f1bf573..0a184ab 100644
    a b from .models import Choice, Question  
    66class ChoiceInline(admin.TabularInline):
    77    model = Choice
    88    extra = 3
     9    filter_horizontal = ['groups']
    910
    1011
    1112class QuestionAdmin(admin.ModelAdmin):
  • polls/models.py

    diff --git a/polls/models.py b/polls/models.py
    index 38ae549..b0756e3 100644
    a b import datetime  
    44
    55from six import python_2_unicode_compatible
    66
     7from django.contrib.auth.models import Group
    78from django.db import models
    89from django.utils import timezone
    910
    class Choice(models.Model):  
    2930    question = models.ForeignKey(Question, on_delete=models.CASCADE)
    3031    choice_text = models.CharField(max_length=200)
    3132    votes = models.IntegerField(default=0)
     33    groups = models.ForeignKey(Group, blank=True, verbose_name='test')
    3234
    3335    def __str__(self):
    3436        return self.choice_text

in reply to:  5 comment:6 by Florian Schuler, 6 years ago

Replying to Ahisahar Pretel:

Hello @Florian Schuler, actually not. It happens in admin.TabularInline view or in the admin modelview once that we have a form with FilteredSelectMultiple?

Thank you

Like Tim Graham just confirmed it happens in admin.TabularInline view, when adding a field to "filter_horizontal". In addition I already tried to overwrite get_formset method of the regarding admin.TabularInline and added FilteredSelectMultiple widget manually with verbose_name redefined on the widget instead, but with the same result. So the bug occurs with manually added FilteredSelectMultiple widgets too.

@Tim Graham thank you for the confirmation of the bug and clarifying the title of the ticket! Of course it is important that this only happens to inlines added via the "add another" button.

comment:7 by Ahisahar Pretel, 6 years ago

I should probably let it to someone who has more experience right @Florian Schuler @Tim Graham? This is my first contribution

in reply to:  7 comment:8 by Florian Schuler, 6 years ago

Replying to Ahisahar Pretel:

I should probably let it to someone who has more experience right @Florian Schuler @Tim Graham? This is my first contribution

Hey @Ahisahar Pretel and @Tim Graham, I just found the origin of this wrong behaviour and fixed it in my local Django system (2.1.7)!
Like already noted in my bug report it seems like a small logic fault in a javascript file. In the file which is responsible for initializing the FilteredSelectMultiple widget at page load (django/contrib/admin/static/admin/js/SelectFilter2.js) there is the correct usage of calling the SelectFilter.init() method at the very end (line 244 to 250):

    window.addEventListener('load', function(e) {
        $('select.selectfilter, select.selectfilterstacked').each(function() {
            var $el = $(this),
                data = $el.data();
            SelectFilter.init($el.attr('id'), data.fieldName, parseInt(data.isStacked, 10));
        });
    });

So second parameter has to be the data attribute fieldName of the HTML element, which represents the verbose_name if one is given for the field.

In the js file which is responsible for triggering the initialization of newly added FilteredSelectMultiple Widgets after clicking the "add another" button (django/contrib/admin/static/admin/js/inlines.js) this parameter isn't set correctly.

For tabular inlines:

Current function:

        var updateSelectFilter = function() {
            // If any SelectFilter widgets are a part of the new form,
            // instantiate a new SelectFilter instance for it.
            if (typeof SelectFilter !== 'undefined') {
                $('.selectfilter').each(function(index, value) {
                    var namearr = value.name.split('-');
                    SelectFilter.init(value.id, namearr[namearr.length - 1], false);
                });
                $('.selectfilterstacked').each(function(index, value) {
                    var namearr = value.name.split('-');
                    SelectFilter.init(value.id, namearr[namearr.length - 1], true);
                });
            }
        };

So it gets the field name out of the "name" attribute which is the real field name, not the verbose name which could be easily taken with "$(this).data('fieldName')"

Corrected function should be:

        var updateSelectFilter = function() {
            // If any SelectFilter widgets are a part of the new form,
            // instantiate a new SelectFilter instance for it.
            if (typeof SelectFilter !== 'undefined') {
                $('.selectfilter').each(function(index, value) {
                    SelectFilter.init(value.id, $(this).data('fieldName'), false);
                });
                $('.selectfilterstacked').each(function(index, value) {
                    SelectFilter.init(value.id, $(this).data('fieldName'), true);
                });
            }
        };

The same for Stacked inline:

Current function:

 var updateSelectFilter = function() {
            // If any SelectFilter widgets were added, instantiate a new instance.
            if (typeof SelectFilter !== "undefined") {
                $(".selectfilter").each(function(index, value) {
                    var namearr = value.name.split('-');
                    SelectFilter.init(value.id, namearr[namearr.length - 1], false);
                });
                $(".selectfilterstacked").each(function(index, value) {
                    var namearr = value.name.split('-');
                    SelectFilter.init(value.id, namearr[namearr.length - 1], true);
                });
            }
        };

Corrected function should be:

var updateSelectFilter = function() {
            // If any SelectFilter widgets were added, instantiate a new instance.
            if (typeof SelectFilter !== "undefined") {
                $(".selectfilter").each(function(index, value) {
                    SelectFilter.init(value.id, $(this).data('fieldName'), false);
                });
                $(".selectfilterstacked").each(function(index, value) {
                    SelectFilter.init(value.id, $(this).data('fieldName'), true);
                });
            }
        };

At least on my side everything is working correctly with these changes.
Maybe one of you could recheck my changes and test it on a clean project if everything is still working like expected and maybe it would also be a good idea to search for more SelectFilter.init() calls at other places in the code?! Don't know if it is anywhere else wrongly used.
It would also be good to check if the data attribute fieldName is guaranteed to be set, but because it is used in this way in the SelectFilter2.js file i would think so... But i still wonder why it was wrongly implemented in the inlines.js file (maybe there was a change someday at the generation of the selectfilter elements and the fieldName attribute was added later and no one thought about changing it in inlines.js file too?!).

@Ahisahar Pretel or @Tim Graham could any of you take care of checking this out and submitting a patch?

comment:9 by Florian Schuler, 6 years ago

Owner: changed from Ahisahar Pretel to Florian Schuler

comment:10 by Florian Schuler, 6 years ago

Just submitted a patch with the changes i already mentioned above and made a pull request.
https://github.com/django/django/pull/11076

comment:11 by Florian Schuler, 6 years ago

Has patch: set

comment:12 by Tim Graham, 6 years ago

Needs tests: set

Tests (probably using selenium in this case) are also required.

in reply to:  12 comment:13 by Florian Schuler, 6 years ago

Replying to Tim Graham:

Tests (probably using selenium in this case) are also required.

Hey Tim, I don't know what tests could be useful in this case of UI bug where only a wrong name was passed to display (it's a very trivial bugfix, simply fetching the name from fieldName attribute instead). I don't have any experience in writing tests using selenium for Django UI and also don't have any time for reading into it and setting up a testing environment at the moment. So if there really should be tests needed, someone else needs to take care of it.

comment:14 by Tim Graham, 6 years ago

Owner: Florian Schuler removed
Status: assignednew

Okay, I'll deassign the ticket then. Even trivial bug fixes require tests to prevent regressions.

in reply to:  14 comment:15 by Florian Schuler, 6 years ago

Replying to Tim Graham:

Okay, I'll deassign the ticket then. Even trivial bug fixes require tests to prevent regressions.

Hey Tim, Okay!
Just noticed that the owner field of this ticket is now empty. Shouldn't it be "nobody" like the other new tickets so that other users can query for unassigned tickets by searching for owner "nobody"?

comment:16 by Teresa Partida, 6 years ago

Owner: set to Teresa Partida
Status: newassigned

comment:17 by Teresa Partida, 6 years ago

Hi! I submitted a new patch with the changes Florian made and the Selenium test
https://github.com/django/django/pull/11282

comment:18 by Mariusz Felisiak, 4 years ago

Cc: Carlton Gibson added
Needs tests: unset

comment:19 by David Smith, 4 years ago

Owner: changed from Teresa Partida to David Smith

comment:20 by Carlton Gibson, 4 years ago

Triage Stage: AcceptedReady for checkin

comment:21 by Carlton Gibson <carlton.gibson@…>, 4 years ago

Resolution: fixed
Status: assignedclosed

In 45bef670:

Fixed #30231 -- Fixed admin filter horizontal/vertical verbose_name generation.

Co-authored-by: David Smith <smithdc@…>

comment:22 by GitHub <noreply@…>, 4 years ago

In 96a50934:

Refs #30231 -- Fixed SeleniumTests.test_inlines_verbose_name with headless mode.

Horizontal scrollbar doesn't appear with the headless mode on small
windows, that's why window.scrollTo() is not an option for these tests
even after fixing #32459.

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