Opened 4 years ago

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

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

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

  • Resolution invalid deleted
  • Status changed from closed to reopened

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

  • Triage Stage changed from Unreviewed to Accepted

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

  • UI/UX set

comment:9 Changed 4 years ago by peter_kese

  • Resolution set to duplicate
  • Status changed from reopened to closed

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