Opened 4 years ago

Closed 2 years ago

Last modified 11 months 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: master
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 claudep 3 years ago.
Moved _has_changed from widget to field

Download all attachments as: .zip

Change History (19)

comment:1 follow-up: Changed 4 years ago by aaugustin

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

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 Changed 4 years ago by aaugustin

  • Triage Stage changed from Accepted to 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 in reply to: ↑ 1 Changed 4 years ago by rviotti@…

+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 Changed 4 years ago by jacob

  • Triage Stage changed from Design decision needed to Accepted

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

Changed 3 years ago by claudep

Moved _has_changed from widget to field

comment:5 Changed 3 years ago by claudep

  • 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 Changed 2 years ago by claudep

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

comment:7 Changed 2 years ago by Claude Paroz <claude@…>

In ebb504db692cac496f4f45762d1d14644c9fa6fa:

Moved has_changed logic from widget to form field

Refs #16612. Thanks Aymeric Augustin for the suggestion.

comment:8 Changed 2 years ago by claudep

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 Changed 2 years ago by claudep

Patch updated to current trunk.

comment:10 Changed 2 years ago by charettes

  • Patch needs improvement set
  • Version changed from 1.3 to 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 Changed 2 years ago by claudep

  • 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 Changed 2 years ago by charettes

  • Triage Stage changed from Accepted to Ready for checkin

Patch is looking good.

comment:13 Changed 2 years ago by Claude Paroz <claude@…>

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

In 892bc91cb0036d6868081363628f65094c4790d6:

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

Thanks Simon Charette for the review.

comment:14 Changed 2 years ago by Claude Paroz <claude@…>

In cbfb8ed53b31ec9701f5fb8e519a8644fd4c8095:

Fixed a regression in forms changed_data

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

comment:15 Changed 21 months ago by sehmaschine

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

comment:16 Changed 21 months ago by timo

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

comment:17 Changed 16 months ago by Tim Graham <timograham@…>

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 Changed 11 months ago by Tim Graham <timograham@…>

In 399cf303cbfe086e78fd9421b75c6df55fc234d0:

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

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