launchpad-reviewers team mailing list archive

[Tim Penhey <tim.penhey@xxxxxxxxxxxxx>]

Tim Penhey has proposed merging lp:~thumper/launchpad/recipe-build-email-fix into lp:launchpad/devel.

Requested reviews:
  Julian Edwards (julian-edwards): release-critical
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #614858 Recipe builds break the world when requester does not have a preferred email address
  https://bugs.launchpad.net/bugs/614858


The extra build parameters is currently causing issues on the build master if the owner of a recipe
is a team without an email address specified.  It is also possible to get the code into the same
state with a person who has deactivated their account.

The fix just uses a known email address and name for the situations where the build requester does
not have a preferred email.

tests:
  TestRecipeBuilder
-- 
https://code.launchpad.net/~thumper/launchpad/recipe-build-email-fix/+merge/32063
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~thumper/launchpad/recipe-build-email-fix into lp:launchpad/devel.

=== modified file 'lib/lp/code/model/recipebuilder.py'
--- lib/lp/code/model/recipebuilder.py	2010-07-13 10:20:47 +0000
+++ lib/lp/code/model/recipebuilder.py	2010-08-09 04:07:45 +0000
@@ -63,8 +63,14 @@
             suite += "-%s" % (self.build.pocket.name.lower())
         args['suite'] = suite
         args['arch_tag'] = distroarchseries.architecturetag
-        args["author_name"] = self.build.requester.displayname
-        args["author_email"] = self.build.requester.preferredemail.email
+        requester = self.build.requester
+        if requester.preferredemail is None:
+            # Use a constant, known, name and email.
+            args["author_name"] = 'Launchpad Package Builder'
+            args["author_email"] = 'noreply@xxxxxxxxxxxxx'
+        else:
+            args["author_name"] = requester.displayname
+            args["author_email"] = requester.preferredemail.email
         args["recipe_text"] = str(self.build.recipe.builder_recipe)
         args['archive_purpose'] = self.build.archive.purpose.name
         args["ogrecomponent"] = get_primary_current_component(

=== modified file 'lib/lp/code/model/tests/test_recipebuilder.py'
--- lib/lp/code/model/tests/test_recipebuilder.py	2010-07-13 10:20:47 +0000
+++ lib/lp/code/model/tests/test_recipebuilder.py	2010-08-09 04:07:45 +0000
@@ -3,17 +3,17 @@
 
 """Test RecipeBuildBehavior."""
 
+from __future__ import with_statement
+
 # pylint: disable-msg=F0401
 
 __metaclass__ = type
 
-import re
 import transaction
 import unittest
 
 from zope.security.proxy import removeSecurityProxy
 
-from canonical.config import config
 from canonical.testing import LaunchpadFunctionalLayer
 from canonical.launchpad.scripts.logger import BufferLogger
 
@@ -34,7 +34,7 @@
     MockBuilder, OkSlave)
 from lp.soyuz.tests.test_publishing import (
     SoyuzTestPublisher,)
-from lp.testing import TestCaseWithFactory
+from lp.testing import person_logged_in, TestCaseWithFactory
 
 
 class TestRecipeBuilder(TestCaseWithFactory):
@@ -52,7 +52,7 @@
         job = IBuildFarmJobBehavior(job)
         self.assertProvides(job, IBuildFarmJobBehavior)
 
-    def makeJob(self):
+    def makeJob(self, recipe_registrant=None, recipe_owner=None):
         """Create a sample `ISourcePackageRecipeBuildJob`."""
         spn = self.factory.makeSourcePackageName("apackage")
         distro = self.factory.makeDistribution(name="distro")
@@ -62,15 +62,20 @@
         distroseries.newArch(
             'i386', processorfamily, True, self.factory.makePerson())
         sourcepackage = self.factory.makeSourcePackage(spn, distroseries)
-        requester = self.factory.makePerson(email="requester@xxxxxxxxxx",
-            name="joe", displayname="Joe User")
-        somebranch = self.factory.makeBranch(owner=requester, name="pkg",
+        if recipe_registrant is None:
+            recipe_registrant = self.factory.makePerson(
+                email="requester@xxxxxxxxxx",
+                name="joe", displayname="Joe User")
+        if recipe_owner is None:
+            recipe_owner = recipe_registrant
+        somebranch = self.factory.makeBranch(
+            owner=recipe_owner, name="pkg",
             product=self.factory.makeProduct("someapp"))
-        recipe = self.factory.makeSourcePackageRecipe(requester, requester,
-             distroseries, u"recept", u"Recipe description",
-             branches=[somebranch])
+        recipe = self.factory.makeSourcePackageRecipe(
+            recipe_registrant, recipe_owner, distroseries, u"recept",
+            u"Recipe description", branches=[somebranch])
         spb = self.factory.makeSourcePackageRecipeBuild(
-            sourcepackage=sourcepackage, recipe=recipe, requester=requester,
+            sourcepackage=sourcepackage, recipe=recipe, requester=recipe_owner,
             distroseries=distroseries)
         job = spb.makeJob()
         job_id = removeSecurityProxy(job.job).id
@@ -124,15 +129,15 @@
             AssertionError, job.verifyBuildRequest, BufferLogger())
         self.assertIn('invalid pocket due to the series status of', str(e))
 
