Merge lp:~abentley/launchpad/recipe-build-email into lp:launchpad

[Aaron Bentley]

Notification for sourcepackagerecipebuilds.

= Summary =
Fix bug #509893: Work out notifications for SourcePackageRecipeBuild

== Proposed fix ==
Provide notifications to the person who requested the build in all cases.

== Pre-implementation notes ==
Preimplementation was with thumper.

== Implementation details ==
Moved RecipientReason into basemailer because it's a core part of how
BaseMailer functions. Added BaseMailer_getFooter so we can provide footers
more conveniently.

Tweaked handle_status_for_build to accept a BuildBase subclass, to allow
polymorphism to work.

== Tests ==
bin/test -t test_generateEmail -t test_notify -t test_handleStatusNotifies

== Demo and Q/A ==
Request a build. An email should be sent when the build is complete.

= Launchpad lint =

Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.

Linting changed files:
  lib/lp/code/model/tests/test_sourcepackagerecipebuild.py
  lib/lp/code/mail/sourcepackagerecipebuild.py
  lib/lp/buildmaster/model/buildbase.py
  lib/lp/code/mail/branch.py
  lib/lp/code/model/sourcepackagerecipebuild.py
  lib/canonical/launchpad/emailtemplates/build-request.txt
  lib/lp/code/mail/tests/test_sourcepackagerecipebuild.py
  lib/lp/services/mail/basemailer.py

[Paul Hummer (rockstar)]

Wow, the base mailer stuff has changed a lot since I last touched it. This looks good to land.

review: Approve (code)

[Jelmer Vernooij (jelmer)]

Nice.

Is there a particular reason that forBuildRequester is a class method and makeRational is static?

I realize they weren't documented earlier, but it would be nice to have documentation for the arguments for ``handle_status_for_build``.

review: Approve (code)

[Aaron Bentley (abentley)]

makeRationale doesn't need to refer to its class, so it can be a static method. forBuildRequester does refer to its class, so it must be a class method.

handle_status_for_build was introduced two days ago. If you think that it should not have been approved without a fuller docstring, please contact its reviewer, Graham Binns. https://code.edge.launchpad.net/~michael.nelson/launchpad/587113-buildbase-handleStatus/+merge/27022

=== added file 'lib/canonical/launchpad/emailtemplates/build-request.txt'
--- lib/canonical/launchpad/emailtemplates/build-request.txt	1970-01-01 00:00:00 +0000
+++ lib/canonical/launchpad/emailtemplates/build-request.txt	2010-06-10 21:13:30 +0000
@@ -0,0 +1,1 @@
+Build %(recipe_owner)s/%(recipe)s into %(archive)s for %(distroseries)s: %(status)s.

=== modified file 'lib/lp/buildmaster/model/buildbase.py'
--- lib/lp/buildmaster/model/buildbase.py	2010-06-07 10:43:01 +0000
+++ lib/lp/buildmaster/model/buildbase.py	2010-06-10 21:13:30 +0000
@@ -3,10 +3,12 @@
 
 # pylint: disable-msg=E0211,E0213
 
+
+"""Common build base classes."""
+
+
 from __future__ import with_statement
 
