Opened 13 years ago

Closed 6 years ago

#16027 closed Cleanup/optimization (fixed)

Include app_label in ContentType.__str__()

Reported by: Jakub Roztočil Owned by: Greg Schmit
Component: contrib.contenttypes Version: dev
Severity: Normal Keywords:
Cc: lemaire.adrien@…, Greg Schmit Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description (last modified by Tim Graham)

When there are multiple models with same name in your project and you use the amazing content types framework, then selects for foreign keys to ContentType contain indistinguishable items. To fix this, the app_label needs to be included in ContentType.__str__().

Attachments (2)

contenttype__unicode__.patch (511 bytes ) - added by Jakub Roztočil 13 years ago.
1627-contenttype__unicode__2.patch (526 bytes ) - added by Jakub Roztočil 13 years ago.

Download all attachments as: .zip

Change History (28)

by Jakub Roztočil, 13 years ago

comment:1 by Julien Phalip, 13 years ago

Needs tests: set
Patch needs improvement: set
Triage Stage: UnreviewedAccepted

This makes sense, and the change can't hurt. However, if we're going to disambiguate the ContentType.__unicode__(), I'd prefer if it followed conventions with the format "app_label.model". The patch also needs to include tests.

comment:2 by Jakub Roztočil, 13 years ago

How about this: "name (app_label.model)"

