[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>

[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: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 1
----------------------------+------------------------------------------

Changes (by HarishBonu0):

* cc: HarishBonu0 (added)
* has_patch: 0 => 1

Comment:

PR submitted with regression tests to confirm the current behavior
described in this ticket:

https://github.com/django/django/pull/20812

The tests cover both admin inline submission and ModelFormSet behavior
involving FilePathField(blank=False). CI is passing.
--
Ticket URL: <https://code.djangoproject.com/ticket/16328#comment:8>

[Django]

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

Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 1
----------------------------+---------------------------------------

Changes (by blighj):

* needs_better_patch: 0 => 1
* owner: Mahmoud Nasser => Harish Bonu

* status: new => assigned

Comment:

The behaviour seen on this ticket is caused because FilePathField doesn't
have a blank 'Select an option' option inserted if it is required. Other
choice fields will insert this option regardless of whether the field is
marked required or not and the inline handling code knows to ignore a
formset if all required fields are sent blank.
Because FilePathField doesn't have this blank option it will always be set
to the first value, even in a new inline row. If there are no other
required fields on that inline form, it will create a new row, which is
what the regression test shows. If there are other required fields you
won't be able to save the form, as the other fields validation will fail,
which is what the reporter saw.

The fix is the same for both, FilePathField should always add the blank
option, similar to other fields.
{{{#!diff
diff --git a/django/forms/fields.py b/django/forms/fields.py
--- a/django/forms/fields.py
+++ b/django/forms/fields.py
@@ -1200,10 +1200,7 @@ class FilePathField(ChoiceField):
self.set_choices()

def set_choices(self):
- if self.required:
- self.choices = []
- else:
- self.choices = [("", get_blank_choice_label())]
+ self.choices = [("", get_blank_choice_label())]

if self.match is not None:
self.match_re = re.compile(self.match)
}}}


You can reproduce the broken behaviour with the following code

{{{#!python
# models.py
import os

from django.db import models

# Any directory containing at least a couple of files works; the app dir
will do.
FILES_DIR = os.path.dirname(__file__)


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


class BlankFalseChild(models.Model):
parent = models.ForeignKey(BlankFalseParent, on_delete=models.CASCADE)
path = models.FilePathField(path=FILES_DIR, match=r".*\.py$",
blank=False)
note = models.CharField(max_length=100) # second required field


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


class BlankTrueChild(models.Model):
parent = models.ForeignKey(BlankTrueParent, on_delete=models.CASCADE)
path = models.FilePathField(path=FILES_DIR, match=r".*\.py$",
blank=True)
note = models.CharField(max_length=100)


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


class ChoicesChild(models.Model):
parent = models.ForeignKey(ChoicesParent, on_delete=models.CASCADE)
choices_field = models.CharField(max_length=100, blank=False,

choices=[("name", "name")])

note = models.CharField(max_length=100)


class StandaloneChoices(models.Model):
choices_field = models.CharField(max_length=100, blank=False,

choices=[("name", "name")])

note = models.CharField(max_length=100)


class StandaloneRecord(models.Model):
path = models.FilePathField(path=FILES_DIR, match=r".*\.py$",
blank=False)
note = models.CharField(max_length=100)
}}}

{{{#!python

# admin.py
from django.contrib import admin

from .models import (
BlankFalseChild, BlankFalseParent,
BlankTrueChild, BlankTrueParent,
ChoicesChild, ChoicesParent,
StandaloneChoices, StandaloneRecord,
)


class BlankFalseChildInline(admin.StackedInline):
model = BlankFalseChild
extra = 1


class BlankTrueChildInline(admin.StackedInline):
model = BlankTrueChild
extra = 1


class ChoicesChildInline(admin.StackedInline):
model = ChoicesChild
extra = 1


@admin.register(BlankFalseParent)
class BlankFalseParentAdmin(admin.ModelAdmin):
inlines = [BlankFalseChildInline]


@admin.register(BlankTrueParent)
class BlankTrueParentAdmin(admin.ModelAdmin):
inlines = [BlankTrueChildInline]


@admin.register(ChoicesParent)
class ChoicesParentAdmin(admin.ModelAdmin):
inlines = [ChoicesChildInline]


admin.site.register(StandaloneChoices)
admin.site.register(StandaloneRecord)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/16328#comment:9>