Opened 6 years ago

Closed 5 years ago

Last modified 4 years ago

#10427 closed (fixed)

Bound field needs an easy way to get form value

Reported by: toxik Owned by: SmileyChris
Component: Forms Version: master
Severity: Keywords: form field value
Cc: michal@…, gabor@…, viksit@…, paulschreiber@…, kmike84@…, proppy@…, matt@…, martin.paquette@…, wsartori@…, postal2600@…, dirleyrls@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

When you want to render your own forms in templates, without any sort of widgetery, you bump into an issue: it is impossible to get the actual value of a field in a succinct and clean way.

Looking at Django's code for figuring this out, it clearly is a non-trivial task, and one you can easily get wrong.

So, to remedy this, I suggest adding a value attribute to BoundField, which simply returns that exact value. This could be used in templates like so:

<input type="text" name="{{ form.username.name }}" value="{{ form.username.value }}">

I for one find this both intuitive and readable.

Attachments (9)

django-forms-value.2.diff (2.9 KB) - added by toxik 6 years ago.
Use empty string instead of None
django-forms-value.3.diff (3.0 KB) - added by toxik 6 years ago.
Remove useless change
django-forms-value.4.diff (3.0 KB) - added by toxik 6 years ago.
Use empty string even if value is from initial data
django-forms-value.5.diff (3.0 KB) - added by toxik 6 years ago.
Added documentation
django-forms-value.6.diff (4.4 KB) - added by toxik 6 years ago.
Added documentation (really)
django-forms-value.7.diff (4.3 KB) - added by toxik 6 years ago.
Against trunk r10454
django-forms-value.diff (4.7 KB) - added by toxik 6 years ago.
Against trunk r10650
django-display_value-r10755.patch (6.1 KB) - added by emes 6 years ago.
with "display_value' method added
django-display_value-r11298.patch (6.5 KB) - added by pdonis 6 years ago.
adds one-line change to formtools/preview.html template

Download all attachments as: .zip

Change History (45)

Changed 6 years ago by toxik

Use empty string instead of None

Changed 6 years ago by toxik

Remove useless change

Changed 6 years ago by toxik

Use empty string even if value is from initial data

comment:1 Changed 6 years ago by jacob

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

This already exists, it's just spelled {{ form.field.data }}`.

comment:2 Changed 6 years ago by toxik

  • Resolution invalid deleted
  • Status changed from closed to reopened

I'm going to take the liberty of reopening this ticket, because they're not the same.

f.value takes initial data into account (both on form level and field level), as well as data. It only uses data when the form is a bound form.

comment:3 follow-up: Changed 6 years ago by mtredinnick

Yeah, this doesn't already exist. All the initial computations can't be simulated in a form. I thought this would have been a dupe of something else, though, since I've asked at least three people on the mailing list to open a ticket when they've asked about it. Apparently "will to fix" doesn't match "will to complain". Thanks for going the next step, Mike (but, geez, dude... how many attempts does it take to get the patch correct?)

comment:4 Changed 6 years ago by mtredinnick

  • Needs documentation set
  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

The patch itself looks ok, although I'd get rid of the property. A method will do fine, since it indicates in Python code that there's some computation going on and still looks the same in a template.

Needs documentation, too.

comment:5 in reply to: ↑ 3 Changed 6 years ago by toxik

Replying to mtredinnick:

Yeah, this doesn't already exist. All the initial computations can't be simulated in a form. I thought this would have been a dupe of something else, though, since I've asked at least three people on the mailing list to open a ticket when they've asked about it. Apparently "will to fix" doesn't match "will to complain". Thanks for going the next step, Mike (but, geez, dude... how many attempts does it take to get the patch correct?)

It might be useful to note that I'm Ludvig Ericson, or lericson. (And would love for my Trac & Subversion username to change.)

It took some attempts because I was a bit quick on the draw, and I started noticing issues after starting to use it.

Replying to mtredinnick:

The patch itself looks ok, although I'd get rid of the property. A method will do fine, since it indicates in Python code that there's some computation going on and still looks the same in a template.

Needs documentation, too.

