[Django] #29132: Incorrect update_last_login signal handler behavior when using custom User model with an arbitrary last_login attribute (not a Field instance)
Django
model with an arbitrary last_login attribute (not a Field instance)
-------------------------------------+-------------------------------------
Reporter: Mikhail | Owner: nobody
Porokhovnichenko |
Type: Bug | Status: new
Component: | Version: 2.0
contrib.auth | Keywords: last_login,
Severity: Normal | update_last_login, signals,
Triage Stage: | user_logged_in
Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
**Please note! It's not a #26823 duplicate, and it's just an issue
related this one.**
In the current implementation, a signal handler connects with a user model
when this model has a `last_login` field.
{{{
#!python
if hasattr(get_user_model(), 'last_login'):
from .models import update_last_login
user_logged_in.connect(update_last_login,
dispatch_uid='update_last_login')
}}}
And it would work when there isn't a `last_login` attribute in the custom
user model. But what if I've inherited my custom model from `AbstractUser`
and dropped the `last_login` by setting it to `None`?
{{{
#!python
class User(AbstractBaseUser):
last_login = None # Drop ``last_login`` field off
}}}
Technically, the model has a ``last_login`` attribute, but it's not a
field!
Suggesting change the check condition something like that:
{{{
#!python
last_login = getattr(get_user_model(), 'last_login', None)
if last_login is not None:
# ...
}}}
or even
{{{
#!python
if isisintance(last_login, models.Field):
# ...
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29132>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
model with an arbitrary last_login attribute (not a Field instance)
-------------------------------------+-------------------------------------
Reporter: Mikhail | Owner: nobody
Porokhovnichenko |
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: last_login, | Triage Stage:
update_last_login, signals, | Unreviewed
user_logged_in |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Old description:
New description:
--
--
Ticket URL: <https://code.djangoproject.com/ticket/29132#comment:1>
Django
attribute isn't a field
-------------------------------------+-------------------------------------
Reporter: Mikhail | Owner: nobody
Porokhovnichenko |
Type: Bug | Status: new
Severity: Normal | Resolution:
update_last_login, signals, |
user_logged_in |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Unreviewed => Accepted
Comment:
I was thinking that `isinstance(... Field)` might be incorrect if
`last_login` was a settable property, but in that case, I guess
`user.save(update_fields['last_login'])` probably wouldn't be desired.
--
Ticket URL: <https://code.djangoproject.com/ticket/29132#comment:2>
Django
attribute isn't a field
Reporter: Mikhail | Owner: nobody
Porokhovnichenko |
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: last_login, | Triage Stage: Accepted
update_last_login, signals, |
user_logged_in |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mikhail Porokhovnichenko):
Sorry, I didn't get it. What approach you'd like to use here? Need I
create a patch?
--
Ticket URL: <https://code.djangoproject.com/ticket/29132#comment:3>
Django
attribute isn't a field
Reporter: Mikhail | Owner: nobody
Porokhovnichenko |
Type: Bug | Status: new
Severity: Normal | Resolution:
Keywords: last_login, | Triage Stage: Accepted
update_last_login, signals, |
user_logged_in |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham):
Try the `isinstance()` approach. You're welcome to create a patch. Most of
the work will be writing a test.
--
Ticket URL: <https://code.djangoproject.com/ticket/29132#comment:4>
Django
attribute isn't a field
-------------------------------------+-------------------------------------
Porokhovnichenko | Porokhovnichenko
Type: Bug | Status: assigned
Component: contrib.auth | Version: 2.0
Severity: Normal | Resolution:
Keywords: last_login, | Triage Stage: Accepted
update_last_login, signals, |
user_logged_in |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* owner: nobody => Mikhail Porokhovnichenko
* status: new => assigned
Comment:
Ok, will do it ASAP
--
Ticket URL: <https://code.djangoproject.com/ticket/29132#comment:5>
Django
attribute isn't a field
-------------------------------------+-------------------------------------
Reporter: Mikhail | Owner: Mikhail
Porokhovnichenko | Porokhovnichenko
Type: Bug | Status: assigned
Component: contrib.auth | Version: 2.0
Severity: Normal | Resolution:
Keywords: last_login, | Triage Stage: Accepted
update_last_login, signals, |
user_logged_in |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Mikhail Porokhovnichenko):
Seems to be done. Can be reviewed
[https://github.com/marazmiki/django/tree/ticket_29132 here]
--
Ticket URL: <https://code.djangoproject.com/ticket/29132#comment:6>
Django
attribute isn't a field
-------------------------------------+-------------------------------------
Reporter: Mikhail | Owner: Mikhail
Porokhovnichenko | Porokhovnichenko
Type: Bug | Status: assigned
Component: contrib.auth | Version: 2.0
Severity: Normal | Resolution:
Keywords: last_login, | Triage Stage: Accepted
update_last_login, signals, |
user_logged_in |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* has_patch: 0 => 1
Comment:
Please check [https://github.com/django/django/pull/9712 PR#9712].
--
Ticket URL: <https://code.djangoproject.com/ticket/29132#comment:7>
Django
attribute isn't a field
-------------------------------------+-------------------------------------
Reporter: Mikhail | Owner: Mikhail
Porokhovnichenko | Porokhovnichenko
Type: Bug | Status: assigned
Component: contrib.auth | Version: 2.0
Severity: Normal | Resolution:
update_last_login, signals, | checkin
user_logged_in |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/29132#comment:8>
Django
attribute isn't a field
-------------------------------------+-------------------------------------
Reporter: Mikhail | Owner: Mikhail
Porokhovnichenko | Porokhovnichenko
Component: contrib.auth | Version: 2.0
Severity: Normal | Resolution: fixed
Keywords: last_login, | Triage Stage: Ready for
update_last_login, signals, | checkin
user_logged_in |
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"14e34dcf8cb935454f4ce02402949d8af204fdab" 14e34dcf]:
{{{
#!CommitTicketReference repository=""
revision="14e34dcf8cb935454f4ce02402949d8af204fdab"
Fixed #29132 -- Avoided connecting update_last_login() handler if
User.last_login isn't a field.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/29132#comment:9>