Stop reintroducing the wrong plural catchall (#4468)

MF2 syntax writes the catchall as a bare `*`, so parsing a message back
from it cannot recover the label and `parse_source_string_to_json` assumed
`other`. gettext addresses plural forms by category, so in be, pl, ru, szl
and uk that produced a catchall matching no form, serialized as an empty
msgstr. Migration 0131 repaired the existing rows; this stops new ones
being written.

Author: mathjazz

pontoon/base/models/locale.py

@@ -1,6 +1,6 @@
 import logging
 
-from typing import TYPE_CHECKING
+from typing import TYPE_CHECKING, Literal
 
 from django.contrib.auth.models import Group
 from django.core.exceptions import ValidationError
@@ -346,8 +346,18 @@ def cldr_plurals_list(self) -> list[str]:
                 log.error(
                     f"Invalid cldr_plurals for locale {self.code}: {self.cldr_plurals}"
                 )
+        if res and res[-1] not in ("many", "other"):
+            log.error(
+                f"cldr_plurals for locale {self.code} does not end in a catchall"
+                f" category: {self.cldr_plurals}"
+            )
         return res
 
+    @property
+    def plural_catchall(self) -> Literal["many", "other"]:
+        categories = self.cldr_plurals_list()
+        return "many" if categories and categories[-1] == "many" else "other"
+
     @property
     def nplurals(self) -> int:
         return self.cldr_plurals.count(",") + 1

pontoon/base/tests/models/test_locale.py

@@ -1,5 +1,7 @@
 import pytest
 
+from pontoon.base.models import Locale
+
 
 @pytest.mark.django_db
 def test_locale_latest_activity_with_latest(translation_a):
@@ -68,3 +70,17 @@ def test_locale_managers_group(locale_a, locale_b, user_a):
     assert user_a.has_perm("base.can_manage_locale") is False
     assert user_a.has_perm("base.can_manage_locale", locale_a) is True
     assert user_a.has_perm("base.can_manage_locale", locale_b) is True
+
+
+@pytest.mark.parametrize(
+    "cldr_plurals, expected",
+    [
+        ("1,3,4", "many"),  # be, pl, ru, szl, uk
+        ("1,5", "other"),
+        ("", "other"),  # no plurals recorded
+        ("nonsense", "other"),  # cldr_plurals_list() logs and skips
+        ("1,3", "other"),  # last category is not a catchall, so logged and coerced
+    ],
+)
+def test_locale_plural_catchall(cldr_plurals, expected):
+    assert Locale(code="kl", cldr_plurals=cldr_plurals).plural_catchall == expected

pontoon/batch/actions.py

@@ -316,7 +316,7 @@ def copy_translation_from_locale(
         # Copy from other locale (entity.string directly)
         for entity in entities:
             _, value, properties = parse_source_string_to_json(
-                entity.resource.format, entity.string
+                entity.resource.format, entity.string, locale.plural_catchall
             )
             translations_to_create.append(
                 Translation(
@@ -335,7 +335,7 @@ def copy_translation_from_locale(
     else:
         for t in other_locale_translations:
             _, value, properties = parse_source_string_to_json(
-                t.entity.resource.format, t.string
+                t.entity.resource.format, t.string, locale.plural_catchall
             )
             translations_to_create.append(
                 Translation(

pontoon/batch/utils.py

@@ -127,7 +127,11 @@ def find_and_replace(
         errors = False
         try:
             _, new_translation.value, new_translation.properties = (
-                parse_source_string_to_json(res_format, new_translation.string)
+                parse_source_string_to_json(
+                    res_format,
+                    new_translation.string,
+                    new_translation.locale.plural_catchall,
+                )
             )
         except ValueError:
             errors = True

pontoon/pretranslation/tasks.py

@@ -142,7 +142,7 @@ def pretranslate(project: Project, paths: set[str] | None):
             string, author_key = pretranslation
             try:
                 _, value, properties = parse_source_string_to_json(
-                    entity.resource.format, string
+                    entity.resource.format, string, locale.plural_catchall
                 )
             except ValueError as e:
                 log.error(

pontoon/translations/tests/test_utils.py

@@ -0,0 +1,43 @@
+from pontoon.base.models import Resource
+from pontoon.translations.utils import parse_source_string_to_json
+
+
+PLURAL_MF2 = (
+    ".input {$n :number}\n"
+    ".match $n\n"
+    "one {{jedna rzecz}}\n"
+    "few {{kilka rzeczy}}\n"
+    "* {{wiele rzeczy}}\n"
+)
+
+
+def catchall_keys(value):
+    return [
+        key
+        for variant in value["alt"]
+        for key in variant["keys"]
+        if isinstance(key, dict)
+    ]
+
+
+def test_plural_catchall_uses_locale_category():
+    """For a locale whose last plural category is `many`, the catchall must be
+    labelled `many` (#4453).
+    """
+    _, value, _ = parse_source_string_to_json(
+        Resource.Format.GETTEXT, PLURAL_MF2, "many"
+    )
+    assert catchall_keys(value) == [{"*": "many"}]
+
+
+def test_plural_catchall_defaults_to_other():
+    """Without a catchall name, the catchall keeps the source locale's category."""
+    _, value, _ = parse_source_string_to_json(Resource.Format.GETTEXT, PLURAL_MF2)
+    assert catchall_keys(value) == [{"*": "other"}]
+
+
+def test_plural_catchall_for_locale_ending_in_other():
+    _, value, _ = parse_source_string_to_json(
+        Resource.Format.GETTEXT, PLURAL_MF2, "other"
+    )
+    assert catchall_keys(value) == [{"*": "other"}]

pontoon/translations/utils.py

@@ -1,4 +1,4 @@
-from typing import Any
+from typing import Any, Literal
 
 from moz.l10n.formats.fluent import fluent_parse_entry, fluent_serialize_entry
 from moz.l10n.formats.mf2 import mf2_parse_message, mf2_serialize_message
@@ -18,11 +18,16 @@
 def parse_source_string_to_json(
     res_format: str,
     source: str,
+    catchall_name: Literal["many", "other"] = "other",
 ) -> tuple[list[str], JsonMessage, dict[str, JsonMessage] | None]:
     """Parse an entity's `source` string into its `(key, value, properties)` JSON.
 
     Used to build entities that aren't backed by a synced row, i.e. in the
     pretranslate API and in test factories.
+
+    `catchall_name` is the category a catchall plural variant is labelled with,
+    i.e. `Locale.plural_catchall`. Pass it when parsing a translation, leave it
+    unset for source strings.
     """
     match res_format:
         case Resource.Format.FLUENT:
@@ -45,7 +50,7 @@ def parse_source_string_to_json(
                 for keys in msg.variants:
                     for key in keys:
                         if isinstance(key, CatchallKey):
-                            key.value = "other"
+                            key.value = catchall_name
             return [], message_to_json(msg), None
         case Resource.Format.PROPERTIES:
             msg = properties_parse_message(source)