launchpad-reviewers team mailing list archive

[Henning Eggers <henning.eggers@xxxxxxxxxxxxx>]

Henning Eggers has proposed merging lp:~henninge/launchpad/devel-710591-importer into lp:launchpad with lp:~henninge/launchpad/devel-710591-setcurrenttranslation-extension as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #710591 Ubuntu upstream translation imports overwrite Ubuntu translations
  https://bugs.launchpad.net/bugs/710591

For more details, see:
https://code.launchpad.net/~henninge/launchpad/devel-710591-importer/+merge/48682

= Summary =

This is the final branch to fix bug 710591. It brings together
the functions and methods that the previous two introduced.

== Proposed fix ==

Add a boolean property to FileImporter that makes use
has_upstream_template to determine which "accept" method to
use.
Replace the use of "approve" in the importer with the
"accept" mtethods.

== Pre-implementation notes ==

I still refer to my chat with Danilo about this but also
Jeroen and I talked a bit while he reviewed the last
branch.

== Implementation details ==

Pretty straight forward implementation, I'd say.

== Tests ==

bin/test -vvcm lp.translations.utilities.tests.test_file_importer

I also ran this to check that anything related to "import" might
be affected:

bin/test -vvcm lp.translations -t import

== Demo and Q/A ==

Import on a source package and see what happens.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/translations/interfaces/translationmessage.py
  lib/lp/translations/tests/test_translationmessage.py
  lib/lp/testing/factory.py
  lib/lp/translations/utilities/tests/test_file_importer.py
  lib/lp/translations/model/potmsgset.py
  lib/lp/translations/utilities/translation_import.py
  lib/lp/translations/model/translationmessage.py

./lib/lp/translations/tests/test_translationmessage.py
     365: E302 expected 2 blank lines, found 1
-- 
https://code.launchpad.net/~henninge/launchpad/devel-710591-importer/+merge/48682
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~henninge/launchpad/devel-710591-importer into lp:launchpad.