It should be both user-friendly (ContentType.name is the model's verbose_name) and easy to identify the origin of the model.

A ContentType instance for UserProfile from accounts would have the following unicode representation:

"user profile (accounts.userprofile)"

If there is another model with the same name, say facebook.models.UserProfile:

  • user profile (accounts.userprofile)
  • user profile (facebook.userprofile)

As for tests, I'm not sure what exactly to test in this case, any hints? Also, I've realized that this might be a backwards-incompatible change to some extent, because it will break any doctests relying on ContentType.__unicode__ returning only the verbose name of models.

by Jakub Roztočil, 13 years ago

comment:3 by Julien Phalip, 13 years ago

Thanks for your update. ContentType.__unicode__() is mostly useful to developers while debugging, and the most unambiguous and debugging-friendly value would contain both the app_label and the model. Including the verbose name, especially with added parentheses, would add little value and would feel a bit cluttered. So I still recommend using the common format 'app_label.model'.

Note that ContentType.__unicode__() is currently used implicitly in various places, for example in auth.models.Permission.__unicode__(). So that method needs to be changed to explicitly use the contenttype's name:

class Permission(models.Model):

    def __unicode__(self):
        return u"%s | %s | %s" % (
            unicode(self.content_type.app_label),
            unicode(self.content_type.name),
            unicode(self.name))

I didn't scrutinise the whole codebase but it would be useful if you could spot other instances where ContentType.__unicode__() is expected to return the verbose name.

As for testing, you can simply fetch an object from the database and assert its unicode value, for example by applying django.utils.encoding.force_unicode.

comment:4 by Aymeric Augustin, 13 years ago

UI/UX: unset

Change UI/UX from NULL to False.

comment:5 by Adrien Lemaire, 13 years ago

Cc: lemaire.adrien@… added
Resolution: fixed
Status: newclosed
Triage Stage: AcceptedFixed on a branch

Contenttype.__unicode__() has been modified with #16803.

You can close this ticket.

Last edited 6 years ago by Tim Graham (previous) (diff)

comment:6 by Ramiro Morales, 13 years ago

Triage Stage: Fixed on a branchAccepted

comment:7 by Julien Phalip, 13 years ago

Resolution: fixed
Status: closedreopened

This ticket is actually different than #16803 as it suggests including the app's name in the unicode representation. This is useful for when models from different apps have the same name.

comment:8 by Adrien Lemaire, 13 years ago

Owner: changed from nobody to Adrien Lemaire
Status: reopenednew

comment:9 by Adrien Lemaire, 13 years ago

@julien, changing the unicode representation defeats the purpose of #16803 if we're going to do what you suggest, which is to return 'app_name.model'. The name wouldn't be printed anymore, therefore we wouldn't need to translate it anymore.

But you're right in that Contenttype is mostly useful to developers, when a translatable name representation makes sense on interfaces such as django admin

Last edited 13 years ago by Adrien Lemaire (previous) (diff)

comment:10 by Julien Phalip, 13 years ago

Triage Stage: AcceptedDesign decision needed

Yes, this ticket conflicts with the approach in #16803. From my personal experience, I would have preferred using 'app_label.model' instead of the verbose model name as it would have been clearer where the model comes from and it would have helped disambiguate models with the same name (e.g. I've worked on projects with multiple Page and Article models). I do understand the value of translating model names for some situations though. So I'll move this ticket to DDN so it can be re-considered once app labels become translatable (see #3591).

comment:11 by Preston Holmes, 12 years ago

Has patch: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: Design decision neededAccepted
Version: 1.3master

I think the disambiguation should be moved to a __repr__ method, see for example #19543

The translated verbose_name should stay per #16803 for __unicode__ and the debug friendly value can be in __repr__

I'm unsure what use the original description is referring to with selects of FKs though. I've left the title unmodified, because the comment history wouldn't make sense.

comment:12 by Jakub Roztočil, 12 years ago

@ptone:

When there are multiple models with the same class name (or Meta.verbose_name). eg.:

  1. app1.models.Article
  2. app2.models.Article

Then ContentType selects in the admin look like this:

<select>
    <option value="1">Article</option>
    <option value="2">Article</option>
</select>

The problem is that you cannot distinguish those two content types in the select without inspecting the HTML code and use the PKs as a cue.

The patch I submitted changes the ContentType.__unicode__() so that it returns "<verbose_name> (<app_name>.<class_name>)" instead of just the verbose name, eliminating the problem:

<select>
    <option value="1">Article (app1.Article)</option>
    <option value="2">Article (app2.Article)</option>
</select>

comment:13 by djangsters, 11 years ago

Owner: changed from Adrien Lemaire to djangsters
Status: newassigned

Added repr function to the Contenttype model class.

The syntax is <classname: app_label.modelname>

ContentType.objects.all()
[<ContentType: contenttypes.contenttype>, <ContentType: auth.group>, <ContentType: admin.logentry>, <ContentType: auth.permission>, <ContentType: sessions.session>, <ContentType: auth.user>]

I did not change the str representation, since this change is not necessarily useful in the majority of use cases, that do not have conflicting model names. In this case maybe a warning would be useful suggesting the user to set a meaningful verbose_name.

Pull request: https://github.com/django/django/pull/1096

comment:14 by code22, 11 years ago

Has patch: set
Needs tests: set

comment:15 by Kevin Brolly, 11 years ago

This PR will not fix the problem that jakub reported with how ContentType displays in the admin, would it also be worthwhile to change the __unicode__ or __str__ methods to return with the "<verbose_name> (<app_name>.<class_name>)" style?

comment:16 by Jakub Roztočil, 11 years ago

I did not change the str representation, since this change is not necessarily useful in the majority of use cases, that do not have conflicting model names. In this case maybe a warning would be useful suggesting the user to set a meaningful verbose_name.

You can also end up with conflicting names (ie. indistinguishable items in ContenType combo boxes) by installing third-party apps that use the same model class names/verbose_name's.

The names are app-local and therefore should not be used in a global context without indicating where they come from.

Update: A nice solution would also be to use optgroups:

<select>
  <optgroup label="App One">
    <option value="1">Model One</option>
    <option value="2">Model Two</option>
  </optgroup>
  <optgroup label="App Two">
    <option value="3">Model One</option>
    <option value="4">Model Two</option>
  </optgroup>
</select>
Last edited 11 years ago by Jakub Roztočil (previous) (diff)

comment:17 by anonymous, 11 years ago

indeed jakubs suggestion of using optgroups seems to solve the problem in a very elegant way

comment:18 by djangsters, 11 years ago

The optgroups approach looks good to me but this will require a new default widget, right?
I can also add some tests for the widget and the new representation.

comment:19 by Tim Graham, 11 years ago

Easy pickings: unset

comment:20 by Tim Graham, 6 years ago

Description: modified (diff)
Summary: Include app_label in ContentType.__unicode__Include app_label in ContentType.__str__()

#30051 is a duplicate.

comment:21 by Greg Schmit, 6 years ago

So this seems like an easy change and I think we should follow Permissions and do app_label :: model_verbose_name rather than the suggested model_verbose_name (app_label) for sorting purposes. I think the optgroup thing would be a neat idea for a 3rd party package, but I think right now putting it into the Django core isn't necessary, and maybe another ticket could be opened for that as a feature request. Adding the app_label to the __str__ representation I think is the necessary bit.

I mentioned in another ticket that this should be fairly easy but it did break some tests. Would anyone have time to help me get a test-passing version on Django on my local machine (macbook pro, but I have ubuntu VM and could get another linux distro)? Right now I get a lot of errors when trying to run tests (ref https://code.djangoproject.com/ticket/30051). If someone would be willing to give me some pointers (like maybe I'm supposed to download the latest release rather than clone master?), that would be great.

comment:22 by Greg Schmit, 6 years ago

Cc: Greg Schmit added
Owner: changed from djangsters to Greg Schmit

Ok, I figured out the testing errors, just needed to disable parallelism and then use version 2.1.4 for passing tests. After making the change I see which tests need to be changed with this modification. I am working on this now.

-gns

comment:23 by Greg Schmit, 6 years ago

Triage Stage: AcceptedReady for checkin

This is fixed with PR 10776 (https://github.com/django/django/pull/10776). One check failed but it doesn't appear related to this change. All tests passed on my local machine.

-gns

Last edited 6 years ago by Greg Schmit (previous) (diff)

comment:24 by Simon Charette, 6 years ago

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

Please don't mark your own patch as RFC; someone else has to review your changes by going through the review checklist.

in reply to:  24 comment:25 by Greg Schmit, 6 years ago

Replying to Simon Charette:

Please don't mark your own patch as RFC; someone else has to review your changes by going through the review checklist.

Sorry about that! Won't happen again.

comment:26 by Tim Graham <timograham@…>, 6 years ago

Resolution: fixed
Status: assignedclosed

In 48c1780:

Fixed #16027 -- Added app_label to ContentType.str().

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