-"""Common build base classes."""
-
 __metaclass__ = type
 
 __all__ = [
@@ -40,7 +42,8 @@
 UPLOAD_LOG_FILENAME = 'uploader.log'
 
 
-def handle_status_for_build(build, status, librarian, slave_status):
+def handle_status_for_build(build, status, librarian, slave_status,
+                            build_class=None):
     """Find and call the correct method for handling the build status.
 
     This is extracted from build base so that the implementation
@@ -48,7 +51,9 @@
     """
     logger = logging.getLogger(BUILDD_MANAGER_LOG_NAME)
 
-    method = getattr(BuildBase, '_handleStatus_' + status, None)
+    if build_class is None:
+        build_class = BuildBase
+    method = getattr(build_class, '_handleStatus_' + status, None)
 
     if method is None:
         logger.critical("Unknown BuildStatus '%s' for builder '%s'"
@@ -150,7 +155,8 @@
 
     def handleStatus(self, status, librarian, slave_status):
         """See `IBuildBase`."""
-        return handle_status_for_build(self, status, librarian, slave_status)
+        return handle_status_for_build(
+            self, status, librarian, slave_status, self.__class__)
 
     @staticmethod
     def _handleStatus_OK(build, librarian, slave_status, logger):

=== modified file 'lib/lp/code/mail/branch.py'
--- lib/lp/code/mail/branch.py	2010-04-26 01:58:45 +0000
+++ lib/lp/code/mail/branch.py	2010-06-10 21:13:30 +0000
@@ -13,6 +13,7 @@
     BranchSubscriptionDiffSize, BranchSubscriptionNotificationLevel,
     CodeReviewNotificationLevel)
 from lp.registry.interfaces.person import IPerson
+from lp.services.mail import basemailer
 from lp.services.mail.basemailer import BaseMailer
 
 
@@ -27,19 +28,16 @@
     mailer.sendAll()
 
 
-class RecipientReason:
-    """Reason for sending mail to a recipient."""
+class RecipientReason(basemailer.RecipientReason):
 
     def __init__(self, subscriber, recipient, branch, mail_header,
                  reason_template, merge_proposal=None,
                  max_diff_lines=BranchSubscriptionDiffSize.WHOLEDIFF,
                  branch_identity_cache=None,
                  review_level=CodeReviewNotificationLevel.FULL):
-        self.subscriber = subscriber
-        self.recipient = recipient
+        super(RecipientReason, self).__init__(subscriber, recipient,
+              mail_header, reason_template)
         self.branch = branch
-        self.mail_header = mail_header
-        self.reason_template = reason_template
         self.merge_proposal = merge_proposal
         self.max_diff_lines = max_diff_lines
         if branch_identity_cache is None:
@@ -129,20 +127,9 @@
                      ' details.',
                      branch_identity_cache=branch_identity_cache)
 
-    @staticmethod
-    def makeRationale(rationale_base, person):
-        if person.is_team:
-            return '%s @%s' % (rationale_base, person.name)
-        else:
-            return rationale_base
-
-    def getReason(self):
-        """Return a string explaining why the recipient is a recipient."""
-        template_values = {
-            'branch_name': self._getBranchIdentity(self.branch),
-            'entity_is': 'You are',
-            'lc_entity_is': 'you are',
-            }
+    def _getTemplateValues(self):
+        template_values = super(RecipientReason, self)._getTemplateValues()
+        template_values['branch_name'] = self._getBranchIdentity(self.branch)
         if self.merge_proposal is not None:
             source = self._getBranchIdentity(
                 self.merge_proposal.source_branch)
@@ -150,16 +137,7 @@
                 self.merge_proposal.target_branch)
             template_values['merge_proposal'] = (
                 'the proposed merge of %s into %s' % (source, target))
-        if self.recipient != self.subscriber:
-            assert self.recipient.hasParticipationEntryFor(self.subscriber), (
-                '%s does not participate in team %s.' %
-                (self.recipient.displayname, self.subscriber.displayname))
-        if self.recipient != self.subscriber or self.subscriber.is_team:
-            template_values['entity_is'] = (
-                'Your team %s is' % self.subscriber.displayname)
-            template_values['lc_entity_is'] = (
-                'your team %s is' % self.subscriber.displayname)
-        return (self.reason_template % template_values)
+        return template_values
 
 
 class BranchMailer(BaseMailer):

