[Django] #29636: Failing admin.E108 system check after upgrading to Django 2.1
Django
-------------------------------------+-------------------------------------
Reporter: petrboros | Owner: nobody
Type: Bug | Status: new
Component: Core | Version: 2.1
(System checks) |
Severity: Normal | Keywords: SystemCheckError
Triage Stage: | Has patch: 0
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
After upgrading to Django 2.1, we have system checks failing due to
`admin.E108` `SystemCheckError`. This behavior was not present in Django
2.0.8. I don't see any relevant API change documented in the 2.1 Release
notes, therefore I'm filling this as a bug.
The error:
{{{
<class 'automation.admin.TriggerAdmin'>: (admin.E108) The value of
'list_display[3]' refers to 'position', which is not a callable, an
attribute of 'TriggerAdmin', or an attribute or method on
'automation.Trigger'.
}}}
The `automation.admin.TriggerAdmin` class:
{{{
from django.contrib import admin
from automation.models import Trigger
@admin.register(Trigger)
class TriggerAdmin(admin.ModelAdmin):
model = Trigger
list_display = ('company', 'trigger_type', 'action_type', 'position')
list_filter = ('company', 'trigger_type', 'action_type')
}}}
The `automation.models.Trigger` model (abbreviated):
{{{
from django.db import models
from core.fields import PositionField
class Trigger(models.Model):
position = PositionField(
verbose_name=_('Priority'),
collection='company'
)
(...)
}}}
The `core.fields.PositionField` field (abbreviated, originally taken from
the `django-positions` package):
{{{
from django.db import models
class PositionField(models.IntegerField):
(...)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29636>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: petrboros | Owner: nobody
Type: Bug | Status: new
checks) |
Severity: Normal | Resolution:
Keywords: SystemCheckError | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Old description:
New description:
After upgrading to Django 2.1, we have system checks failing due to
`admin.E108` `SystemCheckError`. This behavior was not present in Django
2.0.8. I don't see any relevant API change documented in the 2.1 Release
notes, therefore I'm filing this as a bug.
(...)
}}}
class PositionField(models.IntegerField):
(...)
}}}
--
--
Ticket URL: <https://code.djangoproject.com/ticket/29636#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: Petr Boros | Owner: nobody
Type: Bug | Status: new
Component: Core (System | Version: 2.1
checks) |
Severity: Normal | Resolution:
Keywords: SystemCheckError | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
--
Ticket URL: <https://code.djangoproject.com/ticket/29636#comment:2>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: Core (System | Version: 2.1
checks) |
Severity: Normal | Resolution:
Keywords: SystemCheckError | Triage Stage:
| Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Petr Boros):
I investigated Django source code and believe this is a regression
introduced in commit `47016ad`
(https://github.com/django/django/commit/47016adbf54b54143d4cf052eeb29fc72d27e6b1),
specifically in the `else` branch starting on line `673`.
This is the current code present in 2.1:
{{{
else:
return [
checks.Error(
"The value of '%s' refers to '%s', which is not a
callable, "
"an attribute of '%s', or an attribute or method on
'%s.%s'." % (
label, item, obj.__class__.__name__,
model._meta.app_label, model._meta.object_name,
),
obj=obj.__class__,
id='admin.E108',
)
]
}}}
I modified it to match the original code before the alleged regression,
that is:
{{{
else:
try:
model._meta.get_field(item)
except FieldDoesNotExist:
return [
checks.Error(
"The value of '%s' refers to '%s', which is not a
callable, "
"an attribute of '%s', or an attribute or method
on '%s.%s'." % (
label, item, obj.__class__.__name__,
model._meta.app_label,
model._meta.object_name,
),
obj=obj.__class__,
id='admin.E108',
)
]
else:
return []
}}}
and the checks are not failing.
If this is indeed an unintended regression, I'd be happy to submit a PR.
--
Ticket URL: <https://code.djangoproject.com/ticket/29636#comment:3>
Django
----------------------------------+--------------------------------------
Reporter: Petr Boros | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 2.1
Severity: Normal | Resolution:
Keywords: SystemCheckError | Triage Stage: Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Petr Boros):
* component: Core (System checks) => contrib.admin
--
Ticket URL: <https://code.djangoproject.com/ticket/29636#comment:4>
Django
----------------------------------+--------------------------------------
Type: Bug | Status: new
Component: contrib.admin | Version: 2.1
Severity: Normal | Resolution:
Keywords: SystemCheckError | Triage Stage: Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham):
I think the report is missing some detail to reproduce the issue (perhaps
some implementation detail of `PositionField`). For your case, it looks
like
[https://github.com/django/django/blob/89d4d412404d31ef34ae3170c0c056eff55b2a17/django/contrib/admin/checks.py#L658
hasattr(Trigger, 'position')] is failing which isn't the case for normal
model fields.
--
Ticket URL: <https://code.djangoproject.com/ticket/29636#comment:5>
Django
----------------------------------+--------------------------------------
Type: Bug | Status: new
Component: contrib.admin | Version: 2.1
Severity: Normal | Resolution:
Keywords: SystemCheckError | Triage Stage: Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Petr Boros):
Replying to [comment:5 Tim Graham]:
> I think the report is missing some detail to reproduce the issue
(perhaps some implementation detail of `PositionField`). For your case, it
looks like
[https://github.com/django/django/blob/89d4d412404d31ef34ae3170c0c056eff55b2a17/django/contrib/admin/checks.py#L658
hasattr(Trigger, 'position')] is failing which isn't the case for normal
model fields.
You are right, `hasattr(Trigger, 'position') == False`. However, at the
same time, `'position' in Trigger.__dict__.keys() == True`.
Why is that?
I am attaching the full source code of `PositionField` to this ticket for
reference. ''Note that the original source code comes from `django-
positions` package.''
--
Ticket URL: <https://code.djangoproject.com/ticket/29636#comment:6>
Django
----------------------------------+--------------------------------------
Type: Bug | Status: new
Component: contrib.admin | Version: 2.1
Severity: Normal | Resolution:
Keywords: SystemCheckError | Triage Stage: Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Petr Boros):
* Attachment "position_field.py" added.
PositionField source
Django
----------------------------------+--------------------------------------
Type: Bug | Status: closed
Component: contrib.admin | Version: 2.1
Severity: Normal | Resolution: invalid
Keywords: SystemCheckError | Triage Stage: Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Simon Charette):
* status: new => closed
* resolution: => invalid
Comment:
Hey Petr,
The issue is that `PositionField.__get__` raises an attribute error when
`instance=None`, that is when accessed at the model class level. That
makes `hasattr(Trigger, 'position')` return `False`.
You'll want to fix that to `return self` I believe.
--
Ticket URL: <https://code.djangoproject.com/ticket/29636#comment:7>
Django
----------------------------------+--------------------------------------
Reporter: Petr Boros | Owner: nobody
Type: Bug | Status: closed
Component: contrib.admin | Version: 2.1
Severity: Normal | Resolution: invalid
Keywords: SystemCheckError | Triage Stage: Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Petr Boros):
Replying to [comment:7 Simon Charette]:
Hey Simon,
you're right. Thank you very much.
--
Ticket URL: <https://code.djangoproject.com/ticket/29636#comment:8>