Opened 13 years ago

Closed 12 years ago

#15938 closed Bug (fixed)

USE_THOUSAND_SEPARATOR breaks prepopulated_fields functionality

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

Description

# models.py

from django.db import models

class Book(models.Model):
    slug = models.CharField(max_length=1000)
    name = models.CharField(max_length=1000)

# admin.py

from django.contrib import admin
import models

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

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

# settings.py

USE_L10N = True
USE_THOUSAND_SEPARATOR = True

This bug is present with most locales (including en and en-gb). Common values for DECIMAL_SEPARATOR are space, comma and dot. The former two generate a js error in django-generated js code, the latter one results in wrong max_length for slug field (1.000 = 1)

Disclaimer:

This example is somewhat artificial because NUMBER_GROUPING is usually 3 and it doesn't make much sense to use slugs 1000 chars long.
So the severity of the bug is not at all high. But the bug there is.

The patch is against 1.3 but at the moment of writing the issue is present in trunk as well.

Attachments (4)

unlocalize.patch (762 bytes ) - added by lev 13 years ago.
unlocalize1.patch (761 bytes ) - added by lev 13 years ago.
removed the comma
unlocalize_maxLength.diff (3.1 KB ) - added by Mathieu Agopian 13 years ago.
Lev's patch with added regression tests
unlocalize_maxLength2.diff (4.1 KB ) - added by Filippo Squillace 12 years ago.
Lev's patch with test_prepopulated_maxlength_localized that use the override_settings decorator (I got rid of some new lines).

Download all attachments as: .zip

Change History (14)

by lev, 13 years ago

Attachment: unlocalize.patch added

comment:1 by Mathieu Agopian, 13 years ago

Patch needs improvement: set
Triage Stage: UnreviewedAccepted

I've reproduced the issue, and can confirm everything stated in the ticket with one minor typo (it's maxLength in the javascript and not max_length).

I'm marking this ticket as Accepted but the patch needs improvements: please remove the trailing comma, as it wasn't there in the first place, and it'll break in Internet Explorer.

comment:2 by Mathieu Agopian, 13 years ago

Cc: mathieu.agopian@… added

by lev, 13 years ago

Attachment: unlocalize1.patch added

removed the comma

comment:3 by Mathieu Agopian, 13 years ago

Needs tests: set
Patch needs improvement: unset

Thanks lev for the update of the patch, looks good now. Sorry for that, but i missed the "needs tests" flag in my previous comment

comment:4 by Mathieu Agopian, 13 years ago

see #14895 for a similar issue (localization of a pk)

by Mathieu Agopian, 13 years ago

Attachment: unlocalize_maxLength.diff added

Lev's patch with added regression tests

comment:5 by Mathieu Agopian, 13 years ago

Needs tests: unset

comment:6 by Filippo Squillace, 12 years ago

UI/UX: unset

I also report the same bug and the Lev's patch works well. Does it need to be flagged RFC?

comment:7 by Aymeric Augustin, 12 years ago

Patch needs improvement: set

Patch doesn't apply cleanly and test_prepopulated_maxlength_localized should use the override_settings decorator.

comment:8 by anonymous, 12 years ago

I tried to look for override_settings documentation but I didn't find enough. Is it correct the use of it as follows?

    @override_settings(USE_THOUSAND_SEPARATOR = True, USE_L10N = True)
    def test_prepopulated_maxlength_localized(self):
        """
        Regression test for #15938: if USE_THOUSAND_SEPARATOR is set, make sure
        that maxLength (in the javascript) is rendered without separators.
        """

        response = self.client.get('/test_admin/admin/admin_views/prepopulatedpostlargeslug/add/')
        self.assertContains(response, "maxLength: 1000") # instead of 1,000

comment:9 by Aymeric Augustin, 12 years ago

Yes, that's how it works.

by Filippo Squillace, 12 years ago

Attachment: unlocalize_maxLength2.diff added

Lev's patch with test_prepopulated_maxlength_localized that use the override_settings decorator (I got rid of some new lines).

comment:10 by Julien Phalip, 12 years ago

Resolution: fixed
Status: newclosed

In [17033]:

Fixed #15938 -- Prevented the max_length number in the admin prepopulated_fields javascript to be localized when USE_THOUSAND_SEPARATOR is turned on. Thanks to Lev Maximov, Mathieu Agopian and feel.

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