Regarding it being a property, I chose that to be consistent with the data property. I imagine it won't only be used from templates either, and my_form['field'].value() looks weird.

comment:6 follow-up: Changed 6 years ago by mtredinnick

Sorry about the name mixup, that was just me not concentrating. You choose your username; live with the decision.

Calling a function using parentheses doesn't look at all weird. Normal code doesn't need to access the data attribute (yes, it should also have been a method) on a BoundField, so that's not particular relevant in the public API.

Anyway, add documentation and I, or somebody, will bikeshed the rest at commit time.

Changed 6 years ago by toxik

Added documentation

Changed 6 years ago by toxik

Added documentation (really)

comment:7 in reply to: ↑ 6 Changed 6 years ago by toxik

  • Needs documentation unset
  • Patch needs improvement unset

I'm very sorry about attaching this massive amount of patches, but I figured using "replace existing" when there are already renamed patches would get confusing.

Replying to mtredinnick:

Calling a function using parentheses doesn't look at all weird. Normal code doesn't need to access the data attribute (yes, it should also have been a method) on a BoundField, so that's not particular relevant in the public API.

Yeah, sure, but then it should really be a verb/verb+noun, and not a noun. Like "get_value". I don't see the big win in adding asymmetry and yet not gaining anything; it could just as well be an actual instance attribute (and probably should be?)

Bikeshed away, however.

Changed 6 years ago by toxik

Against trunk r10454

Changed 6 years ago by toxik

Against trunk r10650

comment:8 Changed 6 years ago by emes

  • Cc michal@… added

comment:9 Changed 6 years ago by emes

Two issues here:

  1. Why doesn't it take "initial" attribute into account?
  2. With ChoiceField the db value is returned, not the display one, which is not useful within templates.

Changed 6 years ago by emes

with "display_value' method added

comment:10 Changed 6 years ago by emes

Replying to emes:

  1. Why doesn't it take "initial" attribute into account?

Ok, we're in bound form. Sorry for this question ;)

  1. With ChoiceField the db value is returned, not the display one, which is not useful within templates.

And here's the patch which does it, by adding "display_value" method which looks up choices for the matching description.

comment:11 Changed 6 years ago by gabor

  • Cc gabor@… added

comment:12 Changed 6 years ago by viksit

  • Cc viksit@… added

Second this solution!

Added cc.

comment:13 Changed 6 years ago by margieroginski

I've started using this patch - it's very useful (Thanks!). One thing I'm finding is that it display_value does not return a useful string in the case where the form field is a TypedChoiceField that is in a form that is a ModelForm. For example, if I have a model like this:

class Task(models.Model):
    PRIORITY_CHOICES = (
        (1, _('1 (Critical)')),
        (2, _('2 (High)')),
        (3, _('3 (Normal)')),
    )

    priority = models.IntegerField(choices=PRIORITY_CHOICES, blank=True, null=True,
                                   help_text=_('1 = Highest Priority, 5 = Low Priority'))

and the form is a ModelForm, like this:

class TaskForm(forms.ModelForm):
    class Meta:
        model = Task

When I get the display_value for the form field, I get

<django.utils.functional.__proxy__ object at 0x2a9b486f90>

Here's some pdb output that shows the problem:

(Pdb) type(bf)
<class 'django.forms.forms.BoundField'>

(Pdb) print bf
<select name="priority" id="id_priority">
<option value="">---------</option>
<option value="1">1 (Critical)</option>
<option value="2">2 (High)</option>
<option value="3">3 (Normal)</option>
<option value="4" selected="selected">4 (Low)</option>
<option value="5">5 (Very Low)</option>
</select>
(Pdb) bf.display_value
<django.utils.functional.__proxy__ object at 0x2a9b486f90>
(Pdb) 

I tried replicating this by creating a ChoiceField in my form, and in that case I don't get the same behavior, so this is somehow related to it being a ModelForm. For example, if in my TaskForm I define priority like this:

    priority = forms.ChoiceField(choices=PRIORITY_CHOICES)

In that case display_value returns the expected string:

(Pdb) bf.display_value
'4 (low)'

