From 96891138749a0a0b4c5ccc65c04df547cf8b33ed Mon Sep 17 00:00:00 2001 From: Ayush Ranjan Date: Fri, 4 Sep 2026 22:42:14 -0400 Subject: [PATCH] Fix placeholder warnings not persisted to DB, add log in utils, add regression test --- pontoon/checks/__init__.py | 6 +++ pontoon/checks/libraries/custom.py | 13 +++-- pontoon/checks/tests/test_custom.py | 14 ++--- pontoon/checks/tests/test_libraries.py | 2 +- pontoon/checks/tests/test_models.py | 71 +++++++++++++++++++++++++- pontoon/checks/utils.py | 11 +++- translate/src/api/translation.ts | 1 + 7 files changed, 102 insertions(+), 16 deletions(-) diff --git a/pontoon/checks/__init__.py b/pontoon/checks/__init__.py index e3b47d30ca..80c08beacd 100644 --- a/pontoon/checks/__init__.py +++ b/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", +) diff --git a/pontoon/checks/libraries/custom.py b/pontoon/checks/libraries/custom.py index aad1417d45..a692ee2add 100644 --- a/pontoon/checks/libraries/custom.py +++ b/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 diff --git a/pontoon/checks/tests/test_custom.py b/pontoon/checks/tests/test_custom.py index 4879f9a6a9..fe17babc45 100644 --- a/pontoon/checks/tests/test_custom.py +++ b/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 not found in reference"], - "pndbWarnings": ["Element not found in translation"], + "pWarnings": ["Element 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"], } diff --git a/pontoon/checks/tests/test_libraries.py b/pontoon/checks/tests/test_libraries.py index ed95207b30..ac118a8789 100644 --- a/pontoon/checks/tests/test_libraries.py +++ b/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"], } diff --git a/pontoon/checks/tests/test_models.py b/pontoon/checks/tests/test_models.py index 750771dabf..c6a04b719d 100644 --- a/pontoon/checks/tests/test_models.py +++ b/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 diff --git a/pontoon/checks/utils.py b/pontoon/checks/utils.py index e12aad8d91..0477571641 100644 --- a/pontoon/checks/utils.py +++ b/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"): diff --git a/translate/src/api/translation.ts b/translate/src/api/translation.ts index 035bcd98ee..29d9bb421b 100644 --- a/translate/src/api/translation.ts +++ b/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[]; };