#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_data
always contain the unlocalized object. But submitted data is localized. SoWidget._has_changed
compares 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.Widget
and its subclasses toforms.Field
and 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_data
always contain the unlocalized object. But submitted data is localized. SoWidget._has_changed
compares 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.Widget
and its subclasses toforms.Field
and its subclasses.