[Django] #32797: model_ngettext incorrectly tries to translate already translated words
Django
-------------------------------------+-------------------------------------
Reporter: Maciej | Owner: Maciej Olko
Olko |
Type: Bug | Status: assigned
Component: | Version: 3.2
contrib.admin | Keywords: i18n, gettext,
Severity: Normal | ngettext
Triage Stage: | Has patch: 1
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 1 |
-------------------------------------+-------------------------------------
`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.
--
Ticket URL: <https://code.djangoproject.com/ticket/32797>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
| Olko
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: i18n, gettext, | Triage Stage:
ngettext | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 1
-------------------------------------+-------------------------------------
* cc: Claude Paroz (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/32797#comment:1>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: contrib.admin | Version: 3.2
Severity: Normal | Resolution:
Keywords: i18n, gettext, | Triage Stage:
ngettext | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 1
-------------------------------------+-------------------------------------
* owner: Maciej Olko => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/32797#comment:2>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: contrib.admin | Version: 3.2
Severity: Normal | Resolution:
Keywords: i18n, gettext, | Triage Stage:
ngettext | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 1
-------------------------------------+-------------------------------------
* Attachment "Screenshot 2021-06-08 at 10.23.33.png" added.
verbose_name_plural being translated in message.
Django
-------------------------------------+-------------------------------------
Component: contrib.admin | Version: 3.2
Severity: Normal | Resolution: needsinfo
Keywords: i18n, gettext, | Triage Stage:
ngettext | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 1
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => needsinfo
Comment:
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.
[[Image(https://code.djangoproject.com/raw-
attachment/ticket/32797/Screenshot%202021-06-08%20at%2010.23.33.png)]]
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!
--
Ticket URL: <https://code.djangoproject.com/ticket/32797#comment:3>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: closed
Component: contrib.admin | Version: 3.2
Severity: Normal | Resolution: needsinfo
Keywords: i18n, gettext, | Triage Stage:
ngettext | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 1
-------------------------------------+-------------------------------------
Comment (by 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:"
I've added a draft PR with those changes:
https://github.com/django/django/pull/14690.
Kind regards.
--
Ticket URL: <https://code.djangoproject.com/ticket/32797#comment:4>
Django
-------------------------------------+-------------------------------------
Component: contrib.admin | Version: 3.2
Severity: Normal | Resolution:
ngettext | Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 1
-------------------------------------+-------------------------------------
* status: closed => new
* resolution: needsinfo =>
Comment:
Hi Maciej — thanks for the extra info. I will reopen to review in the
week.
--
Ticket URL: <https://code.djangoproject.com/ticket/32797#comment:5>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: contrib.admin | Version: 3.2
Severity: Normal | Resolution:
ngettext |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 1
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
* needs_tests: 0 => 1
* stage: Unreviewed => Accepted
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/32797#comment:6>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: contrib.admin | Version: 3.2
Severity: Normal | Resolution:
Keywords: i18n, gettext, | Triage Stage: Accepted
ngettext |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
-------------------------------------+-------------------------------------
Comment (by 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
--
Ticket URL: <https://code.djangoproject.com/ticket/32797#comment:7>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: contrib.admin | Version: 3.2
Severity: Normal | Resolution:
Keywords: i18n, gettext, | Triage Stage: Accepted
ngettext |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
-------------------------------------+-------------------------------------
Comment (by Carlton Gibson):
Hey Maciej — thanks for the follow-up.
Can I suggest you post to the
[https://forum.djangoproject.com/c/internals/i18n/14 i18n category on the
forum] to solicit input/feedback?
👍
--
Ticket URL: <https://code.djangoproject.com/ticket/32797#comment:8>
Django
-------------------------------------+-------------------------------------
Type: Bug | Status: new
Component: contrib.admin | Version: 3.2
Severity: Normal | Resolution:
Keywords: i18n, gettext, | Triage Stage: Accepted
ngettext |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
-------------------------------------+-------------------------------------
Comment (by 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.
Thank you,
Regards.
--
Ticket URL: <https://code.djangoproject.com/ticket/32797#comment:9>
Django
-------------------------------------+-------------------------------------
Component: contrib.admin | Version: 3.2
Severity: Normal | Resolution: duplicate
Keywords: i18n, gettext, | Triage Stage: Accepted
ngettext |
Has patch: 1 | Needs documentation: 0
Needs tests: 1 | Patch needs improvement: 1
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => duplicate
Comment:
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!
--
Ticket URL: <https://code.djangoproject.com/ticket/32797#comment:10>