Opened 6 years ago

Closed 3 years ago

#13970 closed New feature (fixed)

SelectDateWidget should be updated to use new is_required Widget attribute

Reported by: Mitar Owned by: nobody
Component: Forms Version: master
Severity: Normal Keywords:
Cc: mmitar@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Overridden widgets in ModelForm do not respect blank (not required) flag of the model field because they are initialized in advance and already made object is passed for the widget.

The problem is for example with SelectDateWidget which is by default configured as required. So I have to manually set required constructor argument to False for model fields which are not required. This obviously contradicts DRY concept.

I propose that if required argument is not passed to the constructor explicitly then default should be taken from the model field and not that it is True by default.

Change History (12)

comment:1 Changed 6 years ago by Carl Meyer

Resolution: invalid
Status: newclosed

Since you haven't provided any sample code, it's hard to know for sure what you're seeing. But if I understand correctly, this is behaving exactly as documented: see http://docs.djangoproject.com/en/dev/topics/forms/modelforms/#overriding-the-default-field-types-or-widgets

If you override a model-based field entirely with your own field, you are responsible for setting all its attributes as you want them.

If you are only changing the widget, use the "widget" attribute of the inner Meta class, as documented in that same section, instead of overriding the entire field, and you won't see this problem.

If you are making other small tweaks to a field, you can override the init method of the ModelForm and make tweaks there, also without completely overriding a field.

If I misunderstand what you are doing, reopen with code examples demonstrating the problem.

comment:2 in reply to:  1 ; Changed 6 years ago by Mitar

Replying to carljm:

If you are only changing the widget, use the "widget" attribute of the inner Meta class, as documented in that same section, instead of overriding the entire field, and you won't see this problem.

No, exactly this is the problem. I also thought so. That I will change only widget and everything will work. But see, when changing the widget you initialize it to an object. And so later on required value on a widget is not set properly based on a model field. For example I had to do this:

class UserProfileChangeForm(forms_models.ModelForm):
  class Meta:
    model = models.UserProfile
    widgets = {
      'birthday': widgets.SelectDateWidget(
        years=range(datetime.date.today().year, 1900, -1),
	required=(not filter(lambda x: x.name == 'birthday', models.UserProfile._meta.fields)[0].blank),
      ),
    }

So SelectDateWidget takes required argument. Once you do SelectDateWidget() ModelForm takes the widget as it is. And required argument is by default set to True. What I am proposing is that if I do:

widgets = {
  'birthday': widgets.SelectDateWidget(required=True),
}

Widget should behave as required.

If I do:

widgets = {
  'birthday': widgets.SelectDateWidget(required=False),
}

Widget should behave as not required.

But if I do:

widgets = {
  'birthday': widgets.SelectDateWidget(),
}

Widget should not be by default required but that of the model field.

comment:3 Changed 6 years ago by Mitar

Resolution: invalid
Status: closedreopened

comment:4 in reply to:  2 Changed 6 years ago by Ramiro Morales

Triage Stage: UnreviewedDesign decision needed

Replying to mitar:

What I am proposing is that [...] if I do:

widgets = {
  'birthday': widgets.SelectDateWidget(),
}

Widget should not be by default required but that of the model field.

Problem is that the widget has no access to its (model)form field or the model field that originates it so I don't think this strategy will be implementable.

comment:5 Changed 6 years ago by Carl Meyer

Summary: Overridden widgets in ModelForm do not respect blank flag of model fieldSelectDateWidget should be updated to use new is_required Widget attribute
Triage Stage: Design decision neededAccepted
Version: 1.2SVN

This is a much more localized problem than I thought at first. It only affects SelectDateWidget; no other form widget takes a "required" argument. In Django 1.2, no other form widget even cares whether it's required or not.

In trunk / 1.3, ClearableFileInput also needs to know whether it is rendering as required or not, and for that purpose the generalized Widget.is_required attribute was introduced. This attribute is not set via an argument to the widget, it is set directly by the Field that owns the widget at Field-initialization time. Because ModelForm.Meta.widgets takes effect as an argument to the creation of the Field, is_required will be set correctly on a widget defined via ModelForm.Meta.widgets; this bug does not occur.

So in the end, for trunk, the only fix that needs to be made here is to update SelectDateWidget to use the new is_required attribute, and deprecate its "required" argument. Updating the summary accordingly.

comment:6 Changed 6 years ago by Carl Meyer

I should also have mentioned: since the suggested fix is only possible in trunk, given the limited scope of the bug, the issue Ramiro mentioned (that widgets have no knowledge of their owning field) and that a workaround (if lengthy and ugly) is available as demonstrated above, I don't think a backport to 1.2.X is reasonable or necessary here.

comment:7 Changed 6 years ago by Graham King

Severity: Normal
Type: New feature

comment:8 Changed 5 years ago by anonymous

Easy pickings: unset
UI/UX: unset

I think this widget's parameter only means, whether the kind of "empty" fields added, or not.

comment:9 Changed 4 years ago by Aymeric Augustin

Status: reopenednew

comment:10 Changed 3 years ago by Claude Paroz

Has patch: set

comment:11 Changed 3 years ago by Tim Graham

Triage Stage: AcceptedReady for checkin

comment:12 Changed 3 years ago by Claude Paroz <claude@…>

Resolution: fixed
Status: newclosed

In bc21e9c0d9253f9d49a0063830a645d1c724c696:

Fixed #13970 -- Made SelectDateWidget use the standard widget is_required attribute

Thanks mitar for the report and Tim Graham for the review.

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