launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/launchpad/bug-594220 into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers): code
Related bugs:
  #594220 Template imports built from branches are owned by VCS imports.
  https://bugs.launchpad.net/bugs/594220


= Bug 594220 =

Translations uploads can enter the queue in several ways:
 * Soyuz can upload ones generated during package build.
 * Users can upload files.
 * We can import them from bzr branches.
 * The build farm can generate templates every time a branch changes.

In that last avenue, the resulting entries on the translations import queue are owned by whoever owns the branch.  But what if the branch is mirrored?  In that case, there is no reliably identifiable owner in Launchpad and such branches are owned by the vcs-imports celebrity.

Each of these imports normally results in an email message (either a success confirmation or a failure notice).  And thus, vcs-imports got inundated with email when we implemented the generate-templates-from-branches feature.  In this branch I stop these notifications from being sent.

We discussed alternatives, including sending the notifications to some other party, but we don't want to dump excessive email messages onto unsuspecting users.  If there are problems with the imports that the emails would warn about, the import-queue UI will provide the same diagnostic information.

To test:
{{{
./bin/test -vvc lp.translations.scripts.tests.test_translations_import
}}}

For Q/A, we can verify that:
 * templates generated from mirrored branches still hit the import queue,
 * notification emails are still sent out, and
 * vcs-imports stops receiving emails.

What with email filtering on staging, the surest way to verify that last point is to inquire after rollout.  We should do that in addition to pre-rollout Q/A.


No lint,

Jeroen
-- 
https://code.launchpad.net/~jtv/launchpad/bug-594220/+merge/35770
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/launchpad/bug-594220 into lp:launchpad/devel.

=== modified file 'lib/lp/translations/scripts/po_import.py'
--- lib/lp/translations/scripts/po_import.py	2010-08-20 20:31:18 +0000
+++ lib/lp/translations/scripts/po_import.py	2010-09-17 03:06:24 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Functions used with the Rosetta PO import script."""
@@ -119,13 +119,21 @@
 
         return True
 
+    def _shouldNotify(self, person):
+        """Is `person` someone we should send notification emails?"""
+        # We don't notify the vcs-imports team, which owns all mirrored
+        # branches.  Templates generated in the build farm based on
+        # mirrored branches are uploaded in the name of this team, but
+        # there is no point in sending out notifications to them.
+        return person != getUtility(ILaunchpadCelebrities).vcs_imports
+
     def _importEntry(self, entry):
         """Perform the import of one entry, and notify the uploader."""
-        self.logger.info('Importing: %s' % entry.import_into.title)
         target = entry.import_into
+        self.logger.info('Importing: %s' % target.title)
         (mail_subject, mail_body) = target.importFromQueue(entry, self.logger)
 
-        if mail_subject is not None:
+        if mail_subject is not None and self._shouldNotify(entry.importer):
             # A `mail_subject` of None indicates that there
             # is no notification worth sending out.
             from_email = config.rosetta.admin_email

=== modified file 'lib/lp/translations/scripts/tests/test_translations_import.py'
--- lib/lp/translations/scripts/tests/test_translations_import.py	2010-08-20 20:31:18 +0000
+++ lib/lp/translations/scripts/tests/test_translations_import.py	2010-09-17 03:06:24 +0000
@@ -1,13 +1,18 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 import logging
 import re
-from unittest import TestLoader
+import transaction
+from zope.component import getUtility
 
+from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
 from canonical.launchpad.webapp import errorlog
 from canonical.testing.layers import LaunchpadScriptLayer
+from lp.services.mail import stub
 from lp.testing import TestCaseWithFactory
+from lp.testing.fakelibrarian import FakeLibrarian
+from lp.testing.fakemethod import FakeMethod
 from lp.translations.interfaces.translationimportqueue import (
     RosettaImportStatus,
     )
@@ -25,16 +30,6 @@
     """Very serious system error."""
 
 
-class Raiser:
-    """Raise a given exception."""
-    def __init__(self, exception):
-        self.exception = exception
-
-    def __call__(self, *args, **kwargs):
-        """Whatever the arguments, just raise self.exception."""
-        raise self.exception
-
-
 class TestTranslationsImport(TestCaseWithFactory):
 
     layer = LaunchpadScriptLayer
@@ -52,8 +47,30 @@
 
     def _makeEntry(self, path, **kwargs):
         """Produce a queue entry."""
+        uploader = kwargs.pop('uploader', self.owner)
         return self.queue.addOrUpdateEntry(
-            path, '# Nothing here', False, self.owner, **kwargs)
+            path, '# Nothing here', False, uploader, **kwargs)
+
+    def _makeApprovedEntry(self, uploader):
+        """Produce an approved queue entry."""
+        fake_librarian = self.installFixture(FakeLibrarian())
+
+        path = '%s.pot' % self.factory.getUniqueString()
+        series = self.factory.makeProductSeries()
+        template = self.factory.makePOTemplate(series)
+        entry = self._makeEntry(
+            path, uploader=uploader, potemplate=template,
+            productseries=template.productseries)
+        entry.status = RosettaImportStatus.APPROVED
+
+        fake_librarian.pretendCommit()
+        return entry
+
+    def _getEmailRecipients(self):
+        """List the recipients of all pending outgoing emails."""
+        return sum([
+            recipients
+            for sender, recipients, text in stub.test_emails], [])
 
     def test_describeEntry_without_target(self):
         productseries = self._makeProductSeries()
@@ -136,7 +153,8 @@
         self.assertNotEqual(None, entry.import_into)
 
         message = "The system has exploded."
-        self.script._importEntry = Raiser(OutrageousSystemError(message))
+        self.script._importEntry = FakeMethod(
+            failure=OutrageousSystemError(message))
         self.assertRaises(OutrageousSystemError, self.script.main)
 
         self.assertEqual(RosettaImportStatus.FAILED, entry.status)
@@ -153,7 +171,8 @@
         self.assertNotEqual(None, entry.import_into)
 
         message = "Nobody expects the Spanish Inquisition!"
-        self.script._importEntry = Raiser(UnexpectedException(message))
+        self.script._importEntry = FakeMethod(
+            failure=UnexpectedException(message))
         self.script.main()
 
         self.assertEqual(RosettaImportStatus.FAILED, entry.status)
@@ -167,6 +186,18 @@
         self.assertEqual(default_reporting.oops_prefix,
                          errorlog.globalErrorUtility.oops_prefix)
 
+    def test_notifies_uploader(self):
+        entry = self._makeApprovedEntry(self.owner)
+        transaction.commit()
+        self.script._importEntry(entry)
+        transaction.commit()
+        self.assertEqual(
+            [self.owner.preferredemail.email], self._getEmailRecipients())
 
-def test_suite():
-    return TestLoader().loadTestsFromName(__name__)
+    def test_does_not_notify_vcs_imports(self):
+        vcs_imports = getUtility(ILaunchpadCelebrities).vcs_imports
+        entry = self._makeApprovedEntry(vcs_imports)
+        transaction.commit()
+        self.script._importEntry(entry)
+        transaction.commit()
+        self.assertEqual([], self._getEmailRecipients())