MultiValueField subwidgets required

smithdc1 · February 18, 2020, 9:27pm

Hi all,

I’ve been learning programming over the last year and have taken an interest in Python & web development and therefore Django. My open source contributions thus far have been with crispy-forms but I’m now plucking up the courage to look at Django itself.

I’ve been doing some research into ticket #29205.

In summary the required status of MultiValueField itself is used in some circumstances rather than the subwidgets. This leads to some unexpected behaviour as outlined in the ticket. I’m happy to investigate further but I’m a bit unsure of what the expected outcome is.

What is the expected behaviour where some subwidgets are optional, e.g for help text, validation warnings etc. Just saying ‘this field is required’ doesn’t really work in this case.

Apologies if this isn’t quite the right place to ask this question!

Thanks

David

carltongibson · February 20, 2020, 9:15am

Hi David!

This is absolutely the right place. I missed your comment on the ticket yesterday.

I’ve commented on the ticket, more or less:

Required on the MWF should mean “Can this set of sub-fields be skipped entirely?”.
Assuming No to that, i.e. that the MWF is required, then the individual fields should respect their own required attributes.

It’s worth throwing an error into MWF to see which existing test cases are exercising it, and seeing what they have to say here. Maybe nothing. It could well be previously undefined. (Either way the current behaviour is incorrect so…)

smithdc1 · February 22, 2020, 7:45pm

Hi Carlton,

Thanks for your response! If it’s OK can we carry on a bit of a detailed conversation here, the salient points can then be added to the ticket?

Thank you for pointing me in the direction of the tests, this was very helpful. I also appreciated your insight in that if a field has required elements then it should defer to the sub-fields, this seems a logical approach.

