Opened 11 years ago

Closed 11 years ago

#20777 closed Bug (fixed)

admin delete page proxy models

Reported by: Collin Anderson Owned by: nobody
Component: contrib.admin Version: 1.6-beta-1
Severity: Normal Keywords:
Cc: Collin Anderson Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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

Change History (10)

comment:1 by Harm Geerts <hgeerts@…>, 11 years ago

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

comment:2 by Collin Anderson, 11 years ago

I just checked both and either one would solve my problem.

comment:3 by Harm Geerts <hgeerts@…>, 11 years ago

Has patch: set

comment:4 by Tim Graham, 11 years ago

Triage Stage: UnreviewedAccepted

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?

comment:5 by Harm Geerts <hgeerts@…>, 11 years ago

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:

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.

comment:6 by Tim Graham, 11 years ago

Patch needs improvement: set

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.

comment:7 by Tim Graham, 11 years ago

@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.

comment:8 by Anssi Kääriäinen, 11 years ago

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.

comment:9 by Tim Graham, 11 years ago

Patch needs improvement: unset
Severity: Release blockerNormal
Triage Stage: AcceptedReady for checkin

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.

comment:10 by Anssi Kääriäinen, 11 years ago

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.
Back to Top