Merge lp:~abentley/launchpad/detailed-build-emails into lp:launchpad

[Aaron Bentley]

Make recipe build emails more like binary build ones

= Summary =
Fix bug #603606: Recipe Build failure emails aren't as binary package build failures

== Proposed fix ==
Update recipe build emails to use similar formatting to bianary build emails.

== Pre-implementation notes ==
Mid-implementation with Tim Penhey

== Implementation details ==
The formatting now matches. The fields supplied are:
State, Recipe, Archive, Distroseries, Duration, Build Log and Builder.

Similarly, the subject line contains recipe build id, recipe, distroseries
and status.

Component and pocket are not provided as these do not vary.

== Tests ==
bin/test test_sourcepackagerecipebuild -t notif -t generateEmail.

== Demo and Q/A ==
Request a build. When is processed, the resulting mail should be as described.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/code/model/tests/test_sourcepackagerecipebuild.py
  lib/lp/code/mail/sourcepackagerecipebuild.py
  lib/lp/testing/factory.py
  lib/lp/code/model/sourcepackagerecipebuild.py
  lib/canonical/launchpad/emailtemplates/build-request.txt
  lib/lp/code/mail/tests/test_sourcepackagerecipebuild.py

./lib/lp/code/mail/sourcepackagerecipebuild.py
       5: E303 too many blank lines (3)
      71: Line exceeds 78 characters.
      46: E231 missing whitespace after ','
./lib/lp/code/mail/tests/test_sourcepackagerecipebuild.py
      15: 'IStore' imported but unused
       5: E303 too many blank lines (3)
      37: W291 trailing whitespace
      38: W291 trailing whitespace
      39: W291 trailing whitespace
      42: E302 expected 2 blank lines, found 1
      37: Line has trailing whitespace.
      38: Line has trailing whitespace.
      39: Line has trailing whitespace.

[Paul Hummer (rockstar)]

Looks fine to me.

review: Approve (code)

=== modified file 'lib/canonical/launchpad/emailtemplates/build-request.txt'
--- lib/canonical/launchpad/emailtemplates/build-request.txt	2010-06-08 18:31:56 +0000
+++ lib/canonical/launchpad/emailtemplates/build-request.txt	2010-08-01 04:36:51 +0000
@@ -1,1 +1,7 @@
-Build %(recipe_owner)s/%(recipe)s into %(archive)s for %(distroseries)s: %(status)s.
+ * State: %(status)s
+ * Recipe: %(recipe_owner)s/%(recipe)s
+ * Archive: %(archive_owner)s/%(archive)s
+ * Distroseries: %(distroseries)s
+ * Duration: %(duration)s
+ * Build Log: %(log_url)s
+ * Builder: %(builder_url)s

=== modified file 'lib/lp/code/mail/sourcepackagerecipebuild.py'
--- lib/lp/code/mail/sourcepackagerecipebuild.py	2010-06-09 20:24:11 +0000
+++ lib/lp/code/mail/sourcepackagerecipebuild.py	2010-08-01 04:36:51 +0000
@@ -11,6 +11,7 @@
 
 from canonical.config import config
 from canonical.launchpad.webapp import canonical_url
+from canonical.launchpad.webapp.tales import DurationFormatterAPI
 from lp.services.mail.basemailer import BaseMailer, RecipientReason
 
 
@@ -25,7 +26,8 @@
         requester = build.requester
         recipients = {requester: RecipientReason.forBuildRequester(requester)}
         return cls(
-            '%(status)s: %(recipe)s for %(distroseries)s',
+            '[recipe build #%(build_id)d] of ~%(recipe_owner)s %(recipe)s in'
+            ' %(distroseries)s: %(status)s',
             'build-request.txt', recipients,
             config.canonical.noreply_from_address, build)
 
