Checks: Persist placeholder warnings to DB

pontoon/checks/__init__.py

@@ -12,3 +12,9 @@
     "p",
     "cl",
 )
+
+# Warnings from these libraries reported back to the submitter and not saved in the DB
+NON_DB_LIBRARIES = (
+    "pndb",
+    "tt",
+)

pontoon/checks/libraries/custom.py

@@ -49,7 +49,7 @@ def visit_TextElement(self, node):
 
 def run_custom_checks(entity: Entity, string: str) -> dict[str, list[str]]:
     """
-    Group all checks related to the base UI that get stored in the DB
+    Group all checks related to the base UI
     """
     if not string:
         if entity.resource.allows_empty_translations:
@@ -61,6 +61,7 @@ def run_custom_checks(entity: Entity, string: str) -> dict[str, list[str]]:
     format = cast(Resource.Format, entity.resource.format)
     errors: list[str] = []
     warnings: list[str] = []
+    ndb_warnings: list[str] = []
     match format:
         case Resource.Format.ANDROID | Resource.Format.XCODE:
             try:
@@ -124,13 +125,13 @@ def run_custom_checks(entity: Entity, string: str) -> dict[str, list[str]]:
             elif entity_ast.id.name != translation_ast.id.name:
                 errors.append("Translation key needs to match source string key")
 
-            # Empty translation entry warning; set here rather than pontoon_non_db.py
-            # to avoid needing to parse the Fluent message twice.
+            # Empty translation entry warning; set here rather than with the other
+            # non-DB warnings to avoid needing to parse the Fluent message twice.
             else:
                 visitor = IsEmptyVisitor()
                 visitor.visit(translation_ast)
                 if visitor.is_empty:
-                    warnings.append("Empty translation")
+                    ndb_warnings.append("Empty translation")
 
         case Resource.Format.WEBEXT:
             try:
@@ -171,7 +172,9 @@ def run_custom_checks(entity: Entity, string: str) -> dict[str, list[str]]:
     if errors:
         checks["pErrors"] = errors
     if warnings:
-        checks["pndbWarnings"] = warnings
+        checks["pWarnings"] = warnings
+    if ndb_warnings:
+        checks["pndbWarnings"] = ndb_warnings
     return checks
 
 

pontoon/checks/tests/test_custom.py

@@ -237,7 +237,7 @@ def test_android_missing_placeholder():
     translation = "Translation"
     entity = mock_entity("android", string=original)
     assert run_custom_checks(entity, translation) == {
-        "pndbWarnings": ["Placeholder %1$s not found in translation"]
+        "pWarnings": ["Placeholder %1$s not found in translation"]
     }
 
 
@@ -247,7 +247,7 @@ def test_android_mistyped_placeholder():
     entity = mock_entity("android", string=original)
     assert run_custom_checks(entity, translation) == {
         "pErrors": ["Placeholder %1 not found in reference"],
-        "pndbWarnings": ["Placeholder %1$s not found in translation"],
+        "pWarnings": ["Placeholder %1$s not found in translation"],
     }
 
 
@@ -277,7 +277,7 @@ def test_android_changed_placeholder():
     entity = mock_entity("android", string=original)
     assert run_custom_checks(entity, translation) == {
         "pErrors": ["Placeholder %@ not found in reference"],
-        "pndbWarnings": ["Placeholder %s not found in translation"],
+        "pWarnings": ["Placeholder %s not found in translation"],
     }
 
 
@@ -286,7 +286,7 @@ def test_android_protections():
     translation = "Translation String with %1$s"
     entity = mock_entity("android", string=original)
     assert run_custom_checks(entity, translation) == {
-        "pndbWarnings": ["Placeholder String not found in translation"]
+        "pWarnings": ["Placeholder String not found in translation"]
     }
 
 
@@ -312,7 +312,7 @@ def test_android_bad_html():
     entity = mock_entity("android", string=original)
     assert run_custom_checks(entity, translation) == {
         "pErrors": ["Element <a> not found in reference"],
-        "pndbWarnings": ["Element <b> not found in translation"],
+        "pWarnings": ["Element <b> not found in translation"],
     }
 
 
@@ -401,7 +401,7 @@ def test_xcode_missing_placeholder():
     translation = "Translation"
     entity = mock_entity("xcode", string=original)
     assert run_custom_checks(entity, translation) == {
-        "pndbWarnings": ["Placeholder %@ not found in translation"]
+        "pWarnings": ["Placeholder %@ not found in translation"]
     }
 
 
@@ -411,7 +411,7 @@ def test_xcode_mistyped_placeholder():
     entity = mock_entity("xcode", string=original)
     assert run_custom_checks(entity, translation) == {
         "pErrors": ["Placeholder % @ not found in reference"],
-        "pndbWarnings": ["Placeholder %@ not found in translation"],
+        "pWarnings": ["Placeholder %@ not found in translation"],
     }
 
 

pontoon/checks/tests/test_libraries.py

@@ -177,6 +177,6 @@ def test_tt_xcode_checks():
         string="You can learn more",
         use_tt_checks=True,
     ) == {
-        "pndbWarnings": ["Placeholder %@ not found in translation"],
+        "pWarnings": ["Placeholder %@ not found in translation"],
         "ttWarnings": ["Ending punctuation", "Printf format string mismatch"],
     }

pontoon/checks/tests/test_models.py

@@ -51,6 +51,30 @@ def translation_compare_locales_error(translation_properties):
     yield translation
 
 