=== added file 'lib/lp/code/mail/sourcepackagerecipebuild.py'
--- lib/lp/code/mail/sourcepackagerecipebuild.py	1970-01-01 00:00:00 +0000
+++ lib/lp/code/mail/sourcepackagerecipebuild.py	2010-06-10 21:13:30 +0000
@@ -0,0 +1,65 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+
+__metaclass__ = type
+
+__all__ = [
+    'SourcePackageRecipeBuildMailer',
+    ]
+
+
+from canonical.config import config
+from canonical.launchpad.webapp import canonical_url
+from lp.services.mail.basemailer import BaseMailer, RecipientReason
+
+
+class SourcePackageRecipeBuildMailer(BaseMailer):
+
+    @classmethod
+    def forStatus(cls, build):
+        """Create a mailer for notifying about build status.
+
+        :param build: The build to notify about the state of.
+        """
+        requester = build.requester
+        recipients = {requester: RecipientReason.forBuildRequester(requester)}
+        return cls(
+            '%(status)s: %(recipe)s for %(distroseries)s',
+            'build-request.txt', recipients,
+            config.canonical.noreply_from_address, build)
+
+    def __init__(self, subject, body_template, recipients, from_address,
+                 build):
+        BaseMailer.__init__(
+            self, subject, body_template, recipients, from_address,
+            notification_type='recipe-build-status')
+        self.build = build
+
+    def _getHeaders(self, email):
+        """See `BaseMailer`"""
+        headers = super(
+            SourcePackageRecipeBuildMailer, self)._getHeaders(email)
+        headers.update({
+            'X-Launchpad-Build-State': self.build.status.name,
+            })
+        return headers
+
+    def _getTemplateParams(self, email):
+        """See `BaseMailer`"""
+        params = super(
+            SourcePackageRecipeBuildMailer, self)._getTemplateParams(email)
+        params.update({
+            '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,
+            'build_url': canonical_url(self.build),
+        })
+        return params
+
+    def _getFooter(self, params):
+        """See `BaseMailer`"""
+        return ('%(build_url)s\n'
+                '%(reason)s\n' % params)

