#16612 closed Bug (fixed)
Localized form fields cause Form.has_changed() to always return True.
| Reported by: | Owned by: | nobody | |
|---|---|---|---|
| Component: | Forms | Version: | dev |
| Severity: | Normal | Keywords: | |
| Cc: | Triage Stage: | Ready for checkin | |
| Has patch: | yes | Needs documentation: | no |
| Needs tests: | no | Patch needs improvement: | no |
| Easy pickings: | no | UI/UX: | no |
Description
Localized form fields are causing Form.has_changed() to always return True.
I've created a minimal project to test this. Here is the relevant part of the settings.py file.
TIME_ZONE = 'America/Sao_Paulo' USE_THOUSAND_SEPARATOR = True LANGUAGE_CODE = 'pt-br' USE_I18N = True USE_L10N = True
The test model.
from django.db import models
class TestModel(models.Model):
numeric_field = models.DecimalField(max_digits=10, decimal_places=2)
@models.permalink
def get_absolute_url(self):
return ('edit', [str(self.pk)])
The test form.
from django import forms
from app.models import TestModel
class TestForm(forms.ModelForm):
class Meta:
model = TestModel
numeric_field = forms.DecimalField(localize=True)
The test view.
from django.shortcuts import get_object_or_404, render, redirect
from app.forms import TestForm
from app.models import TestModel
def edit_instance(request, instance_id):
inst = get_object_or_404(TestModel, pk=instance_id)
if request.method == 'POST':
form = TestForm(request.POST, instance=inst)
if form.is_valid():
inst = form.save()
print 'FORM CHANGED?', form.has_changed()
return redirect(inst)
else:
form = TestForm(instance=inst)
return render(request, 'testmodel.html', {'form': form})
And template code.
<html>
<head>
<title>Bugreports</title>
</head>
<body>
<form action="" method="post">
{% csrf_token %}
{{ form.as_p }}
<input type="submit" />
</form>
</body>
</html>
The print statement above emits True whenever I hit the submit button, regardless of changes on the numeric field value. This is because Form._has_changed_data is comparing raw localized POST data with initial data, coming from the model, both converted to Unicode. For example, the request POST data is "1,23" and the initial data obtained from the model field is Decimal('1.23'). The test "unicode(u'1,23') == unicode(Decimal('1.23'))" fails.
The following ModelForm specialization compares initial data with cleaned data, which, IMHO, is the adequate way to do field change detection.
class ModelFormSubclass(forms.ModelForm):
def _get_changed_data(self):
if self._changed_data is None:
self._changed_data = []
for name, field in self.fields.items():
data_value = self.cleaned_data[name]
initial_value = self.initial.get(name, field.initial)
# Replace instances with their primary key values.
if hasattr(data_value, 'pk'):
data_value = data_value.pk
if data_value != initial_value:
self._changed_data.append(name)
return self._changed_data
changed_data = property(_get_changed_data)
The overriden method fixed my specific problem. I'm posting it primarily for consideration of the proposed concept, and secondly, as a preliminary solution to the aforementioned issue.
Attachments (1)
Change History (19)
follow-up: 3 comment:1 by , 13 years ago
| Triage Stage: | Unreviewed → Accepted |
|---|
comment:2 by , 13 years ago
| Triage Stage: | Accepted → Design decision needed |
|---|
In fact, I'm suggesting a fairly large change. This needs a thumbs up (or down) from a core developer.
comment:3 by , 13 years ago
+1 for "has_changed" being much more data-related than presentation-related.
Replying to aaugustin:
This looks similar to the problems reported for DateTimeField reported in #12858 and for NullBooleanField in #11860.
It's clear that this bug exists for DecimalField. Localization happens in the template, when the widget is rendered; fields and widgets'
initial_dataalways contain the unlocalized object. But submitted data is localized. SoWidget._has_changedcompares localized and unlocalized data, and fails. "Unlocalization" of the submitted value happens later, inDecimalField.to_python.
Generally, I don't understand why the "has_changed" logic is implemented in the widgets. It seems to me that "has_changed" is data-related (= field), not presentation-related (= widget). To compute it, we need the "unlocalized" value, ie. the output of
Field.to_python. I think this logic should be moved fromforms.Widgetand its subclasses toforms.Fieldand its subclasses.
comment:4 by , 13 years ago
| Triage Stage: | Design decision needed → Accepted |
|---|
aaugustin: here's the thumbs up you're waiting for.
comment:5 by , 12 years ago
| Has patch: | set |
|---|
I just experimented the move of the _has_changed method and can confirm it is not only doable but also makes more sense. The patch does not resolve the original issue yet, it should come as a next step.
comment:8 by , 12 years ago
Pull request: https://github.com/django/django/pull/676
In this proposal, we call to_python for the value we get from the form data.
comment:10 by , 12 years ago
| Patch needs improvement: | set |
|---|---|
| Version: | 1.3 → master |
Which patch was updated? I see the attached one is 2 months old and the PR's last commit is one month old.
comment:11 by , 12 years ago
| Patch needs improvement: | unset |
|---|
The pull request is updated, on Feb 23, 2013 (now Feb 27 2013) if you look at the Commits tabs, even if the Discussion tab shows the date of the original PR. Now I just rebased them again after the "big tests move".
comment:13 by , 12 years ago
| Resolution: | → fixed |
|---|---|
| Status: | new → closed |
comment:16 by , 11 years ago
Per our support policy, Django 1.4 is only receiving security updates and 1.5 is only receiving critical bug fixes.
This looks similar to the problems reported for DateTimeField reported in #12858 and for NullBooleanField in #11860.
It's clear that this bug exists for DecimalField. Localization happens in the template, when the widget is rendered; fields and widgets'
initial_dataalways contain the unlocalized object. But submitted data is localized. SoWidget._has_changedcompares localized and unlocalized data, and fails. "Unlocalization" of the submitted value happens later, inDecimalField.to_python.Generally, I don't understand why the "has_changed" logic is implemented in the widgets. It seems to me that "has_changed" is data-related (= field), not presentation-related (= widget). To compute it, we need the "unlocalized" value, ie. the output of
Field.to_python. I think this logic should be moved fromforms.Widgetand its subclasses toforms.Fieldand its subclasses.