[Django] #16328: FilePathField should include blank option even when required=False

[Django]

#16328: FilePathField should include blank option even when required=False
------------------------+----------------------------
Reporter: ringemup@… | Owner: nobody
Type: Bug | Status: new
Milestone: | Component: Forms
Version: 1.3 | Severity: Normal
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Easy pickings: 1
UI/UX: 1 |
------------------------+----------------------------
Because there is no blank option in FilePathField form fields when
required=True, it is not possible to save an admin form with blank inlines
that contain FilePathFields. Since an empty-string option does not pass
the required=True validator, the simplest fix is simply to include a
('','----------') option as the first choice for the field, just as with
other ChoiceFields.

--
Ticket URL: <https://code.djangoproject.com/ticket/16328>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#16328: FilePathField should include blank option even when required=True
--------------------------------------+------------------------

Reporter: ringemup@… | Owner: nobody
Type: Bug | Status: new
Milestone: | Component: Forms
Version: 1.3 | Severity: Normal

Resolution: | Keywords:

Triage Stage: Unreviewed | Has patch: 0

Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 1
UI/UX: 1 |
--------------------------------------+------------------------
Changes (by anonymous):

* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0

--
Ticket URL: <https://code.djangoproject.com/ticket/16328#comment:1>

[Django]

#16328: FilePathField should include blank option even when required=True
--------------------------------------+------------------------

Reporter: ringemup@… | Owner: nobody
Type: Bug | Status: new
Milestone: | Component: Forms
Version: 1.3 | Severity: Normal

Resolution: | Keywords:
Triage Stage: Accepted | Has patch: 0

Needs documentation: 0 | Needs tests: 0

Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 1 |
--------------------------------------+------------------------
Changes (by aaugustin):

* easy: 1 => 0
* stage: Unreviewed => Accepted

Comment:

It took me a bit of time to figure out the problem; here's my analysis.

Django's forms have an (undocumented) `empty_permitted` attribute. When
this attribute is set to `True`, validation is short-circuited (see lines
263-266 in django/forms/forms.py). Formsets need this internal API to
display extra forms to add objects, but ignore them if they are submitted
unchanged (empty). Specifically, the inlines feature of the admin uses
this.

However, it isn't possible to submit a formset unchanged when it contains
a `FilePathField`. This problem may affect other fields that can't be
submitted with an empty value, given the UI (radio buttons, drop-down
selects), when `blank=False`. The common point of these fields is that
their value must be chosen from a finite set defined by their `choices`
attribute.

In my opinion, the proper fix is to render fields as if `blank` was `True`
when `empty_permitted` is `True`, probably by setting their
`include_blank` attribute to `True`. Thus, the select widget for
`FilePathField` will contain the blank choice, `BLANK_CHOICE_DASH`, as the
first item, resolving the problem described originally.

Unfortunately, I failed to write a patch for this because I'm not
sufficiently familiar with the forms implementation. After searching for
all instances of `include_blank` and `empty_permitted`, I couldn't bridge
the gap between them. This isn't an easy picking after all :)

--
Ticket URL: <https://code.djangoproject.com/ticket/16328#comment:2>

[Django]

#16328: FilePathField should include blank option even when required=True
----------------------------+------------------------------------------
Reporter: ringemup@… | Owner: Mahmoud Nasser
Type: Bug | Status: assigned
Component: Forms | Version: 1.3

Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 1

----------------------------+------------------------------------------
Changes (by Mahmoud Nasser):

* owner: nobody => Mahmoud Nasser
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/16328#comment:3>

[Django]

#16328: FilePathField should include blank option even when required=True
----------------------------+------------------------------------------
Reporter: ringemup@… | Owner: Mahmoud Nasser

Type: Bug | Status: closed
Component: Forms | Version: 1.3
Severity: Normal | Resolution: fixed

Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 1
----------------------------+------------------------------------------
Changes (by Mahmoud Nasser):

* resolution: => fixed
* status: assigned => closed

Comment:

I already tested this ticket and the blank option is working fine.

I made the test with the following models:

{{{
class Parent(models.Model):
name = models.CharField(max_length=100)

def __str__(self):
return self.name


class Child(models.Model):
parent = models.ForeignKey(Parent, on_delete=models.CASCADE,
related_name="children")
# FilePathField with blank=True but required by form — we simulate it
file_path = models.FilePathField(path="/tmp", match=".*\.txt$",
recursive=False, blank=True)
choices_field = models.CharField(max_length=100, choices=[("name",
"name")])

def __str__(self):
return self.file_path or "No File"
}}}

And with the following admin:

{{{
# admin.py
from django.contrib import admin
from .models import Parent, Child

class ChildInline(admin.TabularInline):
model = Child
extra = 1 # Shows one empty inline form
can_delete = True

@admin.register(Parent)
class ParentAdmin(admin.ModelAdmin):
inlines = [ChildInline]
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/16328#comment:4>

[Django]

#16328: FilePathField should include blank option even when required=True
----------------------------+------------------------------------------
Reporter: ringemup@… | Owner: Mahmoud Nasser

Type: Bug | Status: new
Component: Forms | Version: 1.3
Severity: Normal | Resolution:

Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 1
----------------------------+------------------------------------------

Changes (by Sarah Boyce):

* resolution: fixed =>
* status: closed => new

Comment:

If you believe this is fixed, this still might need a test to make sure
this doesn't break in future.
We likely want to bisect to when this got fixed and to have others confirm
that this is indeed fixed.
Once this has test coverage, we can close the ticket.
--
Ticket URL: <https://code.djangoproject.com/ticket/16328#comment:5>

[Django]

#16328: FilePathField should include blank option even when required=True
----------------------------+------------------------------------------
Reporter: ringemup@… | Owner: Mahmoud Nasser
Type: Bug | Status: new

Component: Forms | Version: 1.3

Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 1
----------------------------+------------------------------------------

Comment (by Tim Graham):

Mahmoud, I don't think you reproduced this correctly. In your test model,
you have `FilePathField(..., blank=True)` which suggests it would have
`required=False` (rather than `True`) in forms.
--
Ticket URL: <https://code.djangoproject.com/ticket/16328#comment:6>

[Django]

#16328: FilePathField should include blank option even when required=True
----------------------------+------------------------------------------
Reporter: ringemup@… | Owner: Mahmoud Nasser
Type: Bug | Status: new

Component: Forms | Version: 1.3

Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 1
----------------------------+------------------------------------------

Comment (by Mahmoud Nasser):

In the `FilePathField` we have the following line of code:

{{{
if self.required:
self.choices = []
else:
self.choices = [("", "---------")]
}}}

and when we pass the parmater from the model to the form we assign the
`required` parameter based on `blank` that comming from the model as you
could see here:


{{{
def formfield(self, form_class=None, choices_form_class=None,
**kwargs):
"""Return a django.forms.Field instance for this field."""
defaults = {
"required": not self.blank,
"label": capfirst(self.verbose_name),
"help_text": self.help_text,
}
......
}}}

What I am planing to do is to cover this lines of code in the unittests as
you could see in this commit:
https://github.com/django/django/commit/a64a61bf4a which made the change
in the `FilePathField` as I mentioned above , I will add tests for that
change like this in `test_filepathfield.py` to confirm that it is working
fine:


{{{
def test_should_include_blank_option_when_required_is_true(self):
f = FilePathField(path=self.path, match=r"^.*?\.py$",
required=False)
self.assertChoices(f, [('', '---------')] +
self.expected_choices[:4])
}}}


if it is ok I will do a PR for above change.
--
Ticket URL: <https://code.djangoproject.com/ticket/16328#comment:7>