-    def test__extraBuildArgs(self):
+    def _setBuilderConfig(self):
+        """Setup a temporary builder config."""
+        self.pushConfig(
+            "builddmaster",
+            bzr_builder_sources_list="deb http://foo %(series)s main")
+
+    def test_extraBuildArgs(self):
         # _extraBuildArgs will return a sane set of additional arguments
-        bzr_builder_config = """
-            [builddmaster]
-            bzr_builder_sources_list = deb http://foo %(series)s main
-            """
-        config.push("bzr_builder_config", bzr_builder_config)
-        self.addCleanup(config.pop, "bzr_builder_config")
-
+        self._setBuilderConfig()
         job = self.makeJob()
         distroarchseries = job.build.distroseries.architectures[0]
         expected_archives = get_sources_list_for_building(
@@ -152,18 +157,55 @@
            'distroseries_name': job.build.distroseries.name,
             }, job._extraBuildArgs(distroarchseries))
 
+    def test_extraBuildArgs_team_owner_no_email(self):
+        # If the owner of the recipe is a team without a preferred email, the
+        # registrant is used.
+        self._setBuilderConfig()
+        recipe_registrant = self.factory.makePerson(
+            name='eric', displayname='Eric the Viking', email='eric@xxxxxxxxxxxx')
+        recipe_owner = self.factory.makeTeam(
+            name='vikings', members=[recipe_registrant])
+
+        job = self.makeJob(recipe_registrant, recipe_owner)
+        distroarchseries = job.build.distroseries.architectures[0]
+        extra_args = job._extraBuildArgs(distroarchseries)
+        self.assertEqual("Launchpad Package Builder", extra_args['author_name'])
+        self.assertEqual("noreply@xxxxxxxxxxxxx", extra_args['author_email'])
+
+    def test_extraBuildArgs_team_owner_with_email(self):
+        # If the owner of the recipe is a team that has an email set, we use
+        # that.
+        self._setBuilderConfig()
+        recipe_registrant = self.factory.makePerson()
+        recipe_owner = self.factory.makeTeam(
+            name='vikings', email='everyone@xxxxxxxxxxxx',
+            members=[recipe_registrant])
+
+        job = self.makeJob(recipe_registrant, recipe_owner)
+        distroarchseries = job.build.distroseries.architectures[0]
+        extra_args = job._extraBuildArgs(distroarchseries)
+        self.assertEqual("Vikings", extra_args['author_name'])
+        self.assertEqual("everyone@xxxxxxxxxxxx", extra_args['author_email'])
+
+    def test_extraBuildArgs_owner_deactivated(self):
+        # If the owner is deactivated, they have no preferred email.
+        self._setBuilderConfig()
+        owner = self.factory.makePerson()
+        with person_logged_in(owner):
+            owner.deactivateAccount('deactivating')
+        job = self.makeJob(owner)
+        distroarchseries = job.build.distroseries.architectures[0]
+        extra_args = job._extraBuildArgs(distroarchseries)
+        self.assertEqual("Launchpad Package Builder", extra_args['author_name'])
+        self.assertEqual("noreply@xxxxxxxxxxxxx", extra_args['author_email'])
+
     def test_extraBuildArgs_withBadConfigForBzrBuilderPPA(self):
         # Ensure _extraBuildArgs doesn't blow up with a badly formatted
         # bzr_builder_sources_list in the config.
-        bzr_builder_config = """
-            [builddmaster]
-            bzr_builder_sources_list = deb http://foo %(series) main
-            """
+        self.pushConfig(
+            "builddmaster",
+            bzr_builder_sources_list="deb http://foo %(series) main")
         # (note the missing 's' in %(series)
-
-        config.push("bzr_builder_config", bzr_builder_config)
-        self.addCleanup(config.pop, "bzr_builder_config")
-
         job = self.makeJob()
         distroarchseries = job.build.distroseries.architectures[0]
         expected_archives = get_sources_list_for_building(