From cf7894be88f6c27680ef80796b883f6e3b709b8b Mon Sep 17 00:00:00 2001 From: Claude Paroz Date: Sat, 26 Dec 2015 19:51:22 +0100 Subject: [PATCH] Fixed #21113 -- Made LogEntry.change_message language independent Thanks Tim Graham for the review. --- django/contrib/admin/models.py | 48 +++++++++- django/contrib/admin/options.py | 45 ++++++---- .../admin/templates/admin/object_history.html | 2 +- docs/ref/contrib/admin/index.txt | 21 ++++- docs/releases/1.10.txt | 9 ++ tests/admin_utils/admin.py | 13 ++- tests/admin_utils/models.py | 5 +- tests/admin_utils/test_logentry.py | 88 ++++++++++++++++++- tests/admin_utils/tests.py | 8 +- tests/admin_views/tests.py | 2 +- tests/auth_tests/test_views.py | 10 +-- 11 files changed, 216 insertions(+), 35 deletions(-) diff --git a/django/contrib/admin/models.py b/django/contrib/admin/models.py index 404fc6291f..14304e979d 100644 --- a/django/contrib/admin/models.py +++ b/django/contrib/admin/models.py @@ -1,5 +1,7 @@ from __future__ import unicode_literals +import json + from django.conf import settings from django.contrib.admin.utils import quote from django.contrib.contenttypes.models import ContentType @@ -7,6 +9,7 @@ from django.db import models from django.urls import NoReverseMatch, reverse from django.utils import timezone from django.utils.encoding import python_2_unicode_compatible, smart_text +from django.utils.text import get_text_list from django.utils.translation import ugettext, ugettext_lazy as _ ADDITION = 1 @@ -18,6 +21,8 @@ class LogEntryManager(models.Manager): use_in_migrations = True def log_action(self, user_id, content_type_id, object_id, object_repr, action_flag, change_message=''): + if isinstance(change_message, list): + change_message = json.dumps(change_message) self.model.objects.create( user_id=user_id, content_type_id=content_type_id, @@ -50,6 +55,7 @@ class LogEntry(models.Model): # Translators: 'repr' means representation (https://docs.python.org/3/library/functions.html#repr) object_repr = models.CharField(_('object repr'), max_length=200) action_flag = models.PositiveSmallIntegerField(_('action flag')) + # change_message is either a string or a JSON structure change_message = models.TextField(_('change message'), blank=True) objects = LogEntryManager() @@ -69,7 +75,7 @@ class LogEntry(models.Model): elif self.is_change(): return ugettext('Changed "%(object)s" - %(changes)s') % { 'object': self.object_repr, - 'changes': self.change_message, + 'changes': self.get_change_message(), } elif self.is_deletion(): return ugettext('Deleted "%(object)s."') % {'object': self.object_repr} @@ -85,6 +91,46 @@ class LogEntry(models.Model): def is_deletion(self): return self.action_flag == DELETION + def get_change_message(self): + """ + If self.change_message is a JSON structure, interpret it as a change + string, properly translated. + """ + if self.change_message and self.change_message[0] == '[': + try: + change_message = json.loads(self.change_message) + except ValueError: + return self.change_message + messages = [] + for sub_message in change_message: + if 'added' in sub_message: + if sub_message['added']: + sub_message['added']['name'] = ugettext(sub_message['added']['name']) + messages.append(ugettext('Added {name} "{object}".').format(**sub_message['added'])) + else: + messages.append(ugettext('Added.')) + + elif 'changed' in sub_message: + sub_message['changed']['fields'] = get_text_list( + sub_message['changed']['fields'], ugettext('and') + ) + if 'name' in sub_message['changed']: + sub_message['changed']['name'] = ugettext(sub_message['changed']['name']) + messages.append(ugettext('Changed {fields} for {name} "{object}".').format( + **sub_message['changed'] + )) + else: + messages.append(ugettext('Changed {fields}.').format(**sub_message['changed'])) + + elif 'deleted' in sub_message: + sub_message['deleted']['name'] = ugettext(sub_message['deleted']['name']) + messages.append(ugettext('Deleted {name} "{object}".').format(**sub_message['deleted'])) + + change_message = ' '.join(msg[0].upper() + msg[1:] for msg in messages) + return change_message or ugettext('No fields changed.') + else: + return self.change_message + def get_edited_object(self): "Returns the edited object represented by this log entry" return self.content_type.get_object_for_this_type(pk=self.object_id) diff --git a/django/contrib/admin/options.py b/django/contrib/admin/options.py index a09c958282..3361679591 100644 --- a/django/contrib/admin/options.py +++ b/django/contrib/admin/options.py @@ -44,7 +44,9 @@ from django.utils.html import escape, format_html from django.utils.http import urlencode, urlquote from django.utils.safestring import mark_safe from django.utils.text import capfirst, get_text_list -from django.utils.translation import string_concat, ugettext as _, ungettext +from django.utils.translation import ( + override as translation_override, string_concat, ugettext as _, ungettext, +) from django.views.decorators.csrf import csrf_protect from django.views.generic import RedirectView @@ -924,33 +926,44 @@ class ModelAdmin(BaseModelAdmin): return urlencode({'_changelist_filters': preserved_filters}) return '' + @translation_override(None) def construct_change_message(self, request, form, formsets, add=False): """ - Construct a change message from a changed object. + Construct a JSON structure describing changes from a changed object. + Translations are deactivated so that strings are stored untranslated. + Translation happens later on LogEntry access. """ change_message = [] if add: - change_message.append(_('Added.')) + change_message.append({'added': {}}) elif form.changed_data: - change_message.append(_('Changed %s.') % get_text_list(form.changed_data, _('and'))) + change_message.append({'changed': {'fields': form.changed_data}}) if formsets: for formset in formsets: for added_object in formset.new_objects: - change_message.append(_('Added %(name)s "%(object)s".') - % {'name': force_text(added_object._meta.verbose_name), - 'object': force_text(added_object)}) + change_message.append({ + 'added': { + 'name': force_text(added_object._meta.verbose_name), + 'object': force_text(added_object), + } + }) for changed_object, changed_fields in formset.changed_objects: - change_message.append(_('Changed %(list)s for %(name)s "%(object)s".') - % {'list': get_text_list(changed_fields, _('and')), - 'name': force_text(changed_object._meta.verbose_name), - 'object': force_text(changed_object)}) + change_message.append({ + 'changed': { + 'name': force_text(changed_object._meta.verbose_name), + 'object': force_text(changed_object), + 'fields': changed_fields, + } + }) for deleted_object in formset.deleted_objects: - change_message.append(_('Deleted %(name)s "%(object)s".') - % {'name': force_text(deleted_object._meta.verbose_name), - 'object': force_text(deleted_object)}) - change_message = ' '.join(change_message) - return change_message or _('No fields changed.') + change_message.append({ + 'deleted': { + 'name': force_text(deleted_object._meta.verbose_name), + 'object': force_text(deleted_object), + } + }) + return change_message def message_user(self, request, message, level=messages.INFO, extra_tags='', fail_silently=False): diff --git a/django/contrib/admin/templates/admin/object_history.html b/django/contrib/admin/templates/admin/object_history.html index cf3e7e2e0d..f512aa1d1c 100644 --- a/django/contrib/admin/templates/admin/object_history.html +++ b/django/contrib/admin/templates/admin/object_history.html @@ -29,7 +29,7 @@ {{ action.action_time|date:"DATETIME_FORMAT" }} {{ action.user.get_username }}{% if action.user.get_full_name %} ({{ action.user.get_full_name }}){% endif %} - {{ action.change_message }} + {{ action.get_change_message }} {% endfor %} diff --git a/docs/ref/contrib/admin/index.txt b/docs/ref/contrib/admin/index.txt index 35c787ec1b..f6ed8f8e13 100644 --- a/docs/ref/contrib/admin/index.txt +++ b/docs/ref/contrib/admin/index.txt @@ -2823,7 +2823,18 @@ password box. .. attribute:: LogEntry.change_message The detailed description of the modification. In the case of an edit, for - example, the message contains a list of the edited fields. + example, the message contains a list of the edited fields. The Django admin + site formats this content as a JSON structure, so that + :meth:`get_change_message` can recompose a message translated in the current + user language. Custom code might set this as a plain string though. You are + advised to use the :meth:`get_change_message` method to retrieve this value + instead of accessing it directly. + + .. versionchanged:: 1.10 + + Previously, this attribute was always a plain string. It is + now JSON-structured so that the message can be translated in the current + user language. Old messages are untouched. ``LogEntry`` methods -------------------- @@ -2832,6 +2843,14 @@ password box. A shortcut that returns the referenced object. +.. method:: LogEntry.get_change_message() + + .. versionadded:: 1.10 + + Formats and translates :attr:`change_message` into the current user + language. Messages created before Django 1.10 will always be displayed in + the language in which they were logged. + .. currentmodule:: django.contrib.admin .. _admin-reverse-urls: diff --git a/docs/releases/1.10.txt b/docs/releases/1.10.txt index 6ca1d3d4f6..cf9b67d2d0 100644 --- a/docs/releases/1.10.txt +++ b/docs/releases/1.10.txt @@ -51,6 +51,11 @@ Minor features model's changelist will now be rendered (without the add button, of course). This makes it easier to add custom tools in this case. +* The :class:`~django.contrib.admin.models.LogEntry` model now stores change + messages in a JSON structure so that the message can be dynamically translated + using the current active language. A new ``LogEntry.get_change_message()`` + method is now the preferred way of retrieving the change message. + :mod:`django.contrib.admindocs` ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -417,6 +422,10 @@ Miscellaneous * :djadmin:`loaddata` now raises a ``CommandError`` instead of showing a warning when the specified fixture file is not found. +* Instead of directly accessing the ``LogEntry.change_message`` attribute, it's + now better to call the ``LogEntry.get_change_message()`` method which will + provide the message in the current language. + .. _deprecated-features-1.10: Features deprecated in 1.10 diff --git a/tests/admin_utils/admin.py b/tests/admin_utils/admin.py index 37232e3268..a161322bc2 100644 --- a/tests/admin_utils/admin.py +++ b/tests/admin_utils/admin.py @@ -1,7 +1,18 @@ from django.contrib import admin -from .models import Article, ArticleProxy +from .models import Article, ArticleProxy, Site + + +class ArticleInline(admin.TabularInline): + model = Article + fields = ['title'] + + +class SiteAdmin(admin.ModelAdmin): + inlines = [ArticleInline] + site = admin.AdminSite(name='admin') site.register(Article) site.register(ArticleProxy) +site.register(Site, SiteAdmin) diff --git a/tests/admin_utils/models.py b/tests/admin_utils/models.py index a4fe85291c..87060cbff8 100644 --- a/tests/admin_utils/models.py +++ b/tests/admin_utils/models.py @@ -1,6 +1,7 @@ from django.db import models from django.utils import six from django.utils.encoding import python_2_unicode_compatible +from django.utils.translation import ugettext_lazy as _ @python_2_unicode_compatible @@ -17,8 +18,8 @@ class Article(models.Model): """ site = models.ForeignKey(Site, models.CASCADE, related_name="admin_articles") title = models.CharField(max_length=100) - title2 = models.CharField(max_length=100, verbose_name="another name") - created = models.DateTimeField() + hist = models.CharField(max_length=100, verbose_name=_("History")) + created = models.DateTimeField(null=True) def test_from_model(self): return "nothing" diff --git a/tests/admin_utils/test_logentry.py b/tests/admin_utils/test_logentry.py index 70f0de7595..efafefc479 100644 --- a/tests/admin_utils/test_logentry.py +++ b/tests/admin_utils/test_logentry.py @@ -1,5 +1,7 @@ +# -*- coding: utf-8 -*- from __future__ import unicode_literals +import json from datetime import datetime from django.contrib.admin.models import ADDITION, CHANGE, DELETION, LogEntry @@ -8,7 +10,7 @@ from django.contrib.auth.models import User from django.contrib.contenttypes.models import ContentType from django.test import TestCase, override_settings from django.urls import reverse -from django.utils import six +from django.utils import six, translation from django.utils.encoding import force_bytes from django.utils.html import escape @@ -28,7 +30,7 @@ class LogEntryTests(TestCase): self.a1 = Article.objects.create( site=self.site, title="Title", - created=datetime(2008, 3, 18, 11, 54, 58), + created=datetime(2008, 3, 18, 11, 54), ) content_type_pk = ContentType.objects.get_for_model(Article).pk LogEntry.objects.log_action( @@ -47,6 +49,86 @@ class LogEntryTests(TestCase): logentry.save() self.assertEqual(logentry.action_time, action_time) + def test_logentry_change_message(self): + """ + LogEntry.change_message is stored as a dumped JSON structure to be able + to get the message dynamically translated at display time. + """ + post_data = { + 'site': self.site.pk, 'title': 'Changed', 'hist': 'Some content', + 'created_0': '2008-03-18', 'created_1': '11:54', + } + change_url = reverse('admin:admin_utils_article_change', args=[quote(self.a1.pk)]) + response = self.client.post(change_url, post_data) + self.assertRedirects(response, reverse('admin:admin_utils_article_changelist')) + logentry = LogEntry.objects.filter(content_type__model__iexact='article').latest('action_time') + self.assertEqual(logentry.get_change_message(), 'Changed title and hist.') + with translation.override('fr'): + self.assertEqual(logentry.get_change_message(), 'Modification de title et hist.') + + add_url = reverse('admin:admin_utils_article_add') + post_data['title'] = 'New' + response = self.client.post(add_url, post_data) + self.assertRedirects(response, reverse('admin:admin_utils_article_changelist')) + logentry = LogEntry.objects.filter(content_type__model__iexact='article').latest('action_time') + self.assertEqual(logentry.get_change_message(), 'Added.') + with translation.override('fr'): + self.assertEqual(logentry.get_change_message(), 'Ajout.') + + def test_logentry_change_message_formsets(self): + """ + All messages for changed formsets are logged in a change message. + """ + a2 = Article.objects.create( + site=self.site, + title="Title second article", + created=datetime(2012, 3, 18, 11, 54), + ) + post_data = { + 'domain': 'example.com', # domain changed + 'admin_articles-TOTAL_FORMS': '5', + 'admin_articles-INITIAL_FORMS': '2', + 'admin_articles-MIN_NUM_FORMS': '0', + 'admin_articles-MAX_NUM_FORMS': '1000', + # Changed title for 1st article + 'admin_articles-0-id': str(self.a1.pk), + 'admin_articles-0-site': str(self.site.pk), + 'admin_articles-0-title': 'Changed Title', + # Second article is deleted + 'admin_articles-1-id': str(a2.pk), + 'admin_articles-1-site': str(self.site.pk), + 'admin_articles-1-title': 'Title second article', + 'admin_articles-1-DELETE': 'on', + # A new article is added + 'admin_articles-2-site': str(self.site.pk), + 'admin_articles-2-title': 'Added article', + } + change_url = reverse('admin:admin_utils_site_change', args=[quote(self.site.pk)]) + response = self.client.post(change_url, post_data) + self.assertRedirects(response, reverse('admin:admin_utils_site_changelist')) + self.assertQuerysetEqual(Article.objects.filter(pk=a2.pk), []) + logentry = LogEntry.objects.filter(content_type__model__iexact='site').latest('action_time') + self.assertEqual( + json.loads(logentry.change_message), + [ + {"changed": {"fields": ["domain"]}}, + {"added": {"object": "Article object", "name": "article"}}, + {"changed": {"fields": ["title"], "object": "Article object", "name": "article"}}, + {"deleted": {"object": "Article object", "name": "article"}}, + ] + ) + self.assertEqual( + logentry.get_change_message(), + 'Changed domain. Added article "Article object". ' + 'Changed title for article "Article object". Deleted article "Article object".' + ) + with translation.override('fr'): + self.assertEqual( + logentry.get_change_message(), + 'Modification de domain. Article « Article object » ajouté. ' + 'Modification de title pour l\'objet article « Article object ». Article « Article object » supprimé.' + ) + def test_logentry_get_edited_object(self): """ LogEntry.get_edited_object() returns the edited object of a LogEntry @@ -114,7 +196,7 @@ class LogEntryTests(TestCase): """ proxy_content_type = ContentType.objects.get_for_model(ArticleProxy, for_concrete_model=False) post_data = { - 'site': self.site.pk, 'title': "Foo", 'title2': "Bar", + 'site': self.site.pk, 'title': "Foo", 'hist': "Bar", 'created_0': '2015-12-25', 'created_1': '00:00', } changelist_url = reverse('admin:admin_utils_articleproxy_changelist') diff --git a/tests/admin_utils/tests.py b/tests/admin_utils/tests.py index 6e054a8537..694c529b76 100644 --- a/tests/admin_utils/tests.py +++ b/tests/admin_utils/tests.py @@ -203,12 +203,12 @@ class UtilsTests(SimpleTestCase): "title" ) self.assertEqual( - label_for_field("title2", Article), - "another name" + label_for_field("hist", Article), + "History" ) self.assertEqual( - label_for_field("title2", Article, return_attr=True), - ("another name", None) + label_for_field("hist", Article, return_attr=True), + ("History", None) ) self.assertEqual( diff --git a/tests/admin_views/tests.py b/tests/admin_views/tests.py index 7f94358fec..ec4dbc1fa1 100644 --- a/tests/admin_views/tests.py +++ b/tests/admin_views/tests.py @@ -1542,7 +1542,7 @@ class AdminViewPermissionsTest(TestCase): self.assertEqual(addition_log.object_id, str(new_article.pk)) self.assertEqual(addition_log.object_repr, "Døm ikke") self.assertEqual(addition_log.action_flag, ADDITION) - self.assertEqual(addition_log.change_message, "Added.") + self.assertEqual(addition_log.get_change_message(), "Added.") # Super can add too, but is redirected to the change list view self.client.force_login(self.superuser) diff --git a/tests/auth_tests/test_views.py b/tests/auth_tests/test_views.py index 3921cb3963..f3593d46c5 100644 --- a/tests/auth_tests/test_views.py +++ b/tests/auth_tests/test_views.py @@ -931,7 +931,7 @@ class ChangelistTests(AuthViewsTestCase): ) self.assertRedirects(response, reverse('auth_test_admin:auth_user_changelist')) row = LogEntry.objects.latest('id') - self.assertEqual(row.change_message, 'Changed email.') + self.assertEqual(row.get_change_message(), 'Changed email.') def test_user_not_change(self): response = self.client.post( @@ -940,7 +940,7 @@ class ChangelistTests(AuthViewsTestCase): ) self.assertRedirects(response, reverse('auth_test_admin:auth_user_changelist')) row = LogEntry.objects.latest('id') - self.assertEqual(row.change_message, 'No fields changed.') + self.assertEqual(row.get_change_message(), 'No fields changed.') def test_user_change_password(self): user_change_url = reverse('auth_test_admin:auth_user_change', args=(self.admin.pk,)) @@ -966,7 +966,7 @@ class ChangelistTests(AuthViewsTestCase): ) self.assertRedirects(response, user_change_url) row = LogEntry.objects.latest('id') - self.assertEqual(row.change_message, 'Changed password.') + self.assertEqual(row.get_change_message(), 'Changed password.') self.logout() self.login(password='password1') @@ -983,7 +983,7 @@ class ChangelistTests(AuthViewsTestCase): row = LogEntry.objects.latest('id') self.assertEqual(row.user_id, self.admin.pk) self.assertEqual(row.object_id, str(u.pk)) - self.assertEqual(row.change_message, 'Changed password.') + self.assertEqual(row.get_change_message(), 'Changed password.') def test_password_change_bad_url(self): response = self.client.get(reverse('auth_test_admin:auth_user_password_change', args=('foobar',))) @@ -1018,4 +1018,4 @@ class UUIDUserTests(TestCase): row = LogEntry.objects.latest('id') self.assertEqual(row.user_id, 1) # hardcoded in CustomUserAdmin.log_change() self.assertEqual(row.object_id, str(u.pk)) - self.assertEqual(row.change_message, 'Changed password.') + self.assertEqual(row.get_change_message(), 'Changed password.')