model_ngettext incorrectly tries to translate already translated words

[Maciej Olko]

model_ngettext() util function doesn't handle gettext_lazy objects as model's verbose_name and verbose_name_plural.

translation's module ngettext() function is intended to be called with not translated source strings, whereas model_ngettext() puts verbose_name and verbose_name_plural, which may be a gettext_lazy objects, as its arguments.

Effectively it makes Django not use correct plural translations for verbose name for any language that has other plural rules then English for phrases

• Successfully deleted %(count)d %(items)s.
• and %(count)s %(name)s were changed successfully.

in admin panel (they use model_ngettext function to render items and name respectively).

Following test:

Index: tests/admin_utils/tests.py
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/tests/admin_utils/tests.py b/tests/admin_utils/tests.py
--- a/tests/admin_utils/tests.py	(revision d270dd584e0af12fe6229fb712d0704c232dc7e5)
+++ b/tests/admin_utils/tests.py	(date 1622333679821)
@@ -1,5 +1,6 @@
 from datetime import datetime
 from decimal import Decimal
+from unittest.mock import patch
 
 from django import forms
 from django.conf import settings
@@ -8,7 +9,7 @@
 from django.contrib.admin.utils import (
     NestedObjects, display_for_field, display_for_value, flatten,
     flatten_fieldsets, help_text_for_field, label_for_field, lookup_field,
-    quote,
+    quote, model_ngettext,
 )
 from django.db import DEFAULT_DB_ALIAS, models
 from django.test import SimpleTestCase, TestCase, override_settings
@@ -16,7 +17,7 @@
 from django.utils.safestring import mark_safe
 
 from .models import (
-    Article, Car, Count, Event, EventGuide, Location, Site, Vehicle,
+    Article, Car, Count, Event, EventGuide, Location, Site, Vehicle, Foo,
 )
 
 
@@ -410,3 +411,9 @@
 
     def test_quote(self):
         self.assertEqual(quote('something\nor\nother'), 'something_0Aor_0Aother')
+
+    @patch('django.contrib.admin.utils.ngettext')
+    def test_model_ngettext(self, ngettext):
+        model_ngettext(Foo(), None)
+        self.assertIsInstance(ngettext.call_args.args[0], str, type(ngettext.call_args.args[0]))
+        self.assertIsInstance(ngettext.call_args.args[1], str, type(ngettext.call_args.args[1]))
Index: tests/admin_utils/models.py
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/tests/admin_utils/models.py b/tests/admin_utils/models.py
--- a/tests/admin_utils/models.py	(revision d270dd584e0af12fe6229fb712d0704c232dc7e5)
+++ b/tests/admin_utils/models.py	(date 1622333679823)
@@ -85,3 +85,9 @@
 
 class Car(VehicleMixin):
     pass
+
+
+class Foo(models.Model):
+    class Meta:
+        verbose_name = _('foo')
+        verbose_name_plural = _('foos')

Fails with:

Traceback (most recent call last):
  File "my-venv/versions/3.9.1/lib/python3.9/unittest/mock.py", line 1337, in patched
    return func(*newargs, **newkeywargs)
  File "django/tests/admin_utils/tests.py", line 419, in test_model_ngettext
    self.assertIsInstance(ngettext.call_args.args[0], str, type(ngettext.call_args.args[0]))
AssertionError: 'foo' is not an instance of <class 'str'> : <class 'django.utils.functional.lazy.<locals>.__proxy__'>

A fix requires us to recognize gettext_lazy objects as verbose names, and passing their source strings instead of translations to ngettext function.

Backwards compatibility
As third party libraries does not provide us with plural translations of models, we should keep the current behavior in case of missing plural translation of model name.

Let me create a pull request after having ticket number assigned.

[Carlton Gibson]

verbose_name_plural being translated in message.

[Carlton Gibson]

Hi Maciej.

No doubt my lack of knowledge of the internals of the i18n, but I'm struggling to pin this.

Using French as the active language for an example:

1. Create multiple users (who have a lazy verbose_name/verbose_name_plural defined).
2. Use the delete selected action from the changelist (triggering the model_ngettext() flow)
3. Observe correct translation.

It's at that level (which is the public API) that it would be good if you can present a failing test.

Effectively it makes Django not use correct plural translations for verbose name for any language that has other plural rules then English for phrases

So I'm guessing French is not the right example language?

Is this a duplicate of #11688?

From the PR, adding the ngettext_noop('log entry', 'log entries') function call into Meta declarations doesn't look like the right approach. If model_ngettext does need extra logic to detect these cases, we should keep that logic internal to the function. (I'll close the current PR on that basis.)

