Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#16803 closed Bug (fixed)

Unicode representation of ContentType instances is not translated

Reported by: Torsten Bronger Owned by: Ivan Sagalaev
Component: contrib.contenttypes Version: 1.3
Severity: Normal Keywords:
Cc: bronger@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

unicode(contenttype_instance) always returns the English name of the respective model class. More accurately, it returns the "name" field which contains the underlying model class'es verbose_name_raw. It would have helped me if it said in django/django/contrib/contenttypes/models.py line 87:

    def __unicode__(self):
        return unicode(self.model_class()._meta.verbose_name)

or something similar.

Attachments (2)

16803.diff (6.1 KB) - added by Ivan Sagalaev 5 years ago.
Patch
16803.2.diff (6.6 KB) - added by Ivan Sagalaev 5 years ago.
Patch fixing unicode breakage

Download all attachments as: .zip

Change History (13)

comment:1 Changed 5 years ago by Ivan Sagalaev

ContentType instance name is a pretty "technical" name. Can you describe a usecase where it is shown in a user or admin interface?

comment:2 Changed 5 years ago by Torsten Bronger

I have three occurences of M2M relationships to ContentType in my models. These models become model forms. Then, you get selection boxes in the browser with untranslated items.

comment:3 Changed 5 years ago by Ivan Sagalaev

Triage Stage: UnreviewedDesign decision needed

First, I was wrong that the field name is technical. Quite the opposite, it's intended to be a human-readable representation. So the simplest solution to your use-case is changing the name to whatever you want. However this won't work in a multilingual environment since the field's content cannot be translated (at the moment).

I've conducted a little research. ContentType.name is initialized with model's untranslated verbose name — verbose_name_raw, a property which was introduced in [5225] specifically for the purpose of using for content types. I don't know why it was done this way. It might be a good idea to change this to using a usual translated verbose names of a model but it's technically backwards incompatible.

I'm marking this as DDN to get an opinion of a core developer. To summarize:

  • ContentType uses a dedicated field for its human-readable representation. The idea is to use real verbose names of models.
  • Good: ContentTypes will become translatable
  • Good: code around verbose_name_raw will go away
  • Bad: it's technically backwards incompatible: users might be changing and relying on ContentType.name

My own take would be to go for a change since I doubt ContentType.name}} is widely used **and** is different from {{{verbose_name of a model. Most probably we will even save maintenance for people who routinely change names of content type according to model's verbose names.

comment:4 Changed 5 years ago by Carl Meyer

Triage Stage: Design decision neededAccepted

I don't see any good reason why ContentType should have a denormalized name field stored in the database, rather than looking up the appropriate model verbose_name at runtime - its not as if the latter is an expensive operation, and translation is a real problem with the current approach.

I think we can make this fix in a backwards-compatible way. The __unicode__ method can check if the name field is equal to the model's verbose_name_raw (i.e. hasn't been manually changed), and if so look up the real verbose_name (possibly translated) and return that. If the name field has been changed, return it. This still leaves open the possibility that in the future (once we have a schema-alteration framework in core) we could deprecate and then remove the name field entirely.

In the meantime, there's also a workaround that I think can help multilingual sites: on ModelForms for models with an FK to ContentType, you can replace the form field for that FK with a subclass of ModelChoiceField that overrides the label_for_instance method to do the translated verbose_name lookup. This is documented; it's fairly verbose (which is why I'm not simply pointing to it and closing this wontfix), but it should do the trick for the time being.

comment:5 Changed 5 years ago by Ivan Sagalaev

Owner: changed from nobody to Ivan Sagalaev
Status: newassigned

I was thinking along those lines too :-). I think I could make a patch…

Changed 5 years ago by Ivan Sagalaev

Attachment: 16803.diff added

Patch

comment:6 Changed 5 years ago by Ivan Sagalaev

Here's the first iteration of a patch. A couple of notes:

  • touch tests/regressiontests/i18n/contenttypes/__init__.py
  • I don't know if there a point of attaching compiled language files that don't get included in the patch, it might be easier to compile them from tests/regressiontests/i18n/contenttypes directory by hand
  • I left a comment in django/contrib/contenttypes/models.py regarding deprecation of self.name but wasn't sure if we should actually raise a DeprecationWarning there?

comment:7 Changed 5 years ago by Ivan Sagalaev

Has patch: set

comment:8 in reply to:  6 Changed 5 years ago by Carl Meyer

Patch needs improvement: set

Replying to isagalaev:

Here's the first iteration of a patch. A couple of notes:

  • touch tests/regressiontests/i18n/contenttypes/__init__.py
  • I don't know if there a point of attaching compiled language files that don't get included in the patch, it might be easier to compile them from tests/regressiontests/i18n/contenttypes directory by hand
  • I left a comment in django/contrib/contenttypes/models.py regarding deprecation of self.name but wasn't sure if we should actually raise a DeprecationWarning there?

I think actual deprecation of the name field should wait until we have schema migrations in core, so that we can actually remove the field at the end of the deprecation. For now the comment is fine - I might expand on it to note that we'll do a real deprecation later.

Looks good otherwise, except I'm now getting four UnicodeDecodeErrors in the admin_views tests with this patch. Probably due to the added translation with non-ASCII chars in it, haven't dug in to find the best solution.

comment:9 Changed 5 years ago by Ivan Sagalaev

My bad, I should've run the whole test suite. I'll look into it shortly…

Changed 5 years ago by Ivan Sagalaev

Attachment: 16803.2.diff added

Patch fixing unicode breakage

comment:10 Changed 5 years ago by Ivan Sagalaev

This one should be better. UnicodeDecodeError was because of using low-level unicode() instead of force_unicode() — fixed. Tests now pass. I also updated the comment.

comment:11 Changed 5 years ago by Carl Meyer

Resolution: fixed
Status: assignedclosed

In [16839]:

Fixed #16803 -- Use model verbose_name directly as ContentType unicode representation so it can be translated. Thanks to bronger for the report and Ivan Sagalaev for the patch.

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