=== added file 'lib/lp/code/mail/tests/test_sourcepackagerecipebuild.py'
--- lib/lp/code/mail/tests/test_sourcepackagerecipebuild.py	1970-01-01 00:00:00 +0000
+++ lib/lp/code/mail/tests/test_sourcepackagerecipebuild.py	2010-06-10 21:13:30 +0000
@@ -0,0 +1,59 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+
+__metaclass__ = type
+
+
+from unittest import TestLoader
+
+from canonical.config import config
+from canonical.launchpad.interfaces.lpstorm import IStore
+from canonical.testing import DatabaseFunctionalLayer
+from lp.buildmaster.interfaces.buildbase import BuildStatus
+from lp.code.mail.sourcepackagerecipebuild import (
+    SourcePackageRecipeBuildMailer)
+from lp.testing import TestCaseWithFactory
+
+
+class TestSourcePackageRecipeBuildMailer(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    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')
+        secret = self.factory.makeDistroSeries(name=u'distroseries')
+        build = self.factory.makeSourcePackageRecipeBuild(
+            recipe=cake, distroseries=secret, archive=pantry,
+            status=BuildStatus.FULLYBUILT)
+        IStore(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)
+        body, footer = ctrl.body.split('\n-- \n')
+        self.assertEqual(
+            'Build person/recipe into ppa for distroseries: Successfully'
+            ' built.\n', 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(
+            'FULLYBUILT', 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-06-09 15:32:32 +0000
+++ lib/lp/code/model/sourcepackagerecipebuild.py	2010-06-10 21:13:30 +0000
@@ -34,6 +34,8 @@
 from lp.code.interfaces.sourcepackagerecipebuild import (
     ISourcePackageRecipeBuildJob, ISourcePackageRecipeBuildJobSource,
     ISourcePackageRecipeBuild, ISourcePackageRecipeBuildSource)
+from lp.code.mail.sourcepackagerecipebuild import (
+    SourcePackageRecipeBuildMailer)
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.services.job.model.job import Job
 from lp.soyuz.adapters.archivedependencies import (
@@ -65,7 +67,8 @@
     def binary_builds(self):
         """See `ISourcePackageRecipeBuild`."""
         return Store.of(self).find(BinaryPackageBuild,
-            BinaryPackageBuild.source_package_release==SourcePackageRelease.id,
+            BinaryPackageBuild.source_package_release==
+            SourcePackageRelease.id,
             SourcePackageRelease.source_package_recipe_build==self.id)
 
     buildduration = TimeDelta(name='build_duration', default=None)
@@ -240,8 +243,8 @@
 
     def notify(self, extra_info=None):
         """See `IBuildBase`."""
-        # XXX: wgrant 2010-01-20 bug=509893: Implement this.
-        return
+        mailer = SourcePackageRecipeBuildMailer.forStatus(self)
+        mailer.sendAll()
 
     def getFileByName(self, filename):
         """See `ISourcePackageRecipeBuild`."""
@@ -253,6 +256,13 @@
         except KeyError:
             raise NotFoundError(filename)
 
+    @staticmethod
+    def _handleStatus_OK(build, librarian, slave_status, logger):
+        """See `IBuildBase`."""
+        BuildBase._handleStatus_OK(build, librarian, slave_status, logger)
+        # base implementation doesn't notify on success.
+        if build.status == BuildStatus.FULLYBUILT:
+            build.notify()
 
 class SourcePackageRecipeBuildJob(BuildFarmJobOldDerived, Storm):
     classProvides(ISourcePackageRecipeBuildJobSource)

=== modified file 'lib/lp/code/model/tests/test_sourcepackagerecipebuild.py'
--- lib/lp/code/model/tests/test_sourcepackagerecipebuild.py	2010-06-09 15:32:32 +0000
+++ lib/lp/code/model/tests/test_sourcepackagerecipebuild.py	2010-06-10 21:13:30 +0000
@@ -8,6 +8,7 @@
 __metaclass__ = type
 
 import datetime
+import re
 import unittest
 
 from pytz import utc
@@ -16,9 +17,10 @@
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
-from canonical.testing.layers import LaunchpadFunctionalLayer
-
+from canonical.testing.layers import (
+    LaunchpadFunctionalLayer, LaunchpadZopelessLayer)
 from canonical.launchpad.interfaces.launchpad import NotFoundError
+from canonical.launchpad.interfaces.lpstorm import IStore
 from canonical.launchpad.webapp.authorization import check_permission
 from lp.buildmaster.interfaces.buildbase import BuildStatus, IBuildBase
 from lp.buildmaster.interfaces.buildqueue import IBuildQueue
@@ -28,9 +30,13 @@
     ISourcePackageRecipeBuildJob, ISourcePackageRecipeBuild,
     ISourcePackageRecipeBuildSource)
 from lp.code.model.sourcepackagerecipebuild import SourcePackageRecipeBuild
+from lp.services.mail.sendmail import format_address
 from lp.soyuz.interfaces.processor import IProcessorFamilySet
 from lp.soyuz.model.processor import ProcessorFamily
+from lp.soyuz.tests.soyuzbuilddhelpers import WaitingSlave
 from lp.testing import ANONYMOUS, login, person_logged_in, TestCaseWithFactory
+from lp.testing.fakemethod import FakeMethod
+from lp.testing.mail_helpers import pop_notifications
 
 
 class TestSourcePackageRecipeBuild(TestCaseWithFactory):
@@ -150,7 +156,6 @@
         self.assertEqual(
             datetime.timedelta(minutes=5), spb.estimateDuration())
 
-
     def test_datestarted(self):
         """Datestarted is taken from job if not specified in the build.
 
@@ -226,6 +231,57 @@
         removeSecurityProxy(recent_build).datecreated += a_second
         self.assertContentEqual([recent_build], get_recent())
 
+class TestAsBuildmaster(TestCaseWithFactory):
+
+    layer = LaunchpadZopelessLayer
+
+    def test_notify(self):
+        """Notify sends email."""
+        person = self.factory.makePerson(name='person')
+        cake = self.factory.makeSourcePackageRecipe(
+            name=u'recipe', owner=person)
+        pantry = self.factory.makeArchive(name='ppa')
+        secret = self.factory.makeDistroSeries(name=u'distroseries')
+        build = self.factory.makeSourcePackageRecipeBuild(
+            recipe=cake, distroseries=secret, archive=pantry)
+        removeSecurityProxy(build).buildstate = BuildStatus.FULLYBUILT
+        IStore(build).flush()
+        build.notify()
+        (message,) = pop_notifications()
+        requester = build.requester
+        requester_address = format_address(
+            requester.displayname, requester.preferredemail.email)
+        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(
+            'Build person/recipe into ppa for distroseries: Successfully'
+            ' built.\n', body
+            )
+
+    def test_handleStatusNotifies(self):
+        """"handleStatus causes notification, even if OK."""
+        def prepare_build():
+            queue_record = self.factory.makeSourcePackageRecipeBuildJob()
+            build = queue_record.specific_job.build
+            removeSecurityProxy(build).buildstate = BuildStatus.FULLYBUILT
+            queue_record.builder = self.factory.makeBuilder()
+            slave = WaitingSlave('BuildStatus.OK')
+            queue_record.builder.setSlaveForTesting(slave)
+            return build
+        def assertNotifyOnce(status, build):
+            build.handleStatus(status, None, {'filemap': {}})
+            self.assertEqual(1, len(pop_notifications()))
+        for status in ['PACKAGEFAIL', 'OK']:
+            assertNotifyOnce(status, prepare_build())
+        build = prepare_build()
+        removeSecurityProxy(build).verifySuccessfulUpload = FakeMethod(
+        result=True)
+        assertNotifyOnce('OK', prepare_build())
+
+
 class MakeSPRecipeBuildMixin:
     """Provide the common makeBuild method returning a queued build."""
 

=== modified file 'lib/lp/services/mail/basemailer.py'
--- lib/lp/services/mail/basemailer.py	2010-03-18 22:41:03 +0000
+++ lib/lp/services/mail/basemailer.py	2010-06-10 21:13:30 +0000
@@ -5,7 +5,7 @@
 
 __metaclass__ = type
 
-__all__ = ['BaseMailer']
+__all__ = ['BaseMailer', 'RecipientReason']
 
 import logging
 from smtplib import SMTPException
@@ -14,7 +14,9 @@
 
 from lp.services.mail.notificationrecipientset import (
     NotificationRecipientSet)
-from lp.services.mail.sendmail import format_address, MailController
+from lp.services.mail.sendmail import (
+    append_footer, format_address, MailController
+)
 from lp.services.utils import text_delta
 
 
@@ -130,7 +132,16 @@
     def _getBody(self, email):
         """Return the complete body to use for this email."""
         template = get_email_template(self._template_name)
-        return template % self._getTemplateParams(email)
+        params = self._getTemplateParams(email)
+        body = template % params
+        footer = self._getFooter(params)
+        if footer is not None:
+            body = append_footer(body, footer)
+        return body
+
+    def _getFooter(self, params):
+        """Provide a footer to attach to the body, or None."""
+        return None
 
     def sendAll(self):
         """Send notifications to all recipients."""
@@ -150,3 +161,46 @@
                     # Don't want an entire stack trace, just some details.
                     self.logger.warning(
                         'send failed for %s, %s' % (email, e))
+
+
+class RecipientReason:
+    """Reason for sending mail to a recipient."""
+
+    def __init__(self, subscriber, recipient, mail_header, reason_template):
+        self.subscriber = subscriber
+        self.recipient = recipient
+        self.mail_header = mail_header
+        self.reason_template = reason_template
+
+    @staticmethod
+    def makeRationale(rationale_base, person):
+        if person.is_team:
+            return '%s @%s' % (rationale_base, person.name)
+        else:
+            return rationale_base
+
+    def _getTemplateValues(self):
+        template_values = {
+            'entity_is': 'You are',
+            'lc_entity_is': 'you are',
+            }
+        if self.recipient != self.subscriber:
+            assert self.recipient.hasParticipationEntryFor(self.subscriber), (
+                '%s does not participate in team %s.' %
+                (self.recipient.displayname, self.subscriber.displayname))
+        if self.recipient != self.subscriber or self.subscriber.is_team:
+            template_values['entity_is'] = (
+                'Your team %s is' % self.subscriber.displayname)
+            template_values['lc_entity_is'] = (
+                'your team %s is' % self.subscriber.displayname)
+        return template_values
+
+    def getReason(self):
+        """Return a string explaining why the recipient is a recipient."""
+        return (self.reason_template % self._getTemplateValues())
+
+    @classmethod
+    def forBuildRequester(cls, requester):
+        header = cls.makeRationale('Requester', requester)
+        reason = '%(entity_is)s the requester of the build.'
+        return cls(requester, requester, header, reason)