Skip to content

Loading…

Fixed #19019 -- logging change in UserAdmin #464

Closed
wants to merge 1 commit into from

6 participants

@fanatid

When we editing user in UserAdmin and don't modify any fileds in admin_log will added "Changed password" whereas should be "No fields changed". Also nothing is added in admin_log when we change user password.

@apollo13
Django member

Patch misses tests. (Also: what's up with the changed_data, why is that needed?)

@fanatid

If don't modify changed_data in AdminPasswordChangeForm then in admin log will added row "Changed password1 and password2", whereas we have field password in model, but not password1 and password2.

@Rizach

With the patch I get this result in mysql:

| 1 | 2012-11-17 16:21:03 | 1 | 3 | 1 | admin | 2 | Changed password.

@rybaktomasz

After patching Admin behaves OK - logs password changes and correctly logs lack of changes.

@galuszkak

Tests are passing again. I think it could be merged in to master.

@timgraham timgraham commented on the diff
django/contrib/admin/fixtures/admintestdata.json
@@ -0,0 +1,20 @@
+[
@timgraham Django member

the changes to fix the issue are in django.contrib.auth, yet the tests are in django.contrib.admin. I think they should live in a new file, something like django.contrib.auth.tests.test_admin.py instead

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timgraham timgraham commented on the diff
django/contrib/admin/tests/views.py
((4 lines not shown))
+from django.test.utils import override_settings
+
+from django.contrib.auth import SESSION_KEY
+
+
+@override_settings(
+ LANGUAGES=(
+ ('en', 'English'),
+ ),
+ LANGUAGE_CODE='en',
+ USE_TZ=False,
+ PASSWORD_HASHERS=('django.contrib.auth.hashers.SHA1PasswordHasher',),
+)
+class AdminViewsTestCase(TestCase):
+ """
+ Helper base class for all the follow test cases.
@timgraham Django member

follow -> following

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timgraham timgraham commented on the diff
django/contrib/admin/tests/views.py
((51 lines not shown))
+ return {
+ 'username': user.username,
+ 'password': user.password,
+ 'email': user.email,
+ 'is_active': user.is_active,
+ 'is_staff': user.is_staff,
+ 'is_superuser': user.is_superuser,
+ 'last_login_0': user.last_login.strftime('%Y-%m-%d'),
+ 'last_login_1': user.last_login.strftime('%H:%M:%S'),
+ 'initial-last_login_0': user.last_login.strftime('%Y-%m-%d'),
+ 'initial-last_login_1': user.last_login.strftime('%H:%M:%S'),
+ 'date_joined_0': user.date_joined.strftime('%Y-%m-%d'),
+ 'date_joined_1': user.date_joined.strftime('%H:%M:%S'),
+ 'initial-date_joined_0': user.date_joined.strftime('%Y-%m-%d'),
+ 'initial-date_joined_1': user.date_joined.strftime('%H:%M:%S'),
+ }
@timgraham Django member

deindent }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timgraham timgraham commented on the diff
django/contrib/admin/tests/views.py
((62 lines not shown))
+ 'date_joined_0': user.date_joined.strftime('%Y-%m-%d'),
+ 'date_joined_1': user.date_joined.strftime('%H:%M:%S'),
+ 'initial-date_joined_0': user.date_joined.strftime('%Y-%m-%d'),
+ 'initial-date_joined_1': user.date_joined.strftime('%H:%M:%S'),
+ }
+
+ def test_user_change_email(self):
+ self.login()
+ data = self.get_user_data(self.admin)
+ data['email'] = 'new_' + data['email']
+ response = self.client.post(
+ '/admin/auth/user/%s/' % self.admin.pk,
+ data
+ )
+ self.assertEqual(response.status_code, 302)
+ row = LogEntry.objects.reverse()[:1].get()
@timgraham Django member

LogEntry.objects.latest()?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timgraham timgraham commented on the diff
django/contrib/admin/tests/views.py
((71 lines not shown))
+ data['email'] = 'new_' + data['email']
+ response = self.client.post(
+ '/admin/auth/user/%s/' % self.admin.pk,
+ data
+ )
+ self.assertEqual(response.status_code, 302)
+ row = LogEntry.objects.reverse()[:1].get()
+ self.assertEqual(row.change_message, 'Changed email.')
+
+ def test_user_not_change(self):
+ self.login()
+ response = self.client.post(
+ '/admin/auth/user/%s/' % self.admin.pk,
+ self.get_user_data(self.admin)
+ )
+ self.assertEqual(response.status_code, 302)
@timgraham Django member

assertRedirects()?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timgraham timgraham commented on the diff
django/contrib/admin/tests/views.py
((72 lines not shown))
+ response = self.client.post(
+ '/admin/auth/user/%s/' % self.admin.pk,
+ data
+ )
+ self.assertEqual(response.status_code, 302)
+ row = LogEntry.objects.reverse()[:1].get()
+ self.assertEqual(row.change_message, 'Changed email.')
+
+ def test_user_not_change(self):
+ self.login()
+ response = self.client.post(
+ '/admin/auth/user/%s/' % self.admin.pk,
+ self.get_user_data(self.admin)
+ )
+ self.assertEqual(response.status_code, 302)
+ row = LogEntry.objects.reverse()[:1].get()
@timgraham Django member

latest()?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timgraham timgraham commented on the diff
django/contrib/auth/forms.py
@@ -44,6 +44,9 @@ def render(self, name, value, attrs):
return format_html("<div{0}>{1}</div>", flatatt(final_attrs), summary)
+ def _has_changed(self, initial, data):
@timgraham Django member

According to the 1.6 release notes, the _has_changed method on widgets has been deprecated, so I don't think this should be necessary? "If you defined your own form widgets and defined the _has_changed method on a widget, you should now define this method on the form field itself." (looks like ReadOnlyPasswordHashField already has _has_changed implemented)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timgraham
Django member

Updated pull request: #1380

@timgraham timgraham closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 21, 2013
  1. @fanatid
This page is out of date. Refresh to see the latest.
View
20 django/contrib/admin/fixtures/admintestdata.json
@@ -0,0 +1,20 @@
+[
@timgraham Django member

the changes to fix the issue are in django.contrib.auth, yet the tests are in django.contrib.admin. I think they should live in a new file, something like django.contrib.auth.tests.test_admin.py instead

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ {
+ "pk": "1",
+ "model": "auth.user",
+ "fields": {
+ "username": "admin",
+ "first_name": "",
+ "last_name": "",
+ "is_active": true,
+ "is_superuser": true,
+ "is_staff": true,
+ "last_login": "2006-12-17 07:03:31",
+ "groups": [],
+ "user_permissions": [],
+ "password": "sha1$6efc0$f93efe9fd7542f25a7be94871ea45aa95de57161",
+ "email": "admin@example.com",
+ "date_joined": "2006-12-17 07:03:31"
+ }
+ }
+]
View
4 django/contrib/admin/tests/__init__.py
@@ -0,0 +1,4 @@
+from django.contrib.admin.tests.selenium import *
+from django.contrib.admin.tests.views import *
+
+# The password for the fixture data users is 'password'
View
0 django/contrib/admin/tests.py → django/contrib/admin/tests/selenium.py
File renamed without changes.
View
10 django/contrib/admin/tests/urls.py
@@ -0,0 +1,10 @@
+from django.conf.urls import patterns, include, url
+from django.contrib.auth.urls import urlpatterns
+from django.contrib import admin
+
+
+admin.autodiscover()
+
+urlpatterns = urlpatterns + patterns('',
+ url(r'^admin/', include(admin.site.urls)),
+)
View
103 django/contrib/admin/tests/views.py
@@ -0,0 +1,103 @@
+from django.contrib.auth.models import User
+from django.contrib.admin.models import LogEntry
+from django.test import TestCase
+from django.test.utils import override_settings
+
+from django.contrib.auth import SESSION_KEY
+
+
+@override_settings(
+ LANGUAGES=(
+ ('en', 'English'),
+ ),
+ LANGUAGE_CODE='en',
+ USE_TZ=False,
+ PASSWORD_HASHERS=('django.contrib.auth.hashers.SHA1PasswordHasher',),
+)
+class AdminViewsTestCase(TestCase):
+ """
+ Helper base class for all the follow test cases.
@timgraham Django member

follow -> following

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ """
+ fixtures = ['admintestdata.json']
+ urls = 'django.contrib.admin.tests.urls'
+
+ @property
+ def admin(self):
+ if getattr(self, '_admin', None) is None:
+ try:
+ setattr(self, '_admin', User.objects.get(pk=1))
+ except User.DoesNotExist:
+ self.fail("Fail load fixture.")
+ return getattr(self, '_admin')
+
+ def login(self, password='password'):
+ response = self.client.post('/admin/', {
+ 'username': self.admin.username,
+ 'password': password,
+ 'this_is_the_login_form': '1',
+ })
+ self.assertEqual(response.status_code, 302)
+ self.assertTrue(SESSION_KEY in self.client.session)
+
+ def logout(self):
+ response = self.client.get('/admin/logout/')
+ self.assertEqual(response.status_code, 200)
+ self.assertTrue(SESSION_KEY not in self.client.session)
+
+
+class AdminAuthUserLogEntryTest(AdminViewsTestCase):
+
+ def get_user_data(self, user):
+ return {
+ 'username': user.username,
+ 'password': user.password,
+ 'email': user.email,
+ 'is_active': user.is_active,
+ 'is_staff': user.is_staff,
+ 'is_superuser': user.is_superuser,
+ 'last_login_0': user.last_login.strftime('%Y-%m-%d'),
+ 'last_login_1': user.last_login.strftime('%H:%M:%S'),
+ 'initial-last_login_0': user.last_login.strftime('%Y-%m-%d'),
+ 'initial-last_login_1': user.last_login.strftime('%H:%M:%S'),
+ 'date_joined_0': user.date_joined.strftime('%Y-%m-%d'),
+ 'date_joined_1': user.date_joined.strftime('%H:%M:%S'),
+ 'initial-date_joined_0': user.date_joined.strftime('%Y-%m-%d'),
+ 'initial-date_joined_1': user.date_joined.strftime('%H:%M:%S'),
+ }
@timgraham Django member

deindent }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
+ def test_user_change_email(self):
+ self.login()
+ data = self.get_user_data(self.admin)
+ data['email'] = 'new_' + data['email']
+ response = self.client.post(
+ '/admin/auth/user/%s/' % self.admin.pk,
+ data
+ )
+ self.assertEqual(response.status_code, 302)
+ row = LogEntry.objects.reverse()[:1].get()
@timgraham Django member

LogEntry.objects.latest()?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ self.assertEqual(row.change_message, 'Changed email.')
+
+ def test_user_not_change(self):
+ self.login()
+ response = self.client.post(
+ '/admin/auth/user/%s/' % self.admin.pk,
+ self.get_user_data(self.admin)
+ )
+ self.assertEqual(response.status_code, 302)
@timgraham Django member

assertRedirects()?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ row = LogEntry.objects.reverse()[:1].get()
@timgraham Django member

latest()?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ self.assertEqual(row.change_message, 'No fields changed.')
+
+ def test_user_change_password(self):
+ self.login()
+ response = self.client.post(
+ '/admin/auth/user/%s/password/' % self.admin.pk,
+ {
+ 'password1': 'password1',
+ 'password2': 'password1',
+ }
+ )
+ self.assertEqual(response.status_code, 302)
+ row = LogEntry.objects.reverse()[:1].get()
+ self.assertEqual(row.change_message, 'Changed password.')
+ self.logout()
+ self.login(password='password1')
View
2 django/contrib/auth/admin.py
@@ -123,6 +123,8 @@ def user_change_password(self, request, id, form_url=''):
form = self.change_password_form(user, request.POST)
if form.is_valid():
form.save()
+ change_message = self.construct_change_message(request, form, None)
+ self.log_change(request, request.user, change_message)
msg = ugettext('Password changed successfully.')
messages.success(request, msg)
return HttpResponseRedirect('..')
View
11 django/contrib/auth/forms.py
@@ -44,6 +44,9 @@ def render(self, name, value, attrs):
return format_html("<div{0}>{1}</div>", flatatt(final_attrs), summary)
+ def _has_changed(self, initial, data):
@timgraham Django member

According to the 1.6 release notes, the _has_changed method on widgets has been deprecated, so I don't think this should be necessary? "If you defined your own form widgets and defined the _has_changed method on a widget, you should now define this method on the form field itself." (looks like ReadOnlyPasswordHashField already has _has_changed implemented)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ return False
+
class ReadOnlyPasswordHashField(forms.Field):
widget = ReadOnlyPasswordHashWidget
@@ -345,3 +348,11 @@ def save(self, commit=True):
if commit:
self.user.save()
return self.user
+
+ def _get_changed_data(self):
+ data = super(AdminPasswordChangeForm, self).changed_data
+ for name in self.fields.keys():
+ if name not in data:
+ return []
+ return ['password']
+ changed_data = property(_get_changed_data)
Something went wrong with that request. Please try again.