I found test_multivalue_optional_subfields. This test is for the scenarios we are looking at. This also led me to the original ticket (#15511) that introduced this functionality. It shows that required_all_fields was introduced as a flag to allow sub-widgets to be optional whilst retaining backwards compatibility.

The docs also have something to say on required_all_fields.

When set to False, the Field.required attribute can be set to False for individual fields to make them optional. If no value is supplied for a required field, an incomplete validation error will be raised.

My view is that the current implementation doesn’t quite work in this way as an empty response is valid; I think the docs would indicate it should raise an incomplete error.

Given this, I think there are two potential solutions for the clean method.

Option 1: required = false & require_all_fields=false & null response = incomplete error

This option would only change the validation outcome of a small set of test cases. Specifically those where a valid input is currently being returned for a null response despite there being required sub-fields. These tests are line 3034-3036 and I would expect these to raise a ValidationError.

The benefit of this option is it’s a fairly small change from the current behaviour and simply aligns to the description in the documentation.

Option 2: required = false assumes field can be empty in it’s entity; required=true defers to sub-fields

In this solution I propose we remove require_all_fields as the logic would flow down to individual sub-fields when required=true on MVF. Whilst this option seems a cleaner implementation this would be a more fundamental change and causes backwards compatibility issues.

Therefore given where the project is at, is this a solution that we should proceed with? If so I’d appreciate some guidance on how to approach this. In my head we could introduce ‘new’ way and slowly deprecate the ‘old’ way?

HTML5 Required attribute

In the ticket I also commented about the HTML5 required attribute. There is a fairly easy solution here in that it can be overridden by passing the required setting into the attrs dict for each widget e.g.

widgets = [TextInput(attrs={'required': True}), TextInput(attrs={'required': False})]

Having looked at the tests I can’t see any coverage of ‘required’ attributes for multivalue field.

We therefore have the option of trying to improve the default required settings by deferring to the subwidgets. In BoundField.build_widget_attrs() we could check if the field is a MVF, and add the required status based on each subfield rather than MVF itself. Note: Whilst logic above makes sense to me, I’ve not yet given any thought to how this would be implemented.

Questions

Assuming the above makes sense there are a couple of questions which will help me progress this.

Do you have a preference in how we should improve the clean method, either one of my options above or something I’ve not thought of?
Do you have a view on if investigating the build_widget_attrs logic to try and respect the required status of individual widgets is worthwhile?

Thanks

David

p.s. bit of feedback - you can post 2 links in a reply. I had to chop a fair few out to comply with this rule.

carltongibson · February 22, 2020, 8:49pm

Hey David.

Good work. I’ll look into it more in the week and get back to you. Preliminary thought is towards Option 1, less breakage being better, but I need to review the history to make sure the existing behaviour isn’t just correct if you hold it right…

Ref the two links: yes. It’s a spam prevention thing. I think once you’ve posted a few times that restriction drops.

smithdc1 · March 17, 2020, 9:48pm

Hi @carltongibson,

Hope you are well.

I’ve started to put something together for this.

Currently I’ve updated the tests so that a validation error is raised when required=False and require_all_fields=False. I’ve also come up with a solution that results in these tests being passed.

The next thing for me to take a look at is the html5 required attributes.

David

smithdc1 · March 24, 2020, 9:14pm

Hi All,

I’ve spent some time this evening investigating how to set html5 required attributes based upon the attribute of each subwidget. I’ve hit a stage where I need a bit of help with being pointed in the right direction.

I started to look at as_widget which calls build_widget_attrs as this seems to be where the required attribute logic sits. My issue is that this only sets a required attribute for the widget, not sub widgets. When I then try and follow the debugger through widget.render I’m then getting a bit lost.

Appreciate any support & guidance.

github.com

django/django/blob/895f28f9cbed817c00ab68770433170d83132d90/django/forms/boundfield.py#L89


def as_widget(self, widget=None, attrs=None, only_initial=False):
    """
    Render the field by rendering the passed widget, adding any HTML
    attributes passed as attrs. If a widget isn't specified, use the
    field's default widget.
    """
    widget = widget or self.field.widget
    if self.field.localize:
        widget.is_localized = True
    attrs = attrs or {}
    attrs = self.build_widget_attrs(attrs, widget)
    if self.auto_id and 'id' not in widget.attrs:
        attrs.setdefault('id', self.html_initial_id if only_initial else self.auto_id)
    return widget.render(
        name=self.html_initial_name if only_initial else self.html_name,
        value=self.value(),
        attrs=attrs,
        renderer=self.form.renderer,
    )


def as_text(self, attrs=None, **kwargs):

github.com

django/django/blob/895f28f9cbed817c00ab68770433170d83132d90/django/forms/boundfield.py#L221


    @cached_property
    def initial(self):
        data = self.form.get_initial_for_field(self.field, self.name)
        # If this is an auto-generated default date, nix the microseconds for
        # standardized handling. See #22502.
        if (isinstance(data, (datetime.datetime, datetime.time)) and
                not self.field.widget.supports_microseconds):
            data = data.replace(microsecond=0)
        return data


    def build_widget_attrs(self, attrs, widget=None):
        widget = widget or self.field.widget
        attrs = dict(attrs)  # Copy attrs to avoid modifying the argument.
        if widget.use_required_attribute(self.initial) and self.field.required and self.form.use_required_attribute:
            attrs['required'] = True
        if self.field.disabled:
            attrs['disabled'] = True
        return attrs




@html_safe

carltongibson · March 25, 2020, 3:04pm

Hi David.

If you’re lost looking at it, we have no hope.

I can’t recall offhand what the desired behaviour is. (Did we get a clear grid of expected results?)
BUT if the parent needs to control the subwidget, it looks like it’ll have to decorate the sub widget’s use_required_attribute() method.

# Write the comments first, they said. 
# Get the method from the subwidget. 
# Wrap it. 
# Reattach.

That should work right?

If you open a PR with some tests, and as far as you get I’m happy to play. (It’s a bit difficult without code to run…)

Thanks for your efforts on this one!

Kind Regards,

Carlton

smithdc1 · March 29, 2020, 4:41pm

Hi @caroltongibson

Firstly, thanks for your support with this. Following your latest post I’ve made much more progress on this. Here is a pull request with tests and a working solution. Below are some notes and some of my thoughts.

Truth Table

a = required
b = require_all_fields
c = subfield.required
d = HTML5 required
e = Entire field not completed test
f = Partially completed test

a b c d e f [ref]
F | F | F | F | n/a | Valid - Test 4 [2]
F | F | T | T | Incomplete Error | Incomplete Error- Test 4 [1] [3]
F | T | F | F | Valid | Valid - Test 2
F | T | T | F | Valid | Valid - Test 2
T | F | F | F | n/a | Valid- Test 3 [2] [3]
T | F | T | T | Required Error | Incomplete Error - Test 3
T | T | F | T | Required Error | Required Error - Test 1
T | T | T | T | Required Error | Required Error - Test 1

abc((a ∧ (¬b ∧ c)) ∨ ((a ∧ b) ∨ (¬a ∧ (¬b ∧ c))))

Note :
[1] The ticket is to look at the logic for this scenario, when the entire field is not completed. The previous behaviour was that this is a valid response from clean. This pull request results in an incomplete error being raised.
[2] N/A shows where we don’t have test coverage. We have a mix of required/non required fields, so will always raise a required error associated with one of the required fields.
[3] The current behaviour is for HTML5 required to be dependant only upon MVF required (column A). This pull request changes the logic to follow column D. i.e. give a HTML5 required if the field is required to return a VALID response from clean.

Considerations

Clean
The change for the clean function to raise an incomplete error in fields.py is fairly straight forward. I don’t think my solution is too bad here. This is changed in my first commit on the pull request

HTML5 Required
Implementing this I found to be a bit more of a challenge, and most of the code changes is associated with this. Here are the options I considered:

build_widget_attrs in boundfield.py
This is the method I’ve used in one of the earlier commits on the PR. This works as it checks to see if it’s a multiwidget and it is sets the appropriate attr on the subwidget. This felt like the wrong approach, you have to work quite hard to get all the relevant pieces of logic. e.g. rely on widget.is_required being correct as this is set before MVF overwrites the field.required to False.
Change __init__ of MVF.

Currently the __init__ function sets field.required=False if required_all_fields=False.
My current view is that we should required logic here, as this would make the required variable for each field more relevant. We can then use this in boundfield.py to set the required attribute. This simplifies the logic in boundfield.py:

Other thoughts:

This sets attr attributes in bounfield.py, rather than returning a dictionary to pass to render. I’m not sure how to overcome this as we want to set required at a subwidget level. On your previous post you mentioned about decorating use_required_attribute; however, I’m not quite sure this is the right way to solve this. use_required_attribute is set to false if the widget shouldn’t use it, even if field is required. e.g.

github.com

django/django/blob/27746ab28ac88fc213456e63302b408312d95122/django/forms/widgets.py#L769


    template_name = 'django/forms/widgets/radio.html'
    option_template_name = 'django/forms/widgets/radio_option.html'




class CheckboxSelectMultiple(ChoiceWidget):
    allow_multiple_selected = True
    input_type = 'checkbox'
    template_name = 'django/forms/widgets/checkbox_select.html'
    option_template_name = 'django/forms/widgets/checkbox_option.html'


    def use_required_attribute(self, initial):
        # Don't use the 'required' attribute because browser validation would
        # require all checkboxes to be checked instead of at least one.
        return False


    def value_omitted_from_data(self, data, files, name):
        # HTML checkboxes don't appear in POST data if not checked, so it's
        # never known if the value is actually omitted.
        return False


    def id_for_label(self, id_, index=None):

This currently sets the logic in __init__ therefore it’s not that easy to update required or require_all_fields ‘on the fly’.
I’ve also created a branch which shows the html5 tests on master, for comparison purposes. Link
should I add more tests, e.g. a multivaluefield with a CheckboxSelectMultiple, or a formset?
I’m not so sure about the test for required it’s rather a lot of HTML to put into the test. Could count the number of required attributes but then it’s not that obvious what’s going on. I’ve left it as it is, as I think it’s easier to read at the moment.

I feel like I’ve made much progress but I could well be missing something obvious (I don’t know, what I don’t know…)

carltongibson · April 1, 2020, 11:03am

Hi @smithdc1 Good post! I’ve left some initial comments on the PR. Let’s continue there.
(Meanwhile I shall digest this too)