I'm familiar with a lot of the django source, but it's not clear to me what this proxy is or why it is there, so I just wanted to report the issue, since I'm sure the intended behvior is not to have display_value return the proxy string.

Anyway, aside from this small problem, I think this is very nice refactoring of the code.

Margie

comment:14 Changed 6 years ago by Alex

display_value returns a transalatable string because you have it marked for translation(the _ which are ugettext_lazy calls). To convert it to a string (which also translates it), use force_unicode from django.utils.encoding.

comment:15 Changed 6 years ago by margieroginski

Ah ... I see! Sorry, my mistake. You know, I have been ignorant of all those little '_' floating around my code which I don't even need. Thanks for clarifying Alex. I have read up on the whole internationalization scheme and your response makes sense, of course. So my report above and assessment of the problem was obviously wrong - so developers, just ignore it. Sorry for the extra noise.

Margie

comment:16 Changed 6 years ago by Michael Stevens <mstevens@…>

This would be very handy for me having spent hours trying and failing to do related things.

comment:17 Changed 6 years ago by pdonis

I'm using this patch and it's working well for me (in fact, I was in the process of re-inventing this fix for myself when I found this ticket). Just one comment: with the display_value method available, one obvious place to use it would be in the preview template used by contrib.formtools.preview.FormPreview. It would be a simple one-line change: instead of:

field.data

it would say:

field.display_value

I'm attaching a patch against the current trunk (r11298) that's identical to the r10755 patch except that it adds the above one-line change. I realize that most users probably won't directly use the formtools templates as-is, but I expect many will look at them for guidance (I know I did), so it seems to me that they should reflect the "best practice".

Changed 6 years ago by pdonis

adds one-line change to formtools/preview.html template

comment:18 Changed 6 years ago by simple10

Alternatively, you can use the following template filter until this patch is added to Django core.

@register.filter(name='field_value')
def field_value(field):
    value = field.form.initial[field.name]
    if not value:
        return ''
    if isinstance(field.field, ChoiceField):
        value = str(value)
        for (val, desc) in field.field.choices:
     	    if val == value:
     	        return desc
    return value

Usage:

{{ form.field|field_value }}
{{{


}}}

comment:19 Changed 6 years ago by zanuxzan

As taken from the last patch, I think if your going to do this with a template tag it should be as follows;

@register.filter(name='field_value')
def field_value(field):
    """ 
    Returns the value for this BoundField, as rendered in widgets. 
    """ 
    if not field.form.is_bound: 
        val = field.form.initial.get(field.name, field.field.initial) 
        if callable(val): 
            val = val() 
    else: 
        if isinstance(field.field, FileField) and field.data is None: 
            val = field.form.initial.get(field.name, field.field.initial) 
        else: 
            val = field.data 
    if val is None: 
        val = '' 
    return val

@register.filter(name='display_value')
def display_value(field): 
    """ 
    Returns the displayed value for this BoundField, as rendered in widgets. 
    """ 
    value = field.value 
    if isinstance(field.field, ChoiceField): 
        for (val, desc) in field.field.choices: 
            if val == value: 
                return desc 
    return field.value 
display_value = property(_display_value) 

The one above produced errors for me.

comment:20 Changed 6 years ago by zanuxzan

Sorry... correction!

@register.filter(name='field_value')
def field_value(field):
    """ 
    Returns the value for this BoundField, as rendered in widgets. 
    """ 
    if not field.form.is_bound: 
        val = field.form.initial.get(field.name, field.field.initial) 
        if callable(val): 
            val = val() 
    else: 
        if isinstance(field.field, FileField) and field.data is None: 
            val = field.form.initial.get(field.name, field.field.initial) 
        else: 
            val = field.data 
    if val is None: 
        val = '' 
    return val

@register.filter(name='display_value')
def display_value(field): 
    """ 
    Returns the displayed value for this BoundField, as rendered in widgets. 
    """ 
    value = field.value 
    if isinstance(field.field, ChoiceField): 
        for (val, desc) in field.field.choices: 
            if val == value: 
                return desc 
    return field.value

