Code

Opened 3 years ago

Closed 2 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 3 years ago.
unlocalize1.patch (761 bytes) - added by lev 3 years ago.
removed the comma
unlocalize_maxLength.diff (3.1 KB) - added by magopian 3 years ago.
Lev's patch with added regression tests
unlocalize_maxLength2.diff (4.1 KB) - added by feel 2 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)

Changed 3 years ago by lev

comment:1 Changed 3 years ago by magopian

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

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 Changed 3 years ago by magopian

  • Cc mathieu.agopian@… added

Changed 3 years ago by lev

removed the comma

comment:3 Changed 3 years ago by magopian

  • 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 Changed 3 years ago by magopian

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

Changed 3 years ago by magopian

Lev's patch with added regression tests

comment:5 Changed 3 years ago by magopian

  • Needs tests unset

comment:6 Changed 2 years ago by feel

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

  • Patch needs improvement set

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

comment:8 Changed 2 years ago by anonymous

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

Yes, that's how it works.

Changed 2 years ago by feel

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

comment:10 Changed 2 years ago by julien

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

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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.