+@pytest.fixture
+def translation_pontoon_warning(translation_a):
+    """
+    An Android translation that drops a placeholder used by the source string
+    """
+    resource = translation_a.entity.resource
+    resource.path = "strings.xml"
+    resource.format = Resource.Format.ANDROID
+    resource.save()
+
+    entity = translation_a.entity
+    entity.string = "Source string with a {$arg1 :string @source=|%1$s|}"
+    entity.save()
+
+    # Create new instance
+    translation = Translation.objects.get(pk=translation_a.pk)
+    translation.pk = None
+
+    translation.string = "Translation without the placeholder"
+    translation.save()
+
+    yield translation
+
+
 @pytest.fixture
 def translation_pontoon_error(translation_a):
     # Create new instance
@@ -74,11 +98,14 @@ def test_save_failed_checks(translation_a):
         {
             "clErrors": ["compare-locales error 1", "compare-locales error 2"],
             "clWarnings": ["compare-locales warning 1"],
+            "pWarnings": ["pontoon warning 1"],
             # Warnings from Translate Toolkit can't be stored in the Database
             "ttWarnings": [
                 "translate-toolkit warning 1",
                 "translate-toolkit warning 2",
             ],
+            # Neither can Pontoon's non-DB warnings
+            "pndbWarnings": ["Empty translation"],
         },
     )
 
@@ -89,11 +116,14 @@ def test_save_failed_checks(translation_a):
     assert error2.library == FailedCheck.Library.COMPARE_LOCALES
     assert error2.message == "compare-locales error 2"
 
-    (cl_warning,) = Warning.objects.order_by("library", "message")
+    cl_warning, p_warning = Warning.objects.order_by("library", "message")
 
     assert cl_warning.library == FailedCheck.Library.COMPARE_LOCALES
     assert cl_warning.message == "compare-locales warning 1"
 
+    assert p_warning.library == FailedCheck.Library.PONTOON
+    assert p_warning.message == "pontoon warning 1"
+
 
 @pytest.mark.django_db
 def test_save_no_checks(translation_a):
@@ -146,6 +176,37 @@ def test_bulk_run_checks(
     assert p_error.translation == translation_pontoon_error
 
 
+@pytest.mark.django_db
+def test_bulk_run_checks_pontoon_warning(translation_pontoon_warning):
+    """
+    Warnings raised by Pontoon's own checks must be stored
+    """
+    warnings, errors = bulk_run_checks([translation_pontoon_warning])
+
+    assert errors == []
+
+    (p_warning,) = warnings
+    assert p_warning.pk is not None
+    assert p_warning.library == FailedCheck.Library.PONTOON
+    assert p_warning.message == "Placeholder %1$s not found in translation"
+    assert p_warning.translation == translation_pontoon_warning
+
+    assert translation_pontoon_warning.warnings.count() == 1
+
+
+@pytest.mark.django_db
+def test_unknown_library_is_logged(translation_a, caplog):
+    """
+    Failed checks with an unrecognised library prefix are not dropped silently
+    """
+    warnings, errors = get_failed_checks_db_objects(
+        translation_a, {"nopeWarnings": ["dropped warning"]}
+    )
+
+    assert (warnings, errors) == ([], [])
+    assert "unknown library 'nope'" in caplog.text
+
+
 @pytest.mark.django_db
 def test_get_failed_checks_db_objects(translation_a):
     """
@@ -155,8 +216,10 @@ def test_get_failed_checks_db_objects(translation_a):
         translation_a,
         {
             "clWarnings": ["compare-locales warning 1"],
+            "pWarnings": ["pontoon warning 1"],
             # Warnings from some libraries e.g. Translate Toolkit, shouldn't land in the database.
             "ttWarnings": ["translate-toolkit warning 1"],
+            "pndbWarnings": ["Empty translation"],
             "clErrors": ["compare-locales error 1"],
             "pErrors": ["pontoon error 1"],
         },
@@ -166,13 +229,17 @@ def test_get_failed_checks_db_objects(translation_a):
     assert all([w.pk is None for w in warnings])
     assert all([e.pk is None for e in errors])
 
-    (cl_warning,) = warnings
+    cl_warning, p_warning = sorted(warnings, key=lambda w: w.library)
     cl_error, p_error = sorted(errors, key=lambda e: e.library)
 
     assert cl_warning.library == FailedCheck.Library.COMPARE_LOCALES
     assert cl_warning.message == "compare-locales warning 1"
     assert cl_warning.translation == translation_a
 
+    assert p_warning.library == FailedCheck.Library.PONTOON
+    assert p_warning.message == "pontoon warning 1"
+    assert p_warning.translation == translation_a
+
     assert cl_error.library == FailedCheck.Library.COMPARE_LOCALES
     assert cl_error.message == "compare-locales error 1"
     assert cl_error.translation == translation_a

pontoon/checks/utils.py

@@ -1,4 +1,9 @@
-from pontoon.checks import DB_LIBRARIES
+import logging
+
+from pontoon.checks import DB_LIBRARIES, NON_DB_LIBRARIES
+
+
+log = logging.getLogger(__name__)
 
 
 def bulk_run_checks(translations):
@@ -53,6 +58,10 @@ def get_failed_checks_db_objects(translation, failed_checks):
     for check_group, messages in failed_checks.items():
         library = check_group.replace("Warnings", "").replace("Errors", "")
         if library not in DB_LIBRARIES:
+            if library not in NON_DB_LIBRARIES:
+                log.error(
+                    f"Discarding failed checks from unknown library {library!r}: {messages}"
+                )
             continue
 
         if check_group.endswith("Errors"):

translate/src/api/translation.ts

@@ -11,6 +11,7 @@ export type ApiFailedChecks = {
   readonly clErrors?: string[];
   readonly pErrors?: string[];
   readonly clWarnings?: string[];
+  readonly pWarnings?: string[];
   readonly pndbWarnings?: string[];
   readonly ttWarnings?: string[];
 };