=== modified file 'lib/lp/translations/utilities/tests/test_file_importer.py'
--- lib/lp/translations/utilities/tests/test_file_importer.py	2011-01-24 15:51:18 +0000
+++ lib/lp/translations/utilities/tests/test_file_importer.py	2011-02-05 00:29:03 +0000
@@ -11,7 +11,6 @@
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
-from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
 from canonical.librarian.testing.fake import FakeLibrarian
 from canonical.testing import (
     LaunchpadZopelessLayer,
@@ -21,6 +20,7 @@
 from lp.testing import TestCaseWithFactory
 from lp.translations.enums import TranslationPermission
 from lp.translations.interfaces.potemplate import IPOTemplateSet
+from lp.translations.interfaces.side import TranslationSide
 from lp.translations.interfaces.translationfileformat import (
     TranslationFileFormat,
     )
@@ -582,9 +582,6 @@
     """Class test for the sharing operation of the FileImporter base class."""
     layer = LaunchpadZopelessLayer
 
-    UPSTREAM = 0
-    UBUNTU = 1
-
     POFILE = dedent("""\
         msgid ""
         msgstr ""
@@ -612,18 +609,15 @@
 
     def _makeImportEntry(self, side, by_maintainer=False, uploader=None,
                          no_upstream=False):
-        if side == self.UPSTREAM:
+        if side == TranslationSide.UPSTREAM:
             potemplate = self.upstream_template
         else:
             # Create a template in a source package.
-            ubuntu = getUtility(ILaunchpadCelebrities).ubuntu
-            distroseries = self.factory.makeDistroSeries(distribution=ubuntu)
-            ubuntu.translation_focus = distroseries
-            sourcepackagename = self.factory.makeSourcePackageName()
             potemplate = self.factory.makePOTemplate(
-                distroseries=distroseries,
-                sourcepackagename=sourcepackagename,
-                name=self.upstream_template.name)
+                name=self.upstream_template.name, side=side)
+            distroseries = potemplate.distroseries
+            sourcepackagename = potemplate.sourcepackagename
+            distroseries.distribution.translation_focus = distroseries
             if not no_upstream:
                 # Link the source package to the upstream series to
                 # enable sharing.
@@ -657,7 +651,7 @@
 
     def test_makeImportEntry_templates_are_sharing(self):
         # Sharing between upstream and Ubuntu was set up correctly.
-        entry = self._makeImportEntry(self.UBUNTU)
+        entry = self._makeImportEntry(TranslationSide.UBUNTU)
         subset = getUtility(IPOTemplateSet).getSharingSubset(
                 distribution=entry.distroseries.distribution,
                 sourcepackagename=entry.sourcepackagename)
@@ -667,7 +661,7 @@
 
     def test_share_with_other_side_upstream(self):
         # An upstream queue entry will be shared with ubuntu.
-        entry = self._makeImportEntry(self.UPSTREAM)
+        entry = self._makeImportEntry(TranslationSide.UPSTREAM)
         importer = POFileImporter(
             entry, importers[TranslationFileFormat.PO], None)
         self.assertTrue(
@@ -676,7 +670,7 @@
 
     def test_share_with_other_side_ubuntu(self):
         # An ubuntu queue entry will not be shared with upstream.
-        entry = self._makeImportEntry(self.UBUNTU)
+        entry = self._makeImportEntry(TranslationSide.UBUNTU)
         importer = POFileImporter(
             entry, importers[TranslationFileFormat.PO], None)
         self.assertFalse(
@@ -685,7 +679,8 @@
 
     def test_share_with_other_side_ubuntu_no_upstream(self):
         # An ubuntu queue entry cannot share with a non-existent upstream.
-        entry = self._makeImportEntry(self.UBUNTU, no_upstream=True)
+        entry = self._makeImportEntry(
+            TranslationSide.UBUNTU, no_upstream=True)
         importer = POFileImporter(
             entry, importers[TranslationFileFormat.PO], None)
         self.assertFalse(
@@ -696,9 +691,49 @@
         # If the uploader in ubuntu has rights on upstream as well, the
         # translations are shared.
         entry = self._makeImportEntry(
-            self.UBUNTU, uploader=self.translator.translator)
+            TranslationSide.UBUNTU, uploader=self.translator.translator)
         importer = POFileImporter(
             entry, importers[TranslationFileFormat.PO], None)
         self.assertTrue(
             importer.share_with_other_side,
             "Ubuntu import should share with upstream.")
+
+    def test_is_upstream_import_on_sourcepackage_none(self):
+        # To do an upstream import on a sourcepackage, three conditions must
+        # be met.
+        # - It has to be on a sourcepackage.
+        # - The by_maintainer flag must be set.
+        # - There must be no matching template in the upstream project or
+        #   even no upstream project at all.
+        # This case meets none of them.
+        entry = self._makeImportEntry(
+            TranslationSide.UPSTREAM, uploader=self.translator.translator)
+        importer = POFileImporter(
+            entry, importers[TranslationFileFormat.PO], None)
+        self.assertFalse(importer.is_upstream_import_on_sourcepackage)
+
+    def test_is_upstream_import_on_sourcepackage_by_maintainer(self):
+        # This entry is by_maintainer.
+        entry = self._makeImportEntry(
+            TranslationSide.UPSTREAM, by_maintainer=True,
+            uploader=self.translator.translator)
+        importer = POFileImporter(
+            entry, importers[TranslationFileFormat.PO], None)
+        self.assertFalse(importer.is_upstream_import_on_sourcepackage)
+
+    def test_is_upstream_import_on_sourcepackage_upstream_template(self):
+        # This entry is for a sourcepackage with an upstream potemplate.
+        entry = self._makeImportEntry(
+            TranslationSide.UBUNTU, uploader=self.translator.translator)
+        importer = POFileImporter(
+            entry, importers[TranslationFileFormat.PO], None)
+        self.assertFalse(importer.is_upstream_import_on_sourcepackage)
+
+    def test_is_upstream_import_on_sourcepackage_ok(self):
+        # This entry qualifies.
+        entry = self._makeImportEntry(
+            TranslationSide.UBUNTU, by_maintainer=True, no_upstream=True,
+            uploader=self.translator.translator)
+        importer = POFileImporter(
+            entry, importers[TranslationFileFormat.PO], None)
+        self.assertTrue(importer.is_upstream_import_on_sourcepackage)

=== modified file 'lib/lp/translations/utilities/translation_import.py'
--- lib/lp/translations/utilities/translation_import.py	2011-01-04 17:23:42 +0000
+++ lib/lp/translations/utilities/translation_import.py	2011-02-05 00:29:03 +0000
@@ -432,6 +432,25 @@
             purportedly_upstream=from_upstream)
 
     @cachedproperty
+    def is_upstream_import_on_sourcepackage(self):
+        """Use TranslationMessage.acceptFromUpstreamImportOnPackage`."""
+        if self.pofile is None:
+            return False
+        if not self.translation_import_queue_entry.by_maintainer:
+            return False
+        if self.translation_import_queue_entry.sourcepackagename is None:
+            return False
+        distroseries = self.translation_import_queue_entry.distroseries
+        sourcepackagename = (
+            self.translation_import_queue_entry.sourcepackagename)
+        templatename = self.potemplate.name
+        from lp.translations.utilities.translationsharinginfo import (
+            has_upstream_template)
+        return not has_upstream_template(
+            distroseries=distroseries, sourcepackagename=sourcepackagename,
+            templatename=templatename)
+
+    @cachedproperty
     def translations_are_msgids(self):
         """Are these English strings instead of translations?
 
@@ -466,12 +485,16 @@
         message.validation_status = TranslationValidationStatus.OK
         return True
 
-    def _approveMessage(self, potmsgset, message, message_data):
+    def _acceptMessage(self, potmsgset, message, message_data):
         """Try to approve the message, return None on TranslationConflict."""
         try:
-            message.approve(
-                self.pofile, self.last_translator,
-                self.share_with_other_side, self.lock_timestamp)
+            if self.is_upstream_import_on_sourcepackage:
+                message.acceptFromUpstreamImportOnPackage(
+                    self.pofile, self.lock_timestamp)
+            else:
+                message.acceptFromImport(
+                    self.pofile, self.share_with_other_side,
+                    self.lock_timestamp)
         except TranslationConflict:
             self._addConflictError(message_data, potmsgset)
             if self.logger is not None:
@@ -529,7 +552,7 @@
         validation_ok = self._validateMessage(
             potmsgset, new_message, sanitized_translations, message_data)
         if validation_ok and self.is_editor:
-            return self._approveMessage(potmsgset, new_message, message_data)
+            return self._acceptMessage(potmsgset, new_message, message_data)
 
         return new_message