launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Curtis Hovey has proposed merging lp:~sinzui/launchpad/reimport-inactive-templates into lp:launchpad.

Commit message:
Do not recreate inactive translation templates.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #571210 in Launchpad itself: "Unique violation when importing translations from a branch"
  https://bugs.launchpad.net/launchpad/+bug/571210

For more details, see:
https://code.launchpad.net/~sinzui/launchpad/reimport-inactive-templates/+merge/132421

I created OOPS-91882635e8551f53a0bd423d85dadcd5 while investigating
question #211684. The series has inactive templates
https://translations.launchpad.net/openteacher/3.x/+templates
and translation syncing is not seeing the two new POTS. I requested a
one-time import to see if a rescan would find the missing pots. Instead
the log shows an oops because org.openteacher.dialogs.updates is
inactive.
    IntegrityError: duplicate key value violates unique constraint
        "potemplate__productseries__name__key"
    DETAIL: Key (productseries, name)=(40409, org.openteacher.dialogs.updates)
    already exists

--------------------------------------------------------------------

RULES

    Pre-implementation: no one
    * POTemplates are unique to (productseries, name) and
      (distroseries, sourcepackagename, name). The are created by calling
      POTemplateSubset.new().
    * Calls to new() assumes that the set represents the full
      set of templates in a series, but since the set can be initialised
      with iscurrent=True, all the inactive POTemplates are ignored.
    * A bug was fixed 2 years ago that requires the set to iscurrent=True
      to properly work with the found files, but the calls to make new
      POTemplates still think the set is unfiltered.
    * Add a guard to POTemplateSubset.new() that checks the unfiltered
      series-package set, eg iscurrent=None, and raise ValueError to
      ensure we see the TB, not the Postgresql transaction error.
    * POTemplateSubset.new() is called in several places in approver.py,
      and each call is using a different strategy to determine if it
      must create a new template. Some strategies do not know the name
      unitl late in the process.
    * Before calling new(), the code must check if the proposed series,
      package, name combination exists and choose an exit strategy if
      it does.


QA

    * Visit https://translations.qastaging.launchpad.net/openteacher/3.x
    * Choose "Change synchronization settings", then
      "You can request a one-time import.", then "Request a one-time import"
    * Ask a webops to run cronscripts/rosetta-branches.py on qastaging.
    * Ask for the output of check there there is no oopse for "teacher".


LINT

    lib/lp/translations/interfaces/potemplate.py
    lib/lp/translations/model/approver.py
    lib/lp/translations/model/potemplate.py
    lib/lp/translations/tests/test_potemplate.py
    lib/lp/translations/tests/test_translationbranchapprover.py
    lib/lp/translations/tests/test_translationbuildapprover.py
-- 
https://code.launchpad.net/~sinzui/launchpad/reimport-inactive-templates/+merge/132421
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/reimport-inactive-templates into lp:launchpad.

=== modified file 'lib/lp/translations/interfaces/potemplate.py'
--- lib/lp/translations/interfaces/potemplate.py	2011-12-24 16:54:44 +0000
+++ lib/lp/translations/interfaces/potemplate.py	2012-11-01 15:53:21 +0000
@@ -599,8 +599,21 @@
     def __getitem__(name):
         """Get a POTemplate by its name."""
 
+    def isNameUnique(name):
+        """Is the IPOTemplate name unique to the series (and package).
+
+        The subset may only include active `IPOTemplate` objects
+        (iscurrent=True), but the full set that constrains creating new
+        templates includes inactive templates too. Use this method to
+        verify that an `IPOTemplate` can be created before calling new().
+        """
+
     def new(name, translation_domain, path, owner, copy_pofiles=True):
