Code

Opened 9 months ago

Closed 8 months ago

#20777 closed Bug (fixed)

admin delete page proxy models

Reported by: CollinAnderson Owned by: nobody
Component: contrib.admin Version: 1.6-beta-1
Severity: Normal Keywords:
Cc: CollinAnderson 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

Attachments (0)

Change History (10)

comment:1 Changed 9 months ago by Harm Geerts <hgeerts@…>

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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 Changed 9 months ago by CollinAnderson

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

comment:3 Changed 9 months ago by Harm Geerts <hgeerts@…>

  • Has patch set

comment:4 Changed 9 months ago by timo

  • Triage Stage changed from Unreviewed to Accepted

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 Changed 8 months ago 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:

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 Changed 8 months ago by timo

  • 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 Changed 8 months ago 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.

comment:8 Changed 8 months ago 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.

comment:9 Changed 8 months ago by timo

  • Patch needs improvement unset
  • Severity changed from Release blocker to Normal
  • Triage Stage changed from Accepted to Ready 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 Changed 8 months ago by akaariai

  • Resolution set to fixed
  • Status changed from new to closed

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.