@@ -50,13 +52,26 @@
         params = super(
             SourcePackageRecipeBuildMailer, self)._getTemplateParams(email)
         params.update({
+            'build_id': self.build.id,
             'status': self.build.buildstate.title,
             'distroseries': self.build.distroseries.name,
             'recipe': self.build.recipe.name,
             'recipe_owner': self.build.recipe.owner.name,
             'archive': self.build.archive.name,
+            'archive_owner': self.build.archive.owner.name,
+            'log_url': '',
+            'component': self.build.current_component.name,
+            'duration': '',
+            'builder_url': '',
             'build_url': canonical_url(self.build),
         })
+        if self.build.builder is not None:
+            params['builder_url'] = canonical_url(self.build.builder)
+        if self.build.buildduration is not None:
+            duration_formatter = DurationFormatterAPI(self.build.buildduration)
+            params['duration'] = duration_formatter.approximateduration()
+        if self.build.build_log_url is not None:
+            params['log_url'] = self.build.build_log_url
         return params
 
     def _getFooter(self, params):

=== modified file 'lib/lp/code/mail/tests/test_sourcepackagerecipebuild.py'
--- lib/lp/code/mail/tests/test_sourcepackagerecipebuild.py	2010-06-09 20:24:11 +0000
+++ lib/lp/code/mail/tests/test_sourcepackagerecipebuild.py	2010-08-01 04:36:51 +0000
@@ -5,42 +5,68 @@
 __metaclass__ = type
 
 
+from datetime import timedelta
 from unittest import TestLoader
 
+from storm.locals import Store
+from zope.security.proxy import removeSecurityProxy
+
 from canonical.config import config
 from canonical.launchpad.interfaces.lpstorm import IStore
-from canonical.testing import DatabaseFunctionalLayer
+from canonical.testing import LaunchpadFunctionalLayer
 from lp.buildmaster.interfaces.buildbase import BuildStatus
 from lp.code.mail.sourcepackagerecipebuild import (
     SourcePackageRecipeBuildMailer)
 from lp.testing import TestCaseWithFactory
 
+expected_body = u"""\
+ * State: Successfully built
+ * Recipe: person/recipe
+ * Archive: archiveowner/ppa
+ * Distroseries: distroseries
+ * Duration: five minutes
+ * Build Log: %s
+ * Builder: http://launchpad.dev/builders/bob
+"""
+
+superseded_body = u"""\
+ * State: Build for superseded Source
+ * Recipe: person/recipe
+ * Archive: archiveowner/ppa
+ * Distroseries: distroseries
+ * Duration: 
+ * Build Log: 
+ * Builder: 
+"""
 
 class TestSourcePackageRecipeBuildMailer(TestCaseWithFactory):
 
-    layer = DatabaseFunctionalLayer
+    layer = LaunchpadFunctionalLayer
 
     def test_generateEmail(self):
         """GenerateEmail produces the right headers and body."""
         person = self.factory.makePerson(name='person')
         cake = self.factory.makeSourcePackageRecipe(
             name=u'recipe', owner=person)
-        pantry = self.factory.makeArchive(name='ppa')
+        pantry_owner = self.factory.makePerson(name='archiveowner')
+        pantry = self.factory.makeArchive(name='ppa', owner=pantry_owner)
         secret = self.factory.makeDistroSeries(name=u'distroseries')
         build = self.factory.makeSourcePackageRecipeBuild(
             recipe=cake, distroseries=secret, archive=pantry,
-            status=BuildStatus.FULLYBUILT)
-        IStore(build).flush()
+            status=BuildStatus.FULLYBUILT, duration=timedelta(minutes=5))
+        naked_build = removeSecurityProxy(build)
+        naked_build.builder = self.factory.makeBuilder(name='bob')
+        naked_build.buildlog = self.factory.makeLibraryFileAlias()
+        Store.of(build).flush()
         mailer = SourcePackageRecipeBuildMailer.forStatus(build)
         email = build.requester.preferredemail.email
         ctrl = mailer.generateEmail(email, build.requester)
