Opened 7 years ago

Closed 7 years ago

#24428 closed Bug (fixed)

Model field with default=callable and choices is always reported as changed

Reported by: Carsten Fuchs 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

Originating thread at django-users: https://groups.google.com/forum/#!topic/django-users/WhuG7X9eRWk

In a newly created Django project/app, please consider this models.py file:

# -*- coding: utf-8 -*-
from django.db import models

def VormonatsMonat():
    return 1

MONTHS_CHOICES = (
    (1, "Jan"),
    (2, "Feb"),
)

class Choice(models.Model):
    choice_text = models.CharField(max_length=200)
    votes = models.IntegerField(default=0)
    monat = models.IntegerField(default=VormonatsMonat, choices=MONTHS_CHOICES)

Then, at the ./manage.py shell prompt (creating a Choice instance is omitted here):

>>> from django.forms import ModelForm
>>> from TestApp.models import Choice
>>> class ChoiceForm(ModelForm):
...     class Meta:
...         model = Choice
...         fields = ["choice_text", "votes", "monat"]
...
>>> ch =  Choice.objects.get(pk=1)
>>> chv = Choice.objects.values().get(pk=1)
>>> ch
<Choice: Choice object>
>>> chv
{'monat': 1, 'choice_text': u'just a test', 'votes': 0, u'id': 1}
>>> ChoiceForm(chv, initial=chv, instance=ch).has_changed()
True 

The expected output is False, which is (in a new shell session) in fact obtained whenever the definition of field monat is slightly changed, e.g. to a constant default value rather than a callable, or with no choices.

I got as far as BaseForm.changed_data() in django/forms/forms.py, where for field monat we get

  • field.show_hidden_initial == True,
  • hidden_widget.value_from_datadict(self.data, self.files, initial_prefixed_name) == None,
  • resulting in initial_value == u"",

resulting in the observed behaviour.

Is maybe field.show_hidden_initial == True (wrongly) because the default=callable makes it seem that the default value is not one of the available choices?

Change History (8)

comment:1 Changed 7 years ago by Tim Graham

Component: UncategorizedForms
Triage Stage: UnreviewedAccepted

show_hidden_initial = True is correct because we need to know the initially selected choice. It seems the issue is in Field.has_changed() where the initial value is compared as a string to the coerced value as an integer. I think it's a bug.

comment:2 Changed 7 years ago by Carsten Fuchs

Summary: Model field with default=callable and choices always is always reported at changedModel field with default=callable and choices is always reported as changed

comment:3 Changed 7 years ago by Carsten Fuchs

Looking at Django 1.7.5's BaseForm.changed_data(), debugging indicates that line

elif field._has_changed(initial_value, data_value):

for field monat of my above example turns into

elif field._has_changed(u"", 1):

Thus, afaics, it rather seems that initial_value is wrongly determined earlier, when call hidden_widget.value_from_datadict(self.data, self.files, initial_prefixed_name) returns None rather than 1.

self.data in turn has no key initial_prefixed_name (here "initial-monat"), causing the None return value above.

What I wonder about is how and where "initial-monat" should have been added to self.data in the first place...?

comment:4 Changed 7 years ago by Tim Graham

It's a hidden field that appears when the form is rendered. If you are testing in Python only, you should include that field in the form data yourself.

comment:5 Changed 7 years ago by Claude Paroz

Has patch: set
Version: 1.7master

comment:6 Changed 7 years ago by Carsten Fuchs

Gee, Tim and Claude, many thanks!!

Just when I was reading the contributor's docs to attempt a patch myself. ;-) But I would probably have not been able to come up with the proper tests anyway (well, with a bit of luck, next time), so I'm very happy you did!

comment:7 Changed 7 years ago by Tim Graham

Triage Stage: AcceptedReady for checkin

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

Resolution: fixed
Status: newclosed

In 8714403614c4dfa37e806db4a6708b8a91a827a4:

Fixed #24428 -- Fixed has_changed for fields with coercion

Thanks Carsten Fuchs for the report.

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