Opened 13 years ago

Closed 11 years ago

Last modified 10 years ago

#16612 closed Bug (fixed)

Localized form fields cause Form.has_changed() to always return True.

Reported by: rviotti@… 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)

16612-1.diff (40.7 KB ) - added by Claude Paroz 11 years ago.
Moved _has_changed from widget to field

Download all attachments as: .zip

Change History (19)

comment:1 by Aymeric Augustin, 13 years ago

Triage Stage: UnreviewedAccepted

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. So Widget._has_changed compares localized and unlocalized data, and fails. "Unlocalization" of the submitted value happens later, in DecimalField.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 from forms.Widget and its subclasses to forms.Field and its subclasses.

comment:2 by Aymeric Augustin, 13 years ago

Triage Stage: AcceptedDesign decision needed

In fact, I'm suggesting a fairly large change. This needs a thumbs up (or down) from a core developer.

in reply to:  1 comment:3 by rviotti@…, 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. So Widget._has_changed compares localized and unlocalized data, and fails. "Unlocalization" of the submitted value happens later, in DecimalField.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 from forms.Widget and its subclasses to forms.Field and its subclasses.

comment:4 by Jacob, 13 years ago

Triage Stage: Design decision neededAccepted

aaugustin: here's the thumbs up you're waiting for.

by Claude Paroz, 11 years ago

Attachment: 16612-1.diff added

Moved _has_changed from widget to field

comment:5 by Claude Paroz, 11 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:6 by Claude Paroz, 11 years ago

#19557 is a duplicate with a patch (for the specific OP issue)

comment:7 by Claude Paroz <claude@…>, 11 years ago

In ebb504db692cac496f4f45762d1d14644c9fa6fa:

Moved has_changed logic from widget to form field

Refs #16612. Thanks Aymeric Augustin for the suggestion.

comment:8 by Claude Paroz, 11 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:9 by Claude Paroz, 11 years ago

Patch updated to current trunk.

comment:10 by Simon Charette, 11 years ago

Patch needs improvement: set
Version: 1.3master

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 Claude Paroz, 11 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:12 by Simon Charette, 11 years ago

Triage Stage: AcceptedReady for checkin

Patch is looking good.

comment:13 by Claude Paroz <claude@…>, 11 years ago

Resolution: fixed
Status: newclosed

In 892bc91cb0036d6868081363628f65094c4790d6:

Fixed #16612 -- Improved has_changed detection for localized field values

Thanks Simon Charette for the review.

comment:14 by Claude Paroz <claude@…>, 11 years ago

In cbfb8ed53b31ec9701f5fb8e519a8644fd4c8095:

Fixed a regression in forms changed_data

Thanks Loic Bistuer for spotting the regression and the initial
patch. Refs #16612.

comment:15 by Patrick K, 10 years ago

shouldn't this fix be implemented with django 1.5 and 1.4 as well?

comment:16 by Tim Graham, 10 years ago

Per our support policy, Django 1.4 is only receiving security updates and 1.5 is only receiving critical bug fixes.

comment:17 by Tim Graham <timograham@…>, 10 years ago

In d74e33eb0ec289d3125a5a8048d756f9d232bd62:

Removed backwards compatibility code to call field.widget._has_changed()

This logic should be moved to field._has_changed() as described
in ebb504db692cac496f4f45762d1d14644c9fa6f - refs #16612.

comment:18 by Tim Graham <timograham@…>, 10 years ago

In 399cf303cbfe086e78fd9421b75c6df55fc234d0:

Corrected a comment in forms/forms.py; refs #16612.

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