-        self.assertEqual('Successfully built: recipe for distroseries',
-            ctrl.subject)
+        self.assertEqual(
+            u'[recipe build #%d] of ~person recipe in distroseries: '
+            'Successfully built' % (build.id), ctrl.subject)
         body, footer = ctrl.body.split('\n-- \n')
         self.assertEqual(
-            'Build person/recipe into ppa for distroseries: Successfully'
-            ' built.\n', body
-            )
+            expected_body % build.build_log_url, body)
         self.assertEqual(
             'http://code.launchpad.dev/~person/+recipe/recipe/+build/1\n'
             'You are the requester of the build.\n', footer)
@@ -54,6 +80,39 @@
         self.assertEqual(
             'FULLYBUILT', ctrl.headers['X-Launchpad-Build-State'])
 
+    def test_generateEmail_with_null_fields(self):
+        """GenerateEmail works when many fields are NULL."""
+        person = self.factory.makePerson(name='person')
+        cake = self.factory.makeSourcePackageRecipe(
+            name=u'recipe', owner=person)
+        pantry_owner = self.factory.makePerson(name='archiveowner')
+        pantry = self.factory.makeArchive(name='ppa', owner=pantry_owner)
+        secret = self.factory.makeDistroSeries(name=u'distroseries')
+        build = self.factory.makeSourcePackageRecipeBuild(
+            recipe=cake, distroseries=secret, archive=pantry,
+            status=BuildStatus.SUPERSEDED)
+        Store.of(build).flush()
+        mailer = SourcePackageRecipeBuildMailer.forStatus(build)
+        email = build.requester.preferredemail.email
+        ctrl = mailer.generateEmail(email, build.requester)
+        self.assertEqual(
+            u'[recipe build #%d] of ~person recipe in distroseries: '
+            'Build for superseded Source' % (build.id), ctrl.subject)
+        body, footer = ctrl.body.split('\n-- \n')
+        self.assertEqual(superseded_body, body)
+        self.assertEqual(
+            'http://code.launchpad.dev/~person/+recipe/recipe/+build/1\n'
+            'You are the requester of the build.\n', footer)
+        self.assertEqual(
+            config.canonical.noreply_from_address, ctrl.from_addr)
+        self.assertEqual(
+            'Requester', ctrl.headers['X-Launchpad-Message-Rationale'])
+        self.assertEqual(
+            'recipe-build-status',
+            ctrl.headers['X-Launchpad-Notification-Type'])
+        self.assertEqual(
+            'SUPERSEDED', ctrl.headers['X-Launchpad-Build-State'])
+
 
 def test_suite():
     return TestLoader().loadTestsFromName(__name__)

=== modified file 'lib/lp/code/model/sourcepackagerecipebuild.py'
--- lib/lp/code/model/sourcepackagerecipebuild.py	2010-07-28 16:44:00 +0000
+++ lib/lp/code/model/sourcepackagerecipebuild.py	2010-08-01 04:36:51 +0000
@@ -202,7 +202,7 @@
 
     @classmethod
     def new(cls, distroseries, recipe, requester, archive, pocket=None,
-            date_created=None):
+            date_created=None, duration=None):
         """See `ISourcePackageRecipeBuildSource`."""
         store = IMasterStore(SourcePackageRecipeBuild)
         if pocket is None:
@@ -215,7 +215,8 @@
             requester,
             archive,
             pocket,
-            date_created=date_created)
+            date_created=date_created,
+            build_duration=duration)
         store.add(spbuild)
         return spbuild
 

=== modified file 'lib/lp/code/model/tests/test_sourcepackagerecipebuild.py'
--- lib/lp/code/model/tests/test_sourcepackagerecipebuild.py	2010-07-30 14:04:15 +0000
+++ lib/lp/code/model/tests/test_sourcepackagerecipebuild.py	2010-08-01 04:36:51 +0000
@@ -30,6 +30,8 @@
 from lp.code.interfaces.sourcepackagerecipebuild import (
     ISourcePackageRecipeBuildJob, ISourcePackageRecipeBuild,
     ISourcePackageRecipeBuildSource)