comment:21 follow-up: Changed 6 years ago by paulschreiber

  • Cc paulschreiber@… added

comment:22 in reply to: ↑ 21 Changed 6 years ago by jameshfisher@…

Replying to paulschreiber:
In display_value, I think
value = field.value
should be
value = field_value(field)

field.value would make this whole ticket pointless :)

comment:23 Changed 6 years ago by jameshfisher@…

To update the filters by zanuxzan:

from django.forms import ChoiceField

from django import template
register = template.Library()

@register.filter(name='field_value')
def field_value(field):
	""" 
	Returns the value for this BoundField, as rendered in widgets. 
	""" 
	if field.form.is_bound: 
		if isinstance(field.field, FileField) and field.data is None: 
			val = field.form.initial.get(field.name, field.field.initial) 
		else: 
			val = field.data 
	else:
		val = field.form.initial.get(field.name, field.field.initial)
		if callable(val):
			val = val()
	if val is None:
		val = ''
	return val

@register.filter(name='display_value')
def display_value(field): 
	""" 
	Returns the displayed value for this BoundField, as rendered in widgets. 
	""" 
	value = field_value(field)
	if isinstance(field.field, ChoiceField): 
		for (val, desc) in field.field.choices: 
			if val == value: 
			return desc 
	return value

comment:24 Changed 6 years ago by issac.kelly@…

I had an issue with this, causing this error:

TemplateSyntaxError at /first/admin
Caught an exception while rendering: global name 'FileField' is not defined

importing 'FileField' at the top fixed it

from django.forms import ChoiceField, FileField

from django import template
register = template.Library()

@register.filter(name='field_value')
def field_value(field):
	""" 
	Returns the value for this BoundField, as rendered in widgets. 
	""" 
	if field.form.is_bound: 
		if isinstance(field.field, FileField) and field.data is None: 
			val = field.form.initial.get(field.name, field.field.initial) 
		else: 
			val = field.data 
	else:
		val = field.form.initial.get(field.name, field.field.initial)
		if callable(val):
			val = val()
	if val is None:
		val = ''
	return val

@register.filter(name='display_value')
def display_value(field): 
	""" 
	Returns the displayed value for this BoundField, as rendered in widgets. 
	""" 
	value = field_value(field)
	if isinstance(field.field, ChoiceField): 
		for (val, desc) in field.field.choices: 
			if val == value: 
				return desc 
	return value

comment:25 Changed 5 years ago by anonymous

  • Cc kmike84@… added

comment:26 Changed 5 years ago by proppy

  • Cc proppy@… added

comment:27 Changed 5 years ago by anonymous

  • Cc matt@… added

comment:28 Changed 5 years ago by martin

  • Cc martin.paquette@… added

comment:29 follow-up: Changed 5 years ago by toxik

Why isn't this merged into trunk yet? It's been accepted for over a year; Django 1.2 was just released. People are clearly interested in this. What's the holdup? Sure core committers limited time etc. but for the love of god, a year?

comment:30 in reply to: ↑ 29 Changed 5 years ago by trunet

  • Cc wsartori@… added

Replying to toxik:

Why isn't this merged into trunk yet? It's been accepted for over a year; Django 1.2 was just released. People are clearly interested in this. What's the holdup? Sure core committers limited time etc. but for the love of god, a year?

I second this!

comment:31 Changed 5 years ago by postal2600

  • Cc postal2600@… added

comment:32 Changed 5 years ago by dirleyrls

  • Cc dirleyrls@… added
  • milestone set to 1.3

comment:33 Changed 5 years ago by SmileyChris

  • Owner changed from nobody to SmileyChris
  • Status changed from reopened to new

I'm looking to commit at least an abstraction of the values generation in the BoundField.as_widget method.

Here's my branch comparison: https://github.com/SmileyChris/django/compare/master...10427-BoundField.value

comment:34 Changed 5 years ago by SmileyChris

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

(In [14734]) Fixes #10427 -- Abstract the value generation of a BoundField

comment:35 Changed 5 years ago by CollinAnderson

Thank you!

comment:36 Changed 4 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

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