[Django] #20777: admin delete page proxy models
Django
---------------------------------+------------------------
Reporter: CollinAnderson | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 1.6-beta-1
Severity: Release blocker | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
---------------------------------+------------------------
This #18491 broke proxy model `__unicode__()` on the admin delete page:
https://github.com/django/django/commit/2b48fcc607010065c0f8107baf669dd41b164f3c
The concrete model is being instantiated with no attributes except the
primary key. This breaks `obj.__unicode__()` because it can't reference
any of its fields. (It also means that we're displaying
`BaseModel.__unicode__()` instead of `ProxyModel.__unicode__()`, which
isn't a problem for me.)
I have a simple project you can use to try it yourself:
https://github.com/collinanderson/proxyadmin
--
Ticket URL: <https://code.djangoproject.com/ticket/20777>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: CollinAnderson | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 1.6-beta-1
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Harm Geerts <hgeerts@…>):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
I'm not sure what the desired fix is but I have created 2 possible
solutions.
keep the proxy model representation:
https://github.com/django/django/pull/1435
copy the instance dict to the concrete model:
https://github.com/django/django/pull/1436
--
Ticket URL: <https://code.djangoproject.com/ticket/20777#comment:1>
Django
Reporter: CollinAnderson | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 1.6-beta-1
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by CollinAnderson):
I just checked both and either one would solve my problem.
--
Ticket URL: <https://code.djangoproject.com/ticket/20777#comment:2>
Django
Reporter: CollinAnderson | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 1.6-beta-1
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Harm Geerts <hgeerts@…>):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/20777#comment:3>
Django
Reporter: CollinAnderson | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 1.6-beta-1
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timo):
* stage: Unreviewed => Accepted
Comment:
I think the first solution makes the most sense. The model's
`__str__/__unicode__` method may refer to fields that aren't on the model
which would mean the second solution wouldn't work, right?
I'm getting a test failure with Python 3 with the first solution:
{{{
======================================================================
FAIL: test_delete_str_in_model_admin
(proxy_models.tests.ProxyModelAdminTests)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/django/tests/proxy_models/tests.py", line 406, in
test_delete_str_in_model_admin
self.assertEqual(delete_str, proxy_str)
AssertionError: 'Tracker user: <a
href="/admin/proxy_models/trackeruser/100/">TrackerUser:</a>' != 'Tracker
user: <a
href="/admin/proxy_models/trackeruser/100/">ProxyTrackerUser:Django
Pony</a>'
}}}
Changing `obj.__str__` to `obj.__unicode__` in the `if six.PY3` branch
fixes this. Possibly we should set `obj.__unicode__` regardless of Python
version and `obj.__str__` only for Python 3?
--
Ticket URL: <https://code.djangoproject.com/ticket/20777#comment:4>
Django
Reporter: CollinAnderson | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 1.6-beta-1
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Harm Geerts <hgeerts@…>):
I don't think it's possible to define fields on a proxy model but it could
refer to a property that is not defined on the base model itself. For
example:
{{{#!python
class Base(Model):
def __unicode__(self):
return self.kind
class Pony(Base):
kind = 'pony'
class Meta:
proxy = True
}}}
Not sure if this kind of behavior should be supported but it's possible
and would break the second solution.
I'll update the first solution for python 3.
--
Ticket URL: <https://code.djangoproject.com/ticket/20777#comment:5>
Django
Reporter: CollinAnderson | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 1.6-beta-1
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timo):
* needs_better_patch: 0 => 1
Comment:
As discussed on IRC, the first solution may not work as `obj.__unicode__`
and `obj.__str___` invoke the class-level methods and cannot be overridden
on individual instances. The test works for Python 2 only because of the
implementation details of `@python_2_unicode_compatible` which calls
`self.__unicode__()`. Arguably, this should be `klass.__unicode__()` to
mimic the behavior of Python.
--
Ticket URL: <https://code.djangoproject.com/ticket/20777#comment:6>
Django
Reporter: CollinAnderson | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 1.6-beta-1
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by timo):
@akaariai says fixing #16458 will mean we can revert the fix for #18491
which caused this regression. Possibly we should just revert it now, live
with that bug in 1.6, and wait for #16458 to be fixed in 1.7.
--
Ticket URL: <https://code.djangoproject.com/ticket/20777#comment:7>
Django
Reporter: CollinAnderson | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 1.6-beta-1
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Comment (by akaariai):
Pull request https://github.com/django/django/pull/1478 seems to fix this
issue. However the patch can't be applied to 1.6 at this point as it
introduces backwards incompatible changes.
--
Ticket URL: <https://code.djangoproject.com/ticket/20777#comment:8>
Django
Reporter: CollinAnderson | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version:
Keywords: | Resolution:
Has patch: 1 | Triage Stage: Ready for
Needs tests: 0 | checkin
Easy pickings: 0 | Needs documentation: 0
| Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by timo):
* needs_better_patch: 1 => 0
* severity: Release blocker => Normal
* stage: Accepted => Ready for checkin
Comment:
PR looks good. I've reverted the fix for #18491 which caused this
regression in 1.6.x [c769c266017c]. Removing the release blocker flag in
light of that.
--
Ticket URL: <https://code.djangoproject.com/ticket/20777#comment:9>
Django
Reporter: CollinAnderson | Owner: nobody
Type: Bug | Status: closed
Component: contrib.admin | Version:
Severity: Normal | 1.6-beta-1
Has patch: 1 | Triage Stage: Ready for
Needs tests: 0 | checkin
Easy pickings: 0 | Needs documentation: 0
| Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => fixed
Comment:
Fixed in 3844089edc43ff29aab5bac82a0eecab23d8d14a
--
Ticket URL: <https://code.djangoproject.com/ticket/20777#comment:10>