I'm going to mark as needs info, but happy to discuss further if you can help pin it down, perhaps with that test case?

Thanks!

[Maciej Olko]

Hi Carlton,

Thank you for looking into this. What I described is more of a "dead", or superfluous, code – line 260 in admin.utils (last line of model_ngettext() function). Now there is a call of ngettext() function, which could be as well replaced with:

if n == 1:
    return singular
return plural

The ngettext() call from the last line (ngettext(singular, plural, n or 0)) is for Polish called with already translated values, e.g. ngettext('użytkownik', 'użytkownicy', n or 0). Gettext not having that keys in translations catalog will fallback to standard pluralization, which is equivalent of the snippet I wrote above. Moreover even if we'd fix the call, to retrieve original messages, that have been translated, we don't mark that strings to translation anywhere, so anyway these keys will be missing in the translations catalog, and will fallback to English, original, strings.

So the pull request to simplify that, would be just replacing the ngettext call with the snippet.

But taking this further (should I open a new ticket for that?), by pluralizing the phrases externally (not passing the chosen plural or singular form as a parameter, but letting choose the right variable in the message), we are improving the translations. For example for the message "Deleting the selected {{ objects_name }} would result in deleting related objects, but your account doesn't have permission to delete the following types of objects:" now we have only one variant, whereas in Polish we use different wording for one and for many:

• 1: "Usunięcie wybranego(-ej) %(objects_name)s wymaga skasowania następujących chronionych obiektów, które są z nim(-ą) powiązane:"
• many: "Usunięcie wybranych %(objects_name)s wymaga skasowania następujących chronionych obiektów, które są z nimi powiązane:"

Pluralizing all strings that use model_ngettext() has good consequences in my opinion, and makes model_ngettext() function not used. Which is OK in my opinion as it's not giving much value.

I've added a draft PR with the changes I suggest in the second part of my writing: ​https://github.com/django/django/pull/14690.

Kind regards.

[Carlton Gibson]

Hi Maciej — thanks for the extra info. I will reopen to review in the week.

[Carlton Gibson]

Hi Maciej.

I'm going to provisionally accept. Can you please add a test case showing the failure to the PR?

I'm not 100% convinced that duplicating the strings and removing model_ngettext() is the way to go, rather than adding a check inside model_ngettext() — but I'll look again more deeply once you add the tests.

Thanks.

[Maciej Olko]

Hi Carlton,

I've added a test – sorry for my low response rate.

Technically we can fix those missing plural forms without touching model_ngettext() function. We can also "just" simplify it as I mentioned previously.

Nevertheless I am proposing removal of model_ngettext() function.

• The pluralization should happen on message level, not on a parameter level (as for object_name), please refer to [1] [2].
• Moreover the function implementation is broken. Internal ngettext() call is using already translated messages and even if we fix that, there will be no translations for the calls in messages catalog (we are missing so-called gettext noops for plurals of verbose names). Please let me know if I should elaborate more on that.

Thanks.

PS. I plan to publish proposals, I think in a form of DEPs, about admin/Django internationalization – first one about bringing verbose names' grammatical gender for inflection in parametrized messages, and second one about adding an ability to select a verbose name grammatical case inside translated messages. The selection of variable in a message, like proposed in my PR potentially is required by the proposed implementation of the second of those DEPs.

[1] ​https://github.com/projectfluent/fluent/wiki/Good-Practices-for-Developers#prefer-wet-over-dry
[2] https://code.djangoproject.com/ticket/11688#comment:21

[Carlton Gibson]

Hey Maciej — thanks for the follow-up.

Can I suggest you post to the ​i18n category on the forum to solicit input/feedback?
👍

[Maciej Olko]

Hi Carlton,

wouldn't you mind me changing the ticket title to "Missing pluralization in 'delete selected' confirmation messages" and description accordingly and providing unit tests and minimal required changes to fix that?

I'd rather not confuse people on forum by starting with the detail of model_ngettext function. Instead I am thinking of starting a new thread with a request for a structured input about requirements for various languages for internationalizing messages in Django admin (I am preparing the repository for this structured data, which hopefully would become a fine ground for future work).

Otherwise I also would be fine with closing this ticket for now. (As a duplicate of #11688?)

Thank you,
Regards.

[Carlton Gibson]

Hi Maciej — Sorry for the slow follow-up here. I missed your last comment, and just found it swinging back to the PR (finally) now.

Given the scope of the proposal, I think a write up and example repo would be an excellent way forward.

Let's close as a duplicate of #11688.

I think a post to the i18n thread on the forum when you're ready is the way to go, but yes, some context to guide people into the problem likely wouldn't hurt. 🙂

Thanks for the input — this is an oldie!