Merge lp:~thumper/launchpad/recipe-build-email-fix into lp:launchpad

[Tim Penhey]

Handle the case where the build requester does not have a preferred email.

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

[Robert Collins (lifeless)]

I believe I can hand out rc, so rc=lifeless. If I can't please wait for Julian.

A possible tweak would be to use a symbolic name rather than the literal email address.

review: Approve

[Julian Edwards (julian-edwards)]

For Soyuz builds we do this:

        fromaddress = format_address(
            config.builddmaster.default_sender_name,
            config.builddmaster.default_sender_address)

which would address mine (and lifeless's) concern about the hard-coded literal.

r/rc=me with that change.

Also, did you look at lib/lp/soyuz/model/binarypackagebuild.py / notify() ? It has a bunch of rules to prevent spamming the innocent, but I think you'll be ok here if you're just notifying the build requester.

Cheers.

review: Approve (release-critical)

[Julian Edwards (julian-edwards)]

Ok I gather you already landed this from your comment on IRC. It's fine to
file a bug and fix it next cycle.

Cheers.

[Robert Collins (lifeless)]

It was landed with the literal replaced with a config lookup, so
shouldn't need anything more ?

[Julian Edwards (julian-edwards)]

On Monday 09 August 2010 12:11:03 Robert Collins wrote:
> It was landed with the literal replaced with a config lookup, so
> shouldn't need anything more ?

It's not the same config as the Soyuz code uses and we should be consistent.
There's two configs specifically for the builddmaster emails, one for the
sender's email and one for the sender's name. Tim is only using the config
for the email.

[Robert Collins (lifeless)]

Ok, that makes sense.

=== 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:58:43 +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"] = config.canonical.noreply_from_address
+        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:58:43 +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="[e-mail]",
-            name="joe", displayname="Joe User")
-        somebranch = self.factory.makeBranch(owner=requester, name="pkg",
+        if recipe_registrant is None:
+            recipe_registrant = self.factory.makePerson(
+                email="[e-mail]",
+                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='[e-mail]')
+        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("[e-mail]", 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='[e-mail]',
+            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("[e-mail]", 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("[e-mail]", 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(