-        """Create a new template for the context of this Subset."""
+        """Create a new template for the context of this Subset.
+
+        The name must be unique to the full subset of active and inactive
+        templates in a series (and package). See `isNameUnique`.
+        """
 
     def getPOTemplateByName(name):
         """Return the `IPOTemplate` with the given name or None.

=== modified file 'lib/lp/translations/model/approver.py'
--- lib/lp/translations/model/approver.py	2011-05-27 19:53:20 +0000
+++ lib/lp/translations/model/approver.py	2012-11-01 15:53:21 +0000
@@ -141,6 +141,9 @@
                 return entry
             # No (possibly) matching entry found: create one.
             name = make_name(domain)
+            if not self._potemplateset.isNameUnique(name):
+                # The name probably matches an inactive template.
+                return entry
             potemplate = self._potemplateset.new(
                 name, domain, entry.path, entry.importer)
             self._potemplates[entry.path] = potemplate
@@ -206,6 +209,9 @@
             else:
                 domain = self._potemplateset.sourcepackagename.name
             name = domain
+            if not self._potemplateset.isNameUnique(name):
+                # The name probably matches an inactive template.
+                return None
             return self._potemplateset.new(name, domain, path, self.owner)
         elif potemplateset_size == 1:
             # Use the one template that is there.
@@ -240,6 +246,9 @@
                 potemplate = self._potemplateset.getPOTemplateByName(name)
                 if potemplate is None:
                     # Still no template found, create a new one.
+                    if not self._potemplateset.isNameUnique(name):
+                        # The name probably matches an inactive template.
+                        return None
                     potemplate = self._potemplateset.new(
                         name, domain, path, self.owner)
         return potemplate

=== modified file 'lib/lp/translations/model/potemplate.py'
--- lib/lp/translations/model/potemplate.py	2012-09-26 08:09:31 +0000
+++ lib/lp/translations/model/potemplate.py	2012-11-01 15:53:21 +0000
@@ -1181,8 +1181,27 @@
             # Do not continue, else it would trigger an existingpo assertion.
             return
 
+    def _getSuperSet(self):
+        """Return the set of all POTemplates for this series and package."""
+        if self.iscurrent is None:
+            return self
+        else:
+            return getUtility(IPOTemplateSet).getSubset(
+                productseries=self.productseries,
+                distroseries=self.distroseries,
+                sourcepackagename=self.sourcepackagename)
+
+    def isNameUnique(self, name):
+        """See `IPOTemplateSubset`."""
+        return self._getSuperSet().getPOTemplateByName(name) is None
+
     def new(self, name, translation_domain, path, owner, copy_pofiles=True):
         """See `IPOTemplateSubset`."""
+        existing_template = self._getSuperSet().getPOTemplateByName(name)
+        if existing_template is not None:
+            raise ValueError(
+                'POTempate %s already exists and is iscurrent=%s' %
+                (name, existing_template.iscurrent))
         header_params = {
             'origin': 'PACKAGE VERSION',
             'templatedate': datetime.datetime.now(),

=== modified file 'lib/lp/translations/tests/test_potemplate.py'
--- lib/lp/translations/tests/test_potemplate.py	2012-01-01 02:58:52 +0000
+++ lib/lp/translations/tests/test_potemplate.py	2012-11-01 15:53:21 +0000
@@ -847,6 +847,20 @@
 
         self.assertEqual(templates, found_templates)
 
+    def test_isNameUnique(self):
+        # The isNameUnique method ignored the iscurrent filter to provide
+        # an authoritative answer to whether a new template can be created
+        # with the name.
+        series = self.factory.makeProductSeries()
+        self.factory.makePOTemplate(productseries=series, name='cat')
+        self.factory.makePOTemplate(
+            productseries=series, name='dog', iscurrent=False)
+        potset = getUtility(IPOTemplateSet)
+        subset = potset.getSubset(productseries=series, iscurrent=True)
+        self.assertFalse(subset.isNameUnique('cat'))
+        self.assertFalse(subset.isNameUnique('dog'))
+        self.assertTrue(subset.isNameUnique('fnord'))
+
     def test_getPOTemplatesByTranslationDomain_returns_result_set(self):
         subset = getUtility(IPOTemplateSet).getSubset(
             productseries=self.factory.makeProductSeries())

=== modified file 'lib/lp/translations/tests/test_translationbranchapprover.py'
--- lib/lp/translations/tests/test_translationbranchapprover.py	2012-01-20 15:42:44 +0000
+++ lib/lp/translations/tests/test_translationbranchapprover.py	2012-11-01 15:53:21 +0000
@@ -151,6 +151,18 @@
         self._createApprover(template_path).approve(entry)
         self.assertEqual(potemplate, entry.potemplate)
 
+    def test_ignore_existing_inactive_potemplate(self):
+        # When replacing an existing inactive template, the entry is not
+        # approved and no template is created for it.
+        translation_domain = self.factory.getUniqueString()
+        template_path = translation_domain + u'.pot'
+        potemplate = self._createTemplate(template_path, translation_domain)
+        potemplate.setActive(False)
+        entry = self._upload_file(template_path)
+        self._createApprover(template_path).approve(entry)
+        self.assertEqual(RosettaImportStatus.NEEDS_REVIEW, entry.status)
+        self.assertEqual(None, entry.potemplate)
+
     def test_replace_existing_any_path(self):
         # If just one template file is found in the tree and just one
         # POTemplate is in the database, the upload is always approved.

=== modified file 'lib/lp/translations/tests/test_translationbuildapprover.py'
--- lib/lp/translations/tests/test_translationbuildapprover.py	2012-01-20 16:11:11 +0000
+++ lib/lp/translations/tests/test_translationbuildapprover.py	2012-11-01 15:53:21 +0000
@@ -127,6 +127,23 @@
         self.assertEqual('domain3', entries[2].potemplate.name)
         self.assertEqual('domain4', entries[3].potemplate.name)
 
+    def test_approve_inactive_existing(self):
+        # Inactive templates are approved, but they remain inactive.
+        filenames = [
+            'po-domain1/domain1.pot',
+            ]
+        series = self.factory.makeProductSeries()
+        domain1_pot = self.factory.makePOTemplate(
+            productseries=series, name='domain1', iscurrent=False)
+        self._becomeBuilddMaster()
+        approver = TranslationBuildApprover(filenames, productseries=series)
+        entries = self._makeApprovedEntries(series, approver, filenames)
+        self.assertEqual(
+            [RosettaImportStatus.APPROVED],
+            [entry.status for entry in entries])
+        self.assertEqual(
+            [domain1_pot], [entry.potemplate for entry in entries])
+
     def test_approve_generic_name_one_new(self):
         # Generic names are OK, if there is only one.
         filenames = [