Don't reject strings with partial files in Upload translations (#4491)

Co-authored-by: Matjaž Horvat <matjaz.horvat@gmail.com>

Author: flodolo

pontoon/base/tests/views/test_upload.py

@@ -2,7 +2,10 @@
 
 import pytest
 
-from pontoon.base.models import Translation
+from django.contrib.messages import get_messages
+
+from pontoon.base.models import ChangedEntityLocale, Translation
+from pontoon.test.factories import EntityFactory, TranslationFactory
 
 
 @pytest.fixture
@@ -49,6 +52,39 @@ def upload(client, **args):
     return response
 
 
+@pytest.fixture
+def approved_po_translation(po_translation):
+    po_translation.approved = True
+    po_translation.active = True
+    po_translation.save()
+
+    yield po_translation
+
+
+@pytest.fixture
+def upload_po(translator_a, project_locale_a, po_translation):
+    """
+    Upload the given contents as a .po file for the `po_translation` resource,
+    returning the list of (tags, message) pairs.
+    """
+
+    def _upload_po(po_contents):
+        with NamedTemporaryFile("w+", suffix=".po") as fp:
+            fp.write(po_contents)
+            fp.flush()
+            response = upload(
+                translator_a.client,
+                slug=project_locale_a.project.slug,
+                code=project_locale_a.locale.code,
+                part=po_translation.entity.resource.path,
+                uploadfile=open(fp.name),
+            )
+        assert response.status_code == 303
+        return [(m.tags, m.message) for m in get_messages(response.wsgi_request)]
+
+    return _upload_po
+
+
 @pytest.mark.django_db
 def test_upload_login_required(
     client,
@@ -144,33 +180,174 @@ def test_upload_project_locale_is_readonly(
 
 
 @pytest.mark.django_db
-def test_upload_file(
-    translator_a,
-    project_locale_a,
-    po_translation,
-):
+def test_upload_file(upload_po, po_translation):
     """
     Test a positive upload which changes the translation.
     """
-    po_contents = 'msgid "test_key"\nmsgstr "new translation"'
-    with NamedTemporaryFile("w+", suffix=".po") as fp:
-        fp.write(po_contents)
-        fp.flush()
-        response = upload(
-            translator_a.client,
-            slug=project_locale_a.project.slug,
-            code=project_locale_a.locale.code,
-            part=po_translation.entity.resource.path,
-            uploadfile=open(fp.name),
-        )
-
-    assert response.status_code == 303
+    messages = upload_po('msgid "test_key"\nmsgstr "new translation"')
+    assert messages == [
+        ("upload success", "Translations uploaded: 1 updated, 0 unchanged.")
+    ]
 
     translation = Translation.objects.get(string="new translation")
-
-    assert translation.entity.key == ["test_key"]
-    assert translation.entity.resource.path == "resource_a.po"
+    assert translation.entity == po_translation.entity
     assert translation.approved
     assert translation.user
     assert not translation.warnings.exists()
     assert not translation.errors.exists()
+
+
+@pytest.mark.django_db
+def test_upload_is_additive(upload_po, approved_po_translation, locale_a):
+    """
+    Approved translations missing from the uploaded file are left untouched.
+    """
+    resource = approved_po_translation.entity.resource
+    other_translation = TranslationFactory(
+        entity=EntityFactory(resource=resource, string="other", key=["other_key"]),
+        locale=locale_a,
+        string="other translation",
+        value=["other translation"],
+        approved=True,
+        active=True,
+    )
+
+    messages = upload_po('msgid "test_key"\nmsgstr "new translation"')
+    assert messages == [
+        ("upload success", "Translations uploaded: 1 updated, 0 unchanged.")
+    ]
+
+    approved_po_translation.refresh_from_db()
+    assert not approved_po_translation.approved
+    assert approved_po_translation.rejected
+    assert (
+        Translation.objects.get(
+            entity=approved_po_translation.entity, approved=True
+        ).string
+        == "new translation"
+    )
+
+    other_translation.refresh_from_db()
+    assert other_translation.approved
+    assert other_translation.active
+    assert not other_translation.rejected
+
+
+@pytest.mark.django_db
+def test_upload_approves_matching_suggestion(upload_po, po_translation, locale_a):
+    """
+    An uploaded translation matching an existing suggestion approves it,
+    and the entity is still marked as changed for the next sync.
+    """
+    assert not po_translation.approved
+
+    messages = upload_po(f'msgid "test_key"\nmsgstr "{po_translation.string}"')
+    assert messages == [
+        ("upload success", "Translations uploaded: 1 updated, 0 unchanged.")
+    ]
+
+    assert Translation.objects.filter(entity=po_translation.entity).count() == 1
+    po_translation.refresh_from_db()
+    assert po_translation.approved
+    assert ChangedEntityLocale.objects.filter(
+        entity=po_translation.entity, locale=locale_a
+    ).exists()
+
+
+@pytest.mark.django_db
+def test_upload_approves_matching_fuzzy_translation(upload_po, po_translation):
+    """
+    An uploaded non-fuzzy translation matching an existing fuzzy one approves it.
+    """
+    po_translation.fuzzy = True
+    po_translation.active = True
+    po_translation.save()
+
+    messages = upload_po(f'msgid "test_key"\nmsgstr "{po_translation.string}"')
+    assert messages == [
+        ("upload success", "Translations uploaded: 1 updated, 0 unchanged.")
+    ]
+
+    assert Translation.objects.filter(entity=po_translation.entity).count() == 1
+    po_translation.refresh_from_db()
+    assert po_translation.approved
+    assert not po_translation.fuzzy
+
+
+@pytest.mark.django_db
+def test_upload_identical_translation_is_ignored(upload_po, approved_po_translation):
+    messages = upload_po(f'msgid "test_key"\nmsgstr "{approved_po_translation.string}"')
+    assert messages == [
+        ("upload info", "Translations uploaded: 0 updated, 1 unchanged.")
+    ]
+
+    assert (
+        Translation.objects.filter(entity=approved_po_translation.entity).count() == 1
+    )
+    approved_po_translation.refresh_from_db()
+    assert approved_po_translation.approved
+
+
+@pytest.mark.django_db
+def test_upload_undefined_keys_are_reported(upload_po, po_translation):
+    """
+    Translations for keys that are missing or obsolete in Pontoon are ignored.
+    """
+    resource = po_translation.entity.resource
+    EntityFactory(resource=resource, string="old", key=["obsolete_key"], obsolete=True)
+
+    messages = upload_po(
+        'msgid "test_key"\nmsgstr "new translation"\n\n'
+        'msgid "obsolete_key"\nmsgstr "obsolete translation"\n\n'
+        'msgid "missing_key"\nmsgstr "missing translation"\n'
+    )
+    assert messages == [
+        (
+            "upload success",
+            "Translations uploaded: 1 updated, 0 unchanged, 2 not found in Pontoon.",
+        )
+    ]
+    assert set(
+        Translation.objects.filter(entity__resource=resource).values_list(
+            "string", flat=True
+        )
+    ) == {po_translation.string, "new translation"}
+
+
+@pytest.mark.django_db
+def test_upload_ignores_translations_of_obsolete_entities(
+    upload_po, po_translation, locale_a
+):
+    """
+    A key removed and later re-added leaves an obsolete entity with the same key.
+    If the new key is untranslated, the upload should fill it and be counted as
+    translated, even if that matches the original translation in the obsolete key.
+    """
+    TranslationFactory(
+        entity=EntityFactory(
+            resource=po_translation.entity.resource,
+            string="entity a",
+            key=["test_key"],
+            obsolete=True,
+        ),
+        locale=locale_a,
+        string="new translation",
+        value=["new translation"],
+        approved=True,
+        active=True,
+    )
+
+    messages = upload_po('msgid "test_key"\nmsgstr "new translation"')
+    assert messages == [
+        ("upload success", "Translations uploaded: 1 updated, 0 unchanged.")
+    ]
+    assert (
+        Translation.objects.get(entity=po_translation.entity, approved=True).string
+        == "new translation"
+    )
+
+
+@pytest.mark.django_db
+def test_upload_file_without_translations(upload_po):
+    messages = upload_po("# Just a comment\n")
+    assert messages == [("error", "No translations found in uploaded file.")]

pontoon/base/views.py

@@ -34,6 +34,7 @@
 from pontoon.actionlog.models import ActionLog
 from pontoon.actionlog.utils import log_action
 from pontoon.base import forms, utils
+from pontoon.base.badge_utils import badges_review_level, badges_translation_level
 from pontoon.base.get_entities import (
     get_entities_for_project_locale,
     get_mismatched_filters,
@@ -71,7 +72,7 @@
 from pontoon.checks.libraries import run_checks
 from pontoon.checks.utils import are_blocking_checks
 from pontoon.contributors.utils import users_with_translations_counts
-from pontoon.messaging.notifications import send_notification
+from pontoon.messaging.notifications import send_badge_notification, send_notification
 
 
 log = logging.getLogger(__name__)
@@ -1058,26 +1059,42 @@ def upload(request):
         project, locale
     ):
         return HttpResponseForbidden("You don't have permission to upload files.")
-    get_object_or_404(Resource, project=project, path=res_path)
+    resource = get_object_or_404(Resource, project=project, path=res_path)
 
     form = forms.UploadFileForm(request.POST, request.FILES)
     if form.is_valid():
         from pontoon.sync.utils import import_uploaded_file
 
         upload = request.FILES["uploadfile"]
         try:
-            badge_name, badge_level = import_uploaded_file(
-                project, locale, res_path, upload, request.user
+            translation_before_level = badges_translation_level(request.user)
+            review_before_level = badges_review_level(request.user)
+            result = import_uploaded_file(
+                project, locale, resource, upload, request.user
             )
-            messages.success(request, "Translations updated from uploaded file.")
-            if badge_name:
-                message = json.dumps(
-                    {
-                        "name": badge_name,
-                        "level": badge_level,
-                    }
-                )
-                messages.info(request, message, extra_tags="badge")
+            summary = [f"{result.updated} updated", f"{result.unchanged} unchanged"]
+            if result.undefined:
+                summary.append(f"{result.undefined} not found in Pontoon")
+            message = f"Translations uploaded: {', '.join(summary)}."
+            if result.updated:
+                messages.success(request, message, extra_tags="upload")
+            else:
+                messages.info(request, message, extra_tags="upload")
+
+            badge_levels = (
+                (
+                    "Translation Champion",
+                    translation_before_level,
+                    badges_translation_level,
+                ),
+                ("Review Master", review_before_level, badges_review_level),
+            )
+            for badge_name, before_level, get_level in badge_levels:
+                after_level = get_level(request.user)
+                if after_level > before_level:
+                    send_badge_notification(request.user, badge_name, after_level)
+                    message = json.dumps({"name": badge_name, "level": after_level})
+                    messages.info(request, message, extra_tags="badge")
         except Exception as error:
             messages.error(request, str(error))
     else:

pontoon/sync/core/paths.py

@@ -48,24 +48,3 @@ def find_paths(
     log.debug(f"[{project.slug}] Paths({name}): ref_root={rel_root} base={rel_base}")
 
     return paths
-
-
-class UploadPaths:
-    """
-    moz.l10n.paths -like interface for sync'ing content from a single file.
-    Implements minimal functionality required by `find_db_updates()`.
-    """
-
-    ref_root = ""
-
-    def __init__(self, ref_path: str, locale_code: str, file_path: str):
-        self._ref_path = ref_path
-        self._locale_code = locale_code
-        self._file_path = file_path
-
-    def find_reference(self, target_path: str):
-        return (
-            (self._ref_path, {"locale": self._locale_code})
-            if target_path == self._file_path
-            else None
-        )

pontoon/sync/core/translations_from_repo.py

@@ -34,7 +34,6 @@
 from pontoon.checks import DB_FORMATS
 from pontoon.checks.utils import bulk_run_checks
 from pontoon.sync.core.checkout import Checkout, Checkouts
-from pontoon.sync.core.paths import UploadPaths
 from pontoon.sync.formats import RepoTranslation, as_repo_translations
 
 
@@ -136,7 +135,7 @@ def find_db_updates(
     project: Project,
     locale_map: dict[str, Locale],
     changed_target_paths: Iterable[str],
-    paths: L10nConfigPaths | L10nDiscoverPaths | UploadPaths,
+    paths: L10nConfigPaths | L10nDiscoverPaths,
     db_changes: Iterable[ChangedEntityLocale],
 ) -> Updates | None:
     """
@@ -178,9 +177,7 @@ def find_db_updates(
                 except Exception as error:
                     scope = f"[{project.slug}:{db_path}, {locale.code}]"
                     log.warning(f"{scope} Skipping resource with parse error: {error}")
-        elif splitext(target_path)[1] in l10n_extensions and not isinstance(
-            paths, UploadPaths
-        ):
+        elif splitext(target_path)[1] in l10n_extensions:
             log.debug(
                 f"[{project.slug}:{relpath(target_path, paths.base)}] Not an L10n target path"
             )