Prevent having "None" in the placeholder

[Benbb96]

I had None set as a placeholder on my ModelSelect2MultipleWidget because I initialize the widget through the Meta class of my form and I don't explicitly set the empty_label on my field.

So I think it should check if the empty_label is set before assigning it to the placeholder.

This issue is linked to #565

[codecov-io]

Codecov Report

Merging #591 into master will not change coverage by %. The diff coverage is n/a.

@@            Coverage Diff            @@
##            master      #591   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines            8         8           
=========================================
  Hits             8         8           

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 39ab326...d042241. Read the comment docs.

[Benbb96]

Maybe the change should happen in line 83 : default_attrs['data-placeholder'] = self.empty_label if self.empty_label or ''

[codingjoe]

@Benbb96 thanks for the contribution. I am not quite certain about this change though. Why are is your fields empty_label None instead of ""? Wouldn't it be more precise to set the fields label to whatever should be displayed? I will close the PR for now and repoen it if need be, ok?

[Benbb96]

Okay so here is an example of my code when None appears in the placeholder :

class OrderForm(forms.ModelForm):
    class Meta:
        model = Order
        fields = ('status', 'customer_contacts')
        widgets = {
            'status': forms.Select(attrs={'class': 'form-control'}),
            'customer_contacts': ModelSelect2MultipleWidget(
                model=User,
                search_fields=['username__icontains', 'first_name__icontains', 'last_name__icontains']
            ),
        }

And next is the way I have it working without applying the patch I submitted :

class OrderForm(forms.ModelForm):
    customer_contacts = forms.ModelMultipleChoiceField(
        queryset=User.objects.all(),
        widget=ModelSelect2MultipleWidget(
            model=User,
            search_fields=['username__icontains', 'first_name__icontains', 'last_name__icontains']
        )
    )

    class Meta:
        model = Order
        fields = ('status', 'customer_contacts')
        widgets = {
            'status': forms.Select(attrs={'class': 'form-control'})
        }

_Why do I have to explicitly override the field customer_contacts to make it work ?_

Note : I didn't have this issue before this commit.

[codingjoe]

Hi @Benbb96,

thanks for responding so quickly. I had a look at the Django code, to figure out why the label is None. That seem to be the case when a field is not required, which I honestly was not aware of. Anyhow, I believe the reason why you are seeing None is placeholder of the placeholder. https://github.com/applegrew/django-select2/blob/39ab326884c5bcb84f534a883aa204089c19dd53/django_select2/forms.py#L83 Can you test if replacing the line with

default_attrs['data-placeholder'] = self.empty_label or ""

solves your problem?

If so, I am happy to accept this as a patch.

Best -Joe

[Benbb96]

Hi @codingjoe,

You're right, if I set my field as required, then the placeholder isn't None.

I've tested with your proposal and it also works so I updated my code on my fork.
Should I make another Pull Request ?

[codingjoe]

Just update it and maybe add test if possible. Ping me when you are done and I'll review an release it 👍

[Benbb96]

Hi @codingjoe

I added a test and pushed it onto my fork, but I'm not sure if you are able to see it from here since the pull request is closed ?

I've added my test into the test_empty_option function (in test_forms.py) because I wanted to use the AlbumSelect2MultipleWidgetForm which has the non required field featured_artists and produces the bug without my patch. Maybe you prefer to put the test in a separate function ?

[codingjoe]

Released in 7.2.1 :tada: