Opened 15 years ago

Closed 13 years ago

Last modified 4 years ago

#10427 closed (fixed)

Bound field needs an easy way to get form value

Reported by: Ludvig Ericson Owned by: Chris Beaven
Component: Forms Version: dev
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: no UI/UX: no

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 Ludvig Ericson 15 years ago.
Use empty string instead of None
django-forms-value.3.diff (3.0 KB ) - added by Ludvig Ericson 15 years ago.
Remove useless change
django-forms-value.4.diff (3.0 KB ) - added by Ludvig Ericson 15 years ago.
Use empty string even if value is from initial data
django-forms-value.5.diff (3.0 KB ) - added by Ludvig Ericson 15 years ago.
Added documentation
django-forms-value.6.diff (4.4 KB ) - added by Ludvig Ericson 15 years ago.
Added documentation (really)
django-forms-value.7.diff (4.3 KB ) - added by Ludvig Ericson 15 years ago.
Against trunk r10454
django-forms-value.diff (4.7 KB ) - added by Ludvig Ericson 15 years ago.
Against trunk r10650
django-display_value-r10755.patch (6.1 KB ) - added by Michał Sałaban 15 years ago.
with "display_value' method added
django-display_value-r11298.patch (6.5 KB ) - added by pdonis 15 years ago.
adds one-line change to formtools/preview.html template

Download all attachments as: .zip

Change History (47)

by Ludvig Ericson, 15 years ago

Attachment: django-forms-value.2.diff added

Use empty string instead of None

by Ludvig Ericson, 15 years ago

Attachment: django-forms-value.3.diff added

Remove useless change

by Ludvig Ericson, 15 years ago

Attachment: django-forms-value.4.diff added

Use empty string even if value is from initial data

comment:1 by Jacob, 15 years ago

Resolution: invalid
Status: newclosed

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

comment:2 by Ludvig Ericson, 15 years ago

Resolution: invalid
Status: closedreopened

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 by Malcolm Tredinnick, 15 years ago

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 by Malcolm Tredinnick, 15 years ago

Needs documentation: set
Patch needs improvement: set
Triage Stage: UnreviewedAccepted

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.

in reply to:  3 comment:5 by Ludvig Ericson, 15 years ago

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 by Malcolm Tredinnick, 15 years ago

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.

by Ludvig Ericson, 15 years ago

Attachment: django-forms-value.5.diff added

Added documentation

by Ludvig Ericson, 15 years ago

Attachment: django-forms-value.6.diff added

Added documentation (really)

in reply to:  6 comment:7 by Ludvig Ericson, 15 years ago

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.

by Ludvig Ericson, 15 years ago

Attachment: django-forms-value.7.diff added

Against trunk r10454

by Ludvig Ericson, 15 years ago

Attachment: django-forms-value.diff added

Against trunk r10650

comment:8 by Michał Sałaban, 15 years ago

Cc: michal@… added

comment:9 by Michał Sałaban, 15 years ago

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.

by Michał Sałaban, 15 years ago

with "display_value' method added

comment:10 by Michał Sałaban, 15 years ago

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 by Gábor Farkas, 15 years ago

Cc: gabor@… added

comment:12 by viksit, 15 years ago

Cc: viksit@… added

Second this solution!

Added cc.

comment:13 by Margie Roginski, 15 years ago

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 by Alex Gaynor, 15 years ago

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 by Margie Roginski, 15 years ago

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 by Michael Stevens <mstevens@…>, 15 years ago

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

comment:17 by pdonis, 15 years ago

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".

by pdonis, 15 years ago

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

comment:18 by simple10, 15 years ago

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 by Alex Hayes, 14 years ago

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 by Alex Hayes, 14 years ago

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 by Paul Schreiber, 14 years ago

Cc: paulschreiber@… added

in reply to:  21 comment:22 by jameshfisher@…, 14 years ago

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 by jameshfisher@…, 14 years ago

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 by issac.kelly@…, 14 years ago

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 by anonymous, 14 years ago

Cc: kmike84@… added

comment:26 by proppy, 14 years ago

Cc: proppy@… added

comment:27 by anonymous, 14 years ago

Cc: matt@… added

comment:28 by martin, 14 years ago

Cc: martin.paquette@… added

comment:29 by Ludvig Ericson, 14 years ago

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?

in reply to:  29 comment:30 by Wagner Sartori Junior, 14 years ago

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 by postal2600, 13 years ago

Cc: postal2600@… added

comment:32 by dirleyrls, 13 years ago

Cc: dirleyrls@… added
milestone: 1.3

comment:33 by Chris Beaven, 13 years ago

Owner: changed from nobody to Chris Beaven
Status: reopenednew

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 by Chris Beaven, 13 years ago

Resolution: fixed
Status: newclosed

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

comment:35 by Collin Anderson, 13 years ago

Thank you!

comment:36 by Jacob, 12 years ago

milestone: 1.3

Milestone 1.3 deleted

comment:37 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

Easy pickings: unset
UI/UX: unset

In 734fde7:

Refs #10427 -- Corrected BoundField.css_classes() signature in docs.

comment:38 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

In eeab4ab:

[3.0.x] Refs #10427 -- Corrected BoundField.css_classes() signature in docs.

Backport of 734fde771433b9ea525a2b0f4d8ebb3ae426b2fa from master

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