[Django] #16027: Include app_label in ContentType.__unicode__
Django
----------------------------------+--------------------------------------
Reporter: jakub | Owner: nobody
Type: Cleanup/optimization | Status: new
Milestone: | Component: contrib.contenttypes
Version: 1.3 | Severity: Normal
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Easy pickings: 1
----------------------------------+--------------------------------------
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__.
--
Ticket URL: <http://code.djangoproject.com/ticket/16027>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: jakub | Owner: nobody
Type: | Status: new
Cleanup/optimization | Component:
Milestone: | contrib.contenttypes
Version: 1.3 | Severity: Normal
Resolution: | Keywords:
Triage Stage: Accepted | Has patch: 1
Needs documentation: 0 | Needs tests: 1
Patch needs improvement: 1 | Easy pickings: 1
-------------------------------------+-------------------------------------
Changes (by julien):
* needs_docs: => 0
* needs_better_patch: => 1
* needs_tests: => 1
* stage: Unreviewed => Accepted
Comment:
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.
--
Ticket URL: <http://code.djangoproject.com/ticket/16027#comment:1>
Django
Reporter: jakub | Owner: nobody
Cleanup/optimization | Component:
Version: 1.3 | Severity: Normal
Triage Stage: Accepted | Has patch: 1
Needs documentation: 0 | Needs tests: 1
Patch needs improvement: 1 | Easy pickings: 1
-------------------------------------+-------------------------------------
Comment (by jakub):
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.
--
Ticket URL: <http://code.djangoproject.com/ticket/16027#comment:2>
Django
Reporter: jakub | Owner: nobody
Cleanup/optimization | Component:
Version: 1.3 | Severity: Normal
Triage Stage: Accepted | Has patch: 1
Needs documentation: 0 | Needs tests: 1
Patch needs improvement: 1 | Easy pickings: 1
-------------------------------------+-------------------------------------
Comment (by julien):
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:
{{{#!python
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`.
--
Ticket URL: <http://code.djangoproject.com/ticket/16027#comment:3>
Django
Reporter: jakub | Owner: nobody
Type: | Status: closed
Cleanup/optimization | Version: 1.3
Component: | Resolution: fixed
contrib.contenttypes | Triage Stage: Fixed on
Severity: Normal | a branch
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 1 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Changes (by Fandekasp):
* status: new => closed
* cc: lemaire.adrien@… (added)
* resolution: => fixed
* stage: Accepted => Fixed on a branch
Comment:
Contenttype.__unicode__() has been modified with #16803.
You can close this ticket
--
Ticket URL: <https://code.djangoproject.com/ticket/16027#comment:5>
Django
Reporter: jakub | Owner: nobody
Type: Cleanup/optimization | Status: closed
Component: contrib.contenttypes | Version: 1.3
Severity: Normal | Resolution: fixed
Has patch: 1 | Needs documentation: 0
Easy pickings: 1 | UI/UX: 0
--------------------------------------+------------------------------------
Changes (by ramiro):
* stage: Fixed on a branch => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/16027#comment:6>
Django
Reporter: jakub | Owner: nobody
Type: Cleanup/optimization | Status: reopened
Component: contrib.contenttypes | Version: 1.3
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 1 | UI/UX: 0
--------------------------------------+------------------------------------
* status: closed => reopened
* resolution: fixed =>
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/16027#comment:7>
Django
Reporter: jakub | Owner: Fandekasp
Type: | Status: new
Component: | Resolution:
Severity: Normal | Needs documentation: 0
Keywords: | Patch needs improvement: 1
Has patch: 1 | UI/UX: 0
Needs tests: 1 |
-------------------------------------+-------------------------------------
Changes (by Fandekasp):
* status: reopened => new
* owner: nobody => Fandekasp
--
Ticket URL: <https://code.djangoproject.com/ticket/16027#comment:8>
Django
Reporter: jakub | Owner: Fandekasp
Type: | Status: new
Cleanup/optimization | Version: 1.3
Component: | Resolution:
contrib.contenttypes | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Keywords: | Patch needs improvement: 1
Has patch: 1 | UI/UX: 0
Needs tests: 1 |
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Comment (by Fandekasp):
@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
--
Ticket URL: <https://code.djangoproject.com/ticket/16027#comment:9>
Django
Reporter: jakub | Owner: Fandekasp
Type: | Status: new
Cleanup/optimization | Version: 1.3
Component: | Resolution:
Severity: Normal | decision needed
Has patch: 1 | Patch needs improvement: 1
Needs tests: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by julien):
* stage: Accepted => Design decision needed
Comment:
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).
--
Ticket URL: <https://code.djangoproject.com/ticket/16027#comment:10>
Django
Reporter: jakub | Owner: Fandekasp
Type: | Status: new
Component: | Resolution:
contrib.contenttypes | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Changes (by ptone):
* needs_better_patch: 1 => 0
* has_patch: 1 => 0
* version: 1.3 => master
* needs_tests: 1 => 0
* stage: Design decision needed => Accepted
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/16027#comment:11>
Django
Reporter: jakub | Owner: Fandekasp
Type: | Status: new
Cleanup/optimization | Version: master
Component: | Resolution:
contrib.contenttypes | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Keywords: | Patch needs improvement: 0
Has patch: 0 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Comment (by jakub):
@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>
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/16027#comment:12>
Django
Reporter: jakub | Owner:
Type: | djangsters
Cleanup/optimization | Status: assigned
Component: | Version: master
contrib.contenttypes | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Changes (by djangsters):
* status: new => assigned
* owner: Fandekasp => djangsters
Comment:
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
--
Ticket URL: <https://code.djangoproject.com/ticket/16027#comment:13>
Django
Reporter: jakub | Owner:
Type: | djangsters
Cleanup/optimization | Status: assigned
Component: | Version: master
contrib.contenttypes | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Needs tests: 1 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Changes (by code22):
* has_patch: 0 => 1
* needs_tests: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/16027#comment:14>
Django
Reporter: jakub | Owner:
Type: | djangsters
Cleanup/optimization | Status: assigned
Component: | Version: master
contrib.contenttypes | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 1 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Comment (by kevinbrolly):
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/16027#comment:15>
Django
Reporter: jakub | Owner:
Type: | djangsters
Cleanup/optimization | Status: assigned
Component: | Version: master
contrib.contenttypes | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 1 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Comment (by jakub):
> 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/16027#comment:16>
Django
Reporter: jakub | Owner:
Type: | djangsters
Cleanup/optimization | Status: assigned
Component: | Version: master
contrib.contenttypes | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 1 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Comment (by anonymous):
indeed jakubs suggestion of using optgroups seems to solve the problem in
a very elegant way
--
Ticket URL: <https://code.djangoproject.com/ticket/16027#comment:17>
Django
Reporter: jakub | Owner:
Type: | djangsters
Cleanup/optimization | Status: assigned
Component: | Version: master
contrib.contenttypes | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 1 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Comment (by djangsters):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/16027#comment:18>
Django
Reporter: jakub | Owner:
Type: | djangsters
Cleanup/optimization | Status: assigned
Component: | Version: master
contrib.contenttypes | Resolution:
Severity: Normal | Triage Stage: Accepted
Keywords: | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 0
Needs tests: 1 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by timo):
* easy: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/16027#comment:19>
Django
-------------------------------------+-------------------------------------
Reporter: Jakub Roztočil | Owner:
Type: | djangsters
Cleanup/optimization | Status: assigned
Component: | Version: master
contrib.contenttypes |
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Old 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__.
New 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.__str__()`.
--
Comment (by Tim Graham):
#30051 is a duplicate.
--
Ticket URL: <https://code.djangoproject.com/ticket/16027#comment:20>
Django
-------------------------------------+-------------------------------------
Reporter: Jakub Roztočil | Owner:
Type: | djangsters
Cleanup/optimization | Status: assigned
Component: | Version: master
contrib.contenttypes |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Greg Schmit):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/16027#comment:21>
Django
-------------------------------------+-------------------------------------
Type: | Schmit
Cleanup/optimization | Status: assigned
Component: | Version: master
contrib.contenttypes |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* cc: Greg Schmit (added)
* owner: djangsters => Greg Schmit
Comment:
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
--
Ticket URL: <https://code.djangoproject.com/ticket/16027#comment:22>
Django
-------------------------------------+-------------------------------------
Reporter: Jakub Roztočil | Owner: Greg
Type: | Schmit
Cleanup/optimization | Status: assigned
Component: | Version: master
contrib.contenttypes |
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Greg Schmit):
* stage: Accepted => Ready for checkin
Comment:
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
--
Ticket URL: <https://code.djangoproject.com/ticket/16027#comment:23>
Django
-------------------------------------+-------------------------------------
Reporter: Jakub Roztočil | Owner: Greg
Type: | Schmit
Cleanup/optimization | Status: assigned
Component: | Version: master
contrib.contenttypes |
Severity: Normal | Resolution:
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
* stage: Ready for checkin => Accepted
Comment:
Please don't mark your own patch as RFC;
[https://docs.djangoproject.com/en/2.1/internals/contributing/triaging-
tickets/#ready-for-checkin someone else has to review your changes] by
going through the
[https://docs.djangoproject.com/en/2.1/internals/contributing/writing-code
/submitting-patches/#patch-review-checklist review checklist].
--
Ticket URL: <https://code.djangoproject.com/ticket/16027#comment:24>
Django
-------------------------------------+-------------------------------------
Reporter: Jakub Roztočil | Owner: Greg
Type: | Schmit
Cleanup/optimization | Status: assigned
Component: | Version: master
contrib.contenttypes |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Greg Schmit):
Replying to [comment:24 Simon Charette]:
> Please don't mark your own patch as RFC;
[https://docs.djangoproject.com/en/2.1/internals/contributing/triaging-
tickets/#ready-for-checkin someone else has to review your changes] by
going through the
[https://docs.djangoproject.com/en/2.1/internals/contributing/writing-code
/submitting-patches/#patch-review-checklist review checklist].
Sorry about that! Won't happen again.
--
Ticket URL: <https://code.djangoproject.com/ticket/16027#comment:25>
Django
-------------------------------------+-------------------------------------
Reporter: Jakub Roztočil | Owner: Greg
Type: | Schmit
Component: | Version: master
contrib.contenttypes |
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"48c17807a99f7a4341c74db19e16a37b010827c2" 48c1780]:
{{{
#!CommitTicketReference repository=""
revision="48c17807a99f7a4341c74db19e16a37b010827c2"
Fixed #16027 -- Added app_label to ContentType.__str__().
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/16027#comment:26>