Opened 5 years ago

Closed 5 years ago

#15922 closed Bug (duplicate)

prepopulated_fields for DecimalField generates js error

Reported by: lev Owned by: nobody
Component: contrib.admin Version: 1.3
Severity: Normal Keywords: prepopulated_fields Decimal default_if_none
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: yes

Description

1)

maxLength: {{ field.field.field.max_length|default_if_none:"50" }}
)

is rendered as

maxLength:
)

2) DecimalField doesn't have max_length attribute (raises AtttibuteError).

3)

{% ifequal field.field.field.max_length None %}yes{% else %}no{% endif %}

returns yes;

4)

{{ field.field.field.max_length|default:"50" }}

generates 50 being an imperfect workaround since it should be default_if_none

Change History (9)

comment:1 Changed 5 years ago by lev.maximov@…

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

The code I'm talking about is here: django/contrib/admin/templates/admin/prepopulated_fields_js.html:10

comment:2 Changed 5 years ago by Aymeric Augustin

Resolution: invalid
Status: newclosed

Missing attributes are replaced by settings.TEMPLATE_STRING_IF_INVALID, which defaults to '' (the empty string) and not None.
See http://docs.djangoproject.com/en/dev/ref/templates/api/#how-invalid-variables-are-handled
Django works as advertised here.

The doc states that it is invalid to compare something to a Python object such as None with the ifequal tag.
See http://docs.djangoproject.com/en/dev/ref/templates/builtins/#ifequal
What actually happens in your example is that both field.field.field.max_length and None evaluate to '' in a template context, so the condition matches.

comment:3 Changed 5 years ago by lev

Resolution: invalid
Status: closedreopened

OK, once again from the beginning:

class Book(models.Model):
    starting_price = DecimalField()
    price = DecimalField()

class BookAdmin(models.ModelAdmin):
    prepopulated_fields = {'price': ['starting_price']}

This does not work as intended

comment:4 Changed 5 years ago by lev

Documentation states:

prepopulated_fields doesn't accept `DateTimeField`, `ForeignKey`, nor `ManyToManyField` fields.

Also, ImproperlyConfigured is raised in such cases.

So either:

1) perpopulated_js should be fine-tuned somehow to work with DecimalFields as well (ie not trim decimal point/decimal comma) or

2) DecimalField should be included into the list of unsupported fields to docs and raise ImproperlyConfigured as well.

comment:5 Changed 5 years ago by Aymeric Augustin

Triage Stage: UnreviewedAccepted

lev, we can't see your code, so we need some context to understand the problem you're reporting. See the instructions here for how to write a good bug report:
http://docs.djangoproject.com/en/dev/internals/contributing/#reporting-bugs

I could eventually reproduce the problem, here is a minimal test case that works:

# models.py

from django.db import models

class Book(models.Model):
    starting_price = models.DecimalField(decimal_places=2, max_digits=10)
    price = models.DecimalField(decimal_places=2, max_digits=10)

# admin.py

from django.contrib import admin
import models

class BookAdmin(admin.ModelAdmin):
    prepopulated_fields = {
        'price': ['starting_price'],
    }

admin.site.register(models.Book, BookAdmin)

With this code, trying to add a new book with the admin results in the javascript error described in the initial bug report. This error is in dynamically generated javascript in the HTML.

comment:6 Changed 5 years ago by lev

I found out that the bug does not occur if DecimalField is used only as a source field and not as a destination.

So while changing the behaviour of prepopulated_fields so that it accepts DecimalField as destination field is somewhat dubious, here're some fixes that can be applied without such design decision:

1) In documentation:

- prepopulated_fields doesn't accept DateTimeField, ForeignKey, nor ManyToManyField fields.
+ prepopulated_fields doesn't accept DateTimeField, ForeignKey, nor ManyToManyField fields as source fields.

2) Prohibit DecimalField to be used as a destination field in the code.

3) Currently type control is applied to destination field only. Prohibiting DateTimeField as a source field could also make sense as it doesn't work either.

comment:7 Changed 5 years ago by lev

To be more explicit, the following works well:

# models.py

from django.db import models

class Book(models.Model):
    slug = models.CharField(max_length=100)
    starting_price = models.DecimalField(decimal_places=2, max_digits=10)

# admin.py

from django.contrib import admin
import models

class BookAdmin(admin.ModelAdmin):
    prepopulated_fields = {
        'slug': ['starting_price'],
    }

admin.site.register(models.Book, BookAdmin)

comment:8 Changed 5 years ago by Julien Phalip

UI/UX: set

comment:9 Changed 5 years ago by Peter Kese

Resolution: duplicate
Status: reopenedclosed

While trying to fix this bug, I have found some other issues causing it and have thus opened another ticket: #16226
Since I have provided a patch for #16226 which also fixes this ticket, I would like divert attention to the new ticket and will ask anyone interested to review the patch.

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