Opened 5 years ago

Closed 17 months 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 follow-up: Changed 5 years ago by carljm

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to invalid
  • Status changed from new to closed

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 ; follow-up: Changed 5 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 5 years ago by mitar

  • Resolution invalid deleted
  • Status changed from closed to reopened

comment:4 in reply to: ↑ 2 Changed 4 years ago by ramiro

  • Triage Stage changed from Unreviewed to Design 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 4 years ago by carljm

  • Summary changed from Overridden widgets in ModelForm do not respect blank flag of model field to SelectDateWidget should be updated to use new is_required Widget attribute
  • Triage Stage changed from Design decision needed to Accepted
  • Version changed from 1.2 to SVN

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 4 years ago by carljm

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 4 years ago by graham_king

  • Severity set to Normal
  • Type set to New feature

comment:8 Changed 4 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 2 years ago by aaugustin

  • Status changed from reopened to new

comment:11 Changed 17 months ago by timo

  • Triage Stage changed from Accepted to Ready for checkin

comment:12 Changed 17 months ago by Claude Paroz <claude@…>

  • Resolution set to fixed
  • Status changed from new to closed

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