From 87f5d07eededc86f8ce1797fdfca7d4903ee0edc Mon Sep 17 00:00:00 2001 From: Sanyam Khurana <8039608+CuriousLearner@users.noreply.github.com> Date: Fri, 14 Jun 2019 21:50:29 +0530 Subject: [PATCH] Fixed #12952 -- Adjusted admin log change messages to use form labels instead of field names. --- django/contrib/admin/models.py | 2 +- django/contrib/admin/utils.py | 26 +++++++++++++-- docs/releases/3.0.txt | 6 ++++ tests/admin_utils/admin.py | 19 +++++++++++ tests/admin_utils/test_logentry.py | 20 ++++++------ tests/admin_views/admin.py | 19 +++++++++++ tests/admin_views/models.py | 4 +-- tests/admin_views/test_history_view.py | 45 ++++++++++++++++++++++++++ tests/admin_views/tests.py | 2 +- tests/auth_tests/test_views.py | 2 +- 10 files changed, 128 insertions(+), 17 deletions(-) create mode 100644 tests/admin_views/test_history_view.py diff --git a/django/contrib/admin/models.py b/django/contrib/admin/models.py index f0138435ca..eed5f51c57 100644 --- a/django/contrib/admin/models.py +++ b/django/contrib/admin/models.py @@ -114,7 +114,7 @@ class LogEntry(models.Model): elif 'changed' in sub_message: sub_message['changed']['fields'] = get_text_list( - sub_message['changed']['fields'], gettext('and') + [gettext(field_name) for field_name in sub_message['changed']['fields']], gettext('and') ) if 'name' in sub_message['changed']: sub_message['changed']['name'] = gettext(sub_message['changed']['name']) diff --git a/django/contrib/admin/utils.py b/django/contrib/admin/utils.py index dd6b108ff3..15c8ef865f 100644 --- a/django/contrib/admin/utils.py +++ b/django/contrib/admin/utils.py @@ -489,12 +489,21 @@ def construct_change_message(form, formsets, add): Translations are deactivated so that strings are stored untranslated. Translation happens later on LogEntry access. """ + # Evaluating `form.changed_data` prior to disabling translations is required + # to avoid fields affected by localization from being included incorrectly, + # e.g. where date formats differ such as MM/DD/YYYY vs DD/MM/YYYY. + changed_data = form.changed_data + with translation_override(None): + # Deactivate translations while fetching verbose_name for form + # field labels and using `field_name`, if verbose_name is not provided. + # Translations will happen later on LogEntry access. + changed_field_labels = _get_changed_field_labels_from_form(form, changed_data) + change_message = [] if add: change_message.append({'added': {}}) elif form.changed_data: - change_message.append({'changed': {'fields': form.changed_data}}) - + change_message.append({'changed': {'fields': changed_field_labels}}) if formsets: with translation_override(None): for formset in formsets: @@ -510,7 +519,7 @@ def construct_change_message(form, formsets, add): 'changed': { 'name': str(changed_object._meta.verbose_name), 'object': str(changed_object), - 'fields': changed_fields, + 'fields': _get_changed_field_labels_from_form(formset.forms[0], changed_fields), } }) for deleted_object in formset.deleted_objects: @@ -521,3 +530,14 @@ def construct_change_message(form, formsets, add): } }) return change_message + + +def _get_changed_field_labels_from_form(form, changed_data): + changed_field_labels = [] + for field_name in changed_data: + try: + verbose_field_name = form.fields[field_name].label or field_name + except KeyError: + verbose_field_name = field_name + changed_field_labels.append(str(verbose_field_name)) + return changed_field_labels diff --git a/docs/releases/3.0.txt b/docs/releases/3.0.txt index 80c1d8904b..ba7e9f18da 100644 --- a/docs/releases/3.0.txt +++ b/docs/releases/3.0.txt @@ -322,6 +322,12 @@ backends. :class:`~django.db.models.DateTimeField` in ``datetime_cast_date_sql()``, ``datetime_extract_sql()``, etc. +:mod:`django.contrib.admin` +--------------------------- + +* Admin's model history change messages now prefers more readable field labels + instead of field names. + :mod:`django.contrib.gis` ------------------------- diff --git a/tests/admin_utils/admin.py b/tests/admin_utils/admin.py index a161322bc2..33c791769b 100644 --- a/tests/admin_utils/admin.py +++ b/tests/admin_utils/admin.py @@ -1,11 +1,30 @@ +from django import forms from django.contrib import admin from .models import Article, ArticleProxy, Site +class ArticleAdminForm(forms.ModelForm): + nolabel_form_field = forms.BooleanField(required=False) + + class Meta: + model = Article + fields = ['title'] + + @property + def changed_data(self): + data = super().changed_data + if data: + # Add arbitrary name to changed_data to test + # change message construction. + return data + ['not_a_form_field'] + return data + + class ArticleInline(admin.TabularInline): model = Article fields = ['title'] + form = ArticleAdminForm class SiteAdmin(admin.ModelAdmin): diff --git a/tests/admin_utils/test_logentry.py b/tests/admin_utils/test_logentry.py index b56b209433..c2dec073f4 100644 --- a/tests/admin_utils/test_logentry.py +++ b/tests/admin_utils/test_logentry.py @@ -53,9 +53,9 @@ class LogEntryTests(TestCase): 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('id') - self.assertEqual(logentry.get_change_message(), 'Changed title and hist.') + self.assertEqual(logentry.get_change_message(), 'Changed Title and History.') with translation.override('fr'): - self.assertEqual(logentry.get_change_message(), 'Modification de title et hist.') + self.assertEqual(logentry.get_change_message(), 'Modification de Title et Historique.') add_url = reverse('admin:admin_utils_article_add') post_data['title'] = 'New' @@ -85,7 +85,7 @@ class LogEntryTests(TestCase): 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('id') - self.assertEqual(logentry.get_change_message(), 'Changed title and hist.') + self.assertEqual(logentry.get_change_message(), 'Changed Title and History.') def test_logentry_change_message_formsets(self): """ @@ -123,23 +123,25 @@ class LogEntryTests(TestCase): self.assertEqual( json.loads(logentry.change_message), [ - {"changed": {"fields": ["domain"]}}, + {"changed": {"fields": ["Domain"]}}, {"added": {"object": "Added article", "name": "article"}}, - {"changed": {"fields": ["title"], "object": "Changed Title", "name": "article"}}, + {"changed": {"fields": ["Title", "not_a_form_field"], "object": "Changed Title", "name": "article"}}, {"deleted": {"object": "Title second article", "name": "article"}}, ] ) self.assertEqual( logentry.get_change_message(), - 'Changed domain. Added article "Added article". ' - 'Changed title for article "Changed Title". Deleted article "Title second article".' + 'Changed Domain. Added article "Added article". ' + 'Changed Title and not_a_form_field for article "Changed Title". ' + 'Deleted article "Title second article".' ) with translation.override('fr'): self.assertEqual( logentry.get_change_message(), - "Modification de domain. Ajout de article « Added article ». " - "Modification de title pour l'objet article « Changed Title ». " + "Modification de Domain. Ajout de article « Added article ». " + "Modification de Title et not_a_form_field pour l'objet " + "article « Changed Title ». " "Suppression de article « Title second article »." ) diff --git a/tests/admin_views/admin.py b/tests/admin_views/admin.py index 8f0aaf843f..44333e893f 100644 --- a/tests/admin_views/admin.py +++ b/tests/admin_views/admin.py @@ -893,8 +893,27 @@ class CityInlineAdmin(admin.TabularInline): view_on_site = False +class StateAdminForm(forms.ModelForm): + nolabel_form_field = forms.BooleanField(required=False) + + class Meta: + model = State + fields = '__all__' + labels = {"name": "State name (from form's Meta.labels)"} + + @property + def changed_data(self): + data = super().changed_data + if data: + # Add arbitrary name to changed_data to test + # change message construction. + return data + ['not_a_form_field'] + return data + + class StateAdmin(admin.ModelAdmin): inlines = [CityInlineAdmin] + form = StateAdminForm class RestaurantInlineAdmin(admin.TabularInline): diff --git a/tests/admin_views/models.py b/tests/admin_views/models.py index 9ac3c53bab..a519f7395d 100644 --- a/tests/admin_views/models.py +++ b/tests/admin_views/models.py @@ -864,12 +864,12 @@ class EmptyModelMixin(models.Model): class State(models.Model): - name = models.CharField(max_length=100) + name = models.CharField(max_length=100, verbose_name='State verbose_name') class City(models.Model): state = models.ForeignKey(State, models.CASCADE) - name = models.CharField(max_length=100) + name = models.CharField(max_length=100, verbose_name='City verbose_name') def get_absolute_url(self): return '/dummy/%s/' % self.pk diff --git a/tests/admin_views/test_history_view.py b/tests/admin_views/test_history_view.py new file mode 100644 index 0000000000..fdcb9e5267 --- /dev/null +++ b/tests/admin_views/test_history_view.py @@ -0,0 +1,45 @@ +from django.contrib.admin.models import LogEntry +from django.contrib.auth.models import User +from django.test import TestCase, override_settings +from django.urls import reverse + +from .models import City, State + + +@override_settings(ROOT_URLCONF='admin_views.urls') +class AdminHistoryViewTests(TestCase): + + @classmethod + def setUpTestData(cls): + cls.superuser = User.objects.create_superuser( + username='super', password='secret', email='super@example.com', + ) + + def setUp(self): + self.client.force_login(self.superuser) + + def test_changed_message_uses_form_lables(self): + """ + Admin's model history change messages use form labels instead of + field names. + """ + state = State.objects.create(name='My State Name') + city = City.objects.create(name='My City Name', state=state) + change_dict = { + 'name': 'My State Name 2', + 'nolabel_form_field': True, + 'city_set-0-name': 'My City name 2', + 'city_set-0-id': city.pk, + 'city_set-TOTAL_FORMS': '3', + 'city_set-INITIAL_FORMS': '1', + 'city_set-MAX_NUM_FORMS': '0', + } + state_change_url = reverse('admin:admin_views_state_change', args=(state.pk,)) + self.client.post(state_change_url, change_dict) + logentry = LogEntry.objects.filter(content_type__model__iexact='state').latest('id') + self.assertEqual( + logentry.get_change_message(), + 'Changed State name (from form\'s Meta.labels), ' + 'nolabel_form_field and not_a_form_field. ' + 'Changed City verbose_name for city "%s".' % city + ) diff --git a/tests/admin_views/tests.py b/tests/admin_views/tests.py index 87835eb37b..203551fa39 100644 --- a/tests/admin_views/tests.py +++ b/tests/admin_views/tests.py @@ -900,7 +900,7 @@ class AdminViewBasicTest(AdminViewBasicTestCase): self.assertRedirects(response, reverse('admin:admin_views_readablepizza_changelist')) pizza_ctype = ContentType.objects.get_for_model(ReadablePizza, for_concrete_model=False) log = LogEntry.objects.filter(content_type=pizza_ctype, object_id=pizza.pk).first() - self.assertEqual(log.get_change_message(), 'Changed toppings.') + self.assertEqual(log.get_change_message(), 'Changed Toppings.') def test_allows_attributeerror_to_bubble_up(self): """ diff --git a/tests/auth_tests/test_views.py b/tests/auth_tests/test_views.py index 04eaa8574f..42acafd26d 100644 --- a/tests/auth_tests/test_views.py +++ b/tests/auth_tests/test_views.py @@ -1173,7 +1173,7 @@ class ChangelistTests(AuthViewsTestCase): ) self.assertRedirects(response, reverse('auth_test_admin:auth_user_changelist')) row = LogEntry.objects.latest('id') - self.assertEqual(row.get_change_message(), 'Changed email.') + self.assertEqual(row.get_change_message(), 'Changed Email address.') def test_user_not_change(self): response = self.client.post(