Opened 7 years ago
Last modified 5 years ago
#16027 assigned Cleanup/optimization
Include app_label in ContentType.__unicode__
Reported by: | Jakub Roztočil | Owned by: | djangsters |
---|---|---|---|
Component: | contrib.contenttypes | Version: | master |
Severity: | Normal | Keywords: | |
Cc: | lemaire.adrien@… | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | yes | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
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's unicode.
Attachments (2)
Change History (21)
Changed 7 years ago by
Attachment: | contenttype__unicode__.patch added |
---|
comment:1 Changed 7 years ago by
Needs tests: | set |
---|---|
Patch needs improvement: | set |
Triage Stage: | Unreviewed → Accepted |
comment:2 Changed 7 years ago by
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.
Changed 7 years ago by
Attachment: | 1627-contenttype__unicode__2.patch added |
---|
comment:3 Changed 7 years ago by
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:5 Changed 6 years ago by
Cc: | lemaire.adrien@… added |
---|---|
Resolution: | → fixed |
Status: | new → closed |
Triage Stage: | Accepted → Fixed on a branch |
Contenttype.unicode() has been modified with #16803.
You can close this ticket
comment:6 Changed 6 years ago by
Triage Stage: | Fixed on a branch → Accepted |
---|
comment:7 Changed 6 years ago by
Resolution: | fixed |
---|---|
Status: | closed → reopened |
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 Changed 6 years ago by
Owner: | changed from nobody to Adrien Lemaire |
---|---|
Status: | reopened → new |
comment:9 Changed 6 years ago by
@julien, changing the unicode representation defeat 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
comment:10 Changed 6 years ago by
Triage Stage: | Accepted → Design 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 Changed 5 years ago by
Has patch: | unset |
---|---|
Needs tests: | unset |
Patch needs improvement: | unset |
Triage Stage: | Design decision needed → Accepted |
Version: | 1.3 → master |
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 Changed 5 years ago by
@ptone:
When there are multiple models with the same class name (or Meta.verbose_name
). eg.:
app1.models.Article
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 Changed 5 years ago by
Owner: | changed from Adrien Lemaire to djangsters |
---|---|
Status: | new → assigned |
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 Changed 5 years ago by
Has patch: | set |
---|---|
Needs tests: | set |
comment:15 Changed 5 years ago by
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 Changed 5 years ago by
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>
comment:17 Changed 5 years ago by
indeed jakubs suggestion of using optgroups seems to solve the problem in a very elegant way
comment:18 Changed 5 years ago by
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 Changed 5 years ago by
Easy pickings: | unset |
---|
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.