+from lp.code.mail.sourcepackagerecipebuild import (
+    SourcePackageRecipeBuildMailer)
 from lp.code.model.sourcepackagerecipebuild import SourcePackageRecipeBuild
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.services.mail.sendmail import format_address
@@ -320,7 +322,8 @@
         pantry = self.factory.makeArchive(name='ppa')
         secret = self.factory.makeDistroSeries(name=u'distroseries')
         build = self.factory.makeSourcePackageRecipeBuild(
-            recipe=cake, distroseries=secret, archive=pantry)
+            recipe=cake, distroseries=secret, archive=pantry,
+            duration=datetime.timedelta(minutes=5))
         removeSecurityProxy(build).buildstate = BuildStatus.FULLYBUILT
         IStore(build).flush()
         build.notify()
@@ -328,21 +331,24 @@
         requester = build.requester
         requester_address = format_address(
             requester.displayname, requester.preferredemail.email)
+        mailer = SourcePackageRecipeBuildMailer.forStatus(build)
+        expected = mailer.generateEmail(
+            requester.preferredemail.email, requester)
         self.assertEqual(
             requester_address, re.sub(r'\n\t+', ' ', message['To']))
-        self.assertEqual('Successfully built: recipe for distroseries',
-            message['Subject'])
-        body, footer = message.get_payload(decode=True).split('\n-- \n')
+        self.assertEqual(expected.subject, message['Subject'].replace(
+            '\n\t', ' '))
         self.assertEqual(
-            'Build person/recipe into ppa for distroseries: Successfully'
-            ' built.\n', body)
+            expected.body, message.get_payload(decode=True))
 
     def test_handleStatusNotifies(self):
         """"handleStatus causes notification, even if OK."""
 
         def prepare_build():
-            queue_record = self.factory.makeSourcePackageRecipeBuildJob()
-            build = queue_record.specific_job.build
+            build = self.factory.makeSourcePackageRecipeBuild(
+                duration=datetime.timedelta(minutes=5))
+            queue_record = self.factory.makeSourcePackageRecipeBuildJob(
+                recipe_build=build)
             removeSecurityProxy(build).buildstate = BuildStatus.FULLYBUILT
             queue_record.builder = self.factory.makeBuilder()
             slave = WaitingSlave('BuildStatus.OK')
@@ -373,7 +379,8 @@
         distroseries.nominatedarchindep = distroseries_i386
         build = self.factory.makeSourcePackageRecipeBuild(
             distroseries=distroseries,
-            status=BuildStatus.FULLYBUILT)
+            status=BuildStatus.FULLYBUILT,
+            duration=datetime.timedelta(minutes=5))
         build.queueBuild(build)
         return build
 

=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py	2010-07-30 20:33:47 +0000
+++ lib/lp/testing/factory.py	2010-08-01 04:36:51 +0000
@@ -167,8 +167,6 @@
     time_counter,
     )
 from lp.translations.interfaces.potemplate import IPOTemplateSet
-from lp.translations.interfaces.translationimportqueue import (
-    RosettaImportStatus)
 from lp.translations.interfaces.translationgroup import (
     ITranslationGroupSet)
 from lp.translations.interfaces.translationimportqueue import (
@@ -1871,7 +1869,8 @@
                                      requester=None, archive=None,
                                      sourcename=None, distroseries=None,
                                      pocket=None, date_created=None,
-                                     status=BuildStatus.NEEDSBUILD):
+                                     status=BuildStatus.NEEDSBUILD,
+                                     duration=None):
         """Make a new SourcePackageRecipeBuild."""
         if recipe is None:
             recipe = self.makeSourcePackageRecipe(name=sourcename)
@@ -1888,7 +1887,8 @@
             archive=archive,
             requester=requester,
             pocket=pocket,
-            date_created=date_created)
+            date_created=date_created,
+            duration=duration)
         removeSecurityProxy(spr_build).buildstate = status
         return spr_build
 
@@ -2773,8 +2773,7 @@
 
 
 def is_security_proxied_or_harmless(obj):
-    """Check that the given object is security wrapped or a harmless object.
-    """
+    """Check that the object is security wrapped or a harmless object."""
     if obj is None:
         return True
     if builtin_isinstance(obj, Proxy):